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

Johannes Sixt [off-list ref] writes:

Am 11.12.2016 um 12:16 schrieb Johannes Schindelin:

quoted

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.

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()?

quoted

+		}
+	}
+
+	return res;
+}
+
 void winansi_init(void)
 {
 	int con1, con2;

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

quoted

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

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

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

Am 18.12.2016 um 16:26 schrieb Johannes Sixt:

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?

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

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?

Hi Hannes,

On Mon, 19 Dec 2016, Johannes Sixt wrote:

Am 18.12.2016 um 16:37 schrieb Johannes Sixt:

quoted

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

Hi Hannes,

On Sun, 18 Dec 2016, Johannes Sixt wrote:

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

Hi Hannes,

On Sun, 18 Dec 2016, Johannes Sixt wrote:

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 <redacted>
---
I was able to test the idea earlier than anticipated and it does work
for me.

Thank you.

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