{"thread":{"id":"55545","subject":"[PATCH] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn","startedAt":"2021-04-23T10:24:11Z","lastAt":"2021-04-29T08:52:18Z","messageCount":11,"participants":["Han-Wen Nienhuys via GitGitGadget","Jeff King","Han-Wen Nienhuys","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"422747","messageId":"pull.1011.git.git.1619173446857.gitgitgadget@gmail.com","threadId":"55545","inReplyTo":null,"subject":"[PATCH] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-23T10:24:06Z","receivedAt":"2021-04-23T10:24:11Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nA grep for EINVAL */*c reveals that no code inspects EINVAL after reading\nreferences.\n\nThe files ref backend does use EINVAL so parse_loose_ref_contents() can\ncommunicate to lock_raw_ref() about garbage following the hex SHA1, or a short\nread in files_read_raw_ref(), but the files backend does not call into\nrefs_read_raw_ref(), so its EINVAL sideband error is unused.\n\nAs the errno sideband is unintuitive and error-prone, remove EINVAL\nvalue, as a step towards getting rid of the errno sideband altogether.\n\nSpotted by Ævar Arnfjörð Bjarmason <avarab@gmail.com>.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n    refs: remove EINVAL specification from the errno sideband in read_raw…\n    \n    …_ref_fn\n    \n    A grep for EINVAL */*c reveals that no code inspects EINVAL after\n    reading references.\n    \n    The files ref backend does use EINVAL so parse_loose_ref_contents() can\n    communicate to lock_raw_ref() about garbage following the hex SHA1, or a\n    short read in files_read_raw_ref(), but the files backend does not call\n    into refs_read_raw_ref(), so its EINVAL sideband error is unused.\n    \n    As the errno sideband is unintuitive and error-prone, remove EINVAL\n    value, as a step towards getting rid of the errno sideband altogether.\n    \n    Spotted by Ævar Arnfjörð Bjarmason avarab@gmail.com.\n    \n    Signed-off-by: Han-Wen Nienhuys hanwen@google.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1011%2Fhanwen%2Feinval-sideband-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1011/hanwen/einval-sideband-v1\nPull-Request: https://github.com/git/git/pull/1011\n\n refs.c               | 2 --\n refs/refs-internal.h | 9 ++++-----\n 2 files changed, 4 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 261fd82beb98..3179ebd71b2f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1705,7 +1705,6 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n \t\tif (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||\n \t\t    !refname_is_safe(refname)) {\n-\t\t\terrno = EINVAL;\n \t\t\treturn NULL;\n \t\t}\n \n@@ -1765,7 +1764,6 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n \t\t\tif (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||\n \t\t\t    !refname_is_safe(refname)) {\n-\t\t\t\terrno = EINVAL;\n \t\t\t\treturn NULL;\n \t\t\t}\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 467f4b3c936d..29728a339fed 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -617,11 +617,10 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,\n  * properly-formatted or even safe reference name. NEITHER INPUT NOR\n  * OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION.\n  *\n- * Return 0 on success. If the ref doesn't exist, set errno to ENOENT\n- * and return -1. If the ref exists but is neither a symbolic ref nor\n- * an object ID, it is broken; set REF_ISBROKEN in type, set errno to\n- * EINVAL, and return -1. If there is another error reading the ref,\n- * set errno appropriately and return -1.\n+ * Return 0 on success. If the ref doesn't exist, set errno to ENOENT and return\n+ * -1. If the ref exists but is neither a symbolic ref nor an object ID, it is\n+ * broken; set REF_ISBROKEN in type, and return -1. If there is another error\n+ * reading the ref, set errno appropriately and return -1.\n  *\n  * Backend-specific flags might be set in type as well, regardless of\n  * outcome.\n\nbase-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a\n-- \ngitgitgadget\n"},{"id":"422751","messageId":"YILENm8vZE28HyuZ@coredump.intra.peff.net","threadId":"55545","inReplyTo":"pull.1011.git.git.1619173446857.gitgitgadget@gmail.com","subject":"Re: [PATCH] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-04-23T12:57:26Z","receivedAt":"2021-04-23T12:57:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 23, 2021 at 10:24:06AM +0000, Han-Wen Nienhuys via GitGitGadget wrote:\n\n> Subject: refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn\n\nThe subject says \"read_raw_ref_fn\", but the patch is touching\nrefs_resolve_ref_unsafe(). The former is an abstract type, and I didn't\ndig to see the relationships, but I'll focus on the code change in the\npatch.\n\n> A grep for EINVAL */*c reveals that no code inspects EINVAL after reading\n> references.\n\nI don't think that's sufficient, for two reasons:\n\n  - in general we try to be careful about forks and topics in flight,\n    which might end up with semantic conflicts. So we don't necessarily\n    assume that we can see all code, and prefer if any subtle changes\n    like this at least result in a compile failure (e.g., changing\n    function name or signature). In practice, this is balanced with how\n    likely such code is, how bad the breakage would be, what we're\n    gaining, etc.\n\n  - just because they are not looking for EINVAL specifically doesn't\n    mean they are not looking at errno at all (e.g., after calling\n    refs_resolve_ref_unsafe(), lock_ref_oid_basic() does so). So we have\n    to set errno to _something_ after the error. After your patch, we\n    don't set it at all for these error returns, and so we'll be left\n    with whatever junk was in errno from a previous unrelated syscall,\n    which could be very misleading. Since we have to set it to\n    something, EINVAL seems like a reasonable value.\n\nI certainly buy the argument that errno is a pretty lousy channel for\npassing back error data, for a number of reasons.  If we were going all\nthe way towards getting rid of errno in this function (and replacing it\nwith something better, as we must, since some callers _do_ care about\nmore detailed information), I could see the value. But this patch\ndoesn't get us anywhere useful and risks regressions in the meantime.\n\n-Peff\n"},{"id":"422758","messageId":"CAFQ2z_NxSLN-wNnk5gByit+_uhC9j-1QEt1=MZEk-kH5ztU6Jg@mail.gmail.com","threadId":"55545","inReplyTo":"YILENm8vZE28HyuZ@coredump.intra.peff.net","subject":"Re: [PATCH] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-04-23T15:25:52Z","receivedAt":"2021-04-23T15:26:08Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Fri, Apr 23, 2021 at 2:57 PM Jeff King <peff@peff.net> wrote:\n>\n> On Fri, Apr 23, 2021 at 10:24:06AM +0000, Han-Wen Nienhuys via GitGitGadget wrote:\n>\n> > Subject: refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn\n>\n> The subject says \"read_raw_ref_fn\", but the patch is touching\n> refs_resolve_ref_unsafe(). The former is an abstract type, and I didn't\n> dig to see the relationships, but I'll focus on the code change in the\n> patch.\n\nWell spotted. I reverted this part (I did glance over existing\ncallers, and couldn't find anyone inspecting errno)\n\n> > A grep for EINVAL */*c reveals that no code inspects EINVAL after reading\n> > references.\n>\n> I don't think that's sufficient, for two reasons:\n>\n>   - in general we try to be careful about forks and topics in flight,\n>     which might end up with semantic conflicts. So we don't necessarily\n>     assume that we can see all code, and prefer if any subtle changes\n>     like this at least result in a compile failure (e.g., changing\n>     function name or signature). In practice, this is balanced with how\n>     likely such code is, how bad the breakage would be, what we're\n>     gaining, etc.\n\nwould you say this is warranted here? refs.h doesn't mention the word\nerrno, so this behavior isn't documented at all. I also looked over\nthe current callers of read_raw_ref, and outside of refs/*.c none seem\nto inspect errno.\n\n>   - just because they are not looking for EINVAL specifically doesn't\n>     mean they are not looking at errno at all (e.g., after calling\n>     refs_resolve_ref_unsafe(), lock_ref_oid_basic() does so). So we have\n>     to set errno to _something_ after the error. After your patch, we\n>     don't set it at all for these error returns, and so we'll be left\n>     with whatever junk was in errno from a previous unrelated syscall,\n>     which could be very misleading. Since we have to set it to\n>     something, EINVAL seems like a reasonable value.\n\nThe function has several exit paths that don't set errno at all, so\nthe result is kind of random anyway, but I can't see the code I don't\nhave. I've updated the series, with some real progress to stamping out\nerrno. Hope this pleases you better.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"422759","messageId":"pull.1011.v2.git.git.1619191907.gitgitgadget@gmail.com","threadId":"55545","inReplyTo":"pull.1011.git.git.1619173446857.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] refs: cleanup errno sideband ref related functions.","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-23T15:31:44Z","receivedAt":"2021-04-23T15:31:51Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"v2:\n\n * peff's feedback. For now, leave refs.c alone; instead cleanups in\n   files-backend and the ref backend API.\n\nHan-Wen Nienhuys (3):\n  refs: remove EINVAL specification from the errno sideband in\n    read_raw_ref_fn\n  refs/files-backend: stop setting errno from lock_ref_oid_basic\n  refs: make errno output explicit for read_raw_ref_fn\n\n refs.c                |  7 +++++--\n refs/debug.c          |  4 ++--\n refs/files-backend.c  | 26 +++++++++-----------------\n refs/packed-backend.c |  5 +++--\n refs/refs-internal.h  | 14 ++++++++------\n 5 files changed, 27 insertions(+), 29 deletions(-)\n\n\nbase-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1011%2Fhanwen%2Feinval-sideband-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1011/hanwen/einval-sideband-v2\nPull-Request: https://github.com/git/git/pull/1011\n\nRange-diff vs v1:\n\n 1:  ed080f6a9c40 ! 1:  7e8181e77d40 refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn\n     @@ Commit message\n      \n          Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>\n      \n     - ## refs.c ##\n     -@@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n     - \tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n     - \t\tif (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||\n     - \t\t    !refname_is_safe(refname)) {\n     --\t\t\terrno = EINVAL;\n     - \t\t\treturn NULL;\n     - \t\t}\n     - \n     -@@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n     - \t\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n     - \t\t\tif (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||\n     - \t\t\t    !refname_is_safe(refname)) {\n     --\t\t\t\terrno = EINVAL;\n     - \t\t\t\treturn NULL;\n     - \t\t\t}\n     - \n     -\n       ## refs/refs-internal.h ##\n      @@ refs/refs-internal.h: typedef int reflog_expire_fn(struct ref_store *ref_store,\n        * properly-formatted or even safe reference name. NEITHER INPUT NOR\n -:  ------------ > 2:  db5da7d7fb51 refs/files-backend: stop setting errno from lock_ref_oid_basic\n -:  ------------ > 3:  7fbc1c754f43 refs: make errno output explicit for read_raw_ref_fn\n\n-- \ngitgitgadget\n"},{"id":"422760","messageId":"7e8181e77d409af7595e357ad233b7781e026b78.1619191907.git.gitgitgadget@gmail.com","threadId":"55545","inReplyTo":"pull.1011.v2.git.git.1619191907.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-23T15:31:45Z","receivedAt":"2021-04-23T15:31:52Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nA grep for EINVAL */*c reveals that no code inspects EINVAL after reading\nreferences.\n\nThe files ref backend does use EINVAL so parse_loose_ref_contents() can\ncommunicate to lock_raw_ref() about garbage following the hex SHA1, or a short\nread in files_read_raw_ref(), but the files backend does not call into\nrefs_read_raw_ref(), so its EINVAL sideband error is unused.\n\nAs the errno sideband is unintuitive and error-prone, remove EINVAL\nvalue, as a step towards getting rid of the errno sideband altogether.\n\nSpotted by Ævar Arnfjörð Bjarmason <avarab@gmail.com>.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n refs/refs-internal.h | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 467f4b3c936d..29728a339fed 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -617,11 +617,10 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,\n  * properly-formatted or even safe reference name. NEITHER INPUT NOR\n  * OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION.\n  *\n- * Return 0 on success. If the ref doesn't exist, set errno to ENOENT\n- * and return -1. If the ref exists but is neither a symbolic ref nor\n- * an object ID, it is broken; set REF_ISBROKEN in type, set errno to\n- * EINVAL, and return -1. If there is another error reading the ref,\n- * set errno appropriately and return -1.\n+ * Return 0 on success. If the ref doesn't exist, set errno to ENOENT and return\n+ * -1. If the ref exists but is neither a symbolic ref nor an object ID, it is\n+ * broken; set REF_ISBROKEN in type, and return -1. If there is another error\n+ * reading the ref, set errno appropriately and return -1.\n  *\n  * Backend-specific flags might be set in type as well, regardless of\n  * outcome.\n-- \ngitgitgadget\n\n"},{"id":"422761","messageId":"db5da7d7fb5178c14c1f5733d35bb69813c9c644.1619191907.git.gitgitgadget@gmail.com","threadId":"55545","inReplyTo":"pull.1011.v2.git.git.1619191907.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] refs/files-backend: stop setting errno from lock_ref_oid_basic","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-23T15:31:46Z","receivedAt":"2021-04-23T15:31:53Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nErrno is a global variable written by almost all system calls, and therefore it\nis hard to reason about its state.\n\nThis is a functional noop, because calls to lock_ref_oid_basic() in this file\nare followed by:\n\n* lock_ref_oid_basic (copy/rename rollback error path)\n\n* write_ref_to_lockfile (both in the rollback path and the success path of\n  copy/rename)\n\n* create_symref_locked (files_create_symref)\n\n* refs_reflog_exists (reflog expiry)\n\nThese calls do I/O and therefore clobber errno. They are not inspecting the\nincoming errno.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n refs/files-backend.c | 9 +--------\n 1 file changed, 1 insertion(+), 8 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 119972ee16f8..c9511da1d387 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -910,7 +910,6 @@ static int create_reflock(const char *path, void *cb)\n \n /*\n  * Locks a ref returning the lock on success and NULL on failure.\n- * On failure errno is set to something meaningful.\n  */\n static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n \t\t\t\t\t   const char *refname,\n@@ -922,7 +921,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n {\n \tstruct strbuf ref_file = STRBUF_INIT;\n \tstruct ref_lock *lock;\n-\tint last_errno = 0;\n \tint mustexist = (old_oid && !is_null_oid(old_oid));\n \tint resolve_flags = RESOLVE_REF_NO_RECURSE;\n \tint resolved;\n@@ -949,7 +947,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n \t\t * to remain.\n \t\t */\n \t\tif (remove_empty_directories(&ref_file)) {\n-\t\t\tlast_errno = errno;\n \t\t\tif (!refs_verify_refname_available(\n \t\t\t\t\t    &refs->base,\n \t\t\t\t\t    refname, extras, skip, err))\n@@ -962,7 +959,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n \t\t\t\t\t\t     &lock->old_oid, type);\n \t}\n \tif (!resolved) {\n-\t\tlast_errno = errno;\n+\t\tint last_errno = errno;\n \t\tif (last_errno != ENOTDIR ||\n \t\t    !refs_verify_refname_available(&refs->base, refname,\n \t\t\t\t\t\t   extras, skip, err))\n@@ -981,20 +978,17 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n \tif (is_null_oid(&lock->old_oid) &&\n \t    refs_verify_refname_available(refs->packed_ref_store, refname,\n \t\t\t\t\t  extras, skip, err)) {\n-\t\tlast_errno = ENOTDIR;\n \t\tgoto error_return;\n \t}\n \n \tlock->ref_name = xstrdup(refname);\n \n \tif (raceproof_create_file(ref_file.buf, create_reflock, &lock->lk)) {\n-\t\tlast_errno = errno;\n \t\tunable_to_lock_message(ref_file.buf, errno, err);\n \t\tgoto error_return;\n \t}\n \n \tif (verify_lock(&refs->base, lock, old_oid, mustexist, err)) {\n-\t\tlast_errno = errno;\n \t\tgoto error_return;\n \t}\n \tgoto out;\n@@ -1005,7 +999,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n \n  out:\n \tstrbuf_release(&ref_file);\n-\terrno = last_errno;\n \treturn lock;\n }\n \n-- \ngitgitgadget\n\n"},{"id":"422762","messageId":"7fbc1c754f435c6d621734685ebee1bdc539c000.1619191907.git.gitgitgadget@gmail.com","threadId":"55545","inReplyTo":"pull.1011.v2.git.git.1619191907.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] refs: make errno output explicit for read_raw_ref_fn","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-04-23T15:31:47Z","receivedAt":"2021-04-23T15:31:55Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nread_raw_ref_fn needs to supply a credible errno for a number of cases. These\nare primarily:\n\n1) The files backend calls read_raw_ref from lock_raw_ref, and uses the\nresulting error codes to create/remove directories as needed.\n\n2) ENOENT should be translated in a zero OID, optionally with REF_ISBROKEN set,\nreturning the last successfully resolved symref. This is necessary so\nread_raw_ref(\"HEAD\") on an empty repo returns refs/heads/main (or the default branch\ndu-jour), and we know on which branch to create the first commit.\n\nMake this information flow explicit by adding a failure_errno to the signature\nof read_raw_ref. All errnos from the files backend are still propagated\nunchanged, even though inspection suggests only ENOTDIR, EISDIR and ENOENT are\nrelevant.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n refs.c                |  7 +++++--\n refs/debug.c          |  4 ++--\n refs/files-backend.c  | 17 ++++++++---------\n refs/packed-backend.c |  5 +++--\n refs/refs-internal.h  |  9 ++++++---\n 5 files changed, 24 insertions(+), 18 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 261fd82beb98..43e2ad6b612a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1675,13 +1675,16 @@ int refs_read_raw_ref(struct ref_store *ref_store,\n \t\t      const char *refname, struct object_id *oid,\n \t\t      struct strbuf *referent, unsigned int *type)\n {\n+\tint result, failure;\n \tif (!strcmp(refname, \"FETCH_HEAD\") || !strcmp(refname, \"MERGE_HEAD\")) {\n \t\treturn refs_read_special_head(ref_store, refname, oid, referent,\n \t\t\t\t\t      type);\n \t}\n \n-\treturn ref_store->be->read_raw_ref(ref_store, refname, oid, referent,\n-\t\t\t\t\t   type);\n+\tresult = ref_store->be->read_raw_ref(ref_store, refname, oid, referent,\n+\t\t\t\t\t     type, &failure);\n+\terrno = failure;\n+\treturn result;\n }\n \n /* This function needs to return a meaningful errno on failure */\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 922e64fa6ad9..887dbb14be6e 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -238,14 +238,14 @@ debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,\n \n static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t\t\t      struct object_id *oid, struct strbuf *referent,\n-\t\t\t      unsigned int *type)\n+\t\t\t      unsigned int *type, int *failure_errno)\n {\n \tstruct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;\n \tint res = 0;\n \n \toidcpy(oid, &null_oid);\n \tres = drefs->refs->be->read_raw_ref(drefs->refs, refname, oid, referent,\n-\t\t\t\t\t    type);\n+\t\t\t\t\t    type, failure_errno);\n \n \tif (res == 0) {\n \t\ttrace_printf_key(&trace_refs, \"read_raw_ref: %s: %s (=> %s) type %x: %d\\n\",\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex c9511da1d387..3ba3a96e1c6b 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -343,7 +343,7 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)\n \n static int files_read_raw_ref(struct ref_store *ref_store,\n \t\t\t      const char *refname, struct object_id *oid,\n-\t\t\t      struct strbuf *referent, unsigned int *type)\n+\t\t\t      struct strbuf *referent, unsigned int *type, int *failure_errno)\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_READ, \"read_raw_ref\");\n@@ -354,7 +354,6 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n \tstruct stat st;\n \tint fd;\n \tint ret = -1;\n-\tint save_errno;\n \tint remaining_retries = 3;\n \n \t*type = 0;\n@@ -459,10 +458,9 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n \tret = parse_loose_ref_contents(buf, oid, referent, type);\n \n out:\n-\tsave_errno = errno;\n+\t*failure_errno = errno;\n \tstrbuf_release(&sb_path);\n \tstrbuf_release(&sb_contents);\n-\terrno = save_errno;\n \treturn ret;\n }\n \n@@ -541,6 +539,7 @@ static int lock_raw_ref(struct files_ref_store *refs,\n \tstruct strbuf ref_file = STRBUF_INIT;\n \tint attempts_remaining = 3;\n \tint ret = TRANSACTION_GENERIC_ERROR;\n+\tint failure_errno = 0;\n \n \tassert(err);\n \tfiles_assert_main_repository(refs, \"lock_raw_ref\");\n@@ -630,8 +629,8 @@ static int lock_raw_ref(struct files_ref_store *refs,\n \t */\n \n \tif (files_read_raw_ref(&refs->base, refname,\n-\t\t\t       &lock->old_oid, referent, type)) {\n-\t\tif (errno == ENOENT) {\n+\t\t\t       &lock->old_oid, referent, type, &failure_errno)) {\n+\t\tif (failure_errno == ENOENT) {\n \t\t\tif (mustexist) {\n \t\t\t\t/* Garden variety missing reference. */\n \t\t\t\tstrbuf_addf(err, \"unable to resolve reference '%s'\",\n@@ -655,7 +654,7 @@ static int lock_raw_ref(struct files_ref_store *refs,\n \t\t\t\t *   reference named \"refs/foo/bar/baz\".\n \t\t\t\t */\n \t\t\t}\n-\t\t} else if (errno == EISDIR) {\n+\t\t} else if (failure_errno == EISDIR) {\n \t\t\t/*\n \t\t\t * There is a directory in the way. It might have\n \t\t\t * contained references that have been deleted. If\n@@ -693,13 +692,13 @@ static int lock_raw_ref(struct files_ref_store *refs,\n \t\t\t\t\tgoto error_return;\n \t\t\t\t}\n \t\t\t}\n-\t\t} else if (errno == EINVAL && (*type & REF_ISBROKEN)) {\n+\t\t} else if (failure_errno == EINVAL && (*type & REF_ISBROKEN)) {\n \t\t\tstrbuf_addf(err, \"unable to resolve reference '%s': \"\n \t\t\t\t    \"reference broken\", refname);\n \t\t\tgoto error_return;\n \t\t} else {\n \t\t\tstrbuf_addf(err, \"unable to resolve reference '%s': %s\",\n-\t\t\t\t    refname, strerror(errno));\n+\t\t\t\t    refname, strerror(failure_errno));\n \t\t\tgoto error_return;\n \t\t}\n \ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex dfecdbc1db60..9a09ad7f5f29 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -726,7 +726,8 @@ static struct snapshot *get_snapshot(struct packed_ref_store *refs)\n \n static int packed_read_raw_ref(struct ref_store *ref_store,\n \t\t\t       const char *refname, struct object_id *oid,\n-\t\t\t       struct strbuf *referent, unsigned int *type)\n+\t\t\t       struct strbuf *referent, unsigned int *type,\n+\t\t\t       int *failure_errno)\n {\n \tstruct packed_ref_store *refs =\n \t\tpacked_downcast(ref_store, REF_STORE_READ, \"read_raw_ref\");\n@@ -739,7 +740,7 @@ static int packed_read_raw_ref(struct ref_store *ref_store,\n \n \tif (!rec) {\n \t\t/* refname is not a packed reference. */\n-\t\terrno = ENOENT;\n+\t\t*failure_errno = ENOENT;\n \t\treturn -1;\n \t}\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 29728a339fed..15cc0ddd68ab 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -617,9 +617,11 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,\n  * properly-formatted or even safe reference name. NEITHER INPUT NOR\n  * OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION.\n  *\n- * Return 0 on success. If the ref doesn't exist, set errno to ENOENT and return\n+ * Return 0 on success. If the ref doesn't exist, set failure_errno to ENOENT and return\n  * -1. If the ref exists but is neither a symbolic ref nor an object ID, it is\n- * broken; set REF_ISBROKEN in type, and return -1. If there is another error\n+ * broken; set REF_ISBROKEN in type, and return -1. For the files backend, EISDIR and ENOTDIR\n+ * may be set if the ref name is a directory\n+ * If there is another error\n  * reading the ref, set errno appropriately and return -1.\n  *\n  * Backend-specific flags might be set in type as well, regardless of\n@@ -636,7 +638,8 @@ typedef int reflog_expire_fn(struct ref_store *ref_store,\n  */\n typedef int read_raw_ref_fn(struct ref_store *ref_store,\n \t\t\t    const char *refname, struct object_id *oid,\n-\t\t\t    struct strbuf *referent, unsigned int *type);\n+\t\t\t    struct strbuf *referent, unsigned int *type,\n+\t\t\t    int *failure_errno);\n \n struct ref_storage_be {\n \tstruct ref_storage_be *next;\n-- \ngitgitgadget\n"},{"id":"423172","messageId":"xmqqzgxjavks.fsf@gitster.g","threadId":"55545","inReplyTo":"db5da7d7fb5178c14c1f5733d35bb69813c9c644.1619191907.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] refs/files-backend: stop setting errno from lock_ref_oid_basic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-28T04:20:51Z","receivedAt":"2021-04-28T04:20:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Han-Wen Nienhuys via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Han-Wen Nienhuys <hanwen@google.com>\n>\n> Errno is a global variable written by almost all system calls, and therefore it\n> is hard to reason about its state.\n>\n> This is a functional noop, because calls to lock_ref_oid_basic() in this file\n> are followed by:\n>\n> * lock_ref_oid_basic (copy/rename rollback error path)\n>\n> * write_ref_to_lockfile (both in the rollback path and the success path of\n>   copy/rename)\n>\n> * create_symref_locked (files_create_symref)\n>\n> * refs_reflog_exists (reflog expiry)\n>\n> These calls do I/O and therefore clobber errno. They are not inspecting the\n> incoming errno.\n\nHmph, are you saying that these calls do I/O and always the I/O\nwould fail?  A system call that is successfull don't touch errno;\nonly the calls that resulted in failure do.\n"},{"id":"423191","messageId":"CAFQ2z_O=4sUjh1wk6nRijp9Gz2eeqX4=EY+Q-OTi9ppb9ikg3g@mail.gmail.com","threadId":"55545","inReplyTo":"xmqqzgxjavks.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] refs/files-backend: stop setting errno from lock_ref_oid_basic","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-04-28T10:55:07Z","receivedAt":"2021-04-28T10:55:23Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Wed, Apr 28, 2021 at 6:20 AM Junio C Hamano <gitster@pobox.com> wrote:\n> > These calls do I/O and therefore clobber errno. They are not inspecting the\n> > incoming errno.\n>\n> Hmph, are you saying that these calls do I/O and always the I/O\n> would fail?  A system call that is successfull don't touch errno;\n> only the calls that resulted in failure do.\n\nI'm saying that callers cannot reliably observe the errno result of\nlock_ref_oid_basic, because it might be clobbered by a failing\nfollow-up call.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"423227","messageId":"xmqqa6pham83.fsf@gitster.g","threadId":"55545","inReplyTo":"CAFQ2z_O=4sUjh1wk6nRijp9Gz2eeqX4=EY+Q-OTi9ppb9ikg3g@mail.gmail.com","subject":"Re: [PATCH v2 2/3] refs/files-backend: stop setting errno from lock_ref_oid_basic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-29T01:55:08Z","receivedAt":"2021-04-29T01:55:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys <hanwen@google.com> writes:\n\n> On Wed, Apr 28, 2021 at 6:20 AM Junio C Hamano <gitster@pobox.com> wrote:\n>> > These calls do I/O and therefore clobber errno. They are not inspecting the\n>> > incoming errno.\n>>\n>> Hmph, are you saying that these calls do I/O and always the I/O\n>> would fail?  A system call that is successfull don't touch errno;\n>> only the calls that resulted in failure do.\n>\n> I'm saying that callers cannot reliably observe the errno result of\n> lock_ref_oid_basic, because it might be clobbered by a failing\n> follow-up call.\n\nSorry, I still do not quite get it.  For example, you cite that a\ncall to lock_ref_oid_basic() in files_create_symref() is followed by\ncreate_symref_locked() that may clobber errno when the latter fails.\n\nBut a failing lock_ref_oid_basic() would yield NULL and causes the\ncaller to leave, before calling create_symref_locked() and letting\nit clobber errno, and the caller of files_create_symref() can\nobserve, when it returns -1 to signal an error, the errno left by\nlock_ref_oid_basic(), no?  I would understand it if no caller of\nfiles_create_symref() cares what is in errno when it receives\nnegative return to signal a failure, though.\n\nAnd when lock_ref_oid_basic() did not fail, create_symref_locked()\ncalls helpers that can fail (e.g. fdopen_lock_file()) and result in\nerrno getting updated to record how it failed (this is also reported\nto the user via \"error(... strerror(errno))\").\n\nSo a caller of files_create_symref() may not be able to tell between\nlock_ref_oid_basic() and create_symref_locked() which one caused the\nfiles_create_symref() call to fail, but in either case it should be\nable to inspect errno to learn what kind of error we got from the\nunderlying system, no?\n\n"},{"id":"423256","messageId":"CAFQ2z_OXicspXXicWAAGUv4TPXnZVBusueminRLH2+EUTjpzxA@mail.gmail.com","threadId":"55545","inReplyTo":"xmqqa6pham83.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] refs/files-backend: stop setting errno from lock_ref_oid_basic","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-04-29T08:52:01Z","receivedAt":"2021-04-29T08:52:18Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Thu, Apr 29, 2021 at 3:55 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Han-Wen Nienhuys <hanwen@google.com> writes:\n>\n> > On Wed, Apr 28, 2021 at 6:20 AM Junio C Hamano <gitster@pobox.com> wrote:\n> >> > These calls do I/O and therefore clobber errno. They are not inspecting the\n> >> > incoming errno.\n> >>\n> >> Hmph, are you saying that these calls do I/O and always the I/O\n> >> would fail?  A system call that is successfull don't touch errno;\n> >> only the calls that resulted in failure do.\n> >\n> > I'm saying that callers cannot reliably observe the errno result of\n> > lock_ref_oid_basic, because it might be clobbered by a failing\n> > follow-up call.\n>\n> Sorry, I still do not quite get it.  For example, you cite that a\n> call to lock_ref_oid_basic() in files_create_symref() is followed by\n> create_symref_locked() that may clobber errno when the latter fails.\n>\n> But a failing lock_ref_oid_basic() would yield NULL and causes the\n> caller to leave, before calling create_symref_locked() and letting\n> it clobber errno, and the caller of files_create_symref() can\n> observe, when it returns -1 to signal an error, the errno left by\n> lock_ref_oid_basic(), no?  I would understand it if no caller of\n> files_create_symref() cares what is in errno when it receives\n> negative return to signal a failure, though.\n\nYou're right; I didn't look carefully enough.  I did a grep over the\nsource code for create_symref() now, and couldn't find callers that\ninspect errno; the same for reflog_expire().\n\nI'll update the commit message to reflect this.\n\n> And when lock_ref_oid_basic() did not fail, create_symref_locked()\n> calls helpers that can fail (e.g. fdopen_lock_file()) and result in\n> errno getting updated to record how it failed (this is also reported\n> to the user via \"error(... strerror(errno))\").\n>\n> So a caller of files_create_symref() may not be able to tell between\n> lock_ref_oid_basic() and create_symref_locked() which one caused the\n> files_create_symref() call to fail, but in either case it should be\n> able to inspect errno to learn what kind of error we got from the\n> underlying system, no?\n\nI disagree.  create_symref in the refs API gets an error strbuf_t. If\nthe function wants to say something to the user, it should use that\nmechanism. If other operations are meant to provide reasonable error\nmessages, they should also get an error strbuf.\n\nThe files backend touches many files as part of its operation. If the\nerror is something like EPERM, errno reporting leaves no channel to\ndescribe which file and which syscall is the offending one (is it\npacked-refs.lock, refs/heads/branch.lock, refs/heads/ ; is it the\ncreat/rename/unlink syscall?). It's not a realistic mechanism to use\nfor errors that are meant to be understandable for users.\n\nThe errno mechanism is also poorly adjusted for alternate backends. If\nthere is corrupted data in a reftable file, the library returns\nREFTABLE_FORMAT_ERROR, but what errno would correspond to that?\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"}]}