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

From: Johannes Sixt <j6t@kdbg.org>
Date: Dec 18, 2016, 15:37 UTC
Message-ID: <d661dbf1-9852-965a-2ca9-67d763115b9e@kdbg.org>
In-Reply-To: <ffc6a7a0-4ae4-b755-0b09-5bcd7114a2e6@kdbg.org>

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?

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