[bug] git `next' does not do trivial merges

[bug] git `next' does not do trivial merges

[Paolo Bonzini]

I had already posted this bug report yesterday but it was hidden in a 
cover letter at 
http://permalink.gmane.org/gmane.comp.version-control.git/93143 -- so 
I'll copy the relevant info here:

> 	The included testcases demonstrate that trivial merges are
> 	currently broken.  The failing test is:
> 
> 	  git init
> 	  echo a > a
> 	  git add a
> 	  git commit -ma 
> 	  git checkout -b branch
> 	  echo b > b
> 	  git add b
> 	  git commit -mb
> 	  git checkout master
> 	  git merge --no-ff -s resolve branch
> 
> 	Running this in 1.5.5 shows: 
> 
> 	  Trying really trivial in-index merge...
> 	  Wonderful.
> 	  In-index merge
> 
> 	while `next' gives
> 
> 	  Trying really trivial in-index merge...
> 	  error: Untracked working tree file 'a' would be overwritten by merge.
> 	  Nope.
> 	  Trying simple merge.
> 	  Merge made by resolve.

Paolo

Re: [bug] git `next' does not do trivial merges

[Jeff King]

On Fri, Aug 22, 2008 at 08:36:39AM +0200, Paolo Bonzini wrote:

> I had already posted this bug report yesterday but it was hidden in a  
> cover letter at  
> http://permalink.gmane.org/gmane.comp.version-control.git/93143 -- so  
> I'll copy the relevant info here:

Sadly, this bisects to 1c7b76b (Build in merge).

-Peff

Re: [bug] git `next' does not do trivial merges

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 631 bytes --]

On Fri, Aug 22, 2008 at 03:31:17PM -0400, Jeff King <peff@peff.net> wrote:
> On Fri, Aug 22, 2008 at 08:36:39AM +0200, Paolo Bonzini wrote:
> 
> > I had already posted this bug report yesterday but it was hidden in a  
> > cover letter at  
> > http://permalink.gmane.org/gmane.comp.version-control.git/93143 -- so  
> > I'll copy the relevant info here:
> 
> Sadly, this bisects to 1c7b76b (Build in merge).

I guessed the bug is in builtin-merge.c::read_tree_trivial(), but I
don't see how it is different to builtin-read-tree.c::cmd_read_tree(),
at least the unpack_trees_options struct is the same.

I'm on it..

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[PATCH] Fix in-index merge.

[Miklos Vajna]

There were 3 issues:

1) We need read_cache() before using unpack_trees().

2) commit_tree() frees the parents, so we need to malloc them.

3) The current HEAD was missing from the parent list.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Fri, Aug 22, 2008 at 08:36:39AM +0200, Paolo Bonzini <bonzini@gnu.org> wrote:
> I had already posted this bug report yesterday but it was hidden in a
> cover
> letter at
> http://permalink.gmane.org/gmane.comp.version-control.git/93143

This should fix the issue.

 builtin-merge.c          |   11 +++++++----
 t/t7607-merge-inindex.sh |   29 +++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 4 deletions(-)
 create mode 100755 t/t7607-merge-inindex.sh

diff --git a/builtin-merge.c b/builtin-merge.c
index a201c66..dffe4b8 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -469,6 +469,7 @@ static int read_tree_trivial(unsigned char *common, unsigned char *head,
 	struct tree_desc t[MAX_UNPACK_TREES];
 	struct unpack_trees_options opts;
 
+	read_cache();
 	memset(&opts, 0, sizeof(opts));
 	opts.head_idx = 2;
 	opts.src_index = &the_index;
@@ -651,13 +652,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;
diff --git a/t/t7607-merge-inindex.sh b/t/t7607-merge-inindex.sh
new file mode 100755
index 0000000..98b778f
--- /dev/null
+++ b/t/t7607-merge-inindex.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+test_description='git-merge
+
+Testing in-index merge.'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo a > a &&
+	git add a &&
+	git commit -m a &&
+	git tag a &&
+	git checkout -b branch
+	echo b > b &&
+	git add b &&
+	git commit -m b &&
+	git tag b
+'
+
+test_expect_success 'in-index merge' '
+	git checkout master &&
+	git merge --no-ff -s resolve branch > out &&
+	grep Wonderful. out &&
+	test "$(git rev-parse a)" = "$(git rev-parse HEAD^1)" &&
+	test "$(git rev-parse b)" = "$(git rev-parse HEAD^2)"
+'
+
+test_done
-- 
1.6.0.rc3.17.gc14c8.dirty

[PATCH] builtin-merge: fail properly when we are in the middle of a conflicted merge

[Miklos Vajna]

Using unmerged_cache() without reading the cache first never will return
anything. However, if we read the cache early then we have to discard it
when we want to read it again from the disk.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

The code part is pretty much from you, I wanted to add your signoff, but
then realized that you'll do it anyway. ;-)

 builtin-merge.c        |    6 +++---
 t/t7608-merge-dirty.sh |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 3 deletions(-)
 create mode 100755 t/t7608-merge-dirty.sh

diff --git a/builtin-merge.c b/builtin-merge.c
index dffe4b8..71bd13b 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -565,8 +565,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);
@@ -746,6 +744,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");
 
@@ -779,7 +778,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.");
 
 	/*
@@ -1076,6 +1075,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/t7608-merge-dirty.sh b/t/t7608-merge-dirty.sh
new file mode 100755
index 0000000..fb567f6
--- /dev/null
+++ b/t/t7608-merge-dirty.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='git-merge
+
+Merge should fail if the index has unresolved entries.'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo a > a &&
+	git add a &&
+	git commit -m a &&
+	git tag a &&
+	git checkout -b branch1
+	echo b > a &&
+	git add a &&
+	git commit -m b &&
+	git tag b
+	git checkout -b branch2 HEAD~1
+	echo c > a &&
+	git add a &&
+	git commit -m c &&
+	git tag c
+'
+
+test_expect_success 'in-index merge' '
+	git checkout branch1 &&
+	test_must_fail git merge branch2 &&
+	test_must_fail git merge branch2 2> out &&
+	grep "You are in the middle of a conflicted merge" out
+'
+
+test_done
-- 
1.6.0.rc3.17.gc14c8.dirty

Re: [PATCH] builtin-merge: fail properly when we are in the middle of a conflicted merge

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> Using unmerged_cache() without reading the cache first never will return
> anything. However, if we read the cache early then we have to discard it
> when we want to read it again from the disk.

With this, I do not think you would need to keep the read_cache() you
added to the beginning of the read_tree_trivial(), for the same reason you
are removing read_cache_unmerged() from the beginning of
checkout_fast_forward() in this patch.

[PATCH] builtin-merge: fail properly when we are in the middle of a conflicted merge

[Miklos Vajna]

Using unmerged_cache() without reading the cache first never will return
anything. However, if we read the cache early then we have to discard it
when we want to read it again from the disk.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Sat, Aug 23, 2008 at 02:01:43AM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> With this, I do not think you would need to keep the read_cache() you
> added to the beginning of the read_tree_trivial(), for the same reason
> you
> are removing read_cache_unmerged() from the beginning of
> checkout_fast_forward() in this patch.

Right, I removed it as well. Also fixed the testcase (no more new file).

 builtin-merge.c            |    7 +++----
 t/t3030-merge-recursive.sh |   11 +++++++++++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index dffe4b8..b280444 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -469,7 +469,6 @@ static int read_tree_trivial(unsigned char *common, unsigned char *head,
 	struct tree_desc t[MAX_UNPACK_TREES];
 	struct unpack_trees_options opts;
 
-	read_cache();
 	memset(&opts, 0, sizeof(opts));
 	opts.head_idx = 2;
 	opts.src_index = &the_index;
@@ -565,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);
@@ -746,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");
 
@@ -779,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.");
 
 	/*
@@ -1076,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] &&
-- 
1.6.0.rc3.17.gc14c8.dirty

Re: [PATCH] builtin-merge: fail properly when we are in the middle of a conflicted merge

[Junio C Hamano]

We've exchanged quite a few "here is a better one", "oops, that is not
enough", which is confusing to bystanders.  Here is my proposed final
series, meant to be applied to 'maint'.

  [1/2] merge: fix numerus bugs around "trivial merge" area
  [2/2] unpack_trees(): protect the handcrafted in-core index from read_cache()

[PATCH 1/2] merge: fix numerus bugs around "trivial merge" area

[Junio C Hamano]

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

Re: [PATCH 1/2] merge: fix numerus bugs around "trivial merge" area

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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.

By the way, in the medium term, if we are serious about making an internal
call to merge_recursive() from cmd_merge(), I think we may be better off
making it the responsibility for try_merge_strategy() to leave an
committable state in the in-core index (aka "the_index") when they return
with 0 (success) status.  After calling external ones via the run_command
interface, it should do a read_cache() (after calling discard_cache() if
needed); if it calls merge_recursive(), hopefully you already have the
committable state in the in-core index.

That way, when automerge succeeds, write_tree_trivial() can write that
in-core index out and create the tree object to be committed. The
callchain to use merge_recursive() can avoid having to write to the
on-disk index, read it again and write out the tree from it.

[PATCH] builtin-merge: avoid run_command_v_opt() for recursive and subtree

[Miklos Vajna]

The try_merge_strategy() function always ran the strategy in a separate
process, though this is not always necessary. The recursive and subtree
strategy can be called without a fork(). This patch adds a check, and
calls recursive in the same process without wasting resources.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Sat, Aug 23, 2008 at 06:58:37PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> By the way, in the medium term, if we are serious about making an
> internal call to merge_recursive() from cmd_merge(), I think we may be
> better off making it the responsibility for try_merge_strategy() to
> leave an committable state in the in-core index (aka "the_index") when
> they return with 0 (success) status.  After calling external ones via
> the run_command interface, it should do a read_cache() (after calling
> discard_cache() if needed); if it calls merge_recursive(), hopefully
> you already have the committable state in the in-core index.
>
> That way, when automerge succeeds, write_tree_trivial() can write that
> in-core index out and create the tree object to be committed. The
> callchain to use merge_recursive() can avoid having to write to the
> on-disk index, read it again and write out the tree from it.

This is mostly the same patch I submitted earlier, but this is now on
top of mv/merge-recursive, and now it does what you suggested above.

 builtin-merge.c |   92 +++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 63 insertions(+), 29 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index 0bff26e..3a38089 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -22,6 +22,7 @@
 #include "log-tree.h"
 #include "color.h"
 #include "rerere.h"
+#include "merge-recursive.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -511,28 +512,64 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 	struct commit_list *j;
 	struct strbuf buf;
 
-	args = xmalloc((4 + commit_list_count(common) +
-			commit_list_count(remoteheads)) * sizeof(char *));
-	strbuf_init(&buf, 0);
-	strbuf_addf(&buf, "merge-%s", strategy);
-	args[i++] = buf.buf;
-	for (j = common; j; j = j->next)
-		args[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));
-	args[i++] = "--";
-	args[i++] = head_arg;
-	for (j = remoteheads; j; j = j->next)
-		args[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));
-	args[i] = NULL;
-	ret = run_command_v_opt(args, RUN_GIT_CMD);
-	strbuf_release(&buf);
-	i = 1;
-	for (j = common; j; j = j->next)
-		free((void *)args[i++]);
-	i += 2;
-	for (j = remoteheads; j; j = j->next)
-		free((void *)args[i++]);
-	free(args);
-	return -ret;
+	if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
+		int clean;
+		struct commit *result;
+		struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
+		int index_fd;
+		struct commit_list *reversed = NULL;
+		struct merge_options o;
+
+		if (remoteheads->next) {
+			error("Not handling anything other than two heads merge.");
+			return 2;
+		}
+
+		init_merge_options(&o);
+		if (!strcmp(strategy, "subtree"))
+			o.subtree_merge = 1;
+
+		o.branch1 = head_arg;
+		o.branch2 = remoteheads->item->util;
+
+		for (j = common; j; j = j->next)
+			commit_list_insert(j->item, &reversed);
+
+		index_fd = hold_locked_index(lock, 1);
+		clean = merge_recursive(&o, lookup_commit(head),
+				remoteheads->item, reversed, &result);
+		if (active_cache_changed &&
+				(write_cache(index_fd, active_cache, active_nr) ||
+				 commit_locked_index(lock)))
+			die ("unable to write %s", get_index_file());
+		return clean ? 0 : 1;
+	} else {
+		args = xmalloc((4 + commit_list_count(common) +
+					commit_list_count(remoteheads)) * sizeof(char *));
+		strbuf_init(&buf, 0);
+		strbuf_addf(&buf, "merge-%s", strategy);
+		args[i++] = buf.buf;
+		for (j = common; j; j = j->next)
+			args[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));
+		args[i++] = "--";
+		args[i++] = head_arg;
+		for (j = remoteheads; j; j = j->next)
+			args[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));
+		args[i] = NULL;
+		ret = run_command_v_opt(args, RUN_GIT_CMD);
+		strbuf_release(&buf);
+		i = 1;
+		for (j = common; j; j = j->next)
+			free((void *)args[i++]);
+		i += 2;
+		for (j = remoteheads; j; j = j->next)
+			free((void *)args[i++]);
+		free(args);
+		discard_cache();
+		if (read_cache() < 0)
+			die("failed to read the cache");
+		return -ret;
+	}
 }
 
 static void count_diff_files(struct diff_queue_struct *q,
@@ -743,10 +780,6 @@ static int evaluate_result(void)
 	int cnt = 0;
 	struct rev_info rev;
 
-	discard_cache();
-	if (read_cache() < 0)
-		die("failed to read the cache");
-
 	/* Check how many files differ. */
 	init_revisions(&rev, "");
 	setup_revisions(0, NULL, &rev, NULL);
@@ -880,12 +913,14 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 	for (i = 0; i < argc; i++) {
 		struct object *o;
+		struct commit *commit;
 
 		o = peel_to_type(argv[i], 0, NULL, OBJ_COMMIT);
 		if (!o)
 			die("%s - not something we can merge", argv[i]);
-		remotes = &commit_list_insert(lookup_commit(o->sha1),
-			remotes)->next;
+		commit = lookup_commit(o->sha1);
+		commit->util = (void *)argv[i];
+		remotes = &commit_list_insert(commit, remotes)->next;
 
 		strbuf_addf(&buf, "GITHEAD_%s", sha1_to_hex(o->sha1));
 		setenv(buf.buf, argv[i], 1);
@@ -1079,7 +1114,6 @@ 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;
-- 
1.6.0

[PATCH 2/2] unpack_trees(): protect the handcrafted in-core index from read_cache()

[Junio C Hamano]

unpack_trees() rebuilds the in-core index from scratch by allocating a new
structure and finishing it off by copying the built one to the final
index.

The resulting in-core index is Ok for most use, but read_cache() does not
recognize it as such.  The function is meant to be no-op if you already
have loaded the index, until you call discard_cache().

This change the way read_cache() detects an already initialized in-core
index, by introducing an extra bit, and marks the handcrafted in-core
index as initialized, to avoid this problem.

A better fix in the longer term would be to change the read_cache() API so
that it will always discard and re-read from the on-disk index to avoid
confusion.  But there are higher level API that have relied on the current
semantics, and they and their users all need to get converted, which is
outside the scope of 'maint' track.

An example of such a higher level API is write_cache_as_tree(), which is
used by git-write-tree as well as later Porcelains like git-merge, revert
and cherry-pick.  In the longer term, we should remove read_cache() from
there and add one to cmd_write_tree(); other callers expect that the
in-core index they prepared is what gets written as a tree so no other
change is necessary for this particular codepath.

The original version of this patch marked the index by pointing an
otherwise wasted malloc'ed memory with o->result.alloc, but this version
uses Linus's idea to use a new "initialized" bit, which is conceptually
much cleaner.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h                  |    3 ++-
 read-cache.c             |    4 +++-
 t/t7605-merge-resolve.sh |    2 +-
 unpack-trees.c           |    1 +
 4 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index 2475de9..884fae8 100644
--- a/cache.h
+++ b/cache.h
@@ -222,7 +222,8 @@ struct index_state {
 	struct cache_tree *cache_tree;
 	time_t timestamp;
 	void *alloc;
-	unsigned name_hash_initialized : 1;
+	unsigned name_hash_initialized : 1,
+		 initialized : 1;
 	struct hash_table name_hash;
 };
 
diff --git a/read-cache.c b/read-cache.c
index 2c03ec3..35fec46 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1155,7 +1155,7 @@ int read_index_from(struct index_state *istate, const char *path)
 	size_t mmap_size;
 
 	errno = EBUSY;
-	if (istate->alloc)
+	if (istate->initialized)
 		return istate->cache_nr;
 
 	errno = ENOENT;
@@ -1195,6 +1195,7 @@ int read_index_from(struct index_state *istate, const char *path)
 	 * index size
 	 */
 	istate->alloc = xmalloc(estimate_cache_size(mmap_size, istate->cache_nr));
+	istate->initialized = 1;
 
 	src_offset = sizeof(*hdr);
 	dst_offset = 0;
@@ -1247,6 +1248,7 @@ int discard_index(struct index_state *istate)
 	cache_tree_free(&(istate->cache_tree));
 	free(istate->alloc);
 	istate->alloc = NULL;
+	istate->initialized = 0;
 
 	/* no need to throw away allocated active_cache */
 	return 0;
diff --git a/t/t7605-merge-resolve.sh b/t/t7605-merge-resolve.sh
index 5c53608..f1f86dd 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_failure 'merge c1 to c2' '
+test_expect_success 'merge c1 to c2' '
 	git reset --hard c1 &&
 	git merge -s resolve c2 &&
 	test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" &&
diff --git a/unpack-trees.c b/unpack-trees.c
index cba0aca..ef21c62 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -376,6 +376,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	state.refresh_cache = 1;
 
 	memset(&o->result, 0, sizeof(o->result));
+	o->result.initialized = 1;
 	if (o->src_index)
 		o->result.timestamp = o->src_index->timestamp;
 	o->merge_size = len;
-- 
1.6.0.51.g078ae

Re: [PATCH] builtin-merge: fail properly when we are in the middle of a conflicted merge

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> Using unmerged_cache() without reading the cache first never will return
> anything. However, if we read the cache early then we have to discard it
> when we want to read it again from the disk.

I do not think the fix is correct with or without this one.  You are
writing a wrong index and recording a wrong tree object in the commit.

I have a two-liner fix for the issue I am testing right now.

diff --git c/t/t7605-merge-resolve.sh w/t/t7605-merge-resolve.sh
index ee21a10..a251dac 100755
--- c/t/t7605-merge-resolve.sh
+++ w/t/t7605-merge-resolve.sh
@@ -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 2 = $(git ls-files)
 '
 
 test_expect_success 'merge c2 to c3 (fails)' '

Re: [PATCH] Fix in-index merge.

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> There were 3 issues:
>
> 1) We need read_cache() before using unpack_trees().
>
> 2) commit_tree() frees the parents, so we need to malloc them.
>
> 3) The current HEAD was missing from the parent list.
>
> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
> ---
>
> On Fri, Aug 22, 2008 at 08:36:39AM +0200, Paolo Bonzini <bonzini@gnu.org> wrote:
>> I had already posted this bug report yesterday but it was hidden in a
>> cover
>> letter at
>> http://permalink.gmane.org/gmane.comp.version-control.git/93143
>
> This should fix the issue.
>
>  builtin-merge.c          |   11 +++++++----
>  t/t7607-merge-inindex.sh |   29 +++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 4 deletions(-)
>  create mode 100755 t/t7607-merge-inindex.sh

Please do not add new testfiles unnecessarily.  You already have test
scripts that cover "git-merge".  Add new ones to them.  The same comment
goes to the other patch.

[PATCH] Fix in-index merge.

[Miklos Vajna]

There were 3 issues:

1) We need read_cache() before using unpack_trees().

2) commit_tree() frees the parents, so we need to malloc them.

3) The current HEAD was missing from the parent list.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Sat, Aug 23, 2008 at 01:50:52AM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Please do not add new testfiles unnecessarily.  You already have test
> scripts that cover "git-merge".  Add new ones to them.  The same
> comment
> goes to the other patch.

Here it is.

 builtin-merge.c  |   11 +++++++----
 t/t7600-merge.sh |    9 +++++++++
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index a201c66..dffe4b8 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -469,6 +469,7 @@ static int read_tree_trivial(unsigned char *common, unsigned char *head,
 	struct tree_desc t[MAX_UNPACK_TREES];
 	struct unpack_trees_options opts;
 
+	read_cache();
 	memset(&opts, 0, sizeof(opts));
 	opts.head_idx = 2;
 	opts.src_index = &the_index;
@@ -651,13 +652,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;
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
-- 
1.6.0.rc3.17.gc14c8.dirty

Re: [PATCH] Fix in-index merge.

[Paolo Bonzini]

Miklos Vajna wrote:
> There were 3 issues:
> 
> 1) We need read_cache() before using unpack_trees().
> 
> 2) commit_tree() frees the parents, so we need to malloc them.
> 
> 3) The current HEAD was missing from the parent list.

Unfortunately it does not really work yet, since the files only present
on the second branch are not added.  I do get

Trying really trivial in-index merge...
Wonderful.
In-index merge

but then "git status" gives:

# On branch master
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#       b

(in my testcase for pre-merge, this results in a failure of the
"octopus" testcase).

Paolo

Re: [PATCH] Fix in-index merge.

[Junio C Hamano]

Here is a tentative fix on top of Miklos's two patches, even though I
won't take them as-is until the test scripts are cleaned up.

 t/t7605-merge-resolve.sh |    4 +++-
 unpack-trees.c           |    2 ++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git c/t/t7605-merge-resolve.sh w/t/t7605-merge-resolve.sh
index ee21a10..be21ded 100755
--- c/t/t7605-merge-resolve.sh
+++ w/t/t7605-merge-resolve.sh
@@ -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 2 = $(git ls-files | wc -l)
 '
 
 test_expect_success 'merge c2 to c3 (fails)' '
diff --git c/unpack-trees.c w/unpack-trees.c
index cba0aca..016fd46 100644
--- c/unpack-trees.c
+++ w/unpack-trees.c
@@ -378,6 +378,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	memset(&o->result, 0, sizeof(o->result));
 	if (o->src_index)
 		o->result.timestamp = o->src_index->timestamp;
+	if (o->dst_index)
+		o->result.alloc = xmalloc(1);
 	o->merge_size = len;
 
 	if (!dfc)

Re: [PATCH] Fix in-index merge.

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Here is a tentative fix on top of Miklos's two patches, even though I
> won't take them as-is until the test scripts are cleaned up.
>
>  t/t7605-merge-resolve.sh |    4 +++-
>  unpack-trees.c           |    2 ++
>  2 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git c/t/t7605-merge-resolve.sh w/t/t7605-merge-resolve.sh
> index ee21a10..be21ded 100755
> --- c/t/t7605-merge-resolve.sh
> +++ w/t/t7605-merge-resolve.sh
> @@ -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 2 = $(git ls-files | wc -l)
>  '

Sorry, the last "2" should obviously be "3".  We are making sure all three
paths are included in the result.

>  
>  test_expect_success 'merge c2 to c3 (fails)' '
> diff --git c/unpack-trees.c w/unpack-trees.c
> index cba0aca..016fd46 100644
> --- c/unpack-trees.c
> +++ w/unpack-trees.c
> @@ -378,6 +378,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>  	memset(&o->result, 0, sizeof(o->result));
>  	if (o->src_index)
>  		o->result.timestamp = o->src_index->timestamp;
> +	if (o->dst_index)
> +		o->result.alloc = xmalloc(1);
>  	o->merge_size = len;
>  
>  	if (!dfc)

[RFH] two and half potential fixlets to the in-core index handling

[Junio C Hamano]

While debugging merge-in-C with Miklos, I noticed that relatively recent
codepaths that deal with the in-core index are subtly broken when a single
process calls read_cache() more than once.

A historical invariant in the API is "if you have read the index or
populated it, then the next read_cache() is a no-op and you need to run
discard_cache() beforehand if you want to read a new on-disk index."  I
think this was broken by relatively recent unpack-trees rewrite, namely
34110cd (Make 'unpack_trees()' have a separate source and destination
index, 2008-03-06) and also 7a51ed6 (Make on-disk index representation
separate from in-core one, 2008-01-14).

With the current code, unpack_trees() builds its merge into a cleaned
o->result (an index_state structure), and it is copied to o->dst_index,
which often is the_index.  But the logic in read_from_index() that
implements the historical invariant checks if istate->alloc is NULL (in
which case the istate is empty -- just after startup, or just after
discard_index()).  The "result" index_state does not have alloc because it
is built from scratch and never allocated anything.

This wouldn't work very well.

A hacky solution I have in the attached patch is to waste an xmalloc(1)
and store it there when o->result is created, and also make
read_from_index() pay attention to the cache_nr and the cache_changed
bit. I think it is the safest and minimum fix.

Another independent issue is that istate has name_hash_initialized bit
that records the validity of the name_hash.  discard_index() frees the
name_hash, but it never resets the bit to zero.  I am not sure what the
ramifications of not doing so, but it certainly feels wrong.

Also, discard_index() did not free the array of pointers istate->cache[];
I think it should.

 read-cache.c   |   11 ++++-------
 unpack-trees.c |    2 ++
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git c/read-cache.c w/read-cache.c
index 2c03ec3..b29f263 100644
--- c/read-cache.c
+++ w/read-cache.c
@@ -1144,7 +1144,7 @@ static inline size_t estimate_cache_size(size_t ondisk_size, unsigned int entrie
 	return ondisk_size + entries*per_entry;
 }
 
-/* remember to discard_cache() before reading a different cache! */
+/* remember to discard_index() before reading a different cache! */
 int read_index_from(struct index_state *istate, const char *path)
 {
 	int fd, i;
@@ -1155,7 +1155,7 @@ int read_index_from(struct index_state *istate, const char *path)
 	size_t mmap_size;
 
 	errno = EBUSY;
-	if (istate->alloc)
+	if (istate->alloc || istate->cache_nr || istate->cache_changed)
 		return istate->cache_nr;
 
 	errno = ENOENT;
@@ -1240,15 +1240,12 @@ unmap:
 
 int discard_index(struct index_state *istate)
 {
-	istate->cache_nr = 0;
-	istate->cache_changed = 0;
-	istate->timestamp = 0;
 	free_hash(&istate->name_hash);
 	cache_tree_free(&(istate->cache_tree));
 	free(istate->alloc);
-	istate->alloc = NULL;
+	free(istate->cache);
 
-	/* no need to throw away allocated active_cache */
+	memset(istate, 0, sizeof(*istate));
 	return 0;
 }
 
diff --git c/unpack-trees.c w/unpack-trees.c
index cba0aca..016fd46 100644
--- c/unpack-trees.c
+++ w/unpack-trees.c
@@ -378,6 +378,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	memset(&o->result, 0, sizeof(o->result));
 	if (o->src_index)
 		o->result.timestamp = o->src_index->timestamp;
+	if (o->dst_index)
+		o->result.alloc = xmalloc(1);
 	o->merge_size = len;
 
 	if (!dfc)

Re: [RFH] two and half potential fixlets to the in-core index handling

[Linus Torvalds]

On Sat, 23 Aug 2008, Junio C Hamano wrote:
> 
> A hacky solution I have in the attached patch is to waste an xmalloc(1)
> and store it there when o->result is created, and also make
> read_from_index() pay attention to the cache_nr and the cache_changed
> bit. I think it is the safest and minimum fix.

Hmm. Wouldn't it be nicer to just add another bit to istate? We have the 
space already, since we already have a bitfield there, with just one bit 
used?

Something like this..

Untested. Of course.

			Linus

---
 cache.h        |    3 ++-
 read-cache.c   |    4 +++-
 unpack-trees.c |    1 +
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index 68ce6e6..6e41ec4 100644
--- a/cache.h
+++ b/cache.h
@@ -222,7 +222,8 @@ struct index_state {
 	struct cache_tree *cache_tree;
 	time_t timestamp;
 	void *alloc;
-	unsigned name_hash_initialized : 1;
+	unsigned name_hash_initialized : 1,
+		 initialized : 1;
 	struct hash_table name_hash;
 };
 
diff --git a/read-cache.c b/read-cache.c
index 2c03ec3..35fec46 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1155,7 +1155,7 @@ int read_index_from(struct index_state *istate, const char *path)
 	size_t mmap_size;
 
 	errno = EBUSY;
-	if (istate->alloc)
+	if (istate->initialized)
 		return istate->cache_nr;
 
 	errno = ENOENT;
@@ -1195,6 +1195,7 @@ int read_index_from(struct index_state *istate, const char *path)
 	 * index size
 	 */
 	istate->alloc = xmalloc(estimate_cache_size(mmap_size, istate->cache_nr));
+	istate->initialized = 1;
 
 	src_offset = sizeof(*hdr);
 	dst_offset = 0;
@@ -1247,6 +1248,7 @@ int discard_index(struct index_state *istate)
 	cache_tree_free(&(istate->cache_tree));
 	free(istate->alloc);
 	istate->alloc = NULL;
+	istate->initialized = 0;
 
 	/* no need to throw away allocated active_cache */
 	return 0;
diff --git a/unpack-trees.c b/unpack-trees.c
index cba0aca..ef21c62 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -376,6 +376,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	state.refresh_cache = 1;
 
 	memset(&o->result, 0, sizeof(o->result));
+	o->result.initialized = 1;
 	if (o->src_index)
 		o->result.timestamp = o->src_index->timestamp;
 	o->merge_size = len;

Re: [RFH] two and half potential fixlets to the in-core index handling

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Sat, 23 Aug 2008, Junio C Hamano wrote:
>> 
>> A hacky solution I have in the attached patch is to waste an xmalloc(1)
>> and store it there when o->result is created, and also make
>> read_from_index() pay attention to the cache_nr and the cache_changed
>> bit. I think it is the safest and minimum fix.
>
> Hmm. Wouldn't it be nicer to just add another bit to istate? We have the 
> space already, since we already have a bitfield there, with just one bit 
> used?

cache_changed can also become a single-bit field.

The oldest "the_index" users work from it in the BSS initialized all
zero.  We'd also need to mark it as initialized, wouldn't we?

Re: [RFH] two and half potential fixlets to the in-core index handling

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> On Sat, 23 Aug 2008, Junio C Hamano wrote:
>>> 
>>> A hacky solution I have in the attached patch is to waste an xmalloc(1)
>>> and store it there when o->result is created, and also make
>>> read_from_index() pay attention to the cache_nr and the cache_changed
>>> bit. I think it is the safest and minimum fix.
>>
>> Hmm. Wouldn't it be nicer to just add another bit to istate? We have the 
>> space already, since we already have a bitfield there, with just one bit 
>> used?
>
> cache_changed can also become a single-bit field.
>
> The oldest "the_index" users work from it in the BSS initialized all
> zero.  We'd also need to mark it as initialized, wouldn't we?

Ah, sorry, even they start with read_cache() to read .git/index (which may
not exist yet, which is fine).

Sorry for a stupid thinko.