Re: [PATCH] Ensure restore_term works correctly with DUPLEX

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jun 23, 2025, 09:35 UTC
Message-ID: <14cc73ba-3e6a-4994-b966-a5a09dd042f9@gmail.com>
In-Reply-To: <pull.2000.git.git.1750186571037.gitgitgadget@gmail.com>

Hi James

On 17/06/2025 19:56, James Duley via GitGitGadget wrote:

Show 11 quoted lines

> 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

Show 31 quoted 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 <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