{"thread":{"id":"44748","subject":"[PATCH 0/2] Really fix the isatty() problem on Windows","startedAt":"2016-12-21T17:53:43Z","lastAt":"2017-01-18T12:14:16Z","messageCount":29,"participants":["Johannes Schindelin","Junio C Hamano","Johannes Sixt","Beat Bolli"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"308201","messageId":"cover.1482342791.git.johannes.schindelin@gmx.de","threadId":"44748","inReplyTo":null,"subject":"[PATCH 0/2] Really fix the isatty() problem on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-21T17:53:30Z","receivedAt":"2016-12-21T17:53:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"My previous fix may have fixed running the new\nt/t6030-bisect-porcelain.sh script that tested the new bisect--helper,\nwhich in turn used isatty() to determine whether it was running\ninteractively and was fooled by being redirected to /dev/null.\n\nBut it not only broke paging when running in a CMD window, due to\ntesting in the wrong worktree I also missed that it broke paging in Git\nfor Windows 2.x' Git Bash (i.e. a MinTTY terminal emulator).\n\nLet's use this opportunity to actually clean up the entire isatty() mess\nonce and for all, as part of the problem was introduced by a clever hack\nthat messes with internals of the Microsoft C runtime, and which changed\nrecently, so it was not such a clever hack to begin with.\n\nHappily, one of my colleagues had to address that latter problem\nrecently when he was tasked to make Git compile with Microsoft Visual C\n(the rationale: debugging facilities of Visual Studio are really\noutstanding, try them if you get a chance).\n\nAnd incidentally, replacing the previous hack with the clean, new\nsolution, which specifies explicitly for the file descriptors 0, 1 and 2\nwhether we detected an MSYS2 pseudo-tty, whether we detected a real\nWin32 Console, and whether we had to swap out a real Win32 Console for a\npipe to allow child processes to inherit it.\n\nHannes, I am really sorry that I did not test the original attempt\nbetter, I tried to be a better boy this time. Could you maybe also give\nit a try?\n\nThe current patch series is based on `pu`, as that already has the\nwinansi_get_osfhandle() fix. For ease of testing, I also have a branch\nbased on master which you can pull via\n\n\tgit pull https://github.com/dscho/git mingw-isatty-fixup-master\n\n\nJeff Hostetler (1):\n mingw: replace isatty() hack\n\nJohannes Schindelin (1):\n mingw: adjust is_console() to work with stdin\n\n compat/winansi.c | 197 +++++++++++++++++++++++--------------------------------\n 1 file changed, 83 insertions(+), 114 deletions(-)\n\n\nbase-commit: 1921ea0f1c7358b5200a456fba02aa79fb9e76d3\nBased-On: junio/pu at https://github.com/dscho/git\nFetch-Base-Via: git fetch https://github.com/dscho/git junio/pu\nPublished-As: https://github.com/dscho/git/releases/tag/mingw-isatty-fixup-v1\nFetch-It-Via: git fetch https://github.com/dscho/git mingw-isatty-fixup-v1\n\n-- \n2.11.0.rc3.windows.1\n\n"},{"id":"308202","messageId":"8c472bdd0be9d767c5388105f452e27b9118a0f9.1482342791.git.johannes.schindelin@gmx.de","threadId":"44748","inReplyTo":"cover.1482342791.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/2] mingw: adjust is_console() to work with stdin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-21T17:53:36Z","receivedAt":"2016-12-21T17:54:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When determining whether a handle corresponds to a *real* Win32 Console\n(as opposed to, say, a character device such as /dev/null), we use the\nGetConsoleOutputBufferInfo() function as a tell-tale.\n\nHowever, that does not work for *input* handles associated with a\nconsole. Let's just use the GetConsoleMode() function for input handles,\nand since it does not work on output handles fall back to the previous\nmethod for those.\n\nThis patch prepares for using is_console() instead of my previous\nmisguided attempt in cbb3f3c9b1 (mingw: intercept isatty() to handle\n/dev/null as Git expects it, 2016-12-11) that broke everything on\nWindows.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/winansi.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/winansi.c b/compat/winansi.c\nindex 5b2f5638ec..97a004b8ab 100644\n--- a/compat/winansi.c\n+++ b/compat/winansi.c\n@@ -84,6 +84,7 @@ static void warn_if_raster_font(void)\n static int is_console(int fd)\n {\n \tCONSOLE_SCREEN_BUFFER_INFO sbi;\n+\tDWORD mode;\n \tHANDLE hcon;\n \n \tstatic int initialized = 0;\n@@ -98,7 +99,10 @@ static int is_console(int fd)\n \t\treturn 0;\n \n \t/* check if its a handle to a console output screen buffer */\n-\tif (!GetConsoleScreenBufferInfo(hcon, &sbi))\n+\tif (!fd) {\n+\t\tif (!GetConsoleMode(hcon, &mode))\n+\t\t\treturn 0;\n+\t} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))\n \t\treturn 0;\n \n \t/* initialize attributes */\n-- \n2.11.0.rc3.windows.1\n\n\n"},{"id":"308203","messageId":"18174f0a7fbb4a0ccd8ca8380e00161826166a32.1482342791.git.johannes.schindelin@gmx.de","threadId":"44748","inReplyTo":"cover.1482342791.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/2] mingw: replace isatty() hack","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-21T17:53:43Z","receivedAt":"2016-12-21T17:54:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Jeff Hostetler \n\nFor over a year, Git for Windows has carried a patch that detects the\nMSYS2 pseudo ttys used by Git for Windows' default Git Bash (i.e. a\nterminal that is not backed by a Win32 Console).\n\nThis patch accesses internals that of a previous MSVC runtime that is no\nlonger valid in newer versions, therefore we needed a replacement for\nthat hack in order to be able to compile Git using recent Microsoft\nVisual C++.\n\nThis patch back-ports that patch and makes even the MINGW (i.e.\nGCC-compiled) Git use it.\n\nAs a side effect (which was the reason for the back-port), this patch\nalso fixes the previous misguided attempt to intercept isatty() so that\nit handles character devices (such as /dev/null) as Git expects it.\n\nSigned-off-by: Jeff Hostetler \nSigned-off-by: Johannes Schindelin \n---\n compat/winansi.c | 191 +++++++++++++++++++++++--------------------------------\n 1 file changed, 78 insertions(+), 113 deletions(-)\n\ndiff --git a/compat/winansi.c b/compat/winansi.c\nindex 97a004b8ab..3bfad0d065 100644\n--- a/compat/winansi.c\n+++ b/compat/winansi.c\n@@ -6,9 +6,12 @@\n #include \"../git-compat-util.h\"\n #include \n #include \n+#include \"win32.h\"\n \n-/* In this file, we actually want to use Windows' own isatty(). */\n-#undef isatty\n+static int fd_is_interactive[3] = { 0, 0, 0 };\n+#define FD_CONSOLE 0x1\n+#define FD_SWAPPED 0x2\n+#define FD_MSYS 0x4\n \n /*\n ANSI codes used by git: m, K\n@@ -105,6 +108,9 @@ static int is_console(int fd)\n \t} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))\n \t\treturn 0;\n \n+\tif (fd >=0 && fd <= 2)\n+\t\tfd_is_interactive[fd] |= FD_CONSOLE;\n+\n \t/* initialize attributes */\n \tif (!initialized) {\n \t\tconsole = hcon;\n@@ -466,76 +472,50 @@ static HANDLE duplicate_handle(HANDLE hnd)\n \treturn hresult;\n }\n \n-\n-/*\n- * Make MSVCRT's internal file descriptor control structure accessible\n- * so that we can tweak OS handles and flags directly (we need MSVCRT\n- * to treat our pipe handle as if it were a console).\n- *\n- * We assume that the ioinfo structure (exposed by MSVCRT.dll via\n- * __pioinfo) starts with the OS handle and the flags. The exact size\n- * varies between MSVCRT versions, so we try different sizes until\n- * toggling the FDEV bit of _pioinfo(1)->osflags is reflected in\n- * isatty(1).\n- */\n-typedef struct {\n-\tHANDLE osfhnd;\n-\tchar osflags;\n-} ioinfo;\n-\n-extern __declspec(dllimport) ioinfo *__pioinfo[];\n-\n-static size_t sizeof_ioinfo = 0;\n-\n-#define IOINFO_L2E 5\n-#define IOINFO_ARRAY_ELTS (1 << IOINFO_L2E)\n-\n-#define FPIPE 0x08\n-#define FDEV 0x40\n-\n-static inline ioinfo* _pioinfo(int fd)\n-{\n-\treturn (ioinfo*)((char*)__pioinfo[fd >> IOINFO_L2E] +\n-\t\t\t(fd & (IOINFO_ARRAY_ELTS - 1)) * sizeof_ioinfo);\n-}\n-\n-static int init_sizeof_ioinfo(void)\n-{\n-\tint istty, wastty;\n-\t/* don't init twice */\n-\tif (sizeof_ioinfo)\n-\t\treturn sizeof_ioinfo >= 256;\n-\n-\tsizeof_ioinfo = sizeof(ioinfo);\n-\twastty = isatty(1);\n-\twhile (sizeof_ioinfo < 256) {\n-\t\t/* toggle FDEV flag, check isatty, then toggle back */\n-\t\t_pioinfo(1)->osflags ^= FDEV;\n-\t\tistty = isatty(1);\n-\t\t_pioinfo(1)->osflags ^= FDEV;\n-\t\t/* return if we found the correct size */\n-\t\tif (istty != wastty)\n-\t\t\treturn 0;\n-\t\tsizeof_ioinfo += sizeof(void*);\n-\t}\n-\terror(\"Tweaking file descriptors doesn't work with this MSVCRT.dll\");\n-\treturn 1;\n-}\n-\n static HANDLE swap_osfhnd(int fd, HANDLE new_handle)\n {\n-\tioinfo *pioinfo;\n-\tHANDLE old_handle;\n-\n-\t/* init ioinfo size if we haven't done so */\n-\tif (init_sizeof_ioinfo())\n-\t\treturn INVALID_HANDLE_VALUE;\n-\n-\t/* get ioinfo pointer and change the handles */\n-\tpioinfo = _pioinfo(fd);\n-\told_handle = pioinfo->osfhnd;\n-\tpioinfo->osfhnd = new_handle;\n-\treturn old_handle;\n+\t/*\n+\t * Create a copy of the original handle associated with fd\n+\t * because the original will get closed when we dup2().\n+\t */\n+\tHANDLE handle = (HANDLE)_get_osfhandle(fd);\n+\tHANDLE duplicate = duplicate_handle(handle);\n+\n+\t/* Create a temp fd associated with the already open \"new_handle\". */\n+\tint new_fd = _open_osfhandle((intptr_t)new_handle, O_BINARY);\n+\n+\tassert((fd == 1) || (fd == 2));\n+\n+\t/*\n+\t * Use stock dup2() to re-bind fd to the new handle. Note that\n+\t * this will implicitly close(1) and close both fd=1 and the\n+\t * originally associated handle. It will open a new fd=1 and\n+\t * call DuplicateHandle() on the handle associated with new_fd.\n+\t * It is because of this implicit close() that we created the\n+\t * copy of the original.\n+\t *\n+\t * Note that the OS can recycle HANDLE (numbers) just like it\n+\t * recycles fd (numbers), so we must update the cached value\n+\t * of \"console\". You can use GetFileType() to see that\n+\t * handle and _get_osfhandle(fd) may have the same number\n+\t * value, but they refer to different actual files now.\n+\t *\n+\t * Note that dup2() when given target := {0,1,2} will also\n+\t * call SetStdHandle(), so we don't need to worry about that.\n+\t */\n+\tdup2(new_fd, fd);\n+\tif (console == handle)\n+\t\tconsole = duplicate;\n+\thandle = INVALID_HANDLE_VALUE;\n+\n+\t/* Close the temp fd. This explicitly closes \"new_handle\"\n+\t * (because it has been associated with it).\n+\t */\n+\tclose(new_fd);\n+\n+\tfd_is_interactive[fd] |= FD_SWAPPED;\n+\n+\treturn duplicate;\n }\n \n #ifdef DETECT_MSYS_TTY\n@@ -562,49 +542,32 @@ static void detect_msys_tty(int fd)\n \tname = nameinfo->Name.Buffer;\n \tname[nameinfo->Name.Length / sizeof(*name)] = 0;\n \n-\t/* check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX') */\n-\tif (!wcsstr(name, L\"msys-\") || !wcsstr(name, L\"-pty\"))\n+\t/*\n+\t * Check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX')\n+\t * or a cygwin pty pipe ('cygwin-XXXX-ptyN-XX')\n+\t */\n+\tif ((!wcsstr(name, L\"msys-\") && !wcsstr(name, L\"cygwin-\")) ||\n+\t\t\t!wcsstr(name, L\"-pty\"))\n \t\treturn;\n \n-\t/* init ioinfo size if we haven't done so */\n-\tif (init_sizeof_ioinfo())\n-\t\treturn;\n-\n-\t/* set FDEV flag, reset FPIPE flag */\n-\t_pioinfo(fd)->osflags &= ~FPIPE;\n-\t_pioinfo(fd)->osflags |= FDEV;\n+\tfd_is_interactive[fd] |= FD_MSYS;\n }\n \n #endif\n \n+/* Wrapper for isatty(). Most calls in the main git code\n+ * call isatty(1 or 2) to see if the instance is interactive\n+ * and should: be colored, show progress, paginate output.\n+ * We lie and give results for what the descriptor WAS at\n+ * startup (and ignore any pipe redirection we internally\n+ * do).\n+ */\n+#undef isatty\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 = winansi_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+\tif (fd >=0 && fd <= 2)\n+\t\treturn fd_is_interactive[fd] != 0;\n+\treturn isatty(fd);\n }\n \n void winansi_init(void)\n@@ -615,6 +578,10 @@ void winansi_init(void)\n \t/* check if either stdout or stderr is a console output screen buffer */\n \tcon1 = is_console(1);\n \tcon2 = is_console(2);\n+\n+\t/* Also compute console bit for fd 0 even though we don't need the result here. */\n+\tis_console(0);\n+\n \tif (!con1 && !con2) {\n #ifdef DETECT_MSYS_TTY\n \t\t/* check if stdin / stdout / stderr are MSYS2 pty pipes */\n@@ -658,12 +625,10 @@ void winansi_init(void)\n */\n HANDLE winansi_get_osfhandle(int fd)\n {\n-\tHANDLE hnd = (HANDLE) _get_osfhandle(fd);\n-\tif (isatty(fd) && GetFileType(hnd) == FILE_TYPE_PIPE) {\n-\t\tif (fd == 1 && hconsole1)\n-\t\t\treturn hconsole1;\n-\t\telse if (fd == 2 && hconsole2)\n-\t\t\treturn hconsole2;\n-\t}\n-\treturn hnd;\n+\tif (fd == 1 && (fd_is_interactive[1] & FD_SWAPPED))\n+\t\treturn hconsole1;\n+\tif (fd == 2 && (fd_is_interactive[2] & FD_SWAPPED))\n+\t\treturn hconsole2;\n+\n+\treturn (HANDLE)_get_osfhandle(fd);\n }\n-- \n2.11.0.rc3.windows.1\n"},{"id":"308209","messageId":"xmqqpokli0jh.fsf@gitster.mtv.corp.google.com","threadId":"44748","inReplyTo":"18174f0a7fbb4a0ccd8ca8380e00161826166a32.1482342791.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 2/2] mingw: replace isatty() hack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-21T18:45:22Z","receivedAt":"2016-12-21T18:45:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> From: Jeff Hostetler \n>\n> For over a year, Git for Windows has carried a patch that detects the\n> MSYS2 pseudo ttys used by Git for Windows' default Git Bash (i.e. a\n> terminal that is not backed by a Win32 Console).\n>\n> This patch accesses internals that of a previous MSVC runtime that is no\n> longer valid in newer versions, therefore we needed a replacement for\n> that hack in order to be able to compile Git using recent Microsoft\n> Visual C++.\n\nSorry, but I cannot parse the early part of the first sentence of\nthe second paragraph before the comma; I am especially having\ntrouble around the first \"that\".\n\n> This patch back-ports that patch and makes even the MINGW (i.e.\n> GCC-compiled) Git use it.\n>\n> As a side effect (which was the reason for the back-port), this patch\n> also fixes the previous misguided attempt to intercept isatty() so that\n> it handles character devices (such as /dev/null) as Git expects it.\n\nI had to read the above three times to understand which patches\nthree instances of \"This patch\" and one instance of \"that patch\"\nrefer to. I wish it were easier to read, but I think I got them all\nright [*1*] after re-reading, and the story made sense to me.\n\n> +static int fd_is_interactive[3] = { 0, 0, 0 };\n> +#define FD_CONSOLE 0x1\n> +#define FD_SWAPPED 0x2\n> +#define FD_MSYS 0x4\n> \n> /*\n> ANSI codes used by git: m, K\n> @@ -105,6 +108,9 @@ static int is_console(int fd)\n> \t} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))\n> \t\treturn 0;\n> \n> +\tif (fd >=0 && fd <= 2)\n\nStyle: if (fd >= 0 && fd <= 2)\n\n> +/* Wrapper for isatty(). Most calls in the main git code\n\nStyle: /*\n\t* multi-line comment block begins with slash-asterisk\n * and ends with asterisk-slash without anything else on\n\t* the line.\n\t*/\n\n> + * call isatty(1 or 2) to see if the instance is interactive\n> + * and should: be colored, show progress, paginate output.\n> + * We lie and give results for what the descriptor WAS at\n> + * startup (and ignore any pipe redirection we internally\n> + * do).\n> + */\n> +#undef isatty\n> int winansi_isatty(int fd)\n> {\n> +\tif (fd >=0 && fd <= 2)\n\nStyle: if (fd >= 0 && fd <= 2)\n\n> +\t\treturn fd_is_interactive[fd] != 0;\n> +\treturn isatty(fd);\n> }\n\nThanks.\n\n\n[Footnote]\n\n*1* What I thought I understood in my own words:\n\n Git for Windows has carried a patch that depended on internals\n of MSVC runtime, but it does not work correctly with recent MSVC\n runtime. A replacement was written originally for compiling\n with VC++. The patch in this message is a backport of that\n replacement, and it also fixes the previous attempt to make\n isatty() work.\n\n"},{"id":"308219","messageId":"b0541907-ee79-207b-dc0f-1e3e7d761950@kdbg.org","threadId":"44748","inReplyTo":"cover.1482342791.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 0/2] Really fix the isatty() problem on Windows","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-12-21T21:15:10Z","receivedAt":"2016-12-21T21:15:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 21.12.2016 um 18:53 schrieb Johannes Schindelin:\n> The current patch series is based on `pu`, as that already has the\n> winansi_get_osfhandle() fix. For ease of testing, I also have a branch\n> based on master which you can pull via\n>\n> \tgit pull https://github.com/dscho/git mingw-isatty-fixup-master\n\nWill test and report back tomorrow.\n\n-- Hannes\n\n"},{"id":"308220","messageId":"xmqqzijpger6.fsf@gitster.mtv.corp.google.com","threadId":"44748","inReplyTo":"b0541907-ee79-207b-dc0f-1e3e7d761950@kdbg.org","subject":"Re: [PATCH 0/2] Really fix the isatty() problem on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-21T21:21:17Z","receivedAt":"2016-12-21T21:21:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 21.12.2016 um 18:53 schrieb Johannes Schindelin:\n>> The current patch series is based on `pu`, as that already has the\n>> winansi_get_osfhandle() fix. For ease of testing, I also have a branch\n>> based on master which you can pull via\n>>\n>> \tgit pull https://github.com/dscho/git mingw-isatty-fixup-master\n>\n> Will test and report back tomorrow.\n\nThanks.\n"},{"id":"308246","messageId":"cover.1482426497.git.johannes.schindelin@gmx.de","threadId":"44748","inReplyTo":"cover.1482342791.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 0/3] Really fix the isatty() problem on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-22T17:08:51Z","receivedAt":"2016-12-22T17:09:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"My previous fix may have fixed running the new\nt/t6030-bisect-porcelain.sh script that tested the new bisect--helper,\nwhich in turn used isatty() to determine whether it was running\ninteractively and was fooled by being redirected to /dev/null.\n\nBut it not only broke paging when running in a CMD window, due to\ntesting in the wrong worktree I also missed that it broke paging in Git\nfor Windows 2.x' Git Bash (i.e. a MinTTY terminal emulator).\n\nLet's use this opportunity to actually clean up the entire isatty() mess\nonce and for all, as part of the problem was introduced by a clever hack\nthat messes with internals of the Microsoft C runtime, and which changed\nrecently, so it was not such a clever hack to begin with.\n\nHappily, one of my colleagues had to address that latter problem\nrecently when he was tasked to make Git compile with Microsoft Visual C\n(the rationale: debugging facilities of Visual Studio are really\noutstanding, try them if you get a chance).\n\nAnd incidentally, replacing the previous hack with the clean, new\nsolution, which specifies explicitly for the file descriptors 0, 1 and 2\nwhether we detected an MSYS2 pseudo-tty, whether we detected a real\nWin32 Console, and whether we had to swap out a real Win32 Console for a\npipe to allow child processes to inherit it.\n\nWhile at it (or, actually, more like: as I already made this part of v1\nby mistake), upstream the patch carried in Git for Windows that supports\ncolor when running Git for Windows in Cygwin terminals.\n\nChanges since v1:\n\n- rebased onto master\n\n- unsquashed 2/3 which was improperly snuck in before,\n\n- noted that Beat Bolli tested this (see\n https://github.com/git-for-windows/git/issues/997#issuecomment-268764693)\n\n- fixed the confusing commit message by using Junio's suggested\n replacement\n\n- added the missing white space between \">=\" and \"0\"\n\n\nAlan Davies (1):\n mingw: fix colourization on Cygwin pseudo terminals\n\nJeff Hostetler (1):\n mingw: replace isatty() hack\n\nJohannes Schindelin (1):\n mingw: adjust is_console() to work with stdin\n\n compat/winansi.c | 198 +++++++++++++++++++++++--------------------------------\n 1 file changed, 84 insertions(+), 114 deletions(-)\n\n\nbase-commit: 1d1bdafd64266e5ee3bd46c6965228f32e4022ea\nPublished-As: https://github.com/dscho/git/releases/tag/mingw-isatty-fixup-v2\nFetch-It-Via: git fetch https://github.com/dscho/git mingw-isatty-fixup-v2\n\nInterdiff vs v1:\n\n diff --git a/compat/winansi.c b/compat/winansi.c\n index f51a2856d2..477209fce7 100644\n --- a/compat/winansi.c\n +++ b/compat/winansi.c\n @@ -108,7 +108,7 @@ static int is_console(int fd)\n \t} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))\n \t\treturn 0;\n \n -\tif (fd >=0 && fd <= 2)\n +\tif (fd >= 0 && fd <= 2)\n \t\tfd_is_interactive[fd] |= FD_CONSOLE;\n \n \t/* initialize attributes */\n @@ -555,7 +555,8 @@ static void detect_msys_tty(int fd)\n \n #endif\n \n -/* Wrapper for isatty(). Most calls in the main git code\n +/*\n + * Wrapper for isatty(). Most calls in the main git code\n * call isatty(1 or 2) to see if the instance is interactive\n * and should: be colored, show progress, paginate output.\n * We lie and give results for what the descriptor WAS at\n @@ -565,7 +566,7 @@ static void detect_msys_tty(int fd)\n #undef isatty\n int winansi_isatty(int fd)\n {\n -\tif (fd >=0 && fd <= 2)\n +\tif (fd >= 0 && fd <= 2)\n \t\treturn fd_is_interactive[fd] != 0;\n \treturn isatty(fd);\n }\n\n-- \n2.11.0.rc3.windows.1\n\n"},{"id":"308247","messageId":"796e3c4b5bda1170bfbb690a73dab5442b9c77e4.1482426497.git.johannes.schindelin@gmx.de","threadId":"44748","inReplyTo":"cover.1482426497.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 2/3] mingw: fix colourization on Cygwin pseudo terminals","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-22T17:09:18Z","receivedAt":"2016-12-22T17:09:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Alan Davies \n\nGit only colours the output and uses pagination if isatty() returns 1.\nMSYS2 and Cygwin emulate pseudo terminals via named pipes, meaning that\nisatty() returns 0.\n\nf7f90e0f4f (mingw: make isatty() recognize MSYS2's pseudo terminals\n(/dev/pty*), 2016-04-27) fixed this for MSYS2 terminals, but not for\nCygwin.\n\nThe named pipes that Cygwin and MSYS2 use are very similar. MSYS2 PTY pipes\nare called 'msys-*-pty*' and Cygwin uses 'cygwin-*-pty*'. This commit\nmodifies the existing check to allow both MSYS2 and Cygwin PTY pipes to be\nidentified as TTYs.\n\nNote that pagination is still broken when running Git for Windows from\nwithin Cygwin, as MSYS2's less.exe is spawned (and does not like to\ninteract with Cygwin's PTY).\n\nThis partially fixes https://github.com/git-for-windows/git/issues/267\n\nSigned-off-by: Alan Davies \nSigned-off-by: Johannes Schindelin \n---\n compat/winansi.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/winansi.c b/compat/winansi.c\nindex 590d61cb1b..fa37695fca 100644\n--- a/compat/winansi.c\n+++ b/compat/winansi.c\n@@ -562,8 +562,12 @@ static void detect_msys_tty(int fd)\n \tname = nameinfo->Name.Buffer;\n \tname[nameinfo->Name.Length] = 0;\n \n-\t/* check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX') */\n-\tif (!wcsstr(name, L\"msys-\") || !wcsstr(name, L\"-pty\"))\n+\t/*\n+\t * Check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX')\n+\t * or a cygwin pty pipe ('cygwin-XXXX-ptyN-XX')\n+\t */\n+\tif ((!wcsstr(name, L\"msys-\") && !wcsstr(name, L\"cygwin-\")) ||\n+\t\t\t!wcsstr(name, L\"-pty\"))\n \t\treturn;\n \n \t/* init ioinfo size if we haven't done so */\n-- \n2.11.0.rc3.windows.1\n\n\n"},{"id":"308248","messageId":"5e3c505a206a735e6ba0bfaf4c73965e95a928eb.1482426497.git.johannes.schindelin@gmx.de","threadId":"44748","inReplyTo":"cover.1482426497.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 3/3] mingw: replace isatty() hack","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-22T17:09:23Z","receivedAt":"2016-12-22T17:09:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Jeff Hostetler \n\nGit for Windows has carried a patch that depended on internals\nof MSVC runtime, but it does not work correctly with recent MSVC\nruntime. A replacement was written originally for compiling\nwith VC++. The patch in this message is a backport of that\nreplacement, and it also fixes the previous attempt to make\nisatty() tell that /dev/null is *not* an interactive terminal.\n\nSigned-off-by: Jeff Hostetler \nTested-by: Beat Bolli \nSigned-off-by: Johannes Schindelin \n---\n compat/winansi.c | 176 ++++++++++++++++++++++---------------------------------\n 1 file changed, 69 insertions(+), 107 deletions(-)\n\ndiff --git a/compat/winansi.c b/compat/winansi.c\nindex fa37695fca..477209fce7 100644\n--- a/compat/winansi.c\n+++ b/compat/winansi.c\n@@ -6,9 +6,12 @@\n #include \"../git-compat-util.h\"\n #include \n #include \n+#include \"win32.h\"\n \n-/* In this file, we actually want to use Windows' own isatty(). */\n-#undef isatty\n+static int fd_is_interactive[3] = { 0, 0, 0 };\n+#define FD_CONSOLE 0x1\n+#define FD_SWAPPED 0x2\n+#define FD_MSYS 0x4\n \n /*\n ANSI codes used by git: m, K\n@@ -105,6 +108,9 @@ static int is_console(int fd)\n \t} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))\n \t\treturn 0;\n \n+\tif (fd >= 0 && fd <= 2)\n+\t\tfd_is_interactive[fd] |= FD_CONSOLE;\n+\n \t/* initialize attributes */\n \tif (!initialized) {\n \t\tconsole = hcon;\n@@ -466,76 +472,50 @@ static HANDLE duplicate_handle(HANDLE hnd)\n \treturn hresult;\n }\n \n+static HANDLE swap_osfhnd(int fd, HANDLE new_handle)\n+{\n+\t/*\n+\t * Create a copy of the original handle associated with fd\n+\t * because the original will get closed when we dup2().\n+\t */\n+\tHANDLE handle = (HANDLE)_get_osfhandle(fd);\n+\tHANDLE duplicate = duplicate_handle(handle);\n \n-/*\n- * Make MSVCRT's internal file descriptor control structure accessible\n- * so that we can tweak OS handles and flags directly (we need MSVCRT\n- * to treat our pipe handle as if it were a console).\n- *\n- * We assume that the ioinfo structure (exposed by MSVCRT.dll via\n- * __pioinfo) starts with the OS handle and the flags. The exact size\n- * varies between MSVCRT versions, so we try different sizes until\n- * toggling the FDEV bit of _pioinfo(1)->osflags is reflected in\n- * isatty(1).\n- */\n-typedef struct {\n-\tHANDLE osfhnd;\n-\tchar osflags;\n-} ioinfo;\n-\n-extern __declspec(dllimport) ioinfo *__pioinfo[];\n-\n-static size_t sizeof_ioinfo = 0;\n+\t/* Create a temp fd associated with the already open \"new_handle\". */\n+\tint new_fd = _open_osfhandle((intptr_t)new_handle, O_BINARY);\n \n-#define IOINFO_L2E 5\n-#define IOINFO_ARRAY_ELTS (1 << IOINFO_L2E)\n+\tassert((fd == 1) || (fd == 2));\n \n-#define FPIPE 0x08\n-#define FDEV 0x40\n+\t/*\n+\t * Use stock dup2() to re-bind fd to the new handle. Note that\n+\t * this will implicitly close(1) and close both fd=1 and the\n+\t * originally associated handle. It will open a new fd=1 and\n+\t * call DuplicateHandle() on the handle associated with new_fd.\n+\t * It is because of this implicit close() that we created the\n+\t * copy of the original.\n+\t *\n+\t * Note that the OS can recycle HANDLE (numbers) just like it\n+\t * recycles fd (numbers), so we must update the cached value\n+\t * of \"console\". You can use GetFileType() to see that\n+\t * handle and _get_osfhandle(fd) may have the same number\n+\t * value, but they refer to different actual files now.\n+\t *\n+\t * Note that dup2() when given target := {0,1,2} will also\n+\t * call SetStdHandle(), so we don't need to worry about that.\n+\t */\n+\tdup2(new_fd, fd);\n+\tif (console == handle)\n+\t\tconsole = duplicate;\n+\thandle = INVALID_HANDLE_VALUE;\n \n-static inline ioinfo* _pioinfo(int fd)\n-{\n-\treturn (ioinfo*)((char*)__pioinfo[fd >> IOINFO_L2E] +\n-\t\t\t(fd & (IOINFO_ARRAY_ELTS - 1)) * sizeof_ioinfo);\n-}\n+\t/* Close the temp fd. This explicitly closes \"new_handle\"\n+\t * (because it has been associated with it).\n+\t */\n+\tclose(new_fd);\n \n-static int init_sizeof_ioinfo(void)\n-{\n-\tint istty, wastty;\n-\t/* don't init twice */\n-\tif (sizeof_ioinfo)\n-\t\treturn sizeof_ioinfo >= 256;\n-\n-\tsizeof_ioinfo = sizeof(ioinfo);\n-\twastty = isatty(1);\n-\twhile (sizeof_ioinfo < 256) {\n-\t\t/* toggle FDEV flag, check isatty, then toggle back */\n-\t\t_pioinfo(1)->osflags ^= FDEV;\n-\t\tistty = isatty(1);\n-\t\t_pioinfo(1)->osflags ^= FDEV;\n-\t\t/* return if we found the correct size */\n-\t\tif (istty != wastty)\n-\t\t\treturn 0;\n-\t\tsizeof_ioinfo += sizeof(void*);\n-\t}\n-\terror(\"Tweaking file descriptors doesn't work with this MSVCRT.dll\");\n-\treturn 1;\n-}\n+\tfd_is_interactive[fd] |= FD_SWAPPED;\n \n-static HANDLE swap_osfhnd(int fd, HANDLE new_handle)\n-{\n-\tioinfo *pioinfo;\n-\tHANDLE old_handle;\n-\n-\t/* init ioinfo size if we haven't done so */\n-\tif (init_sizeof_ioinfo())\n-\t\treturn INVALID_HANDLE_VALUE;\n-\n-\t/* get ioinfo pointer and change the handles */\n-\tpioinfo = _pioinfo(fd);\n-\told_handle = pioinfo->osfhnd;\n-\tpioinfo->osfhnd = new_handle;\n-\treturn old_handle;\n+\treturn duplicate;\n }\n \n #ifdef DETECT_MSYS_TTY\n@@ -570,45 +550,25 @@ static void detect_msys_tty(int fd)\n \t\t\t!wcsstr(name, L\"-pty\"))\n \t\treturn;\n \n-\t/* init ioinfo size if we haven't done so */\n-\tif (init_sizeof_ioinfo())\n-\t\treturn;\n-\n-\t/* set FDEV flag, reset FPIPE flag */\n-\t_pioinfo(fd)->osflags &= ~FPIPE;\n-\t_pioinfo(fd)->osflags |= FDEV;\n+\tfd_is_interactive[fd] |= FD_MSYS;\n }\n \n #endif\n \n+/*\n+ * Wrapper for isatty(). Most calls in the main git code\n+ * call isatty(1 or 2) to see if the instance is interactive\n+ * and should: be colored, show progress, paginate output.\n+ * We lie and give results for what the descriptor WAS at\n+ * startup (and ignore any pipe redirection we internally\n+ * do).\n+ */\n+#undef isatty\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+\tif (fd >= 0 && fd <= 2)\n+\t\treturn fd_is_interactive[fd] != 0;\n+\treturn isatty(fd);\n }\n \n void winansi_init(void)\n@@ -619,6 +579,10 @@ void winansi_init(void)\n \t/* check if either stdout or stderr is a console output screen buffer */\n \tcon1 = is_console(1);\n \tcon2 = is_console(2);\n+\n+\t/* Also compute console bit for fd 0 even though we don't need the result here. */\n+\tis_console(0);\n+\n \tif (!con1 && !con2) {\n #ifdef DETECT_MSYS_TTY\n \t\t/* check if stdin / stdout / stderr are MSYS2 pty pipes */\n@@ -662,12 +626,10 @@ void winansi_init(void)\n */\n HANDLE winansi_get_osfhandle(int fd)\n {\n-\tHANDLE hnd = (HANDLE) _get_osfhandle(fd);\n-\tif (isatty(fd) && GetFileType(hnd) == FILE_TYPE_PIPE) {\n-\t\tif (fd == 1 && hconsole1)\n-\t\t\treturn hconsole1;\n-\t\telse if (fd == 2 && hconsole2)\n-\t\t\treturn hconsole2;\n-\t}\n-\treturn hnd;\n+\tif (fd == 1 && (fd_is_interactive[1] & FD_SWAPPED))\n+\t\treturn hconsole1;\n+\tif (fd == 2 && (fd_is_interactive[2] & FD_SWAPPED))\n+\t\treturn hconsole2;\n+\n+\treturn (HANDLE)_get_osfhandle(fd);\n }\n-- \n2.11.0.rc3.windows.1\n"},{"id":"308249","messageId":"ca4c61c603247c8ad0b876b068f6cd41fbe01667.1482426497.git.johannes.schindelin@gmx.de","threadId":"44748","inReplyTo":"cover.1482426497.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 1/3] mingw: adjust is_console() to work with stdin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-22T17:08:57Z","receivedAt":"2016-12-22T17:09:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When determining whether a handle corresponds to a *real* Win32 Console\n(as opposed to, say, a character device such as /dev/null), we use the\nGetConsoleOutputBufferInfo() function as a tell-tale.\n\nHowever, that does not work for *input* handles associated with a\nconsole. Let's just use the GetConsoleMode() function for input handles,\nand since it does not work on output handles fall back to the previous\nmethod for those.\n\nThis patch prepares for using is_console() instead of my previous\nmisguided attempt in cbb3f3c9b1 (mingw: intercept isatty() to handle\n/dev/null as Git expects it, 2016-12-11) that broke everything on\nWindows.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/winansi.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/winansi.c b/compat/winansi.c\nindex cb725fb02f..590d61cb1b 100644\n--- a/compat/winansi.c\n+++ b/compat/winansi.c\n@@ -84,6 +84,7 @@ static void warn_if_raster_font(void)\n static int is_console(int fd)\n {\n \tCONSOLE_SCREEN_BUFFER_INFO sbi;\n+\tDWORD mode;\n \tHANDLE hcon;\n \n \tstatic int initialized = 0;\n@@ -98,7 +99,10 @@ static int is_console(int fd)\n \t\treturn 0;\n \n \t/* check if its a handle to a console output screen buffer */\n-\tif (!GetConsoleScreenBufferInfo(hcon, &sbi))\n+\tif (!fd) {\n+\t\tif (!GetConsoleMode(hcon, &mode))\n+\t\t\treturn 0;\n+\t} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))\n \t\treturn 0;\n \n \t/* initialize attributes */\n-- \n2.11.0.rc3.windows.1\n\n\n"},{"id":"308251","messageId":"xmqqd1gjhn0u.fsf@gitster.mtv.corp.google.com","threadId":"44748","inReplyTo":"cover.1482426497.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 0/3] Really fix the isatty() problem on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-22T17:49:37Z","receivedAt":"2016-12-22T17:49:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> My previous fix may have fixed running the new\n> t/t6030-bisect-porcelain.sh script that tested the new bisect--helper,\n> which in turn used isatty() to determine whether it was running\n> interactively and was fooled by being redirected to /dev/null.\n>\n> But it not only broke paging when running in a CMD window, due to\n> testing in the wrong worktree I also missed that it broke paging in Git\n> for Windows 2.x' Git Bash (i.e. a MinTTY terminal emulator).\n>\n> Let's use this opportunity to actually clean up the entire isatty() mess\n> once and for all, as part of the problem was introduced by a clever hack\n> that messes with internals of the Microsoft C runtime, and which changed\n> recently, so it was not such a clever hack to begin with.\n>\n> Happily, one of my colleagues had to address that latter problem\n> recently when he was tasked to make Git compile with Microsoft Visual C\n> (the rationale: debugging facilities of Visual Studio are really\n> outstanding, try them if you get a chance).\n>\n> And incidentally, replacing the previous hack with the clean, new\n> solution, which specifies explicitly for the file descriptors 0, 1 and 2\n> whether we detected an MSYS2 pseudo-tty, whether we detected a real\n> Win32 Console, and whether we had to swap out a real Win32 Console for a\n> pipe to allow child processes to inherit it.\n>\n> While at it (or, actually, more like: as I already made this part of v1\n> by mistake), upstream the patch carried in Git for Windows that supports\n> color when running Git for Windows in Cygwin terminals.\n>\n> Changes since v1:\n>\n> - rebased onto master\n>\n> - unsquashed 2/3 which was improperly snuck in before,\n\nAs Windows specific changes, I didn't notice these two were independent.\n\n> - noted that Beat Bolli tested this (see\n> https://github.com/git-for-windows/git/issues/997#issuecomment-268764693)\n>\n> - fixed the confusing commit message by using Junio's suggested\n> replacement\n\nSorry, but I didn't mean to \"suggest replacement\". I was just\ntesting my understanding by attempt to rephrase the gist of it.\n\nThere was one thing I still wasn't clear in my \"summary of my\nunderstanding\". Is the \"replacement originally done for compiling\nwith VC++\" a solution that still peeks into MSVC runtime internals\nbut is usable with both old and more recent one? Or is it a more\nkosher approach that does not play with the internals to make it\nunlikely that it would have to change again in the future?\n\nYour \"use this opportunity to actually clean up\" above suggests that\nthe answer is the latter, but if you took my \"summary of my\nunderstanding\", it is likely that that fact is not captured in the\nresulting log message.\n\nThe interdiff obviously looks good. Let's move this series forward.\nI'll see if it can be merged down to 'maint', too, but it probably\nwould not matter that much.\n\nThanks.\n\n\n"},{"id":"308253","messageId":"alpine.DEB.2.20.1612221852320.155951@virtualbox","threadId":"44748","inReplyTo":"xmqqd1gjhn0u.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 0/3] Really fix the isatty() problem on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-22T17:59:10Z","receivedAt":"2016-12-22T17:59:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 22 Dec 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > - unsquashed 2/3 which was improperly snuck in before,\n> \n> As Windows specific changes, I didn't notice these two were independent.\n\nYeah, sorry, I only realized today that I had snuck that one in when\nre-reviewing the changes.\n\n> > - fixed the confusing commit message by using Junio's suggested\n> > replacement\n> \n> Sorry, but I didn't mean to \"suggest replacement\". I was just\n> testing my understanding by attempt to rephrase the gist of it.\n\nI did like your phrasing, though.\n\n> There was one thing I still wasn't clear in my \"summary of my\n> understanding\". Is the \"replacement originally done for compiling\n> with VC++\" a solution that still peeks into MSVC runtime internals\n> but is usable with both old and more recent one? Or is it a more\n> kosher approach that does not play with the internals to make it\n> unlikely that it would have to change again in the future?\n\nOh, it is kosher. There is no more messing with internals.\n\n> Your \"use this opportunity to actually clean up\" above suggests that\n> the answer is the latter, but if you took my \"summary of my\n> understanding\", it is likely that that fact is not captured in the\n> resulting log message.\n\nRight... I tried to make that clear by saying that this change replaces\nthe hack.\n\n> The interdiff obviously looks good. Let's move this series forward.\n> I'll see if it can be merged down to 'maint', too, but it probably\n> would not matter that much.\n\nI will have to release a new Git for Windows version Real Soon Now [*1*],\nso I will have to take those patches kind of there (as Git for Windows\nwill be based on upstream/maint for a while now).\n\nMy thinking is that I will publish a prerelease either tonight or\ntomorrow, then go offline until next year, and then immediately publish\nGit for Windows v2.11.0(2) (or v2.11.1 if you publish a bugfix version in\nthe meantime).\n\nCiao,\nDscho\n\nFootnote *1*: There is a new cURL version with some security fixes,\nalthough I do not think they are super-critical, then there is the fix for\nthe double slashes in //server/share/directory, the fix for the empty\ncredentials and I should probably also try to update the MSYS2 runtime to\nthe newest version of Cygwin's runtime. Lots of stuff.\n"},{"id":"308254","messageId":"xmqq37hfhln5.fsf@gitster.mtv.corp.google.com","threadId":"44748","inReplyTo":"xmqqd1gjhn0u.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 0/3] Really fix the isatty() problem on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-22T18:19:26Z","receivedAt":"2016-12-22T18:19:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n> ...\n>> - fixed the confusing commit message by using Junio's suggested\n>> replacement\n>\n> Sorry, but I didn't mean to \"suggest replacement\". I was just\n> testing my understanding by attempt to rephrase the gist of it.\n> ...\n> Your \"use this opportunity to actually clean up\" above suggests that\n> the answer is the latter, but if you took my \"summary of my\n> understanding\", it is likely that that fact is not captured in the\n> resulting log message.\n\nSo using the original log message in v1 and what you wrote in the\nmessage I was responding to as references, let me try a real\n\"suggested\" one as penance.\n\nI need to ask one clarification on what you wrote before completing\nthat effort, though.\n\n>> And incidentally, replacing the previous hack with the clean, new\n>> solution, which specifies explicitly for the file descriptors 0, 1 and 2\n>> whether we detected an MSYS2 pseudo-tty, whether we detected a real\n>> Win32 Console, and whether we had to swap out a real Win32 Console for a\n>> pipe to allow child processes to inherit it.\n\nThis has subject but not verb. I parsed the above like so:\n\n Replacing the previous hack with the clean, new solution (which\n specifies explicitly for the file descriptors 0, 1 and 2\n - whether we detected an MSYS2 pseudo-tty, \n - whether we detected a real Win32 Console, and \n - whether we had to swap out a real Win32 Console for a\n pipe to allow child processes to inherit it\n )\n\nSo the entire thing is a noun phrase \"replacing with a new patch\",\nand I take that as the subject of an unfinished sentence. What did\nthat subject do? Replacing with a new patch allows us to do \"this\nwonderful thing\", but what \"this wonderful thing\" is not clear.\n\nSubject: mingw: replace isatty() hack\nFrom: Jeff Hostetler \n\nFor over a year, Git for Windows has carried a patch that detects\nthe MSYS2 pseudo ttys used by Git for Windows' default Git Bash\n(i.e. a terminal that is not backed by a Win32 Console), but it did\nso by accessing internals of a previous MSVC runtime that is no\nlonger valid in newer versions. \n\nClean up this mess by backporting a patch that was originally done\nto compile Git with a recent VC++. Replacing the previous hack with\nthe clean, new solution, which specifies explicitly for the file\ndescriptors 0, 1 and 2 whether we detected an MSYS2 pseudo-tty,\nwhether we detected a real Win32 Console, and whether we had to swap\nout a real Win32 Console for a pipe to allow child processes to\ninherit it, lets us do XXXXX.\n\nAs a side effect (which was the reason for the back-port), this patch\nalso fixes the previous misguided attempt to intercept isatty() so that\nit handles character devices (such as /dev/null) as Git expects it.\n"},{"id":"308255","messageId":"xmqqy3z7g6hb.fsf@gitster.mtv.corp.google.com","threadId":"44748","inReplyTo":"alpine.DEB.2.20.1612221852320.155951@virtualbox","subject":"Re: [PATCH v2 0/3] Really fix the isatty() problem on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-22T18:32:16Z","receivedAt":"2016-12-22T18:32:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Sorry, but I didn't mean to \"suggest replacement\". I was just\n>> testing my understanding by attempt to rephrase the gist of it.\n>\n> I did like your phrasing, though.\n\nOK, then let's use these three patches as-is. I just made sure that\nthey can go to maintenance track for 2.11.x; so all should be good.\n\nThanks.\n"},{"id":"308256","messageId":"ecc920bc-5323-a85c-c29f-9a3a7e4450e8@kdbg.org","threadId":"44748","inReplyTo":"b0541907-ee79-207b-dc0f-1e3e7d761950@kdbg.org","subject":"Re: [PATCH 0/2] Really fix the isatty() problem on Windows","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-12-22T18:48:33Z","receivedAt":"2016-12-22T18:48:43Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 21.12.2016 um 22:15 schrieb Johannes Sixt:\n> Am 21.12.2016 um 18:53 schrieb Johannes Schindelin:\n>> The current patch series is based on `pu`, as that already has the\n>> winansi_get_osfhandle() fix. For ease of testing, I also have a branch\n>> based on master which you can pull via\n>>\n>> git pull https://github.com/dscho/git mingw-isatty-fixup-master\n>\n> Will test and report back tomorrow.\n\nThis version 1 of the series passes the test suite (next + a handful \nother topics) for me. It has also undergone a bit of field testing, and \nthings look fine.\n\nI haven't looked at the resulting code, yet, but I don't expect to find \nanything fishy.\n\n-- Hannes\n\n"},{"id":"308262","messageId":"e9f8a015-4106-fa88-082a-9e8c06ff61f3@kdbg.org","threadId":"44748","inReplyTo":"5e3c505a206a735e6ba0bfaf4c73965e95a928eb.1482426497.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 3/3] mingw: replace isatty() hack","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-12-22T20:26:30Z","receivedAt":"2016-12-22T20:26:54Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"I've only one request for clarification below. Otherwise, the patch \nlooks good.\n\n(lines removed by the patch trimmed)\n\nAm 22.12.2016 um 18:09 schrieb Johannes Schindelin:\n> +static HANDLE swap_osfhnd(int fd, HANDLE new_handle)\n> +{\n> +\t/*\n> +\t * Create a copy of the original handle associated with fd\n> +\t * because the original will get closed when we dup2().\n> +\t */\n> +\tHANDLE handle = (HANDLE)_get_osfhandle(fd);\n> +\tHANDLE duplicate = duplicate_handle(handle);\n>\n> +\t/* Create a temp fd associated with the already open \"new_handle\". */\n> +\tint new_fd = _open_osfhandle((intptr_t)new_handle, O_BINARY);\n>\n> +\tassert((fd == 1) || (fd == 2));\n>\n> +\t/*\n> +\t * Use stock dup2() to re-bind fd to the new handle. Note that\n> +\t * this will implicitly close(1) and close both fd=1 and the\n> +\t * originally associated handle. It will open a new fd=1 and\n> +\t * call DuplicateHandle() on the handle associated with new_fd.\n> +\t * It is because of this implicit close() that we created the\n> +\t * copy of the original.\n> +\t *\n> +\t * Note that the OS can recycle HANDLE (numbers) just like it\n> +\t * recycles fd (numbers), so we must update the cached value\n> +\t * of \"console\". You can use GetFileType() to see that\n> +\t * handle and _get_osfhandle(fd) may have the same number\n> +\t * value, but they refer to different actual files now.\n\nCertainly, the OS does not recycle handle values that are in use (open). \nThen I do not quite get the point of this paragraph. See...\n\n> +\t *\n> +\t * Note that dup2() when given target := {0,1,2} will also\n> +\t * call SetStdHandle(), so we don't need to worry about that.\n> +\t */\n> +\tdup2(new_fd, fd);\n> +\tif (console == handle)\n> +\t\tconsole = duplicate;\n\n... This is where \"the cached value of console is updated\", right? If \nconsole == handle, then this is not because a handle value was recycled, \nbut because fd *was* console. Since the old value of console has been \nclosed by the dup2(), we must refer to the back-up value in the future. \nAm I missing something?\n\n> +\thandle = INVALID_HANDLE_VALUE;\n>\n> +\t/* Close the temp fd. This explicitly closes \"new_handle\"\n> +\t * (because it has been associated with it).\n> +\t */\n> +\tclose(new_fd);\n>\n> +\tfd_is_interactive[fd] |= FD_SWAPPED;\n>\n> +\treturn duplicate;\n> }\n\n"},{"id":"308267","messageId":"alpine.DEB.2.20.1612222233400.155951@virtualbox","threadId":"44748","inReplyTo":"ecc920bc-5323-a85c-c29f-9a3a7e4450e8@kdbg.org","subject":"Re: [PATCH 0/2] Really fix the isatty() problem on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-22T21:33:52Z","receivedAt":"2016-12-22T21:34:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Thu, 22 Dec 2016, Johannes Sixt wrote:\n\n> Am 21.12.2016 um 22:15 schrieb Johannes Sixt:\n> > Am 21.12.2016 um 18:53 schrieb Johannes Schindelin:\n> > > The current patch series is based on `pu`, as that already has the\n> > > winansi_get_osfhandle() fix. For ease of testing, I also have a branch\n> > > based on master which you can pull via\n> > >\n> > > git pull https://github.com/dscho/git mingw-isatty-fixup-master\n> >\n> > Will test and report back tomorrow.\n> \n> This version 1 of the series passes the test suite (next + a handful other\n> topics) for me. It has also undergone a bit of field testing, and things look\n> fine.\n> \n> I haven't looked at the resulting code, yet, but I don't expect to find\n> anything fishy.\n\nThanks for confirming!\nDscho\n"},{"id":"308268","messageId":"alpine.DEB.2.20.1612222235200.155951@virtualbox","threadId":"44748","inReplyTo":"e9f8a015-4106-fa88-082a-9e8c06ff61f3@kdbg.org","subject":"Re: [PATCH v2 3/3] mingw: replace isatty() hack","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-22T21:37:20Z","receivedAt":"2016-12-22T21:37:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Thu, 22 Dec 2016, Johannes Sixt wrote:\n\n> Am 22.12.2016 um 18:09 schrieb Johannes Schindelin:\n> > +static HANDLE swap_osfhnd(int fd, HANDLE new_handle)\n> > +{\n> > +\t/*\n> > +\t * Create a copy of the original handle associated with fd\n> > +\t * because the original will get closed when we dup2().\n> > +\t */\n> > +\tHANDLE handle = (HANDLE)_get_osfhandle(fd);\n> > +\tHANDLE duplicate = duplicate_handle(handle);\n> >\n> > +\t/* Create a temp fd associated with the already open \"new_handle\". */\n> > +\tint new_fd = _open_osfhandle((intptr_t)new_handle, O_BINARY);\n> >\n> > +\tassert((fd == 1) || (fd == 2));\n> >\n> > +\t/*\n> > +\t * Use stock dup2() to re-bind fd to the new handle. Note that\n> > +\t * this will implicitly close(1) and close both fd=1 and the\n> > +\t * originally associated handle. It will open a new fd=1 and\n> > +\t * call DuplicateHandle() on the handle associated with new_fd.\n> > +\t * It is because of this implicit close() that we created the\n> > +\t * copy of the original.\n> > +\t *\n> > +\t * Note that the OS can recycle HANDLE (numbers) just like it\n> > +\t * recycles fd (numbers), so we must update the cached value\n> > +\t * of \"console\". You can use GetFileType() to see that\n> > +\t * handle and _get_osfhandle(fd) may have the same number\n> > +\t * value, but they refer to different actual files now.\n> \n> Certainly, the OS does not recycle handle values that are in use (open). Then\n> I do not quite get the point of this paragraph. See...\n> \n> > +\t *\n> > +\t * Note that dup2() when given target := {0,1,2} will also\n> > +\t * call SetStdHandle(), so we don't need to worry about that.\n> > +\t */\n> > +\tdup2(new_fd, fd);\n> > +\tif (console == handle)\n> > +\t\tconsole = duplicate;\n> \n> ... This is where \"the cached value of console is updated\", right? If console\n> == handle, then this is not because a handle value was recycled, but because\n> fd *was* console. Since the old value of console has been closed by the\n> dup2(), we must refer to the back-up value in the future. Am I missing\n> something?\n\nYou are correct, we must update `console` because `handle` is no longer\nthe handle we want.\n\nThe comment above only meant to reinforce that we have to forget about the\nprevious handle, too, as we might access something completely different\nthan a console otherwise.\n\nWould you have a suggestion how to rephrase the comment to make it less\nconfusing?\n\nCiao,\nDscho\n"},{"id":"308277","messageId":"04859cf9-e67a-28ab-ccb3-249687e696c8@kdbg.org","threadId":"44748","inReplyTo":"alpine.DEB.2.20.1612222235200.155951@virtualbox","subject":"Re: [PATCH v2 3/3] mingw: replace isatty() hack","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-12-22T22:28:29Z","receivedAt":"2016-12-22T22:28:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 22.12.2016 um 22:37 schrieb Johannes Schindelin:\n> Hi Hannes,\n>\n> On Thu, 22 Dec 2016, Johannes Sixt wrote:\n>\n>> Am 22.12.2016 um 18:09 schrieb Johannes Schindelin:\n>>> +static HANDLE swap_osfhnd(int fd, HANDLE new_handle)\n>>> +{\n>>> +\t/*\n>>> +\t * Create a copy of the original handle associated with fd\n>>> +\t * because the original will get closed when we dup2().\n>>> +\t */\n>>> +\tHANDLE handle = (HANDLE)_get_osfhandle(fd);\n>>> +\tHANDLE duplicate = duplicate_handle(handle);\n>>>\n>>> +\t/* Create a temp fd associated with the already open \"new_handle\". */\n>>> +\tint new_fd = _open_osfhandle((intptr_t)new_handle, O_BINARY);\n>>>\n>>> +\tassert((fd == 1) || (fd == 2));\n>>>\n>>> +\t/*\n>>> +\t * Use stock dup2() to re-bind fd to the new handle. Note that\n>>> +\t * this will implicitly close(1) and close both fd=1 and the\n>>> +\t * originally associated handle. It will open a new fd=1 and\n>>> +\t * call DuplicateHandle() on the handle associated with new_fd.\n>>> +\t * It is because of this implicit close() that we created the\n>>> +\t * copy of the original.\n>>> +\t *\n>>> +\t * Note that the OS can recycle HANDLE (numbers) just like it\n>>> +\t * recycles fd (numbers), so we must update the cached value\n>>> +\t * of \"console\". You can use GetFileType() to see that\n>>> +\t * handle and _get_osfhandle(fd) may have the same number\n>>> +\t * value, but they refer to different actual files now.\n>>\n>> Certainly, the OS does not recycle handle values that are in use (open). Then\n>> I do not quite get the point of this paragraph. See...\n>>\n>>> +\t *\n>>> +\t * Note that dup2() when given target := {0,1,2} will also\n>>> +\t * call SetStdHandle(), so we don't need to worry about that.\n>>> +\t */\n>>> +\tdup2(new_fd, fd);\n>>> +\tif (console == handle)\n>>> +\t\tconsole = duplicate;\n>>\n>> ... This is where \"the cached value of console is updated\", right? If console\n>> == handle, then this is not because a handle value was recycled, but because\n>> fd *was* console. Since the old value of console has been closed by the\n>> dup2(), we must refer to the back-up value in the future. Am I missing\n>> something?\n>\n> You are correct, we must update `console` because `handle` is no longer\n> the handle we want.\n>\n> The comment above only meant to reinforce that we have to forget about the\n> previous handle, too, as we might access something completely different\n> than a console otherwise.\n\nIt is like accessing a pointer after free().\n\nWhen I read the paragraph for the first time, I expected some \ninformation to be saved, then some handles to be closed and re-opened, \nwhich would possibly recycle a handle value, and then same state to be \nresurrected depending on the information saved earlier.\n\nBut nothing of this kind happens, only:\n\n\tdup2(new_fd, fd);\n\tif (console == handle)\n\t\tconsole = duplicate;\n\nwhich is necessary and, once one has understood the context, obvious.\n\n> Would you have a suggestion how to rephrase the comment to make it less\n> confusing?\n\nPerhaps\n\n\t * This might close the cached console handle.\n\t * We must cache the live duplicate instead.\n\nThen update the cache before the dup2, because at this time all 3 of \nconsole, handle, and duplicate are live and cannot be recycled:\n\n\tif (console == handle)\n\t\tconsole = duplicate;\n\tdup2(new_fd, fd);\n\n-- Hannes\n\n"},{"id":"308282","messageId":"fc3e0d9c-86ea-4a62-6b70-b9cdd67f581a@drbeat.li","threadId":"44748","inReplyTo":"ca4c61c603247c8ad0b876b068f6cd41fbe01667.1482426497.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 1/3] mingw: adjust is_console() to work with stdin","fromName":"Beat Bolli","fromEmail":"dev+git@drbeat.li","sentAt":"2016-12-22T23:04:18Z","receivedAt":"2016-12-22T23:04:29Z","isPatch":true,"sender":{"key":"dev+git@drbeat.li","avatar":"https://avatars.githubusercontent.com/u/21444?v=4"},"body":"On 22.12.16 18:08, Johannes Schindelin wrote:\n> When determining whether a handle corresponds to a *real* Win32 Console\n> (as opposed to, say, a character device such as /dev/null), we use the\n> GetConsoleOutputBufferInfo() function as a tell-tale.\n> \n> However, that does not work for *input* handles associated with a\n> console. Let's just use the GetConsoleMode() function for input handles,\n> and since it does not work on output handles fall back to the previous\n> method for those.\n> \n> This patch prepares for using is_console() instead of my previous\n> misguided attempt in cbb3f3c9b1 (mingw: intercept isatty() to handle\n> /dev/null as Git expects it, 2016-12-11) that broke everything on\n> Windows.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> compat/winansi.c | 6 +++++-\n> 1 file changed, 5 insertions(+), 1 deletion(-)\n> \n> diff --git a/compat/winansi.c b/compat/winansi.c\n> index cb725fb02f..590d61cb1b 100644\n> --- a/compat/winansi.c\n> +++ b/compat/winansi.c\n> @@ -84,6 +84,7 @@ static void warn_if_raster_font(void)\n> static int is_console(int fd)\n> {\n> \tCONSOLE_SCREEN_BUFFER_INFO sbi;\n> +\tDWORD mode;\n\nNit: can we move this definition into the block below where it's used?\n\n> \tHANDLE hcon;\n> \n> \tstatic int initialized = 0;\n> @@ -98,7 +99,10 @@ static int is_console(int fd)\n> \t\treturn 0;\n> \n> \t/* check if its a handle to a console output screen buffer */\n> -\tif (!GetConsoleScreenBufferInfo(hcon, &sbi))\n> +\tif (!fd) {\n\nRight here:\n+ DWORD mode;\n\n> +\t\tif (!GetConsoleMode(hcon, &mode))\n> +\t\t\treturn 0;\n> +\t} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))\n> \t\treturn 0;\n> \n> \t/* initialize attributes */\n> \n"},{"id":"308285","messageId":"cover.1482448531.git.johannes.schindelin@gmx.de","threadId":"44748","inReplyTo":"cover.1482426497.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 0/3] Really fix the isatty() problem on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-22T23:16:03Z","receivedAt":"2016-12-22T23:16:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"My previous fix may have fixed running the new\nt/t6030-bisect-porcelain.sh script that tested the new bisect--helper,\nwhich in turn used isatty() to determine whether it was running\ninteractively and was fooled by being redirected to /dev/null.\n\nBut it not only broke paging when running in a CMD window, due to\ntesting in the wrong worktree I also missed that it broke paging in Git\nfor Windows 2.x' Git Bash (i.e. a MinTTY terminal emulator).\n\nLet's use this opportunity to actually clean up the entire isatty() mess\nonce and for all, as part of the problem was introduced by a clever hack\nthat messes with internals of the Microsoft C runtime, and which changed\nrecently, so it was not such a clever hack to begin with.\n\nHappily, one of my colleagues had to address that latter problem\nrecently when he was tasked to make Git compile with Microsoft Visual C\n(the rationale: debugging facilities of Visual Studio are really\noutstanding, try them if you get a chance).\n\nAnd incidentally, replacing the previous hack with the clean, new\nsolution, which specifies explicitly for the file descriptors 0, 1 and 2\nwhether we detected an MSYS2 pseudo-tty, whether we detected a real\nWin32 Console, and whether we had to swap out a real Win32 Console for a\npipe to allow child processes to inherit it.\n\nWhile at it (or, actually, more like: as I already made this part of v1\nby mistake), upstream the patch carried in Git for Windows that supports\ncolor when running Git for Windows in Cygwin terminals.\n\nChanges since v2:\n\n- reworded the comment about \"recycling handles\"\n\n- moved the reassignment of the `console` variable before the dup2()\n call so that it is valid at all times\n\n- removed the \"handle = INVALID_HANDLE_VALUE\" assignment, as the local\n variable `handle` is not used afterwards anyway\n\n- generated the patches with --patience in the hope to improve\n readability\n\n\nAlan Davies (1):\n mingw: fix colourization on Cygwin pseudo terminals\n\nJeff Hostetler (1):\n mingw: replace isatty() hack\n\nJohannes Schindelin (1):\n mingw: adjust is_console() to work with stdin\n\n compat/winansi.c | 195 +++++++++++++++++++++++--------------------------------\n 1 file changed, 81 insertions(+), 114 deletions(-)\n\n\nbase-commit: 1d1bdafd64266e5ee3bd46c6965228f32e4022ea\nPublished-As: https://github.com/dscho/git/releases/tag/mingw-isatty-fixup-v3\nFetch-It-Via: git fetch https://github.com/dscho/git mingw-isatty-fixup-v3\n\nInterdiff vs v2:\n\n diff --git a/compat/winansi.c b/compat/winansi.c\n index 477209fce7..56658ec4f8 100644\n --- a/compat/winansi.c\n +++ b/compat/winansi.c\n @@ -494,19 +494,16 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)\n \t * It is because of this implicit close() that we created the\n \t * copy of the original.\n \t *\n -\t * Note that the OS can recycle HANDLE (numbers) just like it\n -\t * recycles fd (numbers), so we must update the cached value\n -\t * of \"console\". You can use GetFileType() to see that\n -\t * handle and _get_osfhandle(fd) may have the same number\n -\t * value, but they refer to different actual files now.\n +\t * Note that we need to update the cached console handle to the\n +\t * duplicated one because the dup2() call will implicitly close\n +\t * the original one.\n \t *\n \t * Note that dup2() when given target := {0,1,2} will also\n \t * call SetStdHandle(), so we don't need to worry about that.\n \t */\n -\tdup2(new_fd, fd);\n \tif (console == handle)\n \t\tconsole = duplicate;\n -\thandle = INVALID_HANDLE_VALUE;\n +\tdup2(new_fd, fd);\n \n \t/* Close the temp fd. This explicitly closes \"new_handle\"\n \t * (because it has been associated with it).\n\n-- \n2.11.0.rc3.windows.1\n\n"},{"id":"308286","messageId":"ca4c61c603247c8ad0b876b068f6cd41fbe01667.1482448531.git.johannes.schindelin@gmx.de","threadId":"44748","inReplyTo":"cover.1482448531.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 1/3] mingw: adjust is_console() to work with stdin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-22T23:16:16Z","receivedAt":"2016-12-22T23:16:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When determining whether a handle corresponds to a *real* Win32 Console\n(as opposed to, say, a character device such as /dev/null), we use the\nGetConsoleOutputBufferInfo() function as a tell-tale.\n\nHowever, that does not work for *input* handles associated with a\nconsole. Let's just use the GetConsoleMode() function for input handles,\nand since it does not work on output handles fall back to the previous\nmethod for those.\n\nThis patch prepares for using is_console() instead of my previous\nmisguided attempt in cbb3f3c9b1 (mingw: intercept isatty() to handle\n/dev/null as Git expects it, 2016-12-11) that broke everything on\nWindows.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/winansi.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/winansi.c b/compat/winansi.c\nindex cb725fb02f..590d61cb1b 100644\n--- a/compat/winansi.c\n+++ b/compat/winansi.c\n@@ -84,6 +84,7 @@ static void warn_if_raster_font(void)\n static int is_console(int fd)\n {\n \tCONSOLE_SCREEN_BUFFER_INFO sbi;\n+\tDWORD mode;\n \tHANDLE hcon;\n \n \tstatic int initialized = 0;\n@@ -98,7 +99,10 @@ static int is_console(int fd)\n \t\treturn 0;\n \n \t/* check if its a handle to a console output screen buffer */\n-\tif (!GetConsoleScreenBufferInfo(hcon, &sbi))\n+\tif (!fd) {\n+\t\tif (!GetConsoleMode(hcon, &mode))\n+\t\t\treturn 0;\n+\t} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))\n \t\treturn 0;\n \n \t/* initialize attributes */\n-- \n2.11.0.rc3.windows.1\n\n\n"},{"id":"308287","messageId":"796e3c4b5bda1170bfbb690a73dab5442b9c77e4.1482448531.git.johannes.schindelin@gmx.de","threadId":"44748","inReplyTo":"cover.1482448531.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 2/3] mingw: fix colourization on Cygwin pseudo terminals","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-22T23:16:20Z","receivedAt":"2016-12-22T23:16:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Alan Davies \n\nGit only colours the output and uses pagination if isatty() returns 1.\nMSYS2 and Cygwin emulate pseudo terminals via named pipes, meaning that\nisatty() returns 0.\n\nf7f90e0f4f (mingw: make isatty() recognize MSYS2's pseudo terminals\n(/dev/pty*), 2016-04-27) fixed this for MSYS2 terminals, but not for\nCygwin.\n\nThe named pipes that Cygwin and MSYS2 use are very similar. MSYS2 PTY pipes\nare called 'msys-*-pty*' and Cygwin uses 'cygwin-*-pty*'. This commit\nmodifies the existing check to allow both MSYS2 and Cygwin PTY pipes to be\nidentified as TTYs.\n\nNote that pagination is still broken when running Git for Windows from\nwithin Cygwin, as MSYS2's less.exe is spawned (and does not like to\ninteract with Cygwin's PTY).\n\nThis partially fixes https://github.com/git-for-windows/git/issues/267\n\nSigned-off-by: Alan Davies \nSigned-off-by: Johannes Schindelin \n---\n compat/winansi.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/winansi.c b/compat/winansi.c\nindex 590d61cb1b..fa37695fca 100644\n--- a/compat/winansi.c\n+++ b/compat/winansi.c\n@@ -562,8 +562,12 @@ static void detect_msys_tty(int fd)\n \tname = nameinfo->Name.Buffer;\n \tname[nameinfo->Name.Length] = 0;\n \n-\t/* check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX') */\n-\tif (!wcsstr(name, L\"msys-\") || !wcsstr(name, L\"-pty\"))\n+\t/*\n+\t * Check if this could be a MSYS2 pty pipe ('msys-XXXX-ptyN-XX')\n+\t * or a cygwin pty pipe ('cygwin-XXXX-ptyN-XX')\n+\t */\n+\tif ((!wcsstr(name, L\"msys-\") && !wcsstr(name, L\"cygwin-\")) ||\n+\t\t\t!wcsstr(name, L\"-pty\"))\n \t\treturn;\n \n \t/* init ioinfo size if we haven't done so */\n-- \n2.11.0.rc3.windows.1\n\n\n"},{"id":"308288","messageId":"2ce6060b891f8313cc63a95a9cba9064b7f82eb8.1482448531.git.johannes.schindelin@gmx.de","threadId":"44748","inReplyTo":"cover.1482448531.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 3/3] mingw: replace isatty() hack","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-22T23:16:56Z","receivedAt":"2016-12-22T23:17:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Jeff Hostetler \n\nGit for Windows has carried a patch that depended on internals\nof MSVC runtime, but it does not work correctly with recent MSVC\nruntime. A replacement was written originally for compiling\nwith VC++. The patch in this message is a backport of that\nreplacement, and it also fixes the previous attempt to make\nisatty() tell that /dev/null is *not* an interactive terminal.\n\nSigned-off-by: Jeff Hostetler \nTested-by: Beat Bolli \nSigned-off-by: Johannes Schindelin \n---\n compat/winansi.c | 181 +++++++++++++++++++++----------------------------------\n 1 file changed, 70 insertions(+), 111 deletions(-)\n\ndiff --git a/compat/winansi.c b/compat/winansi.c\nindex fa37695fca..56658ec4f8 100644\n--- a/compat/winansi.c\n+++ b/compat/winansi.c\n@@ -6,9 +6,12 @@\n #include \"../git-compat-util.h\"\n #include \n #include \n+#include \"win32.h\"\n \n-/* In this file, we actually want to use Windows' own isatty(). */\n-#undef isatty\n+static int fd_is_interactive[3] = { 0, 0, 0 };\n+#define FD_CONSOLE 0x1\n+#define FD_SWAPPED 0x2\n+#define FD_MSYS 0x4\n \n /*\n ANSI codes used by git: m, K\n@@ -105,6 +108,9 @@ static int is_console(int fd)\n \t} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))\n \t\treturn 0;\n \n+\tif (fd >= 0 && fd <= 2)\n+\t\tfd_is_interactive[fd] |= FD_CONSOLE;\n+\n \t/* initialize attributes */\n \tif (!initialized) {\n \t\tconsole = hcon;\n@@ -466,76 +472,47 @@ static HANDLE duplicate_handle(HANDLE hnd)\n \treturn hresult;\n }\n \n-\n-/*\n- * Make MSVCRT's internal file descriptor control structure accessible\n- * so that we can tweak OS handles and flags directly (we need MSVCRT\n- * to treat our pipe handle as if it were a console).\n- *\n- * We assume that the ioinfo structure (exposed by MSVCRT.dll via\n- * __pioinfo) starts with the OS handle and the flags. The exact size\n- * varies between MSVCRT versions, so we try different sizes until\n- * toggling the FDEV bit of _pioinfo(1)->osflags is reflected in\n- * isatty(1).\n- */\n-typedef struct {\n-\tHANDLE osfhnd;\n-\tchar osflags;\n-} ioinfo;\n-\n-extern __declspec(dllimport) ioinfo *__pioinfo[];\n-\n-static size_t sizeof_ioinfo = 0;\n-\n-#define IOINFO_L2E 5\n-#define IOINFO_ARRAY_ELTS (1 << IOINFO_L2E)\n-\n-#define FPIPE 0x08\n-#define FDEV 0x40\n-\n-static inline ioinfo* _pioinfo(int fd)\n-{\n-\treturn (ioinfo*)((char*)__pioinfo[fd >> IOINFO_L2E] +\n-\t\t\t(fd & (IOINFO_ARRAY_ELTS - 1)) * sizeof_ioinfo);\n-}\n-\n-static int init_sizeof_ioinfo(void)\n-{\n-\tint istty, wastty;\n-\t/* don't init twice */\n-\tif (sizeof_ioinfo)\n-\t\treturn sizeof_ioinfo >= 256;\n-\n-\tsizeof_ioinfo = sizeof(ioinfo);\n-\twastty = isatty(1);\n-\twhile (sizeof_ioinfo < 256) {\n-\t\t/* toggle FDEV flag, check isatty, then toggle back */\n-\t\t_pioinfo(1)->osflags ^= FDEV;\n-\t\tistty = isatty(1);\n-\t\t_pioinfo(1)->osflags ^= FDEV;\n-\t\t/* return if we found the correct size */\n-\t\tif (istty != wastty)\n-\t\t\treturn 0;\n-\t\tsizeof_ioinfo += sizeof(void*);\n-\t}\n-\terror(\"Tweaking file descriptors doesn't work with this MSVCRT.dll\");\n-\treturn 1;\n-}\n-\n static HANDLE swap_osfhnd(int fd, HANDLE new_handle)\n {\n-\tioinfo *pioinfo;\n-\tHANDLE old_handle;\n-\n-\t/* init ioinfo size if we haven't done so */\n-\tif (init_sizeof_ioinfo())\n-\t\treturn INVALID_HANDLE_VALUE;\n-\n-\t/* get ioinfo pointer and change the handles */\n-\tpioinfo = _pioinfo(fd);\n-\told_handle = pioinfo->osfhnd;\n-\tpioinfo->osfhnd = new_handle;\n-\treturn old_handle;\n+\t/*\n+\t * Create a copy of the original handle associated with fd\n+\t * because the original will get closed when we dup2().\n+\t */\n+\tHANDLE handle = (HANDLE)_get_osfhandle(fd);\n+\tHANDLE duplicate = duplicate_handle(handle);\n+\n+\t/* Create a temp fd associated with the already open \"new_handle\". */\n+\tint new_fd = _open_osfhandle((intptr_t)new_handle, O_BINARY);\n+\n+\tassert((fd == 1) || (fd == 2));\n+\n+\t/*\n+\t * Use stock dup2() to re-bind fd to the new handle. Note that\n+\t * this will implicitly close(1) and close both fd=1 and the\n+\t * originally associated handle. It will open a new fd=1 and\n+\t * call DuplicateHandle() on the handle associated with new_fd.\n+\t * It is because of this implicit close() that we created the\n+\t * copy of the original.\n+\t *\n+\t * Note that we need to update the cached console handle to the\n+\t * duplicated one because the dup2() call will implicitly close\n+\t * the original one.\n+\t *\n+\t * Note that dup2() when given target := {0,1,2} will also\n+\t * call SetStdHandle(), so we don't need to worry about that.\n+\t */\n+\tif (console == handle)\n+\t\tconsole = duplicate;\n+\tdup2(new_fd, fd);\n+\n+\t/* Close the temp fd. This explicitly closes \"new_handle\"\n+\t * (because it has been associated with it).\n+\t */\n+\tclose(new_fd);\n+\n+\tfd_is_interactive[fd] |= FD_SWAPPED;\n+\n+\treturn duplicate;\n }\n \n #ifdef DETECT_MSYS_TTY\n@@ -570,45 +547,25 @@ static void detect_msys_tty(int fd)\n \t\t\t!wcsstr(name, L\"-pty\"))\n \t\treturn;\n \n-\t/* init ioinfo size if we haven't done so */\n-\tif (init_sizeof_ioinfo())\n-\t\treturn;\n-\n-\t/* set FDEV flag, reset FPIPE flag */\n-\t_pioinfo(fd)->osflags &= ~FPIPE;\n-\t_pioinfo(fd)->osflags |= FDEV;\n+\tfd_is_interactive[fd] |= FD_MSYS;\n }\n \n #endif\n \n+/*\n+ * Wrapper for isatty(). Most calls in the main git code\n+ * call isatty(1 or 2) to see if the instance is interactive\n+ * and should: be colored, show progress, paginate output.\n+ * We lie and give results for what the descriptor WAS at\n+ * startup (and ignore any pipe redirection we internally\n+ * do).\n+ */\n+#undef isatty\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+\tif (fd >= 0 && fd <= 2)\n+\t\treturn fd_is_interactive[fd] != 0;\n+\treturn isatty(fd);\n }\n \n void winansi_init(void)\n@@ -619,6 +576,10 @@ void winansi_init(void)\n \t/* check if either stdout or stderr is a console output screen buffer */\n \tcon1 = is_console(1);\n \tcon2 = is_console(2);\n+\n+\t/* Also compute console bit for fd 0 even though we don't need the result here. */\n+\tis_console(0);\n+\n \tif (!con1 && !con2) {\n #ifdef DETECT_MSYS_TTY\n \t\t/* check if stdin / stdout / stderr are MSYS2 pty pipes */\n@@ -662,12 +623,10 @@ void winansi_init(void)\n */\n HANDLE winansi_get_osfhandle(int fd)\n {\n-\tHANDLE hnd = (HANDLE) _get_osfhandle(fd);\n-\tif (isatty(fd) && GetFileType(hnd) == FILE_TYPE_PIPE) {\n-\t\tif (fd == 1 && hconsole1)\n-\t\t\treturn hconsole1;\n-\t\telse if (fd == 2 && hconsole2)\n-\t\t\treturn hconsole2;\n-\t}\n-\treturn hnd;\n+\tif (fd == 1 && (fd_is_interactive[1] & FD_SWAPPED))\n+\t\treturn hconsole1;\n+\tif (fd == 2 && (fd_is_interactive[2] & FD_SWAPPED))\n+\t\treturn hconsole2;\n+\n+\treturn (HANDLE)_get_osfhandle(fd);\n }\n-- \n2.11.0.rc3.windows.1\n"},{"id":"308289","messageId":"xmqqlgv7eeoe.fsf@gitster.mtv.corp.google.com","threadId":"44748","inReplyTo":"fc3e0d9c-86ea-4a62-6b70-b9cdd67f581a@drbeat.li","subject":"Re: [PATCH v2 1/3] mingw: adjust is_console() to work with stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-22T23:18:09Z","receivedAt":"2016-12-22T23:18:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Beat Bolli writes:\n\n>> @@ -84,6 +84,7 @@ static void warn_if_raster_font(void)\n>> static int is_console(int fd)\n>> {\n>> \tCONSOLE_SCREEN_BUFFER_INFO sbi;\n>> +\tDWORD mode;\n>\n> Nit: can we move this definition into the block below where it's used?\n>\n>> \tHANDLE hcon;\n>> \n>> \tstatic int initialized = 0;\n>> @@ -98,7 +99,10 @@ static int is_console(int fd)\n>> \t\treturn 0;\n>> \n>> \t/* check if its a handle to a console output screen buffer */\n>> -\tif (!GetConsoleScreenBufferInfo(hcon, &sbi))\n>> +\tif (!fd) {\n>\n> Right here:\n> + DWORD mode;\n>\n>> +\t\tif (!GetConsoleMode(hcon, &mode))\n>> +\t\t\treturn 0;\n>> +\t} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))\n>> \t\treturn 0;\n\nAs these patches have been already merged to 'next' a few hours ago\nas part of today's integration cycle, please make any further\nrefinement (if necessary) as incremental updates.\n\nThanks.\n"},{"id":"308290","messageId":"alpine.DEB.2.20.1612230017030.155951@virtualbox","threadId":"44748","inReplyTo":"04859cf9-e67a-28ab-ccb3-249687e696c8@kdbg.org","subject":"Re: [PATCH v2 3/3] mingw: replace isatty() hack","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-22T23:18:04Z","receivedAt":"2016-12-22T23:18:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Thu, 22 Dec 2016, Johannes Sixt wrote:\n\n> Am 22.12.2016 um 22:37 schrieb Johannes Schindelin:\n>\n> > Would you have a suggestion how to rephrase the comment to make it\n> > less confusing?\n> \n> Perhaps\n> \n> \t * This might close the cached console handle.\n> \t * We must cache the live duplicate instead.\n> \n> Then update the cache before the dup2, because at this time all 3 of console,\n> handle, and duplicate are live and cannot be recycled:\n> \n> \tif (console == handle)\n> \t\tconsole = duplicate;\n> \tdup2(new_fd, fd);\n\nGood point. I reworded the comment slightly differently (see the interdiff\nin v3 0/3), hope you don't mind.\n\nCiao,\nDscho\n"},{"id":"308297","messageId":"alpine.DEB.2.20.1612231024400.155951@virtualbox","threadId":"44748","inReplyTo":"fc3e0d9c-86ea-4a62-6b70-b9cdd67f581a@drbeat.li","subject":"Re: [PATCH v2 1/3] mingw: adjust is_console() to work with stdin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-23T09:30:33Z","receivedAt":"2016-12-23T09:31:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Beat,\n\nOn Fri, 23 Dec 2016, Beat Bolli wrote:\n\n> On 22.12.16 18:08, Johannes Schindelin wrote:\n> > diff --git a/compat/winansi.c b/compat/winansi.c\n> > index cb725fb02f..590d61cb1b 100644\n> > --- a/compat/winansi.c\n> > +++ b/compat/winansi.c\n> > @@ -84,6 +84,7 @@ static void warn_if_raster_font(void)\n> > static int is_console(int fd)\n> > {\n> > \tCONSOLE_SCREEN_BUFFER_INFO sbi;\n> > +\tDWORD mode;\n> \n> Nit: can we move this definition into the block below where it's used?\n> \n> > \tHANDLE hcon;\n> > \n> > \tstatic int initialized = 0;\n> > @@ -98,7 +99,10 @@ static int is_console(int fd)\n> > \t\treturn 0;\n> > \n> > \t/* check if its a handle to a console output screen buffer */\n> > -\tif (!GetConsoleScreenBufferInfo(hcon, &sbi))\n> > +\tif (!fd) {\n> \n> Right here:\n> + DWORD mode;\n\nBy that reasoning, the CONSOLE_SCREEN_BUFFER_INFO declaration that has\nfunction-wide scope should also move below:\n\n> > +\t\tif (!GetConsoleMode(hcon, &mode))\n> > +\t\t\treturn 0;\n\nRight here.\n\n> > +\t} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))\n> > \t\treturn 0;\n> > \n> > \t/* initialize attributes */\n\nAs the existing code followed a different convention, so does my patch.\n\nIf you choose to submit a change that moved the `mode` declaration to\nnarrow its scope, please also move the `sbi` declaration for consistency.\n\nCiao,\nDscho\n"},{"id":"308299","messageId":"f6964a29e4bdc5a68fff56900cf67c36@drbeat.li","threadId":"44748","inReplyTo":"alpine.DEB.2.20.1612231024400.155951@virtualbox","subject":"Re: [PATCH v2 1/3] mingw: adjust is_console() to work with stdin","fromName":"Beat Bolli","fromEmail":"dev+git@drbeat.li","sentAt":"2016-12-23T12:51:47Z","receivedAt":"2016-12-23T12:52:30Z","isPatch":true,"sender":{"key":"dev+git@drbeat.li","avatar":"https://avatars.githubusercontent.com/u/21444?v=4"},"body":"Hi Dscho\n\nOn 2016-12-23 10:30, Johannes Schindelin wrote:\n> Hi Beat,\n> \n> On Fri, 23 Dec 2016, Beat Bolli wrote:\n> \n>> On 22.12.16 18:08, Johannes Schindelin wrote:\n>> > diff --git a/compat/winansi.c b/compat/winansi.c\n>> > index cb725fb02f..590d61cb1b 100644\n>> > --- a/compat/winansi.c\n>> > +++ b/compat/winansi.c\n>> > @@ -84,6 +84,7 @@ static void warn_if_raster_font(void)\n>> > static int is_console(int fd)\n>> > {\n>> > \tCONSOLE_SCREEN_BUFFER_INFO sbi;\n>> > +\tDWORD mode;\n>> \n>> Nit: can we move this definition into the block below where it's used?\n>> \n>> > \tHANDLE hcon;\n>> >\n>> > \tstatic int initialized = 0;\n>> > @@ -98,7 +99,10 @@ static int is_console(int fd)\n>> > \t\treturn 0;\n>> >\n>> > \t/* check if its a handle to a console output screen buffer */\n>> > -\tif (!GetConsoleScreenBufferInfo(hcon, &sbi))\n>> > +\tif (!fd) {\n>> \n>> Right here:\n>> + DWORD mode;\n> \n> By that reasoning, the CONSOLE_SCREEN_BUFFER_INFO declaration that has\n> function-wide scope should also move below:\n> \n>> > +\t\tif (!GetConsoleMode(hcon, &mode))\n>> > +\t\t\treturn 0;\n> \n> Right here.\n> \n>> > +\t} else if (!GetConsoleScreenBufferInfo(hcon, &sbi))\n>> > \t\treturn 0;\n>> >\n>> > \t/* initialize attributes */\n> \n> As the existing code followed a different convention, so does my patch.\n> \n> If you choose to submit a change that moved the `mode` declaration to\n> narrow its scope, please also move the `sbi` declaration for \n> consistency.\n\nIt's probably not worth it. It just jumped at me when reading the patch, \nand, writing much C++ recently, it looked weird to have a definition so \nfar away from the single use of the variable.\n\nCheers,\nBeat\n"},{"id":"309618","messageId":"alpine.DEB.2.20.1701181304490.3469@virtualbox","threadId":"44748","inReplyTo":"2ce6060b891f8313cc63a95a9cba9064b7f82eb8.1482448531.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 3/3] mingw: replace isatty() hack","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-18T12:13:49Z","receivedAt":"2017-01-18T12:14:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 23 Dec 2016, Johannes Schindelin wrote:\n\n> +\t/*\n> +\t * Use stock dup2() to re-bind fd to the new handle. Note that\n> +\t * this will implicitly close(1) and close both fd=1 and the\n> +\t * originally associated handle. It will open a new fd=1 and\n> +\t * call DuplicateHandle() on the handle associated with new_fd.\n> +\t * It is because of this implicit close() that we created the\n> +\t * copy of the original.\n> +\t *\n> +\t * Note that we need to update the cached console handle to the\n> +\t * duplicated one because the dup2() call will implicitly close\n> +\t * the original one.\n> +\t *\n> +\t * Note that dup2() when given target := {0,1,2} will also\n> +\t * call SetStdHandle(), so we don't need to worry about that.\n> +\t */\n> +\tif (console == handle)\n> +\t\tconsole = duplicate;\n> +\tdup2(new_fd, fd);\n\nSadly, v2 was applied to `maint` instead of this revision, so Hannes'\nsuggestions that I addressed in v3 were dropped silently.\n\nI will send out a follow-up patch in a moment.\n\nCiao,\nJohannes\n"}]}