threads / patch / 44673

patchFix a long-standing isatty() problem on Windows

Subject: [PATCH 0/1] Fix a long-standing isatty() problem on Windows

## tl;dr

21 messages between Dec 11, 2016 and Dec 22, 2016. Diffs are folded; open one to read it.

replies: 20people: 3as markdown or json

Johannes Schindelin· Dec 11, 2016, 11:16 UTC · lore

I finally got a chance to debug the problems with the ever-timing-out test runs of `pu` on Windows. Turns out that pb/bisect uncovered a really old, really bad bug: on Windows, isatty() does not do what Git expects, at least not completely: it detects interactive terminals *and character devices*.

Why is this such a big deal?

One such character device is NUL, Windows' equivalent of /dev/null. And guess what happens when the new tests of the bisect--helper run, with stdin redirected from /dev/null? Precisely. Git asks "the user" for reassurance that it may really continue, waiting forever. Or for Ctrl+C.

As we know what Git's source code wants, we have to make extra certain to test whether isatty() reports success for a Console. The common way to do this is to run GetConsoleMode() for input file descriptors, and GetConsoleScreenBufferInfo() for output file descriptors.

One additional note: the new winansi_isatty() function was put into this particular spot not only because it vaguely makes sense to put tty-related stuff into compat/winansi.c, but with required future changes in mind:

The current way in which Git for Windows makes sure that isatty() returns non-zero for Git Bash (which runs in a terminal emulator called MinTTY that does *not* have any Windows Console associated with it, and therefore Windows' _isatty() would actually return 0 if it was not for our detect_msys_tty() function) is hacky and needs to be fixed properly.

It is hacky because it relies on internals of the MSVC runtime that do not hold true for the new Universal runtimes, which are used when compiling with Visual C.

We already have experimental code to future-proof this method, and we use that already when compiling Git for Windows in Visual Studio.

The place in which winansi_isatty() now lives will hopefully make it possible to unify the code paths again, so that both GCC and Visual C use detect_msys_tty() through winansi_isatty().

This will also fix a bug where current Visual C-built Git may misdetect a reopened stdin to be connected to an interactive terminal.

Johannes Schindelin (1):
  mingw: intercept isatty() to handle /dev/null as Git expects it
 compat/mingw.h   |  3 +++
 compat/winansi.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)
base-commit: 8d7a455ed52e2a96debc080dfc011b6bb00db5d2
Published-As: https://github.com/dscho/git/releases/tag/mingw-isatty-v1
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-isatty-v1
-- 
2.11.0.rc3.windows.1
Johannes Schindelin· Dec 11, 2016, 11:16 UTC · re: Johannes Schindelin · lore

[PATCH 1/1] mingw: intercept isatty() to handle /dev/null as Git expects it

When Git's source code calls isatty(), it really asks whether the respective file descriptor is connected to an interactive terminal.

Windows' _isatty() function, however, determines whether the file descriptor is associated with a character device. And NUL, Windows' equivalent of /dev/null, is a character device.

Which means that for years, Git mistakenly detected an associated interactive terminal when being run through the test suite, which almost always redirects stdin, stdout and stderr to /dev/null.

This bug only became obvious, and painfully so, when the new bisect--helper entered the `pu` branch and made the automatic build & test time out because t6030 was waiting for an answer.

For details, see
	https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.h   |  3 +++
 compat/winansi.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)
Show changes to 2 files +36 −0

compat/mingw.h, compat/winansi.c

diff --git a/compat/mingw.h b/compat/mingw.h
index 034fff9479..3350169555 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -384,6 +384,9 @@ int mingw_raise(int sig);
  * ANSI emulation wrappers
  */
 
+int winansi_isatty(int fd);
+#define isatty winansi_isatty
+
 void winansi_init(void);
 HANDLE winansi_get_osfhandle(int fd);
 
diff --git a/compat/winansi.c b/compat/winansi.c
index db4a5b0a37..cb725fb02f 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -7,6 +7,9 @@
 #include <wingdi.h>
 #include <winreg.h>
 
+/* In this file, we actually want to use Windows' own isatty(). */
+#undef isatty
+
 /*
  ANSI codes used by git: m, K
 
@@ -570,6 +573,36 @@ static void detect_msys_tty(int fd)
 
 #endif
 
+int winansi_isatty(int fd)
+{
+	int res = isatty(fd);
+
+	if (res) {
+		/*
+		 * Make sure that /dev/null is not fooling Git into believing
+		 * that we are connected to a terminal, as "_isatty() returns a
+		 * nonzero value if the descriptor is associated with a
+		 * character device."; for more information, see
+		 *
+		 * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx
+		 */
+		HANDLE handle = (HANDLE)_get_osfhandle(fd);
+		if (fd == STDIN_FILENO) {
+			DWORD dummy;
+
+			if (!GetConsoleMode(handle, &dummy))
+				res = 0;
+		} else if (fd == STDOUT_FILENO || fd == STDERR_FILENO) {
+			CONSOLE_SCREEN_BUFFER_INFO dummy;
+
+			if (!GetConsoleScreenBufferInfo(handle, &dummy))
+				res = 0;
+		}
+	}
+
+	return res;
+}
+
 void winansi_init(void)
 {
 	int con1, con2;

-- 
2.11.0.rc3.windows.1
Johannes Sixt· Dec 16, 2016, 17:48 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/1] mingw: intercept isatty() to handle /dev/null as Git expects it

Am 11.12.2016 um 12:16 schrieb Johannes Schindelin:
Show 81 quoted lines
> When Git's source code calls isatty(), it really asks whether the
> respective file descriptor is connected to an interactive terminal.
>
> Windows' _isatty() function, however, determines whether the file
> descriptor is associated with a character device. And NUL, Windows'
> equivalent of /dev/null, is a character device.
>
> Which means that for years, Git mistakenly detected an associated
> interactive terminal when being run through the test suite, which
> almost always redirects stdin, stdout and stderr to /dev/null.
>
> This bug only became obvious, and painfully so, when the new
> bisect--helper entered the `pu` branch and made the automatic build & test
> time out because t6030 was waiting for an answer.
>
> For details, see
>
> 	https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  compat/mingw.h   |  3 +++
>  compat/winansi.c | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
>
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 034fff9479..3350169555 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -384,6 +384,9 @@ int mingw_raise(int sig);
>   * ANSI emulation wrappers
>   */
>
> +int winansi_isatty(int fd);
> +#define isatty winansi_isatty
> +
>  void winansi_init(void);
>  HANDLE winansi_get_osfhandle(int fd);
>
> diff --git a/compat/winansi.c b/compat/winansi.c
> index db4a5b0a37..cb725fb02f 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -7,6 +7,9 @@
>  #include <wingdi.h>
>  #include <winreg.h>
>
> +/* In this file, we actually want to use Windows' own isatty(). */
> +#undef isatty
> +
>  /*
>   ANSI codes used by git: m, K
>
> @@ -570,6 +573,36 @@ static void detect_msys_tty(int fd)
>
>  #endif
>
> +int winansi_isatty(int fd)
> +{
> +	int res = isatty(fd);
> +
> +	if (res) {
> +		/*
> +		 * Make sure that /dev/null is not fooling Git into believing
> +		 * that we are connected to a terminal, as "_isatty() returns a
> +		 * nonzero value if the descriptor is associated with a
> +		 * character device."; for more information, see
> +		 *
> +		 * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx
> +		 */
> +		HANDLE handle = (HANDLE)_get_osfhandle(fd);
> +		if (fd == STDIN_FILENO) {
> +			DWORD dummy;
> +
> +			if (!GetConsoleMode(handle, &dummy))
> +				res = 0;
> +		} else if (fd == STDOUT_FILENO || fd == STDERR_FILENO) {
> +			CONSOLE_SCREEN_BUFFER_INFO dummy;
> +
> +			if (!GetConsoleScreenBufferInfo(handle, &dummy))
> +				res = 0;

I am sorry to have to report that this check does not work as expected. I am operating Git from CMD, not mintty, BTW.

It fails with GetLastError() == 6 (invalid handle value). The reason for this is, I think, that in reality we are not writing to the console directly, but to a pipe, which is drained by console_thread(), which writes to the console.

I have little clue what this winansi.c file is doing. Do you have any pointers where I should start digging?

Wait...
Should we not use winansi_get_osfhandle() instead of _get_osfhandle()?
Show 10 quoted lines
> +		}
> +	}
> +
> +	return res;
> +}
> +
>  void winansi_init(void)
>  {
>  	int con1, con2;
>
Junio C Hamano· Dec 16, 2016, 18:08 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/1] mingw: intercept isatty() to handle /dev/null as Git expects it

Johannes Sixt <j6t@kdbg.org> writes:
Show 9 quoted lines
> Am 11.12.2016 um 12:16 schrieb Johannes Schindelin:
>> When Git's source code calls isatty(), it really asks whether the
>> respective file descriptor is connected to an interactive terminal.
>> ...
>> +			if (!GetConsoleScreenBufferInfo(handle, &dummy))
>> +				res = 0;
>
> I am sorry to have to report that this check does not work as
> expected. I am operating Git from CMD, not mintty, BTW.
Ouch.

Sorry for not having waited for you to chime in before agreeing to fast-track this one, and now this is in 'master'.

I should have known better than that by now, after having seen you uncover bugs that did not trigger in Dscho's environment and vice versa to tell you that Windows specific things both of you deem good have a much higher chance of being correct than a change without an Ack by the other.

Show 22 quoted lines
> It fails with GetLastError() == 6 (invalid handle value). The reason
> for this is, I think, that in reality we are not writing to the
> console directly, but to a pipe, which is drained by console_thread(),
> which writes to the console.
>
> I have little clue what this winansi.c file is doing. Do you have any
> pointers where I should start digging?
>
> Wait...
>
> Should we not use winansi_get_osfhandle() instead of _get_osfhandle()?
>
>> +		}
>> +	}
>> +
>> +	return res;
>> +}
>> +
>>  void winansi_init(void)
>>  {
>>  	int con1, con2;
>>
Johannes Sixt· Dec 16, 2016, 18:44 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/1] mingw: intercept isatty() to handle /dev/null as Git expects it

Am 16.12.2016 um 19:08 schrieb Junio C Hamano:
> Sorry for not having waited for you to chime in before agreeing to
> fast-track this one, and now this is in 'master'.

No reason to be sorry, things happen... Dscho's request for fast-tracking was very reasonable, and the patch looked "obviously correct". I lacked time to test it promptly, and when I finally did, I ignored the symptoms because my workflow was a mess and I had to concentrate on other things. Only after 'git push' did not report the pack progress, was my suspicion raised...

>> Should we not use winansi_get_osfhandle() instead of _get_osfhandle()?

Unfortunately, I'm away from my Windows box over the weekend. It will have to wait until Monday before I can test this idea.

-- Hannes
Junio C Hamano· Dec 16, 2016, 19:05 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/1] mingw: intercept isatty() to handle /dev/null as Git expects it

Johannes Sixt <j6t@kdbg.org> writes:
Show 7 quoted lines
> Am 16.12.2016 um 19:08 schrieb Junio C Hamano:
>> Sorry for not having waited for you to chime in before agreeing to
>> fast-track this one, and now this is in 'master'.
>
> No reason to be sorry, things happen... Dscho's request for
> fast-tracking was very reasonable, and the patch looked "obviously
> correct".

Oh, I do agree it was reasonable and looked obviously correct. And I suspect that it did not make anything worse, either, so there is not much harm done, other than that I now have to remember not to merge it down without further fixes to 'maint' and declare the NUL thing fixed ;-)

> Unfortunately, I'm away from my Windows box over the weekend. It will
> have to wait until Monday before I can test this idea.
Thanks.
Johannes Schindelin· Dec 21, 2016, 17:56 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/1] mingw: intercept isatty() to handle /dev/null as Git expects it

Hi,
On Fri, 16 Dec 2016, Junio C Hamano wrote:
Show 15 quoted lines
> Johannes Sixt <j6t@kdbg.org> writes:
> 
> > Am 16.12.2016 um 19:08 schrieb Junio C Hamano:
> >> Sorry for not having waited for you to chime in before agreeing to
> >> fast-track this one, and now this is in 'master'.
> >
> > No reason to be sorry, things happen... Dscho's request for
> > fast-tracking was very reasonable, and the patch looked "obviously
> > correct".
> 
> Oh, I do agree it was reasonable and looked obviously correct.  And
> I suspect that it did not make anything worse, either, so there is
> not much harm done, other than that I now have to remember not to
> merge it down without further fixes to 'maint' and declare the NUL
> thing fixed ;-)

Actually, due to having way too many worktrees I managed to test the exact wrong build product and failed to notice that the patch I asked to fast-track broke paging in Git for Windows' Git Bash, too, not only in CMD.

Bummer.
> > Unfortunately, I'm away from my Windows box over the weekend. It will
> > have to wait until Monday before I can test this idea.
> 
> Thanks.

I just sent out a fixer-upper patch series, hopefully I got it right this time.

Hannes, it would be very nice if you could beat on it for a bit.

Happy holidays, Dscho

Johannes Sixt· Dec 18, 2016, 15:26 UTC · re: Johannes Sixt · lore

[PATCH] winansi_isatty(): fix when Git is used from CMD

The code in winansi.c emulates ANSI escape sequences when Git is connected to the "real" windows console, CMD.exe. The details are outline in eac14f8909d9 (Win32: Thread-safe windows console output, 2012-01-14). Essentially, it plugs a pipe between C code and the actual console output handle.

This commit also added an override for isatty(), but it was made unnecessary by fcd428f4a952 (Win32: fix broken pipe detection, 2012-03-01).

The new isatty() override implemented by cbb3f3c9b197 (mingw: intercept isatty() to handle /dev/null as Git expects it, 2016-12-11) does not take into account that _get_osfhandle() returns the handle visible by the C code, which is the pipe. But it actually wants to investigate the properties of the handle that is actually connected to the outside world. Fortunately, there is already winansi_get_osfhandle(), which returns exactly this handle. Use it.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
I was able to test the idea earlier than anticipated and it does work
for me.
 compat/winansi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to compat/winansi.c +1 −1
diff --git a/compat/winansi.c b/compat/winansi.c
index cb725fb02f..ba360be69b 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -586,7 +586,7 @@ int winansi_isatty(int fd)
 		 *
 		 * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx
 		 */
-		HANDLE handle = (HANDLE)_get_osfhandle(fd);
+		HANDLE handle = winansi_get_osfhandle(fd);
 		if (fd == STDIN_FILENO) {
 			DWORD dummy;
 

-- 
2.11.0.79.gf6b77ca
Johannes Sixt· Dec 18, 2016, 15:37 UTC · re: Johannes Sixt · lore

Re: [PATCH] winansi_isatty(): fix when Git is used from CMD

Am 18.12.2016 um 16:26 schrieb Johannes Sixt:
Show 7 quoted lines
> The new isatty() override implemented by cbb3f3c9b197 (mingw: intercept
> isatty() to handle /dev/null as Git expects it, 2016-12-11) does not
> take into account that _get_osfhandle() returns the handle visible by
> the C code, which is the pipe. But it actually wants to investigate the
> properties of the handle that is actually connected to the outside
> world. Fortunately, there is already winansi_get_osfhandle(), which
> returns exactly this handle. Use it.

But quite frankly, I find the implementation of winansi_isatty() very unsatisfactory.

I understand that you wanted to be defensive and to override the decision made by MSVCRT only when necessary.

However!

winansi.c is all about overriding MSVCRT's console handling. If we are connected to a console, then by the time isatty() is called (from outside the emulation layer), all handling of file descriptors 1 and 2 is already outside MSVCRT's control. In particular, we have determined unambiguously whether a terminal is connected (see is_console()). I suggest to have the implementation below (on top of the patch I'm responding to).

What do you think?
Show changes to compat/winansi.c +10 −13
diff --git a/compat/winansi.c b/compat/winansi.c
index ba360be69b..1748d17777 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -575,9 +575,8 @@ static void detect_msys_tty(int fd)
 
 int winansi_isatty(int fd)
 {
-	int res = isatty(fd);
-
-	if (res) {
+	switch (fd) {
+	case 0:
 		/*
 		 * Make sure that /dev/null is not fooling Git into believing
 		 * that we are connected to a terminal, as "_isatty() returns a
@@ -586,21 +585,19 @@ int winansi_isatty(int fd)
 		 *
 		 * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx
 		 */
-		HANDLE handle = winansi_get_osfhandle(fd);
-		if (fd == STDIN_FILENO) {
+		{
+			HANDLE handle = (HANDLE)_get_osfhandle(fd);
 			DWORD dummy;
 
-			if (!GetConsoleMode(handle, &dummy))
-				res = 0;
-		} else if (fd == STDOUT_FILENO || fd == STDERR_FILENO) {
-			CONSOLE_SCREEN_BUFFER_INFO dummy;
-
-			if (!GetConsoleScreenBufferInfo(handle, &dummy))
-				res = 0;
+			return !!GetConsoleMode(handle, &dummy);
 		}
+	case 1:
+		return !!hconsole1;
+	case 2:
+		return !!hconsole2;
 	}
 
-	return res;
+	return isatty(fd);
 }
 
 void winansi_init(void)

-- 
2.11.0.79.gf6b77ca
Johannes Sixt· Dec 19, 2016, 19:34 UTC · re: Johannes Sixt · lore

Re: [PATCH] winansi_isatty(): fix when Git is used from CMD

Am 18.12.2016 um 16:37 schrieb Johannes Sixt:
Show 9 quoted lines
> winansi.c is all about overriding MSVCRT's console handling. If we are
> connected to a console, then by the time isatty() is called (from
> outside the emulation layer), all handling of file descriptors 1 and 2
> is already outside MSVCRT's control. In particular, we have determined
> unambiguously whether a terminal is connected (see is_console()). I
> suggest to have the implementation below (on top of the patch I'm
> responding to).
>
> What do you think?

I thought a bit more about this approach, and I retract it. I think it does not work when Git is connected to an MSYS TTY, i.e., when the "console" is in reality the pipe that is detected in detect_msys_tty().

At the same time I wonder how your original winansi_isatty() could have worked: In this case, MSVCRT's isatty() would return 1 (because detect_msys_tty() has set things up that this happens), but then winansi_isatty() checks whether the handle underlying fd 0, 1 or 2 is a real Windows console. But it is not: it is a pipe. Am I missing something?

Show 48 quoted lines
>
> diff --git a/compat/winansi.c b/compat/winansi.c
> index ba360be69b..1748d17777 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -575,9 +575,8 @@ static void detect_msys_tty(int fd)
>
>  int winansi_isatty(int fd)
>  {
> -	int res = isatty(fd);
> -
> -	if (res) {
> +	switch (fd) {
> +	case 0:
>  		/*
>  		 * Make sure that /dev/null is not fooling Git into believing
>  		 * that we are connected to a terminal, as "_isatty() returns a
> @@ -586,21 +585,19 @@ int winansi_isatty(int fd)
>  		 *
>  		 * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx
>  		 */
> -		HANDLE handle = winansi_get_osfhandle(fd);
> -		if (fd == STDIN_FILENO) {
> +		{
> +			HANDLE handle = (HANDLE)_get_osfhandle(fd);
>  			DWORD dummy;
>
> -			if (!GetConsoleMode(handle, &dummy))
> -				res = 0;
> -		} else if (fd == STDOUT_FILENO || fd == STDERR_FILENO) {
> -			CONSOLE_SCREEN_BUFFER_INFO dummy;
> -
> -			if (!GetConsoleScreenBufferInfo(handle, &dummy))
> -				res = 0;
> +			return !!GetConsoleMode(handle, &dummy);
>  		}
> +	case 1:
> +		return !!hconsole1;
> +	case 2:
> +		return !!hconsole2;
>  	}
>
> -	return res;
> +	return isatty(fd);
>  }
>
>  void winansi_init(void)
>
Johannes Schindelin· Dec 21, 2016, 17:57 UTC · re: Johannes Sixt · lore

Re: [PATCH] winansi_isatty(): fix when Git is used from CMD

Hi Hannes,
On Mon, 19 Dec 2016, Johannes Sixt wrote:
Show 20 quoted lines
> Am 18.12.2016 um 16:37 schrieb Johannes Sixt:
> > winansi.c is all about overriding MSVCRT's console handling. If we are
> > connected to a console, then by the time isatty() is called (from
> > outside the emulation layer), all handling of file descriptors 1 and 2
> > is already outside MSVCRT's control. In particular, we have determined
> > unambiguously whether a terminal is connected (see is_console()). I
> > suggest to have the implementation below (on top of the patch I'm
> > responding to).
> >
> > What do you think?
> 
> I thought a bit more about this approach, and I retract it. I think it
> does not work when Git is connected to an MSYS TTY, i.e., when the
> "console" is in reality the pipe that is detected in detect_msys_tty().
> 
> At the same time I wonder how your original winansi_isatty() could have
> worked: In this case, MSVCRT's isatty() would return 1 (because
> detect_msys_tty() has set things up that this happens), but then
> winansi_isatty() checks whether the handle underlying fd 0, 1 or 2 is a real
> Windows console. But it is not: it is a pipe. Am I missing something?
You did not miss anything. I did. I broke everything.

Very sorry for that! Dscho

Johannes Schindelin· Dec 20, 2016, 16:53 UTC · re: Johannes Sixt · lore

Re: [PATCH] winansi_isatty(): fix when Git is used from CMD

Hi Hannes,
On Sun, 18 Dec 2016, Johannes Sixt wrote:
Show 48 quoted lines
> What do you think?
> 
> diff --git a/compat/winansi.c b/compat/winansi.c
> index ba360be69b..1748d17777 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -575,9 +575,8 @@ static void detect_msys_tty(int fd)
>  
>  int winansi_isatty(int fd)
>  {
> -	int res = isatty(fd);
> -
> -	if (res) {
> +	switch (fd) {
> +	case 0:
>  		/*
>  		 * Make sure that /dev/null is not fooling Git into believing
>  		 * that we are connected to a terminal, as "_isatty() returns a
> @@ -586,21 +585,19 @@ int winansi_isatty(int fd)
>  		 *
>  		 * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx
>  		 */
> -		HANDLE handle = winansi_get_osfhandle(fd);
> -		if (fd == STDIN_FILENO) {
> +		{
> +			HANDLE handle = (HANDLE)_get_osfhandle(fd);
>  			DWORD dummy;
>  
> -			if (!GetConsoleMode(handle, &dummy))
> -				res = 0;
> -		} else if (fd == STDOUT_FILENO || fd == STDERR_FILENO) {
> -			CONSOLE_SCREEN_BUFFER_INFO dummy;
> -
> -			if (!GetConsoleScreenBufferInfo(handle, &dummy))
> -				res = 0;
> +			return !!GetConsoleMode(handle, &dummy);
>  		}
> +	case 1:
> +		return !!hconsole1;
> +	case 2:
> +		return !!hconsole2;
>  	}
>  
> -	return res;
> +	return isatty(fd);
>  }
>  
>  void winansi_init(void)
I think that would break running Git in Git Bash (i.e. MinTTY) ;-)

Let me try to come up with a patch series starting from your patch. We need

- to abandon the _pioinfo hack
- to make isatty() work correctly with /dev/null
- to make isatty() work correctly in CMD
- to make isatty() work correctly in MinTTY (i.e. with MSYS2 pipes instead
  of Consoles)
I think we can have it all.

Ciao, Dscho

Junio C Hamano· Dec 19, 2016, 01:12 UTC · re: Johannes Sixt · lore

Re: [PATCH] winansi_isatty(): fix when Git is used from CMD

Johannes Sixt <j6t@kdbg.org> writes:
Show 13 quoted lines
> ..
> The new isatty() override implemented by cbb3f3c9b197 (mingw: intercept
> isatty() to handle /dev/null as Git expects it, 2016-12-11) does not
> take into account that _get_osfhandle() returns the handle visible by
> the C code, which is the pipe. But it actually wants to investigate the
> properties of the handle that is actually connected to the outside
> world. Fortunately, there is already winansi_get_osfhandle(), which
> returns exactly this handle. Use it.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> I was able to test the idea earlier than anticipated and it does work
> for me.

I'll queue this in 'pu' so that I won't forget, but this time I'll make sure I won't act on it until I see you two agree on the right way forward.

Thanks.
Johannes Schindelin· Dec 20, 2016, 16:50 UTC · re: Johannes Sixt · lore

Re: [PATCH] winansi_isatty(): fix when Git is used from CMD

Hi Hannes,
On Sun, 18 Dec 2016, Johannes Sixt wrote:
Show 22 quoted lines
> The code in winansi.c emulates ANSI escape sequences when Git is
> connected to the "real" windows console, CMD.exe. The details are
> outline in eac14f8909d9 (Win32: Thread-safe windows console output,
> 2012-01-14). Essentially, it plugs a pipe between C code and the actual
> console output handle.
> 
> This commit also added an override for isatty(), but it was made
> unnecessary by fcd428f4a952 (Win32: fix broken pipe detection,
> 2012-03-01).
> 
> The new isatty() override implemented by cbb3f3c9b197 (mingw: intercept
> isatty() to handle /dev/null as Git expects it, 2016-12-11) does not
> take into account that _get_osfhandle() returns the handle visible by
> the C code, which is the pipe. But it actually wants to investigate the
> properties of the handle that is actually connected to the outside
> world. Fortunately, there is already winansi_get_osfhandle(), which
> returns exactly this handle. Use it.
> 
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> I was able to test the idea earlier than anticipated and it does work
> for me.
Thank you.
Show 10 quoted lines
> diff --git a/compat/winansi.c b/compat/winansi.c
> index cb725fb02f..ba360be69b 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -586,7 +586,7 @@ int winansi_isatty(int fd)
>  		 *
>  		 * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx
>  		 */
> -		HANDLE handle = (HANDLE)_get_osfhandle(fd);
> +		HANDLE handle = winansi_get_osfhandle(fd);

That code works because winansi_get_osfhandle() is in winansi.c, where its call to isatty() is *not* redirected to winansi_isatty(). Good.

My plan was actually to clean up the "magic" detect_msys_tty() code: it messes with internals of the MSVC runtime that are no longer the same in the Universal Runtime (UCRT), and hence we already had to come up with a different way to detect an MSYS2 pipe. My preference would be to merge that logic into winansi_isatty() and abandon the _pioinfo hack.

Let's just clean up all of this in one go.

Ciao, Dscho

Junio C Hamano· Dec 20, 2016, 16:59 UTC · re: Johannes Schindelin · lore

Re: [PATCH] winansi_isatty(): fix when Git is used from CMD

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 6 quoted lines
> That code works because winansi_get_osfhandle() is in winansi.c, where its
> call to isatty() is *not* redirected to winansi_isatty(). Good.
> ...
> My plan was actually ...
> ...
> Let's just clean up all of this in one go.

I take this to mean that I should hold off applying/merging j6t's one liner and wait for your counterproposal tested by j6t. If I misread you please let me know.

Thanks for working well together, as always.
Johannes Schindelin· Dec 21, 2016, 17:59 UTC · re: Junio C Hamano · lore

Re: [PATCH] winansi_isatty(): fix when Git is used from CMD

Hi Junio,
On Tue, 20 Dec 2016, Junio C Hamano wrote:
Show 14 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > That code works because winansi_get_osfhandle() is in winansi.c, where its
> > call to isatty() is *not* redirected to winansi_isatty(). Good.
> > ...
> > My plan was actually ...
> > ...
> > Let's just clean up all of this in one go.
> 
> I take this to mean that I should hold off applying/merging j6t's
> one liner and wait for your counterproposal tested by j6t.  If I
> misread you please let me know.
> 
> Thanks for working well together, as always.

I prepared a patch series based on `pu`, on top of Hannes' patch, and I also prepared a branch that is based on `master`, obviously without Hannes' patch.

I am indifferent whether to include it or not, as long as we have something robust in the end that everybody is happy with.

Ciao, Dscho

Junio C Hamano· Dec 21, 2016, 18:54 UTC · re: Johannes Schindelin · lore

Re: [PATCH] winansi_isatty(): fix when Git is used from CMD

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> I prepared a patch series based on `pu`, on top of Hannes' patch, and I
> also prepared a branch that is based on `master`, obviously without
> Hannes' patch.
I think the latter is what you have at
 $ git fetch https://github.com/dscho/git mingw-isatty-fixup-master

If that is correct, I am inclined to fetch that and queue it on 'pu', replacing Hannes's patch, and then to ask him to Test/Ack it.

Thanks.
Johannes Schindelin· Dec 22, 2016, 09:23 UTC · re: Junio C Hamano · lore

Re: [PATCH] winansi_isatty(): fix when Git is used from CMD

Hi Junio,
On Wed, 21 Dec 2016, Junio C Hamano wrote:
Show 9 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > I prepared a patch series based on `pu`, on top of Hannes' patch, and
> > I also prepared a branch that is based on `master`, obviously without
> > Hannes' patch.
> 
> I think the latter is what you have at
> 
>  $ git fetch https://github.com/dscho/git mingw-isatty-fixup-master
Correct.
> If that is correct, I am inclined to fetch that and queue it on 'pu',
> replacing Hannes's patch, and then to ask him to Test/Ack it.
Okay, I will prepare v2 based on master and addressing your feedback.

Thanks, Dscho

Junio C Hamano· Dec 11, 2016, 17:49 UTC · re: Johannes Schindelin · lore

Re: [PATCH 0/1] Fix a long-standing isatty() problem on Windows

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 5 quoted lines
> I finally got a chance to debug the problems with the ever-timing-out
> test runs of `pu` on Windows. Turns out that pb/bisect uncovered a
> really old, really bad bug: on Windows, isatty() does not do what Git
> expects, at least not completely: it detects interactive terminals *and
> character devices*.

Sounds as if somebody who did Windows at Microsoft had a good sense of humor to mimick the misnamed ENOTTY gotcha ;-)

This is a great find, and a very impactful fix, as redirecting from /dev/null is how we try to force a "go interactive if talking to tty" program to realize that it is not talking to a tty.

Thanks.
Johannes Schindelin· Dec 12, 2016, 09:59 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/1] Fix a long-standing isatty() problem on Windows

Hi Junio,
On Sun, 11 Dec 2016, Junio C Hamano wrote:
Show 10 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > I finally got a chance to debug the problems with the ever-timing-out
> > test runs of `pu` on Windows. Turns out that pb/bisect uncovered a
> > really old, really bad bug: on Windows, isatty() does not do what Git
> > expects, at least not completely: it detects interactive terminals *and
> > character devices*.
> 
> Sounds as if somebody who did Windows at Microsoft had a good sense
> of humor to mimick the misnamed ENOTTY gotcha ;-) 
Hehe...
> This is a great find, and a very impactful fix, as redirecting from
> /dev/null is how we try to force a "go interactive if talking to
> tty" program to realize that it is not talking to a tty.
Can we fast-track this to maint?

I will definitely ship this fix in the next Git for Windows version, but still, it would be nice to have this in git.git as soon as possible.

Thanks, Dscho

Junio C Hamano· Dec 12, 2016, 16:46 UTC · re: Johannes Schindelin · lore

Re: [PATCH 0/1] Fix a long-standing isatty() problem on Windows

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 5 quoted lines
>> This is a great find, and a very impactful fix, as redirecting from
>> /dev/null is how we try to force a "go interactive if talking to
>> tty" program to realize that it is not talking to a tty.
>
> Can we fast-track this to maint?

Yes, "fast-track" is a good phrase, as there is not much point cooking a subsystem specific fix like this one that came directly from a subsystem maintainer in 'next' for 1/2 to 2 weeks as usual.

Let's make sure that the first maintenance release won't ship without merging this.

Thanks.

← back to recent threads