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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 10, 2024, 20:03 UTC
Message-ID: <xmqqcyk7k9nx.fsf@gitster.g>
In-Reply-To: <J1frhnJqftB1IUvIGDnJiz6uYTDNl_Bc92NLj1gXJtgRxapI3Uy-fyFHYtv5_p8e7yzF0pegawV6NlHQTn0GHcXP8MZ_1X8CNkIvO3_MP9c=@pm.me>

Caleb White <cdwhite3@pm.me> writes:

Show 7 quoted lines

>> If so, why not just return "infer_backlink.len"?
>
> I would say because the purpose of the return is a boolean,
> either the call was successful or it wasn't. The developer
> knowledge that you speak of should be a given---if the
> function returned true then there's obviously a path
> in the strbuf.

That does not say what should be left in the strbuf when the request failed. If it is undefined, then using its .len (or .buf) is not quite right, without relying on too much implementation details of the called function.

Usually, this project uses 0 to signal a success, while errors are signalled by returning a negative value (and if there can be multiple ways to fail, such a design leaves different negative values as a possibility).

If the result the caller finds useful is always positive value (or non-negative value), however, it is perfectly fine and often more convenient if you used the "positive (or non-negative) value gives the intended result and signals a success, negative (or non-positive) value signals an error" convention. The caller can

    if (0 < do_the_thing())
	; /* success */

just fine, instead of the boolean "0 is success, negative values are various errors",

    if (!do_the_thing())
	; /* success */

when it does not care how/why the thing failed.