{"thread":{"id":"52917","subject":"[PATCH v2] pull: warn if the user didn't say whether to rebase or to merge","startedAt":"2020-02-29T01:11:02Z","lastAt":"2020-02-29T21:21:35Z","messageCount":3,"participants":["Alex Henrie","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"392704","messageId":"20200229010927.335119-1-alexhenrie24@gmail.com","threadId":"52917","inReplyTo":null,"subject":"[PATCH v2] pull: warn if the user didn't say whether to rebase or to merge","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2020-02-29T01:09:27Z","receivedAt":"2020-02-29T01:11:02Z","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 \n---\nv2:\n- Use the commit message that Junio suggested\n- Drop \"!= 0\" from the if condition\n- Mention in the warning that --ff-only will also suppress the warning\n---\n builtin/pull.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 3e624d1e00..96512a4b27 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -327,6 +327,13 @@ 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 (strcmp(opt_ff, \"--ff-only\")) {\n+\t\twarning(_(\"Pulling without specifying whether to rebase or to merge is discouraged\\n\"\n+\t\t\t\"and will be disallowed in a future Git release.\\n\"\n+\t\t\t\"Next time, run `git config pull.rebase (true|false)` first\\n\"\n+\t\t\t\"or pass --rebase, --no-rebase, or --ff-only on the command line.\\n\"));\n+\t}\n+\n \treturn REBASE_FALSE;\n }\n \n-- \n2.25.1\n\n"},{"id":"392714","messageId":"xmqqzhd1balh.fsf@gitster-ct.c.googlers.com","threadId":"52917","inReplyTo":"20200229010927.335119-1-alexhenrie24@gmail.com","subject":"Re: [PATCH v2] pull: warn if the user didn't say whether to rebase or to merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-29T16:51:38Z","receivedAt":"2020-02-29T16:51:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie writes:\n\n> +\t\twarning(_(\"Pulling without specifying whether to rebase or to merge is discouraged\\n\"\n> +\t\t\t\"and will be disallowed in a future Git release.\\n\"\n\nSorry for not catching this in the earlier round, but I do not think\nanybody argued, let alone the community came to a concensus on\ndoing, such a strong move. Am I mistaken?\n\nI certainly did not intend to, at least when I commented on the\nearlier round and proposed an updated log message, I wasn't even\naware of the possibility that we may want to turn this into die()\nafter a transition period.\n\nNot that I'd object strongly to the idea, but it was somewhat\nunexpected.\n\nIf we are proposing to make it a long-term plan, that should\ncertainly be written down in the proposed log message.\n\n> +\t\t\t\"Next time, run `git config pull.rebase (true|false)` first\\n\"\n> +\t\t\t\"or pass --rebase, --no-rebase, or --ff-only on the command line.\\n\n\nI am somewhat puzzled by \"first, or\". You certainly mean the config\nto be \"set and forget\", and you do not want to say \"before you pull,\ndo this first, always\", but somehow the latter is the impression I\ngot.\n\nBut it does not sound to me like \"Next time, and only next time, do\nthis configuration. You can countermand the choice you make from\nthe command line later if needed\", which I think is what you meant\nto convey to your readers.\n \n You can squelch this message by `pull.rebase` configuration\n variable to show your preference. By passing --[no-]rebase\n or --ff-only from the command line, you can countermand the\n choice per invocation.\n\nis what I came up with, but I am not quite happy with it. It is\noverly long to start with X-<.\n\n\"));\n> +\t}\n> +\n> \treturn REBASE_FALSE;\n> }\n"},{"id":"392718","messageId":"CAMMLpeSSET8F+BTooDhBTBGkh2JjvLWOPwA-ibspoDO0NqGy7A@mail.gmail.com","threadId":"52917","inReplyTo":"xmqqzhd1balh.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] pull: warn if the user didn't say whether to rebase or to merge","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2020-02-29T21:21:18Z","receivedAt":"2020-02-29T21:21:35Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Sat, Feb 29, 2020 at 9:51 AM Junio C Hamano wrote:\n>\n> Alex Henrie writes:\n>\n> > + warning(_(\"Pulling without specifying whether to rebase or to merge is discouraged\\n\"\n> > + \"and will be disallowed in a future Git release.\\n\"\n>\n> Sorry for not catching this in the earlier round, but I do not think\n> anybody argued, let alone the community came to a concensus on\n> doing, such a strong move. Am I mistaken?\n>\n> I certainly did not intend to, at least when I commented on the\n> earlier round and proposed an updated log message, I wasn't even\n> aware of the possibility that we may want to turn this into die()\n> after a transition period.\n>\n> Not that I'd object strongly to the idea, but it was somewhat\n> unexpected.\n>\n> If we are proposing to make it a long-term plan, that should\n> certainly be written down in the proposed log message.\n\nI really don't think it's a good idea to start making a merge commit\nwithout confirming that that's what the user wants. Elijah and Robert\nseemed to agree. However, we can start by just putting in a friendly\neducational message and then continue the discussion about whether or\nnot to make a future version of Git abort upon printing the warning.\n\n> > + \"Next time, run `git config pull.rebase (true|false)` first\\n\"\n> > + \"or pass --rebase, --no-rebase, or --ff-only on the command line.\\n\n>\n> I am somewhat puzzled by \"first, or\". You certainly mean the config\n> to be \"set and forget\", and you do not want to say \"before you pull,\n> do this first, always\", but somehow the latter is the impression I\n> got.\n>\n> But it does not sound to me like \"Next time, and only next time, do\n> this configuration. You can countermand the choice you make from\n> the command line later if needed\", which I think is what you meant\n> to convey to your readers.\n\nI'll send a new version that has a longer and more clear warning message.\n\n-Alex\n"}]}