{"thread":{"id":"56110","subject":"[PATCH v2] pull: abort if --ff-only is given and fast-forwarding is impossible","startedAt":"2021-07-15T05:06:32Z","lastAt":"2021-07-15T09:55:33Z","messageCount":2,"participants":["Alex Henrie","Felipe Contreras"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"430180","messageId":"20210715050559.3371470-1-alexhenrie24@gmail.com","threadId":"56110","inReplyTo":null,"subject":"[PATCH v2] pull: abort if --ff-only is given and fast-forwarding is impossible","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2021-07-15T05:05:59Z","receivedAt":"2021-07-15T05:06:32Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"The warning about pulling without specifying how to reconcile divergent\nbranches says that after setting pull.rebase to true, --ff-only can\nstill be passed on the command line to require a fast-forward. Make that\nactually work.\n\nPer the discussion on the mailing list, explicitly passing --rebase on\nthe command line countermands pull.ff=only in the configuration or a\nprevious --ff-only on the command line.\n\nThe --no-ff in `git pull --rebase --no-ff` is still ignored.\n\nSigned-off-by: Alex Henrie <alexhenrie24@gmail.com>\n---\nThis is the code I was working on when Elijah sent his patch set to\naccomplish the same general goals. Mine is a less ambitious (and less\ninvasive) approach, but I don't care which solution wins.\n---\n Documentation/git-pull.txt   |  2 +-\n advice.c                     |  5 +++++\n advice.h                     |  1 +\n builtin/merge.c              |  2 +-\n builtin/pull.c               | 23 +++++++++++++++++------\n t/t7601-merge-pull-config.sh | 35 +++++++++++++++++++++++++++++++++++\n 6 files changed, 60 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\nindex 5c3fb67c01..3cf957d2ed 100644\n--- a/Documentation/git-pull.txt\n+++ b/Documentation/git-pull.txt\n@@ -107,7 +107,7 @@ include::merge-options.txt[]\n \tbranch after fetching. If there is a remote-tracking branch\n \tcorresponding to the upstream branch and the upstream branch\n \twas rebased since last fetched, the rebase uses that information\n-\tto avoid rebasing non-local changes.\n+\tto avoid rebasing non-local changes. Implies --ff.\n +\n When set to `merges`, rebase using `git rebase --rebase-merges` so that\n the local merge commits are included in the rebase (see\ndiff --git a/advice.c b/advice.c\nindex 0b9c89c48a..337e8f342b 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -286,6 +286,11 @@ void NORETURN die_conclude_merge(void)\n \tdie(_(\"Exiting because of unfinished merge.\"));\n }\n \n+void NORETURN die_ff_impossible(void)\n+{\n+\tdie(_(\"Not possible to fast-forward, aborting.\"));\n+}\n+\n void advise_on_updating_sparse_paths(struct string_list *pathspec_list)\n {\n \tstruct string_list_item *item;\ndiff --git a/advice.h b/advice.h\nindex bd26c385d0..1624043838 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -95,6 +95,7 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...);\n int error_resolve_conflict(const char *me);\n void NORETURN die_resolve_conflict(const char *me);\n void NORETURN die_conclude_merge(void);\n+void NORETURN die_ff_impossible(void);\n void advise_on_updating_sparse_paths(struct string_list *pathspec_list);\n void detach_advice(const char *new_name);\n \ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a8a843b1f5..aa920ac524 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1620,7 +1620,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (fast_forward == FF_ONLY)\n-\t\tdie(_(\"Not possible to fast-forward, aborting.\"));\n+\t\tdie_ff_impossible();\n \n \tif (autostash)\n \t\tcreate_autostash(the_repository,\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 3e13f81084..256b96633e 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -55,12 +55,17 @@ static enum rebase_type parse_config_rebase(const char *key, const char *value,\n static int parse_opt_rebase(const struct option *opt, const char *arg, int unset)\n {\n \tenum rebase_type *value = opt->value;\n+\tchar **ff_value = (char **)opt->extra;\n \n \tif (arg)\n \t\t*value = parse_config_rebase(\"--rebase\", arg, 0);\n \telse\n \t\t*value = unset ? REBASE_FALSE : REBASE_TRUE;\n-\treturn *value == REBASE_INVALID ? -1 : 0;\n+\tif (*value == REBASE_INVALID)\n+\t\treturn -1;\n+\tif (*value != REBASE_FALSE)\n+\t\t*ff_value = xstrdup(\"--ff\");\n+\treturn 0;\n }\n \n static const char * const pull_usage[] = {\n@@ -125,10 +130,11 @@ static struct option pull_options[] = {\n \n \t/* Options passed to git-merge or git-rebase */\n \tOPT_GROUP(N_(\"Options related to merging\")),\n-\tOPT_CALLBACK_F('r', \"rebase\", &opt_rebase,\n+\t{ OPTION_CALLBACK, 'r', \"rebase\", &opt_rebase,\n \t\t\"(false|true|merges|preserve|interactive)\",\n \t\tN_(\"incorporate changes by rebasing rather than merging\"),\n-\t\tPARSE_OPT_OPTARG, parse_opt_rebase),\n+\t\tPARSE_OPT_OPTARG, parse_opt_rebase, REBASE_FALSE, NULL,\n+\t\t(intptr_t)&opt_ff },\n \tOPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,\n \t\tN_(\"do not show a diffstat at the end of the merge\"),\n \t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n@@ -1046,9 +1052,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \n \tcan_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);\n \n-\tif (rebase_unspecified && !opt_ff && !can_ff) {\n-\t\tif (opt_verbosity >= 0)\n-\t\t\tshow_advice_pull_non_ff();\n+\tif (!can_ff) {\n+\t\tif (opt_ff) {\n+\t\t\tif (!strcmp(opt_ff, \"--ff-only\"))\n+\t\t\t\tdie_ff_impossible();\n+\t\t} else {\n+\t\t\tif (rebase_unspecified && opt_verbosity >= 0)\n+\t\t\t\tshow_advice_pull_non_ff();\n+\t\t}\n \t}\n \n \tif (opt_rebase) {\ndiff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\nindex 52e8ccc933..8922aa6f7c 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -183,6 +183,41 @@ test_expect_success 'pull prevents non-fast-forward with \"only\" in pull.ff' '\n \ttest_must_fail git pull . c3\n '\n \n+test_expect_success 'pull prevents non-fast-forward with pull.ff=only and pull.rebase=true' '\n+\tgit reset --hard c1 &&\n+\ttest_config pull.ff only &&\n+\ttest_config pull.rebase true &&\n+\ttest_must_fail git pull . c3\n+'\n+\n+test_expect_success 'pull prevents non-fast-forward with pull.ff=only and pull.rebase=false' '\n+\tgit reset --hard c1 &&\n+\ttest_config pull.ff only &&\n+\ttest_config pull.rebase false &&\n+\ttest_must_fail git pull . c3\n+'\n+\n+test_expect_success 'pull prevents non-fast-forward with --rebase --ff-only' '\n+\tgit reset --hard c1 &&\n+\ttest_must_fail git pull --rebase --ff-only . c3\n+'\n+\n+test_expect_success 'pull prevents non-fast-forward with --no-rebase --ff-only' '\n+\tgit reset --hard c1 &&\n+\ttest_must_fail git pull --no-rebase --ff-only . c3\n+'\n+\n+test_expect_success 'pull allows non-fast-forward with --ff-only --rebase' '\n+\tgit reset --hard c1 &&\n+\tgit pull --ff-only --rebase . c3\n+'\n+\n+test_expect_success 'pull allows non-fast-forward with pull.ff=only and --rebase' '\n+\tgit reset --hard c1 &&\n+\ttest_config pull.ff only &&\n+\tgit pull --rebase . c3\n+'\n+\n test_expect_success 'merge c1 with c2 (ours in pull.twohead)' '\n \tgit reset --hard c1 &&\n \tgit config pull.twohead ours &&\n-- \n2.32.0.265.g52796feb00\n\n"},{"id":"430194","messageId":"60f00611796d0_2e672085d@natae.notmuch","threadId":"56110","inReplyTo":"20210715050559.3371470-1-alexhenrie24@gmail.com","subject":"RE: [PATCH v2] pull: abort if --ff-only is given and fast-forwarding is impossible","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-15T09:55:29Z","receivedAt":"2021-07-15T09:55:33Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Alex Henrie wrote:\n> The warning about pulling without specifying how to reconcile divergent\n> branches says that after setting pull.rebase to true, --ff-only can\n> still be passed on the command line to require a fast-forward. Make that\n> actually work.\n> \n> Per the discussion on the mailing list, explicitly passing --rebase on\n> the command line countermands pull.ff=only in the configuration or a\n> previous --ff-only on the command line.\n> \n> The --no-ff in `git pull --rebase --no-ff` is still ignored.\n> \n> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n> ---\n> This is the code I was working on when Elijah sent his patch set to\n> accomplish the same general goals. Mine is a less ambitious (and less\n> invasive) approach, but I don't care which solution wins.\n\nThis is a better approach than Elijah's because it doesn't fail if both\npull.ff and pull.rebase are configured.\n\nBut there's still the problem that the new behavior cannot be\nconfigured.\n\n-- \nFelipe Contreras\n"}]}