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

Elijah Newren [off-list ref] writes:

On Fri, Jul 12, 2019 at 6:50 AM Sergey Organov [off-list ref] wrote:

quoted

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 [off-list ref]
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.

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