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

From: Junio C Hamano <[email-masked]>
Date: Tue, 19 Nov 2024 15:54:05 +0900

Patrick Steinhardt <[email-masked]> writes:

> There are only three callers:
>
>   - "remote.c:ignore_symref_update()" only cares whether the return
>     value is 0 or not.
>
>   - "builtin/remote.c:mv()" is the same.
>
>   - "refs.c:migrate_one_ref()" assumes the behaviour of the reftable
>     backend and only checks for negative error codes.
>
> So you could argue that it's the "files" backend that is broken, not the
> "reftable" backend. Doesn't matter ultimately though, the real problem
> is that this wasn't ever documented anywhere.

You're correct that it does not matter ultimately.  But as a general
rule, which also applies here, anything newly introduced one does
differently without having a good reason is a bug in the new thing,
I would say.

> Another solution could be to have the comment in "refs.h" for the
> caller-facing function, while the backend pointer simply says "Please
> refer to the documentation of `refs_read_symbolic_ref()`."

Yup, avoiding unnecessary duplication is a good thing.

> 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.