Re: BUG?: xdl_merge surprisingly does not recognize content conflict

[Jeff King <peff@peff.net>]

On Thu, Aug 15, 2019 at 03:03:03PM -0700, Elijah Newren wrote:

> 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:
> [...]
> 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?

Interesting case. If you look at the combined diff, you can see that the
two hunks are actually separated by a single blank line from the
original:

  $ git show 4a3ed2bec603
  [...]
  diff --cc builtin/checkout.c
  index 2e72a5e5a9,7cd01f62be..ffa776c6e1
  --- a/builtin/checkout.c
  +++ b/builtin/checkout.c
  @@@ -737,14 -738,13 +738,20 @@@ static int merge_working_tree(const str
                           */
                          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 */
    
                          /*

which is itself an interesting diff artifact. The original had a line at
the end, splitting the "if (!old_branch...)" conditional from the "Do
more real merge" comment.  The upper half of the hunk _didn't_ add a new
line between that old conditional and the new code. But the lower half
did, but diff reports it as adding the line at the end (which is equally
valid to adding the line at the top; who knows what the author actually
did!).

That tidbit aside, in general I'd think a single line would not be
enough to separate two hunks and consider them independent. At first I
thought that XDL_MERGE_ZEALOUS was to blame. If I do this:

diff --git a/ll-merge.c b/ll-merge.c
index 5b8d46aede..ea445dfb55 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -107,7 +107,6 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 	}
 
 	memset(&xmp, 0, sizeof(xmp));
-	xmp.level = XDL_MERGE_ZEALOUS;
 	xmp.favor = opts->variant;
 	xmp.xpp.flags = opts->xdl_opts;
 	if (git_xmerge_style >= 0)

and re-run the merge, I get a conflict. But it's not in those lines! The
diff of the conflicted state is:

diff --cc builtin/checkout.c
index 2e72a5e5a9,7cd01f62be..0000000000
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@@ -725,9 -725,10 +725,15 @@@ static int merge_working_tree(const str
  			 */
  			struct tree *result;
  			struct tree *work;
+ 			struct tree *old_tree;
  			struct merge_options o;
++<<<<<<< HEAD
  			struct strbuf sb = STRBUF_INIT;
  
++=======
++			struct strbuf sb = STRBUF_INIT;
++
++>>>>>>> 4a3ed2bec603^2
  			if (!opts->merge)
  				return 1;
  
@@@ -737,14 -738,13 +743,20 @@@
  			 */
  			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 */
  
  			/*

So it found another conflict (where the zealous resolution did the
_right_ thing!) but didn't do anything for the hunk in question.

I do wonder if the fact that the separating line a blank one is relevant
or not (i.e., is it tickling some heuristics in xdiff).

-Peff