Re: [PATCH v2] help: make option --help open man pages only for Git commands
On Mon, Aug 15, 2016 at 09:40:54PM +0100, Philip Oakley wrote: > From: "Junio C Hamano" > > "Philip Oakley" writes: > > > >> I'm still not sure this is enough. One of the problems back when I > >> introduced the --guides option (65f9835 (builtin/help.c: add --guide > >> option, 2013-04-02)) was that we had no easy way of determining what > >> guides were available, especially given the *nix/Windows split where > >> the help defaults are different (--man/--html). > >> > >> At the time[1] we (I) punted on trying to determine which guides were > >> actually installed, and just created a short list of the important > >> guides, which I believe you now check. However the less common guides > >> are still there (gitcvs-migration?), and others may be added locally. > > > > I think we should do both; "git help cvs-migration" should keep the > > same codeflow and behaviour as we have today (so that it would still > > work), while "git cvs-migration --help" should say "'cvs-migration' > > is not a git command". That would be a good clean-up anyway. > > > > It obviously cannot be done if git.c::handle_builtin() does the same > > "swap --help to help " hack, but we could improve that > > part (e.g. rewrite it to "help --swapped " to allow cmd_help() > > to notice). When the user said " --help", we don't do guides, > > when we swapped the word order, we check with guides, too. > > > The other option is to simply build a guide-list in exactly the same format > as the command list (which if it works can be merged later). Re-use the > existing code, etc. One nice thing at the moment is that third-party Git commands can install documentation and have "git help" work correctly (shameless plug for git-integration[1] which does this). I think Junio's suggestion above keeps that working whereas having a hardcoded list of guides will break this. [1] https://github.com/johnkeeping/git-integration -- 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 v2] help: make option --help open man pages only for Git commands
"Philip Oakley" writes: > The other option is to simply build a guide-list in exactly the same > format as the command list (which if it works can be merged > later). Re-use the existing code, etc. Yeah, that sounds like a good way to go forward. To implement typo correction for "git help ", having guide-list would be very useful. A related tangent is that I think "git --help" shouldn't fall back to "git help ", regardless of typo correction. It happens to "work" only because we blindly turned " --help" to "help " without even checking what is. Making it stop "working" would be a bugfix. And having both command and guide list would be helpful to prevent "git --help" from falling back to "git help ". -- 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 v2] help: make option --help open man pages only for Git commands
From: "Junio C Hamano" "Philip Oakley" writes: I'm still not sure this is enough. One of the problems back when I introduced the --guides option (65f9835 (builtin/help.c: add --guide option, 2013-04-02)) was that we had no easy way of determining what guides were available, especially given the *nix/Windows split where the help defaults are different (--man/--html). At the time[1] we (I) punted on trying to determine which guides were actually installed, and just created a short list of the important guides, which I believe you now check. However the less common guides are still there (gitcvs-migration?), and others may be added locally. I think we should do both; "git help cvs-migration" should keep the same codeflow and behaviour as we have today (so that it would still work), while "git cvs-migration --help" should say "'cvs-migration' is not a git command". That would be a good clean-up anyway. It obviously cannot be done if git.c::handle_builtin() does the same "swap --help to help " hack, but we could improve that part (e.g. rewrite it to "help --swapped " to allow cmd_help() to notice). When the user said " --help", we don't do guides, when we swapped the word order, we check with guides, too. The other option is to simply build a guide-list in exactly the same format as the command list (which if it works can be merged later). Re-use the existing code, etc. I did propose that in my very first patch series, but it was probably a step too far at the time, as it stepped on the toes of your (junio's) script for the command list. The link in my previous patch got mangled a possible start point for Ralf for looking at building a guide list would be http://public-inbox.org/git/1361660761-1932-7-git-send-email-philipoak...@iee.org/ (which worked back then ;-) Philip -- 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 v2] help: make option --help open man pages only for Git commands
"Philip Oakley" writes: > I'm still not sure this is enough. One of the problems back when I > introduced the --guides option (65f9835 (builtin/help.c: add --guide > option, 2013-04-02)) was that we had no easy way of determining what > guides were available, especially given the *nix/Windows split where > the help defaults are different (--man/--html). > > At the time[1] we (I) punted on trying to determine which guides were > actually installed, and just created a short list of the important > guides, which I believe you now check. However the less common guides > are still there (gitcvs-migration?), and others may be added locally. I think we should do both; "git help cvs-migration" should keep the same codeflow and behaviour as we have today (so that it would still work), while "git cvs-migration --help" should say "'cvs-migration' is not a git command". That would be a good clean-up anyway. It obviously cannot be done if git.c::handle_builtin() does the same "swap --help to help " hack, but we could improve that part (e.g. rewrite it to "help --swapped " to allow cmd_help() to notice). When the user said " --help", we don't do guides, when we swapped the word order, we check with guides, too. -- 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 v2] help: make option --help open man pages only for Git commands
From: "Ralf Thielow" 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 the command is known to Git before try to open the man page. If we don't know the command, give the usual advice. I'm still not sure this is enough. One of the problems back when I introduced the --guides option (65f9835 (builtin/help.c: add --guide option, 2013-04-02)) was that we had no easy way of determining what guides were available, especially given the *nix/Windows split where the help defaults are different (--man/--html). At the time[1] we (I) punted on trying to determine which guides were actually installed, and just created a short list of the important guides, which I believe you now check. However the less common guides are still there (gitcvs-migration?), and others may be added locally. One option may be to report that "no command or common guide found, will search for other guide (may fail)", which at least allows you to check the command list first, and then the common guide list, and only then warn (option?), and finally go on the rabbit hunt (possibly fruitless) for the missing guide (we've already decided it can't be a command!) -- Philip [1] https://public-inbox.org/git/1364942392-576-1-git-send-email-philipoak...@iee.org/ (V3) plus previous discussions https://public-inbox.org/git/1362342072-1412-1-git-send-email-philipoak...@iee.org/ (V2) see note Patch 6 - 13: All dropped. Drop the separate guide list.txt and extraction script, which was copied from the common command list and script. If the guide usage list is useful, extend the command-list.txt and generate-cmdlist.sh at a later datehttps://public-inbox.org/git/1361660761-1932-1-git-send-email-philipoak...@iee.org/#t (V1) the original series Signed-off-by: Ralf Thielow --- Changes in v2: - not only check for commands but also for guides - use the command assumed by "help_unknown_cmd" builtin/help.c | 34 +++--- t/t0012-help.sh | 15 +++ 2 files changed, 42 insertions(+), 7 deletions(-) create mode 100755 t/t0012-help.sh diff --git a/builtin/help.c b/builtin/help.c index 8848013..7d2110e 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -433,10 +433,35 @@ static void list_common_guides_help(void) putchar('\n'); } +static int is_common_guide(const char* cmd) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(common_guides); i++) + if (!strcmp(cmd, common_guides[i].name)) + return 1; + return 0; +} + +static const char* check_git_cmd(const char* cmd) +{ + char *alias; + + if (is_git_command(cmd) || is_common_guide(cmd)) + return cmd; + + alias = alias_lookup(cmd); + if (alias) { + printf_ln(_("`git %s' is aliased to `%s'"), cmd, alias); + free(alias); + exit(0); + } else + return help_unknown_cmd(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 +501,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/t/t0012-help.sh b/t/t0012-help.sh new file mode 100755 index 000..0dab88d --- /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 unknown command" " + cat <<-EOF >expected && + git: '123' is not a git command. See 'git --help'. + EOF + (git 123 --help 2>actual || true) && + test_i18ncmp expected actual +" + +test_done -- 2.9.2.912.g51c4565.dirty -- 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 -- 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 v2] 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 the command is known to Git before try to open the man page. If we don't know the command, give the usual advice. Signed-off-by: Ralf Thielow --- Changes in v2: - not only check for commands but also for guides - use the command assumed by "help_unknown_cmd" builtin/help.c | 34 +++--- t/t0012-help.sh | 15 +++ 2 files changed, 42 insertions(+), 7 deletions(-) create mode 100755 t/t0012-help.sh diff --git a/builtin/help.c b/builtin/help.c index 8848013..7d2110e 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -433,10 +433,35 @@ static void list_common_guides_help(void) putchar('\n'); } +static int is_common_guide(const char* cmd) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(common_guides); i++) + if (!strcmp(cmd, common_guides[i].name)) + return 1; + return 0; +} + +static const char* check_git_cmd(const char* cmd) +{ + char *alias; + + if (is_git_command(cmd) || is_common_guide(cmd)) + return cmd; + + alias = alias_lookup(cmd); + if (alias) { + printf_ln(_("`git %s' is aliased to `%s'"), cmd, alias); + free(alias); + exit(0); + } else + return help_unknown_cmd(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 +501,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/t/t0012-help.sh b/t/t0012-help.sh new file mode 100755 index 000..0dab88d --- /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 unknown command" " + cat <<-EOF >expected && + git: '123' is not a git command. See 'git --help'. + EOF + (git 123 --help 2>actual || true) && + test_i18ncmp expected actual +" + +test_done -- 2.9.2.912.g51c4565.dirty -- 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