{"thread":{"id":"34313","subject":"[PATCH] merge: allow using --no-ff and --ff-only at the same time","startedAt":"2013-07-01T07:01:43Z","lastAt":"2013-07-02T20:12:14Z","messageCount":12,"participants":["Miklos Vajna","Michael Haggerty","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"222267","messageId":"20130701070143.GB17269@suse.cz","threadId":"34313","inReplyTo":null,"subject":"[PATCH] merge: allow using --no-ff and --ff-only at the same time","fromName":"Miklos Vajna","fromEmail":"vmiklos@suse.cz","sentAt":"2013-07-01T07:01:43Z","receivedAt":"2013-07-01T07:01:43Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"1347483 (Teach 'git merge' and 'git pull' the option --ff-only,\n2009-10-29) says this is not allowed, as they contradict each other.\n\nHowever, --ff-only is about asserting the input of the merge, and\n--no-ff is about instructing merge to always create a merge commit, i.e.\nit makes sense to use these options together in some workflow, e.g. when\nbranches are integrated by rebasing then merging, and the maintainer\nwants to be sure the branch is rebased.\n\nSigned-off-by: Miklos Vajna <vmiklos@suse.cz>\n---\n builtin/merge.c  | 12 ++++++++----\n t/t7600-merge.sh | 11 ++++++++---\n 2 files changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 2ebe732..7026ce0 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1162,9 +1162,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\toption_commit = 0;\n \t}\n \n-\tif (!allow_fast_forward && fast_forward_only)\n-\t\tdie(_(\"You cannot combine --no-ff with --ff-only.\"));\n-\n \tif (!abort_current_merge) {\n \t\tif (!argc) {\n \t\t\tif (default_to_upstream)\n@@ -1433,7 +1430,14 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (fast_forward_only)\n+\t/*\n+\t * If --ff-only was used without --no-ff, or: --ff-only and --no-ff was\n+\t * used at the same time, and this is not a fast-forward.\n+\t */\n+\tif (fast_forward_only && (allow_fast_forward || remoteheads->next ||\n+\t\t\t\tcommon->next ||\n+\t\t\t\thashcmp(common->item->object.sha1,\n+\t\t\t\t\thead_commit->object.sha1)))\n \t\tdie(_(\"Not possible to fast-forward, aborting.\"));\n \n \t/* We are going to make a new commit. */\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 460d8eb..bf3d9b2 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -497,9 +497,14 @@ test_expect_success 'combining --squash and --no-ff is refused' '\n \ttest_must_fail git merge --no-ff --squash c1\n '\n \n-test_expect_success 'combining --ff-only and --no-ff is refused' '\n-\ttest_must_fail git merge --ff-only --no-ff c1 &&\n-\ttest_must_fail git merge --no-ff --ff-only c1\n+test_expect_success 'combining --ff-only and --no-ff (ff is possible)' '\n+\tgit reset --hard c0 &&\n+\tgit merge --ff-only --no-ff c1\n+'\n+\n+test_expect_success 'combining --ff-only and --no-ff (ff is not possible)' '\n+\tgit reset --hard c1 &&\n+\ttest_must_fail git merge --ff-only --no-ff c2\n '\n \n test_expect_success 'merge c0 with c1 (ff overrides no-ff)' '\n-- \n1.8.1.4\n"},{"id":"222275","messageId":"51D197AD.1070502@alum.mit.edu","threadId":"34313","inReplyTo":"20130701070143.GB17269@suse.cz","subject":"Re: [PATCH] merge: allow using --no-ff and --ff-only at the same time","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-07-01T14:52:29Z","receivedAt":"2013-07-01T14:52:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/01/2013 09:01 AM, Miklos Vajna wrote:\n> 1347483 (Teach 'git merge' and 'git pull' the option --ff-only,\n> 2009-10-29) says this is not allowed, as they contradict each other.\n> \n> However, --ff-only is about asserting the input of the merge, and\n> --no-ff is about instructing merge to always create a merge commit, i.e.\n> it makes sense to use these options together in some workflow, e.g. when\n> branches are integrated by rebasing then merging, and the maintainer\n> wants to be sure the branch is rebased.\n\nThat is one interpretation of what these options should mean, and I\nagree that it is one way of reading the manpage (which says\n\n--ff-only::\n\tRefuse to merge and exit with a non-zero status unless the\n\tcurrent `HEAD` is already up-to-date or the merge can be\n\tresolved as a fast-forward.\n\n).  However, I don't think that the manpage unambiguously demands this\ninterpretation, and that (more importantly) most users would be very\nsurprised if --ff-only and --no-ff were not opposites.\n\nHow does it hurt?  If I have configuration value merge.ff set to \"only\"\nand run \"git merge --no-ff\" and then I merge a branch that *cannot* be\nfast forwarded, the logic of your patch would require the merge to be\nrejected, no?  But I think it is more plausible to expect that the\ncommand line option takes precedence.\n\nHmmph.  I just tested and found out that (before your patch) a \"--no-ff\"\ncommand-line option does *not* override a \"git config merge.ff only\" but\nrather that the combination provokes the error that you are trying to\nremove,\n\n    fatal: You cannot combine --no-ff with --ff-only.\n\nI find *that* surprising; usually command-line options override\nconfiguration file settings.  OK, it's time for some more exhaustive\ntesting:\n\n   situation      merge.ff  option      result\n   -------------------------------------------------------------------\n1  ff possible    false     --ff        works (ff)\n2  ff impossible  false     --ff        works (non-ff)\n3  ff possible    false     --ff-only   error \"cannot combine options\"\n4  ff impossible  false     --ff-only   error \"cannot combine options\"\n5  ff possible    false     --no-ff     works (non-ff)\n6  ff impossible  false     --no-ff     works (non-ff)\n7  ff possible    only      --ff        works (ff)\n8  ff impossible  only      --ff        error \"not possible to ff\"\n9  ff possible    only      --ff-only   works (ff)\n10 ff impossible  only      --ff-only   error \"not possible to ff\"\n11 ff possible    only      --no-ff     error \"cannot combine options\"\n12 ff impossible  only      --no-ff     error \"cannot combine options\"\n\n>From line 1 we see that \"--ff\" overrides \"merge.ff=false\".\n\n>From lines 3 and 4 we see that \"--ff-only\" cannot be combined with\n\"merge.ff=false\".\n\n>From line 8 we see that \"merge.ff=only\" has its effect despite \"--ff\",\nwhich would normally allow a non-ff merge.\n\n>From lines 11 and 12 we see that \"--no-ff\" cannot be combined with\n\"merge.ff=only\".\n\nI find this inconsistent.  I think it would be more consistent to have\nexactly three states,\n\n* merge.ff unset == --ff == \"do ff if possible, otherwise non-ff\"\n\n* merge.ff=false == --no-ff == \"always create merge commit\"\n\n* merge.ff=only == --ff-only == \"do ff if possible, otherwise fail\"\n\nand for the command-line option to always take precedence over the\nconfig file option.\n\nMoreover, isn't it the usual practice for later command-line options to\ntake precedence over earlier ones?  It is the case for at least one command:\n\n    $ git log --oneline --no-walk --no-decorate --decorate\n    cf71d9b (HEAD, master) 2\n    $ git log --oneline --no-walk --decorate --no-decorate\n    cf71d9b 2\n\nSo I think that command invocations with more than one of {\"--ff\",\n\"--no-ff\", \"--ff-only\"} should respect the last option listed rather\nthan complaining about \"cannot combine options\".\n\nIf I find the time (unlikely) I might submit a patch to implement these\nexpectations.\n\n\nIn my opinion, your use case shouldn't be supported by the command\nbecause (1) it is confusing, (2) it is not very common, and (3) it is\neasy to work around:\n\n    if git merge-base --is-ancestor HEAD $branch\n    then\n        git merge --no-ff $branch\n    else\n        echo \"fatal: Not possible to fast-forward, aborting.\"\n        exit 1\n    fi\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"222276","messageId":"20130701152730.GH17269@suse.cz","threadId":"34313","inReplyTo":"51D197AD.1070502@alum.mit.edu","subject":"Re: [PATCH] merge: allow using --no-ff and --ff-only at the same time","fromName":"Miklos Vajna","fromEmail":"vmiklos@suse.cz","sentAt":"2013-07-01T15:27:30Z","receivedAt":"2013-07-01T15:27:30Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"Hi Michael,\n\nOn Mon, Jul 01, 2013 at 04:52:29PM +0200, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> On 07/01/2013 09:01 AM, Miklos Vajna wrote:\n> > 1347483 (Teach 'git merge' and 'git pull' the option --ff-only,\n> > 2009-10-29) says this is not allowed, as they contradict each other.\n> > \n> > However, --ff-only is about asserting the input of the merge, and\n> > --no-ff is about instructing merge to always create a merge commit, i.e.\n> > it makes sense to use these options together in some workflow, e.g. when\n> > branches are integrated by rebasing then merging, and the maintainer\n> > wants to be sure the branch is rebased.\n> \n> That is one interpretation of what these options should mean, and I\n> agree that it is one way of reading the manpage (which says\n> \n> --ff-only::\n> \tRefuse to merge and exit with a non-zero status unless the\n> \tcurrent `HEAD` is already up-to-date or the merge can be\n> \tresolved as a fast-forward.\n> \n> ).  However, I don't think that the manpage unambiguously demands this\n> interpretation, and that (more importantly) most users would be very\n> surprised if --ff-only and --no-ff were not opposites.\n\nYes, I agree that that's an unfortunate naming. --ff and --no-ff is the\nopposite of each other, however --ff-only is independent, and I would\neven rename it to something like --ff-input-only -- but I don't think\nit's worth to do so, seeing the cost of it (probably these options are\nused by scripts as well).\n\n> How does it hurt?  If I have configuration value merge.ff set to \"only\"\n> and run \"git merge --no-ff\" and then I merge a branch that *cannot* be\n> fast forwarded, the logic of your patch would require the merge to be\n> rejected, no?  But I think it is more plausible to expect that the\n> command line option takes precedence.\n\nHmm, I did not remember that actually merge.ff even uses the same\nconfiguration slot for these switches. :-( Yes, that would make sense to\nfix, once the switches can be combined. Maybe merge.ff and\nmerge.ff-only?\n\n> In my opinion, your use case shouldn't be supported by the command\n> because (1) it is confusing,\n\nI don't see why it would be confusing. I think using these two options\ntogether is one way to try to get the benefits of both rebase (cleaner\nhistory) and merge (keeping the history of which commits came from a\ngiven merge).\n\n> (2) it is not very common,\n\nHard to argue that argument. :-) No idea what counts as common, my\nmotivation is that some projects (e.g. syslog-ng) integrate *every*\nfeature branch this way, and doing this \"manually\" (as in indeed\nmanually or by using a helper script) seems suboptimal, when the support\nfor this is already mostly in merge.c, just disabled.\n\n> easy to work around:\n> \n>     if git merge-base --is-ancestor HEAD $branch\n>     then\n>         git merge --no-ff $branch\n>     else\n>         echo \"fatal: Not possible to fast-forward, aborting.\"\n>         exit 1\n>     fi\n\nRight, that's indeed a viable workaround for the problem.\n\nMiklos\n"},{"id":"222277","messageId":"7vmwq6i93m.fsf@alter.siamese.dyndns.org","threadId":"34313","inReplyTo":"51D197AD.1070502@alum.mit.edu","subject":"Re: [PATCH] merge: allow using --no-ff and --ff-only at the same time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-01T15:38:21Z","receivedAt":"2013-07-01T15:38:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> So I think that command invocations with more than one of {\"--ff\",\n> \"--no-ff\", \"--ff-only\"} should respect the last option listed rather\n> than complaining about \"cannot combine options\".\n>\n> If I find the time (unlikely) I might submit a patch to implement these\n> expectations.\n\nAnd I wouldn't reject it on the basis of the design --- I agree\nfully with your analysis above.  Thanks for digging and spelling out\nhow they should be fixed.\n\nAs to \"--no-ff\" vs \"--ff-only\", \"--ff-only\" has always meant \"only\nfast-forward updates are allowed.  We do not want to create a merge\ncommit with this operation.\"  I do agree with you that the proposed\npatch changes the established semantis and may be too disruptive a\nthing to do at this point.\n\n> In my opinion, your use case shouldn't be supported by the command\n> because (1) it is confusing, (2) it is not very common, and (3) it is\n> easy to work around:\n> ...\n\nIf one were designing Git merge from scratch today, however, I could\nsee one may have designed these as two orthogonal switches.\n\n - Precondition on the shape of histories being merged (\"fail unless\n   fast forward\" does not have to be the only criteria);\n\n - How the update is done (\"fast forward to the other head\", \"always\n   create a merge\", \"fast forward if possible, otherwise merge\" do\n   not have to be the only three choices).\n\nI do not fundamentally oppose to such a new feature, but they have\nto interact sanely with the current \"--ff={only,only,never}\".\n"},{"id":"222278","messageId":"20130701161009.GI17269@suse.cz","threadId":"34313","inReplyTo":"7vmwq6i93m.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] merge: allow using --no-ff and --ff-only at the same time","fromName":"Miklos Vajna","fromEmail":"vmiklos@suse.cz","sentAt":"2013-07-01T16:10:09Z","receivedAt":"2013-07-01T16:10:09Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Mon, Jul 01, 2013 at 08:38:21AM -0700, Junio C Hamano <gitster@pobox.com> wrote:\n> As to \"--no-ff\" vs \"--ff-only\", \"--ff-only\" has always meant \"only\n> fast-forward updates are allowed.  We do not want to create a merge\n> commit with this operation.\"  I do agree with you that the proposed\n> patch changes the established semantis and may be too disruptive a\n> thing to do at this point.\n\nHmm, one way around this may be to add a new option that is basically\nthe same as --no-ff --ff-only (with the patch), except it has a\ndifferent name, so it's not confusing. 'git merge --rebase' could be\nused for this, but such a name is misleading as well. Anyone has a\nbetter naming idea? :-)\n\n> If one were designing Git merge from scratch today, however, I could\n> see one may have designed these as two orthogonal switches.\n> \n>  - Precondition on the shape of histories being merged (\"fail unless\n>    fast forward\" does not have to be the only criteria);\n> \n>  - How the update is done (\"fast forward to the other head\", \"always\n>    create a merge\", \"fast forward if possible, otherwise merge\" do\n>    not have to be the only three choices).\n> \n> I do not fundamentally oppose to such a new feature, but they have\n> to interact sanely with the current \"--ff={only,only,never}\".\n\nOK, so if I get it right, the problem is that users got used to that\nthe --ff-only not only means a precondition for the merge, but also\nmeans \"either don't create a merge commit or fail\", while my patch would\nchange this second behaviour.\n\nI could imagine then new switches, like 'git merge --pre=ff\n--update=no-ff\" could provide these, though I'm not sure if it makes\nsense to add such generic switches till the only user is \"ff\".\n"},{"id":"222284","messageId":"7va9m6i63i.fsf@alter.siamese.dyndns.org","threadId":"34313","inReplyTo":"20130701161009.GI17269@suse.cz","subject":"Re: [PATCH] merge: allow using --no-ff and --ff-only at the same time","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-01T16:43:13Z","receivedAt":"2013-07-01T16:43:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna <vmiklos@suse.cz> writes:\n\n> OK, so if I get it right, the problem is that users got used to\n> that the --ff-only not only means a precondition for the merge,\n> but also means \"either don't create a merge commit or fail\", while\n> my patch would change this second behaviour.\n\nIt is not just \"users got used to\".  \"We do not want to create a\nmerge commit with this operation.\" is what \"--ff-only\" means from\nthe day one [*1*].\n\nFor a merge not to create an extra merge commit, the other history\nhas to be a proper descendant, but that \"precondition\" is a mere\nlogical consequence of the ultimate goal of the mode.\n\n> I could imagine then new switches, like 'git merge --pre=ff\n> --update=no-ff\" could provide these, though I'm not sure if it makes\n> sense to add such generic switches till the only user is \"ff\".\n\nYes, that is why I said \"if one were designing it from scratch, I\ncould see...\" in a very weak form.\n\n\n[Footnote]\n\n*1* 13474835 (Teach 'git merge' and 'git pull' the option --ff-only,\n2009-10-29) and also $gmane/107768 whose documentation part says:\n\n  \"Refuse to merge unless the merge is resolved as a fast-forward.\"\n"},{"id":"222300","messageId":"20130701195407.GK17269@suse.cz","threadId":"34313","inReplyTo":"51D197AD.1070502@alum.mit.edu","subject":"[PATCH] merge: handle --ff/--no-ff/--ff-only as a tri-state option","fromName":"Miklos Vajna","fromEmail":"vmiklos@suse.cz","sentAt":"2013-07-01T19:54:07Z","receivedAt":"2013-07-01T19:54:07Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"This has multiple benefits: with more than one of {\"--ff\", \"--no-ff\",\n\"--ff-only\"} respects the last option; also the command-line option to\nalways take precedence over the config file option.\n\nSigned-off-by: Miklos Vajna <vmiklos@suse.cz>\n---\n\nOn Mon, Jul 01, 2013 at 04:52:29PM +0200, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> If I find the time (unlikely) I might submit a patch to implement these\n> expectations.\n\nSeeing that the --no-ff / --ff-only combo wasn't denied just sort of \naccidently, I agree that it makes more sense to merge allow_fast_forward\nand fast_forward_only to a single enum, that automatically gives you \nboth benefits.\n\n builtin/merge.c  | 65 +++++++++++++++++++++++++++++++++++++-------------------\n t/t7600-merge.sh | 12 ++++++++---\n 2 files changed, 52 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 2ebe732..561edf4 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -47,8 +47,8 @@ static const char * const builtin_merge_usage[] = {\n };\n \n static int show_diffstat = 1, shortlog_len = -1, squash;\n-static int option_commit = 1, allow_fast_forward = 1;\n-static int fast_forward_only, option_edit = -1;\n+static int option_commit = 1;\n+static int option_edit = -1;\n static int allow_trivial = 1, have_message, verify_signatures;\n static int overwrite_ignore = 1;\n static struct strbuf merge_msg = STRBUF_INIT;\n@@ -76,6 +76,14 @@ static struct strategy all_strategy[] = {\n \n static const char *pull_twohead, *pull_octopus;\n \n+enum ff_type {\n+\tFF_ALLOW,\n+\tFF_NO,\n+\tFF_ONLY\n+};\n+\n+static enum ff_type fast_forward = FF_ALLOW;\n+\n static int option_parse_message(const struct option *opt,\n \t\t\t\tconst char *arg, int unset)\n {\n@@ -178,6 +186,21 @@ static int option_parse_n(const struct option *opt,\n \treturn 0;\n }\n \n+static int option_parse_ff(const struct option *opt,\n+\t\t\t  const char *arg, int unset)\n+{\n+\tfast_forward = unset ? FF_NO : FF_ALLOW;\n+\treturn 0;\n+}\n+\n+static int option_parse_ff_only(const struct option *opt,\n+\t\t\t  const char *arg, int unset)\n+{\n+\tif (!unset)\n+\t\tfast_forward = FF_ONLY;\n+\treturn 0;\n+}\n+\n static struct option builtin_merge_options[] = {\n \t{ OPTION_CALLBACK, 'n', NULL, NULL, NULL,\n \t\tN_(\"do not show a diffstat at the end of the merge\"),\n@@ -194,10 +217,12 @@ static struct option builtin_merge_options[] = {\n \t\tN_(\"perform a commit if the merge succeeds (default)\")),\n \tOPT_BOOL('e', \"edit\", &option_edit,\n \t\tN_(\"edit message before committing\")),\n-\tOPT_BOOLEAN(0, \"ff\", &allow_fast_forward,\n-\t\tN_(\"allow fast-forward (default)\")),\n-\tOPT_BOOLEAN(0, \"ff-only\", &fast_forward_only,\n-\t\tN_(\"abort if fast-forward is not possible\")),\n+\t{ OPTION_CALLBACK, 0, \"ff\", NULL, NULL,\n+\t\tN_(\"allow fast-forward (default)\"),\n+\t\tPARSE_OPT_NOARG, option_parse_ff },\n+\t{ OPTION_CALLBACK, 0, \"ff-only\", NULL, NULL,\n+\t\tN_(\"abort if fast-forward is not possible\"),\n+\t\tPARSE_OPT_NOARG, option_parse_ff_only },\n \tOPT_RERERE_AUTOUPDATE(&allow_rerere_auto),\n \tOPT_BOOL(0, \"verify-signatures\", &verify_signatures,\n \t\tN_(\"Verify that the named commit has a valid GPG signature\")),\n@@ -581,10 +606,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)\n \telse if (!strcmp(k, \"merge.ff\")) {\n \t\tint boolval = git_config_maybe_bool(k, v);\n \t\tif (0 <= boolval) {\n-\t\t\tallow_fast_forward = boolval;\n+\t\t\tfast_forward = boolval ? FF_ALLOW : FF_NO;\n \t\t} else if (v && !strcmp(v, \"only\")) {\n-\t\t\tallow_fast_forward = 1;\n-\t\t\tfast_forward_only = 1;\n+\t\t\tfast_forward = FF_ONLY;\n \t\t} /* do not barf on values from future versions of git */\n \t\treturn 0;\n \t} else if (!strcmp(k, \"merge.defaulttoupstream\")) {\n@@ -863,7 +887,7 @@ static int finish_automerge(struct commit *head,\n \n \tfree_commit_list(common);\n \tparents = remoteheads;\n-\tif (!head_subsumed || !allow_fast_forward)\n+\tif (!head_subsumed || fast_forward == FF_NO)\n \t\tcommit_list_insert(head, &parents);\n \tstrbuf_addch(&merge_msg, '\\n');\n \tprepare_to_commit(remoteheads);\n@@ -1008,7 +1032,7 @@ static void write_merge_state(struct commit_list *remoteheads)\n \tif (fd < 0)\n \t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n \tstrbuf_reset(&buf);\n-\tif (!allow_fast_forward)\n+\tif (fast_forward == FF_NO)\n \t\tstrbuf_addf(&buf, \"no-ff\");\n \tif (write_in_full(fd, buf.buf, buf.len) != buf.len)\n \t\tdie_errno(_(\"Could not write to '%s'\"), filename);\n@@ -1157,14 +1181,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tshow_diffstat = 0;\n \n \tif (squash) {\n-\t\tif (!allow_fast_forward)\n+\t\tif (fast_forward == FF_NO)\n \t\t\tdie(_(\"You cannot combine --squash with --no-ff.\"));\n \t\toption_commit = 0;\n \t}\n \n-\tif (!allow_fast_forward && fast_forward_only)\n-\t\tdie(_(\"You cannot combine --no-ff with --ff-only.\"));\n-\n \tif (!abort_current_merge) {\n \t\tif (!argc) {\n \t\t\tif (default_to_upstream)\n@@ -1206,7 +1227,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t\t\"empty head\"));\n \t\tif (squash)\n \t\t\tdie(_(\"Squash commit into empty head not supported yet\"));\n-\t\tif (!allow_fast_forward)\n+\t\tif (fast_forward == FF_NO)\n \t\t\tdie(_(\"Non-fast-forward commit does not make sense into \"\n \t\t\t    \"an empty head\"));\n \t\tremoteheads = collect_parents(head_commit, &head_subsumed, argc, argv);\n@@ -1294,11 +1315,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t    sha1_to_hex(commit->object.sha1));\n \t\tsetenv(buf.buf, merge_remote_util(commit)->name, 1);\n \t\tstrbuf_reset(&buf);\n-\t\tif (!fast_forward_only &&\n+\t\tif (fast_forward != FF_ONLY &&\n \t\t    merge_remote_util(commit) &&\n \t\t    merge_remote_util(commit)->obj &&\n \t\t    merge_remote_util(commit)->obj->type == OBJ_TAG)\n-\t\t\tallow_fast_forward = 0;\n+\t\t\tfast_forward = FF_NO;\n \t}\n \n \tif (option_edit < 0)\n@@ -1315,7 +1336,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \n \tfor (i = 0; i < use_strategies_nr; i++) {\n \t\tif (use_strategies[i]->attr & NO_FAST_FORWARD)\n-\t\t\tallow_fast_forward = 0;\n+\t\t\tfast_forward = FF_NO;\n \t\tif (use_strategies[i]->attr & NO_TRIVIAL)\n \t\t\tallow_trivial = 0;\n \t}\n@@ -1345,7 +1366,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t */\n \t\tfinish_up_to_date(\"Already up-to-date.\");\n \t\tgoto done;\n-\t} else if (allow_fast_forward && !remoteheads->next &&\n+\t} else if (fast_forward != FF_NO && !remoteheads->next &&\n \t\t\t!common->next &&\n \t\t\t!hashcmp(common->item->object.sha1, head_commit->object.sha1)) {\n \t\t/* Again the most common case of merging one remote. */\n@@ -1392,7 +1413,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t * only one common.\n \t\t */\n \t\trefresh_cache(REFRESH_QUIET);\n-\t\tif (allow_trivial && !fast_forward_only) {\n+\t\tif (allow_trivial && fast_forward != FF_ONLY) {\n \t\t\t/* See if it is really trivial. */\n \t\t\tgit_committer_info(IDENT_STRICT);\n \t\t\tprintf(_(\"Trying really trivial in-index merge...\\n\"));\n@@ -1433,7 +1454,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (fast_forward_only)\n+\tif (fast_forward == FF_ONLY)\n \t\tdie(_(\"Not possible to fast-forward, aborting.\"));\n \n \t/* We are going to make a new commit. */\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 460d8eb..3ff5fb8 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -497,9 +497,15 @@ test_expect_success 'combining --squash and --no-ff is refused' '\n \ttest_must_fail git merge --no-ff --squash c1\n '\n \n-test_expect_success 'combining --ff-only and --no-ff is refused' '\n-\ttest_must_fail git merge --ff-only --no-ff c1 &&\n-\ttest_must_fail git merge --no-ff --ff-only c1\n+test_expect_success 'option --ff-only overwrites --no-ff' '\n+\tgit merge --no-ff --ff-only c1 &&\n+\ttest_must_fail git merge --no-ff --ff-only c2\n+'\n+\n+test_expect_success 'option --ff-only overwrites merge.ff=only config' '\n+\tgit reset --hard c0 &&\n+\ttest_config merge.ff only &&\n+\tgit merge --no-ff c1\n '\n \n test_expect_success 'merge c0 with c1 (ff overrides no-ff)' '\n-- \n1.8.1.4\n"},{"id":"222302","messageId":"7vppv2f2ku.fsf@alter.siamese.dyndns.org","threadId":"34313","inReplyTo":"20130701195407.GK17269@suse.cz","subject":"Re: [PATCH] merge: handle --ff/--no-ff/--ff-only as a tri-state option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-01T20:27:29Z","receivedAt":"2013-07-01T20:27:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna <vmiklos@suse.cz> writes:\n\n> On Mon, Jul 01, 2013 at 04:52:29PM +0200, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>> If I find the time (unlikely) I might submit a patch to implement these\n>> expectations.\n>\n> Seeing that the --no-ff / --ff-only combo wasn't denied just sort of \n> accidently, I agree that it makes more sense to merge allow_fast_forward\n> and fast_forward_only to a single enum, that automatically gives you \n> both benefits.\n\nYes, this goes in the right direction.  \"Pick one out of these three\npossibilities\" is how the configuration is done, and the command\nline option parsing should follow suit by consolidating these two\nvariables into one.\n\nThanks, will queue.\n\nI didn't read the patch carefully, though, so review comments are\nvery much appreciated.\n"},{"id":"222351","messageId":"51D2927F.3040207@alum.mit.edu","threadId":"34313","inReplyTo":"20130701195407.GK17269@suse.cz","subject":"Re: [PATCH] merge: handle --ff/--no-ff/--ff-only as a tri-state option","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-07-02T08:42:39Z","receivedAt":"2013-07-02T08:42:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/01/2013 09:54 PM, Miklos Vajna wrote:\n> This has multiple benefits: with more than one of {\"--ff\", \"--no-ff\",\n> \"--ff-only\"} respects the last option; also the command-line option to\n> always take precedence over the config file option.\n> \n> Signed-off-by: Miklos Vajna <vmiklos@suse.cz>\n> ---\n> \n> On Mon, Jul 01, 2013 at 04:52:29PM +0200, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n>> If I find the time (unlikely) I might submit a patch to implement these\n>> expectations.\n> \n> Seeing that the --no-ff / --ff-only combo wasn't denied just sort of \n> accidently, I agree that it makes more sense to merge allow_fast_forward\n> and fast_forward_only to a single enum, that automatically gives you \n> both benefits.\n\nThanks a lot for taking this on!  I would definitely be happy to be able\nto set merge.ff=false without preventing the use of explicit \"--ff-only\"\nfrom the command line.\n\nSee comments below...\n\n>  builtin/merge.c  | 65 +++++++++++++++++++++++++++++++++++++-------------------\n>  t/t7600-merge.sh | 12 ++++++++---\n>  2 files changed, 52 insertions(+), 25 deletions(-)\n> \n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 2ebe732..561edf4 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -47,8 +47,8 @@ static const char * const builtin_merge_usage[] = {\n>  };\n>  \n>  static int show_diffstat = 1, shortlog_len = -1, squash;\n> -static int option_commit = 1, allow_fast_forward = 1;\n> -static int fast_forward_only, option_edit = -1;\n> +static int option_commit = 1;\n> +static int option_edit = -1;\n>  static int allow_trivial = 1, have_message, verify_signatures;\n>  static int overwrite_ignore = 1;\n>  static struct strbuf merge_msg = STRBUF_INIT;\n> @@ -76,6 +76,14 @@ static struct strategy all_strategy[] = {\n>  \n>  static const char *pull_twohead, *pull_octopus;\n>  \n> +enum ff_type {\n> +\tFF_ALLOW,\n> +\tFF_NO,\n> +\tFF_ONLY\n> +};\n> +\n> +static enum ff_type fast_forward = FF_ALLOW;\n> +\n>  static int option_parse_message(const struct option *opt,\n>  \t\t\t\tconst char *arg, int unset)\n>  {\n> @@ -178,6 +186,21 @@ static int option_parse_n(const struct option *opt,\n>  \treturn 0;\n>  }\n>  \n> +static int option_parse_ff(const struct option *opt,\n> +\t\t\t  const char *arg, int unset)\n> +{\n> +\tfast_forward = unset ? FF_NO : FF_ALLOW;\n> +\treturn 0;\n> +}\n> +\n> +static int option_parse_ff_only(const struct option *opt,\n> +\t\t\t  const char *arg, int unset)\n> +{\n> +\tif (!unset)\n> +\t\tfast_forward = FF_ONLY;\n> +\treturn 0;\n> +}\n> +\n\nYou allow --no-ff-only but ignore it, which I think is incorrect.  In\n\n    git merge --ff-only --no-ff-only [...]\n\n, the --no-ff-only should presumably cancel the effect of the previous\n--ff-only (i.e., be equivalent to \"--ff\").  But it is a little bit\nsubtle because\n\n    git merge --no-ff --no-ff-only\n\nshould presumably be equivalent to --no-ff.  So I think that\n\"--no-ff-only\" should do something like\n\n    if (fast_forward == FF_ONLY)\n        fast_forward = FF_ALLOW;\n\n(Note that there is an asymmetry here, because \"--no-ff-only\"\n*shouldn't* cancel the effect of \"--no-ff\", whereas \"--ff\" *should*\ncancel the effect of \"--ff-only\".  This is because --ff-only restricts\nwhat the user wants to allow whereas --ff removes a restriction.  So I\nthink it is OK.)\n\n>  static struct option builtin_merge_options[] = {\n>  \t{ OPTION_CALLBACK, 'n', NULL, NULL, NULL,\n>  \t\tN_(\"do not show a diffstat at the end of the merge\"),\n> @@ -194,10 +217,12 @@ static struct option builtin_merge_options[] = {\n>  \t\tN_(\"perform a commit if the merge succeeds (default)\")),\n>  \tOPT_BOOL('e', \"edit\", &option_edit,\n>  \t\tN_(\"edit message before committing\")),\n> -\tOPT_BOOLEAN(0, \"ff\", &allow_fast_forward,\n> -\t\tN_(\"allow fast-forward (default)\")),\n> -\tOPT_BOOLEAN(0, \"ff-only\", &fast_forward_only,\n> -\t\tN_(\"abort if fast-forward is not possible\")),\n> +\t{ OPTION_CALLBACK, 0, \"ff\", NULL, NULL,\n> +\t\tN_(\"allow fast-forward (default)\"),\n> +\t\tPARSE_OPT_NOARG, option_parse_ff },\n> +\t{ OPTION_CALLBACK, 0, \"ff-only\", NULL, NULL,\n> +\t\tN_(\"abort if fast-forward is not possible\"),\n> +\t\tPARSE_OPT_NOARG, option_parse_ff_only },\n>  \tOPT_RERERE_AUTOUPDATE(&allow_rerere_auto),\n>  \tOPT_BOOL(0, \"verify-signatures\", &verify_signatures,\n>  \t\tN_(\"Verify that the named commit has a valid GPG signature\")),\n\nI'm no options guru, but I think it would be possible to implement --ff\nand --no-ff without callbacks if you choose constants such that\nFF_NO==0, something like:\n\n    enum ff_type {\n    \tFF_NO = 0, /* It is important that this value be zero! */\n    \tFF_ALLOW,\n    \tFF_ONLY\n    };\n\n    static int fast_forward = FF_ALLOW;\n\n    static struct option builtin_merge_options[] = {\n        [...]\n        { OPTION_SET_INT, 0, \"ff\", &fast_forward, NULL,\n        \tN_(\"allow fast-forward (default)\"),\n        \tPARSE_OPT_NOARG, NULL, FF_ALLOW },\n        { OPTION_CALLBACK, 0, \"ff-only\", [...]\n\nbecause OPTION_SET_INT resets the value to zero if \"--no-ff\" is\nspecified, which is just what we need.\n\n> @@ -581,10 +606,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)\n>  \telse if (!strcmp(k, \"merge.ff\")) {\n>  \t\tint boolval = git_config_maybe_bool(k, v);\n>  \t\tif (0 <= boolval) {\n> -\t\t\tallow_fast_forward = boolval;\n> +\t\t\tfast_forward = boolval ? FF_ALLOW : FF_NO;\n>  \t\t} else if (v && !strcmp(v, \"only\")) {\n> -\t\t\tallow_fast_forward = 1;\n> -\t\t\tfast_forward_only = 1;\n> +\t\t\tfast_forward = FF_ONLY;\n>  \t\t} /* do not barf on values from future versions of git */\n>  \t\treturn 0;\n>  \t} else if (!strcmp(k, \"merge.defaulttoupstream\")) {\n> @@ -863,7 +887,7 @@ static int finish_automerge(struct commit *head,\n>  \n>  \tfree_commit_list(common);\n>  \tparents = remoteheads;\n> -\tif (!head_subsumed || !allow_fast_forward)\n> +\tif (!head_subsumed || fast_forward == FF_NO)\n>  \t\tcommit_list_insert(head, &parents);\n>  \tstrbuf_addch(&merge_msg, '\\n');\n>  \tprepare_to_commit(remoteheads);\n> @@ -1008,7 +1032,7 @@ static void write_merge_state(struct commit_list *remoteheads)\n>  \tif (fd < 0)\n>  \t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n>  \tstrbuf_reset(&buf);\n> -\tif (!allow_fast_forward)\n> +\tif (fast_forward == FF_NO)\n>  \t\tstrbuf_addf(&buf, \"no-ff\");\n>  \tif (write_in_full(fd, buf.buf, buf.len) != buf.len)\n>  \t\tdie_errno(_(\"Could not write to '%s'\"), filename);\n> @@ -1157,14 +1181,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t\tshow_diffstat = 0;\n>  \n>  \tif (squash) {\n> -\t\tif (!allow_fast_forward)\n> +\t\tif (fast_forward == FF_NO)\n>  \t\t\tdie(_(\"You cannot combine --squash with --no-ff.\"));\n>  \t\toption_commit = 0;\n>  \t}\n>  \n\nSo there is still a problem with setting merge.ff=false, namely that it\nprevents the use of --squash.  That's not good.  (I realize that you are\nnot to blame for this pre-existing behavior.)\n\nHow should --squash and the ff-related options interact?\n\n    git merge --ff --squash\n    git merge --no-ff --squash\n\nI think these should just squash.\n\n    git merge --ff-only --squash\n\nI think this should definitely squash.  But perhaps it should require\nthat HEAD be an ancestor of the branch to be merged?\n\n    git merge --squash --ff\n    git merge --squash --no-ff\n    git merge --squash --ff-only\n\nShould these do the same as the versions with the option order reversed?\n Or should the command line option that appears later take precedence?\nThe latter implies that {--ff, --no-ff, --ff-only, --squash} actually\nconstitute a single *quad-state* option, representing \"how the results\nof the merge should be handled\", and, for example,\n\n    git merge --squash --ff-only\n\nignores the --squash option, and\n\n    git merge --ff-only --squash\n\nignores the --ff-only option.\n\nI'm not sure.\n\n> -\tif (!allow_fast_forward && fast_forward_only)\n> -\t\tdie(_(\"You cannot combine --no-ff with --ff-only.\"));\n> -\n>  \tif (!abort_current_merge) {\n>  \t\tif (!argc) {\n>  \t\t\tif (default_to_upstream)\n> @@ -1206,7 +1227,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t\t\t\t\"empty head\"));\n>  \t\tif (squash)\n>  \t\t\tdie(_(\"Squash commit into empty head not supported yet\"));\n> -\t\tif (!allow_fast_forward)\n> +\t\tif (fast_forward == FF_NO)\n>  \t\t\tdie(_(\"Non-fast-forward commit does not make sense into \"\n>  \t\t\t    \"an empty head\"));\n>  \t\tremoteheads = collect_parents(head_commit, &head_subsumed, argc, argv);\n> @@ -1294,11 +1315,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t\t\t    sha1_to_hex(commit->object.sha1));\n>  \t\tsetenv(buf.buf, merge_remote_util(commit)->name, 1);\n>  \t\tstrbuf_reset(&buf);\n> -\t\tif (!fast_forward_only &&\n> +\t\tif (fast_forward != FF_ONLY &&\n>  \t\t    merge_remote_util(commit) &&\n>  \t\t    merge_remote_util(commit)->obj &&\n>  \t\t    merge_remote_util(commit)->obj->type == OBJ_TAG)\n> -\t\t\tallow_fast_forward = 0;\n> +\t\t\tfast_forward = FF_NO;\n>  \t}\n>  \n>  \tif (option_edit < 0)\n> @@ -1315,7 +1336,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \n>  \tfor (i = 0; i < use_strategies_nr; i++) {\n>  \t\tif (use_strategies[i]->attr & NO_FAST_FORWARD)\n> -\t\t\tallow_fast_forward = 0;\n> +\t\t\tfast_forward = FF_NO;\n>  \t\tif (use_strategies[i]->attr & NO_TRIVIAL)\n>  \t\t\tallow_trivial = 0;\n>  \t}\n> @@ -1345,7 +1366,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t\t */\n>  \t\tfinish_up_to_date(\"Already up-to-date.\");\n>  \t\tgoto done;\n> -\t} else if (allow_fast_forward && !remoteheads->next &&\n> +\t} else if (fast_forward != FF_NO && !remoteheads->next &&\n>  \t\t\t!common->next &&\n>  \t\t\t!hashcmp(common->item->object.sha1, head_commit->object.sha1)) {\n>  \t\t/* Again the most common case of merging one remote. */\n> @@ -1392,7 +1413,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t\t * only one common.\n>  \t\t */\n>  \t\trefresh_cache(REFRESH_QUIET);\n> -\t\tif (allow_trivial && !fast_forward_only) {\n> +\t\tif (allow_trivial && fast_forward != FF_ONLY) {\n>  \t\t\t/* See if it is really trivial. */\n>  \t\t\tgit_committer_info(IDENT_STRICT);\n>  \t\t\tprintf(_(\"Trying really trivial in-index merge...\\n\"));\n> @@ -1433,7 +1454,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t\t}\n>  \t}\n>  \n> -\tif (fast_forward_only)\n> +\tif (fast_forward == FF_ONLY)\n>  \t\tdie(_(\"Not possible to fast-forward, aborting.\"));\n>  \n>  \t/* We are going to make a new commit. */\n> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\n> index 460d8eb..3ff5fb8 100755\n> --- a/t/t7600-merge.sh\n> +++ b/t/t7600-merge.sh\n> @@ -497,9 +497,15 @@ test_expect_success 'combining --squash and --no-ff is refused' '\n>  \ttest_must_fail git merge --no-ff --squash c1\n>  '\n>  \n> -test_expect_success 'combining --ff-only and --no-ff is refused' '\n> -\ttest_must_fail git merge --ff-only --no-ff c1 &&\n> -\ttest_must_fail git merge --no-ff --ff-only c1\n> +test_expect_success 'option --ff-only overwrites --no-ff' '\n> +\tgit merge --no-ff --ff-only c1 &&\n> +\ttest_must_fail git merge --no-ff --ff-only c2\n> +'\n> +\n> +test_expect_success 'option --ff-only overwrites merge.ff=only config' '\n> +\tgit reset --hard c0 &&\n> +\ttest_config merge.ff only &&\n> +\tgit merge --no-ff c1\n>  '\n>  \n>  test_expect_success 'merge c0 with c1 (ff overrides no-ff)' '\n> \n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"222370","messageId":"20130702144757.GG5317@suse.cz","threadId":"34313","inReplyTo":"51D2927F.3040207@alum.mit.edu","subject":"[PATCH v2] merge: handle --ff/--no-ff/--ff-only as a tri-state option","fromName":"Miklos Vajna","fromEmail":"vmiklos@suse.cz","sentAt":"2013-07-02T14:47:57Z","receivedAt":"2013-07-02T14:47:57Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"This has multiple benefits: with more than one of {\"--ff\", \"--no-ff\",\n\"--ff-only\"} respects the last option; also the command-line option to\nalways take precedence over the config file option.\n\nSigned-off-by: Miklos Vajna <vmiklos@suse.cz>\n---\n builtin/merge.c  | 55 +++++++++++++++++++++++++++++++++----------------------\n t/t7600-merge.sh | 12 +++++++++---\n 2 files changed, 42 insertions(+), 25 deletions(-)\n\nOn Tue, Jul 02, 2013 at 10:42:39AM +0200, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> You allow --no-ff-only but ignore it, which I think is incorrect.  In\n> \n>     git merge --ff-only --no-ff-only [...]\n> \n> , the --no-ff-only should presumably cancel the effect of the previous\n> --ff-only (i.e., be equivalent to \"--ff\").  But it is a little bit\n> subtle because\n> \n>     git merge --no-ff --no-ff-only\n> \n> should presumably be equivalent to --no-ff.  So I think that\n> \"--no-ff-only\" should do something like\n> \n>     if (fast_forward == FF_ONLY)\n>         fast_forward = FF_ALLOW;\n\nDo we really want --no-ff-only? I would rather just disable it, see the\nupdated patch.\n\n> I'm no options guru, but I think it would be possible to implement --ff\n> and --no-ff without callbacks if you choose constants such that\n> FF_NO==0, something like:\n\nIndeed, done.\n\n> Should these do the same as the versions with the option order reversed?\n>  Or should the command line option that appears later take precedence?\n> The latter implies that {--ff, --no-ff, --ff-only, --squash} actually\n> constitute a single *quad-state* option, representing \"how the results\n> of the merge should be handled\", and, for example,\n> \n>     git merge --squash --ff-only\n> \n> ignores the --squash option, and\n> \n>     git merge --ff-only --squash\n> \n> ignores the --ff-only option.\n> \n> I'm not sure.\n\nActually there is also --no-squash, used by e.g. git-pull internally.\nYou definitely don't want a five-state option. :-) So for now I would\nrather let --squash/--no-squash alone.\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 2ebe732..149f32a 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -47,8 +47,8 @@ static const char * const builtin_merge_usage[] = {\n };\n \n static int show_diffstat = 1, shortlog_len = -1, squash;\n-static int option_commit = 1, allow_fast_forward = 1;\n-static int fast_forward_only, option_edit = -1;\n+static int option_commit = 1;\n+static int option_edit = -1;\n static int allow_trivial = 1, have_message, verify_signatures;\n static int overwrite_ignore = 1;\n static struct strbuf merge_msg = STRBUF_INIT;\n@@ -76,6 +76,14 @@ static struct strategy all_strategy[] = {\n \n static const char *pull_twohead, *pull_octopus;\n \n+enum ff_type {\n+\tFF_NO,\n+\tFF_ALLOW,\n+\tFF_ONLY\n+};\n+\n+static enum ff_type fast_forward = FF_ALLOW;\n+\n static int option_parse_message(const struct option *opt,\n \t\t\t\tconst char *arg, int unset)\n {\n@@ -178,6 +186,13 @@ static int option_parse_n(const struct option *opt,\n \treturn 0;\n }\n \n+static int option_parse_ff_only(const struct option *opt,\n+\t\t\t  const char *arg, int unset)\n+{\n+\tfast_forward = FF_ONLY;\n+\treturn 0;\n+}\n+\n static struct option builtin_merge_options[] = {\n \t{ OPTION_CALLBACK, 'n', NULL, NULL, NULL,\n \t\tN_(\"do not show a diffstat at the end of the merge\"),\n@@ -194,10 +209,10 @@ static struct option builtin_merge_options[] = {\n \t\tN_(\"perform a commit if the merge succeeds (default)\")),\n \tOPT_BOOL('e', \"edit\", &option_edit,\n \t\tN_(\"edit message before committing\")),\n-\tOPT_BOOLEAN(0, \"ff\", &allow_fast_forward,\n-\t\tN_(\"allow fast-forward (default)\")),\n-\tOPT_BOOLEAN(0, \"ff-only\", &fast_forward_only,\n-\t\tN_(\"abort if fast-forward is not possible\")),\n+\tOPT_SET_INT(0, \"ff\", &fast_forward, N_(\"allow fast-forward (default)\"), FF_ALLOW),\n+\t{ OPTION_CALLBACK, 0, \"ff-only\", NULL, NULL,\n+\t\tN_(\"abort if fast-forward is not possible\"),\n+\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, option_parse_ff_only },\n \tOPT_RERERE_AUTOUPDATE(&allow_rerere_auto),\n \tOPT_BOOL(0, \"verify-signatures\", &verify_signatures,\n \t\tN_(\"Verify that the named commit has a valid GPG signature\")),\n@@ -581,10 +596,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)\n \telse if (!strcmp(k, \"merge.ff\")) {\n \t\tint boolval = git_config_maybe_bool(k, v);\n \t\tif (0 <= boolval) {\n-\t\t\tallow_fast_forward = boolval;\n+\t\t\tfast_forward = boolval ? FF_ALLOW : FF_NO;\n \t\t} else if (v && !strcmp(v, \"only\")) {\n-\t\t\tallow_fast_forward = 1;\n-\t\t\tfast_forward_only = 1;\n+\t\t\tfast_forward = FF_ONLY;\n \t\t} /* do not barf on values from future versions of git */\n \t\treturn 0;\n \t} else if (!strcmp(k, \"merge.defaulttoupstream\")) {\n@@ -863,7 +877,7 @@ static int finish_automerge(struct commit *head,\n \n \tfree_commit_list(common);\n \tparents = remoteheads;\n-\tif (!head_subsumed || !allow_fast_forward)\n+\tif (!head_subsumed || fast_forward == FF_NO)\n \t\tcommit_list_insert(head, &parents);\n \tstrbuf_addch(&merge_msg, '\\n');\n \tprepare_to_commit(remoteheads);\n@@ -1008,7 +1022,7 @@ static void write_merge_state(struct commit_list *remoteheads)\n \tif (fd < 0)\n \t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n \tstrbuf_reset(&buf);\n-\tif (!allow_fast_forward)\n+\tif (fast_forward == FF_NO)\n \t\tstrbuf_addf(&buf, \"no-ff\");\n \tif (write_in_full(fd, buf.buf, buf.len) != buf.len)\n \t\tdie_errno(_(\"Could not write to '%s'\"), filename);\n@@ -1157,14 +1171,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tshow_diffstat = 0;\n \n \tif (squash) {\n-\t\tif (!allow_fast_forward)\n+\t\tif (fast_forward == FF_NO)\n \t\t\tdie(_(\"You cannot combine --squash with --no-ff.\"));\n \t\toption_commit = 0;\n \t}\n \n-\tif (!allow_fast_forward && fast_forward_only)\n-\t\tdie(_(\"You cannot combine --no-ff with --ff-only.\"));\n-\n \tif (!abort_current_merge) {\n \t\tif (!argc) {\n \t\t\tif (default_to_upstream)\n@@ -1206,7 +1217,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t\t\"empty head\"));\n \t\tif (squash)\n \t\t\tdie(_(\"Squash commit into empty head not supported yet\"));\n-\t\tif (!allow_fast_forward)\n+\t\tif (fast_forward == FF_NO)\n \t\t\tdie(_(\"Non-fast-forward commit does not make sense into \"\n \t\t\t    \"an empty head\"));\n \t\tremoteheads = collect_parents(head_commit, &head_subsumed, argc, argv);\n@@ -1294,11 +1305,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t    sha1_to_hex(commit->object.sha1));\n \t\tsetenv(buf.buf, merge_remote_util(commit)->name, 1);\n \t\tstrbuf_reset(&buf);\n-\t\tif (!fast_forward_only &&\n+\t\tif (fast_forward != FF_ONLY &&\n \t\t    merge_remote_util(commit) &&\n \t\t    merge_remote_util(commit)->obj &&\n \t\t    merge_remote_util(commit)->obj->type == OBJ_TAG)\n-\t\t\tallow_fast_forward = 0;\n+\t\t\tfast_forward = FF_NO;\n \t}\n \n \tif (option_edit < 0)\n@@ -1315,7 +1326,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \n \tfor (i = 0; i < use_strategies_nr; i++) {\n \t\tif (use_strategies[i]->attr & NO_FAST_FORWARD)\n-\t\t\tallow_fast_forward = 0;\n+\t\t\tfast_forward = FF_NO;\n \t\tif (use_strategies[i]->attr & NO_TRIVIAL)\n \t\t\tallow_trivial = 0;\n \t}\n@@ -1345,7 +1356,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t */\n \t\tfinish_up_to_date(\"Already up-to-date.\");\n \t\tgoto done;\n-\t} else if (allow_fast_forward && !remoteheads->next &&\n+\t} else if (fast_forward != FF_NO && !remoteheads->next &&\n \t\t\t!common->next &&\n \t\t\t!hashcmp(common->item->object.sha1, head_commit->object.sha1)) {\n \t\t/* Again the most common case of merging one remote. */\n@@ -1392,7 +1403,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t * only one common.\n \t\t */\n \t\trefresh_cache(REFRESH_QUIET);\n-\t\tif (allow_trivial && !fast_forward_only) {\n+\t\tif (allow_trivial && fast_forward != FF_ONLY) {\n \t\t\t/* See if it is really trivial. */\n \t\t\tgit_committer_info(IDENT_STRICT);\n \t\t\tprintf(_(\"Trying really trivial in-index merge...\\n\"));\n@@ -1433,7 +1444,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (fast_forward_only)\n+\tif (fast_forward == FF_ONLY)\n \t\tdie(_(\"Not possible to fast-forward, aborting.\"));\n \n \t/* We are going to make a new commit. */\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 460d8eb..3ff5fb8 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -497,9 +497,15 @@ test_expect_success 'combining --squash and --no-ff is refused' '\n \ttest_must_fail git merge --no-ff --squash c1\n '\n \n-test_expect_success 'combining --ff-only and --no-ff is refused' '\n-\ttest_must_fail git merge --ff-only --no-ff c1 &&\n-\ttest_must_fail git merge --no-ff --ff-only c1\n+test_expect_success 'option --ff-only overwrites --no-ff' '\n+\tgit merge --no-ff --ff-only c1 &&\n+\ttest_must_fail git merge --no-ff --ff-only c2\n+'\n+\n+test_expect_success 'option --ff-only overwrites merge.ff=only config' '\n+\tgit reset --hard c0 &&\n+\ttest_config merge.ff only &&\n+\tgit merge --no-ff c1\n '\n \n test_expect_success 'merge c0 with c1 (ff overrides no-ff)' '\n-- \n1.8.1.4\n"},{"id":"222388","messageId":"7vfvvw94v4.fsf@alter.siamese.dyndns.org","threadId":"34313","inReplyTo":"51D2927F.3040207@alum.mit.edu","subject":"Re: [PATCH] merge: handle --ff/--no-ff/--ff-only as a tri-state option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-02T18:46:55Z","receivedAt":"2013-07-02T18:46:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> You allow --no-ff-only but ignore it, which I think is incorrect.  In\n>\n>     git merge --ff-only --no-ff-only [...]\n>\n> , the --no-ff-only should presumably cancel the effect of the previous\n> --ff-only (i.e., be equivalent to \"--ff\").\n\nIdeally, if we were starting from scratch and living in the \"you\npick one out of three\" world, we should forbid \"--no-ff-only\".  The\n\"--no-ff\" option is spelled as if it is a negation of \"--ff\", and it\ndid start as such before \"--ff-only\" was introduced, but \"--no-ff\"\nwould have been better named \"--always-create-merge\", which is what\nthe option really means.\n\nAnd in the tristate world, with mutually exclusive \"--A\", \"--B\", and\n\"--C\" options, \"--no-C\" does not mean \"I want to do A\" at all.\n\nIf the existing code had allowed with \"--no-ff-only\" to defeat\nconfigured merge.ff=only from the command line, then there may have\nbeen users who are used to that behaviour, and we cannot break them,\nbut luckily or unluckily it does not work, so...\n\n>> @@ -1157,14 +1181,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>>  \t\tshow_diffstat = 0;\n>>  \n>>  \tif (squash) {\n>> -\t\tif (!allow_fast_forward)\n>> +\t\tif (fast_forward == FF_NO)\n>>  \t\t\tdie(_(\"You cannot combine --squash with --no-ff.\"));\n>>  \t\toption_commit = 0;\n>>  \t}\n>>  \n>\n> So there is still a problem with setting merge.ff=false, namely that it\n> prevents the use of --squash.  That's not good.  (I realize that you are\n> not to blame for this pre-existing behavior.)\n>\n> How should --squash and the ff-related options interact?\n\nInteresting point.\n\n>     git merge --ff --squash\n>     git merge --no-ff --squash\n>\n> I think these should just squash.\n>\n>     git merge --ff-only --squash\n>\n> I think this should definitely squash.  But perhaps it should require\n> that HEAD be an ancestor of the branch to be merged?\n>\n>     git merge --squash --ff\n>     git merge --squash --no-ff\n>     git merge --squash --ff-only\n>\n> Should these do the same as the versions with the option order reversed?\n\nAs \"--squash\" is about _not_ moving the head but only updating the\nworking tree and the index, I personally think it should be treated\nas an error if any of these \"ff\" options is explicitly given from\nthe command line.\n"},{"id":"222398","messageId":"7vsizwk9gh.fsf@alter.siamese.dyndns.org","threadId":"34313","inReplyTo":"20130702144757.GG5317@suse.cz","subject":"Re: [PATCH v2] merge: handle --ff/--no-ff/--ff-only as a tri-state option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-02T20:12:14Z","receivedAt":"2013-07-02T20:12:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna <vmiklos@suse.cz> writes:\n\n>>     if (fast_forward == FF_ONLY)\n>>         fast_forward = FF_ALLOW;\n>\n> Do we really want --no-ff-only? I would rather just disable it, see the\n> updated patch.\n\nSounds sane.\n\n>> I'm no options guru, but I think it would be possible to implement --ff\n>> and --no-ff without callbacks if you choose constants such that\n>> FF_NO==0, something like:\n>\n> Indeed, done.\n\nYup, looks good.\n\n> Actually there is also --no-squash, used by e.g. git-pull internally.\n> You definitely don't want a five-state option. :-) So for now I would\n> rather let --squash/--no-squash alone.\n\nSensible for this patch.\n\nWill replace what was queued.  Thanks.\n"}]}