git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 3/5] commit-graph: use parse_options_concat()

From
Taylor Blau <me@ttaylorr.com>
Date
Jul 19, 2021, 16:50 UTC
Message-ID
<YPWtRd+523N1cWR/@nand.local>
In-Reply-To
<patch-3.5-2e7d9b0b8e4-20210718T074936Z-avarab@gmail.com>
On Sun, Jul 18, 2021 at 09:58:07AM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 8 quoted lines
> 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/
Show 23 quoted lines
> 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.

Show 20 quoted lines
> +	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,
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 8 of 40 in “commit-graph: usage fixes”
  1. 0/5 commit-graph: usage fixesÆvar Arnfjörð Bjarmason, Jul 18, 2021
  2. 2/5 commit-graph: remove redundant handling of -hÆvar Arnfjörð Bjarmason, Jul 18, 2021
  3. Andrei RybakJul 18, 2021
  4. Taylor BlauJul 19, 2021
  5. 1/5 commit-graph: define common usage with a macroÆvar Arnfjörð Bjarmason, Jul 18, 2021
  6. Taylor BlauJul 19, 2021
  7. 3/5 commit-graph: use parse_options_concat()Ævar Arnfjörð Bjarmason, Jul 18, 2021
  8. Taylor BlauJul 19, 2021
  9. Ævar Arnfjörð BjarmasonJul 20, 2021
  10. 4/5 commit-graph: early exit to "usage" on !argcÆvar Arnfjörð Bjarmason, Jul 18, 2021
  11. Taylor BlauJul 19, 2021
  12. 5/5 commit-graph: show usage on "commit-graph [write|verify] garbage"Ævar Arnfjörð Bjarmason, Jul 18, 2021
  13. Taylor BlauJul 19, 2021
  14. 0/6 commit-graph: usage fixesÆvar Arnfjörð Bjarmason, Jul 20, 2021
  15. 1/6 commit-graph: define common usage with a macroÆvar Arnfjörð Bjarmason, Jul 20, 2021
  16. 2/6 commit-graph: remove redundant handling of -hÆvar Arnfjörð Bjarmason, Jul 20, 2021
  17. 3/6 commit-graph: use parse_options_concat()Ævar Arnfjörð Bjarmason, Jul 20, 2021
  18. 4/6 multi-pack-index: refactor "goto usage" patternÆvar Arnfjörð Bjarmason, Jul 20, 2021
  19. Taylor BlauJul 20, 2021
  20. 6/6 commit-graph: show usage on "commit-graph [write|verify] garbage"Ævar Arnfjörð Bjarmason, Jul 20, 2021
  21. SZEDER GáborJul 20, 2021
  22. SZEDER GáborJul 20, 2021
  23. Taylor BlauJul 20, 2021
  24. Ævar Arnfjörð BjarmasonJul 20, 2021
  25. Taylor BlauJul 20, 2021
  26. Ævar Arnfjörð BjarmasonJul 21, 2021
  27. Jeff KingJul 21, 2021
  28. Junio C HamanoJul 21, 2021
  29. 5/6 commit-graph: early exit to "usage" on !argcÆvar Arnfjörð Bjarmason, Jul 20, 2021
  30. Taylor BlauJul 20, 2021
  31. 0/7 commit-graph: usage fixesÆvar Arnfjörð Bjarmason, Aug 23, 2021
  32. 1/7 commit-graph: define common usage with a macroÆvar Arnfjörð Bjarmason, Aug 23, 2021
  33. 2/7 commit-graph: remove redundant handling of -hÆvar Arnfjörð Bjarmason, Aug 23, 2021
  34. 3/7 commit-graph: use parse_options_concat()Ævar Arnfjörð Bjarmason, Aug 23, 2021
  35. 4/7 multi-pack-index: refactor "goto usage" patternÆvar Arnfjörð Bjarmason, Aug 23, 2021
  36. 5/7 commit-graph: early exit to "usage" on !argcÆvar Arnfjörð Bjarmason, Aug 23, 2021
  37. 6/7 commit-graph: show usage on "commit-graph [write|verify] garbage"Ævar Arnfjörð Bjarmason, Aug 23, 2021
  38. 7/7 commit-graph: show "unexpected subcommand" errorÆvar Arnfjörð Bjarmason, Aug 23, 2021
  39. Taylor BlauAug 30, 2021
  40. Junio C HamanoAug 30, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.