{"thread":{"id":"56109","subject":"[PATCH 0/5] Handle conflicting pull options","startedAt":"2021-07-15T02:40:56Z","lastAt":"2021-07-16T21:57:02Z","messageCount":26,"participants":["Elijah Newren via GitGitGadget","Alex Henrie via GitGitGadget","Eric Sunshine","Felipe Contreras","Elijah Newren","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"430172","messageId":"pull.1047.git.git.1626316849.gitgitgadget@gmail.com","threadId":"56109","inReplyTo":null,"subject":"[PATCH 0/5] Handle conflicting pull options","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-15T02:40:44Z","receivedAt":"2021-07-15T02:40:56Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"We've recently discussed how to handle conflicting configuration and command\nline options in git pull, including at least a few different\nproposals[1][2][3] to handle different subsets of the possibilities. We also\nhave a user report from someone who had conflicting configuration and got\nsurprised when one of the options overruled the other -- with no warning\nfrom the command or the documentation that such would happen. Here's my\nattempt to impose clear and simple rules, which can be seen in the commit\nmessage of the third patch.\n\n(The first two patches are just preparatory changes to make patch 3 easier\nto read.)\n\nSince the handling of conflicting options was holding up two of Alex's\npatches[4][5], I also include those two patches at the end of my series,\nthough I've made quite a few changes and additions to the latter of those.\n\nPossible areas of concern:\n\n * Documentation/git-pull.txt includes merge-options.txt. While git-rebase\n supports many of those \"merge\" options, I suspect there are others that\n it does not support. We are probably silently ignoring those unsupported\n options whenever one of those is specified at the same time a rebase is\n requested; we should instead likely error out and report the\n incompatibility. I have not yet addressed that, as I was focused on the\n main rebase vs. merge incompatibility and default warning/error\n associated with it.\n\n[1]\nhttps://lore.kernel.org/git/00e246b1-c712-e6a5-5c27-89127d796098@gmail.com/\n[2] https://lore.kernel.org/git/xmqq8s2b489p.fsf@gitster.g/ [3]\nhttps://lore.kernel.org/git/CABPp-BERS0iiiVhSsSs6dkqzBVTQgwJUjjKaZQEzRDGRUdObcQ@mail.gmail.com/\n[4]\nhttps://lore.kernel.org/git/20210711012604.947321-1-alexhenrie24@gmail.com/\n[5]\nhttps://lore.kernel.org/git/20210627000855.530985-1-alexhenrie24@gmail.com/\n\nAlex Henrie (1):\n pull: abort if --ff-only is given and fast-forwarding is impossible\n\nElijah Newren (4):\n pull: move definitions of parse_config_rebase and parse_opt_rebase\n pull: convert OPT_PASSTHRU for fast-forward options to OPT_CALLBACK\n pull: handle conflicting rebase/merge options via last option wins\n pull: abort by default when fast-forwarding is not possible\n\n Documentation/config/pull.txt | 3 +-\n Documentation/git-pull.txt | 19 +++--\n advice.c | 5 ++\n advice.h | 1 +\n builtin/merge.c | 2 +-\n builtin/pull.c | 144 ++++++++++++++++++++--------------\n t/t4013-diff-various.sh | 2 +-\n t/t5520-pull.sh | 20 ++---\n t/t5521-pull-options.sh | 4 +-\n t/t5524-pull-msg.sh | 4 +-\n t/t5553-set-upstream.sh | 14 ++--\n t/t5604-clone-reference.sh | 4 +-\n t/t6402-merge-rename.sh | 18 ++---\n t/t6409-merge-subtree.sh | 6 +-\n t/t6417-merge-ours-theirs.sh | 10 +--\n t/t7601-merge-pull-config.sh | 97 ++++++++++++++++++++++-\n t/t7603-merge-reduce-heads.sh | 2 +-\n 17 files changed, 242 insertions(+), 113 deletions(-)\n\n\nbase-commit: 75ae10bc75336db031ee58d13c5037b929235912\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1047%2Fnewren%2Fhandle-conflicting-pull-options-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1047/newren/handle-conflicting-pull-options-v1\nPull-Request: https://github.com/git/git/pull/1047\n-- \ngitgitgadget\n"},{"id":"430173","messageId":"dc7163aae728d23539406b77faf0d55168e1bd05.1626316849.git.gitgitgadget@gmail.com","threadId":"56109","inReplyTo":"pull.1047.git.git.1626316849.gitgitgadget@gmail.com","subject":"[PATCH 1/5] pull: move definitions of parse_config_rebase and parse_opt_rebase","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-15T02:40:45Z","receivedAt":"2021-07-15T02:40:58Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nNo code changes, just moving these so a subsequent change will have\naccess to some variables.\n\nSigned-off-by: Elijah Newren \n---\n builtin/pull.c | 72 +++++++++++++++++++++++++-------------------------\n 1 file changed, 36 insertions(+), 36 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 3e13f810843..61c68e4143f 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -27,42 +27,6 @@\n #include \"commit-reach.h\"\n #include \"sequencer.h\"\n \n-/**\n- * Parses the value of --rebase. If value is a false value, returns\n- * REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is\n- * \"merges\", returns REBASE_MERGES. If value is \"preserve\", returns\n- * REBASE_PRESERVE. If value is a invalid value, dies with a fatal error if\n- * fatal is true, otherwise returns REBASE_INVALID.\n- */\n-static enum rebase_type parse_config_rebase(const char *key, const char *value,\n-\t\tint fatal)\n-{\n-\tenum rebase_type v = rebase_parse_value(value);\n-\tif (v != REBASE_INVALID)\n-\t\treturn v;\n-\n-\tif (fatal)\n-\t\tdie(_(\"Invalid value for %s: %s\"), key, value);\n-\telse\n-\t\terror(_(\"Invalid value for %s: %s\"), key, value);\n-\n-\treturn REBASE_INVALID;\n-}\n-\n-/**\n- * Callback for --rebase, which parses arg with parse_config_rebase().\n- */\n-static int parse_opt_rebase(const struct option *opt, const char *arg, int unset)\n-{\n-\tenum rebase_type *value = opt->value;\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-}\n-\n static const char * const pull_usage[] = {\n \tN_(\"git pull [] [ [...]]\"),\n \tNULL\n@@ -112,6 +76,42 @@ static int opt_show_forced_updates = -1;\n static char *set_upstream;\n static struct strvec opt_fetch = STRVEC_INIT;\n \n+/**\n+ * Parses the value of --rebase. If value is a false value, returns\n+ * REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is\n+ * \"merges\", returns REBASE_MERGES. If value is \"preserve\", returns\n+ * REBASE_PRESERVE. If value is a invalid value, dies with a fatal error if\n+ * fatal is true, otherwise returns REBASE_INVALID.\n+ */\n+static enum rebase_type parse_config_rebase(const char *key, const char *value,\n+\t\tint fatal)\n+{\n+\tenum rebase_type v = rebase_parse_value(value);\n+\tif (v != REBASE_INVALID)\n+\t\treturn v;\n+\n+\tif (fatal)\n+\t\tdie(_(\"Invalid value for %s: %s\"), key, value);\n+\telse\n+\t\terror(_(\"Invalid value for %s: %s\"), key, value);\n+\n+\treturn REBASE_INVALID;\n+}\n+\n+/**\n+ * Callback for --rebase, which parses arg with parse_config_rebase().\n+ */\n+static int parse_opt_rebase(const struct option *opt, const char *arg, int unset)\n+{\n+\tenum rebase_type *value = opt->value;\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+}\n+\n static struct option pull_options[] = {\n \t/* Shared options */\n \tOPT__VERBOSITY(&opt_verbosity),\n-- \ngitgitgadget\n\n"},{"id":"430174","messageId":"3c07ce978caa832b08c6bef1c48c061e41a6fd0b.1626316849.git.gitgitgadget@gmail.com","threadId":"56109","inReplyTo":"pull.1047.git.git.1626316849.gitgitgadget@gmail.com","subject":"[PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-15T02:40:47Z","receivedAt":"2021-07-15T02:41:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe --rebase[=...] flags and the various ff flags are incompatible,\nexcept that --no-rebase (or --rebase=false) work with any of the ff\nflags, and --ff works with any of the rebase flags.\n\nBoth sets of these flags could also be passed via configuration\noptions, namely pull.rebase and pull.ff.\n\nAs with elsewhere in git:\n * Make the last flag specified win\n * Treat command line flags as coming after any configuration options\n * Do not imply an order between different configuration options; if\n they conflict, just report an error.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/config/pull.txt | 3 +-\n Documentation/git-pull.txt | 3 ++\n builtin/pull.c | 12 +++++++\n t/t7601-merge-pull-config.sh | 67 +++++++++++++++++++++++++++++++++++\n 4 files changed, 84 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/pull.txt b/Documentation/config/pull.txt\nindex 54048306095..e70ed99e408 100644\n--- a/Documentation/config/pull.txt\n+++ b/Documentation/config/pull.txt\n@@ -7,12 +7,13 @@ pull.ff::\n \tline). When set to `only`, only such fast-forward merges are\n \tallowed (equivalent to giving the `--ff-only` option from the\n \tcommand line). This setting overrides `merge.ff` when pulling.\n+\tIncompatible with pull.rebase.\n \n pull.rebase::\n \tWhen true, rebase branches on top of the fetched branch, instead\n \tof merging the default branch from the default remote when \"git\n \tpull\" is run. See \"branch..rebase\" for setting this on a\n-\tper-branch basis.\n+\tper-branch basis. Incompatible with pull.ff.\n +\n When `merges` (or just 'm'), pass the `--rebase-merges` option to 'git rebase'\n so that the local merge commits are included in the rebase (see\ndiff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\nindex 5c3fb67c014..03e8694e146 100644\n--- a/Documentation/git-pull.txt\n+++ b/Documentation/git-pull.txt\n@@ -121,6 +121,9 @@ When false, merge the current branch into the upstream branch.\n +\n When `interactive`, enable the interactive mode of rebase.\n +\n+Note that these flags are incompatible with --no-ff and --ff-only; if\n+such incompatible flags are given, the last one will take precedence.\n++\n See `pull.rebase`, `branch..rebase` and `branch.autoSetupRebase` in\n linkgit:git-config[1] if you want to make `git pull` always use\n `--rebase` instead of merging.\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex d99719403d0..b355fd38794 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -109,6 +109,11 @@ static int parse_opt_rebase(const struct option *opt, const char *arg, int unset\n \t\t*value = parse_config_rebase(\"--rebase\", arg, 0);\n \telse\n \t\t*value = unset ? REBASE_FALSE : REBASE_TRUE;\n+\n+\t/* --rebase overrides earlier --ff-only and --no-ff */\n+\tif (*value != REBASE_FALSE)\n+\t\topt_ff = \"--ff\";\n+\n \treturn *value == REBASE_INVALID ? -1 : 0;\n }\n \n@@ -119,6 +124,10 @@ static int parse_opt_ff(const struct option *opt, const char *arg, int unset)\n \telse\n \t\topt_ff = xstrfmt(\"--%s\", opt->long_name);\n \n+\t/* --ff-only and --no-ff override earlier --rebase */\n+\tif (strcmp(opt_ff, \"--ff\"))\n+\t\topt_rebase = REBASE_FALSE;\n+\n \treturn 0;\n }\n \n@@ -984,6 +993,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tif (opt_rebase < 0)\n \t\topt_rebase = config_get_rebase(&rebase_unspecified);\n \n+\tif (opt_rebase != REBASE_FALSE && opt_ff && strcmp(opt_ff, \"--ff\"))\n+\t\tdie(_(\"pull.rebase and pull.ff are incompatible; please unset one\"));\n+\n \tif (read_cache_unmerged())\n \t\tdie_resolve_conflict(\"pull\");\n \ndiff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\nindex 52e8ccc933a..73a0dbdf25a 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -143,6 +143,73 @@ test_expect_success 'pull.rebase not set and --ff-only given (not-fast-forward)'\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n+test_does_rebase() {\n+\tgit reset --hard c2 &&\n+\tgit \"$@\" . c1 &&\n+\t# Check that we actually did a rebase\n+\tgit rev-list --count HEAD >actual &&\n+\tgit rev-list --merges --count HEAD >>actual &&\n+\ttest_write_lines 3 0 >expect &&\n+\ttest_cmp expect actual &&\n+\trm actual expect\n+}\n+\n+test_does_merge() {\n+\tgit reset --hard c2 &&\n+\tgit \"$@\" . c1 &&\n+\t# Check that we actually did a merge\n+\tgit rev-list --count HEAD >actual &&\n+\tgit rev-list --merges --count HEAD >>actual &&\n+\ttest_write_lines 4 1 >expect &&\n+\ttest_cmp expect actual &&\n+\trm actual expect\n+}\n+\n+test_attempts_fast_forward() {\n+\tgit reset --hard c2 &&\n+\ttest_must_fail git \"$@\" . c1 2>err &&\n+\ttest_i18ngrep \"Not possible to fast-forward, aborting\" err\n+}\n+\n+test_expect_success 'conflicting options: --ff-only --rebase' '\n+\ttest_does_rebase pull --ff-only --rebase\n+'\n+\n+test_expect_success 'conflicting options: --no-ff --rebase' '\n+\ttest_does_rebase pull --no-ff --rebase\n+'\n+\n+test_expect_success 'conflicting options: -c pull.ff=false --rebase' '\n+\ttest_does_rebase -c pull.ff=false pull --rebase\n+'\n+\n+test_expect_success 'conflicting options: -c pull.ff=only --rebase' '\n+\ttest_does_rebase -c pull.ff=only pull --rebase\n+'\n+\n+test_expect_success 'conflicting options: --rebase --ff-only' '\n+\ttest_attempts_fast_forward pull --rebase --ff-only\n+'\n+\n+test_expect_success 'conflicting options: --rebase --no-ff' '\n+\ttest_does_merge pull --rebase --no-ff\n+'\n+\n+test_expect_success 'conflicting options: -c pull.rebase=true --no-ff' '\n+\ttest_does_merge -c pull.rebase=true pull --no-ff\n+'\n+\n+test_expect_success 'conflicting options: -c pull.rebase=true --ff-only' '\n+\ttest_attempts_fast_forward -c pull.rebase=true pull --ff-only\n+'\n+\n+test_expect_success 'report conflicting configuration' '\n+\tgit reset --hard c2 &&\n+\ttest_must_fail git -c pull.ff=false -c pull.rebase=true pull . c1 2>err &&\n+\ttest_i18ngrep \"pull.rebase and pull.ff are incompatible; please unset one\" err\n+\n+'\n+\n test_expect_success 'merge c1 with c2' '\n \tgit reset --hard c1 &&\n \ttest -f c0.c &&\n-- \ngitgitgadget\n\n"},{"id":"430175","messageId":"2fc7344863103dc8f94499e60a16d0a8b47bda22.1626316849.git.gitgitgadget@gmail.com","threadId":"56109","inReplyTo":"pull.1047.git.git.1626316849.gitgitgadget@gmail.com","subject":"[PATCH 2/5] pull: convert OPT_PASSTHRU for fast-forward options to OPT_CALLBACK","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-15T02:40:46Z","receivedAt":"2021-07-15T02:41:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThis results in no functional changes, but allows us to add logic to\nthe callback in a subsequent commit.\n\nSigned-off-by: Elijah Newren \n---\n builtin/pull.c | 24 ++++++++++++++++++------\n 1 file changed, 18 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 61c68e4143f..d99719403d0 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -112,6 +112,16 @@ static int parse_opt_rebase(const struct option *opt, const char *arg, int unset\n \treturn *value == REBASE_INVALID ? -1 : 0;\n }\n \n+static int parse_opt_ff(const struct option *opt, const char *arg, int unset)\n+{\n+\tif (unset)\n+\t\topt_ff = \"--no-ff\";\n+\telse\n+\t\topt_ff = xstrfmt(\"--%s\", opt->long_name);\n+\n+\treturn 0;\n+}\n+\n static struct option pull_options[] = {\n \t/* Shared options */\n \tOPT__VERBOSITY(&opt_verbosity),\n@@ -154,12 +164,14 @@ static struct option pull_options[] = {\n \t\tN_(\"edit message before committing\"),\n \t\tPARSE_OPT_NOARG),\n \tOPT_CLEANUP(&cleanup_arg),\n-\tOPT_PASSTHRU(0, \"ff\", &opt_ff, NULL,\n-\t\tN_(\"allow fast-forward\"),\n-\t\tPARSE_OPT_NOARG),\n-\tOPT_PASSTHRU(0, \"ff-only\", &opt_ff, NULL,\n-\t\tN_(\"abort if fast-forward is not possible\"),\n-\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n+\tOPT_CALLBACK_F(0, \"ff\", &opt_ff, NULL,\n+\t\t\tN_(\"allow fast-forward\"),\n+\t\t\tPARSE_OPT_NOARG,\n+\t\t\tparse_opt_ff),\n+\tOPT_CALLBACK_F(0, \"ff-only\", &opt_ff, NULL,\n+\t\t\tN_(\"abort if fast-forward is not possible\"),\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG,\n+\t\t\tparse_opt_ff),\n \tOPT_PASSTHRU(0, \"verify-signatures\", &opt_verify_signatures, NULL,\n \t\tN_(\"verify that the named commit has a valid GPG signature\"),\n \t\tPARSE_OPT_NOARG),\n-- \ngitgitgadget\n\n"},{"id":"430176","messageId":"12ee3b140b4e1170bc725753cda56a4426823bdc.1626316849.git.gitgitgadget@gmail.com","threadId":"56109","inReplyTo":"pull.1047.git.git.1626316849.gitgitgadget@gmail.com","subject":"[PATCH 4/5] pull: abort if --ff-only is given and fast-forwarding is impossible","fromName":"Alex Henrie via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-15T02:40:48Z","receivedAt":"2021-07-15T02:41:02Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"From: Alex Henrie \n\nThe 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\nSigned-off-by: Alex Henrie \n---\n advice.c | 5 +++++\n advice.h | 1 +\n builtin/merge.c | 2 +-\n builtin/pull.c | 11 ++++++++---\n t/t7601-merge-pull-config.sh | 24 ++++++++++++++++++++++++\n 5 files changed, 39 insertions(+), 4 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex 0b9c89c48ab..337e8f342bc 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 bd26c385d00..16240438387 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 a8a843b1f54..aa920ac524f 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 b355fd38794..2c90bbb1588 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -1070,9 +1070,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 73a0dbdf25a..6b9e80db97b 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -250,6 +250,30 @@ 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 'merge c1 with c2 (ours in pull.twohead)' '\n \tgit reset --hard c1 &&\n \tgit config pull.twohead ours &&\n-- \ngitgitgadget\n\n"},{"id":"430177","messageId":"7e12c45fc9a94e7b56a6efdc085ebe081dd40afc.1626316849.git.gitgitgadget@gmail.com","threadId":"56109","inReplyTo":"pull.1047.git.git.1626316849.gitgitgadget@gmail.com","subject":"[PATCH 5/5] pull: abort by default when fast-forwarding is not possible","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-15T02:40:49Z","receivedAt":"2021-07-15T02:41:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWe have for some time shown a long warning when the user does not\nspecify how to reconcile divergent branches with git pull. Make it an\nerror now.\n\nInitial-patch-by: Alex Henrie \nSigned-off-by: Elijah Newren \n---\n Documentation/git-pull.txt | 18 ++++++++++--------\n builtin/pull.c | 31 +++++++++++++++----------------\n t/t4013-diff-various.sh | 2 +-\n t/t5520-pull.sh | 20 ++++++++++----------\n t/t5521-pull-options.sh | 4 ++--\n t/t5524-pull-msg.sh | 4 ++--\n t/t5553-set-upstream.sh | 14 +++++++-------\n t/t5604-clone-reference.sh | 4 ++--\n t/t6402-merge-rename.sh | 18 +++++++++---------\n t/t6409-merge-subtree.sh | 6 +++---\n t/t6417-merge-ours-theirs.sh | 10 +++++-----\n t/t7601-merge-pull-config.sh | 6 ++----\n t/t7603-merge-reduce-heads.sh | 2 +-\n 13 files changed, 69 insertions(+), 70 deletions(-)\n\ndiff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\nindex 03e8694e146..cb65d33e15e 100644\n--- a/Documentation/git-pull.txt\n+++ b/Documentation/git-pull.txt\n@@ -15,14 +15,16 @@ SYNOPSIS\n DESCRIPTION\n -----------\n \n-Incorporates changes from a remote repository into the current\n-branch. In its default mode, `git pull` is shorthand for\n-`git fetch` followed by `git merge FETCH_HEAD`.\n-\n-More precisely, 'git pull' runs 'git fetch' with the given\n-parameters and calls 'git merge' to merge the retrieved branch\n-heads into the current branch.\n-With `--rebase`, it runs 'git rebase' instead of 'git merge'.\n+Incorporates changes from a remote repository into the current branch.\n+If the current branch is behind the remote, then by default it will\n+fast-forward the current branch to match the remote. If the current\n+branch and the remote have diverged, the user needs to specify how to\n+reconcile the divergent branches with --no-ff or --rebase (or the\n+corresponding configuration options in pull.ff or pull.rebase).\n+\n+More precisely, 'git pull' runs 'git fetch' with the given parameters\n+and then depending on config options or command line flags, will call\n+either 'git merge' or 'git rebase' to reconcile diverging branches.\n \n should be the name of a remote repository as\n passed to linkgit:git-fetch[1]. can name an\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 2c90bbb1588..0f8fdb7d42b 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -946,20 +946,20 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_\n \treturn ret;\n }\n \n-static void show_advice_pull_non_ff(void)\n+static void NORETURN die_pull_non_ff(void)\n {\n-\tadvise(_(\"Pulling without specifying how to reconcile divergent branches is\\n\"\n-\t\t \"discouraged. You can squelch this message by running one of the following\\n\"\n-\t\t \"commands sometime before your next pull:\\n\"\n-\t\t \"\\n\"\n-\t\t \" git config pull.rebase false # merge (the default strategy)\\n\"\n-\t\t \" git config pull.rebase true # rebase\\n\"\n-\t\t \" git config pull.ff only # fast-forward only\\n\"\n-\t\t \"\\n\"\n-\t\t \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n-\t\t \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n-\t\t \"or --ff-only on the command line to override the configured default per\\n\"\n-\t\t \"invocation.\\n\"));\n+\tdie(_(\"You have divergent branches and need to specify how to reconcile them.\\n\"\n+\t \"You can do so by running one of the following commands sometime before\\n\"\n+\t \"your next pull:\\n\"\n+\t \"\\n\"\n+\t \" git config pull.rebase false # merge (the default strategy)\\n\"\n+\t \" git config pull.rebase true # rebase\\n\"\n+\t \" git config pull.ff only # fast-forward only\\n\"\n+\t \"\\n\"\n+\t \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n+\t \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n+\t \"or --ff-only on the command line to override the configured default per\\n\"\n+\t \"invocation.\\n\"));\n }\n \n int cmd_pull(int argc, const char **argv, const char *prefix)\n@@ -1074,9 +1074,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\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} else if (rebase_unspecified) {\n+\t\t\tdie_pull_non_ff();\n \t\t}\n \t}\n \ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 7fadc985ccc..eb989f7f191 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -65,7 +65,7 @@ test_expect_success setup '\n \texport GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&\n \n \tgit checkout master &&\n-\tgit pull -s ours . side &&\n+\tgit pull -s ours --no-rebase . side &&\n \n \tGIT_AUTHOR_DATE=\"2006-06-26 00:05:00 +0000\" &&\n \tGIT_COMMITTER_DATE=\"2006-06-26 00:05:00 +0000\" &&\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex e2c0c510222..672001a18bd 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -136,12 +136,12 @@ test_expect_success 'the default remote . should not break explicit pull' '\n \tgit reset --hard HEAD^ &&\n \techo file >expect &&\n \ttest_cmp expect file &&\n-\tgit pull . second &&\n+\tgit pull --no-rebase . second &&\n \techo modified >expect &&\n \ttest_cmp expect file &&\n \tgit reflog -1 >reflog.actual &&\n \tsed \"s/^[0-9a-f][0-9a-f]*/OBJID/\" reflog.actual >reflog.fuzzy &&\n-\techo \"OBJID HEAD@{0}: pull . second: Fast-forward\" >reflog.expected &&\n+\techo \"OBJID HEAD@{0}: pull --no-rebase . second: Fast-forward\" >reflog.expected &&\n \ttest_cmp reflog.expected reflog.fuzzy\n '\n \n@@ -226,7 +226,7 @@ test_expect_success 'fail if the index has unresolved entries' '\n \ttest_commit modified2 file &&\n \tgit ls-files -u >unmerged &&\n \ttest_must_be_empty unmerged &&\n-\ttest_must_fail git pull . second &&\n+\ttest_must_fail git pull --no-rebase . second &&\n \tgit ls-files -u >unmerged &&\n \ttest_file_not_empty unmerged &&\n \tcp file expected &&\n@@ -409,37 +409,37 @@ test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '\n \n test_expect_success 'pull succeeds with dirty working directory and merge.autostash set' '\n \ttest_config merge.autostash true &&\n-\ttest_pull_autostash 2\n+\ttest_pull_autostash 2 --no-rebase\n '\n \n test_expect_success 'pull --autostash & merge.autostash=true' '\n \ttest_config merge.autostash true &&\n-\ttest_pull_autostash 2 --autostash\n+\ttest_pull_autostash 2 --autostash --no-rebase\n '\n \n test_expect_success 'pull --autostash & merge.autostash=false' '\n \ttest_config merge.autostash false &&\n-\ttest_pull_autostash 2 --autostash\n+\ttest_pull_autostash 2 --autostash --no-rebase\n '\n \n test_expect_success 'pull --autostash & merge.autostash unset' '\n \ttest_unconfig merge.autostash &&\n-\ttest_pull_autostash 2 --autostash\n+\ttest_pull_autostash 2 --autostash --no-rebase\n '\n \n test_expect_success 'pull --no-autostash & merge.autostash=true' '\n \ttest_config merge.autostash true &&\n-\ttest_pull_autostash_fail --no-autostash\n+\ttest_pull_autostash_fail --no-autostash --no-rebase\n '\n \n test_expect_success 'pull --no-autostash & merge.autostash=false' '\n \ttest_config merge.autostash false &&\n-\ttest_pull_autostash_fail --no-autostash\n+\ttest_pull_autostash_fail --no-autostash --no-rebase\n '\n \n test_expect_success 'pull --no-autostash & merge.autostash unset' '\n \ttest_unconfig merge.autostash &&\n-\ttest_pull_autostash_fail --no-autostash\n+\ttest_pull_autostash_fail --no-autostash --no-rebase\n '\n \n test_expect_success 'pull.rebase' '\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex 63a688bdbf5..7601c919fdc 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -113,7 +113,7 @@ test_expect_success 'git pull --force' '\n \tgit pull two &&\n \ttest_commit A &&\n \tgit branch -f origin &&\n-\tgit pull --all --force\n+\tgit pull --no-rebase --all --force\n \t)\n '\n \n@@ -179,7 +179,7 @@ test_expect_success 'git pull --allow-unrelated-histories' '\n \t(\n \t\tcd dst &&\n \t\ttest_must_fail git pull ../src side &&\n-\t\tgit pull --allow-unrelated-histories ../src side\n+\t\tgit pull --no-rebase --allow-unrelated-histories ../src side\n \t)\n '\n \ndiff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh\nindex c278adaa5a2..b2be3605f5a 100755\n--- a/t/t5524-pull-msg.sh\n+++ b/t/t5524-pull-msg.sh\n@@ -28,7 +28,7 @@ test_expect_success setup '\n test_expect_success pull '\n (\n \tcd cloned &&\n-\tgit pull --log &&\n+\tgit pull --no-rebase --log &&\n \tgit log -2 &&\n \tgit cat-file commit HEAD >result &&\n \tgrep Dollar result\n@@ -41,7 +41,7 @@ test_expect_success '--log=1 limits shortlog length' '\n \tgit reset --hard HEAD^ &&\n \ttest \"$(cat afile)\" = original &&\n \ttest \"$(cat bfile)\" = added &&\n-\tgit pull --log=1 &&\n+\tgit pull --no-rebase --log=1 &&\n \tgit log -3 &&\n \tgit cat-file commit HEAD >result &&\n \tgrep Dollar result &&\ndiff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh\nindex b1d614ce18c..9c12c0f8c32 100755\n--- a/t/t5553-set-upstream.sh\n+++ b/t/t5553-set-upstream.sh\n@@ -108,27 +108,27 @@ test_expect_success 'setup commit on main and other pull' '\n \n test_expect_success 'pull --set-upstream upstream main sets branch main but not other' '\n \tclear_config main other &&\n-\tgit pull --set-upstream upstream main &&\n+\tgit pull --no-rebase --set-upstream upstream main &&\n \tcheck_config main upstream refs/heads/main &&\n \tcheck_config_missing other\n '\n \n test_expect_success 'pull --set-upstream main:other2 does not set the branch other2' '\n \tclear_config other2 &&\n-\tgit pull --set-upstream upstream main:other2 &&\n+\tgit pull --no-rebase --set-upstream upstream main:other2 &&\n \tcheck_config_missing other2\n '\n \n test_expect_success 'pull --set-upstream upstream other sets branch main' '\n \tclear_config main other &&\n-\tgit pull --set-upstream upstream other &&\n+\tgit pull --no-rebase --set-upstream upstream other &&\n \tcheck_config main upstream refs/heads/other &&\n \tcheck_config_missing other\n '\n \n test_expect_success 'pull --set-upstream upstream tag does not set the tag' '\n \tclear_config three &&\n-\tgit pull --tags --set-upstream upstream three &&\n+\tgit pull --no-rebase --tags --set-upstream upstream three &&\n \tcheck_config_missing three\n '\n \n@@ -144,16 +144,16 @@ test_expect_success 'pull --set-upstream http://nosuchdomain.example.com fails w\n \n test_expect_success 'pull --set-upstream upstream HEAD sets branch HEAD' '\n \tclear_config main other &&\n-\tgit pull --set-upstream upstream HEAD &&\n+\tgit pull --no-rebase --set-upstream upstream HEAD &&\n \tcheck_config main upstream HEAD &&\n \tgit checkout other &&\n-\tgit pull --set-upstream upstream HEAD &&\n+\tgit pull --no-rebase --set-upstream upstream HEAD &&\n \tcheck_config other upstream HEAD\n '\n \n test_expect_success 'pull --set-upstream upstream with more than one branch does nothing' '\n \tclear_config main three &&\n-\tgit pull --set-upstream upstream main three &&\n+\tgit pull --no-rebase --set-upstream upstream main three &&\n \tcheck_config_missing main &&\n \tcheck_config_missing three\n '\ndiff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh\nindex e845d621f61..24340e6d56e 100755\n--- a/t/t5604-clone-reference.sh\n+++ b/t/t5604-clone-reference.sh\n@@ -87,7 +87,7 @@ test_expect_success 'updating origin' '\n '\n \n test_expect_success 'pulling changes from origin' '\n-\tgit -C C pull origin\n+\tgit -C C pull --no-rebase origin\n '\n \n # the 2 local objects are commit and tree from the merge\n@@ -96,7 +96,7 @@ test_expect_success 'that alternate to origin gets used' '\n '\n \n test_expect_success 'pulling changes from origin' '\n-\tgit -C D pull origin\n+\tgit -C D pull --no-rebase origin\n '\n \n # the 5 local objects are expected; file3 blob, commit in A to add it\ndiff --git a/t/t6402-merge-rename.sh b/t/t6402-merge-rename.sh\nindex 425dad97d54..02a842697b8 100755\n--- a/t/t6402-merge-rename.sh\n+++ b/t/t6402-merge-rename.sh\n@@ -103,7 +103,7 @@ test_expect_success 'setup' '\n test_expect_success 'pull renaming branch into unrenaming one' \\\n '\n \tgit show-branch &&\n-\ttest_expect_code 1 git pull . white &&\n+\ttest_expect_code 1 git pull --no-rebase . white &&\n \tgit ls-files -s &&\n \tgit ls-files -u B >b.stages &&\n \ttest_line_count = 3 b.stages &&\n@@ -121,7 +121,7 @@ test_expect_success 'pull renaming branch into another renaming one' \\\n \trm -f B &&\n \tgit reset --hard &&\n \tgit checkout red &&\n-\ttest_expect_code 1 git pull . white &&\n+\ttest_expect_code 1 git pull --no-rebase . white &&\n \tgit ls-files -u B >b.stages &&\n \ttest_line_count = 3 b.stages &&\n \tgit ls-files -s N >n.stages &&\n@@ -137,7 +137,7 @@ test_expect_success 'pull unrenaming branch into renaming one' \\\n '\n \tgit reset --hard &&\n \tgit show-branch &&\n-\ttest_expect_code 1 git pull . main &&\n+\ttest_expect_code 1 git pull --no-rebase . main &&\n \tgit ls-files -u B >b.stages &&\n \ttest_line_count = 3 b.stages &&\n \tgit ls-files -s N >n.stages &&\n@@ -153,7 +153,7 @@ test_expect_success 'pull conflicting renames' \\\n '\n \tgit reset --hard &&\n \tgit show-branch &&\n-\ttest_expect_code 1 git pull . blue &&\n+\ttest_expect_code 1 git pull --no-rebase . blue &&\n \tgit ls-files -u A >a.stages &&\n \ttest_line_count = 1 a.stages &&\n \tgit ls-files -u B >b.stages &&\n@@ -173,7 +173,7 @@ test_expect_success 'interference with untracked working tree file' '\n \tgit reset --hard &&\n \tgit show-branch &&\n \techo >A this file should not matter &&\n-\ttest_expect_code 1 git pull . white &&\n+\ttest_expect_code 1 git pull --no-rebase . white &&\n \ttest_path_is_file A\n '\n \n@@ -183,7 +183,7 @@ test_expect_success 'interference with untracked working tree file' '\n \tgit show-branch &&\n \trm -f A &&\n \techo >A this file should not matter &&\n-\ttest_expect_code 1 git pull . red &&\n+\ttest_expect_code 1 git pull --no-rebase . red &&\n \ttest_path_is_file A\n '\n \n@@ -193,7 +193,7 @@ test_expect_success 'interference with untracked working tree file' '\n \tgit checkout -f main &&\n \tgit tag -f anchor &&\n \tgit show-branch &&\n-\tgit pull . yellow &&\n+\tgit pull --no-rebase . yellow &&\n \ttest_path_is_missing M &&\n \tgit reset --hard anchor\n '\n@@ -220,7 +220,7 @@ test_expect_success 'updated working tree file should prevent the merge' '\n \techo >>M one line addition &&\n \tcat M >M.saved &&\n \tgit update-index M &&\n-\ttest_expect_code 128 git pull . yellow &&\n+\ttest_expect_code 128 git pull --no-rebase . yellow &&\n \ttest_cmp M M.saved &&\n \trm -f M.saved\n '\n@@ -232,7 +232,7 @@ test_expect_success 'interference with untracked working tree file' '\n \tgit tag -f anchor &&\n \tgit show-branch &&\n \techo >M this file should not matter &&\n-\tgit pull . main &&\n+\tgit pull --no-rebase . main &&\n \ttest_path_is_file M &&\n \t! {\n \t\tgit ls-files -s |\ndiff --git a/t/t6409-merge-subtree.sh b/t/t6409-merge-subtree.sh\nindex d406b2343cb..ba7890ec521 100755\n--- a/t/t6409-merge-subtree.sh\n+++ b/t/t6409-merge-subtree.sh\n@@ -100,7 +100,7 @@ test_expect_success 'merge update' '\n \tgit checkout -b topic_2 &&\n \tgit commit -m \"update git-gui\" &&\n \tcd ../git &&\n-\tgit pull -s subtree gui topic_2 &&\n+\tgit pull --no-rebase -s subtree gui topic_2 &&\n \tgit ls-files -s >actual &&\n \t(\n \t\techo \"100644 $o3 0\tgit-gui/git-gui.sh\" &&\n@@ -129,7 +129,7 @@ test_expect_success 'initial ambiguous subtree' '\n test_expect_success 'merge using explicit' '\n \tcd ../git &&\n \tgit reset --hard topic_2 &&\n-\tgit pull -Xsubtree=git-gui gui topic_2 &&\n+\tgit pull --no-rebase -Xsubtree=git-gui gui topic_2 &&\n \tgit ls-files -s >actual &&\n \t(\n \t\techo \"100644 $o3 0\tgit-gui/git-gui.sh\" &&\n@@ -142,7 +142,7 @@ test_expect_success 'merge using explicit' '\n test_expect_success 'merge2 using explicit' '\n \tcd ../git &&\n \tgit reset --hard topic_2 &&\n-\tgit pull -Xsubtree=git-gui2 gui topic_2 &&\n+\tgit pull --no-rebase -Xsubtree=git-gui2 gui topic_2 &&\n \tgit ls-files -s >actual &&\n \t(\n \t\techo \"100644 $o1 0\tgit-gui/git-gui.sh\" &&\ndiff --git a/t/t6417-merge-ours-theirs.sh b/t/t6417-merge-ours-theirs.sh\nindex ac9aee9a662..ec065d6a658 100755\n--- a/t/t6417-merge-ours-theirs.sh\n+++ b/t/t6417-merge-ours-theirs.sh\n@@ -69,11 +69,11 @@ test_expect_success 'binary file with -Xours/-Xtheirs' '\n '\n \n test_expect_success 'pull passes -X to underlying merge' '\n-\tgit reset --hard main && git pull -s recursive -Xours . side &&\n-\tgit reset --hard main && git pull -s recursive -X ours . side &&\n-\tgit reset --hard main && git pull -s recursive -Xtheirs . side &&\n-\tgit reset --hard main && git pull -s recursive -X theirs . side &&\n-\tgit reset --hard main && test_must_fail git pull -s recursive -X bork . side\n+\tgit reset --hard main && git pull --no-rebase -s recursive -Xours . side &&\n+\tgit reset --hard main && git pull --no-rebase -s recursive -X ours . side &&\n+\tgit reset --hard main && git pull --no-rebase -s recursive -Xtheirs . side &&\n+\tgit reset --hard main && git pull --no-rebase -s recursive -X theirs . side &&\n+\tgit reset --hard main && test_must_fail git pull --no-rebase -s recursive -X bork . side\n '\n \n test_expect_success SYMLINKS 'symlink with -Xours/-Xtheirs' '\ndiff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\nindex 6b9e80db97b..84404f4f0c3 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -86,10 +86,8 @@ test_expect_success 'pull.rebase not set and --ff-only given' '\n \n test_expect_success 'pull.rebase not set (not-fast-forward)' '\n \tgit reset --hard c2 &&\n-\tgit -c color.advice=always pull . c1 2>err &&\n-\ttest_decode_color decoded &&\n-\ttest_i18ngrep \"hint: \" decoded &&\n-\ttest_i18ngrep \"Pulling without specifying how to reconcile\" decoded\n+\ttest_must_fail git pull . c1 2>err &&\n+\ttest_i18ngrep \"You have divergent branches\" err\n '\n \n test_expect_success 'pull.rebase not set and pull.ff=true (not-fast-forward)' '\ndiff --git a/t/t7603-merge-reduce-heads.sh b/t/t7603-merge-reduce-heads.sh\nindex 98948955ae5..27cd94ad6f7 100755\n--- a/t/t7603-merge-reduce-heads.sh\n+++ b/t/t7603-merge-reduce-heads.sh\n@@ -68,7 +68,7 @@ test_expect_success 'merge c1 with c2, c3, c4, c5' '\n \n test_expect_success 'pull c2, c3, c4, c5 into c1' '\n \tgit reset --hard c1 &&\n-\tgit pull . c2 c3 c4 c5 &&\n+\tgit pull --no-rebase . c2 c3 c4 c5 &&\n \ttest \"$(git rev-parse c1)\" != \"$(git rev-parse HEAD)\" &&\n \ttest \"$(git rev-parse c1)\" = \"$(git rev-parse HEAD^1)\" &&\n \ttest \"$(git rev-parse c2)\" = \"$(git rev-parse HEAD^2)\" &&\n-- \ngitgitgadget\n"},{"id":"430179","messageId":"CAPig+cRZ7gFACO9eNtGmXjt_SG2oviXR7JjuW2w0wZeV7pMjkQ@mail.gmail.com","threadId":"56109","inReplyTo":"3c07ce978caa832b08c6bef1c48c061e41a6fd0b.1626316849.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-07-15T04:59:54Z","receivedAt":"2021-07-15T05:00:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 14, 2021 at 10:41 PM Elijah Newren via GitGitGadget\n wrote:\n> The --rebase[=...] flags and the various ff flags are incompatible,\n> except that --no-rebase (or --rebase=false) work with any of the ff\n> flags, and --ff works with any of the rebase flags.\n> [...]\n> Signed-off-by: Elijah Newren \n> ---\n> diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\n> +test_expect_success 'report conflicting configuration' '\n> + git reset --hard c2 &&\n> + test_must_fail git -c pull.ff=false -c pull.rebase=true pull . c1 2>err &&\n> + test_i18ngrep \"pull.rebase and pull.ff are incompatible; please unset one\" err\n> +\n> +'\n\nnit: unnecessary and inconsistent blank line at end of test\n"},{"id":"430182","messageId":"CAPig+cQUQYrkH7YsFio=JBZnaM3FRumti6OtvOTu6cUo4kWKtg@mail.gmail.com","threadId":"56109","inReplyTo":"7e12c45fc9a94e7b56a6efdc085ebe081dd40afc.1626316849.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/5] pull: abort by default when fast-forwarding is not possible","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-07-15T05:18:58Z","receivedAt":"2021-07-15T05:19:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 14, 2021 at 10:41 PM Elijah Newren via GitGitGadget\n wrote:\n> We have for some time shown a long warning when the user does not\n> specify how to reconcile divergent branches with git pull. Make it an\n> error now.\n>\n> Signed-off-by: Elijah Newren \n\nTeeny tiny nits below...\n\n> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\n> @@ -15,14 +15,16 @@ SYNOPSIS\n> -Incorporates changes from a remote repository into the current\n> -branch. In its default mode, `git pull` is shorthand for\n> -`git fetch` followed by `git merge FETCH_HEAD`.\n> -\n> -More precisely, 'git pull' runs 'git fetch' with the given\n> -parameters and calls 'git merge' to merge the retrieved branch\n> -heads into the current branch.\n> -With `--rebase`, it runs 'git rebase' instead of 'git merge'.\n\nThe original text has an odd mix of apostrophes and backticks...\n\n> +Incorporates changes from a remote repository into the current branch.\n> +If the current branch is behind the remote, then by default it will\n> +fast-forward the current branch to match the remote. If the current\n> +branch and the remote have diverged, the user needs to specify how to\n> +reconcile the divergent branches with --no-ff or --rebase (or the\n> +corresponding configuration options in pull.ff or pull.rebase).\n> +\n> +More precisely, 'git pull' runs 'git fetch' with the given parameters\n> +and then depending on config options or command line flags, will call\n> +either 'git merge' or 'git rebase' to reconcile diverging branches.\n\n... and the revised text adds \"no quotes\" to the selection. These\ndays, we'd probably backtick all of these:\n\n --no-ff\n --rebase\n pull.ff\n pull.rebase\n git pull\n git fetch\n git merge\n git rebase\n\nThe rest of this document is actually pretty good about using\nbackticks, though there are some exceptions.\n\nThere's also an odd mix of \"configuration options\" and \"config\noptions\" in the revised text. Perhaps stick with \"configuration\noptions\" to be a bit more formal?\n\n> should be the name of a remote repository as\n> passed to linkgit:git-fetch[1]. can name an\n\nAnd, as an aside, we'd backtick and , though\nyour patch isn't touching that, so outside the scope of this change.\n\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> @@ -1074,9 +1074,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> if (opt_ff) {\n> if (!strcmp(opt_ff, \"--ff-only\"))\n> die_ff_impossible();\n> - } else {\n> - if (rebase_unspecified && opt_verbosity >= 0)\n> - show_advice_pull_non_ff();\n> + } else if (rebase_unspecified) {\n> + die_pull_non_ff();\n> }\n\nWhen reading the previous patch, I was wondering why an `if else`\nwasn't used, and here I see that it does become an `if else`. I guess\nyou didn't want to alter Alex's original patch?\n"},{"id":"430190","messageId":"60f001ddf33fa_2e67208e4@natae.notmuch","threadId":"56109","inReplyTo":"pull.1047.git.git.1626316849.gitgitgadget@gmail.com","subject":"RE: [PATCH 0/5] Handle conflicting pull options","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-15T09:37:34Z","receivedAt":"2021-07-15T09:37:38Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Elijah Newren via GitGitGadget wrote:\n> We've recently discussed how to handle conflicting configuration and command\n> line options in git pull, including at least a few different\n> proposals[1][2][3] to handle different subsets of the possibilities. We also\n> have a user report from someone who had conflicting configuration and got\n> surprised when one of the options overruled the other -- with no warning\n> from the command or the documentation that such would happen.\n\nNo, that's not what happened, you are assuming wrongly.\n\nIt's perfectly fine to have pull.ff and pull.rebase, because the former\nwill be honored when running `git pull --merge`, as I already explained\n[1].\n\n[1] http://lore.kernel.org/git/60ef1f6a83b61_9460a208cc@natae.notmuch\n\n-- \nFelipe Contreras\n"},{"id":"430191","messageId":"60f0036fa3c97_2e67208c3@natae.notmuch","threadId":"56109","inReplyTo":"3c07ce978caa832b08c6bef1c48c061e41a6fd0b.1626316849.git.gitgitgadget@gmail.com","subject":"RE: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-15T09:44:15Z","receivedAt":"2021-07-15T09:44:20Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> The --rebase[=...] flags and the various ff flags are incompatible,\n> except that --no-rebase (or --rebase=false) work with any of the ff\n> flags, and --ff works with any of the rebase flags.\n\nThis is wrong, the following commands should work\n\ngit -c pull.ff=only -c pull.rebase=true pull\ngit -c pull.ff=only -c pull.rebase=true pull --no-rebase\n\n-- \nFelipe Contreras\n"},{"id":"430193","messageId":"60f0045aad10c_2e6720891@natae.notmuch","threadId":"56109","inReplyTo":"7e12c45fc9a94e7b56a6efdc085ebe081dd40afc.1626316849.git.gitgitgadget@gmail.com","subject":"RE: [PATCH 5/5] pull: abort by default when fast-forwarding is not possible","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-15T09:48:10Z","receivedAt":"2021-07-15T09:48:14Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> We have for some time shown a long warning when the user does not\n> specify how to reconcile divergent branches with git pull. Make it an\n> error now.\n\nA warning doesn't imply deprecation.\n\nUsers have been warnged *zero* days that the default is going to change,\nto what it's going to change, how to configure the new behavior, or how\nto keep the old behavior.\n\nNAK.\n\n-- \nFelipe Contreras\n"},{"id":"430239","messageId":"CABPp-BGd8MVMwKHSg4Zx5t_cTQUhJL_XKO2f7pCCMQ8wXzPEQQ@mail.gmail.com","threadId":"56109","inReplyTo":"CAPig+cQUQYrkH7YsFio=JBZnaM3FRumti6OtvOTu6cUo4kWKtg@mail.gmail.com","subject":"Re: [PATCH 5/5] pull: abort by default when fast-forwarding is not possible","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-15T16:56:43Z","receivedAt":"2021-07-15T16:56:57Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jul 14, 2021 at 10:19 PM Eric Sunshine wrote:\n>\n> On Wed, Jul 14, 2021 at 10:41 PM Elijah Newren via GitGitGadget\n> wrote:\n> > We have for some time shown a long warning when the user does not\n> > specify how to reconcile divergent branches with git pull. Make it an\n> > error now.\n> >\n> > Signed-off-by: Elijah Newren \n>\n> Teeny tiny nits below...\n>\n> > diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\n> > @@ -15,14 +15,16 @@ SYNOPSIS\n> > -Incorporates changes from a remote repository into the current\n> > -branch. In its default mode, `git pull` is shorthand for\n> > -`git fetch` followed by `git merge FETCH_HEAD`.\n> > -\n> > -More precisely, 'git pull' runs 'git fetch' with the given\n> > -parameters and calls 'git merge' to merge the retrieved branch\n> > -heads into the current branch.\n> > -With `--rebase`, it runs 'git rebase' instead of 'git merge'.\n>\n> The original text has an odd mix of apostrophes and backticks...\n>\n> > +Incorporates changes from a remote repository into the current branch.\n> > +If the current branch is behind the remote, then by default it will\n> > +fast-forward the current branch to match the remote. If the current\n> > +branch and the remote have diverged, the user needs to specify how to\n> > +reconcile the divergent branches with --no-ff or --rebase (or the\n> > +corresponding configuration options in pull.ff or pull.rebase).\n> > +\n> > +More precisely, 'git pull' runs 'git fetch' with the given parameters\n> > +and then depending on config options or command line flags, will call\n> > +either 'git merge' or 'git rebase' to reconcile diverging branches.\n>\n> ... and the revised text adds \"no quotes\" to the selection. These\n> days, we'd probably backtick all of these:\n>\n> --no-ff\n> --rebase\n> pull.ff\n> pull.rebase\n> git pull\n> git fetch\n> git merge\n> git rebase\n>\n> The rest of this document is actually pretty good about using\n> backticks, though there are some exceptions.\n>\n> There's also an odd mix of \"configuration options\" and \"config\n> options\" in the revised text. Perhaps stick with \"configuration\n> options\" to be a bit more formal?\n>\n> > should be the name of a remote repository as\n> > passed to linkgit:git-fetch[1]. can name an\n>\n> And, as an aside, we'd backtick and , though\n> your patch isn't touching that, so outside the scope of this change.\n\nI'll try to fix these up, at least those in the nearby area I'm\nmodifying, if others agree with the general approach towards --ff-only\n& --no-ff vs. --rebase.\n\n> > diff --git a/builtin/pull.c b/builtin/pull.c\n> > @@ -1074,9 +1074,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> > if (opt_ff) {\n> > if (!strcmp(opt_ff, \"--ff-only\"))\n> > die_ff_impossible();\n> > - } else {\n> > - if (rebase_unspecified && opt_verbosity >= 0)\n> > - show_advice_pull_non_ff();\n> > + } else if (rebase_unspecified) {\n> > + die_pull_non_ff();\n> > }\n>\n> When reading the previous patch, I was wondering why an `if else`\n> wasn't used, and here I see that it does become an `if else`. I guess\n> you didn't want to alter Alex's original patch?\n\nYep. :-)\n"},{"id":"430241","messageId":"CABPp-BENqG5Mr4uPzbNQOxaj--f+mMe=fbeC1UTLA_zwqX34ew@mail.gmail.com","threadId":"56109","inReplyTo":"CAPig+cRZ7gFACO9eNtGmXjt_SG2oviXR7JjuW2w0wZeV7pMjkQ@mail.gmail.com","subject":"Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-15T17:13:59Z","receivedAt":"2021-07-15T17:14:13Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jul 14, 2021 at 10:00 PM Eric Sunshine wrote:\n>\n> On Wed, Jul 14, 2021 at 10:41 PM Elijah Newren via GitGitGadget\n> wrote:\n> > The --rebase[=...] flags and the various ff flags are incompatible,\n> > except that --no-rebase (or --rebase=false) work with any of the ff\n> > flags, and --ff works with any of the rebase flags.\n> > [...]\n> > Signed-off-by: Elijah Newren \n> > ---\n> > diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\n> > +test_expect_success 'report conflicting configuration' '\n> > + git reset --hard c2 &&\n> > + test_must_fail git -c pull.ff=false -c pull.rebase=true pull . c1 2>err &&\n> > + test_i18ngrep \"pull.rebase and pull.ff are incompatible; please unset one\" err\n> > +\n> > +'\n>\n> nit: unnecessary and inconsistent blank line at end of test\n\nGood catch, will fix (assuming the relevant folks agree with my\ngeneral --ff-only & --no-ff vs. --rebase handling approach).\n"},{"id":"430249","messageId":"xmqq7dhrtrc2.fsf@gitster.g","threadId":"56109","inReplyTo":"3c07ce978caa832b08c6bef1c48c061e41a6fd0b.1626316849.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-15T17:33:17Z","receivedAt":"2021-07-15T17:33:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> diff --git a/Documentation/config/pull.txt b/Documentation/config/pull.txt\n> index 54048306095..e70ed99e408 100644\n> --- a/Documentation/config/pull.txt\n> +++ b/Documentation/config/pull.txt\n> @@ -7,12 +7,13 @@ pull.ff::\n> \tline). When set to `only`, only such fast-forward merges are\n> \tallowed (equivalent to giving the `--ff-only` option from the\n> \tcommand line). This setting overrides `merge.ff` when pulling.\n> +\tIncompatible with pull.rebase.\n\nThis update may mean well, but I sense that the description needs to\nbe tightened up a bit more. Unless you mean to say that I am not\nallowed to say \"[pull] rebase=no\" when I set \"[pull] ff\", that is.\n\nOr do you mean pull.ff=only is incompatible with any setting of\npull.rebase?\n\nOr do you mean pull.ff=only is incompatible with any setting of\npull.rebase other than false?\n\nOr do you mean any setting of pull.ff is imcompatible with any\nsetting of pull.rebase other than false?\n\n(You do not have to answer---the above questions just demonstrate\nthat the description is unnecessarily loose).\n\n> pull.rebase::\n> \tWhen true, rebase branches on top of the fetched branch, instead\n> \tof merging the default branch from the default remote when \"git\n> \tpull\" is run. See \"branch..rebase\" for setting this on a\n> -\tper-branch basis.\n> +\tper-branch basis. Incompatible with pull.ff.\n\nLikewise.\n\nI think it technically is OK to say \"only ff update is allowed\" or\n\"ff update is allowed when able\" while saying pull.rebase=yes. \n\n - For those who say ff=only, the actual value of pull.rebase would\n not matter (i.e. the integration immediately finishes by updating\n to what was obtained from the upstream because there is no new\n development on our own on top of theirs to either replay or\n merge).\n\n - For those who merely allow ff, an update may fast-forward even\n when pull.rebase is set to true (or any non-false value), while\n we'll replay our own development on top of theirs when their\n history is not descendent of ours.\n\nSo I do not see need to declare these combinations \"incompatible\".\nBut perhaps I am missing some subtle cases?\n\n> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\n> index 5c3fb67c014..03e8694e146 100644\n> --- a/Documentation/git-pull.txt\n> +++ b/Documentation/git-pull.txt\n> @@ -121,6 +121,9 @@ When false, merge the current branch into the upstream branch.\n> +\n> When `interactive`, enable the interactive mode of rebase.\n> +\n> +Note that these flags are incompatible with --no-ff and --ff-only; if\n> +such incompatible flags are given, the last one will take precedence.\n> ++\n\nI am not sure about these, either. Again, --ff-only (whether it is\ncombined with --rebase or --rebase=no) would mean that the operation\nwould fail when we have our own development on top of their history,\nand we repoint the tip of our history to theirs when we do not.\n\nWe see \"--no-ff\" is explained to \"always create an unnecessary extra\nmerge\", bit I am not sure it is the right mental model to apply when\nthe user prefers rebasing. The merge workflow is to treat our\nhistory as the primary and merging theirs in, so when theirs is a\ndescendant (i.e. we have no new development of our own), some people\nmay want to mark \"we were there before we updated from them\" with an\nextra merge. Without --no-ff, the resulting history would be quite\ndifferent in the merge workflow if you have or do not have your own\ndevelopment. But the rebase workflow is to treat their history as\nthe primary and replay our own development on top of theirs, and the\nshape of the resulting history would be the same whether you have\nyour own development on top of theirs. We start from their tip and\nthen replay our own development on top (which could be an empty\nset), and there is nothing \"--no-ff\" would need to do differently to\nkeep the resulting history similar to cases where we do have\nsomething of our own. IOW, \"--no-ff\" becoming a no-op in a \"rebase\"\nworkflow is a natural consequence, and there is no strong reason to\nsay it is incompatible.\n"},{"id":"430256","messageId":"60f0747b149eb_4b682084c@natae.notmuch","threadId":"56109","inReplyTo":"xmqq7dhrtrc2.fsf@gitster.g","subject":"Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-15T17:46:35Z","receivedAt":"2021-07-15T17:46:40Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> \"Elijah Newren via GitGitGadget\" writes:\n\n> > --- a/Documentation/config/pull.txt\n> > +++ b/Documentation/config/pull.txt\n\n> > pull.rebase::\n> > \tWhen true, rebase branches on top of the fetched branch, instead\n> > \tof merging the default branch from the default remote when \"git\n> > \tpull\" is run. See \"branch..rebase\" for setting this on a\n> > -\tper-branch basis.\n> > +\tper-branch basis. Incompatible with pull.ff.\n> \n> Likewise.\n> \n> I think it technically is OK to say \"only ff update is allowed\" or\n> \"ff update is allowed when able\" while saying pull.rebase=yes. \n> \n> - For those who say ff=only, the actual value of pull.rebase would\n> not matter (i.e. the integration immediately finishes by updating\n> to what was obtained from the upstream because there is no new\n> development on our own on top of theirs to either replay or\n> merge).\n> \n> - For those who merely allow ff, an update may fast-forward even\n> when pull.rebase is set to true (or any non-false value), while\n> we'll replay our own development on top of theirs when their\n> history is not descendent of ours.\n> \n> So I do not see need to declare these combinations \"incompatible\".\n> But perhaps I am missing some subtle cases?\n\nDoing such a thing would be wrong. As I already explained multiple\ntimes, pull.rebase can be overriden by the command line. When doing\n--merge we want to honor pull.ff, when we don't we want to ignore it.\n\nSince both Elijah and Junio have muted me, and they keep making these\nmistakes, can anybody else forward this point to them?\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"430262","messageId":"CABPp-BExWMSFr7CQskjKVhr5fiWCnxoaN_RaJ2Yir+36aiyBjQ@mail.gmail.com","threadId":"56109","inReplyTo":"xmqq7dhrtrc2.fsf@gitster.g","subject":"Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-15T19:04:38Z","receivedAt":"2021-07-15T19:18:18Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jul 15, 2021 at 10:33 AM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > diff --git a/Documentation/config/pull.txt b/Documentation/config/pull.txt\n> > index 54048306095..e70ed99e408 100644\n> > --- a/Documentation/config/pull.txt\n> > +++ b/Documentation/config/pull.txt\n> > @@ -7,12 +7,13 @@ pull.ff::\n> > line). When set to `only`, only such fast-forward merges are\n> > allowed (equivalent to giving the `--ff-only` option from the\n> > command line). This setting overrides `merge.ff` when pulling.\n> > + Incompatible with pull.rebase.\n>\n> This update may mean well, but I sense that the description needs to\n> be tightened up a bit more. Unless you mean to say that I am not\n> allowed to say \"[pull] rebase=no\" when I set \"[pull] ff\", that is.\n>\n> Or do you mean pull.ff=only is incompatible with any setting of\n> pull.rebase?\n>\n> Or do you mean pull.ff=only is incompatible with any setting of\n> pull.rebase other than false?\n>\n> Or do you mean any setting of pull.ff is imcompatible with any\n> setting of pull.rebase other than false?\n>\n> (You do not have to answer---the above questions just demonstrate\n> that the description is unnecessarily loose).\n\nFair enough.\n\n> > pull.rebase::\n> > When true, rebase branches on top of the fetched branch, instead\n> > of merging the default branch from the default remote when \"git\n> > pull\" is run. See \"branch..rebase\" for setting this on a\n> > - per-branch basis.\n> > + per-branch basis. Incompatible with pull.ff.\n>\n> Likewise.\n>\n> I think it technically is OK to say \"only ff update is allowed\" or\n> \"ff update is allowed when able\" while saying pull.rebase=yes.\n>\n> - For those who say ff=only, the actual value of pull.rebase would\n> not matter (i.e. the integration immediately finishes by updating\n> to what was obtained from the upstream because there is no new\n> development on our own on top of theirs to either replay or\n> merge).\n>\n> - For those who merely allow ff, an update may fast-forward even\n> when pull.rebase is set to true (or any non-false value), while\n> we'll replay our own development on top of theirs when their\n> history is not descendent of ours.\n>\n> So I do not see need to declare these combinations \"incompatible\".\n> But perhaps I am missing some subtle cases?\n\nLet me ask two questions:\n\n1. When is it beneficial for users to set both pull.ff and pull.rebase?\n2. Is it harmful to users for us to allow both to be set when we will\njust ignore one?\n\nI believe the answer to (1) is \"never\", and the answer to (2) is \"yes\".\n\nFor the second question in particular, I can think of two example cases:\n\n2a) Users start with pull.ff=only, perhaps suggested by someone else\nand left in their config for a long time. When users hit a case that\ncan't fast-forward and they either ask for help or find a post on\nstack overflow that suggests setting pull.rebase=true, they do so and\nthen get no warning that the setting they just added is being ignored.\n\n2b) Perhaps users start with pull.rebase=true (suggested by a\ncolleague and forgot about it as they are more of a tester than a\ndeveloper and thus usually only see fast-forwards). Then at some\npoint they need to function as an integrator, and they read the docs\nand determine that pull.ff=false should do what they want to create\nmerge commits. But then they get shocked that they've rebased public\ncommits (and perhaps also pushed them out) when they wanted merges.\nOur docs have pretty clearly stated that pull.ff=false and --no-ff\ncreate merges.\n\nAdditionally, pull.rebase=interactive seems like a case that interacts\npoorly with pull.ff. It may be that if you disagree that pull.rebase\nand pull.ff are more generally incompatible, then we may still need to\nmicro-document individual incompatibilities. (And does that list grow\nand get confusing?)\n\n> > diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\n> > index 5c3fb67c014..03e8694e146 100644\n> > --- a/Documentation/git-pull.txt\n> > +++ b/Documentation/git-pull.txt\n> > @@ -121,6 +121,9 @@ When false, merge the current branch into the upstream branch.\n> > +\n> > When `interactive`, enable the interactive mode of rebase.\n> > +\n> > +Note that these flags are incompatible with --no-ff and --ff-only; if\n> > +such incompatible flags are given, the last one will take precedence.\n> > ++\n>\n> I am not sure about these, either. Again, --ff-only (whether it is\n> combined with --rebase or --rebase=no) would mean that the operation\n> would fail when we have our own development on top of their history,\n> and we repoint the tip of our history to theirs when we do not.\n>\n> We see \"--no-ff\" is explained to \"always create an unnecessary extra\n> merge\", bit I am not sure it is the right mental model to apply when\n> the user prefers rebasing. The merge workflow is to treat our\n> history as the primary and merging theirs in, so when theirs is a\n> descendant (i.e. we have no new development of our own), some people\n> may want to mark \"we were there before we updated from them\" with an\n> extra merge. Without --no-ff, the resulting history would be quite\n> different in the merge workflow if you have or do not have your own\n> development. But the rebase workflow is to treat their history as\n> the primary and replay our own development on top of theirs, and the\n> shape of the resulting history would be the same whether you have\n> your own development on top of theirs. We start from their tip and\n> then replay our own development on top (which could be an empty\n> set), and there is nothing \"--no-ff\" would need to do differently to\n> keep the resulting history similar to cases where we do have\n> something of our own. IOW, \"--no-ff\" becoming a no-op in a \"rebase\"\n> workflow is a natural consequence, and there is no strong reason to\n> say it is incompatible.\n\nI think you're overlooking a variety of ways this could harm the user.\n\nBut first let me check if I understand you correctly: I think you're\nsuggesting that\n * if --ff-only is given along with any --rebase[=*] flags, then we\nsilently ignore the --rebase[=*] flags.\n * if --no-ff is given along with any --rebase flag other than\n--rebase=false, then we silently ignore --no-ff.\n\nLet me ask the same two questions as for the config: Does it benefit\nthe users to allow them to specify both flags? Does it hurt some\nusers, especially if undocumented that one of the flags will be\nignored?\n\nI think the same usecases from the config settings apply, though the\nfact that both flags are present simultaneously on the command line at\nleast makes it less problematic in the case of --ff-only (users will\nmore quickly discover why their --rebase= flag was ignored).\nBut there's an additional reason why treating --no-ff as a no-op is\nharmful when --rebase is present:\n\ngit rebase has a --no-ff flag (a synonym for --force-rebase). Some\nusers may intend for git pull --rebase --no-ff to run `git fetch\norigin && git rebase --no-ff FETCH_HEAD`. The fact that --no-ff will\nbe ignored and sometimes results in a fast-forward when the user\nexplicitly requested no fast forward seems buggy.\n\nI thought this was the biggest hole in my proposal: there is both a\ngit merge --no-ff and a git rebase --no-ff, and they avoid\nfast-forwards in different ways with different results. It _might_ be\nbeneficial to allow both to be triggered directly from the pull\ncommand. However, allowing both has some backward compatibility\nproblems because (1) --no-ff is documented as making a merge, and (2)\npeople could have pull.rebase=true set and forget about it and then\nget surprised when they add --no-ff to the command line and get a\nrebase --no-ff (or even a plain rebase without --no-ff) rather than\nthe documented merge --no-ff. I think the split config/parameters of\ngit pull make it really hard for us to determine whether users intend\nto get a merge --no-ff or rebase --no-ff.\n\nI think silently ignoring and not even documenting these pitfalls is\nproblematic. We should at least document them.\n"},{"id":"430264","messageId":"xmqqpmvjs61c.fsf@gitster.g","threadId":"56109","inReplyTo":"CABPp-BExWMSFr7CQskjKVhr5fiWCnxoaN_RaJ2Yir+36aiyBjQ@mail.gmail.com","subject":"Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-15T19:58:39Z","receivedAt":"2021-07-15T19:59:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Let me ask two questions:\n>\n> 1. When is it beneficial for users to set both pull.ff and pull.rebase?\n> 2. Is it harmful to users for us to allow both to be set when we will\n> just ignore one?\n>\n> I believe the answer to (1) is \"never\", and the answer to (2) is \"yes\".\n\nI agree (1) never gives you anything, even though it does not hurt,\nand (2) is \"meh\".\n\n> For the second question in particular, I can think of two example cases:\n>\n> 2a) Users start with pull.ff=only, perhaps suggested by someone else\n> and left in their config for a long time. When users hit a case that\n> can't fast-forward and they either ask for help or find a post on\n> stack overflow that suggests setting pull.rebase=true, they do so and\n> then get no warning that the setting they just added is being ignored.\n\nWell, overriding \"only fast-forward is allowed\" with \"instead of\nmerge, you can rebase\" is a nonsense suggestion in the first place,\nno? Why does Git suddenly become responsible for such a misguided\nsuggestion?\n\n> 2b) Perhaps users start with pull.rebase=true (suggested by a\n> colleague and forgot about it as they are more of a tester than a\n> developer and thus usually only see fast-forwards). Then at some\n> point they need to function as an integrator, and they read the docs\n> and determine that pull.ff=false should do what they want to create\n> merge commits.\n\nAgain, \"I want to pee in the snow\" is not what you need to act as an\nintegrator. I do not see how relevant this example is, either. You\nare just reacting to a wrong suggestion.\n\n> But then they get shocked that they've rebased public\n> commits (and perhaps also pushed them out) when they wanted merges.\n> Our docs have pretty clearly stated that pull.ff=false and --no-ff\n> create merges.\n\nThat is something we need to and can fix. The \"pee in the snow\ncommit can be created by passing --no-ff\" was written back when the\ndesigned audiences of \"pull\" were primarily those who merge (think\nof \"pull --rebase\" as afterthought). IOW, to the minds of those who\noriginally wrote --no-ff feature (and its doc), \"pull --rebase\" was\nnot in the picture.\n\n"},{"id":"430265","messageId":"xmqqlf67s560.fsf@gitster.g","threadId":"56109","inReplyTo":"CABPp-BExWMSFr7CQskjKVhr5fiWCnxoaN_RaJ2Yir+36aiyBjQ@mail.gmail.com","subject":"Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-15T20:17:27Z","receivedAt":"2021-07-15T20:18:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> But first let me check if I understand you correctly: I think you're\n> suggesting that\n> * if --ff-only is given along with any --rebase[=*] flags, then we\n> silently ignore the --rebase[=*] flags.\n> * if --no-ff is given along with any --rebase flag other than\n> --rebase=false, then we silently ignore --no-ff.\n>\n> Let me ask the same two questions as for the config: Does it benefit\n> the users to allow them to specify both flags? Does it hurt some\n> users, especially if undocumented that one of the flags will be\n> ignored?\n\nI see downsides if you make them \"incompatible\" and cause the\ncombination to error out, but otherwise no. I can imagine those who\nregularly use pull.rebase=yes in the configuration to say --ff-only\nfor a single-shot, for example. We can view it as either (1)\ncommand line --ff-only overriding configured pull.rebase, or (2)\nff-only made the choice of pull.rebase irrelevant---the end result\nis the same (as long as you do not say \"no, that's incompatible\" and\nerror out).\n\n> I thought this was the biggest hole in my proposal: there is both a\n> git merge --no-ff and a git rebase --no-ff, and they avoid\n> fast-forwards in different ways with different results.\n\nWhen you say \"git rebase --no-ff upstream\", you are telling the\nmachinery that even if your current work is direct descendant of the\nupstream, you want your commits freshly rebuilt. But in the context\nof \"pull\", you being descendant of the other side is called \"already\nup to date\", not even \"fast forward\" (the ancestry relationship\nbetween you and the other side is the other way around). Does the\n\"rebase --no-ff\" even kick in in the context of \"pull --no-ff\"?\n"},{"id":"430266","messageId":"CABPp-BEhQqG=AtqVzW0Ud0NNPJ5SCXYBWoNVq9_7ERfPYLdhNw@mail.gmail.com","threadId":"56109","inReplyTo":"xmqqlf67s560.fsf@gitster.g","subject":"Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-15T20:38:02Z","receivedAt":"2021-07-15T20:38:21Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jul 15, 2021 at 1:17 PM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > I thought this was the biggest hole in my proposal: there is both a\n> > git merge --no-ff and a git rebase --no-ff, and they avoid\n> > fast-forwards in different ways with different results.\n>\n> When you say \"git rebase --no-ff upstream\", you are telling the\n> machinery that even if your current work is direct descendant of the\n> upstream, you want your commits freshly rebuilt. But in the context\n> of \"pull\", you being descendant of the other side is called \"already\n> up to date\", not even \"fast forward\" (the ancestry relationship\n> between you and the other side is the other way around).\n\nOh, good point.\n"},{"id":"430267","messageId":"CABPp-BE8Qiu8Sdk8FD+UcAtZnToXFOv+Y+8Rwf3DyiZP6Te-SQ@mail.gmail.com","threadId":"56109","inReplyTo":"xmqqpmvjs61c.fsf@gitster.g","subject":"Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-15T20:40:04Z","receivedAt":"2021-07-15T20:40:26Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jul 15, 2021 at 12:58 PM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > Let me ask two questions:\n> >\n> > 1. When is it beneficial for users to set both pull.ff and pull.rebase?\n> > 2. Is it harmful to users for us to allow both to be set when we will\n> > just ignore one?\n> >\n> > I believe the answer to (1) is \"never\", and the answer to (2) is \"yes\".\n>\n> I agree (1) never gives you anything, even though it does not hurt,\n> and (2) is \"meh\".\n\nOkay, let's drop this series then. Thanks for pointing out my mix-up\non the rebase --no-ff thing in the other email.\n"},{"id":"430269","messageId":"xmqqh7gvs2mu.fsf@gitster.g","threadId":"56109","inReplyTo":"CABPp-BE8Qiu8Sdk8FD+UcAtZnToXFOv+Y+8Rwf3DyiZP6Te-SQ@mail.gmail.com","subject":"Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-15T21:12:09Z","receivedAt":"2021-07-15T21:12:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> On Thu, Jul 15, 2021 at 12:58 PM Junio C Hamano wrote:\n>>\n>> Elijah Newren writes:\n>>\n>> > Let me ask two questions:\n>> >\n>> > 1. When is it beneficial for users to set both pull.ff and pull.rebase?\n>> > 2. Is it harmful to users for us to allow both to be set when we will\n>> > just ignore one?\n>> >\n>> > I believe the answer to (1) is \"never\", and the answer to (2) is \"yes\".\n>>\n>> I agree (1) never gives you anything, even though it does not hurt,\n>> and (2) is \"meh\".\n>\n> Okay, let's drop this series then.\n\nNot so fast. I did have problem with some combinations you hinted\n(vaguely---so it is more like \"combinations I thought you hinted\"),\nbut making sure various combinations of options and configuration\nvariables work sensibly is a worthy goal to have, I would think.\n\n\n\n"},{"id":"430315","messageId":"87czriy557.fsf@evledraar.gmail.com","threadId":"56109","inReplyTo":"7e12c45fc9a94e7b56a6efdc085ebe081dd40afc.1626316849.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/5] pull: abort by default when fast-forwarding is not possible","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-16T09:32:06Z","receivedAt":"2021-07-16T09:33:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jul 15 2021, Elijah Newren via GitGitGadget wrote:\n\n> -static void show_advice_pull_non_ff(void)\n> +static void NORETURN die_pull_non_ff(void)\n> {\n> -\tadvise(_(\"Pulling without specifying how to reconcile divergent branches is\\n\"\n> -\t\t \"discouraged. You can squelch this message by running one of the following\\n\"\n> -\t\t \"commands sometime before your next pull:\\n\"\n> -\t\t \"\\n\"\n> -\t\t \" git config pull.rebase false # merge (the default strategy)\\n\"\n> -\t\t \" git config pull.rebase true # rebase\\n\"\n> -\t\t \" git config pull.ff only # fast-forward only\\n\"\n> -\t\t \"\\n\"\n> -\t\t \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n> -\t\t \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n> -\t\t \"or --ff-only on the command line to override the configured default per\\n\"\n> -\t\t \"invocation.\\n\"));\n> +\tdie(_(\"You have divergent branches and need to specify how to reconcile them.\\n\"\n> +\t \"You can do so by running one of the following commands sometime before\\n\"\n> +\t \"your next pull:\\n\"\n> +\t \"\\n\"\n> +\t \" git config pull.rebase false # merge (the default strategy)\\n\"\n> +\t \" git config pull.rebase true # rebase\\n\"\n> +\t \" git config pull.ff only # fast-forward only\\n\"\n> +\t \"\\n\"\n> +\t \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n> +\t \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n> +\t \"or --ff-only on the command line to override the configured default per\\n\"\n> +\t \"invocation.\\n\"));\n> }\n\nDying and advise() shouldn't be mutually exclusive, we should reword the\nadvise() message to idate for this being a die(), and then:\n\n> int cmd_pull(int argc, const char **argv, const char *prefix)\n> @@ -1074,9 +1074,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\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} else if (rebase_unspecified) {\n> +\t\t\tdie_pull_non_ff();\n> \t\t}\n\nHere we should:\n\n show_advice_pull_non_ff();\n die(_(\"some much briefer summary\"))\n\nI.e. we should not being showing giantic advice-y die() messages, the\ndie messages should always be brief, but we might also show advice just\nbefore dying.\n"},{"id":"430368","messageId":"60f1cc4a71af9_3302082a@natae.notmuch","threadId":"56109","inReplyTo":"87czriy557.fsf@evledraar.gmail.com","subject":"Re: [PATCH 5/5] pull: abort by default when fast-forwarding is not possible","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-16T18:13:30Z","receivedAt":"2021-07-16T18:13:35Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Thu, Jul 15 2021, Elijah Newren via GitGitGadget wrote:\n\n> > int cmd_pull(int argc, const char **argv, const char *prefix)\n> > @@ -1074,9 +1074,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\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} else if (rebase_unspecified) {\n> > +\t\t\tdie_pull_non_ff();\n> > \t\t}\n> \n> Here we should:\n> \n> show_advice_pull_non_ff();\n> die(_(\"some much briefer summary\"))\n> \n> I.e. we should not being showing giantic advice-y die() messages, the\n> die messages should always be brief, but we might also show advice just\n> before dying.\n\nIndeed, just like my proposal does:\n\n\t\tdiverging_advice();\n\t\tdie(_(\"The pull was not fast-forward, either merge or rebase.\\n\"));\n\n-- \nFelipe Contreras"},{"id":"430370","messageId":"CABPp-BEHNpfm6mJBDZ30wCEraT03p+2-gGZyZcnUV34trAzAzw@mail.gmail.com","threadId":"56109","inReplyTo":"xmqqh7gvs2mu.fsf@gitster.g","subject":"Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-16T18:39:50Z","receivedAt":"2021-07-16T18:40:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jul 15, 2021 at 2:12 PM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > On Thu, Jul 15, 2021 at 12:58 PM Junio C Hamano wrote:\n> >>\n> >> Elijah Newren writes:\n> >>\n> >> > Let me ask two questions:\n> >> >\n> >> > 1. When is it beneficial for users to set both pull.ff and pull.rebase?\n> >> > 2. Is it harmful to users for us to allow both to be set when we will\n> >> > just ignore one?\n> >> >\n> >> > I believe the answer to (1) is \"never\", and the answer to (2) is \"yes\".\n> >>\n> >> I agree (1) never gives you anything, even though it does not hurt,\n> >> and (2) is \"meh\".\n> >\n> > Okay, let's drop this series then.\n>\n> Not so fast. I did have problem with some combinations you hinted\n> (vaguely---so it is more like \"combinations I thought you hinted\"),\n> but making sure various combinations of options and configuration\n> variables work sensibly is a worthy goal to have, I would think.\n\nIt may be a worthy goal, but I cannot implement correct behavior if I\ncannot determine what correct behavior is.\n\nYou've only specified how to handle a subset of the valid combinations\nin each of your emails, and from those individually or even\ncollectively I cannot deduce rules for handling the others. Reading\nthe dozen+ recent messages in the various recent threads, I think I've\nfigured out your opinion in all but two cases, but I have no idea your\nintent on those two (I would have thought --rebase override there too,\nbut you excluded that), and I'm rather uncertain I've correctly\nunderstood you for the other ones (I really hope gmail doesn't\nwhitespace damage the following table):\n\n pull.ff pull.rebase commandline action\n * * --ff-only --rebase fast-forward only[1]\n * * --rebase --no-ff rebase[1]\n * * --rebase --ff rebase[1]\n * * --ff-only --no-rebase fast-forward only\n * * --no-rebase --no-ff merge --no-ff\n * * --no-rebase --ff merge --ff\n\n * --no-rebase merge --ff\n only * --no-rebase merge --ff[2]\n false * --no-rebase merge --no-ff\n true * --no-rebase merge --ff\n\n * --rebase rebase\n only * --rebase rebase[2]\n false * --rebase ?[2]\n true * --rebase ?[2]\n\n * * --ff-only fast-forward only[1]\n\n * --no-ff merge --no-ff\n * false --no-ff merge --no-ff\n * !false --no-ff rebase (ignore --no-ff)[2][3]\n\n * --ff merge --ff\n * false --ff merge --ff\n * !false --ff rebase (ignore --ff)[2][3]\n\n[1] https://lore.kernel.org/git/xmqq7dhrtrc2.fsf@gitster.g/\n https://lore.kernel.org/git/c62933fb-96b2-99f5-7169-372f486f6e39@aixigo.com/\n[2] https://lore.kernel.org/git/xmqqpmvn5ukj.fsf@gitster.g/\n[3] https://lore.kernel.org/git/xmqq8s2b489p.fsf@gitster.g/\n\nIt appears you, Phillip, and I all had different opinions about\ncorrect behavior and in a few cases though the documentation clearly\nimplied what we thought. So, I'd have to say the documentation is\nrather unclear as well. However, even if the above table is filled\nout, it may be complicated enough that I'm at a bit of a loss about\nhow to update the documentation to explain it short of including the\ntable in the documentation.\n"},{"id":"430394","messageId":"xmqqwnpqot4m.fsf@gitster.g","threadId":"56109","inReplyTo":"CABPp-BEHNpfm6mJBDZ30wCEraT03p+2-gGZyZcnUV34trAzAzw@mail.gmail.com","subject":"Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-16T21:18:01Z","receivedAt":"2021-07-16T21:18:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> the dozen+ recent messages in the various recent threads, I think I've\n> figured out your opinion in all but two cases, but I have no idea your\n> intent on those two (I would have thought --rebase override there too,\n> but you excluded that), and I'm rather uncertain I've correctly\n> understood you for the other ones (I really hope gmail doesn't\n> whitespace damage the following table):\n\nYeah, it's about time somebody made an exhaustive table of cases ;-)\n\nI appreciate it very much. Without such a table, it is hard to have\ndiscussion and keep track of which part is and which part is not\nwhat people agree to.\n\nBelow, I rearranged some rows to group cases I think belong\ntogether to make commenting on them easier, but I didn't remove or\nadd any rows.\n\n> pull.ff pull.rebase commandline action\n> * * --ff-only --rebase fast-forward only[1]\n> * * --ff-only --no-rebase fast-forward only\n> * * --ff-only fast-forward only[1]\n\nI think these three make sense. As you answered to a question in\nanother thread, I agree that --ff-only from the command line should\nfail instead of letting rebase (or merge for that matter) go ahead\nwhen their history is not a descendant of ours and it is a bug if it\ndoes not.\n\n> * * --rebase --no-ff rebase[1]\n> * * --rebase --ff rebase[1]\n\nOK.\n\n> * * --no-rebase --no-ff merge --no-ff\n> * * --no-rebase --ff merge --ff\n\nOK.\n\n> * --no-rebase merge --ff\n> only * --no-rebase merge --ff[2]\n> false * --no-rebase merge --no-ff\n> true * --no-rebase merge --ff\n\nI think the second one deserves an explanation. The rationale for\nignoring --ff-only is because the act of giving an explicit\n\"--rebase\" or \"--no-rebase\" from the command line, when the\nconfigured default is \"I expect to have no development on my own\nhere, and only want to follow along\", is a sign enough that the user\ndoes not want to follow along in this particular invocation of\n\"pull\". And the rationale for the entire thing to become --ff is\nonly because between --ff and --no-ff, the former is the default.\n\nAbout the second one, I would understand it if it became \"merge\n--ff-only\", too. That is more trivially explained. I however\nsuspect that it would be less useful, but that is open to\ndiscussion.\n\nAll three others I think are not controversial.\n\n> * --rebase rebase\n> only * --rebase rebase[2]\n> false * --rebase ?[2]\n> true * --rebase ?[2]\n\nAgain the same reasoning applies to the second case that defeats the\nconfigured pull.ff=only setting. pull.ff=unset and pull.ff=true\nwould be handled the same, so only the pull.ff=false might deserve\nan explanation. I not think there needs anything special, as there\nis no \"I want to do something special to mark the fact that we tried\nto integrate their work\" special case needed even when their history\nis descendant of ours in \"rebase\" workflow, where we replay what we\ndid on top of theirs (in other words, in \"merge\" workflow, the shape\nof history left with --no-ff would be different from --ff; in \"rebase\",\nthere is no difference---either you had your own development before\nyou ran \"pull --rebase\" and they got replayed on top of their history,\nor you had nothing before you ran \"pull --rebase\" and you did your\ndevelopment after that on top of their history, you'd end up with\nhistories of the same shape. This would not happen with \"merge\"\nworkflow and that is why --no-ff may matter).\n\nSo, I agree with the above 4 (and the other 4 for --no-rebase)\nentries.\n\n\n> * --no-ff merge --no-ff\n> * false --no-ff merge --no-ff\n> * --ff merge --ff\n> * false --ff merge --ff\n\nOK.\n\n> * !false --no-ff rebase (ignore --no-ff)[2][3]\n> * !false --ff rebase (ignore --ff)[2][3]\n\nOK. We do specified rebase, allowing fast-forward.\n\npull.rebase= combined with --no-ff could mean \"I do\nexpect I have some development of my own, and please stop me if I\ndon't\" and instead of rebasing nothing, error out without even\nrepointing our branch tip to their history when it fast forwards,\nbut I suspect that it would be less useful. Again, that (and\nanything else I said here) is open to discussion.\n\n> It appears you, Phillip, and I all had different opinions about\n> correct behavior and in a few cases though the documentation clearly\n> implied what we thought. So, I'd have to say the documentation is\n> rather unclear as well. However, even if the above table is filled\n> out, it may be complicated enough that I'm at a bit of a loss about\n> how to update the documentation to explain it short of including the\n> table in the documentation.\n\nThanks.\n"},{"id":"430398","messageId":"60f200a49dac3_14cb2089b@natae.notmuch","threadId":"56109","inReplyTo":"xmqqwnpqot4m.fsf@gitster.g","subject":"Re: [PATCH 3/5] pull: handle conflicting rebase/merge options via last option wins","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-16T21:56:52Z","receivedAt":"2021-07-16T21:57:02Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Elijah Newren writes:\n> \n> > * --no-rebase merge --ff\n> > only * --no-rebase merge --ff[2]\n> > false * --no-rebase merge --no-ff\n> > true * --no-rebase merge --ff\n> \n> I think the second one deserves an explanation. The rationale for\n> ignoring --ff-only is because the act of giving an explicit\n> \"--rebase\" or \"--no-rebase\" from the command line, when the\n> configured default is \"I expect to have no development on my own\n> here, and only want to follow along\", is a sign enough that the user\n> does not want to follow along in this particular invocation of\n> \"pull\". And the rationale for the entire thing to become --ff is\n> only because between --ff and --no-ff, the former is the default.\n> \n> About the second one, I would understand it if it became \"merge\n> --ff-only\", too. That is more trivially explained. I however\n> suspect that it would be less useful, but that is open to\n> discussion.\n\nIt would be very hard to explain in the documentation why these are\ndifferent:\n\n git -c pull.ff=only pull --merge\n git pull --ff-only --merge\n\nAnd this of course will break behavior for people that arely already\nrelying on pull.ff=only to do --ff-only (as it was its whole purpose).\n\nNot to mention that this isn't even listed in the table:\n\n git -c pull.ff=only pull\n\nAs well as *all* the configurations with no command line arguments.\n\n-- \nFelipe Contreras\n"}]}