Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Jeff King <peff@peff.net>
To: Elijah Newren <newren@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: BUG?: xdl_merge surprisingly does not recognize content conflict
Date: Fri, 16 Aug 2019 14:40:51 -0400
Message-ID: <20190816184051.GB13894@sigill.intra.peff.net> (raw)
In-Reply-To: <20190815220303.17209-1-newren@gmail.com>

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

      parent reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-15 22:03 Elijah Newren
2019-08-15 22:06 ` Elijah Newren
2019-08-16 16:51 ` Junio C Hamano
2019-08-16 19:26   ` Elijah Newren
2019-08-16 18:40 ` Jeff King [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190816184051.GB13894@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=newren@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git