Re: Bug in 'git log --remerge-diff' when used with '--find-object' and '--submodule=log|diff'

From

Elijah Newren <newren@gmail.com>

Date

Aug 24, 2022, 07:36 UTC

Message-ID

<CABPp-BEkC8xEkNa+hyKFKhO=cbBZqNqGWehqxbRzE6-BVR27NQ@mail.gmail.com>

In-Reply-To

<43cf2a1d-058a-fd79-befe-7d9bc62581ed@gmail.com>

Hi Philippe,

>
> Hi Elijah,
>
> I found two bugs in '--remerge-diff' when combined with both '--find-object' and
> '--submodule=log|diff'. I don't know if they have the same cause.
>
> When using these flags together, *all* 2-parents merge commits are shown (even in a repo with
> no submodule!)
>
> Moreover, for merges with conflicted paths, all conflicted paths are shown as "(new submodule)",
> even if they are not a submodule (in fact, even when no submodule is present
> in the repository!).
>
> This artificial example reproduces the bug:
>
> ```bash
> #!/bin/bash
>
> set -euEo pipefail
>
> repo=repro
> rm -rf $repo
>
> TEST_AUTHOR_LOCALNAME=author
> TEST_AUTHOR_DOMAIN=example.com
> GIT_AUTHOR_EMAIL=${TEST_AUTHOR_LOCALNAME}@${TEST_AUTHOR_DOMAIN}
> GIT_AUTHOR_NAME='A U Thor'
> GIT_AUTHOR_DATE='1112354055 +0200'
> TEST_COMMITTER_LOCALNAME=committer
> TEST_COMMITTER_DOMAIN=example.com
> GIT_COMMITTER_EMAIL=${TEST_COMMITTER_LOCALNAME}@${TEST_COMMITTER_DOMAIN}
> GIT_COMMITTER_NAME='C O Mitter'
> GIT_COMMITTER_DATE='1112354055 +0200'
> export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME
> export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME
> export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
> export HOME=
>
> git -c init.defaultBranch=unimportant init $repo
> cd $repo
> echo i>file
> git add file
> git commit -m file
> git checkout -b side
> echo s>>file2
> git add file2
> git commit -am side
> git checkout -
> echo m >>file
> git commit -am main
> git merge side -m clean
> git checkout side
> echo c>>file
> git add file
> git commit -am side2
> git checkout -
> echo cc>>file
> git commit -am main2
> git merge side || true
> printf 'i\nm' > file
> git commit -am conflicted
> # look for a dummy object
> git log --oneline --diff-merges=r --submodule=log --find-object=2c042ac4213768e55791098110d2ef2ef845881a
> # same output with --submodule=diff
> ```
>
> This is the output I get from the 'git log' call:
>
> b4f1be9 (HEAD -> unimportant) conflicted
> Submodule file 0000000...0000000 (new submodule)
> a4ef223 clean
>
> Notice both merges are shown despite none of them changing the number of occurences of
> 2c042ac4213768e55791098110d2ef2ef845881a, which does not even exist in this repository,
> and also that the conflicted merge shows 'file' as "new submodule".

Thanks for the report, and the steps to reproduce.  Very helpful.

    b4f1be9 (HEAD -> unimportant) conflicted
    diff --git a/file b/file
    remerge CONFLICT (content): Merge conflict in file
    a4ef223 clean

into what you saw, namely

    b4f1be9 (HEAD -> unimportant) conflicted
    Submodule file 0000000...0000000 (new submodule)
    a4ef223 clean

We can recover the intended remerge-diff header with the following simple patch:

diff --git a/diff.c b/diff.c
index 974626a621..be23f66057 100644
--- a/diff.c
+++ b/diff.c
@@ -3429,14 +3429,16 @@ static void builtin_diff(const char *name_a,

        if (o->submodule_format == DIFF_SUBMODULE_LOG &&
            (!one->mode || S_ISGITLINK(one->mode)) &&
-           (!two->mode || S_ISGITLINK(two->mode))) {
+           (!two->mode || S_ISGITLINK(two->mode)) &&
+           (one->mode || two->mode)) {
                show_submodule_diff_summary(o, one->path ? one->path :
two->path,
                                &one->oid, &two->oid,
                                two->dirty_submodule);
                return;
        } else if (o->submodule_format == DIFF_SUBMODULE_INLINE_DIFF &&
                   (!one->mode || S_ISGITLINK(one->mode)) &&
-                  (!two->mode || S_ISGITLINK(two->mode))) {
+                  (!two->mode || S_ISGITLINK(two->mode)) &&
+                  (one->mode || two->mode)) {
                show_submodule_inline_diff(o, one->path ? one->path : two->path,
                                &one->oid, &two->oid,
                                two->dirty_submodule);

In other words, when we have information about something other than a
submodule, don't attempt to treat it as information about a submodule.

Now, whether the remerge-diff header should be shown is an interesting
one -- it's a carry on to the discussion in the thread at [1], and
isn't simple to answer.
[1] https://lore.kernel.org/git/CABPp-BHjU+wDXNnf-rsGy86GvOFWH6qVLPEfAA2JDfLFRU4WSA@mail.gmail.com/

As for the first bug, the showing of any 2-parent commits, yes that's
true.  But it's not limited to remerge-diff; you can change
--diff-merges=r to --diff-merges=c or --diff-merges=cc and see the
same thing.  I'm not sure if that means that my looking at the
do_diff_combined() logic when developing the do_remerge_diff()
function meant I copied a bug, or if I independently came up with it
on my own.  But, for now, I need some sleep.  Just thought I'd give
you an update on what I have found so far, though.