[Python-Dev] We should be using a tool for code reviews

[Barry Warsaw]

On Sep 29, 2010, at 05:22 PM, Alexander Belopolsky wrote:

>> Many times bigger than what? If you mean svn that's not true (the
>> eval of the DVCS pegged Hg at only 50% larger than svn).
>
>My experience was different.  I may misremember because I did not try
>to use Hg since about a year ago, but the Hg checkout was 3-4x of that
>of an SVN branch.   However, my comment was simply a reaction to the
>argument that Hg checkout would be *less* of a burden than SVN.

With Bazaar, if you're a frequent contributor to a project, you can amortize
the expensive initial checkout across all the branches you'll ever make.  The
first branch may take a while, but subsequent ones are very fast.  I'd be
surprised if Mercurial didn't have at least something similar.

It's true though that for drive-by contributors who rarely develop a patch,
generating a branch may be too high a barrier.  In that case, I think it's
fine to have diffs (which a more experienced developer can turn into a
branch).

>> But honestly, the line has to be drawn somewhere. Right now we won't
>> take a patch submitted to the mailing list, but accepting patches not
>> against a checkout wastes time for everyone involved as soon as it
>> doesn't apply cleanly. At that point either a core developer has to
>> fix it or (what typically happens) the person has to get a checkout,
>> update their patch, and then re-submit. We might as well minimize
>> that when possible by really pushing for checkout patches.
>>
>
>Well, many patches do not apply cleanly by the time they are reviewed
>even when they are originally produced from a clean checkout.   I
>actually wonder if it would be appropriate to encourage *reviewers* to
>upload the patches to a review tool.   A reviewer is much more likely
>to have a clean checkout already than a casual contributor and
>oftentimes applying the patch is the first thing reviewers do anyways.
> If a review tool is one command away, it may be even easier to run it
>rather than to figure out how to reference the code in the patch in
>the roundup comment.

While branches are far better than patches here, you can still have conflicts
when merging the branch to the trunk (which I recommend doing when reviewing a
branch).  I have no problem halting a review right there and asking the
developer to ensure a conflict-free merge.

It's of course at the reviewer's discretion to assist them with this (e.g. by
branching their branch, resolving the conflicts, committing and publishing
this clean branch, for the original developer to merge into *their* branch
before requesting a re-review -- see why branches are so much better?! :).

-Barry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://mail.python.org/pipermail/python-dev/attachments/20100929/789fc2db/attachment.pgp>