BUG?: xdl_merge surprisingly does not recognize content conflict

[Elijah Newren <newren@gmail.com>]

It appears git.git had a case of a patch being resubmitted and both the
original (nd/checkout-m-doc-update) and new (nd/checkout-m) versions
getting applied, with the merge picking to include both versions of some
of the text rather than just one of the two.  I have a patch below to
delete the duplicate hunk, but more surprising to me was the fact that
re-running the merge in question did not show any conflicts despite the
two patches adding different text in the same location.

The "duplicate" commits:
  * commit a7256debd4b6 ("checkout.txt: note about losing staged changes
    with --merge", 2019-03-19), from nd/checkout-m-doc-update
  * commit 6eff409e8a76 ("checkout: prevent losing staged changes with
    --merge", 2019-03-22), from nd/checkout-m
(From reading the mailing list, the former was v1 and the latter was
meant as v2, but the latter was not marked as v2 and apparently both
were picked up.)

The problematic merge was commit 4a3ed2bec603 ("Merge branch
'nd/checkout-m'", 2019-04-25), but redoing that merge produces no merge
conflicts.  This can be seen at the individual file level with the
following:

  $ git show 4a3ed2bec603^1:builtin/checkout.c >ours
  $ git show 4a3ed2bec603^2:builtin/checkout.c >theirs
  $ git show $(git merge-base 4a3ed2bec603^1 4a3ed2bec603^2):builtin/checkout.c >base

At this point, we can note that ours and theirs both added similar code
at the same place; looking at ours:

  $ git diff --no-index base ours | grep -B 4 -A 7 repo_index_has_changes
  @@ -736,6 +738,13 @@ static int merge_working_tree(const struct checkout_opts *opts,
   			if (!old_branch_info->commit)
   				return 1;

  +			if (repo_index_has_changes(the_repository,
  +						   get_commit_tree(old_branch_info->commit),
  +						   &sb))
  +				warning(_("staged changes in the following files may be lost: %s"),
  +					sb.buf);
  +			strbuf_release(&sb);
  +
   			/* Do more real merge */

Looking at theirs:

  $ git diff --no-index base theirs | grep -B 4 -A 7 repo_index_has_changes
   			if (!old_branch_info->commit)
   				return 1;
  +			old_tree = get_commit_tree(old_branch_info->commit);
  +
  +			if (repo_index_has_changes(the_repository, old_tree, &sb))
  +				die(_("cannot continue with staged changes in "
  +				      "the following files:\n%s"), sb.buf);
  +			strbuf_release(&sb);

   			/* Do more real merge */

  @@ -772,7 +781,7 @@ static int merge_working_tree(const struct checkout_opts *opts,

Now, a manual merge of these files gives no conflicts, which surprises me:

  $ git merge-file ours base theirs; echo $?
  0

and this merge includes BOTH additions:

  $ git diff --no-index base ours | grep -B 4 -A 7 repo_index_has_changes
   			if (!old_branch_info->commit)
   				return 1;
  +			old_tree = get_commit_tree(old_branch_info->commit);
  +
  +			if (repo_index_has_changes(the_repository, old_tree, &sb))
  +				die(_("cannot continue with staged changes in "
  +				      "the following files:\n%s"), sb.buf);
  +			strbuf_release(&sb);
  +
  +			if (repo_index_has_changes(the_repository,
  +						   get_commit_tree(old_branch_info->commit),
  +						   &sb))
  +				warning(_("staged changes in the following files may be lost: %s"),
  +					sb.buf);
  +			strbuf_release(&sb);

   			/* Do more real merge */

I'm not that familiar with the xdl_merge stuff, but this seemed buggy
to me.  Or is there something about the content merge that I'm just not
understanding and this merge is actually correct?

Elijah

-- 8< --
Subject: checkout: remove duplicate code

Both commit a7256debd4b6 ("checkout.txt: note about losing staged
changes with --merge", 2019-03-19) from nd/checkout-m-doc-update and
commit 6eff409e8a76 ("checkout: prevent losing staged changes with
--merge", 2019-03-22) from nd/checkout-m were included in git.git
despite the fact that the latter was meant to be v2 of the former.
The merge of these two topics resulted in a redundant chunk of code;
remove it.

Signed-off-by: Elijah Newren <newren@gmail.com>


---
 builtin/checkout.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 6123f732a2..dc61afa066 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -730,13 +730,6 @@ static int merge_working_tree(const struct checkout_opts *opts,
 				      "the following files:\n%s"), sb.buf);
 			strbuf_release(&sb);
 
-			if (repo_index_has_changes(the_repository,
-						   get_commit_tree(old_branch_info->commit),
-						   &sb))
-				warning(_("staged changes in the following files may be lost: %s"),
-					sb.buf);
-			strbuf_release(&sb);
-
 			/* Do more real merge */
 
 			/*
-- 
2.23.0.rc2.32.g2123e9e4e4