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@google.com>
## 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@gmail.com>.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
+ Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
## 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@google.com>
+ Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
## 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@google.com>
+ Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
## 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@google.com>
## 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@google.com>
-
- ## 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@google.com>
## 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@google.com>
+ Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
## 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@google.com>
+ Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
## 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@google.com>
+ Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
## 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;Han-Wen Nienhuys via GitGitGadget <gitgitgadget@gmail.com>