Re: [PATCH 05/12] builtin/show-ref: refactor `--exclude-existing` options

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 24, 2023, 18:48 UTC
Message-ID: <CAPig+cQrD6uh65UzaKbwryv=wcdymKrjqXsAKgrKHYpQNWqSYQ@mail.gmail.com>
In-Reply-To: <8d0b0b5700c7ffed6b3a74760d0d9155b404bb4f.1698152926.git.ps@pks.im>

On Tue, Oct 24, 2023 at 9:11 AM Patrick Steinhardt <ps@pks.im> 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/

Show 5 quoted lines

> 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/

Show 11 quoted lines

> of those options and requires the reader to worry less about global
> state.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> 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)?

> @@ -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`?

> @@ -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_.

Show 8 quoted lines

> @@ -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)
        ...