{"thread":{"id":"56122","subject":"[PATCH v2 0/5] commit-graph: usage fixes","startedAt":"2021-07-18T07:58:18Z","lastAt":"2021-08-30T23:58:32Z","messageCount":40,"participants":["Ævar Arnfjörð Bjarmason","Andrei Rybak","Taylor Blau","SZEDER Gábor","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":5},"messages":[{"id":"430455","messageId":"cover-0.5-00000000000-20210718T074936Z-avarab@gmail.com","threadId":"56122","inReplyTo":null,"subject":"[PATCH v2 0/5] commit-graph: usage fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-18T07:58:04Z","receivedAt":"2021-07-18T07:58:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This set of trivial fixes to commit-graph usage was submitted\noriginally back in February as\nhttps://lore.kernel.org/git/20210215184118.11306-1-avarab@gmail.com/\n\nI omitted the In-Reply-To in the header because this was deep in an\nunrelated thread.\n\nIn the meantime Taylor's similar changes to the midx code landed (the\noriginal v1 was a \"hey, here's how you can do this with\nparse_options()\" on my part to him).\n\nI did some changes based on feedback on the v1, but didn't pick up all\nthe suggestions, it was mostly subjective, so let's see what people\nthink this time around.\n\nÆvar Arnfjörð Bjarmason (5):\n commit-graph: define common usage with a macro\n commit-graph: remove redundant handling of -h\n commit-graph: use parse_options_concat()\n commit-graph: early exit to \"usage\" on !argc\n commit-graph: show usage on \"commit-graph [write|verify] garbage\"\n\n builtin/commit-graph.c | 95 +++++++++++++++++++++++------------------\n t/t5318-commit-graph.sh | 7 +++\n 2 files changed, 60 insertions(+), 42 deletions(-)\n\nRange-diff against v1:\n1: 742648756a5 ! 1: 0b0bb04ecf5 commit-graph: define common usage with a macro\n @@ Commit message\n information, see e.g. 809e0327f5 (builtin/commit-graph.c: introduce\n '--max-new-filters=', 2020-09-18).\n \n + See b25b727494f (builtin/multi-pack-index.c: define common usage with\n + a macro, 2021-03-30) for another use of this pattern (but on-list this\n + one came first).\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## builtin/commit-graph.c ##\n2: 497b6cbc9a5 = 2: 6f386fc32c8 commit-graph: remove redundant handling of -h\n3: fd1deaa3c99 ! 3: 2e7d9b0b8e4 commit-graph: use parse_options_concat()\n @@ builtin/commit-graph.c: static struct opts_commit_graph {\n \t\t\t\t\t const char *obj_dir)\n {\n @@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)\n - \tint fd;\n - \tstruct stat st;\n \tint flags = 0;\n --\n -+\tstruct option *options = NULL;\n + \n \tstatic struct option builtin_commit_graph_verify_options[] = {\n -\t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n -\t\t\t N_(\"dir\"),\n @@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)\n -\t\tOPT_BOOL(0, \"progress\", &opts.progress, N_(\"force progress reporting\")),\n \t\tOPT_END(),\n \t};\n -+\toptions = parse_options_dup(builtin_commit_graph_verify_options);\n ++\tstruct option *options = parse_options_dup(builtin_commit_graph_verify_options);\n +\toptions = add_common_options(options);\n \n \ttrace2_cmd_mode(\"verify\");\n @@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)\n \n \tif (!opts.obj_dir)\n @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n - \tint result = 0;\n - \tenum commit_graph_write_flags flags = 0;\n \tstruct progress *progress = NULL;\n --\n -+\tstruct option *options = NULL;\n + \n \tstatic struct option builtin_commit_graph_write_options[] = {\n -\t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n -\t\t\tN_(\"dir\"),\n @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n \t\t\t0, write_option_max_new_filters),\n \t\tOPT_END(),\n \t};\n -+\toptions = parse_options_dup(builtin_commit_graph_write_options);\n ++\tstruct option *options = parse_options_dup(builtin_commit_graph_write_options);\n +\toptions = add_common_options(options);\n \n \topts.progress = isatty(2);\n4: 3d4a1fb6680 ! 4: d776424e8c8 commit-graph: refactor dispatch loop for style\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - commit-graph: refactor dispatch loop for style\n + commit-graph: early exit to \"usage\" on !argc\n \n - I think it's more readable to have one if/elsif/else chain here than\n - the code this replaces.\n + Rather than guarding all of the !argc with an additional \"if\" arm\n + let's do an early goto to \"usage\". This also makes it clear that\n + \"save_commit_buffer\" is not needed in this case.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## builtin/commit-graph.c ##\n @@ builtin/commit-graph.c: int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n + \t\t\t builtin_commit_graph_options,\n + \t\t\t builtin_commit_graph_usage,\n + \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n ++\tif (!argc)\n ++\t\tgoto usage;\n \n \tsave_commit_buffer = 0;\n \n @@ builtin/commit-graph.c: int cmd_commit_graph(int argc, const char **argv, const\n -\t\tif (!strcmp(argv[0], \"write\"))\n -\t\t\treturn graph_write(argc, argv);\n -\t}\n --\n --\tusage_with_options(builtin_commit_graph_usage,\n --\t\t\t builtin_commit_graph_options);\n -+\tif (argc && !strcmp(argv[0], \"verify\"))\n ++\tif (!strcmp(argv[0], \"verify\"))\n +\t\treturn graph_verify(argc, argv);\n +\telse if (argc && !strcmp(argv[0], \"write\"))\n +\t\treturn graph_write(argc, argv);\n -+\telse\n -+\t\tusage_with_options(builtin_commit_graph_usage,\n -+\t\t\t\t builtin_commit_graph_options);\n + \n ++usage:\n + \tusage_with_options(builtin_commit_graph_usage,\n + \t\t\t builtin_commit_graph_options);\n }\n5: e8481a0932a = 5: 57ffd5812d6 commit-graph: show usage on \"commit-graph [write|verify] garbage\"\n-- \n2.32.0.873.g94a0c75983d\n\n"},{"id":"430456","messageId":"patch-2.5-6f386fc32c8-20210718T074936Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-0.5-00000000000-20210718T074936Z-avarab@gmail.com","subject":"[PATCH v2 2/5] commit-graph: remove redundant handling of -h","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-18T07:58:06Z","receivedAt":"2021-07-18T07:58:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"If we don't handle the -h option here like most parse_options() users\nwe'll fall through and it'll do the right thing for us.\n\nI think this code added in 4ce58ee38d (commit-graph: create\ngit-commit-graph builtin, 2018-04-02) was always redundant,\nparse_options() did this at the time, and the commit-graph code never\nused PARSE_OPT_NO_INTERNAL_HELP.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 4 ----\n t/t5318-commit-graph.sh | 5 +++++\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex c3fa4fde3e4..baead04a03b 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -319,10 +319,6 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n \t\tOPT_END(),\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_commit_graph_usage,\n-\t\t\t\t builtin_commit_graph_options);\n-\n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_commit_graph_options,\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex af88f805aa2..5fccce95724 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -5,6 +5,11 @@ test_description='commit graph'\n \n GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0\n \n+test_expect_success 'usage' '\n+\ttest_expect_code 129 git commit-graph -h 2>err &&\n+\t! grep error: err\n+'\n+\n test_expect_success 'setup full repo' '\n \tmkdir full &&\n \tcd \"$TRASH_DIRECTORY/full\" &&\n-- \n2.32.0.873.g94a0c75983d\n\n"},{"id":"430457","messageId":"patch-1.5-0b0bb04ecf5-20210718T074936Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-0.5-00000000000-20210718T074936Z-avarab@gmail.com","subject":"[PATCH v2 1/5] commit-graph: define common usage with a macro","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-18T07:58:05Z","receivedAt":"2021-07-18T07:58:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Share the usage message between these three variables by using a\nmacro. Before this new options needed to copy/paste the usage\ninformation, see e.g. 809e0327f5 (builtin/commit-graph.c: introduce\n'--max-new-filters=', 2020-09-18).\n\nSee b25b727494f (builtin/multi-pack-index.c: define common usage with\na macro, 2021-03-30) for another use of this pattern (but on-list this\none came first).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 29 +++++++++++++++--------------\n 1 file changed, 15 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex cd863152216..c3fa4fde3e4 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -9,26 +9,27 @@\n #include \"progress.h\"\n #include \"tag.h\"\n \n-static char const * const builtin_commit_graph_usage[] = {\n-\tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n-\tN_(\"git commit-graph write [--object-dir ] [--append] \"\n-\t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \"\n-\t \"[--changed-paths] [--[no-]max-new-filters ] [--[no-]progress] \"\n-\t \"\"),\n+static const char * builtin_commit_graph_verify_usage[] = {\n+#define BUILTIN_COMMIT_GRAPH_VERIFY_USAGE \\\n+\tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\")\n+\tBUILTIN_COMMIT_GRAPH_VERIFY_USAGE,\n \tNULL\n };\n \n-static const char * const builtin_commit_graph_verify_usage[] = {\n-\tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n+static const char * builtin_commit_graph_write_usage[] = {\n+#define BUILTIN_COMMIT_GRAPH_WRITE_USAGE \\\n+\tN_(\"git commit-graph write [--object-dir ] [--append] \" \\\n+\t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \" \\\n+\t \"[--changed-paths] [--[no-]max-new-filters ] [--[no-]progress] \" \\\n+\t \"\")\n+\tBUILTIN_COMMIT_GRAPH_WRITE_USAGE,\n \tNULL\n };\n \n-static const char * const builtin_commit_graph_write_usage[] = {\n-\tN_(\"git commit-graph write [--object-dir ] [--append] \"\n-\t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \"\n-\t \"[--changed-paths] [--[no-]max-new-filters ] [--[no-]progress] \"\n-\t \"\"),\n-\tNULL\n+static char const * const builtin_commit_graph_usage[] = {\n+\tBUILTIN_COMMIT_GRAPH_VERIFY_USAGE,\n+\tBUILTIN_COMMIT_GRAPH_WRITE_USAGE,\n+\tNULL,\n };\n \n static struct opts_commit_graph {\n-- \n2.32.0.873.g94a0c75983d\n\n"},{"id":"430458","messageId":"patch-3.5-2e7d9b0b8e4-20210718T074936Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-0.5-00000000000-20210718T074936Z-avarab@gmail.com","subject":"[PATCH v2 3/5] commit-graph: use parse_options_concat()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-18T07:58:07Z","receivedAt":"2021-07-18T07:58:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Make use of the parse_options_concat() so we don't need to copy/paste\ncommon options like --object-dir. This is inspired by a similar change\nto \"checkout\" in 2087182272\n(checkout: split options[] array in three pieces, 2019-03-29).\n\nA minor behavior change here is that now we're going to list both\n--object-dir and --progress first, before we'd list --progress along\nwith other options.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 39 +++++++++++++++++++++++----------------\n 1 file changed, 23 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex baead04a03b..ff125adf2d5 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -44,6 +44,21 @@ static struct opts_commit_graph {\n \tint enable_changed_paths;\n } opts;\n \n+static struct option *add_common_options(struct option *prevopts)\n+{\n+\tstruct option options[] = {\n+\t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n+\t\t\t N_(\"dir\"),\n+\t\t\t N_(\"the object directory to store the graph\")),\n+\t\tOPT_BOOL(0, \"progress\", &opts.progress,\n+\t\t\t N_(\"force progress reporting\")),\n+\t\tOPT_END()\n+\t};\n+\tstruct option *newopts = parse_options_concat(options, prevopts);\n+\tfree(prevopts);\n+\treturn newopts;\n+}\n+\n static struct object_directory *find_odb(struct repository *r,\n \t\t\t\t\t const char *obj_dir)\n {\n@@ -77,20 +92,18 @@ static int graph_verify(int argc, const char **argv)\n \tint flags = 0;\n \n \tstatic struct option builtin_commit_graph_verify_options[] = {\n-\t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n-\t\t\t N_(\"dir\"),\n-\t\t\t N_(\"the object directory to store the graph\")),\n \t\tOPT_BOOL(0, \"shallow\", &opts.shallow,\n \t\t\t N_(\"if the commit-graph is split, only verify the tip file\")),\n-\t\tOPT_BOOL(0, \"progress\", &opts.progress, N_(\"force progress reporting\")),\n \t\tOPT_END(),\n \t};\n+\tstruct option *options = parse_options_dup(builtin_commit_graph_verify_options);\n+\toptions = add_common_options(options);\n \n \ttrace2_cmd_mode(\"verify\");\n \n \topts.progress = isatty(2);\n \targc = parse_options(argc, argv, NULL,\n-\t\t\t builtin_commit_graph_verify_options,\n+\t\t\t options,\n \t\t\t builtin_commit_graph_verify_usage, 0);\n \n \tif (!opts.obj_dir)\n@@ -207,9 +220,6 @@ static int graph_write(int argc, const char **argv)\n \tstruct progress *progress = NULL;\n \n \tstatic struct option builtin_commit_graph_write_options[] = {\n-\t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n-\t\t\tN_(\"dir\"),\n-\t\t\tN_(\"the object directory to store the graph\")),\n \t\tOPT_BOOL(0, \"reachable\", &opts.reachable,\n \t\t\tN_(\"start walk at all refs\")),\n \t\tOPT_BOOL(0, \"stdin-packs\", &opts.stdin_packs,\n@@ -220,7 +230,6 @@ static int graph_write(int argc, const char **argv)\n \t\t\tN_(\"include all commits already in the commit-graph file\")),\n \t\tOPT_BOOL(0, \"changed-paths\", &opts.enable_changed_paths,\n \t\t\tN_(\"enable computation for changed paths\")),\n-\t\tOPT_BOOL(0, \"progress\", &opts.progress, N_(\"force progress reporting\")),\n \t\tOPT_CALLBACK_F(0, \"split\", &write_opts.split_flags, NULL,\n \t\t\tN_(\"allow writing an incremental commit-graph file\"),\n \t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n@@ -236,6 +245,8 @@ static int graph_write(int argc, const char **argv)\n \t\t\t0, write_option_max_new_filters),\n \t\tOPT_END(),\n \t};\n+\tstruct option *options = parse_options_dup(builtin_commit_graph_write_options);\n+\toptions = add_common_options(options);\n \n \topts.progress = isatty(2);\n \topts.enable_changed_paths = -1;\n@@ -249,7 +260,7 @@ static int graph_write(int argc, const char **argv)\n \tgit_config(git_commit_graph_write_config, &opts);\n \n \targc = parse_options(argc, argv, NULL,\n-\t\t\t builtin_commit_graph_write_options,\n+\t\t\t options,\n \t\t\t builtin_commit_graph_write_usage, 0);\n \n \tif (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1)\n@@ -312,12 +323,8 @@ static int graph_write(int argc, const char **argv)\n \n int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n {\n-\tstatic struct option builtin_commit_graph_options[] = {\n-\t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n-\t\t\tN_(\"dir\"),\n-\t\t\tN_(\"the object directory to store the graph\")),\n-\t\tOPT_END(),\n-\t};\n+\tstruct option *no_options = parse_options_dup(NULL);\n+\tstruct option *builtin_commit_graph_options = add_common_options(no_options);\n \n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix,\n-- \n2.32.0.873.g94a0c75983d\n\n"},{"id":"430459","messageId":"patch-4.5-d776424e8c8-20210718T074936Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-0.5-00000000000-20210718T074936Z-avarab@gmail.com","subject":"[PATCH v2 4/5] commit-graph: early exit to \"usage\" on !argc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-18T07:58:08Z","receivedAt":"2021-07-18T07:58:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Rather than guarding all of the !argc with an additional \"if\" arm\nlet's do an early goto to \"usage\". This also makes it clear that\n\"save_commit_buffer\" is not needed in this case.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex ff125adf2d5..16d2c517e72 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -331,16 +331,17 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n \t\t\t builtin_commit_graph_options,\n \t\t\t builtin_commit_graph_usage,\n \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n+\tif (!argc)\n+\t\tgoto usage;\n \n \tsave_commit_buffer = 0;\n \n-\tif (argc > 0) {\n-\t\tif (!strcmp(argv[0], \"verify\"))\n-\t\t\treturn graph_verify(argc, argv);\n-\t\tif (!strcmp(argv[0], \"write\"))\n-\t\t\treturn graph_write(argc, argv);\n-\t}\n+\tif (!strcmp(argv[0], \"verify\"))\n+\t\treturn graph_verify(argc, argv);\n+\telse if (argc && !strcmp(argv[0], \"write\"))\n+\t\treturn graph_write(argc, argv);\n \n+usage:\n \tusage_with_options(builtin_commit_graph_usage,\n \t\t\t builtin_commit_graph_options);\n }\n-- \n2.32.0.873.g94a0c75983d\n\n"},{"id":"430460","messageId":"patch-5.5-57ffd5812d6-20210718T074936Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-0.5-00000000000-20210718T074936Z-avarab@gmail.com","subject":"[PATCH v2 5/5] commit-graph: show usage on \"commit-graph [write|verify] garbage\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-18T07:58:09Z","receivedAt":"2021-07-18T07:58:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the parse_options() invocation in the commit-graph code to make\nsense. We're calling it twice, once for common options parsing, and\nthen for the sub-commands.\n\nBut we never checked if we had something leftover in argc in \"write\"\nor \"verify\", as a result we'd silently accept garbage in these\nsubcommands. Let's not do that.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 10 ++++++++--\n t/t5318-commit-graph.sh | 4 +++-\n 2 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 16d2c517e72..bb3e767db33 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -104,7 +104,10 @@ static int graph_verify(int argc, const char **argv)\n \topts.progress = isatty(2);\n \targc = parse_options(argc, argv, NULL,\n \t\t\t options,\n-\t\t\t builtin_commit_graph_verify_usage, 0);\n+\t\t\t builtin_commit_graph_verify_usage,\n+\t\t\t PARSE_OPT_KEEP_UNKNOWN);\n+\tif (argc)\n+\t\tusage_with_options(builtin_commit_graph_verify_usage, options);\n \n \tif (!opts.obj_dir)\n \t\topts.obj_dir = get_object_directory();\n@@ -261,7 +264,10 @@ static int graph_write(int argc, const char **argv)\n \n \targc = parse_options(argc, argv, NULL,\n \t\t\t options,\n-\t\t\t builtin_commit_graph_write_usage, 0);\n+\t\t\t builtin_commit_graph_write_usage,\n+\t\t\t PARSE_OPT_KEEP_UNKNOWN);\n+\tif (argc)\n+\t\tusage_with_options(builtin_commit_graph_write_usage, options);\n \n \tif (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1)\n \t\tdie(_(\"use at most one of --reachable, --stdin-commits, or --stdin-packs\"));\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 5fccce95724..5cf07a6dded 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -7,7 +7,9 @@ GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0\n \n test_expect_success 'usage' '\n \ttest_expect_code 129 git commit-graph -h 2>err &&\n-\t! grep error: err\n+\t! grep error: err &&\n+\ttest_expect_code 129 git commit-graph write blah &&\n+\ttest_expect_code 129 git commit-graph write verify\n '\n \n test_expect_success 'setup full repo' '\n-- \n2.32.0.873.g94a0c75983d\n\n"},{"id":"430464","messageId":"25b24cc9-a7fc-1286-6bcf-1fdd21ee8c49@gmail.com","threadId":"56122","inReplyTo":"patch-2.5-6f386fc32c8-20210718T074936Z-avarab@gmail.com","subject":"Re: [PATCH v2 2/5] commit-graph: remove redundant handling of -h","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2021-07-18T12:55:09Z","receivedAt":"2021-07-18T12:55:16Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 18/07/2021 09:58, Ævar Arnfjörð Bjarmason wrote:\n> If we don't handle the -h option here like most parse_options() users\n> we'll fall through and it'll do the right thing for us.\n> \n> I think this code added in 4ce58ee38d (commit-graph: create\n> git-commit-graph builtin, 2018-04-02) was always redundant,\n> parse_options() did this at the time, and the commit-graph code never\n> used PARSE_OPT_NO_INTERNAL_HELP.\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/commit-graph.c | 4 ----\n> t/t5318-commit-graph.sh | 5 +++++\n> 2 files changed, 5 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index c3fa4fde3e4..baead04a03b 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -319,10 +319,6 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n> \t\tOPT_END(),\n> \t};\n> \n> -\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n> -\t\tusage_with_options(builtin_commit_graph_usage,\n> -\t\t\t\t builtin_commit_graph_options);\n> -\n> \tgit_config(git_default_config, NULL);\n> \targc = parse_options(argc, argv, prefix,\n> \t\t\t builtin_commit_graph_options,\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index af88f805aa2..5fccce95724 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -5,6 +5,11 @@ test_description='commit graph'\n> \n> GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0\n> \n> +test_expect_success 'usage' '\n> +\ttest_expect_code 129 git commit-graph -h 2>err &&\n> +\t! grep error: err\n\nNew test is partially redundant to the test in the loop at the bottom of\n't0012-help.sh'.\n\n> +'\n> +\n> test_expect_success 'setup full repo' '\n> \tmkdir full &&\n> \tcd \"$TRASH_DIRECTORY/full\" &&\n> \n\n"},{"id":"430514","messageId":"YPWoTKIWRbg6tGrS@nand.local","threadId":"56122","inReplyTo":"patch-1.5-0b0bb04ecf5-20210718T074936Z-avarab@gmail.com","subject":"Re: [PATCH v2 1/5] commit-graph: define common usage with a macro","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-19T16:29:00Z","receivedAt":"2021-07-19T17:00:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Jul 18, 2021 at 09:58:05AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Share the usage message between these three variables by using a\n> macro. Before this new options needed to copy/paste the usage\n> information, see e.g. 809e0327f5 (builtin/commit-graph.c: introduce\n> '--max-new-filters=', 2020-09-18).\n>\n> See b25b727494f (builtin/multi-pack-index.c: define common usage with\n> a macro, 2021-03-30) for another use of this pattern (but on-list this\n> one came first).\n\nI don't have a strong opinion, but I believe Jonathan Tan suggested that\nwe move the #define's out-of-line with their variable declarations back\nin [1].\n\nI think that what you wrote here is fine, but I tend to agree with\nJonathan that the version we ended up with in the tree is cleaner and a\nlittle easier to read (albeit a few more lines).\n\n[1]: https://lore.kernel.org/git/20210302040625.4035284-1-jonathantanmy@google.com/\n\nThanks,\nTaylor\n"},{"id":"430515","messageId":"YPWpiYInoJ/KyKhx@nand.local","threadId":"56122","inReplyTo":"25b24cc9-a7fc-1286-6bcf-1fdd21ee8c49@gmail.com","subject":"Re: [PATCH v2 2/5] commit-graph: remove redundant handling of -h","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-19T16:34:17Z","receivedAt":"2021-07-19T17:00:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Jul 18, 2021 at 02:55:09PM +0200, Andrei Rybak wrote:\n> On 18/07/2021 09:58, Ævar Arnfjörð Bjarmason wrote:\n> > I think this code added in 4ce58ee38d (commit-graph: create\n> > git-commit-graph builtin, 2018-04-02) was always redundant,\n> > parse_options() did this at the time, and the commit-graph code never\n> > used PARSE_OPT_NO_INTERNAL_HELP.\n\nYep, that matches my findings.\n\n> > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> > index af88f805aa2..5fccce95724 100755\n> > --- a/t/t5318-commit-graph.sh\n> > +++ b/t/t5318-commit-graph.sh\n> > @@ -5,6 +5,11 @@ test_description='commit graph'\n> > GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0\n> > +test_expect_success 'usage' '\n> > +\ttest_expect_code 129 git commit-graph -h 2>err &&\n> > +\t! grep error: err\n>\n> New test is partially redundant to the test in the loop at the bottom of\n> 't0012-help.sh'.\n\nAgreed. The change looks good to me, but we should drop this redundant\ntest (although inspecting `git grep -w 'git .* -h' -- t` shows that\nthere are a handful of other ones that could probably be dropped, too).\n\nI don't really feel strongly enough to worry about cleaning up the\nexisting ones, but we should strive to avoid adding new ones.\n\nThanks,\nTaylor\n"},{"id":"430517","messageId":"YPWtRd+523N1cWR/@nand.local","threadId":"56122","inReplyTo":"patch-3.5-2e7d9b0b8e4-20210718T074936Z-avarab@gmail.com","subject":"Re: [PATCH v2 3/5] commit-graph: use parse_options_concat()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-19T16:50:13Z","receivedAt":"2021-07-19T17:09:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Jul 18, 2021 at 09:58:07AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Make use of the parse_options_concat() so we don't need to copy/paste\n> common options like --object-dir. This is inspired by a similar change\n> to \"checkout\" in 2087182272\n> (checkout: split options[] array in three pieces, 2019-03-29).\n>\n> A minor behavior change here is that now we're going to list both\n> --object-dir and --progress first, before we'd list --progress along\n> with other options.\n\nThis is very reminiscent to the patch I sent to do the same in the\n`multi-pack-index` builtin, probably because you were the person to\nrecommend I do that cleanup in the first place ;).\n\nI got some good advice from Peff in [1] went I sent that patch, which\nI'll try to summarize here, since I think a few pieces of it could be\napplied to clean up this patch a little.\n\n[1]: https://lore.kernel.org/git/YGG7tWBzo5NGl2+g@coredump.intra.peff.net/\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/commit-graph.c | 39 +++++++++++++++++++++++----------------\n> 1 file changed, 23 insertions(+), 16 deletions(-)\n>\n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index baead04a03b..ff125adf2d5 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -44,6 +44,21 @@ static struct opts_commit_graph {\n> \tint enable_changed_paths;\n> } opts;\n>\n> +static struct option *add_common_options(struct option *prevopts)\n> +{\n> +\tstruct option options[] = {\n> +\t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n> +\t\t\t N_(\"dir\"),\n> +\t\t\t N_(\"the object directory to store the graph\")),\n> +\t\tOPT_BOOL(0, \"progress\", &opts.progress,\n> +\t\t\t N_(\"force progress reporting\")),\n> +\t\tOPT_END()\n> +\t};\n\nNot from Peff's mail, but is there any reason to non statically allocate\nthis?\n\nThe only reason I could think of is that `opts` is heap allocated, but\nit's not, so I think we could probably mark `options` as static here\n(and perhaps rename it to `common_opts` while we're at it, if for no\nother reason than to be consistent with what's in the multi-pack-index\nbuiltin).\n\n> +\tstruct option *newopts = parse_options_concat(options, prevopts);\n> +\tfree(prevopts);\n\nSince we're not concatenating more than one layer on top of the common\noptions (unlike in `checkout`), this can be simplified to just return\nparse_options_concat without freeing prevopts.\n\nWe should be careful to make sure we free the return value from\nparse_options_concat eventually, though.\n\n> +\treturn newopts;\n> +}\n> +\n> static struct object_directory *find_odb(struct repository *r,\n> \t\t\t\t\t const char *obj_dir)\n> {\n> @@ -77,20 +92,18 @@ static int graph_verify(int argc, const char **argv)\n> \tint flags = 0;\n>\n> \tstatic struct option builtin_commit_graph_verify_options[] = {\n> -\t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n> -\t\t\t N_(\"dir\"),\n> -\t\t\t N_(\"the object directory to store the graph\")),\n> \t\tOPT_BOOL(0, \"shallow\", &opts.shallow,\n> \t\t\t N_(\"if the commit-graph is split, only verify the tip file\")),\n> -\t\tOPT_BOOL(0, \"progress\", &opts.progress, N_(\"force progress reporting\")),\n> \t\tOPT_END(),\n> \t};\n> +\tstruct option *options = parse_options_dup(builtin_commit_graph_verify_options);\n> +\toptions = add_common_options(options);\n\nLikewise down here (and in the other callers, too) this dup is pointless\nif we're going to immediately free it after calling\nparse_options_concat. So we can drop that, too.\n\nHere's something to consider squashing in on top:\n\n--- >8 ---\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex ff125adf2d..00b0721789 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -44,19 +44,18 @@ static struct opts_commit_graph {\n \tint enable_changed_paths;\n } opts;\n\n-static struct option *add_common_options(struct option *prevopts)\n+static struct option common_opts[] = {\n+\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n+\t\t N_(\"dir\"),\n+\t\t N_(\"the object directory to store the graph\")),\n+\tOPT_BOOL(0, \"progress\", &opts.progress,\n+\t\t N_(\"force progress reporting\")),\n+\tOPT_END()\n+};\n+\n+static struct option *add_common_options(struct option *to)\n {\n-\tstruct option options[] = {\n-\t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n-\t\t\t N_(\"dir\"),\n-\t\t\t N_(\"the object directory to store the graph\")),\n-\t\tOPT_BOOL(0, \"progress\", &opts.progress,\n-\t\t\t N_(\"force progress reporting\")),\n-\t\tOPT_END()\n-\t};\n-\tstruct option *newopts = parse_options_concat(options, prevopts);\n-\tfree(prevopts);\n-\treturn newopts;\n+\treturn parse_options_concat(common_opts, to);\n }\n\n static struct object_directory *find_odb(struct repository *r,\n@@ -96,8 +95,7 @@ static int graph_verify(int argc, const char **argv)\n \t\t\t N_(\"if the commit-graph is split, only verify the tip file\")),\n \t\tOPT_END(),\n \t};\n-\tstruct option *options = parse_options_dup(builtin_commit_graph_verify_options);\n-\toptions = add_common_options(options);\n+\tstruct option *options = add_common_options(builtin_commit_graph_verify_options);\n\n \ttrace2_cmd_mode(\"verify\");\n\n@@ -120,6 +118,7 @@ static int graph_verify(int argc, const char **argv)\n \t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n\n \tFREE_AND_NULL(graph_name);\n+\tFREE_AND_NULL(options);\n\n \tif (open_ok)\n \t\tgraph = load_commit_graph_one_fd_st(the_repository, fd, &st, odb);\n@@ -245,8 +244,7 @@ static int graph_write(int argc, const char **argv)\n \t\t\t0, write_option_max_new_filters),\n \t\tOPT_END(),\n \t};\n-\tstruct option *options = parse_options_dup(builtin_commit_graph_write_options);\n-\toptions = add_common_options(options);\n+\tstruct option *options = add_common_options(builtin_commit_graph_write_options);\n\n \topts.progress = isatty(2);\n \topts.enable_changed_paths = -1;\n@@ -316,6 +314,7 @@ static int graph_write(int argc, const char **argv)\n \t\tresult = 1;\n\n cleanup:\n+\tFREE_AND_NULL(options);\n \tstring_list_clear(&pack_indexes, 0);\n \tstrbuf_release(&buf);\n \treturn result;\n@@ -323,8 +322,7 @@ static int graph_write(int argc, const char **argv)\n\n int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n {\n-\tstruct option *no_options = parse_options_dup(NULL);\n-\tstruct option *builtin_commit_graph_options = add_common_options(no_options);\n+\tstruct option *builtin_commit_graph_options = common_opts;\n\n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix,\n"},{"id":"430518","messageId":"YPWt++xs6lrqiExV@nand.local","threadId":"56122","inReplyTo":"patch-5.5-57ffd5812d6-20210718T074936Z-avarab@gmail.com","subject":"Re: [PATCH v2 5/5] commit-graph: show usage on \"commit-graph [write|verify] garbage\"","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-19T16:53:15Z","receivedAt":"2021-07-19T17:09:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Jul 18, 2021 at 09:58:09AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Change the parse_options() invocation in the commit-graph code to make\n> sense. We're calling it twice, once for common options parsing, and\n> then for the sub-commands.\n>\n> But we never checked if we had something leftover in argc in \"write\"\n> or \"verify\", as a result we'd silently accept garbage in these\n> subcommands. Let's not do that.\n\nAll makes sense and looks good to me. One small note below:\n\n> test_expect_success 'usage' '\n> \ttest_expect_code 129 git commit-graph -h 2>err &&\n> -\t! grep error: err\n> +\t! grep error: err &&\n\nAh, now I see why you added this test back in the first patch. I still\nthink that we should get rid of the first two lines, but...\n\n> +\ttest_expect_code 129 git commit-graph write blah &&\n> +\ttest_expect_code 129 git commit-graph write verify\n\nKeeping these makes sense.\n\nThanks,\nTaylor\n"},{"id":"430519","messageId":"YPWulAIYg+q9Bmr7@nand.local","threadId":"56122","inReplyTo":"patch-4.5-d776424e8c8-20210718T074936Z-avarab@gmail.com","subject":"Re: [PATCH v2 4/5] commit-graph: early exit to \"usage\" on !argc","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-19T16:55:48Z","receivedAt":"2021-07-19T17:09:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Jul 18, 2021 at 09:58:08AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Rather than guarding all of the !argc with an additional \"if\" arm\n> let's do an early goto to \"usage\". This also makes it clear that\n> \"save_commit_buffer\" is not needed in this case.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/commit-graph.c | 13 +++++++------\n> 1 file changed, 7 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index ff125adf2d5..16d2c517e72 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -331,16 +331,17 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n> \t\t\t builtin_commit_graph_options,\n> \t\t\t builtin_commit_graph_usage,\n> \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n> +\tif (!argc)\n> +\t\tgoto usage;\n>\n> \tsave_commit_buffer = 0;\n>\n> -\tif (argc > 0) {\n> -\t\tif (!strcmp(argv[0], \"verify\"))\n> -\t\t\treturn graph_verify(argc, argv);\n> -\t\tif (!strcmp(argv[0], \"write\"))\n> -\t\t\treturn graph_write(argc, argv);\n> -\t}\n> +\tif (!strcmp(argv[0], \"verify\"))\n> +\t\treturn graph_verify(argc, argv);\n> +\telse if (argc && !strcmp(argv[0], \"write\"))\n> +\t\treturn graph_write(argc, argv);\n\nUnrelated to your patch here, but it may be nice to have an extra\nerror() for unrecognized sub-commands. I actually think that this\nsuggestion also comes from Peff originally, but it does make the \"next\nstep\" a lot clearer to users who otherwise would have seen a wall of\nusage text.\n\nPerhaps:\n\n else {\n error(_(\"unrecognized subcommand: %s\"), argv[0]);\nusage:\n usage_with_options(builtin_commit_graph_usage,\n builtin_commit_graph_options);\n }\n\nThanks,\nTaylor\n"},{"id":"430609","messageId":"8735s9w74r.fsf@evledraar.gmail.com","threadId":"56122","inReplyTo":"YPWtRd+523N1cWR/@nand.local","subject":"Re: [PATCH v2 3/5] commit-graph: use parse_options_concat()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-20T11:31:15Z","receivedAt":"2021-07-20T11:35:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jul 19 2021, Taylor Blau wrote:\n\n> On Sun, Jul 18, 2021 at 09:58:07AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> Make use of the parse_options_concat() so we don't need to copy/paste\n>> common options like --object-dir. This is inspired by a similar change\n>> to \"checkout\" in 2087182272\n>> (checkout: split options[] array in three pieces, 2019-03-29).\n>>\n>> A minor behavior change here is that now we're going to list both\n>> --object-dir and --progress first, before we'd list --progress along\n>> with other options.\n>\n> This is very reminiscent to the patch I sent to do the same in the\n> `multi-pack-index` builtin, probably because you were the person to\n> recommend I do that cleanup in the first place ;).\n>\n> I got some good advice from Peff in [1] went I sent that patch, which\n> I'll try to summarize here, since I think a few pieces of it could be\n> applied to clean up this patch a little.\n>\n> [1]: https://lore.kernel.org/git/YGG7tWBzo5NGl2+g@coredump.intra.peff.net/\n\nThanks.\n\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> builtin/commit-graph.c | 39 +++++++++++++++++++++++----------------\n>> 1 file changed, 23 insertions(+), 16 deletions(-)\n>>\n>> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n>> index baead04a03b..ff125adf2d5 100644\n>> --- a/builtin/commit-graph.c\n>> +++ b/builtin/commit-graph.c\n>> @@ -44,6 +44,21 @@ static struct opts_commit_graph {\n>> \tint enable_changed_paths;\n>> } opts;\n>>\n>> +static struct option *add_common_options(struct option *prevopts)\n>> +{\n>> +\tstruct option options[] = {\n>> +\t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n>> +\t\t\t N_(\"dir\"),\n>> +\t\t\t N_(\"the object directory to store the graph\")),\n>> +\t\tOPT_BOOL(0, \"progress\", &opts.progress,\n>> +\t\t\t N_(\"force progress reporting\")),\n>> +\t\tOPT_END()\n>> +\t};\n>\n> Not from Peff's mail, but is there any reason to non statically allocate\n> this?\n>\n> The only reason I could think of is that `opts` is heap allocated, but\n> it's not, so I think we could probably mark `options` as static here\n> (and perhaps rename it to `common_opts` while we're at it, if for no\n> other reason than to be consistent with what's in the multi-pack-index\n> builtin).\n\nI've applied your suggestions in full, thank a lot.\n\nI'm a bit on the fence about this one, because it works here, but not in\nother cases where the \"options\" refers to variables declared in the\nfunction, i.e. &progress after an \"int progress = 0\" or whatever.\n\nMaking it all global seems like a bit of an anti-pattern, and having to\nrefactor it if we ever need to change it to rely on something dynamic in\nthe function is arguably bad.\n\nAnd arguably not, because if we use this static pattern consistently\nthose cases will stick out more, whereas a lot of builtins now declare\nthem non-static in the function for no particular reason, and it's not\nlike we're harmed those commands hardcoding things at the top level.\n\nBut you won't find it trivial to e.g. migrate builtin/checkout.c to this\npattern, whereas using the non-static one is easy everywhere.\n\nAnyway, I squashed this in, just say'n. Having written the above I'm\nstill not sure what I think about it :)\n"},{"id":"430610","messageId":"cover-0.6-00000000000-20210720T113707Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-0.5-00000000000-20210718T074936Z-avarab@gmail.com","subject":"[PATCH v3 0/6] commit-graph: usage fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-20T11:39:39Z","receivedAt":"2021-07-20T11:40:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fixes hopefully all the comments on v2[1], thanks Andrei and Taylor.\n\nThere's now a small mid-series digression before to also refactor the\n\"early exit to \"usage\" on !argc\" for the multi-pack-index.\n\nTaylor suggested using braces for the \"else\" so maybe he won't like it\n:)\n\nI figured having the two similar commands use the same pattern was\nworth the digression.\n\n1. http://lore.kernel.org/git/cover-0.5-00000000000-20210718T074936Z-avarab@gmail.com\n\nÆvar Arnfjörð Bjarmason (6):\n commit-graph: define common usage with a macro\n commit-graph: remove redundant handling of -h\n commit-graph: use parse_options_concat()\n multi-pack-index: refactor \"goto usage\" pattern\n commit-graph: early exit to \"usage\" on !argc\n commit-graph: show usage on \"commit-graph [write|verify] garbage\"\n\n builtin/commit-graph.c | 95 +++++++++++++++++++++-----------------\n builtin/multi-pack-index.c | 11 ++---\n t/t5318-commit-graph.sh | 5 ++\n 3 files changed, 63 insertions(+), 48 deletions(-)\n\nRange-diff against v2:\n1: ee6630b7c0d ! 1: 1b9b4703ce2 commit-graph: define common usage with a macro\n @@ builtin/commit-graph.c\n -\t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \"\n -\t \"[--changed-paths] [--[no-]max-new-filters ] [--[no-]progress] \"\n -\t \"\"),\n -+static const char * builtin_commit_graph_verify_usage[] = {\n +#define BUILTIN_COMMIT_GRAPH_VERIFY_USAGE \\\n +\tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\")\n ++\n ++#define BUILTIN_COMMIT_GRAPH_WRITE_USAGE \\\n ++\tN_(\"git commit-graph write [--object-dir ] [--append] \" \\\n ++\t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \" \\\n ++\t \"[--changed-paths] [--[no-]max-new-filters ] [--[no-]progress] \" \\\n ++\t \"\")\n ++\n ++static const char * builtin_commit_graph_verify_usage[] = {\n +\tBUILTIN_COMMIT_GRAPH_VERIFY_USAGE,\n \tNULL\n };\n @@ builtin/commit-graph.c\n -static const char * const builtin_commit_graph_verify_usage[] = {\n -\tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n +static const char * builtin_commit_graph_write_usage[] = {\n -+#define BUILTIN_COMMIT_GRAPH_WRITE_USAGE \\\n -+\tN_(\"git commit-graph write [--object-dir ] [--append] \" \\\n -+\t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \" \\\n -+\t \"[--changed-paths] [--[no-]max-new-filters ] [--[no-]progress] \" \\\n -+\t \"\")\n +\tBUILTIN_COMMIT_GRAPH_WRITE_USAGE,\n \tNULL\n };\n2: 03581d85781 ! 2: 8f50750ae5e commit-graph: remove redundant handling of -h\n @@ Commit message\n parse_options() did this at the time, and the commit-graph code never\n used PARSE_OPT_NO_INTERNAL_HELP.\n \n + We don't need a test for this, it's tested by the t0012-help.sh test\n + added in d691551192a (t0012: test \"-h\" with builtins, 2017-05-30).\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## builtin/commit-graph.c ##\n @@ builtin/commit-graph.c: int cmd_commit_graph(int argc, const char **argv, const\n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_commit_graph_options,\n -\n - ## t/t5318-commit-graph.sh ##\n -@@ t/t5318-commit-graph.sh: test_description='commit graph'\n - \n - GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0\n - \n -+test_expect_success 'usage' '\n -+\ttest_expect_code 129 git commit-graph -h 2>err &&\n -+\t! grep error: err\n -+'\n -+\n - test_expect_success 'setup full repo' '\n - \tmkdir full &&\n - \tcd \"$TRASH_DIRECTORY/full\" &&\n3: 8e909cd9c23 ! 3: f02da994363 commit-graph: use parse_options_concat()\n @@ Commit message\n commit-graph: use parse_options_concat()\n \n Make use of the parse_options_concat() so we don't need to copy/paste\n - common options like --object-dir. This is inspired by a similar change\n - to \"checkout\" in 2087182272\n - (checkout: split options[] array in three pieces, 2019-03-29).\n + common options like --object-dir.\n +\n + This is inspired by a similar change to \"checkout\" in 2087182272\n + (checkout: split options[] array in three pieces, 2019-03-29), and the\n + same pattern in the multi-pack-index command, see\n + 60ca94769ce (builtin/multi-pack-index.c: split sub-commands,\n + 2021-03-30).\n \n A minor behavior change here is that now we're going to list both\n --object-dir and --progress first, before we'd list --progress along\n with other options.\n \n + Co-authored-by: Taylor Blau \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## builtin/commit-graph.c ##\n @@ builtin/commit-graph.c: static struct opts_commit_graph {\n \tint enable_changed_paths;\n } opts;\n \n -+static struct option *add_common_options(struct option *prevopts)\n ++static struct option common_opts[] = {\n ++\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n ++\t\t N_(\"dir\"),\n ++\t\t N_(\"the object directory to store the graph\")),\n ++\tOPT_BOOL(0, \"progress\", &opts.progress,\n ++\t\t N_(\"force progress reporting\")),\n ++\tOPT_END()\n ++};\n ++\n ++static struct option *add_common_options(struct option *to)\n +{\n -+\tstruct option options[] = {\n -+\t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n -+\t\t\t N_(\"dir\"),\n -+\t\t\t N_(\"the object directory to store the graph\")),\n -+\t\tOPT_BOOL(0, \"progress\", &opts.progress,\n -+\t\t\t N_(\"force progress reporting\")),\n -+\t\tOPT_END()\n -+\t};\n -+\tstruct option *newopts = parse_options_concat(options, prevopts);\n -+\tfree(prevopts);\n -+\treturn newopts;\n ++\treturn parse_options_concat(common_opts, to);\n +}\n +\n static struct object_directory *find_odb(struct repository *r,\n @@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)\n -\t\tOPT_BOOL(0, \"progress\", &opts.progress, N_(\"force progress reporting\")),\n \t\tOPT_END(),\n \t};\n -+\tstruct option *options = parse_options_dup(builtin_commit_graph_verify_options);\n -+\toptions = add_common_options(options);\n ++\tstruct option *options = add_common_options(builtin_commit_graph_verify_options);\n \n \ttrace2_cmd_mode(\"verify\");\n \n @@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)\n \t\t\t builtin_commit_graph_verify_usage, 0);\n \n \tif (!opts.obj_dir)\n +@@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)\n + \t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n + \n + \tFREE_AND_NULL(graph_name);\n ++\tFREE_AND_NULL(options);\n + \n + \tif (open_ok)\n + \t\tgraph = load_commit_graph_one_fd_st(the_repository, fd, &st, odb);\n @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n \tstruct progress *progress = NULL;\n \n @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n \t\t\t0, write_option_max_new_filters),\n \t\tOPT_END(),\n \t};\n -+\tstruct option *options = parse_options_dup(builtin_commit_graph_write_options);\n -+\toptions = add_common_options(options);\n ++\tstruct option *options = add_common_options(builtin_commit_graph_write_options);\n \n \topts.progress = isatty(2);\n \topts.enable_changed_paths = -1;\n @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n \n \tif (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1)\n @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n + \t\tresult = 1;\n + \n + cleanup:\n ++\tFREE_AND_NULL(options);\n + \tstring_list_clear(&pack_indexes, 0);\n + \tstrbuf_release(&buf);\n + \treturn result;\n +@@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n \n int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n {\n @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n -\t\t\tN_(\"the object directory to store the graph\")),\n -\t\tOPT_END(),\n -\t};\n -+\tstruct option *no_options = parse_options_dup(NULL);\n -+\tstruct option *builtin_commit_graph_options = add_common_options(no_options);\n ++\tstruct option *builtin_commit_graph_options = common_opts;\n \n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix,\n-: ----------- > 4: 6e9bd877866 multi-pack-index: refactor \"goto usage\" pattern\n4: 6801fb18faa = 5: 7acb4bd75ce commit-graph: early exit to \"usage\" on !argc\n5: 5c96699496b ! 6: 5c1694e071e commit-graph: show usage on \"commit-graph [write|verify] garbage\"\n @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n \t\tdie(_(\"use at most one of --reachable, --stdin-commits, or --stdin-packs\"));\n \n ## t/t5318-commit-graph.sh ##\n -@@ t/t5318-commit-graph.sh: GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0\n +@@ t/t5318-commit-graph.sh: test_description='commit graph'\n \n - test_expect_success 'usage' '\n - \ttest_expect_code 129 git commit-graph -h 2>err &&\n --\t! grep error: err\n -+\t! grep error: err &&\n + GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0\n + \n ++test_expect_success 'usage' '\n +\ttest_expect_code 129 git commit-graph write blah &&\n +\ttest_expect_code 129 git commit-graph write verify\n - '\n - \n ++'\n ++\n test_expect_success 'setup full repo' '\n + \tmkdir full &&\n + \tcd \"$TRASH_DIRECTORY/full\" &&\n-- \n2.32.0.874.ge7a9d58bfcf\n\n"},{"id":"430611","messageId":"patch-1.6-1b9b4703ce2-20210720T113707Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-0.6-00000000000-20210720T113707Z-avarab@gmail.com","subject":"[PATCH v3 1/6] commit-graph: define common usage with a macro","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-20T11:39:40Z","receivedAt":"2021-07-20T11:40:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Share the usage message between these three variables by using a\nmacro. Before this new options needed to copy/paste the usage\ninformation, see e.g. 809e0327f5 (builtin/commit-graph.c: introduce\n'--max-new-filters=', 2020-09-18).\n\nSee b25b727494f (builtin/multi-pack-index.c: define common usage with\na macro, 2021-03-30) for another use of this pattern (but on-list this\none came first).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 31 +++++++++++++++++--------------\n 1 file changed, 17 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex cd863152216..5af3cd7178f 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -9,26 +9,29 @@\n #include \"progress.h\"\n #include \"tag.h\"\n \n-static char const * const builtin_commit_graph_usage[] = {\n-\tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n-\tN_(\"git commit-graph write [--object-dir ] [--append] \"\n-\t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \"\n-\t \"[--changed-paths] [--[no-]max-new-filters ] [--[no-]progress] \"\n-\t \"\"),\n+#define BUILTIN_COMMIT_GRAPH_VERIFY_USAGE \\\n+\tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\")\n+\n+#define BUILTIN_COMMIT_GRAPH_WRITE_USAGE \\\n+\tN_(\"git commit-graph write [--object-dir ] [--append] \" \\\n+\t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \" \\\n+\t \"[--changed-paths] [--[no-]max-new-filters ] [--[no-]progress] \" \\\n+\t \"\")\n+\n+static const char * builtin_commit_graph_verify_usage[] = {\n+\tBUILTIN_COMMIT_GRAPH_VERIFY_USAGE,\n \tNULL\n };\n \n-static const char * const builtin_commit_graph_verify_usage[] = {\n-\tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n+static const char * builtin_commit_graph_write_usage[] = {\n+\tBUILTIN_COMMIT_GRAPH_WRITE_USAGE,\n \tNULL\n };\n \n-static const char * const builtin_commit_graph_write_usage[] = {\n-\tN_(\"git commit-graph write [--object-dir ] [--append] \"\n-\t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \"\n-\t \"[--changed-paths] [--[no-]max-new-filters ] [--[no-]progress] \"\n-\t \"\"),\n-\tNULL\n+static char const * const builtin_commit_graph_usage[] = {\n+\tBUILTIN_COMMIT_GRAPH_VERIFY_USAGE,\n+\tBUILTIN_COMMIT_GRAPH_WRITE_USAGE,\n+\tNULL,\n };\n \n static struct opts_commit_graph {\n-- \n2.32.0.874.ge7a9d58bfcf\n\n"},{"id":"430612","messageId":"patch-2.6-8f50750ae5e-20210720T113707Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-0.6-00000000000-20210720T113707Z-avarab@gmail.com","subject":"[PATCH v3 2/6] commit-graph: remove redundant handling of -h","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-20T11:39:41Z","receivedAt":"2021-07-20T11:40:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"If we don't handle the -h option here like most parse_options() users\nwe'll fall through and it'll do the right thing for us.\n\nI think this code added in 4ce58ee38d (commit-graph: create\ngit-commit-graph builtin, 2018-04-02) was always redundant,\nparse_options() did this at the time, and the commit-graph code never\nused PARSE_OPT_NO_INTERNAL_HELP.\n\nWe don't need a test for this, it's tested by the t0012-help.sh test\nadded in d691551192a (t0012: test \"-h\" with builtins, 2017-05-30).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 5af3cd7178f..3cf18dc5345 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -321,10 +321,6 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n \t\tOPT_END(),\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_commit_graph_usage,\n-\t\t\t\t builtin_commit_graph_options);\n-\n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_commit_graph_options,\n-- \n2.32.0.874.ge7a9d58bfcf\n\n"},{"id":"430613","messageId":"patch-3.6-f02da994363-20210720T113707Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-0.6-00000000000-20210720T113707Z-avarab@gmail.com","subject":"[PATCH v3 3/6] commit-graph: use parse_options_concat()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-20T11:39:42Z","receivedAt":"2021-07-20T11:41:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Make use of the parse_options_concat() so we don't need to copy/paste\ncommon options like --object-dir.\n\nThis is inspired by a similar change to \"checkout\" in 2087182272\n(checkout: split options[] array in three pieces, 2019-03-29), and the\nsame pattern in the multi-pack-index command, see\n60ca94769ce (builtin/multi-pack-index.c: split sub-commands,\n2021-03-30).\n\nA minor behavior change here is that now we're going to list both\n--object-dir and --progress first, before we'd list --progress along\nwith other options.\n\nCo-authored-by: Taylor Blau \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 37 +++++++++++++++++++++----------------\n 1 file changed, 21 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 3cf18dc5345..6e49184439f 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -46,6 +46,20 @@ static struct opts_commit_graph {\n \tint enable_changed_paths;\n } opts;\n \n+static struct option common_opts[] = {\n+\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n+\t\t N_(\"dir\"),\n+\t\t N_(\"the object directory to store the graph\")),\n+\tOPT_BOOL(0, \"progress\", &opts.progress,\n+\t\t N_(\"force progress reporting\")),\n+\tOPT_END()\n+};\n+\n+static struct option *add_common_options(struct option *to)\n+{\n+\treturn parse_options_concat(common_opts, to);\n+}\n+\n static struct object_directory *find_odb(struct repository *r,\n \t\t\t\t\t const char *obj_dir)\n {\n@@ -79,20 +93,17 @@ static int graph_verify(int argc, const char **argv)\n \tint flags = 0;\n \n \tstatic struct option builtin_commit_graph_verify_options[] = {\n-\t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n-\t\t\t N_(\"dir\"),\n-\t\t\t N_(\"the object directory to store the graph\")),\n \t\tOPT_BOOL(0, \"shallow\", &opts.shallow,\n \t\t\t N_(\"if the commit-graph is split, only verify the tip file\")),\n-\t\tOPT_BOOL(0, \"progress\", &opts.progress, N_(\"force progress reporting\")),\n \t\tOPT_END(),\n \t};\n+\tstruct option *options = add_common_options(builtin_commit_graph_verify_options);\n \n \ttrace2_cmd_mode(\"verify\");\n \n \topts.progress = isatty(2);\n \targc = parse_options(argc, argv, NULL,\n-\t\t\t builtin_commit_graph_verify_options,\n+\t\t\t options,\n \t\t\t builtin_commit_graph_verify_usage, 0);\n \n \tif (!opts.obj_dir)\n@@ -109,6 +120,7 @@ static int graph_verify(int argc, const char **argv)\n \t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n \n \tFREE_AND_NULL(graph_name);\n+\tFREE_AND_NULL(options);\n \n \tif (open_ok)\n \t\tgraph = load_commit_graph_one_fd_st(the_repository, fd, &st, odb);\n@@ -209,9 +221,6 @@ static int graph_write(int argc, const char **argv)\n \tstruct progress *progress = NULL;\n \n \tstatic struct option builtin_commit_graph_write_options[] = {\n-\t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n-\t\t\tN_(\"dir\"),\n-\t\t\tN_(\"the object directory to store the graph\")),\n \t\tOPT_BOOL(0, \"reachable\", &opts.reachable,\n \t\t\tN_(\"start walk at all refs\")),\n \t\tOPT_BOOL(0, \"stdin-packs\", &opts.stdin_packs,\n@@ -222,7 +231,6 @@ static int graph_write(int argc, const char **argv)\n \t\t\tN_(\"include all commits already in the commit-graph file\")),\n \t\tOPT_BOOL(0, \"changed-paths\", &opts.enable_changed_paths,\n \t\t\tN_(\"enable computation for changed paths\")),\n-\t\tOPT_BOOL(0, \"progress\", &opts.progress, N_(\"force progress reporting\")),\n \t\tOPT_CALLBACK_F(0, \"split\", &write_opts.split_flags, NULL,\n \t\t\tN_(\"allow writing an incremental commit-graph file\"),\n \t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n@@ -238,6 +246,7 @@ static int graph_write(int argc, const char **argv)\n \t\t\t0, write_option_max_new_filters),\n \t\tOPT_END(),\n \t};\n+\tstruct option *options = add_common_options(builtin_commit_graph_write_options);\n \n \topts.progress = isatty(2);\n \topts.enable_changed_paths = -1;\n@@ -251,7 +260,7 @@ static int graph_write(int argc, const char **argv)\n \tgit_config(git_commit_graph_write_config, &opts);\n \n \targc = parse_options(argc, argv, NULL,\n-\t\t\t builtin_commit_graph_write_options,\n+\t\t\t options,\n \t\t\t builtin_commit_graph_write_usage, 0);\n \n \tif (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1)\n@@ -307,6 +316,7 @@ static int graph_write(int argc, const char **argv)\n \t\tresult = 1;\n \n cleanup:\n+\tFREE_AND_NULL(options);\n \tstring_list_clear(&pack_indexes, 0);\n \tstrbuf_release(&buf);\n \treturn result;\n@@ -314,12 +324,7 @@ static int graph_write(int argc, const char **argv)\n \n int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n {\n-\tstatic struct option builtin_commit_graph_options[] = {\n-\t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n-\t\t\tN_(\"dir\"),\n-\t\t\tN_(\"the object directory to store the graph\")),\n-\t\tOPT_END(),\n-\t};\n+\tstruct option *builtin_commit_graph_options = common_opts;\n \n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix,\n-- \n2.32.0.874.ge7a9d58bfcf\n\n"},{"id":"430614","messageId":"patch-4.6-6e9bd877866-20210720T113707Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-0.6-00000000000-20210720T113707Z-avarab@gmail.com","subject":"[PATCH v3 4/6] multi-pack-index: refactor \"goto usage\" pattern","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-20T11:39:43Z","receivedAt":"2021-07-20T11:42:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Refactor the \"goto usage\" pattern added in\ncd57bc41bbc (builtin/multi-pack-index.c: display usage on unrecognized\ncommand, 2021-03-30) to maintain the same brevity, but doesn't run\nafoul of the recommendation in CodingGuidelines about braces:\n\n When there are multiple arms to a conditional and some of them\n require braces, enclose even a single line block in braces for\n consistency[...]\n\nLet's also change \"argv == 0\" to juts \"!argv\", per:\n\n Do not explicitly compare an integral value with constant 0 or\n '\\0', or a pointer value with constant NULL[...]\n\nI'm changing this because in a subsequent commit I'll make\nbuiltin/commit-graph.c use the same pattern, having the two similarly\nstructured commands match aids readability.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/multi-pack-index.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex 5d3ea445fdb..2952388a8eb 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -164,7 +164,7 @@ int cmd_multi_pack_index(int argc, const char **argv,\n \tif (!opts.object_dir)\n \t\topts.object_dir = get_object_directory();\n \n-\tif (argc == 0)\n+\tif (!argc)\n \t\tgoto usage;\n \n \tif (!strcmp(argv[0], \"repack\"))\n@@ -175,10 +175,9 @@ int cmd_multi_pack_index(int argc, const char **argv,\n \t\treturn cmd_multi_pack_index_verify(argc, argv);\n \telse if (!strcmp(argv[0], \"expire\"))\n \t\treturn cmd_multi_pack_index_expire(argc, argv);\n-\telse {\n+\n usage:\n-\t\terror(_(\"unrecognized subcommand: %s\"), argv[0]);\n-\t\tusage_with_options(builtin_multi_pack_index_usage,\n-\t\t\t\t builtin_multi_pack_index_options);\n-\t}\n+\terror(_(\"unrecognized subcommand: %s\"), argv[0]);\n+\tusage_with_options(builtin_multi_pack_index_usage,\n+\t\t\t builtin_multi_pack_index_options);\n }\n-- \n2.32.0.874.ge7a9d58bfcf\n\n"},{"id":"430615","messageId":"patch-6.6-5c1694e071e-20210720T113707Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-0.6-00000000000-20210720T113707Z-avarab@gmail.com","subject":"[PATCH v3 6/6] commit-graph: show usage on \"commit-graph [write|verify] garbage\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-20T11:39:45Z","receivedAt":"2021-07-20T11:42:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the parse_options() invocation in the commit-graph code to make\nsense. We're calling it twice, once for common options parsing, and\nthen for the sub-commands.\n\nBut we never checked if we had something leftover in argc in \"write\"\nor \"verify\", as a result we'd silently accept garbage in these\nsubcommands. Let's not do that.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 10 ++++++++--\n t/t5318-commit-graph.sh | 5 +++++\n 2 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex bf34aa43f22..88cbcb5a64f 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -104,7 +104,10 @@ static int graph_verify(int argc, const char **argv)\n \topts.progress = isatty(2);\n \targc = parse_options(argc, argv, NULL,\n \t\t\t options,\n-\t\t\t builtin_commit_graph_verify_usage, 0);\n+\t\t\t builtin_commit_graph_verify_usage,\n+\t\t\t PARSE_OPT_KEEP_UNKNOWN);\n+\tif (argc)\n+\t\tusage_with_options(builtin_commit_graph_verify_usage, options);\n \n \tif (!opts.obj_dir)\n \t\topts.obj_dir = get_object_directory();\n@@ -261,7 +264,10 @@ static int graph_write(int argc, const char **argv)\n \n \targc = parse_options(argc, argv, NULL,\n \t\t\t options,\n-\t\t\t builtin_commit_graph_write_usage, 0);\n+\t\t\t builtin_commit_graph_write_usage,\n+\t\t\t PARSE_OPT_KEEP_UNKNOWN);\n+\tif (argc)\n+\t\tusage_with_options(builtin_commit_graph_write_usage, options);\n \n \tif (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1)\n \t\tdie(_(\"use at most one of --reachable, --stdin-commits, or --stdin-packs\"));\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex af88f805aa2..09a2ccd2920 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -5,6 +5,11 @@ test_description='commit graph'\n \n GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0\n \n+test_expect_success 'usage' '\n+\ttest_expect_code 129 git commit-graph write blah &&\n+\ttest_expect_code 129 git commit-graph write verify\n+'\n+\n test_expect_success 'setup full repo' '\n \tmkdir full &&\n \tcd \"$TRASH_DIRECTORY/full\" &&\n-- \n2.32.0.874.ge7a9d58bfcf\n\n"},{"id":"430616","messageId":"patch-5.6-7acb4bd75ce-20210720T113707Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-0.6-00000000000-20210720T113707Z-avarab@gmail.com","subject":"[PATCH v3 5/6] commit-graph: early exit to \"usage\" on !argc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-20T11:39:44Z","receivedAt":"2021-07-20T11:42:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Rather than guarding all of the !argc with an additional \"if\" arm\nlet's do an early goto to \"usage\". This also makes it clear that\n\"save_commit_buffer\" is not needed in this case.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 6e49184439f..bf34aa43f22 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -331,16 +331,17 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n \t\t\t builtin_commit_graph_options,\n \t\t\t builtin_commit_graph_usage,\n \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n+\tif (!argc)\n+\t\tgoto usage;\n \n \tsave_commit_buffer = 0;\n \n-\tif (argc > 0) {\n-\t\tif (!strcmp(argv[0], \"verify\"))\n-\t\t\treturn graph_verify(argc, argv);\n-\t\tif (!strcmp(argv[0], \"write\"))\n-\t\t\treturn graph_write(argc, argv);\n-\t}\n+\tif (!strcmp(argv[0], \"verify\"))\n+\t\treturn graph_verify(argc, argv);\n+\telse if (argc && !strcmp(argv[0], \"write\"))\n+\t\treturn graph_write(argc, argv);\n \n+usage:\n \tusage_with_options(builtin_commit_graph_usage,\n \t\t\t builtin_commit_graph_options);\n }\n-- \n2.32.0.874.ge7a9d58bfcf\n\n"},{"id":"430671","messageId":"20210720174739.GA2025@szeder.dev","threadId":"56122","inReplyTo":"patch-6.6-5c1694e071e-20210720T113707Z-avarab@gmail.com","subject":"Re: [PATCH v3 6/6] commit-graph: show usage on \"commit-graph [write|verify] garbage\"","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-07-20T17:47:39Z","receivedAt":"2021-07-20T17:47:52Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Jul 20, 2021 at 01:39:45PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Change the parse_options() invocation in the commit-graph code to make\n> sense. We're calling it twice, once for common options parsing, and\n> then for the sub-commands.\n> \n> But we never checked if we had something leftover in argc in \"write\"\n> or \"verify\", as a result we'd silently accept garbage in these\n> subcommands. Let's not do that.\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/commit-graph.c | 10 ++++++++--\n> t/t5318-commit-graph.sh | 5 +++++\n> 2 files changed, 13 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index bf34aa43f22..88cbcb5a64f 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -104,7 +104,10 @@ static int graph_verify(int argc, const char **argv)\n> \topts.progress = isatty(2);\n> \targc = parse_options(argc, argv, NULL,\n> \t\t\t options,\n> -\t\t\t builtin_commit_graph_verify_usage, 0);\n> +\t\t\t builtin_commit_graph_verify_usage,\n> +\t\t\t PARSE_OPT_KEEP_UNKNOWN);\n> +\tif (argc)\n> +\t\tusage_with_options(builtin_commit_graph_verify_usage, options);\n\nChecking 'argc' alone is sufficient to catch unsupported parameters.\n\nUsing PARSE_OPT_KEEP_UNKNOWN is not only unnecessary, but arguably\nwrong here, because 'git commit-graph write --foo' won't print \"error:\nunknown option `foo'\", and we don't want to pass the remaining\nunrecognized options to a different command, like e.g. 'git difftool',\nor another parse_options(), like e.g. 'git archive'.\n\n> \tif (!opts.obj_dir)\n> \t\topts.obj_dir = get_object_directory();\n> @@ -261,7 +264,10 @@ static int graph_write(int argc, const char **argv)\n> \n> \targc = parse_options(argc, argv, NULL,\n> \t\t\t options,\n> -\t\t\t builtin_commit_graph_write_usage, 0);\n> +\t\t\t builtin_commit_graph_write_usage,\n> +\t\t\t PARSE_OPT_KEEP_UNKNOWN);\n> +\tif (argc)\n> +\t\tusage_with_options(builtin_commit_graph_write_usage, options);\n> \n> \tif (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1)\n> \t\tdie(_(\"use at most one of --reachable, --stdin-commits, or --stdin-packs\"));\n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index af88f805aa2..09a2ccd2920 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -5,6 +5,11 @@ test_description='commit graph'\n> \n> GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0\n> \n> +test_expect_success 'usage' '\n> +\ttest_expect_code 129 git commit-graph write blah &&\n> +\ttest_expect_code 129 git commit-graph write verify\n> +'\n> +\n> test_expect_success 'setup full repo' '\n> \tmkdir full &&\n> \tcd \"$TRASH_DIRECTORY/full\" &&\n> -- \n> 2.32.0.874.ge7a9d58bfcf\n> \n"},{"id":"430672","messageId":"20210720175530.GA23408@szeder.dev","threadId":"56122","inReplyTo":"20210720174739.GA2025@szeder.dev","subject":"Re: [PATCH v3 6/6] commit-graph: show usage on \"commit-graph [write|verify] garbage\"","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2021-07-20T17:55:30Z","receivedAt":"2021-07-20T17:55:53Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Jul 20, 2021 at 07:47:39PM +0200, SZEDER Gábor wrote:\n> On Tue, Jul 20, 2021 at 01:39:45PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> > Change the parse_options() invocation in the commit-graph code to make\n> > sense. We're calling it twice, once for common options parsing, and\n> > then for the sub-commands.\n> > \n> > But we never checked if we had something leftover in argc in \"write\"\n> > or \"verify\", as a result we'd silently accept garbage in these\n> > subcommands. Let's not do that.\n> > \n> > Signed-off-by: Ævar Arnfjörð Bjarmason \n> > ---\n> > builtin/commit-graph.c | 10 ++++++++--\n> > t/t5318-commit-graph.sh | 5 +++++\n> > 2 files changed, 13 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> > index bf34aa43f22..88cbcb5a64f 100644\n> > --- a/builtin/commit-graph.c\n> > +++ b/builtin/commit-graph.c\n> > @@ -104,7 +104,10 @@ static int graph_verify(int argc, const char **argv)\n> > \topts.progress = isatty(2);\n> > \targc = parse_options(argc, argv, NULL,\n> > \t\t\t options,\n> > -\t\t\t builtin_commit_graph_verify_usage, 0);\n> > +\t\t\t builtin_commit_graph_verify_usage,\n> > +\t\t\t PARSE_OPT_KEEP_UNKNOWN);\n> > +\tif (argc)\n> > +\t\tusage_with_options(builtin_commit_graph_verify_usage, options);\n> \n> Checking 'argc' alone is sufficient to catch unsupported parameters.\n> \n> Using PARSE_OPT_KEEP_UNKNOWN is not only unnecessary, but arguably\n> wrong here\n\nAnd it's wrong in 'multi-pack-index' and 'env--helper' as well.\n\n> because 'git commit-graph write --foo' won't print \"error:\n> unknown option `foo'\", and we don't want to pass the remaining\n> unrecognized options to a different command, like e.g. 'git difftool',\n> or another parse_options(), like e.g. 'git archive'.\n"},{"id":"430674","messageId":"YPcSemODaPJ8rkOr@nand.local","threadId":"56122","inReplyTo":"patch-4.6-6e9bd877866-20210720T113707Z-avarab@gmail.com","subject":"Re: [PATCH v3 4/6] multi-pack-index: refactor \"goto usage\" pattern","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-20T18:14:18Z","receivedAt":"2021-07-20T18:15:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jul 20, 2021 at 01:39:43PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Refactor the \"goto usage\" pattern added in\n> cd57bc41bbc (builtin/multi-pack-index.c: display usage on unrecognized\n> command, 2021-03-30) to maintain the same brevity, but doesn't run\n> afoul of the recommendation in CodingGuidelines about braces:\n>\n> When there are multiple arms to a conditional and some of them\n> require braces, enclose even a single line block in braces for\n> consistency[...]\n>\n> Let's also change \"argv == 0\" to juts \"!argv\", per:\n>\n> Do not explicitly compare an integral value with constant 0 or\n> '\\0', or a pointer value with constant NULL[...]\n>\n> I'm changing this because in a subsequent commit I'll make\n> builtin/commit-graph.c use the same pattern, having the two similarly\n> structured commands match aids readability.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/multi-pack-index.c | 11 +++++------\n> 1 file changed, 5 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\n> index 5d3ea445fdb..2952388a8eb 100644\n> --- a/builtin/multi-pack-index.c\n> +++ b/builtin/multi-pack-index.c\n> @@ -164,7 +164,7 @@ int cmd_multi_pack_index(int argc, const char **argv,\n> \tif (!opts.object_dir)\n> \t\topts.object_dir = get_object_directory();\n>\n> -\tif (argc == 0)\n> +\tif (!argc)\n> \t\tgoto usage;\n>\n> \tif (!strcmp(argv[0], \"repack\"))\n> @@ -175,10 +175,9 @@ int cmd_multi_pack_index(int argc, const char **argv,\n> \t\treturn cmd_multi_pack_index_verify(argc, argv);\n> \telse if (!strcmp(argv[0], \"expire\"))\n> \t\treturn cmd_multi_pack_index_expire(argc, argv);\n> -\telse {\n> +\n> usage:\n> -\t\terror(_(\"unrecognized subcommand: %s\"), argv[0]);\n> -\t\tusage_with_options(builtin_multi_pack_index_usage,\n> -\t\t\t\t builtin_multi_pack_index_options);\n> -\t}\n> +\terror(_(\"unrecognized subcommand: %s\"), argv[0]);\n\nNot the fault of this patch, but since we jump to this from the \"if\n(!argc)\" conditional, reading \"argv[0]\" is UB. Some compilers will print\nout:\n\n error: unrecognized subcommand: (null)\n\nwhich at least doesn't segfault, but is still ugly. I don't mind calling\nit ugly since I was the one to introduce this behavior (by accident, of\ncourse).\n\nI sent a patch [1] yesterday to squash this, but we should consider\ncombining the two (or dropping this patch from the series and then\ncoming back to it later on).\n\nIf you want to combine forces, here's what I think that could look like\n(without your s-o-b, which you'll have to reattach).\n\n[1]: https://lore.kernel.org/git/8c0bb3e0dc121bd68f7014000fbb60b28750a0fe.1626715096.git.me@ttaylorr.com/\n\n--- >8 ---\n\nFrom: Ævar Arnfjörð Bjarmason \nSubject: [PATCH] multi-pack-index: refactor \"goto usage\" pattern\n\nRefactor the \"goto usage\" pattern added in\ncd57bc41bbc (builtin/multi-pack-index.c: display usage on unrecognized\ncommand, 2021-03-30) to maintain the same brevity, but doesn't run\nafoul of the recommendation in CodingGuidelines about braces:\n\n When there are multiple arms to a conditional and some of them\n require braces, enclose even a single line block in braces for\n consistency[...]\n\nLet's also change \"argv == 0\" to juts \"!argv\", per:\n\n Do not explicitly compare an integral value with constant 0 or\n '\\0', or a pointer value with constant NULL[...]\n\nI'm changing this because in a subsequent commit I'll make\nbuiltin/commit-graph.c use the same pattern, having the two similarly\nstructured commands match aids readability.\n\nWhile here, fix an issue since where cd57bc41bb\n(builtin/multi-pack-index.c: display usage on unrecognized command,\n2021-03-30) we sometimes jump to 'usage' without any arguments.\n\nMany compilers will save us from a segfault here, but the end result is\nugly, since it mentions an unrecognized subcommand when we didn't even\npass one, and (on GCC) includes \"(null)\" in its output.\n\nMove the \"usage\" label down past the error about unrecognized\nsubcommands so that it is only triggered when it should be. While we're\nat it, bulk up our test coverage in this area, too.\n\nCo-authored-by: Taylor Blau \nSigned-off-by: Taylor Blau \n---\n builtin/multi-pack-index.c | 11 +++++------\n t/t5319-multi-pack-index.sh | 6 ++++++\n 2 files changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex 5d3ea445fd..e3a684c842 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -164,7 +164,7 @@ int cmd_multi_pack_index(int argc, const char **argv,\n \tif (!opts.object_dir)\n \t\topts.object_dir = get_object_directory();\n\n-\tif (argc == 0)\n+\tif (!argc)\n \t\tgoto usage;\n\n \tif (!strcmp(argv[0], \"repack\"))\n@@ -175,10 +175,9 @@ int cmd_multi_pack_index(int argc, const char **argv,\n \t\treturn cmd_multi_pack_index_verify(argc, argv);\n \telse if (!strcmp(argv[0], \"expire\"))\n \t\treturn cmd_multi_pack_index_expire(argc, argv);\n-\telse {\n-usage:\n+\telse\n \t\terror(_(\"unrecognized subcommand: %s\"), argv[0]);\n-\t\tusage_with_options(builtin_multi_pack_index_usage,\n-\t\t\t\t builtin_multi_pack_index_options);\n-\t}\n+usage:\n+\tusage_with_options(builtin_multi_pack_index_usage,\n+\t\t\t builtin_multi_pack_index_options);\n }\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 5641d158df..fcde6dcded 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -824,4 +824,10 @@ test_expect_success 'load reverse index when missing .idx, .pack' '\n \t)\n '\n\n+\n+test_expect_success 'usage shown without sub-command' '\n+\ttest_expect_code 129 git multi-pack-index 2>err &&\n+\t! test_i18ngrep \"unrecognized subcommand\" err\n+'\n+\n test_done\n--\n2.31.1.163.ga65ce7f831\n\n"},{"id":"430675","messageId":"YPcTSSUtEuG41knv@nand.local","threadId":"56122","inReplyTo":"patch-5.6-7acb4bd75ce-20210720T113707Z-avarab@gmail.com","subject":"Re: [PATCH v3 5/6] commit-graph: early exit to \"usage\" on !argc","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-20T18:17:45Z","receivedAt":"2021-07-20T18:20:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jul 20, 2021 at 01:39:44PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Rather than guarding all of the !argc with an additional \"if\" arm\n> let's do an early goto to \"usage\". This also makes it clear that\n> \"save_commit_buffer\" is not needed in this case.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/commit-graph.c | 13 +++++++------\n> 1 file changed, 7 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> index 6e49184439f..bf34aa43f22 100644\n> --- a/builtin/commit-graph.c\n> +++ b/builtin/commit-graph.c\n> @@ -331,16 +331,17 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n> \t\t\t builtin_commit_graph_options,\n> \t\t\t builtin_commit_graph_usage,\n> \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n> +\tif (!argc)\n> +\t\tgoto usage;\n>\n> \tsave_commit_buffer = 0;\n>\n> -\tif (argc > 0) {\n> -\t\tif (!strcmp(argv[0], \"verify\"))\n> -\t\t\treturn graph_verify(argc, argv);\n> -\t\tif (!strcmp(argv[0], \"write\"))\n> -\t\t\treturn graph_write(argc, argv);\n> -\t}\n> +\tif (!strcmp(argv[0], \"verify\"))\n> +\t\treturn graph_verify(argc, argv);\n> +\telse if (argc && !strcmp(argv[0], \"write\"))\n> +\t\treturn graph_write(argc, argv);\n\nNice, what you wrote here is definitely an improvement in readability.\nThis could potentially also have an else like I suggested in the\nmulti-pack-index patch earlier (later?) in the thread. Maybe something\nlike:\n\n else if (strcmp(argv[0], \"...\"))\n return graph_xyz(...);\n else\n error(_(\"unrecognized subcommand: %s\"), argv[0]);\n usage:\n usage_with_options(...);\n\nArguably that could be done in this patch, separately, or not at all\n;).\n\nThanks,\nTaylor\n"},{"id":"430677","messageId":"YPcU3LSpa/r5nFCP@nand.local","threadId":"56122","inReplyTo":"20210720175530.GA23408@szeder.dev","subject":"Re: [PATCH v3 6/6] commit-graph: show usage on \"commit-graph [write|verify] garbage\"","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-20T18:24:28Z","receivedAt":"2021-07-20T18:27:32Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jul 20, 2021 at 07:55:30PM +0200, SZEDER Gábor wrote:\n> On Tue, Jul 20, 2021 at 07:47:39PM +0200, SZEDER Gábor wrote:\n> > On Tue, Jul 20, 2021 at 01:39:45PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> > > Change the parse_options() invocation in the commit-graph code to make\n> > > sense. We're calling it twice, once for common options parsing, and\n> > > then for the sub-commands.\n> > >\n> > > But we never checked if we had something leftover in argc in \"write\"\n> > > or \"verify\", as a result we'd silently accept garbage in these\n> > > subcommands. Let's not do that.\n> > >\n> > > Signed-off-by: Ævar Arnfjörð Bjarmason \n> > > ---\n> > > builtin/commit-graph.c | 10 ++++++++--\n> > > t/t5318-commit-graph.sh | 5 +++++\n> > > 2 files changed, 13 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n> > > index bf34aa43f22..88cbcb5a64f 100644\n> > > --- a/builtin/commit-graph.c\n> > > +++ b/builtin/commit-graph.c\n> > > @@ -104,7 +104,10 @@ static int graph_verify(int argc, const char **argv)\n> > > \topts.progress = isatty(2);\n> > > \targc = parse_options(argc, argv, NULL,\n> > > \t\t\t options,\n> > > -\t\t\t builtin_commit_graph_verify_usage, 0);\n> > > +\t\t\t builtin_commit_graph_verify_usage,\n> > > +\t\t\t PARSE_OPT_KEEP_UNKNOWN);\n> > > +\tif (argc)\n> > > +\t\tusage_with_options(builtin_commit_graph_verify_usage, options);\n> >\n> > Checking 'argc' alone is sufficient to catch unsupported parameters.\n> >\n> > Using PARSE_OPT_KEEP_UNKNOWN is not only unnecessary, but arguably\n> > wrong here\n>\n> And it's wrong in 'multi-pack-index' and 'env--helper' as well.\n\nThanks for spotting. Obviously this one is new, but the one in the midx\nbuiltin is my fault; I'm not sure why it's there, because it's clearly\nwrong.\n\nSo we should definitely fix this instance via a reroll of this series,\nbut that still leaves the others up for grabs. Ævar, are those (the ones\nin the 'multi-pack-index' and 'env--helper' builtins) something that you\nwant to clean up while you're working in this area, or would you rather\nthat I take care of it?\n\nI don't mind either way, just want to make sure that we don't duplicate\neffort.\n\nThanks,\nTaylor\n"},{"id":"430703","messageId":"87lf60vfyn.fsf@evledraar.gmail.com","threadId":"56122","inReplyTo":"YPcU3LSpa/r5nFCP@nand.local","subject":"Re: [PATCH v3 6/6] commit-graph: show usage on \"commit-graph [write|verify] garbage\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-20T21:17:13Z","receivedAt":"2021-07-20T21:24:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jul 20 2021, Taylor Blau wrote:\n\n> On Tue, Jul 20, 2021 at 07:55:30PM +0200, SZEDER Gábor wrote:\n>> On Tue, Jul 20, 2021 at 07:47:39PM +0200, SZEDER Gábor wrote:\n>> > On Tue, Jul 20, 2021 at 01:39:45PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> > > Change the parse_options() invocation in the commit-graph code to make\n>> > > sense. We're calling it twice, once for common options parsing, and\n>> > > then for the sub-commands.\n>> > >\n>> > > But we never checked if we had something leftover in argc in \"write\"\n>> > > or \"verify\", as a result we'd silently accept garbage in these\n>> > > subcommands. Let's not do that.\n>> > >\n>> > > Signed-off-by: Ævar Arnfjörð Bjarmason \n>> > > ---\n>> > > builtin/commit-graph.c | 10 ++++++++--\n>> > > t/t5318-commit-graph.sh | 5 +++++\n>> > > 2 files changed, 13 insertions(+), 2 deletions(-)\n>> > >\n>> > > diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\n>> > > index bf34aa43f22..88cbcb5a64f 100644\n>> > > --- a/builtin/commit-graph.c\n>> > > +++ b/builtin/commit-graph.c\n>> > > @@ -104,7 +104,10 @@ static int graph_verify(int argc, const char **argv)\n>> > > \topts.progress = isatty(2);\n>> > > \targc = parse_options(argc, argv, NULL,\n>> > > \t\t\t options,\n>> > > -\t\t\t builtin_commit_graph_verify_usage, 0);\n>> > > +\t\t\t builtin_commit_graph_verify_usage,\n>> > > +\t\t\t PARSE_OPT_KEEP_UNKNOWN);\n>> > > +\tif (argc)\n>> > > +\t\tusage_with_options(builtin_commit_graph_verify_usage, options);\n>> >\n>> > Checking 'argc' alone is sufficient to catch unsupported parameters.\n>> >\n>> > Using PARSE_OPT_KEEP_UNKNOWN is not only unnecessary, but arguably\n>> > wrong here\n>>\n>> And it's wrong in 'multi-pack-index' and 'env--helper' as well.\n>\n> Thanks for spotting. Obviously this one is new, but the one in the midx\n> builtin is my fault; I'm not sure why it's there, because it's clearly\n> wrong.\n>\n> So we should definitely fix this instance via a reroll of this series,\n> but that still leaves the others up for grabs. Ævar, are those (the ones\n> in the 'multi-pack-index' and 'env--helper' builtins) something that you\n> want to clean up while you're working in this area, or would you rather\n> that I take care of it?\n>\n> I don't mind either way, just want to make sure that we don't duplicate\n> effort.\n\nI'm all for you picking it up :)\n\nIf you wanted to pick up these patches (or some of them) and\npartially/entirely replace this series I'd be happy with that too,\ni.e. if it makes conflicts etc. easier.\n\nI just re-submitted this now because it's been staring at me in my\n\"should re-roll at somep point\" list for a while...\n\nFWIW if you're poking at this area more generally we really could do\nwith some standardization around these built-in sub-commands:\n\n\n git built-in --here subcommand \n git built-in subcommand --or-here\n\nVarious commands support one or the other as some confusing amalgamation\nof either taking options to \"built-in\" and/or \"subcommand\". The sane\nthing to do is:\n\n git --git-options built-in --built-in-options subcommand --just-for-the-subcommand\n\nBut not everything does that, and some things that should be\n--git-options or --built-in-options we take as subcommand options,\ne.g. --object-format or whatever.\n\n"},{"id":"430707","messageId":"YPdEeBGi3RVrB/fu@nand.local","threadId":"56122","inReplyTo":"87lf60vfyn.fsf@evledraar.gmail.com","subject":"Re: [PATCH v3 6/6] commit-graph: show usage on \"commit-graph [write|verify] garbage\"","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-20T21:47:36Z","receivedAt":"2021-07-20T21:48:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jul 20, 2021 at 11:17:13PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> > So we should definitely fix this instance via a reroll of this series,\n> > but that still leaves the others up for grabs. Ævar, are those (the ones\n> > in the 'multi-pack-index' and 'env--helper' builtins) something that you\n> > want to clean up while you're working in this area, or would you rather\n> > that I take care of it?\n> >\n> > I don't mind either way, just want to make sure that we don't duplicate\n> > effort.\n>\n> I'm all for you picking it up :)\n>\n> If you wanted to pick up these patches (or some of them) and\n> partially/entirely replace this series I'd be happy with that too,\n> i.e. if it makes conflicts etc. easier.\n\nI think either is fine; there shouldn't be any conflicts in the\nmulti-pack-index code just eyeballing based on what you wrote.\n\nI started working on it (which I suppose can count for me volunteering\nto patch it up myself ;-)), but I wondered why we have env--helper at\nall. When you wrote it back in b4f207f339 (env--helper: new undocumented\nbuiltin wrapping git_env_*(), 2019-06-21), you said that it wasn't added\nas a test-tool because it had some uses outside of tests.\n\nBut I can't find any locations. We do use env--helper (via\ntest_bool_env, which you also introduced) in a couple of t/lib-*.sh\nfiles, but this would be far from the first test-tool that has been used\nin a test library.\n\nSo ISTM that this could be converted to a test-tool and removed from the\nlist of builtins. *And* we could do that without a deprecation warning,\nbecause it was never documented in the first place.\n\nCan you double check my thinking and/or let me know if there is a\ncompelling reason to keep it as a builtin?\n\n> I just re-submitted this now because it's been staring at me in my\n> \"should re-roll at somep point\" list for a while...\n>\n> FWIW if you're poking at this area more generally we really could do\n> with some standardization around these built-in sub-commands:\n>\n> git built-in --here subcommand\n> git built-in subcommand --or-here\n\nThat's probably a step too far for this loose end for me, so if you want\nto work on that please be my guest, but I probably don't have time for\nit in the near future.\n\nThanks,\nTaylor\n"},{"id":"430742","messageId":"87im14unfd.fsf@evledraar.gmail.com","threadId":"56122","inReplyTo":"YPdEeBGi3RVrB/fu@nand.local","subject":"Re: [PATCH v3 6/6] commit-graph: show usage on \"commit-graph [write|verify] garbage\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-21T07:26:38Z","receivedAt":"2021-07-21T07:38:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jul 20 2021, Taylor Blau wrote:\n\n> On Tue, Jul 20, 2021 at 11:17:13PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> > So we should definitely fix this instance via a reroll of this series,\n>> > but that still leaves the others up for grabs. Ævar, are those (the ones\n>> > in the 'multi-pack-index' and 'env--helper' builtins) something that you\n>> > want to clean up while you're working in this area, or would you rather\n>> > that I take care of it?\n>> >\n>> > I don't mind either way, just want to make sure that we don't duplicate\n>> > effort.\n>>\n>> I'm all for you picking it up :)\n>>\n>> If you wanted to pick up these patches (or some of them) and\n>> partially/entirely replace this series I'd be happy with that too,\n>> i.e. if it makes conflicts etc. easier.\n>\n> I think either is fine; there shouldn't be any conflicts in the\n> multi-pack-index code just eyeballing based on what you wrote.\n>\n> I started working on it (which I suppose can count for me volunteering\n> to patch it up myself ;-)), but I wondered why we have env--helper at\n> all. When you wrote it back in b4f207f339 (env--helper: new undocumented\n> builtin wrapping git_env_*(), 2019-06-21), you said that it wasn't added\n> as a test-tool because it had some uses outside of tests.\n>\n> But I can't find any locations. We do use env--helper (via\n> test_bool_env, which you also introduced) in a couple of t/lib-*.sh\n\nFWIW test_bool_env is SZEDER'S, see 43a2afee82a (tests: add\n'test_bool_env' to catch non-bool GIT_TEST_* values, 2019-11-22).\n\n> files, but this would be far from the first test-tool that has been used\n> in a test library.\n>\n> So ISTM that this could be converted to a test-tool and removed from the\n> list of builtins. *And* we could do that without a deprecation warning,\n> because it was never documented in the first place.\n>\n> Can you double check my thinking and/or let me know if there is a\n> compelling reason to keep it as a builtin?\n\nUnlike the test-tools it was used by installed git, namely\ngit-sh-i18n.sh before d162b25f956 (tests: remove support for\nGIT_TEST_GETTEXT_POISON, 2021-01-20), but yes, now it could just be\nmigrated to a test-tool.\n\nI suppose it never really *needed* to be a built-in, i.e. the\ngit-sh-i18n.sh code could have just died without the helper if\nGIT_TEST_GETTEXT_POISON was set, anyway...\n\n...yes, nowadays it can simply be moved to t/helper.\n\n\n\nI do think in general this recent proliferation of t/helper over new\nplumbing built-ins has sent git a bit in the wrong direction.\n\nE.g. I think the likes of t/helper/test-pkt-line.c should really be a\nplumbing tool, the same goes for many (but not all) the test tool, we\ncould just document them as being \"unstable plumbing\" or whatever.\n\nBut I think I've been losing that argument recently, e.g. after [1]\n(which I argued we should put into git-ls-files) even things like git's\nbasic idea of the state of the index are exposed in some helpers, but\nnot corresponding plumbing.\n\nAnyway, even if we assume that's an argument that would carry the day in\ngeneral I'd find it hard to justify git-env--helper being a thing that\nshould be exposed to users or post-\"make install\", it's purely for the\nuse of our own test suite.\n\n\n\n\nSo yeah, it can just be moved to t/helper if you want to do that.\n\n>> I just re-submitted this now because it's been staring at me in my\n>> \"should re-roll at somep point\" list for a while...\n>>\n>> FWIW if you're poking at this area more generally we really could do\n>> with some standardization around these built-in sub-commands:\n>>\n>> git built-in --here subcommand\n>> git built-in subcommand --or-here\n>\n> That's probably a step too far for this loose end for me, so if you want\n> to work on that please be my guest, but I probably don't have time for\n> it in the near future.\n\n*nod*, but just to be clear, were you going to pick up &\nre-roll/re-submit the patches I have in this series? That would be good\nor me, but I don't mind either way, just wondering what state this\nshould be on my TODO list.\n\n1. https://lore.kernel.org/git/a1b8135c0fc890b2c2c0271c923b2f8efa8d1465.1617109864.git.gitgitgadget@gmail.com/\n"},{"id":"430743","messageId":"YPfWA4uA6OTKeOd9@coredump.intra.peff.net","threadId":"56122","inReplyTo":"87im14unfd.fsf@evledraar.gmail.com","subject":"Re: [PATCH v3 6/6] commit-graph: show usage on \"commit-graph [write|verify] garbage\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-21T08:08:35Z","receivedAt":"2021-07-21T08:09:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 21, 2021 at 09:26:38AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> \n> \n> I do think in general this recent proliferation of t/helper over new\n> plumbing built-ins has sent git a bit in the wrong direction.\n> \n> E.g. I think the likes of t/helper/test-pkt-line.c should really be a\n> plumbing tool, the same goes for many (but not all) the test tool, we\n> could just document them as being \"unstable plumbing\" or whatever.\n\nFWIW, I agree with you here. These kind of \"inspection\" tools are handy\nwhen you are debugging something. Building a copy of test-tool on a\nproduction system is only a mild inconvenience for me, but not being\nable to ask a user things like \"what does git pack-bitmap --dump\n.git/objects/pack*.bitmap say\" is occasionally quite annoying.\n\nThe flip side is that we expect the overall quality of user-visible\ntools to be a bit higher, and we're generally on the hook to keep\nsupporting them. Maybe that's solvable with documentation. I dunno.\n\n> But I think I've been losing that argument recently, e.g. after [1]\n> (which I argued we should put into git-ls-files) even things like git's\n> basic idea of the state of the index are exposed in some helpers, but\n> not corresponding plumbing.\n\nYeah. I wish \"ls-files --debug\" showed more of the extension data, for\nexample.\n\n> Anyway, even if we assume that's an argument that would carry the day in\n> general I'd find it hard to justify git-env--helper being a thing that\n> should be exposed to users or post-\"make install\", it's purely for the\n> use of our own test suite.\n\nYeah, I'd agree with that. The most valuable helpers to me are the ones\nthat help us understand what Git is seeing, or what's in a binary file\nformat. Obscure-case \"functional\" helpers are less likely to be\ngenerally useful.\n\n-Peff\n"},{"id":"430775","messageId":"xmqqmtqfehf7.fsf@gitster.g","threadId":"56122","inReplyTo":"YPfWA4uA6OTKeOd9@coredump.intra.peff.net","subject":"Re: [PATCH v3 6/6] commit-graph: show usage on \"commit-graph [write|verify] garbage\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-21T16:54:36Z","receivedAt":"2021-07-21T16:54:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> But I think I've been losing that argument recently, e.g. after [1]\n>> (which I argued we should put into git-ls-files) even things like git's\n>> basic idea of the state of the index are exposed in some helpers, but\n>> not corresponding plumbing.\n>\n> Yeah. I wish \"ls-files --debug\" showed more of the extension data, for\n> example.\n\nLet me second that ;-) With extensions and even more drastic things\nlike sparse index entries, \"pretend that the index is a flat list of\n\" is sometimes not useful in debugging (as bugs\nand design mistakes might lie in the code that makes us pretend).\n\nEven with packed objects, we still \"pretetend that an object file is\na single line ' SP NUL' followed by payload bytes,\ndeflated\", as if packed objects do not exist. I do not offhand\nrecall we have a good debugging option in the plumbing, or a\ndedicated debugging tool, but because the format for packed objects\nhas been stable, we are probably OK. Contrast to that, the index is\ndesigned to be more ephemeral and its format is subject to change,\nso we may in more need for a good debugging option.\n\n> Yeah, I'd agree with that. The most valuable helpers to me are the ones\n> that help us understand what Git is seeing, or what's in a binary file\n> format. Obscure-case \"functional\" helpers are less likely to be\n> generally useful.\n\nYup\n"},{"id":"433426","messageId":"cover-v4-0.7-00000000000-20210823T122854Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-0.6-00000000000-20210720T113707Z-avarab@gmail.com","subject":"[PATCH v4 0/7] commit-graph: usage fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-23T12:30:14Z","receivedAt":"2021-08-23T12:30:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A re-roll of v3 which should fix all outstanding issues &\nfeedback. Thanks SZEDER & Taylor:\nhttps://lore.kernel.org/git/cover-0.6-00000000000-20210720T113707Z-avarab@gmail.com/\n\nI dropped this myself because per\nhttps://lore.kernel.org/git/87im14unfd.fsf@evledraar.gmail.com/ I was\nunder the impression that Taylor was intending to pick up these\npatches as part of some more generale usage() fixes of his (which also\ntouched multi-pack-index.c), but his recent changes to fix\nmulti-pack-index.c didn't pick this up, so here it is as a re-roll instead.\n\nÆvar Arnfjörð Bjarmason (7):\n commit-graph: define common usage with a macro\n commit-graph: remove redundant handling of -h\n commit-graph: use parse_options_concat()\n multi-pack-index: refactor \"goto usage\" pattern\n commit-graph: early exit to \"usage\" on !argc\n commit-graph: show usage on \"commit-graph [write|verify] garbage\"\n commit-graph: show \"unexpected subcommand\" error\n\n builtin/commit-graph.c | 90 +++++++++++++++++++++-----------------\n builtin/multi-pack-index.c | 11 +++--\n t/t5318-commit-graph.sh | 19 ++++++++\n 3 files changed, 74 insertions(+), 46 deletions(-)\n\nRange-diff against v3:\n1: 1b9b4703ce2 = 1: ef37a8243c8 commit-graph: define common usage with a macro\n2: 8f50750ae5e = 2: bbed18ff193 commit-graph: remove redundant handling of -h\n3: f02da994363 = 3: 32cc0d1c7bc commit-graph: use parse_options_concat()\n4: 6e9bd877866 ! 4: 087f98bbec6 multi-pack-index: refactor \"goto usage\" pattern\n @@ Commit message\n \n Refactor the \"goto usage\" pattern added in\n cd57bc41bbc (builtin/multi-pack-index.c: display usage on unrecognized\n - command, 2021-03-30) to maintain the same brevity, but doesn't run\n - afoul of the recommendation in CodingGuidelines about braces:\n + command, 2021-03-30) and 88617d11f9d (multi-pack-index: fix potential\n + segfault without sub-command, 2021-07-19) to maintain the same\n + brevity, but in a form that doesn't run afoul of the recommendation in\n + CodingGuidelines about braces:\n \n When there are multiple arms to a conditional and some of them\n require braces, enclose even a single line block in braces for\n @@ builtin/multi-pack-index.c: int cmd_multi_pack_index(int argc, const char **argv\n \telse if (!strcmp(argv[0], \"expire\"))\n \t\treturn cmd_multi_pack_index_expire(argc, argv);\n -\telse {\n +-\t\terror(_(\"unrecognized subcommand: %s\"), argv[0]);\n +\n ++\terror(_(\"unrecognized subcommand: %s\"), argv[0]);\n usage:\n --\t\terror(_(\"unrecognized subcommand: %s\"), argv[0]);\n -\t\tusage_with_options(builtin_multi_pack_index_usage,\n -\t\t\t\t builtin_multi_pack_index_options);\n -\t}\n -+\terror(_(\"unrecognized subcommand: %s\"), argv[0]);\n +\tusage_with_options(builtin_multi_pack_index_usage,\n +\t\t\t builtin_multi_pack_index_options);\n }\n5: 7acb4bd75ce = 5: 2983e16ba69 commit-graph: early exit to \"usage\" on !argc\n6: 5c1694e071e ! 6: 85552a6f88c commit-graph: show usage on \"commit-graph [write|verify] garbage\"\n @@ Metadata\n ## Commit message ##\n commit-graph: show usage on \"commit-graph [write|verify] garbage\"\n \n - Change the parse_options() invocation in the commit-graph code to make\n - sense. We're calling it twice, once for common options parsing, and\n - then for the sub-commands.\n + Change the parse_options() invocation in the commit-graph code to\n + error on unknown leftover argv elements, in addition to the existing\n + and implicit erroring via parse_options() on unknown options.\n \n - But we never checked if we had something leftover in argc in \"write\"\n - or \"verify\", as a result we'd silently accept garbage in these\n - subcommands. Let's not do that.\n + We'd already error in cmd_commit_graph() on e.g.:\n +\n + git commit-graph unknown verify\n + git commit-graph --unknown verify\n +\n + But here we're calling parse_options() twice more for the \"write\" and\n + \"verify\" subcommands. We did not do the same checking for leftover\n + argv elements there. As a result we'd silently accept garbage in these\n + subcommands, let's not do that.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## builtin/commit-graph.c ##\n @@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)\n - \topts.progress = isatty(2);\n \targc = parse_options(argc, argv, NULL,\n \t\t\t options,\n --\t\t\t builtin_commit_graph_verify_usage, 0);\n -+\t\t\t builtin_commit_graph_verify_usage,\n -+\t\t\t PARSE_OPT_KEEP_UNKNOWN);\n + \t\t\t builtin_commit_graph_verify_usage, 0);\n +\tif (argc)\n +\t\tusage_with_options(builtin_commit_graph_verify_usage, options);\n \n \tif (!opts.obj_dir)\n \t\topts.obj_dir = get_object_directory();\n @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)\n - \n \targc = parse_options(argc, argv, NULL,\n \t\t\t options,\n --\t\t\t builtin_commit_graph_write_usage, 0);\n -+\t\t\t builtin_commit_graph_write_usage,\n -+\t\t\t PARSE_OPT_KEEP_UNKNOWN);\n + \t\t\t builtin_commit_graph_write_usage, 0);\n +\tif (argc)\n +\t\tusage_with_options(builtin_commit_graph_write_usage, options);\n \n-: ----------- > 7: 962521cfa17 commit-graph: show \"unexpected subcommand\" error\n-- \n2.33.0.662.gbc81f8cbdca\n\n"},{"id":"433427","messageId":"patch-v4-1.7-ef37a8243c8-20210823T122854Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-v4-0.7-00000000000-20210823T122854Z-avarab@gmail.com","subject":"[PATCH v4 1/7] commit-graph: define common usage with a macro","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-23T12:30:15Z","receivedAt":"2021-08-23T12:30:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Share the usage message between these three variables by using a\nmacro. Before this new options needed to copy/paste the usage\ninformation, see e.g. 809e0327f5 (builtin/commit-graph.c: introduce\n'--max-new-filters=', 2020-09-18).\n\nSee b25b727494f (builtin/multi-pack-index.c: define common usage with\na macro, 2021-03-30) for another use of this pattern (but on-list this\none came first).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 31 +++++++++++++++++--------------\n 1 file changed, 17 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex cd863152216..5af3cd7178f 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -9,26 +9,29 @@\n #include \"progress.h\"\n #include \"tag.h\"\n \n-static char const * const builtin_commit_graph_usage[] = {\n-\tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n-\tN_(\"git commit-graph write [--object-dir ] [--append] \"\n-\t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \"\n-\t \"[--changed-paths] [--[no-]max-new-filters ] [--[no-]progress] \"\n-\t \"\"),\n+#define BUILTIN_COMMIT_GRAPH_VERIFY_USAGE \\\n+\tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\")\n+\n+#define BUILTIN_COMMIT_GRAPH_WRITE_USAGE \\\n+\tN_(\"git commit-graph write [--object-dir ] [--append] \" \\\n+\t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \" \\\n+\t \"[--changed-paths] [--[no-]max-new-filters ] [--[no-]progress] \" \\\n+\t \"\")\n+\n+static const char * builtin_commit_graph_verify_usage[] = {\n+\tBUILTIN_COMMIT_GRAPH_VERIFY_USAGE,\n \tNULL\n };\n \n-static const char * const builtin_commit_graph_verify_usage[] = {\n-\tN_(\"git commit-graph verify [--object-dir ] [--shallow] [--[no-]progress]\"),\n+static const char * builtin_commit_graph_write_usage[] = {\n+\tBUILTIN_COMMIT_GRAPH_WRITE_USAGE,\n \tNULL\n };\n \n-static const char * const builtin_commit_graph_write_usage[] = {\n-\tN_(\"git commit-graph write [--object-dir ] [--append] \"\n-\t \"[--split[=]] [--reachable|--stdin-packs|--stdin-commits] \"\n-\t \"[--changed-paths] [--[no-]max-new-filters ] [--[no-]progress] \"\n-\t \"\"),\n-\tNULL\n+static char const * const builtin_commit_graph_usage[] = {\n+\tBUILTIN_COMMIT_GRAPH_VERIFY_USAGE,\n+\tBUILTIN_COMMIT_GRAPH_WRITE_USAGE,\n+\tNULL,\n };\n \n static struct opts_commit_graph {\n-- \n2.33.0.662.gbc81f8cbdca\n\n"},{"id":"433428","messageId":"patch-v4-2.7-bbed18ff193-20210823T122854Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-v4-0.7-00000000000-20210823T122854Z-avarab@gmail.com","subject":"[PATCH v4 2/7] commit-graph: remove redundant handling of -h","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-23T12:30:16Z","receivedAt":"2021-08-23T12:30:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"If we don't handle the -h option here like most parse_options() users\nwe'll fall through and it'll do the right thing for us.\n\nI think this code added in 4ce58ee38d (commit-graph: create\ngit-commit-graph builtin, 2018-04-02) was always redundant,\nparse_options() did this at the time, and the commit-graph code never\nused PARSE_OPT_NO_INTERNAL_HELP.\n\nWe don't need a test for this, it's tested by the t0012-help.sh test\nadded in d691551192a (t0012: test \"-h\" with builtins, 2017-05-30).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 5af3cd7178f..3cf18dc5345 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -321,10 +321,6 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n \t\tOPT_END(),\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_commit_graph_usage,\n-\t\t\t\t builtin_commit_graph_options);\n-\n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_commit_graph_options,\n-- \n2.33.0.662.gbc81f8cbdca\n\n"},{"id":"433429","messageId":"patch-v4-3.7-32cc0d1c7bc-20210823T122854Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-v4-0.7-00000000000-20210823T122854Z-avarab@gmail.com","subject":"[PATCH v4 3/7] commit-graph: use parse_options_concat()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-23T12:30:17Z","receivedAt":"2021-08-23T12:30:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Make use of the parse_options_concat() so we don't need to copy/paste\ncommon options like --object-dir.\n\nThis is inspired by a similar change to \"checkout\" in 2087182272\n(checkout: split options[] array in three pieces, 2019-03-29), and the\nsame pattern in the multi-pack-index command, see\n60ca94769ce (builtin/multi-pack-index.c: split sub-commands,\n2021-03-30).\n\nA minor behavior change here is that now we're going to list both\n--object-dir and --progress first, before we'd list --progress along\nwith other options.\n\nCo-authored-by: Taylor Blau \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 37 +++++++++++++++++++++----------------\n 1 file changed, 21 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 3cf18dc5345..6e49184439f 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -46,6 +46,20 @@ static struct opts_commit_graph {\n \tint enable_changed_paths;\n } opts;\n \n+static struct option common_opts[] = {\n+\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n+\t\t N_(\"dir\"),\n+\t\t N_(\"the object directory to store the graph\")),\n+\tOPT_BOOL(0, \"progress\", &opts.progress,\n+\t\t N_(\"force progress reporting\")),\n+\tOPT_END()\n+};\n+\n+static struct option *add_common_options(struct option *to)\n+{\n+\treturn parse_options_concat(common_opts, to);\n+}\n+\n static struct object_directory *find_odb(struct repository *r,\n \t\t\t\t\t const char *obj_dir)\n {\n@@ -79,20 +93,17 @@ static int graph_verify(int argc, const char **argv)\n \tint flags = 0;\n \n \tstatic struct option builtin_commit_graph_verify_options[] = {\n-\t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n-\t\t\t N_(\"dir\"),\n-\t\t\t N_(\"the object directory to store the graph\")),\n \t\tOPT_BOOL(0, \"shallow\", &opts.shallow,\n \t\t\t N_(\"if the commit-graph is split, only verify the tip file\")),\n-\t\tOPT_BOOL(0, \"progress\", &opts.progress, N_(\"force progress reporting\")),\n \t\tOPT_END(),\n \t};\n+\tstruct option *options = add_common_options(builtin_commit_graph_verify_options);\n \n \ttrace2_cmd_mode(\"verify\");\n \n \topts.progress = isatty(2);\n \targc = parse_options(argc, argv, NULL,\n-\t\t\t builtin_commit_graph_verify_options,\n+\t\t\t options,\n \t\t\t builtin_commit_graph_verify_usage, 0);\n \n \tif (!opts.obj_dir)\n@@ -109,6 +120,7 @@ static int graph_verify(int argc, const char **argv)\n \t\tdie_errno(_(\"Could not open commit-graph '%s'\"), graph_name);\n \n \tFREE_AND_NULL(graph_name);\n+\tFREE_AND_NULL(options);\n \n \tif (open_ok)\n \t\tgraph = load_commit_graph_one_fd_st(the_repository, fd, &st, odb);\n@@ -209,9 +221,6 @@ static int graph_write(int argc, const char **argv)\n \tstruct progress *progress = NULL;\n \n \tstatic struct option builtin_commit_graph_write_options[] = {\n-\t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n-\t\t\tN_(\"dir\"),\n-\t\t\tN_(\"the object directory to store the graph\")),\n \t\tOPT_BOOL(0, \"reachable\", &opts.reachable,\n \t\t\tN_(\"start walk at all refs\")),\n \t\tOPT_BOOL(0, \"stdin-packs\", &opts.stdin_packs,\n@@ -222,7 +231,6 @@ static int graph_write(int argc, const char **argv)\n \t\t\tN_(\"include all commits already in the commit-graph file\")),\n \t\tOPT_BOOL(0, \"changed-paths\", &opts.enable_changed_paths,\n \t\t\tN_(\"enable computation for changed paths\")),\n-\t\tOPT_BOOL(0, \"progress\", &opts.progress, N_(\"force progress reporting\")),\n \t\tOPT_CALLBACK_F(0, \"split\", &write_opts.split_flags, NULL,\n \t\t\tN_(\"allow writing an incremental commit-graph file\"),\n \t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n@@ -238,6 +246,7 @@ static int graph_write(int argc, const char **argv)\n \t\t\t0, write_option_max_new_filters),\n \t\tOPT_END(),\n \t};\n+\tstruct option *options = add_common_options(builtin_commit_graph_write_options);\n \n \topts.progress = isatty(2);\n \topts.enable_changed_paths = -1;\n@@ -251,7 +260,7 @@ static int graph_write(int argc, const char **argv)\n \tgit_config(git_commit_graph_write_config, &opts);\n \n \targc = parse_options(argc, argv, NULL,\n-\t\t\t builtin_commit_graph_write_options,\n+\t\t\t options,\n \t\t\t builtin_commit_graph_write_usage, 0);\n \n \tif (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1)\n@@ -307,6 +316,7 @@ static int graph_write(int argc, const char **argv)\n \t\tresult = 1;\n \n cleanup:\n+\tFREE_AND_NULL(options);\n \tstring_list_clear(&pack_indexes, 0);\n \tstrbuf_release(&buf);\n \treturn result;\n@@ -314,12 +324,7 @@ static int graph_write(int argc, const char **argv)\n \n int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n {\n-\tstatic struct option builtin_commit_graph_options[] = {\n-\t\tOPT_STRING(0, \"object-dir\", &opts.obj_dir,\n-\t\t\tN_(\"dir\"),\n-\t\t\tN_(\"the object directory to store the graph\")),\n-\t\tOPT_END(),\n-\t};\n+\tstruct option *builtin_commit_graph_options = common_opts;\n \n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix,\n-- \n2.33.0.662.gbc81f8cbdca\n\n"},{"id":"433430","messageId":"patch-v4-4.7-087f98bbec6-20210823T122854Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-v4-0.7-00000000000-20210823T122854Z-avarab@gmail.com","subject":"[PATCH v4 4/7] multi-pack-index: refactor \"goto usage\" pattern","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-23T12:30:18Z","receivedAt":"2021-08-23T12:30:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Refactor the \"goto usage\" pattern added in\ncd57bc41bbc (builtin/multi-pack-index.c: display usage on unrecognized\ncommand, 2021-03-30) and 88617d11f9d (multi-pack-index: fix potential\nsegfault without sub-command, 2021-07-19) to maintain the same\nbrevity, but in a form that doesn't run afoul of the recommendation in\nCodingGuidelines about braces:\n\n When there are multiple arms to a conditional and some of them\n require braces, enclose even a single line block in braces for\n consistency[...]\n\nLet's also change \"argv == 0\" to juts \"!argv\", per:\n\n Do not explicitly compare an integral value with constant 0 or\n '\\0', or a pointer value with constant NULL[...]\n\nI'm changing this because in a subsequent commit I'll make\nbuiltin/commit-graph.c use the same pattern, having the two similarly\nstructured commands match aids readability.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/multi-pack-index.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c\nindex 8ff0dee2ecb..649aa5f9ab2 100644\n--- a/builtin/multi-pack-index.c\n+++ b/builtin/multi-pack-index.c\n@@ -164,7 +164,7 @@ int cmd_multi_pack_index(int argc, const char **argv,\n \tif (!opts.object_dir)\n \t\topts.object_dir = get_object_directory();\n \n-\tif (argc == 0)\n+\tif (!argc)\n \t\tgoto usage;\n \n \tif (!strcmp(argv[0], \"repack\"))\n@@ -175,10 +175,9 @@ int cmd_multi_pack_index(int argc, const char **argv,\n \t\treturn cmd_multi_pack_index_verify(argc, argv);\n \telse if (!strcmp(argv[0], \"expire\"))\n \t\treturn cmd_multi_pack_index_expire(argc, argv);\n-\telse {\n-\t\terror(_(\"unrecognized subcommand: %s\"), argv[0]);\n+\n+\terror(_(\"unrecognized subcommand: %s\"), argv[0]);\n usage:\n-\t\tusage_with_options(builtin_multi_pack_index_usage,\n-\t\t\t\t builtin_multi_pack_index_options);\n-\t}\n+\tusage_with_options(builtin_multi_pack_index_usage,\n+\t\t\t builtin_multi_pack_index_options);\n }\n-- \n2.33.0.662.gbc81f8cbdca\n\n"},{"id":"433431","messageId":"patch-v4-5.7-2983e16ba69-20210823T122854Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-v4-0.7-00000000000-20210823T122854Z-avarab@gmail.com","subject":"[PATCH v4 5/7] commit-graph: early exit to \"usage\" on !argc","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-23T12:30:19Z","receivedAt":"2021-08-23T12:30:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Rather than guarding all of the !argc with an additional \"if\" arm\nlet's do an early goto to \"usage\". This also makes it clear that\n\"save_commit_buffer\" is not needed in this case.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 6e49184439f..bf34aa43f22 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -331,16 +331,17 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n \t\t\t builtin_commit_graph_options,\n \t\t\t builtin_commit_graph_usage,\n \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n+\tif (!argc)\n+\t\tgoto usage;\n \n \tsave_commit_buffer = 0;\n \n-\tif (argc > 0) {\n-\t\tif (!strcmp(argv[0], \"verify\"))\n-\t\t\treturn graph_verify(argc, argv);\n-\t\tif (!strcmp(argv[0], \"write\"))\n-\t\t\treturn graph_write(argc, argv);\n-\t}\n+\tif (!strcmp(argv[0], \"verify\"))\n+\t\treturn graph_verify(argc, argv);\n+\telse if (argc && !strcmp(argv[0], \"write\"))\n+\t\treturn graph_write(argc, argv);\n \n+usage:\n \tusage_with_options(builtin_commit_graph_usage,\n \t\t\t builtin_commit_graph_options);\n }\n-- \n2.33.0.662.gbc81f8cbdca\n\n"},{"id":"433432","messageId":"patch-v4-6.7-85552a6f88c-20210823T122854Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-v4-0.7-00000000000-20210823T122854Z-avarab@gmail.com","subject":"[PATCH v4 6/7] commit-graph: show usage on \"commit-graph [write|verify] garbage\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-23T12:30:20Z","receivedAt":"2021-08-23T12:30:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the parse_options() invocation in the commit-graph code to\nerror on unknown leftover argv elements, in addition to the existing\nand implicit erroring via parse_options() on unknown options.\n\nWe'd already error in cmd_commit_graph() on e.g.:\n\n git commit-graph unknown verify\n git commit-graph --unknown verify\n\nBut here we're calling parse_options() twice more for the \"write\" and\n\"verify\" subcommands. We did not do the same checking for leftover\nargv elements there. As a result we'd silently accept garbage in these\nsubcommands, let's not do that.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 4 ++++\n t/t5318-commit-graph.sh | 5 +++++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex bf34aa43f22..0457903f18f 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -105,6 +105,8 @@ static int graph_verify(int argc, const char **argv)\n \targc = parse_options(argc, argv, NULL,\n \t\t\t options,\n \t\t\t builtin_commit_graph_verify_usage, 0);\n+\tif (argc)\n+\t\tusage_with_options(builtin_commit_graph_verify_usage, options);\n \n \tif (!opts.obj_dir)\n \t\topts.obj_dir = get_object_directory();\n@@ -262,6 +264,8 @@ static int graph_write(int argc, const char **argv)\n \targc = parse_options(argc, argv, NULL,\n \t\t\t options,\n \t\t\t builtin_commit_graph_write_usage, 0);\n+\tif (argc)\n+\t\tusage_with_options(builtin_commit_graph_write_usage, options);\n \n \tif (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1)\n \t\tdie(_(\"use at most one of --reachable, --stdin-commits, or --stdin-packs\"));\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex af88f805aa2..09a2ccd2920 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -5,6 +5,11 @@ test_description='commit graph'\n \n GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0\n \n+test_expect_success 'usage' '\n+\ttest_expect_code 129 git commit-graph write blah &&\n+\ttest_expect_code 129 git commit-graph write verify\n+'\n+\n test_expect_success 'setup full repo' '\n \tmkdir full &&\n \tcd \"$TRASH_DIRECTORY/full\" &&\n-- \n2.33.0.662.gbc81f8cbdca\n\n"},{"id":"433433","messageId":"patch-v4-7.7-962521cfa17-20210823T122854Z-avarab@gmail.com","threadId":"56122","inReplyTo":"cover-v4-0.7-00000000000-20210823T122854Z-avarab@gmail.com","subject":"[PATCH v4 7/7] commit-graph: show \"unexpected subcommand\" error","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-23T12:30:21Z","receivedAt":"2021-08-23T12:30:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Bring the \"commit-graph\" command in line with the error output and\ngeneral pattern in cmd_multi_pack_index().\n\nLet's test for that output, and also cover the same potential bug as\nwas fixed in the multi-pack-index command in\n88617d11f9d (multi-pack-index: fix potential segfault without\nsub-command, 2021-07-19).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/commit-graph.c | 1 +\n t/t5318-commit-graph.sh | 16 +++++++++++++++-\n 2 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex 0457903f18f..21fc6e934b0 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -345,6 +345,7 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)\n \telse if (argc && !strcmp(argv[0], \"write\"))\n \t\treturn graph_write(argc, argv);\n \n+\terror(_(\"unrecognized subcommand: %s\"), argv[0]);\n usage:\n \tusage_with_options(builtin_commit_graph_usage,\n \t\t\t builtin_commit_graph_options);\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex 09a2ccd2920..295c5bd94d2 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -6,10 +6,24 @@ test_description='commit graph'\n GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0\n \n test_expect_success 'usage' '\n-\ttest_expect_code 129 git commit-graph write blah &&\n+\ttest_expect_code 129 git commit-graph write blah 2>err &&\n \ttest_expect_code 129 git commit-graph write verify\n '\n \n+test_expect_success 'usage shown without sub-command' '\n+\ttest_expect_code 129 git commit-graph 2>err &&\n+\t! grep error: err\n+'\n+\n+test_expect_success 'usage shown with an error on unknown sub-command' '\n+\tcat >expect <<-\\EOF &&\n+\terror: unrecognized subcommand: unknown\n+\tEOF\n+\ttest_expect_code 129 git commit-graph unknown 2>stderr &&\n+\tgrep error stderr >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'setup full repo' '\n \tmkdir full &&\n \tcd \"$TRASH_DIRECTORY/full\" &&\n-- \n2.33.0.662.gbc81f8cbdca\n\n"},{"id":"434206","messageId":"YS1vz16wOyj2GPHP@nand.local","threadId":"56122","inReplyTo":"cover-v4-0.7-00000000000-20210823T122854Z-avarab@gmail.com","subject":"Re: [PATCH v4 0/7] commit-graph: usage fixes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-08-30T23:54:55Z","receivedAt":"2021-08-30T23:54:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 23, 2021 at 02:30:14PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> A re-roll of v3 which should fix all outstanding issues &\n> feedback. Thanks SZEDER & Taylor:\n> https://lore.kernel.org/git/cover-0.6-00000000000-20210720T113707Z-avarab@gmail.com/\n>\n> I dropped this myself because per\n> https://lore.kernel.org/git/87im14unfd.fsf@evledraar.gmail.com/ I was\n> under the impression that Taylor was intending to pick up these\n> patches as part of some more generale usage() fixes of his (which also\n> touched multi-pack-index.c), but his recent changes to fix\n> multi-pack-index.c didn't pick this up, so here it is as a re-roll instead.\n\nThanks, and sorry for totally dropping this on the floor even though I\nsaid that I would pick it up. Since it had been a while, I re-read\nthrough the patches and they all look good to me.\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"434207","messageId":"xmqqpmtuzdqy.fsf@gitster.g","threadId":"56122","inReplyTo":"YS1vz16wOyj2GPHP@nand.local","subject":"Re: [PATCH v4 0/7] commit-graph: usage fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-30T23:58:29Z","receivedAt":"2021-08-30T23:58:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Mon, Aug 23, 2021 at 02:30:14PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> A re-roll of v3 which should fix all outstanding issues &\n>> feedback. Thanks SZEDER & Taylor:\n>> https://lore.kernel.org/git/cover-0.6-00000000000-20210720T113707Z-avarab@gmail.com/\n>>\n>> I dropped this myself because per\n>> https://lore.kernel.org/git/87im14unfd.fsf@evledraar.gmail.com/ I was\n>> under the impression that Taylor was intending to pick up these\n>> patches as part of some more generale usage() fixes of his (which also\n>> touched multi-pack-index.c), but his recent changes to fix\n>> multi-pack-index.c didn't pick this up, so here it is as a re-roll instead.\n>\n> Thanks, and sorry for totally dropping this on the floor even though I\n> said that I would pick it up. Since it had been a while, I re-read\n> through the patches and they all look good to me.\n>\n> Reviewed-by: Taylor Blau \n\nThanks.\n\n"}]}