| Message ID | patch-v3-2.6-036eb0efb5b-20210911T190239Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | parse-options: properly align continued usage output & related | expand |
On Sat, Sep 11, 2021 at 3:10 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > Change the "git blame -h" output to be consistent with "git bundle > -h"'s, i.e. before this we'd emit: Just a couple tiny, tiny nits (which may or may not be worth a re-roll)... > $ 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> This is lacking a hyphen between `git` and `rev-list`... > This makes it consistent with the usage spec used for "git bundle": > > $ git bundle -h > usage: git bundle create [<options>] <file> <git-rev-list args> ...whereas this has a hyphen between the two. > [...] > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > diff --git a/builtin/blame.c b/builtin/blame.c > @@ -29,12 +29,8 @@ > - N_("<rev-opts> are documented in git-rev-list(1)"), > + N_("git blame [<options>] [<git rev-list args>] [<rev>] [--] <file>"), Ditto regarding missing hyphen. > @@ -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); builltin/blame.c seems to be pretty consistent about starting error and warning messages with a lowercase letter, so this perhaps should follow suit. Also, I think you can drop "parameter" without losing clarity: invalid -L range would likely be good enough. > if ((!lno && (top || bottom)) || lno < bottom) > die(Q_("file %s has only %lu line", > "file %s has only %lu lines",
Æ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",
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); if ((!lno && (top || bottom)) || lno < bottom) die(Q_("file %s has only %lu line", "file %s has only %lu 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> [...] This makes it consistent with the usage spec used for "git bundle": $ git bundle -h usage: git bundle create [<options>] <file> <git-rev-list args> [...] The use of this in "blame" dated back to 5817da01434 (git-blame: migrate to incremental parse-option [1/2], 2008-07-08), and the use in "bundle" to 2e0afafebd8 (Add git-bundle: move objects and references by archive, 2007-02-22). Once we get rid of this special case we can also use usage_msg_opt() to emit the error message we'd get on an invalid "-L <range>" argument, which means we can get rid of the old-style "blame_usage" variable entirely. This makes the output friendlier, before we'd emit say: $ git blame -L1,2,3,4 Makefile usage: git blame [<options>] [<rev-opts>] [<rev>] [--] <file> $ Now we'll instead emit: $ git blame -L1,2,3,4 Makefile fatal: Invalid -L <range> parameter usage: git blame [<options>] [<git rev-list args>] [<rev>] [--] <file> [...] $ The "[...]" there elides the "git blame" option summary. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/blame.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)