threads / patch / 21410

patchTeach 'git merge' and 'git pull' the option --ff-only

Subject: [PATCH] Teach 'git merge' and 'git pull' the option --ff-only

## tl;dr

3 messages between Oct 28, 2009 and Oct 29, 2009. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Björn Gustavsson· Oct 28, 2009, 22:15 UTC · lore

For convenience in scripts and aliases, add the option --ff-only to only allow fast-forwards.

Acknowledgements: I did look at Yuval Kogman's earlier
patch (107768 in gmane), mainly as shortcut to find my
way in the code, but I did not copy anything directly.
Signed-off-by: Björn Gustavsson <bgustavsson@gmail.com>
---
When I started to use git earlier this year, I was suprised
that there was a --no-ff option but no --ff-only option.

I saw in the mailing list archive at gmane that there has been two previous attempts to implement --ff-only. The first patch was made to the git-merge.sh script (before builtin-merge.c was created). As far as I understand it, the author of the patch said he would send some corrections of the patch, but he never did, and nothing more happened.

So here is my patch, the third attempt.
 Documentation/merge-options.txt |    4 ++++
 builtin-merge.c                 |   16 ++++++++++++++--
 git-pull.sh                     |    7 +++++--
 t/t7600-merge.sh                |   29 +++++++++++++++++++++++++++++
 4 files changed, 52 insertions(+), 4 deletions(-)
Show changes to 4 files +52 −4

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 adadf8e..fbf8976 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -60,6 +60,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 can be 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 b6b8428..298adfb 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -43,6 +43,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 fast_forward_only;
 static int allow_trivial = 1, have_message;
 static struct strbuf merge_msg;
 static struct commit_list *remoteheads;
@@ -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", &fast_forward_only,
+		"abort if fast forward is not possible"),
 	OPT_CALLBACK('s', "strategy", &use_strategies, "strategy",
 		"merge strategy to use", option_parse_strategy),
 	OPT_CALLBACK('m', "message", &merge_msg, "message",
@@ -874,6 +877,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		option_commit = 0;
 	}
 
+	if (!allow_fast_forward && fast_forward_only)
+		die("You cannot combine --no-ff with --ff-only.");
+
 	if (!argc)
 		usage_with_options(builtin_merge_usage,
 			builtin_merge_options);
@@ -969,8 +975,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	}
 
 	for (i = 0; i < use_strategies_nr; i++) {
-		if (use_strategies[i]->attr & NO_FAST_FORWARD)
+		if (use_strategies[i]->attr & NO_FAST_FORWARD) {
 			allow_fast_forward = 0;
+			if (fast_forward_only)
+				die("You cannot combine --ff-only with the merge strategy '%s'.", use_strategies[i]->name);
+		}
 		if (use_strategies[i]->attr & NO_TRIVIAL)
 			allow_trivial = 0;
 	}
@@ -1040,7 +1049,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 * only one common.
 		 */
 		refresh_cache(REFRESH_QUIET);
-		if (allow_trivial) {
+		if (allow_trivial && !fast_forward_only) {
 			/* See if it is really trivial. */
 			git_committer_info(IDENT_ERROR_ON_NO_NAME);
 			printf("Trying really trivial in-index merge...\n");
@@ -1079,6 +1088,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		}
 	}
 
+	if (fast_forward_only)
+		die("Not possible to 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 fc78592..37f3d93 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -16,7 +16,8 @@ cd_to_toplevel
 test -z "$(git ls-files -u)" ||
 	die "You are in the middle of a conflicted merge."
 
-strategy_args= diffstat= no_commit= squash= no_ff= log_arg= verbosity=
+strategy_args= diffstat= no_commit= squash= no_ff= ff_only=
+log_arg= verbosity=
 curr_branch=$(git symbolic-ref -q HEAD)
 curr_branch_short=$(echo "$curr_branch" | sed "s|refs/heads/||")
 rebase=$(git config --bool branch.$curr_branch_short.rebase)
@@ -45,6 +46,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)
@@ -215,5 +218,5 @@ merge_name=$(git fmt-merge-msg $log_arg <"$GIT_DIR/FETCH_HEAD") || exit
 test true = "$rebase" &&
 	exec git-rebase $diffstat $strategy_args --onto $merge_head \
 	${oldremoteref:-$merge_head}
-exec git-merge $diffstat $no_commit $squash $no_ff $log_arg $strategy_args \
+exec git-merge $diffstat $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..d696ea9 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -243,6 +243,16 @@ test_expect_success 'merge c0 with c1' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge c0 with c1 with --ff-only' '
+	git reset --hard c0 &&
+	git merge --ff-only c1 &&
+	git merge --ff-only HEAD c0 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 &&
@@ -263,6 +273,14 @@ test_expect_success 'merge c1 with c2 and c3' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'failing merges with --ff-only' '
+	git reset --hard c1 &&
+	test_tick &&
+	test_must_fail git merge --ff-only c2 &&
+	test_must_fail git merge --ff-only c3 &&
+	test_must_fail git merge --ff-only c2 c3
+'
+
 test_expect_success 'merge c0 with c1 (no-commit)' '
 	git reset --hard c0 &&
 	git merge --no-commit c1 &&
@@ -432,6 +450,17 @@ test_expect_success 'combining --squash and --no-ff is refused' '
 	test_must_fail git merge --no-ff --squash c1
 '
 
+test_expect_success 'combining --ff-only and --no-ff is refused' '
+	test_must_fail git merge --ff-only --no-ff c1 &&
+	test_must_fail git merge --no-ff --ff-only c1
+'
+
+test_expect_success 'combining --ff-only with certain merge strategies is refused' '
+	git reset --hard c0 &&
+	test_must_fail git merge --ff-only --strategy=ours c1 &&
+	test_must_fail git merge --ff-only --strategy=subtree c1
+'
+
 test_expect_success 'merge c0 with c1 (ff overrides no-ff)' '
 	git reset --hard c0 &&
 	git config branch.master.mergeoptions "--no-ff" &&
-- 
1.6.5.1.69.g36942
Junio C Hamano· Oct 28, 2009, 22:45 UTC · re: Björn Gustavsson · lore

Re: [PATCH] Teach 'git merge' and 'git pull' the option --ff-only

Björn Gustavsson <bgustavsson@gmail.com> writes:
Show 8 quoted lines
> For convenience in scripts and aliases, add the option
> --ff-only to only allow fast-forwards.
>
> Acknowledgements: I did look at Yuval Kogman's earlier
> patch (107768 in gmane), mainly as shortcut to find my
> way in the code, but I did not copy anything directly.
>
> Signed-off-by: Björn Gustavsson <bgustavsson@gmail.com>
Thanks.  I think you covered the points in the old discussion thread.
Show 11 quoted lines
> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> index adadf8e..fbf8976 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -60,6 +60,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 can be resolved as a
> +	fast-forward.

Do you or do you not allow "already up to date"? I think it makes sense to allow it, but it is unclear from these two lines.

Show 6 quoted lines
> @@ -874,6 +877,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		option_commit = 0;
>  	}
>  
> +	if (!allow_fast_forward && fast_forward_only)
> +		die("You cannot combine --no-ff with --ff-only.");

Are these the only nonsensical combinations? How should this interact with other options, e.g. --squash or --message?

Show 10 quoted lines
> @@ -969,8 +975,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	for (i = 0; i < use_strategies_nr; i++) {
> -		if (use_strategies[i]->attr & NO_FAST_FORWARD)
> +		if (use_strategies[i]->attr & NO_FAST_FORWARD) {
>  			allow_fast_forward = 0;
> +			if (fast_forward_only)
> +				die("You cannot combine --ff-only with the merge strategy '%s'.", use_strategies[i]->name);
> +		}

I am not convinced this tests the right condition nor it is placed at the right place in the codepath---even if a specified strategy happens to allow fast-forward, wouldn't it be nonsense to say

    $ git merge --ff-only -s resolve that-one

in the first place? Note that I am not saying "I am convinced this is wrong."

Show 6 quoted lines
> @@ -1040,7 +1049,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		 * only one common.
>  		 */
>  		refresh_cache(REFRESH_QUIET);
> -		if (allow_trivial) {
> +		if (allow_trivial && !fast_forward_only) {
Good.
Show 6 quoted lines
> @@ -1079,6 +1088,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		}
>  	}
>  
> +	if (fast_forward_only)
> +		die("Not possible to fast forward, aborting.");
Good.
Björn Gustavsson· Oct 29, 2009, 06:23 UTC · re: Junio C Hamano · lore

Re: [PATCH] Teach 'git merge' and 'git pull' the option --ff-only

Junio C Hamano wrote:
Show 8 quoted lines
> Björn Gustavsson <bgustavsson@gmail.com> writes:
>>  
>> +--ff-only::
>> +	Refuse to merge unless the merge can be resolved as a
>> +	fast-forward.
> 
> Do you or do you not allow "already up to date"?  I think it makes sense
> to allow it, but it is unclear from these two lines.

I do allow it. I will change the description to the following in the re-roll:

--ff-only::
	Refuse to merge and exit with a non-zero status unless the
	current `HEAD` is already up-to-date or the merge can be
	resolved as a fast-forward.
Show 10 quoted lines
> 
>> @@ -874,6 +877,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>>  		option_commit = 0;
>>  	}
>>  
>> +	if (!allow_fast_forward && fast_forward_only)
>> +		die("You cannot combine --no-ff with --ff-only.");
> 
> Are these the only nonsensical combinations?  How should this interact
> with other options, e.g. --squash or --message?
They are the only options I can think of that flatly contradict each other.

Combining --squash and --ff-only will succeed if the current HEAD can be fast-forwarded and will abort otherwise. I don't know how useful that would be in practice, but I see no strong reason to forbid it.

The -m option will always be ignored, of course, and there will be the usual warning if fast-forward is possible:

   Fast forward (no commit created; -m option ignored)

I don't think there is any need to explicitly forbid the combination of -m and --ff-only.

I should probably update the commit message in the re-roll to include the information in the previous paragraphs.

Show 19 quoted lines
>> @@ -969,8 +975,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>>  	}
>>  
>>  	for (i = 0; i < use_strategies_nr; i++) {
>> -		if (use_strategies[i]->attr & NO_FAST_FORWARD)
>> +		if (use_strategies[i]->attr & NO_FAST_FORWARD) {
>>  			allow_fast_forward = 0;
>> +			if (fast_forward_only)
>> +				die("You cannot combine --ff-only with the merge strategy '%s'.", use_strategies[i]->name);
>> +		}
> 
> I am not convinced this tests the right condition nor it is placed at the
> right place in the codepath---even if a specified strategy happens to
> allow fast-forward, wouldn't it be nonsense to say
> 
>     $ git merge --ff-only -s resolve that-one
> 
> in the first place?  Note that I am not saying "I am convinced this is
> wrong."

Re-thinking it, I think that the test should be removed. It seemed like a good idea at the time to point out which strategy that prevented the fast-forward, but if there is a list of merge strategies, the test will prevent --ff-only to succeed if *any* of merge strategies cannot fast-forward. (Also, but I am not sure about this, a merge strategy that does not allow fast-forward might allow up-to-date.)

Therefore, I will remove the test in the re-roll.
Thanks for the comments!
/Björn

← back to recent threads