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

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 25, 2023, 11:50 UTC
Message-ID: <ZTkBFD7Iffe2bTOE@tanuki>
In-Reply-To: <CAPig+cQrD6uh65UzaKbwryv=wcdymKrjqXsAKgrKHYpQNWqSYQ@mail.gmail.com>

On Tue, Oct 24, 2023 at 02:48:14PM -0400, Eric Sunshine wrote:

Show 28 quoted lines

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

Yeah, we do. It's perfectly valid to pass `--exclude-existing` without the optional pattern argument. We still want to use this mode in that case, but don't populate the pattern.

An alternative would be to assign something like a sentinel value in here. But I'd think that it's clearer to instead have an explicit separate field for this.

Show 12 quoted lines

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

Yes, let's do it. It's been more of an oversight rather than intentional to keep the previous name.

Show 20 quoted lines

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

See the explanation above.

Patrick