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

[Derrick Stolee <stolee@gmail.com>]

On 4/5/2021 1:44 PM, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>> On Mon, Apr 5, 2021 at 12:58 PM Tom Saeger <tom.saeger@oracle.com> wrote:
>>> On Mon, Apr 05, 2021 at 01:04:13PM +0000, Derrick Stolee via GitGitGadget wrote:
>>>> +const char *refspec_item_format(const struct refspec_item *rsi)
>>>> +{
>>>> +     static struct strbuf buf = STRBUF_INIT;
>>>> +
>>>> +     strbuf_reset(&buf);
>>>
>>> is this even needed?
>>
>> This is needed due to the `static` strbuf declaration (which is easy
>> to overlook).
>>
>>>> +     if (rsi->matching)
>>>> +             return ":";
>>>> +
>>>> +     if (rsi->negative)
>>>> +             strbuf_addch(&buf, '^');
>>>> +     else if (rsi->force)
>>>> +             strbuf_addch(&buf, '+');
>>>> +
>>>> +     if (rsi->src)
>>>> +             strbuf_addstr(&buf, rsi->src);
>>>> +
>>>> +     if (rsi->dst) {
>>>> +             strbuf_addch(&buf, ':');
>>>> +             strbuf_addstr(&buf, rsi->dst);
>>>> +     }
>>>> +
>>>> +     return buf.buf;
>>>
>>> should this be strbuf_detach?
>>
>> In normal circumstances, yes, however, with the `static` strbuf, this
>> is correct.
>>
>> However, a more significant question, perhaps, is why this is using a
>> `static` strbuf in the first place? Does this need to be optimized
>> because it is on a hot path? If not, then the only obvious reason why
>> `static` was chosen was that sometimes the function returns a string
>> literal and sometimes a constructed string. However, that's minor, and
>> it would feel cleaner to avoid the `static` strbuf altogether by using
>> strbuf_detach() for the constructed case and xstrdup() for the string
>> literal case, and making it the caller's responsibility to free the
>> result. (The comment in the header file would need to be updated to
>> say as much.)

Yes, we could get around the return of ":" very easily.

> Very good suggestion.  That would also make this codepath
> thread-safe (I do not offhand know how important that is, though).

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.

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.

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);

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.

Thanks,
-Stolee