Re: git_getpass regression?
- From
Jeff King <peff@peff.net>
- Date
- Jun 30, 2012, 18:36 UTC
- Message-ID
- <20120630183607.GA19739@sigill.intra.peff.net>
- In-Reply-To
- <CABPQNSY3hJse6J7fDo0S5=ySZA4_7=JisfzLBUu135gR2nSF-w@mail.gmail.com>
On Sat, Jun 30, 2012 at 01:27:09PM +0200, Erik Faye-Lund wrote:
Show 9 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 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. :)
Show 5 quoted lines
> >> > 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?
Show 6 quoted lines
> >> > 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:
Show 15 quoted lines
> + 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.
Show 11 quoted lines
> + 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. :)
Show 6 quoted lines
> 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