[RFC] should `git rebase --keep-base` imply `--reapply-cherry-picks` ?

[RFC] should `git rebase --keep-base` imply `--reapply-cherry-picks` ?

[Philippe Blain]

Hello,

I learned today that doing `git rebase --keep-base master` 
will drop commits that were cherry-picked from master to the current branch. 
I was simply doing a code clean up on my feature branch (the full command was
`git rebase -i --keep-base master`), and this kind of confused me for a moment.

Is this a sane default ? I understand that it is a good default when we are rebasing 
*on top* of master, but here I'm just doing some squashing and fixup's and I did not
want the commit I had cherry-picked from master to disappear (yet). In fact, because it
was dropped, it created a modify/delete conflict because in a subsequent commit 
in my feature branch I'm modifying files that are added in the commit I cherry-picked.

How would a change that made '--reapply-cherry-picks' be the default when using 'keep-base'
be received ?

Tangential question: in any case, would it make sense to still add the "dropped because 
already upstream" commits to the todo list, in the case of an interactive rebase ? 
(maybe commented out, or listed as 'drop' with some kind of comment saying those 
are dropped because they appear textually upstream?)

Cheers,
Philippe.
P.S. I CC'd those who were involved with the 'keep-base' patch or the 'reapply-cherry-picks' patch.

Re: [RFC] should `git rebase --keep-base` imply `--reapply-cherry-picks` ?

[Denton Liu]

Hi Philippe,

On Mon, Jul 13, 2020 at 10:44:06PM -0400, Philippe Blain wrote:
> Hello,
> 
> I learned today that doing `git rebase --keep-base master` 
> will drop commits that were cherry-picked from master to the current branch. 
> I was simply doing a code clean up on my feature branch (the full command was
> `git rebase -i --keep-base master`), and this kind of confused me for a moment.

Glad I'm not the only one using this feature :)

> Is this a sane default ? I understand that it is a good default when we are rebasing 
> *on top* of master, but here I'm just doing some squashing and fixup's and I did not
> want the commit I had cherry-picked from master to disappear (yet). In fact, because it
> was dropped, it created a modify/delete conflict because in a subsequent commit 
> in my feature branch I'm modifying files that are added in the commit I cherry-picked.

So if I'm not mistaken, if we have the following graph

	A - B - C - D (master)
	     \
	       - C' - D (feature)

and we do `git rebase --keep-base master` from feature, C' will be
dropped? Indeed, I am surprised by how this interacts with the
default setting of --reapply-cherry-picks.

> How would a change that made '--reapply-cherry-picks' be the default when using 'keep-base'
> be received ?

I'm somewhat surprised that --no-reapply-cherry-picks is the default. I
would argue that it _shouldn't_ be the default at all. It's an
optimisation for when no --onto or --keep-base are specified but it
definitely can cause problems otherwise, as we've seen.

I think I would argue for the following in decreasing order of
preference:

	1. Make --no-reapply-cherry-picks the default in all cases.
	   (Those who need the optimisation can enable it manually and
	   we can add a configuration option for it.)

	2. Make --no-reapply-cherry-picks only active if no --onto or
	   --keep-base are given (--keep-base is a special case of --onto
	   so we only have to handle it in one place).

> Tangential question: in any case, would it make sense to still add the "dropped because 
> already upstream" commits to the todo list, in the case of an interactive rebase ? 
> (maybe commented out, or listed as 'drop' with some kind of comment saying those 
> are dropped because they appear textually upstream?)

That would make sense to me. I don't have a preference between either.

Thanks,

Denton

> Cheers,
> Philippe.
> P.S. I CC'd those who were involved with the 'keep-base' patch or the 'reapply-cherry-picks' patch.

Re: [RFC] should `git rebase --keep-base` imply `--reapply-cherry-picks` ?

[Jonathan Tan]

> > Is this a sane default ? I understand that it is a good default when we are rebasing 
> > *on top* of master, but here I'm just doing some squashing and fixup's and I did not
> > want the commit I had cherry-picked from master to disappear (yet). In fact, because it
> > was dropped, it created a modify/delete conflict because in a subsequent commit 
> > in my feature branch I'm modifying files that are added in the commit I cherry-picked.
> 
> So if I'm not mistaken, if we have the following graph
> 
> 	A - B - C - D (master)
> 	     \
> 	       - C' - D (feature)
> 
> and we do `git rebase --keep-base master` from feature, C' will be
> dropped? Indeed, I am surprised by how this interacts with the
> default setting of --reapply-cherry-picks.

Just to clarify, the default setting is --no-reapply-cherry-picks. (I
understand that the naming is confusing - the name was discussed quite a
bit [1].)

[1] https://lore.kernel.org/git/20200309205523.121319-1-jonathantanmy@google.com/

> > How would a change that made '--reapply-cherry-picks' be the default when using 'keep-base'
> > be received ?
> 
> I'm somewhat surprised that --no-reapply-cherry-picks is the default. I
> would argue that it _shouldn't_ be the default at all. It's an
> optimisation for when no --onto or --keep-base are specified but it
> definitely can cause problems otherwise, as we've seen.

When I encountered this feature, it was a surprise to me too, but this
has been documented as a feature for a long time (e.g. see the man page
for 2.1.4 from 2014 [2] - search for "RECOVERING FROM UPSTREAM REBASE").

[2] https://git-scm.com/docs/git-rebase/2.1.4

> I think I would argue for the following in decreasing order of
> preference:
> 
> 	1. Make --no-reapply-cherry-picks the default in all cases.
> 	   (Those who need the optimisation can enable it manually and
> 	   we can add a configuration option for it.)

I assume you mean "make --reapply-cherry-picks" the default (since
earlier you said that you were surprised that --no-reapply-cherry-picks
is the default)? Also what do you mean by "optimization"?
--reapply-cherry-picks is faster because it does not need to read the
upstream commits (optimization for speed) but --no-reapply-cherry-picks
fits some people's workflow better (optimization for workflow).

> 	2. Make --no-reapply-cherry-picks only active if no --onto or
> 	   --keep-base are given (--keep-base is a special case of --onto
> 	   so we only have to handle it in one place).

This might be confusing - I wouldn't expect "git rebase master" and "git
rebase master --onto master" to have different behavior.

> > Tangential question: in any case, would it make sense to still add the "dropped because 
> > already upstream" commits to the todo list, in the case of an interactive rebase ? 
> > (maybe commented out, or listed as 'drop' with some kind of comment saying those 
> > are dropped because they appear textually upstream?)
> 
> That would make sense to me. I don't have a preference between either.

This makes sense to me too.

Re: [RFC] should `git rebase --keep-base` imply `--reapply-cherry-picks` ?

[Phillip Wood]

Hi Denton

On 14/07/2020 04:10, Denton Liu wrote:
> Hi Philippe,
> 
> On Mon, Jul 13, 2020 at 10:44:06PM -0400, Philippe Blain wrote:
>> Hello,
>>
>> I learned today that doing `git rebase --keep-base master`
>> will drop commits that were cherry-picked from master to the current branch.
>> I was simply doing a code clean up on my feature branch (the full command was
>> `git rebase -i --keep-base master`), and this kind of confused me for a moment.
> 
> Glad I'm not the only one using this feature :)
> 
>> Is this a sane default ? I understand that it is a good default when we are rebasing
>> *on top* of master, but here I'm just doing some squashing and fixup's and I did not
>> want the commit I had cherry-picked from master to disappear (yet). In fact, because it
>> was dropped, it created a modify/delete conflict because in a subsequent commit
>> in my feature branch I'm modifying files that are added in the commit I cherry-picked.
> 
> So if I'm not mistaken, if we have the following graph
> 
> 	A - B - C - D (master)
> 	     \
> 	       - C' - D (feature)
> 
> and we do `git rebase --keep-base master` from feature, C' will be
> dropped? Indeed, I am surprised by how this interacts with the
> default setting of --reapply-cherry-picks.

To me the question is why are we looking at the upstream commits at all 
with `--keep-base`? I had expected `rebase --keep-base` to be the same 
as `rebase $(git merge-base [--fork-point] @{upstream} HEAD)` but 
looking at the code it seems to be `rebase --onto $(git merge-base 
@{upstream} HEAD) @{upstream}`. I didn't really follow the development 
of this feature - is there a reason we don't just use the merge-base as 
the upstream commit?

Best Wishes

Phillip

> 
>> How would a change that made '--reapply-cherry-picks' be the default when using 'keep-base'
>> be received ?
> 
> I'm somewhat surprised that --no-reapply-cherry-picks is the default. I
> would argue that it _shouldn't_ be the default at all. It's an
> optimisation for when no --onto or --keep-base are specified but it
> definitely can cause problems otherwise, as we've seen.
> 
> I think I would argue for the following in decreasing order of
> preference:
> 
> 	1. Make --no-reapply-cherry-picks the default in all cases.
> 	   (Those who need the optimisation can enable it manually and
> 	   we can add a configuration option for it.)
> 
> 	2. Make --no-reapply-cherry-picks only active if no --onto or
> 	   --keep-base are given (--keep-base is a special case of --onto
> 	   so we only have to handle it in one place).
> 
>> Tangential question: in any case, would it make sense to still add the "dropped because
>> already upstream" commits to the todo list, in the case of an interactive rebase ?
>> (maybe commented out, or listed as 'drop' with some kind of comment saying those
>> are dropped because they appear textually upstream?)
> 
> That would make sense to me. I don't have a preference between either.
> 
> Thanks,
> 
> Denton
> 
>> Cheers,
>> Philippe.
>> P.S. I CC'd those who were involved with the 'keep-base' patch or the 'reapply-cherry-picks' patch.

Re: [RFC] should `git rebase --keep-base` imply `--reapply-cherry-picks` ?

[Johannes Schindelin]

Hi,

On Tue, 14 Jul 2020, Phillip Wood wrote:

> On 14/07/2020 04:10, Denton Liu wrote:
> >
> > On Mon, Jul 13, 2020 at 10:44:06PM -0400, Philippe Blain wrote:
> > >
> > > I learned today that doing `git rebase --keep-base master` will drop
> > > commits that were cherry-picked from master to the current branch. I
> > > was simply doing a code clean up on my feature branch (the full
> > > command was `git rebase -i --keep-base master`), and this kind of
> > > confused me for a moment.
> >
> > Glad I'm not the only one using this feature :)
> >
> > > Is this a sane default ? I understand that it is a good default when
> > > we are rebasing *on top* of master, but here I'm just doing some
> > > squashing and fixup's and I did not want the commit I had
> > > cherry-picked from master to disappear (yet). In fact, because it
> > > was dropped, it created a modify/delete conflict because in a
> > > subsequent commit in my feature branch I'm modifying files that are
> > > added in the commit I cherry-picked.
> >
> > So if I'm not mistaken, if we have the following graph
> >
> >  A - B - C - D (master)
> >       \
> >         - C' - D (feature)
> >
> > and we do `git rebase --keep-base master` from feature, C' will be
> > dropped? Indeed, I am surprised by how this interacts with the
> > default setting of --reapply-cherry-picks.
>
> To me the question is why are we looking at the upstream commits at all
> with `--keep-base`? I had expected `rebase --keep-base` to be the same
> as `rebase $(git merge-base [--fork-point] @{upstream} HEAD)` but
> looking at the code it seems to be `rebase --onto $(git merge-base
> @{upstream} HEAD) @{upstream}`. I didn't really follow the development
> of this feature - is there a reason we don't just use the merge-base as
> the upstream commit?

Those are interesting questions, indeed.

And I dare to suspect that the answer is indeed: `--keep-base` really
should not only substitute `onto` but also `upstream` with the merge base.

Ciao,
Dscho

Re: [RFC] should `git rebase --keep-base` imply `--reapply-cherry-picks` ?

[Denton Liu]

Hi all,

On Tue, Jul 14, 2020 at 10:38:23PM +0200, Johannes Schindelin wrote:
> > To me the question is why are we looking at the upstream commits at all
> > with `--keep-base`? I had expected `rebase --keep-base` to be the same
> > as `rebase $(git merge-base [--fork-point] @{upstream} HEAD)` but
> > looking at the code it seems to be `rebase --onto $(git merge-base
> > @{upstream} HEAD) @{upstream}`. I didn't really follow the development
> > of this feature - is there a reason we don't just use the merge-base as
> > the upstream commit?

It behaves this way mostly for unimportant reasons. The first is that my
workflow before implementing this feature invoked running
`git rebase --onto master... master` and I wanted to replicate that.

More importantly, one feature of using the upstream I considered is
documented in t3431. Essentially, if we have the following graph,

	A---B---D---E    (master)
	     \
	      C*---F---G (side)
	
	C was formerly part of master but master was rewound to remove C

running `git rebase --keep-base --fork-point master` would drop C.

> Those are interesting questions, indeed.
> 
> And I dare to suspect that the answer is indeed: `--keep-base` really
> should not only substitute `onto` but also `upstream` with the merge base.

I would be open to changing the behaviour since the commit dropping
isn't really a feature that I use very often. However, I am worried
about pulling the rug out from other people if they use it since this is
a documented feature in git-rebase.txt.

Thanks,
Denton

> Ciao,
> Dscho

Re: [RFC] should `git rebase --keep-base` imply `--reapply-cherry-picks` ?

[Denton Liu]

On Mon, Jul 13, 2020 at 08:51:04PM -0700, Jonathan Tan wrote:
> > > How would a change that made '--reapply-cherry-picks' be the default when using 'keep-base'
> > > be received ?
> > 
> > I'm somewhat surprised that --no-reapply-cherry-picks is the default. I
> > would argue that it _shouldn't_ be the default at all. It's an
> > optimisation for when no --onto or --keep-base are specified but it
> > definitely can cause problems otherwise, as we've seen.
> 
> When I encountered this feature, it was a surprise to me too, but this
> has been documented as a feature for a long time (e.g. see the man page
> for 2.1.4 from 2014 [2] - search for "RECOVERING FROM UPSTREAM REBASE").
> 
> [2] https://git-scm.com/docs/git-rebase/2.1.4

Ah, I was mistaken. Thanks for the correction.

Re: [RFC] should `git rebase --keep-base` imply `--reapply-cherry-picks` ?

[Phillip Wood]

Hi all

On 15/07/2020 04:20, Denton Liu wrote:
> Hi all,
> 
> On Tue, Jul 14, 2020 at 10:38:23PM +0200, Johannes Schindelin wrote:
>>> To me the question is why are we looking at the upstream commits at all
>>> with `--keep-base`? I had expected `rebase --keep-base` to be the same
>>> as `rebase $(git merge-base [--fork-point] @{upstream} HEAD)` but
>>> looking at the code it seems to be `rebase --onto $(git merge-base
>>> @{upstream} HEAD) @{upstream}`. I didn't really follow the development
>>> of this feature - is there a reason we don't just use the merge-base as
>>> the upstream commit?
> 
> It behaves this way mostly for unimportant reasons. The first is that my
> workflow before implementing this feature invoked running
> `git rebase --onto master... master` and I wanted to replicate that.
> 
> More importantly, one feature of using the upstream I considered is
> documented in t3431. Essentially, if we have the following graph,
> 
> 	A---B---D---E    (master)
> 	     \
> 	      C*---F---G (side)
> 	
> 	C was formerly part of master but master was rewound to remove C
> 
> running `git rebase --keep-base --fork-point master` would drop C.
> 
>> Those are interesting questions, indeed.
>>
>> And I dare to suspect that the answer is indeed: `--keep-base` really
>> should not only substitute `onto` but also `upstream` with the merge base.
> 
> I would be open to changing the behaviour since the commit dropping
> isn't really a feature that I use very often. However, I am worried
> about pulling the rug out from other people if they use it since this is
> a documented feature in git-rebase.txt.

I think changing it to behave like

   git rebase --onto $(git merge-base @{upstream} HEAD) \
                     $(git merge-base --fork-point @{upstream} HEAD)

when --fork-point is given would keep the existing behavior of dropping 
commits when @{upstream} has been rewound without dropping cherry-picks. 
--fork-point seems less useful when combined with --keep-base than when 
used with a normal rebase as if @{upstream} has rewritten the commit 
that the branch is based on rather than just dropping it we end up 
dropping the original commit without getting the new version from 
upstream as we would with a normal rebase. This could be surprising to 
users if we keep --fork-point on by default with --keep-base.

Best Wishes

Phillip

> 
> Thanks,
> Denton
> 
>> Ciao,
>> Dscho