[Python-Dev] We should be using a tool for code reviews (original) (raw)

Barry Warsaw barry at python.org
Wed Sep 29 23:52:25 CEST 2010

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>

More information about the Python-Dev mailing list