Re: [PATCH] Ensure restore_term works correctly with DUPLEX

From: James Duley <jagduley@gmail.com>
Date: Jun 18, 2025, 20:38 UTC
Message-ID: <CANZAkJuPUYSCd4yGHFhs6hfERovzjwWoM2gU2MyAHvY_AtjLkg@mail.gmail.com>
In-Reply-To: <zpv3nkpsjqlg7evklw4ui5q7wonak2376rclu5ksmumdvk3tic@tfxmcpjmj3nw>

On Wed, 18 Jun 2025 at 22:07, Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote:

Show 47 quoted lines

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