{"thread":{"id":"51651","subject":"BUG?: xdl_merge surprisingly does not recognize content conflict","startedAt":"2019-08-15T22:03:13Z","lastAt":"2019-08-16T19:26:20Z","messageCount":5,"participants":["Elijah Newren","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"380521","messageId":"20190815220303.17209-1-newren@gmail.com","threadId":"51651","inReplyTo":null,"subject":"BUG?: xdl_merge surprisingly does not recognize content conflict","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-08-15T22:03:03Z","receivedAt":"2019-08-15T22:03:13Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"It appears git.git had a case of a patch being resubmitted and both the\noriginal (nd/checkout-m-doc-update) and new (nd/checkout-m) versions\ngetting applied, with the merge picking to include both versions of some\nof the text rather than just one of the two.  I have a patch below to\ndelete the duplicate hunk, but more surprising to me was the fact that\nre-running the merge in question did not show any conflicts despite the\ntwo patches adding different text in the same location.\n\nThe \"duplicate\" commits:\n  * commit a7256debd4b6 (\"checkout.txt: note about losing staged changes\n    with --merge\", 2019-03-19), from nd/checkout-m-doc-update\n  * commit 6eff409e8a76 (\"checkout: prevent losing staged changes with\n    --merge\", 2019-03-22), from nd/checkout-m\n(From reading the mailing list, the former was v1 and the latter was\nmeant as v2, but the latter was not marked as v2 and apparently both\nwere picked up.)\n\nThe problematic merge was commit 4a3ed2bec603 (\"Merge branch\n'nd/checkout-m'\", 2019-04-25), but redoing that merge produces no merge\nconflicts.  This can be seen at the individual file level with the\nfollowing:\n\n  $ git show 4a3ed2bec603^1:builtin/checkout.c >ours\n  $ git show 4a3ed2bec603^2:builtin/checkout.c >theirs\n  $ git show $(git merge-base 4a3ed2bec603^1 4a3ed2bec603^2):builtin/checkout.c >base\n\nAt this point, we can note that ours and theirs both added similar code\nat the same place; looking at ours:\n\n  $ git diff --no-index base ours | grep -B 4 -A 7 repo_index_has_changes\n  @@ -736,6 +738,13 @@ static int merge_working_tree(const struct checkout_opts *opts,\n   \t\t\tif (!old_branch_info->commit)\n   \t\t\t\treturn 1;\n\n  +\t\t\tif (repo_index_has_changes(the_repository,\n  +\t\t\t\t\t\t   get_commit_tree(old_branch_info->commit),\n  +\t\t\t\t\t\t   &sb))\n  +\t\t\t\twarning(_(\"staged changes in the following files may be lost: %s\"),\n  +\t\t\t\t\tsb.buf);\n  +\t\t\tstrbuf_release(&sb);\n  +\n   \t\t\t/* Do more real merge */\n\nLooking at theirs:\n\n  $ git diff --no-index base theirs | grep -B 4 -A 7 repo_index_has_changes\n   \t\t\tif (!old_branch_info->commit)\n   \t\t\t\treturn 1;\n  +\t\t\told_tree = get_commit_tree(old_branch_info->commit);\n  +\n  +\t\t\tif (repo_index_has_changes(the_repository, old_tree, &sb))\n  +\t\t\t\tdie(_(\"cannot continue with staged changes in \"\n  +\t\t\t\t      \"the following files:\\n%s\"), sb.buf);\n  +\t\t\tstrbuf_release(&sb);\n\n   \t\t\t/* Do more real merge */\n\n  @@ -772,7 +781,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n\nNow, a manual merge of these files gives no conflicts, which surprises me:\n\n  $ git merge-file ours base theirs; echo $?\n  0\n\nand this merge includes BOTH additions:\n\n  $ git diff --no-index base ours | grep -B 4 -A 7 repo_index_has_changes\n   \t\t\tif (!old_branch_info->commit)\n   \t\t\t\treturn 1;\n  +\t\t\told_tree = get_commit_tree(old_branch_info->commit);\n  +\n  +\t\t\tif (repo_index_has_changes(the_repository, old_tree, &sb))\n  +\t\t\t\tdie(_(\"cannot continue with staged changes in \"\n  +\t\t\t\t      \"the following files:\\n%s\"), sb.buf);\n  +\t\t\tstrbuf_release(&sb);\n  +\n  +\t\t\tif (repo_index_has_changes(the_repository,\n  +\t\t\t\t\t\t   get_commit_tree(old_branch_info->commit),\n  +\t\t\t\t\t\t   &sb))\n  +\t\t\t\twarning(_(\"staged changes in the following files may be lost: %s\"),\n  +\t\t\t\t\tsb.buf);\n  +\t\t\tstrbuf_release(&sb);\n\n   \t\t\t/* Do more real merge */\n\nI'm not that familiar with the xdl_merge stuff, but this seemed buggy\nto me.  Or is there something about the content merge that I'm just not\nunderstanding and this merge is actually correct?\n\nElijah\n\n-- 8< --\nSubject: checkout: remove duplicate code\n\nBoth commit a7256debd4b6 (\"checkout.txt: note about losing staged\nchanges with --merge\", 2019-03-19) from nd/checkout-m-doc-update and\ncommit 6eff409e8a76 (\"checkout: prevent losing staged changes with\n--merge\", 2019-03-22) from nd/checkout-m were included in git.git\ndespite the fact that the latter was meant to be v2 of the former.\nThe merge of these two topics resulted in a redundant chunk of code;\nremove it.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n\n\n---\n builtin/checkout.c | 7 -------\n 1 file changed, 7 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 6123f732a2..dc61afa066 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -730,13 +730,6 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t\t      \"the following files:\\n%s\"), sb.buf);\n \t\t\tstrbuf_release(&sb);\n \n-\t\t\tif (repo_index_has_changes(the_repository,\n-\t\t\t\t\t\t   get_commit_tree(old_branch_info->commit),\n-\t\t\t\t\t\t   &sb))\n-\t\t\t\twarning(_(\"staged changes in the following files may be lost: %s\"),\n-\t\t\t\t\tsb.buf);\n-\t\t\tstrbuf_release(&sb);\n-\n \t\t\t/* Do more real merge */\n \n \t\t\t/*\n-- \n2.23.0.rc2.32.g2123e9e4e4\n\n"},{"id":"380522","messageId":"CABPp-BHupqJaJzK9yXxvTEPkuaoq_yW0ZJ7z0nZXODhaRdrYyg@mail.gmail.com","threadId":"51651","inReplyTo":"20190815220303.17209-1-newren@gmail.com","subject":"Re: BUG?: xdl_merge surprisingly does not recognize content conflict","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-08-15T22:06:51Z","receivedAt":"2019-08-15T22:07:05Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Aug 15, 2019 at 3:03 PM Elijah Newren <newren@gmail.com> wrote:\n>\n> It appears git.git had a case of a patch being resubmitted and both the\n> original (nd/checkout-m-doc-update) and new (nd/checkout-m) versions\n> getting applied, with the merge picking to include both versions of some\n> of the text rather than just one of the two.  I have a patch below to\n> delete the duplicate hunk, but more surprising to me was the fact that\n> re-running the merge in question did not show any conflicts despite the\n> two patches adding different text in the same location.\n\nSorry, I should specify that I've tested that this behavior goes back\nto at least git-1.6.0, so it certainly isn't a new regression and\ndoesn't need to hold up git-2.23.0 by any means.\n"},{"id":"380540","messageId":"xmqqh86h12au.fsf@gitster-ct.c.googlers.com","threadId":"51651","inReplyTo":"20190815220303.17209-1-newren@gmail.com","subject":"Re: BUG?: xdl_merge surprisingly does not recognize content conflict","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-16T16:51:37Z","receivedAt":"2019-08-16T16:51:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> Now, a manual merge of these files gives no conflicts, which surprises me:\n>\n>   $ git merge-file ours base theirs; echo $?\n>   0\n\nIndeed that is surprising.\n\n> -- 8< --\n> Subject: checkout: remove duplicate code\n>\n> Both commit a7256debd4b6 (\"checkout.txt: note about losing staged\n> changes with --merge\", 2019-03-19) from nd/checkout-m-doc-update and\n> commit 6eff409e8a76 (\"checkout: prevent losing staged changes with\n> --merge\", 2019-03-22) from nd/checkout-m were included in git.git\n> despite the fact that the latter was meant to be v2 of the former.\n> The merge of these two topics resulted in a redundant chunk of code;\n> remove it.\n>\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n\nThe latter half that you remove cannot be reached, because the first\nhalf would have already died after checking the same condition, so\nwe were doing the same repo-index-has-changes check twice when there\nis no change; in other words, we were lucky that this accidental dup\ndid not cause any real damange.\n\nThanks for spotting.  Will apply.\n"},{"id":"380543","messageId":"20190816184051.GB13894@sigill.intra.peff.net","threadId":"51651","inReplyTo":"20190815220303.17209-1-newren@gmail.com","subject":"Re: BUG?: xdl_merge surprisingly does not recognize content conflict","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-16T18:40:51Z","receivedAt":"2019-08-16T18:40:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 15, 2019 at 03:03:03PM -0700, Elijah Newren wrote:\n\n> The problematic merge was commit 4a3ed2bec603 (\"Merge branch\n> 'nd/checkout-m'\", 2019-04-25), but redoing that merge produces no merge\n> conflicts.  This can be seen at the individual file level with the\n> following:\n> [...]\n> I'm not that familiar with the xdl_merge stuff, but this seemed buggy\n> to me.  Or is there something about the content merge that I'm just not\n> understanding and this merge is actually correct?\n\nInteresting case. If you look at the combined diff, you can see that the\ntwo hunks are actually separated by a single blank line from the\noriginal:\n\n  $ git show 4a3ed2bec603\n  [...]\n  diff --cc builtin/checkout.c\n  index 2e72a5e5a9,7cd01f62be..ffa776c6e1\n  --- a/builtin/checkout.c\n  +++ b/builtin/checkout.c\n  @@@ -737,14 -738,13 +738,20 @@@ static int merge_working_tree(const str\n                           */\n                          if (!old_branch_info->commit)\n                                  return 1;\n  +                       old_tree = get_commit_tree(old_branch_info->commit);\n  + \n  +                       if (repo_index_has_changes(the_repository, old_tree, &sb))\n  +                               die(_(\"cannot continue with staged changes in \"\n  +                                     \"the following files:\\n%s\"), sb.buf);\n  +                       strbuf_release(&sb);\n    \n   +                      if (repo_index_has_changes(the_repository,\n   +                                                 get_commit_tree(old_branch_info->commit),\n   +                                                 &sb))\n   +                              warning(_(\"staged changes in the following files may be lost: %s\"),\n   +                                      sb.buf);\n   +                      strbuf_release(&sb);\n   +\n                          /* Do more real merge */\n    \n                          /*\n\nwhich is itself an interesting diff artifact. The original had a line at\nthe end, splitting the \"if (!old_branch...)\" conditional from the \"Do\nmore real merge\" comment.  The upper half of the hunk _didn't_ add a new\nline between that old conditional and the new code. But the lower half\ndid, but diff reports it as adding the line at the end (which is equally\nvalid to adding the line at the top; who knows what the author actually\ndid!).\n\nThat tidbit aside, in general I'd think a single line would not be\nenough to separate two hunks and consider them independent. At first I\nthought that XDL_MERGE_ZEALOUS was to blame. If I do this:\n\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 5b8d46aede..ea445dfb55 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -107,7 +107,6 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n \t}\n \n \tmemset(&xmp, 0, sizeof(xmp));\n-\txmp.level = XDL_MERGE_ZEALOUS;\n \txmp.favor = opts->variant;\n \txmp.xpp.flags = opts->xdl_opts;\n \tif (git_xmerge_style >= 0)\n\nand re-run the merge, I get a conflict. But it's not in those lines! The\ndiff of the conflicted state is:\n\ndiff --cc builtin/checkout.c\nindex 2e72a5e5a9,7cd01f62be..0000000000\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@@ -725,9 -725,10 +725,15 @@@ static int merge_working_tree(const str\n  \t\t\t */\n  \t\t\tstruct tree *result;\n  \t\t\tstruct tree *work;\n+ \t\t\tstruct tree *old_tree;\n  \t\t\tstruct merge_options o;\n++<<<<<<< HEAD\n  \t\t\tstruct strbuf sb = STRBUF_INIT;\n  \n++=======\n++\t\t\tstruct strbuf sb = STRBUF_INIT;\n++\n++>>>>>>> 4a3ed2bec603^2\n  \t\t\tif (!opts->merge)\n  \t\t\t\treturn 1;\n  \n@@@ -737,14 -738,13 +743,20 @@@\n  \t\t\t */\n  \t\t\tif (!old_branch_info->commit)\n  \t\t\t\treturn 1;\n+ \t\t\told_tree = get_commit_tree(old_branch_info->commit);\n+ \n+ \t\t\tif (repo_index_has_changes(the_repository, old_tree, &sb))\n+ \t\t\t\tdie(_(\"cannot continue with staged changes in \"\n+ \t\t\t\t      \"the following files:\\n%s\"), sb.buf);\n+ \t\t\tstrbuf_release(&sb);\n  \n +\t\t\tif (repo_index_has_changes(the_repository,\n +\t\t\t\t\t\t   get_commit_tree(old_branch_info->commit),\n +\t\t\t\t\t\t   &sb))\n +\t\t\t\twarning(_(\"staged changes in the following files may be lost: %s\"),\n +\t\t\t\t\tsb.buf);\n +\t\t\tstrbuf_release(&sb);\n +\n  \t\t\t/* Do more real merge */\n  \n  \t\t\t/*\n\nSo it found another conflict (where the zealous resolution did the\n_right_ thing!) but didn't do anything for the hunk in question.\n\nI do wonder if the fact that the separating line a blank one is relevant\nor not (i.e., is it tickling some heuristics in xdiff).\n\n-Peff\n"},{"id":"380546","messageId":"CABPp-BF_PdzOvsgUQUtQRgiQe1qQ_O9Pn2rbpwJ095e7VA2xKA@mail.gmail.com","threadId":"51651","inReplyTo":"xmqqh86h12au.fsf@gitster-ct.c.googlers.com","subject":"Re: BUG?: xdl_merge surprisingly does not recognize content conflict","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-08-16T19:26:06Z","receivedAt":"2019-08-16T19:26:20Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Aug 16, 2019 at 9:51 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Elijah Newren <newren@gmail.com> writes:\n>\n> > Now, a manual merge of these files gives no conflicts, which surprises me:\n> >\n> >   $ git merge-file ours base theirs; echo $?\n> >   0\n>\n> Indeed that is surprising.\n>\n> > -- 8< --\n> > Subject: checkout: remove duplicate code\n> >\n> > Both commit a7256debd4b6 (\"checkout.txt: note about losing staged\n> > changes with --merge\", 2019-03-19) from nd/checkout-m-doc-update and\n> > commit 6eff409e8a76 (\"checkout: prevent losing staged changes with\n> > --merge\", 2019-03-22) from nd/checkout-m were included in git.git\n> > despite the fact that the latter was meant to be v2 of the former.\n> > The merge of these two topics resulted in a redundant chunk of code;\n> > remove it.\n> >\n> > Signed-off-by: Elijah Newren <newren@gmail.com>\n>\n> The latter half that you remove cannot be reached, because the first\n> half would have already died after checking the same condition, so\n> we were doing the same repo-index-has-changes check twice when there\n> is no change; in other words, we were lucky that this accidental dup\n> did not cause any real damange.\n\nYes, but I am wondering if there are other cases we just don't know\nabout yet and for which we did not get lucky.\n\nI dug a little further and found that even the first version of `git\nmerge-file` that Dscho added to git in 2006 also exhibited the same\nbehavior.  Since he was basically making a minimal replacement for RCS\nmerge, I tried out /usr/bin/merge from the rcs package and found it\nhas the same behavior.  I then downloaded the oldest tarball on the\nGNU ftp site I could find for rcs (5.7 from June 1995) and found that\nits' merge command had the same behavior.\n\nI'm not sure if that alarms me (not only have all versions of git\npotentially been mis-merging things, but all versions of rcs, cvs, and\nprobably any other version control system out there in current use),\nor comforts me (\"hey, it's been around for at least 24 years so it\nmust only mis-merge things in both rare and innocuous ways or else\nsomeone would have noticed it decades ago\")\n\nI'm leaning towards \"It can't be that bad; I'll look at it again later\nif someone else who knows xdiff better hasn't already solved it\"\n"}]}