On 27/03/12 19:49, Roan Kattouw wrote:
On Mon, Mar 26, 2012 at 9:50 PM, Tim Starling tstarling@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