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@xxxxxxxxxxxxxxxxxxxxxxx/ > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> > --- > 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,
