Re: [PATCH v3 04/36] blame: use a more detailed usage_msg_optf() error on bad -L

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> 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@gmail.com>
>> ---
>>  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.