Re: [PATCH v2 3/5] commit-graph: use parse_options_concat()
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jul 20, 2021, 11:31 UTC
- Message-ID
- <8735s9w74r.fsf@evledraar.gmail.com>
- In-Reply-To
- <YPWtRd+523N1cWR/@nand.local>
On Mon, Jul 19 2021, Taylor Blau wrote:
Show 19 quoted lines
> 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.
Show 32 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).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 :)