git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/8] refs: cleanup errno sideband ref related functions
@ 2021-04-29 15:32 Han-Wen Nienhuys via GitGitGadget
  2021-04-29 15:32 ` [PATCH 1/8] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
                   ` (9 more replies)
  0 siblings, 10 replies; 40+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-04-29 15:32 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys

v3

 * remove errno as an implicit communication mechanism from refs support
   completely.

Han-Wen Nienhuys (8):
  refs: remove EINVAL specification from the errno sideband in
    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: add failure_errno to refs_read_raw_ref() signature
  refs: clear errno return in refs_resolve_ref_unsafe()
  refs: stop setting EINVAL and ELOOP in symref resolution
  refs: explicitly propagate errno from refs_read_raw_ref

 refs.c                | 46 ++++++++++++++++++++--------------
 refs.h                |  1 +
 refs/debug.c          |  4 +--
 refs/files-backend.c  | 58 +++++++++++++++++++------------------------
 refs/packed-backend.c | 16 ++++++------
 refs/refs-internal.h  | 31 +++++++++++++++--------
 6 files changed, 86 insertions(+), 70 deletions(-)


base-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1012%2Fhanwen%2Feinval-sideband-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1012/hanwen/einval-sideband-v1
Pull-Request: https://github.com/git/git/pull/1012
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH 1/8] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn
  2021-04-29 15:32 [PATCH 0/8] refs: cleanup errno sideband ref related functions Han-Wen Nienhuys via GitGitGadget
@ 2021-04-29 15:32 ` Han-Wen Nienhuys via GitGitGadget
  2021-04-30  2:38   ` Junio C Hamano
  2021-06-03  2:19   ` Jonathan Tan
  2021-04-29 15:32 ` [PATCH 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic Han-Wen Nienhuys via GitGitGadget
                   ` (8 subsequent siblings)
  9 siblings, 2 replies; 40+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-04-29 15:32 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys

From: Han-Wen Nienhuys <hanwen@google.com>

A grep for EINVAL */*c reveals that no code inspects EINVAL after reading
references.

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.

As the errno sideband is unintuitive and error-prone, remove EINVAL
value, as a step towards getting rid of the errno sideband altogether.

Spotted by Ævar Arnfjörð Bjarmason <avarab@gmail.com>.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 refs/refs-internal.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

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.
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic
  2021-04-29 15:32 [PATCH 0/8] refs: cleanup errno sideband ref related functions Han-Wen Nienhuys via GitGitGadget
  2021-04-29 15:32 ` [PATCH 1/8] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
@ 2021-04-29 15:32 ` Han-Wen Nienhuys via GitGitGadget
  2021-04-30  3:10   ` Junio C Hamano
  2021-06-03  2:33   ` Jonathan Tan
  2021-04-29 15:32 ` [PATCH 3/8] refs: make errno output explicit for read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
                   ` (7 subsequent siblings)
  9 siblings, 2 replies; 40+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-04-29 15:32 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys

From: Han-Wen Nienhuys <hanwen@google.com>

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.

For the copy/rename support, calls to lock_ref_oid_basic() in this file are
followed by:

* lock_ref_oid_basic (copy/rename rollback error path)

* write_ref_to_lockfile (both in the rollback path and the success path of
  copy/rename)

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.

For files_create_symref() and files_reflog_expire(), grepping over callers
showed no callers inspecting errno.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 refs/files-backend.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 119972ee16f8..c9511da1d387 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -910,7 +910,6 @@ static int create_reflock(const char *path, void *cb)
 
 /*
  * Locks a ref returning the lock on success and NULL on failure.
- * On failure errno is set to something meaningful.
  */
 static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 					   const char *refname,
@@ -922,7 +921,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 {
 	struct strbuf ref_file = STRBUF_INIT;
 	struct ref_lock *lock;
-	int last_errno = 0;
 	int mustexist = (old_oid && !is_null_oid(old_oid));
 	int resolve_flags = RESOLVE_REF_NO_RECURSE;
 	int resolved;
@@ -949,7 +947,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 		 * to remain.
 		 */
 		if (remove_empty_directories(&ref_file)) {
-			last_errno = errno;
 			if (!refs_verify_refname_available(
 					    &refs->base,
 					    refname, extras, skip, err))
@@ -962,7 +959,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 						     &lock->old_oid, type);
 	}
 	if (!resolved) {
-		last_errno = errno;
+		int last_errno = errno;
 		if (last_errno != ENOTDIR ||
 		    !refs_verify_refname_available(&refs->base, refname,
 						   extras, skip, err))
@@ -981,20 +978,17 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 	if (is_null_oid(&lock->old_oid) &&
 	    refs_verify_refname_available(refs->packed_ref_store, refname,
 					  extras, skip, err)) {
-		last_errno = ENOTDIR;
 		goto error_return;
 	}
 
 	lock->ref_name = xstrdup(refname);
 
 	if (raceproof_create_file(ref_file.buf, create_reflock, &lock->lk)) {
-		last_errno = errno;
 		unable_to_lock_message(ref_file.buf, errno, err);
 		goto error_return;
 	}
 
 	if (verify_lock(&refs->base, lock, old_oid, mustexist, err)) {
-		last_errno = errno;
 		goto error_return;
 	}
 	goto out;
@@ -1005,7 +999,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 
  out:
 	strbuf_release(&ref_file);
-	errno = last_errno;
 	return lock;
 }
 
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH 3/8] refs: make errno output explicit for read_raw_ref_fn
  2021-04-29 15:32 [PATCH 0/8] refs: cleanup errno sideband ref related functions Han-Wen Nienhuys via GitGitGadget
  2021-04-29 15:32 ` [PATCH 1/8] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
  2021-04-29 15:32 ` [PATCH 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic Han-Wen Nienhuys via GitGitGadget
@ 2021-04-29 15:32 ` Han-Wen Nienhuys via GitGitGadget
  2021-04-30  3:34   ` Junio C Hamano
  2021-06-03  2:37   ` Jonathan Tan
  2021-04-29 15:32 ` [PATCH 4/8] refs: make errno output explicit for refs_resolve_ref_unsafe Han-Wen Nienhuys via GitGitGadget
                   ` (6 subsequent siblings)
  9 siblings, 2 replies; 40+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-04-29 15:32 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys

From: Han-Wen Nienhuys <hanwen@google.com>

read_raw_ref_fn needs to supply a credible errno for a number of cases. These
are primarily:

1) The files backend calls read_raw_ref from lock_raw_ref, and uses the
resulting error codes to create/remove directories as needed.

2) ENOENT should be translated in a zero OID, optionally with REF_ISBROKEN set,
returning the last successfully resolved symref. This is necessary so
read_raw_ref("HEAD") on an empty repo returns refs/heads/main (or the default branch
du-jour), and we know on which branch to create the first commit.

Make this information flow explicit by adding a failure_errno to the signature
of read_raw_ref. All errnos from the files backend are still propagated
unchanged, even though inspection suggests only ENOTDIR, EISDIR and ENOENT are
relevant.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 refs.c                |  7 +++++--
 refs/debug.c          |  4 ++--
 refs/files-backend.c  | 24 ++++++++++++------------
 refs/packed-backend.c |  8 ++++----
 refs/refs-internal.h  | 16 +++++++++-------
 5 files changed, 32 insertions(+), 27 deletions(-)

diff --git a/refs.c b/refs.c
index 261fd82beb98..43e2ad6b612a 100644
--- a/refs.c
+++ b/refs.c
@@ -1675,13 +1675,16 @@ int refs_read_raw_ref(struct ref_store *ref_store,
 		      const char *refname, struct object_id *oid,
 		      struct strbuf *referent, unsigned int *type)
 {
+	int result, failure;
 	if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
 		return refs_read_special_head(ref_store, refname, oid, referent,
 					      type);
 	}
 
-	return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
-					   type);
+	result = ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
+					     type, &failure);
+	errno = failure;
+	return result;
 }
 
 /* This function needs to return a meaningful errno on failure */
diff --git a/refs/debug.c b/refs/debug.c
index 922e64fa6ad9..887dbb14be6e 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -238,14 +238,14 @@ debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,
 
 static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname,
 			      struct object_id *oid, struct strbuf *referent,
-			      unsigned int *type)
+			      unsigned int *type, int *failure_errno)
 {
 	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
 	int res = 0;
 
 	oidcpy(oid, &null_oid);
 	res = drefs->refs->be->read_raw_ref(drefs->refs, refname, oid, referent,
-					    type);
+					    type, failure_errno);
 
 	if (res == 0) {
 		trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n",
diff --git a/refs/files-backend.c b/refs/files-backend.c
index c9511da1d387..efe493ca1425 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -341,9 +341,9 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
 	return refs->loose;
 }
 
-static int files_read_raw_ref(struct ref_store *ref_store,
-			      const char *refname, struct object_id *oid,
-			      struct strbuf *referent, unsigned int *type)
+static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
+			      struct object_id *oid, struct strbuf *referent,
+			      unsigned int *type, int *failure_errno)
 {
 	struct files_ref_store *refs =
 		files_downcast(ref_store, REF_STORE_READ, "read_raw_ref");
@@ -354,7 +354,6 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 	struct stat st;
 	int fd;
 	int ret = -1;
-	int save_errno;
 	int remaining_retries = 3;
 
 	*type = 0;
@@ -459,10 +458,10 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 	ret = parse_loose_ref_contents(buf, oid, referent, type);
 
 out:
-	save_errno = errno;
+	if (failure_errno)
+		*failure_errno = errno;
 	strbuf_release(&sb_path);
 	strbuf_release(&sb_contents);
-	errno = save_errno;
 	return ret;
 }
 
@@ -541,6 +540,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
 	struct strbuf ref_file = STRBUF_INIT;
 	int attempts_remaining = 3;
 	int ret = TRANSACTION_GENERIC_ERROR;
+	int failure_errno = 0;
 
 	assert(err);
 	files_assert_main_repository(refs, "lock_raw_ref");
@@ -629,9 +629,9 @@ static int lock_raw_ref(struct files_ref_store *refs,
 	 * fear that its value will change.
 	 */
 
-	if (files_read_raw_ref(&refs->base, refname,
-			       &lock->old_oid, referent, type)) {
-		if (errno == ENOENT) {
+	if (files_read_raw_ref(&refs->base, refname, &lock->old_oid, referent,
+			       type, &failure_errno)) {
+		if (failure_errno == ENOENT) {
 			if (mustexist) {
 				/* Garden variety missing reference. */
 				strbuf_addf(err, "unable to resolve reference '%s'",
@@ -655,7 +655,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
 				 *   reference named "refs/foo/bar/baz".
 				 */
 			}
-		} else if (errno == EISDIR) {
+		} else if (failure_errno == EISDIR) {
 			/*
 			 * There is a directory in the way. It might have
 			 * contained references that have been deleted. If
@@ -693,13 +693,13 @@ static int lock_raw_ref(struct files_ref_store *refs,
 					goto error_return;
 				}
 			}
-		} else if (errno == EINVAL && (*type & REF_ISBROKEN)) {
+		} else if (failure_errno == EINVAL && (*type & REF_ISBROKEN)) {
 			strbuf_addf(err, "unable to resolve reference '%s': "
 				    "reference broken", refname);
 			goto error_return;
 		} else {
 			strbuf_addf(err, "unable to resolve reference '%s': %s",
-				    refname, strerror(errno));
+				    refname, strerror(failure_errno));
 			goto error_return;
 		}
 
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index dfecdbc1db60..a457f18e93c8 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -724,9 +724,9 @@ static struct snapshot *get_snapshot(struct packed_ref_store *refs)
 	return refs->snapshot;
 }
 
-static int packed_read_raw_ref(struct ref_store *ref_store,
-			       const char *refname, struct object_id *oid,
-			       struct strbuf *referent, unsigned int *type)
+static int packed_read_raw_ref(struct ref_store *ref_store, const char *refname,
+			       struct object_id *oid, struct strbuf *referent,
+			       unsigned int *type, int *failure_errno)
 {
 	struct packed_ref_store *refs =
 		packed_downcast(ref_store, REF_STORE_READ, "read_raw_ref");
@@ -739,7 +739,7 @@ static int packed_read_raw_ref(struct ref_store *ref_store,
 
 	if (!rec) {
 		/* refname is not a packed reference. */
-		errno = ENOENT;
+		*failure_errno = ENOENT;
 		return -1;
 	}
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 29728a339fed..ac8a14086724 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -617,10 +617,12 @@ 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 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.
  *
  * Backend-specific flags might be set in type as well, regardless of
  * outcome.
@@ -634,9 +636,9 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,
  * - in all other cases, referent will be untouched, and therefore
  *   refname will still be valid and unchanged.
  */
-typedef int read_raw_ref_fn(struct ref_store *ref_store,
-			    const char *refname, struct object_id *oid,
-			    struct strbuf *referent, unsigned int *type);
+typedef int read_raw_ref_fn(struct ref_store *ref_store, const char *refname,
+			    struct object_id *oid, struct strbuf *referent,
+			    unsigned int *type, int *failure_errno);
 
 struct ref_storage_be {
 	struct ref_storage_be *next;
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH 4/8] refs: make errno output explicit for refs_resolve_ref_unsafe
  2021-04-29 15:32 [PATCH 0/8] refs: cleanup errno sideband ref related functions Han-Wen Nienhuys via GitGitGadget
                   ` (2 preceding siblings ...)
  2021-04-29 15:32 ` [PATCH 3/8] refs: make errno output explicit for read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
@ 2021-04-29 15:32 ` Han-Wen Nienhuys via GitGitGadget
  2021-06-03  2:51   ` Jonathan Tan
  2021-04-29 15:32 ` [PATCH 5/8] refs: add failure_errno to refs_read_raw_ref() signature Han-Wen Nienhuys via GitGitGadget
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 40+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-04-29 15:32 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys

From: Han-Wen Nienhuys <hanwen@google.com>

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.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 refs.c               | 12 ++++++++++++
 refs.h               |  1 +
 refs/files-backend.c | 19 ++++++++++---------
 refs/refs-internal.h |  8 ++++++++
 4 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/refs.c b/refs.c
index 43e2ad6b612a..e8ce88040b3e 100644
--- a/refs.c
+++ b/refs.c
@@ -1780,6 +1780,18 @@ 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)
 {
diff --git a/refs.h b/refs.h
index 48970dfc7e0f..ede405ac3874 100644
--- a/refs.h
+++ b/refs.h
@@ -68,6 +68,7 @@ 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);
 
diff --git a/refs/files-backend.c b/refs/files-backend.c
index efe493ca1425..3ab09871db5e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -924,6 +924,7 @@ 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;
 
 	files_assert_main_repository(refs, "lock_ref_oid_basic");
 	assert(err);
@@ -936,10 +937,11 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 		resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
 
 	files_ref_path(refs, &ref_file, refname);
-	resolved = !!refs_resolve_ref_unsafe(&refs->base,
-					     refname, resolve_flags,
-					     &lock->old_oid, type);
-	if (!resolved && errno == EISDIR) {
+	resolved = !!refs_resolve_ref_unsafe_with_errno(&refs->base, refname,
+							resolve_flags,
+							&lock->old_oid, type,
+							&resolve_errno);
+	if (!resolved && resolve_errno == EISDIR) {
 		/*
 		 * we are trying to lock foo but we used to
 		 * have foo/bar which now does not exist;
@@ -959,12 +961,11 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 						     &lock->old_oid, type);
 	}
 	if (!resolved) {
-		int last_errno = errno;
-		if (last_errno != ENOTDIR ||
-		    !refs_verify_refname_available(&refs->base, refname,
-						   extras, skip, err))
+		if (resolve_errno != ENOTDIR ||
+		    !refs_verify_refname_available(&refs->base, refname, extras,
+						   skip, err))
 			strbuf_addf(err, "unable to resolve reference '%s': %s",
-				    refname, strerror(last_errno));
+				    refname, strerror(resolve_errno));
 
 		goto error_return;
 	}
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index ac8a14086724..b9f713b5acd6 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -153,6 +153,14 @@ 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
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH 5/8] refs: add failure_errno to refs_read_raw_ref() signature
  2021-04-29 15:32 [PATCH 0/8] refs: cleanup errno sideband ref related functions Han-Wen Nienhuys via GitGitGadget
                   ` (3 preceding siblings ...)
  2021-04-29 15:32 ` [PATCH 4/8] refs: make errno output explicit for refs_resolve_ref_unsafe Han-Wen Nienhuys via GitGitGadget
@ 2021-04-29 15:32 ` Han-Wen Nienhuys via GitGitGadget
  2021-04-29 15:32 ` [PATCH 6/8] refs: clear errno return in refs_resolve_ref_unsafe() Han-Wen Nienhuys via GitGitGadget
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-04-29 15:32 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys

From: Han-Wen Nienhuys <hanwen@google.com>

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>
---
 refs.c                | 26 ++++++++++++--------------
 refs/files-backend.c  |  8 ++++----
 refs/packed-backend.c | 10 ++++++----
 refs/refs-internal.h  |  6 +++---
 4 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/refs.c b/refs.c
index e8ce88040b3e..08f69e2a16f6 100644
--- a/refs.c
+++ b/refs.c
@@ -1671,20 +1671,17 @@ static int refs_read_special_head(struct ref_store *ref_store,
 	return result;
 }
 
-int refs_read_raw_ref(struct ref_store *ref_store,
-		      const char *refname, struct object_id *oid,
-		      struct strbuf *referent, unsigned int *type)
+int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
+		      struct object_id *oid, struct strbuf *referent,
+		      unsigned int *type, int *failure_errno)
 {
-	int result, failure;
 	if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
 		return refs_read_special_head(ref_store, refname, oid, referent,
 					      type);
 	}
 
-	result = ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
-					     type, &failure);
-	errno = failure;
-	return result;
+	return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
+					   type, failure_errno);
 }
 
 /* This function needs to return a meaningful errno on failure */
@@ -1725,9 +1722,10 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
 
 	for (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) {
 		unsigned int read_flags = 0;
+		int read_failure = 0;
 
-		if (refs_read_raw_ref(refs, refname,
-				      oid, &sb_refname, &read_flags)) {
+		if (refs_read_raw_ref(refs, refname, oid, &sb_refname,
+				      &read_flags, &read_failure)) {
 			*flags |= read_flags;
 
 			/* In reading mode, refs must eventually resolve */
@@ -1739,9 +1737,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
 			 * may show errors besides ENOENT if there are
 			 * similarly-named refs.
 			 */
-			if (errno != ENOENT &&
-			    errno != EISDIR &&
-			    errno != ENOTDIR)
+			if (read_failure != ENOENT && read_failure != EISDIR &&
+			    read_failure != ENOTDIR)
 				return NULL;
 
 			oidclr(oid);
@@ -2253,7 +2250,8 @@ int refs_verify_refname_available(struct ref_store *refs,
 		if (skip && string_list_has_string(skip, dirname.buf))
 			continue;
 
-		if (!refs_read_raw_ref(refs, dirname.buf, &oid, &referent, &type)) {
+		if (!refs_read_raw_ref(refs, dirname.buf, &oid, &referent,
+				       &type, NULL)) {
 			strbuf_addf(err, _("'%s' exists; cannot create '%s'"),
 				    dirname.buf, refname);
 			goto cleanup;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 3ab09871db5e..b6dc1b36c752 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -383,8 +383,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
 	if (lstat(path, &st) < 0) {
 		if (errno != ENOENT)
 			goto out;
-		if (refs_read_raw_ref(refs->packed_ref_store, refname,
-				      oid, referent, type)) {
+		if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
+				      referent, type, NULL)) {
 			errno = ENOENT;
 			goto out;
 		}
@@ -423,8 +423,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
 		 * ref is supposed to be, there could still be a
 		 * packed ref:
 		 */
-		if (refs_read_raw_ref(refs->packed_ref_store, refname,
-				      oid, referent, type)) {
+		if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
+				      referent, type, NULL)) {
 			errno = EISDIR;
 			goto out;
 		}
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index a457f18e93c8..03353ce48869 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -739,7 +739,8 @@ static int packed_read_raw_ref(struct ref_store *ref_store, const char *refname,
 
 	if (!rec) {
 		/* refname is not a packed reference. */
-		*failure_errno = ENOENT;
+		if (failure_errno)
+			*failure_errno = ENOENT;
 		return -1;
 	}
 
@@ -1347,6 +1348,7 @@ int is_packed_transaction_needed(struct ref_store *ref_store,
 	ret = 0;
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
+		int failure;
 		unsigned int type;
 		struct object_id oid;
 
@@ -1357,9 +1359,9 @@ int is_packed_transaction_needed(struct ref_store *ref_store,
 			 */
 			continue;
 
-		if (!refs_read_raw_ref(ref_store, update->refname,
-				       &oid, &referent, &type) ||
-		    errno != ENOENT) {
+		if (!refs_read_raw_ref(ref_store, update->refname, &oid,
+				       &referent, &type, &failure) ||
+		    failure != ENOENT) {
 			/*
 			 * We have to actually delete that reference
 			 * -> this transaction is needed.
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index b9f713b5acd6..6a0840e22772 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -149,9 +149,9 @@ struct ref_update {
 	const char refname[FLEX_ARRAY];
 };
 
-int refs_read_raw_ref(struct ref_store *ref_store,
-		      const char *refname, struct object_id *oid,
-		      struct strbuf *referent, unsigned int *type);
+int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
+		      struct object_id *oid, struct strbuf *referent,
+		      unsigned int *type, int *failure_errno);
 
 /* Like refs_resolve_ref_unsafe, but provide access to errno code that lead to a
  * failure. */
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH 6/8] refs: clear errno return in refs_resolve_ref_unsafe()
  2021-04-29 15:32 [PATCH 0/8] refs: cleanup errno sideband ref related functions Han-Wen Nienhuys via GitGitGadget
                   ` (4 preceding siblings ...)
  2021-04-29 15:32 ` [PATCH 5/8] refs: add failure_errno to refs_read_raw_ref() signature Han-Wen Nienhuys via GitGitGadget
@ 2021-04-29 15:32 ` Han-Wen Nienhuys via GitGitGadget
  2021-06-03  2:53   ` Jonathan Tan
  2021-04-29 15:32 ` [PATCH 7/8] refs: stop setting EINVAL and ELOOP in symref resolution Han-Wen Nienhuys via GitGitGadget
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 40+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-04-29 15:32 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys

From: Han-Wen Nienhuys <hanwen@google.com>

This is done in a separate commit, to pinpoint the precise cause should there be
regressions in error reporting.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 refs.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 08f69e2a16f6..6e746cb01f24 100644
--- a/refs.c
+++ b/refs.c
@@ -1685,10 +1685,11 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
 }
 
 /* This function needs to return a meaningful errno on failure */
-const char *refs_resolve_ref_unsafe(struct ref_store *refs,
-				    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 struct strbuf sb_refname = STRBUF_INIT;
 	struct object_id unused_oid;
@@ -1777,14 +1778,24 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
 	return NULL;
 }
 
+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(
+		refs, refname, resolve_flags, oid, flags);
+	errno = 0;
+	return result;
+}
+
 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);
+	const char *result = refs_resolve_ref_unsafe_errno(
+		refs, refname, resolve_flags, oid, flags);
 	*failure_errno = errno;
 	return result;
 }
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH 7/8] refs: stop setting EINVAL and ELOOP in symref resolution
  2021-04-29 15:32 [PATCH 0/8] refs: cleanup errno sideband ref related functions Han-Wen Nienhuys via GitGitGadget
                   ` (5 preceding siblings ...)
  2021-04-29 15:32 ` [PATCH 6/8] refs: clear errno return in refs_resolve_ref_unsafe() Han-Wen Nienhuys via GitGitGadget
@ 2021-04-29 15:32 ` Han-Wen Nienhuys via GitGitGadget
  2021-06-03  2:55   ` Jonathan Tan
  2021-04-29 15:32 ` [PATCH 8/8] refs: explicitly propagate errno from refs_read_raw_ref Han-Wen Nienhuys via GitGitGadget
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 40+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-04-29 15:32 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys

From: Han-Wen Nienhuys <hanwen@google.com>

The only caller of refs_resolve_ref_unsafe_with_errno() is in
refs/files-backend.c, and it only cares about EISDIR and ENOTDIR.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 refs.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/refs.c b/refs.c
index 6e746cb01f24..597e4d1f18f9 100644
--- a/refs.c
+++ b/refs.c
@@ -1706,7 +1706,6 @@ 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) ||
 		    !refname_is_safe(refname)) {
-			errno = EINVAL;
 			return NULL;
 		}
 
@@ -1766,7 +1765,6 @@ 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) ||
 			    !refname_is_safe(refname)) {
-				errno = EINVAL;
 				return NULL;
 			}
 
@@ -1774,7 +1772,6 @@ static const char *refs_resolve_ref_unsafe_errno(struct ref_store *refs,
 		}
 	}
 
-	errno = ELOOP;
 	return NULL;
 }
 
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH 8/8] refs: explicitly propagate errno from refs_read_raw_ref
  2021-04-29 15:32 [PATCH 0/8] refs: cleanup errno sideband ref related functions Han-Wen Nienhuys via GitGitGadget
                   ` (6 preceding siblings ...)
  2021-04-29 15:32 ` [PATCH 7/8] refs: stop setting EINVAL and ELOOP in symref resolution Han-Wen Nienhuys via GitGitGadget
@ 2021-04-29 15:32 ` Han-Wen Nienhuys via GitGitGadget
  2021-06-03  2:13 ` [PATCH 0/8] refs: cleanup errno sideband ref related functions Jonathan Tan
  2021-06-10 12:57 ` [PATCH v2 " Han-Wen Nienhuys via GitGitGadget
  9 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-04-29 15:32 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Han-Wen Nienhuys

From: Han-Wen Nienhuys <hanwen@google.com>

The function refs_resolve_ref_unsafe_with_errno should produce an errno output.
Rather than taking the value from the errno (which might contain garbage
beforehand), explicitly propagate the failure_errno coming out of
refs_read_raw_ref().

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 refs.c | 33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/refs.c b/refs.c
index 597e4d1f18f9..a25d18873c56 100644
--- a/refs.c
+++ b/refs.c
@@ -1684,12 +1684,11 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
 					   type, failure_errno);
 }
 
-/* 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)
+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)
 {
 	static struct strbuf sb_refname = STRBUF_INIT;
 	struct object_id unused_oid;
@@ -1702,6 +1701,7 @@ static const char *refs_resolve_ref_unsafe_errno(struct ref_store *refs,
 		flags = &unused_flags;
 
 	*flags = 0;
+	*failure_errno = 0;
 
 	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
 		if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
@@ -1728,6 +1728,8 @@ static const char *refs_resolve_ref_unsafe_errno(struct ref_store *refs,
 				      &read_flags, &read_failure)) {
 			*flags |= read_flags;
 
+			*failure_errno = read_failure;
+
 			/* In reading mode, refs must eventually resolve */
 			if (resolve_flags & RESOLVE_REF_READING)
 				return NULL;
@@ -1779,22 +1781,9 @@ 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(
-		refs, refname, resolve_flags, oid, flags);
-	errno = 0;
-	return result;
-}
-
-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_errno(
-		refs, refname, resolve_flags, oid, flags);
-	*failure_errno = errno;
-	return result;
+	int ignore;
+	return refs_resolve_ref_unsafe_with_errno(refs, refname, resolve_flags,
+						  oid, flags, &ignore);
 }
 
 /* backend functions */
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 1/8] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn
  2021-04-29 15:32 ` [PATCH 1/8] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
@ 2021-04-30  2:38   ` Junio C Hamano
  2021-05-19 12:25     ` Han-Wen Nienhuys
  2021-06-03  2:19   ` Jonathan Tan
  1 sibling, 1 reply; 40+ messages in thread
From: Junio C Hamano @ 2021-04-30  2:38 UTC (permalink / raw)
  To: Han-Wen Nienhuys via GitGitGadget; +Cc: git, Han-Wen Nienhuys, Han-Wen Nienhuys

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> A grep for EINVAL */*c reveals that no code inspects EINVAL after reading
> references.

We often use a pattern (which is common) like this:

	if (some_func_in_ref_API(...) < 0) {
		if (errno == ENOENT || errno == EISDIR)
			... it is OK for the file to be missing ...
		else
			... error ...
	}

If a piece of code currently sets EINVAL to errno manually when
signalling a failure by returning a negative value to communicate
with such a caller, we wouldn't see EINVAL mentioned, so such a grep
alone would not help us guarantee the correctness of an update to
stop assignment of EINVAL at all.  The callers must be vetted more
carefully than "we are happy that nobody explicitly mentions EINVAL".

> 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 paragraph is confusing.  It says EINVAL is used to signal
lock_raw_ref(), and it says EINVAL is not used by the same files
backend.  Which is correct?  If one part of the backend uses it, and
other parts don't, wouldn't the backend as a whole still use it?

Having said that, ...

> - * 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.

... the mention (and requirement) of EINVAL seems redundant, as it
sounds sufficient for the caller to inspect 'type' to see if it is
REF_ISBOKEN.  So it may be OK for the code that gives REF_ISBROKEN
to type *not* to set errno to EINVAL, as long as it won't leave it
as ENOENT (meaning, an unrelated system call failed earlier may have
set errno to ENOENT, and after having dealt with such an error, the
control may have reached to the codepath we are interested in
here---errno must be cleared to some value other than ENOENT, and
assigning EINVAL is as good as any).

That is because there is a codeflow like this:

	if (files_read_raw_ref(...)) {
		if (errno == ENOENT) {
			... do various things ...
		} else if (errno == EISDIR) {
			... do different and various things ...
		} else if (errno == EINVAL && (*type & REF_ISBROKEN)) {
			... deal with broken ref ...
		}
		 ...
	}

where errno is looked at.

> + * 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.

So, this is not sufficient to let caller correctly and safely handle
errors.  "set REF_ISBROKEN in type, set errno to something other
than ENOENT or EISDIR, and then return -1" is necessary, I would
think.

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic
  2021-04-29 15:32 ` [PATCH 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic Han-Wen Nienhuys via GitGitGadget
@ 2021-04-30  3:10   ` Junio C Hamano
  2021-05-19 12:29     ` Han-Wen Nienhuys
  2021-06-03  2:33   ` Jonathan Tan
  1 sibling, 1 reply; 40+ messages in thread
From: Junio C Hamano @ 2021-04-30  3:10 UTC (permalink / raw)
  To: Han-Wen Nienhuys via GitGitGadget; +Cc: git, Han-Wen Nienhuys, Han-Wen Nienhuys

I 100% agree with you that errno is cumbersome to use and carries
far less information than we want (we do not learn what operation
failed on what path) over a long distance.  It only is useful when
the callchain still knows what path was operated on.

But...

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> For the copy/rename support, calls to lock_ref_oid_basic() in this file are
> followed by:
>
> * lock_ref_oid_basic (copy/rename rollback error path)
>
> * write_ref_to_lockfile (both in the rollback path and the success path of
>   copy/rename)
>
> 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.

In the latter part of the above, "callers" refers to the callers of
"the copy/rename support" (aka files_copy_or_rename_ref())?

Then I am not sure why "callers cannot reliably observe the errno
that lock_ref_oid_basic() generated" is a problem.  They will see
the errno from the last system call that failed, if they care.  So
their performing I/O is perfectly acceptable, too.

Hence, I am not sure what change the above justifies, if any.

If we can show that no caller of files_copy_or_rename_ref() uses
errno, it is a clear indication that lock_ref_oid_basic() is saving
and restoring errno for no good reason.  I think that is what was
done for the other two callers below.

So I traced what happens after the copy-rename thing gets called.

refs_rename_ref(), rename_ref(), refs_copy_existing_ref() and
copy_existing_ref() (all in refs.c) should be the only callers of
the function.  All callers in builtin/branch.c and builtin/remote.c
of these functions (by the way, refs_X() variants do not seem to be
called from anywhere---are they over-engineered?) just die() when
they signal a failure by returning non-zero, so I think it is safe
and much easier to understand to justify this change like so:

    refs/files-backend.c::lock_ref_oid_basic() tries hard to signal
    how it failed to its callers using errno.  The three callers of
    this file-scope static function are 

    * files_copy_or_rename_ref()
    * files_create_symref()
    * files_reflog_expire()

    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.

> For files_create_symref() and files_reflog_expire(), grepping over callers
> showed no callers inspecting errno.

Yes, this is a lot more relevant justification to allow these two
functions, and functions that are called _only_ _by_ these two
functions, stop worrying about errno.

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 3/8] refs: make errno output explicit for read_raw_ref_fn
  2021-04-29 15:32 ` [PATCH 3/8] refs: make errno output explicit for read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
@ 2021-04-30  3:34   ` Junio C Hamano
  2021-04-30  6:02     ` Junio C Hamano
  2021-06-03  2:37   ` Jonathan Tan
  1 sibling, 1 reply; 40+ messages in thread
From: Junio C Hamano @ 2021-04-30  3:34 UTC (permalink / raw)
  To: Han-Wen Nienhuys via GitGitGadget; +Cc: git, Han-Wen Nienhuys, Han-Wen Nienhuys

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> read_raw_ref_fn needs to supply a credible errno for a number of cases. These
> are primarily:
>
> 1) The files backend calls read_raw_ref from lock_raw_ref, and uses the
> resulting error codes to create/remove directories as needed.
>
> 2) ENOENT should be translated in a zero OID, optionally with REF_ISBROKEN set,
> returning the last successfully resolved symref. This is necessary so
> read_raw_ref("HEAD") on an empty repo returns refs/heads/main (or the default branch
> du-jour), and we know on which branch to create the first commit.
>
> Make this information flow explicit by adding a failure_errno to the signature
> of read_raw_ref. All errnos from the files backend are still propagated
> unchanged, even though inspection suggests only ENOTDIR, EISDIR and ENOENT are
> relevant.

I like the general direction to move away from errno, which may make
sense only in the context of files backend (e.g. ENOENT would be
left in errno, only because the original "loose ref" implementation
used one file per ref, when a ref we wanted to see did not exist)
and having other backends use the same errno would not make much
sense, as it is not the goal to make other backends like reftable
emulate files backend.

I wonder if in the ideal world, we'd rather want to define our own
enum, not <errno.h>, that is specific to failure modes of ref API
functions and signal failures by returning these values (and the
enum includes 0 as a value to signal success, all other errors are
negative values).

What I am really getting at is if we need an extra "failure"
out-parameter-pointer in the internal API.  I do not mind if it
helps the transition to the ideal world, but I offhand do not see if
we need result and failure as separate variables.

> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  refs.c                |  7 +++++--
>  refs/debug.c          |  4 ++--
>  refs/files-backend.c  | 24 ++++++++++++------------
>  refs/packed-backend.c |  8 ++++----
>  refs/refs-internal.h  | 16 +++++++++-------
>  5 files changed, 32 insertions(+), 27 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 261fd82beb98..43e2ad6b612a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1675,13 +1675,16 @@ int refs_read_raw_ref(struct ref_store *ref_store,
>  		      const char *refname, struct object_id *oid,
>  		      struct strbuf *referent, unsigned int *type)
>  {
> +	int result, failure;
>  	if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
>  		return refs_read_special_head(ref_store, refname, oid, referent,
>  					      type);
>  	}
>  
> -	return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> -					   type);
> +	result = ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> +					     type, &failure);
> +	errno = failure;
> +	return result;
>  }
>  
>  /* This function needs to return a meaningful errno on failure */
> diff --git a/refs/debug.c b/refs/debug.c
> index 922e64fa6ad9..887dbb14be6e 100644
> --- a/refs/debug.c
> +++ b/refs/debug.c
> @@ -238,14 +238,14 @@ debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,
>  
>  static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname,
>  			      struct object_id *oid, struct strbuf *referent,
> -			      unsigned int *type)
> +			      unsigned int *type, int *failure_errno)
>  {
>  	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
>  	int res = 0;
>  
>  	oidcpy(oid, &null_oid);
>  	res = drefs->refs->be->read_raw_ref(drefs->refs, refname, oid, referent,
> -					    type);
> +					    type, failure_errno);
>  
>  	if (res == 0) {
>  		trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n",
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index c9511da1d387..efe493ca1425 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -341,9 +341,9 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
>  	return refs->loose;
>  }
>  
> -static int files_read_raw_ref(struct ref_store *ref_store,
> -			      const char *refname, struct object_id *oid,
> -			      struct strbuf *referent, unsigned int *type)
> +static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
> +			      struct object_id *oid, struct strbuf *referent,
> +			      unsigned int *type, int *failure_errno)
>  {
>  	struct files_ref_store *refs =
>  		files_downcast(ref_store, REF_STORE_READ, "read_raw_ref");
> @@ -354,7 +354,6 @@ static int files_read_raw_ref(struct ref_store *ref_store,
>  	struct stat st;
>  	int fd;
>  	int ret = -1;
> -	int save_errno;
>  	int remaining_retries = 3;
>  
>  	*type = 0;
> @@ -459,10 +458,10 @@ static int files_read_raw_ref(struct ref_store *ref_store,
>  	ret = parse_loose_ref_contents(buf, oid, referent, type);
>  
>  out:
> -	save_errno = errno;
> +	if (failure_errno)
> +		*failure_errno = errno;
>  	strbuf_release(&sb_path);
>  	strbuf_release(&sb_contents);
> -	errno = save_errno;
>  	return ret;
>  }
>  
> @@ -541,6 +540,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
>  	struct strbuf ref_file = STRBUF_INIT;
>  	int attempts_remaining = 3;
>  	int ret = TRANSACTION_GENERIC_ERROR;
> +	int failure_errno = 0;
>  
>  	assert(err);
>  	files_assert_main_repository(refs, "lock_raw_ref");
> @@ -629,9 +629,9 @@ static int lock_raw_ref(struct files_ref_store *refs,
>  	 * fear that its value will change.
>  	 */
>  
> -	if (files_read_raw_ref(&refs->base, refname,
> -			       &lock->old_oid, referent, type)) {
> -		if (errno == ENOENT) {
> +	if (files_read_raw_ref(&refs->base, refname, &lock->old_oid, referent,
> +			       type, &failure_errno)) {
> +		if (failure_errno == ENOENT) {
>  			if (mustexist) {
>  				/* Garden variety missing reference. */
>  				strbuf_addf(err, "unable to resolve reference '%s'",
> @@ -655,7 +655,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
>  				 *   reference named "refs/foo/bar/baz".
>  				 */
>  			}
> -		} else if (errno == EISDIR) {
> +		} else if (failure_errno == EISDIR) {
>  			/*
>  			 * There is a directory in the way. It might have
>  			 * contained references that have been deleted. If
> @@ -693,13 +693,13 @@ static int lock_raw_ref(struct files_ref_store *refs,
>  					goto error_return;
>  				}
>  			}
> -		} else if (errno == EINVAL && (*type & REF_ISBROKEN)) {
> +		} else if (failure_errno == EINVAL && (*type & REF_ISBROKEN)) {
>  			strbuf_addf(err, "unable to resolve reference '%s': "
>  				    "reference broken", refname);
>  			goto error_return;
>  		} else {
>  			strbuf_addf(err, "unable to resolve reference '%s': %s",
> -				    refname, strerror(errno));
> +				    refname, strerror(failure_errno));
>  			goto error_return;
>  		}
>  
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index dfecdbc1db60..a457f18e93c8 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -724,9 +724,9 @@ static struct snapshot *get_snapshot(struct packed_ref_store *refs)
>  	return refs->snapshot;
>  }
>  
> -static int packed_read_raw_ref(struct ref_store *ref_store,
> -			       const char *refname, struct object_id *oid,
> -			       struct strbuf *referent, unsigned int *type)
> +static int packed_read_raw_ref(struct ref_store *ref_store, const char *refname,
> +			       struct object_id *oid, struct strbuf *referent,
> +			       unsigned int *type, int *failure_errno)
>  {
>  	struct packed_ref_store *refs =
>  		packed_downcast(ref_store, REF_STORE_READ, "read_raw_ref");
> @@ -739,7 +739,7 @@ static int packed_read_raw_ref(struct ref_store *ref_store,
>  
>  	if (!rec) {
>  		/* refname is not a packed reference. */
> -		errno = ENOENT;
> +		*failure_errno = ENOENT;
>  		return -1;
>  	}
>  
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index 29728a339fed..ac8a14086724 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -617,10 +617,12 @@ 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 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.
>   *
>   * Backend-specific flags might be set in type as well, regardless of
>   * outcome.
> @@ -634,9 +636,9 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,
>   * - in all other cases, referent will be untouched, and therefore
>   *   refname will still be valid and unchanged.
>   */
> -typedef int read_raw_ref_fn(struct ref_store *ref_store,
> -			    const char *refname, struct object_id *oid,
> -			    struct strbuf *referent, unsigned int *type);
> +typedef int read_raw_ref_fn(struct ref_store *ref_store, const char *refname,
> +			    struct object_id *oid, struct strbuf *referent,
> +			    unsigned int *type, int *failure_errno);
>  
>  struct ref_storage_be {
>  	struct ref_storage_be *next;

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 3/8] refs: make errno output explicit for read_raw_ref_fn
  2021-04-30  3:34   ` Junio C Hamano
@ 2021-04-30  6:02     ` Junio C Hamano
  2021-05-19 12:33       ` Han-Wen Nienhuys
  0 siblings, 1 reply; 40+ messages in thread
From: Junio C Hamano @ 2021-04-30  6:02 UTC (permalink / raw)
  To: Han-Wen Nienhuys via GitGitGadget; +Cc: git, Han-Wen Nienhuys, Han-Wen Nienhuys

Junio C Hamano <gitster@pobox.com> writes:

> I like the general direction to move away from errno, which may make
> sense only in the context of files backend (e.g. ENOENT would be
> left in errno, only because the original "loose ref" implementation
> used one file per ref, when a ref we wanted to see did not exist)
> and having other backends use the same errno would not make much
> sense, as it is not the goal to make other backends like reftable
> emulate files backend.
>
> I wonder if in the ideal world, we'd rather want to define our own
> enum, not <errno.h>, that is specific to failure modes of ref API
> functions and signal failures by returning these values (and the
> enum includes 0 as a value to signal success, all other errors are
> negative values).
>
> What I am really getting at is if we need an extra "failure"
> out-parameter-pointer in the internal API.  I do not mind if it
> helps the transition to the ideal world, but I offhand do not see if
> we need result and failure as separate variables.

In any case, the change in the function signature helped me catch
that this series invalidates what has been queued on hn/reftable
without updating that topic to align with the new way to handle the
errno (it would have become a silent semantic failure if we instead
encoded the "failure" in the return value).

The following is what I am tentatively using for tonight's
integration when merging this and hn/reftable together to the 'seen'
branch.

Thanks.


 refs/reftable-backend.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git c/refs/reftable-backend.c w/refs/reftable-backend.c
index 1aff21adb4..4385d2d6f5 100644
--- c/refs/reftable-backend.c
+++ w/refs/reftable-backend.c
@@ -52,7 +52,8 @@ static struct reftable_stack *stack_for(struct git_reftable_ref_store *store,
 static int git_reftable_read_raw_ref(struct ref_store *ref_store,
 				     const char *refname, struct object_id *oid,
 				     struct strbuf *referent,
-				     unsigned int *type);
+				     unsigned int *type,
+				     int *failure_errno);
 
 static void clear_reftable_log_record(struct reftable_log_record *log)
 {
@@ -376,7 +377,8 @@ static int fixup_symrefs(struct ref_store *ref_store,
 						&old_oid, &referent,
 						/* mutate input, like
 						   files-backend.c */
-						&update->type);
+						&update->type,
+						&errno);
 		if (err < 0 && errno == ENOENT &&
 		    is_null_oid(&update->old_oid)) {
 			err = 0;
@@ -1538,7 +1540,7 @@ static int reftable_error_to_errno(int err)
 static int git_reftable_read_raw_ref(struct ref_store *ref_store,
 				     const char *refname, struct object_id *oid,
 				     struct strbuf *referent,
-				     unsigned int *type)
+				     unsigned int *type, int *failure_errno)
 {
 	struct git_reftable_ref_store *refs =
 		(struct git_reftable_ref_store *)ref_store;
@@ -1560,12 +1562,12 @@ static int git_reftable_read_raw_ref(struct ref_store *ref_store,
 
 	err = reftable_stack_read_ref(stack, refname, &ref);
 	if (err > 0) {
-		errno = ENOENT;
+		*failure_errno = ENOENT;
 		err = -1;
 		goto done;
 	}
 	if (err < 0) {
-		errno = reftable_error_to_errno(err);
+		*failure_errno = reftable_error_to_errno(err);
 		err = -1;
 		goto done;
 	}
@@ -1578,7 +1580,7 @@ static int git_reftable_read_raw_ref(struct ref_store *ref_store,
 		oidread(oid, reftable_ref_record_val1(&ref));
 	} else {
 		*type |= REF_ISBROKEN;
-		errno = EINVAL;
+		*failure_errno = EINVAL;
 		err = -1;
 	}
 done:

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 1/8] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn
  2021-04-30  2:38   ` Junio C Hamano
@ 2021-05-19 12:25     ` Han-Wen Nienhuys
  0 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys @ 2021-05-19 12:25 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys

On Fri, Apr 30, 2021 at 4:38 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Han-Wen Nienhuys <hanwen@google.com>
> >
> > A grep for EINVAL */*c reveals that no code inspects EINVAL after reading
> > references.
>
> We often use a pattern (which is common) like this:
>
>         if (some_func_in_ref_API(...) < 0) {
>                 if (errno == ENOENT || errno == EISDIR)
>                         ... it is OK for the file to be missing ...
>                 else
>                         ... error ...
>         }
>
> If a piece of code currently sets EINVAL to errno manually when
> signalling a failure by returning a negative value to communicate
> with such a caller, we wouldn't see EINVAL mentioned, so such a grep
> alone would not help us guarantee the correctness of an update to
> stop assignment of EINVAL at all.  The callers must be vetted more
> carefully than "we are happy that nobody explicitly mentions EINVAL".

Sure. I looked at the callers, and documented further what I looked
for. But how far should one go? It's a global variable, so
transitively, almost all of the code could be observing the EINVAL
value under very specific circumstances. But it would also be a
terrible, fragile coding style and use of undocumented behavior.

> > 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 paragraph is confusing.  It says EINVAL is used to signal
> lock_raw_ref(), and it says EINVAL is not used by the same files
> backend.  Which is correct?  If one part of the backend uses it, and
> other parts don't, wouldn't the backend as a whole still use it?

I tried to clarify the message. files-backend.c makes assumptions
about the errno return for files_read_raw_ref, but it's not making
assumptions about the abstracted API in refs.h

> That is because there is a codeflow like this:
>
>         if (files_read_raw_ref(...)) {
>                 if (errno == ENOENT) {
>                         ... do various things ...
>                 } else if (errno == EISDIR) {
>                         ... do different and various things ...
>                 } else if (errno == EINVAL && (*type & REF_ISBROKEN)) {
>                         ... deal with broken ref ...
>                 }
>                  ...
>         }

as mentioned above, this isn't calling refs_read_raw_ref, so it's not
affected by this patch.

> > + * 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.
>
> So, this is not sufficient to let caller correctly and safely handle
> errors.  "set REF_ISBROKEN in type, set errno to something other
> than ENOENT or EISDIR, and then return -1" is necessary, I would
> think.

I tweaked the comment. Note that the function has only a handful of
callers (and only one caller where this behavior is relevant), and
it's changed in a follow-on patch in this series. Is it worth the
effort to wordsmith this further?

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic
  2021-04-30  3:10   ` Junio C Hamano
@ 2021-05-19 12:29     ` Han-Wen Nienhuys
  0 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys @ 2021-05-19 12:29 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys

On Fri, Apr 30, 2021 at 5:10 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> I 100% agree with you that errno is cumbersome to use and carries
> far less information than we want (we do not learn what operation
> failed on what path) over a long distance.  It only is useful when
> the callchain still knows what path was operated on.
>
> But...
>
> "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > For the copy/rename support, calls to lock_ref_oid_basic() in this file are
> > followed by:
> >
> > * lock_ref_oid_basic (copy/rename rollback error path)
> >
> > * write_ref_to_lockfile (both in the rollback path and the success path of
> >   copy/rename)
> >
> > 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.
>
> In the latter part of the above, "callers" refers to the callers of
> "the copy/rename support" (aka files_copy_or_rename_ref())?
>
> Then I am not sure why "callers cannot reliably observe the errno
> that lock_ref_oid_basic() generated" is a problem.  They will see
> the errno from the last system call that failed, if they care.  So
> their performing I/O is perfectly acceptable, too.
>
> Hence, I am not sure what change the above justifies, if any.
>
> If we can show that no caller of files_copy_or_rename_ref() uses
> errno, it is a clear indication that lock_ref_oid_basic() is saving
> and restoring errno for no good reason.  I think that is what was
> done for the other two callers below.
>
> So I traced what happens after the copy-rename thing gets called.
>
> refs_rename_ref(), rename_ref(), refs_copy_existing_ref() and
> copy_existing_ref() (all in refs.c) should be the only callers of
> the function.  All callers in builtin/branch.c and builtin/remote.c
> of these functions (by the way, refs_X() variants do not seem to be
> called from anywhere---are they over-engineered?) just die() when
> they signal a failure by returning non-zero, so I think it is safe
> and much easier to understand to justify this change like so:
>
>     refs/files-backend.c::lock_ref_oid_basic() tries hard to signal
>     how it failed to its callers using errno.  The three callers of
>     this file-scope static function are
>
>     * files_copy_or_rename_ref()
>     * files_create_symref()
>     * files_reflog_expire()
>
>     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.

I stole your message here; hope that's OK. My original message tries
to convey that if you do

/* should return errno */
int a() { .. }

int b() {
   result = a();
   maybe_do_IO();
   return result;
}

then callers of b() can't reason about the errno result of a(),
because they can't know if an error code was generated by
maybe_do_IO() or a(). This means that the errno result of a() is
useless.  (This is assuming that b() doesn't inspect errno, which I
failed to mention.)

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 3/8] refs: make errno output explicit for read_raw_ref_fn
  2021-04-30  6:02     ` Junio C Hamano
@ 2021-05-19 12:33       ` Han-Wen Nienhuys
  0 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys @ 2021-05-19 12:33 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys

On Fri, Apr 30, 2021 at 8:02 AM Junio C Hamano <gitster@pobox.com> wrote:
> > I like the general direction to move away from errno, which may make
> > sense only in the context of files backend (e.g. ENOENT would be
> > left in errno, only because the original "loose ref" implementation
> > used one file per ref, when a ref we wanted to see did not exist)
> > and having other backends use the same errno would not make much
> > sense, as it is not the goal to make other backends like reftable
> > emulate files backend.
> >
> > I wonder if in the ideal world, we'd rather want to define our own
> > enum, not <errno.h>, that is specific to failure modes of ref API
> > functions and signal failures by returning these values (and the
> > enum includes 0 as a value to signal success, all other errors are
> > negative values).

I think it would be healthy to have a set of canonical error codes in
general,  but I don't know if this will help here. The files and
packed backend want to communicate about very specific conditions
(ENOENT, EISDIR, ENOTDIR), which seem too low-level to put in a
generic error code.

> In any case, the change in the function signature helped me catch
> that this series invalidates what has been queued on hn/reftable
> without updating that topic to align with the new way to handle the
> errno (it would have become a silent semantic failure if we instead
> encoded the "failure" in the return value).
>
> The following is what I am tentatively using for tonight's
> integration when merging this and hn/reftable together to the 'seen'
> branch.

That sounds right. How should I post the next update to hn/reftable ?
(based on which branch?)

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 0/8] refs: cleanup errno sideband ref related functions
  2021-04-29 15:32 [PATCH 0/8] refs: cleanup errno sideband ref related functions Han-Wen Nienhuys via GitGitGadget
                   ` (7 preceding siblings ...)
  2021-04-29 15:32 ` [PATCH 8/8] refs: explicitly propagate errno from refs_read_raw_ref Han-Wen Nienhuys via GitGitGadget
@ 2021-06-03  2:13 ` Jonathan Tan
  2021-06-09 11:29   ` Han-Wen Nienhuys
  2021-06-10 12:57 ` [PATCH v2 " Han-Wen Nienhuys via GitGitGadget
  9 siblings, 1 reply; 40+ messages in thread
From: Jonathan Tan @ 2021-06-03  2:13 UTC (permalink / raw)
  To: gitgitgadget; +Cc: git, hanwenn, Jonathan Tan

> v3
> 
>  * remove errno as an implicit communication mechanism from refs support
>    completely.

Looking at all the patches, it seems that this patch set is about
functions in Git code that set errno themselves to indicate the category
of failure encountered, and instead of setting errno, we want them to
transmit that information through an out param instead. I notice that
the cover letter talks about "cleanup" and "remove errno", but that
could have been explained in greater detail, I think.

As for whether it is a good idea overall, it could be said that errno is
idiomatic in C and writing "if (myfunc()) { store_errno = errno; ..." is
not much more difficult than "int store_errno; if (myfunc(&store_errno))
{ ...". But presumably Han-Wen thinks the opposite, since he wrote this
patch set, and I'll defer to his opinion on this since he's working on
the ref code and I'm not. Besides that, having the out param is more
explicit (which might be better) and permits chaining of such functions
(e.g. "if (myfunc(&store_errno) || myotherfunc(&store_errno))"). I'll
review this patch set as if this is the approach we want to take.

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 1/8] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn
  2021-04-29 15:32 ` [PATCH 1/8] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
  2021-04-30  2:38   ` Junio C Hamano
@ 2021-06-03  2:19   ` Jonathan Tan
  2021-06-09 11:28     ` Han-Wen Nienhuys
  1 sibling, 1 reply; 40+ messages in thread
From: Jonathan Tan @ 2021-06-03  2:19 UTC (permalink / raw)
  To: gitgitgadget; +Cc: git, hanwenn, hanwen, Jonathan Tan

> From: Han-Wen Nienhuys <hanwen@google.com>
> 
> A grep for EINVAL */*c reveals that no code inspects EINVAL after reading
> references.
> 
> 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.

Does this mean that there is some code that sets EINVAL, but no code
uses it? If yes, that seems to mean that we can't remove EINVAL from the
documentation, since some code still sets it.

> As the errno sideband is unintuitive and error-prone, remove EINVAL
> value, as a step towards getting rid of the errno sideband altogether.

How is removing one possible value a step towards getting rid of the
errno sideband? I would have thought that we would be working towards
transmitting all existing values to the new sideband (the out param).
Unless we are planning to get rid of all values in the sideband - in
which case, this should be described in the commit message.

[snip]

> ---
>  refs/refs-internal.h | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> 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.

The rewrapping was unnecessary and makes it hard to see what changed.

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic
  2021-04-29 15:32 ` [PATCH 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic Han-Wen Nienhuys via GitGitGadget
  2021-04-30  3:10   ` Junio C Hamano
@ 2021-06-03  2:33   ` Jonathan Tan
  2021-06-10 10:02     ` Han-Wen Nienhuys
  1 sibling, 1 reply; 40+ messages in thread
From: Jonathan Tan @ 2021-06-03  2:33 UTC (permalink / raw)
  To: gitgitgadget; +Cc: git, hanwenn, hanwen, Jonathan Tan

> From: Han-Wen Nienhuys <hanwen@google.com>
> 
> 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.

I don't think this paragraph is useful.

> For the copy/rename support, calls to lock_ref_oid_basic() in this file are
> followed by:
> 
> * lock_ref_oid_basic (copy/rename rollback error path)
> 
> * write_ref_to_lockfile (both in the rollback path and the success path of
>   copy/rename)
> 
> 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.
> 
> For files_create_symref() and files_reflog_expire(), grepping over callers
> showed no callers inspecting errno.

This is probably written more clearly as follows:

 No call to the static function lock_ref_oid_basic() is immediately
 followed by an errno check, so stopping setting errno is safe. But as a
 sanity check, lock_ref_oid_basic() is used in these functions:
 - 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)
 - 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)

 So it is safe to stop setting errno in lock_ref_oid_basic().

The diff itself looks good.

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 3/8] refs: make errno output explicit for read_raw_ref_fn
  2021-04-29 15:32 ` [PATCH 3/8] refs: make errno output explicit for read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
  2021-04-30  3:34   ` Junio C Hamano
@ 2021-06-03  2:37   ` Jonathan Tan
  2021-06-10 10:05     ` Han-Wen Nienhuys
  1 sibling, 1 reply; 40+ messages in thread
From: Jonathan Tan @ 2021-06-03  2:37 UTC (permalink / raw)
  To: gitgitgadget; +Cc: git, hanwenn, hanwen, Jonathan Tan

> From: Han-Wen Nienhuys <hanwen@google.com>
> 
> read_raw_ref_fn needs to supply a credible errno for a number of cases. These
> are primarily:
> 
> 1) The files backend calls read_raw_ref from lock_raw_ref, and uses the
> resulting error codes to create/remove directories as needed.
> 
> 2) ENOENT should be translated in a zero OID, optionally with REF_ISBROKEN set,
> returning the last successfully resolved symref. This is necessary so
> read_raw_ref("HEAD") on an empty repo returns refs/heads/main (or the default branch
> du-jour), and we know on which branch to create the first commit.
> 
> Make this information flow explicit by adding a failure_errno to the signature
> of read_raw_ref. All errnos from the files backend are still propagated
> unchanged, even though inspection suggests only ENOTDIR, EISDIR and ENOENT are
> relevant.

Looks good.

> @@ -1675,13 +1675,16 @@ int refs_read_raw_ref(struct ref_store *ref_store,
>  		      const char *refname, struct object_id *oid,
>  		      struct strbuf *referent, unsigned int *type)
>  {
> +	int result, failure;
>  	if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
>  		return refs_read_special_head(ref_store, refname, oid, referent,
>  					      type);
>  	}
>  
> -	return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> -					   type);
> +	result = ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
> +					     type, &failure);
> +	errno = failure;
> +	return result;
>  }

Should we initialize "failure" to 0 here? As a reader, I would assume
that "failure" has the semantics of errno here, which upon success, may
or may not be set.

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 4/8] refs: make errno output explicit for refs_resolve_ref_unsafe
  2021-04-29 15:32 ` [PATCH 4/8] refs: make errno output explicit for refs_resolve_ref_unsafe Han-Wen Nienhuys via GitGitGadget
@ 2021-06-03  2:51   ` Jonathan Tan
  2021-06-10 11:27     ` Han-Wen Nienhuys
  0 siblings, 1 reply; 40+ messages in thread
From: Jonathan Tan @ 2021-06-03  2:51 UTC (permalink / raw)
  To: gitgitgadget; +Cc: git, hanwenn, hanwen, Jonathan Tan

> From: Han-Wen Nienhuys <hanwen@google.com>
> 
> 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.

Looking at the next few patches, I think the plan is:

 - Introduce a new function refs_resolve_ref_unsafe_with_errno() which
   returns the error information in an out param instead of errno. Right
   now, it wraps refs_resolve_ref_unsafe().
 - Migrate all callers that require the error information to
   refs_resolve_ref_unsafe_with_errno(), and leave all callers that do
   not require the error information alone (that is, using
   refs_resolve_ref_unsafe()).
 - To ensure that all callers using refs_resolve_ref_unsafe() really do
   not use errno, set it to 0. (This is allowed by the errno contract -
   successes can set errno to whatever they want.)

If this is the plan, it should be written in the commit message.

As it is, this patch handles the first and maybe part of the second
step, and patch 5 handles the rest of the second step? I think the
patches should be more clearly divided.

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 6/8] refs: clear errno return in refs_resolve_ref_unsafe()
  2021-04-29 15:32 ` [PATCH 6/8] refs: clear errno return in refs_resolve_ref_unsafe() Han-Wen Nienhuys via GitGitGadget
@ 2021-06-03  2:53   ` Jonathan Tan
  2021-06-10 11:45     ` Han-Wen Nienhuys
  0 siblings, 1 reply; 40+ messages in thread
From: Jonathan Tan @ 2021-06-03  2:53 UTC (permalink / raw)
  To: gitgitgadget; +Cc: git, hanwenn, hanwen, Jonathan Tan

> @@ -1685,10 +1685,11 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
>  }
>  
>  /* This function needs to return a meaningful errno on failure */
> -const char *refs_resolve_ref_unsafe(struct ref_store *refs,
> -				    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)

So a third function (refs_resolve_ref_unsafe_errno() - not to be
confused with refs_resolve_ref_unsafe_with_errno(), which has an extra
"with")? Couldn't we just swap the other 2 functions directly instead of
going through this intermediary?

> +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(
> +		refs, refname, resolve_flags, oid, flags);
> +	errno = 0;
> +	return result;
> +}

This is the errno = 0 part that I was talking about in my review of patch 4.

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 7/8] refs: stop setting EINVAL and ELOOP in symref resolution
  2021-04-29 15:32 ` [PATCH 7/8] refs: stop setting EINVAL and ELOOP in symref resolution Han-Wen Nienhuys via GitGitGadget
@ 2021-06-03  2:55   ` Jonathan Tan
  2021-06-10 11:58     ` Han-Wen Nienhuys
  0 siblings, 1 reply; 40+ messages in thread
From: Jonathan Tan @ 2021-06-03  2:55 UTC (permalink / raw)
  To: gitgitgadget; +Cc: git, hanwenn, hanwen, Jonathan Tan

> From: Han-Wen Nienhuys <hanwen@google.com>
> 
> The only caller of refs_resolve_ref_unsafe_with_errno() is in
> refs/files-backend.c, and it only cares about EISDIR and ENOTDIR.
> 
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  refs.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 6e746cb01f24..597e4d1f18f9 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1706,7 +1706,6 @@ 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) ||
>  		    !refname_is_safe(refname)) {
> -			errno = EINVAL;
>  			return NULL;
>  		}
>  

I don't think this is related to avoiding errno and conveying error
information through an out param. But besides that, as it is, I'm not
sure that this is correct. Even if EINVAL is not checked by the caller,
setting errno to EINVAL here means avoiding exposing a potential
EISDIR/ENOTDIR that a preceding call set. Same comment for the other
instances.

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 1/8] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn
  2021-06-03  2:19   ` Jonathan Tan
@ 2021-06-09 11:28     ` Han-Wen Nienhuys
  0 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys @ 2021-06-09 11:28 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys

On Thu, Jun 3, 2021 at 4:19 AM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> > From: Han-Wen Nienhuys <hanwen@google.com>
> >
> > A grep for EINVAL */*c reveals that no code inspects EINVAL after reading
> > references.
> >
> > 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.
>
> Does this mean that there is some code that sets EINVAL, but no code
> uses it? If yes, that seems to mean that we can't remove EINVAL from the
> documentation, since some code still sets it.

It means that an alternate ref backend doesn't have to return EINVAL
to work properly.

added to commit message.

> > As the errno sideband is unintuitive and error-prone, remove EINVAL
> > value, as a step towards getting rid of the errno sideband altogether.
>
> How is removing one possible value a step towards getting rid of the
> errno sideband? I would have thought that we would be working towards
> transmitting all existing values to the new sideband (the out param).
> Unless we are planning to get rid of all values in the sideband - in
> which case, this should be described in the commit message.

It means that we don't have to spend braincycles in further commits of
this series reasoning about EINVAL.

The existing functions potentially transmit all kinds of errors, eg.
EIO if there was a problem with the media, or ENOTCONN if the git repo
happens to be on a FUSE filesystem that crashed. These are not
relevant to the ref backend, so we don't treat them specially either.

> > - * 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.
>
> The rewrapping was unnecessary and makes it hard to see what changed.

Fixed.  I suppose .clang-format settings should add some configuration
about the line size for wrapping comments.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 0/8] refs: cleanup errno sideband ref related functions
  2021-06-03  2:13 ` [PATCH 0/8] refs: cleanup errno sideband ref related functions Jonathan Tan
@ 2021-06-09 11:29   ` Han-Wen Nienhuys
  0 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys @ 2021-06-09 11:29 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys

On Thu, Jun 3, 2021 at 4:14 AM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> > v3
> >
> >  * remove errno as an implicit communication mechanism from refs support
> >    completely.
>
> Looking at all the patches, it seems that this patch set is about
> functions in Git code that set errno themselves to indicate the category
> of failure encountered, and instead of setting errno, we want them to
> transmit that information through an out param instead. I notice that
> the cover letter talks about "cleanup" and "remove errno", but that
> could have been explained in greater detail, I think.

What is the right place to document these considerations? The data in
the cover letter doesn't end up in the project history, so it seems
like the wrong place.

> As for whether it is a good idea overall, it could be said that errno is
> idiomatic in C and writing "if (myfunc()) { store_errno = errno; ..." is
> not much more difficult than "int store_errno; if (myfunc(&store_errno))
> { ...". But presumably Han-Wen thinks the opposite, since he wrote this
> patch set, and I'll defer to his opinion on this since he's working on
> the ref code and I'm not. Besides that, having the out param is more
> explicit (which might be better) and permits chaining of such functions
> (e.g. "if (myfunc(&store_errno) || myotherfunc(&store_errno))"). I'll
> review this patch set as if this is the approach we want to take.


it's certainly idiomatic and easy for writing. The problem is that it
makes reading code much more difficult: since all of the code has
access to errno, it's hard to tell which code depends on certain
writes to errno. One signal of this are the extensive discussions in
this series about how to argue (in the commit message) that a certain
code change is safe.


-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic
  2021-06-03  2:33   ` Jonathan Tan
@ 2021-06-10 10:02     ` Han-Wen Nienhuys
  0 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys @ 2021-06-10 10:02 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys

On Thu, Jun 3, 2021 at 4:33 AM Jonathan Tan <jonathantanmy@google.com> wrote:
> > 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.
>
> I don't think this paragraph is useful.

Dropped.

> This is probably written more clearly as follows:
>
>  No call to the static function lock_ref_oid_basic() is immediately
>  followed by an errno check, so stopping setting errno is safe. But as a
>  sanity check, lock_ref_oid_basic() is used in these functions:
>  - 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)
>  - 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)
>
>  So it is safe to stop setting errno in lock_ref_oid_basic().

I used some of the above text.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 3/8] refs: make errno output explicit for read_raw_ref_fn
  2021-06-03  2:37   ` Jonathan Tan
@ 2021-06-10 10:05     ` Han-Wen Nienhuys
  0 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys @ 2021-06-10 10:05 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys

On Thu, Jun 3, 2021 at 4:37 AM Jonathan Tan <jonathantanmy@google.com> wrote:
> Should we initialize "failure" to 0 here? As a reader, I would assume
> that "failure" has the semantics of errno here, which upon success, may
> or may not be set.

Done.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 4/8] refs: make errno output explicit for refs_resolve_ref_unsafe
  2021-06-03  2:51   ` Jonathan Tan
@ 2021-06-10 11:27     ` Han-Wen Nienhuys
  0 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys @ 2021-06-10 11:27 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys

On Thu, Jun 3, 2021 at 4:51 AM Jonathan Tan <jonathantanmy@google.com> wrote:
> > 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.
>
> Looking at the next few patches, I think the plan is:
>
>  - Introduce a new function refs_resolve_ref_unsafe_with_errno() which
>    returns the error information in an out param instead of errno. Right
>    now, it wraps refs_resolve_ref_unsafe().
>  - Migrate all callers that require the error information to
>    refs_resolve_ref_unsafe_with_errno(), and leave all callers that do
>    not require the error information alone (that is, using
>    refs_resolve_ref_unsafe()).
>  - To ensure that all callers using refs_resolve_ref_unsafe() really do
>    not use errno, set it to 0. (This is allowed by the errno contract -
>    successes can set errno to whatever they want.)
>
> If this is the plan, it should be written in the commit message.

I added more description to the commit message.

> As it is, this patch handles the first and maybe part of the second
> step, and patch 5 handles the rest of the second step? I think the
> patches should be more clearly divided.

Done.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 6/8] refs: clear errno return in refs_resolve_ref_unsafe()
  2021-06-03  2:53   ` Jonathan Tan
@ 2021-06-10 11:45     ` Han-Wen Nienhuys
  0 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys @ 2021-06-10 11:45 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys

On Thu, Jun 3, 2021 at 4:53 AM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> > @@ -1685,10 +1685,11 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
> >  }
> >
> >  /* This function needs to return a meaningful errno on failure */
> > -const char *refs_resolve_ref_unsafe(struct ref_store *refs,
> > -                                 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)
>
> So a third function (refs_resolve_ref_unsafe_errno() - not to be
> confused with refs_resolve_ref_unsafe_with_errno(), which has an extra
> "with")? Couldn't we just swap the other 2 functions directly instead of
> going through this intermediary?

I've clarified the name. I've done it this way, because it keeps the
diff small. Swapping the functions would require code changes that I
thought would be more work to review.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH 7/8] refs: stop setting EINVAL and ELOOP in symref resolution
  2021-06-03  2:55   ` Jonathan Tan
@ 2021-06-10 11:58     ` Han-Wen Nienhuys
  0 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys @ 2021-06-10 11:58 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: Han-Wen Nienhuys via GitGitGadget, git, Han-Wen Nienhuys

On Thu, Jun 3, 2021 at 4:55 AM Jonathan Tan <jonathantanmy@google.com> wrote:
> I don't think this is related to avoiding errno and conveying error
> information through an out param. But besides that, as it is, I'm not
> sure that this is correct. Even if EINVAL is not checked by the caller,
> setting errno to EINVAL here means avoiding exposing a potential
> EISDIR/ENOTDIR that a preceding call set. Same comment for the other
> instances.

You are right, but it's probably moot, because the follow-up commit
stops using errno (making it impossible for EISDIR/ENOTDIR to
haphazardly appear), but I dropped this commit from the series.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v2 0/8] refs: cleanup errno sideband ref related functions
  2021-04-29 15:32 [PATCH 0/8] refs: cleanup errno sideband ref related functions Han-Wen Nienhuys via GitGitGadget
                   ` (8 preceding siblings ...)
  2021-06-03  2:13 ` [PATCH 0/8] refs: cleanup errno sideband ref related functions Jonathan Tan
@ 2021-06-10 12:57 ` Han-Wen Nienhuys via GitGitGadget
  2021-06-10 12:57   ` [PATCH v2 1/8] refs: remove EINVAL errno output from specification of read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
                     ` (8 more replies)
  9 siblings, 9 replies; 40+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-06-10 12:57 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Jonathan Tan, Han-Wen Nienhuys

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;

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v2 1/8] refs: remove EINVAL errno output from specification of read_raw_ref_fn
  2021-06-10 12:57 ` [PATCH v2 " Han-Wen Nienhuys via GitGitGadget
@ 2021-06-10 12:57   ` Han-Wen Nienhuys via GitGitGadget
  2021-06-10 12:57   ` [PATCH v2 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic Han-Wen Nienhuys via GitGitGadget
                     ` (7 subsequent siblings)
  8 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-06-10 12:57 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Jonathan Tan, Han-Wen Nienhuys, Han-Wen Nienhuys

From: Han-Wen Nienhuys <hanwen@google.com>

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.

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.

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 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 467f4b3c936d..f4445e329045 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -619,9 +619,9 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,
  *
  * 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.
+ * 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.
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v2 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic
  2021-06-10 12:57 ` [PATCH v2 " Han-Wen Nienhuys via GitGitGadget
  2021-06-10 12:57   ` [PATCH v2 1/8] refs: remove EINVAL errno output from specification of read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
@ 2021-06-10 12:57   ` Han-Wen Nienhuys via GitGitGadget
  2021-06-10 12:57   ` [PATCH v2 3/8] refs: make errno output explicit for read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
                     ` (6 subsequent siblings)
  8 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-06-10 12:57 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Jonathan Tan, Han-Wen Nienhuys, Han-Wen Nienhuys

From: Han-Wen Nienhuys <hanwen@google.com>

refs/files-backend.c::lock_ref_oid_basic() tries to signal how it failed
to its callers using errno.

It is safe to stop setting errno here, because the callers of this
file-scope static function are

* files_copy_or_rename_ref()
* files_create_symref()
* files_reflog_expire()

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,

* 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)

* 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 | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 677b7e4cdd2d..6aa0f5c41dd3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -910,7 +910,6 @@ static int create_reflock(const char *path, void *cb)
 
 /*
  * Locks a ref returning the lock on success and NULL on failure.
- * On failure errno is set to something meaningful.
  */
 static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 					   const char *refname,
@@ -922,7 +921,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 {
 	struct strbuf ref_file = STRBUF_INIT;
 	struct ref_lock *lock;
-	int last_errno = 0;
 	int mustexist = (old_oid && !is_null_oid(old_oid));
 	int resolve_flags = RESOLVE_REF_NO_RECURSE;
 	int resolved;
@@ -949,7 +947,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 		 * to remain.
 		 */
 		if (remove_empty_directories(&ref_file)) {
-			last_errno = errno;
 			if (!refs_verify_refname_available(
 					    &refs->base,
 					    refname, extras, skip, err))
@@ -962,7 +959,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 						     &lock->old_oid, type);
 	}
 	if (!resolved) {
-		last_errno = errno;
+		int last_errno = errno;
 		if (last_errno != ENOTDIR ||
 		    !refs_verify_refname_available(&refs->base, refname,
 						   extras, skip, err))
@@ -981,20 +978,17 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 	if (is_null_oid(&lock->old_oid) &&
 	    refs_verify_refname_available(refs->packed_ref_store, refname,
 					  extras, skip, err)) {
-		last_errno = ENOTDIR;
 		goto error_return;
 	}
 
 	lock->ref_name = xstrdup(refname);
 
 	if (raceproof_create_file(ref_file.buf, create_reflock, &lock->lk)) {
-		last_errno = errno;
 		unable_to_lock_message(ref_file.buf, errno, err);
 		goto error_return;
 	}
 
 	if (verify_lock(&refs->base, lock, old_oid, mustexist, err)) {
-		last_errno = errno;
 		goto error_return;
 	}
 	goto out;
@@ -1005,7 +999,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 
  out:
 	strbuf_release(&ref_file);
-	errno = last_errno;
 	return lock;
 }
 
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v2 3/8] refs: make errno output explicit for read_raw_ref_fn
  2021-06-10 12:57 ` [PATCH v2 " Han-Wen Nienhuys via GitGitGadget
  2021-06-10 12:57   ` [PATCH v2 1/8] refs: remove EINVAL errno output from specification of read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
  2021-06-10 12:57   ` [PATCH v2 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic Han-Wen Nienhuys via GitGitGadget
@ 2021-06-10 12:57   ` Han-Wen Nienhuys via GitGitGadget
  2021-06-10 12:57   ` [PATCH v2 4/8] refs: make errno output explicit for refs_resolve_ref_unsafe Han-Wen Nienhuys via GitGitGadget
                     ` (5 subsequent siblings)
  8 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-06-10 12:57 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Jonathan Tan, Han-Wen Nienhuys, Han-Wen Nienhuys

From: Han-Wen Nienhuys <hanwen@google.com>

read_raw_ref_fn needs to supply a credible errno for a number of cases. These
are primarily:

1) The files backend calls read_raw_ref from lock_raw_ref, and uses the
resulting error codes to create/remove directories as needed.

2) ENOENT should be translated in a zero OID, optionally with REF_ISBROKEN set,
returning the last successfully resolved symref. This is necessary so
read_raw_ref("HEAD") on an empty repo returns refs/heads/main (or the default branch
du-jour), and we know on which branch to create the first commit.

Make this information flow explicit by adding a failure_errno to the signature
of read_raw_ref. All errnos from the files backend are still propagated
unchanged, even though inspection suggests only ENOTDIR, EISDIR and ENOENT are
relevant.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
---
 refs.c                |  8 ++++++--
 refs/debug.c          |  4 ++--
 refs/files-backend.c  | 24 ++++++++++++------------
 refs/packed-backend.c |  8 ++++----
 refs/refs-internal.h  | 17 +++++++++--------
 5 files changed, 33 insertions(+), 28 deletions(-)

diff --git a/refs.c b/refs.c
index 8c9490235ea6..bebe3f584da7 100644
--- a/refs.c
+++ b/refs.c
@@ -1675,13 +1675,17 @@ int refs_read_raw_ref(struct ref_store *ref_store,
 		      const char *refname, struct object_id *oid,
 		      struct strbuf *referent, unsigned int *type)
 {
+	int result, failure;
 	if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
 		return refs_read_special_head(ref_store, refname, oid, referent,
 					      type);
 	}
 
-	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;
+	return result;
 }
 
 /* This function needs to return a meaningful errno on failure */
diff --git a/refs/debug.c b/refs/debug.c
index 7db4abccc341..f12413a9bc0f 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -238,7 +238,7 @@ debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,
 
 static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname,
 			      struct object_id *oid, struct strbuf *referent,
-			      unsigned int *type)
+			      unsigned int *type, int *failure_errno)
 {
 	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
 	int res = 0;
@@ -246,7 +246,7 @@ 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);
 
 	if (res == 0) {
 		trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n",
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 6aa0f5c41dd3..8f969c8f711f 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -341,9 +341,9 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
 	return refs->loose;
 }
 
-static int files_read_raw_ref(struct ref_store *ref_store,
-			      const char *refname, struct object_id *oid,
-			      struct strbuf *referent, unsigned int *type)
+static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
+			      struct object_id *oid, struct strbuf *referent,
+			      unsigned int *type, int *failure_errno)
 {
 	struct files_ref_store *refs =
 		files_downcast(ref_store, REF_STORE_READ, "read_raw_ref");
@@ -354,7 +354,6 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 	struct stat st;
 	int fd;
 	int ret = -1;
-	int save_errno;
 	int remaining_retries = 3;
 
 	*type = 0;
@@ -459,10 +458,10 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 	ret = parse_loose_ref_contents(buf, oid, referent, type);
 
 out:
-	save_errno = errno;
+	if (failure_errno)
+		*failure_errno = errno;
 	strbuf_release(&sb_path);
 	strbuf_release(&sb_contents);
-	errno = save_errno;
 	return ret;
 }
 
@@ -541,6 +540,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
 	struct strbuf ref_file = STRBUF_INIT;
 	int attempts_remaining = 3;
 	int ret = TRANSACTION_GENERIC_ERROR;
+	int failure_errno = 0;
 
 	assert(err);
 	files_assert_main_repository(refs, "lock_raw_ref");
@@ -629,9 +629,9 @@ static int lock_raw_ref(struct files_ref_store *refs,
 	 * fear that its value will change.
 	 */
 
-	if (files_read_raw_ref(&refs->base, refname,
-			       &lock->old_oid, referent, type)) {
-		if (errno == ENOENT) {
+	if (files_read_raw_ref(&refs->base, refname, &lock->old_oid, referent,
+			       type, &failure_errno)) {
+		if (failure_errno == ENOENT) {
 			if (mustexist) {
 				/* Garden variety missing reference. */
 				strbuf_addf(err, "unable to resolve reference '%s'",
@@ -655,7 +655,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
 				 *   reference named "refs/foo/bar/baz".
 				 */
 			}
-		} else if (errno == EISDIR) {
+		} else if (failure_errno == EISDIR) {
 			/*
 			 * There is a directory in the way. It might have
 			 * contained references that have been deleted. If
@@ -693,13 +693,13 @@ static int lock_raw_ref(struct files_ref_store *refs,
 					goto error_return;
 				}
 			}
-		} else if (errno == EINVAL && (*type & REF_ISBROKEN)) {
+		} else if (failure_errno == EINVAL && (*type & REF_ISBROKEN)) {
 			strbuf_addf(err, "unable to resolve reference '%s': "
 				    "reference broken", refname);
 			goto error_return;
 		} else {
 			strbuf_addf(err, "unable to resolve reference '%s': %s",
-				    refname, strerror(errno));
+				    refname, strerror(failure_errno));
 			goto error_return;
 		}
 
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index dfecdbc1db60..a457f18e93c8 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -724,9 +724,9 @@ static struct snapshot *get_snapshot(struct packed_ref_store *refs)
 	return refs->snapshot;
 }
 
-static int packed_read_raw_ref(struct ref_store *ref_store,
-			       const char *refname, struct object_id *oid,
-			       struct strbuf *referent, unsigned int *type)
+static int packed_read_raw_ref(struct ref_store *ref_store, const char *refname,
+			       struct object_id *oid, struct strbuf *referent,
+			       unsigned int *type, int *failure_errno)
 {
 	struct packed_ref_store *refs =
 		packed_downcast(ref_store, REF_STORE_READ, "read_raw_ref");
@@ -739,7 +739,7 @@ static int packed_read_raw_ref(struct ref_store *ref_store,
 
 	if (!rec) {
 		/* refname is not a packed reference. */
-		errno = ENOENT;
+		*failure_errno = ENOENT;
 		return -1;
 	}
 
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index f4445e329045..904b2a9e51ae 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -617,11 +617,12 @@ 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
- * (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 (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.
@@ -635,9 +636,9 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,
  * - in all other cases, referent will be untouched, and therefore
  *   refname will still be valid and unchanged.
  */
-typedef int read_raw_ref_fn(struct ref_store *ref_store,
-			    const char *refname, struct object_id *oid,
-			    struct strbuf *referent, unsigned int *type);
+typedef int read_raw_ref_fn(struct ref_store *ref_store, const char *refname,
+			    struct object_id *oid, struct strbuf *referent,
+			    unsigned int *type, int *failure_errno);
 
 struct ref_storage_be {
 	struct ref_storage_be *next;
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v2 4/8] refs: make errno output explicit for refs_resolve_ref_unsafe
  2021-06-10 12:57 ` [PATCH v2 " Han-Wen Nienhuys via GitGitGadget
                     ` (2 preceding siblings ...)
  2021-06-10 12:57   ` [PATCH v2 3/8] refs: make errno output explicit for read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
@ 2021-06-10 12:57   ` Han-Wen Nienhuys via GitGitGadget
  2021-06-10 12:57   ` [PATCH v2 5/8] refs: use refs_resolve_ref_unsafe_with_errno() where needed Han-Wen Nienhuys via GitGitGadget
                     ` (4 subsequent siblings)
  8 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-06-10 12:57 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Jonathan Tan, Han-Wen Nienhuys, Han-Wen Nienhuys

From: Han-Wen Nienhuys <hanwen@google.com>

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 of this refactoring.

In a follow-on commits, we will

* migrate callers that need this error information

* clear errno in refs_resolve_ref_unsafe() to make sure these other callers
aren't using the error output accidentally.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
---
 refs.c               | 12 ++++++++++++
 refs/refs-internal.h |  8 ++++++++
 2 files changed, 20 insertions(+)

diff --git a/refs.c b/refs.c
index bebe3f584da7..64e2d55adcfb 100644
--- a/refs.c
+++ b/refs.c
@@ -1781,6 +1781,18 @@ 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)
 {
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 904b2a9e51ae..eb97023658f8 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -153,6 +153,14 @@ 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
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v2 5/8] refs: use refs_resolve_ref_unsafe_with_errno() where needed
  2021-06-10 12:57 ` [PATCH v2 " Han-Wen Nienhuys via GitGitGadget
                     ` (3 preceding siblings ...)
  2021-06-10 12:57   ` [PATCH v2 4/8] refs: make errno output explicit for refs_resolve_ref_unsafe Han-Wen Nienhuys via GitGitGadget
@ 2021-06-10 12:57   ` Han-Wen Nienhuys via GitGitGadget
  2021-06-10 12:57   ` [PATCH v2 6/8] refs: add failure_errno to refs_read_raw_ref() signature Han-Wen Nienhuys via GitGitGadget
                     ` (3 subsequent siblings)
  8 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-06-10 12:57 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Jonathan Tan, Han-Wen Nienhuys, Han-Wen Nienhuys

From: Han-Wen Nienhuys <hanwen@google.com>

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>
Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
---
 refs/files-backend.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8f969c8f711f..5a430aabf623 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -924,6 +924,7 @@ 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 = 0;
 
 	files_assert_main_repository(refs, "lock_ref_oid_basic");
 	assert(err);
@@ -936,10 +937,11 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 		resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
 
 	files_ref_path(refs, &ref_file, refname);
-	resolved = !!refs_resolve_ref_unsafe(&refs->base,
-					     refname, resolve_flags,
-					     &lock->old_oid, type);
-	if (!resolved && errno == EISDIR) {
+	resolved = !!refs_resolve_ref_unsafe_with_errno(&refs->base, refname,
+							resolve_flags,
+							&lock->old_oid, type,
+							&resolve_errno);
+	if (!resolved && resolve_errno == EISDIR) {
 		/*
 		 * we are trying to lock foo but we used to
 		 * have foo/bar which now does not exist;
@@ -959,12 +961,11 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
 						     &lock->old_oid, type);
 	}
 	if (!resolved) {
-		int last_errno = errno;
-		if (last_errno != ENOTDIR ||
-		    !refs_verify_refname_available(&refs->base, refname,
-						   extras, skip, err))
+		if (resolve_errno != ENOTDIR ||
+		    !refs_verify_refname_available(&refs->base, refname, extras,
+						   skip, err))
 			strbuf_addf(err, "unable to resolve reference '%s': %s",
-				    refname, strerror(last_errno));
+				    refname, strerror(resolve_errno));
 
 		goto error_return;
 	}
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v2 6/8] refs: add failure_errno to refs_read_raw_ref() signature
  2021-06-10 12:57 ` [PATCH v2 " Han-Wen Nienhuys via GitGitGadget
                     ` (4 preceding siblings ...)
  2021-06-10 12:57   ` [PATCH v2 5/8] refs: use refs_resolve_ref_unsafe_with_errno() where needed Han-Wen Nienhuys via GitGitGadget
@ 2021-06-10 12:57   ` Han-Wen Nienhuys via GitGitGadget
  2021-06-10 12:57   ` [PATCH v2 7/8] refs: clear errno return in refs_resolve_ref_unsafe() Han-Wen Nienhuys via GitGitGadget
                     ` (2 subsequent siblings)
  8 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-06-10 12:57 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Jonathan Tan, Han-Wen Nienhuys, Han-Wen Nienhuys

From: Han-Wen Nienhuys <hanwen@google.com>

This makes the errno output of refs_read_raw_ref explicit.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
---
 refs.c                | 29 ++++++++++++++---------------
 refs/files-backend.c  |  8 ++++----
 refs/packed-backend.c | 10 ++++++----
 refs/refs-internal.h  |  6 +++---
 4 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/refs.c b/refs.c
index 64e2d55adcfb..ed2dde1c0c6d 100644
--- a/refs.c
+++ b/refs.c
@@ -1671,21 +1671,19 @@ static int refs_read_special_head(struct ref_store *ref_store,
 	return result;
 }
 
-int refs_read_raw_ref(struct ref_store *ref_store,
-		      const char *refname, struct object_id *oid,
-		      struct strbuf *referent, unsigned int *type)
+int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
+		      struct object_id *oid, struct strbuf *referent,
+		      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;
-	return result;
+	return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
+					   type, failure_errno);
 }
 
 /* This function needs to return a meaningful errno on failure */
@@ -1726,9 +1724,10 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
 
 	for (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) {
 		unsigned int read_flags = 0;
+		int read_failure = 0;
 
-		if (refs_read_raw_ref(refs, refname,
-				      oid, &sb_refname, &read_flags)) {
+		if (refs_read_raw_ref(refs, refname, oid, &sb_refname,
+				      &read_flags, &read_failure)) {
 			*flags |= read_flags;
 
 			/* In reading mode, refs must eventually resolve */
@@ -1740,9 +1739,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
 			 * may show errors besides ENOENT if there are
 			 * similarly-named refs.
 			 */
-			if (errno != ENOENT &&
-			    errno != EISDIR &&
-			    errno != ENOTDIR)
+			if (read_failure != ENOENT && read_failure != EISDIR &&
+			    read_failure != ENOTDIR)
 				return NULL;
 
 			oidclr(oid);
@@ -2254,7 +2252,8 @@ int refs_verify_refname_available(struct ref_store *refs,
 		if (skip && string_list_has_string(skip, dirname.buf))
 			continue;
 
-		if (!refs_read_raw_ref(refs, dirname.buf, &oid, &referent, &type)) {
+		if (!refs_read_raw_ref(refs, dirname.buf, &oid, &referent,
+				       &type, NULL)) {
 			strbuf_addf(err, _("'%s' exists; cannot create '%s'"),
 				    dirname.buf, refname);
 			goto cleanup;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5a430aabf623..01c9bd0dbf04 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -383,8 +383,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
 	if (lstat(path, &st) < 0) {
 		if (errno != ENOENT)
 			goto out;
-		if (refs_read_raw_ref(refs->packed_ref_store, refname,
-				      oid, referent, type)) {
+		if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
+				      referent, type, NULL)) {
 			errno = ENOENT;
 			goto out;
 		}
@@ -423,8 +423,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
 		 * ref is supposed to be, there could still be a
 		 * packed ref:
 		 */
-		if (refs_read_raw_ref(refs->packed_ref_store, refname,
-				      oid, referent, type)) {
+		if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
+				      referent, type, NULL)) {
 			errno = EISDIR;
 			goto out;
 		}
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index a457f18e93c8..03353ce48869 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -739,7 +739,8 @@ static int packed_read_raw_ref(struct ref_store *ref_store, const char *refname,
 
 	if (!rec) {
 		/* refname is not a packed reference. */
-		*failure_errno = ENOENT;
+		if (failure_errno)
+			*failure_errno = ENOENT;
 		return -1;
 	}
 
@@ -1347,6 +1348,7 @@ int is_packed_transaction_needed(struct ref_store *ref_store,
 	ret = 0;
 	for (i = 0; i < transaction->nr; i++) {
 		struct ref_update *update = transaction->updates[i];
+		int failure;
 		unsigned int type;
 		struct object_id oid;
 
@@ -1357,9 +1359,9 @@ int is_packed_transaction_needed(struct ref_store *ref_store,
 			 */
 			continue;
 
-		if (!refs_read_raw_ref(ref_store, update->refname,
-				       &oid, &referent, &type) ||
-		    errno != ENOENT) {
+		if (!refs_read_raw_ref(ref_store, update->refname, &oid,
+				       &referent, &type, &failure) ||
+		    failure != ENOENT) {
 			/*
 			 * We have to actually delete that reference
 			 * -> this transaction is needed.
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index eb97023658f8..c65d26580ce8 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -149,9 +149,9 @@ struct ref_update {
 	const char refname[FLEX_ARRAY];
 };
 
-int refs_read_raw_ref(struct ref_store *ref_store,
-		      const char *refname, struct object_id *oid,
-		      struct strbuf *referent, unsigned int *type);
+int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
+		      struct object_id *oid, struct strbuf *referent,
+		      unsigned int *type, int *failure_errno);
 
 /* Like refs_resolve_ref_unsafe, but provide access to errno code that lead to a
  * failure. */
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v2 7/8] refs: clear errno return in refs_resolve_ref_unsafe()
  2021-06-10 12:57 ` [PATCH v2 " Han-Wen Nienhuys via GitGitGadget
                     ` (5 preceding siblings ...)
  2021-06-10 12:57   ` [PATCH v2 6/8] refs: add failure_errno to refs_read_raw_ref() signature Han-Wen Nienhuys via GitGitGadget
@ 2021-06-10 12:57   ` Han-Wen Nienhuys via GitGitGadget
  2021-06-10 12:57   ` [PATCH v2 8/8] refs: explicitly propagate errno from refs_read_raw_ref Han-Wen Nienhuys via GitGitGadget
  2021-06-14 10:10   ` [PATCH v2 0/8] refs: cleanup errno sideband ref related functions Han-Wen Nienhuys
  8 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-06-10 12:57 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Jonathan Tan, Han-Wen Nienhuys, Han-Wen Nienhuys

From: Han-Wen Nienhuys <hanwen@google.com>

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 | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index ed2dde1c0c6d..191cbf5a330f 100644
--- a/refs.c
+++ b/refs.c
@@ -1687,10 +1687,10 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
 }
 
 /* This function needs to return a meaningful errno on failure */
-const char *refs_resolve_ref_unsafe(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;
@@ -1779,14 +1779,24 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
 	return NULL;
 }
 
+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_implicit_errno(
+		refs, refname, resolve_flags, oid, flags);
+	errno = 0;
+	return result;
+}
+
 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);
+	const char *result = refs_resolve_ref_unsafe_implicit_errno(
+		refs, refname, resolve_flags, oid, flags);
 	*failure_errno = errno;
 	return result;
 }
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v2 8/8] refs: explicitly propagate errno from refs_read_raw_ref
  2021-06-10 12:57 ` [PATCH v2 " Han-Wen Nienhuys via GitGitGadget
                     ` (6 preceding siblings ...)
  2021-06-10 12:57   ` [PATCH v2 7/8] refs: clear errno return in refs_resolve_ref_unsafe() Han-Wen Nienhuys via GitGitGadget
@ 2021-06-10 12:57   ` Han-Wen Nienhuys via GitGitGadget
  2021-06-14 10:10   ` [PATCH v2 0/8] refs: cleanup errno sideband ref related functions Han-Wen Nienhuys
  8 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys via GitGitGadget @ 2021-06-10 12:57 UTC (permalink / raw)
  To: git; +Cc: Han-Wen Nienhuys, Jonathan Tan, Han-Wen Nienhuys, Han-Wen Nienhuys

From: Han-Wen Nienhuys <hanwen@google.com>

The function refs_resolve_ref_unsafe_with_errno should produce an errno output.
Rather than taking the value from the errno (which might contain garbage
beforehand), explicitly propagate the failure_errno coming out of
refs_read_raw_ref().

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
---
 refs.c | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/refs.c b/refs.c
index 191cbf5a330f..92c4796078bb 100644
--- a/refs.c
+++ b/refs.c
@@ -1686,11 +1686,11 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
 					   type, failure_errno);
 }
 
-/* This function needs to return a meaningful errno on failure */
-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,
+					       struct object_id *oid,
+					       int *flags, int *failure_errno)
 {
 	static struct strbuf sb_refname = STRBUF_INIT;
 	struct object_id unused_oid;
@@ -1703,11 +1703,12 @@ refs_resolve_ref_unsafe_implicit_errno(struct ref_store *refs,
 		flags = &unused_flags;
 
 	*flags = 0;
+	*failure_errno = 0;
 
 	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;
 		}
 
@@ -1730,6 +1731,8 @@ refs_resolve_ref_unsafe_implicit_errno(struct ref_store *refs,
 				      &read_flags, &read_failure)) {
 			*flags |= read_flags;
 
+			*failure_errno = read_failure;
+
 			/* In reading mode, refs must eventually resolve */
 			if (resolve_flags & RESOLVE_REF_READING)
 				return NULL;
@@ -1767,7 +1770,7 @@ 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;
 			}
 
@@ -1775,7 +1778,7 @@ refs_resolve_ref_unsafe_implicit_errno(struct ref_store *refs,
 		}
 	}
 
-	errno = ELOOP;
+	*failure_errno = ELOOP;
 	return NULL;
 }
 
@@ -1783,22 +1786,9 @@ 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_implicit_errno(
-		refs, refname, resolve_flags, oid, flags);
-	errno = 0;
-	return result;
-}
-
-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_implicit_errno(
-		refs, refname, resolve_flags, oid, flags);
-	*failure_errno = errno;
-	return result;
+	int ignore;
+	return refs_resolve_ref_unsafe_with_errno(refs, refname, resolve_flags,
+						  oid, flags, &ignore);
 }
 
 /* backend functions */
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v2 0/8] refs: cleanup errno sideband ref related functions
  2021-06-10 12:57 ` [PATCH v2 " Han-Wen Nienhuys via GitGitGadget
                     ` (7 preceding siblings ...)
  2021-06-10 12:57   ` [PATCH v2 8/8] refs: explicitly propagate errno from refs_read_raw_ref Han-Wen Nienhuys via GitGitGadget
@ 2021-06-14 10:10   ` Han-Wen Nienhuys
  8 siblings, 0 replies; 40+ messages in thread
From: Han-Wen Nienhuys @ 2021-06-14 10:10 UTC (permalink / raw)
  To: Han-Wen Nienhuys via GitGitGadget
  Cc: git, Jonathan Tan, Han-Wen Nienhuys, Junio C Hamano

On Thu, Jun 10, 2021 at 2:57 PM Han-Wen Nienhuys via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> v4
>
>  * commit msg tweaks in response to Jun.

I forgot to update the cover letter, but this includes the updates in
response to review by Jonathan Tan.

I think this topic could graduate?

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

^ permalink raw reply	[flat|nested] 40+ messages in thread

end of thread, other threads:[~2021-06-14 10:10 UTC | newest]

Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-29 15:32 [PATCH 0/8] refs: cleanup errno sideband ref related functions Han-Wen Nienhuys via GitGitGadget
2021-04-29 15:32 ` [PATCH 1/8] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-04-30  2:38   ` Junio C Hamano
2021-05-19 12:25     ` Han-Wen Nienhuys
2021-06-03  2:19   ` Jonathan Tan
2021-06-09 11:28     ` Han-Wen Nienhuys
2021-04-29 15:32 ` [PATCH 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic Han-Wen Nienhuys via GitGitGadget
2021-04-30  3:10   ` Junio C Hamano
2021-05-19 12:29     ` Han-Wen Nienhuys
2021-06-03  2:33   ` Jonathan Tan
2021-06-10 10:02     ` Han-Wen Nienhuys
2021-04-29 15:32 ` [PATCH 3/8] refs: make errno output explicit for read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-04-30  3:34   ` Junio C Hamano
2021-04-30  6:02     ` Junio C Hamano
2021-05-19 12:33       ` Han-Wen Nienhuys
2021-06-03  2:37   ` Jonathan Tan
2021-06-10 10:05     ` Han-Wen Nienhuys
2021-04-29 15:32 ` [PATCH 4/8] refs: make errno output explicit for refs_resolve_ref_unsafe Han-Wen Nienhuys via GitGitGadget
2021-06-03  2:51   ` Jonathan Tan
2021-06-10 11:27     ` Han-Wen Nienhuys
2021-04-29 15:32 ` [PATCH 5/8] refs: add failure_errno to refs_read_raw_ref() signature Han-Wen Nienhuys via GitGitGadget
2021-04-29 15:32 ` [PATCH 6/8] refs: clear errno return in refs_resolve_ref_unsafe() Han-Wen Nienhuys via GitGitGadget
2021-06-03  2:53   ` Jonathan Tan
2021-06-10 11:45     ` Han-Wen Nienhuys
2021-04-29 15:32 ` [PATCH 7/8] refs: stop setting EINVAL and ELOOP in symref resolution Han-Wen Nienhuys via GitGitGadget
2021-06-03  2:55   ` Jonathan Tan
2021-06-10 11:58     ` Han-Wen Nienhuys
2021-04-29 15:32 ` [PATCH 8/8] refs: explicitly propagate errno from refs_read_raw_ref Han-Wen Nienhuys via GitGitGadget
2021-06-03  2:13 ` [PATCH 0/8] refs: cleanup errno sideband ref related functions Jonathan Tan
2021-06-09 11:29   ` Han-Wen Nienhuys
2021-06-10 12:57 ` [PATCH v2 " Han-Wen Nienhuys via GitGitGadget
2021-06-10 12:57   ` [PATCH v2 1/8] refs: remove EINVAL errno output from specification of read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-06-10 12:57   ` [PATCH v2 2/8] refs/files-backend: stop setting errno from lock_ref_oid_basic Han-Wen Nienhuys via GitGitGadget
2021-06-10 12:57   ` [PATCH v2 3/8] refs: make errno output explicit for read_raw_ref_fn Han-Wen Nienhuys via GitGitGadget
2021-06-10 12:57   ` [PATCH v2 4/8] refs: make errno output explicit for refs_resolve_ref_unsafe Han-Wen Nienhuys via GitGitGadget
2021-06-10 12:57   ` [PATCH v2 5/8] refs: use refs_resolve_ref_unsafe_with_errno() where needed Han-Wen Nienhuys via GitGitGadget
2021-06-10 12:57   ` [PATCH v2 6/8] refs: add failure_errno to refs_read_raw_ref() signature Han-Wen Nienhuys via GitGitGadget
2021-06-10 12:57   ` [PATCH v2 7/8] refs: clear errno return in refs_resolve_ref_unsafe() Han-Wen Nienhuys via GitGitGadget
2021-06-10 12:57   ` [PATCH v2 8/8] refs: explicitly propagate errno from refs_read_raw_ref Han-Wen Nienhuys via GitGitGadget
2021-06-14 10:10   ` [PATCH v2 0/8] refs: cleanup errno sideband ref related functions Han-Wen Nienhuys

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git