Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Wikipedia: Wikitech

Committing followups: please no --amend

 

 

Wikipedia wikitech RSS feed   Index | Next | Previous | View Threaded


tstarling at wikimedia

Mar 26, 2012, 9:50 PM

Post #1 of 11 (1027 views)
Permalink
Committing followups: please no --amend

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.

For commits with a small number of files, such changes are reviewable
by the use of the "patch history" table in the diff views. But when
there are a large number of files, it becomes difficult to find the
files which have changed, and if there are a lot of changed files, to
produce a compact combined diff.

So if there are no objections, I'm going to change [[Git/Workflow]] to
restrict the recommended applications of "git commit --amend", and to
recommend plain "git commit" as an alternative. A plain commit seems
to work just fine. It gives you a separate commit to analyse with
Gerrit, gitweb and client-side tools, and it provides a link to the
original change in the "dependencies" section of the change page.

-- Tim Starling


_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l


amir.aharoni at mail

Mar 26, 2012, 11:24 PM

Post #2 of 11 (988 views)
Permalink
Re: Committing followups: please no --amend [In reply to]

2012/3/27 Tim Starling <tstarling [at] wikimedia>:
> 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.
>
> For commits with a small number of files, such changes are reviewable
> by the use of the "patch history" table in the diff views. But when
> there are a large number of files, it becomes difficult to find the
> files which have changed, and if there are a lot of changed files, to
> produce a compact combined diff.
>
> So if there are no objections, I'm going to change [[Git/Workflow]] to
> restrict the recommended applications of "git commit --amend", and to
> recommend plain "git commit" as an alternative. A plain commit seems
> to work just fine. It gives you a separate commit to analyse with
> Gerrit, gitweb and client-side tools, and it provides a link to the
> original change in the "dependencies" section of the change page.

It sounds similar to what i said in the thread "consecutive commits in
Gerrit‏", so i probably support it, but i don't completely understand
how will it work with the `git review' command, which doesn't like
multiple commits. If the documentation will explain how to use `git
review' with follow up commits, it will be fine.

--
Amir Elisha Aharoni · אָמִיר אֱלִישָׁע אַהֲרוֹנִי
http://aharoni.wordpress.com
‪“We're living in pieces,
I want to live in peace.” – T. Moore‬

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l


tparscal at wikimedia

Mar 27, 2012, 1:12 AM

Post #3 of 11 (970 views)
Permalink
Re: Committing followups: please no --amend [In reply to]

Good advice here, but I would just say we should mention that git --amend
is still recommended if you committed something and then realized there was
a mistake.

- Using it to fix a typo or minor error in a commit = awesome.
- Using it to pile up tons of changes across tons of files = not awesome.

The former makes review EASIER, the latter makes review HARDER.

- Trevor

On Mon, Mar 26, 2012 at 11:24 PM, Amir E. Aharoni <
amir.aharoni [at] mail> wrote:

> 2012/3/27 Tim Starling <tstarling [at] wikimedia>:
> > 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.
> >
> > For commits with a small number of files, such changes are reviewable
> > by the use of the "patch history" table in the diff views. But when
> > there are a large number of files, it becomes difficult to find the
> > files which have changed, and if there are a lot of changed files, to
> > produce a compact combined diff.
> >
> > So if there are no objections, I'm going to change [[Git/Workflow]] to
> > restrict the recommended applications of "git commit --amend", and to
> > recommend plain "git commit" as an alternative. A plain commit seems
> > to work just fine. It gives you a separate commit to analyse with
> > Gerrit, gitweb and client-side tools, and it provides a link to the
> > original change in the "dependencies" section of the change page.
>
> It sounds similar to what i said in the thread "consecutive commits in
> Gerrit‏", so i probably support it, but i don't completely understand
> how will it work with the `git review' command, which doesn't like
> multiple commits. If the documentation will explain how to use `git
> review' with follow up commits, it will be fine.
>
> --
> Amir Elisha Aharoni · אָמִיר אֱלִישָׁע אַהֲרוֹנִי
> http://aharoni.wordpress.com
> ‪“We're living in pieces,
> I want to live in peace.” – T. Moore‬
>
> _______________________________________________
> Wikitech-l mailing list
> Wikitech-l [at] lists
> https://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l


roan.kattouw at gmail

Mar 27, 2012, 1:49 AM

Post #4 of 11 (984 views)
Permalink
Re: Committing followups: please no --amend [In reply to]

On Mon, Mar 26, 2012 at 9:50 PM, Tim Starling <tstarling [at] wikimedia> 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.

> For commits with a small number of files, such changes are reviewable
> by the use of the "patch history" table in the diff views. But when
> there are a large number of files, it becomes difficult to find the
> files which have changed, and if there are a lot of changed files, to
> produce a compact combined diff.
>
> So if there are no objections, I'm going to change [[Git/Workflow]] to
> restrict the recommended applications of "git commit --amend", and to
> recommend plain "git commit" as an alternative. A plain commit seems
> to work just fine. It gives you a separate commit to analyse with
> Gerrit, gitweb and client-side tools, and it provides a link to the
> original change in the "dependencies" section of the change page.
>
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.")

There are several advantages to using Gerrit the way it's meant to be
used (with amends rather than followup commits):
* Review comments of one logical change are centralized, rather than
being scattered across multiple changes (this is what I said I'd do
differently if I was writing a code review system now)
* Conflicts must be resolved by rebasing the conflicted topic branch
onto the HEAD of master, so there aren't any merge commits containing
conflict resolutions unless you deliberately create them (and someone
else approves them). I imagine I'd be quite annoyed/confused if I was
using git bisect to track down a bug and it blamed a complex merge
commit that had conflicts
* 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)

Of course you can't blindly place absolute trust in any random commit
on master, but at least you know that it 1) has been reviewed as a
unit and approved, and once we have better Jenkins integration you'll
know that 2) it passed the lint checks and 3) it passed the tests as
they existed at the time. Approving commits that are broken because
you know they were fixed later in some follow-up commit somewhere
violates this entire model, and it exposes you to the danger of
merging the broken commit but not the follow-up, if the follow-up is
held up for some reason. Fortunately, once we have proper Jenkins
integration, this will not be possible because Jenkins will mark the
broken commit as having failed the tests, and you will not be able to
approve and merge it without manually overriding Jenkins's V:-1
review.

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.

That said, if you have multiple commits that are related/dependent but
both represent valid, sane, non-broken states (e.g. you introduce a
function, then use it somewhere), then by all means chain them
together. I'll +1 Trevor there, amending is probably not a good idea
if you're adding lots of changes. And if all else fails, you can just
abandon the intermediate changes, squash them together into one
omnibus change, and submit that for review instead.

As for Amir's question about git-review: git-review warns you against
stacked changes, because it's considered an anti-pattern in Gerrit.
However, the warning ends with a question like "Are you sure this is
what you meant to do?", and if you type "yes" it'll continue. You can
also suppress this warning with the -y flag.

Roan

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l


tstarling at wikimedia

Mar 27, 2012, 2:22 AM

Post #5 of 11 (979 views)
Permalink
Re: Committing followups: please no --amend [In reply to]

On 27/03/12 17:24, Amir E. Aharoni wrote:
> It sounds similar to what i said in the thread "consecutive commits in
> Gerrit‏", so i probably support it, but i don't completely understand
> how will it work with the `git review' command, which doesn't like
> multiple commits. If the documentation will explain how to use `git
> review' with follow up commits, it will be fine.

I've done a few test commits, it will work. The procedure is to copy
the download command out of the Gerrit change page and paste it into a
shell, same as amending. Git gives you some output which includes:

: If you want to create a new branch to retain commits you
: create, you may do so (now or later) by using -b with the
: checkout command again. Example:
: git checkout -b new_branch_name

You follow its instructions, creating a branch:

: git checkout -b my_test_commit

Then edit the files, then instead of amend, just a regular

: git commit -a
: git review

It complains that you're committing two things:

: You have more than one commit that you are about to submit.
: The outstanding commits are:
:
: 6e4f490 (HEAD, test2) test 2
: 634b5d7 (junk-2) Test commit 1
:
: Is this really what you meant to do?
: Type 'yes' to confirm:

You answer "yes", then it's done. Here's what the result looks like:

https://gerrit.wikimedia.org/r/#change,3794

git review --no-rebase may be necessary to reduce noise on the parent
change pages, I haven't tested that yet.

It can be done with slightly fewer steps if you make the steps more
complicated.

-- Tim Starling


_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l


hashar+wmf at free

Mar 27, 2012, 2:54 AM

Post #6 of 11 (973 views)
Permalink
Re: Committing followups: please no --amend [In reply to]

Roan Kattouw wrote:
> * 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 <snip>

That is exactly why I think, one day, we will be able to get ride of the
wmf branch and deploy several time per day straight from master.

--
Antoine "hashar" Musso


_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l


tstarling at wikimedia

Mar 27, 2012, 3:06 AM

Post #7 of 11 (972 views)
Permalink
Re: Committing followups: please no --amend [In reply to]

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


innocentkiller at gmail

Mar 27, 2012, 3:39 AM

Post #8 of 11 (969 views)
Permalink
Re: Committing followups: please no --amend [In reply to]

On Tue, Mar 27, 2012 at 6:06 AM, Tim Starling <tstarling [at] wikimedia> wrote:
> 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.
>

Yes, that works. And actually, I think it's a little bit easier. It's a
little more "manual" that trying git one-liners, but it seems to at
least *always* work (and I've used it a couple of times to fix
changes with totally botched history).

>> 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.
>

Also making sure people don't have unrelated commits showing up as
dependencies makes the process much easier. If two things aren't
related--don't relate them!

-Chad

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l


saper at saper

Mar 27, 2012, 11:33 AM

Post #9 of 11 (966 views)
Permalink
Re: Committing followups: please no --amend [In reply to]

>> Tim Starling <tstarling [at] wikimedia> wrote:
>
> It doesn't work, I'm afraid. Because of the implicit rebase on push,
> usually subsequent changesets have a different parent.

How does the "implicit rebase on push" work? Do you mean git-review?

I don't know whether "git push <remote> HEAD:for/master/<topic>"
rebases anything. I still prefer plain "git push" (where I can also
ay "HEAD:refs/changes/XXX") to git-review magic.

//Saper


_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l


tstarling at wikimedia

Mar 27, 2012, 7:56 PM

Post #10 of 11 (974 views)
Permalink
Re: Committing followups: please no --amend [In reply to]

On 28/03/12 05:33, Marcin Cieslak wrote:
>>> Tim Starling <tstarling [at] wikimedia> wrote:
>>
>> It doesn't work, I'm afraid. Because of the implicit rebase on push,
>> usually subsequent changesets have a different parent.
>
> How does the "implicit rebase on push" work? Do you mean git-review?

Yes, git-review.

> I don't know whether "git push <remote> HEAD:for/master/<topic>"
> rebases anything.

It doesn't. But you often have to do a rebase before Gerrit can merge
the change into master.

-- Tim Starling


_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l


innocentkiller at gmail

Apr 2, 2012, 2:05 PM

Post #11 of 11 (947 views)
Permalink
Re: Committing followups: please no --amend [In reply to]

On Tue, Mar 27, 2012 at 6:06 AM, Tim Starling <tstarling [at] wikimedia> wrote:
> On 27/03/12 19:49, Roan Kattouw wrote:
>> On Mon, Mar 26, 2012 at 9:50 PM, Tim Starling <tstarling [at] wikimedia> 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.
>

The problem here is the implicit rebase. As long as the review
backlog isn't long and/or people aren't submitting conflicting
changes, rebasing amended changes against master creates
more harm than good.

For amending commits, you should use `git review -R` so you
don't rebase the change (again) against master (see for example
[0], difference between patch 2 and 3). I've updated the docs[1],
but they are, briefly:

git review -d 123
(make changes)
git commit -a --amend
git review -R

If you're not using git-review and have been using the alias, your
amended patchsets have not been creating this problem.

-Chad

[0] https://gerrit.wikimedia.org/r/#change,4020
[1] https://www.mediawiki.org/wiki/Git/Workflow#Amend_your_change

_______________________________________________
Wikitech-l mailing list
Wikitech-l [at] lists
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Wikipedia wikitech RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.