From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sun, 26 Oct 2025 23:53:38 GMT
Subject: Re: [PATCH v3 1/2] repo: factor out field printing to dedicated function
Message-ID: <CAPig+cQO4_T8K-8wFBDQN-n+rasBF7LR+vJ6ez8swfmDz1ossg@mail.gmail.com>
In-Reply-To: <20251026225409.46647-2-lucasseikioshiro@gmail.com>

On Sun, Oct 26, 2025 at 6:54 PM Lucas Seiki Oshiro
<lucasseikioshiro@gmail.com> wrote:
> Move the field printing in git-repo-info to a new function called
> `print_field`, allowing it to be called by functions other than
> `print_fields`.
>
> Signed-off-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com>
> ---
> diff --git a/builtin/repo.c b/builtin/repo.c
> @@ -77,6 +77,24 @@ static get_value_fn *get_value_fn_for_key(const char *key)
> +static void print_field(enum output_format format, const char *key,
> +                       struct strbuf *valbuf, struct strbuf *quotbuf)
> +{

Let's not pass in 'valbuf' as a 'struct strbuf *' since doing so gives
the false impression that this function will be modifying the strbuf
(which it does not do). Instead, pass in the narrower `const char
*value` which indicates clearly that this function will not be
modifying the value.

> +       strbuf_reset(quotbuf);
> +
> +       switch (format) {
> +       case FORMAT_KEYVALUE:
> +               quote_c_style(valbuf->buf, quotbuf, NULL, 0);
> +               printf("%s=%s\n", key, quotbuf->buf);
> +               break;
> +       case FORMAT_NUL_TERMINATED:
> +               printf("%s\n%s%c", key, valbuf->buf, '\0');
> +               break;
> +       default:
> +               BUG("not a valid output format: %d", format);
> +       }
> +}

Moreover, I'd also say that since this is not on a critical path, you
should avoid the premature optimization of passing in `strfbuf
*quotebuf` and instead make `quotebuf` local to this function.

    static void print_field(enum output_format format,
        const char *key, const char *value)
    {
        struct strbuf quotbuf = STRBUF_INIT;
        ...stuff...
        strbuf_release(&quotbuf);
    }

>  static int print_fields(int argc, const char **argv,
>                         struct repository *repo,
>                         enum output_format format)
> @@ -97,21 +115,8 @@ static int print_fields(int argc, const char **argv,
>                 }
>
>                 strbuf_reset(&valbuf);
> -               strbuf_reset(&quotbuf);
> -
>                 get_value(repo, &valbuf);
> -
> -               switch (format) {
> -               case FORMAT_KEYVALUE:
> -                       quote_c_style(valbuf.buf, &quotbuf, NULL, 0);
> -                       printf("%s=%s\n", key, quotbuf.buf);
> -                       break;
> -               case FORMAT_NUL_TERMINATED:
> -                       printf("%s\n%s%c", key, valbuf.buf, '\0');
> -                       break;
> -               default:
> -                       BUG("not a valid output format: %d", format);
> -               }
> +               print_field(format, key, &valbuf, &quotbuf);
>         }
>
>         strbuf_release(&valbuf);