git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Elijah Newren <newren@gmail.com>
To: git@vger.kernel.org
Cc: Elijah Newren <newren@gmail.com>
Subject: BUG?: xdl_merge surprisingly does not recognize content conflict
Date: Thu, 15 Aug 2019 15:03:03 -0700
Message-ID: <20190815220303.17209-1-newren@gmail.com> (raw)

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


             reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-15 22:03 Elijah Newren [this message]
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

Reply instructions:

You may reply publically 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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=20190815220303.17209-1-newren@gmail.com \
    --to=newren@gmail.com \
    --cc=git@vger.kernel.org \
    /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@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

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