merge --no-ff --no-commit && commit

Hi,

after today's update on master, the merge commit generated by 'git merge --no-ff --no-commit branch && git commit' has only one parent if the merge can be resolved as a fast forward. The test case below demonstrates this behaviour.

Bisecting showed that the regression was introduced by commit 6bb6b034 (builtin-commit: use commit_tree(), 2008-09-10).

Regards,
Gábor

-- >8 --

index 9516f54..98cfc53 100755
@@ -511,4 +511,13 @@ test_expect_success 'in-index merge' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge --no-ff --no-commit && commit' '
+	git reset --hard c0 &&
+	git merge --no-ff --no-commit c1 &&
+	EDITOR=: git commit &&
+	verify_parents $c0 $c1
+'
+
+test_debug 'gitk --all'
+
 test_done

reduce_heads() can filter "duplicated" parents, where "duplicated"
means: A is duplicated if both A and B are parent candidates and A is reachable from B. Doing so in builtin-merge makes sense, but not in builtin-commit, because this breaks git merge --no-commit --no-ff.

Test case by SZEDER Gabor <[email protected]>
---

Here is a patch that fixes the problem for me. In fact I think it would be nice if somehow git-merge could tell git-commit that 'do not reduce heads now' and in other cases it could still do so, but:

1) This is not something git-commit did in the past, either.

2) I have no idea what would be the right interface to do so (maybe a new MERGE_FOO file under .git?)

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

index 55e1087..9daab0a 100644
@@ -988,7 +988,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		reflog_msg = "commit";
 		pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
 	}
-	parents = reduce_heads(parents);
 
 	/* Finally, get the commit message */
 	strbuf_init(&sb, 0);

index 9516f54..98cfc53 100755
@@ -511,4 +511,13 @@ test_expect_success 'in-index merge' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge --no-ff --no-commit && commit' '
+	git reset --hard c0 &&
+	git merge --no-ff --no-commit c1 &&
+	EDITOR=: git commit &&
+	verify_parents $c0 $c1
+'
+
+test_debug 'gitk --all'
+
 test_done

1.6.0.2

Szia Miklós,

Thanks for the quick response. While your patch does fix the bug I reported, unfortunately it introduces a new one: t7502-commit.sh fails at me with the following:

* ok 17: a SIGTERM should break locks

* expecting success:

        git rev-parse second master >expect &&
        test_must_fail git merge second master &&
        git checkout master g &&
        EDITOR=: git commit -a &&
        git cat-file commit HEAD | sed -n -e "s/^parent //p" -e

"/^$/q" >actual &&
test_cmp expect actual

Already up-to-date with 1ae92d674ba95768a00bace571f5ef295ff1696b Trying simple merge with 9af21aa779d9e148680be525ce161baa37e4bdec Simple merge did not work, trying automatic merge. Auto-merging g
ERROR: Merge conflict in g
fatal: merge program failed
Automatic merge failed; fix conflicts and then commit the result. Created commit 45a4b2b: Merge branches 'second' and 'master' into second

@@ -1,2 +1,3 @@
 1ae92d674ba95768a00bace571f5ef295ff1696b
+1ae92d674ba95768a00bace571f5ef295ff1696b
 9af21aa779d9e148680be525ce161baa37e4bdec

* FAIL 18: Hand committing of a redundant merge removes dups

                git rev-parse second master >expect &&
                test_must_fail git merge second master &&
                git checkout master g &&
                EDITOR=: git commit -a &&
                git cat-file commit HEAD | sed -n -e "s/^parent //p"

-e "/^$/q" >actual &&
test_cmp expect actual

* failed 1 among 18 test(s)
make: *** [t7502-commit.sh] Error 1

Regards,
Gábor

:)

Uh-oh. I forgot to make test before git send-email. I'll have a look at it later today.

OK, here is the scenario.

The basic problemis that if it's git-commit that creates the merge commit and not git-merge, then git-commit does not "know" that git-merge was invoked using --no-ff.

--no-ff means that if reduce_heads() removes a parent, that will be a problem, since the resulting commit will no longer be a merge commit.

I think we can't avoid storing this info in a MERGE_MODE file, because we have to add HEAD to the list of parents in case --no-ff is used, but we should not do so in case it's reachable from existing heads and --no-ff is not used.

I'll send a patch that does this in a bit.

In case git merge --no-ff is used with --no-commit or we have a conflict, write info about if fast forwards are allowed or not to $GIT_DIR/MERGE_MODE.

Based on this info, don't run reduce_heads() in case fast-forwards are denied, to avoid turning a 'merge --no-ff' to a non-merge commit.

Test case by SZEDER Gabor <[email protected]>

---

Here it is.

 builtin-commit.c |   16 +++++++++++++++-
 builtin-merge.c  |    9 +++++++++
 t/t7600-merge.sh |    9 +++++++++
 3 files changed, 33 insertions(+), 1 deletions(-)

index 55e1087..aac5cb4 100644
@@ -937,6 +937,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	unsigned char commit_sha1[20];
 	struct ref_lock *ref_lock;
 	struct commit_list *parents = NULL, **pptr = &parents;
+	struct stat statbuf;
+	int allow_fast_forward = 1;
 
 	git_config(git_commit_config, NULL);
 

@@ -988,7 +990,18 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		reflog_msg = "commit";
 		pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
 	}
-	parents = reduce_heads(parents);
+	strbuf_reset(&sb);
+	if (!stat(git_path("MERGE_MODE"), &statbuf)) {
+		if (strbuf_read_file(&sb, git_path("MERGE_MODE"), 0) < 0)
+			die("could not read MERGE_MODE: %s", strerror(errno));
+		printf("debug, contents of buf: '%s'\n",sb.buf);
+		if (!strcmp(sb.buf, "deny_fast_forward")) {
+			printf("debug, deny fast forward\n");
+			allow_fast_forward = 0;
+		}
+	}
+	if (allow_fast_forward)
+		parents = reduce_heads(parents);
 
 	/* Finally, get the commit message */
 	strbuf_init(&sb, 0);

@@ -1040,6 +1053,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	unlink(git_path("MERGE_HEAD"));
 	unlink(git_path("MERGE_MSG"));
+	unlink(git_path("MERGE_MODE"));
 	unlink(git_path("SQUASH_MSG"));
 
 	if (commit_index_files())

index 5c65a58..973b167 100644
@@ -1210,6 +1210,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			merge_msg.len)
 			die("Could not write to %s", git_path("MERGE_MSG"));
 		close(fd);
+		fd = open(git_path("MERGE_MODE"), O_WRONLY | O_CREAT, 0666);
+		if (fd < 0)
+			die("Could open %s for writing", git_path("MERGE_MODE"));
+		strbuf_reset(&buf);
+		if (!allow_fast_forward)
+			strbuf_addf(&buf, "deny_fast_forward");
+		if (write_in_full(fd, buf.buf, buf.len) != buf.len)
+			die("Could not write to %s", git_path("MERGE_MODE"));
+		close(fd);
 	}
 
 	if (merge_was_ok) {

index 9516f54..98cfc53 100755
@@ -511,4 +511,13 @@ test_expect_success 'in-index merge' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge --no-ff --no-commit && commit' '
+	git reset --hard c0 &&
+	git merge --no-ff --no-commit c1 &&
+	EDITOR=: git commit &&
+	verify_parents $c0 $c1
+'
+
+test_debug 'gitk --all'
+
 test_done

1.6.0.2

In case git merge --no-ff is used with --no-commit or we have a conflict, write info about if fast forwards are allowed or not to $GIT_DIR/MERGE_MODE.

Based on this info, don't run reduce_heads() in case fast-forwards are denied, to avoid turning a 'merge --no-ff' to a non-merge commit.

Test case by SZEDER Gabor <[email protected]>

---

Ugh. Removed.

I changed it to "no-ff" so that it's consistent with the option naming.

 builtin-commit.c |   13 ++++++++++++-
 builtin-merge.c  |    9 +++++++++
 t/t7600-merge.sh |    9 +++++++++
 3 files changed, 30 insertions(+), 1 deletions(-)

index 55e1087..f546cf7 100644
@@ -937,6 +937,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	unsigned char commit_sha1[20];
 	struct ref_lock *ref_lock;
 	struct commit_list *parents = NULL, **pptr = &parents;
+	struct stat statbuf;
+	int allow_fast_forward = 1;
 
 	git_config(git_commit_config, NULL);
 

@@ -988,7 +990,15 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		reflog_msg = "commit";
 		pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
 	}
-	parents = reduce_heads(parents);
+	strbuf_reset(&sb);
+	if (!stat(git_path("MERGE_MODE"), &statbuf)) {
+		if (strbuf_read_file(&sb, git_path("MERGE_MODE"), 0) < 0)
+			die("could not read MERGE_MODE: %s", strerror(errno));
+		if (!strcmp(sb.buf, "no-ff"))
+			allow_fast_forward = 0;
+	}
+	if (allow_fast_forward)
+		parents = reduce_heads(parents);
 
 	/* Finally, get the commit message */
 	strbuf_init(&sb, 0);

@@ -1040,6 +1050,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	unlink(git_path("MERGE_HEAD"));
 	unlink(git_path("MERGE_MSG"));
+	unlink(git_path("MERGE_MODE"));
 	unlink(git_path("SQUASH_MSG"));
 
 	if (commit_index_files())

index 5c65a58..20102a0 100644
@@ -1210,6 +1210,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			merge_msg.len)
 			die("Could not write to %s", git_path("MERGE_MSG"));
 		close(fd);
+		fd = open(git_path("MERGE_MODE"), O_WRONLY | O_CREAT, 0666);
+		if (fd < 0)
+			die("Could open %s for writing", git_path("MERGE_MODE"));
+		strbuf_reset(&buf);
+		if (!allow_fast_forward)
+			strbuf_addf(&buf, "no-ff");
+		if (write_in_full(fd, buf.buf, buf.len) != buf.len)
+			die("Could not write to %s", git_path("MERGE_MODE"));
+		close(fd);
 	}
 
 	if (merge_was_ok) {

index 9516f54..98cfc53 100755
@@ -511,4 +511,13 @@ test_expect_success 'in-index merge' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge --no-ff --no-commit && commit' '
+	git reset --hard c0 &&
+	git merge --no-ff --no-commit c1 &&
+	EDITOR=: git commit &&
+	verify_parents $c0 $c1
+'
+
+test_debug 'gitk --all'
+
 test_done

1.6.0.2

In case git merge --no-ff is used with --no-commit or we have a conflict, write info about if fast forwards are allowed or not to $GIT_DIR/MERGE_MODE.

Based on this info, don't run reduce_heads() in case fast-forwards are denied, to avoid turning a 'merge --no-ff' to a non-merge commit.

Test case by SZEDER Gabor <[email protected]>

---

Right, I added it to branch.c::remove_branch_state() and builtin-merge.c::drop_save(). I don't think I should touch builtin-reset.c, it does not delete MERGE_HEAD/MERGE_MSG either.

Fixed.

 branch.c         |    1 +
 builtin-commit.c |   13 ++++++++++++-
 builtin-merge.c  |   10 ++++++++++
 t/t7600-merge.sh |    9 +++++++++
 4 files changed, 32 insertions(+), 1 deletions(-)

index b1e59f2..205b89d 100644
@@ -168,5 +168,6 @@ void remove_branch_state(void)
 	unlink(git_path("MERGE_HEAD"));
 	unlink(git_path("MERGE_RR"));
 	unlink(git_path("MERGE_MSG"));
+	unlink(git_path("MERGE_MODE"));
 	unlink(git_path("SQUASH_MSG"));
 }

index 55e1087..f546cf7 100644
@@ -937,6 +937,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	unsigned char commit_sha1[20];
 	struct ref_lock *ref_lock;
 	struct commit_list *parents = NULL, **pptr = &parents;
+	struct stat statbuf;
+	int allow_fast_forward = 1;
 
 	git_config(git_commit_config, NULL);
 

@@ -988,7 +990,15 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		reflog_msg = "commit";
 		pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
 	}
-	parents = reduce_heads(parents);
+	strbuf_reset(&sb);
+	if (!stat(git_path("MERGE_MODE"), &statbuf)) {
+		if (strbuf_read_file(&sb, git_path("MERGE_MODE"), 0) < 0)
+			die("could not read MERGE_MODE: %s", strerror(errno));
+		if (!strcmp(sb.buf, "no-ff"))
+			allow_fast_forward = 0;
+	}
+	if (allow_fast_forward)
+		parents = reduce_heads(parents);
 
 	/* Finally, get the commit message */
 	strbuf_init(&sb, 0);

@@ -1040,6 +1050,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	unlink(git_path("MERGE_HEAD"));
 	unlink(git_path("MERGE_MSG"));
+	unlink(git_path("MERGE_MODE"));
 	unlink(git_path("SQUASH_MSG"));
 
 	if (commit_index_files())

index 5c65a58..4c9ed5d 100644
@@ -180,6 +180,7 @@ static void drop_save(void)
 {
 	unlink(git_path("MERGE_HEAD"));
 	unlink(git_path("MERGE_MSG"));
+	unlink(git_path("MERGE_MODE"));
 }
 
 static void save_state(void)

@@ -1210,6 +1211,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			merge_msg.len)
 			die("Could not write to %s", git_path("MERGE_MSG"));
 		close(fd);
+		fd = open(git_path("MERGE_MODE"), O_WRONLY | O_CREAT | O_TRUNC, 0666);
+		if (fd < 0)
+			die("Could open %s for writing", git_path("MERGE_MODE"));
+		strbuf_reset(&buf);
+		if (!allow_fast_forward)
+			strbuf_addf(&buf, "no-ff");
+		if (write_in_full(fd, buf.buf, buf.len) != buf.len)
+			die("Could not write to %s", git_path("MERGE_MODE"));
+		close(fd);
 	}
 
 	if (merge_was_ok) {

index 9516f54..98cfc53 100755
@@ -511,4 +511,13 @@ test_expect_success 'in-index merge' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge --no-ff --no-commit && commit' '
+	git reset --hard c0 &&
+	git merge --no-ff --no-commit c1 &&
+	EDITOR=: git commit &&
+	verify_parents $c0 $c1
+'
+
+test_debug 'gitk --all'
+
 test_done

1.6.0.2

*sigh*

$ git co -b mv/merge-noff master && git am -s <this
$ make test
...
*** t0004-unwritable.sh ***
* FAIL 1: setup
...

$ cd t && ./t0004-unwritable.sh -i -v
...
./test-lib.sh: line 237:   362 Segmentation fault      git commit -m initial
* FAIL 1: setup

I leave the debugging to you. ;-)

Shawn.

In case git merge --no-ff is used with --no-commit or we have a conflict, write info about if fast forwards are allowed or not to $GIT_DIR/MERGE_MODE.

Based on this info, don't run reduce_heads() in case fast-forwards are denied, to avoid turning a 'merge --no-ff' to a non-merge commit.

---

That's weird, make test passed for me before I sent this patch. It was based on 15dc66a.

Now I squashed in Gabor's patch and rebased it against 52e8370, I hope it fixed the issue (make test still passes for me).

 branch.c         |    1 +
 builtin-commit.c |   16 ++++++++++++++--
 builtin-merge.c  |   10 ++++++++++
 t/t7600-merge.sh |   16 ++++++++++++++++
 4 files changed, 41 insertions(+), 2 deletions(-)

index b1e59f2..205b89d 100644
@@ -168,5 +168,6 @@ void remove_branch_state(void)
 	unlink(git_path("MERGE_HEAD"));
 	unlink(git_path("MERGE_RR"));
 	unlink(git_path("MERGE_MSG"));
+	unlink(git_path("MERGE_MODE"));
 	unlink(git_path("SQUASH_MSG"));
 }

index b920257..93f360f 100644
@@ -937,6 +937,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	unsigned char commit_sha1[20];
 	struct ref_lock *ref_lock;
 	struct commit_list *parents = NULL, **pptr = &parents;
+	struct stat statbuf;
+	int allow_fast_forward = 1;
 
 	git_config(git_commit_config, NULL);
 

@@ -951,6 +953,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		return 1;
 	}
 
+	strbuf_init(&sb, 0);
 	/* Determine parents */
 	if (initial_commit) {
 		reflog_msg = "commit (initial)";

@@ -984,14 +987,22 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		}
 		fclose(fp);
 		strbuf_release(&m);
+		if (!stat(git_path("MERGE_MODE"), &statbuf)) {
+			if (strbuf_read_file(&sb, git_path("MERGE_MODE"), 0) < 0)
+				die("could not read MERGE_MODE: %s",
+						strerror(errno));
+			if (!strcmp(sb.buf, "no-ff"))
+				allow_fast_forward = 0;
+		}
+		if (allow_fast_forward)
+			parents = reduce_heads(parents);
 	} else {
 		reflog_msg = "commit";
 		pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
 	}
-	parents = reduce_heads(parents);
 
 	/* Finally, get the commit message */
-	strbuf_init(&sb, 0);
+	strbuf_reset(&sb);
 	if (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {
 		rollback_index_files();
 		die("could not read commit message");

@@ -1040,6 +1051,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	unlink(git_path("MERGE_HEAD"));
 	unlink(git_path("MERGE_MSG"));
+	unlink(git_path("MERGE_MODE"));
 	unlink(git_path("SQUASH_MSG"));
 
 	if (commit_index_files())

index 5c65a58..4c9ed5d 100644
@@ -180,6 +180,7 @@ static void drop_save(void)
 {
 	unlink(git_path("MERGE_HEAD"));
 	unlink(git_path("MERGE_MSG"));
+	unlink(git_path("MERGE_MODE"));
 }
 
 static void save_state(void)

@@ -1210,6 +1211,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			merge_msg.len)
 			die("Could not write to %s", git_path("MERGE_MSG"));
 		close(fd);
+		fd = open(git_path("MERGE_MODE"), O_WRONLY | O_CREAT | O_TRUNC, 0666);
+		if (fd < 0)
+			die("Could open %s for writing", git_path("MERGE_MODE"));
+		strbuf_reset(&buf);
+		if (!allow_fast_forward)
+			strbuf_addf(&buf, "no-ff");
+		if (write_in_full(fd, buf.buf, buf.len) != buf.len)
+			die("Could not write to %s", git_path("MERGE_MODE"));
+		close(fd);
 	}
 
 	if (merge_was_ok) {

index 9516f54..209d7cd 100755
@@ -511,4 +511,20 @@ test_expect_success 'in-index merge' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge --no-ff --no-commit && commit' '
+	git reset --hard c0 &&
+	git merge --no-ff --no-commit c1 &&
+	EDITOR=: git commit &&
+	verify_parents $c0 $c1
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'amending no-ff merge commit' '
+	EDITOR=: git commit --amend &&
+	verify_parents $c0 $c1
+'
+
+test_debug 'gitk --all'
+
 test_done

1.6.0.2

Yea, Gabor's patch squashed in is what was needed. This is already in next; it was there on Friday. But thanks anyway.

Shawn.

Since commit 6bb6b034 (builtin-commit: use commit_tree(), 2008-09-10), builtin-commit performs a reduce_heads() unconditionally. However, it's not always needed, and in some cases even harmful.

reduce_heads() is not needed for the initial commit or for an "ordinary" commit, because they don't have any or have only one parent, respectively.

reduce_heads() must be avoided when 'git commit' is run after a 'git merge --no-ff --no-commit', otherwise it will turn the non-fast-forward merge into fast-forward. For the same reason, reduce_heads() must be avoided when amending such a merge commit.

To resolve this issue, 'git merge' will write info about whether fast-forward is allowed or not to $GIT_DIR/MERGE_MODE. Based on this info, 'git commit' will only perform reduce_heads() when it's committing a merge and fast-forward is enabled.

Also add test cases to ensure that non-fast-forward merges are committed and amended properly.

---

I think this should be squashed onto Miklós' patch.

First, it fixes the segmentation fault caused by using the strbuf before initializing it. Second, amending a no-ff merge commit has the same issues with reduce_heads(); see the new test case. And the commit message is also more detailed ;)

I'm not sure about putting these two new test into t7600-merge.sh. Although the infrastructure to keep these tests very short (repo with branches already set up, verify_parents) is there available, the first test tests not only merge, but the cooperation of merge and commit, and this second one tests only commit.

Regards,
Gábor

 builtin-commit.c |   21 +++++++++++----------
 t/t7600-merge.sh |    7 +++++++
 2 files changed, 18 insertions(+), 10 deletions(-)

index e69b4af..93f360f 100644
@@ -953,6 +953,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		return 1;
 	}
 
+	strbuf_init(&sb, 0);
 	/* Determine parents */
 	if (initial_commit) {
 		reflog_msg = "commit (initial)";

@@ -986,22 +987,22 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		}
 		fclose(fp);
 		strbuf_release(&m);
+		if (!stat(git_path("MERGE_MODE"), &statbuf)) {
+			if (strbuf_read_file(&sb, git_path("MERGE_MODE"), 0) < 0)
+				die("could not read MERGE_MODE: %s",
+						strerror(errno));
+			if (!strcmp(sb.buf, "no-ff"))
+				allow_fast_forward = 0;
+		}
+		if (allow_fast_forward)
+			parents = reduce_heads(parents);
 	} else {
 		reflog_msg = "commit";
 		pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
 	}
-	strbuf_reset(&sb);
-	if (!stat(git_path("MERGE_MODE"), &statbuf)) {
-		if (strbuf_read_file(&sb, git_path("MERGE_MODE"), 0) < 0)
-			die("could not read MERGE_MODE: %s", strerror(errno));
-		if (!strcmp(sb.buf, "no-ff"))
-			allow_fast_forward = 0;
-	}
-	if (allow_fast_forward)
-		parents = reduce_heads(parents);
 
 	/* Finally, get the commit message */
-	strbuf_init(&sb, 0);
+	strbuf_reset(&sb);
 	if (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {
 		rollback_index_files();
 		die("could not read commit message");

index 98cfc53..209d7cd 100755
@@ -520,4 +520,11 @@ test_expect_success 'merge --no-ff --no-commit && commit' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'amending no-ff merge commit' '
+	EDITOR=: git commit --amend &&
+	verify_parents $c0 $c1
+'
+
+test_debug 'gitk --all'
+
 test_done

1.6.0.2.430.gfc53

I'll resend a squashed patch with your signoff included.

Given that originally it was git-merge.sh that did this reduction, I think it's OK.

The following is summary of short dicussion about this issue on #git
see: http://colabti.org/irclogger/irclogger_log/git?date=2008-09-26,Fri#l1176

The problem is that sometimes git-commit finishes doing the merge, be it use of --no-commit option, or a conflict occurred; depending on whether git-merge was invoked with or without --no-ff (--ff=never) option it should recurd reduced or non-reduced heads.

The problem for example occurs in the following situation:

  .---.---.---.                  <--- a <--- HEAD
              |\
              | \-1---2          <--- b
              \
               \--x---y          <--- c

$ git merge b c

                      /------------- b 
                      v
  .---.---.---.---1---2---M     <--- a <--- HEAD
              \          /
               \--x---y-/       <--- c

$ git merge --no-ff b c

.---.---.---.---------M <--- a <--- HEAD

              |\       /|
              | \-1---2 |         <--- b
              \         /
               \--x---y/          <--- c

We can select one of the following solutions:

1. As proposed above save git-merge options, for example in MERGE_MODE or MERGE_OPTS file, so git-commit knows what options to use if it was invoked to finish a merge.

2. git-merge saves _reduced_ heads in MERGE_HEAD, and git-commit reduces only HEAD, unless it is in MERGE_HEAD. This means that git-commit uses the following pseudo-code

     if (resolve_ref(HEAD) == MERGE_HEAD[0]) {
        /* non fast-forward case */
        merge HEAD + MERGE_HEAD
     } else {
        reduce_HEAD_maybe()
        merge [HEAD] + MERGE_HEAD
     }

This has the advantage that it doesn't change MERGE_HEAD semantic for simple merge which did not began as octopus

3. Remove reduce_heads() from git-commit entirely, and record in MERGE_HEAD (or rather now MERGE_HEADS) _all_ _reduced_ heads. _All_ means that HEAD is included in MERGE_HEAD if it is not reduced, _reduced_ means that only non-dependent heads are in MERGE_HEAD. This for example means that for simple non-octopus merge case MERGE_HEAD/MERGE_HEADS now contain _all_ parents, and not only other side of merge.

This solution has the advantage of being clear solution, clarifying semantic of MERGE_HEAD (currently HEAD is used both as target, i.e. where merge is to be recorded, and as one of heads to merge/to consider), and making it possible to separate layers: git-merge is about merging, git-commit doesn't need to know anything about merging.

The disadvantage is that it changes format (well, semantic) of MERGE_HEAD, possibly breaking users' scripts; perhaps some of git commands like "git log --merge" or "git diff --merge" should be updated on such change.

Jakub Narebski
Poland
ShadeHawk on #git

First, thank you for the detailed description, I'm really bad in them. :)

ACK, that's why I implemented this one.

This is wrong IMHO. You can write the reduced heads to MERGE_HEAD but you can't know if you can omit the HEAD in case it is reachable already from one of the heads or not. That depends on if the user used --no-ff or not.

Actually I think this would be ugly. MERGE_HEAD is about you can see what will be merged once you commit the merge, but once you include HEAD there, you can't easily check that. Maybe it's just me who sometimes have a look at that file myself directly... :-)

This is I think the fastest and seemingly simplest solution.

I have tried to explain in above pseudocode git-commit is to use additional hack to decide whether to try to reduce HEAD wrt MERGE_HEAD heads (normal case), or not (--no-ff case), namely if resolved HEAD is first head in MERGE_HEAD that means --no-ff case.

It is "halfway" solution and bit hacky (and ugly).

The new semantic would be very simple. If there is no MERGE_HEAD, it is an ordinary no-merge commit, and its only parent would be previous (current) version of HEAD. If there is MERGE_HEAD it contains _all_ parents in a merge commit, and only those heads which will be parents (_reduced_ heads); if HEAD is symref, we modify given branch so it points to newly created merge commit.

Currently parents of merge commits are 'reduce(HEAD + MERGE_HEAD)' in symbolic equation; I propose they would be simply 'MERGE_HEAD'. then we set this branch to new commit

Jakub Narebski
Poland

Yes. Currently - after a merge conflict - you are able to check what heads caused were merged, which caused the conflict, but with this approach you would not be able to. I think this would be a step back...

Uh, I should read my mail before sending it next time.

I just wanted to say that in case, for example, I merge A^ and A, but I get a conflict after octopus tried to merge A^ then it can be a useful info to see that A^ was a head. Putting reduce(HEAD + MERGE_HEAD) to MERGE_HEAD would hide this info, which would make the situation worse, IMHO.

I don't understand: I thought that merge strategy gets _reduced_ heads. Moreover, if head reduction reduces number of heads to two, you would use twohead merge (recursive) instead of octopus, and fast-forward if there is only one head after reduction.

All that I proposed is to put those reduced heads into MERGE_HEAD. I did not proposed to put yet unresolved heads in MERGE_HEAD in case of octopus merge conflict. I think either you misunderstood me, or I misunderstood you.

Take for example the following case of

[H@repo]$ git merge a b c

   .---.---.---.---H                   <-- H <-- HEAD
                    \
                     \.---.---.---a    <-- a
                           \
                            \-b        <-- b
                               \                              
                                \--c   <-- c

Currently after failed merge we have:
HEAD:
refs: refs/heads/H
MERGE_HEAD
sha1(a)
sha1(b)
sha1(c)

I propose it to be
HEAD:
refs: refs/heads/H
MERGE_HEAD
sha1(a)
sha1(c)

And merge strategy chosen would be twohead one (recursive).

If the situation was slightly different

   .---.---.---.---.--.---.---.---H    <-- H <-- HEAD
                    \
                     \.---.---.---a    <-- a
                           \
                            \-b        <-- b
                               \                              
                                \--c   <-- c

I propose it to be
HEAD:
refs: refs/heads/H
MERGE_HEAD
sha1(H)
sha1(a)
sha1(c)

And merge strategy chosen would be octopus.

Jakub Narebski
Poland

Hi all,

Well, I'm just following this from the sidelines... but from design point of view I would prefer #3 because of the clear separation of the merge and commit steps.

Best,
Gábor