Add ability to review a full file and not only a changeset

Issue #144 new
Mathieu Clabaut created an issue

Doing review of changesets works well when the review process was set up at the beginning of the project, but for projects where the reviewing is set up upon existing history, it is cumbersome to review all changesets.

It would be nice to be able to review a full file at a given revision, at which point the team will be able to go with a changeset review scheme.

Comments (19)

  1. Thomas De Schampheleire

    At this moment, the database model attaches comments to changesets or pullrequests (which are groups of changesets).

    @kiilerix thoughts on this one?

  2. Mads Kiilerich

    Reviews of existing full files can pretty much be done with a PR to the null revision. But keeping track of the status per file is more tricky. The right datamodel for this would probably also imply partial reviews of files and keeping track of what has or hasn't been reviewed. It is also something that only will be needed once and probably have special needs.

    I think I would suggest just start doing reviews of code changes and do cleanups as they are noticed. Once the review process for new changesets is running, start reviewing the existing files the manual and cumbersome way. Perhaps use something as lo-tech and flexible as a list of files in a spreadsheet.

  3. Mathieu Clabaut reporter

    Thank you for your help. Using a PR to the null revision may be sufficient to initiate a "clean review state" and begin use the review of changesets workflow. I will investigate this and report here.

  4. Mathieu Clabaut reporter

    I tried the following :

    • adding a tag nullrev to rev 0 : hg tag -r 0 nullrev
    • creating a pullrequest with origin nullrev and destination default

    But the resulting pullrequest contains only the commit adding the tag nullrev. I didn't see a way of creating a pullrequest from a given revision.

    @kiilerix could you precise what you add in mind with the PR to null revision ? Shall kalithea be modified to allow such a PR ?

  5. Andrej Shadura

    I think @kiilerix meant creating a ‘detached’ revision:

    hg up -r null
    # … create files …
    hg add .
    hg ci -m "Added a bunch of meaningless files to review."
    
  6. Mads Kiilerich

    Mathieu: the PR should be the other way: from where the changesets are to where they are not yet. (It is inherently confusing that a "PR" conceptually goes in the opposite direction of a diff ...) (That method will however ignore revision 0 ... and it is not possible to put a tag on the null revision. Perhaps a bookmark or a localtag?)

    I didn't mean to create a 'deteched' revision. Without a common ancestor it is really not possible to merge so i don't know what should happen ... but it shouldn't fail ...

  7. Mads Kiilerich

    A smaller fix that would work for this use case could be the following.

    --- a/kallithea/controllers/pullrequests.py +++ b/kallithea/controllers/pullrequests.py @@ -337,8 +337,10 @@ class PullrequestsController(BaseRepoCon CompareController._get_changesets(org_repo.scm_instance.alias, other_repo.scm_instance, other_rev, # org and other "swapped" org_repo.scm_instance, org_rev, ) + if ancestor_rev is None: + ancestor_rev = org_repo.scm_instance.EMPTY_CHANGESET revisions = [cs.raw_id for cs in cs_ranges]

    I will upstream it if you add a test case for it! ;-)

  8. Mathieu Clabaut reporter

    I tried, but was unable to run the testsuite.

    I get the folowing error :

    ConftestImportFailure: (local('/home/clabaut/contrib/kallithea/kallithea/tests/conftest.py'), (<class 'py._path.local.ImportMismatchError'>, ImportMismatchError('kallithea.tests.conftest', '/home/clabaut/contrib/kallithea/build/lib/kallithea/tests/conftest.py', local('/home/clabaut/contrib/kallithea/kallithea/tests/conftest.py')), <traceback object at 0x7fd0faa51200>))
    

    I'll try to investigate, but in the meanwhile if you have already encountered and solved such an error, do not hesitate to tell me.

  9. Thomas De Schampheleire

    Can you clarify what exactly you did? Which command did you execute? Could you also send the output of 'pip freeze' ?

  10. Mathieu Clabaut reporter

    I also tried the following patch to allow creating a pull request with the null revision:

    diff --git a/kallithea/controllers/pullrequests.py b/kallithea/controllers/pullrequests.py
    --- a/kallithea/controllers/pullrequests.py
    +++ b/kallithea/controllers/pullrequests.py
    @@ -153,10 +153,10 @@
             # prio 1: rev was selected as existing entry above
    
             # prio 2: create special entry for rev; rev _must_ be used
    -        specials = []
    +        specials = [('rev:0000000000:000000000000', '%s: %s' % (_("Changeset"), _("Null revision"))) ]
             if rev and selected is None:
                 selected = 'rev:%s:%s' % (rev, rev)
    -            specials = [(selected, '%s: %s' % (_("Changeset"), rev[:12]))]
    +            specials.append((selected, '%s: %s' % (_("Changeset"), rev[:12])))
    
             # prio 3: most recent peer branch
             if peers and not selected:
    

    But the PR shows 0 files changed with 0 insertions and 0 deletions even if it spans the whole repository history.

  11. Mathieu Clabaut reporter

    @patrickdepinguin I should have been more precise.

    I ran py.test on the kallithea root directory.

    Pip freeze gives:

    Babel==1.3
    Beaker==1.6.4
    FormEncode==1.2.6
    -e hg+*** failed to import extension hggit: No module named hggit
    https://kallithea-scm.org/repos/kallithea@*** failed to import extension hggit: No module named hggit
    1cdfd54a9c8ffbaccb4082677e3c494c638ce9b7#egg=Kallithea-dev
    Mako==1.0.0
    Markdown==2.2.1
    MarkupSafe==0.23
    Paste==1.7.5.1
    PasteDeploy==1.5.2
    PasteScript==1.7.5
    Pygments==1.6
    Pylons==1.0
    Routes==1.13
    SQLAlchemy==0.7.10
    Tempita==0.5.3dev
    UNKNOWN==0.0.0
    URLObject==2.3.4
    WebError==0.10.3
    WebHelpers==1.3
    WebOb==1.0.8
    WebTest==1.4.3
    Whoosh==2.5.7
    amqplib==1.0.2
    anyjson==0.3.3
    celery==2.2.10
    decorator==3.4.0
    docutils==0.11
    dulwich==0.9.9
    kombu==1.5.1
    mercurial==3.3.3
    mock==1.0.1
    nose==1.3.4
    py==1.4.30
    py-bcrypt==0.4
    pycrypto==2.6.1
    pyparsing==1.5.7
    pytest==2.7.2
    python-dateutil==1.5
    pytz==2014.7
    repoze.lru==0.6
    simplejson==2.5.2
    waitress==0.8.8
    wsgiref==0.1.2
    
  12. Mads Kiilerich

    Please use the mailing list or IRC for discussions and keep each issue in the issue tracker "on topic" for discussion of one specific "flaw".

  13. Mathieu Clabaut reporter

    I didn't have time to look at it for the last days… I hope to find some time in the two comming weeks…

  14. Log in to comment