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

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

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

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

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

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

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

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

> 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