[PATCH v2 0/5] commit-graph: usage fixes

This set of trivial fixes to commit-graph usage was submitted
originally back in February as
https://lore.kernel.org/git/20210215184118.11306-1-avarab@gmail.com/

I omitted the In-Reply-To in the header because this was deep in an
unrelated thread.

In the meantime Taylor's similar changes to the midx code landed (the
original v1 was a "hey, here's how you can do this with
parse_options()" on my part to him).

I did some changes based on feedback on the v1, but didn't pick up all
the suggestions, it was mostly subjective, so let's see what people
think this time around.

Ævar Arnfjörð Bjarmason (5):
  commit-graph: define common usage with a macro
  commit-graph: remove redundant handling of -h
  commit-graph: use parse_options_concat()
  commit-graph: early exit to "usage" on !argc
  commit-graph: show usage on "commit-graph [write|verify] garbage"

 builtin/commit-graph.c  | 95 +++++++++++++++++++++++------------------
 t/t5318-commit-graph.sh |  7 +++
 2 files changed, 60 insertions(+), 42 deletions(-)

Range-diff against v1:
1:  742648756a5 ! 1:  0b0bb04ecf5 commit-graph: define common usage with a macro
    @@ Commit message
         information, see e.g. 809e0327f5 (builtin/commit-graph.c: introduce
         '--max-new-filters=<n>', 2020-09-18).
     
    +    See b25b727494f (builtin/multi-pack-index.c: define common usage with
    +    a macro, 2021-03-30) for another use of this pattern (but on-list this
    +    one came first).
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
     
      ## builtin/commit-graph.c ##
2:  497b6cbc9a5 = 2:  6f386fc32c8 commit-graph: remove redundant handling of -h
3:  fd1deaa3c99 ! 3:  2e7d9b0b8e4 commit-graph: use parse_options_concat()
    @@ builtin/commit-graph.c: static struct opts_commit_graph {
      					 const char *obj_dir)
      {
     @@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)
    - 	int fd;
    - 	struct stat st;
      	int flags = 0;
    --
    -+	struct option *options = NULL;
    + 
      	static struct option builtin_commit_graph_verify_options[] = {
     -		OPT_STRING(0, "object-dir", &opts.obj_dir,
     -			   N_("dir"),
    @@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)
     -		OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
      		OPT_END(),
      	};
    -+	options = parse_options_dup(builtin_commit_graph_verify_options);
    ++	struct option *options = parse_options_dup(builtin_commit_graph_verify_options);
     +	options = add_common_options(options);
      
      	trace2_cmd_mode("verify");
    @@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)
      
      	if (!opts.obj_dir)
     @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
    - 	int result = 0;
    - 	enum commit_graph_write_flags flags = 0;
      	struct progress *progress = NULL;
    --
    -+	struct option *options = NULL;
    + 
      	static struct option builtin_commit_graph_write_options[] = {
     -		OPT_STRING(0, "object-dir", &opts.obj_dir,
     -			N_("dir"),
    @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
      			0, write_option_max_new_filters),
      		OPT_END(),
      	};
    -+	options = parse_options_dup(builtin_commit_graph_write_options);
    ++	struct option *options = parse_options_dup(builtin_commit_graph_write_options);
     +	options = add_common_options(options);
      
      	opts.progress = isatty(2);
4:  3d4a1fb6680 ! 4:  d776424e8c8 commit-graph: refactor dispatch loop for style
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason [off-list ref]
     
      ## Commit message ##
    -    commit-graph: refactor dispatch loop for style
    +    commit-graph: early exit to "usage" on !argc
     
    -    I think it's more readable to have one if/elsif/else chain here than
    -    the code this replaces.
    +    Rather than guarding all of the !argc with an additional "if" arm
    +    let's do an early goto to "usage". This also makes it clear that
    +    "save_commit_buffer" is not needed in this case.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
     
      ## builtin/commit-graph.c ##
     @@ builtin/commit-graph.c: int cmd_commit_graph(int argc, const char **argv, const char *prefix)
    + 			     builtin_commit_graph_options,
    + 			     builtin_commit_graph_usage,
    + 			     PARSE_OPT_STOP_AT_NON_OPTION);
    ++	if (!argc)
    ++		goto usage;
      
      	save_commit_buffer = 0;
      
    @@ builtin/commit-graph.c: int cmd_commit_graph(int argc, const char **argv, const
     -		if (!strcmp(argv[0], "write"))
     -			return graph_write(argc, argv);
     -	}
    --
    --	usage_with_options(builtin_commit_graph_usage,
    --			   builtin_commit_graph_options);
    -+	if (argc && !strcmp(argv[0], "verify"))
    ++	if (!strcmp(argv[0], "verify"))
     +		return graph_verify(argc, argv);
     +	else if (argc && !strcmp(argv[0], "write"))
     +		return graph_write(argc, argv);
    -+	else
    -+		usage_with_options(builtin_commit_graph_usage,
    -+				   builtin_commit_graph_options);
    + 
    ++usage:
    + 	usage_with_options(builtin_commit_graph_usage,
    + 			   builtin_commit_graph_options);
      }
5:  e8481a0932a = 5:  57ffd5812d6 commit-graph: show usage on "commit-graph [write|verify] garbage"
-- 
2.32.0.873.g94a0c75983d

If we don't handle the -h option here like most parse_options() users
we'll fall through and it'll do the right thing for us.

I think this code added in 4ce58ee38d (commit-graph: create
git-commit-graph builtin, 2018-04-02) was always redundant,
parse_options() did this at the time, and the commit-graph code never
used PARSE_OPT_NO_INTERNAL_HELP.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c  | 4 ----
 t/t5318-commit-graph.sh | 5 +++++
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index c3fa4fde3e4..baead04a03b 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -319,10 +319,6 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_commit_graph_usage,
-				   builtin_commit_graph_options);
-
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix,
 			     builtin_commit_graph_options,

diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index af88f805aa2..5fccce95724 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh

@@ -5,6 +5,11 @@ test_description='commit graph'
 
 GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
 
+test_expect_success 'usage' '
+	test_expect_code 129 git commit-graph -h 2>err &&
+	! grep error: err
+'
+
 test_expect_success 'setup full repo' '
 	mkdir full &&
 	cd "$TRASH_DIRECTORY/full" &&

-- 
2.32.0.873.g94a0c75983d

Share the usage message between these three variables by using a
macro. Before this new options needed to copy/paste the usage
information, see e.g. 809e0327f5 (builtin/commit-graph.c: introduce
'--max-new-filters=<n>', 2020-09-18).

See b25b727494f (builtin/multi-pack-index.c: define common usage with
a macro, 2021-03-30) for another use of this pattern (but on-list this
one came first).

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index cd863152216..c3fa4fde3e4 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -9,26 +9,27 @@
 #include "progress.h"
 #include "tag.h"
 
-static char const * const builtin_commit_graph_usage[] = {
-	N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
-	N_("git commit-graph write [--object-dir <objdir>] [--append] "
-	   "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] "
-	   "[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress] "
-	   "<split options>"),
+static const char * builtin_commit_graph_verify_usage[] = {
+#define BUILTIN_COMMIT_GRAPH_VERIFY_USAGE \
+	N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]")
+	BUILTIN_COMMIT_GRAPH_VERIFY_USAGE,
 	NULL
 };
 
-static const char * const builtin_commit_graph_verify_usage[] = {
-	N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
+static const char * builtin_commit_graph_write_usage[] = {
+#define BUILTIN_COMMIT_GRAPH_WRITE_USAGE \
+	N_("git commit-graph write [--object-dir <objdir>] [--append] " \
+	   "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] " \
+	   "[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress] " \
+	   "<split options>")
+	BUILTIN_COMMIT_GRAPH_WRITE_USAGE,
 	NULL
 };
 
-static const char * const builtin_commit_graph_write_usage[] = {
-	N_("git commit-graph write [--object-dir <objdir>] [--append] "
-	   "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] "
-	   "[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress] "
-	   "<split options>"),
-	NULL
+static char const * const builtin_commit_graph_usage[] = {
+	BUILTIN_COMMIT_GRAPH_VERIFY_USAGE,
+	BUILTIN_COMMIT_GRAPH_WRITE_USAGE,
+	NULL,
 };
 
 static struct opts_commit_graph {

-- 
2.32.0.873.g94a0c75983d

Make use of the parse_options_concat() so we don't need to copy/paste
common options like --object-dir. This is inspired by a similar change
to "checkout" in 2087182272
(checkout: split options[] array in three pieces, 2019-03-29).

A minor behavior change here is that now we're going to list both
--object-dir and --progress first, before we'd list --progress along
with other options.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index baead04a03b..ff125adf2d5 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -44,6 +44,21 @@ static struct opts_commit_graph {
 	int enable_changed_paths;
 } opts;
 
+static struct option *add_common_options(struct option *prevopts)
+{
+	struct option options[] = {
+		OPT_STRING(0, "object-dir", &opts.obj_dir,
+			   N_("dir"),
+			   N_("the object directory to store the graph")),
+		OPT_BOOL(0, "progress", &opts.progress,
+			 N_("force progress reporting")),
+		OPT_END()
+	};
+	struct option *newopts = parse_options_concat(options, prevopts);
+	free(prevopts);
+	return newopts;
+}
+
 static struct object_directory *find_odb(struct repository *r,
 					 const char *obj_dir)
 {

@@ -77,20 +92,18 @@ static int graph_verify(int argc, const char **argv)
 	int flags = 0;
 
 	static struct option builtin_commit_graph_verify_options[] = {
-		OPT_STRING(0, "object-dir", &opts.obj_dir,
-			   N_("dir"),
-			   N_("the object directory to store the graph")),
 		OPT_BOOL(0, "shallow", &opts.shallow,
 			 N_("if the commit-graph is split, only verify the tip file")),
-		OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
 		OPT_END(),
 	};
+	struct option *options = parse_options_dup(builtin_commit_graph_verify_options);
+	options = add_common_options(options);
 
 	trace2_cmd_mode("verify");
 
 	opts.progress = isatty(2);
 	argc = parse_options(argc, argv, NULL,
-			     builtin_commit_graph_verify_options,
+			     options,
 			     builtin_commit_graph_verify_usage, 0);
 
 	if (!opts.obj_dir)

@@ -207,9 +220,6 @@ static int graph_write(int argc, const char **argv)
 	struct progress *progress = NULL;
 
 	static struct option builtin_commit_graph_write_options[] = {
-		OPT_STRING(0, "object-dir", &opts.obj_dir,
-			N_("dir"),
-			N_("the object directory to store the graph")),
 		OPT_BOOL(0, "reachable", &opts.reachable,
 			N_("start walk at all refs")),
 		OPT_BOOL(0, "stdin-packs", &opts.stdin_packs,

@@ -220,7 +230,6 @@ static int graph_write(int argc, const char **argv)
 			N_("include all commits already in the commit-graph file")),
 		OPT_BOOL(0, "changed-paths", &opts.enable_changed_paths,
 			N_("enable computation for changed paths")),
-		OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
 		OPT_CALLBACK_F(0, "split", &write_opts.split_flags, NULL,
 			N_("allow writing an incremental commit-graph file"),
 			PARSE_OPT_OPTARG | PARSE_OPT_NONEG,

@@ -236,6 +245,8 @@ static int graph_write(int argc, const char **argv)
 			0, write_option_max_new_filters),
 		OPT_END(),
 	};
+	struct option *options = parse_options_dup(builtin_commit_graph_write_options);
+	options = add_common_options(options);
 
 	opts.progress = isatty(2);
 	opts.enable_changed_paths = -1;

@@ -249,7 +260,7 @@ static int graph_write(int argc, const char **argv)
 	git_config(git_commit_graph_write_config, &opts);
 
 	argc = parse_options(argc, argv, NULL,
-			     builtin_commit_graph_write_options,
+			     options,
 			     builtin_commit_graph_write_usage, 0);
 
 	if (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1)

@@ -312,12 +323,8 @@ static int graph_write(int argc, const char **argv)
 
 int cmd_commit_graph(int argc, const char **argv, const char *prefix)
 {
-	static struct option builtin_commit_graph_options[] = {
-		OPT_STRING(0, "object-dir", &opts.obj_dir,
-			N_("dir"),
-			N_("the object directory to store the graph")),
-		OPT_END(),
-	};
+	struct option *no_options = parse_options_dup(NULL);
+	struct option *builtin_commit_graph_options = add_common_options(no_options);
 
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix,

-- 
2.32.0.873.g94a0c75983d

Rather than guarding all of the !argc with an additional "if" arm
let's do an early goto to "usage". This also makes it clear that
"save_commit_buffer" is not needed in this case.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index ff125adf2d5..16d2c517e72 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -331,16 +331,17 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)
 			     builtin_commit_graph_options,
 			     builtin_commit_graph_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
+	if (!argc)
+		goto usage;
 
 	save_commit_buffer = 0;
 
-	if (argc > 0) {
-		if (!strcmp(argv[0], "verify"))
-			return graph_verify(argc, argv);
-		if (!strcmp(argv[0], "write"))
-			return graph_write(argc, argv);
-	}
+	if (!strcmp(argv[0], "verify"))
+		return graph_verify(argc, argv);
+	else if (argc && !strcmp(argv[0], "write"))
+		return graph_write(argc, argv);
 
+usage:
 	usage_with_options(builtin_commit_graph_usage,
 			   builtin_commit_graph_options);
 }

-- 
2.32.0.873.g94a0c75983d

Change the parse_options() invocation in the commit-graph code to make
sense. We're calling it twice, once for common options parsing, and
then for the sub-commands.

But we never checked if we had something leftover in argc in "write"
or "verify", as a result we'd silently accept garbage in these
subcommands. Let's not do that.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c  | 10 ++++++++--
 t/t5318-commit-graph.sh |  4 +++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 16d2c517e72..bb3e767db33 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -104,7 +104,10 @@ static int graph_verify(int argc, const char **argv)
 	opts.progress = isatty(2);
 	argc = parse_options(argc, argv, NULL,
 			     options,
-			     builtin_commit_graph_verify_usage, 0);
+			     builtin_commit_graph_verify_usage,
+			     PARSE_OPT_KEEP_UNKNOWN);
+	if (argc)
+		usage_with_options(builtin_commit_graph_verify_usage, options);
 
 	if (!opts.obj_dir)
 		opts.obj_dir = get_object_directory();

@@ -261,7 +264,10 @@ static int graph_write(int argc, const char **argv)
 
 	argc = parse_options(argc, argv, NULL,
 			     options,
-			     builtin_commit_graph_write_usage, 0);
+			     builtin_commit_graph_write_usage,
+			     PARSE_OPT_KEEP_UNKNOWN);
+	if (argc)
+		usage_with_options(builtin_commit_graph_write_usage, options);
 
 	if (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1)
 		die(_("use at most one of --reachable, --stdin-commits, or --stdin-packs"));

diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 5fccce95724..5cf07a6dded 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh

@@ -7,7 +7,9 @@ GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
 
 test_expect_success 'usage' '
 	test_expect_code 129 git commit-graph -h 2>err &&
-	! grep error: err
+	! grep error: err &&
+	test_expect_code 129 git commit-graph write blah &&
+	test_expect_code 129 git commit-graph write verify
 '
 
 test_expect_success 'setup full repo' '

-- 
2.32.0.873.g94a0c75983d

On 18/07/2021 09:58, Ævar Arnfjörð Bjarmason wrote:

New test is partially redundant to the test in the loop at the bottom of
't0012-help.sh'.

+'
+
  test_expect_success 'setup full repo' '
  	mkdir full &&
  	cd "$TRASH_DIRECTORY/full" &&

On Sun, Jul 18, 2021 at 09:58:05AM +0200, Ævar Arnfjörð Bjarmason wrote:

Share the usage message between these three variables by using a
macro. Before this new options needed to copy/paste the usage
information, see e.g. 809e0327f5 (builtin/commit-graph.c: introduce
'--max-new-filters=<n>', 2020-09-18).

See b25b727494f (builtin/multi-pack-index.c: define common usage with
a macro, 2021-03-30) for another use of this pattern (but on-list this
one came first).

I don't have a strong opinion, but I believe Jonathan Tan suggested that
we move the #define's out-of-line with their variable declarations back
in [1].

I think that what you wrote here is fine, but I tend to agree with
Jonathan that the version we ended up with in the tree is cleaner and a
little easier to read (albeit a few more lines).

[1]: https://lore.kernel.org/git/20210302040625.4035284-1-jonathantanmy@google.com/

Thanks,
Taylor

On Sun, Jul 18, 2021 at 02:55:09PM +0200, Andrei Rybak wrote:

On 18/07/2021 09:58, Ævar Arnfjörð Bjarmason wrote:

quoted

I think this code added in 4ce58ee38d (commit-graph: create
git-commit-graph builtin, 2018-04-02) was always redundant,
parse_options() did this at the time, and the commit-graph code never
used PARSE_OPT_NO_INTERNAL_HELP.

Yep, that matches my findings.

quoted

diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index af88f805aa2..5fccce95724 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh

@@ -5,6 +5,11 @@ test_description='commit graph'
  GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
+test_expect_success 'usage' '
+	test_expect_code 129 git commit-graph -h 2>err &&
+	! grep error: err

New test is partially redundant to the test in the loop at the bottom of
't0012-help.sh'.

Agreed. The change looks good to me, but we should drop this redundant
test (although inspecting `git grep -w 'git .* -h' -- t` shows that
there are a handful of other ones that could probably be dropped, too).

I don't really feel strongly enough to worry about cleaning up the
existing ones, but we should strive to avoid adding new ones.

Thanks,
Taylor

On Sun, Jul 18, 2021 at 09:58:07AM +0200, Ævar Arnfjörð Bjarmason wrote:

Make use of the parse_options_concat() so we don't need to copy/paste
common options like --object-dir. This is inspired by a similar change
to "checkout" in 2087182272
(checkout: split options[] array in three pieces, 2019-03-29).

A minor behavior change here is that now we're going to list both
--object-dir and --progress first, before we'd list --progress along
with other options.

This is very reminiscent to the patch I sent to do the same in the
`multi-pack-index` builtin, probably because you were the person to
recommend I do that cleanup in the first place ;).

I got some good advice from Peff in [1] went I sent that patch, which
I'll try to summarize here, since I think a few pieces of it could be
applied to clean up this patch a little.

[1]: https://lore.kernel.org/git/YGG7tWBzo5NGl2+g@coredump.intra.peff.net/

Not from Peff's mail, but is there any reason to non statically allocate
this?

The only reason I could think of is that `opts` is heap allocated, but
it's not, so I think we could probably mark `options` as static here
(and perhaps rename it to `common_opts` while we're at it, if for no
other reason than to be consistent with what's in the multi-pack-index
builtin).

+	struct option *newopts = parse_options_concat(options, prevopts);
+	free(prevopts);

Since we're not concatenating more than one layer on top of the common
options (unlike in `checkout`), this can be simplified to just return
parse_options_concat without freeing prevopts.

We should be careful to make sure we free the return value from
parse_options_concat eventually, though.

Likewise down here (and in the other callers, too) this dup is pointless
if we're going to immediately free it after calling
parse_options_concat. So we can drop that, too.

Here's something to consider squashing in on top:

--- >8 ---

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index ff125adf2d..00b0721789 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -44,19 +44,18 @@ static struct opts_commit_graph {
 	int enable_changed_paths;
 } opts;

-static struct option *add_common_options(struct option *prevopts)
+static struct option common_opts[] = {
+	OPT_STRING(0, "object-dir", &opts.obj_dir,
+		   N_("dir"),
+		   N_("the object directory to store the graph")),
+	OPT_BOOL(0, "progress", &opts.progress,
+		 N_("force progress reporting")),
+	OPT_END()
+};
+
+static struct option *add_common_options(struct option *to)
 {
-	struct option options[] = {
-		OPT_STRING(0, "object-dir", &opts.obj_dir,
-			   N_("dir"),
-			   N_("the object directory to store the graph")),
-		OPT_BOOL(0, "progress", &opts.progress,
-			 N_("force progress reporting")),
-		OPT_END()
-	};
-	struct option *newopts = parse_options_concat(options, prevopts);
-	free(prevopts);
-	return newopts;
+	return parse_options_concat(common_opts, to);
 }

 static struct object_directory *find_odb(struct repository *r,

@@ -96,8 +95,7 @@ static int graph_verify(int argc, const char **argv)
 			 N_("if the commit-graph is split, only verify the tip file")),
 		OPT_END(),
 	};
-	struct option *options = parse_options_dup(builtin_commit_graph_verify_options);
-	options = add_common_options(options);
+	struct option *options = add_common_options(builtin_commit_graph_verify_options);

 	trace2_cmd_mode("verify");

@@ -120,6 +118,7 @@ static int graph_verify(int argc, const char **argv)
 		die_errno(_("Could not open commit-graph '%s'"), graph_name);

 	FREE_AND_NULL(graph_name);
+	FREE_AND_NULL(options);

 	if (open_ok)
 		graph = load_commit_graph_one_fd_st(the_repository, fd, &st, odb);

@@ -245,8 +244,7 @@ static int graph_write(int argc, const char **argv)
 			0, write_option_max_new_filters),
 		OPT_END(),
 	};
-	struct option *options = parse_options_dup(builtin_commit_graph_write_options);
-	options = add_common_options(options);
+	struct option *options = add_common_options(builtin_commit_graph_write_options);

 	opts.progress = isatty(2);
 	opts.enable_changed_paths = -1;

@@ -316,6 +314,7 @@ static int graph_write(int argc, const char **argv)
 		result = 1;

 cleanup:
+	FREE_AND_NULL(options);
 	string_list_clear(&pack_indexes, 0);
 	strbuf_release(&buf);
 	return result;

@@ -323,8 +322,7 @@ static int graph_write(int argc, const char **argv)

 int cmd_commit_graph(int argc, const char **argv, const char *prefix)
 {
-	struct option *no_options = parse_options_dup(NULL);
-	struct option *builtin_commit_graph_options = add_common_options(no_options);
+	struct option *builtin_commit_graph_options = common_opts;

 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix,

On Sun, Jul 18, 2021 at 09:58:09AM +0200, Ævar Arnfjörð Bjarmason wrote:

Change the parse_options() invocation in the commit-graph code to make
sense. We're calling it twice, once for common options parsing, and
then for the sub-commands.

But we never checked if we had something leftover in argc in "write"
or "verify", as a result we'd silently accept garbage in these
subcommands. Let's not do that.

All makes sense and looks good to me. One small note below:

 test_expect_success 'usage' '
 	test_expect_code 129 git commit-graph -h 2>err &&
-	! grep error: err
+	! grep error: err &&

Ah, now I see why you added this test back in the first patch. I still
think that we should get rid of the first two lines, but...

+	test_expect_code 129 git commit-graph write blah &&
+	test_expect_code 129 git commit-graph write verify

Keeping these makes sense.

Thanks,
Taylor

On Sun, Jul 18, 2021 at 09:58:08AM +0200, Ævar Arnfjörð Bjarmason wrote:

Unrelated to your patch here, but it may be nice to have an extra
error() for unrecognized sub-commands. I actually think that this
suggestion also comes from Peff originally, but it does make the "next
step" a lot clearer to users who otherwise would have seen a wall of
usage text.

Perhaps:

  else {
    error(_("unrecognized subcommand: %s"), argv[0]);
usage:
    usage_with_options(builtin_commit_graph_usage,
                       builtin_commit_graph_options);
  }

Thanks,
Taylor

On Mon, Jul 19 2021, Taylor Blau wrote:

On Sun, Jul 18, 2021 at 09:58:07AM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

Make use of the parse_options_concat() so we don't need to copy/paste
common options like --object-dir. This is inspired by a similar change
to "checkout" in 2087182272
(checkout: split options[] array in three pieces, 2019-03-29).

A minor behavior change here is that now we're going to list both
--object-dir and --progress first, before we'd list --progress along
with other options.

This is very reminiscent to the patch I sent to do the same in the
`multi-pack-index` builtin, probably because you were the person to
recommend I do that cleanup in the first place ;).

I got some good advice from Peff in [1] went I sent that patch, which
I'll try to summarize here, since I think a few pieces of it could be
applied to clean up this patch a little.

[1]: https://lore.kernel.org/git/YGG7tWBzo5NGl2+g@coredump.intra.peff.net/

Thanks.

quoted

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index baead04a03b..ff125adf2d5 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -44,6 +44,21 @@ static struct opts_commit_graph {
 	int enable_changed_paths;
 } opts;

+static struct option *add_common_options(struct option *prevopts)
+{
+	struct option options[] = {
+		OPT_STRING(0, "object-dir", &opts.obj_dir,
+			   N_("dir"),
+			   N_("the object directory to store the graph")),
+		OPT_BOOL(0, "progress", &opts.progress,
+			 N_("force progress reporting")),
+		OPT_END()
+	};

Not from Peff's mail, but is there any reason to non statically allocate
this?

The only reason I could think of is that `opts` is heap allocated, but
it's not, so I think we could probably mark `options` as static here
(and perhaps rename it to `common_opts` while we're at it, if for no
other reason than to be consistent with what's in the multi-pack-index
builtin).

I've applied your suggestions in full, thank a lot.

I'm a bit on the fence about this one, because it works here, but not in
other cases where the "options" refers to variables declared in the
function, i.e. &progress after an "int progress = 0" or whatever.

Making it all global seems like a bit of an anti-pattern, and having to
refactor it if we ever need to change it to rely on something dynamic in
the function is arguably bad.

And arguably not, because if we use this static pattern consistently
those cases will stick out more, whereas a lot of builtins now declare
them non-static in the function for no particular reason, and it's not
like we're harmed those commands hardcoding things at the top level.

But you won't find it trivial to e.g. migrate builtin/checkout.c to this
pattern, whereas using the non-static one is easy everywhere.

Anyway, I squashed this in, just say'n. Having written the above I'm
still not sure what I think about it :)

Fixes hopefully all the comments on v2[1], thanks Andrei and Taylor.

There's now a small mid-series digression before to also refactor the
"early exit to "usage" on !argc" for the multi-pack-index.

Taylor suggested using braces for the "else" so maybe he won't like it
:)

I figured having the two similar commands use the same pattern was
worth the digression.

1. http://lore.kernel.org/git/cover-0.5-00000000000-20210718T074936Z-avarab@gmail.com

Ævar Arnfjörð Bjarmason (6):
  commit-graph: define common usage with a macro
  commit-graph: remove redundant handling of -h
  commit-graph: use parse_options_concat()
  multi-pack-index: refactor "goto usage" pattern
  commit-graph: early exit to "usage" on !argc
  commit-graph: show usage on "commit-graph [write|verify] garbage"

 builtin/commit-graph.c     | 95 +++++++++++++++++++++-----------------
 builtin/multi-pack-index.c | 11 ++---
 t/t5318-commit-graph.sh    |  5 ++
 3 files changed, 63 insertions(+), 48 deletions(-)

Range-diff against v2:
1:  ee6630b7c0d ! 1:  1b9b4703ce2 commit-graph: define common usage with a macro
    @@ builtin/commit-graph.c
     -	   "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] "
     -	   "[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress] "
     -	   "<split options>"),
    -+static const char * builtin_commit_graph_verify_usage[] = {
     +#define BUILTIN_COMMIT_GRAPH_VERIFY_USAGE \
     +	N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]")
    ++
    ++#define BUILTIN_COMMIT_GRAPH_WRITE_USAGE \
    ++	N_("git commit-graph write [--object-dir <objdir>] [--append] " \
    ++	   "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] " \
    ++	   "[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress] " \
    ++	   "<split options>")
    ++
    ++static const char * builtin_commit_graph_verify_usage[] = {
     +	BUILTIN_COMMIT_GRAPH_VERIFY_USAGE,
      	NULL
      };
    @@ builtin/commit-graph.c
     -static const char * const builtin_commit_graph_verify_usage[] = {
     -	N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
     +static const char * builtin_commit_graph_write_usage[] = {
    -+#define BUILTIN_COMMIT_GRAPH_WRITE_USAGE \
    -+	N_("git commit-graph write [--object-dir <objdir>] [--append] " \
    -+	   "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] " \
    -+	   "[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress] " \
    -+	   "<split options>")
     +	BUILTIN_COMMIT_GRAPH_WRITE_USAGE,
      	NULL
      };
2:  03581d85781 ! 2:  8f50750ae5e commit-graph: remove redundant handling of -h
    @@ Commit message
         parse_options() did this at the time, and the commit-graph code never
         used PARSE_OPT_NO_INTERNAL_HELP.
     
    +    We don't need a test for this, it's tested by the t0012-help.sh test
    +    added in d691551192a (t0012: test "-h" with builtins, 2017-05-30).
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
     
      ## builtin/commit-graph.c ##
    @@ builtin/commit-graph.c: int cmd_commit_graph(int argc, const char **argv, const
      	git_config(git_default_config, NULL);
      	argc = parse_options(argc, argv, prefix,
      			     builtin_commit_graph_options,
    -
    - ## t/t5318-commit-graph.sh ##
    -@@ t/t5318-commit-graph.sh: test_description='commit graph'
    - 
    - GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
    - 
    -+test_expect_success 'usage' '
    -+	test_expect_code 129 git commit-graph -h 2>err &&
    -+	! grep error: err
    -+'
    -+
    - test_expect_success 'setup full repo' '
    - 	mkdir full &&
    - 	cd "$TRASH_DIRECTORY/full" &&
3:  8e909cd9c23 ! 3:  f02da994363 commit-graph: use parse_options_concat()
    @@ Commit message
         commit-graph: use parse_options_concat()
     
         Make use of the parse_options_concat() so we don't need to copy/paste
    -    common options like --object-dir. This is inspired by a similar change
    -    to "checkout" in 2087182272
    -    (checkout: split options[] array in three pieces, 2019-03-29).
    +    common options like --object-dir.
    +
    +    This is inspired by a similar change to "checkout" in 2087182272
    +    (checkout: split options[] array in three pieces, 2019-03-29), and the
    +    same pattern in the multi-pack-index command, see
    +    60ca94769ce (builtin/multi-pack-index.c: split sub-commands,
    +    2021-03-30).
     
         A minor behavior change here is that now we're going to list both
         --object-dir and --progress first, before we'd list --progress along
         with other options.
     
    +    Co-authored-by: Taylor Blau [off-list ref]
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
     
      ## builtin/commit-graph.c ##
    @@ builtin/commit-graph.c: static struct opts_commit_graph {
      	int enable_changed_paths;
      } opts;
      
    -+static struct option *add_common_options(struct option *prevopts)
    ++static struct option common_opts[] = {
    ++	OPT_STRING(0, "object-dir", &opts.obj_dir,
    ++		   N_("dir"),
    ++		   N_("the object directory to store the graph")),
    ++	OPT_BOOL(0, "progress", &opts.progress,
    ++		 N_("force progress reporting")),
    ++	OPT_END()
    ++};
    ++
    ++static struct option *add_common_options(struct option *to)
     +{
    -+	struct option options[] = {
    -+		OPT_STRING(0, "object-dir", &opts.obj_dir,
    -+			   N_("dir"),
    -+			   N_("the object directory to store the graph")),
    -+		OPT_BOOL(0, "progress", &opts.progress,
    -+			 N_("force progress reporting")),
    -+		OPT_END()
    -+	};
    -+	struct option *newopts = parse_options_concat(options, prevopts);
    -+	free(prevopts);
    -+	return newopts;
    ++	return parse_options_concat(common_opts, to);
     +}
     +
      static struct object_directory *find_odb(struct repository *r,
    @@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)
     -		OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
      		OPT_END(),
      	};
    -+	struct option *options = parse_options_dup(builtin_commit_graph_verify_options);
    -+	options = add_common_options(options);
    ++	struct option *options = add_common_options(builtin_commit_graph_verify_options);
      
      	trace2_cmd_mode("verify");
      
    @@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)
      			     builtin_commit_graph_verify_usage, 0);
      
      	if (!opts.obj_dir)
    +@@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)
    + 		die_errno(_("Could not open commit-graph '%s'"), graph_name);
    + 
    + 	FREE_AND_NULL(graph_name);
    ++	FREE_AND_NULL(options);
    + 
    + 	if (open_ok)
    + 		graph = load_commit_graph_one_fd_st(the_repository, fd, &st, odb);
     @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
      	struct progress *progress = NULL;
      
    @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
      			0, write_option_max_new_filters),
      		OPT_END(),
      	};
    -+	struct option *options = parse_options_dup(builtin_commit_graph_write_options);
    -+	options = add_common_options(options);
    ++	struct option *options = add_common_options(builtin_commit_graph_write_options);
      
      	opts.progress = isatty(2);
      	opts.enable_changed_paths = -1;
    @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
      
      	if (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1)
     @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
    + 		result = 1;
    + 
    + cleanup:
    ++	FREE_AND_NULL(options);
    + 	string_list_clear(&pack_indexes, 0);
    + 	strbuf_release(&buf);
    + 	return result;
    +@@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
      
      int cmd_commit_graph(int argc, const char **argv, const char *prefix)
      {
    @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
     -			N_("the object directory to store the graph")),
     -		OPT_END(),
     -	};
    -+	struct option *no_options = parse_options_dup(NULL);
    -+	struct option *builtin_commit_graph_options = add_common_options(no_options);
    ++	struct option *builtin_commit_graph_options = common_opts;
      
      	git_config(git_default_config, NULL);
      	argc = parse_options(argc, argv, prefix,
-:  ----------- > 4:  6e9bd877866 multi-pack-index: refactor "goto usage" pattern
4:  6801fb18faa = 5:  7acb4bd75ce commit-graph: early exit to "usage" on !argc
5:  5c96699496b ! 6:  5c1694e071e commit-graph: show usage on "commit-graph [write|verify] garbage"
    @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
      		die(_("use at most one of --reachable, --stdin-commits, or --stdin-packs"));
     
      ## t/t5318-commit-graph.sh ##
    -@@ t/t5318-commit-graph.sh: GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
    +@@ t/t5318-commit-graph.sh: test_description='commit graph'
      
    - test_expect_success 'usage' '
    - 	test_expect_code 129 git commit-graph -h 2>err &&
    --	! grep error: err
    -+	! grep error: err &&
    + GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
    + 
    ++test_expect_success 'usage' '
     +	test_expect_code 129 git commit-graph write blah &&
     +	test_expect_code 129 git commit-graph write verify
    - '
    - 
    ++'
    ++
      test_expect_success 'setup full repo' '
    + 	mkdir full &&
    + 	cd "$TRASH_DIRECTORY/full" &&
-- 
2.32.0.874.ge7a9d58bfcf

Share the usage message between these three variables by using a
macro. Before this new options needed to copy/paste the usage
information, see e.g. 809e0327f5 (builtin/commit-graph.c: introduce
'--max-new-filters=<n>', 2020-09-18).

See b25b727494f (builtin/multi-pack-index.c: define common usage with
a macro, 2021-03-30) for another use of this pattern (but on-list this
one came first).

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index cd863152216..5af3cd7178f 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -9,26 +9,29 @@
 #include "progress.h"
 #include "tag.h"
 
-static char const * const builtin_commit_graph_usage[] = {
-	N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
-	N_("git commit-graph write [--object-dir <objdir>] [--append] "
-	   "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] "
-	   "[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress] "
-	   "<split options>"),
+#define BUILTIN_COMMIT_GRAPH_VERIFY_USAGE \
+	N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]")
+
+#define BUILTIN_COMMIT_GRAPH_WRITE_USAGE \
+	N_("git commit-graph write [--object-dir <objdir>] [--append] " \
+	   "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] " \
+	   "[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress] " \
+	   "<split options>")
+
+static const char * builtin_commit_graph_verify_usage[] = {
+	BUILTIN_COMMIT_GRAPH_VERIFY_USAGE,
 	NULL
 };
 
-static const char * const builtin_commit_graph_verify_usage[] = {
-	N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
+static const char * builtin_commit_graph_write_usage[] = {
+	BUILTIN_COMMIT_GRAPH_WRITE_USAGE,
 	NULL
 };
 
-static const char * const builtin_commit_graph_write_usage[] = {
-	N_("git commit-graph write [--object-dir <objdir>] [--append] "
-	   "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] "
-	   "[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress] "
-	   "<split options>"),
-	NULL
+static char const * const builtin_commit_graph_usage[] = {
+	BUILTIN_COMMIT_GRAPH_VERIFY_USAGE,
+	BUILTIN_COMMIT_GRAPH_WRITE_USAGE,
+	NULL,
 };
 
 static struct opts_commit_graph {

-- 
2.32.0.874.ge7a9d58bfcf

If we don't handle the -h option here like most parse_options() users
we'll fall through and it'll do the right thing for us.

I think this code added in 4ce58ee38d (commit-graph: create
git-commit-graph builtin, 2018-04-02) was always redundant,
parse_options() did this at the time, and the commit-graph code never
used PARSE_OPT_NO_INTERNAL_HELP.

We don't need a test for this, it's tested by the t0012-help.sh test
added in d691551192a (t0012: test "-h" with builtins, 2017-05-30).

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 5af3cd7178f..3cf18dc5345 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -321,10 +321,6 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_commit_graph_usage,
-				   builtin_commit_graph_options);
-
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix,
 			     builtin_commit_graph_options,

-- 
2.32.0.874.ge7a9d58bfcf

Make use of the parse_options_concat() so we don't need to copy/paste
common options like --object-dir.

This is inspired by a similar change to "checkout" in 2087182272
(checkout: split options[] array in three pieces, 2019-03-29), and the
same pattern in the multi-pack-index command, see
60ca94769ce (builtin/multi-pack-index.c: split sub-commands,
2021-03-30).

A minor behavior change here is that now we're going to list both
--object-dir and --progress first, before we'd list --progress along
with other options.

Co-authored-by: Taylor Blau [off-list ref]
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 3cf18dc5345..6e49184439f 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -46,6 +46,20 @@ static struct opts_commit_graph {
 	int enable_changed_paths;
 } opts;
 
+static struct option common_opts[] = {
+	OPT_STRING(0, "object-dir", &opts.obj_dir,
+		   N_("dir"),
+		   N_("the object directory to store the graph")),
+	OPT_BOOL(0, "progress", &opts.progress,
+		 N_("force progress reporting")),
+	OPT_END()
+};
+
+static struct option *add_common_options(struct option *to)
+{
+	return parse_options_concat(common_opts, to);
+}
+
 static struct object_directory *find_odb(struct repository *r,
 					 const char *obj_dir)
 {

@@ -79,20 +93,17 @@ static int graph_verify(int argc, const char **argv)
 	int flags = 0;
 
 	static struct option builtin_commit_graph_verify_options[] = {
-		OPT_STRING(0, "object-dir", &opts.obj_dir,
-			   N_("dir"),
-			   N_("the object directory to store the graph")),
 		OPT_BOOL(0, "shallow", &opts.shallow,
 			 N_("if the commit-graph is split, only verify the tip file")),
-		OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
 		OPT_END(),
 	};
+	struct option *options = add_common_options(builtin_commit_graph_verify_options);
 
 	trace2_cmd_mode("verify");
 
 	opts.progress = isatty(2);
 	argc = parse_options(argc, argv, NULL,
-			     builtin_commit_graph_verify_options,
+			     options,
 			     builtin_commit_graph_verify_usage, 0);
 
 	if (!opts.obj_dir)

@@ -109,6 +120,7 @@ static int graph_verify(int argc, const char **argv)
 		die_errno(_("Could not open commit-graph '%s'"), graph_name);
 
 	FREE_AND_NULL(graph_name);
+	FREE_AND_NULL(options);
 
 	if (open_ok)
 		graph = load_commit_graph_one_fd_st(the_repository, fd, &st, odb);

@@ -209,9 +221,6 @@ static int graph_write(int argc, const char **argv)
 	struct progress *progress = NULL;
 
 	static struct option builtin_commit_graph_write_options[] = {
-		OPT_STRING(0, "object-dir", &opts.obj_dir,
-			N_("dir"),
-			N_("the object directory to store the graph")),
 		OPT_BOOL(0, "reachable", &opts.reachable,
 			N_("start walk at all refs")),
 		OPT_BOOL(0, "stdin-packs", &opts.stdin_packs,

@@ -222,7 +231,6 @@ static int graph_write(int argc, const char **argv)
 			N_("include all commits already in the commit-graph file")),
 		OPT_BOOL(0, "changed-paths", &opts.enable_changed_paths,
 			N_("enable computation for changed paths")),
-		OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
 		OPT_CALLBACK_F(0, "split", &write_opts.split_flags, NULL,
 			N_("allow writing an incremental commit-graph file"),
 			PARSE_OPT_OPTARG | PARSE_OPT_NONEG,

@@ -238,6 +246,7 @@ static int graph_write(int argc, const char **argv)
 			0, write_option_max_new_filters),
 		OPT_END(),
 	};
+	struct option *options = add_common_options(builtin_commit_graph_write_options);
 
 	opts.progress = isatty(2);
 	opts.enable_changed_paths = -1;

@@ -251,7 +260,7 @@ static int graph_write(int argc, const char **argv)
 	git_config(git_commit_graph_write_config, &opts);
 
 	argc = parse_options(argc, argv, NULL,
-			     builtin_commit_graph_write_options,
+			     options,
 			     builtin_commit_graph_write_usage, 0);
 
 	if (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1)

@@ -307,6 +316,7 @@ static int graph_write(int argc, const char **argv)
 		result = 1;
 
 cleanup:
+	FREE_AND_NULL(options);
 	string_list_clear(&pack_indexes, 0);
 	strbuf_release(&buf);
 	return result;

@@ -314,12 +324,7 @@ static int graph_write(int argc, const char **argv)
 
 int cmd_commit_graph(int argc, const char **argv, const char *prefix)
 {
-	static struct option builtin_commit_graph_options[] = {
-		OPT_STRING(0, "object-dir", &opts.obj_dir,
-			N_("dir"),
-			N_("the object directory to store the graph")),
-		OPT_END(),
-	};
+	struct option *builtin_commit_graph_options = common_opts;
 
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix,

-- 
2.32.0.874.ge7a9d58bfcf

Refactor the "goto usage" pattern added in
cd57bc41bbc (builtin/multi-pack-index.c: display usage on unrecognized
command, 2021-03-30) to maintain the same brevity, but doesn't run
afoul of the recommendation in CodingGuidelines about braces:

    When there are multiple arms to a conditional and some of them
    require braces, enclose even a single line block in braces for
    consistency[...]

Let's also change "argv == 0" to juts "!argv", per:

    Do not explicitly compare an integral value with constant 0 or
    '\0', or a pointer value with constant NULL[...]

I'm changing this because in a subsequent commit I'll make
builtin/commit-graph.c use the same pattern, having the two similarly
structured commands match aids readability.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/multi-pack-index.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 5d3ea445fdb..2952388a8eb 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c

@@ -164,7 +164,7 @@ int cmd_multi_pack_index(int argc, const char **argv,
 	if (!opts.object_dir)
 		opts.object_dir = get_object_directory();
 
-	if (argc == 0)
+	if (!argc)
 		goto usage;
 
 	if (!strcmp(argv[0], "repack"))

@@ -175,10 +175,9 @@ int cmd_multi_pack_index(int argc, const char **argv,
 		return cmd_multi_pack_index_verify(argc, argv);
 	else if (!strcmp(argv[0], "expire"))
 		return cmd_multi_pack_index_expire(argc, argv);
-	else {
+
 usage:
-		error(_("unrecognized subcommand: %s"), argv[0]);
-		usage_with_options(builtin_multi_pack_index_usage,
-				   builtin_multi_pack_index_options);
-	}
+	error(_("unrecognized subcommand: %s"), argv[0]);
+	usage_with_options(builtin_multi_pack_index_usage,
+			   builtin_multi_pack_index_options);
 }

-- 
2.32.0.874.ge7a9d58bfcf

Change the parse_options() invocation in the commit-graph code to make
sense. We're calling it twice, once for common options parsing, and
then for the sub-commands.

But we never checked if we had something leftover in argc in "write"
or "verify", as a result we'd silently accept garbage in these
subcommands. Let's not do that.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c  | 10 ++++++++--
 t/t5318-commit-graph.sh |  5 +++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index bf34aa43f22..88cbcb5a64f 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -104,7 +104,10 @@ static int graph_verify(int argc, const char **argv)
 	opts.progress = isatty(2);
 	argc = parse_options(argc, argv, NULL,
 			     options,
-			     builtin_commit_graph_verify_usage, 0);
+			     builtin_commit_graph_verify_usage,
+			     PARSE_OPT_KEEP_UNKNOWN);
+	if (argc)
+		usage_with_options(builtin_commit_graph_verify_usage, options);
 
 	if (!opts.obj_dir)
 		opts.obj_dir = get_object_directory();

@@ -261,7 +264,10 @@ static int graph_write(int argc, const char **argv)
 
 	argc = parse_options(argc, argv, NULL,
 			     options,
-			     builtin_commit_graph_write_usage, 0);
+			     builtin_commit_graph_write_usage,
+			     PARSE_OPT_KEEP_UNKNOWN);
+	if (argc)
+		usage_with_options(builtin_commit_graph_write_usage, options);
 
 	if (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1)
 		die(_("use at most one of --reachable, --stdin-commits, or --stdin-packs"));

diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index af88f805aa2..09a2ccd2920 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh

@@ -5,6 +5,11 @@ test_description='commit graph'
 
 GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=0
 
+test_expect_success 'usage' '
+	test_expect_code 129 git commit-graph write blah &&
+	test_expect_code 129 git commit-graph write verify
+'
+
 test_expect_success 'setup full repo' '
 	mkdir full &&
 	cd "$TRASH_DIRECTORY/full" &&

-- 
2.32.0.874.ge7a9d58bfcf

Rather than guarding all of the !argc with an additional "if" arm
let's do an early goto to "usage". This also makes it clear that
"save_commit_buffer" is not needed in this case.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 6e49184439f..bf34aa43f22 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -331,16 +331,17 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)
 			     builtin_commit_graph_options,
 			     builtin_commit_graph_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
+	if (!argc)
+		goto usage;
 
 	save_commit_buffer = 0;
 
-	if (argc > 0) {
-		if (!strcmp(argv[0], "verify"))
-			return graph_verify(argc, argv);
-		if (!strcmp(argv[0], "write"))
-			return graph_write(argc, argv);
-	}
+	if (!strcmp(argv[0], "verify"))
+		return graph_verify(argc, argv);
+	else if (argc && !strcmp(argv[0], "write"))
+		return graph_write(argc, argv);
 
+usage:
 	usage_with_options(builtin_commit_graph_usage,
 			   builtin_commit_graph_options);
 }

-- 
2.32.0.874.ge7a9d58bfcf

On Tue, Jul 20, 2021 at 01:39:45PM +0200, Ævar Arnfjörð Bjarmason wrote:

Checking 'argc' alone is sufficient to catch unsupported parameters.

Using PARSE_OPT_KEEP_UNKNOWN is not only unnecessary, but arguably
wrong here, because 'git commit-graph write --foo' won't print "error:
unknown option `foo'", and we don't want to pass the remaining
unrecognized options to a different command, like e.g. 'git difftool',
or another parse_options(), like e.g. 'git archive'.

On Tue, Jul 20, 2021 at 07:47:39PM +0200, SZEDER Gábor wrote:

On Tue, Jul 20, 2021 at 01:39:45PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

Change the parse_options() invocation in the commit-graph code to make
sense. We're calling it twice, once for common options parsing, and
then for the sub-commands.

But we never checked if we had something leftover in argc in "write"
or "verify", as a result we'd silently accept garbage in these
subcommands. Let's not do that.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c  | 10 ++++++++--
 t/t5318-commit-graph.sh |  5 +++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index bf34aa43f22..88cbcb5a64f 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -104,7 +104,10 @@ static int graph_verify(int argc, const char **argv)
 	opts.progress = isatty(2);
 	argc = parse_options(argc, argv, NULL,
 			     options,
-			     builtin_commit_graph_verify_usage, 0);
+			     builtin_commit_graph_verify_usage,
+			     PARSE_OPT_KEEP_UNKNOWN);
+	if (argc)
+		usage_with_options(builtin_commit_graph_verify_usage, options);

Checking 'argc' alone is sufficient to catch unsupported parameters.

Using PARSE_OPT_KEEP_UNKNOWN is not only unnecessary, but arguably
wrong here

And it's wrong in 'multi-pack-index' and 'env--helper' as well.

because 'git commit-graph write --foo' won't print "error:
unknown option `foo'", and we don't want to pass the remaining
unrecognized options to a different command, like e.g. 'git difftool',
or another parse_options(), like e.g. 'git archive'.

On Tue, Jul 20, 2021 at 01:39:43PM +0200, Ævar Arnfjörð Bjarmason wrote:

Not the fault of this patch, but since we jump to this from the "if
(!argc)" conditional, reading "argv[0]" is UB. Some compilers will print
out:

    error: unrecognized subcommand: (null)

which at least doesn't segfault, but is still ugly. I don't mind calling
it ugly since I was the one to introduce this behavior (by accident, of
course).

I sent a patch [1] yesterday to squash this, but we should consider
combining the two (or dropping this patch from the series and then
coming back to it later on).

If you want to combine forces, here's what I think that could look like
(without your s-o-b, which you'll have to reattach).

[1]: https://lore.kernel.org/git/8c0bb3e0dc121bd68f7014000fbb60b28750a0fe.1626715096.git.me@ttaylorr.com/

--- >8 ---

From: Ævar Arnfjörð Bjarmason <redacted>
Subject: [PATCH] multi-pack-index: refactor "goto usage" pattern

Refactor the "goto usage" pattern added in
cd57bc41bbc (builtin/multi-pack-index.c: display usage on unrecognized
command, 2021-03-30) to maintain the same brevity, but doesn't run
afoul of the recommendation in CodingGuidelines about braces:

    When there are multiple arms to a conditional and some of them
    require braces, enclose even a single line block in braces for
    consistency[...]

Let's also change "argv == 0" to juts "!argv", per:

    Do not explicitly compare an integral value with constant 0 or
    '\0', or a pointer value with constant NULL[...]

I'm changing this because in a subsequent commit I'll make
builtin/commit-graph.c use the same pattern, having the two similarly
structured commands match aids readability.

While here, fix an issue since where cd57bc41bb
(builtin/multi-pack-index.c: display usage on unrecognized command,
2021-03-30) we sometimes jump to 'usage' without any arguments.

Many compilers will save us from a segfault here, but the end result is
ugly, since it mentions an unrecognized subcommand when we didn't even
pass one, and (on GCC) includes "(null)" in its output.

Move the "usage" label down past the error about unrecognized
subcommands so that it is only triggered when it should be. While we're
at it, bulk up our test coverage in this area, too.

Co-authored-by: Taylor Blau [off-list ref]
Signed-off-by: Taylor Blau <redacted>
---
 builtin/multi-pack-index.c  | 11 +++++------
 t/t5319-multi-pack-index.sh |  6 ++++++
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 5d3ea445fd..e3a684c842 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c

@@ -164,7 +164,7 @@ int cmd_multi_pack_index(int argc, const char **argv,
 	if (!opts.object_dir)
 		opts.object_dir = get_object_directory();

-	if (argc == 0)
+	if (!argc)
 		goto usage;

 	if (!strcmp(argv[0], "repack"))

@@ -175,10 +175,9 @@ int cmd_multi_pack_index(int argc, const char **argv,
 		return cmd_multi_pack_index_verify(argc, argv);
 	else if (!strcmp(argv[0], "expire"))
 		return cmd_multi_pack_index_expire(argc, argv);
-	else {
-usage:
+	else
 		error(_("unrecognized subcommand: %s"), argv[0]);
-		usage_with_options(builtin_multi_pack_index_usage,
-				   builtin_multi_pack_index_options);
-	}
+usage:
+	usage_with_options(builtin_multi_pack_index_usage,
+			   builtin_multi_pack_index_options);
 }

diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 5641d158df..fcde6dcded 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh

@@ -824,4 +824,10 @@ test_expect_success 'load reverse index when missing .idx, .pack' '
 	)
 '

+
+test_expect_success 'usage shown without sub-command' '
+	test_expect_code 129 git multi-pack-index 2>err &&
+	! test_i18ngrep "unrecognized subcommand" err
+'
+
 test_done
--

2.31.1.163.ga65ce7f831

On Tue, Jul 20, 2021 at 01:39:44PM +0200, Ævar Arnfjörð Bjarmason wrote:

Nice, what you wrote here is definitely an improvement in readability.
This could potentially also have an else like I suggested in the
multi-pack-index patch earlier (later?) in the thread. Maybe something
like:

      else if (strcmp(argv[0], "..."))
        return graph_xyz(...);
      else
        error(_("unrecognized subcommand: %s"), argv[0]);
    usage:
        usage_with_options(...);

Arguably that could be done in this patch, separately, or not at all
;).

Thanks,
Taylor

On Tue, Jul 20, 2021 at 07:55:30PM +0200, SZEDER Gábor wrote:

On Tue, Jul 20, 2021 at 07:47:39PM +0200, SZEDER Gábor wrote:

quoted

On Tue, Jul 20, 2021 at 01:39:45PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

Change the parse_options() invocation in the commit-graph code to make
sense. We're calling it twice, once for common options parsing, and
then for the sub-commands.

But we never checked if we had something leftover in argc in "write"
or "verify", as a result we'd silently accept garbage in these
subcommands. Let's not do that.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c  | 10 ++++++++--
 t/t5318-commit-graph.sh |  5 +++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index bf34aa43f22..88cbcb5a64f 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -104,7 +104,10 @@ static int graph_verify(int argc, const char **argv)
 	opts.progress = isatty(2);
 	argc = parse_options(argc, argv, NULL,
 			     options,
-			     builtin_commit_graph_verify_usage, 0);
+			     builtin_commit_graph_verify_usage,
+			     PARSE_OPT_KEEP_UNKNOWN);
+	if (argc)
+		usage_with_options(builtin_commit_graph_verify_usage, options);

Checking 'argc' alone is sufficient to catch unsupported parameters.

Using PARSE_OPT_KEEP_UNKNOWN is not only unnecessary, but arguably
wrong here

And it's wrong in 'multi-pack-index' and 'env--helper' as well.

Thanks for spotting. Obviously this one is new, but the one in the midx
builtin is my fault; I'm not sure why it's there, because it's clearly
wrong.

So we should definitely fix this instance via a reroll of this series,
but that still leaves the others up for grabs. Ævar, are those (the ones
in the 'multi-pack-index' and 'env--helper' builtins) something that you
want to clean up while you're working in this area, or would you rather
that I take care of it?

I don't mind either way, just want to make sure that we don't duplicate
effort.

Thanks,
Taylor

On Tue, Jul 20 2021, Taylor Blau wrote:

On Tue, Jul 20, 2021 at 07:55:30PM +0200, SZEDER Gábor wrote:

quoted

On Tue, Jul 20, 2021 at 07:47:39PM +0200, SZEDER Gábor wrote:

quoted

On Tue, Jul 20, 2021 at 01:39:45PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

Change the parse_options() invocation in the commit-graph code to make
sense. We're calling it twice, once for common options parsing, and
then for the sub-commands.

But we never checked if we had something leftover in argc in "write"
or "verify", as a result we'd silently accept garbage in these
subcommands. Let's not do that.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c  | 10 ++++++++--
 t/t5318-commit-graph.sh |  5 +++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index bf34aa43f22..88cbcb5a64f 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -104,7 +104,10 @@ static int graph_verify(int argc, const char **argv)
 	opts.progress = isatty(2);
 	argc = parse_options(argc, argv, NULL,
 			     options,
-			     builtin_commit_graph_verify_usage, 0);
+			     builtin_commit_graph_verify_usage,
+			     PARSE_OPT_KEEP_UNKNOWN);
+	if (argc)
+		usage_with_options(builtin_commit_graph_verify_usage, options);

Checking 'argc' alone is sufficient to catch unsupported parameters.

Using PARSE_OPT_KEEP_UNKNOWN is not only unnecessary, but arguably
wrong here

And it's wrong in 'multi-pack-index' and 'env--helper' as well.

Thanks for spotting. Obviously this one is new, but the one in the midx
builtin is my fault; I'm not sure why it's there, because it's clearly
wrong.

So we should definitely fix this instance via a reroll of this series,
but that still leaves the others up for grabs. Ævar, are those (the ones
in the 'multi-pack-index' and 'env--helper' builtins) something that you
want to clean up while you're working in this area, or would you rather
that I take care of it?

I don't mind either way, just want to make sure that we don't duplicate
effort.

I'm all for you picking it up :)

If you wanted to pick up these patches (or some of them) and
partially/entirely replace this series I'd be happy with that too,
i.e. if it makes conflicts etc. easier.

I just re-submitted this now because it's been staring at me in my
"should re-roll at somep point" list for a while...

FWIW if you're poking at this area more generally we really could do
with some standardization around these built-in sub-commands:


    git built-in --here subcommand 
    git built-in subcommand --or-here

Various commands support one or the other as some confusing amalgamation
of either taking options to "built-in" and/or "subcommand". The sane
thing to do is:

    git --git-options built-in --built-in-options subcommand --just-for-the-subcommand

But not everything does that, and some things that should be
--git-options or --built-in-options we take as subcommand options,
e.g. --object-format or whatever.

On Tue, Jul 20, 2021 at 11:17:13PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

So we should definitely fix this instance via a reroll of this series,
but that still leaves the others up for grabs. Ævar, are those (the ones
in the 'multi-pack-index' and 'env--helper' builtins) something that you
want to clean up while you're working in this area, or would you rather
that I take care of it?

I don't mind either way, just want to make sure that we don't duplicate
effort.

I'm all for you picking it up :)

If you wanted to pick up these patches (or some of them) and
partially/entirely replace this series I'd be happy with that too,
i.e. if it makes conflicts etc. easier.

I think either is fine; there shouldn't be any conflicts in the
multi-pack-index code just eyeballing based on what you wrote.

I started working on it (which I suppose can count for me volunteering
to patch it up myself ;-)), but I wondered why we have env--helper at
all. When you wrote it back in b4f207f339 (env--helper: new undocumented
builtin wrapping git_env_*(), 2019-06-21), you said that it wasn't added
as a test-tool because it had some uses outside of tests.

But I can't find any locations. We do use env--helper (via
test_bool_env, which you also introduced) in a couple of t/lib-*.sh
files, but this would be far from the first test-tool that has been used
in a test library.

So ISTM that this could be converted to a test-tool and removed from the
list of builtins. *And* we could do that without a deprecation warning,
because it was never documented in the first place.

Can you double check my thinking and/or let me know if there is a
compelling reason to keep it as a builtin?

I just re-submitted this now because it's been staring at me in my
"should re-roll at somep point" list for a while...

FWIW if you're poking at this area more generally we really could do
with some standardization around these built-in sub-commands:

    git built-in --here subcommand
    git built-in subcommand --or-here

That's probably a step too far for this loose end for me, so if you want
to work on that please be my guest, but I probably don't have time for
it in the near future.

Thanks,
Taylor

On Tue, Jul 20 2021, Taylor Blau wrote:

On Tue, Jul 20, 2021 at 11:17:13PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

quoted

So we should definitely fix this instance via a reroll of this series,
but that still leaves the others up for grabs. Ævar, are those (the ones
in the 'multi-pack-index' and 'env--helper' builtins) something that you
want to clean up while you're working in this area, or would you rather
that I take care of it?

I don't mind either way, just want to make sure that we don't duplicate
effort.

I'm all for you picking it up :)

If you wanted to pick up these patches (or some of them) and
partially/entirely replace this series I'd be happy with that too,
i.e. if it makes conflicts etc. easier.

I think either is fine; there shouldn't be any conflicts in the
multi-pack-index code just eyeballing based on what you wrote.

I started working on it (which I suppose can count for me volunteering
to patch it up myself ;-)), but I wondered why we have env--helper at
all. When you wrote it back in b4f207f339 (env--helper: new undocumented
builtin wrapping git_env_*(), 2019-06-21), you said that it wasn't added
as a test-tool because it had some uses outside of tests.

But I can't find any locations. We do use env--helper (via
test_bool_env, which you also introduced) in a couple of t/lib-*.sh

FWIW test_bool_env is SZEDER'S, see 43a2afee82a (tests: add
'test_bool_env' to catch non-bool GIT_TEST_* values, 2019-11-22).

files, but this would be far from the first test-tool that has been used
in a test library.

So ISTM that this could be converted to a test-tool and removed from the
list of builtins. *And* we could do that without a deprecation warning,
because it was never documented in the first place.

Can you double check my thinking and/or let me know if there is a
compelling reason to keep it as a builtin?

Unlike the test-tools it was used by installed git, namely
git-sh-i18n.sh before d162b25f956 (tests: remove support for
GIT_TEST_GETTEXT_POISON, 2021-01-20), but yes, now it could just be
migrated to a test-tool.

I suppose it never really *needed* to be a built-in, i.e. the
git-sh-i18n.sh code could have just died without the helper if
GIT_TEST_GETTEXT_POISON was set, anyway...

...yes, nowadays it can simply be moved to t/helper.

<digression>

I do think in general this recent proliferation of t/helper over new
plumbing built-ins has sent git a bit in the wrong direction.

E.g. I think the likes of t/helper/test-pkt-line.c should really be a
plumbing tool, the same goes for many (but not all) the test tool, we
could just document them as being "unstable plumbing" or whatever.

But I think I've been losing that argument recently, e.g. after [1]
(which I argued we should put into git-ls-files) even things like git's
basic idea of the state of the index are exposed in some helpers, but
not corresponding plumbing.

Anyway, even if we assume that's an argument that would carry the day in
general I'd find it hard to justify git-env--helper being a thing that
should be exposed to users or post-"make install", it's purely for the
use of our own test suite.

</digression>


So yeah, it can just be moved to t/helper if you want to do that.

quoted

I just re-submitted this now because it's been staring at me in my
"should re-roll at somep point" list for a while...

FWIW if you're poking at this area more generally we really could do
with some standardization around these built-in sub-commands:

    git built-in --here subcommand
    git built-in subcommand --or-here

That's probably a step too far for this loose end for me, so if you want
to work on that please be my guest, but I probably don't have time for
it in the near future.

*nod*, but just to be clear, were you going to pick up &
re-roll/re-submit the patches I have in this series? That would be good
or me, but I don't mind either way, just wondering what state this
should be on my TODO list.

1. https://lore.kernel.org/git/a1b8135c0fc890b2c2c0271c923b2f8efa8d1465.1617109864.git.gitgitgadget@gmail.com/

On Wed, Jul 21, 2021 at 09:26:38AM +0200, Ævar Arnfjörð Bjarmason wrote:

<digression>

I do think in general this recent proliferation of t/helper over new
plumbing built-ins has sent git a bit in the wrong direction.

E.g. I think the likes of t/helper/test-pkt-line.c should really be a
plumbing tool, the same goes for many (but not all) the test tool, we
could just document them as being "unstable plumbing" or whatever.

FWIW, I agree with you here. These kind of "inspection" tools are handy
when you are debugging something. Building a copy of test-tool on a
production system is only a mild inconvenience for me, but not being
able to ask a user things like "what does git pack-bitmap --dump
.git/objects/pack*.bitmap say" is occasionally quite annoying.

The flip side is that we expect the overall quality of user-visible
tools to be a bit higher, and we're generally on the hook to keep
supporting them. Maybe that's solvable with documentation. I dunno.

But I think I've been losing that argument recently, e.g. after [1]
(which I argued we should put into git-ls-files) even things like git's
basic idea of the state of the index are exposed in some helpers, but
not corresponding plumbing.

Yeah. I wish "ls-files --debug" showed more of the extension data, for
example.

Anyway, even if we assume that's an argument that would carry the day in
general I'd find it hard to justify git-env--helper being a thing that
should be exposed to users or post-"make install", it's purely for the
use of our own test suite.

Yeah, I'd agree with that. The most valuable helpers to me are the ones
that help us understand what Git is seeing, or what's in a binary file
format. Obscure-case "functional" helpers are less likely to be
generally useful.

-Peff

A re-roll of v3 which should fix all outstanding issues &
feedback. Thanks SZEDER & Taylor:
https://lore.kernel.org/git/cover-0.6-00000000000-20210720T113707Z-avarab@gmail.com/

I dropped this myself because per
https://lore.kernel.org/git/87im14unfd.fsf@evledraar.gmail.com/ I was
under the impression that Taylor was intending to pick up these
patches as part of some more generale usage() fixes of his (which also
touched multi-pack-index.c), but his recent changes to fix
multi-pack-index.c didn't pick this up, so here it is as a re-roll instead.

Ævar Arnfjörð Bjarmason (7):
  commit-graph: define common usage with a macro
  commit-graph: remove redundant handling of -h
  commit-graph: use parse_options_concat()
  multi-pack-index: refactor "goto usage" pattern
  commit-graph: early exit to "usage" on !argc
  commit-graph: show usage on "commit-graph [write|verify] garbage"
  commit-graph: show "unexpected subcommand" error

 builtin/commit-graph.c     | 90 +++++++++++++++++++++-----------------
 builtin/multi-pack-index.c | 11 +++--
 t/t5318-commit-graph.sh    | 19 ++++++++
 3 files changed, 74 insertions(+), 46 deletions(-)

Range-diff against v3:
1:  1b9b4703ce2 = 1:  ef37a8243c8 commit-graph: define common usage with a macro
2:  8f50750ae5e = 2:  bbed18ff193 commit-graph: remove redundant handling of -h
3:  f02da994363 = 3:  32cc0d1c7bc commit-graph: use parse_options_concat()
4:  6e9bd877866 ! 4:  087f98bbec6 multi-pack-index: refactor "goto usage" pattern
    @@ Commit message
     
         Refactor the "goto usage" pattern added in
         cd57bc41bbc (builtin/multi-pack-index.c: display usage on unrecognized
    -    command, 2021-03-30) to maintain the same brevity, but doesn't run
    -    afoul of the recommendation in CodingGuidelines about braces:
    +    command, 2021-03-30) and 88617d11f9d (multi-pack-index: fix potential
    +    segfault without sub-command, 2021-07-19) to maintain the same
    +    brevity, but in a form that doesn't run afoul of the recommendation in
    +    CodingGuidelines about braces:
     
             When there are multiple arms to a conditional and some of them
             require braces, enclose even a single line block in braces for
    @@ builtin/multi-pack-index.c: int cmd_multi_pack_index(int argc, const char **argv
      	else if (!strcmp(argv[0], "expire"))
      		return cmd_multi_pack_index_expire(argc, argv);
     -	else {
    +-		error(_("unrecognized subcommand: %s"), argv[0]);
     +
    ++	error(_("unrecognized subcommand: %s"), argv[0]);
      usage:
    --		error(_("unrecognized subcommand: %s"), argv[0]);
     -		usage_with_options(builtin_multi_pack_index_usage,
     -				   builtin_multi_pack_index_options);
     -	}
    -+	error(_("unrecognized subcommand: %s"), argv[0]);
     +	usage_with_options(builtin_multi_pack_index_usage,
     +			   builtin_multi_pack_index_options);
      }
5:  7acb4bd75ce = 5:  2983e16ba69 commit-graph: early exit to "usage" on !argc
6:  5c1694e071e ! 6:  85552a6f88c commit-graph: show usage on "commit-graph [write|verify] garbage"
    @@ Metadata
      ## Commit message ##
         commit-graph: show usage on "commit-graph [write|verify] garbage"
     
    -    Change the parse_options() invocation in the commit-graph code to make
    -    sense. We're calling it twice, once for common options parsing, and
    -    then for the sub-commands.
    +    Change the parse_options() invocation in the commit-graph code to
    +    error on unknown leftover argv elements, in addition to the existing
    +    and implicit erroring via parse_options() on unknown options.
     
    -    But we never checked if we had something leftover in argc in "write"
    -    or "verify", as a result we'd silently accept garbage in these
    -    subcommands. Let's not do that.
    +    We'd already error in cmd_commit_graph() on e.g.:
    +
    +        git commit-graph unknown verify
    +        git commit-graph --unknown verify
    +
    +    But here we're calling parse_options() twice more for the "write" and
    +    "verify" subcommands. We did not do the same checking for leftover
    +    argv elements there. As a result we'd silently accept garbage in these
    +    subcommands, let's not do that.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
     
      ## builtin/commit-graph.c ##
     @@ builtin/commit-graph.c: static int graph_verify(int argc, const char **argv)
    - 	opts.progress = isatty(2);
      	argc = parse_options(argc, argv, NULL,
      			     options,
    --			     builtin_commit_graph_verify_usage, 0);
    -+			     builtin_commit_graph_verify_usage,
    -+			     PARSE_OPT_KEEP_UNKNOWN);
    + 			     builtin_commit_graph_verify_usage, 0);
     +	if (argc)
     +		usage_with_options(builtin_commit_graph_verify_usage, options);
      
      	if (!opts.obj_dir)
      		opts.obj_dir = get_object_directory();
     @@ builtin/commit-graph.c: static int graph_write(int argc, const char **argv)
    - 
      	argc = parse_options(argc, argv, NULL,
      			     options,
    --			     builtin_commit_graph_write_usage, 0);
    -+			     builtin_commit_graph_write_usage,
    -+			     PARSE_OPT_KEEP_UNKNOWN);
    + 			     builtin_commit_graph_write_usage, 0);
     +	if (argc)
     +		usage_with_options(builtin_commit_graph_write_usage, options);
      
-:  ----------- > 7:  962521cfa17 commit-graph: show "unexpected subcommand" error
-- 
2.33.0.662.gbc81f8cbdca

Share the usage message between these three variables by using a
macro. Before this new options needed to copy/paste the usage
information, see e.g. 809e0327f5 (builtin/commit-graph.c: introduce
'--max-new-filters=<n>', 2020-09-18).

See b25b727494f (builtin/multi-pack-index.c: define common usage with
a macro, 2021-03-30) for another use of this pattern (but on-list this
one came first).

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index cd863152216..5af3cd7178f 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -9,26 +9,29 @@
 #include "progress.h"
 #include "tag.h"
 
-static char const * const builtin_commit_graph_usage[] = {
-	N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
-	N_("git commit-graph write [--object-dir <objdir>] [--append] "
-	   "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] "
-	   "[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress] "
-	   "<split options>"),
+#define BUILTIN_COMMIT_GRAPH_VERIFY_USAGE \
+	N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]")
+
+#define BUILTIN_COMMIT_GRAPH_WRITE_USAGE \
+	N_("git commit-graph write [--object-dir <objdir>] [--append] " \
+	   "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] " \
+	   "[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress] " \
+	   "<split options>")
+
+static const char * builtin_commit_graph_verify_usage[] = {
+	BUILTIN_COMMIT_GRAPH_VERIFY_USAGE,
 	NULL
 };
 
-static const char * const builtin_commit_graph_verify_usage[] = {
-	N_("git commit-graph verify [--object-dir <objdir>] [--shallow] [--[no-]progress]"),
+static const char * builtin_commit_graph_write_usage[] = {
+	BUILTIN_COMMIT_GRAPH_WRITE_USAGE,
 	NULL
 };
 
-static const char * const builtin_commit_graph_write_usage[] = {
-	N_("git commit-graph write [--object-dir <objdir>] [--append] "
-	   "[--split[=<strategy>]] [--reachable|--stdin-packs|--stdin-commits] "
-	   "[--changed-paths] [--[no-]max-new-filters <n>] [--[no-]progress] "
-	   "<split options>"),
-	NULL
+static char const * const builtin_commit_graph_usage[] = {
+	BUILTIN_COMMIT_GRAPH_VERIFY_USAGE,
+	BUILTIN_COMMIT_GRAPH_WRITE_USAGE,
+	NULL,
 };
 
 static struct opts_commit_graph {

-- 
2.33.0.662.gbc81f8cbdca

If we don't handle the -h option here like most parse_options() users
we'll fall through and it'll do the right thing for us.

I think this code added in 4ce58ee38d (commit-graph: create
git-commit-graph builtin, 2018-04-02) was always redundant,
parse_options() did this at the time, and the commit-graph code never
used PARSE_OPT_NO_INTERNAL_HELP.

We don't need a test for this, it's tested by the t0012-help.sh test
added in d691551192a (t0012: test "-h" with builtins, 2017-05-30).

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 5af3cd7178f..3cf18dc5345 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -321,10 +321,6 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_commit_graph_usage,
-				   builtin_commit_graph_options);
-
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix,
 			     builtin_commit_graph_options,

-- 
2.33.0.662.gbc81f8cbdca

Make use of the parse_options_concat() so we don't need to copy/paste
common options like --object-dir.

This is inspired by a similar change to "checkout" in 2087182272
(checkout: split options[] array in three pieces, 2019-03-29), and the
same pattern in the multi-pack-index command, see
60ca94769ce (builtin/multi-pack-index.c: split sub-commands,
2021-03-30).

A minor behavior change here is that now we're going to list both
--object-dir and --progress first, before we'd list --progress along
with other options.

Co-authored-by: Taylor Blau [off-list ref]
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 3cf18dc5345..6e49184439f 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -46,6 +46,20 @@ static struct opts_commit_graph {
 	int enable_changed_paths;
 } opts;
 
+static struct option common_opts[] = {
+	OPT_STRING(0, "object-dir", &opts.obj_dir,
+		   N_("dir"),
+		   N_("the object directory to store the graph")),
+	OPT_BOOL(0, "progress", &opts.progress,
+		 N_("force progress reporting")),
+	OPT_END()
+};
+
+static struct option *add_common_options(struct option *to)
+{
+	return parse_options_concat(common_opts, to);
+}
+
 static struct object_directory *find_odb(struct repository *r,
 					 const char *obj_dir)
 {

@@ -79,20 +93,17 @@ static int graph_verify(int argc, const char **argv)
 	int flags = 0;
 
 	static struct option builtin_commit_graph_verify_options[] = {
-		OPT_STRING(0, "object-dir", &opts.obj_dir,
-			   N_("dir"),
-			   N_("the object directory to store the graph")),
 		OPT_BOOL(0, "shallow", &opts.shallow,
 			 N_("if the commit-graph is split, only verify the tip file")),
-		OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
 		OPT_END(),
 	};
+	struct option *options = add_common_options(builtin_commit_graph_verify_options);
 
 	trace2_cmd_mode("verify");
 
 	opts.progress = isatty(2);
 	argc = parse_options(argc, argv, NULL,
-			     builtin_commit_graph_verify_options,
+			     options,
 			     builtin_commit_graph_verify_usage, 0);
 
 	if (!opts.obj_dir)

@@ -109,6 +120,7 @@ static int graph_verify(int argc, const char **argv)
 		die_errno(_("Could not open commit-graph '%s'"), graph_name);
 
 	FREE_AND_NULL(graph_name);
+	FREE_AND_NULL(options);
 
 	if (open_ok)
 		graph = load_commit_graph_one_fd_st(the_repository, fd, &st, odb);

@@ -209,9 +221,6 @@ static int graph_write(int argc, const char **argv)
 	struct progress *progress = NULL;
 
 	static struct option builtin_commit_graph_write_options[] = {
-		OPT_STRING(0, "object-dir", &opts.obj_dir,
-			N_("dir"),
-			N_("the object directory to store the graph")),
 		OPT_BOOL(0, "reachable", &opts.reachable,
 			N_("start walk at all refs")),
 		OPT_BOOL(0, "stdin-packs", &opts.stdin_packs,

@@ -222,7 +231,6 @@ static int graph_write(int argc, const char **argv)
 			N_("include all commits already in the commit-graph file")),
 		OPT_BOOL(0, "changed-paths", &opts.enable_changed_paths,
 			N_("enable computation for changed paths")),
-		OPT_BOOL(0, "progress", &opts.progress, N_("force progress reporting")),
 		OPT_CALLBACK_F(0, "split", &write_opts.split_flags, NULL,
 			N_("allow writing an incremental commit-graph file"),
 			PARSE_OPT_OPTARG | PARSE_OPT_NONEG,

@@ -238,6 +246,7 @@ static int graph_write(int argc, const char **argv)
 			0, write_option_max_new_filters),
 		OPT_END(),
 	};
+	struct option *options = add_common_options(builtin_commit_graph_write_options);
 
 	opts.progress = isatty(2);
 	opts.enable_changed_paths = -1;

@@ -251,7 +260,7 @@ static int graph_write(int argc, const char **argv)
 	git_config(git_commit_graph_write_config, &opts);
 
 	argc = parse_options(argc, argv, NULL,
-			     builtin_commit_graph_write_options,
+			     options,
 			     builtin_commit_graph_write_usage, 0);
 
 	if (opts.reachable + opts.stdin_packs + opts.stdin_commits > 1)

@@ -307,6 +316,7 @@ static int graph_write(int argc, const char **argv)
 		result = 1;
 
 cleanup:
+	FREE_AND_NULL(options);
 	string_list_clear(&pack_indexes, 0);
 	strbuf_release(&buf);
 	return result;

@@ -314,12 +324,7 @@ static int graph_write(int argc, const char **argv)
 
 int cmd_commit_graph(int argc, const char **argv, const char *prefix)
 {
-	static struct option builtin_commit_graph_options[] = {
-		OPT_STRING(0, "object-dir", &opts.obj_dir,
-			N_("dir"),
-			N_("the object directory to store the graph")),
-		OPT_END(),
-	};
+	struct option *builtin_commit_graph_options = common_opts;
 
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix,

-- 
2.33.0.662.gbc81f8cbdca

Refactor the "goto usage" pattern added in
cd57bc41bbc (builtin/multi-pack-index.c: display usage on unrecognized
command, 2021-03-30) and 88617d11f9d (multi-pack-index: fix potential
segfault without sub-command, 2021-07-19) to maintain the same
brevity, but in a form that doesn't run afoul of the recommendation in
CodingGuidelines about braces:

    When there are multiple arms to a conditional and some of them
    require braces, enclose even a single line block in braces for
    consistency[...]

Let's also change "argv == 0" to juts "!argv", per:

    Do not explicitly compare an integral value with constant 0 or
    '\0', or a pointer value with constant NULL[...]

I'm changing this because in a subsequent commit I'll make
builtin/commit-graph.c use the same pattern, having the two similarly
structured commands match aids readability.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/multi-pack-index.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 8ff0dee2ecb..649aa5f9ab2 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c

@@ -164,7 +164,7 @@ int cmd_multi_pack_index(int argc, const char **argv,
 	if (!opts.object_dir)
 		opts.object_dir = get_object_directory();
 
-	if (argc == 0)
+	if (!argc)
 		goto usage;
 
 	if (!strcmp(argv[0], "repack"))

@@ -175,10 +175,9 @@ int cmd_multi_pack_index(int argc, const char **argv,
 		return cmd_multi_pack_index_verify(argc, argv);
 	else if (!strcmp(argv[0], "expire"))
 		return cmd_multi_pack_index_expire(argc, argv);
-	else {
-		error(_("unrecognized subcommand: %s"), argv[0]);
+
+	error(_("unrecognized subcommand: %s"), argv[0]);
 usage:
-		usage_with_options(builtin_multi_pack_index_usage,
-				   builtin_multi_pack_index_options);
-	}
+	usage_with_options(builtin_multi_pack_index_usage,
+			   builtin_multi_pack_index_options);
 }

-- 
2.33.0.662.gbc81f8cbdca

Rather than guarding all of the !argc with an additional "if" arm
let's do an early goto to "usage". This also makes it clear that
"save_commit_buffer" is not needed in this case.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 6e49184439f..bf34aa43f22 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -331,16 +331,17 @@ int cmd_commit_graph(int argc, const char **argv, const char *prefix)
 			     builtin_commit_graph_options,
 			     builtin_commit_graph_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
+	if (!argc)
+		goto usage;
 
 	save_commit_buffer = 0;
 
-	if (argc > 0) {
-		if (!strcmp(argv[0], "verify"))
-			return graph_verify(argc, argv);
-		if (!strcmp(argv[0], "write"))
-			return graph_write(argc, argv);
-	}
+	if (!strcmp(argv[0], "verify"))
+		return graph_verify(argc, argv);
+	else if (argc && !strcmp(argv[0], "write"))
+		return graph_write(argc, argv);
 
+usage:
 	usage_with_options(builtin_commit_graph_usage,
 			   builtin_commit_graph_options);
 }

-- 
2.33.0.662.gbc81f8cbdca

Change the parse_options() invocation in the commit-graph code to
error on unknown leftover argv elements, in addition to the existing
and implicit erroring via parse_options() on unknown options.

We'd already error in cmd_commit_graph() on e.g.:

    git commit-graph unknown verify
    git commit-graph --unknown verify

But here we're calling parse_options() twice more for the "write" and
"verify" subcommands. We did not do the same checking for leftover
argv elements there. As a result we'd silently accept garbage in these
subcommands, let's not do that.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/commit-graph.c  | 4 ++++
 t/t5318-commit-graph.sh | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index bf34aa43f22..0457903f18f 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c

@@ -105,6 +105,8 @@ static int graph_verify(int argc, const char **argv)
 	argc = parse_options(argc, argv, NULL,
 			     options,
 			     builtin_commit_graph_verify_usage, 0);
+	if (argc)
+		usage_with_options(builtin_commit_graph_verify_usage, options);
 
 	if (!opts.obj_dir)
 		opts.obj_dir = get_object_directory();