On 27/03/12 19:49, Roan Kattouw wrote: > On Mon, Mar 26, 2012 at 9:50 PM, Tim Starling <[email protected]> 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 _______________________________________________ Wikitech-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikitech-l
