threads / patch / 44910

patchmingw: follow-up to "replace isatty() hack"

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

## tl;dr

4 messages between Jan 18, 2017 and Jan 18, 2017. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Johannes Schindelin· Jan 18, 2017, 12:14 UTC · lore

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:

- 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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/mingw-isatty-fixup-fixup-v1
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-isatty-fixup-fixup-v1

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

Show changes to compat/winansi.c +4 −7

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

base-commit: 3313b78c145ba9212272b5318c111cde12bfef4a

-- 
2.11.0.windows.3

Junio C Hamano· Jan 18, 2017, 19:19 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <johannes.schindelin@gmx.de> 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.

Show 8 quoted lines

> - 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 <johannes.schindelin@gmx.de>
> ---

Thanks.

Show 36 quoted lines

> Published-As: https://github.com/dscho/git/releases/tag/mingw-isatty-fixup-fixup-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git mingw-isatty-fixup-fixup-v1
>
>  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).
>
> base-commit: 3313b78c145ba9212272b5318c111cde12bfef4a

Johannes Sixt· Jan 18, 2017, 21:16 UTC · re: Junio C Hamano · lore

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

Am 18.01.2017 um 20:19 schrieb Junio C Hamano:

Show 33 quoted lines

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>  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 <j6t@kdbg.org>

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

-- Hannes

Junio C Hamano· Jan 18, 2017, 21:30 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j6t@kdbg.org> writes:

Show 15 quoted lines

>>> -	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 <j6t@kdbg.org>
>
> but it will take a day or two until I can test the patch.

I think a Reviewed-by is good enough, as the original

<2ce6060b891f8313cc63a95a9cba9064b7f82eb8.1482448531.git.johannes.schindelin@gmx.de>

already has "Tested-by" to indicate that as a whole this have been tested. The "follow-up" we are commenting on was made out of that original to incrementally update the older version that was queued and merged to 'master' 3 weeks ago.

Thanks.