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

On Sun, Oct 26, 2025 at 7:53 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, Oct 26, 2025 at 6:54 PM Lucas Seiki Oshiro
> <lucasseikioshiro@gmail.com> wrote:
> > +       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);
>     }

Or, even better, scope the strbuf just to the `case` branch which needs it:

    case FORMAT_KEYVALUE: {
        struct strbuf buf = STRBUF_INIT;
        quote_c_style(value, buf, NULL, 0);
        printf("%s=%s\n", key, buf->buf);
        strbuf_release(&buf);
        break;
     }