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:

quoted

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

quoted

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

quoted

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

We set up term_fd, and then:

quoted

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

On error, disable it again.

quoted

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