{"thread":{"id":"54823","subject":"[PATCH v7 0/5] making pull advice not to trigger when unneeded","startedAt":"2020-12-14T20:27:58Z","lastAt":"2020-12-23T14:11:04Z","messageCount":21,"participants":["Junio C Hamano","Felipe Contreras"],"isPatch":true,"patchVersion":7,"patchTotal":5},"messages":[{"id":"412198","messageId":"20201214202647.3340193-1-gitster@pobox.com","threadId":"54823","inReplyTo":null,"subject":"[PATCH v7 0/5] making pull advice not to trigger when unneeded","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-14T20:26:42Z","receivedAt":"2020-12-14T20:27:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Just to show what was pushed out on 'seen' while I was cutting the\npreview release...\n\nThis is based on Felipe's v6 (which was mislabled as v5 but sent on\na different day from the true v5), with two clean-up commits\ninserted between Felipe's second step (now 2/5) and the third step\n(now 5/5, with necessary adjustments).\n\nEven with the \"correct condition\" clean-up, I am not quite happy\nwith the clarity of the logic around there.\n\nI think !opt_ff does not exactly belong to the condition, if we\nconsider that the eventual endgame should be to stop non-ff\noperation without merge or rebase by default with an error, and that\nshould happen in this block.  The error message there should say\n\"unable to fast-forward; must merge or rebase\", which should equally\napply to those who gave \"--ff-only\" explicitly, even though they may\nnot need the advice message.\n\nSo with the help of can_ff bit introduced by 5/5, I _think_ this\npart should become like\n\n-\tif (rebase_unspecified && !opt_ff && !can_ff) {\n+\tif (rebase_unspecified && !can_ff &&\n+\t    (!opt_ff || !strcmp(\"--ff-only\", opt_ff))) {\n\nbefore/when we make this codepath error out.  We won't hit the body\nof this if statement when we can fast-forward.\n\nTo avoid the ugly-looking \"strcmp()\" in the above, we may need to\nadjust \"--ff\" (fast-forward without creating an extra merge commit)\nand \"--no-ff\" (create an extra merge commit when the history could\nbe fast-forwarded) to imply \"merge\", though.  It would automatically\nmake rebase_unspecified would become false.  With such a tweak, we\ncan then simplify it further to\n\n-\tif (rebase_unspecified && !can_ff &&\n-\t    (!opt_ff || !strcmp(\"--ff-only\", opt_ff))) {\n+\tif (rebase_unspecified && !can_ff) {\n\nBut these are not part of this round.\n\n\nFelipe Contreras (3):\n  pull: refactor fast-forward check\n  pull: give the advice for choosing rebase/merge much later\n  pull: display default warning only when non-ff\n\nJunio C Hamano (2):\n  pull: get rid of unnecessary global variable\n  pull: correct condition to trigger non-ff advice\n\n builtin/pull.c               | 70 ++++++++++++++++++++++--------------\n t/t7601-merge-pull-config.sh | 66 +++++++++++++++++++++++++++++++---\n 2 files changed, 104 insertions(+), 32 deletions(-)\n\n-- \n2.30.0-rc0-186-g20447144ec\n\n"},{"id":"412199","messageId":"20201214202647.3340193-3-gitster@pobox.com","threadId":"54823","inReplyTo":"20201214202647.3340193-1-gitster@pobox.com","subject":"[PATCH v7 2/5] pull: give the advice for choosing rebase/merge much later","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-14T20:26:44Z","receivedAt":"2020-12-14T20:28:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Felipe Contreras <felipe.contreras@gmail.com>\n\nEventually we want to be omit the advice when we can fast-forward\nin which case there is no reason to require the user to choose\nbetween rebase or merge.\n\nIn order to do so, we need to delay giving the advice up to the\npoint where we can check if we can fast-forward or not.\n\nAdditionally, config_get_rebase() was probably never its true home.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\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.30.0-rc0-186-g20447144ec\n\n"},{"id":"412200","messageId":"20201214202647.3340193-4-gitster@pobox.com","threadId":"54823","inReplyTo":"20201214202647.3340193-1-gitster@pobox.com","subject":"[PATCH v7 3/5] pull: get rid of unnecessary global variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-14T20:26:45Z","receivedAt":"2020-12-14T20:28:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"It is easy enough to do, gives a more descriptive name to the\nvariable, and there is no reason to make the code deliberately worse\nby ignoring improvement offered on the list.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/pull.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex ff8e3ce137..2976b8e5cb 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -27,8 +27,6 @@\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@@ -326,7 +324,7 @@ static const char *config_get_ff(void)\n  * looks for the value of \"pull.rebase\". If both configuration keys do not\n  * exist, returns REBASE_FALSE.\n  */\n-static enum rebase_type config_get_rebase(void)\n+static enum rebase_type config_get_rebase(int *rebase_unspecified)\n {\n \tstruct branch *curr_branch = branch_get(\"HEAD\");\n \tconst char *value;\n@@ -346,7 +344,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-\tdefault_mode = 1;\n+\t*rebase_unspecified = 1;\n \n \treturn REBASE_FALSE;\n }\n@@ -934,6 +932,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 rebase_unspecified = 0;\n \n \tif (!getenv(\"GIT_REFLOG_ACTION\"))\n \t\tset_reflog_message(argc, argv);\n@@ -955,7 +954,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\topt_ff = xstrdup_or_null(config_get_ff());\n \n \tif (opt_rebase < 0)\n-\t\topt_rebase = config_get_rebase();\n+\t\topt_rebase = config_get_rebase(&rebase_unspecified);\n \n \tif (read_cache_unmerged())\n \t\tdie_resolve_conflict(\"pull\");\n@@ -1029,7 +1028,7 @@ 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+\tif (rebase_unspecified && 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-- \n2.30.0-rc0-186-g20447144ec\n\n"},{"id":"412201","messageId":"20201214202647.3340193-5-gitster@pobox.com","threadId":"54823","inReplyTo":"20201214202647.3340193-1-gitster@pobox.com","subject":"[PATCH v7 4/5] pull: correct condition to trigger non-ff advice","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-14T20:26:46Z","receivedAt":"2020-12-14T20:28:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Refactor the advise() call that teaches users how they can choose\nbetween merge and rebase into a helper function.  This revealed that\nthe caller's logic needs to be further clarified to allow future\nactions (like \"erroring out\" instead of the current \"go ahead and\nmerge anyway\") that should happen whether the advice message is\nsquelched out.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/pull.c | 32 +++++++++++++++++++-------------\n 1 file changed, 19 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 2976b8e5cb..1b87ea95eb 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -925,6 +925,22 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_\n \treturn ret;\n }\n \n+static void show_advice_pull_non_ff(void)\n+{\n+\tadvise(_(\"Pulling without specifying how to reconcile divergent branches is\\n\"\n+\t\t \"discouraged. You can squelch this message by running one of the following\\n\"\n+\t\t \"commands sometime before your next pull:\\n\"\n+\t\t \"\\n\"\n+\t\t \"  git config pull.rebase false  # merge (the default strategy)\\n\"\n+\t\t \"  git config pull.rebase true   # rebase\\n\"\n+\t\t \"  git config pull.ff only       # fast-forward only\\n\"\n+\t\t \"\\n\"\n+\t\t \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n+\t\t \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n+\t\t \"or --ff-only on the command line to override the configured default per\\n\"\n+\t\t \"invocation.\\n\"));\n+}\n+\n int cmd_pull(int argc, const char **argv, const char *prefix)\n {\n \tconst char *repo, **refspecs;\n@@ -1028,19 +1044,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 (rebase_unspecified && 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+\tif (rebase_unspecified && !opt_ff) {\n+\t\tif (opt_verbosity >= 0)\n+\t\t\tshow_advice_pull_non_ff();\n \t}\n \n \tif (opt_rebase) {\n-- \n2.30.0-rc0-186-g20447144ec\n\n"},{"id":"412202","messageId":"20201214202647.3340193-2-gitster@pobox.com","threadId":"54823","inReplyTo":"20201214202647.3340193-1-gitster@pobox.com","subject":"[PATCH v7 1/5] pull: refactor fast-forward check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-14T20:26:43Z","receivedAt":"2020-12-14T20:29:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Felipe Contreras <felipe.contreras@gmail.com>\n\nWe would like to be able to make this check before the decision to\nrebase is made in a future step.  Besides, using a separate helper\nmakes the code easier to follow.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\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.30.0-rc0-186-g20447144ec\n\n"},{"id":"412203","messageId":"20201214202647.3340193-6-gitster@pobox.com","threadId":"54823","inReplyTo":"20201214202647.3340193-1-gitster@pobox.com","subject":"[PATCH v7 5/5] pull: display default warning only when non-ff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-14T20:26:47Z","receivedAt":"2020-12-14T20:29:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Felipe Contreras <felipe.contreras@gmail.com>\n\nThere's no need to display the annoying warning on every pull... only\nthe ones that are not fast-forward.\n\nThe current warning tests still pass, but not because of the arguments\nor the configuration, but because they are all fast-forward.\n\nWe need to test non-fast-forward situations now.\n\nSuggestions-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/pull.c               |  7 ++--\n t/t7601-merge-pull-config.sh | 66 +++++++++++++++++++++++++++++++++---\n 2 files changed, 66 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 1b87ea95eb..e8927fc2ff 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -949,6 +949,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tstruct object_id rebase_fork_point;\n \tint autostash;\n \tint rebase_unspecified = 0;\n+\tint can_ff;\n \n \tif (!getenv(\"GIT_REFLOG_ACTION\"))\n \t\tset_reflog_message(argc, argv);\n@@ -1044,7 +1045,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 (rebase_unspecified && !opt_ff) {\n+\tcan_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);\n+\n+\tif (rebase_unspecified && !opt_ff && !can_ff) {\n \t\tif (opt_verbosity >= 0)\n \t\t\tshow_advice_pull_non_ff();\n \t}\n@@ -1063,7 +1066,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..52e8ccc933 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -29,11 +29,8 @@ test_expect_success 'setup' '\n \n test_expect_success 'pull.rebase not set' '\n \tgit reset --hard c0 &&\n-\tgit -c color.advice=always pull . c1 2>err &&\n-\ttest_decode_color <err >decoded &&\n-\ttest_i18ngrep \"<YELLOW>hint: \" decoded &&\n-\ttest_i18ngrep \"Pulling without specifying how to reconcile\" decoded\n-\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@@ -87,6 +84,65 @@ test_expect_success 'pull.rebase not set and --ff-only given' '\n \ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n '\n \n+test_expect_success 'pull.rebase not set (not-fast-forward)' '\n+\tgit reset --hard c2 &&\n+\tgit -c color.advice=always pull . c1 2>err &&\n+\ttest_decode_color <err >decoded &&\n+\ttest_i18ngrep \"<YELLOW>hint: \" decoded &&\n+\ttest_i18ngrep \"Pulling without specifying how to reconcile\" decoded\n+'\n+\n+test_expect_success 'pull.rebase not set and pull.ff=true (not-fast-forward)' '\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 (not-fast-forward)' '\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 (not-fast-forward)' '\n+\tgit reset --hard c2 &&\n+\ttest_config pull.ff only &&\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 (not-fast-forward)' '\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 (not-fast-forward)' '\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 (not-fast-forward)' '\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 (not-fast-forward)' '\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 (not-fast-forward)' '\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 test_expect_success 'merge c1 with c2' '\n \tgit reset --hard c1 &&\n \ttest -f c0.c &&\n-- \n2.30.0-rc0-186-g20447144ec\n\n"},{"id":"412204","messageId":"5fd7d22ceec2b_d4113208c1@natae.notmuch","threadId":"54823","inReplyTo":"20201214202647.3340193-4-gitster@pobox.com","subject":"RE: [PATCH v7 3/5] pull: get rid of unnecessary global variable","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-14T20:59:24Z","receivedAt":"2020-12-14T21:04:10Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> It is easy enough to do,\n\nYes.\n\n> gives a more descriptive name to the variable,\n\nI disagree.\n\n> and there is no reason to make the code deliberately worse by ignoring\n> improvement offered on the list.\n\nI doubt any person contributing to the mailing is making the code\ndeliberately worse.\n\nAnd I certainly did not ignore any improvement on the list. I responded\nto the suggestion, I just disagreed it's actually an improvement.\n\nIn my opinion differences of opinion must be tolerated.\n\nAnd I don't think any of that belongs in the commit message. \"Gives a\nmore descriptive name to the variable\" (in your opinion) should be\nenough.\n\nAnyway, I like the fact that your opinion and my opinion are clearly\ndemarcated in two different commits.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"412207","messageId":"5fd7d67c898e4_d44af20879@natae.notmuch","threadId":"54823","inReplyTo":"20201214202647.3340193-5-gitster@pobox.com","subject":"RE: [PATCH v7 4/5] pull: correct condition to trigger non-ff advice","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-14T21:17:48Z","receivedAt":"2020-12-14T21:18:49Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Refactor the advise() call that teaches users how they can choose\n> between merge and rebase into a helper function.  This revealed that\n> the caller's logic needs to be further clarified to allow future\n> actions (like \"erroring out\" instead of the current \"go ahead and\n> merge anyway\") that should happen whether the advice message is\n> squelched out.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  builtin/pull.c | 32 +++++++++++++++++++-------------\n>  1 file changed, 19 insertions(+), 13 deletions(-)\n> \n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 2976b8e5cb..1b87ea95eb 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -925,6 +925,22 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_\n>  \treturn ret;\n>  }\n>  \n> +static void show_advice_pull_non_ff(void)\n> +{\n> +\tadvise(_(\"Pulling without specifying how to reconcile divergent branches is\\n\"\n> +\t\t \"discouraged. You can squelch this message by running one of the following\\n\"\n> +\t\t \"commands sometime before your next pull:\\n\"\n> +\t\t \"\\n\"\n> +\t\t \"  git config pull.rebase false  # merge (the default strategy)\\n\"\n> +\t\t \"  git config pull.rebase true   # rebase\\n\"\n> +\t\t \"  git config pull.ff only       # fast-forward only\\n\"\n> +\t\t \"\\n\"\n> +\t\t \"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n> +\t\t \"preference for all repositories. You can also pass --rebase, --no-rebase,\\n\"\n> +\t\t \"or --ff-only on the command line to override the configured default per\\n\"\n> +\t\t \"invocation.\\n\"));\n> +}\n\n\nWhile this change is good, I see a lot of advice code setting up a\nseparate constant message, like:\n\n  static const char message_advice_pull_non_ff[] =\n          N_(\"...\");\n\n>  int cmd_pull(int argc, const char **argv, const char *prefix)\n>  {\n>  \tconst char *repo, **refspecs;\n> @@ -1028,19 +1044,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 (rebase_unspecified && 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> +\tif (rebase_unspecified && !opt_ff) {\n> +\t\tif (opt_verbosity >= 0)\n> +\t\t\tshow_advice_pull_non_ff();\n\nThen, we could just do:\n\n  if (opt_verbosity >= 0)\n          advise(_(message_advice_pull_non_ff)).\n\nOr even better:\n\n  if (opt_verbosity >= 0)\n          advise_if_enabled(ADVICE_PULL_NON_FF, _(message_advice_pull_non_ff));\n\nI'm not familiar with the advise code, I don't know if there's any good\nreason to setup these separate messages, so feel free to disregard this\nsuggestion.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"412208","messageId":"5fd7d82a9b6ab_d44af208bb@natae.notmuch","threadId":"54823","inReplyTo":"20201214202647.3340193-6-gitster@pobox.com","subject":"RE: [PATCH v7 5/5] pull: display default warning only when non-ff","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-14T21:24:58Z","receivedAt":"2020-12-14T21:25:50Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> @@ -1044,7 +1045,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 (rebase_unspecified && !opt_ff) {\n> +\tcan_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);\n> +\n> +\tif (rebase_unspecified && !opt_ff && !can_ff) {\n>  \t\tif (opt_verbosity >= 0)\n>  \t\t\tshow_advice_pull_non_ff();\n>  \t}\n\nI strongly predict the conditionals will end up looking similar to:\n\n  if (!can_ff) {\n          if (rebase_unspecified && !opt_ff && opt_verbosity >= 0)\n                  show_advice_pull_non_ff();\n  }\n\nBut OK.\n\n-- \nFelipe Contreras\n"},{"id":"412217","messageId":"xmqqv9d49dxk.fsf@gitster.c.googlers.com","threadId":"54823","inReplyTo":"5fd7d22ceec2b_d4113208c1@natae.notmuch","subject":"Re: [PATCH v7 3/5] pull: get rid of unnecessary global variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-14T23:16:07Z","receivedAt":"2020-12-14T23:18:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>> It is easy enough to do,\n>\n> Yes.\n>\n>> gives a more descriptive name to the variable,\n>\n> I disagree.\n>\n>> and there is no reason to make the code deliberately worse by ignoring\n>> improvement offered on the list.\n>\n> I doubt any person contributing to the mailing is making the code\n> deliberately worse.\n\nOh, I doubt that you do so with what you send out.  I am saying that\nyou do so by not taking improvements.  It wastes reviewers' time,\nraises noise ratio in the list traffic, and demotivates readers.\n\n"},{"id":"412218","messageId":"xmqqr1ns9drv.fsf@gitster.c.googlers.com","threadId":"54823","inReplyTo":"5fd7d67c898e4_d44af20879@natae.notmuch","subject":"Re: [PATCH v7 4/5] pull: correct condition to trigger non-ff advice","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-14T23:19:32Z","receivedAt":"2020-12-14T23:20:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Then, we could just do:\n>\n>   if (opt_verbosity >= 0)\n>           advise(_(message_advice_pull_non_ff)).\n>\n> Or even better:\n>\n>   if (opt_verbosity >= 0)\n>           advise_if_enabled(ADVICE_PULL_NON_FF, _(message_advice_pull_non_ff));\n\nI do not think we've decided what's the right way to squelch this\nadvice, so it is a bit premature to favor the latter over the\nformer.  Between a fixed message[] variable and a single-purpose\nhelper function I have no real preference.  With either, we can\nreword the message easily without disrupting any improvements to the\ncodeflow.  At least the first sentence in the existing message needs\nto be separated out of the advice and turned into a separate error\nor a warning.\n\n"},{"id":"412223","messageId":"xmqqmtyg9dq0.fsf@gitster.c.googlers.com","threadId":"54823","inReplyTo":"5fd7d82a9b6ab_d44af208bb@natae.notmuch","subject":"Re: [PATCH v7 5/5] pull: display default warning only when non-ff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-14T23:20:39Z","receivedAt":"2020-12-14T23:21:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>> @@ -1044,7 +1045,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 (rebase_unspecified && !opt_ff) {\n>> +\tcan_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);\n>> +\n>> +\tif (rebase_unspecified && !opt_ff && !can_ff) {\n>>  \t\tif (opt_verbosity >= 0)\n>>  \t\t\tshow_advice_pull_non_ff();\n>>  \t}\n>\n> I strongly predict the conditionals will end up looking similar to:\n>\n>   if (!can_ff) {\n>           if (rebase_unspecified && !opt_ff && opt_verbosity >= 0)\n>                   show_advice_pull_non_ff();\n>   }\n>\n> But OK.\n\nI may have failed to mention this in the cover letter, but I think\nthe placement of opt_ff in this part of the logic needs to be\nrethought, so I strongly predict that this part will have to further\nchange ;-)\n"},{"id":"412235","messageId":"5fd825898271c_d7c482086d@natae.notmuch","threadId":"54823","inReplyTo":"xmqqv9d49dxk.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v7 3/5] pull: get rid of unnecessary global variable","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-15T02:55:05Z","receivedAt":"2020-12-15T02:55:50Z","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 <felipe.contreras@gmail.com> writes:\n> > Junio C Hamano wrote:\n> >> It is easy enough to do,\n> >\n> > Yes.\n> >\n> >> gives a more descriptive name to the variable,\n> >\n> > I disagree.\n> >\n> >> and there is no reason to make the code deliberately worse by ignoring\n> >> improvement offered on the list.\n> >\n> > I doubt any person contributing to the mailing is making the code\n> > deliberately worse.\n> \n> Oh, I doubt that you do so with what you send out.  I am saying that\n> you do so by not taking improvements.\n\nI do take improvements, when I agree they are improvements. In fact I\ndid take virtually all of Elijah Newren's improvements.\n\n> It wastes reviewers' time, raises noise ratio in the list traffic, and\n> demotivates readers.\n\nAre you saying I must always agree with you, or I waste your time?\n\nIn my view no one is infallible, just because X person says Y is an\nimprovement that doesn't necessarily mean it actually is.\n\nI thought this was a collaborative process where you are supposed to\nlisten to my feedback to your suggestions too.\n\nBut I guess I shall take your \"suggestions\" as *orders*.\n\n-- \nFelipe Contreras\n"},{"id":"412237","messageId":"5fd8260432a08_d7c482083c@natae.notmuch","threadId":"54823","inReplyTo":"xmqqmtyg9dq0.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v7 5/5] pull: display default warning only when non-ff","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-15T02:57:08Z","receivedAt":"2020-12-15T02:58:25Z","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 <felipe.contreras@gmail.com> writes:\n> > Junio C Hamano wrote:\n\n> > I strongly predict the conditionals will end up looking similar to:\n> >\n> >   if (!can_ff) {\n> >           if (rebase_unspecified && !opt_ff && opt_verbosity >= 0)\n> >                   show_advice_pull_non_ff();\n> >   }\n> >\n> > But OK.\n> \n> I may have failed to mention this in the cover letter, but I think\n> the placement of opt_ff in this part of the logic needs to be\n> rethought, so I strongly predict that this part will have to further\n> change ;-)\n\nAnd precisely because I predicted the same is that I deliberately chose\nnot to preemptively organize the conditionals in any particular way.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"412243","messageId":"5fd85811c3a6_d7c48208aa@natae.notmuch","threadId":"54823","inReplyTo":"20201214202647.3340193-1-gitster@pobox.com","subject":"RE: [PATCH v7 0/5] making pull advice not to trigger when unneeded","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-15T06:30:41Z","receivedAt":"2020-12-15T06:32:11Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Just to show what was pushed out on 'seen' while I was cutting the\n> preview release...\n> \n> This is based on Felipe's v6 (which was mislabled as v5 but sent on\n> a different day from the true v5), with two clean-up commits\n> inserted between Felipe's second step (now 2/5) and the third step\n> (now 5/5, with necessary adjustments).\n> \n> Even with the \"correct condition\" clean-up, I am not quite happy\n> with the clarity of the logic around there.\n> \n> I think !opt_ff does not exactly belong to the condition, if we\n> consider that the eventual endgame should be to stop non-ff\n> operation without merge or rebase by default with an error, and that\n> should happen in this block.  The error message there should say\n> \"unable to fast-forward; must merge or rebase\", which should equally\n> apply to those who gave \"--ff-only\" explicitly, even though they may\n> not need the advice message.\n\nAgreed. Additionally it doesn't make sense that --ff skips the warning\nentirely, even though the user has not specified a merge or a rebase (as\nyou yourself noted in a review of the test cases).\n\n> To avoid the ugly-looking \"strcmp()\" in the above, we may need to\n> adjust \"--ff\" (fast-forward without creating an extra merge commit)\n> and \"--no-ff\" (create an extra merge commit when the history could\n> be fast-forwarded) to imply \"merge\", though.  It would automatically\n> make rebase_unspecified would become false.  With such a tweak, we\n> can then simplify it further to\n> \n> -\tif (rebase_unspecified && !can_ff &&\n> -\t    (!opt_ff || !strcmp(\"--ff-only\", opt_ff))) {\n> +\tif (rebase_unspecified && !can_ff) {\n\nCurrently this is a rebase:\n\n  git pull --rebase --ff\n\nWith your proposed change it would be an implied merge. I think it makes\nsense, but it's still a change in behavior.\n\nThis patch should do the trick:\n\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -973,6 +973,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n        if (opt_rebase < 0)\n                opt_rebase = config_get_rebase(&rebase_unspecified);\n \n+       if (opt_ff && (!strcmp(opt_ff, \"--ff\") || !strcmp(opt_ff, \"--no-ff\"))) {\n+               opt_rebase = 0;\n+               rebase_unspecified = 0;\n+       }\n+\n        if (read_cache_unmerged())\n                die_resolve_conflict(\"pull\");\n \nOr do you mean --ff doesn't override --rebase? Therefore it's more of an\ninternal conceptual change.\n\n-- \nFelipe Contreras\n"},{"id":"412244","messageId":"5fd859149990b_d7c48208d8@natae.notmuch","threadId":"54823","inReplyTo":"xmqqr1ns9drv.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v7 4/5] pull: correct condition to trigger non-ff advice","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-15T06:35:00Z","receivedAt":"2020-12-15T06:36:10Z","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 <felipe.contreras@gmail.com> writes:\n> \n> > Then, we could just do:\n> >\n> >   if (opt_verbosity >= 0)\n> >           advise(_(message_advice_pull_non_ff)).\n> >\n> > Or even better:\n> >\n> >   if (opt_verbosity >= 0)\n> >           advise_if_enabled(ADVICE_PULL_NON_FF, _(message_advice_pull_non_ff));\n> \n> I do not think we've decided what's the right way to squelch this\n> advice, so it is a bit premature to favor the latter over the\n> former.\n\nI agree. Just thinking about the future.\n\n> Between a fixed message[] variable and a single-purpose\n> helper function I have no real preference.\n\nOK. I just mentioned for consistencty with other parts of git.\n\n-- \nFelipe Contreras\n"},{"id":"412251","messageId":"xmqqmtyf8hfm.fsf@gitster.c.googlers.com","threadId":"54823","inReplyTo":"5fd85811c3a6_d7c48208aa@natae.notmuch","subject":"Re: [PATCH v7 0/5] making pull advice not to trigger when unneeded","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-15T10:58:05Z","receivedAt":"2020-12-15T10:59:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n>> To avoid the ugly-looking \"strcmp()\" in the above, we may need to\n>> adjust \"--ff\" (fast-forward without creating an extra merge commit)\n>> and \"--no-ff\" (create an extra merge commit when the history could\n>> be fast-forwarded) to imply \"merge\", though.\n>> ...\n>\n> Or do you mean --ff doesn't override --rebase? Therefore it's more of an\n> internal conceptual change.\n\nI meant \"--ff/--no-ff\" without saying anything between merge and\nrebase would make merge implied.  It was merely for the purpose of\ngetting rid of !opt_ff in the condition there and such an implied\nmerge would be one way to ensure that rebase_unspecified becomes\nfalse.\n\n\"--no-ff --rebase\" (in any order) would be a nonsense combination,\nas it asks \"please create an extra merge commit even when the\nhistory fast-forwards, but by the way I do not want merge I want\nrebase\" [*1*].  It should error out when the history fast-forwards,\nI think, and it probably should also error out when the history does\nnot fast-forward, instead of rebasing.\n\n\"--ff --rebase\" (in any order), on the other hand, would be \"if the\nhistory fast-forwards, just do so without creating an extra merge\ncommit, by the way, I prefer rebase and not merge\".  If the history\nfast-forwards, it is OK to just fast-forward (using the merge\nbackend if needed), as rebasing against the history that is ahead of\nus would mean all our unique development since we diverged from them,\nwhich is nothing, will be replayed on top of their history.  If the\nhistory does not fast-forward, then the precondition of \"--ff\" part\ndoes not hold, so just rebasing as if \"--ff\" does not exist would\nprobably be OK, too.\n\nIn any case, I haven't thought the opt_ff part of the expression\nthrough.  During the review of v5 3/3 (and doing v7 5/5), what I was\nmore interested in was to ensure the verbosity option, which would\ncontrol how verbose a message we should be giving, can stay\nindependent of other conditionals, which would control if we even\nneed to error out and/or give hints.\n\nThanks.\n\n\n[Footnote]\n\n*1* Even though the current code may simply ignore --no-ff after the\ncontrol is given to the rebase codepath,\n"},{"id":"412255","messageId":"5fd8aa6a52e81_190cd7208c8@natae.notmuch","threadId":"54823","inReplyTo":"xmqqmtyf8hfm.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v7 0/5] making pull advice not to trigger when unneeded","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-15T12:22:02Z","receivedAt":"2020-12-15T12:23:03Z","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 <felipe.contreras@gmail.com> writes:\n> \n> >> To avoid the ugly-looking \"strcmp()\" in the above, we may need to\n> >> adjust \"--ff\" (fast-forward without creating an extra merge commit)\n> >> and \"--no-ff\" (create an extra merge commit when the history could\n> >> be fast-forwarded) to imply \"merge\", though.\n> >> ...\n> >\n> > Or do you mean --ff doesn't override --rebase? Therefore it's more of an\n> > internal conceptual change.\n> \n> I meant \"--ff/--no-ff\" without saying anything between merge and\n> rebase would make merge implied.  It was merely for the purpose of\n> getting rid of !opt_ff in the condition there and such an implied\n> merge would be one way to ensure that rebase_unspecified becomes\n> false.\n\nI see.\n\n> \"--no-ff --rebase\" (in any order) would be a nonsense combination,\n> as it asks \"please create an extra merge commit even when the\n> history fast-forwards, but by the way I do not want merge I want\n> rebase\" [*1*].  It should error out when the history fast-forwards,\n> I think, and it probably should also error out when the history does\n> not fast-forward, instead of rebasing.\n\nBut we shoud not imply what the user didn't say.\n\nYes, \"--no-ff --rebase\" is obviously nonsense, but that's a\nsimplification of setups the user may have, for example:\n\n  git config pull.ff false\n  git pull --rebase\n\nHere, I think the user is saying: \"please do a rebase, and ignore the\npull.ff configuration\".\n\nBut the other way would be:\n\n  git config pull.rebase true\n  git pull --no-ff\n\nFollowing the same logic, the user is saying: \"please do a\nnon-fast-forward [merge], and ignore the pull.rebase configuration\".\n\nEither we imply the merge, or we don't.\n\nI don't think it makes sense for the code to imply the user did say\n--merge, and therefore don't show the advice (or in the future error\nout), but then continue as if the user did say --rebase.\n\n> In any case, I haven't thought the opt_ff part of the expression\n> through.\n\nIt is tricky.\n\nThe reason I think I see it more clearly is that I have explored many\noptions, and at this point I have 27 branches with different approaches\nof this topic. I see the dead-ends because I literally have a branch\nwith the test-case failure on it.\n\nBut as I said in the other thread [1], it all boils down to this:\n\n 1. git -c pull.ff=only pull\n 2. git -c pull.ff=only pull --merge\n\nEven if you remove the opt_ff part of the expression, and the logic of\nthe advice/error is flawless, opt_ff is still going to come bite us in\nthe end, because it's meant to be passed to cmd_merge(), and it's still\ngoing to fail, just at a different point, with a different error message.\n\nThe best way to get rid of opt_ff from the expression, is to actually\ncompletely ignore it, and leave the current behavior as it is.\n\nBy adding a different configuration, the logic becomes really simple:\n\n  if (!can_ff) {\n\t  if (!mode && opt_verbosity >= 0)\n\t\t  show_advice_pull_non_ff();\n  }\n\nAnd then opt_ff doesn't interact with --rebase at all, just like it is\nthe case right now.\n\nSimple.\n\nCheers.\n\n[1] https://lore.kernel.org/git/5fd8213598c33_d7c4820837@natae.notmuch\n\n-- \nFelipe Contreras\n"},{"id":"412582","messageId":"5fdd154264baf_130e182082b@natae.notmuch","threadId":"54823","inReplyTo":"5fd8aa6a52e81_190cd7208c8@natae.notmuch","subject":"Re: [PATCH v7 0/5] making pull advice not to trigger when unneeded","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-18T20:46:58Z","receivedAt":"2020-12-18T20:47:58Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Felipe Contreras wrote:\n> Junio C Hamano wrote:\n\n> > \"--no-ff --rebase\" (in any order) would be a nonsense combination,\n> > as it asks \"please create an extra merge commit even when the\n> > history fast-forwards, but by the way I do not want merge I want\n> > rebase\" [*1*].  It should error out when the history fast-forwards,\n> > I think, and it probably should also error out when the history does\n> > not fast-forward, instead of rebasing.\n> \n> But we shoud not imply what the user didn't say.\n> \n> Yes, \"--no-ff --rebase\" is obviously nonsense, but that's a\n> simplification of setups the user may have, for example:\n> \n>   git config pull.ff false\n>   git pull --rebase\n> \n> Here, I think the user is saying: \"please do a rebase, and ignore the\n> pull.ff configuration\".\n> \n> But the other way would be:\n> \n>   git config pull.rebase true\n>   git pull --no-ff\n> \n> Following the same logic, the user is saying: \"please do a\n> non-fast-forward [merge], and ignore the pull.rebase configuration\".\n> \n> Either we imply the merge, or we don't.\n> \n> I don't think it makes sense for the code to imply the user did say\n> --merge, and therefore don't show the advice (or in the future error\n> out), but then continue as if the user did say --rebase.\n\nI didn't see a response to this, but after thinking more about, I think\nI have a clean solution: just remove all the opt_ff logic altogether.\n\nIt's clear --ff doesn't imply a merge, so we shouldn't act as if it was.\n\nThe warning should still be displayed.\n\n-- \nFelipe Contreras\n"},{"id":"412915","messageId":"xmqqsg7waleg.fsf@gitster.c.googlers.com","threadId":"54823","inReplyTo":"5fdd154264baf_130e182082b@natae.notmuch","subject":"Re: [PATCH v7 0/5] making pull advice not to trigger when unneeded","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-23T10:04:23Z","receivedAt":"2020-12-23T10:05:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> It's clear --ff doesn't imply a merge, so we shouldn't act as if it was.\n\nDo you specifically mean --ff, or do you talk collectively about\nanything that goes in opt_ff in the C code?\n\nThe \"--ff\" option means \"we are allowing fast-forward, so please do\nnot make new commit object unnecessarily, but it is just we are\nallowing---we are not limiting ourselves to fast-forard; feel free\nto create a merge commit if necessary\".  So it does imply that the\nuser prefers to merge and does not want to rebase.\n\nIf you meant what opt_ff can relay, then there are \"--no-ff\" and\n\"--ff-only\" to consider:\n\n - \"--no-ff\" says \"we do not allow fast-forward; when the other side\n   is pure descendant of ours, create a merge commit to make them\n   the second parent, so that our side of the history stays to be\n   the first-parent chain that merged them as a side topic.\"  It may\n   not say what should happen when the history does not\n   fast-forward, and it _is_ possible to argue, for the sake of\n   argument, that it asks to rebase if not fast-forward (so that\n   their history becomes the primary and we build on top of them)\n   while asking to merge if fast-forward (so that our history stays\n   the primary and we absorb their work as a side branch), but that\n   is a behavior that does not make much sense.  It is much easier\n   to reason about if we accept that the user who says \"--no-ff\"\n   expects a merge to happen, not a rebase.\n\n - \"--ff-only\" says \"when their history is pure descendant of ours,\n   just fast-forward our branch to match their history, and\n   otherwise fail.\"  This one does not have to imply either merge or\n   rebase, as both would give us identical result (i.e. merge would\n   fast-forward and rebase would replay *no* work of our own on top\n   of theirs.  Either case, the result is that our branch tip now\n   points at the tip of their history).\n\n   The topic under discussion is based on the \"we do not have to\n   give advice between merge and rebase if the history\n   fast-forwards\", and anybody in support of the topic would be in\n   agreement with this case.\n\nIn any case, I think what we have in 'seen' already is a good\nstopping point for this cycle.  We are not erroring out any new case\nand simply not showing an advice in a situation that it would not\napply---the question \"does --ff imply merge?\" does not have to be\nanswered in order to evaluate the 5-patch series we have.\n"},{"id":"412924","messageId":"5fe34fc13163_198be208bd@natae.notmuch","threadId":"54823","inReplyTo":"xmqqsg7waleg.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v7 0/5] making pull advice not to trigger when unneeded","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-23T14:10:09Z","receivedAt":"2020-12-23T14:11:04Z","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 <felipe.contreras@gmail.com> writes:\n> \n> > It's clear --ff doesn't imply a merge, so we shouldn't act as if it was.\n> \n> Do you specifically mean --ff, or do you talk collectively about\n> anything that goes in opt_ff in the C code?\n\nI meant --ff, but the rationale can be extended to all of opt_ff.\n\n> The \"--ff\" option means \"we are allowing fast-forward, so please do\n> not make new commit object unnecessarily, but it is just we are\n> allowing---we are not limiting ourselves to fast-forard; feel free\n> to create a merge commit if necessary\".\n\nYes. *If* a rebase is not specified.\n\n> So it does imply that the user prefers to merge and does not want to\n> rebase.\n\nWe could imply that, but currently it doesn't.\n\nCurrently this does not do a merge:\n\n  git config pull.rebase true\n  git pull --ff\n\n> If you meant what opt_ff can relay, then there are \"--no-ff\" and\n> \"--ff-only\" to consider:\n> \n>  - \"--no-ff\" says \"we do not allow fast-forward; when the other side\n>    is pure descendant of ours, create a merge commit to make them\n>    the second parent, so that our side of the history stays to be\n>    the first-parent chain that merged them as a side topic.\"  It may\n>    not say what should happen when the history does not\n>    fast-forward, and it _is_ possible to argue, for the sake of\n>    argument, that it asks to rebase if not fast-forward (so that\n>    their history becomes the primary and we build on top of them)\n>    while asking to merge if fast-forward (so that our history stays\n>    the primary and we absorb their work as a side branch), but that\n>    is a behavior that does not make much sense.\n\nI agree it doesn't make much sense; if the user wants a rebase in case\nof non-fast-forward, --no-ff is the only way.\n\n>    It is much easier to reason about if we accept that the user who\n>    says \"--no-ff\" expects a merge to happen, not a rebase.\n\nYes, but currently that's not the case.\n\nCurrently this doesn't do a merge:\n\n  git config pull.rebase true\n  git pull --no-ff\n\nWe would need to change the semantics.\n\n>  - \"--ff-only\" says \"when their history is pure descendant of ours,\n>    just fast-forward our branch to match their history, and\n>    otherwise fail.\"  This one does not have to imply either merge or\n>    rebase, as both would give us identical result (i.e. merge would\n>    fast-forward and rebase would replay *no* work of our own on top\n>    of theirs.  Either case, the result is that our branch tip now\n>    points at the tip of their history).\n> \n>    The topic under discussion is based on the \"we do not have to\n>    give advice between merge and rebase if the history\n>    fast-forwards\", and anybody in support of the topic would be in\n>    agreement with this case.\n\nYes.\n\n> In any case, I think what we have in 'seen' already is a good\n> stopping point for this cycle.\n\nIt's not a bad stopping point.\n\nBut the next patches are needed too. Up to the first 6 patches should be\nuncontroversial.\n\n> We are not erroring out any new case and simply not showing an advice\n> in a situation that it would not apply---the question \"does --ff imply\n> merge?\" does not have to be answered in order to evaluate the 5-patch\n> series we have.\n\nNot my patches.\n\nThe patch you introduced regarding rebase_unspecified does depend on\nwhat happens next. If we decide to change the semantics of --ff* and\nimply a merge, then my patch to add REBASE_DEFAULT is needed, and as you\ncan see in another patch series [1], that basically has to revert your patch.\n\nCheers.\n\n[1] https://lore.kernel.org/git/20201218211026.1937168-8-felipe.contreras@gmail.com/\n\n-- \nFelipe Contreras\n"}]}