v2, 5 partscommit-graph: usage fixes

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

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

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

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

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

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

Range-diff against v1:
1:  742648756a5 ! 1:  0b0bb04ecf5 commit-graph: define common usage with a macro
    @@ Commit message
         information, see e.g. 809e0327f5 (builtin/commit-graph.c: introduce
         '--max-new-filters=<n>', 2020-09-18).
     
    +    See b25b727494f (builtin/multi-pack-index.c: define common usage with
    +    a macro, 2021-03-30) for another use of this pattern (but on-list this
    +    one came first).
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <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"

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

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

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

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

Yep, that matches my findings.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Here's something to consider squashing in on top:

--- >8 ---

On Mon, Jul 19 2021, Taylor Blau wrote:

Thanks.

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

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

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

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

Perhaps:

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

Thanks, Taylor

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

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

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

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

Keeping these makes sense.

Thanks, Taylor

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

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

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

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

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

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

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

    error: unrecognized subcommand: (null)

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

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

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

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

--- >8 ---

From: Ævar Arnfjörð Bjarmason <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(-)

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

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

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

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

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

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

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

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

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

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

Thanks, Taylor

On Tue, Jul 20 2021, Taylor Blau wrote:

I'm all for you picking it up :)

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

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

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

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

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

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

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

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

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?

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

Thanks, Taylor

On Tue, Jul 20 2021, Taylor Blau wrote:

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

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.

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

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

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

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.

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

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

Jeff King <peff@peff.net> writes:

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.

Yup

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

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

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

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

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

Thanks, Taylor

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Bring the "commit-graph" command in line with the error output and general pattern in cmd_multi_pack_index().

Let's test for that output, and also cover the same potential bug as was fixed in the multi-pack-index command in 88617d11f9d (multi-pack-index: fix potential segfault without sub-command, 2021-07-19).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/commit-graph.c  |  1 +
 t/t5318-commit-graph.sh | 16 +++++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

On Mon, Aug 23, 2021 at 02:30:14PM +0200, Ævar Arnfjörð Bjarmason wrote:

Thanks, and sorry for totally dropping this on the floor even though I said that I would pick it up. Since it had been a while, I re-read through the patches and they all look good to me.

    Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks, Taylor

Taylor Blau <me@ttaylorr.com> writes:

Thanks.