On Fri, Sep 30 2022, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>
>> Improve the error message emitted when there's a bad -L argument, and
>> do so using the parse-options.c flavor of "usage()", instead of using
>> the non-parse-options.c usage() function. This was the last user of
>> usage() in this file.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
>> ---
>> builtin/blame.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> This may not be incorrect (I didn't spend time to see if this "while
> at it" is truly an improvement) but clearly outside the scome of
> "output from the -h option should match synopsis" theme.
I'll eject this & rewrite a subsequent commit, as a re-roll will make
clear the choice was between "convert this last user of the API" early
on, or in a later commit have the "-h" output be correct for both
usage() and usage_with_options()>
>> diff --git a/builtin/blame.c b/builtin/blame.c
>> index a9fe8cf7a68..8ec59fa2096 100644
>> --- a/builtin/blame.c
>> +++ b/builtin/blame.c
>> @@ -1108,12 +1108,13 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>> anchor = 1;
>> range_set_init(&ranges, range_list.nr);
>> for (range_i = 0; range_i < range_list.nr; ++range_i) {
>> + const char *arg = range_list.items[range_i].string;
>> long bottom, top;
>> - if (parse_range_arg(range_list.items[range_i].string,
>> - nth_line_cb, &sb, lno, anchor,
>> + if (parse_range_arg(arg, nth_line_cb, &sb, lno, anchor,
>> &bottom, &top, sb.path,
>> the_repository->index))
>> - usage(blame_usage);
>> + usage_msg_optf(_("failed to parse -L argument '%s'"),
>> + blame_opt_usage, options, arg);
>
> So, it used to be that it emitted only blame_usage which is a rough
> match to the synopsis,
>
> "git blame [<options>] [<rev-opts>] [<rev>] [--] <file>"
>
> but now we use blame_opt_usage + options, which means that the same
> blame_usage, a blank line, and "<rev-opts> are documented in
> git-rev-list(1)" is given, followed by the list of full options.
>
> I do think saying "failed to parse -L" is an improvement, but it is
> dubious it is a good idea to follow it with a wall of text that
> comes from options[]. After all, if the user chose to use "-L 2,8"
> and a typo replaced the comma with a full stop, which caused
> parse_range_arg() to fail, does it make sense to scroll away the
> message that helpfully points out that "-L argument '2.8'" was the
> problem with other option descriptions?
As an aside: This is a good general observation about
usage_with_options(), and usage_msg_opt*() in particular. I.e. it would
be helpful to have a version of it that took a "-L" argument, so we
could know what option we're complaining about.
Then we could either show the usage message just for that option, or
highlight it in the full "-h" output we're showing.
But obviously all for some later date.
> This is why we shouldn't distract a series with "while at it"
> changes that are outside the scope of the theme. Letting the patch
> authors and reviewers concentrate on doing one thing well would
> avoid mistakes, but mixing unrelated changes distracts them. Having
> to think about the differences between usage() and usage_msg_optf()
> with this change already distracted me and stopped my review on this
> series in this sitting. The topic will need to wait for the next
> time I decide to sit with it and start reading it from the next step.
>
> And of course, a series like this, which is supposed to do the same
> thing to many files for consistency, is better written and reviewed
> in one sitting, because that will make it easier in reviewers' mind
> to keep and apply the same review criteria consistently.
Noted, FWIW I was assuming that as other commands make wide use of
usage_msg_opt*() for this sort of thing that the usage here & output
change didn't need too much rationalizing, but live & learn etc. Thanks.
