Re: [PATCH v3 2/6] blame: replace usage end blurb with better option spec

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 12, 2021, 22:22 UTC
Message-ID: <xmqqtuipjuxc.fsf@gitster.g>
In-Reply-To: <patch-v3-2.6-036eb0efb5b-20210911T190239Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 14 quoted lines

> Change the "git blame -h" output to be consistent with "git bundle
> -h"'s, i.e. before this we'd emit:
>
>     $ git blame -h
>     usage: git blame [<options>] [<rev-opts>] [<rev>] [--] <file>
>
>         <rev-opts> are documented in git-rev-list(1)
>     [...]
>
> Now instead of that we'll emit:
>
>     $ git blame -h
>     usage: git blame [<options>] [<git rev-list args>] [<rev>] [--] <file>
>     [...]

What we take are options that rev-list takes, not arguments like A..B, so the updated text seems to be more wrong.

It does not even make much sense as a goal to make "blame" and "bundle" similar to begin with, does it? It may make sense for "bundle" to be more similar to "pack-objects", in that the information the command needs ultimately is about what is needed by "rev-list --objects" (i.e. object enumeration), while "blame" is more similar to "log", in that it is interested in walking commit DAG but not about the trees and blobs connected to the commits in the DAG. From the end-users' point of view, they do not care if "bundle" and "blame" are explained using similar terms.

Also, not all <rev-opts> you can see from "git rev-list -h" would make sense in the context of "git blame". "--no-merges" and any options that are related to the number of parents make no sense, --all, --branches and the friends, when used to give multiple positive ends (i.e. starting points) of traversal, would not make sense at all, options about ordering and formatting output of rev-list of course would not make any difference.

At the very least, we should say <rev-list-options> there, or we should just drop the mention of "we also take some options meant for rev-list" from here, leaving:

	usage: git blame [<options>] [<rev>] [--] <file>

and nothing else?

I am sympathetic to the original reasoning why we wanted to add a parenthetical "by the way, we also take (some) options rev-list takes" here. This is because not all relevant options are described in the options[] array we have there (we delegate what we do not know to parse_revisions_opt()), and it is sort of understandable that they wanted to leave a hint that the list of options given here is not exhaustive.

But in the longer run, if we really wanted to make the -h output useful to end users, what we should aim for is not to make it similar to "git bundle", but to ensure that the option summary includes the relevant options shared with rev-list (in other words, make the list of options cover all the options the command takes, not just the ones in today's options[] array).

When that happens, users do not have to care which ones happen to be parsed by us via our call to parse_options_step(), and which other ones are given to parse_revision_opt(). There won't be any need to say "we also take some options..." with <rev-opts> in the original.

Show 25 quoted lines

> diff --git a/builtin/blame.c b/builtin/blame.c
> index 641523ff9af..e469829bc76 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -29,12 +29,8 @@
>  #include "refs.h"
>  #include "tag.h"
>  
> -static char blame_usage[] = N_("git blame [<options>] [<rev-opts>] [<rev>] [--] <file>");
> -
>  static const char *blame_opt_usage[] = {
> -	blame_usage,
> -	"",
> -	N_("<rev-opts> are documented in git-rev-list(1)"),
> +	N_("git blame [<options>] [<git rev-list args>] [<rev>] [--] <file>"),
>  	NULL
>  };
>  
> @@ -1107,7 +1103,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>  				    nth_line_cb, &sb, lno, anchor,
>  				    &bottom, &top, sb.path,
>  				    the_repository->index))
> -			usage(blame_usage);
> +			usage_msg_opt(_("Invalid -L <range> parameter"),
> +				      blame_opt_usage, options);

"invalid -L <range>" is fine, but can't we parrot what the user gave us here? You can give more than one -L to specify two ranges in the same file that are not contiguous:

	git blame -L1,10 -L100.112 master..seen -- foo.c

and it would be helpful to tell which one is broken.

>  		if ((!lno && (top || bottom)) || lno < bottom)
>  			die(Q_("file %s has only %lu line",
>  			       "file %s has only %lu lines",