Re: [PATCH 3/5] refspec: output a refspec item

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Apr 6, 2021 at 7:21 AM Derrick Stolee <stolee@gmail.com> wrote:
> I was not intending to make this re-entrant/thread safe. The intention
> was to make it easy to consume the formatted string into output such
> as a printf without needing to store a temporary 'char *' and free() it
> afterwards. This ensures that the only lost memory over the life of the
> process is at most one buffer. At minimum, these are things that could
> be part of the message to justify this design.

This has the failing that it won't work if someone calls it twice in
the same printf() or calls it again before even consuming the first
returned value, so this fails:

    printf("foo: %s\nbar: %s\n",
        refspec_item_format(...),
        refspec_item_format(...));

as does this:

    const char *a = refspec_item_format(...);
    const char *b = refspec_item_format(...);

Historically this project would "work around" that problem by using
rotating static buffers in the function, but we've mostly been moving
away from that for several reasons (can't predict how many buffers
will be needed, re-entrancy, etc.).

> So, I'm torn. This seems like a case where there is value in having
> the return buffer be "owned" by this method, and the expected
> consumers will use the buffer before calling it again. I'm not sure
> how important it is to do this the other way.

If history is any indication, we'd probably end up moving away from
such an API eventually anyhow.

> Would it be sufficient to justify this choice in the commit message
> and comment about it in the method declaration? Or is it worth adding
> this templating around every caller:
>
>         char *buf = refspec_item_format(rsi);
>         ...
>         <use 'buf'>
>         ...
>         free(buf);

An alternative would be to have the caller pass in a strbuf to be
populated by the function. It doesn't reduce the boilerplate needed by
the caller (still need to create and release the strbuf), but may
avoid some memory allocations. But if this isn't a critical path and
won't likely ever be, then passing in strbuf may be overkill.

> I don't need much convincing to do this, but I hadn't properly
> described my opinion before. Just a small nudge would convince me to
> do it this way.

For the reasons described above and earlier in the thread, avoiding
the static buffer seems the best course of action.