[BUG] merge --no-ff --no-commit && commit

[BUG] merge --no-ff --no-commit && commit

[SZEDER Gábor]

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

diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 9516f54..98cfc53 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -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

[PATCH] builtin-commit: avoid using reduce_heads()

[Miklos Vajna]

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 <szeder@ira.uka.de>
---

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(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 55e1087..9daab0a 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -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);
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 9516f54..98cfc53 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -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

Re: [PATCH] builtin-commit: avoid using reduce_heads()

[SZEDER Gábor]

Szia Miklós,

On Fri, Sep 26, 2008 at 02:35:59AM +0200, Miklos Vajna wrote:
> 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.

> Here is a patch that fixes the problem for me.

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
--- expect      2008-09-26 00:59:42.000000000 +0000
+++ actual      2008-09-26 00:59:42.000000000 +0000
@@ -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

Re: [PATCH] builtin-commit: avoid using reduce_heads()

[Miklos Vajna]

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

On Fri, Sep 26, 2008 at 03:03:12AM +0200, SZEDER Gábor <szeder@ira.uka.de> wrote:
> Szia Miklós,

:)

> 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

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

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

Re: [PATCH] builtin-commit: avoid using reduce_heads()

[Miklos Vajna]

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

On Fri, Sep 26, 2008 at 03:03:12AM +0200, SZEDER Gábor <szeder@ira.uka.de> wrote:
> * 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

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.

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

[PATCH] builtin-commit: avoid always using reduce_heads()

[Miklos Vajna]

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 <szeder@ira.uka.de>

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

On Fri, Sep 26, 2008 at 05:15:17PM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:
> I'll send a patch that does this in a bit.

Here it is.

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

diff --git a/builtin-commit.c b/builtin-commit.c
index 55e1087..aac5cb4 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -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())
diff --git a/builtin-merge.c b/builtin-merge.c
index 5c65a58..973b167 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -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) {
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 9516f54..98cfc53 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -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

Re: [PATCH] builtin-commit: avoid always using reduce_heads()

[Shawn O. Pearce]

Miklos Vajna <vmiklos@frugalware.org> wrote:
> 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.
 
> diff --git a/builtin-commit.c b/builtin-commit.c
> index 55e1087..aac5cb4 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -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");

Left over debugging.

> diff --git a/builtin-merge.c b/builtin-merge.c
> index 5c65a58..973b167 100644
> --- a/builtin-merge.c
> +++ b/builtin-merge.c
> @@ -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");

I wonder if the option in the file shouldn't be more related to
the merge command line option.  Its --no-ff to the user.  Maybe we
should call it "no-ff" here?  Or "no-fast-forward" if we want to go
with a longer name that is less likely to be ambiguous in the future?

-- 
Shawn.

[PATCH] builtin-commit: avoid always using reduce_heads()

[Miklos Vajna]

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 <szeder@ira.uka.de>

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

On Fri, Sep 26, 2008 at 08:52:04AM -0700, "Shawn O. Pearce" <spearce@spearce.org> wrote:
> > +                   printf("debug, deny fast forward\n");
>
> Left over debugging.

Ugh. Removed.

> > +           if (!allow_fast_forward)
> > +                   strbuf_addf(&buf, "deny_fast_forward");
>
> I wonder if the option in the file shouldn't be more related to
> the merge command line option.  Its --no-ff to the user.  Maybe we
> should call it "no-ff" here?  Or "no-fast-forward" if we want to go
> with a longer name that is less likely to be ambiguous in the future?

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(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 55e1087..f546cf7 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -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())
diff --git a/builtin-merge.c b/builtin-merge.c
index 5c65a58..20102a0 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -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) {
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 9516f54..98cfc53 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -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

Re: [PATCH] builtin-commit: avoid always using reduce_heads()

[Shawn O. Pearce]

Miklos Vajna <vmiklos@frugalware.org> wrote:
> diff --git a/builtin-commit.c b/builtin-commit.c
> index 55e1087..f546cf7 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -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())

Hmmph.  Should branch.c and builtin-reset.c clean this new file
up too?

> diff --git a/builtin-merge.c b/builtin-merge.c
> index 5c65a58..20102a0 100644
> --- a/builtin-merge.c
> +++ b/builtin-merge.c
> @@ -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)

Shouldn't we open this file with O_TRUNC to avoid this scenario:

  $ git merge --no-ff --no-commit foo
  $ git reset --hard
  $ git merge --no-commit foo
  ... *sigh* MERGE_MODE still has "no-ff" in it ...

This is especially true since some porcelain (e.g. git-gui) just
deletes MERGE_HEAD right now and doesn't know about cleaning up
MERGE_MODE.  We'd want to at least reset it correctly on the next
invocation to git-merge.

-- 
Shawn.

[PATCH] builtin-commit: avoid always using reduce_heads()

[Miklos Vajna]

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 <szeder@ira.uka.de>

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

On Thu, Oct 02, 2008 at 07:35:18PM -0700, "Shawn O. Pearce" <spearce@spearce.org> wrote:
> > +   unlink(git_path("MERGE_MODE"));
> >     unlink(git_path("SQUASH_MSG"));
> >
> >     if (commit_index_files())
>
> Hmmph.  Should branch.c and builtin-reset.c clean this new file
> up too?

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.

> > +           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)
>
> Shouldn't we open this file with O_TRUNC to avoid this scenario:
>
>   $ git merge --no-ff --no-commit foo
>   $ git reset --hard
>   $ git merge --no-commit foo
>   ... *sigh* MERGE_MODE still has "no-ff" in it ...
>
> This is especially true since some porcelain (e.g. git-gui) just
> deletes MERGE_HEAD right now and doesn't know about cleaning up
> MERGE_MODE.  We'd want to at least reset it correctly on the next
> invocation to git-merge.

Fixed.

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

diff --git a/branch.c b/branch.c
index b1e59f2..205b89d 100644
--- a/branch.c
+++ b/branch.c
@@ -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"));
 }
diff --git a/builtin-commit.c b/builtin-commit.c
index 55e1087..f546cf7 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -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())
diff --git a/builtin-merge.c b/builtin-merge.c
index 5c65a58..4c9ed5d 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -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) {
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 9516f54..98cfc53 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -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

Re: [PATCH] builtin-commit: avoid always using reduce_heads()

[Shawn O. Pearce]

Miklos Vajna <vmiklos@frugalware.org> wrote:
> 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.

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

[PATCH] builtin-commit: avoid always using reduce_heads()

[Miklos Vajna]

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.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---

On Fri, Oct 03, 2008 at 07:59:15AM -0700, "Shawn O. Pearce" <spearce@spearce.org> wrote:
> ./test-lib.sh: line 237:   362 Segmentation fault      git commit -m
> initial
> * FAIL 1: setup
>
> I leave the debugging to you.  ;-)

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(-)

diff --git a/branch.c b/branch.c
index b1e59f2..205b89d 100644
--- a/branch.c
+++ b/branch.c
@@ -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"));
 }
diff --git a/builtin-commit.c b/builtin-commit.c
index b920257..93f360f 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -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())
diff --git a/builtin-merge.c b/builtin-merge.c
index 5c65a58..4c9ed5d 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -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) {
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 9516f54..209d7cd 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -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

Re: [PATCH] builtin-commit: avoid always using reduce_heads()

[Shawn O. Pearce]

Miklos Vajna <vmiklos@frugalware.org> wrote:
> 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.
 
> On Fri, Oct 03, 2008 at 07:59:15AM -0700, "Shawn O. Pearce" <spearce@spearce.org> wrote:
> > ./test-lib.sh: line 237:   362 Segmentation fault      git commit -m
> > initial
> > * FAIL 1: setup
> >
> > I leave the debugging to you.  ;-)
> 
> 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).

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

-- 
Shawn.

[PATCH] builtin-commit: use reduce_heads() only when appropriate

[SZEDER Gábor]

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.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---

  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(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index e69b4af..93f360f 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -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");
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 98cfc53..209d7cd 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -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

Re: [PATCH] builtin-commit: use reduce_heads() only when appropriate

[Miklos Vajna]

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

On Fri, Oct 03, 2008 at 05:09:27PM +0200, SZEDER Gábor <szeder@ira.uka.de> wrote:
>   I think this should be squashed onto Miklós' patch.

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

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

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

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

Re: [PATCH] builtin-commit: avoid using reduce_heads()

[Jakub Narebski]

Miklos Vajna <vmiklos@frugalware.org> writes:

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

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

Re: [PATCH] builtin-commit: avoid using reduce_heads()

[Miklos Vajna]

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

On Fri, Sep 26, 2008 at 09:17:39AM -0700, Jakub Narebski <jnareb@gmail.com> wrote:
> 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.

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

ACK, that's why I implemented this one.

> 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

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.

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

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

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

Re: [PATCH] builtin-commit: avoid using reduce_heads()

[Jakub Narebski]

On Fri, 26 Sep 2008, Miklos Vajna wrote:
> On Fri, Sep 26, 2008 at 09:17:39AM -0700, Jakub Narebski <jnareb@gmail.com> wrote:
> >
> > 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.
> 
> First, thank you for the detailed description, I'm really bad in them.
> :)
> 
> ACK, that's why I implemented this one.

This is I think the fastest and seemingly simplest solution.

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

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

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

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

Re: [PATCH] builtin-commit: avoid using reduce_heads()

[Miklos Vajna]

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

On Sat, Sep 27, 2008 at 01:51:47AM +0200, Jakub Narebski <jnareb@gmail.com> wrote:
> > 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... :-)
> 
> 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

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

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

Re: [PATCH] builtin-commit: avoid using reduce_heads()

[Miklos Vajna]

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

On Mon, Sep 29, 2008 at 05:07:22PM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:
> > 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
> 
> 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.

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

Re: [PATCH] builtin-commit: avoid using reduce_heads()

[Jakub Narebski]

Dnia poniedziałek 29. września 2008 20:18, Miklos Vajna napisał:
> On Mon, Sep 29, 2008 at 05:07:22PM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:

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

Re: [PATCH] builtin-commit: avoid using reduce_heads()

[SZEDER Gábor]

Hi all,

On Fri, Sep 26, 2008 at 09:17:39AM -0700, Jakub Narebski wrote:
> 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.

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