{"thread":{"id":"52961","subject":"[PATCH v5] pull: warn if the user didn't say whether to rebase or to merge","startedAt":"2020-03-10T03:54:44Z","lastAt":"2020-03-10T20:16:09Z","messageCount":2,"participants":["Alex Henrie","Junio C Hamano"],"isPatch":true,"patchVersion":5,"patchTotal":null},"messages":[{"id":"392982","messageId":"20200310035420.1658876-1-alexhenrie24@gmail.com","threadId":"52961","inReplyTo":null,"subject":"[PATCH v5] pull: warn if the user didn't say whether to rebase or to merge","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2020-03-10T03:54:20Z","receivedAt":"2020-03-10T03:54:44Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"Often novice Git users forget to say \"pull --rebase\" and end up with an\nunnecessary merge from upstream. What they usually want is either \"pull\n--rebase\" in the simpler cases, or \"pull --ff-only\" to update the copy\nof main integration branches, and rebase their work separately. The\npull.rebase configuration variable exists to help them in the simpler\ncases, but there is no mechanism to make these users aware of it.\n\nIssue a warning message when no --[no-]rebase option from the command\nline and no pull.rebase configuration variable is given. This will\ninconvenience those who never want to \"pull --rebase\", who haven't had\nto do anything special, but the cost of the inconvenience is paid only\nonce per user, which should be a reasonable cost to help a number of new\nusers.\n\nSigned-off-by: Alex Henrie <alexhenrie24@gmail.com>\n---\nv5:\n- Supress warning when --quiet is given\n- Ensure that pull option tests still test what they're supposed to test\n- Add tests for the new warning\n---\n builtin/pull.c               | 16 +++++++++++++++\n t/t5521-pull-options.sh      | 22 ++++++++++-----------\n t/t7601-merge-pull-config.sh | 38 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 65 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 3e624d1e00..e42665b681 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -327,6 +327,22 @@ 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 &&\n+\t    (!opt_ff || strcmp(opt_ff, \"--ff-only\"))) {\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 \treturn REBASE_FALSE;\n }\n \ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex ccde8ba491..159afa7ac8 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -11,10 +11,10 @@ test_expect_success 'setup' '\n \t git commit -m one)\n '\n \n-test_expect_success 'git pull -q' '\n+test_expect_success 'git pull -q --no-rebase' '\n \tmkdir clonedq &&\n \t(cd clonedq && git init &&\n-\tgit pull -q \"../parent\" >out 2>err &&\n+\tgit pull -q --no-rebase \"../parent\" >out 2>err &&\n \ttest_must_be_empty err &&\n \ttest_must_be_empty out)\n '\n@@ -30,10 +30,10 @@ test_expect_success 'git pull -q --rebase' '\n \ttest_must_be_empty out)\n '\n \n-test_expect_success 'git pull' '\n+test_expect_success 'git pull --no-rebase' '\n \tmkdir cloned &&\n \t(cd cloned && git init &&\n-\tgit pull \"../parent\" >out 2>err &&\n+\tgit pull --no-rebase \"../parent\" >out 2>err &&\n \ttest -s err &&\n \ttest_must_be_empty out)\n '\n@@ -46,10 +46,10 @@ test_expect_success 'git pull --rebase' '\n \ttest_must_be_empty out)\n '\n \n-test_expect_success 'git pull -v' '\n+test_expect_success 'git pull -v --no-rebase' '\n \tmkdir clonedv &&\n \t(cd clonedv && git init &&\n-\tgit pull -v \"../parent\" >out 2>err &&\n+\tgit pull -v --no-rebase \"../parent\" >out 2>err &&\n \ttest -s err &&\n \ttest_must_be_empty out)\n '\n@@ -62,25 +62,25 @@ test_expect_success 'git pull -v --rebase' '\n \ttest_must_be_empty out)\n '\n \n-test_expect_success 'git pull -v -q' '\n+test_expect_success 'git pull -v -q --no-rebase' '\n \tmkdir clonedvq &&\n \t(cd clonedvq && git init &&\n-\tgit pull -v -q \"../parent\" >out 2>err &&\n+\tgit pull -v -q --no-rebase \"../parent\" >out 2>err &&\n \ttest_must_be_empty out &&\n \ttest_must_be_empty err)\n '\n \n-test_expect_success 'git pull -q -v' '\n+test_expect_success 'git pull -q -v --no-rebase' '\n \tmkdir clonedqv &&\n \t(cd clonedqv && git init &&\n-\tgit pull -q -v \"../parent\" >out 2>err &&\n+\tgit pull -q -v --no-rebase \"../parent\" >out 2>err &&\n \ttest_must_be_empty out &&\n \ttest -s err)\n '\n test_expect_success 'git pull --cleanup errors early on invalid argument' '\n \tmkdir clonedcleanup &&\n \t(cd clonedcleanup && git init &&\n-\ttest_must_fail git pull --cleanup invalid \"../parent\" >out 2>err &&\n+\ttest_must_fail git pull --no-rebase --cleanup invalid \"../parent\" >out 2>err &&\n \ttest_must_be_empty out &&\n \ttest -s err)\n '\ndiff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\nindex c6c44ec570..0f97828cd0 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -27,6 +27,44 @@ test_expect_success 'setup' '\n \tgit tag c3\n '\n \n+test_expect_success 'pull.rebase not set' '\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=false' '\n+\tgit reset --hard c0 &&\n+\ttest_config pull.ff false &&\n+\tgit pull . c1 2>err &&\n+\ttest_i18ngrep \"Pulling without specifying how to reconcile\" err\n+'\n+\n+test_expect_success 'pull.rebase not set and pull.ff=only' '\n+\tgit reset --hard c0 &&\n+\ttest_config pull.ff only &&\n+\tgit pull . c1 2>err &&\n+\ttest_i18ngrep ! \"Pulling without specifying how to reconcile\" err\n+'\n+\n+test_expect_success 'pull.rebase not set and --rebase given' '\n+\tgit reset --hard c0 &&\n+\tgit pull --rebase . c1 2>err &&\n+\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-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 'merge c1 with c2' '\n \tgit reset --hard c1 &&\n \ttest -f c0.c &&\n-- \n2.25.1\n\n"},{"id":"393039","messageId":"xmqqk13shsou.fsf@gitster.c.googlers.com","threadId":"52961","inReplyTo":"20200310035420.1658876-1-alexhenrie24@gmail.com","subject":"Re: [PATCH v5] pull: warn if the user didn't say whether to rebase or to merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-10T20:16:01Z","receivedAt":"2020-03-10T20:16:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie <alexhenrie24@gmail.com> writes:\n\n> Often novice Git users forget to say \"pull --rebase\" and end up with an\n> unnecessary merge from upstream. What they usually want is either \"pull\n> --rebase\" in the simpler cases, or \"pull --ff-only\" to update the copy\n> of main integration branches, and rebase their work separately. The\n> pull.rebase configuration variable exists to help them in the simpler\n> cases, but there is no mechanism to make these users aware of it.\n>\n> Issue a warning message when no --[no-]rebase option from the command\n> line and no pull.rebase configuration variable is given. This will\n> inconvenience those who never want to \"pull --rebase\", who haven't had\n> to do anything special, but the cost of the inconvenience is paid only\n> once per user, which should be a reasonable cost to help a number of new\n> users.\n>\n> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n> ---\n> v5:\n> - Supress warning when --quiet is given\n> - Ensure that pull option tests still test what they're supposed to test\n> - Add tests for the new warning\n\nThanks, will queue.\n"}]}