v4 * commit msg tweaks in response to Jun. Han-Wen Nienhuys (8): refs: remove EINVAL errno output from specification of read_raw_ref_fn refs/files-backend: stop setting errno from lock_ref_oid_basic refs: make errno output explicit for read_raw_ref_fn refs: make errno output explicit for refs_resolve_ref_unsafe refs: use refs_resolve_ref_unsafe_with_errno() where needed refs: add failure_errno to refs_read_raw_ref() signature refs: clear errno return in refs_resolve_ref_unsafe() refs: explicitly propagate errno from refs_read_raw_ref refs.c | 51 +++++++++++++++++++++++-------------- refs/debug.c | 4 +-- refs/files-backend.c | 58 +++++++++++++++++++------------------------ refs/packed-backend.c | 16 ++++++------ refs/refs-internal.h | 31 +++++++++++++++-------- 5 files changed, 90 insertions(+), 70 deletions(-) base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1012%2Fhanwen%2Feinval-sideband-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1012/hanwen/einval-sideband-v2 Pull-Request: https://github.com/git/git/pull/1012 Range-diff vs v1: 1: 7e8181e77d40 ! 1: f9b92e62b598 refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn @@ Metadata Author: Han-Wen Nienhuys <hanwen@xxxxxxxxxx> ## Commit message ## - refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn + refs: remove EINVAL errno output from specification of read_raw_ref_fn - A grep for EINVAL */*c reveals that no code inspects EINVAL after reading - references. + This commit does not change code; it documents the fact that an alternate ref + backend does not need to return EINVAL from read_raw_ref_fn to function + properly. - The files ref backend does use EINVAL so parse_loose_ref_contents() can - communicate to lock_raw_ref() about garbage following the hex SHA1, or a short - read in files_read_raw_ref(), but the files backend does not call into - refs_read_raw_ref(), so its EINVAL sideband error is unused. + This is correct, because refs_read_raw_ref is only called from; + + * resolve_ref_unsafe(), which does not care for the EINVAL errno result. + + * refs_verify_refname_available(), which does not inspect errno. + + * files-backend.c, where errno is overwritten on failure. + + * packed-backend.c (is_packed_transaction_needed), which calls it for the + packed ref backend, which never emits EINVAL. + + A grep for EINVAL */*c reveals that no code checks errno against EINVAL after + reading references. In addition, the refs.h file does not mention errno at all. + + A grep over resolve_ref_unsafe() turned up the following callers that inspect + errno: + + * sequencer.c::print_commit_summary, which uses it for die_errno + + * lock_ref_oid_basic(), which only treats EISDIR and ENOTDIR specially. + + The files ref backend does use EINVAL. The files backend does not call into + the generic API (refs_read_raw), but into the files-specific function + (files_read_raw_ref), which we are not changing in this commit. As the errno sideband is unintuitive and error-prone, remove EINVAL value, as a step towards getting rid of the errno sideband altogether. @@ Commit message Spotted by Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>. Signed-off-by: Han-Wen Nienhuys <hanwen@xxxxxxxxxx> + Reviewed-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx> ## refs/refs-internal.h ## @@ refs/refs-internal.h: 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 + * 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. ++ * an object ID, it is broken; set REF_ISBROKEN in type, and return -1 ++ * (errno should not be ENOENT) 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. 2: b2c72097e5e8 ! 2: cbe09a48036c refs/files-backend: stop setting errno from lock_ref_oid_basic @@ Metadata ## Commit message ## refs/files-backend: stop setting errno from lock_ref_oid_basic - Errno is a global variable written by almost all system calls, and therefore it - is hard to reason about its state. It's also useless for user-visible errors, as - it leaves no place to report the offending file and/or syscall. + refs/files-backend.c::lock_ref_oid_basic() tries to signal how it failed + to its callers using errno. - For the copy/rename support, calls to lock_ref_oid_basic() in this file are - followed by: + It is safe to stop setting errno here, because the callers of this + file-scope static function are - * lock_ref_oid_basic (copy/rename rollback error path) + * files_copy_or_rename_ref() + * files_create_symref() + * files_reflog_expire() - * write_ref_to_lockfile (both in the rollback path and the success path of - copy/rename) + None of them looks at errno after seeing a negative return from + lock_ref_oid_basic() to make any decision, and no caller of these three + functions looks at errno after they signal a failure by returning a + negative value. In particular, - These calls do not inspect the incoming errno. As they perform I/O, they can - clobber errno. For this reason, callers cannot reliably observe the errno that - lock_ref_oid_basic() generated, so it is unsound for programmatic use. + * files_copy_or_rename_ref() - here, calls are followed by error() + (which performs I/O) or write_ref_to_lockfile() (which calls + parse_object() which may perform I/O) - For files_create_symref() and files_reflog_expire(), grepping over callers - showed no callers inspecting errno. + * files_create_symref() - here, calls are followed by error() or + create_symref_locked() (which performs I/O and does not inspect + errno) + + * files_reflog_expire() - here, calls are followed by error() or + refs_reflog_exists() (which calls a function in a vtable that is not + documented to use and/or preserve errno) Signed-off-by: Han-Wen Nienhuys <hanwen@xxxxxxxxxx> + Reviewed-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx> ## refs/files-backend.c ## @@ refs/files-backend.c: static int create_reflock(const char *path, void *cb) 3: ebd7b8380bf7 ! 3: 3e2831e59c8e refs: make errno output explicit for read_raw_ref_fn @@ Commit message relevant. Signed-off-by: Han-Wen Nienhuys <hanwen@xxxxxxxxxx> + Reviewed-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx> ## refs.c ## @@ refs.c: int refs_read_raw_ref(struct ref_store *ref_store, @@ refs.c: int refs_read_raw_ref(struct ref_store *ref_store, - return ref_store->be->read_raw_ref(ref_store, refname, oid, referent, - type); ++ failure = 0; + result = ref_store->be->read_raw_ref(ref_store, refname, oid, referent, + type, &failure); + errno = failure; @@ refs/debug.c: debug_ref_iterator_begin(struct ref_store *ref_store, const char * { struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store; int res = 0; - - oidcpy(oid, &null_oid); +@@ refs/debug.c: static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname, + oidcpy(oid, null_oid()); + errno = 0; res = drefs->refs->be->read_raw_ref(drefs->refs, refname, oid, referent, - type); + type, failure_errno); @@ refs/refs-internal.h: 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, 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 +- * (errno should not be ENOENT) 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 failure_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. For the files -+ * backend, EISDIR and ENOTDIR may be set if the ref name is a directory. If -+ * there is another error reading the ref, set failure_errno appropriately and -+ * return -1. ++ * ID, it is broken; set REF_ISBROKEN in type, and return -1 (failure_errno ++ * should not be ENOENT). The files backend may return EISDIR (if the ref name ++ * is a directory) and ENOTDIR (if a ref prefix is not a directory). If there is ++ * another error reading the ref, set failure_errno appropriately and return -1. * * Backend-specific flags might be set in type as well, regardless of * outcome. -: ------------ > 4: 11b2184044d7 refs: make errno output explicit for refs_resolve_ref_unsafe 4: dd3eceade4fc ! 5: 005ee8e6fb2a refs: make errno output explicit for refs_resolve_ref_unsafe @@ Metadata Author: Han-Wen Nienhuys <hanwen@xxxxxxxxxx> ## Commit message ## - refs: make errno output explicit for refs_resolve_ref_unsafe + refs: use refs_resolve_ref_unsafe_with_errno() where needed - This introduces refs_resolve_ref_unsafe_with_errno(), which makes the API - contract for the errno output explicit. The implementation still relies on - the global errno variable to ensure no side effects. + lock_ref_oid_basic() in files-backend.c is the only caller of refs_resolve_ref() + that needs error information to make logic decisions. Signed-off-by: Han-Wen Nienhuys <hanwen@xxxxxxxxxx> - - ## refs.c ## -@@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs, - return NULL; - } - -+const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs, -+ const char *refname, -+ int resolve_flags, -+ struct object_id *oid, -+ int *flags, int *failure_errno) -+{ -+ const char *result = refs_resolve_ref_unsafe(refs, refname, -+ resolve_flags, oid, flags); -+ *failure_errno = errno; -+ return result; -+} -+ - /* backend functions */ - int refs_init_db(struct strbuf *err) - { - - ## refs.h ## -@@ refs.h: const char *refs_resolve_ref_unsafe(struct ref_store *refs, - int resolve_flags, - struct object_id *oid, - int *flags); -+ - const char *resolve_ref_unsafe(const char *refname, int resolve_flags, - struct object_id *oid, int *flags); - + Reviewed-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx> ## refs/files-backend.c ## @@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, int mustexist = (old_oid && !is_null_oid(old_oid)); int resolve_flags = RESOLVE_REF_NO_RECURSE; int resolved; -+ int resolve_errno; ++ int resolve_errno = 0; files_assert_main_repository(refs, "lock_ref_oid_basic"); assert(err); @@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_re goto error_return; } - - ## refs/refs-internal.h ## -@@ refs/refs-internal.h: int refs_read_raw_ref(struct ref_store *ref_store, - const char *refname, struct object_id *oid, - struct strbuf *referent, unsigned int *type); - -+/* Like refs_resolve_ref_unsafe, but provide access to errno code that lead to a -+ * failure. */ -+const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs, -+ const char *refname, -+ int resolve_flags, -+ struct object_id *oid, -+ int *flags, int *failure_errno); -+ - /* - * Write an error to `err` and return a nonzero value iff the same - * refname appears multiple times in `refnames`. `refnames` must be 5: 039fc4be4b90 ! 6: 2b346caf1aed refs: add failure_errno to refs_read_raw_ref() signature @@ Commit message refs: add failure_errno to refs_read_raw_ref() signature This makes the errno output of refs_read_raw_ref explicit. - lock_raw_ref() now explicitly reads the errno output of refs_read_raw_ref. Signed-off-by: Han-Wen Nienhuys <hanwen@xxxxxxxxxx> + Reviewed-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx> ## refs.c ## @@ refs.c: static int refs_read_special_head(struct ref_store *ref_store, @@ refs.c: static int refs_read_special_head(struct ref_store *ref_store, + unsigned int *type, int *failure_errno) { - int result, failure; ++ if (failure_errno) ++ *failure_errno = 0; if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) { return refs_read_special_head(ref_store, refname, oid, referent, type); } +- failure = 0; - result = ref_store->be->read_raw_ref(ref_store, refname, oid, referent, - type, &failure); - errno = failure; 6: 1bb350ea5d21 ! 7: d86516219689 refs: clear errno return in refs_resolve_ref_unsafe() @@ Commit message This is done in a separate commit, to pinpoint the precise cause should there be regressions in error reporting. + This is implemented by renaming the existing logic to a static function + refs_resolve_unsafe_implicit_errno(), minimizing the code diff. + Signed-off-by: Han-Wen Nienhuys <hanwen@xxxxxxxxxx> + Reviewed-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx> ## refs.c ## @@ refs.c: int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, @@ refs.c: int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, - const char *refname, - int resolve_flags, - struct object_id *oid, int *flags) -+static const char *refs_resolve_ref_unsafe_errno(struct ref_store *refs, -+ const char *refname, -+ int resolve_flags, -+ struct object_id *oid, -+ int *flags) ++static const char * ++refs_resolve_ref_unsafe_implicit_errno(struct ref_store *refs, ++ const char *refname, int resolve_flags, ++ struct object_id *oid, int *flags) { static struct strbuf sb_refname = STRBUF_INIT; struct object_id unused_oid; @@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs, + int resolve_flags, struct object_id *oid, + int *flags) +{ -+ const char *result = refs_resolve_ref_unsafe_errno( ++ const char *result = refs_resolve_ref_unsafe_implicit_errno( + refs, refname, resolve_flags, oid, flags); + errno = 0; + return result; @@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs, { - const char *result = refs_resolve_ref_unsafe(refs, refname, - resolve_flags, oid, flags); -+ const char *result = refs_resolve_ref_unsafe_errno( ++ const char *result = refs_resolve_ref_unsafe_implicit_errno( + refs, refname, resolve_flags, oid, flags); *failure_errno = errno; return result; 7: 95d64d73353d < -: ------------ refs: stop setting EINVAL and ELOOP in symref resolution 8: 9e161eeb5f6b ! 8: 2a9ebe43deac refs: explicitly propagate errno from refs_read_raw_ref @@ Commit message refs_read_raw_ref(). Signed-off-by: Han-Wen Nienhuys <hanwen@xxxxxxxxxx> + Reviewed-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx> ## refs.c ## @@ refs.c: int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, @@ refs.c: int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, } -/* This function needs to return a meaningful errno on failure */ --static const char *refs_resolve_ref_unsafe_errno(struct ref_store *refs, -- const char *refname, -- int resolve_flags, -- struct object_id *oid, -- int *flags) +-static const char * +-refs_resolve_ref_unsafe_implicit_errno(struct ref_store *refs, +- const char *refname, int resolve_flags, +- struct object_id *oid, int *flags) +const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs, + const char *refname, + int resolve_flags, @@ refs.c: int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, { static struct strbuf sb_refname = STRBUF_INIT; struct object_id unused_oid; -@@ refs.c: static const char *refs_resolve_ref_unsafe_errno(struct ref_store *refs, +@@ refs.c: refs_resolve_ref_unsafe_implicit_errno(struct ref_store *refs, flags = &unused_flags; *flags = 0; @@ refs.c: static const char *refs_resolve_ref_unsafe_errno(struct ref_store *refs, if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) || -@@ refs.c: static const char *refs_resolve_ref_unsafe_errno(struct ref_store *refs, + !refname_is_safe(refname)) { +- errno = EINVAL; ++ *failure_errno = EINVAL; + return NULL; + } + +@@ refs.c: refs_resolve_ref_unsafe_implicit_errno(struct ref_store *refs, &read_flags, &read_failure)) { *flags |= read_flags; @@ refs.c: static const char *refs_resolve_ref_unsafe_errno(struct ref_store *refs, /* In reading mode, refs must eventually resolve */ if (resolve_flags & RESOLVE_REF_READING) return NULL; +@@ refs.c: refs_resolve_ref_unsafe_implicit_errno(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; ++ *failure_errno = EINVAL; + return NULL; + } + +@@ refs.c: refs_resolve_ref_unsafe_implicit_errno(struct ref_store *refs, + } + } + +- errno = ELOOP; ++ *failure_errno = ELOOP; + return NULL; + } + @@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname, int resolve_flags, struct object_id *oid, int *flags) { -- const char *result = refs_resolve_ref_unsafe_errno( +- const char *result = refs_resolve_ref_unsafe_implicit_errno( - refs, refname, resolve_flags, oid, flags); - errno = 0; - return result; @@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char * - struct object_id *oid, - int *flags, int *failure_errno) -{ -- const char *result = refs_resolve_ref_unsafe_errno( +- const char *result = refs_resolve_ref_unsafe_implicit_errno( - refs, refname, resolve_flags, oid, flags); - *failure_errno = errno; - return result; -- gitgitgadget
