{"thread":{"id":"21410","subject":"[PATCH] Teach 'git merge' and 'git pull' the option --ff-only","startedAt":"2009-10-28T22:15:29Z","lastAt":"2009-10-29T06:23:22Z","messageCount":3,"participants":["Björn Gustavsson","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"126182","messageId":"4AE8C281.50104@gmail.com","threadId":"21410","inReplyTo":null,"subject":"[PATCH] Teach 'git merge' and 'git pull' the option --ff-only","fromName":"Björn Gustavsson","fromEmail":"bgustavsson@gmail.com","sentAt":"2009-10-28T22:15:29Z","receivedAt":"2009-10-28T22:15:29Z","isPatch":true,"sender":{"key":"bgustavsson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/74840?v=4"},"body":"For convenience in scripts and aliases, add the option\n--ff-only to only allow fast-forwards.\n\nAcknowledgements: I did look at Yuval Kogman's earlier\npatch (107768 in gmane), mainly as shortcut to find my\nway in the code, but I did not copy anything directly.\n\nSigned-off-by: Björn Gustavsson <bgustavsson@gmail.com>\n---\nWhen I started to use git earlier this year, I was suprised\nthat there was a --no-ff option but no --ff-only option.\n\nI saw in the mailing list archive at gmane that there has\nbeen two previous attempts to implement --ff-only. The first\npatch was made to the git-merge.sh script (before\nbuiltin-merge.c was created). As far as I understand it,\nthe author of the patch said he would send some corrections\nof the patch, but he never did, and nothing more happened.\n\nSo here is my patch, the third attempt.\n\n Documentation/merge-options.txt |    4 ++++\n builtin-merge.c                 |   16 ++++++++++++++--\n git-pull.sh                     |    7 +++++--\n t/t7600-merge.sh                |   29 +++++++++++++++++++++++++++++\n 4 files changed, 52 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex adadf8e..fbf8976 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -60,6 +60,10 @@\n \ta fast-forward, only update the branch pointer. This is\n \tthe default behavior of git-merge.\n \n+--ff-only::\n+\tRefuse to merge unless the merge can be resolved as a\n+\tfast-forward.\n+\n -s <strategy>::\n --strategy=<strategy>::\n \tUse the given merge strategy; can be supplied more than\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex b6b8428..298adfb 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -43,6 +43,7 @@ static const char * const builtin_merge_usage[] = {\n \n static int show_diffstat = 1, option_log, squash;\n static int option_commit = 1, allow_fast_forward = 1;\n+static int fast_forward_only;\n static int allow_trivial = 1, have_message;\n static struct strbuf merge_msg;\n static struct commit_list *remoteheads;\n@@ -167,6 +168,8 @@ static struct option builtin_merge_options[] = {\n \t\t\"perform a commit if the merge succeeds (default)\"),\n \tOPT_BOOLEAN(0, \"ff\", &allow_fast_forward,\n \t\t\"allow fast forward (default)\"),\n+\tOPT_BOOLEAN(0, \"ff-only\", &fast_forward_only,\n+\t\t\"abort if fast forward is not possible\"),\n \tOPT_CALLBACK('s', \"strategy\", &use_strategies, \"strategy\",\n \t\t\"merge strategy to use\", option_parse_strategy),\n \tOPT_CALLBACK('m', \"message\", &merge_msg, \"message\",\n@@ -874,6 +877,9 @@ 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 (!argc)\n \t\tusage_with_options(builtin_merge_usage,\n \t\t\tbuiltin_merge_options);\n@@ -969,8 +975,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t}\n \n \tfor (i = 0; i < use_strategies_nr; i++) {\n-\t\tif (use_strategies[i]->attr & NO_FAST_FORWARD)\n+\t\tif (use_strategies[i]->attr & NO_FAST_FORWARD) {\n \t\t\tallow_fast_forward = 0;\n+\t\t\tif (fast_forward_only)\n+\t\t\t\tdie(\"You cannot combine --ff-only with the merge strategy '%s'.\", use_strategies[i]->name);\n+\t\t}\n \t\tif (use_strategies[i]->attr & NO_TRIVIAL)\n \t\t\tallow_trivial = 0;\n \t}\n@@ -1040,7 +1049,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) {\n+\t\tif (allow_trivial && !fast_forward_only) {\n \t\t\t/* See if it is really trivial. */\n \t\t\tgit_committer_info(IDENT_ERROR_ON_NO_NAME);\n \t\t\tprintf(\"Trying really trivial in-index merge...\\n\");\n@@ -1079,6 +1088,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tif (fast_forward_only)\n+\t\tdie(\"Not possible to fast forward, aborting.\");\n+\n \t/* We are going to make a new commit. */\n \tgit_committer_info(IDENT_ERROR_ON_NO_NAME);\n \ndiff --git a/git-pull.sh b/git-pull.sh\nindex fc78592..37f3d93 100755\n--- a/git-pull.sh\n+++ b/git-pull.sh\n@@ -16,7 +16,8 @@ cd_to_toplevel\n test -z \"$(git ls-files -u)\" ||\n \tdie \"You are in the middle of a conflicted merge.\"\n \n-strategy_args= diffstat= no_commit= squash= no_ff= log_arg= verbosity=\n+strategy_args= diffstat= no_commit= squash= no_ff= ff_only=\n+log_arg= verbosity=\n curr_branch=$(git symbolic-ref -q HEAD)\n curr_branch_short=$(echo \"$curr_branch\" | sed \"s|refs/heads/||\")\n rebase=$(git config --bool branch.$curr_branch_short.rebase)\n@@ -45,6 +46,8 @@ do\n \t\tno_ff=--ff ;;\n \t--no-ff)\n \t\tno_ff=--no-ff ;;\n+\t--ff-only)\n+\t\tff_only=--ff-only ;;\n \t-s=*|--s=*|--st=*|--str=*|--stra=*|--strat=*|--strate=*|\\\n \t\t--strateg=*|--strategy=*|\\\n \t-s|--s|--st|--str|--stra|--strat|--strate|--strateg|--strategy)\n@@ -215,5 +218,5 @@ merge_name=$(git fmt-merge-msg $log_arg <\"$GIT_DIR/FETCH_HEAD\") || exit\n test true = \"$rebase\" &&\n \texec git-rebase $diffstat $strategy_args --onto $merge_head \\\n \t${oldremoteref:-$merge_head}\n-exec git-merge $diffstat $no_commit $squash $no_ff $log_arg $strategy_args \\\n+exec git-merge $diffstat $no_commit $squash $no_ff $ff_only $log_arg $strategy_args \\\n \t\"$merge_name\" HEAD $merge_head $verbosity\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex e5b210b..d696ea9 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -243,6 +243,16 @@ test_expect_success 'merge c0 with c1' '\n \n test_debug 'gitk --all'\n \n+test_expect_success 'merge c0 with c1 with --ff-only' '\n+\tgit reset --hard c0 &&\n+\tgit merge --ff-only c1 &&\n+\tgit merge --ff-only HEAD c0 c1 &&\n+\tverify_merge file result.1 &&\n+\tverify_head \"$c1\"\n+'\n+\n+test_debug 'gitk --all'\n+\n test_expect_success 'merge c1 with c2' '\n \tgit reset --hard c1 &&\n \ttest_tick &&\n@@ -263,6 +273,14 @@ test_expect_success 'merge c1 with c2 and c3' '\n \n test_debug 'gitk --all'\n \n+test_expect_success 'failing merges with --ff-only' '\n+\tgit reset --hard c1 &&\n+\ttest_tick &&\n+\ttest_must_fail git merge --ff-only c2 &&\n+\ttest_must_fail git merge --ff-only c3 &&\n+\ttest_must_fail git merge --ff-only c2 c3\n+'\n+\n test_expect_success 'merge c0 with c1 (no-commit)' '\n \tgit reset --hard c0 &&\n \tgit merge --no-commit c1 &&\n@@ -432,6 +450,17 @@ 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+'\n+\n+test_expect_success 'combining --ff-only with certain merge strategies is refused' '\n+\tgit reset --hard c0 &&\n+\ttest_must_fail git merge --ff-only --strategy=ours c1 &&\n+\ttest_must_fail git merge --ff-only --strategy=subtree c1\n+'\n+\n test_expect_success 'merge c0 with c1 (ff overrides no-ff)' '\n \tgit reset --hard c0 &&\n \tgit config branch.master.mergeoptions \"--no-ff\" &&\n-- \n1.6.5.1.69.g36942\n"},{"id":"126185","messageId":"7vk4yfi1dd.fsf@alter.siamese.dyndns.org","threadId":"21410","inReplyTo":"4AE8C281.50104@gmail.com","subject":"Re: [PATCH] Teach 'git merge' and 'git pull' the option --ff-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-28T22:45:34Z","receivedAt":"2009-10-28T22:45:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Björn Gustavsson <bgustavsson@gmail.com> writes:\n\n> For convenience in scripts and aliases, add the option\n> --ff-only to only allow fast-forwards.\n>\n> Acknowledgements: I did look at Yuval Kogman's earlier\n> patch (107768 in gmane), mainly as shortcut to find my\n> way in the code, but I did not copy anything directly.\n>\n> Signed-off-by: Björn Gustavsson <bgustavsson@gmail.com>\n\nThanks.  I think you covered the points in the old discussion thread.\n\n> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\n> index adadf8e..fbf8976 100644\n> --- a/Documentation/merge-options.txt\n> +++ b/Documentation/merge-options.txt\n> @@ -60,6 +60,10 @@\n>  \ta fast-forward, only update the branch pointer. This is\n>  \tthe default behavior of git-merge.\n>  \n> +--ff-only::\n> +\tRefuse to merge unless the merge can be resolved as a\n> +\tfast-forward.\n\nDo you or do you not allow \"already up to date\"?  I think it makes sense\nto allow it, but it is unclear from these two lines.\n\n> @@ -874,6 +877,9 @@ 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\nAre these the only nonsensical combinations?  How should this interact\nwith other options, e.g. --squash or --message?\n\n> @@ -969,8 +975,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t}\n>  \n>  \tfor (i = 0; i < use_strategies_nr; i++) {\n> -\t\tif (use_strategies[i]->attr & NO_FAST_FORWARD)\n> +\t\tif (use_strategies[i]->attr & NO_FAST_FORWARD) {\n>  \t\t\tallow_fast_forward = 0;\n> +\t\t\tif (fast_forward_only)\n> +\t\t\t\tdie(\"You cannot combine --ff-only with the merge strategy '%s'.\", use_strategies[i]->name);\n> +\t\t}\n\nI am not convinced this tests the right condition nor it is placed at the\nright place in the codepath---even if a specified strategy happens to\nallow fast-forward, wouldn't it be nonsense to say\n\n    $ git merge --ff-only -s resolve that-one\n\nin the first place?  Note that I am not saying \"I am convinced this is\nwrong.\"\n\n> @@ -1040,7 +1049,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) {\n> +\t\tif (allow_trivial && !fast_forward_only) {\n\nGood.\n\n> @@ -1079,6 +1088,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t\t}\n>  \t}\n>  \n> +\tif (fast_forward_only)\n> +\t\tdie(\"Not possible to fast forward, aborting.\");\n\nGood.\n"},{"id":"126235","messageId":"4AE934DA.7080907@gmail.com","threadId":"21410","inReplyTo":"7vk4yfi1dd.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Teach 'git merge' and 'git pull' the option --ff-only","fromName":"Björn Gustavsson","fromEmail":"bgustavsson@gmail.com","sentAt":"2009-10-29T06:23:22Z","receivedAt":"2009-10-29T06:23:22Z","isPatch":true,"sender":{"key":"bgustavsson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/74840?v=4"},"body":"Junio C Hamano wrote:\n> Björn Gustavsson <bgustavsson@gmail.com> writes:\n>>  \n>> +--ff-only::\n>> +\tRefuse to merge unless the merge can be resolved as a\n>> +\tfast-forward.\n> \n> Do you or do you not allow \"already up to date\"?  I think it makes sense\n> to allow it, but it is unclear from these two lines.\n\nI do allow it. I will change the description to the following in the\nre-roll:\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\n> \n>> @@ -874,6 +877,9 @@ 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> Are these the only nonsensical combinations?  How should this interact\n> with other options, e.g. --squash or --message?\n\nThey are the only options I can think of that flatly contradict each other.\n\nCombining --squash and --ff-only will succeed if the current HEAD can be\nfast-forwarded and will abort otherwise. I don't know how useful that\nwould be in practice, but I see no strong reason to forbid it.\n\nThe -m option will always be ignored, of course, and there will be\nthe usual warning if fast-forward is possible:\n\n   Fast forward (no commit created; -m option ignored)\n\nI don't think there is any need to explicitly forbid the combination\nof -m and --ff-only.\n\nI should probably update the commit message in the re-roll to include\nthe information in the previous paragraphs.\n\n>> @@ -969,8 +975,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>>  \t}\n>>  \n>>  \tfor (i = 0; i < use_strategies_nr; i++) {\n>> -\t\tif (use_strategies[i]->attr & NO_FAST_FORWARD)\n>> +\t\tif (use_strategies[i]->attr & NO_FAST_FORWARD) {\n>>  \t\t\tallow_fast_forward = 0;\n>> +\t\t\tif (fast_forward_only)\n>> +\t\t\t\tdie(\"You cannot combine --ff-only with the merge strategy '%s'.\", use_strategies[i]->name);\n>> +\t\t}\n> \n> I am not convinced this tests the right condition nor it is placed at the\n> right place in the codepath---even if a specified strategy happens to\n> allow fast-forward, wouldn't it be nonsense to say\n> \n>     $ git merge --ff-only -s resolve that-one\n> \n> in the first place?  Note that I am not saying \"I am convinced this is\n> wrong.\"\n\nRe-thinking it, I think that the test should be removed. It seemed like\na good idea at the time to point out which strategy that prevented the \nfast-forward, but if there is a list of merge strategies, the test will prevent\n--ff-only to succeed if *any* of merge strategies cannot fast-forward.\n(Also, but I am not sure about this, a merge strategy that does not allow\nfast-forward might allow up-to-date.)\n\nTherefore, I will remove the test in the re-roll.\n\n\nThanks for the comments!\n\n/Björn\n"}]}