From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 28 Oct 2009 22:45:34 GMT
Subject: Re: [PATCH] Teach 'git merge' and 'git pull' the option --ff-only
Message-ID: <7vk4yfi1dd.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4AE8C281.50104@gmail.com>

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.