diff mbox series

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

Message ID 20210513214152.34792-2-firminmartin24@gmail.com (mailing list archive)
State New
Headers show
Series Refactor some prompts | expand

Commit Message

Firmin Martin May 13, 2021, 9:41 p.m. UTC
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(+)

Comments

Junio C Hamano May 13, 2021, 10:36 p.m. UTC | #1
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);
Junio C Hamano May 13, 2021, 11:03 p.m. UTC | #2
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).
 */
Jeff King May 15, 2021, 10:12 a.m. UTC | #3
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
diff mbox series

Patch

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);