Re: [PATCH v2 4/4] allow recovery from command name typos
On Thu, Jul 26, 2012 at 10:59:51AM -0700, Junio C Hamano wrote: > > The credential code uses git_terminal_prompt, which actually opens > > /dev/tty directly. So it is probably sane to use for your new prompt, > > but it does not (and should not) rely on isatty. > > I think using git_terminal_prompt() after doing a looser "does the > user sit at a terminal and is capable of answering interactive > prompt" check with isatty(2) is OK, as long as we know that all > implementations of git_terminal_prompt() never read from whatever > happens to be connected to the standard input. I don't think isatty(2) is correct, though. It would yield false negatives when the user has redirected stderr but /dev/tty is still available. I don't know if it possible for getpass to fallback to stdin when stderr is a tty (it would mean that opening /dev/tty failed, which would mean that we have no controlling terminal _but_ our stderr is still connected to some terminal. That might be bizarre enough not to care about). I think the right answer would be a real is_prompt_available() that checked /dev/tty when HAVE_DEV_TTY was set, and otherwise checked isatty(2) (or whatever was appropriate for the platform). > The function falls back to getpass() on platforms without DEV_TTY, > and if getpass() on some platforms reads from the standard input, > that would be a disaster. I wasn't sure about that part. Yeah, although it is already a disaster in those cases, as the main caller of git_terminal_prompt is remote-curl, whose stdin is connected to git via the remote-helper protocol. Which isn't to say this wouldn't make things worse. It would, but the real solution is to implement a sane git_terminal_prompt for those platforms. Erik had a patch for Windows to use their magical CONIN$, but I think it is temporarily stalled. I don't know if there are any other platforms that do not have /dev/tty (I know we do not set HAVE_DEV_TTY by default, but that is only because I was being conservative and waiting for people on particular platforms to confirm that it works before tweaking our Makefile defaults). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] allow recovery from command name typos
Jeff King writes: > - isatty(0) check in cmd_revert to set opts.edit automatically. This > one should match merge's behavior. > ... > So I think the only one that could be improved is the one in cmd_revert. Yeah, that matches the result of my grep. Thanks for sanity checking. > The credential code uses git_terminal_prompt, which actually opens > /dev/tty directly. So it is probably sane to use for your new prompt, > but it does not (and should not) rely on isatty. I think using git_terminal_prompt() after doing a looser "does the user sit at a terminal and is capable of answering interactive prompt" check with isatty(2) is OK, as long as we know that all implementations of git_terminal_prompt() never read from whatever happens to be connected to the standard input. The function falls back to getpass() on platforms without DEV_TTY, and if getpass() on some platforms reads from the standard input, that would be a disaster. I wasn't sure about that part. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] allow recovery from command name typos
Tay Ray Chuan writes: > On Thu, Jul 26, 2012 at 1:57 AM, Junio C Hamano wrote: >> >> Tay Ray Chuan writes: >> >> > If suggestions are available (based on Levenshtein distance) and if the >> > terminal isatty(), present a prompt to the user to select one of the >> > computed suggestions. >> >> The way to determine "If the terminal is a tty" used in this patch >> looks overly dangerous, given that we do not know what kind of "git" >> command we may be invoking at this point. > > Indeed, it should also have considered stdin's tty-ness. Not necessarily. As long as you call git_prompt(), which opens /dev/tty on its own and does not break even if its standard input is coming from elsewhere, you should be OK. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] allow recovery from command name typos
On Fri, Jul 27, 2012 at 01:08:34AM +0800, Tay Ray Chuan wrote: > > Perhaps we should audit "isatty()" calls and replace them with a > > helper function that does this kind of thing consistently in a more > > robust way (my recent favorite is Linus's somewhat anal logic used > > in builtin/merge.c::default_edit_option()). > > Any specific callers to isatty() you have in mind? A quick grep shows > that a significant portion of the "offenders" are isatty(2) calls to > determine whether to display progress, I think those are ok. Yeah, those are probably fine. Grep reveals that besides isatty(2) and the merge default_edit_option check, we have: - isatty(1) for checking auto-output munging, including auto-colors, auto-columns, and the pager. These are all fine, as they are not about interactivity, but specifically about whether stdout is a tty. - isatty(0) in commit.c to print a message when reading "-F -" from stdin. OK. - isatty(0) in pack-redundant to avoid reading stdin when it is a terminal (a questionable choice, perhaps, but not really something that would want a full interactivity check). - isatty(0) check in cmd_revert to set opts.edit automatically. This one should match merge's behavior. - isatty(0) in shortlog; this is a compatibility hack as shortlog traditionally accepted log output on stdin, but can now be used stand-alone. OK. So I think the only one that could be improved is the one in cmd_revert. > The credential helper has some prompting functionality that is close > to what I intend to do here, but I think it can make some assumptions > about stdin/stdout that we can't, as you have pointed out. So that > leaves merge-edit and this patch as the beneficiaries of a > builtin/merge.c::default_edit_option() refactor. That's just off the > top of my head. The credential code uses git_terminal_prompt, which actually opens /dev/tty directly. So it is probably sane to use for your new prompt, but it does not (and should not) rely on isatty. > Perhaps the helper function could be named "git_can_prompt()" and > placed in prompt.c? Please don't. The isatty() checks have nothing to do with whether git_prompt can run. The only thing such a git_can_prompt function should do is see if we can open /dev/tty. The isatty check in merge.c is more about "are we interactive, so that it is sane to run $EDITOR". -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] allow recovery from command name typos
On Thu, Jul 26, 2012 at 1:57 AM, Junio C Hamano wrote: > > Tay Ray Chuan writes: > > > If suggestions are available (based on Levenshtein distance) and if the > > terminal isatty(), present a prompt to the user to select one of the > > computed suggestions. > > The way to determine "If the terminal is a tty" used in this patch > looks overly dangerous, given that we do not know what kind of "git" > command we may be invoking at this point. Indeed, it should also have considered stdin's tty-ness. > Perhaps we should audit "isatty()" calls and replace them with a > helper function that does this kind of thing consistently in a more > robust way (my recent favorite is Linus's somewhat anal logic used > in builtin/merge.c::default_edit_option()). Any specific callers to isatty() you have in mind? A quick grep shows that a significant portion of the "offenders" are isatty(2) calls to determine whether to display progress, I think those are ok. The credential helper has some prompting functionality that is close to what I intend to do here, but I think it can make some assumptions about stdin/stdout that we can't, as you have pointed out. So that leaves merge-edit and this patch as the beneficiaries of a builtin/merge.c::default_edit_option() refactor. That's just off the top of my head. Perhaps the helper function could be named "git_can_prompt()" and placed in prompt.c? -- Cheers, Ray Chuan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] allow recovery from command name typos
Tay Ray Chuan writes:
> If suggestions are available (based on Levenshtein distance) and if the
> terminal isatty(), present a prompt to the user to select one of the
> computed suggestions.
The way to determine "If the terminal is a tty" used in this patch
looks overly dangerous, given that we do not know what kind of "git"
command we may be invoking at this point.
Perhaps we should audit "isatty()" calls and replace them with a
helper function that does this kind of thing consistently in a more
robust way (my recent favorite is Linus's somewhat anal logic used
in builtin/merge.c::default_edit_option()).
> +static int shall_advise = 1;
> +static int shall_prompt;
Naming "shall_foo" is a first here. It is not wrong per-se, but I
think we tend to call these "do we use/perform/etc X" do_X in our
codebase (see builtin/{config.c,fetch-pack.c,notes.c} for examples).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
