Re: [PATCH v3 1/3] worktree: refactor infer_backlink() to use *strbuf

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 10, 2024, 19:14 UTC
Message-ID: <xmqqmsjbkbww.fsf@gitster.g>
In-Reply-To: <e1RptKNShhPZHXDhBkQBaCNCmKBKik4nQzRShqtgVfjcH7vBWpuLYV60PSHaJ0diX-oG3XiKHc7IebhIZM4eSkeYdQQZ_QYK2ixxsK-XwrE=@pm.me>

Caleb White <cdwhite3@pm.me> writes:

Show 7 quoted lines

>> Question here: should we use `strbuf_reset` here? I want to know the
>> reason why you design this. Is the caller's responsibility to clear the
>> "inferred" when calling this function?
>
> Yes we should, sure it is the caller's responsibility but this just helps
> prevent bugs. There's plenty of functions that reset the strbuf that's
> passed to the function before modifying it.

If the API says that it _appends_ what it found in the supplied strbuf, then it is the caller's responsibility to ensure that the strbuf being passed is empty, if the caller wants the resulting strbuf to hold only what the function found.

If the API says it _returns_ what it found in the supplied strbuf, then the function is responsible to discard what the supplied strbuf originally has.

As an API design, if it is rarely useful for callers to be able to append, then the latter is simpler to use, but there are many cases where the "append" semantics is useful (see strbuf.c to find examples).

In this particular case, it is rarely useful for the caller to get the result appended to a string it already has before calling this function, so unconditionally discarding what the caller had, without telling the caller that it is responsible for passing an empty buffer, is the right thing, I wuld think.

Thanks.