[PATCH 0/2] Refactor some prompts

[PATCH 0/2] Refactor some prompts

[Firmin Martin]

As discussed in [1]: 
- The two instances of git_prompt in builtin/bisect--helper.c should be
replaced by git_read_line_interactively;
- The prompts using fgets in "git am -i" could also be refactored using that
helper function;
- It would be nice to add a comment which warns people to not use
git_prompt for regular prompts. 

This patch series addresses these points.

[1]: https://lore.kernel.org/git/20210506165102.123739-2-firminmartin24@gmail.com/

Firmin Martin (2):
  prompt.h: clarify the non-use of git_prompt
  builtin: use git_read_line_interactively to prompt

 builtin/am.c             | 14 +++++++-------
 builtin/bisect--helper.c | 15 ++++++++-------
 prompt.h                 | 10 ++++++++++
 3 files changed, 25 insertions(+), 14 deletions(-)

-- 
2.31.1.443.g67a420f573

[PATCH 1/2] prompt.h: clarify the non-use of git_prompt

[Firmin Martin]

It's really tempting to use git_prompt to prompt user for input, but in
most of the case, we must not [1]. Reflect this as a comment in prompt.h.

[1]: https://lore.kernel.org/git/YJTH+sTP%2FO5Nxtp9@coredump.intra.peff.net/

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Firmin Martin <firminmartin24@gmail.com>
---
 prompt.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/prompt.h b/prompt.h
index e294e93541..90eb3f08a3 100644
--- a/prompt.h
+++ b/prompt.h
@@ -4,6 +4,16 @@
 #define PROMPT_ASKPASS (1<<0)
 #define PROMPT_ECHO    (1<<1)
 
+/*
+ * This function should not be used for regular prompts (i.e., asking user for
+ * confirmation or picking an option from an interactive menu) as it only
+ * accepts input from /dev/tty, thus making it impossible to test with the
+ * current test suite.  Please instead use git_read_line_interactively for that
+ * purpose.  See 97387c8bdd (am: read interactive input from stdin, 2019-05-20)
+ * for historical context.
+ *
+ */
+
 char *git_prompt(const char *prompt, int flags);
 
 int git_read_line_interactively(struct strbuf *line);
-- 
2.31.1.443.g67a420f573

[PATCH 2/2] builtin: use git_read_line_interactively to prompt

[Firmin Martin]

Refactor prompts using the helper function git_read_line_interactively
as done in 08d383f23e (interactive: refactor code asking the user for
interactive input, 2020-04-10).

Also fix two git_prompt instances, introduced in 09535f056b
(bisect--helper: reimplement `bisect_autostart` shell function in C,
2020-09-24). See 97387c8bdd (am: read interactive input from stdin,
2019-05-20) for a detailed motivation to do so.

Signed-off-by: Firmin Martin <firminmartin24@gmail.com>
---
 builtin/am.c             | 14 +++++++-------
 builtin/bisect--helper.c | 15 ++++++++-------
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 8355e3566f..32eae4eb2e 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1643,7 +1643,7 @@ static int do_interactive(struct am_state *state)
 	assert(state->msg);
 
 	for (;;) {
-		char reply[64];
+		struct strbuf reply = STRBUF_INIT;
 
 		puts(_("Commit Body is:"));
 		puts("--------------------------");
@@ -1656,17 +1656,17 @@ static int do_interactive(struct am_state *state)
 		 * input at this point.
 		 */
 		printf(_("Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: "));
-		if (!fgets(reply, sizeof(reply), stdin))
+		if (git_read_line_interactively(&reply) == EOF)
 			die("unable to read from stdin; aborting");
 
-		if (*reply == 'y' || *reply == 'Y') {
+		if (reply.len && !strncasecmp(reply.buf, "yes", reply.len)) {
 			return 0;
-		} else if (*reply == 'a' || *reply == 'A') {
+		} else if (*reply.buf == 'a' || *reply.buf == 'A') {
 			state->interactive = 0;
 			return 0;
-		} else if (*reply == 'n' || *reply == 'N') {
+		} else if (reply.len && !strncasecmp(reply.buf, "no", reply.len)) {
 			return 1;
-		} else if (*reply == 'e' || *reply == 'E') {
+		} else if (*reply.buf == 'e' || *reply.buf == 'E') {
 			struct strbuf msg = STRBUF_INIT;
 
 			if (!launch_editor(am_path(state, "final-commit"), &msg, NULL)) {
@@ -1674,7 +1674,7 @@ static int do_interactive(struct am_state *state)
 				state->msg = strbuf_detach(&msg, &state->msg_len);
 			}
 			strbuf_release(&msg);
-		} else if (*reply == 'v' || *reply == 'V') {
+		} else if (*reply.buf == 'v' || *reply.buf == 'V') {
 			const char *pager = git_pager(1);
 			struct child_process cp = CHILD_PROCESS_INIT;
 
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 1fdb7d9d10..134347eb39 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -340,7 +340,7 @@ static int decide_next(const struct bisect_terms *terms,
 
 	if (missing_good && !missing_bad &&
 	    !strcmp(current_term, terms->term_good)) {
-		char *yesno;
+		struct strbuf yesno = STRBUF_INIT;
 		/*
 		 * have bad (or new) but not good (or old). We could bisect
 		 * although this is less optimum.
@@ -353,8 +353,9 @@ static int decide_next(const struct bisect_terms *terms,
 		 * translation. The program will only accept English input
 		 * at this point.
 		 */
-		yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO);
-		if (starts_with(yesno, "N") || starts_with(yesno, "n"))
+		printf(_("Are you sure [Y/n]? "));
+		git_read_line_interactively(&yesno);
+		if (yesno.len && !strncasecmp(yesno.buf, "no", yesno.len))
 			return -1;
 		return 0;
 	}
@@ -805,7 +806,7 @@ static inline int file_is_not_empty(const char *path)
 static int bisect_autostart(struct bisect_terms *terms)
 {
 	int res;
-	const char *yesno;
+	struct strbuf yesno = STRBUF_INIT;
 
 	if (file_is_not_empty(git_path_bisect_start()))
 		return 0;
@@ -821,9 +822,9 @@ static int bisect_autostart(struct bisect_terms *terms)
 	 * translation. The program will only accept English input
 	 * at this point.
 	 */
-	yesno = git_prompt(_("Do you want me to do it for you "
-			     "[Y/n]? "), PROMPT_ECHO);
-	res = tolower(*yesno) == 'n' ?
+	printf(_("Do you want me to do it for you [Y/n]? "));
+	git_read_line_interactively(&yesno);
+	res = (yesno.len && strncasecmp(yesno.buf, "no", yesno.len)) ?
 		-1 : bisect_start(terms, empty_strvec, 0);
 
 	return res;
-- 
2.31.1.443.g67a420f573

Re: [PATCH 1/2] prompt.h: clarify the non-use of git_prompt

[Junio C Hamano]

Firmin Martin <firminmartin24@gmail.com> writes:

> +/*
> + * This function should not be used for regular prompts (i.e., asking user for
> + * confirmation or picking an option from an interactive menu) as it only
> + * accepts input from /dev/tty, thus making it impossible to test with the
> + * current test suite.  Please instead use git_read_line_interactively for that
> + * purpose.  See 97387c8bdd (am: read interactive input from stdin, 2019-05-20)
> + * for historical context.
> + *
> + */

I have a strong objection against the above phrasing.

If we are asking user for interactive input, this SHOULD be used,
especially if we might be reading the data to work on from the
standard input and we may need to ask the user to interactively
instruct us what to do to that data.  The only plausible reason that
we may want to avoid it and instead prefer the (misnamed)
read_line_interactively() to read whatever from the standard input
(which may not be "interactive" at all, which is why I said
"misnamed") is because our test framework does not use setsid (and
setsid(1) may not be universally available) with pty to emulate tty
input, isn't it?

>  char *git_prompt(const char *prompt, int flags);
>  
>  int git_read_line_interactively(struct strbuf *line);

Re: [PATCH 2/2] builtin: use git_read_line_interactively to prompt

[Junio C Hamano]

Firmin Martin <firminmartin24@gmail.com> writes:

> diff --git a/builtin/am.c b/builtin/am.c
> index 8355e3566f..32eae4eb2e 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1643,7 +1643,7 @@ static int do_interactive(struct am_state *state)
>  	assert(state->msg);
>  
>  	for (;;) {
> -		char reply[64];
> +		struct strbuf reply = STRBUF_INIT;

If you call this reply_strbuf, and introduce "const char *reply",
and ...

>  		puts(_("Commit Body is:"));
>  		puts("--------------------------");
> @@ -1656,17 +1656,17 @@ static int do_interactive(struct am_state *state)
>  		 * input at this point.
>  		 */
>  		printf(_("Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all: "));
> -		if (!fgets(reply, sizeof(reply), stdin))
> +		if (git_read_line_interactively(&reply) == EOF)
>  			die("unable to read from stdin; aborting");

... assign "reply = reply_strbuf.buf" here, you do not have to
change anything below.

> -		if (*reply == 'y' || *reply == 'Y') {
> +		if (reply.len && !strncasecmp(reply.buf, "yes", reply.len)) {

And we shouldn't sneak an unrelated change like this into a patch
whose purpose is to allow reading from the standard input instead of
always from "/dev/tty".

As I explained earlier, git_read_line_interactively() is usable only
when we know that the standard input is *not* used for the data we
are operating on and is available to be used for simulating
interactive input.  Since "git am -i" never reads the mailbox from
the standard input, the use of the helper here is acceptable.

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 1fdb7d9d10..134347eb39 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -340,7 +340,7 @@ static int decide_next(const struct bisect_terms *terms,
>  
>  	if (missing_good && !missing_bad &&
>  	    !strcmp(current_term, terms->term_good)) {
> -		char *yesno;
> +		struct strbuf yesno = STRBUF_INIT;
>  		/*
>  		 * have bad (or new) but not good (or old). We could bisect
>  		 * although this is less optimum.
> @@ -353,8 +353,9 @@ static int decide_next(const struct bisect_terms *terms,
>  		 * translation. The program will only accept English input
>  		 * at this point.
>  		 */
> -		yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO);
> -		if (starts_with(yesno, "N") || starts_with(yesno, "n"))
> +		printf(_("Are you sure [Y/n]? "));
> +		git_read_line_interactively(&yesno);

The use of this helper here is also OK as the standard input to "git
bisect" is available for simulating interactive input.

> +		if (yesno.len && !strncasecmp(yesno.buf, "no", yesno.len))
>  			return -1;
>  		return 0;
>  	}

It may make sense to have a thin wrapper around this pattern, e.g.

	static int ask_yesno(const char *question)
	{
		struct strbuf answer = STRBUF_INIT;
		printf("%s [Y/n]? ", question);
		git_read_line_interactively(&answer);
                return !(answer.buf[0] == 'N' || answer.buf[0] == 'n');
	}

so that the above caller can become

		return ask_yesno(_("Are you sure")) ? 0 : -1;

and ...

> -	yesno = git_prompt(_("Do you want me to do it for you "
> -			     "[Y/n]? "), PROMPT_ECHO);
> -	res = tolower(*yesno) == 'n' ?
> +	printf(_("Do you want me to do it for you [Y/n]? "));
> +	git_read_line_interactively(&yesno);
> +	res = (yesno.len && strncasecmp(yesno.buf, "no", yesno.len)) ?
>  		-1 : bisect_start(terms, empty_strvec, 0);

... this caller can become

	if (ask_yesno(_("Do you want me to do it for you")))
		res = bisect_start(terms, empty_strvec, 0);
	else
		res = -1;

Thanks.

Re: [PATCH 1/2] prompt.h: clarify the non-use of git_prompt

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I have a strong objection against the above phrasing.
>
> If we are asking user for interactive input, this SHOULD be used,
> especially if we might be reading the data to work on from the
> standard input and we may need to ask the user to interactively
> instruct us what to do to that data.  The only plausible reason that
> we may want to avoid it and instead prefer the (misnamed)
> read_line_interactively() to read whatever from the standard input
> (which may not be "interactive" at all, which is why I said
> "misnamed") is because our test framework does not use setsid (and
> setsid(1) may not be universally available) with pty to emulate tty
> input, isn't it?
>
>>  char *git_prompt(const char *prompt, int flags);
>>  
>>  int git_read_line_interactively(struct strbuf *line);

So, here is an alternative that nudges users away from this helper,
but with honesty.  I also suggest a better name for that misnamed
"interactively" function in the comment, but will leave it as an
exercise to readers to come up with a patch to rename the function.

/*
 * Give prompt to the user and accept interactive input from the
 * controlling terminal (/dev/tty).  This function can be used even
 * when the standard input is being used to feed us real data to
 * operate on, as we open /dev/tty ourselves for user interaction.
 *
 * In a codepath that never uses the standard input for real data,
 * consider using git_read_line_from_standard_input() instead, as it
 * is easier to write tests for (our test framework currently does not
 * make it easy to simulate end-user input coming from /dev/tty).
 */

Re: [PATCH 1/2] prompt.h: clarify the non-use of git_prompt

[Jeff King]

On Fri, May 14, 2021 at 07:36:03AM +0900, Junio C Hamano wrote:

> Firmin Martin <firminmartin24@gmail.com> writes:
> 
> > +/*
> > + * This function should not be used for regular prompts (i.e., asking user for
> > + * confirmation or picking an option from an interactive menu) as it only
> > + * accepts input from /dev/tty, thus making it impossible to test with the
> > + * current test suite.  Please instead use git_read_line_interactively for that
> > + * purpose.  See 97387c8bdd (am: read interactive input from stdin, 2019-05-20)
> > + * for historical context.
> > + *
> > + */
> 
> I have a strong objection against the above phrasing.
> 
> If we are asking user for interactive input, this SHOULD be used,
> especially if we might be reading the data to work on from the
> standard input and we may need to ask the user to interactively
> instruct us what to do to that data.  The only plausible reason that
> we may want to avoid it and instead prefer the (misnamed)
> read_line_interactively() to read whatever from the standard input
> (which may not be "interactive" at all, which is why I said
> "misnamed") is because our test framework does not use setsid (and
> setsid(1) may not be universally available) with pty to emulate tty
> input, isn't it?

I'm not sure I agree with your "should". Sometimes it's convenient to
access the controlling terminal directly, but sometimes it's convenient
to be able to drive the interaction of programs over stdin. Obviously
our tests are more interested in doing that than most users would be,
but it can still be handy (e.g., driving them from another non-terminal
program). Arguably nobody should be doing that as these are porcelains,
but I wouldn't be at all surprised if it happens (especially for
something like add--interactive).

In such a case, switching from stdin to /dev/tty may be considered a
regression (I know that the patches here are switching bisect away from
using the tty, but I think it is just reversing what happened with the
recent switch from git-bisect.sh to the builtin, though I think it
insisted on "test -t 0" in the old code, so the distinction may be
moot).

I dunno. I guess I don't feel all that strongly, but I just generally
find stdin more convenient than accessing the tty directly (because it's
easy to do "</dev/tty", but hard to set up a controlling terminal). But
I admit that I am a lot more likely to drive our programs via script for
testing or debugging than normal users would.

-Peff