| Message ID | patch-3.5-2e7d9b0b8e4-20210718T074936Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | commit-graph: usage fixes | expand |
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,
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 :)
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,
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(-)