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

From: Jeff King <peff@peff.net>
Date: Dec 15, 2011, 13:39 UTC
Message-ID: <20111215133939.GA2241@sigill.intra.peff.net>
In-Reply-To: <20111215124851.GA6907@padd.com>

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

Show 13 quoted lines

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

Show 21 quoted lines

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

Show 6 quoted lines

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

On error, disable it again.

Show 14 quoted lines

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