a couple of read_key_without_echo() fixes #1146
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as pull.1146.git.1645008873.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -11,7 +11,7 @@ | |||
| static void restore_term_on_signal(int sig) | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> If VMIN and VTIME are both set to zero then the terminal performs
> non-blocking reads which means that read_key_without_echo() returns
> EOF if there is no key press pending. This results in the user being
> unable to select anything when running "git add -p". Fix this by
> explicitly setting VMIN and VTIME when enabling non-canonical mode.
Makes sense.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> compat/terminal.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/compat/terminal.c b/compat/terminal.c
> index 20803badd03..d882dfa06e3 100644
> --- a/compat/terminal.c
> +++ b/compat/terminal.c
> @@ -57,6 +57,10 @@ static int disable_bits(tcflag_t bits)
> t = old_term;
>
> t.c_lflag &= ~bits;
> + if (bits & ICANON) {
> + t.c_cc[VMIN] = 1;
> + t.c_cc[VTIME] = 0;
> + }
Quite sensible. I wonder if we can simplify the "are we looking at
an ESC that is the first byte of a multi-byte control sequence?"
logic in the caller by using inter-character delay, but it is
probably better to go one step at a time like this patch does.
> if (!tcsetattr(term_fd, TCSAFLUSH, &t))
> return 0;
>
> @@ -159,7 +163,11 @@ static int disable_bits(DWORD bits)
>
> if (bits & ENABLE_LINE_INPUT) {
> string_list_append(&stty_restore, "icanon");
> - strvec_push(&cp.args, "-icanon");
> + /*
> + * POSIX allows VMIN and VTIME to overlap with VEOF and
> + * VEOL - let's hope that is not the case on windows.
> + */
> + strvec_pushl(&cp.args, "-icanon", "min", "1", "time", "0", NULL);
Interesting. So each call to read_key_without_echo() ends up being
a run_command("stty -icanon min 1 time 0") followed by a read
followed by another "stty".
At least while in -icanon mode, VEOF and VEOL do not take effect,
and the potential overlap would not matter. It really depends on
what happens upon restore.
Do we have similar "let's hope" on the tcsetattr() side, too?
> }
>
> if (bits & ENABLE_ECHO_INPUT) {
Thanks.
| @@ -70,6 +70,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r) | |||
| &s->interactive_diff_algorithm); | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/add-interactive.c b/add-interactive.c
> index 6498ae196f1..ad78774ca26 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -70,6 +70,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r)
> &s->interactive_diff_algorithm);
>
> git_config_get_bool("interactive.singlekey", &s->use_single_key);
> + if (s->use_single_key)
> + setbuf(stdin, NULL);
OK. Will queue. Thanks.
> }
>
> void clear_add_i_state(struct add_i_state *s)
|
This branch is now known as |
|
This patch series was integrated into seen via git@c39da26. |
|
This patch series was integrated into seen via git@a00a850. |
|
This patch series was integrated into seen via git@788a220. |
|
This patch series was integrated into seen via git@2f15c70. |
|
There was a status update in the "New Topics" section about the branch The single-key interactive operation used by "git add -p" has been made more robust. Will merge to 'next'? source: <pull.1146.git.1645008873.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@74e0049. |
|
This patch series was integrated into seen via git@e53fb7a. |
|
This patch series was integrated into seen via git@21a4e89. |
Break out of the loop to ensure restore_term() is called before returning. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
When disable_bits() changes the terminal attributes it uses sigchain_push_common() to restore the terminal if a signal is received before restore_term() is called. However there is no corresponding call to sigchain_pop_common() when the settings are restored so the signal handler is left on the sigchain stack. This leaves the stack unbalanced so code such as sigchain_push_common(my_handler); ... read_key_without_echo(...); ... sigchain_pop_common(); pops the handler pushed by disable_bits() rather than the one it intended to. Additionally "git add -p" changes the terminal settings every time it reads a key press so the stack can grow significantly. In order to fix this save_term() now sets up the signal handler so restore_term() can unconditionally call sigchain_pop_common(). There are no callers of save_term() outside of terminal.c as the only external caller was removed by e3f7e01 ("Revert "editor: save and reset terminal after calling EDITOR"", 2021-11-22). Any future callers of save_term() should benefit from having the signal handler set up for them. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
If VMIN and VTIME are both set to zero then the terminal performs non-blocking reads which means that read_key_without_echo() returns EOF if there is no key press pending. This results in the user being unable to select anything when running "git add -p". Fix this by explicitly setting VMIN and VTIME when enabling non-canonical mode. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
The builtin "add -p" reads the key "F2" as three separate keys "^[", "O" and "Q". The "Q" causes it to quit which is probably not what the user was expecting. This is because it uses poll() to check for pending input when reading escape sequences but reads the input with getchar() which is buffered by default and so hoovers up all the pending input leading poll() think there isn't anything pending. Fix this by calling setbuf() to disable input buffering if interactive.singlekey is set. Looking at the comment above mingw_getchar() in terminal.c I wonder if that function is papering over this bug and could be removed. Unfortunately I don't have access to windows to test that. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
phillipwood
force-pushed
the
wip/add-p-vmin-v2
branch
from
6d8423b
to
39b061a
Compare
|
/submit |
|
Submitted as pull.1146.v2.git.1645556015.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
This patch series was integrated into seen via git@a2c89e0. |
|
There was a status update in the "Cooking" section about the branch The single-key interactive operation used by "git add -p" has been made more robust. Will merge to 'next'? source: <pull.1146.v2.git.1645556015.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@91a83ab. |
|
There was a status update in the "Cooking" section about the branch The single-key interactive operation used by "git add -p" has been made more robust. Will merge to 'next'? source: <pull.1146.v2.git.1645556015.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@e5de67c. |
|
This patch series was integrated into seen via git@ef3d53d. |
|
There was a status update in the "Cooking" section about the branch The single-key interactive operation used by "git add -p" has been made more robust. Will merge to 'next'? source: <pull.1146.v2.git.1645556015.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@55febcf. |
|
This patch series was integrated into seen via git@a79eab9. |
|
This patch series was integrated into seen via git@c18fafa. |
|
This patch series was integrated into seen via git@65e035b. |
|
This patch series was integrated into seen via git@32f3ac2. |
|
This patch series was integrated into seen via git@3615c3d. |
|
This patch series was integrated into seen via git@4fbc4d8. |
|
This patch series was integrated into seen via git@25bd290. |
|
This patch series was integrated into next via git@02fd6ac. |
|
There was a status update in the "Cooking" section about the branch The single-key interactive operation used by "git add -p" has been made more robust. Will merge to 'master'. source: <pull.1146.v2.git.1645556015.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@64c0dbf. |
|
This patch series was integrated into seen via git@4cac652. |
|
This patch series was integrated into seen via git@96ea03b. |
|
There was a status update in the "Cooking" section about the branch The single-key interactive operation used by "git add -p" has been made more robust. Will merge to 'master'. source: <pull.1146.v2.git.1645556015.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@352c3f0. |
|
This patch series was integrated into seen via git@0efe6de. |
|
This patch series was integrated into seen via git@b77a4a5. |
|
This patch series was integrated into seen via git@cc1779e. |
|
This patch series was integrated into seen via git@214919b. |
|
This patch series was integrated into master via git@214919b. |
|
This patch series was integrated into next via git@214919b. |
|
Closed via 214919b. |
|
On the Git mailing list, Carlo Arenas wrote (reply to this): |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
On the Git mailing list, Phillip Wood wrote (reply to this): |
|
User |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I have added a new patch to the beginning of the series that fixes a case where we did not call restore_term() when leaving read_key_without_echo(). I have also reworded the commit message to patch 2 as SIGINT is actually ignored while the editor is running (we should probably change that code to use wait_after_clean instead).
Cover letter for V1:
Fix a couple of bugs I noticed when using the builtin "add -p" with interactive.singlekey=true. The first patch is a general fix for the terminal save/restore functionality which forgot to call sigchain_pop() when it restored the terminal. The last two fix reading single keys in non-canonical mode by making sure we wait for an initial key press and only read one byte at a time from the underlying file descriptor.
Note that these are untested on windows beyond our CI compiling them
Cc: Johannes Schindelin Johannes.Schindelin@gmx.de
Cc: Carlo Marcelo Arenas Belón carenas@gmail.com
cc: Phillip Wood phillip.wood123@gmail.com