Tim Starling
<tstarling(a)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