Hi Elijah, Le 2022-08-24 à 03:36, Elijah Newren a écrit : > 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. Thanks for digging into this. >From what I understand in the case of a remerge-diff, both modes are all-zero, and this is not expected by the submodule diff code. Were you planning to submit a proper patch ? I could get to it eventually, but not before mid/end of September... > > 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/ Thanks for a link to a very interesting discussion! However I'm not exactly sure what you meant here. Are you saying that: - we use '--remerge-diff', so we are interested in seeing (re)merge diffs - we do not use '-p', so we are not interested in seeing diffs - to reconcile both of the above, the code shows only the remerge diff header ? Because if I do add '-p', (in my reproducer) I still do not get any diff content. But if I remove '--find-object', then I do... > 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. > Yes, I had noticed that I had the same behaviour with --cc or -c, I forgot to mention it. With 'separate' or 'first-parent', though, no commits are shown (as expected). To be clear, this behaviour is independent of the use of '--submodule=short|log|diff'. Thanks for your answer, Philippe.