{"thread":{"id":"52914","subject":"[PATCH] pull: warn if the user didn't say whether to rebase or to merge","startedAt":"2020-02-28T21:58:56Z","lastAt":"2020-02-29T16:33:14Z","messageCount":5,"participants":["Alex Henrie","Junio C Hamano","Theodore Y. Ts'o","Randall S. Becker"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"392694","messageId":"20200228215833.319691-1-alexhenrie24@gmail.com","threadId":"52914","inReplyTo":null,"subject":"[PATCH] pull: warn if the user didn't say whether to rebase or to merge","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2020-02-28T21:58:33Z","receivedAt":"2020-02-28T21:58:56Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"Most Git users are contributors, not maintainers, and want to use\n`git pull` to rebase their changes before sending a pull request to the\nupstream project. However, novice Git users often forget to specify the\n--rebase option when pulling, leading to an unwanted merge commit. To\navoid that situation, Git should require users to explicitly specify\nwhether their primary workflow is a contributor/rebasing workflow or a\nmaintainer/merging workflow.\n\nSigned-off-by: Alex Henrie \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..8ec8e6f5d3 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\") != 0) {\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 specify --rebase or --no-rebase on the command line.\\n\"));\n+\t}\n+\n \treturn REBASE_FALSE;\n }\n \n-- \n2.25.1\n\n"},{"id":"392701","messageId":"xmqqeeuecngu.fsf@gitster-ct.c.googlers.com","threadId":"52914","inReplyTo":"20200228215833.319691-1-alexhenrie24@gmail.com","subject":"Re: [PATCH] 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-28T23:16:01Z","receivedAt":"2020-02-28T23:16:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie writes:\n\n> Most Git users are contributors, not maintainers, and want to use\n> `git pull` to rebase their changes before sending a pull request to the\n> upstream project. However, novice Git users often forget to specify the\n> --rebase option when pulling, leading to an unwanted merge commit.\n\nThe logic above looks somewhat twisted. Even if most Git users were\nintegrators who would not want \"pull --rebase\", everything you said\nafter \"However\" still holds true.\n\n Often novice Git users forget to say \"pull --rebase\" and ends up\n with an unnecessary merge from upstream. What they usually want\n is either \"pull --rebase\" in the simpler cases, or \"pull\n --ff-only\" to update the copy of main integration branches, and\n rebase their work separately. The pull.rebase configuration\n variable exists to help them in the simpler cases, but there is\n no mechanism to make these users aware of it.\n\n> To\n> avoid that situation, Git should require users to explicitly specify\n> whether their primary workflow is a contributor/rebasing workflow or a\n> maintainer/merging workflow.\n\nThere is nothing Git \"should\" do. There are things we wish Git did,\nand we give orders to the codebase to do so in our proposed log\nmessage. Perhaps like:\n\n Issue a warning message when no --[no-]rebase option from the\n command line and no pull.rebase configuration variable is given.\n This will inconvenience those who never want to \"pull --rebase\",\n who haven't had to do anything special, but the cost of the\n inconvenience is paid only once per user, which should be\n reasonable cost to help number of new users.\n\n\n\n>\n> Signed-off-by: Alex Henrie \n> ---\n> builtin/pull.c | 7 +++++++\n> 1 file changed, 7 insertions(+)\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 3e624d1e00..8ec8e6f5d3 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\nThis helper function is called only when opt_rebase < 0 in the\ncaller, which means there were no --[no-]rebase on the command line.\nThat's why we called this function to learn what the configuration\nsay. So it is the right place to add the new check.\n\nLuckily for us, the caller also made sure opt_ff is already set up\nby calling config_get_ff() first when there was no --ff related\ncommand line option ;-)\n\n> \n> +\tif (strcmp(opt_ff, \"--ff-only\") != 0) {\n\nStyle. Do not write \"!= 0\" in the condition. Just\n\n\tif (strcmp(opt_ff, \"--ff-only\")) {\n\nis enough.\n\n> +\t\twarning(_(\"Pulling without specifying whether to rebase or to merge is discouraged\\n\"\n\nI briefly wondered if this wants to be an advice instead, but the\nway to squelch the message is already built into this codepath so\nthere is no need to ;-)\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 specify --rebase or --no-rebase on the command line.\\n\"));\n> +\t}\n> +\n> \treturn REBASE_FALSE;\n> }\n"},{"id":"392706","messageId":"20200229030344.GG101220@mit.edu","threadId":"52914","inReplyTo":"xmqqeeuecngu.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] pull: warn if the user didn't say whether to rebase or to merge","fromName":"Theodore Y. Ts'o","fromEmail":"tytso@mit.edu","sentAt":"2020-02-29T03:03:44Z","receivedAt":"2020-02-29T03:04:03Z","isPatch":true,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Fri, Feb 28, 2020 at 03:16:01PM -0800, Junio C Hamano wrote:\n> > To\n> > avoid that situation, Git should require users to explicitly specify\n> > whether their primary workflow is a contributor/rebasing workflow or a\n> > maintainer/merging workflow.\n> \n> There is nothing Git \"should\" do. There are things we wish Git did,\n> and we give orders to the codebase to do so in our proposed log\n> message. Perhaps like:\n\nI'd also note that there are some workflows that assume that --rebase\nis *never* a good thing, even for contributors. We can decide whether\nwe want to bias the git man page in favor of one workflow as opposed\nto another, for the sake of new git users, but I don't think it's\naccurate to say (or even imply) that there are only two workflows:\ncontributor/rebasing and maintainer/merging.\n\n\t\t\t\t\t\t\t- Ted\n"},{"id":"392712","messageId":"012801d5ef1b$a0167740$e04365c0$@nexbridge.com","threadId":"52914","inReplyTo":"20200229030344.GG101220@mit.edu","subject":"RE: [PATCH] pull: warn if the user didn't say whether to rebase or to merge","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2020-02-29T16:16:34Z","receivedAt":"2020-02-29T16:16:50Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On February 28, 2020 10:04 PM, Theodore Y. Ts'o wrote:\n> To: Junio C Hamano \n> Cc: Alex Henrie ; git@vger.kernel.org;\n> rcdailey.lists@gmail.com; newren@gmail.com; rsbecker@nexbridge.com;\n> annulen@yandex.ru\n> Subject: Re: [PATCH] pull: warn if the user didn't say whether to rebase\nor to\n> merge\n> \n> On Fri, Feb 28, 2020 at 03:16:01PM -0800, Junio C Hamano wrote:\n> > > To\n> > > avoid that situation, Git should require users to explicitly specify\n> > > whether their primary workflow is a contributor/rebasing workflow or\n> > > a maintainer/merging workflow.\n> >\n> > There is nothing Git \"should\" do. There are things we wish Git did,\n> > and we give orders to the codebase to do so in our proposed log\n> > message. Perhaps like:\n> \n> I'd also note that there are some workflows that assume that --rebase is\n> *never* a good thing, even for contributors. We can decide whether we\n> want to bias the git man page in favor of one workflow as opposed to\n> another, for the sake of new git users, but I don't think it's accurate to\nsay\n> (or even imply) that there are only two workflows:\n> contributor/rebasing and maintainer/merging.\n\nI second this sentiment. The repositories my community (outside my company)\nhas are typically large (3-5Gb of sources) with 10K-100K individual files.\nThey all use a */merging paradigm.\n\nRandall\n\n"},{"id":"392713","messageId":"xmqq7e05cq0r.fsf@gitster-ct.c.googlers.com","threadId":"52914","inReplyTo":"20200229030344.GG101220@mit.edu","subject":"Re: [PATCH] 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:33:08Z","receivedAt":"2020-02-29T16:33:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Theodore Y. Ts'o\" writes:\n\n> I'd also note that there are some workflows that assume that --rebase\n> is *never* a good thing, even for contributors. We can decide whether\n> we want to bias the git man page in favor of one workflow as opposed\n> to another, for the sake of new git users, but I don't think it's\n> accurate to say (or even imply) that there are only two workflows:\n> contributor/rebasing and maintainer/merging.\n\nThanks for raising this.\n"}]}