Tim Starling tstarling@wikimedia.org wrote:
I wrote:
Also, I'm concluding based on Roan's objections that I'm going to have a hard time convincing people to stop amending their commits. So I wrote this script that provides changeset diffs for reviewers:
http://tstarling.com/gerrit-differ.php
It fetches both commits from a local mirror into a temporary branch, rebases the branches to a common ancestor, and then diffs them.
There is an interesting failure mode of the whole plan :) As you noticed on IRC comparing patchset 4 and 5 makes no sense. Patchset 5 actually does not change anything except the commit message relative to the patchset 4, but that's not the issue.
Side note: I have a small tool (https://github.com/saper/gerrit-fetch-all) to fetch all changesets from gerrit and branch the as "change/3841/4". Then you can diff your changesets locally, in the git repository.
Here's why: patchsets 1 till 4 are based off revision
$ git log --pretty=oneline change/3841/4 ^change/3841/4^^ fcc05dee9b93e080b13fc4b0d5b83a1c75d34362 (bug 5445) remove autoblocks when user is unblocked 8824515e571eadd4a63b09e1331f35309315603f Fix Bug 30681 - Wrong escaping for inexistent messages.
While patchset 5 has been rebased to a newer changeset from master:
$ git log --pretty=oneline change/3841/5 ^change/3841/5^^ 4f2ff743ff1bc93d922ab9b5b3135786df5c7b69 (bug 5445) remove autoblocks when user is unblocked 571e63cd2c2bac9a033e1816f5ad8b6a14b4f42b Merge "Use local context to get messages" 95c35e52113b9a98accc1e9b0e9fffc15b1661a8 Use local context to get messages
$ git branch -vv |grep change/3841/ change/3841/1 89daac5 Remove autoblocks when original block goes (Bug #5445) change/3841/2 b9090b3 (bug 5445) remove autoblocks when user is unblocked change/3841/3 96692fb (bug 5445) remove autoblocks when user is unblocked change/3841/4 fcc05de (bug 5445) remove autoblocks when user is unblocked * change/3841/5 4f2ff74 (bug 5445) remove autoblocks when user is unblocked
So here's how patchsets 4 and 5 differ according to git:
$ git log --pretty=oneline change/3841/4...change/3841/5 * 4f2ff743ff1bc93d922ab9b5b3135786df5c7b69 (bug 5445) remove autoblocks when user is unblocked * 571e63cd2c2bac9a033e1816f5ad8b6a14b4f42b Merge "Use local context to get messages" |\ | * 95c35e52113b9a98accc1e9b0e9fffc15b1661a8 Use local context to get messages * | 681a170f290ca0a7b0d771155ddc59f091a5576d Merge "Add phpunit testcases for Bug 30681" |\ \ | * | b91ffd7b09b445224cdef27a3a40bc9ded1fb8c7 Add phpunit testcases for Bug 30681 | / * | dde3821ac130486a24a7f7a97eaf0eb6d67e55d2 (bug 35541) ns gender aliases for Croatian (hr) |/ * cc2f70df0d106f84877591113d3973214bcfd36a gitignore mwsql script history file * fcc05dee9b93e080b13fc4b0d5b83a1c75d34362 (bug 5445) remove autoblocks when user is unblocked
The common revision for both changesets is the next one:
$ git merge-base change/3841/4 change/3841/5 8824515e571eadd4a63b09e1331f35309315603f
(this is the parent of change/3841/1..4)
So it is clear that diff between them will include all the changes merged to master in between.
My git-fu is limited, so I don't know how to compare such revisions.
I think we generally run into git architectural assumption - that git is meant to store trees of files and "diffs" or "changes" are not an object in the git world at all.
So I think that rebasing the patchset to the current master is a bad idea for review. However, this increases likelihood of merge conflict once the change is approved. Maybe we should have a workflow like this:
patchset 1 - proposed change patchset 1 - review, negative patchset 2 - updated change patchset 2 - review, negative patchset 3 - updated change patchset 3 - review, possitve - change approved patchset 4 - patchset 3 rebased to the master branch patchset 4 - merged, closed (if ok)
Would that work in practice?
//Saper