Re: Change branch in the middle of a merge

From: Tao Klerks <tao@klerks.biz>
Date: May 1, 2023, 05:51 UTC
Message-ID: <CAPMMpognK_g5GL4EDamUmA5bzWiy9_VPf=a+0D5AnFtngUNa+A@mail.gmail.com>
In-Reply-To: <CAPx1GveK2J8Wu4cBYzO535c4bQ7P_t2fd1KFJqqphrm2HGQPSA@mail.gmail.com>

(background note: Chris accidentally sent a non-plaintext response, and I failed to respond for some time, but I still believe this is something worth fixing)

On Mon, Feb 27, 2023 at 10:16 PM Chris Torek <chris.torek@gmail.com> wrote:

Show 9 quoted lines

>
> On Mon, Feb 27, 2023 at 4:18 AM Tao Klerks <tao@klerks.biz> wrote:
>>
>>
>> All three of these strategies are quite awful things to have to teach users.
>
>
> I agree.
>

Thanks for confirming, this helped motivate my at least attempting a patch.

It's not ready yet, but what I have so far seems to work.

Show 15 quoted lines

>>
>> I'd like to understand whether this behavior, the fact that you can't
>> easily/straightforwardly/intuitively complete a merge onto a new
>> branch (with the exact same "first parent" commit of course), is
>> intentional for some reason that I am failing to grasp, or just a
>> missing feature.
>
>
> I don't know the answer to this.
>
> Internally, it seems trivial to fix: for the "create new branch" cases,
> simply *don't* remove the `MERGE_HEAD` ref.  This has an
> annoying side effect: you can now create new branch names but
> not switch among existing branches.  But that's kind of inherent to
> this state.

I don't understand this bit, but I'm probably missing something.

The approach I've taken here is to, when the original commit and the new commit are the same (you're switching to a same-head branch, or creating a new branch at the current head), change the "remove_branch_state()" call at https://github.com/git/git/blob/2807bd2c10606e590886543afe4e4f208dddd489/builtin/checkout.c#L1012 / implemented at https://github.com/git/git/blob/2807bd2c10606e590886543afe4e4f208dddd489/branch.c#L818, to not do the "remove_merge_branch_state()" bit - that is, avoid all the removals at https://github.com/git/git/blob/2807bd2c10606e590886543afe4e4f208dddd489/branch.c#L808.

This behaves as expected for switching to new and expected branches. To change the "git checkout" behavior this change is basically all that's needed (plus tests etc), but for "git switch" we also need to change some validation which would refuse to do the switch in the middle of a merge.

With these changes things behave in what I think is a very reasonable way, with one potentially-surprising aspect: the merge message proposed when you go to commit still refers to the branch you were on when you prepared the merge, rather than the one you're on now. I personally believe this is quite reasonable (you'd still see this if you followed any other process involving fast-forwarding for example), but I can see how it's not perfect.

The bit I'm having more conceptual difficulty with is considering what
to "say" when a checkout/switch completes with a merge-in-progress.
Historically, this happened "as normal", with no special messaging.
Now, I think there should be a message depending on whether the merge
state was discarded (head commit changed) or not:
 * If the commit remained the same and so the merge state was
retained, state as much and warn about the commit message referring to
the previous branch name.
 * If the commit changed and so the merge state was discarded ("git
checkout" only - not possible with "git switch"), state as much and
suggest that using "git switch" would prevent this happening
accidentally in future.

Does this kind of messaging make sense to you?

Show 6 quoted lines

>
> (Whether this is generally acceptable to others, I don't know. It would
> break backwards compatibility with `git checkout -b`, obviously, and
> making `git checkout -b` and `git switch -c` behave *differently* here
> feels wrong to me, but that's a different thing.)
>

After thinking about this a bit more, I agree that having divergent different behavior for checkout and switch would suck. The current existing divergences, as far as I can tell, are around validation/safety, not behavior when actions are completed.

I think the best thing is to propose a patch that incorporates a change in checkout behavior (stop throwing away merge metadata when you do a checkout/switch in the middle of a merge), with new messaging as outlined above, and see how people feel about it. I'm hopeful that a patch will see some more reactions than this thread did in its first round.