[PATCH] Add --ff-only flag to git-merge

[PATCH] Add --ff-only flag to git-merge

[Yuval Kogman]

This patch adds an --ff-only flag to git-merge. When specified git-merge
will exit with an error whenver a merge will not be a simple fast
forward, similar to the default behavior of git push.
---
 builtin-merge.c  |    6 ++++++
 t/t7600-merge.sh |   45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index e4555b0..d37423b 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -44,6 +44,7 @@ static const char * const builtin_merge_usage[] = {
 static int show_diffstat = 1, option_log, squash;
 static int option_commit = 1, allow_fast_forward = 1;
 static int allow_trivial = 1, have_message;
+static int only_fast_forward;
 static struct strbuf merge_msg;
 static struct commit_list *remoteheads;
 static unsigned char head[20], stash[20];
@@ -167,6 +168,8 @@ static struct option builtin_merge_options[] = {
 		"perform a commit if the merge succeeds (default)"),
 	OPT_BOOLEAN(0, "ff", &allow_fast_forward,
 		"allow fast forward (default)"),
+	OPT_BOOLEAN(0, "ff-only", &only_fast_forward,
+		"allow only fast forward"),
 	OPT_CALLBACK('s', "strategy", &use_strategies, "strategy",
 		"merge strategy to use", option_parse_strategy),
 	OPT_CALLBACK('m', "message", &merge_msg, "message",
@@ -1012,6 +1015,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		finish(o->sha1, msg.buf);
 		drop_save();
 		return 0;
+	} else if ( only_fast_forward ) {
+		printf("Merge is non fast forward, aborting.\n");
+		return 1;
 	} else if (!remoteheads->next && common->next)
 		;
 		/*
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index e5b210b..6c2febc 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -234,6 +234,8 @@ test_expect_success 'reject non-strategy with a git-merge-foo name' '
 	test_must_fail git merge -s index c1
 '
 
+test_debug 'gitk --all'
+
 test_expect_success 'merge c0 with c1' '
 	git reset --hard c0 &&
 	git merge c1 &&
@@ -243,6 +245,15 @@ test_expect_success 'merge c0 with c1' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge c0 with c1 (fast forward only)' '
+	git reset --hard c0 &&
+	git merge --ff-only c1 &&
+	verify_merge file result.1 &&
+	verify_head "$c1"
+'
+
+test_debug 'gitk --all'
+
 test_expect_success 'merge c1 with c2' '
 	git reset --hard c1 &&
 	test_tick &&
@@ -253,6 +264,14 @@ test_expect_success 'merge c1 with c2' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge c1 with c2' '
+	git reset --hard c1 &&
+	test_tick &&
+	test_must_fail git merge --ff-only c2
+'
+
+test_debug 'gitk --all'
+
 test_expect_success 'merge c1 with c2 and c3' '
 	git reset --hard c1 &&
 	test_tick &&
@@ -263,6 +282,14 @@ test_expect_success 'merge c1 with c2 and c3' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge c1 with c2 and c3 (fast forward only' '
+	git reset --hard c1 &&
+	test_tick &&
+	test_must_fail git merge --ff-only c2 c3
+'
+
+test_debug 'gitk --all'
+
 test_expect_success 'merge c0 with c1 (no-commit)' '
 	git reset --hard c0 &&
 	git merge --no-commit c1 &&
@@ -470,6 +497,15 @@ test_expect_success 'merge c1 with c0, c2, c0, and c1' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge fast forward only' '
+       git reset --hard c1 &&
+       git config branch.master.mergeoptions "" &&
+       test_tick &&
+       test_must_fail git merge --ff-only c0 c2 c0 c1
+'
+
+test_debug 'gitk --all'
+
 test_expect_success 'merge c1 with c0, c2, c0, and c1' '
        git reset --hard c1 &&
        git config branch.master.mergeoptions "" &&
@@ -481,6 +517,15 @@ test_expect_success 'merge c1 with c0, c2, c0, and c1' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge fast forward only' '
+       git reset --hard c1 &&
+       git config branch.master.mergeoptions "" &&
+       test_tick &&
+       test_must_fail git merge --ff-only c1 c2
+'
+
+test_debug 'gitk --all'
+
 test_expect_success 'merge c1 with c1 and c2' '
        git reset --hard c1 &&
        git config branch.master.mergeoptions "" &&
-- 
1.6.1

Re: [PATCH] Add --ff-only flag to git-merge

[Johannes Schindelin]

Hi,

On Wed, 28 Jan 2009, Yuval Kogman wrote:

> @@ -1012,6 +1015,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		finish(o->sha1, msg.buf);
>  		drop_save();
>  		return 0;
> +	} else if ( only_fast_forward ) {
> +		printf("Merge is non fast forward, aborting.\n");
> +		return 1;
>  	} else if (!remoteheads->next && common->next)

If you compare the two if() lines, you will see that you mixed style.

Ciao,
Dscho

Re: [PATCH] Add --ff-only flag to git-merge

[Yuval Kogman]

2009/1/28 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> If you compare the two if() lines, you will see that you mixed style.

Thanks,

I will be fixing it here:

http://github.com/nothingmuch/git/tree/ff-only

before I send a new version, as I likely screwed up many things, not
just that =)

Re: [PATCH] Add --ff-only flag to git-merge

[Sverre Rabbelier]

On Wed, Jan 28, 2009 at 13:53, Yuval Kogman <nothingmuch@woobling.org> wrote:
> This patch adds an --ff-only flag to git-merge. When specified git-merge
> will exit with an error whenver a merge will not be a simple fast
> forward, similar to the default behavior of git push.

I like! This calls for a new alias 'git config alias.integrate "merge
--no-ff"', thanks!
*hopes this hits next soon*

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] Add --ff-only flag to git-merge

[Junio C Hamano]

Yuval Kogman <nothingmuch@woobling.org> writes:

> This patch adds an --ff-only flag to git-merge. When specified git-merge
> will exit with an error whenver a merge will not be a simple fast
> forward, similar to the default behavior of git push.
> ---

Lacks a sign-off.

> @@ -167,6 +168,8 @@ static struct option builtin_merge_options[] = {
>  		"perform a commit if the merge succeeds (default)"),
>  	OPT_BOOLEAN(0, "ff", &allow_fast_forward,
>  		"allow fast forward (default)"),
> +	OPT_BOOLEAN(0, "ff-only", &only_fast_forward,
> +		"allow only fast forward"),
>  	OPT_CALLBACK('s', "strategy", &use_strategies, "strategy",
>  		"merge strategy to use", option_parse_strategy),
>  	OPT_CALLBACK('m', "message", &merge_msg, "message",

After parse_options() returns, there are a few existing tests to reject
nonsensical combination of options.  I think there are some combinations
you would want to reject (for example, "--no-ff --ff-only" is an obvious
such nonsensical combination, but there may be others, such as giving a
strategy explicitly).

> @@ -1012,6 +1015,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		finish(o->sha1, msg.buf);
>  		drop_save();
>  		return 0;
> +	} else if ( only_fast_forward ) {
> +		printf("Merge is non fast forward, aborting.\n");
> +		return 1;
>  	} else if (!remoteheads->next && common->next)
>  		;

This is wrong for at least three reasons:

 * Style of "if (expression) {", somebody else already pointed out.

 * Only this case exits with 1, while others die() that exits with 128.

 * The placement of this misses the case where a merge of two unrelated
   histories is attempted.  You would need to also have a check at "No
   common ancestors found. We need a real merge." part.  The octopus
   codepath could also end up with a fast forward or up-to-date.

Re: [PATCH] Add --ff-only flag to git-merge

[Yuval Kogman]

Hi,

I started incorperating your feedback but before I send a new patch I
have several questions about the trickier bits:

2009/1/28 Junio C Hamano <gitster@pobox.com>:

>  * The placement of this misses the case where a merge of two unrelated
>   histories is attempted.  You would need to also have a check at "No
>   common ancestors found. We need a real merge." part.

Won't that fall through? The if (!common) is above, and this is
eventually an else if for it (see line 978)

> The octopus
>   codepath could also end up with a fast forward or up-to-date.

So this case is obviously more convoluted... If an octopus merge is
chosen should it just pass --ff-only to git-merge-octopus? Or maybe it
should always pass --ff-only and the various different strategies
would just die unconditionally?

Re: [PATCH] Add --ff-only flag to git-merge

[Junio C Hamano]

Yuval Kogman <nothingmuch@woobling.org> writes:

> I started incorperating your feedback but before I send a new patch I
> have several questions about the trickier bits:
>
> 2009/1/28 Junio C Hamano <gitster@pobox.com>:
>
>>  * The placement of this misses the case where a merge of two unrelated
>>   histories is attempted.  You would need to also have a check at "No
>>   common ancestors found. We need a real merge." part.
>
> Won't that fall through? The if (!common) is above, and this is
> eventually an else if for it (see line 978)

When "if (!common)" is true, the empty statement ";" is executed, and all
its "else if" conditional arms will be skipped.  Is that what you want to
happen?

>> The octopus
>>   codepath could also end up with a fast forward or up-to-date.
>
> So this case is obviously more convoluted... If an octopus merge is
> chosen should it just pass --ff-only to git-merge-octopus? Or maybe it
> should always pass --ff-only and the various different strategies
> would just die unconditionally?

I was referring to this part:

	} else {
		/*
		 * An octopus.  If we can reach all the remote we are up
		 * to date.
		 */
		int up_to_date = 1;
		...
		if (up_to_date) {
			finish_up_to_date("Already up-to-date. Yeeah!");
			return 0;
		}
	}

You do not want to fail this case, where you tried to merge others that
have already been merged, when --ff-only is given, do you?  After all, all
that you are interested in is "do not create a new merge commit".

If you scroll down a bit from there, you will see:


	/* We are going to make a new commit. */
	git_committer_info(IDENT_ERROR_ON_NO_NAME);

	/*
	 * At this point, we need a real merge.  No matter what strategy
	 * we use, it would operate on the index, possibly affecting the

This is where "if (!common) ;" will fall through to.

If your goal is to prevent the user from creating a new merge commit,
the logical place to do so would be immediately before that comment,
independent from all the if..elseif..fi conditional arms that come before
it, I think.

You also need to disable allow_trivial when ff-only is given, but I think
that goes without saying.  If you do not want to allow creating a new
merge commit, you do not want even a trivial merge to happen.

[PATCH] Add --ff-only flag to git-merge (take 2)

[Yuval Kogman]

Hi,

This attempt fixes the various issues raised, except for the case of octopus
ending in 1 or more fast forwards. I'm not sure what the best way to fix this
is. I see three obvious options, none of which seem optimal:

	1. git-merge-octopus will take a --ff-only flag as well, adding a branch for that

	2. ff-only could be a merge strategy, and octopus is forbidden in
	   --ff-only, while this new strategy will behave just like octopus when
	   the merge can be fast forward only.

	3. the logic from git-merge-octopus could be duplicated into builtin-merge,
	   to handle only the fast forward case.

Please advise.

Regards,
Yuval

[PATCH] Add --ff-only flag to git-merge

[Yuval Kogman]

When specified git-merge will exit with an error unless the merge is
resolved as a fast-forward.

This is similar to the default behavior of git push.
---
 Documentation/merge-options.txt |    4 +++
 builtin-merge.c                 |   18 +++++++++++++++
 git-pull.sh                     |    4 ++-
 t/t7600-merge.sh                |   45 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 70 insertions(+), 1 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 637b53f..ac742f8 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -59,6 +59,10 @@
 	a fast-forward, only update the branch pointer. This is
 	the default behavior of git-merge.
 
+--ff-only::
+	Refuse to merge unless the merge is resolved as a
+	fast-forward.
+
 -s <strategy>::
 --strategy=<strategy>::
 	Use the given merge strategy; can be supplied more than
diff --git a/builtin-merge.c b/builtin-merge.c
index e4555b0..dae53fe 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -44,6 +44,7 @@ static const char * const builtin_merge_usage[] = {
 static int show_diffstat = 1, option_log, squash;
 static int option_commit = 1, allow_fast_forward = 1;
 static int allow_trivial = 1, have_message;
+static int only_fast_forward;
 static struct strbuf merge_msg;
 static struct commit_list *remoteheads;
 static unsigned char head[20], stash[20];
@@ -167,6 +168,8 @@ static struct option builtin_merge_options[] = {
 		"perform a commit if the merge succeeds (default)"),
 	OPT_BOOLEAN(0, "ff", &allow_fast_forward,
 		"allow fast forward (default)"),
+	OPT_BOOLEAN(0, "ff-only", &only_fast_forward,
+		"allow only fast forward"),
 	OPT_CALLBACK('s', "strategy", &use_strategies, "strategy",
 		"merge strategy to use", option_parse_strategy),
 	OPT_CALLBACK('m', "message", &merge_msg, "message",
@@ -858,6 +861,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		option_commit = 0;
 	}
 
+	if (only_fast_forward && !allow_fast_forward)
+		die("You cannot combine --ff-only with --no-ff.");
+
 	if (!argc)
 		usage_with_options(builtin_merge_usage,
 			builtin_merge_options);
@@ -959,6 +965,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			allow_trivial = 0;
 	}
 
+	if (only_fast_forward && !allow_fast_forward)
+		die("You cannot combine --ff-only with the selected"
+		    " merge strategies.");
+
+
 	if (!remoteheads->next)
 		common = get_merge_bases(lookup_commit(head),
 				remoteheads->item, 1);
@@ -1023,6 +1034,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 * We are not doing octopus, not fast forward, and have
 		 * only one common.
 		 */
+
+		if (only_fast_forward)
+			die("Merge is non fast forward, aborting.");
+
 		refresh_cache(REFRESH_QUIET);
 		if (allow_trivial) {
 			/* See if it is really trivial. */
@@ -1063,6 +1078,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		}
 	}
 
+	if (only_fast_forward)
+		die("Merge is non fast forward, aborting.");
+
 	/* We are going to make a new commit. */
 	git_committer_info(IDENT_ERROR_ON_NO_NAME);
 
diff --git a/git-pull.sh b/git-pull.sh
index 2c7f432..242d36f 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -45,6 +45,8 @@ do
 		no_ff=--ff ;;
 	--no-ff)
 		no_ff=--no-ff ;;
+	--ff-only)
+		ff-only=--ff-only ;;
 	-s=*|--s=*|--st=*|--str=*|--stra=*|--strat=*|--strate=*|\
 		--strateg=*|--strategy=*|\
 	-s|--s|--st|--str|--stra|--strat|--strate|--strateg|--strategy)
@@ -185,5 +187,5 @@ merge_name=$(git fmt-merge-msg $log_arg <"$GIT_DIR/FETCH_HEAD") || exit
 test true = "$rebase" &&
 	exec git-rebase $strategy_args --onto $merge_head \
 	${oldremoteref:-$merge_head}
-exec git-merge $no_stat $no_commit $squash $no_ff $log_arg $strategy_args \
+exec git-merge $no_stat $no_commit $squash $no_ff $ff_only $log_arg $strategy_args \
 	"$merge_name" HEAD $merge_head $verbosity
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index e5b210b..6c2febc 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -234,6 +234,8 @@ test_expect_success 'reject non-strategy with a git-merge-foo name' '
 	test_must_fail git merge -s index c1
 '
 
+test_debug 'gitk --all'
+
 test_expect_success 'merge c0 with c1' '
 	git reset --hard c0 &&
 	git merge c1 &&
@@ -243,6 +245,15 @@ test_expect_success 'merge c0 with c1' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge c0 with c1 (fast forward only)' '
+	git reset --hard c0 &&
+	git merge --ff-only c1 &&
+	verify_merge file result.1 &&
+	verify_head "$c1"
+'
+
+test_debug 'gitk --all'
+
 test_expect_success 'merge c1 with c2' '
 	git reset --hard c1 &&
 	test_tick &&
@@ -253,6 +264,14 @@ test_expect_success 'merge c1 with c2' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge c1 with c2' '
+	git reset --hard c1 &&
+	test_tick &&
+	test_must_fail git merge --ff-only c2
+'
+
+test_debug 'gitk --all'
+
 test_expect_success 'merge c1 with c2 and c3' '
 	git reset --hard c1 &&
 	test_tick &&
@@ -263,6 +282,14 @@ test_expect_success 'merge c1 with c2 and c3' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge c1 with c2 and c3 (fast forward only' '
+	git reset --hard c1 &&
+	test_tick &&
+	test_must_fail git merge --ff-only c2 c3
+'
+
+test_debug 'gitk --all'
+
 test_expect_success 'merge c0 with c1 (no-commit)' '
 	git reset --hard c0 &&
 	git merge --no-commit c1 &&
@@ -470,6 +497,15 @@ test_expect_success 'merge c1 with c0, c2, c0, and c1' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge fast forward only' '
+       git reset --hard c1 &&
+       git config branch.master.mergeoptions "" &&
+       test_tick &&
+       test_must_fail git merge --ff-only c0 c2 c0 c1
+'
+
+test_debug 'gitk --all'
+
 test_expect_success 'merge c1 with c0, c2, c0, and c1' '
        git reset --hard c1 &&
        git config branch.master.mergeoptions "" &&
@@ -481,6 +517,15 @@ test_expect_success 'merge c1 with c0, c2, c0, and c1' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge fast forward only' '
+       git reset --hard c1 &&
+       git config branch.master.mergeoptions "" &&
+       test_tick &&
+       test_must_fail git merge --ff-only c1 c2
+'
+
+test_debug 'gitk --all'
+
 test_expect_success 'merge c1 with c1 and c2' '
        git reset --hard c1 &&
        git config branch.master.mergeoptions "" &&
-- 
1.6.1