[PATCH 0/5] win32: support echo for terminal-prompt

[Erik Faye-Lund]

We currently only support getpass, which does not echo at all, for
git_terminal_prompt on Windows. The Windows console is perfectly
capable of doing this, so let's make it so.

This implementation tries to reuse the /dev/tty-code as much as
possible.

The big reason that this becomes a bit hairy is that Ctrl+C needs
to be handled correctly, so we don't leak the console state to a
non-echoing setting when a user aborts.

Windows makes this bit a little bit tricky, in that we need to
implement SIGINT for fgetc. However, I suspect that this is a good
thing to do in the first place.

An earlier iteration was also breifly discussed here:
http://mid.gmane.org/CABPQNSaUCEDU4+2N63n0k_Xw...@mail.gmail.com

The series can also be found here, only with an extra patch that
makes the (interactive) testing a bit easier:

https://github.com/kusma/git/tree/work/terminal-cleanup

Erik Faye-Lund (5):
mingw: make fgetc raise SIGINT if apropriate
compat/terminal: factor out echo-disabling
compat/terminal: separate input and output handles
mingw: reuse tty-version of git_terminal_prompt
mingw: get rid of getpass implementation

compat/mingw.c | 91 +++++++++++++++++++++++++++-----------
compat/mingw.h | 8 +++-
compat/terminal.c | 129 ++++++++++++++++++++++++++++++++++++++++--------------
3 files changed, 169 insertions(+), 59 deletions(-)

--
1.8.0.7.gbeffeda

[Erik Faye-Lund]

[Erik Faye-Lund]

Set a control-handler to prevent the process from terminating, and
simulate SIGINT so it can be handled by a signal-handler as usual.

Signed-off-by: Erik Faye-Lund <kusm...@gmail.com>
---
compat/mingw.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
compat/mingw.h | 6 +++++
2 files changed, 72 insertions(+), 10 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 78e8f54..33ddfdf 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -319,6 +319,31 @@ ssize_t mingw_write(int fd, const void *buf, size_t count)
return write(fd, buf, min(count, 31 * 1024 * 1024));
}

+static BOOL WINAPI ctrl_ignore(DWORD type)
+{
+ return TRUE;
+}
+
+#undef fgetc
+int mingw_fgetc(FILE *stream)
+{
+ int ch;
+ if (!isatty(_fileno(stream)))
+ return fgetc(stream);
+
+ SetConsoleCtrlHandler(ctrl_ignore, TRUE);
+ while (1) {
+ ch = fgetc(stream);
+ if (ch != EOF || GetLastError() != ERROR_OPERATION_ABORTED)
+ break;
+
+ /* Ctrl+C was pressed, simulate SIGINT and retry */
+ mingw_raise(SIGINT);
+ }
+ SetConsoleCtrlHandler(ctrl_ignore, FALSE);
+ return ch;
+}
+
#undef fopen
FILE *mingw_fopen (const char *filename, const char *otype)
{
@@ -1524,7 +1549,7 @@ static HANDLE timer_event;
static HANDLE timer_thread;
static int timer_interval;
static int one_shot;
-static sig_handler_t timer_fn = SIG_DFL;
+static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL;

/* The timer works like this:
* The thread, ticktack(), is a trivial routine that most of the time
@@ -1538,13 +1563,7 @@ static sig_handler_t timer_fn = SIG_DFL;
static unsigned __stdcall ticktack(void *dummy)
{
while (WaitForSingleObject(timer_event, timer_interval) == WAIT_TIMEOUT) {
- if (timer_fn == SIG_DFL) {
- if (isatty(STDERR_FILENO))
- fputs("Alarm clock\n", stderr);
- exit(128 + SIGALRM);
- }
- if (timer_fn != SIG_IGN)
- timer_fn(SIGALRM);
+ mingw_raise(SIGALRM);
if (one_shot)
break;
}
@@ -1635,12 +1654,49 @@ int sigaction(int sig, struct sigaction *in, struct sigaction *out)
sig_handler_t mingw_signal(int sig, sig_handler_t handler)
{
sig_handler_t old = timer_fn;
- if (sig != SIGALRM)
+
+ switch (sig) {
+ case SIGALRM:
+ timer_fn = handler;
+ break;
+
+ case SIGINT:
+ sigint_fn = handler;
+ break;
+
+ default:
return signal(sig, handler);
- timer_fn = handler;
+ }
+
return old;
}

+#undef raise
+int mingw_raise(int sig)
+{
+ switch (sig) {
+ case SIGALRM:
+ if (timer_fn == SIG_DFL) {
+ if (isatty(STDERR_FILENO))
+ fputs("Alarm clock\n", stderr);
+ exit(128 + SIGALRM);
+ } else if (timer_fn != SIG_IGN)
+ timer_fn(SIGALRM);
+ return 0;
+
+ case SIGINT:
+ if (sigint_fn == SIG_DFL)
+ exit(128 + SIGINT);
+ else if (sigint_fn != SIG_IGN)
+ sigint_fn(SIGINT);
+ return 0;
+
+ default:
+ return raise(sig);
+ }
+}
+
+
static const char *make_backslash_path(const char *path)
{
static char buf[PATH_MAX + 1];
diff --git a/compat/mingw.h b/compat/mingw.h
index 61a6521..6b9e69a 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -179,6 +179,9 @@ int mingw_open (const char *filename, int oflags, ...);
ssize_t mingw_write(int fd, const void *buf, size_t count);
#define write mingw_write

+int mingw_fgetc(FILE *stream);
+#define fgetc mingw_fgetc
+
FILE *mingw_fopen (const char *filename, const char *otype);
#define fopen mingw_fopen

@@ -287,6 +290,9 @@ static inline unsigned int git_ntohl(unsigned int x)
sig_handler_t mingw_signal(int sig, sig_handler_t handler);
#define signal mingw_signal

+int mingw_raise(int sig);
+#define raise mingw_raise
+
/*
* ANSI emulation wrappers
*/
--
1.8.0.7.gbeffeda

[Erik Faye-Lund]

By moving the echo-disabling code to a separate function, we can
implement OS-specific versions of it for non-POSIX platforms.

Signed-off-by: Erik Faye-Lund <kusm...@gmail.com>
---

compat/terminal.c | 43 +++++++++++++++++++++++++------------------
1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index bbb038d..3217838 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -14,6 +14,7 @@ static void restore_term(void)
return;

tcsetattr(term_fd, TCSAFLUSH, &old_term);
+ close(term_fd);
term_fd = -1;
}

@@ -24,6 +25,27 @@ static void restore_term_on_signal(int sig)
raise(sig);
}

+static int disable_echo()
+{
+ struct termios t;
+
+ term_fd = open("/dev/tty", O_RDWR);
+ if (tcgetattr(term_fd, &t) < 0)
+ goto error;
+
+ old_term = t;
+ sigchain_push_common(restore_term_on_signal);
+
+ t.c_lflag &= ~ECHO;
+ if (!tcsetattr(term_fd, TCSAFLUSH, &t))
+ return 0;
+
+error:
+ close(term_fd);
+ term_fd = -1;
+ return -1;
+}
+
char *git_terminal_prompt(const char *prompt, int echo)
{
static struct strbuf buf = STRBUF_INIT;
@@ -34,24 +56,9 @@ char *git_terminal_prompt(const char *prompt, int echo)
if (!fh)
return NULL;

- if (!echo) {
- struct termios t;
-
- if (tcgetattr(fileno(fh), &t) < 0) {
- fclose(fh);
- return NULL;
- }
-
- old_term = t;
- term_fd = fileno(fh);
- sigchain_push_common(restore_term_on_signal);
-
- t.c_lflag &= ~ECHO;
- if (tcsetattr(fileno(fh), TCSAFLUSH, &t) < 0) {
- term_fd = -1;
- fclose(fh);
- return NULL;
- }
+ if (!echo && disable_echo()) {
+ fclose(fh);
+ return NULL;
}

fputs(prompt, fh);
--
1.8.0.7.gbeffeda

[Erik Faye-Lund]

On Windows, the terminal cannot be opened in read-write mode, so
we need distinct pairs for reading and writing. Since this works
fine on other platforms as well, always open them in pairs.

Signed-off-by: Erik Faye-Lund <kusm...@gmail.com>
---

compat/terminal.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 3217838..4a1fd3d 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -50,29 +50,36 @@ char *git_terminal_prompt(const char *prompt, int echo)

{
static struct strbuf buf = STRBUF_INIT;

int r;
- FILE *fh;
+ FILE *input_fh, *output_fh;

- fh = fopen("/dev/tty", "w+");
- if (!fh)
+ input_fh = fopen("/dev/tty", "r");
+ if (!input_fh)
return NULL;

+ output_fh = fopen("/dev/tty", "w");
+ if (!output_fh) {
+ fclose(input_fh);
+ return NULL;
+ }

+
if (!echo && disable_echo()) {

- fclose(fh);
+ fclose(input_fh);
+ fclose(output_fh);
return NULL;
}

- fputs(prompt, fh);
- fflush(fh);
+ fputs(prompt, output_fh);
+ fflush(output_fh);

- r = strbuf_getline(&buf, fh, '\n');
+ r = strbuf_getline(&buf, input_fh, '\n');
if (!echo) {
- fseek(fh, SEEK_CUR, 0);
- putc('\n', fh);
- fflush(fh);
+ putc('\n', output_fh);
+ fflush(output_fh);
}

restore_term();
- fclose(fh);
+ fclose(input_fh);
+ fclose(output_fh);

if (r == EOF)
return NULL;
--
1.8.0.7.gbeffeda

[Erik Faye-Lund]

The getpass-implementation we use on Windows isn't at all ideal;
it works in raw-mode (as opposed to cooked mode), and as a result
does not deal correcly with deletion, arrow-keys etc.

Instead, use cooked mode to read a line at the time, allowing the
C run-time to process the input properly.

Since we set files to be opened in binary-mode by default on
Windows, introduce a FORCE_TEXT macro that expands to the "t"
modifier that forces the terminal to be opened in text-mode so we
do not have to deal with CRLF issues.

Signed-off-by: Erik Faye-Lund <kusm...@gmail.com>
---

compat/terminal.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 60 insertions(+), 9 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 4a1fd3d..ce0fbd9 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -3,8 +3,22 @@
#include "sigchain.h"
#include "strbuf.h"

+#if defined(HAVE_DEV_TTY) || defined(WIN32)
+
+static void restore_term(void);
+
+static void restore_term_on_signal(int sig)
+{
+ restore_term();
+ sigchain_pop(sig);
+ raise(sig);
+}
+
#ifdef HAVE_DEV_TTY

+#define INPUT_PATH "/dev/tty"
+#define OUTPUT_PATH "/dev/tty"
+
static int term_fd = -1;
static struct termios old_term;

@@ -18,13 +32,6 @@ static void restore_term(void)
term_fd = -1;
}

-static void restore_term_on_signal(int sig)
-{
- restore_term();
- sigchain_pop(sig);
- raise(sig);
-}
-
static int disable_echo()
{
struct termios t;
@@ -46,17 +53,61 @@ error:
return -1;
}

+#elif defined(WIN32)
+
+#define INPUT_PATH "CONIN$"
+#define OUTPUT_PATH "CONOUT$"
+#define FORCE_TEXT "t"
+
+static HANDLE hconin = INVALID_HANDLE_VALUE;
+static DWORD cmode;
+
+static void restore_term(void)
+{
+ if (hconin == INVALID_HANDLE_VALUE)
+ return;
+
+ SetConsoleMode(hconin, cmode);
+ CloseHandle(hconin);
+ hconin = INVALID_HANDLE_VALUE;
+}
+
+static int disable_echo(void)
+{
+ hconin = CreateFile("CONIN$", GENERIC_READ | GENERIC_WRITE,
+ FILE_SHARE_READ, NULL, OPEN_EXISTING,
+ FILE_ATTRIBUTE_NORMAL, NULL);
+ if (hconin == INVALID_HANDLE_VALUE)
+ return -1;
+
+ GetConsoleMode(hconin, &cmode);
+ sigchain_push_common(restore_term_on_signal);
+ if (!SetConsoleMode(hconin, cmode & (~ENABLE_ECHO_INPUT))) {
+ CloseHandle(hconin);
+ hconin = INVALID_HANDLE_VALUE;

+ return -1;
+ }
+

+ return 0;
+}
+

+#endif
+
+#ifndef FORCE_TEXT
+#define FORCE_TEXT
+#endif
+

char *git_terminal_prompt(const char *prompt, int echo)
{
static struct strbuf buf = STRBUF_INIT;
int r;

FILE *input_fh, *output_fh;

- input_fh = fopen("/dev/tty", "r");
+ input_fh = fopen(INPUT_PATH, "r" FORCE_TEXT);
if (!input_fh)
return NULL;

- output_fh = fopen("/dev/tty", "w");
+ output_fh = fopen(OUTPUT_PATH, "w" FORCE_TEXT);
if (!output_fh) {
fclose(input_fh);
return NULL;
--
1.8.0.7.gbeffeda

[Erik Faye-Lund]

There's no remaining call-sites, and as pointed out in the
previous commit message, it's not quite ideal. So let's just
lose it.

Signed-off-by: Erik Faye-Lund <kusm...@gmail.com>
---

compat/mingw.c | 15 ---------------
compat/mingw.h | 2 --
2 files changed, 17 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 33ddfdf..5fc14b7 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1758,21 +1758,6 @@ int link(const char *oldpath, const char *newpath)
return 0;
}

-char *getpass(const char *prompt)
-{
- struct strbuf buf = STRBUF_INIT;
-
- fputs(prompt, stderr);
- for (;;) {
- char c = _getch();
- if (c == '\r' || c == '\n')
- break;
- strbuf_addch(&buf, c);
- }
- fputs("\n", stderr);
- return strbuf_detach(&buf, NULL);
-}
-
pid_t waitpid(pid_t pid, int *status, int options)
{
HANDLE h = OpenProcess(SYNCHRONIZE | PROCESS_QUERY_INFORMATION,
diff --git a/compat/mingw.h b/compat/mingw.h
index 6b9e69a..f494ecb 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -55,8 +55,6 @@ struct passwd {
char *pw_dir;
};

-extern char *getpass(const char *prompt);
-
typedef void (__cdecl *sig_handler_t)(int);
struct sigaction {
sig_handler_t sa_handler;
--
1.8.0.7.gbeffeda

[Erik Faye-Lund]

Sorry, I messed up the subject (lacking RFC-prefix), so I aborted
after sending the cover-letter. I'll resend with a proper prefix right
away.

[Erik Faye-Lund]

Ping?

[Johannes Schindelin]

Hi,

On Tue, 13 Nov 2012, Erik Faye-Lund wrote:

> Set a control-handler to prevent the process from terminating, and
> simulate SIGINT so it can be handled by a signal-handler as usual.

One thing you might want to mention is that the fgetc() handling is not
thread-safe, and intentionally so: if two threads read from the same
console, we are in trouble anyway.

BTW I like the new mingw_raise() very much!

Ciao,
Dscho

[Johannes Schindelin]

Hi,

On Tue, 13 Nov 2012, Erik Faye-Lund wrote:

> By moving the echo-disabling code to a separate function, we can
> implement OS-specific versions of it for non-POSIX platforms.
>
> Signed-off-by: Erik Faye-Lund <kusm...@gmail.com>
> ---
> compat/terminal.c | 43 +++++++++++++++++++++++++------------------
> 1 file changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/compat/terminal.c b/compat/terminal.c
> index bbb038d..3217838 100644
> --- a/compat/terminal.c
> +++ b/compat/terminal.c
> @@ -14,6 +14,7 @@ static void restore_term(void)
> return;
>
> tcsetattr(term_fd, TCSAFLUSH, &old_term);
> + close(term_fd);
> term_fd = -1;
> }

That looks like an independent resource leak fix... correct?

Rest looks awsomely fine.
Dscho

[Johannes Schindelin]

Hi kusma,

On Tue, 13 Nov 2012, Erik Faye-Lund wrote:

> The getpass-implementation we use on Windows isn't at all ideal;
> it works in raw-mode (as opposed to cooked mode), and as a result
> does not deal correcly with deletion, arrow-keys etc.
>
> Instead, use cooked mode to read a line at the time, allowing the
> C run-time to process the input properly.

Awesome!

The patch itself has a couple Windows-specific things in compat/terminal.c
that I would have loved to see in compat/mingw.c instead, but it looks as
if we have no choice: restore_term() and disable_echo() need to be
substantially different enough from the implementation in compat/mingw.c.

(Read: I am fine with this patch.)

Ciao,
Dscho

[Johannes Schindelin]

Hi kusma,

On Tue, 13 Nov 2012, Erik Faye-Lund wrote:

> There's no remaining call-sites, and as pointed out in the
> previous commit message, it's not quite ideal. So let's just
> lose it.

Awesome!
Dscho

[Jeff King]

On Fri, Nov 30, 2012 at 06:58:11PM +0100, Johannes Schindelin wrote:

> Hi,
>
> On Tue, 13 Nov 2012, Erik Faye-Lund wrote:
>
> > Set a control-handler to prevent the process from terminating, and
> > simulate SIGINT so it can be handled by a signal-handler as usual.
>
> One thing you might want to mention is that the fgetc() handling is not
> thread-safe, and intentionally so: if two threads read from the same
> console, we are in trouble anyway.

That makes sense to me, but I'm confused why it is part of mingw_fgetc,
which could in theory read from arbitrary streams, no? It it is not
necessarily a console operation at all. I feel like I'm probably missing
something subtle here...

-Peff

[Jeff King]

On Fri, Nov 30, 2012 at 06:59:30PM +0100, Johannes Schindelin wrote:

> > diff --git a/compat/terminal.c b/compat/terminal.c
> > index bbb038d..3217838 100644
> > --- a/compat/terminal.c
> > +++ b/compat/terminal.c
> > @@ -14,6 +14,7 @@ static void restore_term(void)
> > return;
> >
> > tcsetattr(term_fd, TCSAFLUSH, &old_term);
> > + close(term_fd);
> > term_fd = -1;
> > }
>
> That looks like an independent resource leak fix... correct?

I don't think so. In the current code, term_fd does not take ownership
of the fd. It is fundamentally part of the FILE* in git_terminal_prompt,
and is closed when we fclose() that. But in Erik's refactor, we actually
open a _second_ descriptor to /dev/tty, which needs to be closed.

I don't think there is any reason that should not work (the terminal
characteristics should be per-tty, not per-descriptor), though it does
feel a little hacky to have to open /dev/tty twice.

-Peff

[Jeff King]

On Tue, Nov 13, 2012 at 03:04:05PM +0100, Erik Faye-Lund wrote:

> On Windows, the terminal cannot be opened in read-write mode, so
> we need distinct pairs for reading and writing. Since this works
> fine on other platforms as well, always open them in pairs.

Looks OK. We're now opening /dev/tty three separate times in the no-echo
case, but it's not like this is in a performance critical loop.

-Peff

[Jeff King]

On Tue, Nov 13, 2012 at 03:04:06PM +0100, Erik Faye-Lund wrote:

> The getpass-implementation we use on Windows isn't at all ideal;
> it works in raw-mode (as opposed to cooked mode), and as a result
> does not deal correcly with deletion, arrow-keys etc.
>
> Instead, use cooked mode to read a line at the time, allowing the
> C run-time to process the input properly.
>
> Since we set files to be opened in binary-mode by default on
> Windows, introduce a FORCE_TEXT macro that expands to the "t"
> modifier that forces the terminal to be opened in text-mode so we
> do not have to deal with CRLF issues.

I think this is OK. I had originally envisioned just having a separate
"#ifdef WIN32" and not really sharing code at all with /dev/tty because
I was worried that the conditionals would end up making it hard to read.
This is a little more complex than I would have liked, but I do not see
how the code sharing could be simplified any more than what you have
done, and it is nice to avoid repeating ourselves.

-Peff

[Jeff King]

On Tue, Nov 13, 2012 at 03:04:07PM +0100, Erik Faye-Lund wrote:

> There's no remaining call-sites, and as pointed out in the
> previous commit message, it's not quite ideal. So let's just
> lose it.
>
> Signed-off-by: Erik Faye-Lund <kusm...@gmail.com>
> ---
> compat/mingw.c | 15 ---------------
> compat/mingw.h | 2 --
> 2 files changed, 17 deletions(-)

Yay!

-Peff

[Jeff King]

On Fri, Nov 30, 2012 at 11:16:59AM +0100, Erik Faye-Lund wrote:

> Ping?

Thanks for the reminder; your initial series came while I was traveling.

I think it looks good. The compat/terminal code ends up a little uglier,
but I think you overall did a good job of balancing code reuse across
platforms with readability.

-Peff

[Erik Faye-Lund]

I did add an early out for the non-console cases. Is this what you're
missing, perhaps?

[Erik Faye-Lund]

On Fri, Nov 30, 2012 at 6:58 PM, Johannes Schindelin
<Johannes....@gmx.de> wrote:
> Hi,
>
> On Tue, 13 Nov 2012, Erik Faye-Lund wrote:
>
>> Set a control-handler to prevent the process from terminating, and
>> simulate SIGINT so it can be handled by a signal-handler as usual.
>
> One thing you might want to mention is that the fgetc() handling is not
> thread-safe, and intentionally so: if two threads read from the same
> console, we are in trouble anyway.

I'm not entirely sure if I know what you mean. Do you suggest that two
threads can race for setting the console ctrl-handler? I don't think
that's the case; "SetConsoleCtrlHandler(x, TRUE)" adds a console
handler to the handler-chain, and SetConsoleCtrlHandler(x, FALSE)
removes it. If two threads add handlers, it is my understanding that
one of them will be run, only to report "no, no more ctrl-handling
needed". Since both handlers block further ctrl-handling, I don't
think there's a problem.

Do you care to clarify what your thread-safety complaint is?

> BTW I like the new mingw_raise() very much!

Thanks! I originally implemented it for a different reason, but that
patch didn't turn out to be useful, so it's nice to finally put it to
use ;)

[Erik Faye-Lund]

It might look like it, but it's not; term_fd used to be returned by
"fileno(fh)", and fh did get properly closed.

With my refactoring, disable_echo/restore_term takes opens /dev/tty a
second time, like Jeff points out. And that second file descriptor
needs to be closed.

[Jeff King]

On Sat, Dec 01, 2012 at 01:31:23PM +0100, Erik Faye-Lund wrote:

> >> One thing you might want to mention is that the fgetc() handling is not
> >> thread-safe, and intentionally so: if two threads read from the same
> >> console, we are in trouble anyway.
> >
> > That makes sense to me, but I'm confused why it is part of mingw_fgetc,
> > which could in theory read from arbitrary streams, no? It it is not
> > necessarily a console operation at all. I feel like I'm probably missing
> > something subtle here...
>
> I did add an early out for the non-console cases. Is this what you're
> missing, perhaps?

Oops, yes. That is exactly what I was missing. :)

Sorry for the noise.

-Peff

[Junio C Hamano]

Erik Faye-Lund <kusm...@gmail.com> writes:

> @@ -1538,13 +1563,7 @@ static sig_handler_t timer_fn = SIG_DFL;
> static unsigned __stdcall ticktack(void *dummy)
> {
> while (WaitForSingleObject(timer_event, timer_interval) == WAIT_TIMEOUT) {
> - if (timer_fn == SIG_DFL) {
> - if (isatty(STDERR_FILENO))
> - fputs("Alarm clock\n", stderr);
> - exit(128 + SIGALRM);
> - }
> - if (timer_fn != SIG_IGN)
> - timer_fn(SIGALRM);
> + mingw_raise(SIGALRM);
> if (one_shot)
> break;
> }

This hunk seems to have been based on a slightly newer codebase than
what I have, and I had to wiggle the patch a bit to make the series
apply. Please double check the result when I push out the 'pu'
branch.

Thanks.

[Erik Faye-Lund]

On Sun, Dec 2, 2012 at 11:42 AM, Junio C Hamano <git...@pobox.com> wrote:
> Erik Faye-Lund <kusm...@gmail.com> writes:
>
>> @@ -1538,13 +1563,7 @@ static sig_handler_t timer_fn = SIG_DFL;
>> static unsigned __stdcall ticktack(void *dummy)
>> {
>> while (WaitForSingleObject(timer_event, timer_interval) == WAIT_TIMEOUT) {
>> - if (timer_fn == SIG_DFL) {
>> - if (isatty(STDERR_FILENO))
>> - fputs("Alarm clock\n", stderr);
>> - exit(128 + SIGALRM);
>> - }
>> - if (timer_fn != SIG_IGN)
>> - timer_fn(SIGALRM);
>> + mingw_raise(SIGALRM);
>> if (one_shot)
>> break;
>> }
>
> This hunk seems to have been based on a slightly newer codebase than
> what I have, and I had to wiggle the patch a bit to make the series
> apply.

Huh, no, it shouldn't be; it's based on 8c7a786 ("Git 1.8.0").

OH! I see now what the problem is... I *missed* a patch in the series! :P

That patch corrected the exit-code for our SIGALRM's SIG_DFL routine;
the old code did "die("Alarm");", but the new one does "fputs("Alarm
clock\n", stderr); exit(128 + SIGALRM)"

> Please double check the result when I push out the 'pu'
> branch.

The resolution is fine; you effectively got the two commits squashed.
I'll send out a new version with the extra patch added, and your
signature-fixup squashed in, OK?

[Junio C Hamano]

Erik Faye-Lund <kusm...@gmail.com> writes:

> That patch corrected the exit-code for our SIGALRM's SIG_DFL routine;
> the old code did "die("Alarm");", but the new one does "fputs("Alarm
> clock\n", stderr); exit(128 + SIGALRM)"
>
>> Please double check the result when I push out the 'pu'
>> branch.
>
> The resolution is fine; you effectively got the two commits squashed.
> I'll send out a new version with the extra patch added, and your
> signature-fixup squashed in, OK?

Sure; thanks for a prompt response.

[Erik Faye-Lund]

So here's v2 of this series. For reference, you can find v1 and
it's discussions here:

http://mid.gmane.org/1352815288-3996-1-git...@gmail.com

The changes since the last round:
* 1/6: This patch has been added. It was missing in the last round,
due to stupidity on my behalf. I'm sorry about that.
* 3/6: This patch got a fixup for the disable_echo function signature
squashed in. I forgot "void" for the empty parameter list.
Thanks to Junio for noticing.

Otherwise, things are unchanged.

Erik Faye-Lund (6):
mingw: correct exit-code for SIGALRM's SIG_DFL

mingw: make fgetc raise SIGINT if apropriate
compat/terminal: factor out echo-disabling
compat/terminal: separate input and output handles
mingw: reuse tty-version of git_terminal_prompt
mingw: get rid of getpass implementation

compat/mingw.c | 88 +++++++++++++++++++++++++++----------

compat/mingw.h | 8 +++-
compat/terminal.c | 129 ++++++++++++++++++++++++++++++++++++++++--------------

3 files changed, 169 insertions(+), 56 deletions(-)

--
1.8.0.4.g3c6fb4f.dirty

[Erik Faye-Lund]

Make sure SIG_DFL for SIGALRM exits with 128 + SIGALRM so other
processes can diagnose why it exits.

While we're at it, make sure we only write to stderr if it's a
terminal, and change the output to match that of Linux.

Signed-off-by: Erik Faye-Lund <kusm...@gmail.com>
---

compat/mingw.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index afc892d..78e8f54 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1538,8 +1538,11 @@ static sig_handler_t timer_fn = SIG_DFL;

static unsigned __stdcall ticktack(void *dummy)
{
while (WaitForSingleObject(timer_event, timer_interval) == WAIT_TIMEOUT) {
- if (timer_fn == SIG_DFL)

- die("Alarm");

+ if (timer_fn == SIG_DFL) {
+ if (isatty(STDERR_FILENO))
+ fputs("Alarm clock\n", stderr);
+ exit(128 + SIGALRM);
+ }

if (timer_fn != SIG_IGN)
timer_fn(SIGALRM);
if (one_shot)
--
1.8.0.4.g3c6fb4f.dirty

[Erik Faye-Lund]

Set a control-handler to prevent the process from terminating, and
simulate SIGINT so it can be handled by a signal-handler as usual.

Signed-off-by: Erik Faye-Lund <kusm...@gmail.com>
---

compat/mingw.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
compat/mingw.h | 6 +++++
2 files changed, 72 insertions(+), 10 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 78e8f54..33ddfdf 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -319,6 +319,31 @@ ssize_t mingw_write(int fd, const void *buf, size_t count)
return write(fd, buf, min(count, 31 * 1024 * 1024));
}

+static BOOL WINAPI ctrl_ignore(DWORD type)
+{
+ return TRUE;
+}
+
+#undef fgetc
+int mingw_fgetc(FILE *stream)
+{
+ int ch;
+ if (!isatty(_fileno(stream)))
+ return fgetc(stream);
+
+ SetConsoleCtrlHandler(ctrl_ignore, TRUE);
+ while (1) {
+ ch = fgetc(stream);
+ if (ch != EOF || GetLastError() != ERROR_OPERATION_ABORTED)
+ break;
+
+ /* Ctrl+C was pressed, simulate SIGINT and retry */
+ mingw_raise(SIGINT);
+ }
+ SetConsoleCtrlHandler(ctrl_ignore, FALSE);
+ return ch;
+}
+
#undef fopen
FILE *mingw_fopen (const char *filename, const char *otype)
{
@@ -1524,7 +1549,7 @@ static HANDLE timer_event;
static HANDLE timer_thread;
static int timer_interval;
static int one_shot;
-static sig_handler_t timer_fn = SIG_DFL;
+static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL;

/* The timer works like this:
* The thread, ticktack(), is a trivial routine that most of the time
@@ -1538,13 +1563,7 @@ static sig_handler_t timer_fn = SIG_DFL;

static unsigned __stdcall ticktack(void *dummy)
{
while (WaitForSingleObject(timer_event, timer_interval) == WAIT_TIMEOUT) {
- if (timer_fn == SIG_DFL) {

- if (isatty(STDERR_FILENO))
- fputs("Alarm clock\n", stderr);
- exit(128 + SIGALRM);
- }
- if (timer_fn != SIG_IGN)
- timer_fn(SIGALRM);
+ mingw_raise(SIGALRM);
if (one_shot)
break;
}

@@ -1635,12 +1654,49 @@ int sigaction(int sig, struct sigaction *in, struct sigaction *out)
sig_handler_t mingw_signal(int sig, sig_handler_t handler)
{
sig_handler_t old = timer_fn;
- if (sig != SIGALRM)
+
+ switch (sig) {
+ case SIGALRM:
+ timer_fn = handler;
+ break;
+
+ case SIGINT:
+ sigint_fn = handler;
+ break;
+
+ default:
return signal(sig, handler);
- timer_fn = handler;
+ }
+
return old;
}

+#undef raise
+int mingw_raise(int sig)
+{
+ switch (sig) {
+ case SIGALRM:

+ if (timer_fn == SIG_DFL) {
+ if (isatty(STDERR_FILENO))
+ fputs("Alarm clock\n", stderr);
+ exit(128 + SIGALRM);

+ } else if (timer_fn != SIG_IGN)
+ timer_fn(SIGALRM);
+ return 0;
+
+ case SIGINT:
+ if (sigint_fn == SIG_DFL)
+ exit(128 + SIGINT);
+ else if (sigint_fn != SIG_IGN)
+ sigint_fn(SIGINT);
+ return 0;
+
+ default:
+ return raise(sig);
+ }
+}
+
+
static const char *make_backslash_path(const char *path)
{
static char buf[PATH_MAX + 1];
diff --git a/compat/mingw.h b/compat/mingw.h
index 61a6521..6b9e69a 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -179,6 +179,9 @@ int mingw_open (const char *filename, int oflags, ...);
ssize_t mingw_write(int fd, const void *buf, size_t count);
#define write mingw_write

+int mingw_fgetc(FILE *stream);
+#define fgetc mingw_fgetc
+
FILE *mingw_fopen (const char *filename, const char *otype);
#define fopen mingw_fopen

@@ -287,6 +290,9 @@ static inline unsigned int git_ntohl(unsigned int x)
sig_handler_t mingw_signal(int sig, sig_handler_t handler);
#define signal mingw_signal

+int mingw_raise(int sig);
+#define raise mingw_raise
+
/*
* ANSI emulation wrappers
*/
--
1.8.0.4.g3c6fb4f.dirty

[Erik Faye-Lund]

By moving the echo-disabling code to a separate function, we can
implement OS-specific versions of it for non-POSIX platforms.

Signed-off-by: Erik Faye-Lund <kusm...@gmail.com>
---

compat/terminal.c | 43 +++++++++++++++++++++++++------------------
1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c

index bbb038d..a6212ca 100644

--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -14,6 +14,7 @@ static void restore_term(void)
return;

tcsetattr(term_fd, TCSAFLUSH, &old_term);
+ close(term_fd);
term_fd = -1;
}

@@ -24,6 +25,27 @@ static void restore_term_on_signal(int sig)
raise(sig);
}

+static int disable_echo(void)
+{
+ struct termios t;
+
+ term_fd = open("/dev/tty", O_RDWR);
+ if (tcgetattr(term_fd, &t) < 0)
+ goto error;
+
+ old_term = t;
+ sigchain_push_common(restore_term_on_signal);
+
+ t.c_lflag &= ~ECHO;
+ if (!tcsetattr(term_fd, TCSAFLUSH, &t))
+ return 0;
+
+error:
+ close(term_fd);
+ term_fd = -1;

+ return -1;
+}
+

char *git_terminal_prompt(const char *prompt, int echo)
{

static struct strbuf buf = STRBUF_INIT;
@@ -34,24 +56,9 @@ char *git_terminal_prompt(const char *prompt, int echo)
if (!fh)
return NULL;

- if (!echo) {
- struct termios t;
-
- if (tcgetattr(fileno(fh), &t) < 0) {
- fclose(fh);
- return NULL;
- }
-
- old_term = t;
- term_fd = fileno(fh);
- sigchain_push_common(restore_term_on_signal);
-
- t.c_lflag &= ~ECHO;
- if (tcsetattr(fileno(fh), TCSAFLUSH, &t) < 0) {
- term_fd = -1;
- fclose(fh);
- return NULL;
- }

+ if (!echo && disable_echo()) {

+ fclose(fh);
+ return NULL;
}

fputs(prompt, fh);
--
1.8.0.4.g3c6fb4f.dirty

[Erik Faye-Lund]

On Windows, the terminal cannot be opened in read-write mode, so
we need distinct pairs for reading and writing. Since this works
fine on other platforms as well, always open them in pairs.

Signed-off-by: Erik Faye-Lund <kusm...@gmail.com>
---

compat/terminal.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index a6212ca..9aecad6 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -50,29 +50,36 @@ char *git_terminal_prompt(const char *prompt, int echo)

{
static struct strbuf buf = STRBUF_INIT;

int r;
- FILE *fh;
+ FILE *input_fh, *output_fh;

- fh = fopen("/dev/tty", "w+");
- if (!fh)
+ input_fh = fopen("/dev/tty", "r");
+ if (!input_fh)
return NULL;

+ output_fh = fopen("/dev/tty", "w");
+ if (!output_fh) {
+ fclose(input_fh);
+ return NULL;
+ }

+
if (!echo && disable_echo()) {

- fclose(fh);
+ fclose(input_fh);
+ fclose(output_fh);
return NULL;
}

- fputs(prompt, fh);
- fflush(fh);
+ fputs(prompt, output_fh);
+ fflush(output_fh);

- r = strbuf_getline(&buf, fh, '\n');
+ r = strbuf_getline(&buf, input_fh, '\n');
if (!echo) {
- fseek(fh, SEEK_CUR, 0);
- putc('\n', fh);
- fflush(fh);
+ putc('\n', output_fh);
+ fflush(output_fh);
}

restore_term();
- fclose(fh);
+ fclose(input_fh);
+ fclose(output_fh);

if (r == EOF)
return NULL;
--
1.8.0.4.g3c6fb4f.dirty

[Erik Faye-Lund]

The getpass-implementation we use on Windows isn't at all ideal;
it works in raw-mode (as opposed to cooked mode), and as a result
does not deal correcly with deletion, arrow-keys etc.

Instead, use cooked mode to read a line at the time, allowing the
C run-time to process the input properly.

Since we set files to be opened in binary-mode by default on
Windows, introduce a FORCE_TEXT macro that expands to the "t"
modifier that forces the terminal to be opened in text-mode so we
do not have to deal with CRLF issues.

Signed-off-by: Erik Faye-Lund <kusm...@gmail.com>
---

compat/terminal.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 60 insertions(+), 9 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 9aecad6..9b5e3d1 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -3,8 +3,22 @@
#include "sigchain.h"
#include "strbuf.h"

+#if defined(HAVE_DEV_TTY) || defined(WIN32)
+
+static void restore_term(void);
+
+static void restore_term_on_signal(int sig)
+{
+ restore_term();
+ sigchain_pop(sig);
+ raise(sig);
+}
+
#ifdef HAVE_DEV_TTY

+#define INPUT_PATH "/dev/tty"
+#define OUTPUT_PATH "/dev/tty"
+
static int term_fd = -1;
static struct termios old_term;

@@ -18,13 +32,6 @@ static void restore_term(void)
term_fd = -1;
}

-static void restore_term_on_signal(int sig)
-{
- restore_term();
- sigchain_pop(sig);
- raise(sig);
-}
-
static int disable_echo(void)
{
struct termios t;
@@ -46,17 +53,61 @@ error:
return -1;
}

+#elif defined(WIN32)
+
+#define INPUT_PATH "CONIN$"
+#define OUTPUT_PATH "CONOUT$"
+#define FORCE_TEXT "t"
+
+static HANDLE hconin = INVALID_HANDLE_VALUE;
+static DWORD cmode;
+
+static void restore_term(void)
+{
+ if (hconin == INVALID_HANDLE_VALUE)
+ return;
+
+ SetConsoleMode(hconin, cmode);
+ CloseHandle(hconin);
+ hconin = INVALID_HANDLE_VALUE;
+}
+
+static int disable_echo(void)
+{
+ hconin = CreateFile("CONIN$", GENERIC_READ | GENERIC_WRITE,
+ FILE_SHARE_READ, NULL, OPEN_EXISTING,
+ FILE_ATTRIBUTE_NORMAL, NULL);
+ if (hconin == INVALID_HANDLE_VALUE)
+ return -1;
+
+ GetConsoleMode(hconin, &cmode);
+ sigchain_push_common(restore_term_on_signal);
+ if (!SetConsoleMode(hconin, cmode & (~ENABLE_ECHO_INPUT))) {
+ CloseHandle(hconin);
+ hconin = INVALID_HANDLE_VALUE;

+ return -1;
+ }
+

+ return 0;
+}
+

+#endif
+
+#ifndef FORCE_TEXT
+#define FORCE_TEXT
+#endif
+

char *git_terminal_prompt(const char *prompt, int echo)
{
static struct strbuf buf = STRBUF_INIT;
int r;

FILE *input_fh, *output_fh;

- input_fh = fopen("/dev/tty", "r");
+ input_fh = fopen(INPUT_PATH, "r" FORCE_TEXT);
if (!input_fh)
return NULL;

- output_fh = fopen("/dev/tty", "w");
+ output_fh = fopen(OUTPUT_PATH, "w" FORCE_TEXT);
if (!output_fh) {
fclose(input_fh);
return NULL;
--
1.8.0.4.g3c6fb4f.dirty

[Erik Faye-Lund]

There's no remaining call-sites, and as pointed out in the
previous commit message, it's not quite ideal. So let's just
lose it.

Signed-off-by: Erik Faye-Lund <kusm...@gmail.com>
---

compat/mingw.c | 15 ---------------
compat/mingw.h | 2 --

2 files changed, 17 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 33ddfdf..5fc14b7 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1758,21 +1758,6 @@ int link(const char *oldpath, const char *newpath)
return 0;
}

-char *getpass(const char *prompt)
-{
- struct strbuf buf = STRBUF_INIT;
-
- fputs(prompt, stderr);
- for (;;) {
- char c = _getch();
- if (c == '\r' || c == '\n')
- break;
- strbuf_addch(&buf, c);
- }
- fputs("\n", stderr);
- return strbuf_detach(&buf, NULL);
-}
-
pid_t waitpid(pid_t pid, int *status, int options)
{
HANDLE h = OpenProcess(SYNCHRONIZE | PROCESS_QUERY_INFORMATION,
diff --git a/compat/mingw.h b/compat/mingw.h
index 6b9e69a..f494ecb 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -55,8 +55,6 @@ struct passwd {
char *pw_dir;
};

-extern char *getpass(const char *prompt);
-
typedef void (__cdecl *sig_handler_t)(int);
struct sigaction {
sig_handler_t sa_handler;
--
1.8.0.4.g3c6fb4f.dirty

[Johannes Schindelin]

Hi kusma,

On Sat, 1 Dec 2012, Erik Faye-Lund wrote:

> On Fri, Nov 30, 2012 at 6:58 PM, Johannes Schindelin
> <Johannes....@gmx.de> wrote:
> > Hi,
> >
> > On Tue, 13 Nov 2012, Erik Faye-Lund wrote:
> >
> >> Set a control-handler to prevent the process from terminating, and
> >> simulate SIGINT so it can be handled by a signal-handler as usual.
> >
> > One thing you might want to mention is that the fgetc() handling is not
> > thread-safe, and intentionally so: if two threads read from the same
> > console, we are in trouble anyway.
>
> I'm not entirely sure if I know what you mean. Do you suggest that two
> threads can race for setting the console ctrl-handler?

That was my idea, yes.

> I don't think that's the case; "SetConsoleCtrlHandler(x, TRUE)" adds a
> console handler to the handler-chain, and SetConsoleCtrlHandler(x,
> FALSE) removes it. If two threads add handlers, it is my understanding
> that one of them will be run, only to report "no, no more ctrl-handling
> needed". Since both handlers block further ctrl-handling, I don't think
> there's a problem.

My idea was that the SetConsoleCtrlHandler(x, FALSE) could remove the
handler prematurely iff another thread wanted to install the very same
handler (but it was already installed by the first thread).

But as I said: for this to happen, *two* threads need to want to access
the console for reading. In that case we'd be in bigger trouble than
thread unsafety... We cannot read two passwords from the same console at
the same time.

Ciao,
Dscho

[Johannes Schindelin]

Hi kusma,

On Sat, 1 Dec 2012, Erik Faye-Lund wrote:

Thanks for clarifying,
Dscho

[Erik Faye-Lund]

On Tue, Dec 4, 2012 at 6:12 PM, Johannes Schindelin

<Johannes....@gmx.de> wrote:
> Hi kusma,
>
> On Sat, 1 Dec 2012, Erik Faye-Lund wrote:
>
>> On Fri, Nov 30, 2012 at 6:58 PM, Johannes Schindelin
>> <Johannes....@gmx.de> wrote:
>> > Hi,
>> >
>> > On Tue, 13 Nov 2012, Erik Faye-Lund wrote:
>> >
>> >> Set a control-handler to prevent the process from terminating, and
>> >> simulate SIGINT so it can be handled by a signal-handler as usual.
>> >
>> > One thing you might want to mention is that the fgetc() handling is not
>> > thread-safe, and intentionally so: if two threads read from the same
>> > console, we are in trouble anyway.
>>
>> I'm not entirely sure if I know what you mean. Do you suggest that two
>> threads can race for setting the console ctrl-handler?
>
> That was my idea, yes.
>
>> I don't think that's the case; "SetConsoleCtrlHandler(x, TRUE)" adds a
>> console handler to the handler-chain, and SetConsoleCtrlHandler(x,
>> FALSE) removes it. If two threads add handlers, it is my understanding
>> that one of them will be run, only to report "no, no more ctrl-handling
>> needed". Since both handlers block further ctrl-handling, I don't think
>> there's a problem.
>
> My idea was that the SetConsoleCtrlHandler(x, FALSE) could remove the
> handler prematurely iff another thread wanted to install the very same
> handler (but it was already installed by the first thread).
>

Yes, but that's not how SetConsoleCtrlHandler works; if a routine x
gets added twice, it needs to be removed twice as well to not get
called. This little C-program demonstrates that:

---8<---
#include <windows.h>
#include <stdio.h>

BOOL WINAPI HandlerRoutine1(DWORD dwCtrlType)
{
printf("Hello from handler1!\n");
return TRUE;
}

BOOL WINAPI HandlerRoutine2(DWORD dwCtrlType)
{
printf("Hello from handler2!\n");
return FALSE;
}

int main()
{
SetConsoleCtrlHandler(HandlerRoutine1, TRUE);
SetConsoleCtrlHandler(HandlerRoutine2, TRUE);
SetConsoleCtrlHandler(HandlerRoutine2, TRUE);
SetConsoleCtrlHandler(HandlerRoutine2, FALSE);
GenerateConsoleCtrlEvent(CTRL_C_EVENT, 0);
SetConsoleCtrlHandler(HandlerRoutine2, FALSE);
SetConsoleCtrlHandler(HandlerRoutine1, FALSE);
}
---8<---

This program outputs:
Hello from handler2!
Hello from handler1!

So since that other thread would add the console ctrl handler before
it removed it again, this should still be thread-safe as far as I can
tell.

> But as I said: for this to happen, *two* threads need to want to access
> the console for reading. In that case we'd be in bigger trouble than
> thread unsafety... We cannot read two passwords from the same console at
> the same time.

I agree with that, but I'm saying that *even if* we didn't have this
limitation, the code shouldn't be problematic.