{"thread":{"id":"55686","subject":"[PATCH 1/2] prompt.h: clarify the non-use of git_prompt","startedAt":"2021-05-13T21:42:02Z","lastAt":"2021-05-15T10:12:45Z","messageCount":7,"participants":["Firmin Martin","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"424516","messageId":"20210513214152.34792-2-firminmartin24@gmail.com","threadId":"55686","inReplyTo":"20210513214152.34792-1-firminmartin24@gmail.com","subject":"[PATCH 1/2] prompt.h: clarify the non-use of git_prompt","fromName":"Firmin Martin","fromEmail":"firminmartin24@gmail.com","sentAt":"2021-05-13T21:41:51Z","receivedAt":"2021-05-13T21:42:02Z","isPatch":true,"sender":{"key":"firminmartin24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20540328?v=4"},"body":"It's really tempting to use git_prompt to prompt user for input, but in\nmost of the case, we must not [1]. Reflect this as a comment in prompt.h.\n\n[1]: https://lore.kernel.org/git/YJTH+sTP%2FO5Nxtp9@coredump.intra.peff.net/\n\nReported-by: Jeff King \nSigned-off-by: Firmin Martin \n---\n prompt.h | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/prompt.h b/prompt.h\nindex e294e93541..90eb3f08a3 100644\n--- a/prompt.h\n+++ b/prompt.h\n@@ -4,6 +4,16 @@\n #define PROMPT_ASKPASS (1<<0)\n #define PROMPT_ECHO (1<<1)\n \n+/*\n+ * This function should not be used for regular prompts (i.e., asking user for\n+ * confirmation or picking an option from an interactive menu) as it only\n+ * accepts input from /dev/tty, thus making it impossible to test with the\n+ * current test suite. Please instead use git_read_line_interactively for that\n+ * purpose. See 97387c8bdd (am: read interactive input from stdin, 2019-05-20)\n+ * for historical context.\n+ *\n+ */\n+\n char *git_prompt(const char *prompt, int flags);\n \n int git_read_line_interactively(struct strbuf *line);\n-- \n2.31.1.443.g67a420f573\n\n"},{"id":"424517","messageId":"20210513214152.34792-1-firminmartin24@gmail.com","threadId":"55686","inReplyTo":null,"subject":"[PATCH 0/2] Refactor some prompts","fromName":"Firmin Martin","fromEmail":"firminmartin24@gmail.com","sentAt":"2021-05-13T21:41:50Z","receivedAt":"2021-05-13T21:42:02Z","isPatch":true,"sender":{"key":"firminmartin24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20540328?v=4"},"body":"As discussed in [1]: \n- The two instances of git_prompt in builtin/bisect--helper.c should be\nreplaced by git_read_line_interactively;\n- The prompts using fgets in \"git am -i\" could also be refactored using that\nhelper function;\n- It would be nice to add a comment which warns people to not use\ngit_prompt for regular prompts. \n\nThis patch series addresses these points.\n\n[1]: https://lore.kernel.org/git/20210506165102.123739-2-firminmartin24@gmail.com/\n\nFirmin Martin (2):\n prompt.h: clarify the non-use of git_prompt\n builtin: use git_read_line_interactively to prompt\n\n builtin/am.c | 14 +++++++-------\n builtin/bisect--helper.c | 15 ++++++++-------\n prompt.h | 10 ++++++++++\n 3 files changed, 25 insertions(+), 14 deletions(-)\n\n-- \n2.31.1.443.g67a420f573\n\n"},{"id":"424518","messageId":"20210513214152.34792-3-firminmartin24@gmail.com","threadId":"55686","inReplyTo":"20210513214152.34792-1-firminmartin24@gmail.com","subject":"[PATCH 2/2] builtin: use git_read_line_interactively to prompt","fromName":"Firmin Martin","fromEmail":"firminmartin24@gmail.com","sentAt":"2021-05-13T21:41:52Z","receivedAt":"2021-05-13T21:42:04Z","isPatch":true,"sender":{"key":"firminmartin24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20540328?v=4"},"body":"Refactor prompts using the helper function git_read_line_interactively\nas done in 08d383f23e (interactive: refactor code asking the user for\ninteractive input, 2020-04-10).\n\nAlso fix two git_prompt instances, introduced in 09535f056b\n(bisect--helper: reimplement `bisect_autostart` shell function in C,\n2020-09-24). See 97387c8bdd (am: read interactive input from stdin,\n2019-05-20) for a detailed motivation to do so.\n\nSigned-off-by: Firmin Martin \n---\n builtin/am.c | 14 +++++++-------\n builtin/bisect--helper.c | 15 ++++++++-------\n 2 files changed, 15 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 8355e3566f..32eae4eb2e 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1643,7 +1643,7 @@ static int do_interactive(struct am_state *state)\n \tassert(state->msg);\n \n \tfor (;;) {\n-\t\tchar reply[64];\n+\t\tstruct strbuf reply = STRBUF_INIT;\n \n \t\tputs(_(\"Commit Body is:\"));\n \t\tputs(\"--------------------------\");\n@@ -1656,17 +1656,17 @@ static int do_interactive(struct am_state *state)\n \t\t * input at this point.\n \t\t */\n \t\tprintf(_(\"Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: \"));\n-\t\tif (!fgets(reply, sizeof(reply), stdin))\n+\t\tif (git_read_line_interactively(&reply) == EOF)\n \t\t\tdie(\"unable to read from stdin; aborting\");\n \n-\t\tif (*reply == 'y' || *reply == 'Y') {\n+\t\tif (reply.len && !strncasecmp(reply.buf, \"yes\", reply.len)) {\n \t\t\treturn 0;\n-\t\t} else if (*reply == 'a' || *reply == 'A') {\n+\t\t} else if (*reply.buf == 'a' || *reply.buf == 'A') {\n \t\t\tstate->interactive = 0;\n \t\t\treturn 0;\n-\t\t} else if (*reply == 'n' || *reply == 'N') {\n+\t\t} else if (reply.len && !strncasecmp(reply.buf, \"no\", reply.len)) {\n \t\t\treturn 1;\n-\t\t} else if (*reply == 'e' || *reply == 'E') {\n+\t\t} else if (*reply.buf == 'e' || *reply.buf == 'E') {\n \t\t\tstruct strbuf msg = STRBUF_INIT;\n \n \t\t\tif (!launch_editor(am_path(state, \"final-commit\"), &msg, NULL)) {\n@@ -1674,7 +1674,7 @@ static int do_interactive(struct am_state *state)\n \t\t\t\tstate->msg = strbuf_detach(&msg, &state->msg_len);\n \t\t\t}\n \t\t\tstrbuf_release(&msg);\n-\t\t} else if (*reply == 'v' || *reply == 'V') {\n+\t\t} else if (*reply.buf == 'v' || *reply.buf == 'V') {\n \t\t\tconst char *pager = git_pager(1);\n \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n \ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 1fdb7d9d10..134347eb39 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -340,7 +340,7 @@ static int decide_next(const struct bisect_terms *terms,\n \n \tif (missing_good && !missing_bad &&\n \t !strcmp(current_term, terms->term_good)) {\n-\t\tchar *yesno;\n+\t\tstruct strbuf yesno = STRBUF_INIT;\n \t\t/*\n \t\t * have bad (or new) but not good (or old). We could bisect\n \t\t * although this is less optimum.\n@@ -353,8 +353,9 @@ static int decide_next(const struct bisect_terms *terms,\n \t\t * translation. The program will only accept English input\n \t\t * at this point.\n \t\t */\n-\t\tyesno = git_prompt(_(\"Are you sure [Y/n]? \"), PROMPT_ECHO);\n-\t\tif (starts_with(yesno, \"N\") || starts_with(yesno, \"n\"))\n+\t\tprintf(_(\"Are you sure [Y/n]? \"));\n+\t\tgit_read_line_interactively(&yesno);\n+\t\tif (yesno.len && !strncasecmp(yesno.buf, \"no\", yesno.len))\n \t\t\treturn -1;\n \t\treturn 0;\n \t}\n@@ -805,7 +806,7 @@ static inline int file_is_not_empty(const char *path)\n static int bisect_autostart(struct bisect_terms *terms)\n {\n \tint res;\n-\tconst char *yesno;\n+\tstruct strbuf yesno = STRBUF_INIT;\n \n \tif (file_is_not_empty(git_path_bisect_start()))\n \t\treturn 0;\n@@ -821,9 +822,9 @@ static int bisect_autostart(struct bisect_terms *terms)\n \t * translation. The program will only accept English input\n \t * at this point.\n \t */\n-\tyesno = git_prompt(_(\"Do you want me to do it for you \"\n-\t\t\t \"[Y/n]? \"), PROMPT_ECHO);\n-\tres = tolower(*yesno) == 'n' ?\n+\tprintf(_(\"Do you want me to do it for you [Y/n]? \"));\n+\tgit_read_line_interactively(&yesno);\n+\tres = (yesno.len && strncasecmp(yesno.buf, \"no\", yesno.len)) ?\n \t\t-1 : bisect_start(terms, empty_strvec, 0);\n \n \treturn res;\n-- \n2.31.1.443.g67a420f573\n\n"},{"id":"424521","messageId":"xmqqv97m8dnw.fsf@gitster.g","threadId":"55686","inReplyTo":"20210513214152.34792-2-firminmartin24@gmail.com","subject":"Re: [PATCH 1/2] prompt.h: clarify the non-use of git_prompt","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-13T22:36:03Z","receivedAt":"2021-05-13T22:36:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Firmin Martin writes:\n\n> +/*\n> + * This function should not be used for regular prompts (i.e., asking user for\n> + * confirmation or picking an option from an interactive menu) as it only\n> + * accepts input from /dev/tty, thus making it impossible to test with the\n> + * current test suite. Please instead use git_read_line_interactively for that\n> + * purpose. See 97387c8bdd (am: read interactive input from stdin, 2019-05-20)\n> + * for historical context.\n> + *\n> + */\n\nI have a strong objection against the above phrasing.\n\nIf we are asking user for interactive input, this SHOULD be used,\nespecially if we might be reading the data to work on from the\nstandard input and we may need to ask the user to interactively\ninstruct us what to do to that data. The only plausible reason that\nwe may want to avoid it and instead prefer the (misnamed)\nread_line_interactively() to read whatever from the standard input\n(which may not be \"interactive\" at all, which is why I said\n\"misnamed\") is because our test framework does not use setsid (and\nsetsid(1) may not be universally available) with pty to emulate tty\ninput, isn't it?\n\n> char *git_prompt(const char *prompt, int flags);\n> \n> int git_read_line_interactively(struct strbuf *line);\n"},{"id":"424524","messageId":"xmqqr1ia8cxs.fsf@gitster.g","threadId":"55686","inReplyTo":"20210513214152.34792-3-firminmartin24@gmail.com","subject":"Re: [PATCH 2/2] builtin: use git_read_line_interactively to prompt","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-13T22:51:43Z","receivedAt":"2021-05-13T22:51:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Firmin Martin writes:\n\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 8355e3566f..32eae4eb2e 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -1643,7 +1643,7 @@ static int do_interactive(struct am_state *state)\n> \tassert(state->msg);\n> \n> \tfor (;;) {\n> -\t\tchar reply[64];\n> +\t\tstruct strbuf reply = STRBUF_INIT;\n\nIf you call this reply_strbuf, and introduce \"const char *reply\",\nand ...\n\n> \t\tputs(_(\"Commit Body is:\"));\n> \t\tputs(\"--------------------------\");\n> @@ -1656,17 +1656,17 @@ static int do_interactive(struct am_state *state)\n> \t\t * input at this point.\n> \t\t */\n> \t\tprintf(_(\"Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: \"));\n> -\t\tif (!fgets(reply, sizeof(reply), stdin))\n> +\t\tif (git_read_line_interactively(&reply) == EOF)\n> \t\t\tdie(\"unable to read from stdin; aborting\");\n\n... assign \"reply = reply_strbuf.buf\" here, you do not have to\nchange anything below.\n\n> -\t\tif (*reply == 'y' || *reply == 'Y') {\n> +\t\tif (reply.len && !strncasecmp(reply.buf, \"yes\", reply.len)) {\n\nAnd we shouldn't sneak an unrelated change like this into a patch\nwhose purpose is to allow reading from the standard input instead of\nalways from \"/dev/tty\".\n\nAs I explained earlier, git_read_line_interactively() is usable only\nwhen we know that the standard input is *not* used for the data we\nare operating on and is available to be used for simulating\ninteractive input. Since \"git am -i\" never reads the mailbox from\nthe standard input, the use of the helper here is acceptable.\n\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 1fdb7d9d10..134347eb39 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -340,7 +340,7 @@ static int decide_next(const struct bisect_terms *terms,\n> \n> \tif (missing_good && !missing_bad &&\n> \t !strcmp(current_term, terms->term_good)) {\n> -\t\tchar *yesno;\n> +\t\tstruct strbuf yesno = STRBUF_INIT;\n> \t\t/*\n> \t\t * have bad (or new) but not good (or old). We could bisect\n> \t\t * although this is less optimum.\n> @@ -353,8 +353,9 @@ static int decide_next(const struct bisect_terms *terms,\n> \t\t * translation. The program will only accept English input\n> \t\t * at this point.\n> \t\t */\n> -\t\tyesno = git_prompt(_(\"Are you sure [Y/n]? \"), PROMPT_ECHO);\n> -\t\tif (starts_with(yesno, \"N\") || starts_with(yesno, \"n\"))\n> +\t\tprintf(_(\"Are you sure [Y/n]? \"));\n> +\t\tgit_read_line_interactively(&yesno);\n\nThe use of this helper here is also OK as the standard input to \"git\nbisect\" is available for simulating interactive input.\n\n> +\t\tif (yesno.len && !strncasecmp(yesno.buf, \"no\", yesno.len))\n> \t\t\treturn -1;\n> \t\treturn 0;\n> \t}\n\nIt may make sense to have a thin wrapper around this pattern, e.g.\n\n\tstatic int ask_yesno(const char *question)\n\t{\n\t\tstruct strbuf answer = STRBUF_INIT;\n\t\tprintf(\"%s [Y/n]? \", question);\n\t\tgit_read_line_interactively(&answer);\n return !(answer.buf[0] == 'N' || answer.buf[0] == 'n');\n\t}\n\nso that the above caller can become\n\n\t\treturn ask_yesno(_(\"Are you sure\")) ? 0 : -1;\n\nand ...\n\n> -\tyesno = git_prompt(_(\"Do you want me to do it for you \"\n> -\t\t\t \"[Y/n]? \"), PROMPT_ECHO);\n> -\tres = tolower(*yesno) == 'n' ?\n> +\tprintf(_(\"Do you want me to do it for you [Y/n]? \"));\n> +\tgit_read_line_interactively(&yesno);\n> +\tres = (yesno.len && strncasecmp(yesno.buf, \"no\", yesno.len)) ?\n> \t\t-1 : bisect_start(terms, empty_strvec, 0);\n\n... this caller can become\n\n\tif (ask_yesno(_(\"Do you want me to do it for you\")))\n\t\tres = bisect_start(terms, empty_strvec, 0);\n\telse\n\t\tres = -1;\n\nThanks.\n"},{"id":"424525","messageId":"xmqqmtsy8cdt.fsf@gitster.g","threadId":"55686","inReplyTo":"xmqqv97m8dnw.fsf@gitster.g","subject":"Re: [PATCH 1/2] prompt.h: clarify the non-use of git_prompt","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-13T23:03:42Z","receivedAt":"2021-05-13T23:03:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I have a strong objection against the above phrasing.\n>\n> If we are asking user for interactive input, this SHOULD be used,\n> especially if we might be reading the data to work on from the\n> standard input and we may need to ask the user to interactively\n> instruct us what to do to that data. The only plausible reason that\n> we may want to avoid it and instead prefer the (misnamed)\n> read_line_interactively() to read whatever from the standard input\n> (which may not be \"interactive\" at all, which is why I said\n> \"misnamed\") is because our test framework does not use setsid (and\n> setsid(1) may not be universally available) with pty to emulate tty\n> input, isn't it?\n>\n>> char *git_prompt(const char *prompt, int flags);\n>> \n>> int git_read_line_interactively(struct strbuf *line);\n\nSo, here is an alternative that nudges users away from this helper,\nbut with honesty. I also suggest a better name for that misnamed\n\"interactively\" function in the comment, but will leave it as an\nexercise to readers to come up with a patch to rename the function.\n\n/*\n * Give prompt to the user and accept interactive input from the\n * controlling terminal (/dev/tty). This function can be used even\n * when the standard input is being used to feed us real data to\n * operate on, as we open /dev/tty ourselves for user interaction.\n *\n * In a codepath that never uses the standard input for real data,\n * consider using git_read_line_from_standard_input() instead, as it\n * is easier to write tests for (our test framework currently does not\n * make it easy to simulate end-user input coming from /dev/tty).\n */\n\n"},{"id":"424638","messageId":"YJ+elwq2OkvSgvJc@coredump.intra.peff.net","threadId":"55686","inReplyTo":"xmqqv97m8dnw.fsf@gitster.g","subject":"Re: [PATCH 1/2] prompt.h: clarify the non-use of git_prompt","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-15T10:12:39Z","receivedAt":"2021-05-15T10:12:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 14, 2021 at 07:36:03AM +0900, Junio C Hamano wrote:\n\n> Firmin Martin writes:\n> \n> > +/*\n> > + * This function should not be used for regular prompts (i.e., asking user for\n> > + * confirmation or picking an option from an interactive menu) as it only\n> > + * accepts input from /dev/tty, thus making it impossible to test with the\n> > + * current test suite. Please instead use git_read_line_interactively for that\n> > + * purpose. See 97387c8bdd (am: read interactive input from stdin, 2019-05-20)\n> > + * for historical context.\n> > + *\n> > + */\n> \n> I have a strong objection against the above phrasing.\n> \n> If we are asking user for interactive input, this SHOULD be used,\n> especially if we might be reading the data to work on from the\n> standard input and we may need to ask the user to interactively\n> instruct us what to do to that data. The only plausible reason that\n> we may want to avoid it and instead prefer the (misnamed)\n> read_line_interactively() to read whatever from the standard input\n> (which may not be \"interactive\" at all, which is why I said\n> \"misnamed\") is because our test framework does not use setsid (and\n> setsid(1) may not be universally available) with pty to emulate tty\n> input, isn't it?\n\nI'm not sure I agree with your \"should\". Sometimes it's convenient to\naccess the controlling terminal directly, but sometimes it's convenient\nto be able to drive the interaction of programs over stdin. Obviously\nour tests are more interested in doing that than most users would be,\nbut it can still be handy (e.g., driving them from another non-terminal\nprogram). Arguably nobody should be doing that as these are porcelains,\nbut I wouldn't be at all surprised if it happens (especially for\nsomething like add--interactive).\n\nIn such a case, switching from stdin to /dev/tty may be considered a\nregression (I know that the patches here are switching bisect away from\nusing the tty, but I think it is just reversing what happened with the\nrecent switch from git-bisect.sh to the builtin, though I think it\ninsisted on \"test -t 0\" in the old code, so the distinction may be\nmoot).\n\nI dunno. I guess I don't feel all that strongly, but I just generally\nfind stdin more convenient than accessing the tty directly (because it's\neasy to do \"