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

[Patrick Steinhardt <ps@pks.im>]

On Tue, Nov 19, 2024 at 10:22:31AM +0900, Junio C Hamano wrote:
> Bence Ferdinandy <bence@ferdinandy.com> writes:
> 
> > Subject: Re: [PATCH v13 2/9] refs: standardize output of refs_read_symbolic_ref
> 
> "output" -> "return values", or something.
> 
> > When the symbolic reference we want to read with refs_read_symbolic_ref
> > is actually not a symbolic reference, the files and the reftable
> > backends return different values (1 and -1 respectively). Standardize
> > the returned values so that 0 is success, -1 is a generic error and -2
> > is that the reference was actually non-symbolic.
> 
> Are all the existing callers OK with this switch from 1 to -2?
> 
> IOW, if a caller using the ref-files backend start behaving
> differently with this change upon seeing a return value of -2 (where
> it previously was expecting to see 1), that would not be nice.
> 
> Because "reftable was already broken" is not a good excuse to
> introduce a separate regression to ref-files users, we'd want to be
> careful if we want to do this kind of "while at it" change.

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.

I agree that this should be part of the commit message.

> > +/*
> > + * Return 0 if the symbolic reference could be read without error.
> > + * Return -1 for generic errors.
> > + * Return -2 if the reference was actually non-symbolic.
> > + */
> 
> As this is an implementation of ref_stroage_be.read_symbolic_ref,
> the above comment must stay in sync with the comment over there (and
> a comment before the corresponding function in the other backend).
> 
> I personally would not add the above comment for that reason, but as
> long as we are consistent, I am OK either way.  So if we add this,
> we should do the same to the reftable side as well.

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()`."

> By the way, it is arguable that it is an error when a caller asks
> "here is a ref, please read it as a symbolic ref" and the ref turns
> out to be not a symbolic ref.  I'd say that it is a valid view that
> the caller asked the question to find out if the ref was a symbolic
> before making the call, and "that ref is not symbolic" is one of the
> valid and expected results.  So if you wanted to change the value
> from 1 to -2 only because "you called read_symbolic_ref() without
> checking if it is a symbolic to begin with, which is your fault and
> you deserve to get an error", I am not sure if I agree with that
> view and reasoning.

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.

> In any case, this "not a symbolic ref" may want to have a symbolic
> constant.  With NOT_A_SYMREF being a non-error, you could structure
> the caller like so:

That'd be a good idea.

> 	status = read_symref()
> 	if (status < 0)
> 		... we got an error so we stop here ...
> 		... we do not behave differently on error type ...
> 
> 	switch (status) {
> 	case 0:
> 		... everything is peachy ...
> 		break ;;
> 	case NOT_A_SYMREF:
> 		... we want to handle non-symref here ...
> 		break ;;
> 	default:
> 		BUG("unhandled case");
> 	}
> 
> Also, even if we decide that we want to treat this as an error,
> lumping everything else as "generic error" might make it awkward to
> evolve the API, I suspect.

I guess most callers don't care about the exact error code, they only
care about whether or not they have been able to read the symref. I
think this idiom would easily be extensible in the future if all callers
know to check for `err < 0` by default, because introducing a new error
code would continue to work for them. And if they want to handle that
new error code they can adapt as you propose above with the switch.

Patrick