Re: [PATCH v13 2/9] refs: standardize output of refs_read_symbolic_ref

From: Patrick Steinhardt <ps@pks.im>
Date: Nov 19, 2024, 07:26 UTC
Message-ID: <Zzw9iqf5bcj5_lV8@pks.im>
In-Reply-To: <xmqqr077d88i.fsf@gitster.g>

On Tue, Nov 19, 2024 at 03:54:05PM +0900, Junio C Hamano wrote:

Show 11 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> > The reason why I've been proposing to return negative is because we have
> > the idiom of checking `err < 0` in many places, so a function that
> > returns a positive value in the case where it didn't return the expected
> > result can easily lead to bugs.
> 
> I agree with the general reasoning.  I am saying this may or may not
> be an error, and if it turns out that it is not an error but is just
> one of the generally expected outcome, treating it as an error and
> having "if (status < 0)" to lump the case together with other error
> cases may not be nice to the callers.

The question to me is whether the function returns something sensible in all non-error cases that a caller can use properly without having to explicitly check for the value. And I'd say that this is not the case with `refs_read_symbolic_ref()`, which wouldn't end up setting the value of `referent`.

So regardless of whether we define this as error or non-error, the caller would have to exlicitly handle the case where it's not a symref in order to make sense of it because the result is not well-defined.

Patrick