{"thread":{"id":"54813","subject":"[PATCH v5 0/3] pull: stop warning on every pull","startedAt":"2020-12-12T16:53:09Z","lastAt":"2020-12-12T16:57:07Z","messageCount":5,"participants":["Felipe Contreras"],"isPatch":true,"patchVersion":5,"patchTotal":3},"messages":[{"id":"412118","messageId":"20201212165208.780798-1-felipe.contreras@gmail.com","threadId":"54813","inReplyTo":null,"subject":"[PATCH v5 0/3] pull: stop warning on every pull","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-12T16:52:05Z","receivedAt":"2020-12-12T16:53:09Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"The future of having a sensible \"git pull\" default is uncertain (it's\nvery likely that nothing will change), in the meantime there's no reason\nto keep annoying our users unnecessarily.\n\nThis patch is doing one thing, and one thing only: stops warning the\nusers on every single pull (warns only if they are not-fast-forward).\n\nChanges since v4:\n\n 1. Improved some commit messages\n 2. In patch 3 current the tests are kept as is, per Junio's request\n    (IMO they are no-ops)\n\ndiff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\nindex 6b4adab8b1..52e8ccc933 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -28,66 +28,116 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'pull.rebase not set' '\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+\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 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 (fast-forward)' '\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=true' '\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+\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+\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+\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+\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+\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' '\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' '\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' '\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' '\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' '\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' '\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' '\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\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 | 66 +++++++++++++++++++++++++++++++++---\n 2 files changed, 97 insertions(+), 30 deletions(-)\n\n-- \n2.29.2\n\n"},{"id":"412119","messageId":"20201212165208.780798-2-felipe.contreras@gmail.com","threadId":"54813","inReplyTo":"20201212165208.780798-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-12T16:52:06Z","receivedAt":"2020-12-12T16:53:28Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"It makes the code much cleaner.\n\nPlus, we would like to be able to make this check before the decision to\nrebase is made in a future step.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.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.29.2\n\n"},{"id":"412120","messageId":"20201212165208.780798-3-felipe.contreras@gmail.com","threadId":"54813","inReplyTo":"20201212165208.780798-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-12T16:52:07Z","receivedAt":"2020-12-12T16:53:28Z","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). But this variable\ncan be removed later on with further code reorganization.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.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.29.2\n\n"},{"id":"412121","messageId":"20201212165208.780798-4-felipe.contreras@gmail.com","threadId":"54813","inReplyTo":"20201212165208.780798-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-12T16:52:08Z","receivedAt":"2020-12-12T16:53:28Z","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\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>\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 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..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.29.2\n\n"},{"id":"412122","messageId":"CAMP44s0WPpGzZHU2tQS4OJwuC2P_zXW+ZpO4rB4w2hEgVUcp+A@mail.gmail.com","threadId":"54813","inReplyTo":"20201212165208.780798-1-felipe.contreras@gmail.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:56:13Z","receivedAt":"2020-12-12T16:57:07Z","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:52 AM Felipe Contreras\n<felipe.contreras@gmail.com> wrote:\n>\n> The future of having a sensible \"git pull\" default is uncertain (it's\n> very likely that nothing will change), in the meantime there's no reason\n> to keep annoying our users unnecessarily.\n>\n> This patch is doing one thing, and one thing only: stops warning the\n> users on every single pull (warns only if they are not-fast-forward).\n>\n> Changes since v4:\n\nHmm, it's not 4 -> 5, It's 5 -> 6.\n\n-- \nFelipe Contreras\n"}]}