[PATCH] merge: allow using --no-ff and --ff-only at the same time

[PATCH] merge: allow using --no-ff and --ff-only at the same time

[Miklos Vajna]

1347483 (Teach 'git merge' and 'git pull' the option --ff-only,
2009-10-29) says this is not allowed, as they contradict each other.

However, --ff-only is about asserting the input of the merge, and
--no-ff is about instructing merge to always create a merge commit, i.e.
it makes sense to use these options together in some workflow, e.g. when
branches are integrated by rebasing then merging, and the maintainer
wants to be sure the branch is rebased.

Signed-off-by: Miklos Vajna <vmiklos@suse.cz>
---
 builtin/merge.c  | 12 ++++++++----
 t/t7600-merge.sh | 11 ++++++++---
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 2ebe732..7026ce0 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1162,9 +1162,6 @@ 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 (!abort_current_merge) {
 		if (!argc) {
 			if (default_to_upstream)
@@ -1433,7 +1430,14 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	if (fast_forward_only)
+	/*
+	 * If --ff-only was used without --no-ff, or: --ff-only and --no-ff was
+	 * used at the same time, and this is not a fast-forward.
+	 */
+	if (fast_forward_only && (allow_fast_forward || remoteheads->next ||
+				common->next ||
+				hashcmp(common->item->object.sha1,
+					head_commit->object.sha1)))
 		die(_("Not possible to fast-forward, aborting."));
 
 	/* We are going to make a new commit. */
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 460d8eb..bf3d9b2 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -497,9 +497,14 @@ 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 and --no-ff (ff is possible)' '
+	git reset --hard c0 &&
+	git merge --ff-only --no-ff c1
+'
+
+test_expect_success 'combining --ff-only and --no-ff (ff is not possible)' '
+	git reset --hard c1 &&
+	test_must_fail git merge --ff-only --no-ff c2
 '
 
 test_expect_success 'merge c0 with c1 (ff overrides no-ff)' '
-- 
1.8.1.4

Re: [PATCH] merge: allow using --no-ff and --ff-only at the same time

[Michael Haggerty]

On 07/01/2013 09:01 AM, Miklos Vajna wrote:
> 1347483 (Teach 'git merge' and 'git pull' the option --ff-only,
> 2009-10-29) says this is not allowed, as they contradict each other.
> 
> However, --ff-only is about asserting the input of the merge, and
> --no-ff is about instructing merge to always create a merge commit, i.e.
> it makes sense to use these options together in some workflow, e.g. when
> branches are integrated by rebasing then merging, and the maintainer
> wants to be sure the branch is rebased.

That is one interpretation of what these options should mean, and I
agree that it is one way of reading the manpage (which says

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

).  However, I don't think that the manpage unambiguously demands this
interpretation, and that (more importantly) most users would be very
surprised if --ff-only and --no-ff were not opposites.

How does it hurt?  If I have configuration value merge.ff set to "only"
and run "git merge --no-ff" and then I merge a branch that *cannot* be
fast forwarded, the logic of your patch would require the merge to be
rejected, no?  But I think it is more plausible to expect that the
command line option takes precedence.

Hmmph.  I just tested and found out that (before your patch) a "--no-ff"
command-line option does *not* override a "git config merge.ff only" but
rather that the combination provokes the error that you are trying to
remove,

    fatal: You cannot combine --no-ff with --ff-only.

I find *that* surprising; usually command-line options override
configuration file settings.  OK, it's time for some more exhaustive
testing:

   situation      merge.ff  option      result
   -------------------------------------------------------------------
1  ff possible    false     --ff        works (ff)
2  ff impossible  false     --ff        works (non-ff)
3  ff possible    false     --ff-only   error "cannot combine options"
4  ff impossible  false     --ff-only   error "cannot combine options"
5  ff possible    false     --no-ff     works (non-ff)
6  ff impossible  false     --no-ff     works (non-ff)
7  ff possible    only      --ff        works (ff)
8  ff impossible  only      --ff        error "not possible to ff"
9  ff possible    only      --ff-only   works (ff)
10 ff impossible  only      --ff-only   error "not possible to ff"
11 ff possible    only      --no-ff     error "cannot combine options"
12 ff impossible  only      --no-ff     error "cannot combine options"

>From line 1 we see that "--ff" overrides "merge.ff=false".

>From lines 3 and 4 we see that "--ff-only" cannot be combined with
"merge.ff=false".

>From line 8 we see that "merge.ff=only" has its effect despite "--ff",
which would normally allow a non-ff merge.

>From lines 11 and 12 we see that "--no-ff" cannot be combined with
"merge.ff=only".

I find this inconsistent.  I think it would be more consistent to have
exactly three states,

* merge.ff unset == --ff == "do ff if possible, otherwise non-ff"

* merge.ff=false == --no-ff == "always create merge commit"

* merge.ff=only == --ff-only == "do ff if possible, otherwise fail"

and for the command-line option to always take precedence over the
config file option.

Moreover, isn't it the usual practice for later command-line options to
take precedence over earlier ones?  It is the case for at least one command:

    $ git log --oneline --no-walk --no-decorate --decorate
    cf71d9b (HEAD, master) 2
    $ git log --oneline --no-walk --decorate --no-decorate
    cf71d9b 2

So I think that command invocations with more than one of {"--ff",
"--no-ff", "--ff-only"} should respect the last option listed rather
than complaining about "cannot combine options".

If I find the time (unlikely) I might submit a patch to implement these
expectations.


In my opinion, your use case shouldn't be supported by the command
because (1) it is confusing, (2) it is not very common, and (3) it is
easy to work around:

    if git merge-base --is-ancestor HEAD $branch
    then
        git merge --no-ff $branch
    else
        echo "fatal: Not possible to fast-forward, aborting."
        exit 1
    fi

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH] merge: allow using --no-ff and --ff-only at the same time

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 3046 bytes --]

Hi Michael,

On Mon, Jul 01, 2013 at 04:52:29PM +0200, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 07/01/2013 09:01 AM, Miklos Vajna wrote:
> > 1347483 (Teach 'git merge' and 'git pull' the option --ff-only,
> > 2009-10-29) says this is not allowed, as they contradict each other.
> > 
> > However, --ff-only is about asserting the input of the merge, and
> > --no-ff is about instructing merge to always create a merge commit, i.e.
> > it makes sense to use these options together in some workflow, e.g. when
> > branches are integrated by rebasing then merging, and the maintainer
> > wants to be sure the branch is rebased.
> 
> That is one interpretation of what these options should mean, and I
> agree that it is one way of reading the manpage (which says
> 
> --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.
> 
> ).  However, I don't think that the manpage unambiguously demands this
> interpretation, and that (more importantly) most users would be very
> surprised if --ff-only and --no-ff were not opposites.

Yes, I agree that that's an unfortunate naming. --ff and --no-ff is the
opposite of each other, however --ff-only is independent, and I would
even rename it to something like --ff-input-only -- but I don't think
it's worth to do so, seeing the cost of it (probably these options are
used by scripts as well).

> How does it hurt?  If I have configuration value merge.ff set to "only"
> and run "git merge --no-ff" and then I merge a branch that *cannot* be
> fast forwarded, the logic of your patch would require the merge to be
> rejected, no?  But I think it is more plausible to expect that the
> command line option takes precedence.

Hmm, I did not remember that actually merge.ff even uses the same
configuration slot for these switches. :-( Yes, that would make sense to
fix, once the switches can be combined. Maybe merge.ff and
merge.ff-only?

> In my opinion, your use case shouldn't be supported by the command
> because (1) it is confusing,

I don't see why it would be confusing. I think using these two options
together is one way to try to get the benefits of both rebase (cleaner
history) and merge (keeping the history of which commits came from a
given merge).

> (2) it is not very common,

Hard to argue that argument. :-) No idea what counts as common, my
motivation is that some projects (e.g. syslog-ng) integrate *every*
feature branch this way, and doing this "manually" (as in indeed
manually or by using a helper script) seems suboptimal, when the support
for this is already mostly in merge.c, just disabled.

> easy to work around:
> 
>     if git merge-base --is-ancestor HEAD $branch
>     then
>         git merge --no-ff $branch
>     else
>         echo "fatal: Not possible to fast-forward, aborting."
>         exit 1
>     fi

Right, that's indeed a viable workaround for the problem.

Miklos

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] merge: allow using --no-ff and --ff-only at the same time

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> So I think that command invocations with more than one of {"--ff",
> "--no-ff", "--ff-only"} should respect the last option listed rather
> than complaining about "cannot combine options".
>
> If I find the time (unlikely) I might submit a patch to implement these
> expectations.

And I wouldn't reject it on the basis of the design --- I agree
fully with your analysis above.  Thanks for digging and spelling out
how they should be fixed.

As to "--no-ff" vs "--ff-only", "--ff-only" has always meant "only
fast-forward updates are allowed.  We do not want to create a merge
commit with this operation."  I do agree with you that the proposed
patch changes the established semantis and may be too disruptive a
thing to do at this point.

> In my opinion, your use case shouldn't be supported by the command
> because (1) it is confusing, (2) it is not very common, and (3) it is
> easy to work around:
> ...

If one were designing Git merge from scratch today, however, I could
see one may have designed these as two orthogonal switches.

 - Precondition on the shape of histories being merged ("fail unless
   fast forward" does not have to be the only criteria);

 - How the update is done ("fast forward to the other head", "always
   create a merge", "fast forward if possible, otherwise merge" do
   not have to be the only three choices).

I do not fundamentally oppose to such a new feature, but they have
to interact sanely with the current "--ff={only,only,never}".

Re: [PATCH] merge: allow using --no-ff and --ff-only at the same time

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 1745 bytes --]

On Mon, Jul 01, 2013 at 08:38:21AM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> As to "--no-ff" vs "--ff-only", "--ff-only" has always meant "only
> fast-forward updates are allowed.  We do not want to create a merge
> commit with this operation."  I do agree with you that the proposed
> patch changes the established semantis and may be too disruptive a
> thing to do at this point.

Hmm, one way around this may be to add a new option that is basically
the same as --no-ff --ff-only (with the patch), except it has a
different name, so it's not confusing. 'git merge --rebase' could be
used for this, but such a name is misleading as well. Anyone has a
better naming idea? :-)

> If one were designing Git merge from scratch today, however, I could
> see one may have designed these as two orthogonal switches.
> 
>  - Precondition on the shape of histories being merged ("fail unless
>    fast forward" does not have to be the only criteria);
> 
>  - How the update is done ("fast forward to the other head", "always
>    create a merge", "fast forward if possible, otherwise merge" do
>    not have to be the only three choices).
> 
> I do not fundamentally oppose to such a new feature, but they have
> to interact sanely with the current "--ff={only,only,never}".

OK, so if I get it right, the problem is that users got used to that
the --ff-only not only means a precondition for the merge, but also
means "either don't create a merge commit or fail", while my patch would
change this second behaviour.

I could imagine then new switches, like 'git merge --pre=ff
--update=no-ff" could provide these, though I'm not sure if it makes
sense to add such generic switches till the only user is "ff".

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] merge: allow using --no-ff and --ff-only at the same time

[Junio C Hamano]

Miklos Vajna <vmiklos@suse.cz> writes:

> OK, so if I get it right, the problem is that users got used to
> that the --ff-only not only means a precondition for the merge,
> but also means "either don't create a merge commit or fail", while
> my patch would change this second behaviour.

It is not just "users got used to".  "We do not want to create a
merge commit with this operation." is what "--ff-only" means from
the day one [*1*].

For a merge not to create an extra merge commit, the other history
has to be a proper descendant, but that "precondition" is a mere
logical consequence of the ultimate goal of the mode.

> I could imagine then new switches, like 'git merge --pre=ff
> --update=no-ff" could provide these, though I'm not sure if it makes
> sense to add such generic switches till the only user is "ff".

Yes, that is why I said "if one were designing it from scratch, I
could see..." in a very weak form.


[Footnote]

*1* 13474835 (Teach 'git merge' and 'git pull' the option --ff-only,
2009-10-29) and also $gmane/107768 whose documentation part says:

  "Refuse to merge unless the merge is resolved as a fast-forward."

[PATCH] merge: handle --ff/--no-ff/--ff-only as a tri-state option

[Miklos Vajna]

This has multiple benefits: with more than one of {"--ff", "--no-ff",
"--ff-only"} respects the last option; also the command-line option to
always take precedence over the config file option.

Signed-off-by: Miklos Vajna <vmiklos@suse.cz>
---

On Mon, Jul 01, 2013 at 04:52:29PM +0200, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> If I find the time (unlikely) I might submit a patch to implement these
> expectations.

Seeing that the --no-ff / --ff-only combo wasn't denied just sort of 
accidently, I agree that it makes more sense to merge allow_fast_forward
and fast_forward_only to a single enum, that automatically gives you 
both benefits.

 builtin/merge.c  | 65 +++++++++++++++++++++++++++++++++++++-------------------
 t/t7600-merge.sh | 12 ++++++++---
 2 files changed, 52 insertions(+), 25 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 2ebe732..561edf4 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -47,8 +47,8 @@ static const char * const builtin_merge_usage[] = {
 };
 
 static int show_diffstat = 1, shortlog_len = -1, squash;
-static int option_commit = 1, allow_fast_forward = 1;
-static int fast_forward_only, option_edit = -1;
+static int option_commit = 1;
+static int option_edit = -1;
 static int allow_trivial = 1, have_message, verify_signatures;
 static int overwrite_ignore = 1;
 static struct strbuf merge_msg = STRBUF_INIT;
@@ -76,6 +76,14 @@ static struct strategy all_strategy[] = {
 
 static const char *pull_twohead, *pull_octopus;
 
+enum ff_type {
+	FF_ALLOW,
+	FF_NO,
+	FF_ONLY
+};
+
+static enum ff_type fast_forward = FF_ALLOW;
+
 static int option_parse_message(const struct option *opt,
 				const char *arg, int unset)
 {
@@ -178,6 +186,21 @@ static int option_parse_n(const struct option *opt,
 	return 0;
 }
 
+static int option_parse_ff(const struct option *opt,
+			  const char *arg, int unset)
+{
+	fast_forward = unset ? FF_NO : FF_ALLOW;
+	return 0;
+}
+
+static int option_parse_ff_only(const struct option *opt,
+			  const char *arg, int unset)
+{
+	if (!unset)
+		fast_forward = FF_ONLY;
+	return 0;
+}
+
 static struct option builtin_merge_options[] = {
 	{ OPTION_CALLBACK, 'n', NULL, NULL, NULL,
 		N_("do not show a diffstat at the end of the merge"),
@@ -194,10 +217,12 @@ static struct option builtin_merge_options[] = {
 		N_("perform a commit if the merge succeeds (default)")),
 	OPT_BOOL('e', "edit", &option_edit,
 		N_("edit message before committing")),
-	OPT_BOOLEAN(0, "ff", &allow_fast_forward,
-		N_("allow fast-forward (default)")),
-	OPT_BOOLEAN(0, "ff-only", &fast_forward_only,
-		N_("abort if fast-forward is not possible")),
+	{ OPTION_CALLBACK, 0, "ff", NULL, NULL,
+		N_("allow fast-forward (default)"),
+		PARSE_OPT_NOARG, option_parse_ff },
+	{ OPTION_CALLBACK, 0, "ff-only", NULL, NULL,
+		N_("abort if fast-forward is not possible"),
+		PARSE_OPT_NOARG, option_parse_ff_only },
 	OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
 	OPT_BOOL(0, "verify-signatures", &verify_signatures,
 		N_("Verify that the named commit has a valid GPG signature")),
@@ -581,10 +606,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 	else if (!strcmp(k, "merge.ff")) {
 		int boolval = git_config_maybe_bool(k, v);
 		if (0 <= boolval) {
-			allow_fast_forward = boolval;
+			fast_forward = boolval ? FF_ALLOW : FF_NO;
 		} else if (v && !strcmp(v, "only")) {
-			allow_fast_forward = 1;
-			fast_forward_only = 1;
+			fast_forward = FF_ONLY;
 		} /* do not barf on values from future versions of git */
 		return 0;
 	} else if (!strcmp(k, "merge.defaulttoupstream")) {
@@ -863,7 +887,7 @@ static int finish_automerge(struct commit *head,
 
 	free_commit_list(common);
 	parents = remoteheads;
-	if (!head_subsumed || !allow_fast_forward)
+	if (!head_subsumed || fast_forward == FF_NO)
 		commit_list_insert(head, &parents);
 	strbuf_addch(&merge_msg, '\n');
 	prepare_to_commit(remoteheads);
@@ -1008,7 +1032,7 @@ static void write_merge_state(struct commit_list *remoteheads)
 	if (fd < 0)
 		die_errno(_("Could not open '%s' for writing"), filename);
 	strbuf_reset(&buf);
-	if (!allow_fast_forward)
+	if (fast_forward == FF_NO)
 		strbuf_addf(&buf, "no-ff");
 	if (write_in_full(fd, buf.buf, buf.len) != buf.len)
 		die_errno(_("Could not write to '%s'"), filename);
@@ -1157,14 +1181,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		show_diffstat = 0;
 
 	if (squash) {
-		if (!allow_fast_forward)
+		if (fast_forward == FF_NO)
 			die(_("You cannot combine --squash with --no-ff."));
 		option_commit = 0;
 	}
 
-	if (!allow_fast_forward && fast_forward_only)
-		die(_("You cannot combine --no-ff with --ff-only."));
-
 	if (!abort_current_merge) {
 		if (!argc) {
 			if (default_to_upstream)
@@ -1206,7 +1227,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 				"empty head"));
 		if (squash)
 			die(_("Squash commit into empty head not supported yet"));
-		if (!allow_fast_forward)
+		if (fast_forward == FF_NO)
 			die(_("Non-fast-forward commit does not make sense into "
 			    "an empty head"));
 		remoteheads = collect_parents(head_commit, &head_subsumed, argc, argv);
@@ -1294,11 +1315,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			    sha1_to_hex(commit->object.sha1));
 		setenv(buf.buf, merge_remote_util(commit)->name, 1);
 		strbuf_reset(&buf);
-		if (!fast_forward_only &&
+		if (fast_forward != FF_ONLY &&
 		    merge_remote_util(commit) &&
 		    merge_remote_util(commit)->obj &&
 		    merge_remote_util(commit)->obj->type == OBJ_TAG)
-			allow_fast_forward = 0;
+			fast_forward = FF_NO;
 	}
 
 	if (option_edit < 0)
@@ -1315,7 +1336,7 @@ 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)
-			allow_fast_forward = 0;
+			fast_forward = FF_NO;
 		if (use_strategies[i]->attr & NO_TRIVIAL)
 			allow_trivial = 0;
 	}
@@ -1345,7 +1366,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 */
 		finish_up_to_date("Already up-to-date.");
 		goto done;
-	} else if (allow_fast_forward && !remoteheads->next &&
+	} else if (fast_forward != FF_NO && !remoteheads->next &&
 			!common->next &&
 			!hashcmp(common->item->object.sha1, head_commit->object.sha1)) {
 		/* Again the most common case of merging one remote. */
@@ -1392,7 +1413,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 * only one common.
 		 */
 		refresh_cache(REFRESH_QUIET);
-		if (allow_trivial && !fast_forward_only) {
+		if (allow_trivial && fast_forward != FF_ONLY) {
 			/* See if it is really trivial. */
 			git_committer_info(IDENT_STRICT);
 			printf(_("Trying really trivial in-index merge...\n"));
@@ -1433,7 +1454,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	if (fast_forward_only)
+	if (fast_forward == FF_ONLY)
 		die(_("Not possible to fast-forward, aborting."));
 
 	/* We are going to make a new commit. */
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 460d8eb..3ff5fb8 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -497,9 +497,15 @@ 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 'option --ff-only overwrites --no-ff' '
+	git merge --no-ff --ff-only c1 &&
+	test_must_fail git merge --no-ff --ff-only c2
+'
+
+test_expect_success 'option --ff-only overwrites merge.ff=only config' '
+	git reset --hard c0 &&
+	test_config merge.ff only &&
+	git merge --no-ff c1
 '
 
 test_expect_success 'merge c0 with c1 (ff overrides no-ff)' '
-- 
1.8.1.4

Re: [PATCH] merge: handle --ff/--no-ff/--ff-only as a tri-state option

[Junio C Hamano]

Miklos Vajna <vmiklos@suse.cz> writes:

> On Mon, Jul 01, 2013 at 04:52:29PM +0200, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> If I find the time (unlikely) I might submit a patch to implement these
>> expectations.
>
> Seeing that the --no-ff / --ff-only combo wasn't denied just sort of 
> accidently, I agree that it makes more sense to merge allow_fast_forward
> and fast_forward_only to a single enum, that automatically gives you 
> both benefits.

Yes, this goes in the right direction.  "Pick one out of these three
possibilities" is how the configuration is done, and the command
line option parsing should follow suit by consolidating these two
variables into one.

Thanks, will queue.

I didn't read the patch carefully, though, so review comments are
very much appreciated.

Re: [PATCH] merge: handle --ff/--no-ff/--ff-only as a tri-state option

[Michael Haggerty]

On 07/01/2013 09:54 PM, Miklos Vajna wrote:
> This has multiple benefits: with more than one of {"--ff", "--no-ff",
> "--ff-only"} respects the last option; also the command-line option to
> always take precedence over the config file option.
> 
> Signed-off-by: Miklos Vajna <vmiklos@suse.cz>
> ---
> 
> On Mon, Jul 01, 2013 at 04:52:29PM +0200, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> If I find the time (unlikely) I might submit a patch to implement these
>> expectations.
> 
> Seeing that the --no-ff / --ff-only combo wasn't denied just sort of 
> accidently, I agree that it makes more sense to merge allow_fast_forward
> and fast_forward_only to a single enum, that automatically gives you 
> both benefits.

Thanks a lot for taking this on!  I would definitely be happy to be able
to set merge.ff=false without preventing the use of explicit "--ff-only"
from the command line.

See comments below...

>  builtin/merge.c  | 65 +++++++++++++++++++++++++++++++++++++-------------------
>  t/t7600-merge.sh | 12 ++++++++---
>  2 files changed, 52 insertions(+), 25 deletions(-)
> 
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 2ebe732..561edf4 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -47,8 +47,8 @@ static const char * const builtin_merge_usage[] = {
>  };
>  
>  static int show_diffstat = 1, shortlog_len = -1, squash;
> -static int option_commit = 1, allow_fast_forward = 1;
> -static int fast_forward_only, option_edit = -1;
> +static int option_commit = 1;
> +static int option_edit = -1;
>  static int allow_trivial = 1, have_message, verify_signatures;
>  static int overwrite_ignore = 1;
>  static struct strbuf merge_msg = STRBUF_INIT;
> @@ -76,6 +76,14 @@ static struct strategy all_strategy[] = {
>  
>  static const char *pull_twohead, *pull_octopus;
>  
> +enum ff_type {
> +	FF_ALLOW,
> +	FF_NO,
> +	FF_ONLY
> +};
> +
> +static enum ff_type fast_forward = FF_ALLOW;
> +
>  static int option_parse_message(const struct option *opt,
>  				const char *arg, int unset)
>  {
> @@ -178,6 +186,21 @@ static int option_parse_n(const struct option *opt,
>  	return 0;
>  }
>  
> +static int option_parse_ff(const struct option *opt,
> +			  const char *arg, int unset)
> +{
> +	fast_forward = unset ? FF_NO : FF_ALLOW;
> +	return 0;
> +}
> +
> +static int option_parse_ff_only(const struct option *opt,
> +			  const char *arg, int unset)
> +{
> +	if (!unset)
> +		fast_forward = FF_ONLY;
> +	return 0;
> +}
> +

You allow --no-ff-only but ignore it, which I think is incorrect.  In

    git merge --ff-only --no-ff-only [...]

, the --no-ff-only should presumably cancel the effect of the previous
--ff-only (i.e., be equivalent to "--ff").  But it is a little bit
subtle because

    git merge --no-ff --no-ff-only

should presumably be equivalent to --no-ff.  So I think that
"--no-ff-only" should do something like

    if (fast_forward == FF_ONLY)
        fast_forward = FF_ALLOW;

(Note that there is an asymmetry here, because "--no-ff-only"
*shouldn't* cancel the effect of "--no-ff", whereas "--ff" *should*
cancel the effect of "--ff-only".  This is because --ff-only restricts
what the user wants to allow whereas --ff removes a restriction.  So I
think it is OK.)

>  static struct option builtin_merge_options[] = {
>  	{ OPTION_CALLBACK, 'n', NULL, NULL, NULL,
>  		N_("do not show a diffstat at the end of the merge"),
> @@ -194,10 +217,12 @@ static struct option builtin_merge_options[] = {
>  		N_("perform a commit if the merge succeeds (default)")),
>  	OPT_BOOL('e', "edit", &option_edit,
>  		N_("edit message before committing")),
> -	OPT_BOOLEAN(0, "ff", &allow_fast_forward,
> -		N_("allow fast-forward (default)")),
> -	OPT_BOOLEAN(0, "ff-only", &fast_forward_only,
> -		N_("abort if fast-forward is not possible")),
> +	{ OPTION_CALLBACK, 0, "ff", NULL, NULL,
> +		N_("allow fast-forward (default)"),
> +		PARSE_OPT_NOARG, option_parse_ff },
> +	{ OPTION_CALLBACK, 0, "ff-only", NULL, NULL,
> +		N_("abort if fast-forward is not possible"),
> +		PARSE_OPT_NOARG, option_parse_ff_only },
>  	OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
>  	OPT_BOOL(0, "verify-signatures", &verify_signatures,
>  		N_("Verify that the named commit has a valid GPG signature")),

I'm no options guru, but I think it would be possible to implement --ff
and --no-ff without callbacks if you choose constants such that
FF_NO==0, something like:

    enum ff_type {
    	FF_NO = 0, /* It is important that this value be zero! */
    	FF_ALLOW,
    	FF_ONLY
    };

    static int fast_forward = FF_ALLOW;

    static struct option builtin_merge_options[] = {
        [...]
        { OPTION_SET_INT, 0, "ff", &fast_forward, NULL,
        	N_("allow fast-forward (default)"),
        	PARSE_OPT_NOARG, NULL, FF_ALLOW },
        { OPTION_CALLBACK, 0, "ff-only", [...]

because OPTION_SET_INT resets the value to zero if "--no-ff" is
specified, which is just what we need.

> @@ -581,10 +606,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)
>  	else if (!strcmp(k, "merge.ff")) {
>  		int boolval = git_config_maybe_bool(k, v);
>  		if (0 <= boolval) {
> -			allow_fast_forward = boolval;
> +			fast_forward = boolval ? FF_ALLOW : FF_NO;
>  		} else if (v && !strcmp(v, "only")) {
> -			allow_fast_forward = 1;
> -			fast_forward_only = 1;
> +			fast_forward = FF_ONLY;
>  		} /* do not barf on values from future versions of git */
>  		return 0;
>  	} else if (!strcmp(k, "merge.defaulttoupstream")) {
> @@ -863,7 +887,7 @@ static int finish_automerge(struct commit *head,
>  
>  	free_commit_list(common);
>  	parents = remoteheads;
> -	if (!head_subsumed || !allow_fast_forward)
> +	if (!head_subsumed || fast_forward == FF_NO)
>  		commit_list_insert(head, &parents);
>  	strbuf_addch(&merge_msg, '\n');
>  	prepare_to_commit(remoteheads);
> @@ -1008,7 +1032,7 @@ static void write_merge_state(struct commit_list *remoteheads)
>  	if (fd < 0)
>  		die_errno(_("Could not open '%s' for writing"), filename);
>  	strbuf_reset(&buf);
> -	if (!allow_fast_forward)
> +	if (fast_forward == FF_NO)
>  		strbuf_addf(&buf, "no-ff");
>  	if (write_in_full(fd, buf.buf, buf.len) != buf.len)
>  		die_errno(_("Could not write to '%s'"), filename);
> @@ -1157,14 +1181,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		show_diffstat = 0;
>  
>  	if (squash) {
> -		if (!allow_fast_forward)
> +		if (fast_forward == FF_NO)
>  			die(_("You cannot combine --squash with --no-ff."));
>  		option_commit = 0;
>  	}
>  

So there is still a problem with setting merge.ff=false, namely that it
prevents the use of --squash.  That's not good.  (I realize that you are
not to blame for this pre-existing behavior.)

How should --squash and the ff-related options interact?

    git merge --ff --squash
    git merge --no-ff --squash

I think these should just squash.

    git merge --ff-only --squash

I think this should definitely squash.  But perhaps it should require
that HEAD be an ancestor of the branch to be merged?

    git merge --squash --ff
    git merge --squash --no-ff
    git merge --squash --ff-only

Should these do the same as the versions with the option order reversed?
 Or should the command line option that appears later take precedence?
The latter implies that {--ff, --no-ff, --ff-only, --squash} actually
constitute a single *quad-state* option, representing "how the results
of the merge should be handled", and, for example,

    git merge --squash --ff-only

ignores the --squash option, and

    git merge --ff-only --squash

ignores the --ff-only option.

I'm not sure.

> -	if (!allow_fast_forward && fast_forward_only)
> -		die(_("You cannot combine --no-ff with --ff-only."));
> -
>  	if (!abort_current_merge) {
>  		if (!argc) {
>  			if (default_to_upstream)
> @@ -1206,7 +1227,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  				"empty head"));
>  		if (squash)
>  			die(_("Squash commit into empty head not supported yet"));
> -		if (!allow_fast_forward)
> +		if (fast_forward == FF_NO)
>  			die(_("Non-fast-forward commit does not make sense into "
>  			    "an empty head"));
>  		remoteheads = collect_parents(head_commit, &head_subsumed, argc, argv);
> @@ -1294,11 +1315,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  			    sha1_to_hex(commit->object.sha1));
>  		setenv(buf.buf, merge_remote_util(commit)->name, 1);
>  		strbuf_reset(&buf);
> -		if (!fast_forward_only &&
> +		if (fast_forward != FF_ONLY &&
>  		    merge_remote_util(commit) &&
>  		    merge_remote_util(commit)->obj &&
>  		    merge_remote_util(commit)->obj->type == OBJ_TAG)
> -			allow_fast_forward = 0;
> +			fast_forward = FF_NO;
>  	}
>  
>  	if (option_edit < 0)
> @@ -1315,7 +1336,7 @@ 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)
> -			allow_fast_forward = 0;
> +			fast_forward = FF_NO;
>  		if (use_strategies[i]->attr & NO_TRIVIAL)
>  			allow_trivial = 0;
>  	}
> @@ -1345,7 +1366,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		 */
>  		finish_up_to_date("Already up-to-date.");
>  		goto done;
> -	} else if (allow_fast_forward && !remoteheads->next &&
> +	} else if (fast_forward != FF_NO && !remoteheads->next &&
>  			!common->next &&
>  			!hashcmp(common->item->object.sha1, head_commit->object.sha1)) {
>  		/* Again the most common case of merging one remote. */
> @@ -1392,7 +1413,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		 * only one common.
>  		 */
>  		refresh_cache(REFRESH_QUIET);
> -		if (allow_trivial && !fast_forward_only) {
> +		if (allow_trivial && fast_forward != FF_ONLY) {
>  			/* See if it is really trivial. */
>  			git_committer_info(IDENT_STRICT);
>  			printf(_("Trying really trivial in-index merge...\n"));
> @@ -1433,7 +1454,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		}
>  	}
>  
> -	if (fast_forward_only)
> +	if (fast_forward == FF_ONLY)
>  		die(_("Not possible to fast-forward, aborting."));
>  
>  	/* We are going to make a new commit. */
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 460d8eb..3ff5fb8 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -497,9 +497,15 @@ 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 'option --ff-only overwrites --no-ff' '
> +	git merge --no-ff --ff-only c1 &&
> +	test_must_fail git merge --no-ff --ff-only c2
> +'
> +
> +test_expect_success 'option --ff-only overwrites merge.ff=only config' '
> +	git reset --hard c0 &&
> +	test_config merge.ff only &&
> +	git merge --no-ff c1
>  '
>  
>  test_expect_success 'merge c0 with c1 (ff overrides no-ff)' '
> 


-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

[PATCH v2] merge: handle --ff/--no-ff/--ff-only as a tri-state option

[Miklos Vajna]

This has multiple benefits: with more than one of {"--ff", "--no-ff",
"--ff-only"} respects the last option; also the command-line option to
always take precedence over the config file option.

Signed-off-by: Miklos Vajna <vmiklos@suse.cz>
---
 builtin/merge.c  | 55 +++++++++++++++++++++++++++++++++----------------------
 t/t7600-merge.sh | 12 +++++++++---
 2 files changed, 42 insertions(+), 25 deletions(-)

On Tue, Jul 02, 2013 at 10:42:39AM +0200, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> You allow --no-ff-only but ignore it, which I think is incorrect.  In
> 
>     git merge --ff-only --no-ff-only [...]
> 
> , the --no-ff-only should presumably cancel the effect of the previous
> --ff-only (i.e., be equivalent to "--ff").  But it is a little bit
> subtle because
> 
>     git merge --no-ff --no-ff-only
> 
> should presumably be equivalent to --no-ff.  So I think that
> "--no-ff-only" should do something like
> 
>     if (fast_forward == FF_ONLY)
>         fast_forward = FF_ALLOW;

Do we really want --no-ff-only? I would rather just disable it, see the
updated patch.

> I'm no options guru, but I think it would be possible to implement --ff
> and --no-ff without callbacks if you choose constants such that
> FF_NO==0, something like:

Indeed, done.

> Should these do the same as the versions with the option order reversed?
>  Or should the command line option that appears later take precedence?
> The latter implies that {--ff, --no-ff, --ff-only, --squash} actually
> constitute a single *quad-state* option, representing "how the results
> of the merge should be handled", and, for example,
> 
>     git merge --squash --ff-only
> 
> ignores the --squash option, and
> 
>     git merge --ff-only --squash
> 
> ignores the --ff-only option.
> 
> I'm not sure.

Actually there is also --no-squash, used by e.g. git-pull internally.
You definitely don't want a five-state option. :-) So for now I would
rather let --squash/--no-squash alone.

diff --git a/builtin/merge.c b/builtin/merge.c
index 2ebe732..149f32a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -47,8 +47,8 @@ static const char * const builtin_merge_usage[] = {
 };
 
 static int show_diffstat = 1, shortlog_len = -1, squash;
-static int option_commit = 1, allow_fast_forward = 1;
-static int fast_forward_only, option_edit = -1;
+static int option_commit = 1;
+static int option_edit = -1;
 static int allow_trivial = 1, have_message, verify_signatures;
 static int overwrite_ignore = 1;
 static struct strbuf merge_msg = STRBUF_INIT;
@@ -76,6 +76,14 @@ static struct strategy all_strategy[] = {
 
 static const char *pull_twohead, *pull_octopus;
 
+enum ff_type {
+	FF_NO,
+	FF_ALLOW,
+	FF_ONLY
+};
+
+static enum ff_type fast_forward = FF_ALLOW;
+
 static int option_parse_message(const struct option *opt,
 				const char *arg, int unset)
 {
@@ -178,6 +186,13 @@ static int option_parse_n(const struct option *opt,
 	return 0;
 }
 
+static int option_parse_ff_only(const struct option *opt,
+			  const char *arg, int unset)
+{
+	fast_forward = FF_ONLY;
+	return 0;
+}
+
 static struct option builtin_merge_options[] = {
 	{ OPTION_CALLBACK, 'n', NULL, NULL, NULL,
 		N_("do not show a diffstat at the end of the merge"),
@@ -194,10 +209,10 @@ static struct option builtin_merge_options[] = {
 		N_("perform a commit if the merge succeeds (default)")),
 	OPT_BOOL('e', "edit", &option_edit,
 		N_("edit message before committing")),
-	OPT_BOOLEAN(0, "ff", &allow_fast_forward,
-		N_("allow fast-forward (default)")),
-	OPT_BOOLEAN(0, "ff-only", &fast_forward_only,
-		N_("abort if fast-forward is not possible")),
+	OPT_SET_INT(0, "ff", &fast_forward, N_("allow fast-forward (default)"), FF_ALLOW),
+	{ OPTION_CALLBACK, 0, "ff-only", NULL, NULL,
+		N_("abort if fast-forward is not possible"),
+		PARSE_OPT_NOARG | PARSE_OPT_NONEG, option_parse_ff_only },
 	OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
 	OPT_BOOL(0, "verify-signatures", &verify_signatures,
 		N_("Verify that the named commit has a valid GPG signature")),
@@ -581,10 +596,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 	else if (!strcmp(k, "merge.ff")) {
 		int boolval = git_config_maybe_bool(k, v);
 		if (0 <= boolval) {
-			allow_fast_forward = boolval;
+			fast_forward = boolval ? FF_ALLOW : FF_NO;
 		} else if (v && !strcmp(v, "only")) {
-			allow_fast_forward = 1;
-			fast_forward_only = 1;
+			fast_forward = FF_ONLY;
 		} /* do not barf on values from future versions of git */
 		return 0;
 	} else if (!strcmp(k, "merge.defaulttoupstream")) {
@@ -863,7 +877,7 @@ static int finish_automerge(struct commit *head,
 
 	free_commit_list(common);
 	parents = remoteheads;
-	if (!head_subsumed || !allow_fast_forward)
+	if (!head_subsumed || fast_forward == FF_NO)
 		commit_list_insert(head, &parents);
 	strbuf_addch(&merge_msg, '\n');
 	prepare_to_commit(remoteheads);
@@ -1008,7 +1022,7 @@ static void write_merge_state(struct commit_list *remoteheads)
 	if (fd < 0)
 		die_errno(_("Could not open '%s' for writing"), filename);
 	strbuf_reset(&buf);
-	if (!allow_fast_forward)
+	if (fast_forward == FF_NO)
 		strbuf_addf(&buf, "no-ff");
 	if (write_in_full(fd, buf.buf, buf.len) != buf.len)
 		die_errno(_("Could not write to '%s'"), filename);
@@ -1157,14 +1171,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		show_diffstat = 0;
 
 	if (squash) {
-		if (!allow_fast_forward)
+		if (fast_forward == FF_NO)
 			die(_("You cannot combine --squash with --no-ff."));
 		option_commit = 0;
 	}
 
-	if (!allow_fast_forward && fast_forward_only)
-		die(_("You cannot combine --no-ff with --ff-only."));
-
 	if (!abort_current_merge) {
 		if (!argc) {
 			if (default_to_upstream)
@@ -1206,7 +1217,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 				"empty head"));
 		if (squash)
 			die(_("Squash commit into empty head not supported yet"));
-		if (!allow_fast_forward)
+		if (fast_forward == FF_NO)
 			die(_("Non-fast-forward commit does not make sense into "
 			    "an empty head"));
 		remoteheads = collect_parents(head_commit, &head_subsumed, argc, argv);
@@ -1294,11 +1305,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			    sha1_to_hex(commit->object.sha1));
 		setenv(buf.buf, merge_remote_util(commit)->name, 1);
 		strbuf_reset(&buf);
-		if (!fast_forward_only &&
+		if (fast_forward != FF_ONLY &&
 		    merge_remote_util(commit) &&
 		    merge_remote_util(commit)->obj &&
 		    merge_remote_util(commit)->obj->type == OBJ_TAG)
-			allow_fast_forward = 0;
+			fast_forward = FF_NO;
 	}
 
 	if (option_edit < 0)
@@ -1315,7 +1326,7 @@ 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)
-			allow_fast_forward = 0;
+			fast_forward = FF_NO;
 		if (use_strategies[i]->attr & NO_TRIVIAL)
 			allow_trivial = 0;
 	}
@@ -1345,7 +1356,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 */
 		finish_up_to_date("Already up-to-date.");
 		goto done;
-	} else if (allow_fast_forward && !remoteheads->next &&
+	} else if (fast_forward != FF_NO && !remoteheads->next &&
 			!common->next &&
 			!hashcmp(common->item->object.sha1, head_commit->object.sha1)) {
 		/* Again the most common case of merging one remote. */
@@ -1392,7 +1403,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 * only one common.
 		 */
 		refresh_cache(REFRESH_QUIET);
-		if (allow_trivial && !fast_forward_only) {
+		if (allow_trivial && fast_forward != FF_ONLY) {
 			/* See if it is really trivial. */
 			git_committer_info(IDENT_STRICT);
 			printf(_("Trying really trivial in-index merge...\n"));
@@ -1433,7 +1444,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	if (fast_forward_only)
+	if (fast_forward == FF_ONLY)
 		die(_("Not possible to fast-forward, aborting."));
 
 	/* We are going to make a new commit. */
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 460d8eb..3ff5fb8 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -497,9 +497,15 @@ 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 'option --ff-only overwrites --no-ff' '
+	git merge --no-ff --ff-only c1 &&
+	test_must_fail git merge --no-ff --ff-only c2
+'
+
+test_expect_success 'option --ff-only overwrites merge.ff=only config' '
+	git reset --hard c0 &&
+	test_config merge.ff only &&
+	git merge --no-ff c1
 '
 
 test_expect_success 'merge c0 with c1 (ff overrides no-ff)' '
-- 
1.8.1.4

Re: [PATCH v2] merge: handle --ff/--no-ff/--ff-only as a tri-state option

[Junio C Hamano]

Miklos Vajna <vmiklos@suse.cz> writes:

>>     if (fast_forward == FF_ONLY)
>>         fast_forward = FF_ALLOW;
>
> Do we really want --no-ff-only? I would rather just disable it, see the
> updated patch.

Sounds sane.

>> I'm no options guru, but I think it would be possible to implement --ff
>> and --no-ff without callbacks if you choose constants such that
>> FF_NO==0, something like:
>
> Indeed, done.

Yup, looks good.

> Actually there is also --no-squash, used by e.g. git-pull internally.
> You definitely don't want a five-state option. :-) So for now I would
> rather let --squash/--no-squash alone.

Sensible for this patch.

Will replace what was queued.  Thanks.

Re: [PATCH] merge: handle --ff/--no-ff/--ff-only as a tri-state option

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> You allow --no-ff-only but ignore it, which I think is incorrect.  In
>
>     git merge --ff-only --no-ff-only [...]
>
> , the --no-ff-only should presumably cancel the effect of the previous
> --ff-only (i.e., be equivalent to "--ff").

Ideally, if we were starting from scratch and living in the "you
pick one out of three" world, we should forbid "--no-ff-only".  The
"--no-ff" option is spelled as if it is a negation of "--ff", and it
did start as such before "--ff-only" was introduced, but "--no-ff"
would have been better named "--always-create-merge", which is what
the option really means.

And in the tristate world, with mutually exclusive "--A", "--B", and
"--C" options, "--no-C" does not mean "I want to do A" at all.

If the existing code had allowed with "--no-ff-only" to defeat
configured merge.ff=only from the command line, then there may have
been users who are used to that behaviour, and we cannot break them,
but luckily or unluckily it does not work, so...

>> @@ -1157,14 +1181,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>>  		show_diffstat = 0;
>>  
>>  	if (squash) {
>> -		if (!allow_fast_forward)
>> +		if (fast_forward == FF_NO)
>>  			die(_("You cannot combine --squash with --no-ff."));
>>  		option_commit = 0;
>>  	}
>>  
>
> So there is still a problem with setting merge.ff=false, namely that it
> prevents the use of --squash.  That's not good.  (I realize that you are
> not to blame for this pre-existing behavior.)
>
> How should --squash and the ff-related options interact?

Interesting point.

>     git merge --ff --squash
>     git merge --no-ff --squash
>
> I think these should just squash.
>
>     git merge --ff-only --squash
>
> I think this should definitely squash.  But perhaps it should require
> that HEAD be an ancestor of the branch to be merged?
>
>     git merge --squash --ff
>     git merge --squash --no-ff
>     git merge --squash --ff-only
>
> Should these do the same as the versions with the option order reversed?

As "--squash" is about _not_ moving the head but only updating the
working tree and the index, I personally think it should be treated
as an error if any of these "ff" options is explicitly given from
the command line.