| Message ID | pull.1011.git.git.1619173446857.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn | expand |
On Fri, Apr 23, 2021 at 10:24:06AM +0000, Han-Wen Nienhuys via GitGitGadget wrote: > Subject: refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn The subject says "read_raw_ref_fn", but the patch is touching refs_resolve_ref_unsafe(). The former is an abstract type, and I didn't dig to see the relationships, but I'll focus on the code change in the patch. > A grep for EINVAL */*c reveals that no code inspects EINVAL after reading > references. I don't think that's sufficient, for two reasons: - in general we try to be careful about forks and topics in flight, which might end up with semantic conflicts. So we don't necessarily assume that we can see all code, and prefer if any subtle changes like this at least result in a compile failure (e.g., changing function name or signature). In practice, this is balanced with how likely such code is, how bad the breakage would be, what we're gaining, etc. - just because they are not looking for EINVAL specifically doesn't mean they are not looking at errno at all (e.g., after calling refs_resolve_ref_unsafe(), lock_ref_oid_basic() does so). So we have to set errno to _something_ after the error. After your patch, we don't set it at all for these error returns, and so we'll be left with whatever junk was in errno from a previous unrelated syscall, which could be very misleading. Since we have to set it to something, EINVAL seems like a reasonable value. I certainly buy the argument that errno is a pretty lousy channel for passing back error data, for a number of reasons. If we were going all the way towards getting rid of errno in this function (and replacing it with something better, as we must, since some callers _do_ care about more detailed information), I could see the value. But this patch doesn't get us anywhere useful and risks regressions in the meantime. -Peff
On Fri, Apr 23, 2021 at 2:57 PM Jeff King <peff@peff.net> wrote: > > On Fri, Apr 23, 2021 at 10:24:06AM +0000, Han-Wen Nienhuys via GitGitGadget wrote: > > > Subject: refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn > > The subject says "read_raw_ref_fn", but the patch is touching > refs_resolve_ref_unsafe(). The former is an abstract type, and I didn't > dig to see the relationships, but I'll focus on the code change in the > patch. Well spotted. I reverted this part (I did glance over existing callers, and couldn't find anyone inspecting errno) > > A grep for EINVAL */*c reveals that no code inspects EINVAL after reading > > references. > > I don't think that's sufficient, for two reasons: > > - in general we try to be careful about forks and topics in flight, > which might end up with semantic conflicts. So we don't necessarily > assume that we can see all code, and prefer if any subtle changes > like this at least result in a compile failure (e.g., changing > function name or signature). In practice, this is balanced with how > likely such code is, how bad the breakage would be, what we're > gaining, etc. would you say this is warranted here? refs.h doesn't mention the word errno, so this behavior isn't documented at all. I also looked over the current callers of read_raw_ref, and outside of refs/*.c none seem to inspect errno. > - just because they are not looking for EINVAL specifically doesn't > mean they are not looking at errno at all (e.g., after calling > refs_resolve_ref_unsafe(), lock_ref_oid_basic() does so). So we have > to set errno to _something_ after the error. After your patch, we > don't set it at all for these error returns, and so we'll be left > with whatever junk was in errno from a previous unrelated syscall, > which could be very misleading. Since we have to set it to > something, EINVAL seems like a reasonable value. The function has several exit paths that don't set errno at all, so the result is kind of random anyway, but I can't see the code I don't have. I've updated the series, with some real progress to stamping out errno. Hope this pleases you better.
diff --git a/refs.c b/refs.c index 261fd82beb98..3179ebd71b2f 100644 --- a/refs.c +++ b/refs.c @@ -1705,7 +1705,6 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) || !refname_is_safe(refname)) { - errno = EINVAL; return NULL; } @@ -1765,7 +1764,6 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) || !refname_is_safe(refname)) { - errno = EINVAL; return NULL; } diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 467f4b3c936d..29728a339fed 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -617,11 +617,10 @@ typedef int reflog_expire_fn(struct ref_store *ref_store, * properly-formatted or even safe reference name. NEITHER INPUT NOR * OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION. * - * Return 0 on success. If the ref doesn't exist, set errno to ENOENT - * and return -1. If the ref exists but is neither a symbolic ref nor - * an object ID, it is broken; set REF_ISBROKEN in type, set errno to - * EINVAL, and return -1. If there is another error reading the ref, - * set errno appropriately and return -1. + * Return 0 on success. If the ref doesn't exist, set errno to ENOENT and return + * -1. If the ref exists but is neither a symbolic ref nor an object ID, it is + * broken; set REF_ISBROKEN in type, and return -1. If there is another error + * reading the ref, set errno appropriately and return -1. * * Backend-specific flags might be set in type as well, regardless of * outcome.