threads / patch / 44748

patch, 2 partsReally fix the isatty() problem on Windows

Subject: [PATCH 0/2] Really fix the isatty() problem on Windows

## tl;dr

29 messages between Dec 21, 2016 and Jan 18, 2017. Diffs are folded; open one to read it.

replies: 28people: 4as markdown or json

Johannes Schindelin· Dec 21, 2016, 17:53 UTC · lore

My previous fix may have fixed running the new t/t6030-bisect-porcelain.sh script that tested the new bisect--helper, which in turn used isatty() to determine whether it was running interactively and was fooled by being redirected to /dev/null.

But it not only broke paging when running in a CMD window, due to testing in the wrong worktree I also missed that it broke paging in Git for Windows 2.x' Git Bash (i.e. a MinTTY terminal emulator).

Let's use this opportunity to actually clean up the entire isatty() mess once and for all, as part of the problem was introduced by a clever hack that messes with internals of the Microsoft C runtime, and which changed recently, so it was not such a clever hack to begin with.

Happily, one of my colleagues had to address that latter problem recently when he was tasked to make Git compile with Microsoft Visual C (the rationale: debugging facilities of Visual Studio are really outstanding, try them if you get a chance).

And incidentally, replacing the previous hack with the clean, new solution, which specifies explicitly for the file descriptors 0, 1 and 2 whether we detected an MSYS2 pseudo-tty, whether we detected a real Win32 Console, and whether we had to swap out a real Win32 Console for a pipe to allow child processes to inherit it.

Hannes, I am really sorry that I did not test the original attempt better, I tried to be a better boy this time. Could you maybe also give it a try?

The current patch series is based on `pu`, as that already has the winansi_get_osfhandle() fix. For ease of testing, I also have a branch based on master which you can pull via

	git pull https://github.com/dscho/git mingw-isatty-fixup-master
Jeff Hostetler (1):
  mingw: replace isatty() hack
Johannes Schindelin (1):
  mingw: adjust is_console() to work with stdin
 compat/winansi.c | 197 +++++++++++++++++++++++--------------------------------
 1 file changed, 83 insertions(+), 114 deletions(-)
base-commit: 1921ea0f1c7358b5200a456fba02aa79fb9e76d3
Based-On: junio/pu at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git junio/pu
Published-As: https://github.com/dscho/git/releases/tag/mingw-isatty-fixup-v1
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-isatty-fixup-v1
-- 
2.11.0.rc3.windows.1
Johannes Schindelin· Dec 21, 2016, 17:53 UTC · re: Johannes Schindelin · lore

[PATCH 1/2] mingw: adjust is_console() to work with stdin

When determining whether a handle corresponds to a *real* Win32 Console (as opposed to, say, a character device such as /dev/null), we use the GetConsoleOutputBufferInfo() function as a tell-tale.

However, that does not work for *input* handles associated with a console. Let's just use the GetConsoleMode() function for input handles, and since it does not work on output handles fall back to the previous method for those.

This patch prepares for using is_console() instead of my previous misguided attempt in cbb3f3c9b1 (mingw: intercept isatty() to handle /dev/null as Git expects it, 2016-12-11) that broke everything on Windows.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/winansi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
Show changes to compat/winansi.c +5 −1
diff --git a/compat/winansi.c b/compat/winansi.c
index 5b2f5638ec..97a004b8ab 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -84,6 +84,7 @@ static void warn_if_raster_font(void)
 static int is_console(int fd)
 {
 	CONSOLE_SCREEN_BUFFER_INFO sbi;
+	DWORD mode;
 	HANDLE hcon;
 
 	static int initialized = 0;
@@ -98,7 +99,10 @@ static int is_console(int fd)
 		return 0;
 
 	/* check if its a handle to a console output screen buffer */
-	if (!GetConsoleScreenBufferInfo(hcon, &sbi))
+	if (!fd) {
+		if (!GetConsoleMode(hcon, &mode))
+			return 0;
+	} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
 		return 0;
 
 	/* initialize attributes */

-- 
2.11.0.rc3.windows.1
Johannes Schindelin· Dec 21, 2016, 17:53 UTC · re: Johannes Schindelin · lore

[PATCH 2/2] mingw: replace isatty() hack

From: Jeff Hostetler <jeffhost@microsoft.com>

For over a year, Git for Windows has carried a patch that detects the MSYS2 pseudo ttys used by Git for Windows' default Git Bash (i.e. a terminal that is not backed by a Win32 Console).

This patch accesses internals that of a previous MSVC runtime that is no longer valid in newer versions, therefore we needed a replacement for that hack in order to be able to compile Git using recent Microsoft Visual C++.

This patch back-ports that patch and makes even the MINGW (i.e. GCC-compiled) Git use it.

As a side effect (which was the reason for the back-port), this patch also fixes the previous misguided attempt to intercept isatty() so that it handles character devices (such as /dev/null) as Git expects it.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/winansi.c | 191 +++++++++++++++++++++++--------------------------------
 1 file changed, 78 insertions(+), 113 deletions(-)
Show changes to compat/winansi.c +78 −113
diff --git a/compat/winansi.c b/compat/winansi.c
index 97a004b8ab..3bfad0d065 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -6,9 +6,12 @@
 #include "../git-compat-util.h"
 #include <wingdi.h>
 #include <winreg.h>
+#include "win32.h"
 
-/* In this file, we actually want to use Windows' own isatty(). */
-#undef isatty
+static int fd_is_interactive[3] = { 0, 0, 0 };
+#define FD_CONSOLE 0x1
+#define FD_SWAPPED 0x2
+#define FD_MSYS    0x4
 
 /*
  ANSI codes used by git: m, K
@@ -105,6 +108,9 @@ static int is_console(int fd)
 	} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
 		return 0;
 
+	if (fd >=0 && fd <= 2)
+		fd_is_interactive[fd] |= FD_CONSOLE;
+
 	/* initialize attributes */
 	if (!initialized) {
 		console = hcon;
@@ -466,76 +472,50 @@ static HANDLE duplicate_handle(HANDLE hnd)
 	return hresult;
 }
 
-
-/*
- * Make MSVCRT's internal file descriptor control structure accessible
- * so that we can tweak OS handles and flags directly (we need MSVCRT
- * to treat our pipe handle as if it were a console).
- *
- * We assume that the ioinfo structure (exposed by MSVCRT.dll via
- * __pioinfo) starts with the OS handle and the flags. The exact size
- * varies between MSVCRT versions, so we try different sizes until
- * toggling the FDEV bit of _pioinfo(1)->osflags is reflected in
- * isatty(1).
- */
-typedef struct {
-	HANDLE osfhnd;
-	char osflags;
-} ioinfo;
-
-extern __declspec(dllimport) ioinfo *__pioinfo[];
-
-static size_t sizeof_ioinfo = 0;
-
-#define IOINFO_L2E 5
-#define IOINFO_ARRAY_ELTS (1 << IOINFO_L2E)
-
-#define FPIPE 0x08
-#define FDEV  0x40
-
-static inline ioinfo* _pioinfo(int fd)
-{
-	return (ioinfo*)((char*)__pioinfo[fd >> IOINFO_L2E] +
-			(fd & (IOINFO_ARRAY_ELTS - 1)) * sizeof_ioinfo);
-}
-
-static int init_sizeof_ioinfo(void)
-{
-	int istty, wastty;
-	/* don't init twice */
-	if (sizeof_ioinfo)
-		return sizeof_ioinfo >= 256;
-
-	sizeof_ioinfo = sizeof(ioinfo);
-	wastty = isatty(1);
-	while (sizeof_ioinfo < 256) {
-		/* toggle FDEV flag, check isatty, then toggle back */
-		_pioinfo(1)->osflags ^= FDEV;
-		istty = isatty(1);
-		_pioinfo(1)->osflags ^= FDEV;
-		/* return if we found the correct size */
-		if (istty != wastty)
-			return 0;
-		sizeof_ioinfo += sizeof(void*);
-	}
-	error("Tweaking file descriptors doesn't work with this MSVCRT.dll");
-	return 1;
-}
-
 static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
 {
-	ioinfo *pioinfo;
-	HANDLE old_handle;
-
-	/* init ioinfo size if we haven't done so */
-	if (init_sizeof_ioinfo())
-		return INVALID_HANDLE_VALUE;
-
-	/* get ioinfo pointer and change the handles */
-	pioinfo = _pioinfo(fd);
-	old_handle = pioinfo->osfhnd;
-	pioinfo->osfhnd = new_handle;
-	return old_handle;
+	/*
+	 * Create a copy of the original handle associated with fd
+	 * because the original will get closed when we dup2().
+	 */
+	HANDLE handle = (HANDLE)_get_osfhandle(fd);
+	HANDLE duplicate = duplicate_handle(handle);
+
+	/* Create a temp fd associated with the already open "new_handle". */
+	int new_fd = _open_osfhandle((intptr_t)new_handle, O_BINARY);
+
+	assert((fd == 1) || (fd == 2));
+
+	/*
+	 * Use stock dup2() to re-bind fd to the new handle.  Note that
+	 * this will implicitly close(1) and close both fd=1 and the
+	 * originally associated handle.  It will open a new fd=1 and
+	 * call DuplicateHandle() on the handle associated with new_fd.
+	 * It is because of this implicit close() that we created the
+	 * copy of the original.
+	 *
+	 * Note that the OS can recycle HANDLE (numbers) just like it
+	 * recycles fd (numbers), so we must update the cached value
+	 * of "console".  You can use GetFileType() to see that
+	 * handle and _get_osfhandle(fd) may have the same number
+	 * value, but they refer to different actual files now.
+	 *
+	 * Note that dup2() when given target := {0,1,2} will also
+	 * call SetStdHandle(), so we don't need to worry about that.
+	 */
+	dup2(new_fd, fd);
+	if (console == handle)
+		console = duplicate;
+	handle = INVALID_HANDLE_VALUE;
+
+	/* Close the temp fd.  This explicitly closes "new_handle"
+	 * (because it has been associated with it).
+	 */
+	close(new_fd);
+
+	fd_is_interactive[fd] |= FD_SWAPPED;
+
+	return duplicate;
 }
 
 #ifdef DETECT_MSYS_TTY
@@ -562,49 +542,32 @@ static void detect_msys_tty(int fd)
 	name = nameinfo->Name.Buffer;
 	name[nameinfo->Name.Length / sizeof(*name)] = 0;
 
-	/* check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX') */
-	if (!wcsstr(name, L"msys-") || !wcsstr(name, L"-pty"))
+	/*
+	 * Check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX')
+	 * or a cygwin pty pipe ('cygwin-XXXX-ptyN-XX')
+	 */
+	if ((!wcsstr(name, L"msys-") && !wcsstr(name, L"cygwin-")) ||
+			!wcsstr(name, L"-pty"))
 		return;
 
-	/* init ioinfo size if we haven't done so */
-	if (init_sizeof_ioinfo())
-		return;
-
-	/* set FDEV flag, reset FPIPE flag */
-	_pioinfo(fd)->osflags &= ~FPIPE;
-	_pioinfo(fd)->osflags |= FDEV;
+	fd_is_interactive[fd] |= FD_MSYS;
 }
 
 #endif
 
+/* Wrapper for isatty().  Most calls in the main git code
+ * call isatty(1 or 2) to see if the instance is interactive
+ * and should: be colored, show progress, paginate output.
+ * We lie and give results for what the descriptor WAS at
+ * startup (and ignore any pipe redirection we internally
+ * do).
+ */
+#undef isatty
 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 = winansi_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;
+	if (fd >=0 && fd <= 2)
+		return fd_is_interactive[fd] != 0;
+	return isatty(fd);
 }
 
 void winansi_init(void)
@@ -615,6 +578,10 @@ void winansi_init(void)
 	/* check if either stdout or stderr is a console output screen buffer */
 	con1 = is_console(1);
 	con2 = is_console(2);
+
+	/* Also compute console bit for fd 0 even though we don't need the result here. */
+	is_console(0);
+
 	if (!con1 && !con2) {
 #ifdef DETECT_MSYS_TTY
 		/* check if stdin / stdout / stderr are MSYS2 pty pipes */
@@ -658,12 +625,10 @@ void winansi_init(void)
  */
 HANDLE winansi_get_osfhandle(int fd)
 {
-	HANDLE hnd = (HANDLE) _get_osfhandle(fd);
-	if (isatty(fd) && GetFileType(hnd) == FILE_TYPE_PIPE) {
-		if (fd == 1 && hconsole1)
-			return hconsole1;
-		else if (fd == 2 && hconsole2)
-			return hconsole2;
-	}
-	return hnd;
+	if (fd == 1 && (fd_is_interactive[1] & FD_SWAPPED))
+		return hconsole1;
+	if (fd == 2 && (fd_is_interactive[2] & FD_SWAPPED))
+		return hconsole2;
+
+	return (HANDLE)_get_osfhandle(fd);
 }

-- 
2.11.0.rc3.windows.1
Junio C Hamano· Dec 21, 2016, 18:45 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/2] mingw: replace isatty() hack

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 10 quoted lines
> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> For over a year, Git for Windows has carried a patch that detects the
> MSYS2 pseudo ttys used by Git for Windows' default Git Bash (i.e. a
> terminal that is not backed by a Win32 Console).
>
> This patch accesses internals that of a previous MSVC runtime that is no
> longer valid in newer versions, therefore we needed a replacement for
> that hack in order to be able to compile Git using recent Microsoft
> Visual C++.

Sorry, but I cannot parse the early part of the first sentence of the second paragraph before the comma; I am especially having trouble around the first "that".

Show 6 quoted lines
> This patch back-ports that patch and makes even the MINGW (i.e.
> GCC-compiled) Git use it.
>
> As a side effect (which was the reason for the back-port), this patch
> also fixes the previous misguided attempt to intercept isatty() so that
> it handles character devices (such as /dev/null) as Git expects it.

I had to read the above three times to understand which patches three instances of "This patch" and one instance of "that patch" refer to. I wish it were easier to read, but I think I got them all right [*1*] after re-reading, and the story made sense to me.

Show 12 quoted lines
> +static int fd_is_interactive[3] = { 0, 0, 0 };
> +#define FD_CONSOLE 0x1
> +#define FD_SWAPPED 0x2
> +#define FD_MSYS    0x4
>  
>  /*
>   ANSI codes used by git: m, K
> @@ -105,6 +108,9 @@ static int is_console(int fd)
>  	} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
>  		return 0;
>  
> +	if (fd >=0 && fd <= 2)
Style: if (fd >= 0 && fd <= 2)
> +/* Wrapper for isatty().  Most calls in the main git code
Style: /*
	* multi-line comment block begins with slash-asterisk
        * and ends with asterisk-slash without anything else on
	* the line.
	*/
Show 10 quoted lines
> + * call isatty(1 or 2) to see if the instance is interactive
> + * and should: be colored, show progress, paginate output.
> + * We lie and give results for what the descriptor WAS at
> + * startup (and ignore any pipe redirection we internally
> + * do).
> + */
> +#undef isatty
>  int winansi_isatty(int fd)
>  {
> +	if (fd >=0 && fd <= 2)
Style: if (fd >= 0 && fd <= 2)
> +		return fd_is_interactive[fd] != 0;
> +	return isatty(fd);
>  }
Thanks.
[Footnote]
*1* What I thought I understood in my own words:
    Git for Windows has carried a patch that depended on internals
    of MSVC runtime, but it does not work correctly with recent MSVC
    runtime.  A replacement was written originally for compiling
    with VC++.  The patch in this message is a backport of that
    replacement, and it also fixes the previous attempt to make
    isatty() work.
Johannes Sixt· Dec 21, 2016, 21:15 UTC · re: Johannes Schindelin · lore

Re: [PATCH 0/2] Really fix the isatty() problem on Windows

Am 21.12.2016 um 18:53 schrieb Johannes Schindelin:
Show 5 quoted lines
> The current patch series is based on `pu`, as that already has the
> winansi_get_osfhandle() fix. For ease of testing, I also have a branch
> based on master which you can pull via
>
> 	git pull https://github.com/dscho/git mingw-isatty-fixup-master
Will test and report back tomorrow.
-- Hannes
Junio C Hamano· Dec 21, 2016, 21:21 UTC · re: Johannes Sixt · lore

Re: [PATCH 0/2] Really fix the isatty() problem on Windows

Johannes Sixt <j6t@kdbg.org> writes:
Show 8 quoted lines
> Am 21.12.2016 um 18:53 schrieb Johannes Schindelin:
>> The current patch series is based on `pu`, as that already has the
>> winansi_get_osfhandle() fix. For ease of testing, I also have a branch
>> based on master which you can pull via
>>
>> 	git pull https://github.com/dscho/git mingw-isatty-fixup-master
>
> Will test and report back tomorrow.
Thanks.
Johannes Sixt· Dec 22, 2016, 18:48 UTC · re: Johannes Sixt · lore

Re: [PATCH 0/2] Really fix the isatty() problem on Windows

Am 21.12.2016 um 22:15 schrieb Johannes Sixt:
Show 8 quoted lines
> Am 21.12.2016 um 18:53 schrieb Johannes Schindelin:
>> The current patch series is based on `pu`, as that already has the
>> winansi_get_osfhandle() fix. For ease of testing, I also have a branch
>> based on master which you can pull via
>>
>>     git pull https://github.com/dscho/git mingw-isatty-fixup-master
>
> Will test and report back tomorrow.

This version 1 of the series passes the test suite (next + a handful other topics) for me. It has also undergone a bit of field testing, and things look fine.

I haven't looked at the resulting code, yet, but I don't expect to find anything fishy.

-- Hannes
Johannes Schindelin· Dec 22, 2016, 21:33 UTC · re: Johannes Sixt · lore

Re: [PATCH 0/2] Really fix the isatty() problem on Windows

Hi Hannes,
On Thu, 22 Dec 2016, Johannes Sixt wrote:
Show 16 quoted lines
> Am 21.12.2016 um 22:15 schrieb Johannes Sixt:
> > Am 21.12.2016 um 18:53 schrieb Johannes Schindelin:
> > > The current patch series is based on `pu`, as that already has the
> > > winansi_get_osfhandle() fix. For ease of testing, I also have a branch
> > > based on master which you can pull via
> > >
> > >     git pull https://github.com/dscho/git mingw-isatty-fixup-master
> >
> > Will test and report back tomorrow.
> 
> This version 1 of the series passes the test suite (next + a handful other
> topics) for me. It has also undergone a bit of field testing, and things look
> fine.
> 
> I haven't looked at the resulting code, yet, but I don't expect to find
> anything fishy.

Thanks for confirming! Dscho

Johannes Schindelin· Dec 22, 2016, 17:08 UTC · re: Johannes Schindelin · lore

[PATCH v2 0/3] Really fix the isatty() problem on Windows

My previous fix may have fixed running the new t/t6030-bisect-porcelain.sh script that tested the new bisect--helper, which in turn used isatty() to determine whether it was running interactively and was fooled by being redirected to /dev/null.

But it not only broke paging when running in a CMD window, due to testing in the wrong worktree I also missed that it broke paging in Git for Windows 2.x' Git Bash (i.e. a MinTTY terminal emulator).

Let's use this opportunity to actually clean up the entire isatty() mess once and for all, as part of the problem was introduced by a clever hack that messes with internals of the Microsoft C runtime, and which changed recently, so it was not such a clever hack to begin with.

Happily, one of my colleagues had to address that latter problem recently when he was tasked to make Git compile with Microsoft Visual C (the rationale: debugging facilities of Visual Studio are really outstanding, try them if you get a chance).

And incidentally, replacing the previous hack with the clean, new solution, which specifies explicitly for the file descriptors 0, 1 and 2 whether we detected an MSYS2 pseudo-tty, whether we detected a real Win32 Console, and whether we had to swap out a real Win32 Console for a pipe to allow child processes to inherit it.

While at it (or, actually, more like: as I already made this part of v1 by mistake), upstream the patch carried in Git for Windows that supports color when running Git for Windows in Cygwin terminals.

Changes since v1:
- rebased onto master
- unsquashed 2/3 which was improperly snuck in before,
- noted that Beat Bolli tested this (see
  https://github.com/git-for-windows/git/issues/997#issuecomment-268764693)
- fixed the confusing commit message by using Junio's suggested
  replacement
- added the missing white space between ">=" and "0"
Alan Davies (1):
  mingw: fix colourization on Cygwin pseudo terminals
Jeff Hostetler (1):
  mingw: replace isatty() hack
Johannes Schindelin (1):
  mingw: adjust is_console() to work with stdin
 compat/winansi.c | 198 +++++++++++++++++++++++--------------------------------
 1 file changed, 84 insertions(+), 114 deletions(-)
base-commit: 1d1bdafd64266e5ee3bd46c6965228f32e4022ea
Published-As: https://github.com/dscho/git/releases/tag/mingw-isatty-fixup-v2
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-isatty-fixup-v2
Interdiff vs v1:
 diff --git a/compat/winansi.c b/compat/winansi.c
 index f51a2856d2..477209fce7 100644
 --- a/compat/winansi.c
 +++ b/compat/winansi.c
 @@ -108,7 +108,7 @@ static int is_console(int fd)
  	} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
  		return 0;
  
 -	if (fd >=0 && fd <= 2)
 +	if (fd >= 0 && fd <= 2)
  		fd_is_interactive[fd] |= FD_CONSOLE;
  
  	/* initialize attributes */
 @@ -555,7 +555,8 @@ static void detect_msys_tty(int fd)
  
  #endif
  
 -/* Wrapper for isatty().  Most calls in the main git code
 +/*
 + * Wrapper for isatty().  Most calls in the main git code
   * call isatty(1 or 2) to see if the instance is interactive
   * and should: be colored, show progress, paginate output.
   * We lie and give results for what the descriptor WAS at
 @@ -565,7 +566,7 @@ static void detect_msys_tty(int fd)
  #undef isatty
  int winansi_isatty(int fd)
  {
 -	if (fd >=0 && fd <= 2)
 +	if (fd >= 0 && fd <= 2)
  		return fd_is_interactive[fd] != 0;
  	return isatty(fd);
  }
-- 
2.11.0.rc3.windows.1
Johannes Schindelin· Dec 22, 2016, 17:09 UTC · re: Johannes Schindelin · lore

[PATCH v2 2/3] mingw: fix colourization on Cygwin pseudo terminals

From: Alan Davies <alan.n.davies@gmail.com>

Git only colours the output and uses pagination if isatty() returns 1. MSYS2 and Cygwin emulate pseudo terminals via named pipes, meaning that isatty() returns 0.

f7f90e0f4f (mingw: make isatty() recognize MSYS2's pseudo terminals (/dev/pty*), 2016-04-27) fixed this for MSYS2 terminals, but not for Cygwin.

The named pipes that Cygwin and MSYS2 use are very similar. MSYS2 PTY pipes are called 'msys-*-pty*' and Cygwin uses 'cygwin-*-pty*'. This commit modifies the existing check to allow both MSYS2 and Cygwin PTY pipes to be identified as TTYs.

Note that pagination is still broken when running Git for Windows from within Cygwin, as MSYS2's less.exe is spawned (and does not like to interact with Cygwin's PTY).

This partially fixes https://github.com/git-for-windows/git/issues/267
Signed-off-by: Alan Davies <alan.n.davies@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/winansi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
Show changes to compat/winansi.c +6 −2
diff --git a/compat/winansi.c b/compat/winansi.c
index 590d61cb1b..fa37695fca 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -562,8 +562,12 @@ static void detect_msys_tty(int fd)
 	name = nameinfo->Name.Buffer;
 	name[nameinfo->Name.Length] = 0;
 
-	/* check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX') */
-	if (!wcsstr(name, L"msys-") || !wcsstr(name, L"-pty"))
+	/*
+	 * Check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX')
+	 * or a cygwin pty pipe ('cygwin-XXXX-ptyN-XX')
+	 */
+	if ((!wcsstr(name, L"msys-") && !wcsstr(name, L"cygwin-")) ||
+			!wcsstr(name, L"-pty"))
 		return;
 
 	/* init ioinfo size if we haven't done so */

-- 
2.11.0.rc3.windows.1
Johannes Schindelin· Dec 22, 2016, 17:09 UTC · re: Johannes Schindelin · lore

[PATCH v2 3/3] mingw: replace isatty() hack

From: Jeff Hostetler <jeffhost@microsoft.com>

Git for Windows has carried a patch that depended on internals of MSVC runtime, but it does not work correctly with recent MSVC runtime. A replacement was written originally for compiling with VC++. The patch in this message is a backport of that replacement, and it also fixes the previous attempt to make isatty() tell that /dev/null is *not* an interactive terminal.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Tested-by: Beat Bolli <dev+git@drbeat.li>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/winansi.c | 176 ++++++++++++++++++++++---------------------------------
 1 file changed, 69 insertions(+), 107 deletions(-)
Show changes to compat/winansi.c +69 −107
diff --git a/compat/winansi.c b/compat/winansi.c
index fa37695fca..477209fce7 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -6,9 +6,12 @@
 #include "../git-compat-util.h"
 #include <wingdi.h>
 #include <winreg.h>
+#include "win32.h"
 
-/* In this file, we actually want to use Windows' own isatty(). */
-#undef isatty
+static int fd_is_interactive[3] = { 0, 0, 0 };
+#define FD_CONSOLE 0x1
+#define FD_SWAPPED 0x2
+#define FD_MSYS    0x4
 
 /*
  ANSI codes used by git: m, K
@@ -105,6 +108,9 @@ static int is_console(int fd)
 	} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
 		return 0;
 
+	if (fd >= 0 && fd <= 2)
+		fd_is_interactive[fd] |= FD_CONSOLE;
+
 	/* initialize attributes */
 	if (!initialized) {
 		console = hcon;
@@ -466,76 +472,50 @@ static HANDLE duplicate_handle(HANDLE hnd)
 	return hresult;
 }
 
+static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
+{
+	/*
+	 * Create a copy of the original handle associated with fd
+	 * because the original will get closed when we dup2().
+	 */
+	HANDLE handle = (HANDLE)_get_osfhandle(fd);
+	HANDLE duplicate = duplicate_handle(handle);
 
-/*
- * Make MSVCRT's internal file descriptor control structure accessible
- * so that we can tweak OS handles and flags directly (we need MSVCRT
- * to treat our pipe handle as if it were a console).
- *
- * We assume that the ioinfo structure (exposed by MSVCRT.dll via
- * __pioinfo) starts with the OS handle and the flags. The exact size
- * varies between MSVCRT versions, so we try different sizes until
- * toggling the FDEV bit of _pioinfo(1)->osflags is reflected in
- * isatty(1).
- */
-typedef struct {
-	HANDLE osfhnd;
-	char osflags;
-} ioinfo;
-
-extern __declspec(dllimport) ioinfo *__pioinfo[];
-
-static size_t sizeof_ioinfo = 0;
+	/* Create a temp fd associated with the already open "new_handle". */
+	int new_fd = _open_osfhandle((intptr_t)new_handle, O_BINARY);
 
-#define IOINFO_L2E 5
-#define IOINFO_ARRAY_ELTS (1 << IOINFO_L2E)
+	assert((fd == 1) || (fd == 2));
 
-#define FPIPE 0x08
-#define FDEV  0x40
+	/*
+	 * Use stock dup2() to re-bind fd to the new handle.  Note that
+	 * this will implicitly close(1) and close both fd=1 and the
+	 * originally associated handle.  It will open a new fd=1 and
+	 * call DuplicateHandle() on the handle associated with new_fd.
+	 * It is because of this implicit close() that we created the
+	 * copy of the original.
+	 *
+	 * Note that the OS can recycle HANDLE (numbers) just like it
+	 * recycles fd (numbers), so we must update the cached value
+	 * of "console".  You can use GetFileType() to see that
+	 * handle and _get_osfhandle(fd) may have the same number
+	 * value, but they refer to different actual files now.
+	 *
+	 * Note that dup2() when given target := {0,1,2} will also
+	 * call SetStdHandle(), so we don't need to worry about that.
+	 */
+	dup2(new_fd, fd);
+	if (console == handle)
+		console = duplicate;
+	handle = INVALID_HANDLE_VALUE;
 
-static inline ioinfo* _pioinfo(int fd)
-{
-	return (ioinfo*)((char*)__pioinfo[fd >> IOINFO_L2E] +
-			(fd & (IOINFO_ARRAY_ELTS - 1)) * sizeof_ioinfo);
-}
+	/* Close the temp fd.  This explicitly closes "new_handle"
+	 * (because it has been associated with it).
+	 */
+	close(new_fd);
 
-static int init_sizeof_ioinfo(void)
-{
-	int istty, wastty;
-	/* don't init twice */
-	if (sizeof_ioinfo)
-		return sizeof_ioinfo >= 256;
-
-	sizeof_ioinfo = sizeof(ioinfo);
-	wastty = isatty(1);
-	while (sizeof_ioinfo < 256) {
-		/* toggle FDEV flag, check isatty, then toggle back */
-		_pioinfo(1)->osflags ^= FDEV;
-		istty = isatty(1);
-		_pioinfo(1)->osflags ^= FDEV;
-		/* return if we found the correct size */
-		if (istty != wastty)
-			return 0;
-		sizeof_ioinfo += sizeof(void*);
-	}
-	error("Tweaking file descriptors doesn't work with this MSVCRT.dll");
-	return 1;
-}
+	fd_is_interactive[fd] |= FD_SWAPPED;
 
-static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
-{
-	ioinfo *pioinfo;
-	HANDLE old_handle;
-
-	/* init ioinfo size if we haven't done so */
-	if (init_sizeof_ioinfo())
-		return INVALID_HANDLE_VALUE;
-
-	/* get ioinfo pointer and change the handles */
-	pioinfo = _pioinfo(fd);
-	old_handle = pioinfo->osfhnd;
-	pioinfo->osfhnd = new_handle;
-	return old_handle;
+	return duplicate;
 }
 
 #ifdef DETECT_MSYS_TTY
@@ -570,45 +550,25 @@ static void detect_msys_tty(int fd)
 			!wcsstr(name, L"-pty"))
 		return;
 
-	/* init ioinfo size if we haven't done so */
-	if (init_sizeof_ioinfo())
-		return;
-
-	/* set FDEV flag, reset FPIPE flag */
-	_pioinfo(fd)->osflags &= ~FPIPE;
-	_pioinfo(fd)->osflags |= FDEV;
+	fd_is_interactive[fd] |= FD_MSYS;
 }
 
 #endif
 
+/*
+ * Wrapper for isatty().  Most calls in the main git code
+ * call isatty(1 or 2) to see if the instance is interactive
+ * and should: be colored, show progress, paginate output.
+ * We lie and give results for what the descriptor WAS at
+ * startup (and ignore any pipe redirection we internally
+ * do).
+ */
+#undef isatty
 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;
+	if (fd >= 0 && fd <= 2)
+		return fd_is_interactive[fd] != 0;
+	return isatty(fd);
 }
 
 void winansi_init(void)
@@ -619,6 +579,10 @@ void winansi_init(void)
 	/* check if either stdout or stderr is a console output screen buffer */
 	con1 = is_console(1);
 	con2 = is_console(2);
+
+	/* Also compute console bit for fd 0 even though we don't need the result here. */
+	is_console(0);
+
 	if (!con1 && !con2) {
 #ifdef DETECT_MSYS_TTY
 		/* check if stdin / stdout / stderr are MSYS2 pty pipes */
@@ -662,12 +626,10 @@ void winansi_init(void)
  */
 HANDLE winansi_get_osfhandle(int fd)
 {
-	HANDLE hnd = (HANDLE) _get_osfhandle(fd);
-	if (isatty(fd) && GetFileType(hnd) == FILE_TYPE_PIPE) {
-		if (fd == 1 && hconsole1)
-			return hconsole1;
-		else if (fd == 2 && hconsole2)
-			return hconsole2;
-	}
-	return hnd;
+	if (fd == 1 && (fd_is_interactive[1] & FD_SWAPPED))
+		return hconsole1;
+	if (fd == 2 && (fd_is_interactive[2] & FD_SWAPPED))
+		return hconsole2;
+
+	return (HANDLE)_get_osfhandle(fd);
 }

-- 
2.11.0.rc3.windows.1
Johannes Sixt· Dec 22, 2016, 20:26 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 3/3] mingw: replace isatty() hack

I've only one request for clarification below. Otherwise, the patch looks good.

(lines removed by the patch trimmed)
Am 22.12.2016 um 18:09 schrieb Johannes Schindelin:
Show 27 quoted lines
> +static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
> +{
> +	/*
> +	 * Create a copy of the original handle associated with fd
> +	 * because the original will get closed when we dup2().
> +	 */
> +	HANDLE handle = (HANDLE)_get_osfhandle(fd);
> +	HANDLE duplicate = duplicate_handle(handle);
>
> +	/* Create a temp fd associated with the already open "new_handle". */
> +	int new_fd = _open_osfhandle((intptr_t)new_handle, O_BINARY);
>
> +	assert((fd == 1) || (fd == 2));
>
> +	/*
> +	 * Use stock dup2() to re-bind fd to the new handle.  Note that
> +	 * this will implicitly close(1) and close both fd=1 and the
> +	 * originally associated handle.  It will open a new fd=1 and
> +	 * call DuplicateHandle() on the handle associated with new_fd.
> +	 * It is because of this implicit close() that we created the
> +	 * copy of the original.
> +	 *
> +	 * Note that the OS can recycle HANDLE (numbers) just like it
> +	 * recycles fd (numbers), so we must update the cached value
> +	 * of "console".  You can use GetFileType() to see that
> +	 * handle and _get_osfhandle(fd) may have the same number
> +	 * value, but they refer to different actual files now.

Certainly, the OS does not recycle handle values that are in use (open). Then I do not quite get the point of this paragraph. See...

Show 7 quoted lines
> +	 *
> +	 * Note that dup2() when given target := {0,1,2} will also
> +	 * call SetStdHandle(), so we don't need to worry about that.
> +	 */
> +	dup2(new_fd, fd);
> +	if (console == handle)
> +		console = duplicate;

... This is where "the cached value of console is updated", right? If console == handle, then this is not because a handle value was recycled, but because fd *was* console. Since the old value of console has been closed by the dup2(), we must refer to the back-up value in the future. Am I missing something?

Show 11 quoted lines
> +	handle = INVALID_HANDLE_VALUE;
>
> +	/* Close the temp fd.  This explicitly closes "new_handle"
> +	 * (because it has been associated with it).
> +	 */
> +	close(new_fd);
>
> +	fd_is_interactive[fd] |= FD_SWAPPED;
>
> +	return duplicate;
>  }
Johannes Schindelin· Dec 22, 2016, 21:37 UTC · re: Johannes Sixt · lore

Re: [PATCH v2 3/3] mingw: replace isatty() hack

Hi Hannes,
On Thu, 22 Dec 2016, Johannes Sixt wrote:
Show 45 quoted lines
> Am 22.12.2016 um 18:09 schrieb Johannes Schindelin:
> > +static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
> > +{
> > +	/*
> > +	 * Create a copy of the original handle associated with fd
> > +	 * because the original will get closed when we dup2().
> > +	 */
> > +	HANDLE handle = (HANDLE)_get_osfhandle(fd);
> > +	HANDLE duplicate = duplicate_handle(handle);
> >
> > +	/* Create a temp fd associated with the already open "new_handle". */
> > +	int new_fd = _open_osfhandle((intptr_t)new_handle, O_BINARY);
> >
> > +	assert((fd == 1) || (fd == 2));
> >
> > +	/*
> > +	 * Use stock dup2() to re-bind fd to the new handle.  Note that
> > +	 * this will implicitly close(1) and close both fd=1 and the
> > +	 * originally associated handle.  It will open a new fd=1 and
> > +	 * call DuplicateHandle() on the handle associated with new_fd.
> > +	 * It is because of this implicit close() that we created the
> > +	 * copy of the original.
> > +	 *
> > +	 * Note that the OS can recycle HANDLE (numbers) just like it
> > +	 * recycles fd (numbers), so we must update the cached value
> > +	 * of "console".  You can use GetFileType() to see that
> > +	 * handle and _get_osfhandle(fd) may have the same number
> > +	 * value, but they refer to different actual files now.
> 
> Certainly, the OS does not recycle handle values that are in use (open). Then
> I do not quite get the point of this paragraph. See...
> 
> > +	 *
> > +	 * Note that dup2() when given target := {0,1,2} will also
> > +	 * call SetStdHandle(), so we don't need to worry about that.
> > +	 */
> > +	dup2(new_fd, fd);
> > +	if (console == handle)
> > +		console = duplicate;
> 
> ... This is where "the cached value of console is updated", right? If console
> == handle, then this is not because a handle value was recycled, but because
> fd *was* console. Since the old value of console has been closed by the
> dup2(), we must refer to the back-up value in the future. Am I missing
> something?

You are correct, we must update `console` because `handle` is no longer the handle we want.

The comment above only meant to reinforce that we have to forget about the previous handle, too, as we might access something completely different than a console otherwise.

Would you have a suggestion how to rephrase the comment to make it less confusing?

Ciao, Dscho

Johannes Sixt· Dec 22, 2016, 22:28 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 3/3] mingw: replace isatty() hack

Am 22.12.2016 um 22:37 schrieb Johannes Schindelin:
Show 56 quoted lines
> Hi Hannes,
>
> On Thu, 22 Dec 2016, Johannes Sixt wrote:
>
>> Am 22.12.2016 um 18:09 schrieb Johannes Schindelin:
>>> +static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
>>> +{
>>> +	/*
>>> +	 * Create a copy of the original handle associated with fd
>>> +	 * because the original will get closed when we dup2().
>>> +	 */
>>> +	HANDLE handle = (HANDLE)_get_osfhandle(fd);
>>> +	HANDLE duplicate = duplicate_handle(handle);
>>>
>>> +	/* Create a temp fd associated with the already open "new_handle". */
>>> +	int new_fd = _open_osfhandle((intptr_t)new_handle, O_BINARY);
>>>
>>> +	assert((fd == 1) || (fd == 2));
>>>
>>> +	/*
>>> +	 * Use stock dup2() to re-bind fd to the new handle.  Note that
>>> +	 * this will implicitly close(1) and close both fd=1 and the
>>> +	 * originally associated handle.  It will open a new fd=1 and
>>> +	 * call DuplicateHandle() on the handle associated with new_fd.
>>> +	 * It is because of this implicit close() that we created the
>>> +	 * copy of the original.
>>> +	 *
>>> +	 * Note that the OS can recycle HANDLE (numbers) just like it
>>> +	 * recycles fd (numbers), so we must update the cached value
>>> +	 * of "console".  You can use GetFileType() to see that
>>> +	 * handle and _get_osfhandle(fd) may have the same number
>>> +	 * value, but they refer to different actual files now.
>>
>> Certainly, the OS does not recycle handle values that are in use (open). Then
>> I do not quite get the point of this paragraph. See...
>>
>>> +	 *
>>> +	 * Note that dup2() when given target := {0,1,2} will also
>>> +	 * call SetStdHandle(), so we don't need to worry about that.
>>> +	 */
>>> +	dup2(new_fd, fd);
>>> +	if (console == handle)
>>> +		console = duplicate;
>>
>> ... This is where "the cached value of console is updated", right? If console
>> == handle, then this is not because a handle value was recycled, but because
>> fd *was* console. Since the old value of console has been closed by the
>> dup2(), we must refer to the back-up value in the future. Am I missing
>> something?
>
> You are correct, we must update `console` because `handle` is no longer
> the handle we want.
>
> The comment above only meant to reinforce that we have to forget about the
> previous handle, too, as we might access something completely different
> than a console otherwise.
It is like accessing a pointer after free().

When I read the paragraph for the first time, I expected some information to be saved, then some handles to be closed and re-opened, which would possibly recycle a handle value, and then same state to be resurrected depending on the information saved earlier.

But nothing of this kind happens, only:
	dup2(new_fd, fd);
	if (console == handle)
		console = duplicate;
which is necessary and, once one has understood the context, obvious.
> Would you have a suggestion how to rephrase the comment to make it less
> confusing?
Perhaps
	 * This might close the cached console handle.
	 * We must cache the live duplicate instead.

Then update the cache before the dup2, because at this time all 3 of console, handle, and duplicate are live and cannot be recycled:

	if (console == handle)
		console = duplicate;
	dup2(new_fd, fd);
-- Hannes
Johannes Schindelin· Dec 22, 2016, 23:18 UTC · re: Johannes Sixt · lore

Re: [PATCH v2 3/3] mingw: replace isatty() hack

Hi Hannes,
On Thu, 22 Dec 2016, Johannes Sixt wrote:
Show 16 quoted lines
> Am 22.12.2016 um 22:37 schrieb Johannes Schindelin:
>
> > Would you have a suggestion how to rephrase the comment to make it
> > less confusing?
> 
> Perhaps
> 
> 	 * This might close the cached console handle.
> 	 * We must cache the live duplicate instead.
> 
> Then update the cache before the dup2, because at this time all 3 of console,
> handle, and duplicate are live and cannot be recycled:
> 
> 	if (console == handle)
> 		console = duplicate;
> 	dup2(new_fd, fd);

Good point. I reworded the comment slightly differently (see the interdiff in v3 0/3), hope you don't mind.

Ciao, Dscho

Johannes Schindelin· Dec 22, 2016, 17:08 UTC · re: Johannes Schindelin · lore

[PATCH v2 1/3] mingw: adjust is_console() to work with stdin

When determining whether a handle corresponds to a *real* Win32 Console (as opposed to, say, a character device such as /dev/null), we use the GetConsoleOutputBufferInfo() function as a tell-tale.

However, that does not work for *input* handles associated with a console. Let's just use the GetConsoleMode() function for input handles, and since it does not work on output handles fall back to the previous method for those.

This patch prepares for using is_console() instead of my previous misguided attempt in cbb3f3c9b1 (mingw: intercept isatty() to handle /dev/null as Git expects it, 2016-12-11) that broke everything on Windows.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/winansi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
Show changes to compat/winansi.c +5 −1
diff --git a/compat/winansi.c b/compat/winansi.c
index cb725fb02f..590d61cb1b 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -84,6 +84,7 @@ static void warn_if_raster_font(void)
 static int is_console(int fd)
 {
 	CONSOLE_SCREEN_BUFFER_INFO sbi;
+	DWORD mode;
 	HANDLE hcon;
 
 	static int initialized = 0;
@@ -98,7 +99,10 @@ static int is_console(int fd)
 		return 0;
 
 	/* check if its a handle to a console output screen buffer */
-	if (!GetConsoleScreenBufferInfo(hcon, &sbi))
+	if (!fd) {
+		if (!GetConsoleMode(hcon, &mode))
+			return 0;
+	} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
 		return 0;
 
 	/* initialize attributes */

-- 
2.11.0.rc3.windows.1
Beat Bolli· Dec 22, 2016, 23:04 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 1/3] mingw: adjust is_console() to work with stdin

On 22.12.16 18:08, Johannes Schindelin wrote:
Show 28 quoted lines
> When determining whether a handle corresponds to a *real* Win32 Console
> (as opposed to, say, a character device such as /dev/null), we use the
> GetConsoleOutputBufferInfo() function as a tell-tale.
> 
> However, that does not work for *input* handles associated with a
> console. Let's just use the GetConsoleMode() function for input handles,
> and since it does not work on output handles fall back to the previous
> method for those.
> 
> This patch prepares for using is_console() instead of my previous
> misguided attempt in cbb3f3c9b1 (mingw: intercept isatty() to handle
> /dev/null as Git expects it, 2016-12-11) that broke everything on
> Windows.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  compat/winansi.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/compat/winansi.c b/compat/winansi.c
> index cb725fb02f..590d61cb1b 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -84,6 +84,7 @@ static void warn_if_raster_font(void)
>  static int is_console(int fd)
>  {
>  	CONSOLE_SCREEN_BUFFER_INFO sbi;
> +	DWORD mode;
Nit: can we move this definition into the block below where it's used?
Show 9 quoted lines
>  	HANDLE hcon;
>  
>  	static int initialized = 0;
> @@ -98,7 +99,10 @@ static int is_console(int fd)
>  		return 0;
>  
>  	/* check if its a handle to a console output screen buffer */
> -	if (!GetConsoleScreenBufferInfo(hcon, &sbi))
> +	if (!fd) {
Right here:
+               DWORD mode;
Show 7 quoted lines
> +		if (!GetConsoleMode(hcon, &mode))
> +			return 0;
> +	} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
>  		return 0;
>  
>  	/* initialize attributes */
>
Junio C Hamano· Dec 22, 2016, 23:18 UTC · re: Beat Bolli · lore

Re: [PATCH v2 1/3] mingw: adjust is_console() to work with stdin

Beat Bolli <dev+git@drbeat.li> writes:
Show 25 quoted lines
>> @@ -84,6 +84,7 @@ static void warn_if_raster_font(void)
>>  static int is_console(int fd)
>>  {
>>  	CONSOLE_SCREEN_BUFFER_INFO sbi;
>> +	DWORD mode;
>
> Nit: can we move this definition into the block below where it's used?
>
>>  	HANDLE hcon;
>>  
>>  	static int initialized = 0;
>> @@ -98,7 +99,10 @@ static int is_console(int fd)
>>  		return 0;
>>  
>>  	/* check if its a handle to a console output screen buffer */
>> -	if (!GetConsoleScreenBufferInfo(hcon, &sbi))
>> +	if (!fd) {
>
> Right here:
> +               DWORD mode;
>
>> +		if (!GetConsoleMode(hcon, &mode))
>> +			return 0;
>> +	} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
>>  		return 0;

As these patches have been already merged to 'next' a few hours ago as part of today's integration cycle, please make any further refinement (if necessary) as incremental updates.

Thanks.
Johannes Schindelin· Dec 23, 2016, 09:30 UTC · re: Beat Bolli · lore

Re: [PATCH v2 1/3] mingw: adjust is_console() to work with stdin

Hi Beat,
On Fri, 23 Dec 2016, Beat Bolli wrote:
Show 25 quoted lines
> On 22.12.16 18:08, Johannes Schindelin wrote:
> > diff --git a/compat/winansi.c b/compat/winansi.c
> > index cb725fb02f..590d61cb1b 100644
> > --- a/compat/winansi.c
> > +++ b/compat/winansi.c
> > @@ -84,6 +84,7 @@ static void warn_if_raster_font(void)
> >  static int is_console(int fd)
> >  {
> >  	CONSOLE_SCREEN_BUFFER_INFO sbi;
> > +	DWORD mode;
> 
> Nit: can we move this definition into the block below where it's used?
> 
> >  	HANDLE hcon;
> >  
> >  	static int initialized = 0;
> > @@ -98,7 +99,10 @@ static int is_console(int fd)
> >  		return 0;
> >  
> >  	/* check if its a handle to a console output screen buffer */
> > -	if (!GetConsoleScreenBufferInfo(hcon, &sbi))
> > +	if (!fd) {
> 
> Right here:
> +               DWORD mode;

By that reasoning, the CONSOLE_SCREEN_BUFFER_INFO declaration that has function-wide scope should also move below:

> > +		if (!GetConsoleMode(hcon, &mode))
> > +			return 0;
Right here.
> > +	} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
> >  		return 0;
> >  
> >  	/* initialize attributes */
As the existing code followed a different convention, so does my patch.

If you choose to submit a change that moved the `mode` declaration to narrow its scope, please also move the `sbi` declaration for consistency.

Ciao, Dscho

Beat Bolli· Dec 23, 2016, 12:51 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 1/3] mingw: adjust is_console() to work with stdin

Hi Dscho
On 2016-12-23 10:30, Johannes Schindelin wrote:
Show 48 quoted lines
> Hi Beat,
> 
> On Fri, 23 Dec 2016, Beat Bolli wrote:
> 
>> On 22.12.16 18:08, Johannes Schindelin wrote:
>> > diff --git a/compat/winansi.c b/compat/winansi.c
>> > index cb725fb02f..590d61cb1b 100644
>> > --- a/compat/winansi.c
>> > +++ b/compat/winansi.c
>> > @@ -84,6 +84,7 @@ static void warn_if_raster_font(void)
>> >  static int is_console(int fd)
>> >  {
>> >  	CONSOLE_SCREEN_BUFFER_INFO sbi;
>> > +	DWORD mode;
>> 
>> Nit: can we move this definition into the block below where it's used?
>> 
>> >  	HANDLE hcon;
>> >
>> >  	static int initialized = 0;
>> > @@ -98,7 +99,10 @@ static int is_console(int fd)
>> >  		return 0;
>> >
>> >  	/* check if its a handle to a console output screen buffer */
>> > -	if (!GetConsoleScreenBufferInfo(hcon, &sbi))
>> > +	if (!fd) {
>> 
>> Right here:
>> +               DWORD mode;
> 
> By that reasoning, the CONSOLE_SCREEN_BUFFER_INFO declaration that has
> function-wide scope should also move below:
> 
>> > +		if (!GetConsoleMode(hcon, &mode))
>> > +			return 0;
> 
> Right here.
> 
>> > +	} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
>> >  		return 0;
>> >
>> >  	/* initialize attributes */
> 
> As the existing code followed a different convention, so does my patch.
> 
> If you choose to submit a change that moved the `mode` declaration to
> narrow its scope, please also move the `sbi` declaration for 
> consistency.

It's probably not worth it. It just jumped at me when reading the patch, and, writing much C++ recently, it looked weird to have a definition so far away from the single use of the variable.

Cheers, Beat

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

Re: [PATCH v2 0/3] Really fix the isatty() problem on Windows

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 34 quoted lines
> My previous fix may have fixed running the new
> t/t6030-bisect-porcelain.sh script that tested the new bisect--helper,
> which in turn used isatty() to determine whether it was running
> interactively and was fooled by being redirected to /dev/null.
>
> But it not only broke paging when running in a CMD window, due to
> testing in the wrong worktree I also missed that it broke paging in Git
> for Windows 2.x' Git Bash (i.e. a MinTTY terminal emulator).
>
> Let's use this opportunity to actually clean up the entire isatty() mess
> once and for all, as part of the problem was introduced by a clever hack
> that messes with internals of the Microsoft C runtime, and which changed
> recently, so it was not such a clever hack to begin with.
>
> Happily, one of my colleagues had to address that latter problem
> recently when he was tasked to make Git compile with Microsoft Visual C
> (the rationale: debugging facilities of Visual Studio are really
> outstanding, try them if you get a chance).
>
> And incidentally, replacing the previous hack with the clean, new
> solution, which specifies explicitly for the file descriptors 0, 1 and 2
> whether we detected an MSYS2 pseudo-tty, whether we detected a real
> Win32 Console, and whether we had to swap out a real Win32 Console for a
> pipe to allow child processes to inherit it.
>
> While at it (or, actually, more like: as I already made this part of v1
> by mistake), upstream the patch carried in Git for Windows that supports
> color when running Git for Windows in Cygwin terminals.
>
> Changes since v1:
>
> - rebased onto master
>
> - unsquashed 2/3 which was improperly snuck in before,
As Windows specific changes, I didn't notice these two were independent.
Show 5 quoted lines
> - noted that Beat Bolli tested this (see
>   https://github.com/git-for-windows/git/issues/997#issuecomment-268764693)
>
> - fixed the confusing commit message by using Junio's suggested
>   replacement

Sorry, but I didn't mean to "suggest replacement". I was just testing my understanding by attempt to rephrase the gist of it.

There was one thing I still wasn't clear in my "summary of my understanding". Is the "replacement originally done for compiling with VC++" a solution that still peeks into MSVC runtime internals but is usable with both old and more recent one? Or is it a more kosher approach that does not play with the internals to make it unlikely that it would have to change again in the future?

Your "use this opportunity to actually clean up" above suggests that the answer is the latter, but if you took my "summary of my understanding", it is likely that that fact is not captured in the resulting log message.

The interdiff obviously looks good. Let's move this series forward. I'll see if it can be merged down to 'maint', too, but it probably would not matter that much.

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

Re: [PATCH v2 0/3] Really fix the isatty() problem on Windows

Hi Junio,
On Thu, 22 Dec 2016, Junio C Hamano wrote:
Show 5 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > - unsquashed 2/3 which was improperly snuck in before,
> 
> As Windows specific changes, I didn't notice these two were independent.

Yeah, sorry, I only realized today that I had snuck that one in when re-reviewing the changes.

Show 5 quoted lines
> > - fixed the confusing commit message by using Junio's suggested
> >   replacement
> 
> Sorry, but I didn't mean to "suggest replacement".  I was just
> testing my understanding by attempt to rephrase the gist of it.
I did like your phrasing, though.
Show 6 quoted lines
> There was one thing I still wasn't clear in my "summary of my
> understanding".  Is the "replacement originally done for compiling
> with VC++" a solution that still peeks into MSVC runtime internals
> but is usable with both old and more recent one?  Or is it a more
> kosher approach that does not play with the internals to make it
> unlikely that it would have to change again in the future?
Oh, it is kosher. There is no more messing with internals.
> Your "use this opportunity to actually clean up" above suggests that
> the answer is the latter, but if you took my "summary of my
> understanding", it is likely that that fact is not captured in the
> resulting log message.

Right... I tried to make that clear by saying that this change replaces the hack.

> The interdiff obviously looks good.  Let's move this series forward.
> I'll see if it can be merged down to 'maint', too, but it probably
> would not matter that much.

I will have to release a new Git for Windows version Real Soon Now [*1*], so I will have to take those patches kind of there (as Git for Windows will be based on upstream/maint for a while now).

My thinking is that I will publish a prerelease either tonight or tomorrow, then go offline until next year, and then immediately publish Git for Windows v2.11.0(2) (or v2.11.1 if you publish a bugfix version in the meantime).

Ciao, Dscho

Footnote *1*: There is a new cURL version with some security fixes, although I do not think they are super-critical, then there is the fix for the double slashes in //server/share/directory, the fix for the empty credentials and I should probably also try to update the MSYS2 runtime to the newest version of Cygwin's runtime. Lots of stuff.

Junio C Hamano· Dec 22, 2016, 18:32 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 0/3] Really fix the isatty() problem on Windows

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> Sorry, but I didn't mean to "suggest replacement".  I was just
>> testing my understanding by attempt to rephrase the gist of it.
>
> I did like your phrasing, though.

OK, then let's use these three patches as-is. I just made sure that they can go to maintenance track for 2.11.x; so all should be good.

Thanks.
Junio C Hamano· Dec 22, 2016, 18:19 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 0/3] Really fix the isatty() problem on Windows

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> ...
>> - fixed the confusing commit message by using Junio's suggested
>>   replacement
>
> Sorry, but I didn't mean to "suggest replacement".  I was just
> testing my understanding by attempt to rephrase the gist of it.
> ...
> Your "use this opportunity to actually clean up" above suggests that
> the answer is the latter, but if you took my "summary of my
> understanding", it is likely that that fact is not captured in the
> resulting log message.

So using the original log message in v1 and what you wrote in the message I was responding to as references, let me try a real "suggested" one as penance.

I need to ask one clarification on what you wrote before completing that effort, though.

Show 5 quoted lines
>> And incidentally, replacing the previous hack with the clean, new
>> solution, which specifies explicitly for the file descriptors 0, 1 and 2
>> whether we detected an MSYS2 pseudo-tty, whether we detected a real
>> Win32 Console, and whether we had to swap out a real Win32 Console for a
>> pipe to allow child processes to inherit it.
This has subject but not verb.  I parsed the above like so:
    Replacing the previous hack with the clean, new solution (which
    specifies explicitly for the file descriptors 0, 1 and 2
     - whether we detected an MSYS2 pseudo-tty, 
     - whether we detected a real Win32 Console, and 
     - whether we had to swap out a real Win32 Console for a
       pipe to allow child processes to inherit it
    )

So the entire thing is a noun phrase "replacing with a new patch", and I take that as the subject of an unfinished sentence. What did that subject do? Replacing with a new patch allows us to do "this wonderful thing", but what "this wonderful thing" is not clear.

Subject: mingw: replace isatty() hack
From: Jeff Hostetler <jeffhost@microsoft.com>

For over a year, Git for Windows has carried a patch that detects the MSYS2 pseudo ttys used by Git for Windows' default Git Bash (i.e. a terminal that is not backed by a Win32 Console), but it did so by accessing internals of a previous MSVC runtime that is no longer valid in newer versions.

Clean up this mess by backporting a patch that was originally done to compile Git with a recent VC++. Replacing the previous hack with the clean, new solution, which specifies explicitly for the file descriptors 0, 1 and 2 whether we detected an MSYS2 pseudo-tty, whether we detected a real Win32 Console, and whether we had to swap out a real Win32 Console for a pipe to allow child processes to inherit it, lets us do XXXXX.

As a side effect (which was the reason for the back-port), this patch also fixes the previous misguided attempt to intercept isatty() so that it handles character devices (such as /dev/null) as Git expects it.

Johannes Schindelin· Dec 22, 2016, 23:16 UTC · re: Johannes Schindelin · lore

[PATCH v3 0/3] Really fix the isatty() problem on Windows

My previous fix may have fixed running the new t/t6030-bisect-porcelain.sh script that tested the new bisect--helper, which in turn used isatty() to determine whether it was running interactively and was fooled by being redirected to /dev/null.

But it not only broke paging when running in a CMD window, due to testing in the wrong worktree I also missed that it broke paging in Git for Windows 2.x' Git Bash (i.e. a MinTTY terminal emulator).

Let's use this opportunity to actually clean up the entire isatty() mess once and for all, as part of the problem was introduced by a clever hack that messes with internals of the Microsoft C runtime, and which changed recently, so it was not such a clever hack to begin with.

Happily, one of my colleagues had to address that latter problem recently when he was tasked to make Git compile with Microsoft Visual C (the rationale: debugging facilities of Visual Studio are really outstanding, try them if you get a chance).

And incidentally, replacing the previous hack with the clean, new solution, which specifies explicitly for the file descriptors 0, 1 and 2 whether we detected an MSYS2 pseudo-tty, whether we detected a real Win32 Console, and whether we had to swap out a real Win32 Console for a pipe to allow child processes to inherit it.

While at it (or, actually, more like: as I already made this part of v1 by mistake), upstream the patch carried in Git for Windows that supports color when running Git for Windows in Cygwin terminals.

Changes since v2:
- reworded the comment about "recycling handles"
- moved the reassignment of the `console` variable before the dup2()
  call so that it is valid at all times
- removed the "handle = INVALID_HANDLE_VALUE" assignment, as the local
  variable `handle` is not used afterwards anyway
- generated the patches with --patience in the hope to improve
  readability
Alan Davies (1):
  mingw: fix colourization on Cygwin pseudo terminals
Jeff Hostetler (1):
  mingw: replace isatty() hack
Johannes Schindelin (1):
  mingw: adjust is_console() to work with stdin
 compat/winansi.c | 195 +++++++++++++++++++++++--------------------------------
 1 file changed, 81 insertions(+), 114 deletions(-)
base-commit: 1d1bdafd64266e5ee3bd46c6965228f32e4022ea
Published-As: https://github.com/dscho/git/releases/tag/mingw-isatty-fixup-v3
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-isatty-fixup-v3
Interdiff vs v2:
 diff --git a/compat/winansi.c b/compat/winansi.c
 index 477209fce7..56658ec4f8 100644
 --- a/compat/winansi.c
 +++ b/compat/winansi.c
 @@ -494,19 +494,16 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
  	 * It is because of this implicit close() that we created the
  	 * copy of the original.
  	 *
 -	 * Note that the OS can recycle HANDLE (numbers) just like it
 -	 * recycles fd (numbers), so we must update the cached value
 -	 * of "console".  You can use GetFileType() to see that
 -	 * handle and _get_osfhandle(fd) may have the same number
 -	 * value, but they refer to different actual files now.
 +	 * Note that we need to update the cached console handle to the
 +	 * duplicated one because the dup2() call will implicitly close
 +	 * the original one.
  	 *
  	 * Note that dup2() when given target := {0,1,2} will also
  	 * call SetStdHandle(), so we don't need to worry about that.
  	 */
 -	dup2(new_fd, fd);
  	if (console == handle)
  		console = duplicate;
 -	handle = INVALID_HANDLE_VALUE;
 +	dup2(new_fd, fd);
  
  	/* Close the temp fd.  This explicitly closes "new_handle"
  	 * (because it has been associated with it).
-- 
2.11.0.rc3.windows.1
Johannes Schindelin· Dec 22, 2016, 23:16 UTC · re: Johannes Schindelin · lore

[PATCH v3 1/3] mingw: adjust is_console() to work with stdin

When determining whether a handle corresponds to a *real* Win32 Console (as opposed to, say, a character device such as /dev/null), we use the GetConsoleOutputBufferInfo() function as a tell-tale.

However, that does not work for *input* handles associated with a console. Let's just use the GetConsoleMode() function for input handles, and since it does not work on output handles fall back to the previous method for those.

This patch prepares for using is_console() instead of my previous misguided attempt in cbb3f3c9b1 (mingw: intercept isatty() to handle /dev/null as Git expects it, 2016-12-11) that broke everything on Windows.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/winansi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
Show changes to compat/winansi.c +5 −1
diff --git a/compat/winansi.c b/compat/winansi.c
index cb725fb02f..590d61cb1b 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -84,6 +84,7 @@ static void warn_if_raster_font(void)
 static int is_console(int fd)
 {
 	CONSOLE_SCREEN_BUFFER_INFO sbi;
+	DWORD mode;
 	HANDLE hcon;
 
 	static int initialized = 0;
@@ -98,7 +99,10 @@ static int is_console(int fd)
 		return 0;
 
 	/* check if its a handle to a console output screen buffer */
-	if (!GetConsoleScreenBufferInfo(hcon, &sbi))
+	if (!fd) {
+		if (!GetConsoleMode(hcon, &mode))
+			return 0;
+	} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
 		return 0;
 
 	/* initialize attributes */

-- 
2.11.0.rc3.windows.1
Johannes Schindelin· Dec 22, 2016, 23:16 UTC · re: Johannes Schindelin · lore

[PATCH v3 2/3] mingw: fix colourization on Cygwin pseudo terminals

From: Alan Davies <alan.n.davies@gmail.com>

Git only colours the output and uses pagination if isatty() returns 1. MSYS2 and Cygwin emulate pseudo terminals via named pipes, meaning that isatty() returns 0.

f7f90e0f4f (mingw: make isatty() recognize MSYS2's pseudo terminals (/dev/pty*), 2016-04-27) fixed this for MSYS2 terminals, but not for Cygwin.

The named pipes that Cygwin and MSYS2 use are very similar. MSYS2 PTY pipes are called 'msys-*-pty*' and Cygwin uses 'cygwin-*-pty*'. This commit modifies the existing check to allow both MSYS2 and Cygwin PTY pipes to be identified as TTYs.

Note that pagination is still broken when running Git for Windows from within Cygwin, as MSYS2's less.exe is spawned (and does not like to interact with Cygwin's PTY).

This partially fixes https://github.com/git-for-windows/git/issues/267
Signed-off-by: Alan Davies <alan.n.davies@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/winansi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
Show changes to compat/winansi.c +6 −2
diff --git a/compat/winansi.c b/compat/winansi.c
index 590d61cb1b..fa37695fca 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -562,8 +562,12 @@ static void detect_msys_tty(int fd)
 	name = nameinfo->Name.Buffer;
 	name[nameinfo->Name.Length] = 0;
 
-	/* check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX') */
-	if (!wcsstr(name, L"msys-") || !wcsstr(name, L"-pty"))
+	/*
+	 * Check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX')
+	 * or a cygwin pty pipe ('cygwin-XXXX-ptyN-XX')
+	 */
+	if ((!wcsstr(name, L"msys-") && !wcsstr(name, L"cygwin-")) ||
+			!wcsstr(name, L"-pty"))
 		return;
 
 	/* init ioinfo size if we haven't done so */

-- 
2.11.0.rc3.windows.1
Johannes Schindelin· Dec 22, 2016, 23:16 UTC · re: Johannes Schindelin · lore

[PATCH v3 3/3] mingw: replace isatty() hack

From: Jeff Hostetler <jeffhost@microsoft.com>

Git for Windows has carried a patch that depended on internals of MSVC runtime, but it does not work correctly with recent MSVC runtime. A replacement was written originally for compiling with VC++. The patch in this message is a backport of that replacement, and it also fixes the previous attempt to make isatty() tell that /dev/null is *not* an interactive terminal.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Tested-by: Beat Bolli <dev+git@drbeat.li>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/winansi.c | 181 +++++++++++++++++++++----------------------------------
 1 file changed, 70 insertions(+), 111 deletions(-)
Show changes to compat/winansi.c +70 −111
diff --git a/compat/winansi.c b/compat/winansi.c
index fa37695fca..56658ec4f8 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -6,9 +6,12 @@
 #include "../git-compat-util.h"
 #include <wingdi.h>
 #include <winreg.h>
+#include "win32.h"
 
-/* In this file, we actually want to use Windows' own isatty(). */
-#undef isatty
+static int fd_is_interactive[3] = { 0, 0, 0 };
+#define FD_CONSOLE 0x1
+#define FD_SWAPPED 0x2
+#define FD_MSYS    0x4
 
 /*
  ANSI codes used by git: m, K
@@ -105,6 +108,9 @@ static int is_console(int fd)
 	} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))
 		return 0;
 
+	if (fd >= 0 && fd <= 2)
+		fd_is_interactive[fd] |= FD_CONSOLE;
+
 	/* initialize attributes */
 	if (!initialized) {
 		console = hcon;
@@ -466,76 +472,47 @@ static HANDLE duplicate_handle(HANDLE hnd)
 	return hresult;
 }
 
-
-/*
- * Make MSVCRT's internal file descriptor control structure accessible
- * so that we can tweak OS handles and flags directly (we need MSVCRT
- * to treat our pipe handle as if it were a console).
- *
- * We assume that the ioinfo structure (exposed by MSVCRT.dll via
- * __pioinfo) starts with the OS handle and the flags. The exact size
- * varies between MSVCRT versions, so we try different sizes until
- * toggling the FDEV bit of _pioinfo(1)->osflags is reflected in
- * isatty(1).
- */
-typedef struct {
-	HANDLE osfhnd;
-	char osflags;
-} ioinfo;
-
-extern __declspec(dllimport) ioinfo *__pioinfo[];
-
-static size_t sizeof_ioinfo = 0;
-
-#define IOINFO_L2E 5
-#define IOINFO_ARRAY_ELTS (1 << IOINFO_L2E)
-
-#define FPIPE 0x08
-#define FDEV  0x40
-
-static inline ioinfo* _pioinfo(int fd)
-{
-	return (ioinfo*)((char*)__pioinfo[fd >> IOINFO_L2E] +
-			(fd & (IOINFO_ARRAY_ELTS - 1)) * sizeof_ioinfo);
-}
-
-static int init_sizeof_ioinfo(void)
-{
-	int istty, wastty;
-	/* don't init twice */
-	if (sizeof_ioinfo)
-		return sizeof_ioinfo >= 256;
-
-	sizeof_ioinfo = sizeof(ioinfo);
-	wastty = isatty(1);
-	while (sizeof_ioinfo < 256) {
-		/* toggle FDEV flag, check isatty, then toggle back */
-		_pioinfo(1)->osflags ^= FDEV;
-		istty = isatty(1);
-		_pioinfo(1)->osflags ^= FDEV;
-		/* return if we found the correct size */
-		if (istty != wastty)
-			return 0;
-		sizeof_ioinfo += sizeof(void*);
-	}
-	error("Tweaking file descriptors doesn't work with this MSVCRT.dll");
-	return 1;
-}
-
 static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
 {
-	ioinfo *pioinfo;
-	HANDLE old_handle;
-
-	/* init ioinfo size if we haven't done so */
-	if (init_sizeof_ioinfo())
-		return INVALID_HANDLE_VALUE;
-
-	/* get ioinfo pointer and change the handles */
-	pioinfo = _pioinfo(fd);
-	old_handle = pioinfo->osfhnd;
-	pioinfo->osfhnd = new_handle;
-	return old_handle;
+	/*
+	 * Create a copy of the original handle associated with fd
+	 * because the original will get closed when we dup2().
+	 */
+	HANDLE handle = (HANDLE)_get_osfhandle(fd);
+	HANDLE duplicate = duplicate_handle(handle);
+
+	/* Create a temp fd associated with the already open "new_handle". */
+	int new_fd = _open_osfhandle((intptr_t)new_handle, O_BINARY);
+
+	assert((fd == 1) || (fd == 2));
+
+	/*
+	 * Use stock dup2() to re-bind fd to the new handle.  Note that
+	 * this will implicitly close(1) and close both fd=1 and the
+	 * originally associated handle.  It will open a new fd=1 and
+	 * call DuplicateHandle() on the handle associated with new_fd.
+	 * It is because of this implicit close() that we created the
+	 * copy of the original.
+	 *
+	 * Note that we need to update the cached console handle to the
+	 * duplicated one because the dup2() call will implicitly close
+	 * the original one.
+	 *
+	 * Note that dup2() when given target := {0,1,2} will also
+	 * call SetStdHandle(), so we don't need to worry about that.
+	 */
+	if (console == handle)
+		console = duplicate;
+	dup2(new_fd, fd);
+
+	/* Close the temp fd.  This explicitly closes "new_handle"
+	 * (because it has been associated with it).
+	 */
+	close(new_fd);
+
+	fd_is_interactive[fd] |= FD_SWAPPED;
+
+	return duplicate;
 }
 
 #ifdef DETECT_MSYS_TTY
@@ -570,45 +547,25 @@ static void detect_msys_tty(int fd)
 			!wcsstr(name, L"-pty"))
 		return;
 
-	/* init ioinfo size if we haven't done so */
-	if (init_sizeof_ioinfo())
-		return;
-
-	/* set FDEV flag, reset FPIPE flag */
-	_pioinfo(fd)->osflags &= ~FPIPE;
-	_pioinfo(fd)->osflags |= FDEV;
+	fd_is_interactive[fd] |= FD_MSYS;
 }
 
 #endif
 
+/*
+ * Wrapper for isatty().  Most calls in the main git code
+ * call isatty(1 or 2) to see if the instance is interactive
+ * and should: be colored, show progress, paginate output.
+ * We lie and give results for what the descriptor WAS at
+ * startup (and ignore any pipe redirection we internally
+ * do).
+ */
+#undef isatty
 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;
+	if (fd >= 0 && fd <= 2)
+		return fd_is_interactive[fd] != 0;
+	return isatty(fd);
 }
 
 void winansi_init(void)
@@ -619,6 +576,10 @@ void winansi_init(void)
 	/* check if either stdout or stderr is a console output screen buffer */
 	con1 = is_console(1);
 	con2 = is_console(2);
+
+	/* Also compute console bit for fd 0 even though we don't need the result here. */
+	is_console(0);
+
 	if (!con1 && !con2) {
 #ifdef DETECT_MSYS_TTY
 		/* check if stdin / stdout / stderr are MSYS2 pty pipes */
@@ -662,12 +623,10 @@ void winansi_init(void)
  */
 HANDLE winansi_get_osfhandle(int fd)
 {
-	HANDLE hnd = (HANDLE) _get_osfhandle(fd);
-	if (isatty(fd) && GetFileType(hnd) == FILE_TYPE_PIPE) {
-		if (fd == 1 && hconsole1)
-			return hconsole1;
-		else if (fd == 2 && hconsole2)
-			return hconsole2;
-	}
-	return hnd;
+	if (fd == 1 && (fd_is_interactive[1] & FD_SWAPPED))
+		return hconsole1;
+	if (fd == 2 && (fd_is_interactive[2] & FD_SWAPPED))
+		return hconsole2;
+
+	return (HANDLE)_get_osfhandle(fd);
 }

-- 
2.11.0.rc3.windows.1
Johannes Schindelin· Jan 18, 2017, 12:13 UTC · re: Johannes Schindelin · lore

Re: [PATCH v3 3/3] mingw: replace isatty() hack

Hi,
On Fri, 23 Dec 2016, Johannes Schindelin wrote:
Show 18 quoted lines
> +	/*
> +	 * Use stock dup2() to re-bind fd to the new handle.  Note that
> +	 * this will implicitly close(1) and close both fd=1 and the
> +	 * originally associated handle.  It will open a new fd=1 and
> +	 * call DuplicateHandle() on the handle associated with new_fd.
> +	 * It is because of this implicit close() that we created the
> +	 * copy of the original.
> +	 *
> +	 * Note that we need to update the cached console handle to the
> +	 * duplicated one because the dup2() call will implicitly close
> +	 * the original one.
> +	 *
> +	 * Note that dup2() when given target := {0,1,2} will also
> +	 * call SetStdHandle(), so we don't need to worry about that.
> +	 */
> +	if (console == handle)
> +		console = duplicate;
> +	dup2(new_fd, fd);

Sadly, v2 was applied to `maint` instead of this revision, so Hannes' suggestions that I addressed in v3 were dropped silently.

I will send out a follow-up patch in a moment.

Ciao, Johannes

← back to recent threads