Re: git_getpass regression?

From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Jun 30, 2012, 11:27 UTC
Message-ID: <CABPQNSY3hJse6J7fDo0S5=ySZA4_7=JisfzLBUu135gR2nSF-w@mail.gmail.com>
In-Reply-To: <20120629203001.GA12937@sigill.intra.peff.net>

On Fri, Jun 29, 2012 at 10:30 PM, Jeff King <peff@peff.net> wrote:

Show 23 quoted lines

> On Fri, Jun 29, 2012 at 10:14:35PM +0200, Erik Faye-Lund wrote:
>
>> > Backspace shouldn't be making it to git at all; it should be handled at
>> > the terminal input layer, because we are not putting the terminal into
>> > raw mode.
>>
>> I don't get it. How can the terminal ever interpret the backspace, when
>> we've already put the character it's supposed to erase in a strbuf? Sure,
>> the echo can be correctly dealt with (probably with the exception of the
>> border case of erasing characters from the prompt text), but once we've put
>> the characters into a buffer, the terminal cannot erase it.
>
> Because a terminal in non-raw mode is typically line-oriented, and your
> program doesn't get to see any input at all until the user hits enter.
>
> Try this on your linux box:
>
>  strace -e read cat
>
> You should be able to type characters and backspace erase them, all
> without seeing any activity from strace. When you hit enter, you should
> see the full text you typed read as a single syscall.
>

Thanks for the explanation.

Show 5 quoted lines

> If the backspacing doesn't work, then there is a configuration mismatch
> between what linux's tty driver expects and what your terminal emulator
> is sending (the former is probably expecting ^? as the erase character,
> and the latter is sending ^H).
>

The only thing I actually tested, was that getpass behaved correctly. I just assumed fgetc returned the input right away. I had no idea that fgetc didn't fetch characters until a whole line was entered. I guess you learn something every day ;)

I have now written a small test-application that confirms that the prompt works just fine; I was making conclusions without testing properly, my bad. So I'm sorry for the noise. But please read on ;)

Show 14 quoted lines

>> > If that is the case, I wonder if your 'stty erase' settings
>> > do not match what your terminal emulator is sending.
>>
>> I have no idea what that even means. And having to fiddle around with
>> terminal settings just makes git feel cheap.
>
> But it's not git that is broken. It's your configuration. Fixing git is
> a band-aid, and other programs will still be broken. Of course, I may be
> mis-diagnosing your problem. Did you try this:
>
>> > If you run "stty erase ^H" and then run git, does it work?
>
> ?
>

I'm sorry, I didn't have access to a box at that time, that's why I didn't answer that part.

But now, I see that "stty erase ^H" makes the terminal behave all strange when it comes to the backspace character; it starts printing "^?" instead.

>> >  I think (3) is the only sane thing,
>> > though.

Yes, you are right. But perhaps with one exception: we probably want to piggy-back on those terminal-handling goodies on Windows. If only to get git to behave more consistently with other applications.

So perhaps we should do something like this?

---8<---

diff --git a/compat/terminal.c b/compat/terminal.c
index 53c5166..2f44fb5 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -76,18 +76,58 @@ char *git_terminal_prompt(const char *prompt, int echo)
 char *git_terminal_prompt(const char *prompt, int echo)
 {
 	static struct strbuf buf = STRBUF_INIT;
+	int r;
+	FILE *input_fh, *output_fh;
+	DWORD cmode = 0;
+	HANDLE hconin;
+
+	input_fh = fopen("CONIN$", "r");
+	if (!input_fh)
+		return NULL;
+
+	output_fh = fopen("CONOUT$", "w");
+	if (!output_fh) {
+		fclose(input_fh);
+		return NULL;
+	}

-	fputs(prompt, stderr);
-	strbuf_reset(&buf);
-	for (;;) {
-		int c = _getch();
-		if (c == '\n' || c == '\r')
-			break;
-		if (echo)
-			putc(c, stderr);
-		strbuf_addch(&buf, c);
+	if (!echo) {
+		hconin = CreateFile("CONIN$", GENERIC_READ | GENERIC_WRITE,
+		    FILE_SHARE_READ, NULL, OPEN_EXISTING,
+		    FILE_ATTRIBUTE_NORMAL, NULL);
+		if (hconin == INVALID_HANDLE_VALUE) {
+			fclose(input_fh);
+			fclose(output_fh);
+			return NULL;
+		}
+		GetConsoleMode(hconin, &cmode);
+		if (!SetConsoleMode(hconin, cmode & (~ENABLE_ECHO_INPUT))) {
+			fclose(input_fh);
+			fclose(output_fh);
+			return NULL;
+		}
 	}
-	putc('\n', stderr);
+
+	fputs(prompt, output_fh);
+	fflush(output_fh);
+
+	r = strbuf_getline(&buf, input_fh, '\n');
+	if (!echo) {
+		putc('\n', output_fh);
+		fflush(output_fh);
+
+		SetConsoleMode(hconin, cmode);
+		CloseHandle(hconin);
+	}
+
+	if (buf.buf[buf.len - 1] == '\r')
+		strbuf_setlen(&buf, buf.len - 1);
+
+	fclose(input_fh);
+	fclose(output_fh);
+
+	if (r == EOF)
+		return NULL;
 	return buf.buf;
 }

---8<---

Notice how this looks very similar to the HAVE_TTY code-path, with the
exception of needing two file handles instead of one, and the actual
enabling/resetting of the prompt-setting. So the code-paths can
probably be refactored to share code...

>> As I already said, I don't think this is the case. Inserting 0x7f in a
>> strbuf does not delete the preceding char...
>
> Of course not. Control characters should be handled by the terminal
> driver, and shouldn't make it to git at all. Backspacing works perfectly
> well on correctly configured systems, and it should not be git's
> responsibility to care about it at all for line-oriented input (and if
> we _did_ want to handle it ourselves, we should use a real terminal
> library like readline or curses).

Yes, I absolutely agree. Sorry for getting confused and wasting your
time with unfounded accusations.

But perhaps something good came from it; the Windows-prompt doesn't
support erasing until the patch above is applied. I don't know if you
care or not, but I certainly do ;)