Re: [PATCH 02/12] builtin/show-ref: split up different subcommands

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Oct 24, 2023 at 9:10 AM Patrick Steinhardt <ps@pks.im> wrote:
> While not immediately obvious, git-show-ref(1) actually implements three
> different subcommands:
>
>     - `git show-ref <patterns>` can be used to list references that
>       match a specific pattern.
>
>     - `git show-ref --verify <refs>` can be used to list references.
>       These are _not_ patterns.
>
>     - `git show-ref --exclude-existing` can be used as a filter that
>       reads references from standard input, performing some conversions
>       on each of them.
>
> Let's make this more explicit in the code by splitting up the three
> subcommands into separate functions. This also allows us to address the
> confusingly named `patterns` variable, which may hold either patterns or
> reference names.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> @@ -142,6 +142,53 @@ static int exclude_existing(const char *match)
> +static int cmd_show_ref__verify(const char **refs)
> +{
> +       if (!refs || !*refs)
> +               die("--verify requires a reference");
> +
> +       while (*refs) {
> +               struct object_id oid;
> +
> +               if ((starts_with(*refs, "refs/") || !strcmp(*refs, "HEAD")) &&
> +                   !read_ref(*refs, &oid)) {
> +                       show_one(*refs, &oid);
> +               }
> +               else if (!quiet)
> +                       die("'%s' - not a valid ref", *refs);
> +               else
> +                       return 1;
> +               refs++;
> +       }

A couple style-nits here caught my attention...

- "}" and "else" should be cuddled: `} else if`

- coding guidelines these days want braces on all branches if any
branch needs them

However, since this code is merely being relocated from elsewhere in
this file and since these style-nits were already present, moving the
code verbatim without correcting the style problems is more
reviewer-friendly. Okay.

> +       return 0;
> +}
> +
> +static int cmd_show_ref__patterns(const char **patterns)
> +{
> +       struct show_ref_data show_ref_data = {
> +               .patterns = (patterns && *patterns) ? patterns : NULL,
> +       };

Are we allowing non-constant initializers in the codebase? If not,
this should probably initialize .patterns to NULL and then
conditionally assign `patterns` separately in code below the
initializer.

> +       if (show_head)
> +               head_ref(show_ref, &show_ref_data);
> +       if (heads_only || tags_only) {
> +               if (heads_only)
> +                       for_each_fullref_in("refs/heads/", show_ref, &show_ref_data);
> +               if (tags_only)
> +                       for_each_fullref_in("refs/tags/", show_ref, &show_ref_data);
> +       } else {
> +               for_each_ref(show_ref, &show_ref_data);
> +       }
> +       if (!found_match) {
> +               if (verify && !quiet)
> +                       die("No match");
> +               return 1;
> +       }
> +
> +       return 0;
> +}