threads / rfc / 32097

RFC patch, 5 partswin32: support echo for terminal-prompt

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

## tl;dr

27 messages between Nov 13, 2012 and Dec 4, 2012. Diffs are folded; open one to read it.

replies: 26people: 4as markdown or json

Erik Faye-Lund· Nov 13, 2012, 14:04 UTC · lore

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
Erik Faye-Lund· Nov 13, 2012, 14:04 UTC · re: Erik Faye-Lund · lore

[PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate

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 <kusmabite@gmail.com>
---
 compat/mingw.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
 compat/mingw.h |  6 +++++
 2 files changed, 72 insertions(+), 10 deletions(-)
Show changes to 2 files +72 −10

compat/mingw.c, compat/mingw.h

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
Johannes Schindelin· Nov 30, 2012, 17:58 UTC · re: Erik Faye-Lund · lore

Re: [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate

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
Jeff King· Nov 30, 2012, 18:11 UTC · re: Johannes Schindelin · lore

Re: [msysGit] [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate

On Fri, Nov 30, 2012 at 06:58:11PM +0100, Johannes Schindelin wrote:
Show 10 quoted lines
> 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
Erik Faye-Lund· Dec 1, 2012, 12:31 UTC · re: Jeff King · lore

Re: [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate

On Fri, Nov 30, 2012 at 7:11 PM, Jeff King <peff@peff.net> wrote:
Show 17 quoted lines
> 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...

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
Jeff King· Dec 1, 2012, 17:39 UTC · re: Erik Faye-Lund · lore

Re: [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate

On Sat, Dec 01, 2012 at 01:31:23PM +0100, Erik Faye-Lund wrote:
Show 11 quoted lines
> >> 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· Dec 1, 2012, 12:36 UTC · re: Johannes Schindelin · lore

Re: [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate

On Fri, Nov 30, 2012 at 6:58 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 10 quoted lines
> 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 ;)

-- 
*** 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
Johannes Schindelin· Dec 4, 2012, 17:12 UTC · re: Erik Faye-Lund · lore

Re: [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate

Hi kusma,
On Sat, 1 Dec 2012, Erik Faye-Lund wrote:
Show 15 quoted lines
> On Fri, Nov 30, 2012 at 6:58 PM, Johannes Schindelin
> <Johannes.Schindelin@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.
Show 6 quoted lines
> 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
Erik Faye-Lund· Dec 4, 2012, 17:40 UTC · re: Johannes Schindelin · lore

Re: [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate

On Tue, Dec 4, 2012 at 6:12 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 33 quoted lines
> Hi kusma,
>
> On Sat, 1 Dec 2012, Erik Faye-Lund wrote:
>
>> On Fri, Nov 30, 2012 at 6:58 PM, Johannes Schindelin
>> <Johannes.Schindelin@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.

-- 
*** 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
Junio C Hamano· Dec 2, 2012, 10:42 UTC · re: Erik Faye-Lund · lore

Re: [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate

Erik Faye-Lund <kusmabite@gmail.com> writes:
Show 15 quoted lines
> @@ -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.
-- 
*** 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· Dec 3, 2012, 23:29 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate

On Sun, Dec 2, 2012 at 11:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 21 quoted lines
> Erik Faye-Lund <kusmabite@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· Dec 4, 2012, 00:23 UTC · re: Erik Faye-Lund · lore

Re: [PATCH/RFC 1/5] mingw: make fgetc raise SIGINT if apropriate

Erik Faye-Lund <kusmabite@gmail.com> writes:
Show 10 quoted lines
> 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.
-- 
*** 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· Nov 13, 2012, 14:04 UTC · re: Erik Faye-Lund · lore

[PATCH/RFC 2/5] compat/terminal: factor out echo-disabling

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 <kusmabite@gmail.com>
---
 compat/terminal.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)
Show changes to compat/terminal.c +25 −18
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
Johannes Schindelin· Nov 30, 2012, 17:59 UTC · re: Erik Faye-Lund · lore

Re: [PATCH/RFC 2/5] compat/terminal: factor out echo-disabling

Hi,
On Tue, 13 Nov 2012, Erik Faye-Lund wrote:
Show 19 quoted lines
> 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 <kusmabite@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

-- 
*** 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
Jeff King· Nov 30, 2012, 18:19 UTC · re: Johannes Schindelin · lore

Re: [PATCH/RFC 2/5] compat/terminal: factor out echo-disabling

On Fri, Nov 30, 2012 at 06:59:30PM +0100, Johannes Schindelin wrote:
Show 13 quoted lines
> > 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
Erik Faye-Lund· Dec 1, 2012, 12:43 UTC · re: Johannes Schindelin · lore

Re: [PATCH/RFC 2/5] compat/terminal: factor out echo-disabling

On Fri, Nov 30, 2012 at 6:59 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 25 quoted lines
> 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 <kusmabite@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?

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
Johannes Schindelin· Dec 4, 2012, 17:12 UTC · re: Erik Faye-Lund · lore

Re: [PATCH/RFC 2/5] compat/terminal: factor out echo-disabling

Hi kusma,
On Sat, 1 Dec 2012, Erik Faye-Lund wrote:
Show 34 quoted lines
> On Fri, Nov 30, 2012 at 6:59 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > 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 <kusmabite@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?
> 
> 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
Erik Faye-Lund· Nov 13, 2012, 14:04 UTC · re: Erik Faye-Lund · lore

[PATCH/RFC 3/5] compat/terminal: separate input and output handles

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 <kusmabite@gmail.com>
---
 compat/terminal.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)
Show changes to compat/terminal.c +18 −11
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
Jeff King· Nov 30, 2012, 18:22 UTC · re: Erik Faye-Lund · lore

Re: [PATCH/RFC 3/5] compat/terminal: separate input and output handles

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
Erik Faye-Lund· Nov 13, 2012, 14:04 UTC · re: Erik Faye-Lund · lore

[PATCH/RFC 4/5] mingw: reuse tty-version of git_terminal_prompt

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 <kusmabite@gmail.com>
---
 compat/terminal.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 60 insertions(+), 9 deletions(-)
Show changes to compat/terminal.c +60 −9
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
Johannes Schindelin· Nov 30, 2012, 18:05 UTC · re: Erik Faye-Lund · lore

Re: [PATCH/RFC 4/5] mingw: reuse tty-version of git_terminal_prompt

Hi kusma,
On Tue, 13 Nov 2012, Erik Faye-Lund wrote:
Show 6 quoted lines
> 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
Jeff King· Nov 30, 2012, 18:27 UTC · re: Erik Faye-Lund · lore

Re: [PATCH/RFC 4/5] mingw: reuse tty-version of git_terminal_prompt

On Tue, Nov 13, 2012 at 03:04:06PM +0100, Erik Faye-Lund wrote:
Show 11 quoted lines
> 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
Erik Faye-Lund· Nov 13, 2012, 14:04 UTC · re: Erik Faye-Lund · lore

[PATCH/RFC 5/5] mingw: get rid of getpass implementation

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 <kusmabite@gmail.com>
---
 compat/mingw.c | 15 ---------------
 compat/mingw.h |  2 --
 2 files changed, 17 deletions(-)
Show changes to 2 files +0 −17

compat/mingw.c, compat/mingw.h

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
Johannes Schindelin· Nov 30, 2012, 18:06 UTC · re: Erik Faye-Lund · lore

Re: [PATCH/RFC 5/5] mingw: get rid of getpass implementation

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
Jeff King· Nov 30, 2012, 18:27 UTC · re: Erik Faye-Lund · lore

Re: [PATCH/RFC 5/5] mingw: get rid of getpass implementation

On Tue, Nov 13, 2012 at 03:04:07PM +0100, Erik Faye-Lund wrote:
Show 9 quoted lines
> 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 <kusmabite@gmail.com>
> ---
>  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
Erik Faye-Lund· Nov 30, 2012, 10:16 UTC · re: Erik Faye-Lund · lore

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

Ping?
On Tue, Nov 13, 2012 at 3:04 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
Show 38 quoted lines
> 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
>
Jeff King· Nov 30, 2012, 18:30 UTC · re: Erik Faye-Lund · lore

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

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

← back to recent threads