Re: git_getpass regression?
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 ;)
Erik Faye-Lund
