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

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

[Johannes Schindelin]

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

Why is this such a big deal?

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

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

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

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

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

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

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

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


Johannes Schindelin (1):
  mingw: intercept isatty() to handle /dev/null as Git expects it

 compat/mingw.h   |  3 +++
 compat/winansi.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)


base-commit: 8d7a455ed52e2a96debc080dfc011b6bb00db5d2
Published-As: https://github.com/dscho/git/releases/tag/mingw-isatty-v1
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-isatty-v1

-- 
2.11.0.rc3.windows.1

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

[Johannes Schindelin]

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

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

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

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

For details, see

	https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.h   |  3 +++
 compat/winansi.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

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

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

[Junio C Hamano]

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

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

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

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

Thanks.

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

[Johannes Schindelin]

Hi Junio,

On Sun, 11 Dec 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > I finally got a chance to debug the problems with the ever-timing-out
> > test runs of `pu` on Windows. Turns out that pb/bisect uncovered a
> > really old, really bad bug: on Windows, isatty() does not do what Git
> > expects, at least not completely: it detects interactive terminals *and
> > character devices*.
> 
> Sounds as if somebody who did Windows at Microsoft had a good sense
> of humor to mimick the misnamed ENOTTY gotcha ;-) 

Hehe...

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

Can we fast-track this to maint?

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

Thanks,
Dscho

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

[Junio C Hamano]

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

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

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

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

Thanks.

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

[Johannes Sixt]

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

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

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

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

Wait...

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

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

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

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

Ouch.

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

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



> It fails with GetLastError() == 6 (invalid handle value). The reason
> for this is, I think, that in reality we are not writing to the
> console directly, but to a pipe, which is drained by console_thread(),
> which writes to the console.
>
> I have little clue what this winansi.c file is doing. Do you have any
> pointers where I should start digging?
>
> Wait...
>
> Should we not use winansi_get_osfhandle() instead of _get_osfhandle()?
>
>> +		}
>> +	}
>> +
>> +	return res;
>> +}
>> +
>>  void winansi_init(void)
>>  {
>>  	int con1, con2;
>>

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

[Johannes Sixt]

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

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

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

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

-- Hannes

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

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

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

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

Thanks.

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

[Johannes Sixt]

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

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

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

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
I was able to test the idea earlier than anticipated and it does work
for me.

 compat/winansi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/winansi.c b/compat/winansi.c
index cb725fb02f..ba360be69b 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -586,7 +586,7 @@ int winansi_isatty(int fd)
 		 *
 		 * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx
 		 */
-		HANDLE handle = (HANDLE)_get_osfhandle(fd);
+		HANDLE handle = winansi_get_osfhandle(fd);
 		if (fd == STDIN_FILENO) {
 			DWORD dummy;
 
-- 
2.11.0.79.gf6b77ca

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

[Johannes Sixt]

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

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

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

However!

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

What do you think?

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

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

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

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

Thanks.  

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

[Johannes Sixt]

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

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

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

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

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

[Johannes Schindelin]

Hi Hannes,

On Sun, 18 Dec 2016, Johannes Sixt wrote:

> The code in winansi.c emulates ANSI escape sequences when Git is
> connected to the "real" windows console, CMD.exe. The details are
> outline in eac14f8909d9 (Win32: Thread-safe windows console output,
> 2012-01-14). Essentially, it plugs a pipe between C code and the actual
> console output handle.
> 
> This commit also added an override for isatty(), but it was made
> unnecessary by fcd428f4a952 (Win32: fix broken pipe detection,
> 2012-03-01).
> 
> The new isatty() override implemented by cbb3f3c9b197 (mingw: intercept
> isatty() to handle /dev/null as Git expects it, 2016-12-11) does not
> take into account that _get_osfhandle() returns the handle visible by
> the C code, which is the pipe. But it actually wants to investigate the
> properties of the handle that is actually connected to the outside
> world. Fortunately, there is already winansi_get_osfhandle(), which
> returns exactly this handle. Use it.
> 
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> I was able to test the idea earlier than anticipated and it does work
> for me.

Thank you.

> diff --git a/compat/winansi.c b/compat/winansi.c
> index cb725fb02f..ba360be69b 100644
> --- a/compat/winansi.c
> +++ b/compat/winansi.c
> @@ -586,7 +586,7 @@ int winansi_isatty(int fd)
>  		 *
>  		 * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx
>  		 */
> -		HANDLE handle = (HANDLE)_get_osfhandle(fd);
> +		HANDLE handle = winansi_get_osfhandle(fd);

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

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

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

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Hannes,

On Sun, 18 Dec 2016, Johannes Sixt wrote:

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

I think that would break running Git in Git Bash (i.e. MinTTY) ;-)

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

- to abandon the _pioinfo hack

- to make isatty() work correctly with /dev/null

- to make isatty() work correctly in CMD

- to make isatty() work correctly in MinTTY (i.e. with MSYS2 pipes instead
  of Consoles)

I think we can have it all.

Ciao,
Dscho

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

[Junio C Hamano]

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

> That code works because winansi_get_osfhandle() is in winansi.c, where its
> call to isatty() is *not* redirected to winansi_isatty(). Good.
> ...
> My plan was actually ...
> ...
> Let's just clean up all of this in one go.

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

Thanks for working well together, as always.

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

[Johannes Schindelin]

Hi,

On Fri, 16 Dec 2016, Junio C Hamano wrote:

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

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

Bummer.

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

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

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

Happy holidays,
Dscho

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

[Johannes Schindelin]

Hi Hannes,

On Mon, 19 Dec 2016, Johannes Sixt wrote:

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

You did not miss anything. I did. I broke everything.

Very sorry for that!
Dscho

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

[Johannes Schindelin]

Hi Junio,

On Tue, 20 Dec 2016, Junio C Hamano wrote:

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

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

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

Ciao,
Dscho

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

[Junio C Hamano]

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

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

I think the latter is what you have at

 $ git fetch https://github.com/dscho/git mingw-isatty-fixup-master

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

Thanks.

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

[Johannes Schindelin]

Hi Junio,

On Wed, 21 Dec 2016, Junio C Hamano wrote:

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

Correct.

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

Okay, I will prepare v2 based on master and addressing your feedback.

Thanks,
Dscho