Ensure restore_term works correctly with DUPLEX #2000
Conversation
Welcome to GitGitGadgetHi @parched, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that either:
You can CC potential reviewers by adding a footer to the PR description with the following syntax: NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description, Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join [email protected], where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
|
There are issues in commit ed9fbe9: |
|
/allow |
|
User parched is now allowed to use GitGitGadget. WARNING: parched has no public email address set on GitHub; GitGitGadget needs an email address to Cc: you on your contribution, so that you receive any feedback on the Git mailing list. Go to https://github.com/settings/profile to make your preferred email public to let GitGitGadget know which email address to use. |
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 <[email protected]>
ed9fbe9 to
f9b38a9
Compare
|
/preview |
|
Preview email sent as [email protected] |
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "James Duley via GitGitGadget" <[email protected]> writes:
> 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.
>> 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
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 |
|
On the Git mailing list, Carlo Marcelo Arenas Belón wrote (reply to this): On Tue, Jun 17, 2025 at 12:18:07PM -0800, Junio C Hamano wrote:
> "James Duley via GitGitGadget" <[email protected]> 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 |
|
User |
|
On the Git mailing list, James Duley wrote (reply to this): On Wed, 18 Jun 2025 at 22:07, Carlo Marcelo Arenas Belón
<[email protected]> wrote:
>
> On Tue, Jun 17, 2025 at 12:18:07PM -0800, Junio C Hamano wrote:
> > "James Duley via GitGitGadget" <[email protected]> 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 |
|
On the Git mailing list, Carlo Marcelo Arenas Belón wrote (reply to this): 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
> <[email protected]> wrote:
> >
> > On Tue, Jun 17, 2025 at 12:18:07PM -0800, Junio C Hamano wrote:
> > > "James Duley via GitGitGadget" <[email protected]> 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 |
|
On the Git mailing list, Phillip Wood wrote (reply to this): Hi James
On 17/06/2025 19:56, James Duley via GitGitGadget wrote:
> 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.
>> 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 <[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(-)
> > 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
|
|
User |
|
On the Git mailing list, Phillip Wood wrote (reply to this): 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
>> <[email protected]> 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
> |
cc: Carlo Marcelo Arenas Belón [email protected]
cc: Phillip Wood [email protected]