git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH v2 0/5] commit-graph: usage fixes
@ 2021-07-18  7:58 Ævar Arnfjörð Bjarmason
  2021-07-18  7:58 ` [PATCH v2 1/5] commit-graph: define common usage with a macro Ævar Arnfjörð Bjarmason
                   ` (5 more replies)
  0 siblings, 6 replies; 30+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-18  7:58 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Taylor Blau,
	Ævar Arnfjörð Bjarmason

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 <avarab@gmail.com>
     
      ## 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 <avarab@gmail.com>
     
      ## 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 <avarab@gmail.com>
     
      ## 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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 1/5] commit-graph: define common usage with a macro
  2021-07-18  7:58 [PATCH v2 0/5] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
@ 2021-07-18  7:58 ` Ævar Arnfjörð Bjarmason
  2021-07-19 16:29   ` Taylor Blau
  2021-07-18  7:58 ` [PATCH v2 2/5] commit-graph: remove redundant handling of -h Ævar Arnfjörð Bjarmason
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 30+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-18  7:58 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Taylor Blau,
	Ævar Arnfjörð Bjarmason

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 <avarab@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 2/5] commit-graph: remove redundant handling of -h
  2021-07-18  7:58 [PATCH v2 0/5] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
  2021-07-18  7:58 ` [PATCH v2 1/5] commit-graph: define common usage with a macro Ævar Arnfjörð Bjarmason
@ 2021-07-18  7:58 ` Ævar Arnfjörð Bjarmason
  2021-07-18 12:55   ` Andrei Rybak
  2021-07-18  7:58 ` [PATCH v2 3/5] commit-graph: use parse_options_concat() Ævar Arnfjörð Bjarmason
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 30+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-18  7:58 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Taylor Blau,
	Ævar Arnfjörð Bjarmason

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 <avarab@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 3/5] commit-graph: use parse_options_concat()
  2021-07-18  7:58 [PATCH v2 0/5] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
  2021-07-18  7:58 ` [PATCH v2 1/5] commit-graph: define common usage with a macro Ævar Arnfjörð Bjarmason
  2021-07-18  7:58 ` [PATCH v2 2/5] commit-graph: remove redundant handling of -h Ævar Arnfjörð Bjarmason
@ 2021-07-18  7:58 ` Ævar Arnfjörð Bjarmason
  2021-07-19 16:50   ` Taylor Blau
  2021-07-18  7:58 ` [PATCH v2 4/5] commit-graph: early exit to "usage" on !argc Ævar Arnfjörð Bjarmason
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 30+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-18  7:58 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Taylor Blau,
	Ævar Arnfjörð Bjarmason

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 <avarab@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 4/5] commit-graph: early exit to "usage" on !argc
  2021-07-18  7:58 [PATCH v2 0/5] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
                   ` (2 preceding siblings ...)
  2021-07-18  7:58 ` [PATCH v2 3/5] commit-graph: use parse_options_concat() Ævar Arnfjörð Bjarmason
@ 2021-07-18  7:58 ` Ævar Arnfjörð Bjarmason
  2021-07-19 16:55   ` Taylor Blau
  2021-07-18  7:58 ` [PATCH v2 5/5] commit-graph: show usage on "commit-graph [write|verify] garbage" Ævar Arnfjörð Bjarmason
  2021-07-20 11:39 ` [PATCH v3 0/6] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
  5 siblings, 1 reply; 30+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-18  7:58 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Taylor Blau,
	Ævar Arnfjörð Bjarmason

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 <avarab@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 5/5] commit-graph: show usage on "commit-graph [write|verify] garbage"
  2021-07-18  7:58 [PATCH v2 0/5] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
                   ` (3 preceding siblings ...)
  2021-07-18  7:58 ` [PATCH v2 4/5] commit-graph: early exit to "usage" on !argc Ævar Arnfjörð Bjarmason
@ 2021-07-18  7:58 ` Ævar Arnfjörð Bjarmason
  2021-07-19 16:53   ` Taylor Blau
  2021-07-20 11:39 ` [PATCH v3 0/6] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
  5 siblings, 1 reply; 30+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-18  7:58 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Taylor Blau,
	Ævar Arnfjörð Bjarmason

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 <avarab@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 2/5] commit-graph: remove redundant handling of -h
  2021-07-18  7:58 ` [PATCH v2 2/5] commit-graph: remove redundant handling of -h Ævar Arnfjörð Bjarmason
@ 2021-07-18 12:55   ` Andrei Rybak
  2021-07-19 16:34     ` Taylor Blau
  0 siblings, 1 reply; 30+ messages in thread
From: Andrei Rybak @ 2021-07-18 12:55 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Junio C Hamano, Derrick Stolee, Taylor Blau

On 18/07/2021 09:58, Ævar Arnfjörð Bjarmason wrote:
> 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 <avarab@gmail.com>
> ---
>   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

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" &&
> 


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 1/5] commit-graph: define common usage with a macro
  2021-07-18  7:58 ` [PATCH v2 1/5] commit-graph: define common usage with a macro Ævar Arnfjörð Bjarmason
@ 2021-07-19 16:29   ` Taylor Blau
  0 siblings, 0 replies; 30+ messages in thread
From: Taylor Blau @ 2021-07-19 16:29 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Derrick Stolee

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 2/5] commit-graph: remove redundant handling of -h
  2021-07-18 12:55   ` Andrei Rybak
@ 2021-07-19 16:34     ` Taylor Blau
  0 siblings, 0 replies; 30+ messages in thread
From: Taylor Blau @ 2021-07-19 16:34 UTC (permalink / raw)
  To: Andrei Rybak
  Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano,
	Derrick Stolee

On Sun, Jul 18, 2021 at 02:55:09PM +0200, Andrei Rybak wrote:
> On 18/07/2021 09:58, Ævar Arnfjörð Bjarmason wrote:
> > 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.

> > 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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 3/5] commit-graph: use parse_options_concat()
  2021-07-18  7:58 ` [PATCH v2 3/5] commit-graph: use parse_options_concat() Ævar Arnfjörð Bjarmason
@ 2021-07-19 16:50   ` Taylor Blau
  2021-07-20 11:31     ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 30+ messages in thread
From: Taylor Blau @ 2021-07-19 16:50 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Derrick Stolee

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/

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  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).

> +	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.

> +	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);

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,

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 5/5] commit-graph: show usage on "commit-graph [write|verify] garbage"
  2021-07-18  7:58 ` [PATCH v2 5/5] commit-graph: show usage on "commit-graph [write|verify] garbage" Ævar Arnfjörð Bjarmason
@ 2021-07-19 16:53   ` Taylor Blau
  0 siblings, 0 replies; 30+ messages in thread
From: Taylor Blau @ 2021-07-19 16:53 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Derrick Stolee

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 4/5] commit-graph: early exit to "usage" on !argc
  2021-07-18  7:58 ` [PATCH v2 4/5] commit-graph: early exit to "usage" on !argc Ævar Arnfjörð Bjarmason
@ 2021-07-19 16:55   ` Taylor Blau
  0 siblings, 0 replies; 30+ messages in thread
From: Taylor Blau @ 2021-07-19 16:55 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Derrick Stolee

On Sun, Jul 18, 2021 at 09:58:08AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 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 <avarab@gmail.com>
> ---
>  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);

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 3/5] commit-graph: use parse_options_concat()
  2021-07-19 16:50   ` Taylor Blau
@ 2021-07-20 11:31     ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 30+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-20 11:31 UTC (permalink / raw)
  To: Taylor Blau; +Cc: git, Junio C Hamano, Derrick Stolee


On Mon, Jul 19 2021, Taylor Blau wrote:

> 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/

Thanks.

>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  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 :)

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v3 0/6] commit-graph: usage fixes
  2021-07-18  7:58 [PATCH v2 0/5] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
                   ` (4 preceding siblings ...)
  2021-07-18  7:58 ` [PATCH v2 5/5] commit-graph: show usage on "commit-graph [write|verify] garbage" Ævar Arnfjörð Bjarmason
@ 2021-07-20 11:39 ` Ævar Arnfjörð Bjarmason
  2021-07-20 11:39   ` [PATCH v3 1/6] commit-graph: define common usage with a macro Ævar Arnfjörð Bjarmason
                     ` (5 more replies)
  5 siblings, 6 replies; 30+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-20 11:39 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
	Ævar Arnfjörð Bjarmason

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 <avarab@gmail.com>
     
      ## 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 <me@ttaylorr.com>
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## 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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v3 1/6] commit-graph: define common usage with a macro
  2021-07-20 11:39 ` [PATCH v3 0/6] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
@ 2021-07-20 11:39   ` Ævar Arnfjörð Bjarmason
  2021-07-20 11:39   ` [PATCH v3 2/6] commit-graph: remove redundant handling of -h Ævar Arnfjörð Bjarmason
                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 30+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-20 11:39 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
	Ævar Arnfjörð Bjarmason

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 <avarab@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v3 2/6] commit-graph: remove redundant handling of -h
  2021-07-20 11:39 ` [PATCH v3 0/6] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
  2021-07-20 11:39   ` [PATCH v3 1/6] commit-graph: define common usage with a macro Ævar Arnfjörð Bjarmason
@ 2021-07-20 11:39   ` Ævar Arnfjörð Bjarmason
  2021-07-20 11:39   ` [PATCH v3 3/6] commit-graph: use parse_options_concat() Ævar Arnfjörð Bjarmason
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 30+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-20 11:39 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
	Ævar Arnfjörð Bjarmason

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 <avarab@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v3 3/6] commit-graph: use parse_options_concat()
  2021-07-20 11:39 ` [PATCH v3 0/6] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
  2021-07-20 11:39   ` [PATCH v3 1/6] commit-graph: define common usage with a macro Ævar Arnfjörð Bjarmason
  2021-07-20 11:39   ` [PATCH v3 2/6] commit-graph: remove redundant handling of -h Ævar Arnfjörð Bjarmason
@ 2021-07-20 11:39   ` Ævar Arnfjörð Bjarmason
  2021-07-20 11:39   ` [PATCH v3 4/6] multi-pack-index: refactor "goto usage" pattern Ævar Arnfjörð Bjarmason
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 30+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-20 11:39 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
	Ævar Arnfjörð Bjarmason

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 <me@ttaylorr.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v3 4/6] multi-pack-index: refactor "goto usage" pattern
  2021-07-20 11:39 ` [PATCH v3 0/6] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
                     ` (2 preceding siblings ...)
  2021-07-20 11:39   ` [PATCH v3 3/6] commit-graph: use parse_options_concat() Ævar Arnfjörð Bjarmason
@ 2021-07-20 11:39   ` Ævar Arnfjörð Bjarmason
  2021-07-20 18:14     ` Taylor Blau
  2021-07-20 11:39   ` [PATCH v3 5/6] commit-graph: early exit to "usage" on !argc Ævar Arnfjörð Bjarmason
  2021-07-20 11:39   ` [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage" Ævar Arnfjörð Bjarmason
  5 siblings, 1 reply; 30+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-20 11:39 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
	Ævar Arnfjörð Bjarmason

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 <avarab@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v3 5/6] commit-graph: early exit to "usage" on !argc
  2021-07-20 11:39 ` [PATCH v3 0/6] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
                     ` (3 preceding siblings ...)
  2021-07-20 11:39   ` [PATCH v3 4/6] multi-pack-index: refactor "goto usage" pattern Ævar Arnfjörð Bjarmason
@ 2021-07-20 11:39   ` Ævar Arnfjörð Bjarmason
  2021-07-20 18:17     ` Taylor Blau
  2021-07-20 11:39   ` [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage" Ævar Arnfjörð Bjarmason
  5 siblings, 1 reply; 30+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-20 11:39 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
	Ævar Arnfjörð Bjarmason

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 <avarab@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"
  2021-07-20 11:39 ` [PATCH v3 0/6] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
                     ` (4 preceding siblings ...)
  2021-07-20 11:39   ` [PATCH v3 5/6] commit-graph: early exit to "usage" on !argc Ævar Arnfjörð Bjarmason
@ 2021-07-20 11:39   ` Ævar Arnfjörð Bjarmason
  2021-07-20 17:47     ` SZEDER Gábor
  5 siblings, 1 reply; 30+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-20 11:39 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak,
	Ævar Arnfjörð Bjarmason

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 <avarab@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"
  2021-07-20 11:39   ` [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage" Ævar Arnfjörð Bjarmason
@ 2021-07-20 17:47     ` SZEDER Gábor
  2021-07-20 17:55       ` SZEDER Gábor
  0 siblings, 1 reply; 30+ messages in thread
From: SZEDER Gábor @ 2021-07-20 17:47 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak

On Tue, Jul 20, 2021 at 01:39:45PM +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.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  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, 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'.

>  	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
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"
  2021-07-20 17:47     ` SZEDER Gábor
@ 2021-07-20 17:55       ` SZEDER Gábor
  2021-07-20 18:24         ` Taylor Blau
  0 siblings, 1 reply; 30+ messages in thread
From: SZEDER Gábor @ 2021-07-20 17:55 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak

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:
> > 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 <avarab@gmail.com>
> > ---
> >  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'.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v3 4/6] multi-pack-index: refactor "goto usage" pattern
  2021-07-20 11:39   ` [PATCH v3 4/6] multi-pack-index: refactor "goto usage" pattern Ævar Arnfjörð Bjarmason
@ 2021-07-20 18:14     ` Taylor Blau
  0 siblings, 0 replies; 30+ messages in thread
From: Taylor Blau @ 2021-07-20 18:14 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak

On Tue, Jul 20, 2021 at 01:39:43PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 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 <avarab@gmail.com>
> ---
>  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]);

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 <avarab@gmail.com>
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 <me@ttaylorr.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 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


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v3 5/6] commit-graph: early exit to "usage" on !argc
  2021-07-20 11:39   ` [PATCH v3 5/6] commit-graph: early exit to "usage" on !argc Ævar Arnfjörð Bjarmason
@ 2021-07-20 18:17     ` Taylor Blau
  0 siblings, 0 replies; 30+ messages in thread
From: Taylor Blau @ 2021-07-20 18:17 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Derrick Stolee, Taylor Blau, Andrei Rybak

On Tue, Jul 20, 2021 at 01:39:44PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 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 <avarab@gmail.com>
> ---
>  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);

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"
  2021-07-20 17:55       ` SZEDER Gábor
@ 2021-07-20 18:24         ` Taylor Blau
  2021-07-20 21:17           ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 30+ messages in thread
From: Taylor Blau @ 2021-07-20 18:24 UTC (permalink / raw)
  To: SZEDER Gábor
  Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano,
	Derrick Stolee, Taylor Blau, Andrei Rybak

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:
> > On Tue, Jul 20, 2021 at 01:39:45PM +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.
> > >
> > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> > > ---
> > >  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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"
  2021-07-20 18:24         ` Taylor Blau
@ 2021-07-20 21:17           ` Ævar Arnfjörð Bjarmason
  2021-07-20 21:47             ` Taylor Blau
  0 siblings, 1 reply; 30+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-20 21:17 UTC (permalink / raw)
  To: Taylor Blau
  Cc: SZEDER Gábor, git, Junio C Hamano, Derrick Stolee, Andrei Rybak


On Tue, Jul 20 2021, Taylor Blau wrote:

> 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:
>> > On Tue, Jul 20, 2021 at 01:39:45PM +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.
>> > >
>> > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> > > ---
>> > >  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.


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"
  2021-07-20 21:17           ` Ævar Arnfjörð Bjarmason
@ 2021-07-20 21:47             ` Taylor Blau
  2021-07-21  7:26               ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 30+ messages in thread
From: Taylor Blau @ 2021-07-20 21:47 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: SZEDER Gábor, git, Junio C Hamano, Derrick Stolee, Andrei Rybak

On Tue, Jul 20, 2021 at 11:17:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
> > 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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"
  2021-07-20 21:47             ` Taylor Blau
@ 2021-07-21  7:26               ` Ævar Arnfjörð Bjarmason
  2021-07-21  8:08                 ` Jeff King
  0 siblings, 1 reply; 30+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-21  7:26 UTC (permalink / raw)
  To: Taylor Blau
  Cc: SZEDER Gábor, git, Junio C Hamano, Derrick Stolee, Andrei Rybak


On Tue, Jul 20 2021, Taylor Blau wrote:

> On Tue, Jul 20, 2021 at 11:17:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> > 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.

>> 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/

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"
  2021-07-21  7:26               ` Ævar Arnfjörð Bjarmason
@ 2021-07-21  8:08                 ` Jeff King
  2021-07-21 16:54                   ` Junio C Hamano
  0 siblings, 1 reply; 30+ messages in thread
From: Jeff King @ 2021-07-21  8:08 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Taylor Blau, SZEDER Gábor, git, Junio C Hamano,
	Derrick Stolee, Andrei Rybak

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage"
  2021-07-21  8:08                 ` Jeff King
@ 2021-07-21 16:54                   ` Junio C Hamano
  0 siblings, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2021-07-21 16:54 UTC (permalink / raw)
  To: Jeff King
  Cc: Ævar Arnfjörð Bjarmason, Taylor Blau,
	SZEDER Gábor, git, Derrick Stolee, Andrei Rybak

Jeff King <peff@peff.net> writes:

>> 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.

Let me second that ;-)  With extensions and even more drastic things
like sparse index entries, "pretend that the index is a flat list of
<mode, object, path>" is sometimes not useful in debugging (as bugs
and design mistakes might lie in the code that makes us pretend).

Even with packed objects, we still "pretetend that an object file is
a single line '<type> SP <len> NUL' followed by payload bytes,
deflated", as if packed objects do not exist.  I do not offhand
recall we have a good debugging option in the plumbing, or a
dedicated debugging tool, but because the format for packed objects
has been stable, we are probably OK.  Contrast to that, the index is
designed to be more ephemeral and its format is subject to change,
so we may in more need for a good debugging option.

> 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.

Yup

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2021-07-21 16:54 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-18  7:58 [PATCH v2 0/5] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
2021-07-18  7:58 ` [PATCH v2 1/5] commit-graph: define common usage with a macro Ævar Arnfjörð Bjarmason
2021-07-19 16:29   ` Taylor Blau
2021-07-18  7:58 ` [PATCH v2 2/5] commit-graph: remove redundant handling of -h Ævar Arnfjörð Bjarmason
2021-07-18 12:55   ` Andrei Rybak
2021-07-19 16:34     ` Taylor Blau
2021-07-18  7:58 ` [PATCH v2 3/5] commit-graph: use parse_options_concat() Ævar Arnfjörð Bjarmason
2021-07-19 16:50   ` Taylor Blau
2021-07-20 11:31     ` Ævar Arnfjörð Bjarmason
2021-07-18  7:58 ` [PATCH v2 4/5] commit-graph: early exit to "usage" on !argc Ævar Arnfjörð Bjarmason
2021-07-19 16:55   ` Taylor Blau
2021-07-18  7:58 ` [PATCH v2 5/5] commit-graph: show usage on "commit-graph [write|verify] garbage" Ævar Arnfjörð Bjarmason
2021-07-19 16:53   ` Taylor Blau
2021-07-20 11:39 ` [PATCH v3 0/6] commit-graph: usage fixes Ævar Arnfjörð Bjarmason
2021-07-20 11:39   ` [PATCH v3 1/6] commit-graph: define common usage with a macro Ævar Arnfjörð Bjarmason
2021-07-20 11:39   ` [PATCH v3 2/6] commit-graph: remove redundant handling of -h Ævar Arnfjörð Bjarmason
2021-07-20 11:39   ` [PATCH v3 3/6] commit-graph: use parse_options_concat() Ævar Arnfjörð Bjarmason
2021-07-20 11:39   ` [PATCH v3 4/6] multi-pack-index: refactor "goto usage" pattern Ævar Arnfjörð Bjarmason
2021-07-20 18:14     ` Taylor Blau
2021-07-20 11:39   ` [PATCH v3 5/6] commit-graph: early exit to "usage" on !argc Ævar Arnfjörð Bjarmason
2021-07-20 18:17     ` Taylor Blau
2021-07-20 11:39   ` [PATCH v3 6/6] commit-graph: show usage on "commit-graph [write|verify] garbage" Ævar Arnfjörð Bjarmason
2021-07-20 17:47     ` SZEDER Gábor
2021-07-20 17:55       ` SZEDER Gábor
2021-07-20 18:24         ` Taylor Blau
2021-07-20 21:17           ` Ævar Arnfjörð Bjarmason
2021-07-20 21:47             ` Taylor Blau
2021-07-21  7:26               ` Ævar Arnfjörð Bjarmason
2021-07-21  8:08                 ` Jeff King
2021-07-21 16:54                   ` Junio C Hamano

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git