Re: [PATCH v2 4/4] allow recovery from command name typos

From: Jeff King <peff@peff.net>
Date: Jul 26, 2012, 18:37 UTC
Message-ID: <20120726183734.GA16037@sigill.intra.peff.net>
In-Reply-To: <7vy5m67694.fsf@alter.siamese.dyndns.org>

On Thu, Jul 26, 2012 at 10:59:51AM -0700, Junio C Hamano wrote:

Show 9 quoted lines

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