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

On Mon, Jul 19 2021, Taylor Blau wrote:

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

quoted

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

Thanks.

quoted

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 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 :)