Re: [PATCHv2 5/9] add generic terminal prompt function

[Jeff King <peff@peff.net>]

On Thu, Dec 15, 2011 at 07:48:51AM -0500, Pete Wyckoff wrote:

> peff@peff.net wrote on Sat, 10 Dec 2011 05:41 -0500:
> > +static struct termios old_term;
> > +
> > +static void restore_term(void)
> > +{
> > +	if (term_fd < 0)
> > +		return;
> > +
> > +	tcsetattr(term_fd, TCSAFLUSH, &old_term);
> > +	term_fd = -1;
> > +}
> 
> Restores from static old_term.

Right. But note that it is protected by term_fd being set.

> > +char *git_terminal_prompt(const char *prompt, int echo)
> > +{
> > +	static struct strbuf buf = STRBUF_INIT;
> > +	int r;
> > +	FILE *fh;
> > +
> > +	fh = fopen("/dev/tty", "w+");
> > +	if (!fh)
> > +		return NULL;
> > +
> > +	if (!echo) {
> > +		struct termios t;
> > +
> > +		if (tcgetattr(fileno(fh), &t) < 0) {
> > +			fclose(fh);
> > +			return NULL;
> > +		}
> > +
> > +		old_term = t;
> 
> Which is only saved if echo is true.

Yes, but just below:

> > +		term_fd = fileno(fh);
> > +		sigchain_push_common(restore_term_on_signal);

We set up term_fd, and then:

> > +		t.c_lflag &= ~ECHO;
> > +		if (tcsetattr(fileno(fh), TCSAFLUSH, &t) < 0) {
> > +			term_fd = -1;
> > +			fclose(fh);
> > +			return NULL;
> > +		}

On error, disable it again.

> > +	}
> > +
> > +	fputs(prompt, fh);
> > +	fflush(fh);
> > +
> > +	r = strbuf_getline(&buf, fh, '\n');
> > +	if (!echo) {
> > +		putc('\n', fh);
> > +		fflush(fh);
> > +	}
> > +
> > +	restore_term();
> 
> Perhaps this line should go in !echo.

It could, but it's a no-op as-is, as term_fd will be -1.

I agree it might be a little more obvious to put it there (I think what
happened is my initial revision did not look at "echo" ever again, and
then that conditional was added later when I realized that the "!echo"
case needed us to print the newline manually).

> And why no sigchain_pop() for the signal handler?

Because I used sigchain_push_common, which has no pop_common analog. But
it's OK, because calling restore_term sets term_fd to -1, making further
calls a no-op. So leaving the handler in place is fine.

Another option would be to add sigchain_pop_common, which pops the
same signals from push_common.

-Peff