Thoughts about the -m option of cherry-pick and revert

Thoughts about the -m option of cherry-pick and revert

[Stefan Haller]

There are plenty of StackOverflow questions and blog posts about the
error message that you get when you use git cherry-pick or git revert on
a merge commit without specifying the -m option. Many people don't seem
to understand what the error message means, or why they even get an
error in the first place.

The answers to these questions patiently explain what the error means
and why the -m option is necessary. Many of them contain example
scenarios; but I haven't seen a single one that doesn't use -m1 to
illustrate the usage.

I have two questions:

- What are real-world scenarios where you would use a mainline number
  other than 1? I could only come up with a single example myself, which
  is that you have a topic branch, and right before merging it back to
  main, you merge main into the topic branch; and then you merge it to
  main with a fast-forward merge. If you then want to cherry-pick or
  revert that topic, you'd have to use -m2 on that last merge from main.
  Any other examples?
- Wouldn't it make sense to default to -m1 when no -m option is given?
  It seems that this would do the expected thing in the vast majority of
  cases.

For the GUI client that I'm co-maintaining (lazygit), I'm actually
considering going so far as to not providing a choice at all, and always
using -m1. I'm not fully decided yet if that's a good idea, but it seems
that most people expect this, most of the time.

I'm probably missing something though, but what?

-Stefan

Re: Thoughts about the -m option of cherry-pick and revert

[Junio C Hamano]

Stefan Haller <lists@haller-berlin.de> writes:

> I have two questions:
>
> - What are real-world scenarios where you would use a mainline number
>   other than 1? I could only come up with a single example myself, which
>   is that you have a topic branch, and right before merging it back to
>   main, you merge main into the topic branch; and then you merge it to
>   main with a fast-forward merge. If you then want to cherry-pick or
>   revert that topic, you'd have to use -m2 on that last merge from main.
>   Any other examples?

I do think your example is a real issue that is helped by using -m2;
I do not think of any other cases offhand myself.

> - Wouldn't it make sense to default to -m1 when no -m option is given?
>   It seems that this would do the expected thing in the vast majority of
>   cases.

I do agree -m2 or higher would be rare when doing "git revert".  

Given that the current behaviour was chosen to make sure that the
user is aware that the commit being reverted/cherry-picked is a
merge and has a chance to choose the right parent (as opposed to
blindly picking the first parent that happened to be the right one
by accident), I am not sure if it is prudent to change the
behaviour.

If I were simplifying this, I would probably

 (1) disallow cherry-picking a merge (and suggest redoing the same
     merge, possibly after rebasing the copy of the merged history
     to an appropriate base as needed), and
 (2) allowing reverting a merge only wrt the first parent,

but that is a different story.

Re: Thoughts about the -m option of cherry-pick and revert

[Stefan Haller]

On 21.06.24 04:03, Junio C Hamano wrote:
> Stefan Haller <lists@haller-berlin.de> writes:
> 
>> - Wouldn't it make sense to default to -m1 when no -m option is given?
> 
> Given that the current behaviour was chosen to make sure that the
> user is aware that the commit being reverted/cherry-picked is a
> merge and has a chance to choose the right parent (as opposed to
> blindly picking the first parent that happened to be the right one
> by accident), I am not sure if it is prudent to change the
> behaviour.

Hm, in all example scenarios I experimented with, picking the wrong
parent would result in an empty diff, and consequently an error message
like this:

   nothing to commit, working tree clean
   The previous cherry-pick is now empty, possibly due to conflict
   resolution.
   If you wish to commit it anyway, use:

       git commit --allow-empty

   Otherwise, please use 'git cherry-pick --skip'

I'm not sure if this error is easier or harder to understand than the
one you get today when omitting -m, but we could probably improve it by
mentioning the -m option if the cherry-picked commit was a merge.

I'd be interested in example scenarios where both sides of the merge
have non-empty diffs. Won't this only happen for evil merges?

> If I were simplifying this, I would probably
> 
>  (1) disallow cherry-picking a merge (and suggest redoing the same
>      merge, possibly after rebasing the copy of the merged history
>      to an appropriate base as needed), and

This seems unnecessarily restrictive to me. Cherry-picking merge commits
using -m1 is useful, it's an important part of our release workflow at
my day job.

>  (2) allowing reverting a merge only wrt the first parent,

Interesting, that's what I'm considering doing in lazygit (except for
both revert and cherry-pick), but I kind of didn't expect much support
for that idea. :-)

-Stefan

Re: Thoughts about the -m option of cherry-pick and revert

[Phillip Wood]

On 21/06/2024 07:33, Stefan Haller wrote:
> On 21.06.24 04:03, Junio C Hamano wrote:
>> Stefan Haller <lists@haller-berlin.de> writes:
 >
> Hm, in all example scenarios I experimented with, picking the wrong
> parent would result in an empty diff, and consequently an error message
> like this:
> 
>     nothing to commit, working tree clean
>     The previous cherry-pick is now empty, possibly due to conflict
>     resolution.
>     If you wish to commit it anyway, use:
> 
>         git commit --allow-empty
> 
>     Otherwise, please use 'git cherry-pick --skip'
> 
> I'm not sure if this error is easier or harder to understand than the
> one you get today when omitting -m, but we could probably improve it by
> mentioning the -m option if the cherry-picked commit was a merge.

That might be helpful - if we do that we'd want to make sure that the 
user can retry this pick with "-m" without restarting the whole cherry-pick.

> I'd be interested in example scenarios where both sides of the merge
> have non-empty diffs. Won't this only happen for evil merges?

I think you'd need a conflicting merge that is resolved in a way that 
the resolution of the conflicting lines doesn't match either parent. (I 
assume that's what you mean by evil but I thought it best to check)

>> If I were simplifying this, I would probably
>>
>>   (1) disallow cherry-picking a merge (and suggest redoing the same
>>       merge, possibly after rebasing the copy of the merged history
>>       to an appropriate base as needed), and
> 
> This seems unnecessarily restrictive to me. Cherry-picking merge commits
> using -m1 is useful, it's an important part of our release workflow at
> my day job.

I can see why people want to revert merges but cherry-picking them 
always feels strange to me - what is the advantage over actually merging 
the branch and seeing the full history of that commit?

>>   (2) allowing reverting a merge only wrt the first parent,
> 
> Interesting, that's what I'm considering doing in lazygit (except for
> both revert and cherry-pick), but I kind of didn't expect much support
> for that idea. :-)

For lazygit I would think it would be fine to be a bit more restrictive 
that git as anyone with an unusual requirement can always fall back to 
using git for that.

Best Wishes

Phillip


> -Stefan
> 

Re: Thoughts about the -m option of cherry-pick and revert

[Stefan Haller]

On 21.06.24 12:19, Phillip Wood wrote:
> On 21/06/2024 07:33, Stefan Haller wrote:
> 
>> I'd be interested in example scenarios where both sides of the merge
>> have non-empty diffs. Won't this only happen for evil merges?
> 
> I think you'd need a conflicting merge that is resolved in a way that
> the resolution of the conflicting lines doesn't match either parent. (I
> assume that's what you mean by evil but I thought it best to check)

Ah yes, that's another example, but it's not an evil merge. An evil
merge is one that has additional changes that don't come from either
side, and don't come from conflict resolution (e.g. they were amended
into the merge). I thought that was commonly understood terminology.

>> This seems unnecessarily restrictive to me. Cherry-picking merge commits
>> using -m1 is useful, it's an important part of our release workflow at
>> my day job.
> 
> I can see why people want to revert merges but cherry-picking them
> always feels strange to me - what is the advantage over actually merging
> the branch and seeing the full history of that commit?

It's less work (if you otherwise insist on rebasing the branch to the
destination before merging), and results in a simpler graph that's
easier to understand (if you don't).

And I suppose you could ask the same question about the --squash option
of git-merge.

-Stefan

Re: Thoughts about the -m option of cherry-pick and revert

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> I can see why people want to revert merges but cherry-picking them
> always feels strange to me - what is the advantage over actually
> merging the branch and seeing the full history of that commit?

One case that comes to my mind is when you failed to plan ahead and
used a wrong base when building a series to "fix" an old bug.  You
built a 7-patch series to fix a bug that you introduced in release
1.0, but instead of basing the fix on maint-1.0 maintenance track,
you forked from the tip of master that is preparing for your next
feature release that is release 1.4.

Even if you realized that the fix is important enough to warrant
applying to the maint-1.0 maintenance track, you cannot merge the
topic that houses 7-patch series down to the old maintenance track
without bringing all the new features that happened since 1.0 on the
master track.

A kosher way may be to rebase the 7-patch series to maint-1.0 and
merge the result into the maint-1.0 track (and upwards if needed).
But cherry-picking the commit that merged the original "fix" topic
into master _may_ be simpler, as you need to resolve a larger
conflict but (hopefully) only once, instead of up to 7 times, once
per each commit on the "fix" topic while rebasing.

But of course if something goes wrong, it makes the result
impossible to bisect---exactly the same reason why you should think
twice before doing a "merge --squash".  In addition, if you somehow
figured out why the cherry-picked fix was inadequate, you'd now need
to forward-port the fix for the fix to the master track or whereever
the cherry-picked-merge was taken from.

On the other hand, if the original "fix" branch was rebased on
maint-1.0 and then further fixed, the result can be merged to
maint-1.0 as well as all the way to the master track.

So, I can understand why people may want to cherry-pick a merge,
I suspect it is a false economy.  Optimizing for picking, paying
higher price when the result of (incorrect) picking has to be
corrected later.

Re: Thoughts about the -m option of cherry-pick and revert

[Stefan Haller]

On 21.06.24 18:34, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> I can see why people want to revert merges but cherry-picking them
>> always feels strange to me - what is the advantage over actually
>> merging the branch and seeing the full history of that commit?
> 
> One case that comes to my mind is when you failed to plan ahead and
> used a wrong base when building a series to "fix" an old bug.  You
> built a 7-patch series to fix a bug that you introduced in release
> 1.0, but instead of basing the fix on maint-1.0 maintenance track,
> you forked from the tip of master that is preparing for your next
> feature release that is release 1.4.
> 
> Even if you realized that the fix is important enough to warrant
> applying to the maint-1.0 maintenance track, you cannot merge the
> topic that houses 7-patch series down to the old maintenance track
> without bringing all the new features that happened since 1.0 on the
> master track.
> 
> A kosher way may be to rebase the 7-patch series to maint-1.0 and
> merge the result into the maint-1.0 track (and upwards if needed).
> But cherry-picking the commit that merged the original "fix" topic
> into master _may_ be simpler, as you need to resolve a larger
> conflict but (hopefully) only once, instead of up to 7 times, once
> per each commit on the "fix" topic while rebasing.
> 
> But of course if something goes wrong, it makes the result
> impossible to bisect---exactly the same reason why you should think
> twice before doing a "merge --squash".  In addition, if you somehow
> figured out why the cherry-picked fix was inadequate, you'd now need
> to forward-port the fix for the fix to the master track or whereever
> the cherry-picked-merge was taken from.
> 
> On the other hand, if the original "fix" branch was rebased on
> maint-1.0 and then further fixed, the result can be merged to
> maint-1.0 as well as all the way to the master track.
> 
> So, I can understand why people may want to cherry-pick a merge,
> I suspect it is a false economy.  Optimizing for picking, paying
> higher price when the result of (incorrect) picking has to be
> corrected later.

You may call this "failed to plan ahead", but for us it's a deliberate
decision to work this way. Developers work exclusively on main, and
merge their branches to main, always. Release management decides later
(sometimes much later) which of these branches are cherry-picked to
which release branches. We never merge back from a release branch to main.

And we prefer single-commit cherry-picks of the merge commits because it
makes the history of the release branches easier to read. Bisectability
is not an issue; developers bisect failures on the main branch. (Yes,
I'm aware that there may be cases where a defect manifests itself
differently (or not at all) on main than on the release branch, but
these are so rare that it hasn't been an issue for us so far.)

I'm not saying I'm very happy with this workflow, it wasn't my decision.
And in particular I'm not trying to argue which workflow is better than
the other; all I'm saying is that there are teams who decide they want
to cherry-pick merge commits, so git should continue to allow it. This
is only in response to your earlier "If I were simplifying this, I would
probably [...] disallow cherry-picking a merge".

(Side note: my main gripe about cherry-picking in general is, of course,
that it makes it impossible to use "git branch --contains" or "git tag
--contains" to find out which releases contain a given bug fix; but
that's a problem no matter whether you cherry-pick the merge commit, or
replay the branch on maint and merge it there again.)

-Stefan

Re: Thoughts about the -m option of cherry-pick and revert

[Junio C Hamano]

Stefan Haller <lists@haller-berlin.de> writes:

> And we prefer single-commit cherry-picks of the merge commits because it
> makes the history of the release branches easier to read.

I would have expected that "git log --first-parent" would give you
the same "easy-to-read" history as a run of squached cherry-picks.

It of course takes some discipline to ensure that the first-parent
chain is kept meaningful, but "never merge, always cherry-pick the
merges of topics" also already takes some discipline, so I do not
see either workflow has much upside with respect to this point.

> I'm not saying I'm very happy with this workflow, it wasn't my decision.
> And in particular I'm not trying to argue which workflow is better than
> the other; all I'm saying is that there are teams who decide they want
> to cherry-pick merge commits, so git should continue to allow it. This
> is only in response to your earlier "If I were simplifying this, I would
> probably [...] disallow cherry-picking a merge".

Sure. I thought it was fairly obvious to everybody that I was not
"simplifying this", at least unilaterally, so raising a concern like
you did was the right thing ;-).

> (Side note: my main gripe about cherry-picking in general is, of course,
> that it makes it impossible to use "git branch --contains" or "git tag
> --contains" to find out which releases contain a given bug fix; but
> that's a problem no matter whether you cherry-pick the merge commit, or
> replay the branch on maint and merge it there again.)

Correct for the "cherry-picking", but not necessarily for a "rebase
and merge".

You can have (1) a "fix" based on the "main", and (2) the backport
of the same "fix" rebased on the "maint", the latter of which has
likely been spawned after the former was merged to "main".  You can
merge the "rebased fix" to the "maint" *as well as* to the "main".

If we think about it, that is a natural thing to do.  By rebasing
"fix" to "maint" to create "rebased fix", we are "correcting" an
earlier mistake of basing the fix on a wrong (iow, too recent)
point, so after such a rebase, we treat the rebased result as the
primary thing.

After that, "contains" will do the right thing for the "rebased
fix", which is now the primary fix, and shows both "main" and
"maint" has it.

Of course, a project can choose to adopt a workflow that refuses to
obtain such a benefit (presumably for other reasons that gives
benefits that outweigh the "sigh, we cannot merge but have to
cherry-pick, with all the inconveniences").

Re: Thoughts about the -m option of cherry-pick and revert

[Phillip Wood]

On 21/06/2024 03:03, Junio C Hamano wrote:
> Stefan Haller <lists@haller-berlin.de> writes:
>
> Given that the current behaviour was chosen to make sure that the
> user is aware that the commit being reverted/cherry-picked is a
> merge and has a chance to choose the right parent (as opposed to
> blindly picking the first parent that happened to be the right one
> by accident), I am not sure if it is prudent to change the
> behaviour.


FWIW I agree with this, for me the main benefit of the current behavior 
is stopping when I'm not expecting to cherry-pick a merge.

Best Wishes

Phillip

> If I were simplifying this, I would probably
> 
>   (1) disallow cherry-picking a merge (and suggest redoing the same
>       merge, possibly after rebasing the copy of the merged history
>       to an appropriate base as needed), and
>   (2) allowing reverting a merge only wrt the first parent,
> 
> but that is a different story.
>