git_getpass regression?

git_getpass regression?

[Erik Faye-Lund]

Since 9b4b894 ("Makefile: linux has /dev/tty", 2011-12-10) we're
reading input from the terminal using strbuf_getline instead of
getpass. But at least on my linux-box, getpass treats the
backspace-key ('\b') as an actual deletion. strbuf_getline obviously
shouldn't do this, as it's a utility function. But I think as a
user-interface feature, it would be much more pleasant to be allowed
to edit the entered text ;)

I can't find anything in POSIX that standardize this behavior, but for
most text-input use-cases it's probably what the user intended.

I guess this is technically a regression, but probably not a very important one.

I think we have multiple possible solutions:
 1) Read a character at the time, and special-case '\r' to erase the
previously entered character.
 2) Post-process the strbuf to explicitly perform the erasing.
 3) Do nothing.

I'm in favor of 2) because I'm a Windows-user, and we never had the
erasing-behavior to begin with. And it's a nice feature, so we could
make the post-processing a function that can be reusable by the
Windows-version of git_terminal_prompt. We could even use it on the
plain getpass-fallback, to unify the user-experience across platforms.

We can probably also be do 1) in a reusable form by adding some kind
of on_char-callback, but it's probably something that'd end up more
confusing than than the alternative.

Thoughts?

Re: git_getpass regression?

[Jeff King]

On Fri, Jun 29, 2012 at 12:06:31PM +0200, Erik Faye-Lund wrote:

> Since 9b4b894 ("Makefile: linux has /dev/tty", 2011-12-10) we're
> reading input from the terminal using strbuf_getline instead of
> getpass. But at least on my linux-box, getpass treats the
> backspace-key ('\b') as an actual deletion. strbuf_getline obviously
> shouldn't do this, as it's a utility function. But I think as a
> user-interface feature, it would be much more pleasant to be allowed
> to edit the entered text ;)
> 
> I can't find anything in POSIX that standardize this behavior, but for
> most text-input use-cases it's probably what the user intended.

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. It works just fine for me here, both on the linux console and
in an xterm. What terminal are you using? Are you ssh-ing to a remote
linux box? If that is the case, I wonder if your 'stty erase' settings
do not match what your terminal emulator is sending.

If you run "stty erase ^H" and then run git, does it work?

> I think we have multiple possible solutions:
>  1) Read a character at the time, and special-case '\r' to erase the
> previously entered character.
>  2) Post-process the strbuf to explicitly perform the erasing.
>  3) Do nothing.

I assume you meant '\b' in (1).  I think (3) is the only sane thing,
though. Even if we handled \b, that is not what all terminals generate.
In particular, most linux terminal emulators these days will generate
DEL (aka ^? or 0x7f) on backspace. This problem needs to be solved at
the terminal layer, and I suspect is just a configuration issue in your
setup.

-Peff

Re: git_getpass regression?

[Jeff King]

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.

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

> > 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 think (3) is the only sane thing,
> > though. Even if we handled \b, that is not what all terminals generate.
> > In particular, most linux terminal emulators these days will generate
> > DEL (aka ^? or 0x7f) on backspace. This problem needs to be solved at
> > the terminal layer, and I suspect is just a configuration issue in your
> > setup.
>
> 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).

-Peff

Re: git_getpass regression?

[Erik Faye-Lund]

On Fri, Jun 29, 2012 at 10:30 PM, Jeff King <peff@peff.net> wrote:
> 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.

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

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

Re: git_getpass regression?

[Jeff King]

On Sat, Jun 30, 2012 at 01:27:09PM +0200, Erik Faye-Lund wrote:

> > 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 think this is one of those places where unix people like me take it
for granted that the world works one way, but it is completely foreign
for people coming from other systems. :)

> >> > If you run "stty erase ^H" and then run git, does it work?
> [...]
> 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.

OK. Then that implies your terminal is sending ^?, which is reasonable.
So I take it that everything is now working on your Linux box? Did you
figure out what caused the problems before?

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

Yeah, that makes sense. The idea of abstracting git_terminal_prompt was
to do whatever would make the most sense for user input on the given
platform; I just had no idea how to do that on Windows.

> So perhaps we should do something like this?

That looks like a good direction overall. I'm not really qualified to
comment on the Windows-specific bits, of course, but:

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

The HAVE_DEV_TTY version takes care to reset this on signal death or
premature exit (so if you ^C out of the program, your terminal is not
left in a funny state). You might want to do something similar here.

> +	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);

This will read random memory if buf.len == 0 (no clue if that can ever
happen on Windows, but it's nice to be defensive).

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

I'd be OK with leaving them separate, too. The shared parts are small
enough (and are mostly in strbuf_getline) that I think it might end up
less readable. But I'd withhold my judgement until seeing how good or
bad the refactored version looked. :)

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

No problem at all. Progress usually starts with somebody wondering
whether and how something is broken.

-Peff

Re: git_getpass regression?

[Erik Faye-Lund]

On Sat, Jun 30, 2012 at 10:15 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> On Sat, Jun 30, 2012 at 8:36 PM, Jeff King <peff@peff.net> wrote:
>> On Sat, Jun 30, 2012 at 01:27:09PM +0200, Erik Faye-Lund wrote:
>>> +     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;
>>> +             }
>>
>> The HAVE_DEV_TTY version takes care to reset this on signal death or
>> premature exit (so if you ^C out of the program, your terminal is not
>> left in a funny state). You might want to do something similar here.
>>
>
> Good point. I'll look into adding a Ctrl+C handler routine that cleans up.
>

Actually, it seems the shells (at least CMD.exe and MSYS bash) sets
the console mode to something sane when the process exits, so in
reality this doesn't seem to matter much. But I'd like to play nice
with the system if I can.

Adding a Control-handler that simply restores the console-mode only
made it behave slightly worse for me. A control-handler in Windows
runs from a separate thread *without halting the other threads*.
Combine that with the fact that the IO-call returns EOF when Ctrl+C is
entered, the result is that the calling thread and the Control-handler
thread race to restore the console-mode. The result is a correctly
restored terminal, but with a sporadous "could not read"-error based
on who wins the race.

But there are more gotchas: Ctrl+C is sent to all processes that share
the console on Windows. And it seems that Bash actually terminate it's
child-process when it receive Ctrl+C! This can happen *before* we even
get the Ctrl+C handler in our process, preventing us from restoring
the console mode. Luckily this doesn't seem to cause any harm, because
Bash seems to set a sane console mode itself.

Is there some other way of getting EOF from the console than Ctrl+C?
If not, perhaps we can disable the Ctrl+C handling altogether for the
current process, and restore the console mode on EOF? That only leaves
the "Bash kills our process"-case, but Bash seems to set a sane
console mode anyway.

Re: git_getpass regression?

[Jeff King]

On Tue, Jul 03, 2012 at 06:28:11PM +0200, Erik Faye-Lund wrote:

> Is there some other way of getting EOF from the console than Ctrl+C?
> If not, perhaps we can disable the Ctrl+C handling altogether for the
> current process, and restore the console mode on EOF? That only leaves
> the "Bash kills our process"-case, but Bash seems to set a sane
> console mode anyway.

On unix systems, you can generally send EOF on a terminal using Ctrl+D
(and strbuf_getline should handle it properly).  I have no clue if that
works on a Windows console, though. Also, I wonder if these kind of
terminal issues are different based on the terminal emulator you are
using (e.g., msys bash window versus something like pterm).

-Peff

Re: git_getpass regression?

[Erik Faye-Lund]

On Tue, Jul 3, 2012 at 7:11 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jul 03, 2012 at 06:28:11PM +0200, Erik Faye-Lund wrote:
>
>> Is there some other way of getting EOF from the console than Ctrl+C?
>> If not, perhaps we can disable the Ctrl+C handling altogether for the
>> current process, and restore the console mode on EOF? That only leaves
>> the "Bash kills our process"-case, but Bash seems to set a sane
>> console mode anyway.
>
> On unix systems, you can generally send EOF on a terminal using Ctrl+D
> (and strbuf_getline should handle it properly).  I have no clue if that
> works on a Windows console, though.

Nope. On Windows, Ctrl+D seens to give EOT (0x4).

> Also, I wonder if these kind of
> terminal issues are different based on the terminal emulator you are
> using (e.g., msys bash window versus something like pterm).

No, they should not. Windows doesn't have a concept of different
terminal emulators. There's only ever one, which is built into
windows.

Re: git_getpass regression?

[Junio C Hamano]

Ping on seemingly stalled discussion.

Re: git_getpass regression?

[Erik Faye-Lund]

On Wed, Jul 18, 2012 at 7:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ping on seemingly stalled discussion.

Just to recap quickly from my point of view:
1. There's no regression.
2. There's room for improvement of the prompting on Windows.
3. I'm not entirely confident that I've found a safe way of improving
the Windows prompt yet.
4. I've been busy with other stuff, but I'll get back to hacking soon :)