git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Miklos Vajna <vmiklos@frugalware.org>
Cc: Paolo Bonzini <bonzini@gnu.org>, Jeff King <peff@peff.net>,
	git@vger.kernel.org
Subject: [PATCH 1/2] merge: fix numerus bugs around "trivial merge" area
Date: Sat, 23 Aug 2008 12:56:57 -0700	[thread overview]
Message-ID: <7vfxovy06u.fsf_-_@gitster.siamese.dyndns.org> (raw)
In-Reply-To: <7vk5e7y0a0.fsf@gitster.siamese.dyndns.org> (Junio C. Hamano's message of "Sat, 23 Aug 2008 12:55:03 -0700")

The "trivial merge" codepath wanted to optimize itself by making an
internal call to the read-tree machinery, but it did not read the index
before doing so, and the codepath was never exercised.  Incidentally, this
failure to read the index upfront meant that the safety to refuse doing
anything when the index is unmerged did not kick in, either.

These two problem are fixed by using read_cache_unmerged() that does read
the index before checking if it is unmerged at the beginning of
cmd_merge().

The primary logic of the merge, however, had an assumption that the
process never read the index in-core, and write_cache_as_tree() call it
makes from write_tree_trivial() will always read from the on-disk index
the strategies created and write it out as a tree.  This assumption is now
broken by the above fix.  It now calls discard_cache() before calling
write_tree_trivial() when it wants to write the on-disk index as a tree to
fix this issue.

When multiple strategies are tried, their results are evaluated by reading
the resulting index and inspecting it.  The codepath needs to make a call
to read_cache() for each successful strategy, and for that to work, they
need to discard_cache() the one from the previous round.

Also the "trivial merge" forgot that the current commit is one of the
parents of the resulting commit.

This still has breakage in the way it writes out the resulting tree out of
the trivial merge codepath, which is a topic of the next patch.  One test
in t7605-merge-resolve.sh exposes this breakage.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-merge.c            |   16 +++++++++-------
 t/t3030-merge-recursive.sh |   11 +++++++++++
 t/t7600-merge.sh           |    9 +++++++++
 t/t7605-merge-resolve.sh   |    6 ++++--
 4 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index a201c66..b280444 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -564,8 +564,6 @@ static int checkout_fast_forward(unsigned char *head, unsigned char *remote)
 	struct dir_struct dir;
 	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
 
-	if (read_cache_unmerged())
-		die("you need to resolve your current index first");
 	refresh_cache(REFRESH_QUIET);
 
 	fd = hold_locked_index(lock_file, 1);
@@ -651,13 +649,15 @@ static void add_strategies(const char *string, unsigned attr)
 static int merge_trivial(void)
 {
 	unsigned char result_tree[20], result_commit[20];
-	struct commit_list parent;
+	struct commit_list *parent = xmalloc(sizeof(struct commit_list *));
 
 	write_tree_trivial(result_tree);
 	printf("Wonderful.\n");
-	parent.item = remoteheads->item;
-	parent.next = NULL;
-	commit_tree(merge_msg.buf, result_tree, &parent, result_commit);
+	parent->item = lookup_commit(head);
+	parent->next = xmalloc(sizeof(struct commit_list *));
+	parent->next->item = remoteheads->item;
+	parent->next->next = NULL;
+	commit_tree(merge_msg.buf, result_tree, parent, result_commit);
 	finish(result_commit, "In-index merge");
 	drop_save();
 	return 0;
@@ -743,6 +743,7 @@ static int evaluate_result(void)
 	int cnt = 0;
 	struct rev_info rev;
 
+	discard_cache();
 	if (read_cache() < 0)
 		die("failed to read the cache");
 
@@ -776,7 +777,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	struct commit_list **remotes = &remoteheads;
 
 	setup_work_tree();
-	if (unmerged_cache())
+	if (read_cache_unmerged())
 		die("You are in the middle of a conflicted merge.");
 
 	/*
@@ -1073,6 +1074,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		}
 
 		/* Automerge succeeded. */
+		discard_cache();
 		write_tree_trivial(result_tree);
 		automerge_was_ok = 1;
 		break;
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index aff3603..f288015 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -269,6 +269,17 @@ test_expect_success 'merge-recursive result' '
 
 '
 
+test_expect_success 'fail if the index has unresolved entries' '
+
+	rm -fr [abcd] &&
+	git checkout -f "$c1" &&
+
+	test_must_fail git merge "$c5" &&
+	test_must_fail git merge "$c5" 2> out &&
+	grep "You are in the middle of a conflicted merge" out
+
+'
+
 test_expect_success 'merge-recursive remove conflict' '
 
 	rm -fr [abcd] &&
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index fee8fb7..dbc90bc 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -498,4 +498,13 @@ test_expect_success 'merge fast-forward in a dirty tree' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'in-index merge' '
+	git reset --hard c0 &&
+	git merge --no-ff -s resolve c1 > out &&
+	grep "Wonderful." out &&
+	verify_parents $c0 $c1
+'
+
+test_debug 'gitk --all'
+
 test_done
diff --git a/t/t7605-merge-resolve.sh b/t/t7605-merge-resolve.sh
index ee21a10..5c53608 100755
--- a/t/t7605-merge-resolve.sh
+++ b/t/t7605-merge-resolve.sh
@@ -27,7 +27,7 @@ test_expect_success 'setup' '
 	git tag c3
 '
 
-test_expect_success 'merge c1 to c2' '
+test_expect_failure 'merge c1 to c2' '
 	git reset --hard c1 &&
 	git merge -s resolve c2 &&
 	test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" &&
@@ -36,7 +36,9 @@ test_expect_success 'merge c1 to c2' '
 	git diff --exit-code &&
 	test -f c0.c &&
 	test -f c1.c &&
-	test -f c2.c
+	test -f c2.c &&
+	test 3 = $(git ls-tree -r HEAD | wc -l) &&
+	test 3 = $(git ls-files | wc -l)
 '
 
 test_expect_success 'merge c2 to c3 (fails)' '
-- 
1.6.0.51.g078ae

  reply	other threads:[~2008-08-23 19:58 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-08-22  6:36 [bug] git `next' does not do trivial merges Paolo Bonzini
2008-08-22 19:31 ` Jeff King
2008-08-23  6:08   ` Miklos Vajna
2008-08-23  8:14     ` [PATCH] Fix in-index merge Miklos Vajna
2008-08-23  8:17       ` [PATCH] builtin-merge: fail properly when we are in the middle of a conflicted merge Miklos Vajna
2008-08-23  9:01         ` Junio C Hamano
2008-08-23 10:57           ` Miklos Vajna
2008-08-23 19:55             ` Junio C Hamano
2008-08-23 19:56               ` Junio C Hamano [this message]
2008-08-24  1:58                 ` [PATCH 1/2] merge: fix numerus bugs around "trivial merge" area Junio C Hamano
2008-08-28 13:43                   ` [PATCH] builtin-merge: avoid run_command_v_opt() for recursive and subtree Miklos Vajna
2008-08-23 19:57               ` [PATCH 2/2] unpack_trees(): protect the handcrafted in-core index from read_cache() Junio C Hamano
2008-08-23  9:50         ` [PATCH] builtin-merge: fail properly when we are in the middle of a conflicted merge Junio C Hamano
2008-08-23  8:50       ` [PATCH] Fix in-index merge Junio C Hamano
2008-08-23 10:41         ` Miklos Vajna
2008-08-23  9:19       ` Paolo Bonzini
2008-08-23  9:55         ` Junio C Hamano
2008-08-23 10:00           ` Junio C Hamano
2008-08-23 10:41             ` [RFH] two and half potential fixlets to the in-core index handling Junio C Hamano
2008-08-23 18:13               ` Linus Torvalds
2008-08-23 19:14                 ` Junio C Hamano
2008-08-23 19:21                   ` Junio C Hamano

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

  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=7vfxovy06u.fsf_-_@gitster.siamese.dyndns.org \
    --to=gitster@pobox.com \
    --cc=bonzini@gnu.org \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --cc=vmiklos@frugalware.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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).