Issue #144: Add ability to review a full file and not only a changeset
|Reported by:||Mathieu Clabaut|
|Created on:||2015-06-30 14:29|
|Updated on:||2015-08-06 19:40|
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.
Comment by Mathieu Clabaut, on 2015-06-30 14:30
Comment by Thomas De Schampheleire, on 2015-07-27 20:17
At this moment, the database model attaches comments to changesets or pullrequests (which are groups of changesets).
@kiilerix thoughts on this one?
Comment by Mads Kiilerich, on 2015-07-27 20:29
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.
Comment by Mathieu Clabaut, on 2015-07-28 09:11
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.
Comment by Mathieu Clabaut, on 2015-07-28 09:47
I tried the following :
- adding a tag nullrev to rev 0 :
hg tag -r 0 nullrev
- creating a pullrequest with origin
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 ?
Comment by Andrej Shadura, on 2015-07-28 09:53
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."
Comment by Mathieu Clabaut, on 2015-07-28 10:25
Trying this leads to
I will investigate as soon as I get some time.
Comment by Mads Kiilerich, on 2015-07-28 13:45
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 ...
Comment by Mads Kiilerich, on 2015-07-28 13:54
I am using https://bitbucket.org/Unity-Technologies/kallithea/commits/78edbf29d5824d09c733f4b6a9fda70c9fea0a58#Lkallithea/controllers/compare.pyT244 which prevents this situation ... but I'm not entirely happy with it yet and haven't upstreamed it ...
Comment by Mads Kiilerich, on 2015-07-28 14:02
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! ;-)
Comment by Mathieu Clabaut, on 2015-08-01 09:32
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.
Comment by Thomas De Schampheleire, on 2015-08-01 10:03
Can you clarify what exactly you did? Which command did you execute? Could you also send the output of 'pip freeze' ?
Comment by Mathieu Clabaut, on 2015-08-01 10:15
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.
Comment by Mathieu Clabaut, on 2015-08-01 10:17
@patrickdepinguin I should have been more precise.
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==126.96.36.199 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
Comment by Thomas De Schampheleire, on 2015-08-01 10:36
@matclab This looks like: https://bitbucket.org/pytest-dev/pytest/issues/539/pytest-doctest-modules-fails-if-python Is it possible you ran 'python setup.py build' at some point? I suggest removing that directory and running 'python setup.py develop' again, or (if that doesn't work) try again from a fresh setup.
Comment by Mathieu Clabaut, on 2015-08-01 11:23
Thanks @patrickdepinguin, it works now.
Comment by Mads Kiilerich, on 2015-08-01 14:00
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".
Comment by Thomas De Schampheleire, on 2015-08-06 19:40
@Matclab What is the status of this issue? Were you able to add a test as suggested by @kiilerix in 144.html#comment-20523688 ?
Comment by Mathieu Clabaut, on 2015-08-07 20:26
I didn't have time to look at it for the last days… I hope to find some time in the two comming weeks…