Æ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.
> 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?
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.
> if ((!lno && (top || bottom)) || lno < bottom)
> die(Q_("file %s has only %lu line",
> "file %s has only %lu lines",
