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

[Derrick Stolee <stolee@gmail.com>]

On 4/6/2021 11:23 AM, Eric Sunshine wrote:
> 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.

OK, convinced. I'll return a string that must be freed in my
next version. Thanks!

-Stolee