{"thread":{"id":"44910","subject":"[PATCH] mingw: follow-up to \"replace isatty() hack\"","startedAt":"2017-01-18T12:14:51Z","lastAt":"2017-01-18T21:38:31Z","messageCount":4,"participants":["Johannes Schindelin","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"309619","messageId":"867bafbe582df549b10729a5d688458bb6a98d51.1484741665.git.johannes.schindelin@gmx.de","threadId":"44910","inReplyTo":null,"subject":"[PATCH] mingw: follow-up to \"replace isatty() hack\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-18T12:14:35Z","receivedAt":"2017-01-18T12:14:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The version of the \"replace isatty() hack\" that got applied to the\n`maint` branch did not actually reflect the latest iteration of the\npatch series: v3 was sent out with these changes, as requested by\nthe reviewer Johannes Sixt:\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\nSigned-off-by: Johannes Schindelin \n---\nPublished-As: https://github.com/dscho/git/releases/tag/mingw-isatty-fixup-fixup-v1\nFetch-It-Via: git fetch https://github.com/dscho/git mingw-isatty-fixup-fixup-v1\n\n compat/winansi.c | 11 ++++-------\n 1 file changed, 4 insertions(+), 7 deletions(-)\n\ndiff --git a/compat/winansi.c b/compat/winansi.c\nindex 3c9ed3cfe0..82b89ab137 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\nbase-commit: 3313b78c145ba9212272b5318c111cde12bfef4a\n-- \n2.11.0.windows.3\n"},{"id":"309650","messageId":"xmqq7f5s9nvt.fsf@gitster.mtv.corp.google.com","threadId":"44910","inReplyTo":"867bafbe582df549b10729a5d688458bb6a98d51.1484741665.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] mingw: follow-up to \"replace isatty() hack\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-18T19:19:50Z","receivedAt":"2017-01-18T19:27:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> The version of the \"replace isatty() hack\" that got applied to the\n> `maint` branch did not actually reflect the latest iteration of the\n> patch series: v3 was sent out with these changes, as requested by\n> the reviewer Johannes Sixt:\n\nThanks for an update. \n\nDoes the above \"the version taken was not updated before getting\nmerged\" mistake only apply to 'maint', or is it also true for\n'master'? \n\nAs a rule we only merge things to 'maint' that have already been\nmerged to 'master', so I am guessing that the answer is yes, in\nwhich case I'd queue it on js/mingw-isatty and then merge it to\nnext, master and maint in that order as usual.\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\nAlso if the v3 had been reviewed and acked, it would be nice to have\nthe acked-by around here (which I can locally do). Hannes?\n\n> Signed-off-by: Johannes Schindelin \n> ---\n\nThanks.\n\n> Published-As: https://github.com/dscho/git/releases/tag/mingw-isatty-fixup-fixup-v1\n> Fetch-It-Via: git fetch https://github.com/dscho/git mingw-isatty-fixup-fixup-v1\n>\n> compat/winansi.c | 11 ++++-------\n> 1 file changed, 4 insertions(+), 7 deletions(-)\n>\n> diff --git a/compat/winansi.c b/compat/winansi.c\n> index 3c9ed3cfe0..82b89ab137 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> base-commit: 3313b78c145ba9212272b5318c111cde12bfef4a\n"},{"id":"309671","messageId":"c329c981-2b92-13b5-0561-d1d2e3fa2803@kdbg.org","threadId":"44910","inReplyTo":"xmqq7f5s9nvt.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] mingw: follow-up to \"replace isatty() hack\"","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-01-18T21:16:28Z","receivedAt":"2017-01-18T21:16:44Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 18.01.2017 um 20:19 schrieb Junio C Hamano:\n> Johannes Schindelin writes:\n>> compat/winansi.c | 11 ++++-------\n>> 1 file changed, 4 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/compat/winansi.c b/compat/winansi.c\n>> index 3c9ed3cfe0..82b89ab137 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\nLooks good and obviously correct (FLW). I can offer a\n\nReviewed-by: Johannes Sixt \n\nbut it will take a day or two until I can test the patch.\n\n-- Hannes\n\n"},{"id":"309677","messageId":"xmqq37gg3vjy.fsf@gitster.mtv.corp.google.com","threadId":"44910","inReplyTo":"c329c981-2b92-13b5-0561-d1d2e3fa2803@kdbg.org","subject":"Re: [PATCH] mingw: follow-up to \"replace isatty() hack\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-18T21:30:41Z","receivedAt":"2017-01-18T21:38:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\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>\n> Looks good and obviously correct (FLW). I can offer a\n>\n> Reviewed-by: Johannes Sixt \n>\n> but it will take a day or two until I can test the patch.\n\nI think a Reviewed-by is good enough, as the original\n\n<2ce6060b891f8313cc63a95a9cba9064b7f82eb8.1482448531.git.johannes.schindelin@gmx.de>\n\nalready has \"Tested-by\" to indicate that as a whole this have been\ntested. The \"follow-up\" we are commenting on was made out of that\noriginal to incrementally update the older version that was queued\nand merged to 'master' 3 weeks ago.\n\nThanks.\n\n\n"}]}