| Message ID | xmqqh6eqiwgf.fsf@gitster.g (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] add-patch: enforce only one-letter response to prompts | expand |
On Tue, May 21, 2024 at 7:20 PM Junio C Hamano <gitster@pobox.com> wrote: > In an "git add -p" session, especially when we are not using the > single-char mode, we may see 'qa' as a response to a prompt > > (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? > > and then just do the 'q' thing (i.e. quit the session), ignoring > everything other than the first byte. > > If 'q' and 'a' are next to each other on the user's keyboard, there > is a plausible chance that we see 'qa' when the user who wanted to > say 'a' fat-fingered and we ended up doing the 'q' thing instead. > > As we didn't think of a good reason during the review discussion why > we want to accept excess letters only to ignore them, it appears to > be a safe change to simply reject input that is longer than just one > byte. > > The two exceptions are the 'g' command that takes a hunk number, and > the '/' command that takes a regular expression. They has to be s/has/have/ > accompanied by their operands (this makes me wonder how users who > set the interactive.singlekey configuration feed these operands---it > turns out that we notice there is no operand and give them another > chance to type the operand separately, without using single key > input this time), so we accept a string that is more than one byte > long. > > Keep the "use only the first byte, downcased" behaviour when we ask > yes/no question, though. Neither on Qwerty or on Dvorak, 'y' and > 'n' are not close to each other. > > Signed-off-by: Junio C Hamano <gitster@pobox.com>
Eric Sunshine <sunshine@sunshineco.com> writes: >> The two exceptions are the 'g' command that takes a hunk number, and >> the '/' command that takes a regular expression. They has to be > > s/has/have/ Thanks for a typofix. Input to possibly update this part ... >> As we didn't think of a good reason during the review discussion why >> we want to accept excess letters only to ignore them,... ... of the proposed log message, or convince us that it is not such a great idea, is also welcome. Thanks. Will queue but keep out of 'next' for a few more days.
Hello Junio, Please see my comments below. On 2024-05-22 01:20, Junio C Hamano wrote: > In an "git add -p" session, especially when we are not using the s/In an/In a/ > single-char mode, we may see 'qa' as a response to a prompt Perhaps s/single-char/single-character/ > > (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? > > and then just do the 'q' thing (i.e. quit the session), ignoring > everything other than the first byte. > > If 'q' and 'a' are next to each other on the user's keyboard, there > is a plausible chance that we see 'qa' when the user who wanted to > say 'a' fat-fingered and we ended up doing the 'q' thing instead. > > As we didn't think of a good reason during the review discussion why > we want to accept excess letters only to ignore them, it appears to > be a safe change to simply reject input that is longer than just one > byte. > > The two exceptions are the 'g' command that takes a hunk number, and > the '/' command that takes a regular expression. They has to be > accompanied by their operands (this makes me wonder how users who > set the interactive.singlekey configuration feed these operands---it > turns out that we notice there is no operand and give them another > chance to type the operand separately, without using single key > input this time), so we accept a string that is more than one byte > long. > > Keep the "use only the first byte, downcased" behaviour when we ask > yes/no question, though. Neither on Qwerty or on Dvorak, 'y' and > 'n' are not close to each other. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > * This version fixes the breakage in t3701 where we exercise the > '/' command. Further code inspection reveals that 'g' also needs > to be special cased. > > The previous iteration was <xmqqr0dvb1sh.fsf_-_@gitster.g>. > > add-patch.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/add-patch.c b/add-patch.c > index 79eda168eb..a6c3367d59 100644 > --- a/add-patch.c > +++ b/add-patch.c > @@ -1228,6 +1228,7 @@ static int prompt_yesno(struct add_p_state *s, > const char *prompt) > fflush(stdout); > if (read_single_character(s) == EOF) > return -1; > + /* do not limit to 1-byte input to allow 'no' etc. */ > switch (tolower(s->answer.buf[0])) { > case 'n': return 0; > case 'y': return 1; > @@ -1506,6 +1507,12 @@ static int patch_update_file(struct add_p_state > *s, > if (!s->answer.len) > continue; > ch = tolower(s->answer.buf[0]); > + > + /* 'g' takes a hunk number, '/' takes a regexp */ > + if (1 < s->answer.len && (ch != 'g' && ch != '/')) { To me, "s->answer.len > 1" would be much more readable, and I was surprised a bit to see the flipped variant. This made me curious; would you, please, let me know why do you prefer this form? > + error(_("only one letter is expected, got '%s'"), s->answer.buf); > + continue; > + } > if (ch == 'y') { > hunk->use = USE_HUNK; > soft_increment: The patch is looking good to me, and I find it good that it improves the strictness of the user input, which should also improve the overall user experience.
On Tue, May 21, 2024 at 04:20:16PM -0700, Junio C Hamano wrote: > In an "git add -p" session, especially when we are not using the > single-char mode, we may see 'qa' as a response to a prompt > > (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? > > and then just do the 'q' thing (i.e. quit the session), ignoring > everything other than the first byte. > > If 'q' and 'a' are next to each other on the user's keyboard, there > is a plausible chance that we see 'qa' when the user who wanted to > say 'a' fat-fingered and we ended up doing the 'q' thing instead. I think it's a good idea regardless of the layout. There are tons of layouts out there that are very esoteric (I for one use NEO2, which most nobody has ever heard of), and I'm sure you will find at least one layout where characters are positioned such that you can fat finger things. Another argument that is independent of fat fingering is that it potentially allows us to expand this feature with multi-byte verbs going forward. [snip] > Keep the "use only the first byte, downcased" behaviour when we ask > yes/no question, though. Neither on Qwerty or on Dvorak, 'y' and > 'n' are not close to each other. Just to prove my point: Workman layout has them right next to each other :) What we make of that information is a different question though. > diff --git a/add-patch.c b/add-patch.c > index 79eda168eb..a6c3367d59 100644 > --- a/add-patch.c > +++ b/add-patch.c > @@ -1228,6 +1228,7 @@ static int prompt_yesno(struct add_p_state *s, const char *prompt) > fflush(stdout); > if (read_single_character(s) == EOF) > return -1; > + /* do not limit to 1-byte input to allow 'no' etc. */ > switch (tolower(s->answer.buf[0])) { > case 'n': return 0; > case 'y': return 1; > @@ -1506,6 +1507,12 @@ static int patch_update_file(struct add_p_state *s, > if (!s->answer.len) > continue; > ch = tolower(s->answer.buf[0]); > + > + /* 'g' takes a hunk number, '/' takes a regexp */ > + if (1 < s->answer.len && (ch != 'g' && ch != '/')) { I find this condition a bit unusual and thus hard to read. If it instead said `s->answer.len != 1` then it would be way easier to comprehend. Also, none of the branches othar than for 'g' and '/' use `s->answer`, so this should be safe. I also very much agree with the general idea of this patch. > + error(_("only one letter is expected, got '%s'"), s->answer.buf); > + continue; > + } > if (ch == 'y') { > hunk->use = USE_HUNK; > soft_increment: I assume we also want a test for this new behaviour, right? Patrick
Dragan Simic <dsimic@manjaro.org> writes: > Hello Junio, > > Please see my comments below. > > On 2024-05-22 01:20, Junio C Hamano wrote: >> In an "git add -p" session, especially when we are not using the > > s/In an/In a/ Good eyes. > >> single-char mode, we may see 'qa' as a response to a prompt > > Perhaps s/single-char/single-character/ I shouldn't have been loose in the language. Rather, we should say "single key mode", as the knob to control the feature is the "interactive.singlekey" variable. >> + /* 'g' takes a hunk number, '/' takes a regexp */ >> + if (1 < s->answer.len && (ch != 'g' && ch != '/')) { > > To me, "s->answer.len > 1" would be much more readable, and > I was surprised a bit to see the flipped variant. This made > me curious; would you, please, let me know why do you prefer > this form? "textual order should reflect actual order" (read CodingGuidelines). For more backstory, https://lore.kernel.org/git/?q=%22textual+order%22+%22actual+order%22 Thanks.
Patrick Steinhardt <ps@pks.im> writes: >> + /* 'g' takes a hunk number, '/' takes a regexp */ >> + if (1 < s->answer.len && (ch != 'g' && ch != '/')) { > > I find this condition a bit unusual and thus hard to read. If it instead > said `s->answer.len != 1` then it would be way easier to comprehend. We have already eliminated the "it is 0" case, .len cannot be negative, and the case we really care about is "is it not just one?", so I agree with you that the inequality comparison with 1 is easier to grok. > I assume we also want a test for this new behaviour, right? Hmph, yeah. 'g' has already been tested (that was what led me to do the v2), but we probably should do 'qa' or something. Thanks.
On 2024-05-22 18:23, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> writes: >> On 2024-05-22 01:20, Junio C Hamano wrote: >>> single-char mode, we may see 'qa' as a response to a prompt >> >> Perhaps s/single-char/single-character/ > > I shouldn't have been loose in the language. Rather, we should say > "single key mode", as the knob to control the feature is the > "interactive.singlekey" variable. Yes, "single-key mode" is better; "when interactive.singlekey is not enabled" may be even a bit better. Not worth a reroll, of course. >>> + /* 'g' takes a hunk number, '/' takes a regexp */ >>> + if (1 < s->answer.len && (ch != 'g' && ch != '/')) { >> >> To me, "s->answer.len > 1" would be much more readable, and >> I was surprised a bit to see the flipped variant. This made >> me curious; would you, please, let me know why do you prefer >> this form? > > "textual order should reflect actual order" (read CodingGuidelines). > > For more backstory, > > > https://lore.kernel.org/git/?q=%22textual+order%22+%22actual+order%22 That's exactly what I assumed, but frankly, in this particular case I really can't force myself, despite trying quite hard, into liking it. It's simply strange to me.
Dragan Simic <dsimic@manjaro.org> writes: >> For more backstory, >> https://lore.kernel.org/git/?q=%22textual+order%22+%22actual+order%22 > > That's exactly what I assumed, but frankly, in this particular case > I really can't force myself, despite trying quite hard, into liking > it. It's simply strange to me. You asked me why, and the reason was given to you. End of story. I never asked you to like it and you do not have to like it ;-).
diff --git a/add-patch.c b/add-patch.c index 79eda168eb..a6c3367d59 100644 --- a/add-patch.c +++ b/add-patch.c @@ -1228,6 +1228,7 @@ static int prompt_yesno(struct add_p_state *s, const char *prompt) fflush(stdout); if (read_single_character(s) == EOF) return -1; + /* do not limit to 1-byte input to allow 'no' etc. */ switch (tolower(s->answer.buf[0])) { case 'n': return 0; case 'y': return 1; @@ -1506,6 +1507,12 @@ static int patch_update_file(struct add_p_state *s, if (!s->answer.len) continue; ch = tolower(s->answer.buf[0]); + + /* 'g' takes a hunk number, '/' takes a regexp */ + if (1 < s->answer.len && (ch != 'g' && ch != '/')) { + error(_("only one letter is expected, got '%s'"), s->answer.buf); + continue; + } if (ch == 'y') { hunk->use = USE_HUNK; soft_increment:
In an "git add -p" session, especially when we are not using the single-char mode, we may see 'qa' as a response to a prompt (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? and then just do the 'q' thing (i.e. quit the session), ignoring everything other than the first byte. If 'q' and 'a' are next to each other on the user's keyboard, there is a plausible chance that we see 'qa' when the user who wanted to say 'a' fat-fingered and we ended up doing the 'q' thing instead. As we didn't think of a good reason during the review discussion why we want to accept excess letters only to ignore them, it appears to be a safe change to simply reject input that is longer than just one byte. The two exceptions are the 'g' command that takes a hunk number, and the '/' command that takes a regular expression. They has to be accompanied by their operands (this makes me wonder how users who set the interactive.singlekey configuration feed these operands---it turns out that we notice there is no operand and give them another chance to type the operand separately, without using single key input this time), so we accept a string that is more than one byte long. Keep the "use only the first byte, downcased" behaviour when we ask yes/no question, though. Neither on Qwerty or on Dvorak, 'y' and 'n' are not close to each other. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * This version fixes the breakage in t3701 where we exercise the '/' command. Further code inspection reveals that 'g' also needs to be special cased. The previous iteration was <xmqqr0dvb1sh.fsf_-_@gitster.g>. add-patch.c | 7 +++++++ 1 file changed, 7 insertions(+)