Re: [PATCH] pull: conflict hint pull.rebase suggestion should offer "merges" vs "true"

From: Tao Klerks <tao@klerks.biz>
Date: Feb 20, 2023, 08:03 UTC
Message-ID: <CAPMMpogi_QoGKD824JW+85v_Sgaf5d3TAd_P55YyT5NF6AUJ=w@mail.gmail.com>
In-Reply-To: <c3ef69e0-c37a-01fe-a40a-c2940e329793@dunelm.org.uk>

On Sat, Feb 18, 2023 at 5:39 PM Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 16 quoted lines

>
> On 18/02/2023 03:17, Elijah Newren wrote:
> >
> > One concern I have is that "--rebase-merges" itself has negative user
> > surprises in store.  In particular, "--rebase-merges", despite its
> > name, does not rebase merges.  It uses the existing author & commit
> > message info, but otherwise just discards the existing merge and
> > creates a new one.  Any information it contained about fixing
> > conflicts, or making adjustments to make the two branches work
> > together, is summarily and silently discarded.
>
> That's a good point. Another potentially surprising behavior is that
> when I'm rebasing an integration branch with -rno-rebase-cousins then if
> one of the topic branches merged into the integration branch happens to
> share the same base as the integration branch itself the topic branch
> gets rebased as well.

I've been trying to understand how this behavior is (potentially) surprising - I imagine it's been discussed elsewhere but I'm having a hard time understanding, sorry.

The situation you described is a boundary condition between two others, right?
* The topic branch could be branched from the integration branch
(potentially *after* some other change were made to the integration
branch, but not in this case) - in which case rebasing is what you
would expect
* The topic branch could be branched from the main branch (potentially
*before* the integration branch branched, but not in this case) - in
which case not rebasing is what you would expect.

If topic branched from main (at around the same time as integration), it might be surprising that it rebases; if it branched from integration (before that had any changes), then it is expected.

Show 7 quoted lines

> -rno-rebase-cousins is also slower that it needs
> to be because it creates a todo list that contains all the commits on
> the topic branches merged into the integration branch rather than just
> the merges. The commits on the topic branches are fast-forwarded rather
> than rewritten so long as they don't share the same base as the
> integration branch but it noticeably slower than using a todo list with
> just the merge commands.

This seems improvable, but no worse than a plain legacy rebase (as Alex's new patch would have it, "rebase-merges=drop"), right? Insofar as we're discussing why it might make sense to avoid promoting this over a plain rebase, I don't understand the concern.

Show 13 quoted lines

>
> > My personal opinion would be adding such a capability should be step
> > 2.5 in your list, though I suspect that would make Tao unhappy (it's a
> > non-trivial amount of work, unlike the other steps in your list).
>
> I've got a couple of patches[1] that cherry-pick the merge if only one
> of the parents has changed. I've never tried upstreaming them as it is
> only a partial solution to the problem of rebasing merges but that
> approach should work well with "git pull --rebase=merges" as only the
> upstream side will have changed (when rebasing my git integration branch
> with that patch the merges are cherry-picked). They might make a useful
> starting point if anyone wants to try and improve the rebasing of merges.
>

This is awesome!

It feels like the first step towards the general strategy that was (I believe) best described by Buga at https://public-inbox.org/git/a0cc88d2-bfed-ce7b-1b3f-3c447d2b32da@gmail.com/ !

(unless I'm missing something, the result of this is exactly the same as the result of that strategy, in these "simple" cases where it kicks in)

The one concern I have with this is that, *if I understand correctly*, it sometimes throws away the existing merge information, and sometimes doesn't, and there's no easy way to know which it is at runtime. Would adding a warning on stderr when a both-parents merge is encountered (and any merge resolutions or related changes are still discarded) be enough to make this shippable?

Are there *any* circumstances where the new cherry-picking behavior introduced here wouldn't be the right thing to have happen?