{"thread":{"id":"54807","subject":"[PATCH v5 0/3] pull: stop warning on every pull","startedAt":"2020-12-10T10:06:25Z","lastAt":"2020-12-14T21:41:53Z","messageCount":26,"participants":["Felipe Contreras","Junio C Hamano"],"isPatch":true,"patchVersion":5,"patchTotal":3},"messages":[{"id":"411987","messageId":"20201210100538.696787-1-felipe.contreras@gmail.com","threadId":"54807","inReplyTo":null,"subject":"[PATCH v5 0/3] pull: stop warning on every pull","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-10T10:05:35Z","receivedAt":"2020-12-10T10:06:25Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"The discussion about making fast-forward-only pulls the default is stuck on mud, and there's no\nagreement about what we should even be warning our users about.\n\nEven my straightforward patches about improving documentation, and the consistency of the UI with\n--merge and other obvious fixes lost traction.\n\nThis is not the totality of part I, it's doing one thing, and one thing only: stops warning the\nusers on every single pull (warns only if they are not-fast-forward).\n\nEverything else is dropped.\n\ndiff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt\nindex 21b50aff77..5c3fb67c01 100644\n--- a/Documentation/git-pull.txt\n+++ b/Documentation/git-pull.txt\n@@ -38,20 +38,6 @@ as set by linkgit:git-branch[1] `--track`.\n Assume the following history exists and the current branch is\n \"`master`\":\n \n-------------\n-\t A---B---C master on origin\n-\t /\n- D---E master\n-------------\n-\n-Then `git pull` will merge in a fast-forward way up to the new master.\n-\n-------------\n- D---E---A---B---C master, origin/master\n-------------\n-\n-However, a non-fast-forward case looks very different.\n-\n ------------\n \t A---B---C master on origin\n \t /\n@@ -60,9 +46,6 @@ However, a non-fast-forward case looks very different.\n \torigin/master in your repository\n ------------\n \n-By default `git pull` will warn about these situations, however, most likely\n-you would want to force a merge, which you can do with `git pull --merge`.\n-\n Then \"`git pull`\" will fetch and replay the changes from the remote\n `master` branch since it diverged from the local `master` (i.e., `E`)\n until its current commit (`C`) on top of `master` and record the\n@@ -148,11 +131,8 @@ It rewrites history, which does not bode well when you\n published that history already. Do *not* use this option\n unless you have read linkgit:git-rebase[1] carefully.\n \n--m::\n---merge::\n-\tForce a merge.\n-+\n-Previously this was --no-rebase, but that usage has been deprecated.\n+--no-rebase::\n+\tOverride earlier --rebase.\n \n Options related to fetching\n ~~~~~~~~~~~~~~~~~~~~~~~~~~~\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 118fbdeb62..9a7caf3a3e 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -27,6 +27,8 @@\n #include \"commit-reach.h\"\n #include \"sequencer.h\"\n \n+static int default_mode;\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@@ -74,7 +76,7 @@ static char *opt_progress;\n static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n \n /* Options passed to git-merge or git-rebase */\n-static enum rebase_type opt_rebase;\n+static enum rebase_type opt_rebase = -1;\n static char *opt_diffstat;\n static char *opt_log;\n static char *opt_signoff;\n@@ -126,11 +128,9 @@ static struct option pull_options[] = {\n \t/* Options passed to git-merge or git-rebase */\n \tOPT_GROUP(N_(\"Options related to merging\")),\n \tOPT_CALLBACK_F('r', \"rebase\", &opt_rebase,\n-\t\t\"(false|true|merges|preserve|interactive)\",\n-\t\tN_(\"incorporate changes by rebasing rather than merging\"),\n-\t\tPARSE_OPT_OPTARG, parse_opt_rebase),\n-\tOPT_SET_INT('m', \"merge\", &opt_rebase,\n-\t\tN_(\"incorporate changes by merging\"), REBASE_FALSE),\n+\t \"(false|true|merges|preserve|interactive)\",\n+\t N_(\"incorporate changes by rebasing rather than merging\"),\n+\t PARSE_OPT_OPTARG, parse_opt_rebase),\n \tOPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,\n \t\tN_(\"do not show a diffstat at the end of the merge\"),\n \t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG),\n@@ -346,7 +346,9 @@ static enum rebase_type config_get_rebase(void)\n \tif (!git_config_get_value(\"pull.rebase\", &value))\n \t\treturn parse_config_rebase(\"pull.rebase\", value, 1);\n \n-\treturn REBASE_DEFAULT;\n+\tdefault_mode = 1;\n+\n+\treturn REBASE_FALSE;\n }\n \n /**\n@@ -441,7 +443,7 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs\n \tconst char *remote = curr_branch ? curr_branch->remote_name : NULL;\n \n \tif (*refspecs) {\n-\t\tif (opt_rebase >= REBASE_TRUE)\n+\t\tif (opt_rebase)\n \t\t\tfprintf_ln(stderr, _(\"There is no candidate for rebasing against among the refs that you just fetched.\"));\n \t\telse\n \t\t\tfprintf_ln(stderr, _(\"There are no candidates for merging among the refs that you just fetched.\"));\n@@ -454,7 +456,7 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs\n \t\t\trepo);\n \t} else if (!curr_branch) {\n \t\tfprintf_ln(stderr, _(\"You are not currently on a branch.\"));\n-\t\tif (opt_rebase >= REBASE_TRUE)\n+\t\tif (opt_rebase)\n \t\t\tfprintf_ln(stderr, _(\"Please specify which branch you want to rebase against.\"));\n \t\telse\n \t\t\tfprintf_ln(stderr, _(\"Please specify which branch you want to merge with.\"));\n@@ -469,7 +471,7 @@ static void NORETURN die_no_merge_candidates(const char *repo, const char **refs\n \t\t\tremote_name = _(\"\");\n \n \t\tfprintf_ln(stderr, _(\"There is no tracking information for the current branch.\"));\n-\t\tif (opt_rebase >= REBASE_TRUE)\n+\t\tif (opt_rebase)\n \t\t\tfprintf_ln(stderr, _(\"Please specify which branch you want to rebase against.\"));\n \t\telse\n \t\t\tfprintf_ln(stderr, _(\"Please specify which branch you want to merge with.\"));\n@@ -931,11 +933,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tstruct oid_array merge_heads = OID_ARRAY_INIT;\n \tstruct object_id orig_head, curr_head;\n \tstruct object_id rebase_fork_point;\n+\tint autostash;\n \tint can_ff;\n \n-\topt_ff = xstrdup_or_null(config_get_ff());\n-\topt_rebase = config_get_rebase();\n-\n \tif (!getenv(\"GIT_REFLOG_ACTION\"))\n \t\tset_reflog_message(argc, argv);\n \n@@ -952,6 +952,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \n \tparse_repo_refspecs(argc, argv, &repo, &refspecs);\n \n+\tif (!opt_ff)\n+\t\topt_ff = xstrdup_or_null(config_get_ff());\n+\n+\tif (opt_rebase < 0)\n+\t\topt_rebase = config_get_rebase();\n+\n \tif (read_cache_unmerged())\n \t\tdie_resolve_conflict(\"pull\");\n \n@@ -961,8 +967,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tif (get_oid(\"HEAD\", &orig_head))\n \t\toidclr(&orig_head);\n \n-\tif (opt_rebase >= REBASE_TRUE) {\n-\t\tint autostash = config_autostash;\n+\tautostash = config_autostash;\n+\tif (opt_rebase) {\n \t\tif (opt_autostash != -1)\n \t\t\tautostash = opt_autostash;\n \n@@ -1021,28 +1027,29 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"Cannot merge multiple branches into empty head.\"));\n \t\treturn pull_into_void(merge_heads.oid, &curr_head);\n \t}\n-\tif (opt_rebase >= REBASE_TRUE && merge_heads.nr > 1)\n+\tif (opt_rebase && merge_heads.nr > 1)\n \t\tdie(_(\"Cannot rebase onto multiple branches.\"));\n \n \tcan_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);\n \n-\tif (!opt_rebase && !can_ff && opt_verbosity >= 0 && (!opt_ff || !strcmp(opt_ff, \"--ff\"))) {\n-\t\tadvise(_(\"Pulling without specifying how to reconcile divergent branches is discouraged;\\n\"\n-\t\t\t\"you need to specify if you want a merge, or a rebase.\\n\"\n-\t\t\t\"You can squelch this message by running one of the following commands:\\n\"\n-\t\t\t\"\\n\"\n-\t\t\t\" git config pull.rebase false # merge (the default strategy)\\n\"\n-\t\t\t\" git config pull.rebase true # rebase\\n\"\n-\t\t\t\" git config pull.ff only # fast-forward only\\n\"\n-\t\t\t\"\\n\"\n-\t\t\t\"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n-\t\t\t\"preference for all repositories.\\n\"\n-\t\t\t\"If unsure, run \\\"git pull --merge\\\".\\n\"\n-\t\t\t\"Read \\\"git pull --help\\\" for more information.\"));\n+\tif (default_mode && !can_ff && opt_verbosity >= 0 && !opt_ff) {\n+\t\tadvise(_(\"Pulling without specifying how to reconcile divergent branches is\\n\"\n+\t\t\t \"discouraged. You can squelch this message by running one of the following\\n\"\n+\t\t\t \"commands sometime before your next pull:\\n\"\n+\t\t\t \"\\n\"\n+\t\t\t \" git config pull.rebase false # merge (the default strategy)\\n\"\n+\t\t\t \" git config pull.rebase true # rebase\\n\"\n+\t\t\t \" git config pull.ff only # fast-forward only\\n\"\n+\t\t\t \"\\n\"\n+\t\t\t \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n+\t\t\t \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n+\t\t\t \"or --ff-only on the command line to override the configured default per\\n\"\n+\t\t\t \"invocation.\\n\"));\n \t}\n \n-\tif (opt_rebase >= REBASE_TRUE) {\n+\tif (opt_rebase) {\n \t\tint ret = 0;\n+\t\tint ran_ff = 0;\n \n \t\tstruct object_id newbase;\n \t\tstruct object_id upstream;\n@@ -1053,15 +1060,16 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\t recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&\n \t\t submodule_touches_in_range(the_repository, &upstream, &curr_head))\n \t\t\tdie(_(\"cannot rebase with locally recorded submodule modifications\"));\n-\n-\t\tif (can_ff) {\n-\t\t\t/* we can fast-forward this without invoking rebase */\n-\t\t\tfree(opt_ff);\n-\t\t\topt_ff = xstrdup_or_null(\"--ff-only\");\n-\t\t\tret = run_merge();\n-\t\t} else {\n-\t\t\tret = run_rebase(&newbase, &upstream);\n+\t\tif (!autostash) {\n+\t\t\tif (can_ff) {\n+\t\t\t\t/* we can fast-forward this without invoking rebase */\n+\t\t\t\topt_ff = \"--ff-only\";\n+\t\t\t\tran_ff = 1;\n+\t\t\t\tret = run_merge();\n+\t\t\t}\n \t\t}\n+\t\tif (!ran_ff)\n+\t\t\tret = run_rebase(&newbase, &upstream);\n \n \t\tif (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||\n \t\t\t recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))\ndiff --git a/rebase.h b/rebase.h\nindex 34d4acfd74..cc723d4748 100644\n--- a/rebase.h\n+++ b/rebase.h\n@@ -3,8 +3,7 @@\n \n enum rebase_type {\n \tREBASE_INVALID = -1,\n-\tREBASE_DEFAULT = 0,\n-\tREBASE_FALSE,\n+\tREBASE_FALSE = 0,\n \tREBASE_TRUE,\n \tREBASE_PRESERVE,\n \tREBASE_MERGES,\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex 1a4fe2583a..db1a381cd9 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -11,10 +11,10 @@ test_expect_success 'setup' '\n \t git commit -m one)\n '\n \n-test_expect_success 'git pull -q' '\n+test_expect_success 'git pull -q --no-rebase' '\n \tmkdir clonedq &&\n \t(cd clonedq && git init &&\n-\tgit pull -q \"../parent\" >out 2>err &&\n+\tgit pull -q --no-rebase \"../parent\" >out 2>err &&\n \ttest_must_be_empty err &&\n \ttest_must_be_empty out)\n '\n@@ -30,10 +30,10 @@ test_expect_success 'git pull -q --rebase' '\n \ttest_must_be_empty out)\n '\n \n-test_expect_success 'git pull' '\n+test_expect_success 'git pull --no-rebase' '\n \tmkdir cloned &&\n \t(cd cloned && git init &&\n-\tgit pull \"../parent\" >out 2>err &&\n+\tgit pull --no-rebase \"../parent\" >out 2>err &&\n \ttest -s err &&\n \ttest_must_be_empty out)\n '\n@@ -46,10 +46,10 @@ test_expect_success 'git pull --rebase' '\n \ttest_must_be_empty out)\n '\n \n-test_expect_success 'git pull -v' '\n+test_expect_success 'git pull -v --no-rebase' '\n \tmkdir clonedv &&\n \t(cd clonedv && git init &&\n-\tgit pull -v \"../parent\" >out 2>err &&\n+\tgit pull -v --no-rebase \"../parent\" >out 2>err &&\n \ttest -s err &&\n \ttest_must_be_empty out)\n '\n@@ -62,25 +62,25 @@ test_expect_success 'git pull -v --rebase' '\n \ttest_must_be_empty out)\n '\n \n-test_expect_success 'git pull -v -q' '\n+test_expect_success 'git pull -v -q --no-rebase' '\n \tmkdir clonedvq &&\n \t(cd clonedvq && git init &&\n-\tgit pull -v -q \"../parent\" >out 2>err &&\n+\tgit pull -v -q --no-rebase \"../parent\" >out 2>err &&\n \ttest_must_be_empty out &&\n \ttest_must_be_empty err)\n '\n \n-test_expect_success 'git pull -q -v' '\n+test_expect_success 'git pull -q -v --no-rebase' '\n \tmkdir clonedqv &&\n \t(cd clonedqv && git init &&\n-\tgit pull -q -v \"../parent\" >out 2>err &&\n+\tgit pull -q -v --no-rebase \"../parent\" >out 2>err &&\n \ttest_must_be_empty out &&\n \ttest -s err)\n '\n test_expect_success 'git pull --cleanup errors early on invalid argument' '\n \tmkdir clonedcleanup &&\n \t(cd clonedcleanup && git init &&\n-\ttest_must_fail git pull --cleanup invalid \"../parent\" >out 2>err &&\n+\ttest_must_fail git pull --no-rebase --cleanup invalid \"../parent\" >out 2>err &&\n \ttest_must_be_empty out &&\n \ttest -s err)\n '\ndiff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\nindex 8a6aae564a..6b4adab8b1 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -33,6 +33,7 @@ test_expect_success 'pull.rebase not set' '\n \ttest_decode_color decoded &&\n \ttest_i18ngrep \"hint: \" decoded &&\n \ttest_i18ngrep \"Pulling without specifying how to reconcile\" decoded\n+\n '\n \n test_expect_success 'pull.rebase not set (fast-forward)' '\n@@ -45,7 +46,7 @@ test_expect_success 'pull.rebase not set and pull.ff=true' '\n \tgit reset --hard c2 &&\n \ttest_config pull.ff true &&\n \tgit pull . c1 2>err &&\n-\ttest_i18ngrep \"Pulling without specifying how to reconcile\" err\n+\ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n test_expect_success 'pull.rebase not set and pull.ff=false' '\n@@ -68,16 +69,16 @@ test_expect_success 'pull.rebase not set and --rebase given' '\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n-test_expect_success 'pull.rebase not set and --merge given' '\n+test_expect_success 'pull.rebase not set and --no-rebase given' '\n \tgit reset --hard c2 &&\n-\tgit pull --merge . c1 2>err &&\n+\tgit pull --no-rebase . c1 2>err &&\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n test_expect_success 'pull.rebase not set and --ff given' '\n \tgit reset --hard c2 &&\n \tgit pull --ff . c1 2>err &&\n-\ttest_i18ngrep \"Pulling without specifying how to reconcile\" err\n+\ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n test_expect_success 'pull.rebase not set and --no-ff given' '\n\n\nFelipe Contreras (3):\n pull: refactor fast-forward check\n pull: move default warning\n pull: display default warning only when non-ff\n\n builtin/pull.c | 61 +++++++++++++++++++++---------------\n t/t7601-merge-pull-config.sh | 28 ++++++++++-------\n 2 files changed, 53 insertions(+), 36 deletions(-)\n\n-- \n2.29.2\n\n"},{"id":"411988","messageId":"20201210100538.696787-3-felipe.contreras@gmail.com","threadId":"54807","inReplyTo":"20201210100538.696787-1-felipe.contreras@gmail.com","subject":"[PATCH v5 2/3] pull: move default warning","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-10T10:05:37Z","receivedAt":"2020-12-10T10:06:49Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Eventually we want to be able to display the warning only when\nfast-forward merges are not possible.\n\nIn order to do so we need to move the default warning up to the point\nwhere we can check if we can fast-forward or not.\n\nAdditionally, config_get_rebase() was probably never its true home.\n\nThis requires a temporary variable to check if we are in the\n\"default mode\" (no --rebase or --no-rebase specified).\n\nSigned-off-by: Felipe Contreras \n---\n builtin/pull.c | 32 ++++++++++++++++++--------------\n 1 file changed, 18 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 03e6d53243..ff8e3ce137 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -27,6 +27,8 @@\n #include \"commit-reach.h\"\n #include \"sequencer.h\"\n \n+static int default_mode;\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@@ -344,20 +346,7 @@ static enum rebase_type config_get_rebase(void)\n \tif (!git_config_get_value(\"pull.rebase\", &value))\n \t\treturn parse_config_rebase(\"pull.rebase\", value, 1);\n \n-\tif (opt_verbosity >= 0 && !opt_ff) {\n-\t\tadvise(_(\"Pulling without specifying how to reconcile divergent branches is\\n\"\n-\t\t\t \"discouraged. You can squelch this message by running one of the following\\n\"\n-\t\t\t \"commands sometime before your next pull:\\n\"\n-\t\t\t \"\\n\"\n-\t\t\t \" git config pull.rebase false # merge (the default strategy)\\n\"\n-\t\t\t \" git config pull.rebase true # rebase\\n\"\n-\t\t\t \" git config pull.ff only # fast-forward only\\n\"\n-\t\t\t \"\\n\"\n-\t\t\t \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n-\t\t\t \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n-\t\t\t \"or --ff-only on the command line to override the configured default per\\n\"\n-\t\t\t \"invocation.\\n\"));\n-\t}\n+\tdefault_mode = 1;\n \n \treturn REBASE_FALSE;\n }\n@@ -1040,6 +1029,21 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tif (opt_rebase && merge_heads.nr > 1)\n \t\tdie(_(\"Cannot rebase onto multiple branches.\"));\n \n+\tif (default_mode && opt_verbosity >= 0 && !opt_ff) {\n+\t\tadvise(_(\"Pulling without specifying how to reconcile divergent branches is\\n\"\n+\t\t\t \"discouraged. You can squelch this message by running one of the following\\n\"\n+\t\t\t \"commands sometime before your next pull:\\n\"\n+\t\t\t \"\\n\"\n+\t\t\t \" git config pull.rebase false # merge (the default strategy)\\n\"\n+\t\t\t \" git config pull.rebase true # rebase\\n\"\n+\t\t\t \" git config pull.ff only # fast-forward only\\n\"\n+\t\t\t \"\\n\"\n+\t\t\t \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n+\t\t\t \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n+\t\t\t \"or --ff-only on the command line to override the configured default per\\n\"\n+\t\t\t \"invocation.\\n\"));\n+\t}\n+\n \tif (opt_rebase) {\n \t\tint ret = 0;\n \t\tint ran_ff = 0;\n-- \n2.29.2\n\n"},{"id":"411989","messageId":"20201210100538.696787-2-felipe.contreras@gmail.com","threadId":"54807","inReplyTo":"20201210100538.696787-1-felipe.contreras@gmail.com","subject":"[PATCH v5 1/3] pull: refactor fast-forward check","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-10T10:05:36Z","receivedAt":"2020-12-10T10:07:14Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"It's much cleaner this way.\n\nAlso, we would like to be able to make this check before the decision to\nrebase is made.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/pull.c | 26 +++++++++++++++-----------\n 1 file changed, 15 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex aa56ebcdd0..03e6d53243 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -924,6 +924,20 @@ static int run_rebase(const struct object_id *newbase,\n \treturn ret;\n }\n \n+static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_head)\n+{\n+\tint ret;\n+\tstruct commit_list *list = NULL;\n+\tstruct commit *merge_head, *head;\n+\n+\thead = lookup_commit_reference(the_repository, orig_head);\n+\tcommit_list_insert(head, &list);\n+\tmerge_head = lookup_commit_reference(the_repository, orig_merge_head);\n+\tret = repo_is_descendant_of(the_repository, merge_head, list);\n+\tfree_commit_list(list);\n+\treturn ret;\n+}\n+\n int cmd_pull(int argc, const char **argv, const char *prefix)\n {\n \tconst char *repo, **refspecs;\n@@ -1040,22 +1054,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\t submodule_touches_in_range(the_repository, &upstream, &curr_head))\n \t\t\tdie(_(\"cannot rebase with locally recorded submodule modifications\"));\n \t\tif (!autostash) {\n-\t\t\tstruct commit_list *list = NULL;\n-\t\t\tstruct commit *merge_head, *head;\n-\n-\t\t\thead = lookup_commit_reference(the_repository,\n-\t\t\t\t\t\t &orig_head);\n-\t\t\tcommit_list_insert(head, &list);\n-\t\t\tmerge_head = lookup_commit_reference(the_repository,\n-\t\t\t\t\t\t\t &merge_heads.oid[0]);\n-\t\t\tif (repo_is_descendant_of(the_repository,\n-\t\t\t\t\t\t merge_head, list)) {\n+\t\t\tif (get_can_ff(&orig_head, &merge_heads.oid[0])) {\n \t\t\t\t/* we can fast-forward this without invoking rebase */\n \t\t\t\topt_ff = \"--ff-only\";\n \t\t\t\tran_ff = 1;\n \t\t\t\tret = run_merge();\n \t\t\t}\n-\t\t\tfree_commit_list(list);\n \t\t}\n \t\tif (!ran_ff)\n \t\t\tret = run_rebase(&newbase, &upstream);\n-- \n2.29.2\n\n"},{"id":"411990","messageId":"20201210100538.696787-4-felipe.contreras@gmail.com","threadId":"54807","inReplyTo":"20201210100538.696787-1-felipe.contreras@gmail.com","subject":"[PATCH v5 3/3] pull: display default warning only when non-ff","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-10T10:05:38Z","receivedAt":"2020-12-10T10:07:14Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"There's no need to display the annoying warning on every pull... only\nthe ones that are not fast-forward.\n\nThis requires the tests to pick another base, so the merge is not\nfast-forward. And in the cases where --ff-only is specified add\ntest_must_fail (since now they are non-fast-forward).\n\nSigned-off-by: Felipe Contreras \n---\n builtin/pull.c | 7 +++++--\n t/t7601-merge-pull-config.sh | 28 +++++++++++++++++-----------\n 2 files changed, 22 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex ff8e3ce137..9a7caf3a3e 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -934,6 +934,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tstruct object_id orig_head, curr_head;\n \tstruct object_id rebase_fork_point;\n \tint autostash;\n+\tint can_ff;\n \n \tif (!getenv(\"GIT_REFLOG_ACTION\"))\n \t\tset_reflog_message(argc, argv);\n@@ -1029,7 +1030,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tif (opt_rebase && merge_heads.nr > 1)\n \t\tdie(_(\"Cannot rebase onto multiple branches.\"));\n \n-\tif (default_mode && opt_verbosity >= 0 && !opt_ff) {\n+\tcan_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);\n+\n+\tif (default_mode && !can_ff && opt_verbosity >= 0 && !opt_ff) {\n \t\tadvise(_(\"Pulling without specifying how to reconcile divergent branches is\\n\"\n \t\t\t \"discouraged. You can squelch this message by running one of the following\\n\"\n \t\t\t \"commands sometime before your next pull:\\n\"\n@@ -1058,7 +1061,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\t submodule_touches_in_range(the_repository, &upstream, &curr_head))\n \t\t\tdie(_(\"cannot rebase with locally recorded submodule modifications\"));\n \t\tif (!autostash) {\n-\t\t\tif (get_can_ff(&orig_head, &merge_heads.oid[0])) {\n+\t\t\tif (can_ff) {\n \t\t\t\t/* we can fast-forward this without invoking rebase */\n \t\t\t\topt_ff = \"--ff-only\";\n \t\t\t\tran_ff = 1;\ndiff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\nindex 6774e9d86f..6b4adab8b1 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -28,7 +28,7 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'pull.rebase not set' '\n-\tgit reset --hard c0 &&\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@@ -36,54 +36,60 @@ test_expect_success 'pull.rebase not set' '\n \n '\n \n-test_expect_success 'pull.rebase not set and pull.ff=true' '\n+test_expect_success 'pull.rebase not set (fast-forward)' '\n \tgit reset --hard c0 &&\n+\tgit pull . c1 2>err &&\n+\ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n+'\n+\n+test_expect_success 'pull.rebase not set and pull.ff=true' '\n+\tgit reset --hard c2 &&\n \ttest_config pull.ff true &&\n \tgit pull . c1 2>err &&\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n test_expect_success 'pull.rebase not set and pull.ff=false' '\n-\tgit reset --hard c0 &&\n+\tgit reset --hard c2 &&\n \ttest_config pull.ff false &&\n \tgit pull . c1 2>err &&\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n test_expect_success 'pull.rebase not set and pull.ff=only' '\n-\tgit reset --hard c0 &&\n+\tgit reset --hard c2 &&\n \ttest_config pull.ff only &&\n-\tgit pull . c1 2>err &&\n+\ttest_must_fail git pull . c1 2>err &&\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n test_expect_success 'pull.rebase not set and --rebase given' '\n-\tgit reset --hard c0 &&\n+\tgit reset --hard c2 &&\n \tgit pull --rebase . c1 2>err &&\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n test_expect_success 'pull.rebase not set and --no-rebase given' '\n-\tgit reset --hard c0 &&\n+\tgit reset --hard c2 &&\n \tgit pull --no-rebase . c1 2>err &&\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n test_expect_success 'pull.rebase not set and --ff given' '\n-\tgit reset --hard c0 &&\n+\tgit reset --hard c2 &&\n \tgit pull --ff . c1 2>err &&\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n test_expect_success 'pull.rebase not set and --no-ff given' '\n-\tgit reset --hard c0 &&\n+\tgit reset --hard c2 &&\n \tgit pull --no-ff . c1 2>err &&\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n test_expect_success 'pull.rebase not set and --ff-only given' '\n-\tgit reset --hard c0 &&\n-\tgit pull --ff-only . c1 2>err &&\n+\tgit reset --hard c2 &&\n+\ttest_must_fail git pull --ff-only . c1 2>err &&\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n-- \n2.29.2\n\n"},{"id":"412049","messageId":"xmqqy2i4ltnv.fsf@gitster.c.googlers.com","threadId":"54807","inReplyTo":"20201210100538.696787-3-felipe.contreras@gmail.com","subject":"Re: [PATCH v5 2/3] pull: move default warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-11T06:54:12Z","receivedAt":"2020-12-11T06:56:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> Eventually we want to be able to display the warning only when\n> fast-forward merges are not possible.\n>\n> In order to do so we need to move the default warning up to the point\n> where we can check if we can fast-forward or not.\n\nMakes sense.\n\n> Additionally, config_get_rebase() was probably never its true home.\n\nI agree with this point. I've always found it suboptimal.\n\n> This requires a temporary variable to check if we are in the\n> \"default mode\" (no --rebase or --no-rebase specified).\n\nTwo points:\n\n - \"mode\" is so overused a word; a more focused word is preferrable.\n\n - by introducing a local variable in cmd_pull() and passing a\n pointer to it to config_get_rebase(), we can easily avoid having\n to rely on an extra global variable.\n\nI'd suggest addressing the above along the following lines.\n\n -static enum rebase_type config_get_rebase(void)\n +static enum rebase_type config_get_rebase(int *rebase_unspecified)\n {\n + *rebase_unspecified = 0;\n\n ... various \"return\" of configured values ...\n\n + *rebase_unspecified = 1;\n return REBASE_FALSE;\n }\n\nThen the caller would declare\n\n\tint rebase_unspecified = 0;\n\nand call \n\n\tif (opt_rebase < 0)\n\t\topt_rebase = config_get_rebase(&rebase_unspecified);\n\nto possibly cause it to set to true, and use that instead of the\nglobal variable to decide if we want to give the help text. When\nthe helper is not called due to opt_rebase already being set, it is\nnot using configured value but using the choice from the command\nline, so rebase_unspecified is still false after this point.\n\n> @@ -1040,6 +1029,21 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> \tif (opt_rebase && merge_heads.nr > 1)\n> \t\tdie(_(\"Cannot rebase onto multiple branches.\"));\n\nAnd this is the point where we finish various error checks and\nstarts to run either rebase or merge. It is as late as we could\ndelay the \"non-ff and you are not configured\" message. In other\nwords, the place chosen in cmd_pull() to move this code to is\noptimal.\n\n> +\tif (default_mode && opt_verbosity >= 0 && !opt_ff) {\n> +\t\tadvise(_(\"Pulling without specifying how to reconcile divergent branches is\\n\"\n> +\t\t\t \"discouraged. You can squelch this message by running one of the following\\n\"\n> +\t\t\t \"commands sometime before your next pull:\\n\"\n> +\t\t\t \"\\n\"\n> +\t\t\t \" git config pull.rebase false # merge (the default strategy)\\n\"\n> +\t\t\t \" git config pull.rebase true # rebase\\n\"\n> +\t\t\t \" git config pull.ff only # fast-forward only\\n\"\n> +\t\t\t \"\\n\"\n> +\t\t\t \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n> +\t\t\t \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n> +\t\t\t \"or --ff-only on the command line to override the configured default per\\n\"\n> +\t\t\t \"invocation.\\n\"));\n> +\t}\n\nEither as a part of this step, as a part of the next step, or a\nseparate follow-up patch, we should\n\n - create a single-purpose helper function that only calls advise()\n with the message and returns; name it show_advice_pull_non_ff().\n\n - correct the if() statement above, so that regardless of verbosity\n level, we can do _something_ common when the history does not\n fast-forward. I.e.\n\n\tif (rebase_unspecified && !opt_ff) {\n\t\tif (opt_verbosity >= 0)\n\t\t\tshow_advice_pull_non_ff();\n\t}\n\nThese would allow us to further turn the logic to\n\n\tif (rebase_unspecified && !opt_ff) {\n\t\tif (opt_verbosity >= 0 && advice_pull_non_ff)\n\t\t\tshow_advice_pull_non_ff();\n\t\tdie(\"not a fast-forward; must merge or rebase\");\n\t}\n\nlater in the far future, and we do not want that die() to be\naffected by verbosity settings.\n\nI'll queue such a fix-up patch on top of the series before pushing\nthe integration results out on 'seen'.\n\nThanks.\n"},{"id":"412050","messageId":"xmqq5z58n88i.fsf@gitster.c.googlers.com","threadId":"54807","inReplyTo":"20201210100538.696787-2-felipe.contreras@gmail.com","subject":"Re: [PATCH v5 1/3] pull: refactor fast-forward check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-11T06:54:05Z","receivedAt":"2020-12-11T06:56:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> It's much cleaner this way.\n\nIt is obvious that a focused single purpose helper with less\nindentation is easier to follow both at the calling site and the\nimplementation itself.\n\n\"It's much cleaner\" is not something you need to say.\n\nIf you were making it uglier, but other benefits outweigh the\nugliness, that may deserve to be said, but that is not the case\nhere.\n\n> Also, we would like to be able to make this check before the decision to\n> rebase is made.\n\n... in a future step.\n\nThat is something we want to say upfront, not \"Also\".\n\nThe patch itself looks quite straightforward.\n\nThanks.\n"},{"id":"412051","messageId":"CAMP44s170qZuZqmh_gVLGgJJ5005HzDM0fdQKQamjHkZCOu7_Q@mail.gmail.com","threadId":"54807","inReplyTo":"xmqqy2i4ltnv.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v5 2/3] pull: move default warning","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-11T07:55:45Z","receivedAt":"2020-12-11T07:57:31Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Dec 11, 2020 at 12:54 AM Junio C Hamano wrote:\n>\n> Felipe Contreras writes:\n>\n> > Eventually we want to be able to display the warning only when\n> > fast-forward merges are not possible.\n> >\n> > In order to do so we need to move the default warning up to the point\n> > where we can check if we can fast-forward or not.\n>\n> Makes sense.\n>\n> > Additionally, config_get_rebase() was probably never its true home.\n>\n> I agree with this point. I've always found it suboptimal.\n>\n> > This requires a temporary variable to check if we are in the\n> > \"default mode\" (no --rebase or --no-rebase specified).\n>\n> Two points:\n>\n> - \"mode\" is so overused a word; a more focused word is preferrable.\n\nThere's literally only one instance in the file, and it's a call to an\nexternal function.\n\nPlus, it refers to \"git pull\" without any --merge or --rebase (that's\nthe default mode in my book).\n\n> - by introducing a local variable in cmd_pull() and passing a\n> pointer to it to config_get_rebase(), we can easily avoid having\n> to rely on an extra global variable.\n>\n> I'd suggest addressing the above along the following lines.\n\n...\n\n> to possibly cause it to set to true, and use that instead of the\n> global variable to decide if we want to give the help text.\n\nYeah, there's only 38 global variables. We wouldn't want another one\nwhich makes the code pretty straightforward.\n\n> > @@ -1040,6 +1029,21 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> > if (opt_rebase && merge_heads.nr > 1)\n> > die(_(\"Cannot rebase onto multiple branches.\"));\n>\n> And this is the point where we finish various error checks and\n> starts to run either rebase or merge. It is as late as we could\n> delay the \"non-ff and you are not configured\" message. In other\n> words, the place chosen in cmd_pull() to move this code to is\n> optimal.\n\nWhich is right before the fork between rebase and merge.\n\n> > + if (default_mode && opt_verbosity >= 0 && !opt_ff) {\n> > + advise(_(\"Pulling without specifying how to reconcile divergent branches is\\n\"\n> > + \"discouraged. You can squelch this message by running one of the following\\n\"\n> > + \"commands sometime before your next pull:\\n\"\n> > + \"\\n\"\n> > + \" git config pull.rebase false # merge (the default strategy)\\n\"\n> > + \" git config pull.rebase true # rebase\\n\"\n> > + \" git config pull.ff only # fast-forward only\\n\"\n> > + \"\\n\"\n> > + \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n> > + \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n> > + \"or --ff-only on the command line to override the configured default per\\n\"\n> > + \"invocation.\\n\"));\n> > + }\n>\n> Either as a part of this step, as a part of the next step, or a\n> separate follow-up patch, we should\n>\n> - create a single-purpose helper function that only calls advise()\n> with the message and returns; name it show_advice_pull_non_ff().\n\nIf we are going for low-hanging fruit there were many in v4 of this\nseries, which actually improve behavior, not just code organization,\nbut OK.\n\n> - correct the if() statement above, so that regardless of verbosity\n> level, we can do _something_ common when the history does not\n> fast-forward. I.e.\n>\n> if (rebase_unspecified && !opt_ff) {\n> if (opt_verbosity >= 0)\n> show_advice_pull_non_ff();\n> }\n>\n> These would allow us to further turn the logic to\n>\n> if (rebase_unspecified && !opt_ff) {\n> if (opt_verbosity >= 0 && advice_pull_non_ff)\n> show_advice_pull_non_ff();\n> die(\"not a fast-forward; must merge or rebase\");\n> }\n\nShould actually be something like:\n\n if (rebase_unspecified && !can_ff)\n die(\"Not a fast-forward; must either merge or rebase\");\n\nBut yeah.\n\n-- \nFelipe Contreras\n"},{"id":"412062","messageId":"xmqqzh2kitn9.fsf@gitster.c.googlers.com","threadId":"54807","inReplyTo":"20201210100538.696787-4-felipe.contreras@gmail.com","subject":"Re: [PATCH v5 3/3] pull: display default warning only when non-ff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-11T07:16:25Z","receivedAt":"2020-12-11T09:24:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> There's no need to display the annoying warning on every pull... only\n> the ones that are not fast-forward.\n\nYes! And thanks to the previous two steps, the change to the code\nis quite obvious. I don't have to give any further comment on the\npart that changes code---it is well done, period.\n\n> This requires the tests to pick another base, so the merge is not\n> fast-forward. And in the cases where --ff-only is specified add\n> test_must_fail (since now they are non-fast-forward).\n\nI am not sure what this means. \n\nExisting tests pull histories that may or may not be descendants of\nthe HEAD. If we do not change the pair of commits involved in the\ntests, i.e. if we do not apply many s/c0/c2/ replacement I see in\nthe patch, some of these tests would change their behaviour with\nrespect to their advice output (but not the outcome). The ones that\npulled fast-forward would stop showing the warning, and that is a\ngood effect of this change. We want to see that in the update to\nthe tests, no?\n\nThe ones that pulled non-fast-forward history would still show the\nwarning as they used to, and that is also what we want to see after\nthis change. Which means we want the tests to keep doing what they\ndo, while adjusting what we expect out of the tested commands to the\nworld that is made better with the code change in this patch. I do\nnot think we need to add new tests to demonstrate the new behaviour,\nas they (i.e. when the test pulls fast-forwardable history, with\nvarious combinations of options and configuration) seem to be pretty\nwell covered already.\n\nIn other words, the changes the paragraph says that the commit made\nto the tests sound quite backwards.\n\nThe actual changes to some of the tests do look sensible, testing\nboth sides of the coin. I've looked at the changes to the tests,\nbut cannot convince me that we are not making irrelevant changes\nto the tests, or losing coverage needlessly because of s/c0/c2/\n(i.e. turning tests that used to check fast-forward situations\ninto tests that check non-ff situations).\n\n> diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\n> index 6774e9d86f..6b4adab8b1 100755\n> --- a/t/t7601-merge-pull-config.sh\n> +++ b/t/t7601-merge-pull-config.sh\n> @@ -28,7 +28,7 @@ test_expect_success 'setup' '\n> '\n> \n> test_expect_success 'pull.rebase not set' '\n> -\tgit reset --hard c0 &&\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\nThis is not \"keeping what the original test does and adjusting the\nexpectation, to demonstrate how behaviour changed\"; instead, we make\nsure that the message we originally gave and we intend to keep\ngiving is shown in non-ff situation by choosing the current commit\nthat won't allow a ff merge. This is OK if we did not lose test\ncoverage---as long as we test that we no longer give the message in\nff situation somewhere else, And that happens later, I think.\n\n> @@ -36,54 +36,60 @@ test_expect_success 'pull.rebase not set' '\n> \n> '\n> \n> -test_expect_success 'pull.rebase not set and pull.ff=true' '\n> +test_expect_success 'pull.rebase not set (fast-forward)' '\n> \tgit reset --hard c0 &&\n> +\tgit pull . c1 2>err &&\n> +\ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n> +'\n\nThis is the new test to check the other side of the coin. It sees\nhow the original test to merge c1 into c0 would behave with the new\ncode. We make sure we do not give the advice because it is\nirrelevant in this situation.\n\nSo the above two are good, even though the way this patch updates\ntests is probably a bit more error prone than necessary.\n\nSince we have checked how the new code behave for fast-forward with\nthis new test, the remainder of the entire test script can be\nmodified to test only non-ff situation without losing test coverage?\n\nI am not sure if that is the case.\n\n> +test_expect_success 'pull.rebase not set and pull.ff=true' '\n> +\tgit reset --hard c2 &&\n> \ttest_config pull.ff true &&\n> \tgit pull . c1 2>err &&\n> \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n> '\n\nWe are merely allowing fast-forward merges without unnecessary merge\ncommits, but we are faced to merge c1 into c2, which is not ff. The\ncommand goes ahead and merges anyway, but why shouldn't we be seeing\nthe message? I am puzzled.\n\n> test_expect_success 'pull.rebase not set and pull.ff=false' '\n> -\tgit reset --hard c0 &&\n> +\tgit reset --hard c2 &&\n> \ttest_config pull.ff false &&\n> \tgit pull . c1 2>err &&\n> \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n> '\n> \n> test_expect_success 'pull.rebase not set and pull.ff=only' '\n> -\tgit reset --hard c0 &&\n> +\tgit reset --hard c2 &&\n> \ttest_config pull.ff only &&\n> -\tgit pull . c1 2>err &&\n> +\ttest_must_fail git pull . c1 2>err &&\n> \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n> '\n\nThis used to test that fast-forwarding the HEAD from c0 to c2 would\nbe done successfully without issuing the message. Shouldn't that\nstill be true in the improved \"do not complain on fast-forward\" code?\n\nWe seem to be losing test coverage by checking how pull.ff=only prevents\nthe command from working in a non-ff merge.\n\nI'd stop my review on the tests here, but I generally think s/c0/c2/\ndone in this patch is a wrong thing to do. We are changing the\ncondition under which the messages is given (we are narrowing it to\navoid giving it when it is irrelevant), without changing the final\noutcome (even though we changed the condition to give the message,\nwe didn't change what the final outcome of the pull command would\nbe), so I'd strongly prefer testing the same set of scenarios and\nupdate the expectation to an improved reality.\n\n> test_expect_success 'pull.rebase not set and --rebase given' '\n> -\tgit reset --hard c0 &&\n> +\tgit reset --hard c2 &&\n> \tgit pull --rebase . c1 2>err &&\n> \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n> '\n> \n> test_expect_success 'pull.rebase not set and --no-rebase given' '\n> -\tgit reset --hard c0 &&\n> +\tgit reset --hard c2 &&\n> \tgit pull --no-rebase . c1 2>err &&\n> \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n> '\n> \n> test_expect_success 'pull.rebase not set and --ff given' '\n> -\tgit reset --hard c0 &&\n> +\tgit reset --hard c2 &&\n> \tgit pull --ff . c1 2>err &&\n> \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n> '\n> \n> test_expect_success 'pull.rebase not set and --no-ff given' '\n> -\tgit reset --hard c0 &&\n> +\tgit reset --hard c2 &&\n> \tgit pull --no-ff . c1 2>err &&\n> \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n> '\n> \n> test_expect_success 'pull.rebase not set and --ff-only given' '\n> -\tgit reset --hard c0 &&\n> -\tgit pull --ff-only . c1 2>err &&\n> +\tgit reset --hard c2 &&\n> +\ttest_must_fail git pull --ff-only . c1 2>err &&\n> \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n> '\n"},{"id":"412068","messageId":"xmqqo8j0io39.fsf@gitster.c.googlers.com","threadId":"54807","inReplyTo":"20201210100538.696787-1-felipe.contreras@gmail.com","subject":"Re: [PATCH v5 0/3] pull: stop warning on every pull","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-11T07:17:13Z","receivedAt":"2020-12-11T11:24:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> The discussion about making fast-forward-only pulls the default is\n> stuck on mud, and there's no agreement about what we should even\n> be warning our users about.\n\nThe above perception of yours is mostly due to misunderstanding, I\nwould have to say. We are in agreement on what we should be warning\nabout at least, assuming that you are expressing what you want\nclearly in the latest round of responses and I understood them\ncorrectly [*1*].\n\nI do not know if others on the list agree, though.\n\nI do agree that there is no agreement on the behaviour in the\nendgame. In principle, I am in favor of disabling the more\ndangerous half of the \"git pull\" command for those who haven't\nconfigured anything. But I can understand those who do not want\nthat behaviour, as the fallout would be quite big.\n\n> Even my straightforward patches about improving documentation, and\n> the consistency of the UI with --merge and other obvious fixes\n> lost traction.\n\nIt may be obvious to you, but may not be to others on the list who\nspoke in the thread and who didn't speak but read the discussion.\n\nI did see potential goodness in the documentation update and that\nwas why I offered polishment on top of your patches in a v3 round,\nbut seeing the suggestions dismissed without convincing arguments\nbefore v4 was sent out would have discouraged even the most patient\nreviewers among us. If you meant by \"lost traction\" the lack of\ncomments on v4, that was my reason for not commenting.\n\nIn any case, these three patches in this round looked quite sensible\nto me, except for the tests in 3/3, and minor details of 2/3, both\nof which I gave a more detailed review and suggestion.\n\nThanks.\n\n\n[Footnote]\n\n*1* The only difference between us is whether it is sensible to\nallow explicitly ask to see the same behaviour as an unconfigured\nuser except for the help text---I do not think it is, and I do want\nto avoid introducing pull.mode, but I've shown a way or two to get\nthe behaviour without adding pull.mode in the mix.\n"},{"id":"412077","messageId":"CAMP44s0wjfZ9TeQzpJvVD-OzFA47HFd87TABiJo3Ec9H8j-fjA@mail.gmail.com","threadId":"54807","inReplyTo":"xmqqzh2kitn9.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v5 3/3] pull: display default warning only when non-ff","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-11T12:48:33Z","receivedAt":"2020-12-11T12:50:13Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Dec 11, 2020 at 3:22 AM Junio C Hamano wrote:\n>\n> Felipe Contreras writes:\n>\n> > There's no need to display the annoying warning on every pull... only\n> > the ones that are not fast-forward.\n>\n> Yes! And thanks to the previous two steps, the change to the code\n> is quite obvious. I don't have to give any further comment on the\n> part that changes code---it is well done, period.\n>\n> > This requires the tests to pick another base, so the merge is not\n> > fast-forward. And in the cases where --ff-only is specified add\n> > test_must_fail (since now they are non-fast-forward).\n>\n> I am not sure what this means.\n\nIt means in order to test the previous warning--which appeared in\nfast-forward and non-fast-forward merges, and now appears only on\nnon-fast-forward merges--we need non-fast-forward merge situations.\n\n> Existing tests pull histories that may or may not be descendants of\n> the HEAD. If we do not change the pair of commits involved in the\n> tests, i.e. if we do not apply many s/c0/c2/ replacement I see in\n> the patch, some of these tests would change their behaviour with\n> respect to their advice output (but not the outcome).\n\nAll the tests, actually.\n\n> The ones that\n> pulled fast-forward would stop showing the warning, and that is a\n> good effect of this change. We want to see that in the update to\n> the tests, no?\n\nYes.\n\n> The ones that pulled non-fast-forward history would still show the\n> warning as they used to, and that is also what we want to see after\n> this change.\n\nCurrently no test is doing that; all are in a fast-forward situation.\n\n> In other words, the changes the paragraph says that the commit made\n> to the tests sound quite backwards.\n\nI'm not sure how it is backwards. All the tests are fast-forward, we\nneed them not fast-forward, so we pick another base... so the merges\nare not fast-forward.\n\n> The actual changes to some of the tests do look sensible, testing\n> both sides of the coin. I've looked at the changes to the tests,\n> but cannot convince me that we are not making irrelevant changes\n> to the tests, or losing coverage needlessly because of s/c0/c2/\n> (i.e. turning tests that used to check fast-forward situations\n> into tests that check non-ff situations).\n\nNo test is checking fast-forward situations, because the warning\ndoesn't check fast-forward situations.\n\n> > diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\n> > index 6774e9d86f..6b4adab8b1 100755\n> > --- a/t/t7601-merge-pull-config.sh\n> > +++ b/t/t7601-merge-pull-config.sh\n> > @@ -28,7 +28,7 @@ test_expect_success 'setup' '\n> > '\n> >\n> > test_expect_success 'pull.rebase not set' '\n> > - git reset --hard c0 &&\n> > + git reset --hard c2 &&\n> > git -c color.advice=always pull . c1 2>err &&\n> > test_decode_color decoded &&\n> > test_i18ngrep \"hint: \" decoded &&\n>\n> This is not \"keeping what the original test does and adjusting the\n> expectation, to demonstrate how behaviour changed\"; instead, we make\n> sure that the message we originally gave and we intend to keep\n> giving is shown in non-ff situation by choosing the current commit\n> that won't allow a ff merge. This is OK if we did not lose test\n> coverage---as long as we test that we no longer give the message in\n> ff situation somewhere else, And that happens later, I think.\n>\n> > @@ -36,54 +36,60 @@ test_expect_success 'pull.rebase not set' '\n> >\n> > '\n> >\n> > -test_expect_success 'pull.rebase not set and pull.ff=true' '\n> > +test_expect_success 'pull.rebase not set (fast-forward)' '\n> > git reset --hard c0 &&\n> > + git pull . c1 2>err &&\n> > + test_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n> > +'\n>\n> This is the new test to check the other side of the coin. It sees\n> how the original test to merge c1 into c0 would behave with the new\n> code. We make sure we do not give the advice because it is\n> irrelevant in this situation.\n>\n> So the above two are good, even though the way this patch updates\n> tests is probably a bit more error prone than necessary.\n\nAny suggestions to make it less error prone are welcome.\n\n> Since we have checked how the new code behave for fast-forward with\n> this new test, the remainder of the entire test script can be\n> modified to test only non-ff situation without losing test coverage?\n>\n> I am not sure if that is the case.\n\nIt is the case. That's what the patch does.\n\n> > +test_expect_success 'pull.rebase not set and pull.ff=true' '\n> > + git reset --hard c2 &&\n> > test_config pull.ff true &&\n> > git pull . c1 2>err &&\n> > test_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n> > '\n>\n> We are merely allowing fast-forward merges without unnecessary merge\n> commits, but we are faced to merge c1 into c2, which is not ff. The\n> command goes ahead and merges anyway, but why shouldn't we be seeing\n> the message? I am puzzled.\n\nBecause that's what the current code does; it just checks that any\nopt_ff is set, it doesn't check for any specific values.\n\nI tried to fix that since v1, v2, v3, and v4 of the series [1], which\nonly received comments from Elijah Newren, but that's a separate\npatch, and as I mentioned in the cover letter of v5; I've dropped all\nthose other patches.\n\n> > test_expect_success 'pull.rebase not set and pull.ff=false' '\n> > - git reset --hard c0 &&\n> > + git reset --hard c2 &&\n> > test_config pull.ff false &&\n> > git pull . c1 2>err &&\n> > test_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n> > '\n> >\n> > test_expect_success 'pull.rebase not set and pull.ff=only' '\n> > - git reset --hard c0 &&\n> > + git reset --hard c2 &&\n> > test_config pull.ff only &&\n> > - git pull . c1 2>err &&\n> > + test_must_fail git pull . c1 2>err &&\n> > test_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n> > '\n>\n> This used to test that fast-forwarding the HEAD from c0 to c2 would\n> be done successfully without issuing the message. Shouldn't that\n> still be true in the improved \"do not complain on fast-forward\" code?\n\nNo. It doesn't care if it's fast-forward or not; it's just checking\nthat \"pull.ff=only\" is set.\n\nBefore the code just checks \"!opt_ff\", now it checks \"!opt_ff && !can_ff\".\n\nSo, in order to do the \"pull.ff=only\" check, we need a non-fast-forward merge.\n\n> We seem to be losing test coverage by checking how pull.ff=only prevents\n> the command from working in a non-ff merge.\n\nNo we don't. Remove the \"test_config pull.ff only\" and the test fails,\nas expected.\n\n> I'd stop my review on the tests here, but I generally think s/c0/c2/\n> done in this patch is a wrong thing to do. We are changing the\n> condition under which the messages is given (we are narrowing it to\n> avoid giving it when it is irrelevant), without changing the final\n> outcome (even though we changed the condition to give the message,\n> we didn't change what the final outcome of the pull command would\n> be), so I'd strongly prefer testing the same set of scenarios and\n> update the expectation to an improved reality.\n\nWe are testing *exactly* the same test scenarios, we are just forcing\ncan_ff to be false.\n\nIf I remove the precise thing each test-case is supposed to test:\n\ndiff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\nindex 6b4adab8b1..6c3413ddc9 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -44,52 +44,49 @@ test_expect_success 'pull.rebase not set (fast-forward)' '\n\n test_expect_success 'pull.rebase not set and pull.ff=true' '\n git reset --hard c2 &&\n- test_config pull.ff true &&\n git pull . c1 2>err &&\n test_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n\n test_expect_success 'pull.rebase not set and pull.ff=false' '\n git reset --hard c2 &&\n- test_config pull.ff false &&\n git pull . c1 2>err &&\n test_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n\n test_expect_success 'pull.rebase not set and pull.ff=only' '\n git reset --hard c2 &&\n- test_config pull.ff only &&\n- test_must_fail git pull . c1 2>err &&\n+ git pull . c1 2>err &&\n test_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n\n test_expect_success 'pull.rebase not set and --rebase given' '\n git reset --hard c2 &&\n- git pull --rebase . c1 2>err &&\n+ git pull . c1 2>err &&\n test_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n\n test_expect_success 'pull.rebase not set and --no-rebase given' '\n git reset --hard c2 &&\n- git pull --no-rebase . c1 2>err &&\n+ git pull . c1 2>err &&\n test_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n\n test_expect_success 'pull.rebase not set and --ff given' '\n git reset --hard c2 &&\n- git pull --ff . c1 2>err &&\n+ git pull . c1 2>err &&\n test_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n\n test_expect_success 'pull.rebase not set and --no-ff given' '\n git reset --hard c2 &&\n- git pull --no-ff . c1 2>err &&\n+ git pull . c1 2>err &&\n test_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n\n test_expect_success 'pull.rebase not set and --ff-only given' '\n git reset --hard c2 &&\n- test_must_fail git pull --ff-only . c1 2>err &&\n+ git pull . c1 2>err &&\n test_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n\nWhat do we get?\n\nnot ok 4 - pull.rebase not set and pull.ff=true\nnot ok 5 - pull.rebase not set and pull.ff=false\nnot ok 6 - pull.rebase not set and pull.ff=only\nnot ok 7 - pull.rebase not set and --rebase given\nnot ok 8 - pull.rebase not set and --no-rebase given\nnot ok 9 - pull.rebase not set and --ff given\nnot ok 10 - pull.rebase not set and --no-ff given\nnot ok 11 - pull.rebase not set and --ff-only given\n\nAll failures. Exactly as expected.\n\nSo what coverage precisely are we losing?\n\nCheers.\n\n[1] https://lore.kernel.org/git/20201204061623.1170745-13-felipe.contreras@gmail.com/\n\n-- \nFelipe Contreras\n"},{"id":"412081","messageId":"CAMP44s0uyxs4p+HJ5ZDrrKJs9wQW4tSCZzPonpvP=FcTGCcxSA@mail.gmail.com","threadId":"54807","inReplyTo":"xmqqo8j0io39.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v5 0/3] pull: stop warning on every pull","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-11T13:28:24Z","receivedAt":"2020-12-11T13:30:16Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Dec 11, 2020 at 5:22 AM Junio C Hamano wrote:\n>\n> Felipe Contreras writes:\n>\n> > The discussion about making fast-forward-only pulls the default is\n> > stuck on mud, and there's no agreement about what we should even\n> > be warning our users about.\n>\n> The above perception of yours is mostly due to misunderstanding, I\n> would have to say. We are in agreement on what we should be warning\n> about at least, assuming that you are expressing what you want\n> clearly in the latest round of responses and I understood them\n> correctly [*1*].\n\nI'm not trying to be difficult here, but at every round where you have\nstated what it is that I want, it's not actually what I want, and the\nlast round is no exception, in my option.\n\nLet's assume that I'm not explaining clearly what I want.\n\nIn the last round you said you wanted an error, not a warning. That's\nnot what I want; I'm proposing a warning.\n\nBut that's not what I was referring to here.\n\n> I do not know if others on the list agree, though.\n\nThis is what I was referring to. Initially there seemed to be some\ninterest, and suddenly that interest disappeared.\n\n> I do agree that there is no agreement on the behaviour in the\n> endgame.\n\nSee? I disagree.\n\nI think the endgame is clear. How we get there is where there's no agreement.\n\n> In principle, I am in favor of disabling the more\n> dangerous half of the \"git pull\" command for those who haven't\n> configured anything. But I can understand those who do not want\n> that behaviour, as the fallout would be quite big.\n\nAnd who is that? Did anyone in the list express that they did not want\nthat behavior?\n\n> > Even my straightforward patches about improving documentation, and\n> > the consistency of the UI with --merge and other obvious fixes\n> > lost traction.\n>\n> It may be obvious to you, but may not be to others on the list who\n> spoke in the thread and who didn't speak but read the discussion.\n>\n> I did see potential goodness in the documentation update and that\n> was why I offered polishment on top of your patches in a v3 round,\n> but seeing the suggestions dismissed without convincing arguments\n> before v4 was sent out would have discouraged even the most patient\n> reviewers among us. If you meant by \"lost traction\" the lack of\n> comments on v4, that was my reason for not commenting.\n\nI did not dismiss your suggestions, I replied to your suggestions [1].\nYou did not reply back.\n\nMoreover, in patch 2 I saw you had some confusion [2], in which you\nsaid you didn't see any value in updating the message without changing\nthe condition that triggers, to which I replied [3]: \"Maybe it will be\nclearer when I send all the patches.\"\n\nThat's why I sent v4; not because I thought the review of v3 was done,\nbut because we were stuck not seeing the evolution of the warning.\n\nIn v4 I went through every step of the evolution [4], and I went back\nto what I said in v3:\n\n At this point we can update the warning to mention that we are inside\n a non-fast-forward case. But it's not necessary.\n\nSo I did not dismiss the suggestion, I replied to it, and put a pin on it.\n\nYou can certainly bring the same suggestion in v4, but I seem to have\nconvinced Elijah Newren that \"fast-forward\" can be used as an adverb\nperfectly well, and it in fact is, in many places in the documentation\nboth internal, and external.\n\n> In any case, these three patches in this round looked quite sensible\n> to me, except for the tests in 3/3, and minor details of 2/3, both\n> of which I gave a more detailed review and suggestion.\n\nGreat.\n\nThat should improve the situation of most users. And also has the\nadded benefit that it's 3 less patches I have to carry around on every\nround.\n\nCheers.\n\n[1] https://lore.kernel.org/git/CAMP44s1ZDXzGfEqpTeiG=aGAYK40ebnBLQKAbA7KGtcePGARfw@mail.gmail.com/\n[2] https://lore.kernel.org/git/xmqq4kkx9vzx.fsf@gitster.c.googlers.com/\n[3] https://lore.kernel.org/git/CAMP44s1aYqzCVvELH8zULaTkOdgLSSAQ0LE8WfgQKLPfU2MHfg@mail.gmail.com/\n[4] https://lore.kernel.org/git/CAMP44s2hUCd9qc83LReGyjy8N+u++eK6VjwGhDhrX0f0SbKmig@mail.gmail.com\n\n-- \nFelipe Contreras\n"},{"id":"412101","messageId":"xmqqa6ujj3s4.fsf@gitster.c.googlers.com","threadId":"54807","inReplyTo":"CAMP44s0wjfZ9TeQzpJvVD-OzFA47HFd87TABiJo3Ec9H8j-fjA@mail.gmail.com","subject":"Re: [PATCH v5 3/3] pull: display default warning only when non-ff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-11T23:56:11Z","receivedAt":"2020-12-12T00:04:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>> We seem to be losing test coverage by checking how pull.ff=only prevents\n>> the command from working in a non-ff merge.\n>\n> No we don't. Remove the \"test_config pull.ff only\" and the test fails,\n> as expected.\n\n> ...\n> What do we get?\n>\n> not ok 4 - pull.rebase not set and pull.ff=true\n> not ok 5 - pull.rebase not set and pull.ff=false\n> not ok 6 - pull.rebase not set and pull.ff=only\n> not ok 7 - pull.rebase not set and --rebase given\n> not ok 8 - pull.rebase not set and --no-rebase given\n> not ok 9 - pull.rebase not set and --ff given\n> not ok 10 - pull.rebase not set and --no-ff given\n> not ok 11 - pull.rebase not set and --ff-only given\n>\n> All failures. Exactly as expected.\n\nAssuming only one kind of breakage and try to break exactly that\nthing does not prove much.\n\nI'll keep this short as I am supposed to be off officially.\n\nWith pulling without choosing between rebase/merge, the old code\nhad one behaviour wrt the message---it always advised, whether\nthe pull was ff or not.\n\nThe new code has two behaviour wrt this aspect. It behaves\ndifferently when the pull is ff or non-ff. That would double the\npossibility that needs to be tested if we wanted to keep covering\nthe original set of conditions *and* cover all new possibilities.\n\nI am saying that you should keep the original ones, and add new ones\nto cover the new cases if that matters. Otherwise the conditions\nunder which the original tests were checking would no longer be\ntested.\n\n> test_expect_success 'pull.rebase not set and --rebase given' '\n> -\tgit reset --hard c0 &&\n> +\tgit reset --hard c2 &&\n> \tgit pull --rebase . c1 2>err &&\n> \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n> '\n\nThis used to make sure an attempt to rebase c1 onto c0, which can be\nfast-forwarded, would work fine, even though it used to give\nwarning. We should keep testing the same condition. The\nexpectation of seeing the warning is what must be changed, not the\ntest condition (i.e. rebasing c1 onto c2 instead of c0)---you are no\nlonger making sure that c1 can be rebased onto c0 cleanly.\n\n"},{"id":"412106","messageId":"xmqq360bj3kt.fsf@gitster.c.googlers.com","threadId":"54807","inReplyTo":"CAMP44s170qZuZqmh_gVLGgJJ5005HzDM0fdQKQamjHkZCOu7_Q@mail.gmail.com","subject":"Re: [PATCH v5 2/3] pull: move default warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-12T00:00:34Z","receivedAt":"2020-12-12T01:01:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>> - correct the if() statement above, so that regardless of verbosity\n>> level, we can do _something_ common when the history does not\n>> fast-forward. I.e.\n>>\n>> if (rebase_unspecified && !opt_ff) {\n>> if (opt_verbosity >= 0)\n>> show_advice_pull_non_ff();\n>> }\n>>\n>> These would allow us to further turn the logic to\n>>\n>> if (rebase_unspecified && !opt_ff) {\n>> if (opt_verbosity >= 0 && advice_pull_non_ff)\n>> show_advice_pull_non_ff();\n>> die(\"not a fast-forward; must merge or rebase\");\n>> }\n>\n> Should actually be something like:\n>\n> if (rebase_unspecified && !can_ff)\n> die(\"Not a fast-forward; must either merge or rebase\");\n\nThe illustration I gave in the message you are responding to was\nmade in the context of patch 2/3; with patch 3/3 where can_ff\nexists, it would not become like what you gave above. It should\ninstead become\n\n\tif (rebase_unspecified && !opt_ff && !can_ff) {\n\t\tif (opt_verbosity >= 0 && advice_pull_non_ff)\n\t\t\tshow_advice_pull_non_ff();\n\t\tdie(\"not a fast-forward; must merge or rebase\");\n\t}\n\ni.e. when we can fast-forward, we do not trigger the \"you must\nspecify rebase/merge\" message, and we do not trigger the \"not a\nfast-forward\" error.\n"},{"id":"412107","messageId":"CAMP44s256RnOy0pPDCa1M18Ahu30i3NY4S7OZD41U0tfZWYTCQ@mail.gmail.com","threadId":"54807","inReplyTo":"xmqqa6ujj3s4.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v5 3/3] pull: display default warning only when non-ff","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-12T01:01:55Z","receivedAt":"2020-12-12T01:03:30Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Dec 11, 2020 at 5:56 PM Junio C Hamano wrote:\n>\n> Felipe Contreras writes:\n>\n> >> We seem to be losing test coverage by checking how pull.ff=only prevents\n> >> the command from working in a non-ff merge.\n> >\n> > No we don't. Remove the \"test_config pull.ff only\" and the test fails,\n> > as expected.\n>\n> > ...\n> > What do we get?\n> >\n> > not ok 4 - pull.rebase not set and pull.ff=true\n> > not ok 5 - pull.rebase not set and pull.ff=false\n> > not ok 6 - pull.rebase not set and pull.ff=only\n> > not ok 7 - pull.rebase not set and --rebase given\n> > not ok 8 - pull.rebase not set and --no-rebase given\n> > not ok 9 - pull.rebase not set and --ff given\n> > not ok 10 - pull.rebase not set and --no-ff given\n> > not ok 11 - pull.rebase not set and --ff-only given\n> >\n> > All failures. Exactly as expected.\n>\n> Assuming only one kind of breakage and try to break exactly that\n> thing does not prove much.\n\nIt proves we are testing exactly the thing the test is meant to test.\n\nNo test is ever perfect.\n\n> I'll keep this short as I am supposed to be off officially.\n\nI appreciate that.\n\n> With pulling without choosing between rebase/merge, the old code\n> had one behaviour wrt the message---it always advised, whether\n> the pull was ff or not.\n>\n> The new code has two behaviour wrt this aspect. It behaves\n> differently when the pull is ff or non-ff. That would double the\n> possibility that needs to be tested if we wanted to keep covering\n> the original set of conditions *and* cover all new possibilities.\n>\n> I am saying that you should keep the original ones, and add new ones\n> to cover the new cases if that matters. Otherwise the conditions\n> under which the original tests were checking would no longer be\n> tested.\n\nI disagree. The old condition is perfectly well tested with this test:\n\n test_expect_success 'pull.rebase not set' '\n git reset --hard c2 &&\n git -c color.advice=always pull . c1 2>err &&\n test_decode_color decoded &&\n test_i18ngrep \"hint: \" decoded &&\n test_i18ngrep \"Pulling without specifying how to reconcile\" decoded\n '\n\nIn other words; we have tests going horizontally, and tests going\nvertically. That should cover all the bases.\n\nBut fine, if you want a matrix of tests I can do that.\n\n> > test_expect_success 'pull.rebase not set and --rebase given' '\n> > - git reset --hard c0 &&\n> > + git reset --hard c2 &&\n> > git pull --rebase . c1 2>err &&\n> > test_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n> > '\n>\n> This used to make sure an attempt to rebase c1 onto c0, which can be\n> fast-forwarded, would work fine,\n\nNo it didn't. It may very well have done a merge, or done nothing at all.\n\nThe tests that actually check that \"git pull --rebase\" works both in\nthe fast-forward and non-fast-forward are in t/t5520-pull.sh.\n\nThe goal of this particular test is to check that the warning is not there.\n\n> even though it used to give\n> warning. We should keep testing the same condition.\n\nWe are testing what was originally tested: that the warning is not presented.\n\n> The expectation of seeing the warning is what must be changed, not the\n> test condition (i.e. rebasing c1 onto c2 instead of c0)---you are no\n> longer making sure that c1 can be rebased onto c0 cleanly.\n\nWe disagree on what the \"test condition\" is supposed to be on this test.\n\nBut fine, I'll create the matrix.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"412108","messageId":"CAMP44s1v+ESk9y8WC8rKPb61KD_kUoM9F1gO-TZhEEgOzUAPTw@mail.gmail.com","threadId":"54807","inReplyTo":"xmqq360bj3kt.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v5 2/3] pull: move default warning","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-12T01:05:04Z","receivedAt":"2020-12-12T01:06:57Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Dec 11, 2020 at 6:00 PM Junio C Hamano wrote:\n> Felipe Contreras writes:\n\n> > Should actually be something like:\n> >\n> > if (rebase_unspecified && !can_ff)\n> > die(\"Not a fast-forward; must either merge or rebase\");\n>\n> The illustration I gave in the message you are responding to was\n> made in the context of patch 2/3; with patch 3/3 where can_ff\n> exists, it would not become like what you gave above. It should\n> instead become\n>\n> if (rebase_unspecified && !opt_ff && !can_ff) {\n> if (opt_verbosity >= 0 && advice_pull_non_ff)\n> show_advice_pull_non_ff();\n> die(\"not a fast-forward; must merge or rebase\");\n> }\n>\n> i.e. when we can fast-forward, we do not trigger the \"you must\n> specify rebase/merge\" message, and we do not trigger the \"not a\n> fast-forward\" error.\n\nIt's not the !can_ff part I'm trying to highlight, it's the lack of\nadvice *after* we have decided to flip the switch.\n\nAs I said in another thread: I don't think we have any long\ncondescending error in any other command.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"412111","messageId":"xmqq1rfvgtvx.fsf@gitster.c.googlers.com","threadId":"54807","inReplyTo":"xmqqa6ujj3s4.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v5 3/3] pull: display default warning only when non-ff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-12T02:11:44Z","receivedAt":"2020-12-12T11:14:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> test_expect_success 'pull.rebase not set and --rebase given' '\n>> -\tgit reset --hard c0 &&\n>> +\tgit reset --hard c2 &&\n>> \tgit pull --rebase . c1 2>err &&\n>> \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n>> '\n>\n> This used to make sure an attempt to rebase c1 onto c0, which can be\n> fast-forwarded, would work fine, even though it used to give\n> warning. We should keep testing the same condition. The\n> expectation of seeing the warning is what must be changed, not the\n> test condition (i.e. rebasing c1 onto c2 instead of c0)---you are no\n> longer making sure that c1 can be rebased onto c0 cleanly.\n\nLet's try to explain it in a different way.\n\nThe original author of this test cared that pulling c1 with --rebase\ninto c0 succeeds, and that it does not give the error message. We\nhave no right [*1*] to say that scenario (i.e. \"pull --rebase\" c1\ninto c0) no longer matters without a good justification. And it is\nnot a good justification to say that the current code happens to\nbehave identically whether running \"pull --rebase\" of c1 into c0 or\nc2 so it is sufficient to test the operation into c2. The test is\n*not* about how the current code happens to work. It is to make\nsure the scenarios test authors care about will keep behaving the\nsame way.\n\nSome tests may be expecting that pulling c1 into c0 would issue the\nmessage, and that the command succeeds, and with the patch 3/3 the\noutcome may become different, i.e. the command succeeds without\nannoying message. That would break the expectation of the original\ntest authors, and it is a good thing. By recording a change to the\nexpectation, we can document how the new behaviour works better under\nthe same scenario.\n\n\n[Footnote]\n\n*1* That does not mean we must not care about other scenarios that\nare different from what have been tested with existing tests. If\nthere is new behaviour introduced by patch 3/3, it is prudent to\nprotect it from future breakage by adding a test that pulls c1 into\nc2, if that case is not already tested with existing tests. \n\nI suspect we already make sure a non-ff merge gives the annoying\nmessage while going ahead, so there may be no new additional test\nrequired, though.\n"},{"id":"412113","messageId":"xmqqv9d7faot.fsf@gitster.c.googlers.com","threadId":"54807","inReplyTo":"CAMP44s0uyxs4p+HJ5ZDrrKJs9wQW4tSCZzPonpvP=FcTGCcxSA@mail.gmail.com","subject":"Re: [PATCH v5 0/3] pull: stop warning on every pull","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-12T02:50:14Z","receivedAt":"2020-12-12T12:53:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> But that's not what I was referring to here.\n>\n>> I do not know if others on the list agree, though.\n>\n> This is what I was referring to. Initially there seemed to be some\n> interest, and suddenly that interest disappeared.\n\nPerhaps most of them are happy enough with the current behaviour.\n\nPerhaps nobody cares strongly enough to say what they want to see,\nas they fear that by speaking up they would be drawn into a\ndiscussion that is needlessly hot and unpleasant.\n\n>> I do agree that there is no agreement on the behaviour in the\n>> endgame.\n>\n> See? I disagree.\n>\n> I think the endgame is clear. How we get there is where there's no agreement.\n\nWhat you want as the endgame may be clear to you.\n\nBut I do not think there is clear concensus among people on the\nlist.\n\n>> In principle, I am in favor of disabling the more\n>> dangerous half of the \"git pull\" command for those who haven't\n>> configured anything. But I can understand those who do not want\n>> that behaviour, as the fallout would be quite big.\n>\n> And who is that? Did anyone in the list express that they did not want\n> that behavior?\n\nI thought that you at least saw Dscho's reaction to the breakage\ncaused by \"future\" patch in response to one of the recent What's\ncooking reports. Doesn't that count \"anyone on the list express\"?\n\nI am starting to feel myself that \"don't do anything if this is not\na fast-forward\" may be something that would have been great if we\nhad it from day one but is no longer a feasible default with\nexisting users, to be quite honest, so you can count my 20% as\nanother example.\n"},{"id":"412114","messageId":"5fd4df3f51f40_bc1eb20810@natae.notmuch","threadId":"54807","inReplyTo":"xmqq5z58n88i.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v5 1/3] pull: refactor fast-forward check","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-12T15:18:23Z","receivedAt":"2020-12-12T15:19:12Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hello,\n\nJust to state that I'm not ignoring this feedback.\n\nJunio C Hamano wrote:\n> Felipe Contreras writes:\n> \n> > It's much cleaner this way.\n> \n> It is obvious that a focused single purpose helper with less\n> indentation is easier to follow both at the calling site and the\n> implementation itself.\n\nYes, but the reader hasn't reached that point of the story yet.\n\n> \"It's much cleaner\" is not something you need to say.\n\nFrom the top of my head I can think of a few other reasons to refactor\ncode: a) it's more logical, b) it's less code, c) it's helps further\nchanges.\n\nIt's not necessary to say, but that's what I want to say; this reason,\nand no other reason, is the main reason for this patch's existence.\n\n> > Also, we would like to be able to make this check before the decision to\n> > rebase is made.\n> \n> ... in a future step.\n\nRight.\n\nIt's not necessarily the case (could be an indeterminate future), but it\nis the case in this patch series.\n\n> That is something we want to say upfront, not \"Also\".\n\nNot from my point of view. Even if it didn't help in future steps,\ncleaning up code is generally desirable.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"412115","messageId":"5fd4e94317d67_bc1eb208da@natae.notmuch","threadId":"54807","inReplyTo":"xmqq1rfvgtvx.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v5 3/3] pull: display default warning only when non-ff","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-12T16:01:07Z","receivedAt":"2020-12-12T16:02:22Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> >> test_expect_success 'pull.rebase not set and --rebase given' '\n> >> -\tgit reset --hard c0 &&\n> >> +\tgit reset --hard c2 &&\n> >> \tgit pull --rebase . c1 2>err &&\n> >> \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n> >> '\n> >\n> > This used to make sure an attempt to rebase c1 onto c0, which can be\n> > fast-forwarded, would work fine, even though it used to give\n> > warning. We should keep testing the same condition. The\n> > expectation of seeing the warning is what must be changed, not the\n> > test condition (i.e. rebasing c1 onto c2 instead of c0)---you are no\n> > longer making sure that c1 can be rebased onto c0 cleanly.\n> \n> Let's try to explain it in a different way.\n> \n> The original author of this test cared that pulling c1 with --rebase\n> into c0 succeeds, and that it does not give the error message.\n\nI prefer not to attempt to read minds (plus, the author might have not\ncared that the pulling succeeds), and anyway; that's not what matters.\n\nWhat matters is the current situation, and the desired situation.\n\n> We have no right [*1*] to say that scenario (i.e. \"pull --rebase\" c1\n> into c0) no longer matters without a good justification.\n\nBut nobody is saying that. What I said is that in *this* particular\ntest-case that's not the objective of the test. And if you consider\nhypothetical secondary objectives of the test, those are being tested\nelsewhere.\n\n> And it is not a good justification to say that the current code\n> happens to behave identically whether running \"pull --rebase\" of c1\n> into c0 or c2 so it is sufficient to test the operation into c2. The\n> test is *not* about how the current code happens to work. It is to\n> make sure the scenarios test authors care about will keep behaving the\n> same way.\n\nAgain, I don't particularly care to mindread what the test authors might\nhave cared about.\n\nIt's clear from the tests themselves what they are trying to do: check\nif the warning exists, or doesn't.\n\n> Some tests may be expecting that pulling c1 into c0 would issue the\n> message, and that the command succeeds, and with the patch 3/3 the\n> outcome may become different, i.e. the command succeeds without\n> annoying message.\n\nNo. All the tests (sans 1) check that the warning is *not* present.\n\nIf you do a fast-forward pull, the warning will not be present\n(regardless of options), and the test would pass, but for the wrong\nreasons.\n\n> That would break the expectation of the original\n> test authors, and it is a good thing. By recording a change to the\n> expectation, we can document how the new behaviour works better under\n> the same scenario.\n\nNo, the expectation has not changed one iota; it's exactly same.\n\nIt's the reason for the same output that changed.\n\nIf I take the current tests, and I remove the thing that makes them\nspecial (arguments and/or configuration), essentially making them all\n\"git pull\":\n\n@@ -35,52 +35,49 @@ test_expect_success 'pull.rebase not set' '\n \n test_expect_success 'pull.rebase not set and pull.ff=true' '\n \tgit reset --hard c0 &&\n-\ttest_config pull.ff true &&\n \tgit pull . c1 2>err &&\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n test_expect_success 'pull.rebase not set and pull.ff=false' '\n \tgit reset --hard c0 &&\n-\ttest_config pull.ff false &&\n \tgit pull . c1 2>err &&\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n test_expect_success 'pull.rebase not set and pull.ff=only' '\n \tgit reset --hard c0 &&\n-\ttest_config pull.ff only &&\n \tgit pull . c1 2>err &&\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n test_expect_success 'pull.rebase not set and --rebase given' '\n \tgit reset --hard c0 &&\n-\tgit pull --rebase . c1 2>err &&\n+\tgit pull . c1 2>err &&\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n test_expect_success 'pull.rebase not set and --no-rebase given' '\n \tgit reset --hard c0 &&\n-\tgit pull --no-rebase . c1 2>err &&\n+\tgit pull . c1 2>err &&\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n test_expect_success 'pull.rebase not set and --ff given' '\n \tgit reset --hard c0 &&\n-\tgit pull --ff . c1 2>err &&\n+\tgit pull . c1 2>err &&\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n test_expect_success 'pull.rebase not set and --no-ff given' '\n \tgit reset --hard c0 &&\n-\tgit pull --no-ff . c1 2>err &&\n+\tgit pull . c1 2>err &&\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n test_expect_success 'pull.rebase not set and --ff-only given' '\n \tgit reset --hard c0 &&\n-\tgit pull --ff-only . c1 2>err &&\n+\tgit pull . c1 2>err &&\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \nWhat happens?\n\nok 3 - pull.rebase not set and pull.ff=true\nok 4 - pull.rebase not set and pull.ff=false\nok 5 - pull.rebase not set and pull.ff=only\nok 6 - pull.rebase not set and --rebase given\nok 7 - pull.rebase not set and --no-rebase given\nok 8 - pull.rebase not set and --ff given\nok 9 - pull.rebase not set and --no-ff given\nok 10 - pull.rebase not set and --ff-only given\n\nThey all pass. What are they supposed to be testing now? I don't know.\n\nIn my opinion they are no-ops now, but fine; I'll leave them as is.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"412116","messageId":"5fd4f189a1f4e_bc1eb20891@natae.notmuch","threadId":"54807","inReplyTo":"xmqqv9d7faot.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v5 0/3] pull: stop warning on every pull","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-12T16:36:25Z","receivedAt":"2020-12-12T16:37:42Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Felipe Contreras writes:\n> \n> > But that's not what I was referring to here.\n> >\n> >> I do not know if others on the list agree, though.\n> >\n> > This is what I was referring to. Initially there seemed to be some\n> > interest, and suddenly that interest disappeared.\n> \n> Perhaps most of them are happy enough with the current behaviour.\n\nOr perhaps they are happhy enough the current behavior *for them*, but\nnot the other users.\n\n> Perhaps nobody cares strongly enough to say what they want to see,\n> as they fear that by speaking up they would be drawn into a\n> discussion that is needlessly hot and unpleasant.\n\nOr perhaps they got busy.\n\nOr perhaps they got sick.\n\nI don't see how listing hypotheticals is fruitful.\n\n> >> I do agree that there is no agreement on the behaviour in the\n> >> endgame.\n> >\n> > See? I disagree.\n> >\n> > I think the endgame is clear. How we get there is where there's no agreement.\n> \n> What you want as the endgame may be clear to you.\n> \n> But I do not think there is clear concensus among people on the\n> list.\n\nI do. It has been clear for many years.\n\n> >> In principle, I am in favor of disabling the more\n> >> dangerous half of the \"git pull\" command for those who haven't\n> >> configured anything. But I can understand those who do not want\n> >> that behaviour, as the fallout would be quite big.\n> >\n> > And who is that? Did anyone in the list express that they did not want\n> > that behavior?\n> \n> I thought that you at least saw Dscho's reaction to the breakage\n> caused by \"future\" patch in response to one of the recent What's\n> cooking reports.\n\nYes, I saw it, but it was a complaint about the breakage of test due to\nthe merge of a patch that wasn't meant to be merged today.\n\n> Doesn't that count \"anyone on the list express\"?\n\nNo. These are two different statements.\n\n 1. This will break things for *me*\n 2. This is a bad thing for *everyone*\n\nThese two statements have a different object. Also, the first doesn't\nnecessarily require rationale, we can take his word for it. But the\nsecond one does.\n\nIt's not enough to say \"I think this patch is bad for everyone\"; you\nhave to explain *why*. I'd be happy to discuss with him his rationale,\nwhich he hasn't given.\n\nMoreover, isn't Schindelin advocating for a change that will cause\nbreakage (renaming the master branch).\n\n> I am starting to feel myself that \"don't do anything if this is not\n> a fast-forward\" may be something that would have been great if we\n> had it from day one but is no longer a feasible default with\n> existing users, to be quite honest, so you can count my 20% as\n> another example.\n\nAs opposed to virtually everyone in this mailing list that has given an\nopinion about the topic in the last 10 years (including you I count 3\nout of literally dozens).\n\nI will do some mail archeology and present the results.\n\nAnyway, this is not relevant today, because you were the one that\nproposed to go straight to an error.\n\nWhat I propose for today is to introduce the option\n\"pull.mode=fast-forward\", and improve the warning. Not an error.\n\nThat doesn't break the behavior for anyone, including Schindelin.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"412117","messageId":"5fd4f2f03f28e_bc1eb20890@natae.notmuch","threadId":"54807","inReplyTo":"CAMP44s170qZuZqmh_gVLGgJJ5005HzDM0fdQKQamjHkZCOu7_Q@mail.gmail.com","subject":"Re: [PATCH v5 2/3] pull: move default warning","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-12T16:42:24Z","receivedAt":"2020-12-12T16:43:10Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Felipe Contreras wrote:\n> On Fri, Dec 11, 2020 at 12:54 AM Junio C Hamano wrote:\n\n> > - by introducing a local variable in cmd_pull() and passing a\n> > pointer to it to config_get_rebase(), we can easily avoid having\n> > to rely on an extra global variable.\n> >\n> > I'd suggest addressing the above along the following lines.\n> \n> ...\n> \n> > to possibly cause it to set to true, and use that instead of the\n> > global variable to decide if we want to give the help text.\n> \n> Yeah, there's only 38 global variables. We wouldn't want another one\n> which makes the code pretty straightforward.\n\nAnother thing that I mentioned in another version of the patch series\nbut not on this one (since the relevant patch was removed) is that this\nvariable is only needed temporarily; it can be removed by further code\nreorganization.\n\nSo, I don't think it makes sense to change more code than necessary for\nthe sake of this variable (that might very well disappear).\n\n-- \nFelipe Contreras\n"},{"id":"412158","messageId":"CAMP44s2VoMeFaS-eBSJt2N9T2T1aPHJ4-XeaA34J95t+RztXHw@mail.gmail.com","threadId":"54807","inReplyTo":"5fd4f189a1f4e_bc1eb20891@natae.notmuch","subject":"Re: [PATCH v5 0/3] pull: stop warning on every pull","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-14T00:57:15Z","receivedAt":"2020-12-14T00:58:12Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Dec 12, 2020 at 10:36 AM Felipe Contreras\n wrote:\n> Junio C Hamano wrote:\n\n> > I am starting to feel myself that \"don't do anything if this is not\n> > a fast-forward\" may be something that would have been great if we\n> > had it from day one but is no longer a feasible default with\n> > existing users, to be quite honest, so you can count my 20% as\n> > another example.\n>\n> As opposed to virtually everyone in this mailing list that has given an\n> opinion about the topic in the last 10 years (including you I count 3\n> out of literally dozens).\n\nA quick update after reading a great deal of mails.\n\nThe only person I counted in 2013 that was against changing the\ndefault is Matthieu Moy, and reading back what he said, even him was\nin favor of adding a configuration for the new mode [1]. What he was\nagainst was making it the default, and adding a warning stating it\nwould be the default.\n\nFortunately my patches introduce the mode [2] without making it\nnecessarily the default (we don't even have to change the warning and\nsay it will be changed in the future [3]).\n\nSo *nobody* was against the introduction of such mode.\n\nAlso, after reading the input from GitHub trainers, which Jeff\nprovided [4], perhaps leaving the default as \"merge\" makes sense. Our\ncurse of knowledge may bias us into thinking the user knows what a\nrebase is. Maybe a permanent advice warning is the way to go, I'm\nstill mulling it over.\n\n> Anyway, this is not relevant today, because you were the one that\n> proposed to go straight to an error.\n>\n> What I propose for today is to introduce the option\n> \"pull.mode=fast-forward\", and improve the warning. Not an error.\n>\n> That doesn't break the behavior for anyone, including Schindelin.\n\nThis is still true. We don't lose anything by introducing the mode.\n\nWe can make the decision to flip the default later (or not).\n\nCheers.\n\n[1] https://lore.kernel.org/git/vpqbo41lo2v.fsf@anie.imag.fr/\n[2] https://lore.kernel.org/git/20201208002648.1370414-18-felipe.contreras@gmail.com/\n[3] https://lore.kernel.org/git/20201208002648.1370414-19-felipe.contreras@gmail.com/\n[4] https://lore.kernel.org/git/20130909201751.GA14437@sigill.intra.peff.net/\n\n-- \nFelipe Contreras\n"},{"id":"412160","messageId":"xmqqpn3cep51.fsf@gitster.c.googlers.com","threadId":"54807","inReplyTo":"CAMP44s1v+ESk9y8WC8rKPb61KD_kUoM9F1gO-TZhEEgOzUAPTw@mail.gmail.com","subject":"Re: [PATCH v5 2/3] pull: move default warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-13T20:58:41Z","receivedAt":"2020-12-14T09:03:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Fri, Dec 11, 2020 at 6:00 PM Junio C Hamano wrote:\n>> Felipe Contreras writes:\n>\n>> > Should actually be something like:\n>> >\n>> > if (rebase_unspecified && !can_ff)\n>> > die(\"Not a fast-forward; must either merge or rebase\");\n>>\n>> The illustration I gave in the message you are responding to was\n>> made in the context of patch 2/3; with patch 3/3 where can_ff\n>> exists, it would not become like what you gave above. It should\n>> instead become\n>>\n>> if (rebase_unspecified && !opt_ff && !can_ff) {\n>> if (opt_verbosity >= 0 && advice_pull_non_ff)\n>> show_advice_pull_non_ff();\n>> die(\"not a fast-forward; must merge or rebase\");\n>> }\n>>\n>> i.e. when we can fast-forward, we do not trigger the \"you must\n>> specify rebase/merge\" message, and we do not trigger the \"not a\n>> fast-forward\" error.\n>\n> It's not the !can_ff part I'm trying to highlight, it's the lack of\n> advice *after* we have decided to flip the switch.\n>\n> As I said in another thread: I don't think we have any long\n> condescending error in any other command.\n\nOnly the \"not a fast-forward and you must choose between merge and\nrebase\" part (i.e. what I listed as the first part of three-part\nmessage) is the error.\n\nThe rest, the message that teaches how to choose between merge and\nrebase from command line and configuration (or how to choose\npermanently not to make the choice between the two), is not an\nerror---it's called advice.\n\nEven if we were to introduce the third choice (i.e. permanently not\nto choose between rebase or merge and instead just error out without\ngetting advice), the advice must stay for those who didn't make any\nchoice among the three (i.e. merge, rebase or ff-only).\n\nWe have many of them. An easy example to spot is a similar sized\nonefor \"git checkout HEAD^0\". Neither that one or the one under\ndiscussion is particularly condescending.\n"},{"id":"412161","messageId":"5fd7466250a4f_d17f5208fc@natae.notmuch","threadId":"54807","inReplyTo":"xmqqpn3cep51.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v5 2/3] pull: move default warning","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-14T11:02:58Z","receivedAt":"2020-12-14T11:16:40Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Felipe Contreras writes:\n> \n> > On Fri, Dec 11, 2020 at 6:00 PM Junio C Hamano wrote:\n> >> Felipe Contreras writes:\n> >\n> >> > Should actually be something like:\n> >> >\n> >> > if (rebase_unspecified && !can_ff)\n> >> > die(\"Not a fast-forward; must either merge or rebase\");\n> >>\n> >> The illustration I gave in the message you are responding to was\n> >> made in the context of patch 2/3; with patch 3/3 where can_ff\n> >> exists, it would not become like what you gave above. It should\n> >> instead become\n> >>\n> >> if (rebase_unspecified && !opt_ff && !can_ff) {\n> >> if (opt_verbosity >= 0 && advice_pull_non_ff)\n> >> show_advice_pull_non_ff();\n> >> die(\"not a fast-forward; must merge or rebase\");\n> >> }\n> >>\n> >> i.e. when we can fast-forward, we do not trigger the \"you must\n> >> specify rebase/merge\" message, and we do not trigger the \"not a\n> >> fast-forward\" error.\n> >\n> > It's not the !can_ff part I'm trying to highlight, it's the lack of\n> > advice *after* we have decided to flip the switch.\n> >\n> > As I said in another thread: I don't think we have any long\n> > condescending error in any other command.\n> \n> Only the \"not a fast-forward and you must choose between merge and\n> rebase\" part (i.e. what I listed as the first part of three-part\n> message) is the error.\n> \n> The rest, the message that teaches how to choose between merge and\n> rebase from command line and configuration (or how to choose\n> permanently not to make the choice between the two), is not an\n> error---it's called advice.\n\nRight. After re-reading the 2013 discussion, and the GitHub trainers'\ninput that Jeff King shared with us [1], it might make sense to\neventually keep this advice (permanently), since many users might not\nknow what a rebase is, and would not want to know at this point of their\nlearning curve.\n\nThat being said, the advice still needs to be improved, and I have\nalready yet another version taking this into consideration ready to be\nsent. Just waiting for feedback on the previous can_ff series.\n\n> Even if we were to introduce the third choice (i.e. permanently not\n> to choose between rebase or merge and instead just error out without\n> getting advice), the advice must stay for those who didn't make any\n> choice among the three (i.e. merge, rebase or ff-only).\n\nWith the introduction of such mode, and the permanent advice, we would\nhave:\n\n if (!can_ff) {\n\t if (!mode && opt_verbosity >= 0 && advice_pull_non_ff)\n\t\t show_advice_pull_non_ff();\n\n\t if (mode == PULL_MODE_FF_ONLY)\n\t\t die(_(\"Not a fast-forward, either merge or rebase.\\n\"));\n }\n\nIf in the future we decide to make it the default, we would have:\n\n if (!can_ff) {\n\t if (!mode && opt_verbosity >= 0 && advice_pull_non_ff)\n\t\t show_advice_pull_non_ff();\n\n\t if (!mode || mode == PULL_MODE_FF_ONLY)\n\t\t die(_(\"Not a fast-forward, either merge or rebase.\\n\"));\n }\n\nThe transition is very straightforward.\n\n> We have many of them. An easy example to spot is a similar sized\n> onefor \"git checkout HEAD^0\". Neither that one or the one under\n> discussion is particularly condescending.\n\nI am not familiar with these \"advices\", since I basically ignore them,\nand they are not presented to me in any different color, or special\npresentation. I generally ignore textwalls in my command line.\n\nBut fine, if these walls of text already exist, perhaps it makes sense\nto have yet another permanent one (just with better text, and better\noptions).\n\nWith an improved advice--which includes an option that is good enough we\n*can* make the default (pull.mode=ff-only)--perhaps that would be\nenough, and in a couple of years we can make the assessment if it's\nenough or not.\n\nBut clearly *today* we don't have such mode, which people have found\nmissing for more than a decadte.\n\nCheers.\n\n[1] https://lore.kernel.org/git/20130909201751.GA14437@sigill.intra.peff.net/\n\n-- \nFelipe Contreras\n"},{"id":"412205","messageId":"xmqqo8iwje0h.fsf@gitster.c.googlers.com","threadId":"54807","inReplyTo":"5fd4e94317d67_bc1eb208da@natae.notmuch","subject":"Re: [PATCH v5 3/3] pull: display default warning only when non-ff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-14T21:04:14Z","receivedAt":"2020-12-14T21:05:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n>> The original author of this test cared that pulling c1 with --rebase\n>> into c0 succeeds, and that it does not give the error message.\n>\n> I prefer not to attempt to read minds (plus, the author might have not\n> cared that the pulling succeeds), and anyway; that's not what matters.\n> ...\n> Again, I don't particularly care to mindread what the test authors might\n> have cared about.\n\nYou do not have to read mind. What is written in the test is clear\nenough: run \"git pull --rebase . c1\" into c0 and see what happens.\n"},{"id":"412209","messageId":"5fd7dbe1eb26d_d5985208e1@natae.notmuch","threadId":"54807","inReplyTo":"xmqqo8iwje0h.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v5 3/3] pull: display default warning only when non-ff","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-14T21:40:49Z","receivedAt":"2020-12-14T21:41:53Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Felipe Contreras writes:\n> \n> >> The original author of this test cared that pulling c1 with --rebase\n> >> into c0 succeeds, and that it does not give the error message.\n> >\n> > I prefer not to attempt to read minds (plus, the author might have not\n> > cared that the pulling succeeds), and anyway; that's not what matters.\n> > ...\n> > Again, I don't particularly care to mindread what the test authors might\n> > have cared about.\n> \n> You do not have to read mind. What is written in the test is clear\n> enough: run \"git pull --rebase . c1\" into c0 and see what happens.\n\nThat is precisely my point: it is written in the test, no mindreading\nneccessary.\n\n-- \nFelipe Contreras\n"}]}