Patrick Steinhardt <ps@xxxxxx> 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.
