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

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_XwSXOP_iFZG3GEYSPSBPcSVV8wRQ@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

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

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

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

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

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

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

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

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

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

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

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

Ping?

On Tue, Nov 13, 2012 at 3:04 PM, Erik Faye-Lund [off-list ref] wrote:

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_XwSXOP_iFZG3GEYSPSBPcSVV8wRQ@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

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

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

Hi,

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

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

Rest looks awsomely fine.
Dscho

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

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

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

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

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

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

Hi,

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

quoted

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

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

quoted

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

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

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

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

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

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

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 <redacted>
---
 compat/mingw.c | 15 ---------------
 compat/mingw.h |  2 --
 2 files changed, 17 deletions(-)

Yay!

-Peff

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

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

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

On Fri, Nov 30, 2012 at 7:11 PM, Jeff King [off-list ref] wrote:

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

quoted

Hi,

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

quoted

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

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

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

On Fri, Nov 30, 2012 at 6:58 PM, Johannes Schindelin
[off-list ref] wrote:

Hi,

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

quoted

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

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

On Fri, Nov 30, 2012 at 6:59 PM, Johannes Schindelin
[off-list ref] wrote:

Hi,

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

quoted

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 <redacted>
---
 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?

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.

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

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

quoted

quoted

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

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

Erik Faye-Lund [off-list ref] writes:

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.

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

On Sun, Dec 2, 2012 at 11:42 AM, Junio C Hamano [off-list ref] wrote:

Erik Faye-Lund [off-list ref] writes:

quoted

@@ -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?

Hi kusma,

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

On Fri, Nov 30, 2012 at 6:58 PM, Johannes Schindelin
[off-list ref] wrote:

quoted

Hi,

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

quoted

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

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

Hi kusma,

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

On Fri, Nov 30, 2012 at 6:59 PM, Johannes Schindelin
[off-list ref] wrote:

quoted

Hi,

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

quoted

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 <redacted>
---
 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?

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.

Thanks for clarifying,
Dscho

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

On Tue, Dec 4, 2012 at 6:12 PM, Johannes Schindelin
[off-list ref] wrote:

Hi kusma,

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

quoted

On Fri, Nov 30, 2012 at 6:58 PM, Johannes Schindelin
[off-list ref] wrote:

quoted

Hi,

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

quoted

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.

quoted

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.

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en