Re: [PATCH 00/12] RFC: In-core git merge-tree ("Server side merges")

[Elijah Newren <newren@gmail.com>]

On Sat, Jan 29, 2022 at 9:43 AM Elijah Newren <newren@gmail.com> wrote:
>
> Hi Christian,
>
> On Sat, Jan 29, 2022 at 12:18 AM Christian Couder
> <christian.couder@gmail.com> wrote:
> >
> > On Sat, Jan 29, 2022 at 8:04 AM Elijah Newren <newren@gmail.com> wrote:
> > >
> > > On Wed, Jan 26, 2022 at 12:48 AM Christian Couder
> > > <christian.couder@gmail.com> wrote:
[...]
> > > > The reason is
> > > > that I think in many cases when there are conflicts, the conflicts
> > > > will be small and the user will want to see them.
> > >
> > > I'm a little worried about the assumption here that conflict size is
> > > measurable and visible via diffs.  That might be true in some cases,
> > > but a UI written with that assumption is going to be very confusing
> > > when hitting cases where that assumption does not hold.  For example:
> > >
> > >   * What if there is a binary file conflict, or a modify/delete or
> > > rename/delete conflict, or failed-to-merge submodule conflict, or a
> > > file location conflict? (For these, there is no diff relative to the
> > > first parent and hence this conflict would have no diff output for
> > > it)?
> > >   * What if there was a simple file/directory conflict?  A diff would
> > > show a rename (even when neither side did any renames), but not any
> > > conflict markers.
> > >   * What if there was a rename/rename conflict (both sides renamed
> > > same file differently) or a distinct types conflict?  The former
> > > results in three different conflicting files, none of them with
> > > conflict markers, while the latter results in two different
> > > conflicting files both without conflict markers?  Showing individual
> > > per-file diffs totally loses all context here -- it'll show no-diff
> > > for one of the files, and totally new additions for the ones.
> >
> > In those cases we just tell users that they cannot resolve those
> > conflicts in the user interface, see the following doc:
> >
> > https://docs.gitlab.com/ee/user/project/merge_requests/conflicts.html#conflicts-you-can-resolve-in-the-user-interface
>
> So...I think you may have just convinced me that my fears were
> justified and that I should probably NAK any attempt to add diffs to
> the merge-tree command.  I won't jump to conclusions but you've
> provided some pretty strong signal to me against going down that
> route.  The list of limitations in the link you provide do mostly
> avoid the broken cases I listed above, but it enshrines those
> limitations on that webpage as fundamental rather than just as current
> implementation shortcomings.  You may not be able to remove those
> limitations on that webpage without either expunging the diffs from
> the UI or exposing the brokenness of the various cases above.
>
> If you do propose a diff option in the future, come prepared to
> discuss how you'll avoid accidentally leading others down into paths
> with the same fundamental issues, and/or how the above types of
> conflicts might still be meaningfully handled.

Actually, after having a few extra days to think about it, I thought
of something that should have been obvious to me, given my other
in-flight series that this depends upon...

If you used the same trick that remerge-diff does to include the
CONFLICT (and related messages) headers in the diff, then the kinds of
conflicts that are normally either invisible or misleading/confusing
to show via a diff would suddenly have the extra notices needed to
explain them, and make this problem tractable.

Further, it'd only make sense to do the special diff as part of the
merge-tree process, since it has the conflict messages strmap needed
to do this.

And there's not all that much work that would be needed to take
advantage of this, especially since this series already depends upon
the remerge-diff series.

So, maybe this is fine after all.

> Also, the list of limitations you have may not be quite comprehensive
> enough to avoid all problems (though it certainly avoids most of
> them).  Can I ask a couple clarifying questions about your list of
> limitations in that link? :
>
>   * When that page says the file cannot already contain conflict
> markers, is the check performed on the version of the file in the two
> trees being merged, or is the check performed on the 2nd and 3rd index
> stage of the merge result (these are not equivalent checks, even if
> they often give the same answer)?
>   * When that page says the file must already exist in the same path
> on both branches, is the check performed on by checking the path in
> the two trees being merged, or is the check performed on the 2nd and
> 3rd index stage of the merge result (again, these are not equivalent
> checks)?

I am still curious about this either way.