{"thread":{"id":"63661","subject":"[PATCH] Ensure restore_term works correctly with DUPLEX","startedAt":"2025-06-17T18:56:14Z","lastAt":"2025-06-23T09:42:15Z","messageCount":7,"participants":["James Duley via GitGitGadget","Junio C Hamano","Carlo Marcelo Arenas Belón","James Duley","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"520328","messageId":"pull.2000.git.git.1750186571037.gitgitgadget@gmail.com","threadId":"63661","inReplyTo":null,"subject":"[PATCH] Ensure restore_term works correctly with DUPLEX","fromName":"James Duley via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-06-17T18:56:10Z","receivedAt":"2025-06-17T18:56:14Z","isPatch":true,"sender":{"key":"jagduley@gmail.com","avatar":null},"body":"From: James Duley \n\nPreviously, if save_term/restore_term was called with the DUPLEX flag\nand then without the flag, an assertion was hit.\n> Assertion failed: hconout != INVALID_HANDLE_VALUE,\n> file compat/terminal.c, line 283\n\nThis is because save_term doesn't set cmode_out when not DUPLEX,\nso an old version of cmode_out was being used.\nTherefore, hconout is the correct thing for restore to check\nto decide whether to restore stdout console mode.\n\nI saw this on Windows with interactive.singleKey when doing `git add -p`.\nSpecifically, after hitting `e` to edit in vim, once on to the prompt\nfor the next hunk, pressing any key results in the assertion.\n\nSigned-off-by: James Duley \n---\n Ensure restore_term works correctly with DUPLEX\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2000%2Fparched%2Frestore-term-windows-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2000/parched/restore-term-windows-fix-v1\nPull-Request: https://github.com/git/git/pull/2000\n\n compat/terminal.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 584f27bf7e1..72b184555ff 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -279,8 +279,7 @@ void restore_term(void)\n \n \tSetConsoleMode(hconin, cmode_in);\n \tCloseHandle(hconin);\n-\tif (cmode_out) {\n-\t\tassert(hconout != INVALID_HANDLE_VALUE);\n+\tif (hconout != INVALID_HANDLE_VALUE) {\n \t\tSetConsoleMode(hconout, cmode_out);\n \t\tCloseHandle(hconout);\n \t}\n\nbase-commit: 16bd9f20a403117f2e0d9bcda6c6e621d3763e77\n-- \ngitgitgadget\n"},{"id":"520330","messageId":"xmqqh60euqhc.fsf@gitster.g","threadId":"63661","inReplyTo":"pull.2000.git.git.1750186571037.gitgitgadget@gmail.com","subject":"Re: [PATCH] Ensure restore_term works correctly with DUPLEX","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-17T19:18:07Z","receivedAt":"2025-06-17T19:18:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"James Duley via GitGitGadget\" writes:\n\n> From: James Duley \n>\n> Previously, if save_term/restore_term was called with the DUPLEX flag\n> and then without the flag, an assertion was hit.\n>> Assertion failed: hconout != INVALID_HANDLE_VALUE,\n>> file compat/terminal.c, line 283\n>\n> This is because save_term doesn't set cmode_out when not DUPLEX,\n> so an old version of cmode_out was being used.\n> Therefore, hconout is the correct thing for restore to check\n> to decide whether to restore stdout console mode.\n>\n> I saw this on Windows with interactive.singleKey when doing `git add -p`.\n> Specifically, after hitting `e` to edit in vim, once on to the prompt\n> for the next hunk, pressing any key results in the assertion.\n>\n> Signed-off-by: James Duley \n> ---\n> Ensure restore_term works correctly with DUPLEX\n\nRan \"git log -L247,290:compat/terminal.c\" to find commits that\ntouched this Windows specific area of the code to see who might be\ncapable of reviewing this change, as I am certainly not one of them.\n\ne22b245e (terminal: teach git how to save/restore its terminal\nsettings, 2021-10-05) is where the assert being removed came from,\nit seems.\n\nThe author of that commit should be CC'ed if we want to ask for a\nquicker review. I'll also add Git-for-Windows maintainer as well.\n\nThanks.\n\n\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2000%2Fparched%2Frestore-term-windows-fix-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2000/parched/restore-term-windows-fix-v1\n> Pull-Request: https://github.com/git/git/pull/2000\n>\n> compat/terminal.c | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n>\n> diff --git a/compat/terminal.c b/compat/terminal.c\n> index 584f27bf7e1..72b184555ff 100644\n> --- a/compat/terminal.c\n> +++ b/compat/terminal.c\n> @@ -279,8 +279,7 @@ void restore_term(void)\n> \n> \tSetConsoleMode(hconin, cmode_in);\n> \tCloseHandle(hconin);\n> -\tif (cmode_out) {\n> -\t\tassert(hconout != INVALID_HANDLE_VALUE);\n> +\tif (hconout != INVALID_HANDLE_VALUE) {\n> \t\tSetConsoleMode(hconout, cmode_out);\n> \t\tCloseHandle(hconout);\n> \t}\n>\n> base-commit: 16bd9f20a403117f2e0d9bcda6c6e621d3763e77\n"},{"id":"520350","messageId":"zpv3nkpsjqlg7evklw4ui5q7wonak2376rclu5ksmumdvk3tic@tfxmcpjmj3nw","threadId":"63661","inReplyTo":"xmqqh60euqhc.fsf@gitster.g","subject":"Re: [PATCH] Ensure restore_term works correctly with DUPLEX","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-06-18T10:07:11Z","receivedAt":"2025-06-18T10:07:14Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Jun 17, 2025 at 12:18:07PM -0800, Junio C Hamano wrote:\n> \"James Duley via GitGitGadget\" writes:\n> \n> > This is because save_term doesn't set cmode_out when not DUPLEX,\n> > so an old version of cmode_out was being used.\n\nTo fully address that bug though, something like the following\n(untested) needs to be squashed on top, right?:\n\n----\ndiff --git a/compat/terminal.c b/compat/terminal.c\nindex 72b184555f..8a197ffea1 100644\n--- a/compat/terminal.c\n+++ b/compat/terminal.c\n@@ -279,7 +279,7 @@ void restore_term(void)\n \n \tSetConsoleMode(hconin, cmode_in);\n \tCloseHandle(hconin);\n-\tif (hconout != INVALID_HANDLE_VALUE) {\n+\tif (cmode_out && hconout != INVALID_HANDLE_VALUE) {\n \t\tSetConsoleMode(hconout, cmode_out);\n \t\tCloseHandle(hconout);\n \t}\n@@ -299,11 +299,15 @@ int save_term(enum save_term_flags flags)\n \t\thconout = CreateFileA(\"CONOUT$\", GENERIC_READ | GENERIC_WRITE,\n \t\t\tFILE_SHARE_WRITE, NULL, OPEN_EXISTING,\n \t\t\tFILE_ATTRIBUTE_NORMAL, NULL);\n-\t\tif (hconout == INVALID_HANDLE_VALUE)\n+\t\tif (hconout == INVALID_HANDLE_VALUE) {\n+\t\t\tcmode_out = 0;\n \t\t\tgoto error;\n+\t\t}\n \n \t\tGetConsoleMode(hconout, &cmode_out);\n \t}\n+\telse\n+\t\tcmode_out = 0;\n \n \tGetConsoleMode(hconin, &cmode_in);\n \tuse_stty = 0;\n\nIt would be nice to know, if the problem with vi that this was meant to\naddress, and that needs further changes, that are only in the git for\nwindows fork is stll relevant, so this could be cleaned further.\n\nCarlo\n"},{"id":"520367","messageId":"CANZAkJuPUYSCd4yGHFhs6hfERovzjwWoM2gU2MyAHvY_AtjLkg@mail.gmail.com","threadId":"63661","inReplyTo":"zpv3nkpsjqlg7evklw4ui5q7wonak2376rclu5ksmumdvk3tic@tfxmcpjmj3nw","subject":"Re: [PATCH] Ensure restore_term works correctly with DUPLEX","fromName":"James Duley","fromEmail":"jagduley@gmail.com","sentAt":"2025-06-18T20:38:29Z","receivedAt":"2025-06-18T20:38:40Z","isPatch":true,"sender":{"key":"jagduley@gmail.com","avatar":null},"body":"On Wed, 18 Jun 2025 at 22:07, Carlo Marcelo Arenas Belón\n wrote:\n>\n> On Tue, Jun 17, 2025 at 12:18:07PM -0800, Junio C Hamano wrote:\n> > \"James Duley via GitGitGadget\" writes:\n> >\n> > > This is because save_term doesn't set cmode_out when not DUPLEX,\n> > > so an old version of cmode_out was being used.\n>\n> To fully address that bug though, something like the following\n> (untested) needs to be squashed on top, right?:\n>\n> ----\n> diff --git a/compat/terminal.c b/compat/terminal.c\n> index 72b184555f..8a197ffea1 100644\n> --- a/compat/terminal.c\n> +++ b/compat/terminal.c\n> @@ -279,7 +279,7 @@ void restore_term(void)\n>\n> SetConsoleMode(hconin, cmode_in);\n> CloseHandle(hconin);\n> - if (hconout != INVALID_HANDLE_VALUE) {\n> + if (cmode_out && hconout != INVALID_HANDLE_VALUE) {\n> SetConsoleMode(hconout, cmode_out);\n> CloseHandle(hconout);\n> }\n> @@ -299,11 +299,15 @@ int save_term(enum save_term_flags flags)\n> hconout = CreateFileA(\"CONOUT$\", GENERIC_READ | GENERIC_WRITE,\n> FILE_SHARE_WRITE, NULL, OPEN_EXISTING,\n> FILE_ATTRIBUTE_NORMAL, NULL);\n> - if (hconout == INVALID_HANDLE_VALUE)\n> + if (hconout == INVALID_HANDLE_VALUE) {\n> + cmode_out = 0;\n> goto error;\n> + }\n>\n> GetConsoleMode(hconout, &cmode_out);\n> }\n> + else\n> + cmode_out = 0;\n>\n> GetConsoleMode(hconin, &cmode_in);\n> use_stty = 0;\n>\n> It would be nice to know, if the problem with vi that this was meant to\n> address, and that needs further changes, that are only in the git for\n> windows fork is stll relevant, so this could be cleaned further.\n>\n> Carlo\n\nI thought about something like that, but I figured:\n* restore_term is only called if save_term is successful\n* hconout is always invalid before save_term is called\n* 0 might be a valid cmode_out that should be restored\n\nThis is my first patch so I didn't realize git-for-windows had a\nseparate fork. That makes sense now because I couldn't find where\nsave_term was called from in this repo. To test this works I had\ndownloaded the artifacts from\nhttps://github.com/git/git/actions/runs/15692373534/job/44210362705\nbut is that right? If I should submit this patch to the git-for-windows\nfork, please let me know. Or, if someone, who knows what they're\ndoing, wants to pick this up, they're more than welcome.\n\nJames Duley\n"},{"id":"520388","messageId":"ckzjxolc5jthge62zcm5abnssefwntc3ryu6sumpvrdontmyyq@d4ahvg5umked","threadId":"63661","inReplyTo":"CANZAkJuPUYSCd4yGHFhs6hfERovzjwWoM2gU2MyAHvY_AtjLkg@mail.gmail.com","subject":"Re: [PATCH] Ensure restore_term works correctly with DUPLEX","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2025-06-18T23:43:07Z","receivedAt":"2025-06-18T23:43:10Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Jun 19, 2025 at 08:38:29AM -0800, James Duley wrote:\n> On Wed, 18 Jun 2025 at 22:07, Carlo Marcelo Arenas Belón\n> wrote:\n> >\n> > On Tue, Jun 17, 2025 at 12:18:07PM -0800, Junio C Hamano wrote:\n> > > \"James Duley via GitGitGadget\" writes:\n> > >\n> > > > This is because save_term doesn't set cmode_out when not DUPLEX,\n> > > > so an old version of cmode_out was being used.\n> >\n> > To fully address that bug though, something like the following\n> > (untested) needs to be squashed on top, right?:\n> >\n> > ----\n> > diff --git a/compat/terminal.c b/compat/terminal.c\n> > index 72b184555f..8a197ffea1 100644\n> > --- a/compat/terminal.c\n> > +++ b/compat/terminal.c\n> > @@ -279,7 +279,7 @@ void restore_term(void)\n> >\n> > SetConsoleMode(hconin, cmode_in);\n> > CloseHandle(hconin);\n> > - if (hconout != INVALID_HANDLE_VALUE) {\n> > + if (cmode_out && hconout != INVALID_HANDLE_VALUE) {\n> > SetConsoleMode(hconout, cmode_out);\n> > CloseHandle(hconout);\n> > }\n> > @@ -299,11 +299,15 @@ int save_term(enum save_term_flags flags)\n> > hconout = CreateFileA(\"CONOUT$\", GENERIC_READ | GENERIC_WRITE,\n> > FILE_SHARE_WRITE, NULL, OPEN_EXISTING,\n> > FILE_ATTRIBUTE_NORMAL, NULL);\n> > - if (hconout == INVALID_HANDLE_VALUE)\n> > + if (hconout == INVALID_HANDLE_VALUE) {\n> > + cmode_out = 0;\n> > goto error;\n> > + }\n> >\n> > GetConsoleMode(hconout, &cmode_out);\n> > }\n> > + else\n> > + cmode_out = 0;\n> >\n> > GetConsoleMode(hconin, &cmode_in);\n> > use_stty = 0;\n> >\n> > It would be nice to know, if the problem with vi that this was meant to\n> > address, and that needs further changes, that are only in the git for\n> > windows fork is stll relevant, so this could be cleaned further.\n> >\n> > Carlo\n> \n> I thought about something like that, but I figured:\n> * restore_term is only called if save_term is successful\n> * hconout is always invalid before save_term is called\n\nbut it is not always set to invalid at the end of restore_term()\nso it can't be relied upon either.\n\n> * 0 might be a valid cmode_out that should be restored\n\ncmode_out == 0 is not a valid mode that should be restored, and\nindeed the original code was (ab)using that fact to decide if\nSetConsoleMode(hcounout) would be called at all (as a proxy to\nknow if DUPLEX was used or not), hence why it is a bug not to\nupdate it, as you pointed out and found unexpectally, sorry\nabout that.\n\n> This is my first patch so I didn't realize git-for-windows had a\n> separate fork. That makes sense now because I couldn't find where\n> save_term was called from in this repo. To test this works I had\n> downloaded the artifacts from\n> https://github.com/git/git/actions/runs/15692373534/job/44210362705\n> but is that right? If I should submit this patch to the git-for-windows\n> fork, please let me know. Or, if someone, who knows what they're\n> doing, wants to pick this up, they're more than welcome.\n\nYou are going to need to get the Git for Windows SDK installed to\nbe able to apply this patch and build your own version of GfW.\n\nIMHO getting the change that makes \"cmode_out\" reliable again (which\nwould include both our changes) should be a good start regardless,\nand at least that change could be submitted here.\n\nCarlo\n"},{"id":"520561","messageId":"14cc73ba-3e6a-4994-b966-a5a09dd042f9@gmail.com","threadId":"63661","inReplyTo":"pull.2000.git.git.1750186571037.gitgitgadget@gmail.com","subject":"Re: [PATCH] Ensure restore_term works correctly with DUPLEX","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-06-23T09:35:10Z","receivedAt":"2025-06-23T09:35:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi James\n\nOn 17/06/2025 19:56, James Duley via GitGitGadget wrote:\n> From: James Duley \n> \n> Previously, if save_term/restore_term was called with the DUPLEX flag\n> and then without the flag, an assertion was hit.\n>> Assertion failed: hconout != INVALID_HANDLE_VALUE,\n>> file compat/terminal.c, line 283\n> \n> This is because save_term doesn't set cmode_out when not DUPLEX,\n> so an old version of cmode_out was being used.\n> Therefore, hconout is the correct thing for restore to check\n> to decide whether to restore stdout console mode.\n\nI found this paragraph (especially the first sentence) rather hard to \nunderstand - it was only after I'd figured out what the problem was by \nreading the code that I could understand what it was saying.\n\nAs I understand it the problem is caused by calling\n\n save_term(SAVE_TERM_DUPLEX);\n restore_term();\n save_term(0);\n restore_term();\n\nThe first call to save_term() sets hconout to a valid handle and \ncmode_out to a non-zero value. The first call to restore_term() then \nsets hconout to INVALID_HANDLE_VALUE but leaves cmode_out unchanged. The \nsecond call to save_term does not touch hconout or cmode_out. The second \ncall to restore_term() then hits the assertion because cmode_out is \nnon-zero but hconout is an invalid handle.\n\nI think it would be helpful to include an explanation like that in the \ncommit message.\n\nI agree with you diagnosis and the proposed fix - using the filehandle \nto tell whether we should restore the output settings is much cleaner \nthat looking at the mode. I would suggest that we should also set \nhconout to INVALID_HANDLE_VALUE when save_term() is called without \nSAVE_TERM_DUPLEX to avoid any problems with call sequences like\n\n save_term(SAVE_TERM_DUPLEX);\n save_term(0);\n restore_term();\n\nThanks for reporting and working on this\n\nPhillip\n\n> I saw this on Windows with interactive.singleKey when doing `git add -p`.\n> Specifically, after hitting `e` to edit in vim, once on to the prompt\n> for the next hunk, pressing any key results in the assertion.\n> \n> Signed-off-by: James Duley \n> ---\n> Ensure restore_term works correctly with DUPLEX\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2000%2Fparched%2Frestore-term-windows-fix-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2000/parched/restore-term-windows-fix-v1\n> Pull-Request: https://github.com/git/git/pull/2000\n> \n> compat/terminal.c | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n> \n> diff --git a/compat/terminal.c b/compat/terminal.c\n> index 584f27bf7e1..72b184555ff 100644\n> --- a/compat/terminal.c\n> +++ b/compat/terminal.c\n> @@ -279,8 +279,7 @@ void restore_term(void)\n> \n> \tSetConsoleMode(hconin, cmode_in);\n> \tCloseHandle(hconin);\n> -\tif (cmode_out) {\n> -\t\tassert(hconout != INVALID_HANDLE_VALUE);\n> +\tif (hconout != INVALID_HANDLE_VALUE) {\n> \t\tSetConsoleMode(hconout, cmode_out);\n> \t\tCloseHandle(hconout);\n> \t}\n> \n> base-commit: 16bd9f20a403117f2e0d9bcda6c6e621d3763e77\n\n\n\n"},{"id":"520562","messageId":"d6e22dba-637b-4b5d-bd88-25fa320e22ff@gmail.com","threadId":"63661","inReplyTo":"ckzjxolc5jthge62zcm5abnssefwntc3ryu6sumpvrdontmyyq@d4ahvg5umked","subject":"Re: [PATCH] Ensure restore_term works correctly with DUPLEX","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-06-23T09:42:12Z","receivedAt":"2025-06-23T09:42:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 19/06/2025 00:43, Carlo Marcelo Arenas Belón wrote:\n> On Thu, Jun 19, 2025 at 08:38:29AM -0800, James Duley wrote:\n>> On Wed, 18 Jun 2025 at 22:07, Carlo Marcelo Arenas Belón\n>> wrote:\n>>>\n>> I thought about something like that, but I figured:\n>> * restore_term is only called if save_term is successful\n>> * hconout is always invalid before save_term is called\n> \n> but it is not always set to invalid at the end of restore_term()\n> so it can't be relied upon either.\n\nAre you sure about that? It looks to me like the only time we don't \nreset hconout is when we use stty in which case I think hconout is \nalready set to the invalid handle value.\n\n>> * 0 might be a valid cmode_out that should be restored\n> \n> cmode_out == 0 is not a valid mode that should be restored,\n\ncmode_out is a bitfield. The documentation for SetConsoleMode() says the \nmode parameter \"can be one or more of the following values\" which \nimplies zero is not a valid mode. It seems very hacky to be relying on \nthat when we can just check if the output handle is valid though.\n\n> and\n> indeed the original code was (ab)using that fact to decide if\n> SetConsoleMode(hcounout) would be called at all (as a proxy to\n> know if DUPLEX was used or not), hence why it is a bug not to\n> update it, as you pointed out and found unexpectally, sorry\n> about that.\n> \n>> This is my first patch so I didn't realize git-for-windows had a\n>> separate fork. That makes sense now because I couldn't find where\n>> save_term was called from in this repo. To test this works I had\n>> downloaded the artifacts from\n>> https://github.com/git/git/actions/runs/15692373534/job/44210362705\n>> but is that right? If I should submit this patch to the git-for-windows\n>> fork, please let me know. Or, if someone, who knows what they're\n>> doing, wants to pick this up, they're more than welcome.\n> \n> You are going to need to get the Git for Windows SDK installed to\n> be able to apply this patch and build your own version of GfW.\n> \n> IMHO getting the change that makes \"cmode_out\" reliable again (which\n> would include both our changes) should be a good start regardless,\n> and at least that change could be submitted here.\n\nYes, this change should absolutely be submitted here as it is fixing \ncode that is upstream of git-for-windows.\n\nThanks\n\nPhillip\n\n> \n> Carlo\n> \n\n"}]}