[PATCH] Ensure restore_term works correctly with DUPLEX

[PATCH] Ensure restore_term works correctly with DUPLEX

[James Duley via GitGitGadget]

From: James Duley <jagduley@gmail.com>

Previously, if save_term/restore_term was called with the DUPLEX flag
and then without the flag, an assertion was hit.
> Assertion failed: hconout != INVALID_HANDLE_VALUE,
> file compat/terminal.c, line 283

This is because save_term doesn't set cmode_out when not DUPLEX,
so an old version of cmode_out was being used.
Therefore, hconout is the correct thing for restore to check
to decide whether to restore stdout console mode.

I saw this on Windows with interactive.singleKey when doing `git add -p`.
Specifically, after hitting `e` to edit in vim, once on to the prompt
for the next hunk, pressing any key results in the assertion.

Signed-off-by: James Duley <jagduley@gmail.com>
---
    Ensure restore_term works correctly with DUPLEX

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2000%2Fparched%2Frestore-term-windows-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2000/parched/restore-term-windows-fix-v1
Pull-Request: https://github.com/git/git/pull/2000

 compat/terminal.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/compat/terminal.c b/compat/terminal.c
index 584f27bf7e1..72b184555ff 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -279,8 +279,7 @@ void restore_term(void)
 
 	SetConsoleMode(hconin, cmode_in);
 	CloseHandle(hconin);
-	if (cmode_out) {
-		assert(hconout != INVALID_HANDLE_VALUE);
+	if (hconout != INVALID_HANDLE_VALUE) {
 		SetConsoleMode(hconout, cmode_out);
 		CloseHandle(hconout);
 	}

base-commit: 16bd9f20a403117f2e0d9bcda6c6e621d3763e77
-- 
gitgitgadget

Re: [PATCH] Ensure restore_term works correctly with DUPLEX

[Junio C Hamano]

"James Duley via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: James Duley <jagduley@gmail.com>
>
> Previously, if save_term/restore_term was called with the DUPLEX flag
> and then without the flag, an assertion was hit.
>> Assertion failed: hconout != INVALID_HANDLE_VALUE,
>> file compat/terminal.c, line 283
>
> This is because save_term doesn't set cmode_out when not DUPLEX,
> so an old version of cmode_out was being used.
> Therefore, hconout is the correct thing for restore to check
> to decide whether to restore stdout console mode.
>
> I saw this on Windows with interactive.singleKey when doing `git add -p`.
> Specifically, after hitting `e` to edit in vim, once on to the prompt
> for the next hunk, pressing any key results in the assertion.
>
> Signed-off-by: James Duley <jagduley@gmail.com>
> ---
>     Ensure restore_term works correctly with DUPLEX

Ran "git log -L247,290:compat/terminal.c" to find commits that
touched this Windows specific area of the code to see who might be
capable of reviewing this change, as I am certainly not one of them.

e22b245e (terminal: teach git how to save/restore its terminal
settings, 2021-10-05) is where the assert being removed came from,
it seems.

The author of that commit should be CC'ed if we want to ask for a
quicker review.  I'll also add Git-for-Windows maintainer as well.

Thanks.


> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2000%2Fparched%2Frestore-term-windows-fix-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2000/parched/restore-term-windows-fix-v1
> Pull-Request: https://github.com/git/git/pull/2000
>
>  compat/terminal.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/compat/terminal.c b/compat/terminal.c
> index 584f27bf7e1..72b184555ff 100644
> --- a/compat/terminal.c
> +++ b/compat/terminal.c
> @@ -279,8 +279,7 @@ void restore_term(void)
>  
>  	SetConsoleMode(hconin, cmode_in);
>  	CloseHandle(hconin);
> -	if (cmode_out) {
> -		assert(hconout != INVALID_HANDLE_VALUE);
> +	if (hconout != INVALID_HANDLE_VALUE) {
>  		SetConsoleMode(hconout, cmode_out);
>  		CloseHandle(hconout);
>  	}
>
> base-commit: 16bd9f20a403117f2e0d9bcda6c6e621d3763e77

Re: [PATCH] Ensure restore_term works correctly with DUPLEX

[Carlo Marcelo Arenas Belón]

On Tue, Jun 17, 2025 at 12:18:07PM -0800, Junio C Hamano wrote:
> "James Duley via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > This is because save_term doesn't set cmode_out when not DUPLEX,
> > so an old version of cmode_out was being used.

To fully address that bug though, something like the following
(untested) needs to be squashed on top, right?:

----
diff --git a/compat/terminal.c b/compat/terminal.c
index 72b184555f..8a197ffea1 100644
--- a/compat/terminal.c
+++ b/compat/terminal.c
@@ -279,7 +279,7 @@ void restore_term(void)
 
 	SetConsoleMode(hconin, cmode_in);
 	CloseHandle(hconin);
-	if (hconout != INVALID_HANDLE_VALUE) {
+	if (cmode_out && hconout != INVALID_HANDLE_VALUE) {
 		SetConsoleMode(hconout, cmode_out);
 		CloseHandle(hconout);
 	}
@@ -299,11 +299,15 @@ int save_term(enum save_term_flags flags)
 		hconout = CreateFileA("CONOUT$", GENERIC_READ | GENERIC_WRITE,
 			FILE_SHARE_WRITE, NULL, OPEN_EXISTING,
 			FILE_ATTRIBUTE_NORMAL, NULL);
-		if (hconout == INVALID_HANDLE_VALUE)
+		if (hconout == INVALID_HANDLE_VALUE) {
+			cmode_out = 0;
 			goto error;
+		}
 
 		GetConsoleMode(hconout, &cmode_out);
 	}
+	else
+		cmode_out = 0;
 
 	GetConsoleMode(hconin, &cmode_in);
 	use_stty = 0;

It would be nice to know, if the problem with vi that this was meant to
address, and that needs further changes, that are only in the git for
windows fork is stll relevant, so this could be cleaned further.

Carlo

Re: [PATCH] Ensure restore_term works correctly with DUPLEX

[James Duley]

On Wed, 18 Jun 2025 at 22:07, Carlo Marcelo Arenas Belón
<carenas@gmail.com> wrote:
>
> On Tue, Jun 17, 2025 at 12:18:07PM -0800, Junio C Hamano wrote:
> > "James Duley via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> > > This is because save_term doesn't set cmode_out when not DUPLEX,
> > > so an old version of cmode_out was being used.
>
> To fully address that bug though, something like the following
> (untested) needs to be squashed on top, right?:
>
> ----
> diff --git a/compat/terminal.c b/compat/terminal.c
> index 72b184555f..8a197ffea1 100644
> --- a/compat/terminal.c
> +++ b/compat/terminal.c
> @@ -279,7 +279,7 @@ void restore_term(void)
>
>         SetConsoleMode(hconin, cmode_in);
>         CloseHandle(hconin);
> -       if (hconout != INVALID_HANDLE_VALUE) {
> +       if (cmode_out && hconout != INVALID_HANDLE_VALUE) {
>                 SetConsoleMode(hconout, cmode_out);
>                 CloseHandle(hconout);
>         }
> @@ -299,11 +299,15 @@ int save_term(enum save_term_flags flags)
>                 hconout = CreateFileA("CONOUT$", GENERIC_READ | GENERIC_WRITE,
>                         FILE_SHARE_WRITE, NULL, OPEN_EXISTING,
>                         FILE_ATTRIBUTE_NORMAL, NULL);
> -               if (hconout == INVALID_HANDLE_VALUE)
> +               if (hconout == INVALID_HANDLE_VALUE) {
> +                       cmode_out = 0;
>                         goto error;
> +               }
>
>                 GetConsoleMode(hconout, &cmode_out);
>         }
> +       else
> +               cmode_out = 0;
>
>         GetConsoleMode(hconin, &cmode_in);
>         use_stty = 0;
>
> It would be nice to know, if the problem with vi that this was meant to
> address, and that needs further changes, that are only in the git for
> windows fork is stll relevant, so this could be cleaned further.
>
> Carlo

I thought about something like that, but I figured:
* restore_term is only called if save_term is successful
* hconout is always invalid before save_term is called
* 0 might be a valid cmode_out that should be restored

This is my first patch so I didn't realize git-for-windows had a
separate fork. That makes sense now because I couldn't find where
save_term was called from in this repo. To test this works I had
downloaded the artifacts from
https://github.com/git/git/actions/runs/15692373534/job/44210362705
but is that right? If I should submit this patch to the git-for-windows
fork, please let me know. Or, if someone, who knows what they're
doing, wants to pick this up, they're more than welcome.

James Duley

Re: [PATCH] Ensure restore_term works correctly with DUPLEX

[Carlo Marcelo Arenas Belón]

On Thu, Jun 19, 2025 at 08:38:29AM -0800, James Duley wrote:
> On Wed, 18 Jun 2025 at 22:07, Carlo Marcelo Arenas Belón
> <carenas@gmail.com> wrote:
> >
> > On Tue, Jun 17, 2025 at 12:18:07PM -0800, Junio C Hamano wrote:
> > > "James Duley via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > >
> > > > This is because save_term doesn't set cmode_out when not DUPLEX,
> > > > so an old version of cmode_out was being used.
> >
> > To fully address that bug though, something like the following
> > (untested) needs to be squashed on top, right?:
> >
> > ----
> > diff --git a/compat/terminal.c b/compat/terminal.c
> > index 72b184555f..8a197ffea1 100644
> > --- a/compat/terminal.c
> > +++ b/compat/terminal.c
> > @@ -279,7 +279,7 @@ void restore_term(void)
> >
> >         SetConsoleMode(hconin, cmode_in);
> >         CloseHandle(hconin);
> > -       if (hconout != INVALID_HANDLE_VALUE) {
> > +       if (cmode_out && hconout != INVALID_HANDLE_VALUE) {
> >                 SetConsoleMode(hconout, cmode_out);
> >                 CloseHandle(hconout);
> >         }
> > @@ -299,11 +299,15 @@ int save_term(enum save_term_flags flags)
> >                 hconout = CreateFileA("CONOUT$", GENERIC_READ | GENERIC_WRITE,
> >                         FILE_SHARE_WRITE, NULL, OPEN_EXISTING,
> >                         FILE_ATTRIBUTE_NORMAL, NULL);
> > -               if (hconout == INVALID_HANDLE_VALUE)
> > +               if (hconout == INVALID_HANDLE_VALUE) {
> > +                       cmode_out = 0;
> >                         goto error;
> > +               }
> >
> >                 GetConsoleMode(hconout, &cmode_out);
> >         }
> > +       else
> > +               cmode_out = 0;
> >
> >         GetConsoleMode(hconin, &cmode_in);
> >         use_stty = 0;
> >
> > It would be nice to know, if the problem with vi that this was meant to
> > address, and that needs further changes, that are only in the git for
> > windows fork is stll relevant, so this could be cleaned further.
> >
> > Carlo
> 
> I thought about something like that, but I figured:
> * restore_term is only called if save_term is successful
> * hconout is always invalid before save_term is called

but it is not always set to invalid at the end of restore_term()
so it can't be relied upon either.

> * 0 might be a valid cmode_out that should be restored

cmode_out == 0 is not a valid mode that should be restored, and
indeed the original code was (ab)using that fact to decide if
SetConsoleMode(hcounout) would be called at all (as a proxy to
know if DUPLEX was used or not), hence why it is a bug not to
update it, as you pointed out and found unexpectally, sorry
about that.

> This is my first patch so I didn't realize git-for-windows had a
> separate fork. That makes sense now because I couldn't find where
> save_term was called from in this repo. To test this works I had
> downloaded the artifacts from
> https://github.com/git/git/actions/runs/15692373534/job/44210362705
> but is that right? If I should submit this patch to the git-for-windows
> fork, please let me know. Or, if someone, who knows what they're
> doing, wants to pick this up, they're more than welcome.

You are going to need to get the Git for Windows SDK installed to
be able to apply this patch and build your own version of GfW.

IMHO getting the change that makes "cmode_out" reliable again (which
would include both our changes) should be a good start regardless,
and at least that change could be submitted here.

Carlo

Re: [PATCH] Ensure restore_term works correctly with DUPLEX

[Phillip Wood]

Hi James

On 17/06/2025 19:56, James Duley via GitGitGadget wrote:
> From: James Duley <jagduley@gmail.com>
> 
> Previously, if save_term/restore_term was called with the DUPLEX flag
> and then without the flag, an assertion was hit.
>> Assertion failed: hconout != INVALID_HANDLE_VALUE,
>> file compat/terminal.c, line 283
> 
> This is because save_term doesn't set cmode_out when not DUPLEX,
> so an old version of cmode_out was being used.
> Therefore, hconout is the correct thing for restore to check
> to decide whether to restore stdout console mode.

I found this paragraph (especially the first sentence) rather hard to 
understand - it was only after I'd figured out what the problem was by 
reading the code that I could understand what it was saying.

As I understand it the problem is caused by calling

     save_term(SAVE_TERM_DUPLEX);
     restore_term();
     save_term(0);
     restore_term();

The first call to save_term() sets hconout to a valid handle and 
cmode_out to a non-zero value. The first call to restore_term() then 
sets hconout to INVALID_HANDLE_VALUE but leaves cmode_out unchanged. The 
second call to save_term does not touch hconout or cmode_out. The second 
call to restore_term() then hits the assertion because cmode_out is 
non-zero but hconout is an invalid handle.

I think it would be helpful to include an explanation like that in the 
commit message.

I agree with you diagnosis and the proposed fix - using the filehandle 
to tell whether we should restore the output settings is much cleaner 
that looking at the mode. I would suggest that we should also set 
hconout to INVALID_HANDLE_VALUE when save_term() is called without 
SAVE_TERM_DUPLEX to avoid any problems with call sequences like

     save_term(SAVE_TERM_DUPLEX);
     save_term(0);
     restore_term();

Thanks for reporting and working on this

Phillip

> I saw this on Windows with interactive.singleKey when doing `git add -p`.
> Specifically, after hitting `e` to edit in vim, once on to the prompt
> for the next hunk, pressing any key results in the assertion.
> 
> Signed-off-by: James Duley <jagduley@gmail.com>
> ---
>      Ensure restore_term works correctly with DUPLEX
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2000%2Fparched%2Frestore-term-windows-fix-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2000/parched/restore-term-windows-fix-v1
> Pull-Request: https://github.com/git/git/pull/2000
> 
>   compat/terminal.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/compat/terminal.c b/compat/terminal.c
> index 584f27bf7e1..72b184555ff 100644
> --- a/compat/terminal.c
> +++ b/compat/terminal.c
> @@ -279,8 +279,7 @@ void restore_term(void)
>   
>   	SetConsoleMode(hconin, cmode_in);
>   	CloseHandle(hconin);
> -	if (cmode_out) {
> -		assert(hconout != INVALID_HANDLE_VALUE);
> +	if (hconout != INVALID_HANDLE_VALUE) {
>   		SetConsoleMode(hconout, cmode_out);
>   		CloseHandle(hconout);
>   	}
> 
> base-commit: 16bd9f20a403117f2e0d9bcda6c6e621d3763e77

Re: [PATCH] Ensure restore_term works correctly with DUPLEX

[Phillip Wood]

On 19/06/2025 00:43, Carlo Marcelo Arenas Belón wrote:
> On Thu, Jun 19, 2025 at 08:38:29AM -0800, James Duley wrote:
>> On Wed, 18 Jun 2025 at 22:07, Carlo Marcelo Arenas Belón
>> <carenas@gmail.com> wrote:
>>>
>> I thought about something like that, but I figured:
>> * restore_term is only called if save_term is successful
>> * hconout is always invalid before save_term is called
> 
> but it is not always set to invalid at the end of restore_term()
> so it can't be relied upon either.

Are you sure about that? It looks to me like the only time we don't 
reset hconout is when we use stty in which case I think hconout is 
already set to the invalid handle value.

>> * 0 might be a valid cmode_out that should be restored
> 
> cmode_out == 0 is not a valid mode that should be restored,

cmode_out is a bitfield. The documentation for SetConsoleMode() says the 
mode parameter "can be one or more of the following values" which 
implies zero is not a valid mode. It seems very hacky to be relying on 
that when we can just check if the output handle is valid though.

> and
> indeed the original code was (ab)using that fact to decide if
> SetConsoleMode(hcounout) would be called at all (as a proxy to
> know if DUPLEX was used or not), hence why it is a bug not to
> update it, as you pointed out and found unexpectally, sorry
> about that.
> 
>> This is my first patch so I didn't realize git-for-windows had a
>> separate fork. That makes sense now because I couldn't find where
>> save_term was called from in this repo. To test this works I had
>> downloaded the artifacts from
>> https://github.com/git/git/actions/runs/15692373534/job/44210362705
>> but is that right? If I should submit this patch to the git-for-windows
>> fork, please let me know. Or, if someone, who knows what they're
>> doing, wants to pick this up, they're more than welcome.
> 
> You are going to need to get the Git for Windows SDK installed to
> be able to apply this patch and build your own version of GfW.
> 
> IMHO getting the change that makes "cmode_out" reliable again (which
> would include both our changes) should be a good start regardless,
> and at least that change could be submitted here.

Yes, this change should absolutely be submitted here as it is fixing 
code that is upstream of git-for-windows.

Thanks

Phillip

> 
> Carlo
>