Re: Unexpected or wrong ff, no-ff and ff-only behaviour

[Sergey Organov <sorganov@gmail.com>]

Elijah Newren <newren@gmail.com> writes:

> On Fri, Jul 12, 2019 at 6:50 AM Sergey Organov <sorganov@gmail.com> wrote:
>>
>> That said, even if we rather all do agree rebase workflow is always
>> inferior to merge one, is it satisfactory excuse to actively resist
>> otherwise logical behavior of 'git merge' that is even documented? I
>> don't think so.
>>
>> Thus, one way or another, I'd still vote for keeping options description
>> intact, and rather fix the implementation to match the manual, i.e.,
>> make the --[no]-ff actually orthogonal to --ff-only.
>
> This seems like it's asking for trouble.  --ff-only and --no-ff,
> ignoring any knowledge of what they do, are contradictory by their
> names.

I agree --ff-only is not the best name for what I have in mind. However,
it's not the best name for actual behavior either, as for this case
[--ff|--no-ff|--only-ff], or even '--ff=[prefer|never|only]' would look
more consistent.

> Attempting to explain to users that they are not opposites but
> can be used together compatibly sounds very problematic to me.

For me current documentation would be clear enough, provided the options
would indeed behave the way they are described. Unfortunately (for me),
they don't behave that way, and it looks like they never will.

> But let me dig a little deeper into your statement of "resisting
> otherwise logical behavior of 'git merge" that is even documented."
> Going back to when the --ff-only option was introduced:
>
> commit 134748353b2a71a34f899c9b1326ccf7ae082412
> Author: Björn Gustavsson <bgustavsson@gmail.com>
> Date:   Thu Oct 29 23:08:31 2009 +0100
>
>     Teach 'git merge' and 'git pull' the option --ff-only
>
>     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.
>
> We see that the original author even stated pretty clearly and
> strongly that these were polar opposite options.  Our description
> today of --ff-only is the same one he introduced in that patch, and I
> always read the description the same way I think he did: that it
> implied a polar opposite of --no-ff.

OK, so it was at the very beginning where opportunity for better design
has been missed.

> You and Bryan came along and
> pointed out that the description was actually ambiguous and could be
> interpreted a different way...and I then had to re-read the text a
> time or two after you pointed it out to see the alternate reading.  I
> totally admit that the wording is apparently ambiguous and can be read
> the way you have, but I don't at all see that as a reason to keep the
> documentation as-is.  Either it should be clarified to rule out what
> you and usbuser wanted and perhaps misunderstood it to do, or if the
> behavior is changed then it should be clarified so that folks like me
> don't read it to behave as it has since Björn added it.
>
> Personally, I think that if new functionality is wanted, it should
> definitely go in a different flag (perhaps someone can find something
> shorter than --no-ff-and-only-if-ff-is-possible).  But I think Junio
> makes a pretty good argument to be leery of such a new option, and
> Brian points out how to easily get this behavior by just stringing
> together a merge-base command with merge --no-ff.

My argument was *not* that we absolutely must have this functionality,
it was rather that if we had made '--ff-only' option orthogonal (that is
a good thing in general by itself), this functionality would come almost
for free. No documentation change then. However, now I see that such
change at this point will change behavior of certain combinations of
options in incompatible manner, and so is not a good way to go.

BTW, I did suggest a new option for this functionality once, Junio being
in opposition then as well, arguing that it looks like adding a feature
just for the sake of having a feature. At that time I found 'git merge'
happily takes both '--no-ff --ff-only', and made a mistake of assuming,
after re-reading the manual, that it in fact does what is documented, so
I dropped the issue of adding a new option then.

> That's just my $0.02, but since this is the second time in the thread
> that I've suggested improving the documentation, here's a patch to do
> so:
>
> -- 8< --
> Subject: [PATCH] merge-options.txt: clarify meaning of various ff-related
>  options
>
> As discovered on the mailing list, some of the descriptions of the
> ff-related options were unclear.  Try to be more precise with what these
> options do.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  Documentation/merge-options.txt | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> index 79a00d2a4a..e888c99d48 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -40,20 +40,21 @@ set to `no` at the beginning of them.
>  	case of a merge conflict.
>
>  --ff::
> -	When the merge resolves as a fast-forward, only update the branch
> -	pointer, without creating a merge commit.  This is the default
> -	behavior.
> +	When the merge resolves as a fast-forward, only update the
> +	branch pointer (without creating a merge commit).  When a fast
> +	forward update is not possible, create a merge commit.  This is
> +	the default behavior.

A side note: except it is *not* the default "when merging an
annotated...", described below.

>
>  --no-ff::
> -	Create a merge commit even when the merge resolves as a
> -	fast-forward.  This is the default behaviour when merging an
> -	annotated (and possibly signed) tag that is not stored in
> -	its natural place in 'refs/tags/' hierarchy.
> +	Create a merge commit even when the merge could instead resolve
> +	as a fast-forward.  This is the default behaviour when merging
> +	an annotated (and possibly signed) tag that is not stored in its
> +	natural place in 'refs/tags/' hierarchy.
>
>  --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.
> +	When possible, resolve the merge as a fast-forward (do not
> +	create a merge commit).  When not possible, refuse to merge and
> +	exit with a non-zero status.

Yeah, this resolves uncertainty.

However, I think it'd be better to format it after other similar cases,
to make it explicit these options are 3-way choice. I mean something
like this:

--ff::
--no-ff::
--ff-only::
	When the merge resolves as a fast-forward, only update the
	branch pointer (without creating a merge commit).  When a fast
	forward update is not possible, create a merge commit.  This is
	the default behavior, unless merging an annotated (and possibly
	signed) tag that is not stored in its natural place in
	'refs/tags/' hierarchy, in which case --no-ff is assumed.
+
With --no-ff create a merge commit even when the merge could instead
resolve as a fast-forward.
+
With --ff-only resolve the merge as a fast-forward (never create a merge
commit). When fast-forward is not possible, refuse to merge and exit
with non-zero status.


-- Sergey