Re: [PATCH] mingw: follow-up to "replace isatty() hack"

Johannes Schindelin [off-list ref] writes:

The version of the "replace isatty() hack" that got applied to the
`maint` branch did not actually reflect the latest iteration of the
patch series: v3 was sent out with these changes, as requested by
the reviewer Johannes Sixt:

Thanks for an update.  

Does the above "the version taken was not updated before getting
merged" mistake only apply to 'maint', or is it also true for
'master'?  

As a rule we only merge things to 'maint' that have already been
merged to 'master', so I am guessing that the answer is yes, in
which case I'd queue it on js/mingw-isatty and then merge it to
next, master and maint in that order as usual.

- reworded the comment about "recycling handles"

- moved the reassignment of the `console` variable before the dup2()
  call so that it is valid at all times

- removed the "handle = INVALID_HANDLE_VALUE" assignment, as the local
  variable `handle` is not used afterwards anyway

Also if the v3 had been reviewed and acked, it would be nice to have
the acked-by around here (which I can locally do).  Hannes?

Signed-off-by: Johannes Schindelin <redacted>
---

Thanks.

Am 18.01.2017 um 20:19 schrieb Junio C Hamano:

Johannes Schindelin [off-list ref] writes:

quoted

 compat/winansi.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/compat/winansi.c b/compat/winansi.c
index 3c9ed3cfe0..82b89ab137 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c

@@ -494,19 +494,16 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
 	 * It is because of this implicit close() that we created the
 	 * copy of the original.
 	 *
-	 * Note that the OS can recycle HANDLE (numbers) just like it
-	 * recycles fd (numbers), so we must update the cached value
-	 * of "console".  You can use GetFileType() to see that
-	 * handle and _get_osfhandle(fd) may have the same number
-	 * value, but they refer to different actual files now.
+	 * Note that we need to update the cached console handle to the
+	 * duplicated one because the dup2() call will implicitly close
+	 * the original one.
 	 *
 	 * Note that dup2() when given target := {0,1,2} will also
 	 * call SetStdHandle(), so we don't need to worry about that.
 	 */
-	dup2(new_fd, fd);
 	if (console == handle)
 		console = duplicate;
-	handle = INVALID_HANDLE_VALUE;
+	dup2(new_fd, fd);

 	/* Close the temp fd.  This explicitly closes "new_handle"
 	 * (because it has been associated with it).

Looks good and obviously correct (FLW). I can offer a

Reviewed-by: Johannes Sixt <redacted>

but it will take a day or two until I can test the patch.

-- Hannes