Re: [PATCH] add-patch: response to invalid option
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Apr 16, 2024, 05:51 UTC
- Message-ID
- <Zh4R5yonRLBcunCR@tanuki>
- In-Reply-To
- <4e2bc660-ee33-4641-aca5-783d0cefcd23@gmail.com>
On Mon, Apr 15, 2024 at 09:00:28PM +0200, Rubén Justo wrote:
Show 26 quoted lines
> 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.
Show 10 quoted lines
> @@ -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.
Show 15 quoted lines
> 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
Show 11 quoted lines
> + 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 > > >