| Message ID | 20180922174707.16498-1-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git help: promote 'git help -av' | expand |
On Sat, Sep 22 2018, Nguyễn Thái Ngọc Duy wrote: > When you type "git help" (or just "git") you are greeted with a list > with commonly used commands and their short description and are > suggested to use "git help -a" or "git help -g" for more details. > > "git help -av" would be more friendly and inline with what is shown > with "git help" since it shows list of commands with description as > well, and commands are properly grouped. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > git.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/git.c b/git.c > index a6f4b44af5..69c21f378b 100644 > --- a/git.c > +++ b/git.c > @@ -31,7 +31,7 @@ const char git_usage_string[] = > " <command> [<args>]"); > > const char git_more_info_string[] = > - N_("'git help -a' and 'git help -g' list available subcommands and some\n" > + N_("'git help -av' and 'git help -g' list available subcommands and some\n" > "concept guides. See 'git help <command>' or 'git help <concept>'\n" > "to read about a specific subcommand or concept."); A side-effect of this not noted in your commit message is that we'll now invoke the pager, perhaps we should just do: diff --git a/builtin/help.c b/builtin/help.c index 8d4f6dd301..1a3b174aaf 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -436,9 +436,9 @@ int cmd_help(int argc, const char **argv, const char *prefix) parsed_help_format = help_format; if (show_all) { + setup_pager(); git_config(git_help_config, NULL); if (verbose) { - setup_pager(); list_all_cmds_help(); return 0; } @@ -460,8 +460,10 @@ int cmd_help(int argc, const char **argv, const char *prefix) return 0; } - if (show_guides) + if (show_guides) { + setup_pager(); list_common_guides_help(); + } if (show_all || show_guides) { printf("%s\n", _(git_more_info_string)); Or is there a good reason we shouldn't invoke the pager for e.g. -g when the terminal is too small (per our default less config)? Another thing I noticed: We don't list -v in the git-help manpage, but since we use OPT_VERBOSE it's supported.
On Sat, Sep 22, 2018 at 9:29 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > On Sat, Sep 22 2018, Nguyễn Thái Ngọc Duy wrote: > > > When you type "git help" (or just "git") you are greeted with a list > > with commonly used commands and their short description and are > > suggested to use "git help -a" or "git help -g" for more details. > > > > "git help -av" would be more friendly and inline with what is shown > > with "git help" since it shows list of commands with description as > > well, and commands are properly grouped. > > > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > > --- > > git.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/git.c b/git.c > > index a6f4b44af5..69c21f378b 100644 > > --- a/git.c > > +++ b/git.c > > @@ -31,7 +31,7 @@ const char git_usage_string[] = > > " <command> [<args>]"); > > > > const char git_more_info_string[] = > > - N_("'git help -a' and 'git help -g' list available subcommands and some\n" > > + N_("'git help -av' and 'git help -g' list available subcommands and some\n" > > "concept guides. See 'git help <command>' or 'git help <concept>'\n" > > "to read about a specific subcommand or concept."); > > A side-effect of this not noted in your commit message is that we'll now > invoke the pager, perhaps we should just do: > > diff --git a/builtin/help.c b/builtin/help.c > index 8d4f6dd301..1a3b174aaf 100644 > --- a/builtin/help.c > +++ b/builtin/help.c > @@ -436,9 +436,9 @@ int cmd_help(int argc, const char **argv, const char *prefix) > parsed_help_format = help_format; > > if (show_all) { > + setup_pager(); > git_config(git_help_config, NULL); > if (verbose) { > - setup_pager(); > list_all_cmds_help(); > return 0; > } > @@ -460,8 +460,10 @@ int cmd_help(int argc, const char **argv, const char *prefix) > return 0; > } > > - if (show_guides) > + if (show_guides) { > + setup_pager(); > list_common_guides_help(); > + } > > if (show_all || show_guides) { > printf("%s\n", _(git_more_info_string)); > > Or is there a good reason we shouldn't invoke the pager for e.g. -g when > the terminal is too small (per our default less config)? Different pagers may behave differently (and so far "help -a" still fits in my screen). So I don't think we should invoke pager more than necessary.
On Sat, Sep 22, 2018 at 07:47:07PM +0200, Nguyễn Thái Ngọc Duy wrote: > When you type "git help" (or just "git") you are greeted with a list > with commonly used commands and their short description and are > suggested to use "git help -a" or "git help -g" for more details. > > "git help -av" would be more friendly and inline with what is shown > with "git help" since it shows list of commands with description as > well, and commands are properly grouped. I agree that "help -av" is likely to be more friendly. I kind of wonder if it should just be the default for "-a". Do we have any obligation not to change the format of that output? Assuming we want to keep "-a" and "-av" as-is, your patch seems quite sensible to me. -Peff
On Mon, Sep 24, 2018 at 02:19:28PM -0400, Jeff King wrote: > On Sat, Sep 22, 2018 at 07:47:07PM +0200, Nguyễn Thái Ngọc Duy wrote: > > > When you type "git help" (or just "git") you are greeted with a list > > with commonly used commands and their short description and are > > suggested to use "git help -a" or "git help -g" for more details. > > > > "git help -av" would be more friendly and inline with what is shown > > with "git help" since it shows list of commands with description as > > well, and commands are properly grouped. > > I agree that "help -av" is likely to be more friendly. I kind of wonder > if it should just be the default for "-a". Do we have any obligation not > to change the format of that output? I agree, though I'd like to clarify what you said before doing so wholeheartedly. Did you mean that all existing uses of 'git help -a' should instead mean 'git help -av' (i.e., that '-a' after your proposed patch means the same as '-av' in revisions prior to this one?) If so, I agree. I can't imagine a case where I'd like to provide '-a' and _not_ '-v', so certainly the later should come as a two-for-one deal. Less, more well-intentioned knobs seems a positive trend to me, so I am for this idea. Thanks, Taylor
On Mon, Sep 24, 2018 at 03:20:00PM -0500, Taylor Blau wrote: > On Mon, Sep 24, 2018 at 02:19:28PM -0400, Jeff King wrote: > > On Sat, Sep 22, 2018 at 07:47:07PM +0200, Nguyễn Thái Ngọc Duy wrote: > > > > > When you type "git help" (or just "git") you are greeted with a list > > > with commonly used commands and their short description and are > > > suggested to use "git help -a" or "git help -g" for more details. > > > > > > "git help -av" would be more friendly and inline with what is shown > > > with "git help" since it shows list of commands with description as > > > well, and commands are properly grouped. > > > > I agree that "help -av" is likely to be more friendly. I kind of wonder > > if it should just be the default for "-a". Do we have any obligation not > > to change the format of that output? > > I agree, though I'd like to clarify what you said before doing so > wholeheartedly. > > Did you mean that all existing uses of 'git help -a' should instead mean > 'git help -av' (i.e., that '-a' after your proposed patch means the same > as '-av' in revisions prior to this one?) Yes, exactly. I think the vast majority of uses would prefer the categorized list. The obvious exceptions are: - you can't remember the name of the command so an alphabetized list is easier for sifting through (without having to re-sift for each category). - you need a machine-readable version of the list (e.g., for programmable completion). We have "git --list-cmds", but we may need to advertise it better and mark it non-experimental. I dunno. Maybe it is not worth the effort. Duy's existing patch is an easy one liner. ;) -Peff
Jeff King <peff@peff.net> writes: > I agree that "help -av" is likely to be more friendly. I kind of wonder > if it should just be the default for "-a". Do we have any obligation not > to change the format of that output? I know that at least older versions of git-completion.bash (I think it was up to 2.17.x series, but do not quote me on that) have parsed "git help -a" output to obtain the list of commands. So such a change would impact those minority users who keep stale completion script in their $HOME/.bashrc without ever update it, thinking it is good enough. I personally find "help -av" a bit too loud to my taste than plain "-a", and more importantly, I look at "help -a" primarily to check the last section "avaialble from elsewhere on your $PATH" to find things like "clang-format", which I do not think is available anywhere in "help -av", so I do not think "-av" can be promoted as an upward-compatible replacement in its current form. I probably am not a part of the target audience, though.
On Mon, Sep 24, 2018 at 10:58 PM Junio C Hamano <gitster@pobox.com> wrote: > I personally find "help -av" a bit too loud to my taste than plain > "-a", and more importantly, I look at "help -a" primarily to check > the last section "avaialble from elsewhere on your $PATH" to find > things like "clang-format", which I do not think is available > anywhere in "help -av", so I do not think "-av" can be promoted as > an upward-compatible replacement in its current form. Yep. I also thought "help -a" was denser but wasn't sure if it actually helps or not. Whenever I look at that block of commands, I end up searching anyway. For my use case, "help -a" could be better served with something like "git apropos". I think adding another section about external commands in "help -av" would address the "clang-format" stuff. With that, it's probably good enough to completely replace "help -a". It may also be good to list aliases there too in a separate section so you have "all you can type" in one (big) list. -- Duy
On Tue, Sep 25, 2018 at 05:15:38PM +0200, Duy Nguyen wrote: > On Mon, Sep 24, 2018 at 10:58 PM Junio C Hamano <gitster@pobox.com> wrote: > > I personally find "help -av" a bit too loud to my taste than plain > > "-a", and more importantly, I look at "help -a" primarily to check > > the last section "avaialble from elsewhere on your $PATH" to find > > things like "clang-format", which I do not think is available > > anywhere in "help -av", so I do not think "-av" can be promoted as > > an upward-compatible replacement in its current form. > > Yep. I also thought "help -a" was denser but wasn't sure if it > actually helps or not. Whenever I look at that block of commands, I > end up searching anyway. For my use case, "help -a" could be better > served with something like "git apropos". > > I think adding another section about external commands in "help -av" > would address the "clang-format" stuff. With that, it's probably good > enough to completely replace "help -a". It may also be good to list > aliases there too in a separate section so you have "all you can type" > in one (big) list. Here's the patch that adds that external commands and aliases sections. I feel that external commands section is definitely good to have even if we don't replace "help -a". Aliases are more subjective... -- 8< -- diff --git a/builtin/help.c b/builtin/help.c index 8d4f6dd301..23a34b36e7 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -38,7 +38,6 @@ static const char *html_path; static int show_all = 0; static int show_guides = 0; static int show_config; -static int verbose; static unsigned int colopts; static enum help_format help_format = HELP_FORMAT_NONE; static int exclude_guides; @@ -53,7 +52,6 @@ static struct option builtin_help_options[] = { HELP_FORMAT_WEB), OPT_SET_INT('i', "info", &help_format, N_("show info page"), HELP_FORMAT_INFO), - OPT__VERBOSE(&verbose, N_("print command description")), OPT_END(), }; @@ -437,14 +435,9 @@ int cmd_help(int argc, const char **argv, const char *prefix) if (show_all) { git_config(git_help_config, NULL); - if (verbose) { - setup_pager(); - list_all_cmds_help(); - return 0; - } - printf(_("usage: %s%s"), _(git_usage_string), "\n\n"); - load_command_list("git-", &main_cmds, &other_cmds); - list_commands(colopts, &main_cmds, &other_cmds); + setup_pager(); + list_all_cmds_help(); + return 0; } if (show_config) { diff --git a/help.c b/help.c index 96f6d221ed..4a168230dc 100644 --- a/help.c +++ b/help.c @@ -98,7 +98,8 @@ static int cmd_name_cmp(const void *elem1, const void *elem2) return strcmp(e1->name, e2->name); } -static void print_cmd_by_category(const struct category_description *catdesc) +static void print_cmd_by_category(const struct category_description *catdesc, + int *longest_p) { struct cmdname_help *cmds; int longest = 0; @@ -124,6 +125,8 @@ static void print_cmd_by_category(const struct category_description *catdesc) print_command_list(cmds, mask, longest); } free(cmds); + if (longest_p) + *longest_p = longest; } void add_cmdname(struct cmdnames *cmds, const char *name, int len) @@ -193,26 +196,6 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes) cmds->cnt = cj; } -static void pretty_print_cmdnames(struct cmdnames *cmds, unsigned int colopts) -{ - struct string_list list = STRING_LIST_INIT_NODUP; - struct column_options copts; - int i; - - for (i = 0; i < cmds->cnt; i++) - string_list_append(&list, cmds->names[i]->name); - /* - * always enable column display, we only consult column.* - * about layout strategy and stuff - */ - colopts = (colopts & ~COL_ENABLE_MASK) | COL_ENABLED; - memset(&copts, 0, sizeof(copts)); - copts.indent = " "; - copts.padding = 2; - print_columns(&list, colopts, &copts); - string_list_clear(&list, 0); -} - static void list_commands_in_dir(struct cmdnames *cmds, const char *path, const char *prefix) @@ -285,29 +268,10 @@ void load_command_list(const char *prefix, exclude_cmds(other_cmds, main_cmds); } -void list_commands(unsigned int colopts, - struct cmdnames *main_cmds, struct cmdnames *other_cmds) -{ - if (main_cmds->cnt) { - const char *exec_path = git_exec_path(); - printf_ln(_("available git commands in '%s'"), exec_path); - putchar('\n'); - pretty_print_cmdnames(main_cmds, colopts); - putchar('\n'); - } - - if (other_cmds->cnt) { - printf_ln(_("git commands available from elsewhere on your $PATH")); - putchar('\n'); - pretty_print_cmdnames(other_cmds, colopts); - putchar('\n'); - } -} - void list_common_cmds_help(void) { puts(_("These are common Git commands used in various situations:")); - print_cmd_by_category(common_categories); + print_cmd_by_category(common_categories, NULL); } void list_all_main_cmds(struct string_list *list) @@ -405,7 +369,7 @@ void list_common_guides_help(void) { CAT_guide, N_("The common Git guides are:") }, { 0, NULL } }; - print_cmd_by_category(catdesc); + print_cmd_by_category(catdesc, NULL); putchar('\n'); } @@ -494,9 +458,48 @@ void list_config_help(int for_human) string_list_clear(&keys, 0); } +static int get_alias(const char *var, const char *value, void *data) +{ + struct string_list *list = data; + + if (skip_prefix(var, "alias.", &var)) + string_list_append(list, var)->util = xstrdup(value); + + return 0; +} + void list_all_cmds_help(void) { - print_cmd_by_category(main_categories); + struct string_list others = STRING_LIST_INIT_DUP; + struct string_list alias_list = STRING_LIST_INIT_DUP; + struct cmdname_help *aliases; + int i, longest; + + printf_ln(_("See 'git help <command>' to read about a specific subcommand")); + print_cmd_by_category(main_categories, &longest); + + list_all_other_cmds(&others); + if (others.nr) + printf("\n%s\n", _("External commands")); + for (i = 0; i < others.nr; i++) + printf(" %s\n", others.items[i].string); + string_list_clear(&others, 0); + + git_config(get_alias, &alias_list); + string_list_sort(&alias_list); + if (alias_list.nr) { + printf("\n%s\n", _("Command aliases")); + ALLOC_ARRAY(aliases, alias_list.nr + 1); + for (i = 0; i < alias_list.nr; i++) { + aliases[i].name = alias_list.items[i].string; + aliases[i].help = alias_list.items[i].util; + aliases[i].category = 1; + } + aliases[alias_list.nr].name = NULL; + print_command_list(aliases, 1, longest); + free(aliases); + } + string_list_clear(&alias_list, 1); } int is_in_cmdlist(struct cmdnames *c, const char *s) diff --git a/help.h b/help.h index 9eab6a3f89..105de6195a 100644 --- a/help.h +++ b/help.h @@ -37,7 +37,6 @@ extern void add_cmdname(struct cmdnames *cmds, const char *name, int len); /* Here we require that excludes is a sorted list. */ extern void exclude_cmds(struct cmdnames *cmds, struct cmdnames *excludes); extern int is_in_cmdlist(struct cmdnames *cmds, const char *name); -extern void list_commands(unsigned int colopts, struct cmdnames *main_cmds, struct cmdnames *other_cmds); /* * call this to die(), when it is suspected that the user mistyped a diff --git a/t/t0012-help.sh b/t/t0012-help.sh index bc27df7f38..964615de2f 100755 --- a/t/t0012-help.sh +++ b/t/t0012-help.sh @@ -30,8 +30,7 @@ test_expect_success "setup" ' test_expect_success 'basic help commands' ' git help >/dev/null && git help -a >/dev/null && - git help -g >/dev/null && - git help -av >/dev/null + git help -g >/dev/null ' test_expect_success "works for commands and guides by default" ' -- 8< -- -- Duy
On Tue, Sep 25, 2018 at 07:44:51PM +0200, Duy Nguyen wrote: > > I think adding another section about external commands in "help -av" > > would address the "clang-format" stuff. With that, it's probably good > > enough to completely replace "help -a". It may also be good to list > > aliases there too in a separate section so you have "all you can type" > > in one (big) list. > > Here's the patch that adds that external commands and aliases > sections. I feel that external commands section is definitely good to > have even if we don't replace "help -a". Aliases are more > subjective... Just eyeballing the output, it looks pretty reasonable to me. The lack of explanation for external commands really stands out, but there's not much we can do. That part of the output is not very compact, and we _could_ put it in columns, but that might be confusing since the rest of the output is one-per-line. Mentioning aliases seems reasonable to me. The definitions of some of mine are pretty nasty bits of shell, but I guess that people either don't have any ugly aliases, or are comfortable enough with them that they won't be scared away. :) > -- 8< -- > @@ -53,7 +52,6 @@ static struct option builtin_help_options[] = { > HELP_FORMAT_WEB), > OPT_SET_INT('i', "info", &help_format, N_("show info page"), > HELP_FORMAT_INFO), > - OPT__VERBOSE(&verbose, N_("print command description")), > OPT_END(), > }; Would we want to continue respecting "-v" as a noop? I admit I did not even know it existed until this thread, but if people have trained themselves to run "git help -av", we should probably continue to give them this output. -Peff
On Tue, Sep 25, 2018 at 10:54 PM Jeff King <peff@peff.net> wrote: > Mentioning aliases seems reasonable to me. The definitions of some of > mine are pretty nasty bits of shell, but I guess that people either > don't have any ugly aliases, or are comfortable enough with them that > they won't be scared away. :) I have one with a very long pretty format for git-log. Well, you wrote your aliases you learn to live with them :) We could certainly cut too long aliased commands to keep the listing pretty, but I don't think we should do that until somebody asks for it. > > -- 8< -- > > @@ -53,7 +52,6 @@ static struct option builtin_help_options[] = { > > HELP_FORMAT_WEB), > > OPT_SET_INT('i', "info", &help_format, N_("show info page"), > > HELP_FORMAT_INFO), > > - OPT__VERBOSE(&verbose, N_("print command description")), > > OPT_END(), > > }; > > Would we want to continue respecting "-v" as a noop? I admit I did not > even know it existed until this thread, but if people have trained > themselves to run "git help -av", we should probably continue to give > them this output. -v was recently added just for the new "help -a" in May 2018. I think it's ok to get rid of it. Memory muscles probably take a couple more months to kick in.
Duy Nguyen <pclouds@gmail.com> writes: > -v was recently added just for the new "help -a" in May 2018. I think > it's ok to get rid of it. Memory muscles probably take a couple more > months to kick in. If it is not hurting, keeping it lets people say "--no-verbose" to get a less verbose output to help those who do not like the new default. Unless you are removing code to show the current "help -a" output, that is.
Duy Nguyen <pclouds@gmail.com> writes: > Here's the patch that adds that external commands and aliases > sections. I feel that external commands section is definitely good to > have even if we don't replace "help -a". Aliases are more > subjective... I didn't apply this (so I didn't try running it), but a quick eyeballing tells me that the listing of external commands in -av output done in this patch seems reasonable. I do not think removing "-v" and making the current "help -a" output unavailable is a wise idea, though.
On Wed, Sep 26, 2018 at 10:28:31AM -0700, Junio C Hamano wrote: > Duy Nguyen <pclouds@gmail.com> writes: > > > Here's the patch that adds that external commands and aliases > > sections. I feel that external commands section is definitely good to > > have even if we don't replace "help -a". Aliases are more > > subjective... > > I didn't apply this (so I didn't try running it), but a quick > eyeballing tells me that the listing of external commands in -av > output done in this patch seems reasonable. > > I do not think removing "-v" and making the current "help -a" output > unavailable is a wise idea, though. I think that your point about having to react in a split-second in order to ^C before we open the manual page for a command is a good one. So, I agree with this. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > On Wed, Sep 26, 2018 at 10:28:31AM -0700, Junio C Hamano wrote: >> Duy Nguyen <pclouds@gmail.com> writes: >> >> > Here's the patch that adds that external commands and aliases >> > sections. I feel that external commands section is definitely good to >> > have even if we don't replace "help -a". Aliases are more >> > subjective... >> >> I didn't apply this (so I didn't try running it), but a quick >> eyeballing tells me that the listing of external commands in -av >> output done in this patch seems reasonable. >> >> I do not think removing "-v" and making the current "help -a" output >> unavailable is a wise idea, though. > > I think that your point about having to react in a split-second in order > to ^C before we open the manual page for a command is a good one. So, I > agree with this. Responding to a wrong thread? I thought "now I need ^C within a handful of deciseconds if I want only alias?" was not about the change to make "-v" default when "help -a" is asked, but about what to do with "git foo --help" that only gives "foo is aliased to ...".
On Fri, Sep 28, 2018 at 09:30:51AM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > On Wed, Sep 26, 2018 at 10:28:31AM -0700, Junio C Hamano wrote: > >> Duy Nguyen <pclouds@gmail.com> writes: > >> > >> > Here's the patch that adds that external commands and aliases > >> > sections. I feel that external commands section is definitely good to > >> > have even if we don't replace "help -a". Aliases are more > >> > subjective... > >> > >> I didn't apply this (so I didn't try running it), but a quick > >> eyeballing tells me that the listing of external commands in -av > >> output done in this patch seems reasonable. > >> > >> I do not think removing "-v" and making the current "help -a" output > >> unavailable is a wise idea, though. > > > > I think that your point about having to react in a split-second in order > > to ^C before we open the manual page for a command is a good one. So, I > > agree with this. > > Responding to a wrong thread? Ah, I certainly am. Thanks for catching my mistake :-). > I thought "now I need ^C within a handful of deciseconds if I want > only alias?" was not about the change to make "-v" default when > "help -a" is asked, but about what to do with "git foo --help" that > only gives "foo is aliased to ...". Thanks, Taylor
diff --git a/git.c b/git.c index a6f4b44af5..69c21f378b 100644 --- a/git.c +++ b/git.c @@ -31,7 +31,7 @@ const char git_usage_string[] = " <command> [<args>]"); const char git_more_info_string[] = - N_("'git help -a' and 'git help -g' list available subcommands and some\n" + N_("'git help -av' and 'git help -g' list available subcommands and some\n" "concept guides. See 'git help <command>' or 'git help <concept>'\n" "to read about a specific subcommand or concept.");
When you type "git help" (or just "git") you are greeted with a list with commonly used commands and their short description and are suggested to use "git help -a" or "git help -g" for more details. "git help -av" would be more friendly and inline with what is shown with "git help" since it shows list of commands with description as well, and commands are properly grouped. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- git.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)