Skip to contentSkip to conversation

Ensure restore_term works correctly with DUPLEX

PATCH In review James Duley via GitGitGadget 5 participants 7 messages Jun 17, 2025 – Jun 23, 2025

lore
Share & tools
lore

James Duley via GitGitGadget

author
#0 lore raw

PATCH 1/1 Ensure restore_term works correctly with DUPLEX

From: James Duley <[email protected]>

Previously, if save_term/restore_term was called with the DUPLEX flag and then without the flag, an assertion was hit.

Quoted 2 lines

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 <[email protected]>

---
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(-)
compat/terminal.c +1 -2
index 584f27bf7e1..72b184555ff 100644
@@ -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

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 12 lines · James Duley via GitGitGadget

From: James Duley <[email protected]>

Previously, if save_term/restore_term was called with the DUPLEX flag and then without the flag, an assertion was hit.

Quoted 2 lines

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.

Quoted 7 lines

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 <[email protected]>

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

Quoted 23 lines

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(-)
compat/terminal.c +1 -2
index 584f27bf7e1..72b184555ff 100644
@@ -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

#2 lore raw
Quoted 4 lines · Junio C Hamano
Quoted 2 lines · James Duley via GitGitGadget

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?:

----

compat/terminal.c +6 -2
index 72b184555f..8a197ffea1 100644
@@ -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

#3 lore raw
Quoted 42 lines · Carlo Marcelo Arenas Belón
Quoted 4 lines · Junio C Hamano
Quoted 2 lines · James Duley via GitGitGadget

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?:

----

compat/terminal.c +6 -2
index 72b184555f..8a197ffea1 100644
@@ -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;
Quoted 5 lines

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

#4 lore raw
Quoted 48 lines · James Duley
Quoted 46 lines · Carlo Marcelo Arenas Belón
Quoted 4 lines · Junio C Hamano
Quoted 2 lines · James Duley via GitGitGadget

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?:

----

compat/terminal.c +6 -2
index 72b184555f..8a197ffea1 100644
@@ -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.

Quoted 5 lines
Quoted 1 line

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.

Quoted 1 line

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

Quoted 8 lines

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

#5 lore raw
Quoted 10 lines · Carlo Marcelo Arenas Belón
Quoted 6 lines · James Duley
Quoted 1 line · Carlo Marcelo Arenas Belón

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.

Quoted 3 lines
Quoted 1 line

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

Quoted 16 lines

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.

Quoted 8 lines

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.

Quoted 6 lines

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

Quoted 3 lines

Carlo

#6 lore raw
In reply to James Duley via GitGitGadget

Hi James

Quoted 11 lines · James Duley via GitGitGadget

From: James Duley <[email protected]>

Previously, if save_term/restore_term was called with the DUPLEX flag and then without the flag, an assertion was hit.

Quoted 2 lines

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

Quoted 31 lines

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 <[email protected]>

---
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(-)
compat/terminal.c +1 -2
index 584f27bf7e1..72b184555ff 100644
@@ -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