| Message ID | 4e2bc660-ee33-4641-aca5-783d0cefcd23@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | add-patch: response to invalid option | expand |
On Mon, Apr 15, 2024 at 09:00:28PM +0200, Rubén Justo wrote: > When the user introduces an invalid option, we respond with the whole > help text. > > Instead of displaying the long help description, display a short error > message indicating the incorrectly introduced option with a note on how > to get the help text. > > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- > add-patch.c | 5 ++++- > t/t3701-add-interactive.sh | 10 ++++++++++ > 2 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/add-patch.c b/add-patch.c > index a06dd18985..c77902fec5 100644 > --- a/add-patch.c > +++ b/add-patch.c > @@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s, > } > } else if (s->answer.buf[0] == 'p') { > rendered_hunk_index = -1; > - } else { > + } else if (s->answer.buf[0] == '?') { > const char *p = _(help_patch_remainder), *eol = p; > > color_fprintf(stdout, s->s.help_color, "%s", I think it would've made sense to describe this change in the commit message, as well. Currently, the reader is left wondering whether "?" is a new shortcut that you introduce in this patch or whether it is already documented as such. > @@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s, > color_fprintf_ln(stdout, s->s.help_color, > "%.*s", (int)(eol - p), p); > } > + } else { > + err(s, _("Unknown option '%s' (use '?' for help)"), > + s->answer.buf); > } > } > I was wondering why we have `err()` here, and whether we shouldn't convert this to use `error()` instead. Similarly, I was wondering whether we should convert the error message to start with a lower-case letter to match our other errors. But scanning through "add-patch.c" I don't think that makes sense. `err()` knows to handle colors, which `error()` doesn't. And given that this is an interactive interface where all the other error messages start with an upper-case letter, too, it would feel out of place to adapt the error message. So all of this is just to say that this looks sensible to me. > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > index bc55255b0a..b38fd5388a 100755 > --- a/t/t3701-add-interactive.sh > +++ b/t/t3701-add-interactive.sh > @@ -61,6 +61,16 @@ test_expect_success 'setup (initial)' ' > echo more >>file && > echo lines >>file > ' > + > +test_expect_success 'invalid option' ' > + cat >expect <<-EOF && > + Unknown option ${SQ}W${SQ} (use ${SQ}?${SQ} for help) > + EOF > + test_write_lines W | > + git -c core.filemode=true add -p 2>actual && Nit: it might be sensible to write the lines into a temporary file first so that Git isn't spawned as part of a pipeline. But on the other hand it's fine to have Git on the right-hand side of pipelines, so this way to write it is fine, too. Patrick > + test_cmp expect actual > +' > + > test_expect_success 'status works (initial)' ' > git add -i </dev/null >output && > grep "+1/-0 *+2/-0 file" output > -- > 2.44.0.782.g480309b2c8 > > >
Hi Rubén Thanks for working on this, it is a nice follow up to your last series. On 15/04/2024 20:00, Rubén Justo wrote: > > + } else { > + err(s, _("Unknown option '%s' (use '?' for help)"), As this is an interactive program I think "Unknown key" would be clearer. > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > index bc55255b0a..b38fd5388a 100755 > --- a/t/t3701-add-interactive.sh > +++ b/t/t3701-add-interactive.sh > @@ -61,6 +61,16 @@ test_expect_success 'setup (initial)' ' > echo more >>file && > echo lines >>file > ' > + > +test_expect_success 'invalid option' ' > + cat >expect <<-EOF && > + Unknown option ${SQ}W${SQ} (use ${SQ}?${SQ} for help) > + EOF > + test_write_lines W | > + git -c core.filemode=true add -p 2>actual && > + test_cmp expect actual > +' I was confused by this test as "add -p" doesn't seem to be printing anything apart from the error. That's because it only captures stderr (quite why an interactive program is writing its some of its output to stderr and the rest to stdout is another question). I think we want to capture the whole output otherwise we can't assert that the help isn't printed. Something like this (which also adds coverage for '?' and 'p') diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index bc55255b0a8..0fc7d4b5d89 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -1130,4 +1130,26 @@ test_expect_success 'reset -p with unmerged files' ' test_must_be_empty staged ' +test_expect_success 'invalid key' ' + echo changed >file && + test_write_lines æ \? p q | force_color git add -p >actual.colored 2>&1 && + test_decode_color <actual.colored >actual && + force_color git diff >diff.colored && + test_decode_color <diff.colored >diff.decoded && + cat diff.decoded >expect && + cat >>expect <<-EOF && + <BOLD;BLUE>(1/1) Stage this hunk [y,n,q,a,d,e,p,?]? <RESET><BOLD;RED>Unknown key ${SQ}æ${SQ}. Use ${SQ}?${SQ} for help<RESET> + <BOLD;BLUE>(1/1) Stage this hunk [y,n,q,a,d,e,p,?]? <RESET><BOLD;RED>y - stage this hunk + n - do not stage this hunk + q - quit; do not stage this hunk or any of the remaining ones + a - stage this hunk and all later hunks in the file + d - do not stage this hunk or any of the later hunks in the file + <RESET><BOLD;RED>e - manually edit the current hunk<RESET> + <BOLD;RED>p - print the current hunk<RESET> + <BOLD;RED>? - print help<RESET> + <BOLD;BLUE>(1/1) Stage this hunk [y,n,q,a,d,e,p,?]? <RESET>$(sed -n "/@/,\$ p" diff.decoded) + <BOLD;BLUE>(1/1) Stage this hunk [y,n,q,a,d,e,p,?]? <RESET> + EOF + test_cmp expect actual +' + test_done Best Wishes Phillip
Rubén Justo <rjusto@gmail.com> writes: > When the user introduces an invalid option, we respond with the whole > help text. The verb "introduces" in the first sentence looked weird to me. add -p: require two steps to get help after mistyping an option During a "git add -p" session, if the user chooses an option that is not offered, the help text for the entire available choices is given. or something, perhaps. > Instead of displaying the long help description, display a short error > message indicating the incorrectly introduced option with a note on how > to get the help text. When you wanted to 'q'uit but touched the 'w' key that sits next to it by mistake, you do not need to be told about what any of the options would do; you may want to be told "the option to quit is 'q', not 'w'", though ;-). On the other hand, if you are truly lost and do not know what each of the listed choices mean, you'd type '?' anyway because that is one of the offered choices. So the only change needed here is to make sure that '?' is the only thing that gives the help message, and all other unrecognised option 'x' are made to say "we do not know 'x'". That flow of thought makes sort-of sense, if the choices that are offered are too numerous (say, around a dozen or more), but with the current command set, I am not sure if this change is an improvement (note: I did not say "I do not think that"---I simply am not sure). If we implemented the UI this way 20 years ago in the first version, perhaps we would have had happily been using it since, but given that the way we implemented the UI 20 years ago has been used happily by our users without much complaint, either way must be just fine. Is it worth changing it at this point? Does it improve the end-user experience in any noticeable way? I do not think I can answer these two questions with confident "yes". > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- > add-patch.c | 5 ++++- > t/t3701-add-interactive.sh | 10 ++++++++++ > 2 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/add-patch.c b/add-patch.c > index a06dd18985..c77902fec5 100644 > --- a/add-patch.c > +++ b/add-patch.c > @@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s, > } > } else if (s->answer.buf[0] == 'p') { > rendered_hunk_index = -1; > - } else { > + } else if (s->answer.buf[0] == '?') { > const char *p = _(help_patch_remainder), *eol = p; > > color_fprintf(stdout, s->s.help_color, "%s", > @@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s, > color_fprintf_ln(stdout, s->s.help_color, > "%.*s", (int)(eol - p), p); > } > + } else { > + err(s, _("Unknown option '%s' (use '?' for help)"), > + s->answer.buf); > } > } > > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > index bc55255b0a..b38fd5388a 100755 > --- a/t/t3701-add-interactive.sh > +++ b/t/t3701-add-interactive.sh > @@ -61,6 +61,16 @@ test_expect_success 'setup (initial)' ' > echo more >>file && > echo lines >>file > ' > + > +test_expect_success 'invalid option' ' > + cat >expect <<-EOF && > + Unknown option ${SQ}W${SQ} (use ${SQ}?${SQ} for help) > + EOF > + test_write_lines W | > + git -c core.filemode=true add -p 2>actual && > + test_cmp expect actual > +' > + > test_expect_success 'status works (initial)' ' > git add -i </dev/null >output && > grep "+1/-0 *+2/-0 file" output
Hi Junio On 16/04/2024 11:26, Junio C Hamano wrote: > Rubén Justo <rjusto@gmail.com> writes: > >> When the user introduces an invalid option, we respond with the whole >> help text. > > The verb "introduces" in the first sentence looked weird to me. > > add -p: require two steps to get help after mistyping an option > > During a "git add -p" session, if the user chooses an option > that is not offered, the help text for the entire available > choices is given. I find this suggestion clearer as well. > [...] > On the other hand, if you are truly lost and do not know what each > of the listed choices mean, you'd type '?' anyway because that is > one of the offered choices. So the only change needed here is to > make sure that '?' is the only thing that gives the help message, > and all other unrecognised option 'x' are made to say "we do not > know 'x'". > > That flow of thought makes sort-of sense, if the choices that are > offered are too numerous (say, around a dozen or more), but with the > current command set, I am not sure if this change is an improvement > (note: I did not say "I do not think that"---I simply am not sure). The complete list of help is 15 lines long (we don't always print everything but if you're in the middle of staging several hunks from the same file we do) y - stage this hunk n - do not stage this hunk q - quit; do not stage this hunk or any of the remaining ones a - stage this hunk and all later hunks in the file d - do not stage this hunk or any of the later hunks in the file j - leave this hunk undecided, see next undecided hunk J - leave this hunk undecided, see next hunk k - leave this hunk undecided, see previous undecided hunk K - leave this hunk undecided, see previous hunk g - select a hunk to go to / - search for a hunk matching the given regex s - split the current hunk into smaller hunks e - manually edit the current hunk p - print the current hunk ? - print help I find it long enough to be annoying as it means there is a significant distance between the prompt and the hunk text. > If we implemented the UI this way 20 years ago in the first version, > perhaps we would have had happily been using it since, but given > that the way we implemented the UI 20 years ago has been used > happily by our users without much complaint, either way must be just > fine. > > Is it worth changing it at this point? Does it improve the end-user > experience in any noticeable way? I do not think I can answer these > two questions with confident "yes". Personally I think it would be an improvement but I suspect it is a matter of opinion. Best Wishes Phillip >> Signed-off-by: Rubén Justo <rjusto@gmail.com> >> --- >> add-patch.c | 5 ++++- >> t/t3701-add-interactive.sh | 10 ++++++++++ >> 2 files changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/add-patch.c b/add-patch.c >> index a06dd18985..c77902fec5 100644 >> --- a/add-patch.c >> +++ b/add-patch.c >> @@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s, >> } >> } else if (s->answer.buf[0] == 'p') { >> rendered_hunk_index = -1; >> - } else { >> + } else if (s->answer.buf[0] == '?') { >> const char *p = _(help_patch_remainder), *eol = p; >> >> color_fprintf(stdout, s->s.help_color, "%s", >> @@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s, >> color_fprintf_ln(stdout, s->s.help_color, >> "%.*s", (int)(eol - p), p); >> } >> + } else { >> + err(s, _("Unknown option '%s' (use '?' for help)"), >> + s->answer.buf); >> } >> } >> >> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh >> index bc55255b0a..b38fd5388a 100755 >> --- a/t/t3701-add-interactive.sh >> +++ b/t/t3701-add-interactive.sh >> @@ -61,6 +61,16 @@ test_expect_success 'setup (initial)' ' >> echo more >>file && >> echo lines >>file >> ' >> + >> +test_expect_success 'invalid option' ' >> + cat >expect <<-EOF && >> + Unknown option ${SQ}W${SQ} (use ${SQ}?${SQ} for help) >> + EOF >> + test_write_lines W | >> + git -c core.filemode=true add -p 2>actual && >> + test_cmp expect actual >> +' >> + >> test_expect_success 'status works (initial)' ' >> git add -i </dev/null >output && >> grep "+1/-0 *+2/-0 file" output >
Phillip Wood <phillip.wood123@gmail.com> writes: >> That flow of thought makes sort-of sense, if the choices that are >> offered are too numerous (say, around a dozen or more), but with the >> current command set, I am not sure if this change is an improvement >> (note: I did not say "I do not think that"---I simply am not sure). > > The complete list of help is 15 lines long (we don't always print > everything but if you're in the middle of staging several hunks from > the same file we do) Ah, OK. Even though I was the one who stole this feature from elsewhere, I forgot that we had that many commands, and it makes the decision a lot simpler ;-) > y - stage this hunk > n - do not stage this hunk > q - quit; do not stage this hunk or any of the remaining ones > a - stage this hunk and all later hunks in the file > d - do not stage this hunk or any of the later hunks in the file > j - leave this hunk undecided, see next undecided hunk > J - leave this hunk undecided, see next hunk > k - leave this hunk undecided, see previous undecided hunk > K - leave this hunk undecided, see previous hunk > g - select a hunk to go to > / - search for a hunk matching the given regex > s - split the current hunk into smaller hunks > e - manually edit the current hunk > p - print the current hunk > ? - print help > > I find it long enough to be annoying as it means there is a > significant distance between the prompt and the hunk text. Yes, I now agree. So the log message may want a bit of rewrite in the title and the introductory part, with a clarification that '?' is what we already had and not what this adds (from Patrick's comment), and there may be other small improvements we want to see that I may have missed, but other than these small-ish points, the basic direction of this patch is good? This is a tangent, but I think we _mostly_ take these commands case insensitively (e.g., you can say 'Q' to quit the program, as well as 'q' that is listed above). But 'k' and 'j' (there may be others) act differently when given in the uppercase. This would limit our ability to later add capitalized variant of an already used lower case letter without retraining users---should we find it disturbing, or is "add -p" mostly done and we do not have to worry? Thanks.
On 16/04/2024 16:22, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > >>> That flow of thought makes sort-of sense, if the choices that are >>> offered are too numerous (say, around a dozen or more), but with the >>> current command set, I am not sure if this change is an improvement >>> (note: I did not say "I do not think that"---I simply am not sure). >> >> The complete list of help is 15 lines long (we don't always print >> everything but if you're in the middle of staging several hunks from >> the same file we do) > > Ah, OK. Even though I was the one who stole this feature from > elsewhere, I forgot that we had that many commands, and it makes the > decision a lot simpler ;-) Yes, it always surprises me how many there are when the help is printed. > So the log message may want a bit of rewrite in the title and the > introductory part, with a clarification that '?' is what we already > had and not what this adds (from Patrick's comment), and there may > be other small improvements we want to see that I may have missed, > but other than these small-ish points, the basic direction of this > patch is good? I think so. > This is a tangent, but I think we _mostly_ take these commands case > insensitively (e.g., you can say 'Q' to quit the program, as well as > 'q' that is listed above). But 'k' and 'j' (there may be others) > act differently when given in the uppercase. 'J' and 'K' are the only keys where we have different actions for upper and lowers cases. The implementation looks a bit confused though - we treat y,n,q,a,d case insensitively but not g,s,e,p. As no one has complained about the latter we should perhaps leave them as is in case we ever want to add upper case variants. > This would limit our > ability to later add capitalized variant of an already used lower > case letter without retraining users---should we find it disturbing, > or is "add -p" mostly done and we do not have to worry? I think its mostly done. I'd like a way of selecting lines from a hunk that does not involve making the user edit the hunk and I wish the search remembered the last string but I don't think we're in danger of running out of keys for new commands. Best Wishes Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: >> Ah, OK. Even though I was the one who stole this feature from >> elsewhere, I forgot that we had that many commands, and it makes the >> decision a lot simpler ;-) > > Yes, it always surprises me how many there are when the help is printed. > >> So the log message may want a bit of rewrite in the title and the >> introductory part, with a clarification that '?' is what we already >> had and not what this adds (from Patrick's comment), and there may >> be other small improvements we want to see that I may have missed, >> but other than these small-ish points, the basic direction of this >> patch is good? > > I think so. OK, let me mark the topic as expecting hopefully a final reroll in the draft "What's cooking" report. Thanks.
On Tue, Apr 16, 2024 at 07:51:35AM +0200, Patrick Steinhardt wrote: > I think it would've made sense to describe this change in the commit > message, as well. Currently, the reader is left wondering whether "?" is > a new shortcut that you introduce in this patch or whether it is already > documented as such. Perhaps, something like?: Instead of displaying the long help description, display a short error message indicating the incorrectly introduced option with a note on how to use the '?' option, to get the help text. Thanks.
On Tue, Apr 16, 2024 at 10:41:32AM +0100, phillip.wood123@gmail.com wrote: > Thanks for working on this, it is a nice follow up to your last series. Thank you for your interest! > > On 15/04/2024 20:00, Rubén Justo wrote: > > > > + } else { > > + err(s, _("Unknown option '%s' (use '?' for help)"), > > As this is an interactive program I think "Unknown key" would be clearer. Maybe you have "interactive.singleKey" set to "true"? Perhaps "key" refers more to the key pressed by the user. My impression is that "option" have a clearer and wider audience. > Something like this (which also adds coverage for '?' and 'p') I know we don't have a test for the help. I noticed this in my other series. And I decided not to include a test for it then. Maybe in this series it makes sense. However, I would prefer to keep a test only on the new error message. Thanks.
On Tue, Apr 16, 2024 at 03:26:33AM -0700, Junio C Hamano wrote: > > When the user introduces an invalid option, we respond with the whole > > help text. > > The verb "introduces" in the first sentence looked weird to me. OK. I'll reword it. > > Instead of displaying the long help description, display a short error > > message indicating the incorrectly introduced option with a note on how > > to get the help text. > Is it worth changing it at this point? Does it improve the end-user > experience in any noticeable way? I do not think I can answer these > two questions with confident "yes". Indeed, this has been with us for a long time. We're not fixing or changing any normal usage here. I know you know, but let me put it this way; We are introducing two changes here: - a new error message to inform the user that an invalid option has been entered. And, - not displaying the help if not requested. Both are improvements to the user experience, but especially the former, I think. This: $ echo W | git add -p diff --git a/add-interactive.c b/add-interactive.c [...] (1/1) Stage this hunk [y,n,q,a,d,e,?]? y - stage this hunk n - do not stage this hunk q - quit; do not stage this hunk or any of the remaining ones a - stage this hunk and all later hunks in the file [...] Becomes: $ echo W | ./git add -p diff --git a/add-interactive.c b/add-interactive.c [...] (1/1) Stage this hunk [y,n,q,a,d,e,p,?]? Unknown option 'W' (use '?' for help) Thanks.
Hi Rubén On 16/04/2024 20:24, Rubén Justo wrote: > On Tue, Apr 16, 2024 at 10:41:32AM +0100, phillip.wood123@gmail.com wrote: >> On 15/04/2024 20:00, Rubén Justo wrote: >>> >>> + } else { >>> + err(s, _("Unknown option '%s' (use '?' for help)"), >> >> As this is an interactive program I think "Unknown key" would be clearer. > > Maybe you have "interactive.singleKey" set to "true"? > > Perhaps "key" refers more to the key pressed by the user. My impression > is that "option" have a clearer and wider audience. I tend to associate "option" with a command-line argument, not interactive input to a program. >> Something like this (which also adds coverage for '?' and 'p') > > I know we don't have a test for the help. I noticed this in my other > series. And I decided not to include a test for it then. Maybe in this > series it makes sense. > > However, I would prefer to keep a test only on the new error message. Why? This commit makes three changes - it stops printing the help when the user presses an unknown key - it starts treating '?' differently to an unknown key - it starts printing an error message when the user presses an unknown key The test you are proposing only tests the last of these changes. We should be aiming to write tests that (a) verify all of the changes introduced by a commit (b) are likely to detect regressions to those changes (c) are reasonably efficient, for example if it is possible to test more than one key with a single "add -p" process we should do so. As this is an interactive program I have a strong preference for testing what the user sees printed to their screen, not just what happens to come out on stderr. Thanks Phillip
phillip.wood123@gmail.com writes: > I tend to associate "option" with a command-line argument, not > interactive input to a program. "git add --help" is a bit mixed. The choices offered by "git add -i" are called "subcommand" (see "INTERACTIVE MODE" section), but the choices you give to the prompt "patch" subcommand gives you are presented with "You can select one of the following options and type return". So "option" is not too wrong, even though it is a word used in other contexts as well. I am OK with "option", but if I were adding this new error message, I probably would have said "unknown command". In any case, whether you said option, command, or key , it is so obvious from the context that we could even say "error: 'W' not known, use '?' for help" without any noun there, so it would not matter too much which noun you pick. I'd still avoid "key", though, because to those who do not do single-key input, myself included, it does not match their user experience, and it is even more so if they forgot or do not even know that they could choose to use single-key input. > The test you are proposing only tests the last of these changes. We > should be aiming to write tests that (a) verify all of the changes > introduced by a commit (b) are likely to detect regressions to those > changes (c) are reasonably efficient, for example if it is possible to > test more than one key with a single "add -p" process we should do > so. As this is an interactive program I have a strong preference for > testing what the user sees printed to their screen, not just what > happens to come out on stderr. I do agree with these three points, but I do not have a strong opinion on the new test that was added by the patch when judging with them used as a yardstick. Thanks.
On 17/04/2024 16:05, Junio C Hamano wrote: > phillip.wood123@gmail.com writes: > >> I tend to associate "option" with a command-line argument, not >> interactive input to a program. > > "git add --help" is a bit mixed. The choices offered by "git add > -i" are called "subcommand" (see "INTERACTIVE MODE" section), but > the choices you give to the prompt "patch" subcommand gives you are > presented with "You can select one of the following options and type > return". So "option" is not too wrong, even though it is a word > used in other contexts as well. I am OK with "option", but if I > were adding this new error message, I probably would have said > "unknown command". I think "unknown command" is a good suggestion, I take your point about "unknown key" not being so clear for users who do not use single-key input. Best Wishes Phillip > In any case, whether you said option, command, or key , it is so > obvious from the context that we could even say "error: 'W' not > known, use '?' for help" without any noun there, so it would not > matter too much which noun you pick. > > I'd still avoid "key", though, because to those who do not do > single-key input, myself included, it does not match their user > experience, and it is even more so if they forgot or do not even > know that they could choose to use single-key input. > >> The test you are proposing only tests the last of these changes. We >> should be aiming to write tests that (a) verify all of the changes >> introduced by a commit (b) are likely to detect regressions to those >> changes (c) are reasonably efficient, for example if it is possible to >> test more than one key with a single "add -p" process we should do >> so. As this is an interactive program I have a strong preference for >> testing what the user sees printed to their screen, not just what >> happens to come out on stderr. > > I do agree with these three points, but I do not have a strong > opinion on the new test that was added by the patch when judging > with them used as a yardstick. > > Thanks.
diff --git a/add-patch.c b/add-patch.c index a06dd18985..c77902fec5 100644 --- a/add-patch.c +++ b/add-patch.c @@ -1667,7 +1667,7 @@ static int patch_update_file(struct add_p_state *s, } } else if (s->answer.buf[0] == 'p') { rendered_hunk_index = -1; - } else { + } else if (s->answer.buf[0] == '?') { const char *p = _(help_patch_remainder), *eol = p; color_fprintf(stdout, s->s.help_color, "%s", @@ -1691,6 +1691,9 @@ static int patch_update_file(struct add_p_state *s, color_fprintf_ln(stdout, s->s.help_color, "%.*s", (int)(eol - p), p); } + } else { + err(s, _("Unknown option '%s' (use '?' for help)"), + s->answer.buf); } } diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index bc55255b0a..b38fd5388a 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -61,6 +61,16 @@ test_expect_success 'setup (initial)' ' echo more >>file && echo lines >>file ' + +test_expect_success 'invalid option' ' + cat >expect <<-EOF && + Unknown option ${SQ}W${SQ} (use ${SQ}?${SQ} for help) + EOF + test_write_lines W | + git -c core.filemode=true add -p 2>actual && + test_cmp expect actual +' + test_expect_success 'status works (initial)' ' git add -i </dev/null >output && grep "+1/-0 *+2/-0 file" output
When the user introduces an invalid option, we respond with the whole help text. Instead of displaying the long help description, display a short error message indicating the incorrectly introduced option with a note on how to get the help text. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- add-patch.c | 5 ++++- t/t3701-add-interactive.sh | 10 ++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-)