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

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

[Björn Gustavsson]

For convenience in scripts and aliases, add the option
--ff-only to only allow fast-forwards (and up-to-date,
despite the name).

Disallow combining --ff-only and --no-ff, since they
flatly contradict each other.

Allow all other options to be combined with --ff-only
(i.e. do not add any code to handle them specially),
including the following options:

* --strategy (one or more): As long as the chosen merge
  strategy results in up-to-date or fast-forward, the
  command will succeed.

* --squash: I cannot imagine why anyone would want to
  squash commits only if fast-forward is possible, but I
  also see no reason why it should not be allowed.

* --message: The message will always be ignored, but I see
  no need to explicitly disallow providing a redundant message.

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>
---
Here is a re-roll of my previous patch with improvements
suggested by Junio's review. Compared to the previous version,
I have changed the following:

* The documentation now clearly states that up-to-date
  is also allowed.

* The redundant and incorrect test to reject merge strategies
  that do not allow fast-forward has been removed (along
  with the corresponding test case).

* I have added some background on the design decisions to
  the commit message.

* I have added two test cases to test --squash in combination
  with --ff-only.

 Documentation/merge-options.txt |    5 ++++
 builtin-merge.c                 |   11 +++++++++-
 git-pull.sh                     |    7 ++++-
 t/t7600-merge.sh                |   41 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 61 insertions(+), 3 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index adadf8e..27a9a84 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -60,6 +60,11 @@
 	a fast-forward, only update the branch pointer. This is
 	the default behavior of git-merge.
 
+--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.
+
 -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..5e8c4b5 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);
@@ -1040,7 +1046,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 +1085,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..57f6d2b 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 &&
@@ -303,6 +321,17 @@ test_expect_success 'merge c0 with c1 (squash)' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'merge c0 with c1 (squash, ff-only)' '
+	git reset --hard c0 &&
+	git merge --squash --ff-only c1 &&
+	verify_merge file result.1 &&
+	verify_head $c0 &&
+	verify_no_mergehead &&
+	verify_diff squash.1 .git/SQUASH_MSG "[OOPS] bad squash message"
+'
+
+test_debug 'gitk --all'
+
 test_expect_success 'merge c1 with c2 (squash)' '
 	git reset --hard c1 &&
 	git merge --squash c2 &&
@@ -314,6 +343,13 @@ test_expect_success 'merge c1 with c2 (squash)' '
 
 test_debug 'gitk --all'
 
+test_expect_success 'unsuccesful merge of c1 with c2 (squash, ff-only)' '
+	git reset --hard c1 &&
+	test_must_fail git merge --squash --ff-only c2
+'
+
+test_debug 'gitk --all'
+
 test_expect_success 'merge c1 with c2 and c3 (squash)' '
 	git reset --hard c1 &&
 	git merge --squash c2 c3 &&
@@ -432,6 +468,11 @@ 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 '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

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

[Björn Gustavsson]

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

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

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

[Junio C Hamano]

Björn Gustavsson <bgustavsson@gmail.com> writes:

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

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

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

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

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

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

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

[Björn Gustavsson]

Junio C Hamano wrote:
> 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.


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

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