{"thread":{"id":"54280","subject":"[PATCH] pull: do not warn when opt_ff is explicitly specified","startedAt":"2020-09-23T09:56:44Z","lastAt":"2020-09-23T18:33:53Z","messageCount":4,"participants":["Denton Liu","Junio C Hamano","Alex Henrie"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"406186","messageId":"1093128c646b154a14d89321454f5361c0e616b4.1600854717.git.liu.denton@gmail.com","threadId":"54280","inReplyTo":null,"subject":"[PATCH] pull: do not warn when opt_ff is explicitly specified","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-23T09:56:31Z","receivedAt":"2020-09-23T09:56:44Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In d18c950a69 (pull: warn if the user didn't say whether to rebase or to\nmerge, 2020-03-09), `git pull` was taught to warn users if they\nhave `pull.rebase` unset or `pull.ff != \"only\"`. However, this warning\nis a little too eager about happening.\n\nIf the warning is silenced by specifying `pull.ff = \"only\"`, as\ninstructed, the warning will arise again if the user runs\nsomething like `git pull --no-ff`. However, the warning should not\nhappen as the user clearly knows what they're doing.\n\nDon't display the warning if opt_ff is explicitly set by a command-line\noption given by a user.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n builtin/pull.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 015f6ded0b..307b4b5d21 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -83,6 +83,7 @@ static char *opt_commit;\n static char *opt_edit;\n static char *cleanup_arg;\n static char *opt_ff;\n+static int opt_ff_explicit;\n static char *opt_verify_signatures;\n static int opt_autostash = -1;\n static int config_autostash;\n@@ -344,7 +345,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 &&\n+\tif (!opt_ff_explicit && 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@@ -933,6 +934,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \n \tif (!opt_ff)\n \t\topt_ff = xstrdup_or_null(config_get_ff());\n+\telse\n+\t\topt_ff_explicit = 1;\n \n \tif (opt_rebase < 0)\n \t\topt_rebase = config_get_rebase();\n-- \n2.28.0.760.g8d73e04208\n\n"},{"id":"406201","messageId":"xmqq5z84jvk2.fsf@gitster.c.googlers.com","threadId":"54280","inReplyTo":"1093128c646b154a14d89321454f5361c0e616b4.1600854717.git.liu.denton@gmail.com","subject":"Re: [PATCH] pull: do not warn when opt_ff is explicitly specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-23T16:40:29Z","receivedAt":"2020-09-23T16:40:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> In d18c950a69 (pull: warn if the user didn't say whether to rebase or to\n> merge, 2020-03-09), `git pull` was taught to warn users if they\n> have `pull.rebase` unset or `pull.ff != \"only\"`. However, this warning\n> is a little too eager about happening.\n\nIn d18c950a69, we added some tests to keep the warning working as\nintended, and the fact that this patch does not touch anything in\nthe t/ directory means that its test coverage was not perfect.\n\nWe'd need to add a test or two to make sure this new behaviour is\nprotected against future breakages.\n\nThanks.\n\n> If the warning is silenced by specifying `pull.ff = \"only\"`, as\n> instructed, the warning will arise again if the user runs\n> something like `git pull --no-ff`. However, the warning should not\n> happen as the user clearly knows what they're doing.\n>\n> Don't display the warning if opt_ff is explicitly set by a command-line\n> option given by a user.\n>\n> Signed-off-by: Denton Liu <liu.denton@gmail.com>\n> ---\n>  builtin/pull.c | 5 ++++-\n>  1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 015f6ded0b..307b4b5d21 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -83,6 +83,7 @@ static char *opt_commit;\n>  static char *opt_edit;\n>  static char *cleanup_arg;\n>  static char *opt_ff;\n> +static int opt_ff_explicit;\n>  static char *opt_verify_signatures;\n>  static int opt_autostash = -1;\n>  static int config_autostash;\n> @@ -344,7 +345,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 &&\n> +\tif (!opt_ff_explicit && 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> @@ -933,6 +934,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n>  \n>  \tif (!opt_ff)\n>  \t\topt_ff = xstrdup_or_null(config_get_ff());\n> +\telse\n> +\t\topt_ff_explicit = 1;\n>  \n>  \tif (opt_rebase < 0)\n>  \t\topt_rebase = config_get_rebase();\n"},{"id":"406209","messageId":"CAMMLpeRD6hKuAHjHh2AXrx1PVXHx7m9haUPAR_uNHR=YuT5xbQ@mail.gmail.com","threadId":"54280","inReplyTo":"1093128c646b154a14d89321454f5361c0e616b4.1600854717.git.liu.denton@gmail.com","subject":"Re: [PATCH] pull: do not warn when opt_ff is explicitly specified","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2020-09-23T18:25:55Z","receivedAt":"2020-09-23T18:26:11Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Wed, Sep 23, 2020 at 3:56 AM Denton Liu <liu.denton@gmail.com> wrote:\n>\n> In d18c950a69 (pull: warn if the user didn't say whether to rebase or to\n> merge, 2020-03-09), `git pull` was taught to warn users if they\n> have `pull.rebase` unset or `pull.ff != \"only\"`. However, this warning\n> is a little too eager about happening.\n>\n> If the warning is silenced by specifying `pull.ff = \"only\"`, as\n> instructed, the warning will arise again if the user runs\n> something like `git pull --no-ff`. However, the warning should not\n> happen as the user clearly knows what they're doing.\n>\n> Don't display the warning if opt_ff is explicitly set by a command-line\n> option given by a user.\n\nHi Denton, thanks for working on this! We can also assume that the\nuser knows what they are doing and does not need a warning if they\nhave run `git config --global pull.ff no`. So really, we can just get\nrid of the check for strcmp(opt_ff, \"--ff-only\") altogether and\ninstead only check !opt_ff. Could you do that and add some more tests\nto t5521-pull-options.sh?\n\n-Alex\n"},{"id":"406210","messageId":"CAMMLpeR=g6gfjokxpmcj5kfnJxPO-ZLTUpa_QMT_oAcQQ6AR=Q@mail.gmail.com","threadId":"54280","inReplyTo":"CAMMLpeRD6hKuAHjHh2AXrx1PVXHx7m9haUPAR_uNHR=YuT5xbQ@mail.gmail.com","subject":"Re: [PATCH] pull: do not warn when opt_ff is explicitly specified","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2020-09-23T18:33:38Z","receivedAt":"2020-09-23T18:33:53Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Wed, Sep 23, 2020 at 12:25 PM Alex Henrie <alexhenrie24@gmail.com> wrote:\n>\n> Hi Denton, thanks for working on this! We can also assume that the\n> user knows what they are doing and does not need a warning if they\n> have run `git config --global pull.ff no`. So really, we can just get\n> rid of the check for strcmp(opt_ff, \"--ff-only\") altogether and\n> instead only check !opt_ff. Could you do that and add some more tests\n> to t5521-pull-options.sh?\n\nActually, never mind, I'll send the patch myself. When I originally\nput in the warning I didn't stop to think that if the user is\nproficient enough to set pull.ff to any value, they really don't need\na warning.\n\n-Alex\n"}]}