{"thread":{"id":"56465","subject":"[PATCH 0/6] help: fix usage nits & bugs, completion shellscript->C","startedAt":"2021-09-08T15:24:34Z","lastAt":"2021-09-23T18:05:30Z","messageCount":45,"participants":["Ævar Arnfjörð Bjarmason","Eric Sunshine","Junio C Hamano","Philip Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"435096","messageId":"cover-0.6-00000000000-20210908T151949Z-avarab@gmail.com","threadId":"56465","inReplyTo":null,"subject":"[PATCH 0/6] help: fix usage nits & bugs, completion shellscript->C","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-08T15:24:16Z","receivedAt":"2021-09-08T15:24:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series fixes various bugs & edge cases in the \"git help\" command,\nand improves and splits the internal-only \"--completion-for-config\"\noption into two, and as a result can get rid of an awk/sort -u\npipeline in the bash completion.\n\nThis is all rather straightforward and boring, the first patch\nmentions an interaction with my not-picked-up-by-Junio-yet series to\nfix parse_options usage alignment at [1], but while the two strictly\nspeaking \"semantically conflict\" (in combination they'll produce\ndifferent results), the exact whitespace padding of \"git help -h\" is a\ntrivial enough issue that they can and should be considered\nindepedently.\n\n1. https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (6):\n help: correct the usage string in -h and documentation\n help: correct usage string for \"git help --guides\"\n help tests: add test for --config output\n help: refactor \"for_human\" control flow in cmd_help()\n help: correct logic error in combining --all and --config\n help / completion: make \"git help\" do the hard work\n\n Documentation/git-help.txt | 9 ++-\n builtin/help.c | 102 +++++++++++++++++--------\n contrib/completion/git-completion.bash | 21 +++--\n t/t0012-help.sh | 45 +++++++++++\n 4 files changed, 131 insertions(+), 46 deletions(-)\n\n-- \n2.33.0.825.g2bf60429931\n\n"},{"id":"435097","messageId":"patch-1.6-b5c79d87847-20210908T151949Z-avarab@gmail.com","threadId":"56465","inReplyTo":"cover-0.6-00000000000-20210908T151949Z-avarab@gmail.com","subject":"[PATCH 1/6] help: correct the usage string in -h and documentation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-08T15:24:17Z","receivedAt":"2021-09-08T15:24:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Clarify the usage string in the documentation so we group e.g. -i and\n--info, and add the missing short options to the \"-h\" output.\n\nThe alignment of the second line is off now, but will be fixed with\nanother series of mine[1]. In the meantime let's just assume that fix\nwill make it in eventually for the purposes of this patch, if it's\nmisaligned for a bit it doesn't matter much.\n\n1. https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-help.txt | 2 +-\n builtin/help.c | 3 ++-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-help.txt b/Documentation/git-help.txt\nindex 44fe8860b3f..568a0b606f3 100644\n--- a/Documentation/git-help.txt\n+++ b/Documentation/git-help.txt\n@@ -9,7 +9,7 @@ SYNOPSIS\n --------\n [verse]\n 'git help' [-a|--all [--[no-]verbose]] [-g|--guides]\n-\t [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE]\n+\t [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]\n \n DESCRIPTION\n -----------\ndiff --git a/builtin/help.c b/builtin/help.c\nindex b7eec06c3de..44ea2798cda 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -59,7 +59,8 @@ static struct option builtin_help_options[] = {\n };\n \n static const char * const builtin_help_usage[] = {\n-\tN_(\"git help [--all] [--guides] [--man | --web | --info] []\"),\n+\tN_(\"git help [-a|--all] [-g|--guides] [--[no-]verbose]]\\n\"\n+\t \" [[-i|--info] [-m|--man] [-w|--web]] []\"),\n \tNULL\n };\n \n-- \n2.33.0.825.g2bf60429931\n\n"},{"id":"435098","messageId":"patch-2.6-1ebd443e43c-20210908T151949Z-avarab@gmail.com","threadId":"56465","inReplyTo":"cover-0.6-00000000000-20210908T151949Z-avarab@gmail.com","subject":"[PATCH 2/6] help: correct usage string for \"git help --guides\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-08T15:24:18Z","receivedAt":"2021-09-08T15:24:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As noted in 65f98358c0c (builtin/help.c: add --guide option,\n2013-04-02) and a133737b809 (doc: include --guide option description\nfor \"git help\", 2013-04-02) which introduced the --guide option it\ncannot be combined with e.g. .\n\nChange both the usage string to reflect that, and test and assert for\nthis behavior in the command itself. Now that we assert this in code\nwe don't need to exhaustively describe the previous confusing behavior\nin the documentation either, instead of silently ignoring the provided\nargument we'll now error out.\n\nThe comment being removed was added in 15f7d494380 (builtin/help.c:\nsplit \"-a\" processing into two, 2013-04-02) and is no longer\napplicable as explained above.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-help.txt | 6 +++---\n builtin/help.c | 14 +++++++++-----\n t/t0012-help.sh | 5 +++++\n 3 files changed, 17 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-help.txt b/Documentation/git-help.txt\nindex 568a0b606f3..cb8e3d4da9e 100644\n--- a/Documentation/git-help.txt\n+++ b/Documentation/git-help.txt\n@@ -8,8 +8,9 @@ git-help - Display help information about Git\n SYNOPSIS\n --------\n [verse]\n-'git help' [-a|--all [--[no-]verbose]] [-g|--guides]\n+'git help' [-a|--all [--[no-]verbose]]\n \t [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]\n+'git help' [-g|--guides]\n \n DESCRIPTION\n -----------\n@@ -58,8 +59,7 @@ OPTIONS\n \n -g::\n --guides::\n-\tPrints a list of the Git concept guides on the standard output. This\n-\toption overrides any given command or guide name.\n+\tPrints a list of the Git concept guides on the standard output.\n \n -i::\n --info::\ndiff --git a/builtin/help.c b/builtin/help.c\nindex 44ea2798cda..0f9dc31c40f 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -59,8 +59,9 @@ static struct option builtin_help_options[] = {\n };\n \n static const char * const builtin_help_usage[] = {\n-\tN_(\"git help [-a|--all] [-g|--guides] [--[no-]verbose]]\\n\"\n+\tN_(\"git help [-a|--all] [--[no-]verbose]]\\n\"\n \t \" [[-i|--info] [-m|--man] [-w|--web]] []\"),\n+\tN_(\"git help [-g|--guides]\"),\n \tNULL\n };\n \n@@ -547,11 +548,17 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \tint nongit;\n \tenum help_format parsed_help_format;\n \tconst char *page;\n+\tint standalone = 0;\n \n \targc = parse_options(argc, argv, prefix, builtin_help_options,\n \t\t\tbuiltin_help_usage, 0);\n \tparsed_help_format = help_format;\n \n+\t/* Options that take no further arguments */\n+\tstandalone = show_config || show_guides;\n+\tif (standalone && argc)\n+\t\tusage_with_options(builtin_help_usage, builtin_help_options);\n+\n \tif (show_all) {\n \t\tgit_config(git_help_config, NULL);\n \t\tif (verbose) {\n@@ -580,11 +587,8 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \tif (show_guides)\n \t\tlist_guides_help();\n \n-\tif (show_all || show_guides) {\n+\tif (show_all || standalone) {\n \t\tprintf(\"%s\\n\", _(git_more_info_string));\n-\t\t/*\n-\t\t* We're done. Ignore any remaining args\n-\t\t*/\n \t\treturn 0;\n \t}\n \ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 5679e29c624..6e01da614f0 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -34,6 +34,11 @@ test_expect_success 'basic help commands' '\n \tgit help -a >/dev/null\n '\n \n+test_expect_success 'invalid usage' '\n+\ttest_expect_code 129 git help -c git-add &&\n+\ttest_expect_code 129 git help -g git-add\n+'\n+\n test_expect_success \"works for commands and guides by default\" '\n \tconfigure_help &&\n \tgit help status &&\n-- \n2.33.0.825.g2bf60429931\n\n"},{"id":"435099","messageId":"patch-3.6-d0a8045c9ed-20210908T151949Z-avarab@gmail.com","threadId":"56465","inReplyTo":"cover-0.6-00000000000-20210908T151949Z-avarab@gmail.com","subject":"[PATCH 3/6] help tests: add test for --config output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-08T15:24:19Z","receivedAt":"2021-09-08T15:24:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a missing test for checking what the --config output added in\nac68a93fd2 (help: add --config to list all available config,\n2018-05-26) looks like. We should not be emitting anything except\nconfig variables and the brief usage information at the end here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t0012-help.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 6e01da614f0..94d1f481c8b 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -78,6 +78,19 @@ test_expect_success 'git help -g' '\n \ttest_i18ngrep \"^ tutorial \" help.output\n '\n \n+test_expect_success 'git help -c' '\n+\tgit help -c >help.output &&\n+\tcat >expect <<-\\EOF &&\n+\n+\t'\"'\"'git help config'\"'\"' for more information\n+\tEOF\n+\tgrep -v -E \\\n+\t\t-e \"^[^.]+\\.[^.]+$\" \\\n+\t\t-e \"^[^.]+\\.[^.]+\\.[^.]+$\" \\\n+\t\thelp.output >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'generate builtin list' '\n \tgit --list-cmds=builtins >builtins\n '\n-- \n2.33.0.825.g2bf60429931\n\n"},{"id":"435100","messageId":"patch-4.6-e4bc7e57a6d-20210908T151949Z-avarab@gmail.com","threadId":"56465","inReplyTo":"cover-0.6-00000000000-20210908T151949Z-avarab@gmail.com","subject":"[PATCH 4/6] help: refactor \"for_human\" control flow in cmd_help()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-08T15:24:20Z","receivedAt":"2021-09-08T15:24:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Instead of having two lines that call list_config_help(for_human)\nlet's setup the pager and print the trailer conditionally. This makes\nit clearer at a glance how the two differ in behavior.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/help.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex 0f9dc31c40f..0737b22069b 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -574,13 +574,12 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \tif (show_config) {\n \t\tint for_human = show_config == 1;\n \n-\t\tif (!for_human) {\n-\t\t\tlist_config_help(for_human);\n-\t\t\treturn 0;\n-\t\t}\n-\t\tsetup_pager();\n+\t\tif (for_human)\n+\t\t\tsetup_pager();\n \t\tlist_config_help(for_human);\n-\t\tprintf(\"\\n%s\\n\", _(\"'git help config' for more information\"));\n+\t\tif (for_human)\n+\t\t\tprintf(\"\\n%s\\n\", _(\"'git help config' for more information\"));\n+\n \t\treturn 0;\n \t}\n \n-- \n2.33.0.825.g2bf60429931\n\n"},{"id":"435101","messageId":"patch-5.6-bcc640d32a1-20210908T151949Z-avarab@gmail.com","threadId":"56465","inReplyTo":"cover-0.6-00000000000-20210908T151949Z-avarab@gmail.com","subject":"[PATCH 5/6] help: correct logic error in combining --all and --config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-08T15:24:21Z","receivedAt":"2021-09-08T15:24:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a bug in the --config option that's been there ever since its\nintroduction in 3ac68a93fd2 (help: add --config to list all available\nconfig, 2018-05-26). Die when --all and --config are combined,\ncombining them doesn't make sense.\n\nThe code for the --config option when combined with an earlier\nrefactoring done to support the --guide option in\n65f98358c0c (builtin/help.c: add --guide option, 2013-04-02) would\ncause us to take the \"--all\" branch early and ignore the --config\noption.\n\nLet's instead list these as incompatible, both in the synopsis and\nhelp output, and enforce it in the code itself.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-help.txt | 1 +\n builtin/help.c | 27 ++++++++++++++++++---------\n t/t0012-help.sh | 5 ++++-\n 3 files changed, 23 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-help.txt b/Documentation/git-help.txt\nindex cb8e3d4da9e..96d5f598b4b 100644\n--- a/Documentation/git-help.txt\n+++ b/Documentation/git-help.txt\n@@ -11,6 +11,7 @@ SYNOPSIS\n 'git help' [-a|--all [--[no-]verbose]]\n \t [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]\n 'git help' [-g|--guides]\n+'git help' [-c|--config]\n \n DESCRIPTION\n -----------\ndiff --git a/builtin/help.c b/builtin/help.c\nindex 0737b22069b..83f71d6765e 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -62,6 +62,7 @@ static const char * const builtin_help_usage[] = {\n \tN_(\"git help [-a|--all] [--[no-]verbose]]\\n\"\n \t \" [[-i|--info] [-m|--man] [-w|--web]] []\"),\n \tN_(\"git help [-g|--guides]\"),\n+\tN_(\"git help [-c|--config]\"),\n \tNULL\n };\n \n@@ -549,18 +550,26 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \tenum help_format parsed_help_format;\n \tconst char *page;\n \tint standalone = 0;\n+\tint need_config = 0;\n \n \targc = parse_options(argc, argv, prefix, builtin_help_options,\n \t\t\tbuiltin_help_usage, 0);\n \tparsed_help_format = help_format;\n \n+\t/* Incompatible options */\n+\tif (show_all + !!show_config + show_guides > 1)\n+\t\tusage_with_options(builtin_help_usage, builtin_help_options);\n+\n \t/* Options that take no further arguments */\n \tstandalone = show_config || show_guides;\n \tif (standalone && argc)\n \t\tusage_with_options(builtin_help_usage, builtin_help_options);\n \n-\tif (show_all) {\n+\tneed_config = show_all || show_config;\n+\tif (need_config)\n \t\tgit_config(git_help_config, NULL);\n+\n+\tif (show_all) {\n \t\tif (verbose) {\n \t\t\tsetup_pager();\n \t\t\tlist_all_cmds_help();\n@@ -571,6 +580,14 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \t\tlist_commands(colopts, &main_cmds, &other_cmds);\n \t}\n \n+\tif (show_guides)\n+\t\tlist_guides_help();\n+\n+\tif (show_all || show_guides) {\n+\t\tprintf(\"%s\\n\", _(git_more_info_string));\n+\t\treturn 0;\n+\t}\n+\n \tif (show_config) {\n \t\tint for_human = show_config == 1;\n \n@@ -583,14 +600,6 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \t}\n \n-\tif (show_guides)\n-\t\tlist_guides_help();\n-\n-\tif (show_all || standalone) {\n-\t\tprintf(\"%s\\n\", _(git_more_info_string));\n-\t\treturn 0;\n-\t}\n-\n \tif (!argv[0]) {\n \t\tprintf(_(\"usage: %s%s\"), _(git_usage_string), \"\\n\\n\");\n \t\tlist_common_cmds_help();\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 94d1f481c8b..68e7f57470e 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -36,7 +36,10 @@ test_expect_success 'basic help commands' '\n \n test_expect_success 'invalid usage' '\n \ttest_expect_code 129 git help -c git-add &&\n-\ttest_expect_code 129 git help -g git-add\n+\ttest_expect_code 129 git help -g git-add &&\n+\n+\ttest_expect_code 129 git help -a -c &&\n+\ttest_expect_code 129 git help -g -c\n '\n \n test_expect_success \"works for commands and guides by default\" '\n-- \n2.33.0.825.g2bf60429931\n\n"},{"id":"435102","messageId":"patch-6.6-940061e84d1-20210908T151949Z-avarab@gmail.com","threadId":"56465","inReplyTo":"cover-0.6-00000000000-20210908T151949Z-avarab@gmail.com","subject":"[PATCH 6/6] help / completion: make \"git help\" do the hard work","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-08T15:24:22Z","receivedAt":"2021-09-08T15:24:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"help\" builtin has been able to emit configuration variables since\ne17ca926371 (completion: drop the hard coded list of config vars,\n2018-05-26), but it hasn't produced exactly the format the completion\nscript wanted. Let's do that.\n\nWe got partway there in 2675ea1cc0f (completion: use 'sort -u' to\ndeduplicate config variable names, 2019-08-13) and\nd9438873c4d (completion: deduplicate configuration sections,\n2019-08-13), but after both we still needed some sorting,\nde-duplicating and awk post-processing of the list.\n\nWe can instead simply do the relevant parsing ourselves (we were doing\nmost of it already), and call string_list_remove_duplicates() after\nalready sorting the list, so the caller doesn't need to invoke \"sort\n-u\".\n\nThis changes the output of the section list to emit lines like \"alias\"\ninstead of \"alias.\". The dot suffix is better done as an argument to\n__gitcomp().\n\nThis means that we'll have the list_config_help() function do a bit\nmore work, let's switch its \"for_human\" to passing a full\n\"show_config\", but as an enum type so we can have the compiler check\nwhat values we're expecting to get.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/help.c | 67 ++++++++++++++++++--------\n contrib/completion/git-completion.bash | 21 ++++----\n t/t0012-help.sh | 24 +++++++++\n 3 files changed, 81 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex 83f71d6765e..b33ef27ac79 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -38,7 +38,12 @@ static const char *html_path;\n \n static int show_all = 0;\n static int show_guides = 0;\n-static int show_config;\n+enum show_config_type {\n+\tSHOW_CONFIG_UNSET = 0,\n+\tSHOW_CONFIG_HUMAN,\n+\tSHOW_CONFIG_VARS,\n+\tSHOW_CONFIG_SECTIONS,\n+} show_config;\n static int verbose = 1;\n static unsigned int colopts;\n static enum help_format help_format = HELP_FORMAT_NONE;\n@@ -48,7 +53,10 @@ static struct option builtin_help_options[] = {\n \tOPT_HIDDEN_BOOL(0, \"exclude-guides\", &exclude_guides, N_(\"exclude guides\")),\n \tOPT_BOOL('g', \"guides\", &show_guides, N_(\"print list of useful guides\")),\n \tOPT_BOOL('c', \"config\", &show_config, N_(\"print all configuration variable names\")),\n-\tOPT_SET_INT_F(0, \"config-for-completion\", &show_config, \"\", 2, PARSE_OPT_HIDDEN),\n+\tOPT_SET_INT_F(0, \"config-for-completion-vars\", &show_config, \"\",\n+\t\t SHOW_CONFIG_VARS, PARSE_OPT_HIDDEN),\n+\tOPT_SET_INT_F(0, \"config-for-completion-sections\", &show_config, \"\",\n+\t\t SHOW_CONFIG_SECTIONS, PARSE_OPT_HIDDEN),\n \tOPT_SET_INT('m', \"man\", &help_format, N_(\"show man page\"), HELP_FORMAT_MAN),\n \tOPT_SET_INT('w', \"web\", &help_format, N_(\"show manual in web browser\"),\n \t\t\tHELP_FORMAT_WEB),\n@@ -73,7 +81,7 @@ struct slot_expansion {\n \tint found;\n };\n \n-static void list_config_help(int for_human)\n+static void list_config_help(enum show_config_type type)\n {\n \tstruct slot_expansion slot_expansions[] = {\n \t\t{ \"advice\", \"*\", list_config_advices },\n@@ -91,6 +99,8 @@ static void list_config_help(int for_human)\n \tconst char **p;\n \tstruct slot_expansion *e;\n \tstruct string_list keys = STRING_LIST_INIT_DUP;\n+\tstruct string_list keys_uniq = STRING_LIST_INIT_DUP;\n+\tstruct string_list_item *item;\n \tint i;\n \n \tfor (p = config_name_list; *p; p++) {\n@@ -121,34 +131,48 @@ static void list_config_help(int for_human)\n \tfor (i = 0; i < keys.nr; i++) {\n \t\tconst char *var = keys.items[i].string;\n \t\tconst char *wildcard, *tag, *cut;\n+\t\tconst char *dot = NULL;\n+\t\tstruct strbuf sb = STRBUF_INIT;\n \n-\t\tif (for_human) {\n+\t\tswitch (type) {\n+\t\tcase SHOW_CONFIG_HUMAN:\n \t\t\tputs(var);\n \t\t\tcontinue;\n+\t\tcase SHOW_CONFIG_SECTIONS:\n+\t\t\tdot = strchr(var, '.');\n+\t\t\tbreak;\n+\t\tcase SHOW_CONFIG_VARS:\n+\t\t\tbreak;\n+\t\tcase SHOW_CONFIG_UNSET:\n+\t\t\tBUG(\"should not get SHOW_CONFIG_UNSET here\");\n \t\t}\n-\n \t\twildcard = strchr(var, '*');\n \t\ttag = strchr(var, '<');\n \n-\t\tif (!wildcard && !tag) {\n-\t\t\tputs(var);\n+\t\tif (!dot && !wildcard && !tag) {\n+\t\t\tstring_list_append(&keys_uniq, var);\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (wildcard && !tag)\n+\t\tif (dot)\n+\t\t\tcut = dot;\n+\t\telse if (wildcard && !tag)\n \t\t\tcut = wildcard;\n \t\telse if (!wildcard && tag)\n \t\t\tcut = tag;\n \t\telse\n \t\t\tcut = wildcard < tag ? wildcard : tag;\n \n-\t\t/*\n-\t\t * We may produce duplicates, but that's up to\n-\t\t * git-completion.bash to handle\n-\t\t */\n-\t\tprintf(\"%.*s\\n\", (int)(cut - var), var);\n+\t\tstrbuf_add(&sb, var, cut - var);\n+\t\tstring_list_append(&keys_uniq, sb.buf);\n+\t\tstrbuf_release(&sb);\n+\n \t}\n \tstring_list_clear(&keys, 0);\n+\tstring_list_remove_duplicates(&keys_uniq, 0);\n+\tfor_each_string_list_item(item, &keys_uniq)\n+\t\tputs(item->string);\n+\tstring_list_clear(&keys_uniq, 0);\n }\n \n static enum help_format parse_help_format(const char *format)\n@@ -588,13 +612,16 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \t}\n \n-\tif (show_config) {\n-\t\tint for_human = show_config == 1;\n-\n-\t\tif (for_human)\n-\t\t\tsetup_pager();\n-\t\tlist_config_help(for_human);\n-\t\tif (for_human)\n+\tswitch (show_config) {\n+\tcase SHOW_CONFIG_UNSET:\n+\t\tbreak;\n+\tcase SHOW_CONFIG_HUMAN:\n+\t\tsetup_pager();\n+\t\t/* fallthrough */\n+\tcase SHOW_CONFIG_VARS:\n+\tcase SHOW_CONFIG_SECTIONS:\n+\t\tlist_config_help(show_config);\n+\t\tif (show_config == SHOW_CONFIG_HUMAN)\n \t\t\tprintf(\"\\n%s\\n\", _(\"'git help config' for more information\"));\n \n \t\treturn 0;\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 8108eda1e86..19b8a172878 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2503,7 +2503,14 @@ __git_config_vars=\n __git_compute_config_vars ()\n {\n \ttest -n \"$__git_config_vars\" ||\n-\t__git_config_vars=\"$(git help --config-for-completion | sort -u)\"\n+\t__git_config_vars=\"$(git help --config-for-completion-vars)\"\n+}\n+\n+__git_config_sections=\n+__git_compute_config_sections ()\n+{\n+\ttest -n \"$__git_config_sections\" ||\n+\t__git_config_sections=\"$(git help --config-for-completion-sections)\"\n }\n \n # Completes possible values of various configuration variables.\n@@ -2717,16 +2724,8 @@ __git_complete_config_variable_name ()\n \t\t__gitcomp \"$__git_config_vars\" \"\" \"$cur_\" \"$sfx\"\n \t\t;;\n \t*)\n-\t\t__git_compute_config_vars\n-\t\t__gitcomp \"$(echo \"$__git_config_vars\" |\n-\t\t\t\tawk -F . '{\n-\t\t\t\t\tsections[$1] = 1\n-\t\t\t\t}\n-\t\t\t\tEND {\n-\t\t\t\t\tfor (s in sections)\n-\t\t\t\t\t\tprint s \".\"\n-\t\t\t\t}\n-\t\t\t\t')\" \"\" \"$cur_\"\n+\t\t__git_compute_config_sections\n+\t\t__gitcomp \"$__git_config_sections\" \"\" \"$cur_\" \".\"\n \t\t;;\n \tesac\n }\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 68e7f57470e..6d169ae3cbd 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -94,6 +94,30 @@ test_expect_success 'git help -c' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'git help --config-for-completion-vars' '\n+\tgit help -c >human &&\n+\tgrep -E \\\n+\t -e \"^[^.]+\\.[^.]+$\" \\\n+\t -e \"^[^.]+\\.[^.]+\\.[^.]+$\" human |\n+\t sed -e \"s/\\*.*//\" -e \"s/<.*//\" |\n+\t sort -u >human.munged &&\n+\n+\tgit help --config-for-completion-vars >vars &&\n+\ttest_cmp human.munged vars\n+'\n+\n+test_expect_success 'git help --config-for-completion-sections' '\n+\tgit help -c >human &&\n+\tgrep -E \\\n+\t -e \"^[^.]+\\.[^.]+$\" \\\n+\t -e \"^[^.]+\\.[^.]+\\.[^.]+$\" human |\n+\t sed -e \"s/\\..*//\" |\n+\t sort -u >human.munged &&\n+\n+\tgit help --config-for-completion-sections >sections &&\n+\ttest_cmp human.munged sections\n+'\n+\n test_expect_success 'generate builtin list' '\n \tgit --list-cmds=builtins >builtins\n '\n-- \n2.33.0.825.g2bf60429931\n\n"},{"id":"435120","messageId":"CAPig+cSK+wLPUDuGf1d41J_F5jQS+J=a=7kHQLV07-ZKZW9GsA@mail.gmail.com","threadId":"56465","inReplyTo":"patch-5.6-bcc640d32a1-20210908T151949Z-avarab@gmail.com","subject":"Re: [PATCH 5/6] help: correct logic error in combining --all and --config","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-08T16:39:02Z","receivedAt":"2021-09-08T16:39:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason\n wrote:\n> Fix a bug in the --config option that's been there ever since its\n> introduction in 3ac68a93fd2 (help: add --config to list all available\n> config, 2018-05-26). Die when --all and --config are combined,\n> combining them doesn't make sense.\n>\n> The code for the --config option when combined with an earlier\n> refactoring done to support the --guide option in\n> 65f98358c0c (builtin/help.c: add --guide option, 2013-04-02) would\n> cause us to take the \"--all\" branch early and ignore the --config\n> option.\n>\n> Let's instead list these as incompatible, both in the synopsis and\n> help output, and enforce it in the code itself.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> diff --git a/builtin/help.c b/builtin/help.c\n> @@ -549,18 +550,26 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n> + /* Incompatible options */\n> + if (show_all + !!show_config + show_guides > 1)\n> + usage_with_options(builtin_help_usage, builtin_help_options);\n\nI personally find it highly frustrating when a program merely dumps\nthe usage statement without any explanation of what exactly it doesn't\nlike about the command-line. Printing out a simple:\n\n --all, --guides, --config are mutually exclusive\n\nmessage would go a long way toward reducing the frustration level.\n\n(Aside: I also find it more hostile than helpful when programs dump\nthe usage statement for a command-line invocation error -- even if\npreceded by an explanation of the error -- since the explanation\nusually gets drowned-out by the often multi-page usage text, and the\nuser has to go spelunking around the wall of output to try to figure\nout what actually went wrong. It's much more helpful and easy to\nfigure out what went wrong with the invocation when only a simple\nerror message is printed -- without usage statement. However, that's a\nseparate battle, as Git already has plenty of places which dump the\nusage statement in response to an invocation error.)\n"},{"id":"435121","messageId":"CAPig+cQVsFvkLMXrm7d3mCUoJKj7x+0PNcAYJ0-uFHaqbRw0ag@mail.gmail.com","threadId":"56465","inReplyTo":"patch-4.6-e4bc7e57a6d-20210908T151949Z-avarab@gmail.com","subject":"Re: [PATCH 4/6] help: refactor \"for_human\" control flow in cmd_help()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-08T16:41:46Z","receivedAt":"2021-09-08T16:42:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason\n wrote:\n> Instead of having two lines that call list_config_help(for_human)\n> let's setup the pager and print the trailer conditionally. This makes\n> it clearer at a glance how the two differ in behavior.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> @@ -574,13 +574,12 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n> - if (!for_human) {\n> - list_config_help(for_human);\n> - return 0;\n> - }\n> - setup_pager();\n> + if (for_human)\n> + setup_pager();\n> list_config_help(for_human);\n> - printf(\"\\n%s\\n\", _(\"'git help config' for more information\"));\n> + if (for_human)\n> + printf(\"\\n%s\\n\", _(\"'git help config' for more information\"));\n> +\n\nFor what it's worth, I find the original logic easier to reason about\nsince it gets the \"simple\" case out of the way early, thus I don't\nhave to keep as much (or any) state in mind as I'm reading the rest of\nthe code. However, it's highly subjective, of course; just one\nperson's opinion.\n"},{"id":"435124","messageId":"xmqqsfyf3srv.fsf@gitster.g","threadId":"56465","inReplyTo":"CAPig+cQVsFvkLMXrm7d3mCUoJKj7x+0PNcAYJ0-uFHaqbRw0ag@mail.gmail.com","subject":"Re: [PATCH 4/6] help: refactor \"for_human\" control flow in cmd_help()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-08T17:02:12Z","receivedAt":"2021-09-08T17:02:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason\n> wrote:\n>> Instead of having two lines that call list_config_help(for_human)\n>> let's setup the pager and print the trailer conditionally. This makes\n>> it clearer at a glance how the two differ in behavior.\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> @@ -574,13 +574,12 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n>> - if (!for_human) {\n>> - list_config_help(for_human);\n>> - return 0;\n>> - }\n>> - setup_pager();\n>> + if (for_human)\n>> + setup_pager();\n>> list_config_help(for_human);\n>> - printf(\"\\n%s\\n\", _(\"'git help config' for more information\"));\n>> + if (for_human)\n>> + printf(\"\\n%s\\n\", _(\"'git help config' for more information\"));\n>> +\n>\n> For what it's worth, I find the original logic easier to reason about\n> since it gets the \"simple\" case out of the way early, thus I don't\n> have to keep as much (or any) state in mind as I'm reading the rest of\n> the code. However, it's highly subjective, of course; just one\n> person's opinion.\n\nFWIW, it makes two of us ;-)\n\nQuite honestly, I do not see much commonality in the code above that\ntargets two different audiences, so whether you handle simple one or\ncomplex one first, a single big switch upfront that gives clearly\nseparate control flow to two distinct cases is easier to follow than\n\"as the middle step that calls list_config_help() is the same, let's\nhave two conditionals before and after and serve these two audiences\nwith a single code path that is slightly tweaked\", which is the\nresult of this patch.\n\n"},{"id":"435171","messageId":"87fsuehnal.fsf@evledraar.gmail.com","threadId":"56465","inReplyTo":"xmqqsfyf3srv.fsf@gitster.g","subject":"Re: [PATCH 4/6] help: refactor \"for_human\" control flow in cmd_help()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-08T19:36:04Z","receivedAt":"2021-09-08T19:36:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 08 2021, Junio C Hamano wrote:\n\n> Eric Sunshine writes:\n>\n>> On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason\n>> wrote:\n>>> Instead of having two lines that call list_config_help(for_human)\n>>> let's setup the pager and print the trailer conditionally. This makes\n>>> it clearer at a glance how the two differ in behavior.\n>>>\n>>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>>> ---\n>>> @@ -574,13 +574,12 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n>>> - if (!for_human) {\n>>> - list_config_help(for_human);\n>>> - return 0;\n>>> - }\n>>> - setup_pager();\n>>> + if (for_human)\n>>> + setup_pager();\n>>> list_config_help(for_human);\n>>> - printf(\"\\n%s\\n\", _(\"'git help config' for more information\"));\n>>> + if (for_human)\n>>> + printf(\"\\n%s\\n\", _(\"'git help config' for more information\"));\n>>> +\n>>\n>> For what it's worth, I find the original logic easier to reason about\n>> since it gets the \"simple\" case out of the way early, thus I don't\n>> have to keep as much (or any) state in mind as I'm reading the rest of\n>> the code. However, it's highly subjective, of course; just one\n>> person's opinion.\n>\n> FWIW, it makes two of us ;-)\n>\n> Quite honestly, I do not see much commonality in the code above that\n> targets two different audiences, so whether you handle simple one or\n> complex one first, a single big switch upfront that gives clearly\n> separate control flow to two distinct cases is easier to follow than\n> \"as the middle step that calls list_config_help() is the same, let's\n> have two conditionals before and after and serve these two audiences\n> with a single code path that is slightly tweaked\", which is the\n> result of this patch.\n\nWhat do you two think of the end-state of this in 6/6? I went back &\nforth a bit with whether to keep this similar to pre-4/6 logic, or the\nswitch statement in 6/6, and then for that switch statement whether to\nhave the fallthrough case or not. Perhaps 6/6 with no fallthrough (but a\nbit of duplication) is the best?\n"},{"id":"435172","messageId":"87bl52hn40.fsf@evledraar.gmail.com","threadId":"56465","inReplyTo":"CAPig+cSK+wLPUDuGf1d41J_F5jQS+J=a=7kHQLV07-ZKZW9GsA@mail.gmail.com","subject":"Re: [PATCH 5/6] help: correct logic error in combining --all and --config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-08T19:37:01Z","receivedAt":"2021-09-08T19:40:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 08 2021, Eric Sunshine wrote:\n\n> On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason\n> wrote:\n>> Fix a bug in the --config option that's been there ever since its\n>> introduction in 3ac68a93fd2 (help: add --config to list all available\n>> config, 2018-05-26). Die when --all and --config are combined,\n>> combining them doesn't make sense.\n>>\n>> The code for the --config option when combined with an earlier\n>> refactoring done to support the --guide option in\n>> 65f98358c0c (builtin/help.c: add --guide option, 2013-04-02) would\n>> cause us to take the \"--all\" branch early and ignore the --config\n>> option.\n>>\n>> Let's instead list these as incompatible, both in the synopsis and\n>> help output, and enforce it in the code itself.\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> diff --git a/builtin/help.c b/builtin/help.c\n>> @@ -549,18 +550,26 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n>> + /* Incompatible options */\n>> + if (show_all + !!show_config + show_guides > 1)\n>> + usage_with_options(builtin_help_usage, builtin_help_options);\n>\n> I personally find it highly frustrating when a program merely dumps\n> the usage statement without any explanation of what exactly it doesn't\n> like about the command-line. Printing out a simple:\n>\n> --all, --guides, --config are mutually exclusive\n>\n> message would go a long way toward reducing the frustration level.\n>\n> (Aside: I also find it more hostile than helpful when programs dump\n> the usage statement for a command-line invocation error -- even if\n> preceded by an explanation of the error -- since the explanation\n> usually gets drowned-out by the often multi-page usage text, and the\n> user has to go spelunking around the wall of output to try to figure\n> out what actually went wrong. It's much more helpful and easy to\n> figure out what went wrong with the invocation when only a simple\n> error message is printed -- without usage statement. However, that's a\n> separate battle, as Git already has plenty of places which dump the\n> usage statement in response to an invocation error.)\n\nI'll make it emit something more helpful.\n\nMore generally I've got quite a bit of parse_options() improvements\nqueued up locally that I've been trying to trickle in at the rate I can\nget them through the list, review over at [1] would be much appreciated.\n\nI wonder if we can do this automatically, we already have the\nbuiltin_help_usage, we could parse that in the general case and find\nthat certain options are mutually exclusive per the examples there.\n\nWe'd then discover what option we parsed when usage_with_options() was\ncalled, and automatically emit a useful message in these sorts of cases.\n\nOf course the usage strings might be incomplete or wrong, but part of\nthe point would be to find those cases & a test mode to die() if a\ncommand was called with some option combinations not suggested as\nworking according to its documented usage...\n\nhttps://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com/\n"},{"id":"435372","messageId":"CAPig+cROO1VT9Umdtor9zc8eftyDTBg+ER2igZG+jxygM9gLJw@mail.gmail.com","threadId":"56465","inReplyTo":"87bl52hn40.fsf@evledraar.gmail.com","subject":"Re: [PATCH 5/6] help: correct logic error in combining --all and --config","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-10T08:08:50Z","receivedAt":"2021-09-10T08:09:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 8, 2021 at 3:40 PM Ævar Arnfjörð Bjarmason wrote:\n> On Wed, Sep 08 2021, Eric Sunshine wrote:\n> > I personally find it highly frustrating when a program merely dumps\n> > the usage statement without any explanation of what exactly it doesn't\n> > like about the command-line. Printing out a simple:\n> >\n> > --all, --guides, --config are mutually exclusive\n> >\n> > message would go a long way toward reducing the frustration level.\n>\n> I'll make it emit something more helpful.\n>\n> More generally I've got quite a bit of parse_options() improvements\n> queued up locally that I've been trying to trickle in at the rate I can\n> get them through the list, review over at [1] would be much appreciated.\n> https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com/\n\nMy review time is very limited these days (which is why most of my\nreview comments lately are superficial), but I set aside some time to\nreview that series for you[1]. Most of my review is subjective, but I\ndid identify one lurking bug (assuming I understand the code\ncorrectly).\n\n[1]: https://lore.kernel.org/git/f8560b11-ba56-0a52-8bb4-5b71f0657764@sunshineco.com/\n\n> I wonder if we can do this automatically, we already have the\n> builtin_help_usage, we could parse that in the general case and find\n> that certain options are mutually exclusive per the examples there.\n>\n> We'd then discover what option we parsed when usage_with_options() was\n> called, and automatically emit a useful message in these sorts of cases.\n>\n> Of course the usage strings might be incomplete or wrong, but part of\n> the point would be to find those cases & a test mode to die() if a\n> command was called with some option combinations not suggested as\n> working according to its documented usage...\n\nPerhaps, though I imagine it would have to employ some, um,\n\"interesting\" heuristics, and be quite hit-and-miss at first, at least\nuntil people get around to formalizing existing and new usage strings\nwith the specific goal of supporting that feature.\n\nSpeaking of heuristics and wishful thinking, when I read the cover\nletter of your series which I just reviewed, I thought at first that\nthe end-goal would be to ignore whatever indentation the caller\nprovided following each embedded newline, and instead insert the\ncorrect computed indentation automatically. This approach would\nobviate the need for the [1/2] indentation cleanup patch. However,\ndoing so would require heuristics or at least manual help from the\ncaller to indicate the proper indentation width. I also thought\nperhaps the intention of the series was to do the line-wrapping\nautomatically (ignoring any caller-provided embedded newlines), thus\nensuring that the lines were indented correctly _and_ fit the terminal\nwidth properly regardless, but that's a somewhat more substantial\nchange.\n"},{"id":"435394","messageId":"87y284d6nc.fsf@evledraar.gmail.com","threadId":"56465","inReplyTo":"CAPig+cROO1VT9Umdtor9zc8eftyDTBg+ER2igZG+jxygM9gLJw@mail.gmail.com","subject":"Re: [PATCH 5/6] help: correct logic error in combining --all and --config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-10T11:09:28Z","receivedAt":"2021-09-10T11:14:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Sep 10 2021, Eric Sunshine wrote:\n\n> On Wed, Sep 8, 2021 at 3:40 PM Ævar Arnfjörð Bjarmason wrote:\n>> On Wed, Sep 08 2021, Eric Sunshine wrote:\n>> > I personally find it highly frustrating when a program merely dumps\n>> > the usage statement without any explanation of what exactly it doesn't\n>> > like about the command-line. Printing out a simple:\n>> >\n>> > --all, --guides, --config are mutually exclusive\n>> >\n>> > message would go a long way toward reducing the frustration level.\n>>\n>> I'll make it emit something more helpful.\n>>\n>> More generally I've got quite a bit of parse_options() improvements\n>> queued up locally that I've been trying to trickle in at the rate I can\n>> get them through the list, review over at [1] would be much appreciated.\n>> https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com/\n>\n> My review time is very limited these days (which is why most of my\n> review comments lately are superficial), but I set aside some time to\n> review that series for you[1]. Most of my review is subjective, but I\n> did identify one lurking bug (assuming I understand the code\n> correctly).\n\nThanks, I'll try to take a look at that in detail & re-roll soon.\n\n> [1]: https://lore.kernel.org/git/f8560b11-ba56-0a52-8bb4-5b71f0657764@sunshineco.com/\n>\n>> I wonder if we can do this automatically, we already have the\n>> builtin_help_usage, we could parse that in the general case and find\n>> that certain options are mutually exclusive per the examples there.\n>>\n>> We'd then discover what option we parsed when usage_with_options() was\n>> called, and automatically emit a useful message in these sorts of cases.\n>>\n>> Of course the usage strings might be incomplete or wrong, but part of\n>> the point would be to find those cases & a test mode to die() if a\n>> command was called with some option combinations not suggested as\n>> working according to its documented usage...\n>\n> Perhaps, though I imagine it would have to employ some, um,\n> \"interesting\" heuristics, and be quite hit-and-miss at first, at least\n> until people get around to formalizing existing and new usage strings\n> with the specific goal of supporting that feature.\n>\n> Speaking of heuristics and wishful thinking, when I read the cover\n> letter of your series which I just reviewed, I thought at first that\n> the end-goal would be to ignore whatever indentation the caller\n> provided following each embedded newline, and instead insert the\n> correct computed indentation automatically. This approach would\n> obviate the need for the [1/2] indentation cleanup patch. However,\n> doing so would require heuristics or at least manual help from the\n> caller to indicate the proper indentation width. I also thought\n> perhaps the intention of the series was to do the line-wrapping\n> automatically (ignoring any caller-provided embedded newlines), thus\n> ensuring that the lines were indented correctly _and_ fit the terminal\n> width properly regardless, but that's a somewhat more substantial\n> change.\n\nYeah, I considered it but decided not to mainly not for the heuristics\nreason, but that any such thing means that you won't have the same\nindentation in the C code. I.e. instead of:\n\n \"git foo [--some-option]\\n\"\n \" [-even -more options]\"\n\nYou'd have:\n\n \"git foo [--some-option]\\n\"\n \"[--even --more options]\"\n\nWhich I think is just not as legible, but also in the general case you\nget into the quagmire of how do align a thing like:\n\n \"git [-c foo=bar] something [--option]\\n\"\n \"[--more-options]\"\n\nAre we doing to align on the \"git \" or \"git [-c foo=bar] something \", or\nis it a \"git something\" where \"something\" is the name of a sub-command,\nor is that a filename?\n\nWe control the source code so we can do it, but I thought it would be\nnasty, and in any case any working solution wouldn't give you alignment\nin the source code, so I dropped the idea.\n\n"},{"id":"435396","messageId":"cover-v2-0.5-00000000000-20210910T112545Z-avarab@gmail.com","threadId":"56465","inReplyTo":"cover-0.6-00000000000-20210908T151949Z-avarab@gmail.com","subject":"[PATCH v2 0/5] help: fix usage nits & bugs, completion shellscript->C","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-10T11:28:41Z","receivedAt":"2021-09-10T11:30:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series fixes various bugs & edge cases in the \"git help\" command,\nand improves and splits the internal-only \"--completion-for-config\"\noption into two, and as a result can get rid of an awk/sort -u\npipeline in the bash completion.\n\nFor v1, see : https://lore.kernel.org/git/cover-0.6-00000000000-20210908T151949Z-avarab@gmail.com/\n\nThis should address all the feedback on v1 and more. I dropped the 4/6\nrefactoring change, we now also specifically say what option\ncombinations are bad before emitting usage info, as requested by Eric\nSunshine.\n\nÆvar Arnfjörð Bjarmason (5):\n help: correct the usage string in -h and documentation\n help: correct usage & behavior of \"git help --guides\"\n help tests: add test for --config output\n help: correct logic error in combining --all and --config\n help / completion: make \"git help\" do the hard work\n\n Documentation/git-help.txt | 9 +-\n builtin/help.c | 110 ++++++++++++++++++-------\n contrib/completion/git-completion.bash | 21 +++--\n t/t0012-help.sh | 46 +++++++++++\n 4 files changed, 140 insertions(+), 46 deletions(-)\n\nRange-diff against v1:\n1: b5c79d87847 = 1: b10bfd21f14 help: correct the usage string in -h and documentation\n2: 1ebd443e43c ! 2: 039639a0dd3 help: correct usage string for \"git help --guides\"\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - help: correct usage string for \"git help --guides\"\n + help: correct usage & behavior of \"git help --guides\"\n \n As noted in 65f98358c0c (builtin/help.c: add --guide option,\n 2013-04-02) and a133737b809 (doc: include --guide option description\n @@ Commit message\n argument we'll now error out.\n \n The comment being removed was added in 15f7d494380 (builtin/help.c:\n - split \"-a\" processing into two, 2013-04-02) and is no longer\n - applicable as explained above.\n + split \"-a\" processing into two, 2013-04-02). The \"Ignore any remaining\n + args\" part of it is now no longer applicable as explained above, let's\n + just remove it entirely, it's rather obvious that if we're returning\n + we're done.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n @@ builtin/help.c: static struct option builtin_help_options[] = {\n };\n \n @@ builtin/help.c: int cmd_help(int argc, const char **argv, const char *prefix)\n - \tint nongit;\n - \tenum help_format parsed_help_format;\n - \tconst char *page;\n -+\tint standalone = 0;\n - \n - \targc = parse_options(argc, argv, prefix, builtin_help_options,\n \t\t\tbuiltin_help_usage, 0);\n \tparsed_help_format = help_format;\n \n +\t/* Options that take no further arguments */\n -+\tstandalone = show_config || show_guides;\n -+\tif (standalone && argc)\n -+\t\tusage_with_options(builtin_help_usage, builtin_help_options);\n ++\tif (argc && show_guides)\n ++\t\tusage_msg_opt(_(\"--guides cannot be combined with other options\"),\n ++\t\t\t builtin_help_usage, builtin_help_options);\n +\n \tif (show_all) {\n \t\tgit_config(git_help_config, NULL);\n \t\tif (verbose) {\n @@ builtin/help.c: int cmd_help(int argc, const char **argv, const char *prefix)\n - \tif (show_guides)\n - \t\tlist_guides_help();\n \n --\tif (show_all || show_guides) {\n -+\tif (show_all || standalone) {\n + \tif (show_all || show_guides) {\n \t\tprintf(\"%s\\n\", _(git_more_info_string));\n -\t\t/*\n -\t\t* We're done. Ignore any remaining args\n @@ t/t0012-help.sh: test_expect_success 'basic help commands' '\n '\n \n +test_expect_success 'invalid usage' '\n -+\ttest_expect_code 129 git help -c git-add &&\n +\ttest_expect_code 129 git help -g git-add\n +'\n +\n3: d0a8045c9ed = 3: 258282095de help tests: add test for --config output\n4: e4bc7e57a6d < -: ----------- help: refactor \"for_human\" control flow in cmd_help()\n5: bcc640d32a1 ! 4: 32d73d5273c help: correct logic error in combining --all and --config\n @@ builtin/help.c: static const char * const builtin_help_usage[] = {\n };\n \n @@ builtin/help.c: int cmd_help(int argc, const char **argv, const char *prefix)\n + \tint nongit;\n \tenum help_format parsed_help_format;\n \tconst char *page;\n - \tint standalone = 0;\n +\tint need_config = 0;\n \n \targc = parse_options(argc, argv, prefix, builtin_help_options,\n @@ builtin/help.c: int cmd_help(int argc, const char **argv, const char *prefix)\n \tparsed_help_format = help_format;\n \n +\t/* Incompatible options */\n -+\tif (show_all + !!show_config + show_guides > 1)\n -+\t\tusage_with_options(builtin_help_usage, builtin_help_options);\n ++\tif (show_all && show_config)\n ++\t\tusage_msg_opt(_(\"--config and --all cannot be combined\"),\n ++\t\t\t builtin_help_usage, builtin_help_options);\n ++\n ++\tif (show_config && show_guides)\n ++\t\tusage_msg_opt(_(\"--config and --guides cannot be combined\"),\n ++\t\t\t builtin_help_usage, builtin_help_options);\n +\n \t/* Options that take no further arguments */\n - \tstandalone = show_config || show_guides;\n - \tif (standalone && argc)\n - \t\tusage_with_options(builtin_help_usage, builtin_help_options);\n ++\tif (argc && show_config)\n ++\t\tusage_msg_opt(_(\"--config cannot be combined with command or guide names\"),\n ++\t\t\t builtin_help_usage, builtin_help_options);\n + \tif (argc && show_guides)\n +-\t\tusage_msg_opt(_(\"--guides cannot be combined with other options\"),\n ++\t\tusage_msg_opt(_(\"--guides cannot be combined with command or guide names\"),\n + \t\t\t builtin_help_usage, builtin_help_options);\n \n -\tif (show_all) {\n +\tneed_config = show_all || show_config;\n @@ builtin/help.c: int cmd_help(int argc, const char **argv, const char *prefix)\n -\tif (show_guides)\n -\t\tlist_guides_help();\n -\n --\tif (show_all || standalone) {\n +-\tif (show_all || show_guides) {\n -\t\tprintf(\"%s\\n\", _(git_more_info_string));\n -\t\treturn 0;\n -\t}\n @@ builtin/help.c: int cmd_help(int argc, const char **argv, const char *prefix)\n \n ## t/t0012-help.sh ##\n @@ t/t0012-help.sh: test_expect_success 'basic help commands' '\n + '\n \n test_expect_success 'invalid usage' '\n - \ttest_expect_code 129 git help -c git-add &&\n -\ttest_expect_code 129 git help -g git-add\n +\ttest_expect_code 129 git help -g git-add &&\n ++\ttest_expect_code 129 git help -c git-add &&\n ++\ttest_expect_code 129 git help -g git-add &&\n +\n +\ttest_expect_code 129 git help -a -c &&\n +\ttest_expect_code 129 git help -g -c\n6: 940061e84d1 ! 5: e995a42cb8d help / completion: make \"git help\" do the hard work\n @@ builtin/help.c: int cmd_help(int argc, const char **argv, const char *prefix)\n \n -\tif (show_config) {\n -\t\tint for_human = show_config == 1;\n --\n --\t\tif (for_human)\n --\t\t\tsetup_pager();\n --\t\tlist_config_help(for_human);\n --\t\tif (for_human)\n +\tswitch (show_config) {\n +\tcase SHOW_CONFIG_UNSET:\n +\t\tbreak;\n -+\tcase SHOW_CONFIG_HUMAN:\n -+\t\tsetup_pager();\n -+\t\t/* fallthrough */\n +\tcase SHOW_CONFIG_VARS:\n +\tcase SHOW_CONFIG_SECTIONS:\n +\t\tlist_config_help(show_config);\n -+\t\tif (show_config == SHOW_CONFIG_HUMAN)\n - \t\t\tprintf(\"\\n%s\\n\", _(\"'git help config' for more information\"));\n \n +-\t\tif (!for_human) {\n +-\t\t\tlist_config_help(for_human);\n +-\t\t\treturn 0;\n +-\t\t}\n ++\t\treturn 0;\n ++\tcase SHOW_CONFIG_HUMAN:\n + \t\tsetup_pager();\n +-\t\tlist_config_help(for_human);\n ++\t\tlist_config_help(show_config);\n + \t\tprintf(\"\\n%s\\n\", _(\"'git help config' for more information\"));\n ++\n \t\treturn 0;\n + \t}\n + \n \n ## contrib/completion/git-completion.bash ##\n @@ contrib/completion/git-completion.bash: __git_config_vars=\n-- \n2.33.0.873.g125ff7b9940\n\n"},{"id":"435397","messageId":"patch-v2-1.5-b10bfd21f14-20210910T112545Z-avarab@gmail.com","threadId":"56465","inReplyTo":"cover-v2-0.5-00000000000-20210910T112545Z-avarab@gmail.com","subject":"[PATCH v2 1/5] help: correct the usage string in -h and documentation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-10T11:28:42Z","receivedAt":"2021-09-10T11:30:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Clarify the usage string in the documentation so we group e.g. -i and\n--info, and add the missing short options to the \"-h\" output.\n\nThe alignment of the second line is off now, but will be fixed with\nanother series of mine[1]. In the meantime let's just assume that fix\nwill make it in eventually for the purposes of this patch, if it's\nmisaligned for a bit it doesn't matter much.\n\n1. https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-help.txt | 2 +-\n builtin/help.c | 3 ++-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-help.txt b/Documentation/git-help.txt\nindex 44fe8860b3f..568a0b606f3 100644\n--- a/Documentation/git-help.txt\n+++ b/Documentation/git-help.txt\n@@ -9,7 +9,7 @@ SYNOPSIS\n --------\n [verse]\n 'git help' [-a|--all [--[no-]verbose]] [-g|--guides]\n-\t [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE]\n+\t [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]\n \n DESCRIPTION\n -----------\ndiff --git a/builtin/help.c b/builtin/help.c\nindex b7eec06c3de..44ea2798cda 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -59,7 +59,8 @@ static struct option builtin_help_options[] = {\n };\n \n static const char * const builtin_help_usage[] = {\n-\tN_(\"git help [--all] [--guides] [--man | --web | --info] []\"),\n+\tN_(\"git help [-a|--all] [-g|--guides] [--[no-]verbose]]\\n\"\n+\t \" [[-i|--info] [-m|--man] [-w|--web]] []\"),\n \tNULL\n };\n \n-- \n2.33.0.873.g125ff7b9940\n\n"},{"id":"435398","messageId":"patch-v2-2.5-039639a0dd3-20210910T112545Z-avarab@gmail.com","threadId":"56465","inReplyTo":"cover-v2-0.5-00000000000-20210910T112545Z-avarab@gmail.com","subject":"[PATCH v2 2/5] help: correct usage & behavior of \"git help --guides\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-10T11:28:43Z","receivedAt":"2021-09-10T11:30:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As noted in 65f98358c0c (builtin/help.c: add --guide option,\n2013-04-02) and a133737b809 (doc: include --guide option description\nfor \"git help\", 2013-04-02) which introduced the --guide option it\ncannot be combined with e.g. .\n\nChange both the usage string to reflect that, and test and assert for\nthis behavior in the command itself. Now that we assert this in code\nwe don't need to exhaustively describe the previous confusing behavior\nin the documentation either, instead of silently ignoring the provided\nargument we'll now error out.\n\nThe comment being removed was added in 15f7d494380 (builtin/help.c:\nsplit \"-a\" processing into two, 2013-04-02). The \"Ignore any remaining\nargs\" part of it is now no longer applicable as explained above, let's\njust remove it entirely, it's rather obvious that if we're returning\nwe're done.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-help.txt | 6 +++---\n builtin/help.c | 11 +++++++----\n t/t0012-help.sh | 4 ++++\n 3 files changed, 14 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-help.txt b/Documentation/git-help.txt\nindex 568a0b606f3..cb8e3d4da9e 100644\n--- a/Documentation/git-help.txt\n+++ b/Documentation/git-help.txt\n@@ -8,8 +8,9 @@ git-help - Display help information about Git\n SYNOPSIS\n --------\n [verse]\n-'git help' [-a|--all [--[no-]verbose]] [-g|--guides]\n+'git help' [-a|--all [--[no-]verbose]]\n \t [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]\n+'git help' [-g|--guides]\n \n DESCRIPTION\n -----------\n@@ -58,8 +59,7 @@ OPTIONS\n \n -g::\n --guides::\n-\tPrints a list of the Git concept guides on the standard output. This\n-\toption overrides any given command or guide name.\n+\tPrints a list of the Git concept guides on the standard output.\n \n -i::\n --info::\ndiff --git a/builtin/help.c b/builtin/help.c\nindex 44ea2798cda..51b18c291d8 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -59,8 +59,9 @@ static struct option builtin_help_options[] = {\n };\n \n static const char * const builtin_help_usage[] = {\n-\tN_(\"git help [-a|--all] [-g|--guides] [--[no-]verbose]]\\n\"\n+\tN_(\"git help [-a|--all] [--[no-]verbose]]\\n\"\n \t \" [[-i|--info] [-m|--man] [-w|--web]] []\"),\n+\tN_(\"git help [-g|--guides]\"),\n \tNULL\n };\n \n@@ -552,6 +553,11 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \t\t\tbuiltin_help_usage, 0);\n \tparsed_help_format = help_format;\n \n+\t/* Options that take no further arguments */\n+\tif (argc && show_guides)\n+\t\tusage_msg_opt(_(\"--guides cannot be combined with other options\"),\n+\t\t\t builtin_help_usage, builtin_help_options);\n+\n \tif (show_all) {\n \t\tgit_config(git_help_config, NULL);\n \t\tif (verbose) {\n@@ -582,9 +588,6 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \n \tif (show_all || show_guides) {\n \t\tprintf(\"%s\\n\", _(git_more_info_string));\n-\t\t/*\n-\t\t* We're done. Ignore any remaining args\n-\t\t*/\n \t\treturn 0;\n \t}\n \ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 5679e29c624..c3aa016fd30 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -34,6 +34,10 @@ test_expect_success 'basic help commands' '\n \tgit help -a >/dev/null\n '\n \n+test_expect_success 'invalid usage' '\n+\ttest_expect_code 129 git help -g git-add\n+'\n+\n test_expect_success \"works for commands and guides by default\" '\n \tconfigure_help &&\n \tgit help status &&\n-- \n2.33.0.873.g125ff7b9940\n\n"},{"id":"435399","messageId":"patch-v2-3.5-258282095de-20210910T112545Z-avarab@gmail.com","threadId":"56465","inReplyTo":"cover-v2-0.5-00000000000-20210910T112545Z-avarab@gmail.com","subject":"[PATCH v2 3/5] help tests: add test for --config output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-10T11:28:44Z","receivedAt":"2021-09-10T11:30:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a missing test for checking what the --config output added in\nac68a93fd2 (help: add --config to list all available config,\n2018-05-26) looks like. We should not be emitting anything except\nconfig variables and the brief usage information at the end here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t0012-help.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex c3aa016fd30..595bf81f133 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -77,6 +77,19 @@ test_expect_success 'git help -g' '\n \ttest_i18ngrep \"^ tutorial \" help.output\n '\n \n+test_expect_success 'git help -c' '\n+\tgit help -c >help.output &&\n+\tcat >expect <<-\\EOF &&\n+\n+\t'\"'\"'git help config'\"'\"' for more information\n+\tEOF\n+\tgrep -v -E \\\n+\t\t-e \"^[^.]+\\.[^.]+$\" \\\n+\t\t-e \"^[^.]+\\.[^.]+\\.[^.]+$\" \\\n+\t\thelp.output >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'generate builtin list' '\n \tgit --list-cmds=builtins >builtins\n '\n-- \n2.33.0.873.g125ff7b9940\n\n"},{"id":"435400","messageId":"patch-v2-4.5-32d73d5273c-20210910T112545Z-avarab@gmail.com","threadId":"56465","inReplyTo":"cover-v2-0.5-00000000000-20210910T112545Z-avarab@gmail.com","subject":"[PATCH v2 4/5] help: correct logic error in combining --all and --config","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-10T11:28:45Z","receivedAt":"2021-09-10T11:30:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a bug in the --config option that's been there ever since its\nintroduction in 3ac68a93fd2 (help: add --config to list all available\nconfig, 2018-05-26). Die when --all and --config are combined,\ncombining them doesn't make sense.\n\nThe code for the --config option when combined with an earlier\nrefactoring done to support the --guide option in\n65f98358c0c (builtin/help.c: add --guide option, 2013-04-02) would\ncause us to take the \"--all\" branch early and ignore the --config\noption.\n\nLet's instead list these as incompatible, both in the synopsis and\nhelp output, and enforce it in the code itself.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-help.txt | 1 +\n builtin/help.c | 37 +++++++++++++++++++++++++++----------\n t/t0012-help.sh | 7 ++++++-\n 3 files changed, 34 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/git-help.txt b/Documentation/git-help.txt\nindex cb8e3d4da9e..96d5f598b4b 100644\n--- a/Documentation/git-help.txt\n+++ b/Documentation/git-help.txt\n@@ -11,6 +11,7 @@ SYNOPSIS\n 'git help' [-a|--all [--[no-]verbose]]\n \t [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]\n 'git help' [-g|--guides]\n+'git help' [-c|--config]\n \n DESCRIPTION\n -----------\ndiff --git a/builtin/help.c b/builtin/help.c\nindex 51b18c291d8..05ba2cbe380 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -62,6 +62,7 @@ static const char * const builtin_help_usage[] = {\n \tN_(\"git help [-a|--all] [--[no-]verbose]]\\n\"\n \t \" [[-i|--info] [-m|--man] [-w|--web]] []\"),\n \tN_(\"git help [-g|--guides]\"),\n+\tN_(\"git help [-c|--config]\"),\n \tNULL\n };\n \n@@ -548,18 +549,34 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \tint nongit;\n \tenum help_format parsed_help_format;\n \tconst char *page;\n+\tint need_config = 0;\n \n \targc = parse_options(argc, argv, prefix, builtin_help_options,\n \t\t\tbuiltin_help_usage, 0);\n \tparsed_help_format = help_format;\n \n+\t/* Incompatible options */\n+\tif (show_all && show_config)\n+\t\tusage_msg_opt(_(\"--config and --all cannot be combined\"),\n+\t\t\t builtin_help_usage, builtin_help_options);\n+\n+\tif (show_config && show_guides)\n+\t\tusage_msg_opt(_(\"--config and --guides cannot be combined\"),\n+\t\t\t builtin_help_usage, builtin_help_options);\n+\n \t/* Options that take no further arguments */\n+\tif (argc && show_config)\n+\t\tusage_msg_opt(_(\"--config cannot be combined with command or guide names\"),\n+\t\t\t builtin_help_usage, builtin_help_options);\n \tif (argc && show_guides)\n-\t\tusage_msg_opt(_(\"--guides cannot be combined with other options\"),\n+\t\tusage_msg_opt(_(\"--guides cannot be combined with command or guide names\"),\n \t\t\t builtin_help_usage, builtin_help_options);\n \n-\tif (show_all) {\n+\tneed_config = show_all || show_config;\n+\tif (need_config)\n \t\tgit_config(git_help_config, NULL);\n+\n+\tif (show_all) {\n \t\tif (verbose) {\n \t\t\tsetup_pager();\n \t\t\tlist_all_cmds_help();\n@@ -570,6 +587,14 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \t\tlist_commands(colopts, &main_cmds, &other_cmds);\n \t}\n \n+\tif (show_guides)\n+\t\tlist_guides_help();\n+\n+\tif (show_all || show_guides) {\n+\t\tprintf(\"%s\\n\", _(git_more_info_string));\n+\t\treturn 0;\n+\t}\n+\n \tif (show_config) {\n \t\tint for_human = show_config == 1;\n \n@@ -583,14 +608,6 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \t}\n \n-\tif (show_guides)\n-\t\tlist_guides_help();\n-\n-\tif (show_all || show_guides) {\n-\t\tprintf(\"%s\\n\", _(git_more_info_string));\n-\t\treturn 0;\n-\t}\n-\n \tif (!argv[0]) {\n \t\tprintf(_(\"usage: %s%s\"), _(git_usage_string), \"\\n\\n\");\n \t\tlist_common_cmds_help();\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 595bf81f133..cbc9b64f79f 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -35,7 +35,12 @@ test_expect_success 'basic help commands' '\n '\n \n test_expect_success 'invalid usage' '\n-\ttest_expect_code 129 git help -g git-add\n+\ttest_expect_code 129 git help -g git-add &&\n+\ttest_expect_code 129 git help -c git-add &&\n+\ttest_expect_code 129 git help -g git-add &&\n+\n+\ttest_expect_code 129 git help -a -c &&\n+\ttest_expect_code 129 git help -g -c\n '\n \n test_expect_success \"works for commands and guides by default\" '\n-- \n2.33.0.873.g125ff7b9940\n\n"},{"id":"435401","messageId":"patch-v2-5.5-e995a42cb8d-20210910T112545Z-avarab@gmail.com","threadId":"56465","inReplyTo":"cover-v2-0.5-00000000000-20210910T112545Z-avarab@gmail.com","subject":"[PATCH v2 5/5] help / completion: make \"git help\" do the hard work","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-10T11:28:46Z","receivedAt":"2021-09-10T11:30:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"help\" builtin has been able to emit configuration variables since\ne17ca926371 (completion: drop the hard coded list of config vars,\n2018-05-26), but it hasn't produced exactly the format the completion\nscript wanted. Let's do that.\n\nWe got partway there in 2675ea1cc0f (completion: use 'sort -u' to\ndeduplicate config variable names, 2019-08-13) and\nd9438873c4d (completion: deduplicate configuration sections,\n2019-08-13), but after both we still needed some sorting,\nde-duplicating and awk post-processing of the list.\n\nWe can instead simply do the relevant parsing ourselves (we were doing\nmost of it already), and call string_list_remove_duplicates() after\nalready sorting the list, so the caller doesn't need to invoke \"sort\n-u\".\n\nThis changes the output of the section list to emit lines like \"alias\"\ninstead of \"alias.\". The dot suffix is better done as an argument to\n__gitcomp().\n\nThis means that we'll have the list_config_help() function do a bit\nmore work, let's switch its \"for_human\" to passing a full\n\"show_config\", but as an enum type so we can have the compiler check\nwhat values we're expecting to get.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/help.c | 67 ++++++++++++++++++--------\n contrib/completion/git-completion.bash | 21 ++++----\n t/t0012-help.sh | 24 +++++++++\n 3 files changed, 81 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex 05ba2cbe380..9eb09d4804e 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -38,7 +38,12 @@ static const char *html_path;\n \n static int show_all = 0;\n static int show_guides = 0;\n-static int show_config;\n+enum show_config_type {\n+\tSHOW_CONFIG_UNSET = 0,\n+\tSHOW_CONFIG_HUMAN,\n+\tSHOW_CONFIG_VARS,\n+\tSHOW_CONFIG_SECTIONS,\n+} show_config;\n static int verbose = 1;\n static unsigned int colopts;\n static enum help_format help_format = HELP_FORMAT_NONE;\n@@ -48,7 +53,10 @@ static struct option builtin_help_options[] = {\n \tOPT_HIDDEN_BOOL(0, \"exclude-guides\", &exclude_guides, N_(\"exclude guides\")),\n \tOPT_BOOL('g', \"guides\", &show_guides, N_(\"print list of useful guides\")),\n \tOPT_BOOL('c', \"config\", &show_config, N_(\"print all configuration variable names\")),\n-\tOPT_SET_INT_F(0, \"config-for-completion\", &show_config, \"\", 2, PARSE_OPT_HIDDEN),\n+\tOPT_SET_INT_F(0, \"config-for-completion-vars\", &show_config, \"\",\n+\t\t SHOW_CONFIG_VARS, PARSE_OPT_HIDDEN),\n+\tOPT_SET_INT_F(0, \"config-for-completion-sections\", &show_config, \"\",\n+\t\t SHOW_CONFIG_SECTIONS, PARSE_OPT_HIDDEN),\n \tOPT_SET_INT('m', \"man\", &help_format, N_(\"show man page\"), HELP_FORMAT_MAN),\n \tOPT_SET_INT('w', \"web\", &help_format, N_(\"show manual in web browser\"),\n \t\t\tHELP_FORMAT_WEB),\n@@ -73,7 +81,7 @@ struct slot_expansion {\n \tint found;\n };\n \n-static void list_config_help(int for_human)\n+static void list_config_help(enum show_config_type type)\n {\n \tstruct slot_expansion slot_expansions[] = {\n \t\t{ \"advice\", \"*\", list_config_advices },\n@@ -91,6 +99,8 @@ static void list_config_help(int for_human)\n \tconst char **p;\n \tstruct slot_expansion *e;\n \tstruct string_list keys = STRING_LIST_INIT_DUP;\n+\tstruct string_list keys_uniq = STRING_LIST_INIT_DUP;\n+\tstruct string_list_item *item;\n \tint i;\n \n \tfor (p = config_name_list; *p; p++) {\n@@ -121,34 +131,48 @@ static void list_config_help(int for_human)\n \tfor (i = 0; i < keys.nr; i++) {\n \t\tconst char *var = keys.items[i].string;\n \t\tconst char *wildcard, *tag, *cut;\n+\t\tconst char *dot = NULL;\n+\t\tstruct strbuf sb = STRBUF_INIT;\n \n-\t\tif (for_human) {\n+\t\tswitch (type) {\n+\t\tcase SHOW_CONFIG_HUMAN:\n \t\t\tputs(var);\n \t\t\tcontinue;\n+\t\tcase SHOW_CONFIG_SECTIONS:\n+\t\t\tdot = strchr(var, '.');\n+\t\t\tbreak;\n+\t\tcase SHOW_CONFIG_VARS:\n+\t\t\tbreak;\n+\t\tcase SHOW_CONFIG_UNSET:\n+\t\t\tBUG(\"should not get SHOW_CONFIG_UNSET here\");\n \t\t}\n-\n \t\twildcard = strchr(var, '*');\n \t\ttag = strchr(var, '<');\n \n-\t\tif (!wildcard && !tag) {\n-\t\t\tputs(var);\n+\t\tif (!dot && !wildcard && !tag) {\n+\t\t\tstring_list_append(&keys_uniq, var);\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (wildcard && !tag)\n+\t\tif (dot)\n+\t\t\tcut = dot;\n+\t\telse if (wildcard && !tag)\n \t\t\tcut = wildcard;\n \t\telse if (!wildcard && tag)\n \t\t\tcut = tag;\n \t\telse\n \t\t\tcut = wildcard < tag ? wildcard : tag;\n \n-\t\t/*\n-\t\t * We may produce duplicates, but that's up to\n-\t\t * git-completion.bash to handle\n-\t\t */\n-\t\tprintf(\"%.*s\\n\", (int)(cut - var), var);\n+\t\tstrbuf_add(&sb, var, cut - var);\n+\t\tstring_list_append(&keys_uniq, sb.buf);\n+\t\tstrbuf_release(&sb);\n+\n \t}\n \tstring_list_clear(&keys, 0);\n+\tstring_list_remove_duplicates(&keys_uniq, 0);\n+\tfor_each_string_list_item(item, &keys_uniq)\n+\t\tputs(item->string);\n+\tstring_list_clear(&keys_uniq, 0);\n }\n \n static enum help_format parse_help_format(const char *format)\n@@ -595,16 +619,19 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \t\treturn 0;\n \t}\n \n-\tif (show_config) {\n-\t\tint for_human = show_config == 1;\n+\tswitch (show_config) {\n+\tcase SHOW_CONFIG_UNSET:\n+\t\tbreak;\n+\tcase SHOW_CONFIG_VARS:\n+\tcase SHOW_CONFIG_SECTIONS:\n+\t\tlist_config_help(show_config);\n \n-\t\tif (!for_human) {\n-\t\t\tlist_config_help(for_human);\n-\t\t\treturn 0;\n-\t\t}\n+\t\treturn 0;\n+\tcase SHOW_CONFIG_HUMAN:\n \t\tsetup_pager();\n-\t\tlist_config_help(for_human);\n+\t\tlist_config_help(show_config);\n \t\tprintf(\"\\n%s\\n\", _(\"'git help config' for more information\"));\n+\n \t\treturn 0;\n \t}\n \ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 8108eda1e86..19b8a172878 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2503,7 +2503,14 @@ __git_config_vars=\n __git_compute_config_vars ()\n {\n \ttest -n \"$__git_config_vars\" ||\n-\t__git_config_vars=\"$(git help --config-for-completion | sort -u)\"\n+\t__git_config_vars=\"$(git help --config-for-completion-vars)\"\n+}\n+\n+__git_config_sections=\n+__git_compute_config_sections ()\n+{\n+\ttest -n \"$__git_config_sections\" ||\n+\t__git_config_sections=\"$(git help --config-for-completion-sections)\"\n }\n \n # Completes possible values of various configuration variables.\n@@ -2717,16 +2724,8 @@ __git_complete_config_variable_name ()\n \t\t__gitcomp \"$__git_config_vars\" \"\" \"$cur_\" \"$sfx\"\n \t\t;;\n \t*)\n-\t\t__git_compute_config_vars\n-\t\t__gitcomp \"$(echo \"$__git_config_vars\" |\n-\t\t\t\tawk -F . '{\n-\t\t\t\t\tsections[$1] = 1\n-\t\t\t\t}\n-\t\t\t\tEND {\n-\t\t\t\t\tfor (s in sections)\n-\t\t\t\t\t\tprint s \".\"\n-\t\t\t\t}\n-\t\t\t\t')\" \"\" \"$cur_\"\n+\t\t__git_compute_config_sections\n+\t\t__gitcomp \"$__git_config_sections\" \"\" \"$cur_\" \".\"\n \t\t;;\n \tesac\n }\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex cbc9b64f79f..4405eb6dde5 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -95,6 +95,30 @@ test_expect_success 'git help -c' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'git help --config-for-completion-vars' '\n+\tgit help -c >human &&\n+\tgrep -E \\\n+\t -e \"^[^.]+\\.[^.]+$\" \\\n+\t -e \"^[^.]+\\.[^.]+\\.[^.]+$\" human |\n+\t sed -e \"s/\\*.*//\" -e \"s/<.*//\" |\n+\t sort -u >human.munged &&\n+\n+\tgit help --config-for-completion-vars >vars &&\n+\ttest_cmp human.munged vars\n+'\n+\n+test_expect_success 'git help --config-for-completion-sections' '\n+\tgit help -c >human &&\n+\tgrep -E \\\n+\t -e \"^[^.]+\\.[^.]+$\" \\\n+\t -e \"^[^.]+\\.[^.]+\\.[^.]+$\" human |\n+\t sed -e \"s/\\..*//\" |\n+\t sort -u >human.munged &&\n+\n+\tgit help --config-for-completion-sections >sections &&\n+\ttest_cmp human.munged sections\n+'\n+\n test_expect_success 'generate builtin list' '\n \tgit --list-cmds=builtins >builtins\n '\n-- \n2.33.0.873.g125ff7b9940\n\n"},{"id":"435456","messageId":"2493437c-01c5-ddcc-6a61-666f87e70f20@iee.email","threadId":"56465","inReplyTo":"patch-v2-2.5-039639a0dd3-20210910T112545Z-avarab@gmail.com","subject":"Re: [PATCH v2 2/5] help: correct usage & behavior of \"git help --guides\"","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2021-09-10T18:15:47Z","receivedAt":"2021-09-10T18:15:51Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 10/09/2021 12:28, Ævar Arnfjörð Bjarmason wrote:\n> As noted in 65f98358c0c (builtin/help.c: add --guide option,\n> 2013-04-02) and a133737b809 (doc: include --guide option description\n> for \"git help\", 2013-04-02) which introduced the --guide option it\n> cannot be combined with e.g. .\n>\n> Change both the usage string to reflect that, and test and assert for\n> this behavior in the command itself. Now that we assert this in code\n> we don't need to exhaustively describe the previous confusing behavior\n> in the documentation either, instead of silently ignoring the provided\n> argument we'll now error out.\n>\n> The comment being removed was added in 15f7d494380 (builtin/help.c:\n> split \"-a\" processing into two, 2013-04-02). The \"Ignore any remaining\n> args\" part of it is now no longer applicable as explained above, let's\n> just remove it entirely, it's rather obvious that if we're returning\n> we're done.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> Documentation/git-help.txt | 6 +++---\n> builtin/help.c | 11 +++++++----\n> t/t0012-help.sh | 4 ++++\n> 3 files changed, 14 insertions(+), 7 deletions(-)\n>\n> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt\n> index 568a0b606f3..cb8e3d4da9e 100644\n> --- a/Documentation/git-help.txt\n> +++ b/Documentation/git-help.txt\n> @@ -8,8 +8,9 @@ git-help - Display help information about Git\n> SYNOPSIS\n> --------\n> [verse]\n> -'git help' [-a|--all [--[no-]verbose]] [-g|--guides]\n> +'git help' [-a|--all [--[no-]verbose]]\n> \t [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]\n> +'git help' [-g|--guides]\n\nShouldn't we also include the [-c|--config] options here in the synopsis,\nand the help_usage below?\n\nFurther, shouldn't we mention this (git help -c) on the git config man\npage, e.g. \"A list all available configuration variables can be\ngenerated by `git help -c`.\" \n\nI hadn't realised the facility was even available.\n> \n> DESCRIPTION\n> -----------\n> @@ -58,8 +59,7 @@ OPTIONS\n> \n> -g::\n> --guides::\n> -\tPrints a list of the Git concept guides on the standard output. This\n> -\toption overrides any given command or guide name.\n> +\tPrints a list of the Git concept guides on the standard output.\n> \n> -i::\n> --info::\n> diff --git a/builtin/help.c b/builtin/help.c\n> index 44ea2798cda..51b18c291d8 100644\n> --- a/builtin/help.c\n> +++ b/builtin/help.c\n> @@ -59,8 +59,9 @@ static struct option builtin_help_options[] = {\n> };\n> \n> static const char * const builtin_help_usage[] = {\n> -\tN_(\"git help [-a|--all] [-g|--guides] [--[no-]verbose]]\\n\"\n> +\tN_(\"git help [-a|--all] [--[no-]verbose]]\\n\"\n> \t \" [[-i|--info] [-m|--man] [-w|--web]] []\"),\n> +\tN_(\"git help [-g|--guides]\"),\n> \tNULL\n> };\n> \n> @@ -552,6 +553,11 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n> \t\t\tbuiltin_help_usage, 0);\n> \tparsed_help_format = help_format;\n> \n> +\t/* Options that take no further arguments */\n> +\tif (argc && show_guides)\n> +\t\tusage_msg_opt(_(\"--guides cannot be combined with other options\"),\n> +\t\t\t builtin_help_usage, builtin_help_options);\n> +\n> \tif (show_all) {\n> \t\tgit_config(git_help_config, NULL);\n> \t\tif (verbose) {\n> @@ -582,9 +588,6 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n> \n> \tif (show_all || show_guides) {\n> \t\tprintf(\"%s\\n\", _(git_more_info_string));\n> -\t\t/*\n> -\t\t* We're done. Ignore any remaining args\n> -\t\t*/\n> \t\treturn 0;\n> \t}\n> \n> diff --git a/t/t0012-help.sh b/t/t0012-help.sh\n> index 5679e29c624..c3aa016fd30 100755\n> --- a/t/t0012-help.sh\n> +++ b/t/t0012-help.sh\n> @@ -34,6 +34,10 @@ test_expect_success 'basic help commands' '\n> \tgit help -a >/dev/null\n> '\n> \n> +test_expect_success 'invalid usage' '\n> +\ttest_expect_code 129 git help -g git-add\n> +'\n> +\n> test_expect_success \"works for commands and guides by default\" '\n> \tconfigure_help &&\n> \tgit help status &&\n\n"},{"id":"435457","messageId":"8c72d383-4ae2-b96c-6886-7c36153d8991@iee.email","threadId":"56465","inReplyTo":"2493437c-01c5-ddcc-6a61-666f87e70f20@iee.email","subject":"Re: [PATCH v2 2/5] help: correct usage & behavior of \"git help --guides\"","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2021-09-10T18:21:29Z","receivedAt":"2021-09-10T18:29:42Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 10/09/2021 19:15, Philip Oakley wrote:\n> On 10/09/2021 12:28, Ævar Arnfjörð Bjarmason wrote:\n>> As noted in 65f98358c0c (builtin/help.c: add --guide option,\n>> 2013-04-02) and a133737b809 (doc: include --guide option description\n>> for \"git help\", 2013-04-02) which introduced the --guide option it\n>> cannot be combined with e.g. .\n>>\n>> Change both the usage string to reflect that, and test and assert for\n>> this behavior in the command itself. Now that we assert this in code\n>> we don't need to exhaustively describe the previous confusing behavior\n>> in the documentation either, instead of silently ignoring the provided\n>> argument we'll now error out.\n>>\n>> The comment being removed was added in 15f7d494380 (builtin/help.c:\n>> split \"-a\" processing into two, 2013-04-02). The \"Ignore any remaining\n>> args\" part of it is now no longer applicable as explained above, let's\n>> just remove it entirely, it's rather obvious that if we're returning\n>> we're done.\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> Documentation/git-help.txt | 6 +++---\n>> builtin/help.c | 11 +++++++----\n>> t/t0012-help.sh | 4 ++++\n>> 3 files changed, 14 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt\n>> index 568a0b606f3..cb8e3d4da9e 100644\n>> --- a/Documentation/git-help.txt\n>> +++ b/Documentation/git-help.txt\n>> @@ -8,8 +8,9 @@ git-help - Display help information about Git\n>> SYNOPSIS\n>> --------\n>> [verse]\n>> -'git help' [-a|--all [--[no-]verbose]] [-g|--guides]\n>> +'git help' [-a|--all [--[no-]verbose]]\n>> \t [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]\n>> +'git help' [-g|--guides]\n> Shouldn't we also include the [-c|--config] options here in the synopsis,\n> and the help_usage below?\n\nI see this is fixed in 4/5\n>\n> Further, shouldn't we mention this (git help -c) on the git config man\n> page, e.g. \"A list all available configuration variables can be\n> generated by `git help -c`.\" \n\nStill feel this one would be useful (but may be out of scope of this series)\n>\n> I hadn't realised the facility was even available.\n\nPhilip\n>> \n>> DESCRIPTION\n>> -----------\n>> @@ -58,8 +59,7 @@ OPTIONS\n>> \n>> -g::\n>> --guides::\n>> -\tPrints a list of the Git concept guides on the standard output. This\n>> -\toption overrides any given command or guide name.\n>> +\tPrints a list of the Git concept guides on the standard output.\n>> \n>> -i::\n>> --info::\n>> diff --git a/builtin/help.c b/builtin/help.c\n>> index 44ea2798cda..51b18c291d8 100644\n>> --- a/builtin/help.c\n>> +++ b/builtin/help.c\n>> @@ -59,8 +59,9 @@ static struct option builtin_help_options[] = {\n>> };\n>> \n>> static const char * const builtin_help_usage[] = {\n>> -\tN_(\"git help [-a|--all] [-g|--guides] [--[no-]verbose]]\\n\"\n>> +\tN_(\"git help [-a|--all] [--[no-]verbose]]\\n\"\n>> \t \" [[-i|--info] [-m|--man] [-w|--web]] []\"),\n>> +\tN_(\"git help [-g|--guides]\"),\n>> \tNULL\n>> };\n>> \n>> @@ -552,6 +553,11 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n>> \t\t\tbuiltin_help_usage, 0);\n>> \tparsed_help_format = help_format;\n>> \n>> +\t/* Options that take no further arguments */\n>> +\tif (argc && show_guides)\n>> +\t\tusage_msg_opt(_(\"--guides cannot be combined with other options\"),\n>> +\t\t\t builtin_help_usage, builtin_help_options);\n>> +\n>> \tif (show_all) {\n>> \t\tgit_config(git_help_config, NULL);\n>> \t\tif (verbose) {\n>> @@ -582,9 +588,6 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n>> \n>> \tif (show_all || show_guides) {\n>> \t\tprintf(\"%s\\n\", _(git_more_info_string));\n>> -\t\t/*\n>> -\t\t* We're done. Ignore any remaining args\n>> -\t\t*/\n>> \t\treturn 0;\n>> \t}\n>> \n>> diff --git a/t/t0012-help.sh b/t/t0012-help.sh\n>> index 5679e29c624..c3aa016fd30 100755\n>> --- a/t/t0012-help.sh\n>> +++ b/t/t0012-help.sh\n>> @@ -34,6 +34,10 @@ test_expect_success 'basic help commands' '\n>> \tgit help -a >/dev/null\n>> '\n>> \n>> +test_expect_success 'invalid usage' '\n>> +\ttest_expect_code 129 git help -g git-add\n>> +'\n>> +\n>> test_expect_success \"works for commands and guides by default\" '\n>> \tconfigure_help &&\n>> \tgit help status &&\n\n"},{"id":"435488","messageId":"xmqqy284ov0e.fsf@gitster.g","threadId":"56465","inReplyTo":"patch-v2-4.5-32d73d5273c-20210910T112545Z-avarab@gmail.com","subject":"Re: [PATCH v2 4/5] help: correct logic error in combining --all and --config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-10T23:45:05Z","receivedAt":"2021-09-10T23:45:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Fix a bug in the --config option that's been there ever since its\n> introduction in 3ac68a93fd2 (help: add --config to list all available\n> config, 2018-05-26). Die when --all and --config are combined,\n> combining them doesn't make sense.\n>\n> The code for the --config option when combined with an earlier\n> refactoring done to support the --guide option in\n> 65f98358c0c (builtin/help.c: add --guide option, 2013-04-02) would\n> cause us to take the \"--all\" branch early and ignore the --config\n> option.\n>\n> Let's instead list these as incompatible, both in the synopsis and\n> help output, and enforce it in the code itself.\n\nOf course, \"git help -c -a\" might be a good enough UI to ask for\nthese variables that are usually hidden due to being deprecated, but\nimplementing that would be a much larger surgery (I do not think the\nconfig_name_list[] has enough information as the process to generate\nit discards the deprecated ones), so this is OK, I think.\n\n> @@ -548,18 +549,34 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n> \tint nongit;\n> \tenum help_format parsed_help_format;\n> \tconst char *page;\n> +\tint need_config = 0;\n> \n> \targc = parse_options(argc, argv, prefix, builtin_help_options,\n> \t\t\tbuiltin_help_usage, 0);\n> \tparsed_help_format = help_format;\n> \n> +\t/* Incompatible options */\n> +\tif (show_all && show_config)\n> +\t\tusage_msg_opt(_(\"--config and --all cannot be combined\"),\n> +\t\t\t builtin_help_usage, builtin_help_options);\n> +\n> +\tif (show_config && show_guides)\n> +\t\tusage_msg_opt(_(\"--config and --guides cannot be combined\"),\n> +\t\t\t builtin_help_usage, builtin_help_options);\n> +\n> \t/* Options that take no further arguments */\n> +\tif (argc && show_config)\n> +\t\tusage_msg_opt(_(\"--config cannot be combined with command or guide names\"),\n> +\t\t\t builtin_help_usage, builtin_help_options);\n> \tif (argc && show_guides)\n> -\t\tusage_msg_opt(_(\"--guides cannot be combined with other options\"),\n> +\t\tusage_msg_opt(_(\"--guides cannot be combined with command or guide names\"),\n> \t\t\t builtin_help_usage, builtin_help_options);\n\nThis almost makes me wonder if we should be using OPT_CMDMODE and\ntaking advantage of its built-in \"X and Y are incompatible\"\ndetection.\n\n> -\tif (show_all) {\n> +\tneed_config = show_all || show_config;\n> +\tif (need_config)\n> \t\tgit_config(git_help_config, NULL);\n\nThis change does not seem to be explained. \n\nWe didn't handle help-config when \"git help -c\" was asked, but now\nwe do, because...?\n\n> +\tif (show_all) {\n> \t\tif (verbose) {\n> \t\t\tsetup_pager();\n> \t\t\tlist_all_cmds_help();\n> @@ -570,6 +587,14 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n> \t\tlist_commands(colopts, &main_cmds, &other_cmds);\n> \t}\n> \n> +\tif (show_guides)\n> +\t\tlist_guides_help();\n> +\n> +\tif (show_all || show_guides) {\n> +\t\tprintf(\"%s\\n\", _(git_more_info_string));\n> +\t\treturn 0;\n> +\t}\n\nAs guides, all and config are mutually exclusive, it is unclear what\nwe gain by moving these two above. The resulting code does not seem\nto be more clear then before.\n\nIf anything, \n\n\tswitch (show_mode) {\n\tcase HELP_ALL_MODE:\n\t\t... the body of the first \"if\" ...\n\t\tprintf(\"%s\\n\", _(git_more_info_string));\n\t\treturn 0;\n\tcase HELP_GUIDES_MODE:\n\t\tlist_guides_help();\n\t\tprintf(\"%s\\n\", _(git_more_info_string));\n\t\treturn 0;\n\tcase HELP_CONFIG_MODE:\n\t\t... the body of the follwing \"if\" ...\n\t\treturn 0;\n\tdefault: /* show a manual page */\n\t\tbreak;\n\t}\n\nmay make it easier to follow.\n\n> \tif (show_config) {\n> \t\tint for_human = show_config == 1;\n> \n> @@ -583,14 +608,6 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n> \t\treturn 0;\n> \t}\n> \n> -\tif (show_guides)\n> -\t\tlist_guides_help();\n> -\n> -\tif (show_all || show_guides) {\n> -\t\tprintf(\"%s\\n\", _(git_more_info_string));\n> -\t\treturn 0;\n> -\t}\n> -\n> \tif (!argv[0]) {\n> \t\tprintf(_(\"usage: %s%s\"), _(git_usage_string), \"\\n\\n\");\n> \t\tlist_common_cmds_help();\n> diff --git a/t/t0012-help.sh b/t/t0012-help.sh\n> index 595bf81f133..cbc9b64f79f 100755\n> --- a/t/t0012-help.sh\n> +++ b/t/t0012-help.sh\n> @@ -35,7 +35,12 @@ test_expect_success 'basic help commands' '\n> '\n> \n> test_expect_success 'invalid usage' '\n> -\ttest_expect_code 129 git help -g git-add\n\n> +\ttest_expect_code 129 git help -g git-add &&\n> +\ttest_expect_code 129 git help -c git-add &&\n> +\ttest_expect_code 129 git help -g git-add &&\n\nWhy two \"-g git-add\"?\n\n> +\n> +\ttest_expect_code 129 git help -a -c &&\n> +\ttest_expect_code 129 git help -g -c\n> '\n> \n> test_expect_success \"works for commands and guides by default\" '\n"},{"id":"435495","messageId":"xmqqr1dvoqyo.fsf@gitster.g","threadId":"56465","inReplyTo":"patch-v2-1.5-b10bfd21f14-20210910T112545Z-avarab@gmail.com","subject":"Re: [PATCH v2 1/5] help: correct the usage string in -h and documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-11T01:12:31Z","receivedAt":"2021-09-11T01:12:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt\n> index 44fe8860b3f..568a0b606f3 100644\n> --- a/Documentation/git-help.txt\n> +++ b/Documentation/git-help.txt\n> @@ -9,7 +9,7 @@ SYNOPSIS\n> --------\n> [verse]\n> 'git help' [-a|--all [--[no-]verbose]] [-g|--guides]\n> -\t [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE]\n> +\t [[-i|--info] [-m|--man] [-w|--web]] [COMMAND|GUIDE]\n\nThis one is good, but ...\n\n> diff --git a/builtin/help.c b/builtin/help.c\n> index b7eec06c3de..44ea2798cda 100644\n> --- a/builtin/help.c\n> +++ b/builtin/help.c\n> @@ -59,7 +59,8 @@ static struct option builtin_help_options[] = {\n> };\n> \n> static const char * const builtin_help_usage[] = {\n> -\tN_(\"git help [--all] [--guides] [--man | --web | --info] []\"),\n> +\tN_(\"git help [-a|--all] [-g|--guides] [--[no-]verbose]]\\n\"\n> +\t \" [[-i|--info] [-m|--man] [-w|--web]] []\"),\n\nAside from the addition of so-far-missing \"verbose\", which is\nobviously a good change, I am not sure if the other change is a good\nidea.\n\nThis is because *_usage[] is designed to be always shown together\nwith the list of options built from the option table the\nparse_options() uses, and the readers will see the correspondence\nbetween long and short options even more clear there.\n\n $ git help -h\n usage: git help [--all] [--guides] [--man | --web | --info] []\n\n -a, --all print all available commands\n -g, --guides print list of useful guides\n -c, --config print all configuration variable names\n -m, --man show man page\n -w, --web show manual in web browser\n -i, --info show info page\n -v, --verbose print command description\n\nIf you look at output from \n\n $ git grep -A4 -e '_usage\\[' builtin/\\*.c\n\nyou'll notice that many of them do not even spell out each option\nand instead have a single [] placeholder unless the command\nhas only very small number of options. With the number of options\nthat \"git help\" takes, it might even be warranted to switch to the\nmore generic \"git help [