{"thread":{"id":"52934","subject":"[PATCH v4] pull: warn if the user didn't say whether to rebase or to merge","startedAt":"2020-03-04T02:31:44Z","lastAt":"2020-03-05T18:30:28Z","messageCount":3,"participants":["Alex Henrie","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"392836","messageId":"20200304022931.2469455-1-alexhenrie24@gmail.com","threadId":"52934","inReplyTo":null,"subject":"[PATCH v4] pull: warn if the user didn't say whether to rebase or to merge","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2020-03-04T02:29:31Z","receivedAt":"2020-03-04T02:31: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---\nv4:\n- Revise warning message based on Junio's feedback\n- Consistently wrap warning lines to 75 characters for easy viewing in\nPO files\n- Fix test failures\n---\n builtin/pull.c          | 15 +++++++++++++++\n t/t5521-pull-options.sh |  3 ++-\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 3e624d1e00..351b933c4d 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -327,6 +327,21 @@ 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_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..6e890ec936 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -8,7 +8,8 @@ test_expect_success 'setup' '\n \tmkdir parent &&\n \t(cd parent && git init &&\n \t echo one >file && git add file &&\n-\t git commit -m one)\n+\t git commit -m one) &&\n+\tgit config pull.rebase false\n '\n \n test_expect_success 'git pull -q' '\n-- \n2.25.1\n\n"},{"id":"392866","messageId":"xmqq4kv45995.fsf@gitster-ct.c.googlers.com","threadId":"52934","inReplyTo":"20200304022931.2469455-1-alexhenrie24@gmail.com","subject":"Re: [PATCH v4] 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-04T17:18:30Z","receivedAt":"2020-03-04T17:18:36Z","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> - Revise warning message based on Junio's feedback\n> - Consistently wrap warning lines to 75 characters for easy viewing in\n> PO files\n\nNice to see attention to such a detail ;-)\n\n> - Fix test failures\n\nAh, OK, hmmm.  \n\nFor --quiet test, that wants to ensure that \"pull --quiet\" does not\nsay anything, it certainly stops the test from failing if we set the\nconfiguration before executing such a test, but I wonder if that is\nin line with the spirit of the feature the test tries to protect in\nthe first place.  I would imagine those who write \"pull --quiet\" in\nautomation would not want to see any non-error message, and because\nthis is not an error, they do not want to see any output.  Shouldn't\nsuch a use of \"pull --quiet\" bypass this warning, too?\n\n> ---\n>  builtin/pull.c          | 15 +++++++++++++++\n>  t/t5521-pull-options.sh |  3 ++-\n>  2 files changed, 17 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 3e624d1e00..351b933c4d 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -327,6 +327,21 @@ 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_ff || strcmp(opt_ff, \"--ff-only\")) {\n\nIf we want to squelch the warning under \"--quiet\", I think we can do\nthis.\n\n\tif (0 < opt_verbosity &&\n\t    (!opt_ff || strcmp(opt_ff, \"--ff-only\"))) {\n\nand ...\n\n\n> +\t\twarning(_(\"Pulling without specifying how to reconcile divergent branches is\\n\"\n> ...\n> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\n> index ccde8ba491..6e890ec936 100755\n> --- a/t/t5521-pull-options.sh\n> +++ b/t/t5521-pull-options.sh\n> @@ -8,7 +8,8 @@ test_expect_success 'setup' '\n>  \tmkdir parent &&\n>  \t(cd parent && git init &&\n>  \t echo one >file && git add file &&\n> -\t git commit -m one)\n> +\t git commit -m one) &&\n> +\tgit config pull.rebase false\n\n... this change can safely go.\n\nIf we agree that \"git pull --quiet\" should stay quiet in an\nenvironment where the user has been happy with the default choice,\nthat is.  I am raising this issue to invite others to think about\nit, and I am on the fence, but I am leaning towards saying \"yes\".\n\n>  '\n\nRegardless of what the resolution for \"pull --quiet\" would be,\nshouldn't we have a test for this change to ensure that we do warn\nunder the condition we should, and that we do not do so when we\nshould not?\n\nThanks.\n\n"},{"id":"392885","messageId":"xmqqk13y4ptz.fsf@gitster-ct.c.googlers.com","threadId":"52934","inReplyTo":"xmqq4kv45995.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4] 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-05T18:30:16Z","receivedAt":"2020-03-05T18:30:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n>\n> Alex Henrie <alexhenrie24@gmail.com> writes:\n>\n>> - Revise warning message based on Junio's feedback\n>> - Consistently wrap warning lines to 75 characters for easy viewing in\n>> PO files\n>\n> Nice to see attention to such a detail ;-)\n>\n>> - Fix test failures\n>\n> Ah, OK, hmmm.  \n>\n> For --quiet test, that wants to ensure that \"pull --quiet\" does not\n> say anything, it certainly stops the test from failing if we set the\n> configuration before executing such a test, but I wonder if that is\n> in line with the spirit of the feature the test tries to protect in\n> the first place.  I would imagine those who write \"pull --quiet\" in\n> automation would not want to see any non-error message, and because\n> this is not an error, they do not want to see any output.  Shouldn't\n> such a use of \"pull --quiet\" bypass this warning, too?\n\nActually, the change to t/t5521 does not even sweep the problem\nunder the rug X-<\n\n>> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\n>> index ccde8ba491..6e890ec936 100755\n>> --- a/t/t5521-pull-options.sh\n>> +++ b/t/t5521-pull-options.sh\n>> @@ -8,7 +8,8 @@ test_expect_success 'setup' '\n>>  \tmkdir parent &&\n>>  \t(cd parent && git init &&\n>>  \t echo one >file && git add file &&\n>> -\t git commit -m one)\n>> +\t git commit -m one) &&\n>> +\tgit config pull.rebase false\n\nThis affects the repository at the top of the trash directory, but\nthe test immediately after this one looks like this:\n\n    test_expect_success 'git pull -q' '\n            mkdir clonedq &&\n            (cd clonedq && git init &&\n            git pull -q \"../parent\" >out 2>err &&\n            test_must_be_empty err &&\n            test_must_be_empty out)\n    '\n\nThe patch added pull.rebase to \"t/trash directory.5521/.git/config\",\nbut it would not affect this \"git pull -q\", that is run in the\nrepository that uses \"t/trash directory.5521/clonedq/.git/config\"\nas its configuration.\n\nFor now, I added the following patch on top of the topic before\nqueuing it to 'pu'.\n\nThanks.\n\n-- >8 --\nSubject: [PATCH] SQUASH???\n\nLet's declare that for the purpose of \"pull --quiet\", the new \"you\nmight have created a merge when you may wanted to rebase\" warning is\nan unwanted noise.\n\nRevert the hack made to t5521; the second test that creates a new\nrepository and runs a pull inside it will not get affected by the\nlocal configuration setting it makes to the original trash\nrepository and the breakage won't be worked around with it anyways.\n\nThis is not a full remedy for the previous step, yet.  The topic\nneeds a test to ensure the warning is emitted when it should be.\nExisting t5521 is a test that ensures the warning is not given in\none of the conditions it should not be (i.e. when --quiet is in\neffect).  Negative tests for other cases (e.g. when --ff-only is\ngiven from the command line, without pull.rebase configured) should\nalso be added.\n---\n builtin/pull.c          | 3 ++-\n t/t5521-pull-options.sh | 3 +--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 351b933c4d..0ef192fd64 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -327,7 +327,8 @@ 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_ff || strcmp(opt_ff, \"--ff-only\")) {\n+\tif (0 <= opt_verbosity &&\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\"\ndiff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh\nindex 6e890ec936..ccde8ba491 100755\n--- a/t/t5521-pull-options.sh\n+++ b/t/t5521-pull-options.sh\n@@ -8,8 +8,7 @@ test_expect_success 'setup' '\n \tmkdir parent &&\n \t(cd parent && git init &&\n \t echo one >file && git add file &&\n-\t git commit -m one) &&\n-\tgit config pull.rebase false\n+\t git commit -m one)\n '\n \n test_expect_success 'git pull -q' '\n-- \n2.25.1-595-g3aa4872962\n\n"}]}