Hi Philippe,
On Mon, Aug 22, 2022 at 4:58 PM Philippe Blain
<levraiphilippeblain@xxxxxxxxx> wrote:
>
> 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.
After some digging, it appears the remerge-diff headers are
misinterpreted by the submodule code. They transform what would have
been the following output:
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@xxxxxxxxxxxxxx/
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.
