git
Ensure restore_term works correctly with DUPLEX
Welcome to GitGitGadget
Hi @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:
- Your Pull Request has a good description, if it consists of multiple commits, as it will be used as cover letter.
- Your Pull Request description is empty, if it consists of a single commit, as the commit message should be descriptive enough by itself.
You can CC potential reviewers by adding a footer to the PR description with the following syntax:
CC: Revi Ewer <[email protected]>, Ill Takalook <[email protected]>
NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description, because it will result in a malformed CC list on the mailing list. See example.
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:
- the lines should not exceed 76 columns,
- the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
- the commit messages' body should be describing the "why?" of the change.
- Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.
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 patches
Before 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 /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:
Both the person who commented /allow and the PR author are able to /allow you.
An alternative is the channel #git-devel on the Libera Chat IRC network:
<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!
Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.
If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this. Note that any reviewers CC'd via the list in the PR description will not actually be sent emails.
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 (raw) link), then import it into your mail program. If you use GMail, you can do this via:
curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt
To 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):
Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...
To send a new iteration, just add another PR comment with the contents: /submit.
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, #git-devel on Libera Chat. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.
![gitgitgadget-git[bot] avatar](https://avatars.githubusercontent.com/in/46807?v=4 "Published by a pub.dev verified publisher"){kind=small}
There are issues in commit ed9fbe9676b289a313db5cedd66e899dd1520fba:
Ensure restore_term works correctly with DUPLEX
Commit not signed off
Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
Indented lines, and lines without whitespace, are exempt
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.
Submitted as [email protected]
To fetch this version into FETCH_HEAD:
git fetch https://github.com/gitgitgadget/git/ pr-git-2000/parched/restore-term-windows-fix-v1
To fetch this version to local tag pr-git-2000/parched/restore-term-windows-fix-v1:
git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-2000/parched/restore-term-windows-fix-v1
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
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
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
>