Re: [PATCH] Ensure restore_term works correctly with DUPLEX
On Thu, Jun 19, 2025 at 08:38:29AM -0800, James Duley wrote:
Show 53 quoted lines
> 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 calledbut 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.
Show 8 quoted 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
