threads / patch / 17415

patchAdd --ff-only flag to git-merge

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

## tl;dr

9 messages between Jan 28, 2009 and Jan 30, 2009. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Yuval Kogman· Jan 28, 2009, 12:53 UTC · lore
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(-)
Show changes to 2 files +51 −0

builtin-merge.c, t/t7600-merge.sh

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
Johannes Schindelin· Jan 28, 2009, 13:26 UTC · re: Yuval Kogman · lore

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

Hi,
On Wed, 28 Jan 2009, Yuval Kogman wrote:
Show 8 quoted lines
> @@ -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

Yuval Kogman· Jan 28, 2009, 13:29 UTC · re: Johannes Schindelin · lore

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

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

Sverre Rabbelier· Jan 28, 2009, 14:35 UTC · re: Yuval Kogman · lore

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

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
Junio C Hamano· Jan 28, 2009, 21:12 UTC · re: Yuval Kogman · lore

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

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.
Show 9 quoted lines
> @@ -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).

Show 9 quoted lines
> @@ -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.
Yuval Kogman· Jan 29, 2009, 20:57 UTC · re: Junio C Hamano · lore

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

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?

Junio C Hamano· Jan 29, 2009, 22:29 UTC · re: Yuval Kogman · lore

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

Yuval Kogman <nothingmuch@woobling.org> writes:
Show 11 quoted lines
> 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?

Show 7 quoted lines
>> 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.

Yuval Kogman· Jan 30, 2009, 04:32 UTC · re: Junio C Hamano · lore

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

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

Yuval Kogman· Jan 30, 2009, 04:32 UTC · re: Yuval Kogman · lore

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(-)
Show changes to 4 files +70 −1

Documentation/merge-options.txt, builtin-merge.c, git-pull.sh, t/t7600-merge.sh

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

← back to recent threads