{"thread":{"id":"54705","subject":"[PATCH 0/6] pull: improve default warning","startedAt":"2020-11-24T13:31:03Z","lastAt":"2020-11-24T13:31:05Z","messageCount":7,"participants":["Felipe Contreras"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"410673","messageId":"20201124133037.89949-1-felipe.contreras@gmail.com","threadId":"54705","inReplyTo":null,"subject":"[PATCH 0/6] pull: improve default warning","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-11-24T13:30:31Z","receivedAt":"2020-11-24T13:31:03Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This patch series attempts to silence a bit more the default pull warning (when no pull.rebase is\nconfigured, or pull.ff).\n\nBasically; display the warning only when a fast-forward merge is not possible.\n\nMore work is needed to add a ff-only option.\n\n\nFelipe Contreras (6):\n pull: refactor fast-forward check\n pull: cleanup autostash check\n pull: trivial cleanup\n pull: move default warning\n pull: display default warning only when non-ff\n test: pull-options: revert unnecessary changes\n\n builtin/pull.c | 79 ++++++++++++++++++++----------------\n t/t5521-pull-options.sh | 22 +++++-----\n t/t7601-merge-pull-config.sh | 8 +++-\n 3 files changed, 61 insertions(+), 48 deletions(-)\n\n-- \n2.29.2\n\n"},{"id":"410674","messageId":"20201124133037.89949-2-felipe.contreras@gmail.com","threadId":"54705","inReplyTo":"20201124133037.89949-1-felipe.contreras@gmail.com","subject":"[PATCH 1/6] pull: refactor fast-forward check","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-11-24T13:30:32Z","receivedAt":"2020-11-24T13:31:03Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This way we will be able to do the check unconditionally (merge or\nrebase).\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 17aa63cd35..e2de0d4c91 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -906,6 +906,20 @@ static int run_rebase(const struct object_id *curr_head,\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@@ -1016,22 +1030,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\t submodule_touches_in_range(the_repository, &rebase_fork_point, &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(&curr_head, merge_heads.oid, &rebase_fork_point);\n-- \n2.29.2\n\n"},{"id":"410675","messageId":"20201124133037.89949-3-felipe.contreras@gmail.com","threadId":"54705","inReplyTo":"20201124133037.89949-1-felipe.contreras@gmail.com","subject":"[PATCH 2/6] pull: cleanup autostash check","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-11-24T13:30:33Z","receivedAt":"2020-11-24T13:31:03Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This essentially reverts commit f15e7cf5cc.\n\nOnce commit d9f15d37f1 introduced the autostash option for the merge\nmode, it's no necessary to skip the fast-forward run_merge() when\nautostash is set.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/pull.c | 15 ++++++---------\n 1 file changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex e2de0d4c91..eaa268c559 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -926,7 +926,6 @@ 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 \n \tif (!getenv(\"GIT_REFLOG_ACTION\"))\n \t\tset_reflog_message(argc, argv);\n@@ -959,8 +958,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-\tautostash = config_autostash;\n \tif (opt_rebase) {\n+\t\tint autostash = config_autostash;\n \t\tif (opt_autostash != -1)\n \t\t\tautostash = opt_autostash;\n \n@@ -1029,13 +1028,11 @@ 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, &rebase_fork_point, &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\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\tif (get_can_ff(&orig_head, &merge_heads.oid[0])) {\n+\t\t\t/* we can fast-forward this without invoking rebase */\n+\t\t\topt_ff = \"--ff-only\";\n+\t\t\tran_ff = 1;\n+\t\t\tret = run_merge();\n \t\t}\n \t\tif (!ran_ff)\n \t\t\tret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);\n-- \n2.29.2\n\n"},{"id":"410676","messageId":"20201124133037.89949-4-felipe.contreras@gmail.com","threadId":"54705","inReplyTo":"20201124133037.89949-1-felipe.contreras@gmail.com","subject":"[PATCH 3/6] pull: trivial cleanup","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-11-24T13:30:34Z","receivedAt":"2020-11-24T13:31:03Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"There's no need to store ran_ff. Now it's obvious from the conditionals.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/pull.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex eaa268c559..121b9fb0e1 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -1023,19 +1023,18 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \n \tif (opt_rebase) {\n \t\tint ret = 0;\n-\t\tint ran_ff = 0;\n \t\tif ((recurse_submodules == RECURSE_SUBMODULES_ON ||\n \t\t recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&\n \t\t submodule_touches_in_range(the_repository, &rebase_fork_point, &curr_head))\n \t\t\tdie(_(\"cannot rebase with locally recorded submodule modifications\"));\n+\n \t\tif (get_can_ff(&orig_head, &merge_heads.oid[0])) {\n \t\t\t/* we can fast-forward this without invoking rebase */\n \t\t\topt_ff = \"--ff-only\";\n-\t\t\tran_ff = 1;\n \t\t\tret = run_merge();\n-\t\t}\n-\t\tif (!ran_ff)\n+\t\t} else {\n \t\t\tret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);\n+\t\t}\n \n \t\tif (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||\n \t\t\t recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))\n-- \n2.29.2\n\n"},{"id":"410677","messageId":"20201124133037.89949-5-felipe.contreras@gmail.com","threadId":"54705","inReplyTo":"20201124133037.89949-1-felipe.contreras@gmail.com","subject":"[PATCH 4/6] pull: move default warning","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-11-24T13:30:35Z","receivedAt":"2020-11-24T13:31:04Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Up to the point where can check if we can fast-forward or not.\n\nNo functional changes.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/pull.c | 37 ++++++++++++++++++++++---------------\n 1 file changed, 22 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 121b9fb0e1..9a0f7937c2 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\twarning(_(\"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@@ -926,6 +915,7 @@ 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 can_ff;\n \n \tif (!getenv(\"GIT_REFLOG_ACTION\"))\n \t\tset_reflog_message(argc, argv);\n@@ -1021,6 +1011,23 @@ 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+\tcan_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);\n+\n+\tif (default_mode && opt_verbosity >= 0 && !opt_ff) {\n+\t\twarning(_(\"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\tif ((recurse_submodules == RECURSE_SUBMODULES_ON ||\n@@ -1028,7 +1035,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\t submodule_touches_in_range(the_repository, &rebase_fork_point, &curr_head))\n \t\t\tdie(_(\"cannot rebase with locally recorded submodule modifications\"));\n \n-\t\tif (get_can_ff(&orig_head, &merge_heads.oid[0])) {\n+\t\tif (can_ff) {\n \t\t\t/* we can fast-forward this without invoking rebase */\n \t\t\topt_ff = \"--ff-only\";\n \t\t\tret = run_merge();\n-- \n2.29.2\n\n"},{"id":"410678","messageId":"20201124133037.89949-6-felipe.contreras@gmail.com","threadId":"54705","inReplyTo":"20201124133037.89949-1-felipe.contreras@gmail.com","subject":"[PATCH 5/6] pull: display default warning only when non-ff","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-11-24T13:30:36Z","receivedAt":"2020-11-24T13:31:04Z","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\nSigned-off-by: Felipe Contreras \n---\n builtin/pull.c | 2 +-\n t/t7601-merge-pull-config.sh | 8 +++++++-\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 9a0f7937c2..479bdaf0ee 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -1013,7 +1013,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \n \tcan_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);\n \n-\tif (default_mode && opt_verbosity >= 0 && !opt_ff) {\n+\tif (default_mode && !can_ff && opt_verbosity >= 0 && !opt_ff) {\n \t\twarning(_(\"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\"\ndiff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\nindex c5c4ea5fc0..d79723877b 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -28,11 +28,17 @@ 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 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+\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-- \n2.29.2\n\n"},{"id":"410679","messageId":"20201124133037.89949-7-felipe.contreras@gmail.com","threadId":"54705","inReplyTo":"20201124133037.89949-1-felipe.contreras@gmail.com","subject":"[PATCH 6/6] test: pull-options: revert unnecessary changes","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-11-24T13:30:37Z","receivedAt":"2020-11-24T13:31:05Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Commit d18c950a69 changed these tests, but it's unclear why. Probably\nbecause earlier versions of the patch series died instead of printing a\nwarning.\n\nCc: Alex Henrie \nSigned-off-by: Felipe Contreras \n---\n t/t5521-pull-options.sh | 22 +++++++++++-----------\n 1 file changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex db1a381cd9..1a4fe2583a 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 --no-rebase' '\n+test_expect_success 'git pull -q' '\n \tmkdir clonedq &&\n \t(cd clonedq && git init &&\n-\tgit pull -q --no-rebase \"../parent\" >out 2>err &&\n+\tgit pull -q \"../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 --no-rebase' '\n+test_expect_success 'git pull' '\n \tmkdir cloned &&\n \t(cd cloned && git init &&\n-\tgit pull --no-rebase \"../parent\" >out 2>err &&\n+\tgit pull \"../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 --no-rebase' '\n+test_expect_success 'git pull -v' '\n \tmkdir clonedv &&\n \t(cd clonedv && git init &&\n-\tgit pull -v --no-rebase \"../parent\" >out 2>err &&\n+\tgit pull -v \"../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 --no-rebase' '\n+test_expect_success 'git pull -v -q' '\n \tmkdir clonedvq &&\n \t(cd clonedvq && git init &&\n-\tgit pull -v -q --no-rebase \"../parent\" >out 2>err &&\n+\tgit pull -v -q \"../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 --no-rebase' '\n+test_expect_success 'git pull -q -v' '\n \tmkdir clonedqv &&\n \t(cd clonedqv && git init &&\n-\tgit pull -q -v --no-rebase \"../parent\" >out 2>err &&\n+\tgit pull -q -v \"../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 --no-rebase --cleanup invalid \"../parent\" >out 2>err &&\n+\ttest_must_fail git pull --cleanup invalid \"../parent\" >out 2>err &&\n \ttest_must_be_empty out &&\n \ttest -s err)\n '\n-- \n2.29.2\n\n"}]}