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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jul 14, 2020, 03:51 UTC
Message-ID: <20200714035104.1465772-1-jonathantanmy@google.com>
In-Reply-To: <20200714031017.GA15143@generichostname>

Show 15 quoted lines

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

Show 7 quoted lines

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

Show 6 quoted lines

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

Show 6 quoted lines

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