Re: [PATCH v2 0/7] Drop support for git rebase --preserve-merges

[Elijah Newren <newren@gmail.com>]

On Fri, Sep 10, 2021 at 5:08 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Elijah,
>
> On Tue, 7 Sep 2021, Elijah Newren wrote:
>
> > On Tue, Sep 7, 2021 at 11:51 AM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > >
...
> > > Thank you for clarifying.
> > >
> > > Yes, I remember how that idea came up, and I even tried that strategy for
> > > a couple of merging rebases of Git for Windows' branch thicket. Sadly, it
> > > did not work half as well as I had hoped.
> > >
> > > The best idea I had back then still is in want of being implemented: sort
> > > of a "four-way merge". It is basically the same as a three-way merge, but
> > > allows for the pre-images to differ in the context (and yes, this cannot
> > > be represented using the current conflict markers). Definitely not
> > > trivial.
> >
> > merge-ort opens a new possibility (since it does merges without
> > touching the index or working tree): Take the merge commit, M, that
> > you are trying to transplant.  Hold on to it for a minute.  Do what
> > rebase-merges does now; namely, do a simple merge of the desired new
> > branches that otherwise ignores M to get your new merge commit N.
> > Hang on to N too for a minute.  Now use merge-ort to auto-remerge M
> > (much like AUTO_MERGE or --remerge-diff does) to get a new merge
> > commit that we'll call pre-M.  If M was a clean merge that the user
> > didn't amend, then pre-M will match M.  If M wasn't a clean merge or
> > was amended, then pre-M will otherwise differ from M by not including
> > any manual changes the user made when they originally created M --
> > such as removing conflict markers, fixing semantic conflicts, evil
> > changes, etc.
> >
> > Now we've got three merge commits: pre-M, M, and N.  (Technically,
> > pre-M and N might be toplevel trees rather than full commits, but
> > whatever.)  The difference between pre-M and M represent the manual
> > work the user did in order to create M.  Now, do a three-way
> > (non-recursive) merge of those commits, to get the rebased result, R.
> > This operation has the affect of applying the changes from pre-M to M
> > on top of N.
> >
> > There's obviously some edge cases (e.g. nested conflict markers), but
> > I think they're better than the edge cases presented by the
> > alternatives:
> >   * the first-parent difference idea silently discards intermediate
> > changes from reapplying other patches (especially if other patches are
> > added or dropped), which to me feels _very_ dangerous
> >   * the current rebase-merges idea silently discards manual user
> > changes within the original merge commit (i.e. it hopes that there is
> > no difference between pre-M and M), which can also be lossy
> >   * I don't think this idea drops any data, but it does run the risk
> > of conflicts that are difficult to understand.  But I suspect less so
> > than your five-way merge would entail.
> >
> > If the difficulty of conflicts in this scheme is too high, we could do
> > a few things like providing multiple versions (e.g. if either
> > pre-M:file or N:file had conflicts, or maybe if R:file has nested
> > conflicts, then place both R:file and N:file in the working tree
> > somewhere) or pointing at special commands that help users figure out
> > what went on (e.g. 'git log -1 --remerge-diff M -- file').
>
> While I agree that `merge-ort` makes a lot of things much better, I think
> in this context we need to keep in mind that those nested merge conflicts
> can really hurt.

Yes, and I'm not sure you fully appreciate how much either.  Your
discussion in the other thread of nested conflicts suggests you may
not be aware of a few facts; for regular merges (not talking about
rebase-merges yet):

* Merges can have nested conflicts DESPITE having unique merge bases
and NOT using merge.conflictstyle=diff3
* With unique merge bases (i.e. not a "recursive" merge), a merge will
have at most two layers of conflicts
* non-unique merge-bases and merge.conflictstyle=diff3 make nested
conflicts much more likely
* There is no limit on the number of nestings of conflict markers for a merge

Now, in terms of rebase-merges:

You noted in the other thread the possibility of being unable to
differentiate conflict markers because they had the same length.
However, increasing the length on the second merge by one or two
characters is not sufficient in general, because that might just make
you match the length of one of the nested conflicts from the other
merge.  You need to programmatically determine the sizes of the
conflict markers in the first merge, and then adjust based on it for
your other merge(s).


I know this will sound like I'm scaring you off of my idea even
further, but I actually think despite the above that my ideas for
rebase-merges may have merit.  I just want people to actually
understand the edge and corner cases.  From my reading of the previous
threads on rebasing merges, I'm concerned that there is no discussion
of arbitrarily nested conflict markers and a complete omission of any
considerations for path-based rather than content-based conflicts.

> In my tests (I tried to implement a strategy where a 3-way merge is done
> with M and N^, using the parent commits of M as merge parents
> successively, see
> https://lore.kernel.org/git/nycvar.QRO.7.76.6.1804130002090.65@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz/
> for the nitty gritty), I ran into _nasty_ nested merge conflicts, even
> with trivial examples. And I came to the conviction that treating the
> merge conflict markers as Just Another Line was the main culprit.

I agree that we should not treat merge conflict markers as Just
Another Line.  There are other issues I can mention besides the above
here, but I'm getting kind of long already.

However, I think a big part of the problem you ran into was that the
previous suggestions only work nicely in _really_ simple cases.  In
particular, although I like Phillip's suggested sequence of merges[1]
a lot more than the other suggestions in that thread or from prior
threads, his suggestion is going to be prone to unnecessary nested
conflict markers, as you found in your testcase.

[1] https://lore.kernel.org/git/6c8749ca-ec5d-b4b7-f1a0-50d9ad2949a5@talktalk.net/

> I wish I had the time to try out your proposed strategy with the
> conconcted example I presented in that mail I linked above. Because now I
> am curious what it would do...

For this simple testcase, as best I understood it (you didn't quite
describe it fully so I had to take a guess or two), it provides a
simpler conflict than either of the two you showed you got (from
different merge orderings of Phillip's suggestion).   However, let me
double check that I understood your testcase correctly; please correct
any errors in my understanding.  I believe the commit graph you were
describing was this:

* rebase-of-original-merge
|\
| * rebase-of-local-add-caller-of-core
* | rebase-of-local-rename-to-hi
|/
* upstream
|
|
|
| * original-merge
| |\
| | * local-add-caller-of-core
| |/
|/|
| * local-rename-to-hi
|/
* initial-version


Further, the versions of main.c in each of those are as follows:

==> initial-version:
int core(void) {
    printf("Hello, world!\n");
}


==> local-rename-to-hi:
int hi(void) {
    printf("Hello, world!\n");
}


==> local-add-caller-of-core:
int core(void) {
    printf("Hello, world!\n");
}
/* caller */
void caller(void) {
    core();
}


==> what an automatic merge of the two local-* branches would give:
int hi(void) {
    printf("Hello, world!\n");
}
/* caller */
void caller(void) {
    core();
}


==> original-merge (amended from above by s/core/hi/ in caller()):
int hi(void) {
    printf("Hello, world!\n");
}
/* caller */
void caller(void) {
    hi();
}


==> upstream:
int greeting(void) {
    printf("Hello, world!\n");
}
/* main event loop */
void event_loop(void) {
    /* TODO: place holder for now */
}


==> rebase-of-local-rename-to-hi (kept 'hi' over 'greeting'):
int hi(void) {
    printf("Hello, world!\n");
}
/* main event loop */
void event_loop(void) {
    /* TODO: place holder for now */
}


==> rebase-of-local-add-caller-of-core (kept both new functions,
updated caller):
int greeting(void) {
    printf("Hello, world!\n");
}
/* main event loop */
void event_loop(void) {
    /* TODO: place holder for now */
}
/* caller */
void caller(void) {
    greeting();
}



If I've understood that all correctly, then my idea will give you the
following conflict to resolve:

==> rebase-of-original-merge, before conflict resolution:
int hi(void) {
    printf("Hello, world!\n");
}
/* main event loop */
void event_loop(void) {
    /* TODO: place holder for now */
}
/* caller */
void caller(void) {
<<<<<<< HEAD
    greeting();
||||||| auto-remerge of original-merge
    core();
=======
    hi();
>>>>>>> original-merge
}