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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 14, 2020, 09:52 UTC
Message-ID: <9c6dff59-b204-1ace-e0aa-0885dd502214@gmail.com>
In-Reply-To: <20200714031017.GA15143@generichostname>

Hi Denton

On 14/07/2020 04:10, Denton Liu wrote:

Show 27 quoted lines

> 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

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