Re: [PATCH 2/2] help: make option --help open man pages only for Git commands
Sorry for being late in responding. It's been busy days. 2016-08-18 21:51 GMT+02:00 Junio C Hamano: > Ralf Thielow writes: > > The same comment applies to 1/2, too, in that the word "command" > will be interpreted differently by different people. For example, > "git co --help" and "git help co" would work, with or without 1/2 in > place when you have "[alias] co = checkout", so we are calling "Git > subcommands that we ship, custom commands 'git-$foo' the users have > in their $PATH, and aliases the users create" collectively "command". > > As long as the reader understands that definition, both the log > messages of 1/2 and 2/2 _and_ the updated description for "git help" > we have in 1/2 are all very clear. I do not care too much about the > commit log message, but we may want to think about the documentation > a bit more. > > Here is what 1/2 adds to "git help" documentation: > > +Note that `git --help ...` is almost identical to `git help ...` because > +the former is internally converted into the latter with option > --command-only > +being added. > > To display the linkgit:git[1] man page, use `git help git`. > > @@ -43,6 +44,10 @@ OPTIONS > Prints all the available commands on the standard output. This > option overrides any given command or guide name. > > +-c:: > +--command-only:: > + Display help information only for commands. > + > > First, I do not think a short form is unnecessary; the users are not > expected to use that form, once they started typing "git help...". > If we flip the polarity and call it --exclude-guides or something, > would it make it less ambiguous? > Sure. Since "command" is understood as both Git command and guide in this context, the name --exclude-guides would describe the behaviour of that option less ambiguous. I'll rename it. >> This breaks "git --help" while "git help " still works. > > I wouldn't call that a breakage; "git everyday --help" shouldn't > have worked in the first place. It did something useful merely by > accident ;-). > OK, I'll call it "doesn't work anymore". >> diff --git a/git.c b/git.c >> index 0f1937f..2cd2e06 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 --command-only 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(); >> + for (i = 0; i < argc; i++) { >> + argv_array_push(, argv[i]); >> + if (!i) >> + argv_array_push(, "--command-only"); >> + } >> + >> + argc++; >> + argv = argv_array_detach(); >> } >> >> 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. I wonder if it is worth doing > > static void handle_builtin(int argc, const char **argv) > { > struct argv_array args = ARGV_ARRAY_INIT; > ... > if (argc > 1 && !strcmp(argv[1], "--help")) { > ... > argv = args.argv; > } > builtin = get_builtin(cmd); > if (builtin) > exit(run_builtin(...)); > argv_array_clear(); > } > > to help unconfuse them. > I'll do it this way. 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
[PATCH 2/2] help: make option --help open man pages only for Git commands
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. This breaks "git --help" while "git help " still works. Signed-off-by: Ralf Thielow--- git.c | 15 ++- t/t0012-help.sh | 8 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/git.c b/git.c index 0f1937f..2cd2e06 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 --command-only 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(); + for (i = 0; i < argc; i++) { + argv_array_push(, argv[i]); + if (!i) + argv_array_push(, "--command-only"); + } + + argc++; + argv = argv_array_detach(); } builtin = get_builtin(cmd); diff --git a/t/t0012-help.sh b/t/t0012-help.sh index e20f907..81fec90 100755 --- a/t/t0012-help.sh +++ b/t/t0012-help.sh @@ -18,4 +18,12 @@ test_expect_success "--command-only does not work for guides" " test_i18ncmp expected actual " +test_expect_success "--help does not work for guides" " + 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 -- 2.9.2.912.gd0c0e83 -- 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 2/2] help: make option --help open man pages only for Git commands
Ralf Thielowwrites: > 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. What the patch does is correct, I think, but the explanation may invite a false alarm. If you added a custom command git-who in your $PATH, with an appropriate documentation for git-who(1), we would still show its documentation, no? The same comment applies to 1/2, too, in that the word "command" will be interpreted differently by different people. For example, "git co --help" and "git help co" would work, with or without 1/2 in place when you have "[alias] co = checkout", so we are calling "Git subcommands that we ship, custom commands 'git-$foo' the users have in their $PATH, and aliases the users create" collectively "command". As long as the reader understands that definition, both the log messages of 1/2 and 2/2 _and_ the updated description for "git help" we have in 1/2 are all very clear. I do not care too much about the commit log message, but we may want to think about the documentation a bit more. Here is what 1/2 adds to "git help" documentation: +Note that `git --help ...` is almost identical to `git help ...` because +the former is internally converted into the latter with option --command-only +being added. To display the linkgit:git[1] man page, use `git help git`. @@ -43,6 +44,10 @@ OPTIONS Prints all the available commands on the standard output. This option overrides any given command or guide name. +-c:: +--command-only:: + Display help information only for commands. + First, I do not think a short form is unnecessary; the users are not expected to use that form, once they started typing "git help...". If we flip the polarity and call it --exclude-guides or something, would it make it less ambiguous? > This breaks "git --help" while "git help " still works. I wouldn't call that a breakage; "git everyday --help" shouldn't have worked in the first place. It did something useful merely by accident ;-). > diff --git a/git.c b/git.c > index 0f1937f..2cd2e06 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 --command-only 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(); > + for (i = 0; i < argc; i++) { > + argv_array_push(, argv[i]); > + if (!i) > + argv_array_push(, "--command-only"); > + } > + > + argc++; > + argv = argv_array_detach(); > } > > 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. I wonder if it is worth doing static void handle_builtin(int argc, const char **argv) { struct argv_array args = ARGV_ARRAY_INIT; ... if (argc > 1 && !strcmp(argv[1], "--help")) { ... argv = args.argv; } builtin = get_builtin(cmd); if (builtin) exit(run_builtin(...)); argv_array_clear(); } to help unconfuse them. By the way, I do not see these patches on gmane, public-inbox or usual suspects. Perhaps vger is having a bad day or something? -- 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