Re: [PATCH v3] help: make option --help open man pages only for Git commands
Ralf Thielow writes: > 2016-08-16 19:27 GMT+02:00 Junio C Hamano : >> Ralf Thielow writes: >>> + >>> + if (swapped) >>> + return help_unknown_cmd(cmd); >> >> I am guilty of suggesting "swapped"; even if we are going to mark >> this as OPT_HIDDEN, I think we should be able to think of a better >> name. I think the meaning of this boolean is "we know that this is >> not a guide and is meant to be a command.", and I hope we can come >> up with a name that concisely expresses that (e.g. "--not-a-guide", >> "--must-be-a-command"). >> > > I think "--cmd-only" is a good name. With a good name I think it's worth > to make this option visible to the user. Sure. I'd prefer "cmd" to be spelled out in anything that is end-user facing, though. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] help: make option --help open man pages only for Git commands
2016-08-16 19:27 GMT+02:00 Junio C Hamano : > Ralf Thielow writes: >> + >> + if (swapped) >> + return help_unknown_cmd(cmd); > > I am guilty of suggesting "swapped"; even if we are going to mark > this as OPT_HIDDEN, I think we should be able to think of a better > name. I think the meaning of this boolean is "we know that this is > not a guide and is meant to be a command.", and I hope we can come > up with a name that concisely expresses that (e.g. "--not-a-guide", > "--must-be-a-command"). > I think "--cmd-only" is a good name. With a good name I think it's worth to make this option visible to the user. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] help: make option --help open man pages only for Git commands
Ralf Thielow writes: > builtin/help.c | 30 +++--- > git.c | 15 ++- > t/t0012-help.sh | 15 +++ > 3 files changed, 52 insertions(+), 8 deletions(-) > create mode 100755 t/t0012-help.sh > > diff --git a/builtin/help.c b/builtin/help.c > index 8848013..76f07c7 100644 > --- a/builtin/help.c > +++ b/builtin/help.c > @@ -37,7 +37,9 @@ static int show_all = 0; > static int show_guides = 0; > static unsigned int colopts; > static enum help_format help_format = HELP_FORMAT_NONE; > +static int swapped = 0; This is not the first offender (show_guides above does so, too), but please do not initialize static explicitly to 0 or NULL. > static struct option builtin_help_options[] = { > + OPT_BOOL('s', "swapped", &swapped, "mark as being called by > --help"), > OPT_BOOL('a', "all", &show_all, N_("print all available commands")), > OPT_BOOL('g', "guides", &show_guides, N_("print list of useful > guides")), > OPT_SET_INT('m', "man", &help_format, N_("show man page"), > HELP_FORMAT_MAN), > @@ -433,10 +435,29 @@ static void list_common_guides_help(void) > putchar('\n'); > } > > +static const char* check_git_cmd(const char* cmd) Style: "static const char *check_git_cmd(const char *cmd)". The asterisk that turns the base type to a pointer to the base type sticks to the identifier, not to the type. > +{ > + char *alias; > + > + if (is_git_command(cmd)) > + return cmd; > + > + alias = alias_lookup(cmd); > + if (alias) { > + printf_ln(_("`git %s' is aliased to `%s'"), cmd, alias); > + free(alias); > + exit(0); > + } > + > + if (swapped) > + return help_unknown_cmd(cmd); I am guilty of suggesting "swapped"; even if we are going to mark this as OPT_HIDDEN, I think we should be able to think of a better name. I think the meaning of this boolean is "we know that this is not a guide and is meant to be a command.", and I hope we can come up with a name that concisely expresses that (e.g. "--not-a-guide", "--must-be-a-command"). > + return cmd; > +} > + > int cmd_help(int argc, const char **argv, const char *prefix) > { > int nongit; > - char *alias; > enum help_format parsed_help_format; > > argc = parse_options(argc, argv, prefix, builtin_help_options, > @@ -476,12 +497,7 @@ int cmd_help(int argc, const char **argv, const char > *prefix) > if (help_format == HELP_FORMAT_NONE) > help_format = parse_help_format(DEFAULT_HELP_FORMAT); > > - alias = alias_lookup(argv[0]); > - if (alias && !is_git_command(argv[0])) { > - printf_ln(_("`git %s' is aliased to `%s'"), argv[0], alias); > - free(alias); > - return 0; > - } > + argv[0] = check_git_cmd(argv[0]); > > switch (help_format) { > case HELP_FORMAT_NONE: > diff --git a/git.c b/git.c > index 0f1937f..71ea983 100644 > --- a/git.c > +++ b/git.c > @@ -528,10 +528,23 @@ static void handle_builtin(int argc, const char **argv) > strip_extension(argv); > cmd = argv[0]; > > - /* Turn "git cmd --help" into "git help cmd" */ > + /* Turn "git cmd --help" into "git help --swapped cmd" */ > if (argc > 1 && !strcmp(argv[1], "--help")) { > + struct argv_array args; > + int i; > + > argv[1] = argv[0]; > argv[0] = cmd = "help"; > + > + argv_array_init(&args); > + for (i = 0; i < argc; i++) { > + argv_array_push(&args, argv[i]); > + if (i == 0) It is more idiomatic to say if (!i) around here. > + argv_array_push(&args, "--swapped"); > + } > + > + argc++; > + argv = argv_array_detach(&args); > } > > builtin = get_builtin(cmd); The code does this after it: if (builtin) exit(run_builtin(...)); and returns. If we didn't get builtin, we risk leaking args.argv here, but we assume argv[0] = cmd = "help" is always a builtin, which I think is a safe assumption, so the code is OK. Static checkers that are only half intelligent may yell at you for not releasing the resources, though. > diff --git a/t/t0012-help.sh b/t/t0012-help.sh > new file mode 100755 > index 000..6f700b1 > --- /dev/null > +++ b/t/t0012-help.sh > @@ -0,0 +1,15 @@ > +#!/bin/sh > + > +test_description='help' > + > +. ./test-lib.sh > + > +test_expect_success "pass --help to common guide" " > + cat <<-EOF >expected && > + git: 'revisions' is not a git command. See 'git --help'. > + EOF > + (git revisions --help 2>actual || true) && > + test_i18ncmp expected actual > +" > + > +test_done -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger
Re: [PATCH v3] help: make option --help open man pages only for Git commands
2016-08-16 18:33 GMT+02:00 John Keeping : > On Tue, Aug 16, 2016 at 06:20:30PM +0200, Ralf Thielow wrote: >> static struct option builtin_help_options[] = { >> + OPT_BOOL('s', "swapped", &swapped, "mark as being called by >> --help"), > > OPT_HIDDEN_BOOL maybe? > Yeah >_< Thanks! -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] help: make option --help open man pages only for Git commands
On Tue, Aug 16, 2016 at 06:20:30PM +0200, Ralf Thielow wrote: > If option --help is passed to a Git command, we try to open > the man page of that command. However, we do it even for commands > we don't know. Make sure it is a Git command by using "help_unknown_cmd" > which is even able to assume a command if the user made a typo. > > This breaks "git --help" while "git help " still works. > > As " --help" will internally be turned into "help ", > introduce the hidden option "--swapped" in order to know which > version has been called. > > Signed-off-by: Ralf Thielow > --- > Thanks, all, for the help! > > Changes since v2: > - don't check for common guides as the list is very incomplete > - only check for git commands when called via --help (introduce > option --swapped for that), as suggested by Junio > - change test case to check for --help being passed to a concept > used as a git command > > builtin/help.c | 30 +++--- > git.c | 15 ++- > t/t0012-help.sh | 15 +++ > 3 files changed, 52 insertions(+), 8 deletions(-) > create mode 100755 t/t0012-help.sh > > diff --git a/builtin/help.c b/builtin/help.c > index 8848013..76f07c7 100644 > --- a/builtin/help.c > +++ b/builtin/help.c > @@ -37,7 +37,9 @@ static int show_all = 0; > static int show_guides = 0; > static unsigned int colopts; > static enum help_format help_format = HELP_FORMAT_NONE; > +static int swapped = 0; > static struct option builtin_help_options[] = { > + OPT_BOOL('s', "swapped", &swapped, "mark as being called by > --help"), OPT_HIDDEN_BOOL maybe? > OPT_BOOL('a', "all", &show_all, N_("print all available commands")), > OPT_BOOL('g', "guides", &show_guides, N_("print list of useful > guides")), > OPT_SET_INT('m', "man", &help_format, N_("show man page"), > HELP_FORMAT_MAN), -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html