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

[Junio C Hamano <gitster@pobox.com>]

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

> 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.

> 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",