Re: [PATCH 1/7] merge-ort: mark a few more conflict messages as omittable

From: Elijah Newren <newren@gmail.com>
Date: Sep 1, 2021, 00:03 UTC
Message-ID: <CABPp-BEZjMLGFxSsTfRnAffom9CKRU4if4PsdTh+85L6D=FseA@mail.gmail.com>
In-Reply-To: <xmqqzgsxs4rk.fsf@gitster.g>

On Tue, Aug 31, 2021 at 2:06 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > path_msg() has the ability to mark messages as omittable when recording
> > conflict messages in an in-tree file.  This conflict message file will
> > then be shown by diffing the merge-created tree to the actual merge
> > commit that is created.  While all the messages touched in this commit
> > are very useful when trying to create a merge initially, early use with
> > the --remerge-diff feature (the only user of this omittable conflict
> > message capability), suggests that the particular messages marked in
> > this commit are just noise when trying to see what changes users made to
> > create a merge commit.  Mark them as omittable.
>
> Sorry for asking something that may be obvious

No, it's not obvious. I had a hard time figuring out the right way to split up this series and order the patches in part because so many little pieces are needed before I can show the solution. So some of this comes from the later patches, but let me see if I can motivate the issue and solution I picked a bit more right here.

> but if you recreate
> an automated and potentially conflicting merge in-core, in order to
> then compare the recorded merge result, is there *any* message that
> is worth showing while doing the first step,

Yes, absolutely. While three-way *content* conflicts are easily representable within a file in the working tree using conflict markers, *non-content* conflicts are usually not easily representable in such a fashion. For example, a rename/delete conflict will not result in any conflict markers, and will result in the renamed version of the file being left in the working tree; the only way you know there is a conflict for that file is either because of the stage in the index or the the message that is printed to the terminal:

    CONFLICT (rename/delete): %s renamed to %s in %s, but deleted in %s.

But relying on higher order stages in the index and messages printed to the terminal present a bit of a problem for --remerge-diff; as described, it ignores both those sources of input. Here's a few other conflict types that face similar issues:

  * modify/delete conflicts
  * failure to merge submodule
  * directory rename detection (i.e. new file added to old directory
that other side renamed; which directory should file end up in?)
  * distinct types of files (e.g. regular file and symlink) located at
the same path
  * rename/rename conflicts

In all these cases (and some others), relying on a diff of "what the working directory looks like at the end of a merge" to "the tree recorded in the merge commit" does not convey enough (if any) information about the above types of conflicts.

> and where in the output do users see them?

For --remerge-diff, I chose to handle the fact that the working directory didn't naturally have enough information, by augmenting the working directory with additional information. So, for example, if there was a file named `dir/my.file` that had a modify/delete conflict (and the user who did the real merge edited that file a bit as part of conflict resolution), then I would also add a `dir/my.fil e.conflict_msg` file whose contents are

""" == Conflict notices for my.file == CONFLICT (modify/delete): dir/my.file deleted in HASH1 (SHORT SUMMARY1) and modified in HASH2 (SHORT SUMMARY 2). Version HASH2 (SHORT SUMMARY2) of dir/my.file left in tree. """

Creating this file means you see something like this in the remerge-diff (note there are diffs for two files, with the synthetic file appearing just before the file it has messages about):

diff --git a/dir/my.fil e.conflict_msg b/dir/my.fil e.conflict_msg
deleted file mode 100644
index 2bd215a32f06..000000000000
--- a/dir/my.fil e.conflict_msg
+++ /dev/null
@@ -1,2 +0,0 @@
-== Conflict notices for my.file ==
-CONFLICT (modify/delete): dir/my.file deleted in HASH1 (SHORT
SUMMARY1) and modified in HASH2 (SHORT SUMMARY 2).  Version HASH2
(SHORT SUMMARY2) of  dir/my.file left in tree.
diff --git a/dir/my.file b/dir/my.file
index 09c78c725a3b..79f9d1fb7611 100644
--- a/dir/my.file
+++ b/dir/my.file
@@ -950,15 +912,15 @@ int my_func(struct stuff *data,
                                        rq->timeline,
                                        rq);

-               old_func(data, 1);
+               new_func("for funsies", data, VALID);
                obj->value = 8;
                obj->read_attempts = 0;
        } else {
-               err = old_func(data, 0);
+               err = new_func("for funsies", data, INVALID);
                if (unlikely(err))
                        return err;

-               another_old_func(data, obj->value);
+               another_new_func(data, obj->value, INVALID);
                obj->value++;
        }
        obj->read_attempts += 1;


The `dir/my.fil e.conflict_msg` file is definitely slightly weird, but
any solution here would be.  I think it's fairly intuitive what is
meant.  No one has commented on this choice in the 9+ months it's been
in use internally by ~50 users (even with -p implying --remerge-diff
automatically to make it more likely people have used this option), so
either it really is intuitive, or it doesn't come up much.  It could
well be the latter.