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

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

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

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

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

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

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

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

> +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.
	*/

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

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

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

-- Hannes

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

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

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

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

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

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

[Johannes Schindelin]

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

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

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

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

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

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

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

[Johannes Schindelin]

Hi Junio,

On Thu, 22 Dec 2016, Junio C Hamano wrote:

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

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

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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

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

[Junio C Hamano]

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.

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

[Johannes Sixt]

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.

-- Hannes

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

[Johannes Sixt]

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:
> +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?

> +	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;
>  }

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

[Johannes Schindelin]

Hi Hannes,

On Thu, 22 Dec 2016, Johannes Sixt wrote:

> 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

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

[Johannes Schindelin]

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.

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

Ciao,
Dscho

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

[Johannes Sixt]

Am 22.12.2016 um 22:37 schrieb Johannes Schindelin:
> 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

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

[Beat Bolli]

On 22.12.16 18:08, Johannes Schindelin wrote:
> 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?

>  	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;
>  
>  	/* initialize attributes */
> 

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

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

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

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

[Johannes Schindelin]

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

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

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

[Johannes Schindelin]

Hi Hannes,

On Thu, 22 Dec 2016, Johannes Sixt wrote:

> 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

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

[Junio C Hamano]

Beat Bolli <dev+git@drbeat.li> writes:

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

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

[Johannes Schindelin]

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.

Ciao,
Dscho

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

[Beat Bolli]

Hi Dscho

On 2016-12-23 10:30, Johannes Schindelin wrote:
> 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

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

[Johannes Schindelin]

Hi,

On Fri, 23 Dec 2016, Johannes Schindelin wrote:

> +	/*
> +	 * 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