{"thread":{"id":"57504","subject":"Silly 'git am' UI issue","startedAt":"2022-03-03T20:31:27Z","lastAt":"2022-03-04T18:51:21Z","messageCount":7,"participants":["Linus Torvalds","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"450303","messageId":"CAHk-=wi2HkQ648sYe3BusCEh5tBYjJbQ9c5T-DrykfEdgRbi=g@mail.gmail.com","threadId":"57504","inReplyTo":null,"subject":"Silly 'git am' UI issue","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2022-03-03T20:31:05Z","receivedAt":"2022-03-03T20:31:27Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"Ok, so I'm \"intellectually challenged\" sometimes.\n\nI tend to apply patches with\n\n $ git am -s --whitespace=fix \n\nand just a moment ago I had a senior moment, and instead did\n\n $ git am -s --whitespace \n\nand then wondered what the heck was wrong with my machine for being so slow.\n\nI ^C'd it, and tried again, because I really am not a smart man. On\nthe *third* try, it finally dawned on me what a maroon I was.\n\nThe problem is obvious when I list those \"right way\" and \"wrong way\"\nnext to each other, but even then the *behavior* is most certainly not\nobvious.\n\nAnd this is really just a problem with \"git am\" (and possibly \"git\nrebase\"), because they know that the \"whitespace\" option needs a\nvalue, but they don't actually _check_ that value. They just know it's\na \"passthru\" argument to \"git apply\" (and \"git am\" in the case of\nrebase).\n\nSo what happens is that \"git am\" decides that is the\nargument to the \"--whitespace\" option, then doesn't see an argument at\nall, and expects the input from stdin. So it \"hangs\", waiting for\ninput. No error messages, no nothing.\n\nIf you actually do the same thing to \"git apply\", you don't see the\nproblem, and it clarifies things:\n\n $ git apply --whitespace \n error: unrecognized whitespace option ''\n\nand it's all very obvious what is going on.\n\nBut \"git am\" never even gets to that stage, because it is busy waiting\nfor input that will never come from the terminally confused person\nsitting in front of the keyboard.\n\nI don't think this is a bug, exactly, but it did make me go \"Whaa?\".\nBut that may be because my other neuron hasn't had enough coffee yet.\n\n Linus\n"},{"id":"450311","messageId":"xmqqlexq8zqo.fsf@gitster.g","threadId":"57504","inReplyTo":"CAHk-=wi2HkQ648sYe3BusCEh5tBYjJbQ9c5T-DrykfEdgRbi=g@mail.gmail.com","subject":"Re: Silly 'git am' UI issue","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-03T21:58:39Z","receivedAt":"2022-03-03T21:58:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> So what happens is that \"git am\" decides that is the\n> argument to the \"--whitespace\" option, then doesn't see an argument at\n> all, and expects the input from stdin. So it \"hangs\", waiting for\n> input. No error messages, no nothing.\n\nOuch. I suspect there are other (e.g. \"pull -> fetch\") passthru\nthat share similar issues, but \"am\" may be quite special in that\nit is prepared to read its input from the standard input stream.\n\nI wonder if something like this would have helped.\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\nSubject: am/apply: warn if we end up reading patches from terminal\n\nIn an interactive session, \"git am\" without arguments, or even\nworse, \"git am --whitespace file\", waits silently for the user to\nfeed the patches from the standard input (presumably by typing or\ncopy-pasting). Give a feedback message to the user when this\nhappens, as it is unlikely that the user meant to do so.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/mailsplit.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git c/builtin/mailsplit.c w/builtin/mailsplit.c\nindex 7baef30569..c45f542341 100644\n--- c/builtin/mailsplit.c\n+++ w/builtin/mailsplit.c\n@@ -223,6 +223,9 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,\n \tFILE *f = !strcmp(file, \"-\") ? stdin : fopen(file, \"r\");\n \tint file_done = 0;\n \n+\tif (isatty(fileno(f)))\n+\t\twarning(_(\"reading patches from stdin/tty...\"));\n+\n \tif (!f) {\n \t\terror_errno(\"cannot open mbox %s\", file);\n \t\tgoto out;\n"},{"id":"450332","messageId":"CAHk-=wjQP8PwYZMYUhKu3s9wMhE2yDqdAO_OKagqXuvPrHRyAQ@mail.gmail.com","threadId":"57504","inReplyTo":"xmqqlexq8zqo.fsf@gitster.g","subject":"Re: Silly 'git am' UI issue","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2022-03-04T00:42:34Z","receivedAt":"2022-03-04T00:43:00Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Thu, Mar 3, 2022 at 1:58 PM Junio C Hamano wrote:\n>\n> I wonder if something like this would have helped.\n\nThat would certainly have helped, yes.\n\nBut I think we can do better.\n\nHow about just parsing the \"--whitespace\" option in 'git am' before\npassing it through?\n\nSomething like the attached patch seems to work for me.\n\nWith this one, I simply get\n\n $ git am --whitespace 0001-Dummy.patch\n error: unrecognized whitespace option '0001-Dummy.patch'\n\nwhen I make the mistake of not giving that whitespace argument.\n\nI'm not claiming this is extensively tested, but I did *some* testing\non it, and it's not a complicated patch.\n\n Linus\n\n\n apply.c | 2 +-\n apply.h | 2 ++\n builtin/am.c | 14 ++++++++++++--\n 3 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 0912307bd9..28e6fe0cfa 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -36,7 +36,7 @@ static void git_apply_config(void)\n \tgit_config(git_xmerge_config, NULL);\n }\n \n-static int parse_whitespace_option(struct apply_state *state, const char *option)\n+int parse_whitespace_option(struct apply_state *state, const char *option)\n {\n \tif (!option) {\n \t\tstate->ws_error_action = warn_on_ws_error;\ndiff --git a/apply.h b/apply.h\nindex 4052da50c0..c931786a2a 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -173,6 +173,8 @@ int parse_git_diff_header(struct strbuf *root,\n \t\t\t unsigned int size,\n \t\t\t struct patch *patch);\n \n+int parse_whitespace_option(struct apply_state *state, const char *option);\n+\n /*\n * Some aspects of the apply behavior are controlled by the following\n * bits in the \"options\" parameter passed to apply_all_patches().\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 0f4111bafa..542c6c5cab 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2298,6 +2298,16 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar\n \treturn 0;\n }\n \n+static int parse_opt_whitespace(const struct option *opt, const char *arg, int unset)\n+{\n+\tstruct apply_state dummy = { };\n+\n+\tif (parse_whitespace_option(&dummy, arg))\n+\t\treturn -1;\n+\n+\treturn parse_opt_passthru_argv(opt, arg, unset);\n+}\n+\n static int git_am_config(const char *k, const char *v, void *cb)\n {\n \tint status;\n@@ -2355,9 +2365,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\tOPT_CALLBACK_F(0, \"quoted-cr\", &state.quoted_cr, N_(\"action\"),\n \t\t\t N_(\"pass it through git-mailinfo\"),\n \t\t\t PARSE_OPT_NONEG, am_option_parse_quoted_cr),\n-\t\tOPT_PASSTHRU_ARGV(0, \"whitespace\", &state.git_apply_opts, N_(\"action\"),\n+\t\tOPT_CALLBACK(0, \"whitespace\", &state.git_apply_opts, N_(\"action\"),\n \t\t\tN_(\"pass it through git-apply\"),\n-\t\t\t0),\n+\t\t\tparse_opt_whitespace),\n \t\tOPT_PASSTHRU_ARGV(0, \"ignore-space-change\", &state.git_apply_opts, NULL,\n \t\t\tN_(\"pass it through git-apply\"),\n \t\t\tPARSE_OPT_NOARG),\n"},{"id":"450351","messageId":"xmqq1qzi4bpk.fsf@gitster.g","threadId":"57504","inReplyTo":"CAHk-=wjQP8PwYZMYUhKu3s9wMhE2yDqdAO_OKagqXuvPrHRyAQ@mail.gmail.com","subject":"Re: Silly 'git am' UI issue","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-04T03:51:19Z","receivedAt":"2022-03-04T03:51:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Thu, Mar 3, 2022 at 1:58 PM Junio C Hamano wrote:\n>>\n>> I wonder if something like this would have helped.\n>\n> That would certainly have helped, yes.\n>\n> But I think we can do better.\n>\n> How about just parsing the \"--whitespace\" option in 'git am' before\n> passing it through?\n>\n> Something like the attached patch seems to work for me.\n>\n> With this one, I simply get\n>\n> $ git am --whitespace 0001-Dummy.patch\n> error: unrecognized whitespace option '0001-Dummy.patch'\n>\n> when I make the mistake of not giving that whitespace argument.\n>\n> I'm not claiming this is extensively tested, but I did *some* testing\n> on it, and it's not a complicated patch.\n>\n> Linus\n\nYup, reusing what apply.c already uses gives reviewers a solid\nassurance. It already is named appropriately for a global\nnamespace, so the patch text, as far as I can see, looks good\nenough.\n\n> +static int parse_opt_whitespace(const struct option *opt, const char *arg, int unset)\n> +{\n> +\tstruct apply_state dummy = { };\n\nIt is rare to see a completely empty initializer in this codebase, I\nthink.\n\n> +\tif (parse_whitespace_option(&dummy, arg))\n> +\t\treturn -1;\n> +\n> +\treturn parse_opt_passthru_argv(opt, arg, unset);\n> +}\n\nLooks good.\n\n> static int git_am_config(const char *k, const char *v, void *cb)\n> {\n> \tint status;\n> @@ -2355,9 +2365,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n> \t\tOPT_CALLBACK_F(0, \"quoted-cr\", &state.quoted_cr, N_(\"action\"),\n> \t\t\t N_(\"pass it through git-mailinfo\"),\n> \t\t\t PARSE_OPT_NONEG, am_option_parse_quoted_cr),\n> -\t\tOPT_PASSTHRU_ARGV(0, \"whitespace\", &state.git_apply_opts, N_(\"action\"),\n> +\t\tOPT_CALLBACK(0, \"whitespace\", &state.git_apply_opts, N_(\"action\"),\n> \t\t\tN_(\"pass it through git-apply\"),\n> -\t\t\t0),\n> +\t\t\tparse_opt_whitespace),\n> \t\tOPT_PASSTHRU_ARGV(0, \"ignore-space-change\", &state.git_apply_opts, NULL,\n> \t\t\tN_(\"pass it through git-apply\"),\n> \t\t\tPARSE_OPT_NOARG),\n"},{"id":"450358","messageId":"220304.86v8wufbf2.gmgdl@evledraar.gmail.com","threadId":"57504","inReplyTo":"xmqq1qzi4bpk.fsf@gitster.g","subject":"Re: Silly 'git am' UI issue","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-04T07:01:15Z","receivedAt":"2022-03-04T07:02:15Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Mar 03 2022, Junio C Hamano wrote:\n\n>> +static int parse_opt_whitespace(const struct option *opt, const char *arg, int unset)\n>> +{\n>> +\tstruct apply_state dummy = { };\n>\n> It is rare to see a completely empty initializer in this codebase, I\n> think.\n\nI don't think we use them at all. It's GNU GCC-specific syntax that\ntranslates to the equivalent of { 0 }, which we can use instead.\n"},{"id":"450359","messageId":"220304.86r17ifahr.gmgdl@evledraar.gmail.com","threadId":"57504","inReplyTo":"xmqq1qzi4bpk.fsf@gitster.g","subject":"Re: Silly 'git am' UI issue","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-04T07:14:47Z","receivedAt":"2022-03-04T07:22:16Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Mar 03 2022, Junio C Hamano wrote:\n\n> Linus Torvalds writes:\n> [...]\n>> +static int parse_opt_whitespace(const struct option *opt, const char *arg, int unset)\n>> +{\n>> +\tstruct apply_state dummy = { };\n>\n> It is rare to see a completely empty initializer in this codebase, I\n> think.\n>\n>> +\tif (parse_whitespace_option(&dummy, arg))\n>> +\t\treturn -1;\n>> +\n>> +\treturn parse_opt_passthru_argv(opt, arg, unset);\n>> +}\n>\n> Looks good.\n>\n>> static int git_am_config(const char *k, const char *v, void *cb)\n>> {\n>> \tint status;\n>> @@ -2355,9 +2365,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n>> \t\tOPT_CALLBACK_F(0, \"quoted-cr\", &state.quoted_cr, N_(\"action\"),\n>> \t\t\t N_(\"pass it through git-mailinfo\"),\n>> \t\t\t PARSE_OPT_NONEG, am_option_parse_quoted_cr),\n>> -\t\tOPT_PASSTHRU_ARGV(0, \"whitespace\", &state.git_apply_opts, N_(\"action\"),\n>> +\t\tOPT_CALLBACK(0, \"whitespace\", &state.git_apply_opts, N_(\"action\"),\n>> \t\t\tN_(\"pass it through git-apply\"),\n>> -\t\t\t0),\n>> +\t\t\tparse_opt_whitespace),\n>> \t\tOPT_PASSTHRU_ARGV(0, \"ignore-space-change\", &state.git_apply_opts, NULL,\n>> \t\t\tN_(\"pass it through git-apply\"),\n>> \t\t\tPARSE_OPT_NOARG),\n\nPerfect shouldn't be the enemy of the good & all that, and this is an\nimprovement.\n\nBut having looked at this the general problem is that any\nOPT_PASSTHRU_ARGV without a PARSE_OPT_NOARG has the same potential issue\nin the case of \"am\", and I don't see how we can resolve the ambiguity\nwithout calling e.g. parse_whitespace_option(), i.e. we need to call\nwhatever the validation function is for each one.\n\nThis change leaves the same problem in place for --exclude, --include,\nand also -C, -p (but one is less likely to do -Cname).\n\nA more general solution would be some continuation of this, i.e. we can\nuse the \"defval\" in \"struct option\" as a pointer to a validation\nfunction for any arguments.\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 0f4111bafa0..fa922fda200 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2355,9 +2355,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\tOPT_CALLBACK_F(0, \"quoted-cr\", &state.quoted_cr, N_(\"action\"),\n \t\t\t N_(\"pass it through git-mailinfo\"),\n \t\t\t PARSE_OPT_NONEG, am_option_parse_quoted_cr),\n-\t\tOPT_PASSTHRU_ARGV(0, \"whitespace\", &state.git_apply_opts, N_(\"action\"),\n+\t\tOPT_PASSTHRU_ARGV_CHECK(0, \"whitespace\", &state.git_apply_opts, N_(\"action\"),\n \t\t\tN_(\"pass it through git-apply\"),\n-\t\t\t0),\n+\t\t\t0, parse_whitespace_option),\n \t\tOPT_PASSTHRU_ARGV(0, \"ignore-space-change\", &state.git_apply_opts, NULL,\n \t\t\tN_(\"pass it through git-apply\"),\n \t\t\tPARSE_OPT_NOARG),\ndiff --git a/parse-options.h b/parse-options.h\nindex 685fccac137..8348343bf59 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -356,6 +356,8 @@ int parse_opt_tracking_mode(const struct option *, const char *, int);\n \t{ OPTION_CALLBACK, (s), (l), (v), (a), (h), (f), parse_opt_passthru }\n #define OPT_PASSTHRU_ARGV(s, l, v, a, h, f) \\\n \t{ OPTION_CALLBACK, (s), (l), (v), (a), (h), (f), parse_opt_passthru_argv }\n+#define OPT_PASSTHRU_ARGV_CHECK(s, l, v, a, h, f, c) \\\n+\t{ OPTION_CALLBACK, (s), (l), (v), (a), (h), (f), parse_opt_passthru_argv, (c) }\n #define _OPT_CONTAINS_OR_WITH(name, variable, help, flag) \\\n \t{ OPTION_CALLBACK, 0, name, (variable), N_(\"commit\"), (help), \\\n \t PARSE_OPT_LASTARG_DEFAULT | flag, \\\n\n\n"},{"id":"450432","messageId":"CAHk-=whaqdakNBwVZ-AZ0Ei2WLA+=3oqofhKfdQ618dWrYySfQ@mail.gmail.com","threadId":"57504","inReplyTo":"220304.86r17ifahr.gmgdl@evledraar.gmail.com","subject":"Re: Silly 'git am' UI issue","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2022-03-04T18:50:57Z","receivedAt":"2022-03-04T18:51:21Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Thu, Mar 3, 2022 at 11:22 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> A more general solution would be some continuation of this, i.e. we can\n> use the \"defval\" in \"struct option\" as a pointer to a validation\n> function for any arguments.\n\nI wasn't familiar enough with the option parsing code to do this, but\nyes, I think your approach is nicer and makes it easier to add\nchecking to the other passthrough cases\n\nSo Ack from me on that approach instead.\n\nI do suspect that you'll notice when trying to code up a proper patch\nthat it's probably complicated by the fact that the \"validation\"\nfunction wants a different argument (in this case a 'struct\napply_state') than the \"passthrough\" function does (it wants that\n\"struct strvec\").\n\nAgain, I'm not familiar with the argument parsing code, it all\npost-dates my work, so what do I know..\n\n Linus\n"}]}