Re: [PATCH v2 8/8] show, log: include conflict/warning messages in --remerge-diff headers

From: Elijah Newren <newren@gmail.com>
Date: Dec 28, 2021, 23:42 UTC
Message-ID: <CABPp-BFP5Oc3vDQVJYZ4L=4ivSYuZBNk+Y9RHioJuSZKVzW0Dg@mail.gmail.com>
In-Reply-To: <20211228105755.zgahkoxn6ewjpdol@gmail.com>

On Tue, Dec 28, 2021 at 2:57 AM Johannes Altmanninger <aclopte@gmail.com> wrote:

>
> On Sat, Dec 25, 2021 at 07:59:19AM +0000, Elijah Newren via GitGitGadget wrote:

...

Show 7 quoted lines

> > +test_expect_success 'remerge-diff with non-content conflicts' '
> > +     git log -1 --oneline resolution >tmp &&
> > +     cat <<-EOF >>tmp &&
> > +     diff --git a/file_or_directory~HASH (side1) b/wanted_content
>
> the "~HASH (side1)" suffix will probably mess with some programs that extract
> the filename from the diff.

"~HASH (side1)" is part of the filename, so this won't mess those programs up at all (unless those programs can't deal with filenames containing spaces or parentheses or something).

> I don't know what programs are supposed to expect.  I can see arguments for
> either dropping the suffix or including only "~HASH" since that's part of
> the actual filename that's left in the worktree.

When there are conflicts that prevent the file from being recorded in the tree, such as file/directory conflicts, we have to rename the file elsewhere. We want the new name to be something that the user can find and reason about. So, both merge-recursive and merge-ort use ${filename}~${branchname}, where ${branchname} is defined in the struct merge_options (opt->branch1 or opt->branch2) that were passed in to the function.

For a regular `git merge` we just use opt->branch1 = "HEAD" and opt->branch2 = <name of branch/commit typed on command line to merge>.

Neither of those strings make sense for remerge-diff. We could just use hashes, but why are users going to be familiar with the hashes of the parents of a merge commit when looking at --remerge-diff output? Parents are not part of the output by default. So, log-tree.c uses this bit of logic

    format_commit_message(parent1, "%h (%s)", &parent1_desc, &ctx);
    format_commit_message(parent2, "%h (%s)", &parent2_desc, &ctx);
    o.branch1 = parent1_desc.buf;
    o.branch2 = parent2_desc.buf;

which means that the branch name is of the form "$HASH ($EXTRA_DESCRIPTION)", and thus that files in file/directory conflicts (or add/add + file/symlink conflicts or file/submodule conflicts) will get renamed to a file of the form "$FILENAME~$HASH ($EXTRA_DESCRIPTION)"

Note that these branch names also appear in CONFLICT messages, in conflict markers, etc., and in fact are used much more frequently in those locations. In those places it's perhaps even more important to attempt to provide meaningful names, so dropping the extra description doesn't make sense.

Show 13 quoted lines

> The file/link typechange conflict test I'll add below exposes what looks
> like an accidental interaction with the trailing tab characters that we emit
> on --- and +++ lines if the "filename" contains a space (since 1a9eb3b9d5
> (git-diff/git-apply: make diff output a bit friendlier to GNU patch (part
> 2), 2006-09-22)).
>
>         index 70885e4..0000000
>         --- a/typechange~738109f (side1)        <-- git diff adds a trailing tab!
>         +++ /dev/null
>
> I haven't formed an opinion yet, but since Tig uses the --- and +++ lines
> to extract file names, I'd drop the " (side1)" suffix from at least the ---
> and +++ lines. Maybe also the ^diff lines, I'm not sure

As above, " (side1)" is part of the filename and thus belongs here.

Show 7 quoted lines

> > +     similarity index 100%
> > +     rename from file_or_directory~HASH (side1)
> > +     rename to wanted_content
> > +     CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead.
>
> I wonder if it's better to have this line further up, before the "rename"
> resolution, to correct the temporal order.

Yeah, I've gone back and forth about where these would best be placed. You make a good point, even if the code is slightly uglier to move earlier. However, I do really like having the CONFLICT notices being close to the file text being shown, which makes me conflicted (no pun intended) about moving it earlier. Hmm....

Show 38 quoted lines

> > +     diff --git a/letters b/letters
> > +     CONFLICT (rename/rename): letters renamed to letters_side1 in HASH (side1) and to letters_side2 in HASH (side2).
> > +     diff --git a/letters_side2 b/letters_side2
> > +     deleted file mode 100644
> > +     index b236ae5..0000000
> > +     --- a/letters_side2
> > +     +++ /dev/null
> > +     @@ -1,9 +0,0 @@
> > +     -a
> > +     -b
> > +     -c
> > +     -d
> > +     -e
> > +     -f
> > +     -g
> > +     -h
> > +     -i
> > +     diff --git a/numbers b/numbers
> > +     CONFLICT (modify/delete): numbers deleted in HASH (side2) and modified in HASH (side1).  Version HASH (side1) of numbers left in tree.
> > +     EOF
>
> Took me some time to grok these but the output makes sense (it's loud and
> ugly but that's okay since these are serious conflicts).
>
> > +     # We still have some sha1 hashes above; rip them out so test works
> > +     # with sha256
> > +     sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >expect &&
> > +
> > +     git show --oneline --remerge-diff resolution >tmp &&
> > +     sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >actual &&
> > +     test_cmp expect actual
> > +'
> > +
> >  test_done
> > --
> > gitgitgadget
>
> We're missing a test case for typechange.

<grin> ...and a testcase for an add/add conflict, and a file/submodule conflict, and file/symlink, and symlink/submodule, and submodule/submodule (different submodules both added at same path), and binary merge conflict, and a variety of failure-to-merge-submodule-updates, and at least half a dozen regular rename-based conflict types, and several directory-rename-based conflict types...and that's just beginning to scratch the surface once you start dreaming of combinations of the different conflict types occurring for the same path (in particular, I'm thinking of examples from the testcases found in t6416, t6422, t6423 -- or at least sections 7, 9, & 12 of t6423).

I don't think providing a comprehensive set of possible conflicts is useful here; we just need a representative sample. I was curious whether that was best served with just two examples or three, but ultimately decided on 3. I would have been more likely to pick 2 than 4, though.

However, while I fail to see how typechange stresses --remerge-diff in ways the other conflict types don't, or how it might help clarify the output for users, I might be overlooking something. Is there a particular reason you wanted to see the typechange conflict included?

Show 11 quoted lines

>  Here's is a quick draft I've been
> playing around with. Seems ugly that the "diff --git a/typechange b/typechange"
> is doubled but okay.
>
> Maybe a rename/delete conflict is interesting as well, I'm not sure.  (Also I
> wonder if switching the order of parents will give any interesting difference,
> I guess not)
>
> test_expect_success 'remerge-diff with file/link conflict' '
>         git branch -d base side1 side2 &&
>         git switch --orphan base &&

I'd rather have subdirectories with git repositories (much like t6416, t6422, and t6423) if we're going to be adding many more tests here.

Show 61 quoted lines

>         echo base >typechange &&
>         git add typechange &&
>         git commit -m base &&
>
>         git branch side1 &&
>         git branch side2 &&
>
>         git checkout side1 &&
>         echo orig-file-contents >typechange &&
>         git commit -a -m side1 &&
>
>         git checkout side2 &&
>         ln -sf . typechange &&
>         git add typechange &&
>         git commit -m side2 &&
>
>         git checkout -b resolution2 side1 &&
>         test_must_fail git merge side2 &&
>         rm typechange &&
>         mv typechange~HEAD typechange &&
>         echo resolved >>typechange &&
>         git add typechange~HEAD typechange &&
>         git merge --continue &&
>
>         git show --oneline --remerge-diff resolution2 >tmp &&
>         sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >actual &&
>
>         cat <<-EOF >tmp &&
>         7759b27 Merge branch ${SQ}side2${SQ} into resolution2
>         diff --git a/typechange b/typechange
>         deleted file mode 120000
>         CONFLICT (distinct types): typechange had different types on each side; renamed one of them so each can be recorded somewhere.
>         index 945c9b4..0000000
>         --- a/typechange
>         +++ /dev/null
>         @@ -1 +0,0 @@
>         -.
>         \ No newline at end of file
>         diff --git a/typechange b/typechange
>         new file mode 100644
>         CONFLICT (distinct types): typechange had different types on each side; renamed one of them so each can be recorded somewhere.
>         index 0000000..70885e4
>         --- /dev/null
>         +++ b/typechange
>         @@ -0,0 +1,2 @@
>         +orig-file-contents
>         +resolved
>         diff --git a/typechange~738109f (side1) b/typechange~738109f (side1)
>         deleted file mode 100644
>         index 70885e4..0000000
>         --- a/typechange~738109f (side1)
>         +++ /dev/null
>         @@ -1 +0,0 @@
>         -orig-file-contents
>         EOF
>         # We still have some sha1 hashes above; rip them out so test works
>         # with sha256
>         sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >expect &&
>
>         test_cmp expect actual
> '

but otherwise the testcase looks good, if we want it.