* [PATCH] mingw: follow-up to "replace isatty() hack"
@ 2017-01-18 12:14 Johannes Schindelin
2017-01-18 19:19 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Johannes Schindelin @ 2017-01-18 12:14 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Pranit Bauva, Johannes Sixt, Beat Bolli
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(-)
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] mingw: follow-up to "replace isatty() hack"
2017-01-18 12:14 [PATCH] mingw: follow-up to "replace isatty() hack" Johannes Schindelin
@ 2017-01-18 19:19 ` Junio C Hamano
2017-01-18 21:16 ` Johannes Sixt
0 siblings, 1 reply; 4+ messages in thread
From: Junio C Hamano @ 2017-01-18 19:19 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Pranit Bauva, Johannes Sixt, Beat Bolli
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.
> - 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.
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mingw: follow-up to "replace isatty() hack"
2017-01-18 19:19 ` Junio C Hamano
@ 2017-01-18 21:16 ` Johannes Sixt
2017-01-18 21:30 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Johannes Sixt @ 2017-01-18 21:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git, Pranit Bauva, Beat Bolli
Am 18.01.2017 um 20:19 schrieb Junio C Hamano:
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mingw: follow-up to "replace isatty() hack"
2017-01-18 21:16 ` Johannes Sixt
@ 2017-01-18 21:30 ` Junio C Hamano
0 siblings, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2017-01-18 21:30 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, git, Pranit Bauva, Beat Bolli
Johannes Sixt <j6t@kdbg.org> writes:
>>> - 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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-01-18 21:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-18 12:14 [PATCH] mingw: follow-up to "replace isatty() hack" Johannes Schindelin
2017-01-18 19:19 ` Junio C Hamano
2017-01-18 21:16 ` Johannes Sixt
2017-01-18 21:30 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).