On 27/03/12 19:49, Roan Kattouw wrote:
On Mon, Mar 26, 2012 at 9:50 PM, Tim Starling
<tstarling(a)wikimedia.org> wrote:
For commits with lots of files, Gerrit's diff
interface is too broken
to be useful. It does not provide a compact overview of the change
which is essential for effective review.
Luckily, there are alternatives, specifically local git clients and
gitweb. However, these don't work when git's change model is broken by
the use of git commit --amend.
They do; it just wasn't obvious to you how to do it, but that doesn't
mean it can't be done.
$ git fetch
https://gerrit.wikimedia.org/r/p/analytics/udp-filters
refs/changes/22/3222/3 && git branch foo FETCH_HEAD
$ git fetch
https://gerrit.wikimedia.org/r/p/analytics/udp-filters
refs/changes/22/3222/4 && git branch bar FETCH_HEAD
$ git diff foo..bar
The two 'git fetch' commands (or at least the part before the &&) can
be taken from the change page in Gerrit.
It doesn't work, I'm afraid. Because of the implicit rebase on push,
usually subsequent changesets have a different parent. So when you
diff between the two branches, you get all of the intervening commits
which were merged to the master.
Examples from today:
https://gerrit.wikimedia.org/r/#change,3367
Patchsets 1 and 2 have different parents.
https://gerrit.wikimedia.org/r/#change,3363
Patchsets 1, 2 and 3 have different parents.
It's possible to get a diff between them, and I did, but it's tedious.
I figure we should pick a workflow that doesn't waste the reviewer's
time quite so much.
I have mixed feelings towards this. One time at the
office, over
lunch, I was bitching about how Gerrit used amends instead of real
branches, and after discussing this for 15 minutes we felt like we'd
just reverse-engineered the Gerrit developers' decision process
(RobLa: "I think we just figured out why the Gerrit people did it this
way.")
I could not find a recommendation to use --amend in the Gerrit manual.
The manual says that it is possible to submit subsequent commits to
the same change by just adding a Change-Id footer to the commit
message. That seems to be the reason for the copy button next to the
Change-Id on the change page.
When I tried to do a test push containing several commits which
depended on each other, but with the same Change-Id, Gerrit rejected
it. But I'm not sure if that's a configuration issue or if it just
expected them to be done in separate pushes.
If it was possible to follow the Gerrit manual in this way, and submit
non-amending followup commits with the same Change-Id, then we'd have
the comment grouping advantages without the code review disadvantages.
* Every commit that ends up being merged into master
is "clean", in
that it's been approved and passes the tests. This is the entire point
of continuous integration / pre-commit review / gated trunk / etc.,
and it's a major advantage because:
** you can rewind master to any point in history and it'll be in a sane state
** you can start a branch (including a deployment branch) off any
point in master and be confident that that's a reasonably sane branch
point
** if you find subtle breakage later, you can use git bisect on master
to find out where it broke, and there will not be any botched
intermediate states confusing bisect (e.g. if there's a commit
somewhere that breaks half the code and you merge it in together with
a followup commit that fixes it, bisect might find that commit and
wrongly blame it for the breakage you're looking for; this probability
increases as merged-in feature branches get longer)
I think significantly increasing review time and breaking authorship
and history information would be a high price to pay for these advantages.
Sure, you can bisect, but when you find the commit and discover that
there were 5 people amending it, how do you work out who was responsible?
I don't think bisect is such a useful feature that it's worth throwing
away every other feature in git just to get it.
you will not be able to
approve and merge it without manually overriding Jenkins's V:-1
review.
It seems like that will be a lot easier and a lot more rare than
finding a diff between amended commits with different parents.
An unrelated disadvantage of "stacked
changes" in Gerrit (i.e. changes
that depend on other unmerged changes) is that if B.1 (change B
patchset 1) depends on A.1, and someone amends A.1 later to produce
A.2, B.1 will still depend on A.1 and will need to be rebased to
depend on A.2 instead. I've done this before with a stack of four
commits (amended B and had to rebase C and D), and I can tell you it's
not fun. I think I've figured out a trick for it now (use an
interactive rebase from the top of the stack), but it's not intuitive
and I've never tried it.
Tell people to not amend their commits then.
-- Tim Starling