{"thread":{"id":"44673","subject":"[PATCH 0/1] Fix a long-standing isatty() problem on Windows","startedAt":"2016-12-11T11:17:03Z","lastAt":"2016-12-22T09:23:24Z","messageCount":21,"participants":["Johannes Schindelin","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"307461","messageId":"cover.1481454992.git.johannes.schindelin@gmx.de","threadId":"44673","inReplyTo":null,"subject":"[PATCH 0/1] Fix a long-standing isatty() problem on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-11T11:16:49Z","receivedAt":"2016-12-11T11:17:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"I finally got a chance to debug the problems with the ever-timing-out\ntest runs of `pu` on Windows. Turns out that pb/bisect uncovered a\nreally old, really bad bug: on Windows, isatty() does not do what Git\nexpects, at least not completely: it detects interactive terminals *and\ncharacter devices*.\n\nWhy is this such a big deal?\n\nOne such character device is NUL, Windows' equivalent of /dev/null. And\nguess what happens when the new tests of the bisect--helper run, with\nstdin redirected from /dev/null? Precisely. Git asks \"the user\" for\nreassurance that it may really continue, waiting forever. Or for Ctrl+C.\n\nAs we know what Git's source code wants, we have to make extra certain\nto test whether isatty() reports success for a Console. The common way\nto do this is to run GetConsoleMode() for input file descriptors, and\nGetConsoleScreenBufferInfo() for output file descriptors.\n\nOne additional note: the new winansi_isatty() function was put into this\nparticular spot not only because it vaguely makes sense to put\ntty-related stuff into compat/winansi.c, but with required future\nchanges in mind:\n\nThe current way in which Git for Windows makes sure that isatty()\nreturns non-zero for Git Bash (which runs in a terminal emulator called\nMinTTY that does *not* have any Windows Console associated with it, and\ntherefore Windows' _isatty() would actually return 0 if it was not for\nour detect_msys_tty() function) is hacky and needs to be fixed properly.\n\nIt is hacky because it relies on internals of the MSVC runtime that do\nnot hold true for the new Universal runtimes, which are used when\ncompiling with Visual C.\n\nWe already have experimental code to future-proof this method, and we\nuse that already when compiling Git for Windows in Visual Studio.\n\nThe place in which winansi_isatty() now lives will hopefully make it\npossible to unify the code paths again, so that both GCC and Visual C\nuse detect_msys_tty() through winansi_isatty().\n\nThis will also fix a bug where current Visual C-built Git may misdetect\na reopened stdin to be connected to an interactive terminal.\n\n\nJohannes Schindelin (1):\n mingw: intercept isatty() to handle /dev/null as Git expects it\n\n compat/mingw.h | 3 +++\n compat/winansi.c | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+)\n\n\nbase-commit: 8d7a455ed52e2a96debc080dfc011b6bb00db5d2\nPublished-As: https://github.com/dscho/git/releases/tag/mingw-isatty-v1\nFetch-It-Via: git fetch https://github.com/dscho/git mingw-isatty-v1\n\n-- \n2.11.0.rc3.windows.1\n\n"},{"id":"307462","messageId":"42ddc270ea04e01e899cc479063e5d602e4a4448.1481454992.git.johannes.schindelin@gmx.de","threadId":"44673","inReplyTo":"cover.1481454992.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/1] mingw: intercept isatty() to handle /dev/null as Git expects it","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-11T11:16:57Z","receivedAt":"2016-12-11T11:17:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When Git's source code calls isatty(), it really asks whether the\nrespective file descriptor is connected to an interactive terminal.\n\nWindows' _isatty() function, however, determines whether the file\ndescriptor is associated with a character device. And NUL, Windows'\nequivalent of /dev/null, is a character device.\n\nWhich means that for years, Git mistakenly detected an associated\ninteractive terminal when being run through the test suite, which\nalmost always redirects stdin, stdout and stderr to /dev/null.\n\nThis bug only became obvious, and painfully so, when the new\nbisect--helper entered the `pu` branch and made the automatic build & test\ntime out because t6030 was waiting for an answer.\n\nFor details, see\n\n\thttps://msdn.microsoft.com/en-us/library/f4s0ddew.aspx\n\nSigned-off-by: Johannes Schindelin \n---\n compat/mingw.h | 3 +++\n compat/winansi.c | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+)\n\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 034fff9479..3350169555 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -384,6 +384,9 @@ int mingw_raise(int sig);\n * ANSI emulation wrappers\n */\n \n+int winansi_isatty(int fd);\n+#define isatty winansi_isatty\n+\n void winansi_init(void);\n HANDLE winansi_get_osfhandle(int fd);\n \ndiff --git a/compat/winansi.c b/compat/winansi.c\nindex db4a5b0a37..cb725fb02f 100644\n--- a/compat/winansi.c\n+++ b/compat/winansi.c\n@@ -7,6 +7,9 @@\n #include \n #include \n \n+/* In this file, we actually want to use Windows' own isatty(). */\n+#undef isatty\n+\n /*\n ANSI codes used by git: m, K\n \n@@ -570,6 +573,36 @@ static void detect_msys_tty(int fd)\n \n #endif\n \n+int winansi_isatty(int fd)\n+{\n+\tint res = isatty(fd);\n+\n+\tif (res) {\n+\t\t/*\n+\t\t * Make sure that /dev/null is not fooling Git into believing\n+\t\t * that we are connected to a terminal, as \"_isatty() returns a\n+\t\t * nonzero value if the descriptor is associated with a\n+\t\t * character device.\"; for more information, see\n+\t\t *\n+\t\t * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx\n+\t\t */\n+\t\tHANDLE handle = (HANDLE)_get_osfhandle(fd);\n+\t\tif (fd == STDIN_FILENO) {\n+\t\t\tDWORD dummy;\n+\n+\t\t\tif (!GetConsoleMode(handle, &dummy))\n+\t\t\t\tres = 0;\n+\t\t} else if (fd == STDOUT_FILENO || fd == STDERR_FILENO) {\n+\t\t\tCONSOLE_SCREEN_BUFFER_INFO dummy;\n+\n+\t\t\tif (!GetConsoleScreenBufferInfo(handle, &dummy))\n+\t\t\t\tres = 0;\n+\t\t}\n+\t}\n+\n+\treturn res;\n+}\n+\n void winansi_init(void)\n {\n \tint con1, con2;\n-- \n2.11.0.rc3.windows.1\n"},{"id":"307472","messageId":"xmqqmvg2nyo6.fsf@gitster.mtv.corp.google.com","threadId":"44673","inReplyTo":"cover.1481454992.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 0/1] Fix a long-standing isatty() problem on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-11T17:49:45Z","receivedAt":"2016-12-11T17:49:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> I finally got a chance to debug the problems with the ever-timing-out\n> test runs of `pu` on Windows. Turns out that pb/bisect uncovered a\n> really old, really bad bug: on Windows, isatty() does not do what Git\n> expects, at least not completely: it detects interactive terminals *and\n> character devices*.\n\nSounds as if somebody who did Windows at Microsoft had a good sense\nof humor to mimick the misnamed ENOTTY gotcha ;-) \n\nThis is a great find, and a very impactful fix, as redirecting from\n/dev/null is how we try to force a \"go interactive if talking to\ntty\" program to realize that it is not talking to a tty.\n\nThanks.\n\n\n"},{"id":"307497","messageId":"alpine.DEB.2.20.1612121052310.23160@virtualbox","threadId":"44673","inReplyTo":"xmqqmvg2nyo6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/1] Fix a long-standing isatty() problem on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-12T09:59:01Z","receivedAt":"2016-12-12T09:59:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sun, 11 Dec 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > I finally got a chance to debug the problems with the ever-timing-out\n> > test runs of `pu` on Windows. Turns out that pb/bisect uncovered a\n> > really old, really bad bug: on Windows, isatty() does not do what Git\n> > expects, at least not completely: it detects interactive terminals *and\n> > character devices*.\n> \n> Sounds as if somebody who did Windows at Microsoft had a good sense\n> of humor to mimick the misnamed ENOTTY gotcha ;-) \n\nHehe...\n\n> This is a great find, and a very impactful fix, as redirecting from\n> /dev/null is how we try to force a \"go interactive if talking to\n> tty\" program to realize that it is not talking to a tty.\n\nCan we fast-track this to maint?\n\nI will definitely ship this fix in the next Git for Windows version, but\nstill, it would be nice to have this in git.git as soon as possible.\n\nThanks,\nDscho\n"},{"id":"307511","messageId":"xmqq8trlnlhn.fsf@gitster.mtv.corp.google.com","threadId":"44673","inReplyTo":"alpine.DEB.2.20.1612121052310.23160@virtualbox","subject":"Re: [PATCH 0/1] Fix a long-standing isatty() problem on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-12T16:46:44Z","receivedAt":"2016-12-12T16:46:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> This is a great find, and a very impactful fix, as redirecting from\n>> /dev/null is how we try to force a \"go interactive if talking to\n>> tty\" program to realize that it is not talking to a tty.\n>\n> Can we fast-track this to maint?\n\nYes, \"fast-track\" is a good phrase, as there is not much point\ncooking a subsystem specific fix like this one that came directly\nfrom a subsystem maintainer in 'next' for 1/2 to 2 weeks as usual.\n\nLet's make sure that the first maintenance release won't ship\nwithout merging this.\n\nThanks.\n"},{"id":"307891","messageId":"129f000c-49c1-0e75-26b3-c96e9b442443@kdbg.org","threadId":"44673","inReplyTo":"42ddc270ea04e01e899cc479063e5d602e4a4448.1481454992.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 1/1] mingw: intercept isatty() to handle /dev/null as Git expects it","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-12-16T17:48:01Z","receivedAt":"2016-12-16T17:48:10Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11.12.2016 um 12:16 schrieb Johannes Schindelin:\n> When Git's source code calls isatty(), it really asks whether the\n> respective file descriptor is connected to an interactive terminal.\n>\n> Windows' _isatty() function, however, determines whether the file\n> descriptor is associated with a character device. And NUL, Windows'\n> equivalent of /dev/null, is a character device.\n>\n> Which means that for years, Git mistakenly detected an associated\n> interactive terminal when being run through the test suite, which\n> almost always redirects stdin, stdout and stderr to /dev/null.\n>\n> This bug only became obvious, and painfully so, when the new\n> bisect--helper entered the `pu` branch and made the automatic build & test\n> time out because t6030 was waiting for an answer.\n>\n> For details, see\n>\n> \thttps://msdn.microsoft.com/en-us/library/f4s0ddew.aspx\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> compat/mingw.h | 3 +++\n> compat/winansi.c | 33 +++++++++++++++++++++++++++++++++\n> 2 files changed, 36 insertions(+)\n>\n> diff --git a/compat/mingw.h b/compat/mingw.h\n> index 034fff9479..3350169555 100644\n> --- a/compat/mingw.h\n> +++ b/compat/mingw.h\n> @@ -384,6 +384,9 @@ int mingw_raise(int sig);\n> * ANSI emulation wrappers\n> */\n>\n> +int winansi_isatty(int fd);\n> +#define isatty winansi_isatty\n> +\n> void winansi_init(void);\n> HANDLE winansi_get_osfhandle(int fd);\n>\n> diff --git a/compat/winansi.c b/compat/winansi.c\n> index db4a5b0a37..cb725fb02f 100644\n> --- a/compat/winansi.c\n> +++ b/compat/winansi.c\n> @@ -7,6 +7,9 @@\n> #include \n> #include \n>\n> +/* In this file, we actually want to use Windows' own isatty(). */\n> +#undef isatty\n> +\n> /*\n> ANSI codes used by git: m, K\n>\n> @@ -570,6 +573,36 @@ static void detect_msys_tty(int fd)\n>\n> #endif\n>\n> +int winansi_isatty(int fd)\n> +{\n> +\tint res = isatty(fd);\n> +\n> +\tif (res) {\n> +\t\t/*\n> +\t\t * Make sure that /dev/null is not fooling Git into believing\n> +\t\t * that we are connected to a terminal, as \"_isatty() returns a\n> +\t\t * nonzero value if the descriptor is associated with a\n> +\t\t * character device.\"; for more information, see\n> +\t\t *\n> +\t\t * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx\n> +\t\t */\n> +\t\tHANDLE handle = (HANDLE)_get_osfhandle(fd);\n> +\t\tif (fd == STDIN_FILENO) {\n> +\t\t\tDWORD dummy;\n> +\n> +\t\t\tif (!GetConsoleMode(handle, &dummy))\n> +\t\t\t\tres = 0;\n> +\t\t} else if (fd == STDOUT_FILENO || fd == STDERR_FILENO) {\n> +\t\t\tCONSOLE_SCREEN_BUFFER_INFO dummy;\n> +\n> +\t\t\tif (!GetConsoleScreenBufferInfo(handle, &dummy))\n> +\t\t\t\tres = 0;\n\nI am sorry to have to report that this check does not work as expected. \nI am operating Git from CMD, not mintty, BTW.\n\nIt fails with GetLastError() == 6 (invalid handle value). The reason for \nthis is, I think, that in reality we are not writing to the console \ndirectly, but to a pipe, which is drained by console_thread(), which \nwrites to the console.\n\nI have little clue what this winansi.c file is doing. Do you have any \npointers where I should start digging?\n\nWait...\n\nShould we not use winansi_get_osfhandle() instead of _get_osfhandle()?\n\n> +\t\t}\n> +\t}\n> +\n> +\treturn res;\n> +}\n> +\n> void winansi_init(void)\n> {\n> \tint con1, con2;\n>\n\n"},{"id":"307896","messageId":"xmqqy3zfsq4q.fsf@gitster.mtv.corp.google.com","threadId":"44673","inReplyTo":"129f000c-49c1-0e75-26b3-c96e9b442443@kdbg.org","subject":"Re: [PATCH 1/1] mingw: intercept isatty() to handle /dev/null as Git expects it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-16T18:08:53Z","receivedAt":"2016-12-16T18:09:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 11.12.2016 um 12:16 schrieb Johannes Schindelin:\n>> When Git's source code calls isatty(), it really asks whether the\n>> respective file descriptor is connected to an interactive terminal.\n>> ...\n>> +\t\t\tif (!GetConsoleScreenBufferInfo(handle, &dummy))\n>> +\t\t\t\tres = 0;\n>\n> I am sorry to have to report that this check does not work as\n> expected. I am operating Git from CMD, not mintty, BTW.\n\nOuch.\n\nSorry for not having waited for you to chime in before agreeing to\nfast-track this one, and now this is in 'master'.\n\nI should have known better than that by now, after having seen you\nuncover bugs that did not trigger in Dscho's environment and vice\nversa to tell you that Windows specific things both of you deem good\nhave a much higher chance of being correct than a change without an\nAck by the other.\n\n\n\n> It fails with GetLastError() == 6 (invalid handle value). The reason\n> for this is, I think, that in reality we are not writing to the\n> console directly, but to a pipe, which is drained by console_thread(),\n> which writes to the console.\n>\n> I have little clue what this winansi.c file is doing. Do you have any\n> pointers where I should start digging?\n>\n> Wait...\n>\n> Should we not use winansi_get_osfhandle() instead of _get_osfhandle()?\n>\n>> +\t\t}\n>> +\t}\n>> +\n>> +\treturn res;\n>> +}\n>> +\n>> void winansi_init(void)\n>> {\n>> \tint con1, con2;\n>>\n"},{"id":"307899","messageId":"5977e71d-da58-7cb0-bc69-343bb3a1341d@kdbg.org","threadId":"44673","inReplyTo":"xmqqy3zfsq4q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/1] mingw: intercept isatty() to handle /dev/null as Git expects it","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-12-16T18:44:37Z","receivedAt":"2016-12-16T18:44:46Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 16.12.2016 um 19:08 schrieb Junio C Hamano:\n> Sorry for not having waited for you to chime in before agreeing to\n> fast-track this one, and now this is in 'master'.\n\nNo reason to be sorry, things happen... Dscho's request for \nfast-tracking was very reasonable, and the patch looked \"obviously \ncorrect\". I lacked time to test it promptly, and when I finally did, I \nignored the symptoms because my workflow was a mess and I had to \nconcentrate on other things. Only after 'git push' did not report the \npack progress, was my suspicion raised...\n\n>> Should we not use winansi_get_osfhandle() instead of _get_osfhandle()?\n\nUnfortunately, I'm away from my Windows box over the weekend. It will \nhave to wait until Monday before I can test this idea.\n\n-- Hannes\n\n"},{"id":"307910","messageId":"xmqqh963snhs.fsf@gitster.mtv.corp.google.com","threadId":"44673","inReplyTo":"5977e71d-da58-7cb0-bc69-343bb3a1341d@kdbg.org","subject":"Re: [PATCH 1/1] mingw: intercept isatty() to handle /dev/null as Git expects it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-16T19:05:51Z","receivedAt":"2016-12-16T19:05:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 16.12.2016 um 19:08 schrieb Junio C Hamano:\n>> Sorry for not having waited for you to chime in before agreeing to\n>> fast-track this one, and now this is in 'master'.\n>\n> No reason to be sorry, things happen... Dscho's request for\n> fast-tracking was very reasonable, and the patch looked \"obviously\n> correct\".\n\nOh, I do agree it was reasonable and looked obviously correct. And\nI suspect that it did not make anything worse, either, so there is\nnot much harm done, other than that I now have to remember not to\nmerge it down without further fixes to 'maint' and declare the NUL\nthing fixed ;-)\n\n> Unfortunately, I'm away from my Windows box over the weekend. It will\n> have to wait until Monday before I can test this idea.\n\nThanks.\n"},{"id":"307996","messageId":"ffc6a7a0-4ae4-b755-0b09-5bcd7114a2e6@kdbg.org","threadId":"44673","inReplyTo":"5977e71d-da58-7cb0-bc69-343bb3a1341d@kdbg.org","subject":"[PATCH] winansi_isatty(): fix when Git is used from CMD","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-12-18T15:26:02Z","receivedAt":"2016-12-18T15:27:12Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"The code in winansi.c emulates ANSI escape sequences when Git is\nconnected to the \"real\" windows console, CMD.exe. The details are\noutline in eac14f8909d9 (Win32: Thread-safe windows console output,\n2012-01-14). Essentially, it plugs a pipe between C code and the actual\nconsole output handle.\n\nThis commit also added an override for isatty(), but it was made\nunnecessary by fcd428f4a952 (Win32: fix broken pipe detection,\n2012-03-01).\n\nThe new isatty() override implemented by cbb3f3c9b197 (mingw: intercept\nisatty() to handle /dev/null as Git expects it, 2016-12-11) does not\ntake into account that _get_osfhandle() returns the handle visible by\nthe C code, which is the pipe. But it actually wants to investigate the\nproperties of the handle that is actually connected to the outside\nworld. Fortunately, there is already winansi_get_osfhandle(), which\nreturns exactly this handle. Use it.\n\nSigned-off-by: Johannes Sixt \n---\nI was able to test the idea earlier than anticipated and it does work\nfor me.\n\n compat/winansi.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/compat/winansi.c b/compat/winansi.c\nindex cb725fb02f..ba360be69b 100644\n--- a/compat/winansi.c\n+++ b/compat/winansi.c\n@@ -586,7 +586,7 @@ int winansi_isatty(int fd)\n \t\t *\n \t\t * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx\n \t\t */\n-\t\tHANDLE handle = (HANDLE)_get_osfhandle(fd);\n+\t\tHANDLE handle = winansi_get_osfhandle(fd);\n \t\tif (fd == STDIN_FILENO) {\n \t\t\tDWORD dummy;\n \n-- \n2.11.0.79.gf6b77ca\n\n"},{"id":"307997","messageId":"d661dbf1-9852-965a-2ca9-67d763115b9e@kdbg.org","threadId":"44673","inReplyTo":"ffc6a7a0-4ae4-b755-0b09-5bcd7114a2e6@kdbg.org","subject":"Re: [PATCH] winansi_isatty(): fix when Git is used from CMD","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-12-18T15:37:45Z","receivedAt":"2016-12-18T15:38:47Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 18.12.2016 um 16:26 schrieb Johannes Sixt:\n> The new isatty() override implemented by cbb3f3c9b197 (mingw: intercept\n> isatty() to handle /dev/null as Git expects it, 2016-12-11) does not\n> take into account that _get_osfhandle() returns the handle visible by\n> the C code, which is the pipe. But it actually wants to investigate the\n> properties of the handle that is actually connected to the outside\n> world. Fortunately, there is already winansi_get_osfhandle(), which\n> returns exactly this handle. Use it.\n\nBut quite frankly, I find the implementation of winansi_isatty()\nvery unsatisfactory.\n\nI understand that you wanted to be defensive and to override the\ndecision made by MSVCRT only when necessary.\n\nHowever!\n\nwinansi.c is all about overriding MSVCRT's console handling. If we are\nconnected to a console, then by the time isatty() is called (from\noutside the emulation layer), all handling of file descriptors 1 and 2\nis already outside MSVCRT's control. In particular, we have determined\nunambiguously whether a terminal is connected (see is_console()). I\nsuggest to have the implementation below (on top of the patch I'm\nresponding to).\n\nWhat do you think?\n\ndiff --git a/compat/winansi.c b/compat/winansi.c\nindex ba360be69b..1748d17777 100644\n--- a/compat/winansi.c\n+++ b/compat/winansi.c\n@@ -575,9 +575,8 @@ static void detect_msys_tty(int fd)\n \n int winansi_isatty(int fd)\n {\n-\tint res = isatty(fd);\n-\n-\tif (res) {\n+\tswitch (fd) {\n+\tcase 0:\n \t\t/*\n \t\t * Make sure that /dev/null is not fooling Git into believing\n \t\t * that we are connected to a terminal, as \"_isatty() returns a\n@@ -586,21 +585,19 @@ int winansi_isatty(int fd)\n \t\t *\n \t\t * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx\n \t\t */\n-\t\tHANDLE handle = winansi_get_osfhandle(fd);\n-\t\tif (fd == STDIN_FILENO) {\n+\t\t{\n+\t\t\tHANDLE handle = (HANDLE)_get_osfhandle(fd);\n \t\t\tDWORD dummy;\n \n-\t\t\tif (!GetConsoleMode(handle, &dummy))\n-\t\t\t\tres = 0;\n-\t\t} else if (fd == STDOUT_FILENO || fd == STDERR_FILENO) {\n-\t\t\tCONSOLE_SCREEN_BUFFER_INFO dummy;\n-\n-\t\t\tif (!GetConsoleScreenBufferInfo(handle, &dummy))\n-\t\t\t\tres = 0;\n+\t\t\treturn !!GetConsoleMode(handle, &dummy);\n \t\t}\n+\tcase 1:\n+\t\treturn !!hconsole1;\n+\tcase 2:\n+\t\treturn !!hconsole2;\n \t}\n \n-\treturn res;\n+\treturn isatty(fd);\n }\n \n void winansi_init(void)\n-- \n2.11.0.79.gf6b77ca\n\n"},{"id":"308004","messageId":"xmqq37hkrabx.fsf@gitster.mtv.corp.google.com","threadId":"44673","inReplyTo":"ffc6a7a0-4ae4-b755-0b09-5bcd7114a2e6@kdbg.org","subject":"Re: [PATCH] winansi_isatty(): fix when Git is used from CMD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-19T01:12:18Z","receivedAt":"2016-12-19T01:13:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> ..\n> The new isatty() override implemented by cbb3f3c9b197 (mingw: intercept\n> isatty() to handle /dev/null as Git expects it, 2016-12-11) does not\n> take into account that _get_osfhandle() returns the handle visible by\n> the C code, which is the pipe. But it actually wants to investigate the\n> properties of the handle that is actually connected to the outside\n> world. Fortunately, there is already winansi_get_osfhandle(), which\n> returns exactly this handle. Use it.\n>\n> Signed-off-by: Johannes Sixt \n> ---\n> I was able to test the idea earlier than anticipated and it does work\n> for me.\n\nI'll queue this in 'pu' so that I won't forget, but this time I'll\nmake sure I won't act on it until I see you two agree on the right\nway forward.\n\nThanks. \n"},{"id":"308086","messageId":"3f06ea33-b4de-48b4-593e-239eb6e87dd4@kdbg.org","threadId":"44673","inReplyTo":"d661dbf1-9852-965a-2ca9-67d763115b9e@kdbg.org","subject":"Re: [PATCH] winansi_isatty(): fix when Git is used from CMD","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-12-19T19:34:30Z","receivedAt":"2016-12-19T19:35:38Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 18.12.2016 um 16:37 schrieb Johannes Sixt:\n> winansi.c is all about overriding MSVCRT's console handling. If we are\n> connected to a console, then by the time isatty() is called (from\n> outside the emulation layer), all handling of file descriptors 1 and 2\n> is already outside MSVCRT's control. In particular, we have determined\n> unambiguously whether a terminal is connected (see is_console()). I\n> suggest to have the implementation below (on top of the patch I'm\n> responding to).\n>\n> What do you think?\n\nI thought a bit more about this approach, and I retract it. I think it \ndoes not work when Git is connected to an MSYS TTY, i.e., when the \n\"console\" is in reality the pipe that is detected in detect_msys_tty().\n\nAt the same time I wonder how your original winansi_isatty() could have \nworked: In this case, MSVCRT's isatty() would return 1 (because \ndetect_msys_tty() has set things up that this happens), but then \nwinansi_isatty() checks whether the handle underlying fd 0, 1 or 2 is a \nreal Windows console. But it is not: it is a pipe. Am I missing something?\n\n>\n> diff --git a/compat/winansi.c b/compat/winansi.c\n> index ba360be69b..1748d17777 100644\n> --- a/compat/winansi.c\n> +++ b/compat/winansi.c\n> @@ -575,9 +575,8 @@ static void detect_msys_tty(int fd)\n>\n> int winansi_isatty(int fd)\n> {\n> -\tint res = isatty(fd);\n> -\n> -\tif (res) {\n> +\tswitch (fd) {\n> +\tcase 0:\n> \t\t/*\n> \t\t * Make sure that /dev/null is not fooling Git into believing\n> \t\t * that we are connected to a terminal, as \"_isatty() returns a\n> @@ -586,21 +585,19 @@ int winansi_isatty(int fd)\n> \t\t *\n> \t\t * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx\n> \t\t */\n> -\t\tHANDLE handle = winansi_get_osfhandle(fd);\n> -\t\tif (fd == STDIN_FILENO) {\n> +\t\t{\n> +\t\t\tHANDLE handle = (HANDLE)_get_osfhandle(fd);\n> \t\t\tDWORD dummy;\n>\n> -\t\t\tif (!GetConsoleMode(handle, &dummy))\n> -\t\t\t\tres = 0;\n> -\t\t} else if (fd == STDOUT_FILENO || fd == STDERR_FILENO) {\n> -\t\t\tCONSOLE_SCREEN_BUFFER_INFO dummy;\n> -\n> -\t\t\tif (!GetConsoleScreenBufferInfo(handle, &dummy))\n> -\t\t\t\tres = 0;\n> +\t\t\treturn !!GetConsoleMode(handle, &dummy);\n> \t\t}\n> +\tcase 1:\n> +\t\treturn !!hconsole1;\n> +\tcase 2:\n> +\t\treturn !!hconsole2;\n> \t}\n>\n> -\treturn res;\n> +\treturn isatty(fd);\n> }\n>\n> void winansi_init(void)\n>\n\n"},{"id":"308148","messageId":"alpine.DEB.2.20.1612201732160.54750@virtualbox","threadId":"44673","inReplyTo":"ffc6a7a0-4ae4-b755-0b09-5bcd7114a2e6@kdbg.org","subject":"Re: [PATCH] winansi_isatty(): fix when Git is used from CMD","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-20T16:50:04Z","receivedAt":"2016-12-20T16:50:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Sun, 18 Dec 2016, Johannes Sixt wrote:\n\n> The code in winansi.c emulates ANSI escape sequences when Git is\n> connected to the \"real\" windows console, CMD.exe. The details are\n> outline in eac14f8909d9 (Win32: Thread-safe windows console output,\n> 2012-01-14). Essentially, it plugs a pipe between C code and the actual\n> console output handle.\n> \n> This commit also added an override for isatty(), but it was made\n> unnecessary by fcd428f4a952 (Win32: fix broken pipe detection,\n> 2012-03-01).\n> \n> The new isatty() override implemented by cbb3f3c9b197 (mingw: intercept\n> isatty() to handle /dev/null as Git expects it, 2016-12-11) does not\n> take into account that _get_osfhandle() returns the handle visible by\n> the C code, which is the pipe. But it actually wants to investigate the\n> properties of the handle that is actually connected to the outside\n> world. Fortunately, there is already winansi_get_osfhandle(), which\n> returns exactly this handle. Use it.\n> \n> Signed-off-by: Johannes Sixt \n> ---\n> I was able to test the idea earlier than anticipated and it does work\n> for me.\n\nThank you.\n\n> diff --git a/compat/winansi.c b/compat/winansi.c\n> index cb725fb02f..ba360be69b 100644\n> --- a/compat/winansi.c\n> +++ b/compat/winansi.c\n> @@ -586,7 +586,7 @@ int winansi_isatty(int fd)\n> \t\t *\n> \t\t * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx\n> \t\t */\n> -\t\tHANDLE handle = (HANDLE)_get_osfhandle(fd);\n> +\t\tHANDLE handle = winansi_get_osfhandle(fd);\n\nThat code works because winansi_get_osfhandle() is in winansi.c, where its\ncall to isatty() is *not* redirected to winansi_isatty(). Good.\n\nMy plan was actually to clean up the \"magic\" detect_msys_tty() code: it\nmesses with internals of the MSVC runtime that are no longer the same in\nthe Universal Runtime (UCRT), and hence we already had to come up with a\ndifferent way to detect an MSYS2 pipe. My preference would be to merge\nthat logic into winansi_isatty() and abandon the _pioinfo hack.\n\nLet's just clean up all of this in one go.\n\nCiao,\nDscho\n"},{"id":"308149","messageId":"alpine.DEB.2.20.1612201751060.54750@virtualbox","threadId":"44673","inReplyTo":"d661dbf1-9852-965a-2ca9-67d763115b9e@kdbg.org","subject":"Re: [PATCH] winansi_isatty(): fix when Git is used from CMD","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-20T16:53:45Z","receivedAt":"2016-12-20T16:53:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Sun, 18 Dec 2016, Johannes Sixt wrote:\n\n> What do you think?\n> \n> diff --git a/compat/winansi.c b/compat/winansi.c\n> index ba360be69b..1748d17777 100644\n> --- a/compat/winansi.c\n> +++ b/compat/winansi.c\n> @@ -575,9 +575,8 @@ static void detect_msys_tty(int fd)\n> \n> int winansi_isatty(int fd)\n> {\n> -\tint res = isatty(fd);\n> -\n> -\tif (res) {\n> +\tswitch (fd) {\n> +\tcase 0:\n> \t\t/*\n> \t\t * Make sure that /dev/null is not fooling Git into believing\n> \t\t * that we are connected to a terminal, as \"_isatty() returns a\n> @@ -586,21 +585,19 @@ int winansi_isatty(int fd)\n> \t\t *\n> \t\t * https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx\n> \t\t */\n> -\t\tHANDLE handle = winansi_get_osfhandle(fd);\n> -\t\tif (fd == STDIN_FILENO) {\n> +\t\t{\n> +\t\t\tHANDLE handle = (HANDLE)_get_osfhandle(fd);\n> \t\t\tDWORD dummy;\n> \n> -\t\t\tif (!GetConsoleMode(handle, &dummy))\n> -\t\t\t\tres = 0;\n> -\t\t} else if (fd == STDOUT_FILENO || fd == STDERR_FILENO) {\n> -\t\t\tCONSOLE_SCREEN_BUFFER_INFO dummy;\n> -\n> -\t\t\tif (!GetConsoleScreenBufferInfo(handle, &dummy))\n> -\t\t\t\tres = 0;\n> +\t\t\treturn !!GetConsoleMode(handle, &dummy);\n> \t\t}\n> +\tcase 1:\n> +\t\treturn !!hconsole1;\n> +\tcase 2:\n> +\t\treturn !!hconsole2;\n> \t}\n> \n> -\treturn res;\n> +\treturn isatty(fd);\n> }\n> \n> void winansi_init(void)\n\nI think that would break running Git in Git Bash (i.e. MinTTY) ;-)\n\nLet me try to come up with a patch series starting from your patch. We\nneed\n\n- to abandon the _pioinfo hack\n\n- to make isatty() work correctly with /dev/null\n\n- to make isatty() work correctly in CMD\n\n- to make isatty() work correctly in MinTTY (i.e. with MSYS2 pipes instead\n of Consoles)\n\nI think we can have it all.\n\nCiao,\nDscho\n"},{"id":"308151","messageId":"xmqqtw9yleop.fsf@gitster.mtv.corp.google.com","threadId":"44673","inReplyTo":"alpine.DEB.2.20.1612201732160.54750@virtualbox","subject":"Re: [PATCH] winansi_isatty(): fix when Git is used from CMD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-20T16:59:18Z","receivedAt":"2016-12-20T16:59:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> That code works because winansi_get_osfhandle() is in winansi.c, where its\n> call to isatty() is *not* redirected to winansi_isatty(). Good.\n> ...\n> My plan was actually ...\n> ...\n> Let's just clean up all of this in one go.\n\nI take this to mean that I should hold off applying/merging j6t's\none liner and wait for your counterproposal tested by j6t. If I\nmisread you please let me know.\n\nThanks for working well together, as always.\n"},{"id":"308204","messageId":"alpine.DEB.2.20.1612211854530.155951@virtualbox","threadId":"44673","inReplyTo":"xmqqh963snhs.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/1] mingw: intercept isatty() to handle /dev/null as Git expects it","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-21T17:56:41Z","receivedAt":"2016-12-21T17:56:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 16 Dec 2016, Junio C Hamano wrote:\n\n> Johannes Sixt writes:\n> \n> > Am 16.12.2016 um 19:08 schrieb Junio C Hamano:\n> >> Sorry for not having waited for you to chime in before agreeing to\n> >> fast-track this one, and now this is in 'master'.\n> >\n> > No reason to be sorry, things happen... Dscho's request for\n> > fast-tracking was very reasonable, and the patch looked \"obviously\n> > correct\".\n> \n> Oh, I do agree it was reasonable and looked obviously correct. And\n> I suspect that it did not make anything worse, either, so there is\n> not much harm done, other than that I now have to remember not to\n> merge it down without further fixes to 'maint' and declare the NUL\n> thing fixed ;-)\n\nActually, due to having way too many worktrees I managed to test the exact\nwrong build product and failed to notice that the patch I asked to\nfast-track broke paging in Git for Windows' Git Bash, too, not only in\nCMD.\n\nBummer.\n\n> > Unfortunately, I'm away from my Windows box over the weekend. It will\n> > have to wait until Monday before I can test this idea.\n> \n> Thanks.\n\nI just sent out a fixer-upper patch series, hopefully I got it right this\ntime.\n\nHannes, it would be very nice if you could beat on it for a bit.\n\nHappy holidays,\nDscho\n"},{"id":"308205","messageId":"alpine.DEB.2.20.1612211857000.155951@virtualbox","threadId":"44673","inReplyTo":"3f06ea33-b4de-48b4-593e-239eb6e87dd4@kdbg.org","subject":"Re: [PATCH] winansi_isatty(): fix when Git is used from CMD","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-21T17:57:30Z","receivedAt":"2016-12-21T17:57:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Mon, 19 Dec 2016, Johannes Sixt wrote:\n\n> Am 18.12.2016 um 16:37 schrieb Johannes Sixt:\n> > winansi.c is all about overriding MSVCRT's console handling. If we are\n> > connected to a console, then by the time isatty() is called (from\n> > outside the emulation layer), all handling of file descriptors 1 and 2\n> > is already outside MSVCRT's control. In particular, we have determined\n> > unambiguously whether a terminal is connected (see is_console()). I\n> > suggest to have the implementation below (on top of the patch I'm\n> > responding to).\n> >\n> > What do you think?\n> \n> I thought a bit more about this approach, and I retract it. I think it\n> does not work when Git is connected to an MSYS TTY, i.e., when the\n> \"console\" is in reality the pipe that is detected in detect_msys_tty().\n> \n> At the same time I wonder how your original winansi_isatty() could have\n> worked: In this case, MSVCRT's isatty() would return 1 (because\n> detect_msys_tty() has set things up that this happens), but then\n> winansi_isatty() checks whether the handle underlying fd 0, 1 or 2 is a real\n> Windows console. But it is not: it is a pipe. Am I missing something?\n\nYou did not miss anything. I did. I broke everything.\n\nVery sorry for that!\nDscho\n"},{"id":"308206","messageId":"alpine.DEB.2.20.1612211857480.155951@virtualbox","threadId":"44673","inReplyTo":"xmqqtw9yleop.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] winansi_isatty(): fix when Git is used from CMD","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-21T17:59:42Z","receivedAt":"2016-12-21T17:59:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 20 Dec 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > That code works because winansi_get_osfhandle() is in winansi.c, where its\n> > call to isatty() is *not* redirected to winansi_isatty(). Good.\n> > ...\n> > My plan was actually ...\n> > ...\n> > Let's just clean up all of this in one go.\n> \n> I take this to mean that I should hold off applying/merging j6t's\n> one liner and wait for your counterproposal tested by j6t. If I\n> misread you please let me know.\n> \n> Thanks for working well together, as always.\n\nI prepared a patch series based on `pu`, on top of Hannes' patch, and I\nalso prepared a branch that is based on `master`, obviously without\nHannes' patch.\n\nI am indifferent whether to include it or not, as long as we have\nsomething robust in the end that everybody is happy with.\n\nCiao,\nDscho\n"},{"id":"308210","messageId":"xmqqlgv9i04z.fsf@gitster.mtv.corp.google.com","threadId":"44673","inReplyTo":"alpine.DEB.2.20.1612211857480.155951@virtualbox","subject":"Re: [PATCH] winansi_isatty(): fix when Git is used from CMD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-21T18:54:04Z","receivedAt":"2016-12-21T18:54:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> I prepared a patch series based on `pu`, on top of Hannes' patch, and I\n> also prepared a branch that is based on `master`, obviously without\n> Hannes' patch.\n\nI think the latter is what you have at\n\n $ git fetch https://github.com/dscho/git mingw-isatty-fixup-master\n\nIf that is correct, I am inclined to fetch that and queue it on\n'pu', replacing Hannes's patch, and then to ask him to Test/Ack it.\n\nThanks.\n"},{"id":"308242","messageId":"alpine.DEB.2.20.1612221019200.155951@virtualbox","threadId":"44673","inReplyTo":"xmqqlgv9i04z.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] winansi_isatty(): fix when Git is used from CMD","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-22T09:23:09Z","receivedAt":"2016-12-22T09:23:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 21 Dec 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > I prepared a patch series based on `pu`, on top of Hannes' patch, and\n> > I also prepared a branch that is based on `master`, obviously without\n> > Hannes' patch.\n> \n> I think the latter is what you have at\n> \n> $ git fetch https://github.com/dscho/git mingw-isatty-fixup-master\n\nCorrect.\n\n> If that is correct, I am inclined to fetch that and queue it on 'pu',\n> replacing Hannes's patch, and then to ask him to Test/Ack it.\n\nOkay, I will prepare v2 based on master and addressing your feedback.\n\nThanks,\nDscho\n"}]}