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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html