Re: [PATCH 05/12] builtin/show-ref: refactor `--exclude-existing` options
From: Eric Sunshine <hidden>
Date: 2023-10-24 18:48:27
On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt [off-list ref] wrote:
It's not immediately obvious options which options are applicable to what subcommand ni git-show-ref(1) because all options exist as global
s/ni/in/
state. This can easily cause confusion for the reader. Refactor options for the `--exclude-existing` subcommand to be contained in a separate structure. This structure is stored on the stack and passed down as required. Consequentially, it clearly delimits the scope
s/Consequentially/Consequently/
quoted hunk ↗ jump to hunk
of those options and requires the reader to worry less about global state. Signed-off-by: Patrick Steinhardt <redacted> ---diff --git a/builtin/show-ref.c b/builtin/show-ref.c@@ -95,6 +94,11 @@ static int add_existing(const char *refname, +struct exclude_existing_options { + int enabled; + const char *pattern; +};
Do we need this `enabled` flag? Can't the same be achieved by checking whether `pattern` is NULL or not (see below)?
quoted hunk ↗ jump to hunk
@@ -104,11 +108,11 @@ static int add_existing(const char *refname, -static int cmd_show_ref__exclude_existing(const char *match) +static int cmd_show_ref__exclude_existing(const struct exclude_existing_options *opts)
Since you're renaming `match` to `opts->pattern`...
{
- int matchlen = match ? strlen(match) : 0;
+ int matchlen = opts->pattern ? strlen(opts->pattern) : 0;... and since you're touching this line anyway, maybe it makes sense to rename `matchlen` to `patternlen`?
quoted hunk ↗ jump to hunk
@@ -124,11 +128,11 @@ static int cmd_show_ref__exclude_existing(const char *match) - if (strncmp(ref, match, matchlen)) + if (strncmp(ref, opts->pattern, matchlen))
Especially since, as shown in this context, `matchlen` is really the length of the _pattern_, not the length of the resulting _match_.
quoted hunk ↗ jump to hunk
@@ -200,44 +204,46 @@ static int hash_callback(const struct option *opt, const char *arg, int unset) int cmd_show_ref(int argc, const char **argv, const char *prefix) { [...] - if (exclude_arg) - return cmd_show_ref__exclude_existing(exclude_existing_arg); + if (exclude_existing_opts.enabled) + return cmd_show_ref__exclude_existing(&exclude_existing_opts);
(continued from above) Can't this be handled without a separate `enabled` flag?
if (exclude_existing_opts.pattern)
...