{"thread":{"id":"56705","subject":"[PATCH 01/20] branch tests: test for errno propagating on failing read","startedAt":"2021-10-14T00:06:38Z","lastAt":"2021-10-16T09:41:18Z","messageCount":45,"participants":["Ævar Arnfjörð Bjarmason","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":20},"messages":[{"id":"438718","messageId":"patch-01.20-bea88e382c0-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 01/20] branch tests: test for errno propagating on failing read","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:13Z","receivedAt":"2021-10-14T00:06:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Han-Wen Nienhuys \n\nAdd a test for \"git branch\" to cover the case where .git/refs is\nsymlinked. To check availability, refs_verify_refname_available() will\nrun refs_read_raw_ref() on each prefix, leading to a read() from\n.git/refs (which is a directory).\n\nIt would probably be more robust to re-issue the lstat() as a normal\nstat(), in which case, we would fall back to the directory case, but\nfor now let's just test for the existing behavior as-is. This test\ncovers a regression in a commit that only ever made it to \"next\", see\n[1].\n\n1. http://lore.kernel.org/git/pull.1068.git.git.1629203489546.gitgitgadget@gmail.com\n\nSigned-off-by: Han-Wen Nienhuys \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t3200-branch.sh | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex e575ffb4ffb..b5242719159 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -731,6 +731,26 @@ test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog for\n \ttest_must_fail git branch -m u v\n '\n \n+test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '\n+\ttest_when_finished \"rm -rf subdir\" &&\n+\tgit init subdir &&\n+\n+\t(\n+\t\tcd subdir &&\n+\t\tfor d in refs objects packed-refs\n+\t\tdo\n+\t\t\trm -rf .git/$d &&\n+\t\t\tln -s ../../.git/$d .git/$d\n+\t\tdone\n+\t) &&\n+\tgit --git-dir subdir/.git/ branch rename-src &&\n+\tgit rev-parse rename-src >expect &&\n+\tgit --git-dir subdir/.git/ branch -m rename-src rename-dest &&\n+\tgit rev-parse rename-dest >actual &&\n+\ttest_cmp expect actual &&\n+\tgit branch -D rename-dest\n+'\n+\n test_expect_success 'test tracking setup via --track' '\n \tgit config remote.local.url . &&\n \tgit config remote.local.fetch refs/heads/*:refs/remotes/local/* &&\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438719","messageId":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":null,"subject":"[PATCH 00/20] refs: stop having the API set \"errno\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:12Z","receivedAt":"2021-10-14T00:06:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series changes the refs API to stop explicitly or implicitly\nexposing \"errno\" to indicate its state, and instead moves to pushing\nup an explicit \"failure_errno\" variable, or in other cases we migrate\ncallers entirely away from getting any errno values at all (because\nthey never cared).\n\nThis makes the eventual integration of the reftable backend much\neasier to reason about, because we can be assured that the refs.c\nfunction interface is everything we need to care about, and that some\ncallers e.g. don't expect the reftable backend to fake up certain\nerrno values in case of certain types of failures.\n\nThis topic replaces ab/refs-errno-cleanup, which was the result of\nJunio peeling off the first 4 patches of this 10-part series:\nhttps://lore.kernel.org/git/cover-v10-0.8-00000000000-20210823T114712Z-avarab@gmail.com/\n\nIt's been marked as \"meh\" for a while in What's Cooking, I think just\ndoing those first patches (or those 10) doesn't really show the\nend-state benefit, so here's all of it.\n\nIn re-rolling this I found more cases where we could remove dead code\nin the refs file backend, which made this easier, see in particular\n06/20. Patches 08-18/20 are piecemeal moves away from setting \"errno\"\n(which in most cases wasn't needed), and finally 19-20/20 removes a\ntransitory API function used in this series so that this migration\ncould be done incrementally.\n\nHan-Wen Nienhuys (3):\n branch tests: test for errno propagating on failing read\n refs API: make refs_read_raw_ref() not set errno\n refs API: make parse_loose_ref_contents() not set errno\n\nÆvar Arnfjörð Bjarmason (17):\n refs API: add a version of refs_resolve_ref_unsafe() with \"errno\"\n refs API: make refs_rename_ref_available() static\n refs/files: remove \"name exist?\" check in lock_ref_oid_basic()\n refs API: remove refs_read_ref_full() wrapper\n refs API: make resolve_gitlink_ref() not set errno\n refs API: make loose_fill_ref_dir() not set errno\n refs API: make files_copy_or_rename_ref() et al not set errno\n refs API: ignore errno in worktree.c's add_head_info()\n refs API: ignore errno in worktree.c's find_shared_symref()\n refs tests: ignore ignore errno in test-ref-store helper\n refs API: make refs_resolve_refdup() not set errno\n refs API: make refs_ref_exists() not set errno\n refs API: make resolve_ref_unsafe() not set errno\n refs API: make expand_ref() & repo_dwim_log() not set errno\n refs API: don't expose \"errno\" in run_transaction_hook()\n refs API: post-migration API renaming [1/2]\n refs API: post-migration API renaming [2/2]\n\n refs.c | 124 +++++++++++++++++++------------------\n refs.h | 10 ++-\n refs/files-backend.c | 125 +++++++++++++++++++++++++-------------\n refs/packed-backend.c | 7 ++-\n refs/refs-internal.h | 26 +++-----\n sequencer.c | 10 ++-\n t/helper/test-ref-store.c | 3 +-\n t/t3200-branch.sh | 21 +++++++\n worktree.c | 17 ++++--\n 9 files changed, 209 insertions(+), 134 deletions(-)\n\nRange-diff:\n 6: 6dae8b643ad = 1: bea88e382c0 branch tests: test for errno propagating on failing read\n 4: 05f3a346e3b ! 2: 46641111885 refs: make errno output explicit for read_raw_ref_fn\n @@\n ## Metadata ##\n -Author: Han-Wen Nienhuys \n +Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - refs: make errno output explicit for read_raw_ref_fn\n + refs API: add a version of refs_resolve_ref_unsafe() with \"errno\"\n \n - This makes it explicit how alternative ref backends should report errors in\n - read_raw_ref_fn.\n + Add a new refs_werrres_ref_unsafe() function, which is like\n + refs_resolve_ref_unsafe() except that it explicitly saves away the\n + \"errno\" to a passed-in parameter, the refs_resolve_ref_unsafe() then\n + becomes a wrapper for it.\n \n - read_raw_ref_fn needs to supply a credible errno for a number of cases. These\n - are primarily:\n + In subsequent commits we'll migrate code over to it, before finally\n + making \"refs_resolve_ref_unsafe()\" with an \"errno\" parameter the\n + canonical version, so this this function exists only so that we can\n + incrementally migrate callers, it will be going away in a subsequent\n + commit.\n \n - 1) The files backend calls read_raw_ref from lock_raw_ref, and uses the\n - resulting error codes to create/remove directories as needed.\n + As the added comment notes has a rather tortured name to be the same\n + length as \"refs_resolve_ref_unsafe\", to avoid churn as we won't need\n + to re-indent the argument lists, similarly the documentation and\n + structure of it in refs.h is designed to minimize a diff in a\n + subsequent commit, where that documentation will be added to the new\n + refs_resolve_ref_unsafe().\n \n - 2) ENOENT should be translated in a zero OID, optionally with REF_ISBROKEN set,\n - returning the last successfully resolved symref. This is necessary so\n - read_raw_ref(\"HEAD\") on an empty repo returns refs/heads/main (or the default branch\n - du-jour), and we know on which branch to create the first commit.\n + At the end of this migration the \"meaningful errno\" TODO item left in\n + 76d70dc0c63 (refs.c: make resolve_ref_unsafe set errno to something\n + meaningful on error, 2014-06-20) will be resolved.\n \n - Make this information flow explicit by adding a failure_errno to the signature\n - of read_raw_ref. All errnos from the files backend are still propagated\n - unchanged, even though inspection suggests only ENOTDIR, EISDIR and ENOENT are\n - relevant.\n + As can be seen from the use of refs_read_raw_ref() we'll also need to\n + convert some functions that the new refs_werrres_ref_unsafe() itself\n + calls to take this \"failure_errno\". That will be done in subsequent\n + commits.\n \n Signed-off-by: Han-Wen Nienhuys \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## refs.c ##\n @@ refs.c: int refs_read_raw_ref(struct ref_store *ref_store,\n - \t}\n - \n - \treturn ref_store->be->read_raw_ref(ref_store, refname, oid, referent,\n --\t\t\t\t\t type);\n -+\t\t\t\t\t type, &errno);\n - }\n - \n - /* This function needs to return a meaningful errno on failure */\n -\n - ## refs/debug.c ##\n -@@ refs/debug.c: 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 --\terrno = 0;\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\",\n -@@ refs/debug.c: static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname,\n - \t} else {\n - \t\ttrace_printf_key(&trace_refs,\n - \t\t\t\t \"read_raw_ref: %s: %d (errno %d)\\n\", refname,\n --\t\t\t\t res, errno);\n -+\t\t\t\t res, *failure_errno);\n - \t}\n - \treturn res;\n - }\n -\n - ## refs/files-backend.c ##\n -@@ refs/files-backend.c: static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)\n - \treturn refs->loose;\n + \t\t\t\t\t type, &errno);\n }\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 -+static int files_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, int *failure_errno)\n +-/* This function needs to return a meaningful errno on failure */\n +-const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n ++const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n + \t\t\t\t const char *refname,\n + \t\t\t\t int resolve_flags,\n +-\t\t\t\t struct object_id *oid, int *flags)\n ++\t\t\t\t struct object_id *oid,\n ++\t\t\t\t int *flags, int *failure_errno)\n {\n - \tstruct files_ref_store *refs =\n - \t\tfiles_downcast(ref_store, REF_STORE_READ, \"read_raw_ref\");\n -@@ refs/files-backend.c: 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 -@@ refs/files-backend.c: 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 + \tstatic struct strbuf sb_refname = STRBUF_INIT;\n + \tstruct object_id unused_oid;\n + \tint unused_flags;\n + \tint symref_count;\n + \n ++\tassert(failure_errno);\n ++\n + \tif (!oid)\n + \t\toid = &unused_oid;\n + \tif (!flags)\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\t*failure_errno = EINVAL;\n + \t\t\treturn NULL;\n + \t\t}\n \n -@@ refs/files-backend.c: 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;\n +@@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n + \tfor (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) {\n + \t\tunsigned int read_flags = 0;\n \n - \tassert(err);\n - \tfiles_assert_main_repository(refs, \"lock_raw_ref\");\n -@@ refs/files-backend.c: static int lock_raw_ref(struct files_ref_store *refs,\n - \tif (hold_lock_file_for_update_timeout(\n - \t\t\t &lock->lk, ref_file.buf, LOCK_NO_DEREF,\n - \t\t\t get_files_ref_lock_timeout_ms()) < 0) {\n --\t\tif (errno == ENOENT && --attempts_remaining > 0) {\n -+\t\tint myerr = errno;\n +\t\terrno = 0;\n -+\t\tif (myerr == ENOENT && --attempts_remaining > 0) {\n - \t\t\t/*\n - \t\t\t * Maybe somebody just deleted one of the\n - \t\t\t * directories leading to ref_file. Try\n -@@ refs/files-backend.c: static int lock_raw_ref(struct files_ref_store *refs,\n + \t\tif (refs_read_raw_ref(refs, refname,\n + \t\t\t\t oid, &sb_refname, &read_flags)) {\n + \t\t\t*flags |= read_flags;\n ++\t\t\tif (errno)\n ++\t\t\t\t*failure_errno = errno;\n + \n + \t\t\t/* In reading mode, refs must eventually resolve */\n + \t\t\tif (resolve_flags & RESOLVE_REF_READING)\n +@@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n + \t\t\t * may show errors besides ENOENT if there are\n + \t\t\t * similarly-named refs.\n \t\t\t */\n - \t\t\tgoto retry;\n - \t\t} else {\n --\t\t\tunable_to_lock_message(ref_file.buf, errno, err);\n -+\t\t\tunable_to_lock_message(ref_file.buf, myerr, err);\n - \t\t\tgoto error_return;\n - \t\t}\n - \t}\n -@@ refs/files-backend.c: static int lock_raw_ref(struct files_ref_store *refs,\n - \t * fear that its value will change.\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 -+\tif (files_read_raw_ref(&refs->base, refname, &lock->old_oid, referent,\n -+\t\t\t 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 -@@ refs/files-backend.c: 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 -@@ refs/files-backend.c: 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\tif (errno != ENOENT &&\n +-\t\t\t errno != EISDIR &&\n +-\t\t\t errno != ENOTDIR)\n ++\t\t\tif (*failure_errno != ENOENT &&\n ++\t\t\t *failure_errno != EISDIR &&\n ++\t\t\t *failure_errno != ENOTDIR)\n + \t\t\t\treturn NULL;\n + \n + \t\t\toidclr(oid);\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\t*failure_errno = EINVAL;\n + \t\t\t\treturn NULL;\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 + \n +@@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t}\n + \t}\n \n -\n - ## refs/packed-backend.c ##\n -@@ refs/packed-backend.c: static struct snapshot *get_snapshot(struct packed_ref_store *refs)\n - \treturn refs->snapshot;\n +-\terrno = ELOOP;\n ++\t*failure_errno = ELOOP;\n + \treturn NULL;\n }\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 -+static int packed_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, int *failure_errno)\n ++const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname,\n ++\t\t\t\t int resolve_flags, struct object_id *oid,\n ++\t\t\t\t int *flags)\n ++{\n ++\tint failure_errno = 0;\n ++\tconst char *refn;\n ++\trefn = refs_werrres_ref_unsafe(refs, refname, resolve_flags,\n ++\t\t\t\t oid, flags, &failure_errno);\n ++\tif (!refn)\n ++\t\terrno = failure_errno;\n ++\treturn refn;\n ++}\n ++\n + /* backend functions */\n + int refs_init_db(struct strbuf *err)\n {\n - \tstruct packed_ref_store *refs =\n - \t\tpacked_downcast(ref_store, REF_STORE_READ, \"read_raw_ref\");\n -@@ refs/packed-backend.c: 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 - \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 - * 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, and return -1\n -- * (errno should not be ENOENT) If there is another error reading the\n -- * ref, set errno appropriately and return -1.\n -+ * Return 0 on success, or -1 on failure. If the ref exists but is neither a\n -+ * symbolic ref nor an object ID, it is broken. In this case set REF_ISBROKEN in\n -+ * type, and return -1 (failure_errno should not be ENOENT)\n -+ *\n -+ * failure_errno provides errno codes that are interpreted beyond error\n -+ * reporting. The following error codes have special meaning:\n -+ * * ENOENT: the ref doesn't exist\n -+ * * EISDIR: ref name is a directory\n -+ * * ENOTDIR: ref prefix is not a directory\n + ## refs.h ##\n +@@ refs.h: struct string_list;\n + struct string_list_item;\n + struct worktree;\n + \n ++/*\n ++ * Callers should not inspect \"errno\" on failure, but rather pass in a\n ++ * \"failure_errno\" parameter, on failure the \"errno\" will indicate the\n ++ * type of failure encountered, but not necessarily one that came from\n ++ * a syscall. We might have faked it up.\n ++ */\n ++const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n ++\t\t\t\t const char *refname,\n ++\t\t\t\t int resolve_flags,\n ++\t\t\t\t struct object_id *oid,\n ++\t\t\t\t int *flags, int *failure_errno);\n ++\n + /*\n + * Resolve a reference, recursively following symbolic refererences.\n *\n - * Backend-specific flags might be set in type as well, regardless of\n - * outcome.\n -@@ refs/refs-internal.h: typedef int reflog_expire_fn(struct ref_store *ref_store,\n - * - in all other cases, referent will be untouched, and therefore\n - * refname will still be valid and unchanged.\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 -+typedef int read_raw_ref_fn(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, int *failure_errno);\n - \n - struct ref_storage_be {\n - \tstruct ref_storage_be *next;\n 5: fa9260f25fa ! 3: a1a80715ffe refs: add failure_errno to refs_read_raw_ref() signature\n @@ Metadata\n Author: Han-Wen Nienhuys \n \n ## Commit message ##\n - refs: add failure_errno to refs_read_raw_ref() signature\n + refs API: make refs_read_raw_ref() not set errno\n \n - This lets us use the explicit errno output parameter in refs_resolve_ref_unsafe.\n -\n - Some of our callers explicitly do not care about the errno, rather\n - than understanding NULL let's have them declare that they don't care\n - by passing in an \"ignore_errno\". There's only three of them, and using\n - that pattern will make it more obvious that they want to throw away\n - data, let's also add a comment to one of the callers about why we'd\n - like to ignore the errno.\n -\n - Let's not extend that to refs_resolve_ref_unsafe() itself for now, it\n - has a large set of legacy callers, so we're faking up the old \"errno\"\n - behavior for it. We can convert those callers to\n - refs_resolve_ref_unsafe_with_errno() later.\n + Add a \"failure_errno\" to refs_read_raw_ref(), his allows\n + refs_werrres_ref_unsafe() to pass along its \"failure_errno\", as a\n + first step before its own callers are migrated to pass it further up\n + the chain.\n \n We are leaving out out the refs_read_special_head() in\n - refs_read_raw_ref() for now, as noted in the next commit moving it to\n - \"failure_errno\" will require some special consideration.\n -\n - We're intentionally mis-indenting the argument list of the new\n - refs_resolve_ref_unsafe_with_errno(), it will be non-static in a\n - subsequent commit, doing it this way makes that diff smaller.\n + refs_read_raw_ref() for now, as noted in a subsequent commit moving it\n + to \"failure_errno\" will require some special consideration.\n \n Signed-off-by: Han-Wen Nienhuys \n Signed-off-by: Ævar Arnfjörð Bjarmason \n @@ refs.c: static int refs_read_special_head(struct ref_store *ref_store,\n +\t\t\t\t\t type, failure_errno);\n }\n \n --/* This function needs to return a meaningful errno on failure */\n --const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n --\t\t\t\t const char *refname,\n --\t\t\t\t int resolve_flags,\n --\t\t\t\t struct object_id *oid, int *flags)\n -+static const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs,\n -+\t\t\t\t\t const char *refname,\n -+\t\t\t\t\t int resolve_flags,\n -+\t\t\t\t\t struct object_id *oid,\n -+\t\t\t\t\t int *flags, int *failure_errno)\n - {\n - \tstatic struct strbuf sb_refname = STRBUF_INIT;\n - \tstruct object_id unused_oid;\n - \tint unused_flags;\n - \tint symref_count;\n - \n -+\tassert(failure_errno);\n -+\n - \tif (!oid)\n - \t\toid = &unused_oid;\n - \tif (!flags)\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\t*failure_errno = EINVAL;\n - \t\t\treturn NULL;\n - \t\t}\n - \n -@@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n + const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n +@@ refs.c: const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n \tfor (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) {\n \t\tunsigned int read_flags = 0;\n \n +-\t\terrno = 0;\n -\t\tif (refs_read_raw_ref(refs, refname,\n -\t\t\t\t oid, &sb_refname, &read_flags)) {\n +\t\tif (refs_read_raw_ref(refs, refname, oid, &sb_refname,\n +\t\t\t\t &read_flags, failure_errno)) {\n \t\t\t*flags |= read_flags;\n - \n - \t\t\t/* In reading mode, refs must eventually resolve */\n -@@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n - \t\t\t * may show errors besides ENOENT if there are\n - \t\t\t * similarly-named refs.\n - \t\t\t */\n --\t\t\tif (errno != ENOENT &&\n --\t\t\t errno != EISDIR &&\n --\t\t\t errno != ENOTDIR)\n -+\t\t\tif (*failure_errno != ENOENT &&\n -+\t\t\t *failure_errno != EISDIR &&\n -+\t\t\t *failure_errno != ENOTDIR)\n - \t\t\t\treturn NULL;\n - \n - \t\t\toidclr(oid);\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\t*failure_errno = EINVAL;\n - \t\t\t\treturn NULL;\n - \t\t\t}\n - \n -@@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n - \t\t}\n - \t}\n - \n --\terrno = ELOOP;\n -+\t*failure_errno = ELOOP;\n - \treturn NULL;\n - }\n - \n -+const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname,\n -+\t\t\t\t int resolve_flags, struct object_id *oid,\n -+\t\t\t\t int *flags)\n -+{\n -+\tint failure_errno = 0;\n -+\tconst char *refn;\n -+\trefn = refs_resolve_ref_unsafe_with_errno(refs, refname, resolve_flags,\n -+\t\t\t\t\t\t oid, flags, &failure_errno);\n -+\tif (!refn)\n -+\t\t/* For unmigrated legacy callers */\n -+\t\terrno = failure_errno;\n -+\treturn refn;\n -+}\n -+\n - /* backend functions */\n - int refs_init_db(struct strbuf *err)\n - {\n + \t\t\tif (errno)\n + \t\t\t\t*failure_errno = errno;\n @@ refs.c: int refs_verify_refname_available(struct ref_store *refs,\n \n \tstrbuf_grow(&dirname, strlen(refname) + 1);\n 7: 18bf4a0e97c ! 4: 758c761abcf refs: explicitly return failure_errno from parse_loose_ref_contents\n @@ Metadata\n Author: Han-Wen Nienhuys \n \n ## Commit message ##\n - refs: explicitly return failure_errno from parse_loose_ref_contents\n + refs API: make parse_loose_ref_contents() not set errno\n +\n + Change the parse_loose_ref_contents() function to stop setting \"errno\"\n + and failure, and to instead pass up a \"failure_errno\" via a\n + parameter. This requires changing its callers to do the same.\n \n The EINVAL error from parse_loose_ref_contents is used in files-backend\n to create a custom error message.\n 1: f06b054e861 ! 5: cdf17fa34a4 refs file backend: move raceproof_create_file() here\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - refs file backend: move raceproof_create_file() here\n + refs API: make refs_rename_ref_available() static\n \n - Move the raceproof_create_file() API added to cache.h and\n - object-file.c in 177978f56ad (raceproof_create_file(): new function,\n - 2017-01-06) to its only user, refs/files-backend.c.\n + Move the refs_rename_ref_available() function into\n + \"refs/files-backend.c\". It is file-backend specific.\n \n - Signed-off-by: Ævar Arnfjörð Bjarmason \n + This function was added in 5fe7d825da8 (refs.c: pass a list of names\n + to skip to is_refname_available, 2014-05-01) as rename_ref_available()\n + and was only ever used in this one file-backend specific codepath. So\n + let's move it there.\n \n - ## cache.h ##\n -@@ cache.h: enum scld_error safe_create_leading_directories(char *path);\n - enum scld_error safe_create_leading_directories_const(const char *path);\n - enum scld_error safe_create_leading_directories_no_share(char *path);\n - \n --/*\n -- * Callback function for raceproof_create_file(). This function is\n -- * expected to do something that makes dirname(path) permanent despite\n -- * the fact that other processes might be cleaning up empty\n -- * directories at the same time. Usually it will create a file named\n -- * path, but alternatively it could create another file in that\n -- * directory, or even chdir() into that directory. The function should\n -- * return 0 if the action was completed successfully. On error, it\n -- * should return a nonzero result and set errno.\n -- * raceproof_create_file() treats two errno values specially:\n -- *\n -- * - ENOENT -- dirname(path) does not exist. In this case,\n -- * raceproof_create_file() tries creating dirname(path)\n -- * (and any parent directories, if necessary) and calls\n -- * the function again.\n -- *\n -- * - EISDIR -- the file already exists and is a directory. In this\n -- * case, raceproof_create_file() removes the directory if\n -- * it is empty (and recursively any empty directories that\n -- * it contains) and calls the function again.\n -- *\n -- * Any other errno causes raceproof_create_file() to fail with the\n -- * callback's return value and errno.\n -- *\n -- * Obviously, this function should be OK with being called again if it\n -- * fails with ENOENT or EISDIR. In other scenarios it will not be\n -- * called again.\n -- */\n --typedef int create_file_fn(const char *path, void *cb);\n --\n --/*\n -- * Create a file in dirname(path) by calling fn, creating leading\n -- * directories if necessary. Retry a few times in case we are racing\n -- * with another process that is trying to clean up the directory that\n -- * contains path. See the documentation for create_file_fn for more\n -- * details.\n -- *\n -- * Return the value and set the errno that resulted from the most\n -- * recent call of fn. fn is always called at least once, and will be\n -- * called more than once if it returns ENOENT or EISDIR.\n -- */\n --int raceproof_create_file(const char *path, create_file_fn fn, void *cb);\n --\n - int mkdir_in_gitdir(const char *path);\n - char *expand_user_path(const char *path, int real_home);\n - const char *enter_repo(const char *path, int strict);\n + Signed-off-by: Ævar Arnfjörð Bjarmason \n \n - ## object-file.c ##\n -@@ object-file.c: enum scld_error safe_create_leading_directories_const(const char *path)\n - \treturn result;\n + ## refs.c ##\n +@@ refs.c: const char *find_descendant_ref(const char *dirname,\n + \treturn NULL;\n }\n \n --int raceproof_create_file(const char *path, create_file_fn fn, void *cb)\n +-int refs_rename_ref_available(struct ref_store *refs,\n +-\t\t\t const char *old_refname,\n +-\t\t\t const char *new_refname)\n -{\n --\t/*\n --\t * The number of times we will try to remove empty directories\n --\t * in the way of path. This is only 1 because if another\n --\t * process is racily creating directories that conflict with\n --\t * us, we don't want to fight against them.\n --\t */\n --\tint remove_directories_remaining = 1;\n --\n --\t/*\n --\t * The number of times that we will try to create the\n --\t * directories containing path. We are willing to attempt this\n --\t * more than once, because another process could be trying to\n --\t * clean up empty directories at the same time as we are\n --\t * trying to create them.\n --\t */\n --\tint create_directories_remaining = 3;\n --\n --\t/* A scratch copy of path, filled lazily if we need it: */\n --\tstruct strbuf path_copy = STRBUF_INIT;\n --\n --\tint ret, save_errno;\n --\n --\t/* Sanity check: */\n --\tassert(*path);\n +-\tstruct string_list skip = STRING_LIST_INIT_NODUP;\n +-\tstruct strbuf err = STRBUF_INIT;\n +-\tint ok;\n -\n --retry_fn:\n --\tret = fn(path, cb);\n --\tsave_errno = errno;\n --\tif (!ret)\n --\t\tgoto out;\n +-\tstring_list_insert(&skip, old_refname);\n +-\tok = !refs_verify_refname_available(refs, new_refname,\n +-\t\t\t\t\t NULL, &skip, &err);\n +-\tif (!ok)\n +-\t\terror(\"%s\", err.buf);\n -\n --\tif (errno == EISDIR && remove_directories_remaining-- > 0) {\n --\t\t/*\n --\t\t * A directory is in the way. Maybe it is empty; try\n --\t\t * to remove it:\n --\t\t */\n --\t\tif (!path_copy.len)\n --\t\t\tstrbuf_addstr(&path_copy, path);\n --\n --\t\tif (!remove_dir_recursively(&path_copy, REMOVE_DIR_EMPTY_ONLY))\n --\t\t\tgoto retry_fn;\n --\t} else if (errno == ENOENT && create_directories_remaining-- > 0) {\n --\t\t/*\n --\t\t * Maybe the containing directory didn't exist, or\n --\t\t * maybe it was just deleted by a process that is\n --\t\t * racing with us to clean up empty directories. Try\n --\t\t * to create it:\n --\t\t */\n --\t\tenum scld_error scld_result;\n --\n --\t\tif (!path_copy.len)\n --\t\t\tstrbuf_addstr(&path_copy, path);\n --\n --\t\tdo {\n --\t\t\tscld_result = safe_create_leading_directories(path_copy.buf);\n --\t\t\tif (scld_result == SCLD_OK)\n --\t\t\t\tgoto retry_fn;\n --\t\t} while (scld_result == SCLD_VANISHED && create_directories_remaining-- > 0);\n --\t}\n --\n --out:\n --\tstrbuf_release(&path_copy);\n --\terrno = save_errno;\n --\treturn ret;\n +-\tstring_list_clear(&skip, 0);\n +-\tstrbuf_release(&err);\n +-\treturn ok;\n -}\n -\n - static void fill_loose_path(struct strbuf *buf, const struct object_id *oid)\n + int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n {\n - \tint i;\n + \tstruct object_id oid;\n \n ## refs/files-backend.c ##\n -@@ refs/files-backend.c: static struct ref_iterator *files_ref_iterator_begin(\n - \treturn ref_iterator;\n - }\n +@@ refs/files-backend.c: static int commit_ref_update(struct files_ref_store *refs,\n + \t\t\t const struct object_id *oid, const char *logmsg,\n + \t\t\t struct strbuf *err);\n \n +/*\n -+ * Callback function for raceproof_create_file(). This function is\n -+ * expected to do something that makes dirname(path) permanent despite\n -+ * the fact that other processes might be cleaning up empty\n -+ * directories at the same time. Usually it will create a file named\n -+ * path, but alternatively it could create another file in that\n -+ * directory, or even chdir() into that directory. The function should\n -+ * return 0 if the action was completed successfully. On error, it\n -+ * should return a nonzero result and set errno.\n -+ * raceproof_create_file() treats two errno values specially:\n ++ * Check whether an attempt to rename old_refname to new_refname would\n ++ * cause a D/F conflict with any existing reference (other than\n ++ * possibly old_refname). If there would be a conflict, emit an error\n ++ * message and return false; otherwise, return true.\n + *\n -+ * - ENOENT -- dirname(path) does not exist. In this case,\n -+ * raceproof_create_file() tries creating dirname(path)\n -+ * (and any parent directories, if necessary) and calls\n -+ * the function again.\n -+ *\n -+ * - EISDIR -- the file already exists and is a directory. In this\n -+ * case, raceproof_create_file() removes the directory if\n -+ * it is empty (and recursively any empty directories that\n -+ * it contains) and calls the function again.\n -+ *\n -+ * Any other errno causes raceproof_create_file() to fail with the\n -+ * callback's return value and errno.\n -+ *\n -+ * Obviously, this function should be OK with being called again if it\n -+ * fails with ENOENT or EISDIR. In other scenarios it will not be\n -+ * called again.\n -+ */\n -+typedef int create_file_fn(const char *path, void *cb);\n -+\n -+/*\n -+ * Create a file in dirname(path) by calling fn, creating leading\n -+ * directories if necessary. Retry a few times in case we are racing\n -+ * with another process that is trying to clean up the directory that\n -+ * contains path. See the documentation for create_file_fn for more\n -+ * details.\n -+ *\n -+ * Return the value and set the errno that resulted from the most\n -+ * recent call of fn. fn is always called at least once, and will be\n -+ * called more than once if it returns ENOENT or EISDIR.\n ++ * Note that this function is not safe against all races with other\n ++ * processes (though rename_ref() catches some races that might get by\n ++ * this check).\n + */\n -+static int raceproof_create_file(const char *path, create_file_fn fn, void *cb)\n ++static int refs_rename_ref_available(struct ref_store *refs,\n ++\t\t\t const char *old_refname,\n ++\t\t\t const char *new_refname)\n +{\n -+\t/*\n -+\t * The number of times we will try to remove empty directories\n -+\t * in the way of path. This is only 1 because if another\n -+\t * process is racily creating directories that conflict with\n -+\t * us, we don't want to fight against them.\n -+\t */\n -+\tint remove_directories_remaining = 1;\n -+\n -+\t/*\n -+\t * The number of times that we will try to create the\n -+\t * directories containing path. We are willing to attempt this\n -+\t * more than once, because another process could be trying to\n -+\t * clean up empty directories at the same time as we are\n -+\t * trying to create them.\n -+\t */\n -+\tint create_directories_remaining = 3;\n -+\n -+\t/* A scratch copy of path, filled lazily if we need it: */\n -+\tstruct strbuf path_copy = STRBUF_INIT;\n -+\n -+\tint ret, save_errno;\n -+\n -+\t/* Sanity check: */\n -+\tassert(*path);\n -+\n -+retry_fn:\n -+\tret = fn(path, cb);\n -+\tsave_errno = errno;\n -+\tif (!ret)\n -+\t\tgoto out;\n ++\tstruct string_list skip = STRING_LIST_INIT_NODUP;\n ++\tstruct strbuf err = STRBUF_INIT;\n ++\tint ok;\n +\n -+\tif (errno == EISDIR && remove_directories_remaining-- > 0) {\n -+\t\t/*\n -+\t\t * A directory is in the way. Maybe it is empty; try\n -+\t\t * to remove it:\n -+\t\t */\n -+\t\tif (!path_copy.len)\n -+\t\t\tstrbuf_addstr(&path_copy, path);\n ++\tstring_list_insert(&skip, old_refname);\n ++\tok = !refs_verify_refname_available(refs, new_refname,\n ++\t\t\t\t\t NULL, &skip, &err);\n ++\tif (!ok)\n ++\t\terror(\"%s\", err.buf);\n +\n -+\t\tif (!remove_dir_recursively(&path_copy, REMOVE_DIR_EMPTY_ONLY))\n -+\t\t\tgoto retry_fn;\n -+\t} else if (errno == ENOENT && create_directories_remaining-- > 0) {\n -+\t\t/*\n -+\t\t * Maybe the containing directory didn't exist, or\n -+\t\t * maybe it was just deleted by a process that is\n -+\t\t * racing with us to clean up empty directories. Try\n -+\t\t * to create it:\n -+\t\t */\n -+\t\tenum scld_error scld_result;\n -+\n -+\t\tif (!path_copy.len)\n -+\t\t\tstrbuf_addstr(&path_copy, path);\n -+\n -+\t\tdo {\n -+\t\t\tscld_result = safe_create_leading_directories(path_copy.buf);\n -+\t\t\tif (scld_result == SCLD_OK)\n -+\t\t\t\tgoto retry_fn;\n -+\t\t} while (scld_result == SCLD_VANISHED && create_directories_remaining-- > 0);\n -+\t}\n -+\n -+out:\n -+\tstrbuf_release(&path_copy);\n -+\terrno = save_errno;\n -+\treturn ret;\n ++\tstring_list_clear(&skip, 0);\n ++\tstrbuf_release(&err);\n ++\treturn ok;\n +}\n +\n - static int remove_empty_directories(struct strbuf *path)\n - {\n - \t/*\n + static int files_copy_or_rename_ref(struct ref_store *ref_store,\n + \t\t\t const char *oldrefname, const char *newrefname,\n + \t\t\t const char *logmsg, int copy)\n +\n + ## refs/refs-internal.h ##\n +@@ refs/refs-internal.h: const char *find_descendant_ref(const char *dirname,\n + \t\t\t\tconst struct string_list *extras,\n + \t\t\t\tconst struct string_list *skip);\n + \n +-/*\n +- * Check whether an attempt to rename old_refname to new_refname would\n +- * cause a D/F conflict with any existing reference (other than\n +- * possibly old_refname). If there would be a conflict, emit an error\n +- * message and return false; otherwise, return true.\n +- *\n +- * Note that this function is not safe against all races with other\n +- * processes (though rename_ref() catches some races that might get by\n +- * this check).\n +- */\n +-int refs_rename_ref_available(struct ref_store *refs,\n +-\t\t\t const char *old_refname,\n +-\t\t\t const char *new_refname);\n +-\n + /* We allow \"recursive\" symbolic refs. Only within reason, though */\n + #define SYMREF_MAXDEPTH 5\n + \n 2: ba0f5f5fb0a < -: ----------- refs: remove EINVAL errno output from specification of read_raw_ref_fn\n -: ----------- > 6: 3162bf28505 refs/files: remove \"name exist?\" check in lock_ref_oid_basic()\n 3: 2c4c30e8e06 ! 7: 288237b1900 refs/files-backend: stop setting errno from lock_ref_oid_basic\n @@\n ## Metadata ##\n -Author: Han-Wen Nienhuys \n +Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - refs/files-backend: stop setting errno from lock_ref_oid_basic\n + refs API: remove refs_read_ref_full() wrapper\n \n - refs/files-backend.c::lock_ref_oid_basic() tries to signal how it failed\n - to its callers using errno.\n + Remove the refs_read_ref_full() wrapper in favor of migrating various\n + refs.c API users to the underlying refs_werrres_ref_unsafe() function.\n \n - It is safe to stop setting errno here, because the callers of this\n - file-scope static function are\n + A careful reading of these callers shows that the callers of this\n + function did not care about \"errno\", by moving away from the\n + refs_resolve_ref_unsafe() wrapper we can be sure that nothing relies\n + on it anymore.\n \n - * files_copy_or_rename_ref()\n - * files_create_symref()\n - * files_reflog_expire()\n -\n - None of them looks at errno after seeing a negative return from\n - lock_ref_oid_basic() to make any decision, and no caller of these three\n - functions looks at errno after they signal a failure by returning a\n - negative value. In particular,\n -\n - * files_copy_or_rename_ref() - here, calls are followed by error()\n - (which performs I/O) or write_ref_to_lockfile() (which calls\n - parse_object() which may perform I/O)\n -\n - * files_create_symref() - here, calls are followed by error() or\n - create_symref_locked() (which performs I/O and does not inspect\n - errno)\n + Signed-off-by: Ævar Arnfjörð Bjarmason \n \n - * files_reflog_expire() - here, calls are followed by error() or\n - refs_reflog_exists() (which calls a function in a vtable that is not\n - documented to use and/or preserve errno)\n + ## refs.c ##\n +@@ refs.c: struct ref_filter {\n + \tvoid *cb_data;\n + };\n + \n +-int refs_read_ref_full(struct ref_store *refs, const char *refname,\n +-\t\t int resolve_flags, struct object_id *oid, int *flags)\n ++int read_ref_full(const char *refname, int resolve_flags, struct object_id *oid, int *flags)\n + {\n +-\tif (refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags))\n ++\tint ignore_errno;\n ++\tstruct ref_store *refs = get_main_ref_store(the_repository);\n ++\n ++\tif (refs_werrres_ref_unsafe(refs, refname, resolve_flags,\n ++\t\t\t\t oid, flags, &ignore_errno))\n + \t\treturn 0;\n + \treturn -1;\n + }\n + \n +-int read_ref_full(const char *refname, int resolve_flags, struct object_id *oid, int *flags)\n +-{\n +-\treturn refs_read_ref_full(get_main_ref_store(the_repository), refname,\n +-\t\t\t\t resolve_flags, oid, flags);\n +-}\n +-\n + int read_ref(const char *refname, struct object_id *oid)\n + {\n + \treturn read_ref_full(refname, RESOLVE_REF_READING, oid, NULL);\n +@@ refs.c: int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n + {\n + \tstruct object_id oid;\n + \tint flag;\n ++\tint ignore_errno;\n + \n +-\tif (!refs_read_ref_full(refs, \"HEAD\", RESOLVE_REF_READING,\n +-\t\t\t\t&oid, &flag))\n ++\tif (refs_werrres_ref_unsafe(refs, \"HEAD\", RESOLVE_REF_READING,\n ++\t\t\t\t &oid, &flag, &ignore_errno))\n + \t\treturn fn(\"HEAD\", &oid, flag, cb_data);\n + \n + \treturn 0;\n \n - Signed-off-by: Han-Wen Nienhuys \n - Signed-off-by: Ævar Arnfjörð Bjarmason \n + ## refs.h ##\n +@@ refs.h: char *refs_resolve_refdup(struct ref_store *refs,\n + char *resolve_refdup(const char *refname, int resolve_flags,\n + \t\t struct object_id *oid, int *flags);\n + \n +-int refs_read_ref_full(struct ref_store *refs, const char *refname,\n +-\t\t int resolve_flags, struct object_id *oid, int *flags);\n + int read_ref_full(const char *refname, int resolve_flags,\n + \t\t struct object_id *oid, int *flags);\n + int read_ref(const char *refname, struct object_id *oid);\n \n ## refs/files-backend.c ##\n -@@ refs/files-backend.c: 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, int *type,\n @@ refs/files-backend.c: 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 ignore_errno;\n \n \tfiles_assert_main_repository(refs, \"lock_ref_oid_basic\");\n \tassert(err);\n @@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n - \tif (!refs_resolve_ref_unsafe(&refs->base, refname,\n - \t\t\t\t RESOLVE_REF_NO_RECURSE,\n - \t\t\t\t &lock->old_oid, type)) {\n --\t\tlast_errno = errno;\n - \t\tif (!refs_verify_refname_available(&refs->base, refname,\n - \t\t\t\t\t\t NULL, NULL, err))\n - \t\t\tstrbuf_addf(err, \"unable to resolve reference '%s': %s\",\n --\t\t\t\t refname, strerror(last_errno));\n -+\t\t\t\t refname, strerror(errno));\n - \n \t\tgoto error_return;\n \t}\n -@@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n + \n +-\tif (refs_read_ref_full(&refs->base, lock->ref_name,\n +-\t\t\t 0,\n +-\t\t\t &lock->old_oid, NULL))\n ++\tif (!refs_werrres_ref_unsafe(&refs->base, lock->ref_name, 0,\n ++\t\t\t\t &lock->old_oid, NULL, &ignore_errno))\n + \t\toidclr(&lock->old_oid);\n + \tgoto out;\n + \n +@@ refs/files-backend.c: static int files_copy_or_rename_ref(struct ref_store *ref_store,\n + \tstruct strbuf tmp_renamed_log = STRBUF_INIT;\n + \tint log, ret;\n + \tstruct strbuf err = STRBUF_INIT;\n ++\tint ignore_errno;\n + \n + \tfiles_reflog_path(refs, &sb_oldref, oldrefname);\n + \tfiles_reflog_path(refs, &sb_newref, newrefname);\n +@@ refs/files-backend.c: static int files_copy_or_rename_ref(struct ref_store *ref_store,\n + \t * the safety anyway; we want to delete the reference whatever\n + \t * its current value.\n \t */\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 NULL, NULL, err)) {\n --\t\tlast_errno = ENOTDIR;\n -+\t\t\t\t\t NULL, NULL, err))\n - \t\tgoto error_return;\n --\t}\n +-\tif (!copy && !refs_read_ref_full(&refs->base, newrefname,\n +-\t\t\t\tRESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n +-\t\t\t\tNULL, NULL) &&\n ++\tif (!copy && refs_werrres_ref_unsafe(&refs->base, newrefname,\n ++\t\t\t\t\t RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n ++\t\t\t\t\t NULL, NULL, &ignore_errno) &&\n + \t refs_delete_ref(&refs->base, NULL, newrefname,\n + \t\t\t NULL, REF_NO_DEREF)) {\n + \t\tif (errno == EISDIR) {\n +@@ refs/files-backend.c: static void update_symref_reflog(struct files_ref_store *refs,\n + {\n + \tstruct strbuf err = STRBUF_INIT;\n + \tstruct object_id new_oid;\n ++\tint ignore_errno;\n ++\n + \tif (logmsg &&\n +-\t !refs_read_ref_full(&refs->base, target,\n +-\t\t\t\tRESOLVE_REF_READING, &new_oid, NULL) &&\n ++\t refs_werrres_ref_unsafe(&refs->base, target,\n ++\t\t\t\t RESOLVE_REF_READING, &new_oid, NULL,\n ++\t\t\t\t &ignore_errno) &&\n + \t files_log_ref_write(refs, refname, &lock->old_oid,\n + \t\t\t\t&new_oid, logmsg, 0, &err)) {\n + \t\terror(\"%s\", err.buf);\n +@@ refs/files-backend.c: static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n + \t\t(struct files_reflog_iterator *)ref_iterator;\n + \tstruct dir_iterator *diter = iter->dir_iterator;\n + \tint ok;\n ++\tint ignore_errno;\n \n - \tlock->ref_name = xstrdup(refname);\n + \twhile ((ok = dir_iterator_advance(diter)) == ITER_OK) {\n + \t\tint flags;\n +@@ refs/files-backend.c: static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n + \t\tif (ends_with(diter->basename, \".lock\"))\n + \t\t\tcontinue;\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 -@@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n +-\t\tif (refs_read_ref_full(iter->ref_store,\n +-\t\t\t\t diter->relative_path, 0,\n +-\t\t\t\t &iter->oid, &flags)) {\n ++\t\tif (!refs_werrres_ref_unsafe(iter->ref_store,\n ++\t\t\t\t\t diter->relative_path, 0,\n ++\t\t\t\t\t &iter->oid, &flags,\n ++\t\t\t\t\t &ignore_errno)) {\n + \t\t\terror(\"bad ref for %s\", diter->path.buf);\n + \t\t\tcontinue;\n + \t\t}\n +@@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *refs,\n + \t\t\t * the transaction, so we have to read it here\n + \t\t\t * to record and possibly check old_oid:\n + \t\t\t */\n +-\t\t\tif (refs_read_ref_full(&refs->base,\n +-\t\t\t\t\t referent.buf, 0,\n +-\t\t\t\t\t &lock->old_oid, NULL)) {\n ++\t\t\tint ignore_errno;\n ++\t\t\tif (!refs_werrres_ref_unsafe(&refs->base,\n ++\t\t\t\t\t\t referent.buf, 0,\n ++\t\t\t\t\t\t &lock->old_oid, NULL,\n ++\t\t\t\t\t\t &ignore_errno)) {\n + \t\t\t\tif (update->flags & REF_HAVE_OLD) {\n + \t\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n + \t\t\t\t\t\t \"error reading reference\",\n +\n + ## worktree.c ##\n +@@ worktree.c: int other_head_refs(each_ref_fn fn, void *cb_data)\n + \t\tstruct worktree *wt = *p;\n + \t\tstruct object_id oid;\n + \t\tint flag;\n ++\t\tint ignore_errno;\n \n - out:\n - \tstrbuf_release(&ref_file);\n --\terrno = last_errno;\n - \treturn lock;\n - }\n + \t\tif (wt->is_current)\n + \t\t\tcontinue;\n \n + \t\tstrbuf_reset(&refname);\n + \t\tstrbuf_worktree_ref(wt, &refname, \"HEAD\");\n +-\t\tif (!refs_read_ref_full(get_main_ref_store(the_repository),\n +-\t\t\t\t\trefname.buf,\n +-\t\t\t\t\tRESOLVE_REF_READING,\n +-\t\t\t\t\t&oid, &flag))\n ++\t\tif (refs_werrres_ref_unsafe(get_main_ref_store(the_repository),\n ++\t\t\t\t\t refname.buf,\n ++\t\t\t\t\t RESOLVE_REF_READING,\n ++\t\t\t\t\t &oid, &flag, &ignore_errno))\n + \t\t\tret = fn(refname.buf, &oid, flag, cb_data);\n + \t\tif (ret)\n + \t\t\tbreak;\n -: ----------- > 8: acb484ea547 refs API: make resolve_gitlink_ref() not set errno\n -: ----------- > 9: 4be84c9bf53 refs API: make loose_fill_ref_dir() not set errno\n -: ----------- > 10: 8753210f9cc refs API: make files_copy_or_rename_ref() et al not set errno\n -: ----------- > 11: 9fe85926140 refs API: ignore errno in worktree.c's add_head_info()\n -: ----------- > 12: 8d87db98041 refs API: ignore errno in worktree.c's find_shared_symref()\n -: ----------- > 13: 954633bcbb2 refs tests: ignore ignore errno in test-ref-store helper\n -: ----------- > 14: fbbc08d3ebd refs API: make refs_resolve_refdup() not set errno\n -: ----------- > 15: 4b2a4dbe7d5 refs API: make refs_ref_exists() not set errno\n -: ----------- > 16: 888b1884c29 refs API: make resolve_ref_unsafe() not set errno\n -: ----------- > 17: e2885f13c9b refs API: make expand_ref() & repo_dwim_log() not set errno\n -: ----------- > 18: df50373a272 refs API: don't expose \"errno\" in run_transaction_hook()\n 8: 7d94a32af83 ! 19: 4c80b05bf1d refs: make errno output explicit for refs_resolve_ref_unsafe\n @@\n ## Metadata ##\n -Author: Han-Wen Nienhuys \n +Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - refs: make errno output explicit for refs_resolve_ref_unsafe\n + refs API: post-migration API renaming [1/2]\n \n - This introduces refs_resolve_ref_unsafe_with_errno(), which makes the API\n - contract for the errno output explicit. The implementation still relies on\n - the global errno variable to ensure no side effects of this refactoring.\n + In preceding commits all callers of refs_resolve_ref_unsafe() were\n + migrated to the transitory refs_werrres_ref_unsafe() function.\n \n - lock_ref_oid_basic() in files-backend.c is the only caller of refs_resolve_ref()\n - that needs error information to make logic decisions, so update that caller\n + As a first step in getting rid of it let's remove the old function\n + from the public API (it went unused in a preceding commit).\n +\n + We then provide both a coccinelle rule to do the rename, and a macro\n + to avoid breaking the existing callers.\n \n - Signed-off-by: Han-Wen Nienhuys \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n + ## contrib/coccinelle/refs.pending.cocci (new) ##\n +@@\n ++@@\n ++expression refs, refname, resolve_flags, oid, flags, failure_errno;\n ++@@\n ++- refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)\n +++ refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)\n +\n ## refs.c ##\n @@ refs.c: int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t\t\t\t\t type, failure_errno);\n }\n \n --static const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs,\n -+const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs,\n - \t\t\t\t\t const char *refname,\n - \t\t\t\t\t int resolve_flags,\n - \t\t\t\t\t struct object_id *oid,\n -\n - ## refs.h ##\n -@@ refs.h: const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n +-const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n ++const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n + \t\t\t\t const char *refname,\n \t\t\t\t int resolve_flags,\n \t\t\t\t struct object_id *oid,\n - \t\t\t\t int *flags);\n -+/**\n -+ * refs_resolve_ref_unsafe_with_errno() is like\n -+ * refs_resolve_ref_unsafe(), but provide access to errno code that\n -+ * lead to a failure. We guarantee that errno is set to a meaningful\n -+ * value on non-zero return.\n -+ */\n -+const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs,\n -+\t\t\t\t\t const char *refname,\n -+\t\t\t\t\t int resolve_flags,\n -+\t\t\t\t\t struct object_id *oid,\n -+\t\t\t\t\t int *flags, int *failure_errno);\n - const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n - \t\t\t struct object_id *oid, int *flags);\n +@@ refs.c: const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n + \treturn NULL;\n + }\n \n -\n - ## refs/files-backend.c ##\n -@@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n +-const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname,\n +-\t\t\t\t int resolve_flags, struct object_id *oid,\n +-\t\t\t\t int *flags)\n +-{\n +-\tint failure_errno = 0;\n +-\tconst char *refn;\n +-\trefn = refs_werrres_ref_unsafe(refs, refname, resolve_flags,\n +-\t\t\t\t oid, flags, &failure_errno);\n +-\tif (!refn)\n +-\t\terrno = failure_errno;\n +-\treturn refn;\n +-}\n +-\n + /* backend functions */\n + int refs_init_db(struct strbuf *err)\n {\n - \tstruct strbuf ref_file = STRBUF_INIT;\n - \tstruct ref_lock *lock;\n -+\tint resolve_errno = 0;\n +\n + ## refs.h ##\n +@@ refs.h: struct string_list;\n + struct string_list_item;\n + struct worktree;\n \n - \tfiles_assert_main_repository(refs, \"lock_ref_oid_basic\");\n - \tassert(err);\n -@@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n - \tCALLOC_ARRAY(lock, 1);\n +-/*\n +- * Callers should not inspect \"errno\" on failure, but rather pass in a\n +- * \"failure_errno\" parameter, on failure the \"errno\" will indicate the\n +- * type of failure encountered, but not necessarily one that came from\n +- * a syscall. We might have faked it up.\n +- */\n +-const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n +-\t\t\t\t const char *refname,\n +-\t\t\t\t int resolve_flags,\n +-\t\t\t\t struct object_id *oid,\n +-\t\t\t\t int *flags, int *failure_errno);\n +-\n + /*\n + * Resolve a reference, recursively following symbolic refererences.\n + *\n +@@ refs.h: const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n + * resolved. The function returns NULL for such ref names.\n + * Caps and underscores refers to the special refs, such as HEAD,\n + * FETCH_HEAD and friends, that all live outside of the refs/ directory.\n ++ *\n ++ * Callers should not inspect \"errno\" on failure, but rather pass in a\n ++ * \"failure_errno\" parameter, on failure the \"errno\" will indicate the\n ++ * type of failure encountered, but not necessarily one that came from\n ++ * a syscall. We might have faked it up.\n + */\n + #define RESOLVE_REF_READING 0x01\n + #define RESOLVE_REF_NO_RECURSE 0x02\n + #define RESOLVE_REF_ALLOW_BAD_NAME 0x04\n \n - \tfiles_ref_path(refs, &ref_file, refname);\n --\tif (!refs_resolve_ref_unsafe(&refs->base, refname,\n --\t\t\t\t RESOLVE_REF_NO_RECURSE,\n --\t\t\t\t &lock->old_oid, type)) {\n -+\tif (!refs_resolve_ref_unsafe_with_errno(&refs->base, refname,\n -+\t\t\t\t\t\tRESOLVE_REF_NO_RECURSE,\n -+\t\t\t\t\t\t&lock->old_oid, type,\n -+\t\t\t\t\t\t&resolve_errno)) {\n - \t\tif (!refs_verify_refname_available(&refs->base, refname,\n - \t\t\t\t\t\t NULL, NULL, err))\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(resolve_errno));\n ++#define refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno) \\\n ++\trefs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)\n + const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n + \t\t\t\t const char *refname,\n + \t\t\t\t int resolve_flags,\n + \t\t\t\t struct object_id *oid,\n +-\t\t\t\t int *flags);\n ++\t\t\t\t int *flags, int *failure_errno);\n ++\n + const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n + \t\t\t struct object_id *oid, int *flags);\n \n - \t\tgoto error_return;\n - \t}\n -: ----------- > 20: 54b18e3a719 refs API: post-migration API renaming [2/2]\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438720","messageId":"patch-02.20-46641111885-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 02/20] refs API: add a version of refs_resolve_ref_unsafe() with \"errno\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:14Z","receivedAt":"2021-10-14T00:06:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a new refs_werrres_ref_unsafe() function, which is like\nrefs_resolve_ref_unsafe() except that it explicitly saves away the\n\"errno\" to a passed-in parameter, the refs_resolve_ref_unsafe() then\nbecomes a wrapper for it.\n\nIn subsequent commits we'll migrate code over to it, before finally\nmaking \"refs_resolve_ref_unsafe()\" with an \"errno\" parameter the\ncanonical version, so this this function exists only so that we can\nincrementally migrate callers, it will be going away in a subsequent\ncommit.\n\nAs the added comment notes has a rather tortured name to be the same\nlength as \"refs_resolve_ref_unsafe\", to avoid churn as we won't need\nto re-indent the argument lists, similarly the documentation and\nstructure of it in refs.h is designed to minimize a diff in a\nsubsequent commit, where that documentation will be added to the new\nrefs_resolve_ref_unsafe().\n\nAt the end of this migration the \"meaningful errno\" TODO item left in\n76d70dc0c63 (refs.c: make resolve_ref_unsafe set errno to something\nmeaningful on error, 2014-06-20) will be resolved.\n\nAs can be seen from the use of refs_read_raw_ref() we'll also need to\nconvert some functions that the new refs_werrres_ref_unsafe() itself\ncalls to take this \"failure_errno\". That will be done in subsequent\ncommits.\n\nSigned-off-by: Han-Wen Nienhuys \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 36 +++++++++++++++++++++++++++---------\n refs.h | 12 ++++++++++++\n 2 files changed, 39 insertions(+), 9 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 2166f37399d..46e3c785016 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1678,17 +1678,19 @@ int refs_read_raw_ref(struct ref_store *ref_store,\n \t\t\t\t\t type, &errno);\n }\n \n-/* This function needs to return a meaningful errno on failure */\n-const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n+const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n \t\t\t\t const char *refname,\n \t\t\t\t int resolve_flags,\n-\t\t\t\t struct object_id *oid, int *flags)\n+\t\t\t\t struct object_id *oid,\n+\t\t\t\t int *flags, int *failure_errno)\n {\n \tstatic struct strbuf sb_refname = STRBUF_INIT;\n \tstruct object_id unused_oid;\n \tint unused_flags;\n \tint symref_count;\n \n+\tassert(failure_errno);\n+\n \tif (!oid)\n \t\toid = &unused_oid;\n \tif (!flags)\n@@ -1699,7 +1701,7 @@ 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\t*failure_errno = EINVAL;\n \t\t\treturn NULL;\n \t\t}\n \n@@ -1717,9 +1719,12 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \tfor (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) {\n \t\tunsigned int read_flags = 0;\n \n+\t\terrno = 0;\n \t\tif (refs_read_raw_ref(refs, refname,\n \t\t\t\t oid, &sb_refname, &read_flags)) {\n \t\t\t*flags |= read_flags;\n+\t\t\tif (errno)\n+\t\t\t\t*failure_errno = errno;\n \n \t\t\t/* In reading mode, refs must eventually resolve */\n \t\t\tif (resolve_flags & RESOLVE_REF_READING)\n@@ -1730,9 +1735,9 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t\t * may show errors besides ENOENT if there are\n \t\t\t * similarly-named refs.\n \t\t\t */\n-\t\t\tif (errno != ENOENT &&\n-\t\t\t errno != EISDIR &&\n-\t\t\t errno != ENOTDIR)\n+\t\t\tif (*failure_errno != ENOENT &&\n+\t\t\t *failure_errno != EISDIR &&\n+\t\t\t *failure_errno != ENOTDIR)\n \t\t\t\treturn NULL;\n \n \t\t\toidclr(oid);\n@@ -1759,7 +1764,7 @@ 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\t*failure_errno = EINVAL;\n \t\t\t\treturn NULL;\n \t\t\t}\n \n@@ -1767,10 +1772,23 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t}\n \t}\n \n-\terrno = ELOOP;\n+\t*failure_errno = ELOOP;\n \treturn NULL;\n }\n \n+const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname,\n+\t\t\t\t int resolve_flags, struct object_id *oid,\n+\t\t\t\t int *flags)\n+{\n+\tint failure_errno = 0;\n+\tconst char *refn;\n+\trefn = refs_werrres_ref_unsafe(refs, refname, resolve_flags,\n+\t\t\t\t oid, flags, &failure_errno);\n+\tif (!refn)\n+\t\terrno = failure_errno;\n+\treturn refn;\n+}\n+\n /* backend functions */\n int refs_init_db(struct strbuf *err)\n {\ndiff --git a/refs.h b/refs.h\nindex d5099d4984e..c8afde6bb50 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -11,6 +11,18 @@ struct string_list;\n struct string_list_item;\n struct worktree;\n \n+/*\n+ * Callers should not inspect \"errno\" on failure, but rather pass in a\n+ * \"failure_errno\" parameter, on failure the \"errno\" will indicate the\n+ * type of failure encountered, but not necessarily one that came from\n+ * a syscall. We might have faked it up.\n+ */\n+const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n+\t\t\t\t const char *refname,\n+\t\t\t\t int resolve_flags,\n+\t\t\t\t struct object_id *oid,\n+\t\t\t\t int *flags, int *failure_errno);\n+\n /*\n * Resolve a reference, recursively following symbolic refererences.\n *\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438721","messageId":"patch-03.20-a1a80715ffe-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 03/20] refs API: make refs_read_raw_ref() not set errno","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:15Z","receivedAt":"2021-10-14T00:06:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Han-Wen Nienhuys \n\nAdd a \"failure_errno\" to refs_read_raw_ref(), his allows\nrefs_werrres_ref_unsafe() to pass along its \"failure_errno\", as a\nfirst step before its own callers are migrated to pass it further up\nthe chain.\n\nWe are leaving out out the refs_read_special_head() in\nrefs_read_raw_ref() for now, as noted in a subsequent commit moving it\nto \"failure_errno\" will require some special consideration.\n\nSigned-off-by: Han-Wen Nienhuys \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 24 ++++++++++++++++--------\n refs/files-backend.c | 10 ++++++----\n refs/packed-backend.c | 7 ++++---\n refs/refs-internal.h | 6 +++---\n 4 files changed, 29 insertions(+), 18 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 46e3c785016..acc5d5fa578 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1665,17 +1665,18 @@ static int refs_read_special_head(struct ref_store *ref_store,\n \treturn result;\n }\n \n-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+int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,\n+\t\t struct object_id *oid, struct strbuf *referent,\n+\t\t unsigned int *type, int *failure_errno)\n {\n+\tassert(failure_errno);\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, &errno);\n+\t\t\t\t\t type, failure_errno);\n }\n \n const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n@@ -1719,9 +1720,8 @@ const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n \tfor (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) {\n \t\tunsigned int read_flags = 0;\n \n-\t\terrno = 0;\n-\t\tif (refs_read_raw_ref(refs, refname,\n-\t\t\t\t oid, &sb_refname, &read_flags)) {\n+\t\tif (refs_read_raw_ref(refs, refname, oid, &sb_refname,\n+\t\t\t\t &read_flags, failure_errno)) {\n \t\t\t*flags |= read_flags;\n \t\t\tif (errno)\n \t\t\t\t*failure_errno = errno;\n@@ -2239,6 +2239,13 @@ int refs_verify_refname_available(struct ref_store *refs,\n \n \tstrbuf_grow(&dirname, strlen(refname) + 1);\n \tfor (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) {\n+\t\t/*\n+\t\t * Just saying \"Is a directory\" when we e.g. can't\n+\t\t * lock some multi-level ref isn't very informative,\n+\t\t * the user won't be told *what* is a directory, so\n+\t\t * let's not use strerror() below.\n+\t\t */\n+\t\tint ignore_errno;\n \t\t/* Expand dirname to the new prefix, not including the trailing slash: */\n \t\tstrbuf_add(&dirname, refname + dirname.len, slash - refname - dirname.len);\n \n@@ -2250,7 +2257,8 @@ int refs_verify_refname_available(struct ref_store *refs,\n \t\tif (skip && string_list_has_string(skip, dirname.buf))\n \t\t\tcontinue;\n \n-\t\tif (!refs_read_raw_ref(refs, dirname.buf, &oid, &referent, &type)) {\n+\t\tif (!refs_read_raw_ref(refs, dirname.buf, &oid, &referent,\n+\t\t\t\t &type, &ignore_errno)) {\n \t\t\tstrbuf_addf(err, _(\"'%s' exists; cannot create '%s'\"),\n \t\t\t\t dirname.buf, refname);\n \t\t\tgoto cleanup;\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 6a6ead0b99b..94c194665ed 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -381,10 +381,11 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t\tgoto out;\n \n \tif (lstat(path, &st) < 0) {\n+\t\tint ignore_errno;\n \t\tif (errno != ENOENT)\n \t\t\tgoto out;\n-\t\tif (refs_read_raw_ref(refs->packed_ref_store, refname,\n-\t\t\t\t oid, referent, type)) {\n+\t\tif (refs_read_raw_ref(refs->packed_ref_store, refname, oid,\n+\t\t\t\t referent, type, &ignore_errno)) {\n \t\t\terrno = ENOENT;\n \t\t\tgoto out;\n \t\t}\n@@ -418,13 +419,14 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \n \t/* Is it a directory? */\n \tif (S_ISDIR(st.st_mode)) {\n+\t\tint ignore_errno;\n \t\t/*\n \t\t * Even though there is a directory where the loose\n \t\t * ref is supposed to be, there could still be a\n \t\t * packed ref:\n \t\t */\n-\t\tif (refs_read_raw_ref(refs->packed_ref_store, refname,\n-\t\t\t\t oid, referent, type)) {\n+\t\tif (refs_read_raw_ref(refs->packed_ref_store, refname, oid,\n+\t\t\t\t referent, type, &ignore_errno)) {\n \t\t\terrno = EISDIR;\n \t\t\tgoto out;\n \t\t}\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 47247a14917..52cdc94a26e 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1347,6 +1347,7 @@ int is_packed_transaction_needed(struct ref_store *ref_store,\n \tret = 0;\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n+\t\tint failure_errno;\n \t\tunsigned int type;\n \t\tstruct object_id oid;\n \n@@ -1357,9 +1358,9 @@ int is_packed_transaction_needed(struct ref_store *ref_store,\n \t\t\t */\n \t\t\tcontinue;\n \n-\t\tif (!refs_read_raw_ref(ref_store, update->refname,\n-\t\t\t\t &oid, &referent, &type) ||\n-\t\t errno != ENOENT) {\n+\t\tif (!refs_read_raw_ref(ref_store, update->refname, &oid,\n+\t\t\t\t &referent, &type, &failure_errno) ||\n+\t\t failure_errno != ENOENT) {\n \t\t\t/*\n \t\t\t * We have to actually delete that reference\n \t\t\t * -> this transaction is needed.\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 72746407fc3..c87f1135e5b 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -149,9 +149,9 @@ struct ref_update {\n \tconst char refname[FLEX_ARRAY];\n };\n \n-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+int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,\n+\t\t struct object_id *oid, struct strbuf *referent,\n+\t\t unsigned int *type, int *failure_errno);\n \n /*\n * Write an error to `err` and return a nonzero value iff the same\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438722","messageId":"patch-04.20-758c761abcf-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 04/20] refs API: make parse_loose_ref_contents() not set errno","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:16Z","receivedAt":"2021-10-14T00:06:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Han-Wen Nienhuys \n\nChange the parse_loose_ref_contents() function to stop setting \"errno\"\nand failure, and to instead pass up a \"failure_errno\" via a\nparameter. This requires changing its callers to do the same.\n\nThe EINVAL error from parse_loose_ref_contents is used in files-backend\nto create a custom error message.\n\nIn untangling this we discovered a tricky edge case. The\nrefs_read_special_head() function was relying on\nparse_loose_ref_contents() setting EINVAL.\n\nBy converting it to use \"saved_errno\" we can migrate away from \"errno\"\nin this part of the code entirely, and do away with an existing\n\"save_errno\" pattern, its only purpose was to not clobber the \"errno\"\nwe previously needed at the end of files_read_raw_ref().\n\nLet's assert that we can do that by not having files_read_raw_ref()\nitself operate on *failure_errno in addition to passing it on. Instead\nwe'll assert that if we return non-zero we actually do set errno, thus\nassuring ourselves and callers that they can trust the resulting\n\"failure_errno\".\n\nSigned-off-by: Han-Wen Nienhuys \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 8 +++++---\n refs/files-backend.c | 31 ++++++++++++++++++++-----------\n refs/refs-internal.h | 6 ++++--\n t/t3200-branch.sh | 1 +\n 4 files changed, 30 insertions(+), 16 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex acc5d5fa578..b1640b5d582 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1647,7 +1647,8 @@ int for_each_fullref_in_prefixes(const char *namespace,\n \n static int refs_read_special_head(struct ref_store *ref_store,\n \t\t\t\t const char *refname, struct object_id *oid,\n-\t\t\t\t struct strbuf *referent, unsigned int *type)\n+\t\t\t\t struct strbuf *referent, unsigned int *type,\n+\t\t\t\t int *failure_errno)\n {\n \tstruct strbuf full_path = STRBUF_INIT;\n \tstruct strbuf content = STRBUF_INIT;\n@@ -1657,7 +1658,8 @@ static int refs_read_special_head(struct ref_store *ref_store,\n \tif (strbuf_read_file(&content, full_path.buf, 0) < 0)\n \t\tgoto done;\n \n-\tresult = parse_loose_ref_contents(content.buf, oid, referent, type);\n+\tresult = parse_loose_ref_contents(content.buf, oid, referent, type,\n+\t\t\t\t\t failure_errno);\n \n done:\n \tstrbuf_release(&full_path);\n@@ -1672,7 +1674,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \tassert(failure_errno);\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\t\t\t\t type, failure_errno);\n \t}\n \n \treturn ref_store->be->read_raw_ref(ref_store, refname, oid, referent,\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 94c194665ed..c73ffd1ca33 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -355,6 +355,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \tint fd;\n \tint ret = -1;\n \tint remaining_retries = 3;\n+\tint myerr = 0;\n \n \t*type = 0;\n \tstrbuf_reset(&sb_path);\n@@ -382,11 +383,13 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \n \tif (lstat(path, &st) < 0) {\n \t\tint ignore_errno;\n-\t\tif (errno != ENOENT)\n+\t\tmyerr = errno;\n+\t\terrno = 0;\n+\t\tif (myerr != ENOENT)\n \t\t\tgoto out;\n \t\tif (refs_read_raw_ref(refs->packed_ref_store, refname, oid,\n \t\t\t\t referent, type, &ignore_errno)) {\n-\t\t\terrno = ENOENT;\n+\t\t\tmyerr = ENOENT;\n \t\t\tgoto out;\n \t\t}\n \t\tret = 0;\n@@ -397,7 +400,9 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \tif (S_ISLNK(st.st_mode)) {\n \t\tstrbuf_reset(&sb_contents);\n \t\tif (strbuf_readlink(&sb_contents, path, st.st_size) < 0) {\n-\t\t\tif (errno == ENOENT || errno == EINVAL)\n+\t\t\tmyerr = errno;\n+\t\t\terrno = 0;\n+\t\t\tif (myerr == ENOENT || myerr == EINVAL)\n \t\t\t\t/* inconsistent with lstat; retry */\n \t\t\t\tgoto stat_ref;\n \t\t\telse\n@@ -427,7 +432,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t\t */\n \t\tif (refs_read_raw_ref(refs->packed_ref_store, refname, oid,\n \t\t\t\t referent, type, &ignore_errno)) {\n-\t\t\terrno = EISDIR;\n+\t\t\tmyerr = EISDIR;\n \t\t\tgoto out;\n \t\t}\n \t\tret = 0;\n@@ -440,7 +445,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t */\n \tfd = open(path, O_RDONLY);\n \tif (fd < 0) {\n-\t\tif (errno == ENOENT && !S_ISLNK(st.st_mode))\n+\t\tmyerr = errno;\n+\t\tif (myerr == ENOENT && !S_ISLNK(st.st_mode))\n \t\t\t/* inconsistent with lstat; retry */\n \t\t\tgoto stat_ref;\n \t\telse\n@@ -448,26 +454,29 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t}\n \tstrbuf_reset(&sb_contents);\n \tif (strbuf_read(&sb_contents, fd, 256) < 0) {\n-\t\tint save_errno = errno;\n+\t\tmyerr = errno;\n \t\tclose(fd);\n-\t\terrno = save_errno;\n \t\tgoto out;\n \t}\n \tclose(fd);\n \tstrbuf_rtrim(&sb_contents);\n \tbuf = sb_contents.buf;\n \n-\tret = parse_loose_ref_contents(buf, oid, referent, type);\n+\tret = parse_loose_ref_contents(buf, oid, referent, type, &myerr);\n \n out:\n-\t*failure_errno = errno;\n+\tif (ret && !myerr)\n+\t\tBUG(\"returning non-zero %d, should have set myerr!\", ret);\n+\t*failure_errno = myerr;\n+\n \tstrbuf_release(&sb_path);\n \tstrbuf_release(&sb_contents);\n \treturn ret;\n }\n \n int parse_loose_ref_contents(const char *buf, 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 \tconst char *p;\n \tif (skip_prefix(buf, \"ref:\", &buf)) {\n@@ -486,7 +495,7 @@ int parse_loose_ref_contents(const char *buf, struct object_id *oid,\n \tif (parse_oid_hex(buf, oid, &p) ||\n \t (*p != '\\0' && !isspace(*p))) {\n \t\t*type |= REF_ISBROKEN;\n-\t\terrno = EINVAL;\n+\t\t*failure_errno = EINVAL;\n \t\treturn -1;\n \t}\n \treturn 0;\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex c87f1135e5b..121b8fdad08 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -706,10 +706,12 @@ struct ref_store {\n };\n \n /*\n- * Parse contents of a loose ref file.\n+ * Parse contents of a loose ref file. *failure_errno maybe be set to EINVAL for\n+ * invalid contents.\n */\n int parse_loose_ref_contents(const char *buf, 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 /*\n * Fill in the generic part of refs and add it to our collection of\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex b5242719159..7486688d292 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -745,6 +745,7 @@ test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '\n \t) &&\n \tgit --git-dir subdir/.git/ branch rename-src &&\n \tgit rev-parse rename-src >expect &&\n+\t# Tests a BUG() assertion in files_read_raw_ref()\n \tgit --git-dir subdir/.git/ branch -m rename-src rename-dest &&\n \tgit rev-parse rename-dest >actual &&\n \ttest_cmp expect actual &&\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438723","messageId":"patch-05.20-cdf17fa34a4-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 05/20] refs API: make refs_rename_ref_available() static","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:17Z","receivedAt":"2021-10-14T00:06:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move the refs_rename_ref_available() function into\n\"refs/files-backend.c\". It is file-backend specific.\n\nThis function was added in 5fe7d825da8 (refs.c: pass a list of names\nto skip to is_refname_available, 2014-05-01) as rename_ref_available()\nand was only ever used in this one file-backend specific codepath. So\nlet's move it there.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 19 -------------------\n refs/files-backend.c | 29 +++++++++++++++++++++++++++++\n refs/refs-internal.h | 14 --------------\n 3 files changed, 29 insertions(+), 33 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex b1640b5d582..4398a4c0257 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1371,25 +1371,6 @@ const char *find_descendant_ref(const char *dirname,\n \treturn NULL;\n }\n \n-int refs_rename_ref_available(struct ref_store *refs,\n-\t\t\t const char *old_refname,\n-\t\t\t const char *new_refname)\n-{\n-\tstruct string_list skip = STRING_LIST_INIT_NODUP;\n-\tstruct strbuf err = STRBUF_INIT;\n-\tint ok;\n-\n-\tstring_list_insert(&skip, old_refname);\n-\tok = !refs_verify_refname_available(refs, new_refname,\n-\t\t\t\t\t NULL, &skip, &err);\n-\tif (!ok)\n-\t\terror(\"%s\", err.buf);\n-\n-\tstring_list_clear(&skip, 0);\n-\tstrbuf_release(&err);\n-\treturn ok;\n-}\n-\n int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n {\n \tstruct object_id oid;\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex c73ffd1ca33..0af6ee44552 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1363,6 +1363,35 @@ static int commit_ref_update(struct files_ref_store *refs,\n \t\t\t const struct object_id *oid, const char *logmsg,\n \t\t\t struct strbuf *err);\n \n+/*\n+ * Check whether an attempt to rename old_refname to new_refname would\n+ * cause a D/F conflict with any existing reference (other than\n+ * possibly old_refname). If there would be a conflict, emit an error\n+ * message and return false; otherwise, return true.\n+ *\n+ * Note that this function is not safe against all races with other\n+ * processes (though rename_ref() catches some races that might get by\n+ * this check).\n+ */\n+static int refs_rename_ref_available(struct ref_store *refs,\n+\t\t\t const char *old_refname,\n+\t\t\t const char *new_refname)\n+{\n+\tstruct string_list skip = STRING_LIST_INIT_NODUP;\n+\tstruct strbuf err = STRBUF_INIT;\n+\tint ok;\n+\n+\tstring_list_insert(&skip, old_refname);\n+\tok = !refs_verify_refname_available(refs, new_refname,\n+\t\t\t\t\t NULL, &skip, &err);\n+\tif (!ok)\n+\t\terror(\"%s\", err.buf);\n+\n+\tstring_list_clear(&skip, 0);\n+\tstrbuf_release(&err);\n+\treturn ok;\n+}\n+\n static int files_copy_or_rename_ref(struct ref_store *ref_store,\n \t\t\t const char *oldrefname, const char *newrefname,\n \t\t\t const char *logmsg, int copy)\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 121b8fdad08..2ecd3dea9fb 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -228,20 +228,6 @@ const char *find_descendant_ref(const char *dirname,\n \t\t\t\tconst struct string_list *extras,\n \t\t\t\tconst struct string_list *skip);\n \n-/*\n- * Check whether an attempt to rename old_refname to new_refname would\n- * cause a D/F conflict with any existing reference (other than\n- * possibly old_refname). If there would be a conflict, emit an error\n- * message and return false; otherwise, return true.\n- *\n- * Note that this function is not safe against all races with other\n- * processes (though rename_ref() catches some races that might get by\n- * this check).\n- */\n-int refs_rename_ref_available(struct ref_store *refs,\n-\t\t\t const char *old_refname,\n-\t\t\t const char *new_refname);\n-\n /* We allow \"recursive\" symbolic refs. Only within reason, though */\n #define SYMREF_MAXDEPTH 5\n \n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438724","messageId":"patch-06.20-3162bf28505-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 06/20] refs/files: remove \"name exist?\" check in lock_ref_oid_basic()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:18Z","receivedAt":"2021-10-14T00:06:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In lock_ref_oid_basic() we'll happily lock a reference that doesn't\nexist yet. That's normal, and is how references are initially born,\nbut we don't need to retain checks here in lock_ref_oid_basic() about\nthe state of the ref, when what we're checking is either checked\nalready, or something we're about to discover by trying to lock the\nref with raceproof_create_file().\n\nThe improved diagnostics here were added in\n5b2d8d6f218 (lock_ref_sha1_basic(): improve diagnostics for ref D/F\nconflicts, 2015-05-11), and then much of the surrounding code went\naway recently in my 245fbba46d6 (refs/files: remove unused \"errno ==\nEISDIR\" code, 2021-08-23).\n\nThe refs_resolve_ref_unsafe() code being removed here looks like it\nshould be tasked with doing that, but it's actually redundant to other\ncode.\n\nThe reason for that is as noted in 245fbba46d6 this once widely used\nfunction now only has a handful of callers left, which all handle this\ncase themselves.\n\nTo the extent that we're racy between their check and ours removing\nthis check actually improves the situation, as we'll be doing fewer\nthings between the not-under-lock initial check and acquiring the\nlock.\n\nWhy this is OK for all the remaining callers of lock_ref_oid_basic()\nis noted below. There are only two of those callers:\n\n* \"git branch -[cm] \":\n\n In files_copy_or_rename_ref() we'll call this when we copy or rename\n refs via rename_ref() and copy_ref(). but only after we've checked\n if the refname exists already via its own call to\n refs_resolve_ref_unsafe() and refs_rename_ref_available().\n\n As the updated comment to the latter here notes neither of those are\n actually needed. If we delete not only this code but also\n refs_rename_ref_available() we'll do just fine, we'll just emit a\n less friendly error message if e.g. \"git branch -m A B/C\" would have\n a D/F conflict with a \"B\" file.\n\n Actually we'd probably die before that in case reflogs for the\n branch existed, i.e. when the try to rename() or copy_file() the\n relevant reflog, since if we've got a D/F conflict with a branch\n name we'll probably also have the same with its reflogs (but not\n necessarily, we might have reflogs, but it might not).\n\n As some #leftoverbits that code seems buggy to me, i.e. the reflog\n \"protocol\" should be to get a lock on the main ref, and then perform\n ref and/or reflog operations. That code dates back to\n c976d415e53 (git-branch: add options and tests for branch renaming,\n 2006-11-28) and probably pre-dated the solidifying of that\n convention. But in any case, that edge case is not our bug or\n problem right now.\n\n* \"git reflog expire \":\n\n In files_reflog_expire() we'll call this without previous ref\n existence checking in files-backend.c, but that code is in turn\n called by code that's just finished checking if the refname whose\n reflog we're expiring exists.\n\n See ae35e16cd43 (reflog expire: don't lock reflogs using previously\n seen OID, 2021-08-23) for the current state of that code.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs/files-backend.c | 20 +++++---------------\n 1 file changed, 5 insertions(+), 15 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 0af6ee44552..0dd21b2e205 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1013,16 +1013,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n \tCALLOC_ARRAY(lock, 1);\n \n \tfiles_ref_path(refs, &ref_file, refname);\n-\tif (!refs_resolve_ref_unsafe(&refs->base, refname,\n-\t\t\t\t RESOLVE_REF_NO_RECURSE,\n-\t\t\t\t &lock->old_oid, type)) {\n-\t\tif (!refs_verify_refname_available(&refs->base, refname,\n-\t\t\t\t\t\t NULL, NULL, err))\n-\t\t\tstrbuf_addf(err, \"unable to resolve reference '%s': %s\",\n-\t\t\t\t refname, strerror(errno));\n-\n-\t\tgoto error_return;\n-\t}\n \n \t/*\n \t * If the ref did not exist and we are creating it, make sure\n@@ -1364,14 +1354,14 @@ static int commit_ref_update(struct files_ref_store *refs,\n \t\t\t struct strbuf *err);\n \n /*\n- * Check whether an attempt to rename old_refname to new_refname would\n- * cause a D/F conflict with any existing reference (other than\n- * possibly old_refname). If there would be a conflict, emit an error\n+ * Emit a better error message than lockfile.c's\n+ * unable_to_lock_message() would in case there is a D/F conflict with\n+ * another existing reference. If there would be a conflict, emit an error\n * message and return false; otherwise, return true.\n *\n * Note that this function is not safe against all races with other\n- * processes (though rename_ref() catches some races that might get by\n- * this check).\n+ * processes, and that's not its job. We'll emit a more verbose error on D/f\n+ * conflicts if we get past it into lock_ref_oid_basic().\n */\n static int refs_rename_ref_available(struct ref_store *refs,\n \t\t\t const char *old_refname,\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438725","messageId":"patch-07.20-288237b1900-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 07/20] refs API: remove refs_read_ref_full() wrapper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:19Z","receivedAt":"2021-10-14T00:06:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the refs_read_ref_full() wrapper in favor of migrating various\nrefs.c API users to the underlying refs_werrres_ref_unsafe() function.\n\nA careful reading of these callers shows that the callers of this\nfunction did not care about \"errno\", by moving away from the\nrefs_resolve_ref_unsafe() wrapper we can be sure that nothing relies\non it anymore.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 20 +++++++++-----------\n refs.h | 2 --\n refs/files-backend.c | 36 ++++++++++++++++++++++--------------\n worktree.c | 9 +++++----\n 4 files changed, 36 insertions(+), 31 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 4398a4c0257..08101a13318 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -289,20 +289,17 @@ struct ref_filter {\n \tvoid *cb_data;\n };\n \n-int refs_read_ref_full(struct ref_store *refs, const char *refname,\n-\t\t int resolve_flags, struct object_id *oid, int *flags)\n+int read_ref_full(const char *refname, int resolve_flags, struct object_id *oid, int *flags)\n {\n-\tif (refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags))\n+\tint ignore_errno;\n+\tstruct ref_store *refs = get_main_ref_store(the_repository);\n+\n+\tif (refs_werrres_ref_unsafe(refs, refname, resolve_flags,\n+\t\t\t\t oid, flags, &ignore_errno))\n \t\treturn 0;\n \treturn -1;\n }\n \n-int read_ref_full(const char *refname, int resolve_flags, struct object_id *oid, int *flags)\n-{\n-\treturn refs_read_ref_full(get_main_ref_store(the_repository), refname,\n-\t\t\t\t resolve_flags, oid, flags);\n-}\n-\n int read_ref(const char *refname, struct object_id *oid)\n {\n \treturn read_ref_full(refname, RESOLVE_REF_READING, oid, NULL);\n@@ -1375,9 +1372,10 @@ int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n {\n \tstruct object_id oid;\n \tint flag;\n+\tint ignore_errno;\n \n-\tif (!refs_read_ref_full(refs, \"HEAD\", RESOLVE_REF_READING,\n-\t\t\t\t&oid, &flag))\n+\tif (refs_werrres_ref_unsafe(refs, \"HEAD\", RESOLVE_REF_READING,\n+\t\t\t\t &oid, &flag, &ignore_errno))\n \t\treturn fn(\"HEAD\", &oid, flag, cb_data);\n \n \treturn 0;\ndiff --git a/refs.h b/refs.h\nindex c8afde6bb50..3938f99c902 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -89,8 +89,6 @@ char *refs_resolve_refdup(struct ref_store *refs,\n char *resolve_refdup(const char *refname, int resolve_flags,\n \t\t struct object_id *oid, int *flags);\n \n-int refs_read_ref_full(struct ref_store *refs, const char *refname,\n-\t\t int resolve_flags, struct object_id *oid, int *flags);\n int read_ref_full(const char *refname, int resolve_flags,\n \t\t struct object_id *oid, int *flags);\n int read_ref(const char *refname, struct object_id *oid);\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 0dd21b2e205..be27823d099 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1006,6 +1006,7 @@ 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 ignore_errno;\n \n \tfiles_assert_main_repository(refs, \"lock_ref_oid_basic\");\n \tassert(err);\n@@ -1032,9 +1033,8 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n \t\tgoto error_return;\n \t}\n \n-\tif (refs_read_ref_full(&refs->base, lock->ref_name,\n-\t\t\t 0,\n-\t\t\t &lock->old_oid, NULL))\n+\tif (!refs_werrres_ref_unsafe(&refs->base, lock->ref_name, 0,\n+\t\t\t\t &lock->old_oid, NULL, &ignore_errno))\n \t\toidclr(&lock->old_oid);\n \tgoto out;\n \n@@ -1397,6 +1397,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,\n \tstruct strbuf tmp_renamed_log = STRBUF_INIT;\n \tint log, ret;\n \tstruct strbuf err = STRBUF_INIT;\n+\tint ignore_errno;\n \n \tfiles_reflog_path(refs, &sb_oldref, oldrefname);\n \tfiles_reflog_path(refs, &sb_newref, newrefname);\n@@ -1454,9 +1455,9 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,\n \t * the safety anyway; we want to delete the reference whatever\n \t * its current value.\n \t */\n-\tif (!copy && !refs_read_ref_full(&refs->base, newrefname,\n-\t\t\t\tRESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n-\t\t\t\tNULL, NULL) &&\n+\tif (!copy && refs_werrres_ref_unsafe(&refs->base, newrefname,\n+\t\t\t\t\t RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n+\t\t\t\t\t NULL, NULL, &ignore_errno) &&\n \t refs_delete_ref(&refs->base, NULL, newrefname,\n \t\t\t NULL, REF_NO_DEREF)) {\n \t\tif (errno == EISDIR) {\n@@ -1868,9 +1869,12 @@ static void update_symref_reflog(struct files_ref_store *refs,\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct object_id new_oid;\n+\tint ignore_errno;\n+\n \tif (logmsg &&\n-\t !refs_read_ref_full(&refs->base, target,\n-\t\t\t\tRESOLVE_REF_READING, &new_oid, NULL) &&\n+\t refs_werrres_ref_unsafe(&refs->base, target,\n+\t\t\t\t RESOLVE_REF_READING, &new_oid, NULL,\n+\t\t\t\t &ignore_errno) &&\n \t files_log_ref_write(refs, refname, &lock->old_oid,\n \t\t\t\t&new_oid, logmsg, 0, &err)) {\n \t\terror(\"%s\", err.buf);\n@@ -2144,6 +2148,7 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\t(struct files_reflog_iterator *)ref_iterator;\n \tstruct dir_iterator *diter = iter->dir_iterator;\n \tint ok;\n+\tint ignore_errno;\n \n \twhile ((ok = dir_iterator_advance(diter)) == ITER_OK) {\n \t\tint flags;\n@@ -2155,9 +2160,10 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\tif (ends_with(diter->basename, \".lock\"))\n \t\t\tcontinue;\n \n-\t\tif (refs_read_ref_full(iter->ref_store,\n-\t\t\t\t diter->relative_path, 0,\n-\t\t\t\t &iter->oid, &flags)) {\n+\t\tif (!refs_werrres_ref_unsafe(iter->ref_store,\n+\t\t\t\t\t diter->relative_path, 0,\n+\t\t\t\t\t &iter->oid, &flags,\n+\t\t\t\t\t &ignore_errno)) {\n \t\t\terror(\"bad ref for %s\", diter->path.buf);\n \t\t\tcontinue;\n \t\t}\n@@ -2501,9 +2507,11 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t * the transaction, so we have to read it here\n \t\t\t * to record and possibly check old_oid:\n \t\t\t */\n-\t\t\tif (refs_read_ref_full(&refs->base,\n-\t\t\t\t\t referent.buf, 0,\n-\t\t\t\t\t &lock->old_oid, NULL)) {\n+\t\t\tint ignore_errno;\n+\t\t\tif (!refs_werrres_ref_unsafe(&refs->base,\n+\t\t\t\t\t\t referent.buf, 0,\n+\t\t\t\t\t\t &lock->old_oid, NULL,\n+\t\t\t\t\t\t &ignore_errno)) {\n \t\t\t\tif (update->flags & REF_HAVE_OLD) {\n \t\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n \t\t\t\t\t\t \"error reading reference\",\ndiff --git a/worktree.c b/worktree.c\nindex 092a4f92ad2..cfffcdb62b3 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -563,16 +563,17 @@ int other_head_refs(each_ref_fn fn, void *cb_data)\n \t\tstruct worktree *wt = *p;\n \t\tstruct object_id oid;\n \t\tint flag;\n+\t\tint ignore_errno;\n \n \t\tif (wt->is_current)\n \t\t\tcontinue;\n \n \t\tstrbuf_reset(&refname);\n \t\tstrbuf_worktree_ref(wt, &refname, \"HEAD\");\n-\t\tif (!refs_read_ref_full(get_main_ref_store(the_repository),\n-\t\t\t\t\trefname.buf,\n-\t\t\t\t\tRESOLVE_REF_READING,\n-\t\t\t\t\t&oid, &flag))\n+\t\tif (refs_werrres_ref_unsafe(get_main_ref_store(the_repository),\n+\t\t\t\t\t refname.buf,\n+\t\t\t\t\t RESOLVE_REF_READING,\n+\t\t\t\t\t &oid, &flag, &ignore_errno))\n \t\t\tret = fn(refname.buf, &oid, flag, cb_data);\n \t\tif (ret)\n \t\t\tbreak;\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438726","messageId":"patch-08.20-acb484ea547-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 08/20] refs API: make resolve_gitlink_ref() not set errno","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:20Z","receivedAt":"2021-10-14T00:06:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I have carefully read the upstream callers of resolve_gitlink_ref()\nand determined that they don't care about errno.\n\nSo let's move away from the errno-setting refs_resolve_ref_unsafe()\nwrapper to refs_werrres_ref_unsafe(), and explicitly ignore the errno\nit sets for us.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 08101a13318..001f54845a7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1790,14 +1790,15 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,\n {\n \tstruct ref_store *refs;\n \tint flags;\n+\tint ignore_errno;\n \n \trefs = get_submodule_ref_store(submodule);\n \n \tif (!refs)\n \t\treturn -1;\n \n-\tif (!refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags) ||\n-\t is_null_oid(oid))\n+\tif (!refs_werrres_ref_unsafe(refs, refname, 0, oid, &flags,\n+\t\t\t\t &ignore_errno) || is_null_oid(oid))\n \t\treturn -1;\n \treturn 0;\n }\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438727","messageId":"patch-09.20-4be84c9bf53-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 09/20] refs API: make loose_fill_ref_dir() not set errno","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:21Z","receivedAt":"2021-10-14T00:06:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the refs_resolve_ref_unsafe() invoked in loose_fill_ref_dir()\nto a form that ignores errno. The only eventual caller of this\nfunction is create_ref_cache(), whose callers in turn don't have their\nfailure depend on any errno set here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs/files-backend.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex be27823d099..1d27f915638 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -280,10 +280,11 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,\n \t\t\t\t\t create_dir_entry(dir->cache, refname.buf,\n \t\t\t\t\t\t\t refname.len));\n \t\t} else {\n-\t\t\tif (!refs_resolve_ref_unsafe(&refs->base,\n+\t\t\tint ignore_errno;\n+\t\t\tif (!refs_werrres_ref_unsafe(&refs->base,\n \t\t\t\t\t\t refname.buf,\n \t\t\t\t\t\t RESOLVE_REF_READING,\n-\t\t\t\t\t\t &oid, &flag)) {\n+\t\t\t\t\t\t &oid, &flag, &ignore_errno)) {\n \t\t\t\toidclr(&oid);\n \t\t\t\tflag |= REF_ISBROKEN;\n \t\t\t} else if (is_null_oid(&oid)) {\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438728","messageId":"patch-10.20-8753210f9cc-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 10/20] refs API: make files_copy_or_rename_ref() et al not set errno","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:22Z","receivedAt":"2021-10-14T00:06:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"None of the callers of rename_ref() and copy_ref() care about errno,\nand as seen in the context here we already emit our own non-errno\nusing error() in the case where we'd use it.\n\nSo let's have it explicitly ignore errno, and do the same in\ncommit_ref_update(), which is only used within other code in\nfiles_copy_or_rename_ref() itself which doesn't care about errno\neither.\n\nIt might actually be sensible to have the callers use errno if the\nfailure was filesystem-specific, and with the upcoming reftable\nbackend we don't want to rely on that sort of thing, so let's keep\nignoring that for now.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs/files-backend.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 1d27f915638..b3d4544dcf0 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1410,9 +1410,9 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,\n \t\tgoto out;\n \t}\n \n-\tif (!refs_resolve_ref_unsafe(&refs->base, oldrefname,\n+\tif (!refs_werrres_ref_unsafe(&refs->base, oldrefname,\n \t\t\t\t RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n-\t\t\t\t&orig_oid, &flag)) {\n+\t\t\t\t &orig_oid, &flag, &ignore_errno)) {\n \t\tret = error(\"refname %s not found\", oldrefname);\n \t\tgoto out;\n \t}\n@@ -1823,10 +1823,12 @@ static int commit_ref_update(struct files_ref_store *refs,\n \t\t */\n \t\tint head_flag;\n \t\tconst char *head_ref;\n+\t\tint ignore_errno;\n \n-\t\thead_ref = refs_resolve_ref_unsafe(&refs->base, \"HEAD\",\n+\t\thead_ref = refs_werrres_ref_unsafe(&refs->base, \"HEAD\",\n \t\t\t\t\t\t RESOLVE_REF_READING,\n-\t\t\t\t\t\t NULL, &head_flag);\n+\t\t\t\t\t\t NULL, &head_flag,\n+\t\t\t\t\t\t &ignore_errno);\n \t\tif (head_ref && (head_flag & REF_ISSYMREF) &&\n \t\t !strcmp(head_ref, lock->ref_name)) {\n \t\t\tstruct strbuf log_err = STRBUF_INIT;\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438729","messageId":"patch-11.20-9fe85926140-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 11/20] refs API: ignore errno in worktree.c's add_head_info()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:23Z","receivedAt":"2021-10-14T00:06:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The static add_head_info() function is only used indirectly by callers\nof get_worktrees(), none of whom care about errno, and even if they\ndid having the faked-up one from refs_resolve_ref_unsafe() would only\nconfuse them if they used die_errno() et al. So let's explicitly\nignore it here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n worktree.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex cfffcdb62b3..fa988ee978f 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -28,11 +28,13 @@ static void add_head_info(struct worktree *wt)\n {\n \tint flags;\n \tconst char *target;\n+\tint ignore_errno;\n \n-\ttarget = refs_resolve_ref_unsafe(get_worktree_ref_store(wt),\n+\ttarget = refs_werrres_ref_unsafe(get_worktree_ref_store(wt),\n \t\t\t\t\t \"HEAD\",\n \t\t\t\t\t 0,\n-\t\t\t\t\t &wt->head_oid, &flags);\n+\t\t\t\t\t &wt->head_oid, &flags,\n+\t\t\t\t\t &ignore_errno);\n \tif (!target)\n \t\treturn;\n \n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438730","messageId":"patch-12.20-8d87db98041-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 12/20] refs API: ignore errno in worktree.c's find_shared_symref()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:24Z","receivedAt":"2021-10-14T00:07:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There are only handful of callers of find_shared_symref(), none of\nwhom care about errno, so let's migrate to the non-errno-propagating\nversion of refs_resolve_ref_unsafe() and explicitly ignore errno here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n worktree.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex fa988ee978f..7d7cf058154 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -420,6 +420,7 @@ const struct worktree *find_shared_symref(const char *symref,\n \t\tconst char *symref_target;\n \t\tstruct ref_store *refs;\n \t\tint flags;\n+\t\tint ignore_errno;\n \n \t\tif (wt->is_bare)\n \t\t\tcontinue;\n@@ -436,8 +437,9 @@ const struct worktree *find_shared_symref(const char *symref,\n \t\t}\n \n \t\trefs = get_worktree_ref_store(wt);\n-\t\tsymref_target = refs_resolve_ref_unsafe(refs, symref, 0,\n-\t\t\t\t\t\t\tNULL, &flags);\n+\t\tsymref_target = refs_werrres_ref_unsafe(refs, symref, 0,\n+\t\t\t\t\t\t\tNULL, &flags,\n+\t\t\t\t\t\t\t&ignore_errno);\n \t\tif ((flags & REF_ISSYMREF) &&\n \t\t symref_target && !strcmp(symref_target, target)) {\n \t\t\texisting = wt;\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438731","messageId":"patch-14.20-fbbc08d3ebd-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 14/20] refs API: make refs_resolve_refdup() not set errno","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:26Z","receivedAt":"2021-10-14T00:07:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move refs_resolve_refdup() from the legacy refs_resolve_ref_unsafe()\nto the new refs_werrres_ref_unsafe(). I have read its callers and\ndetermined that they don't care about errno being set.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 001f54845a7..c64ed6285a6 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -267,9 +267,10 @@ char *refs_resolve_refdup(struct ref_store *refs,\n \t\t\t struct object_id *oid, int *flags)\n {\n \tconst char *result;\n+\tint ignore_errno;\n \n-\tresult = refs_resolve_ref_unsafe(refs, refname, resolve_flags,\n-\t\t\t\t\t oid, flags);\n+\tresult = refs_werrres_ref_unsafe(refs, refname, resolve_flags,\n+\t\t\t\t\t oid, flags, &ignore_errno);\n \treturn xstrdup_or_null(result);\n }\n \n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438732","messageId":"patch-13.20-954633bcbb2-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 13/20] refs tests: ignore ignore errno in test-ref-store helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:25Z","receivedAt":"2021-10-14T00:07:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The cmd_resolve_ref() function has always ignored errno on failure,\nbut let's do so explicitly when using the refs_resolve_ref_unsafe()\nfunction.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/helper/test-ref-store.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex b314b81a45b..2f91fb9b227 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -123,9 +123,10 @@ static int cmd_resolve_ref(struct ref_store *refs, const char **argv)\n \tint resolve_flags = arg_flags(*argv++, \"resolve-flags\");\n \tint flags;\n \tconst char *ref;\n+\tint ignore_errno;\n \n-\tref = refs_resolve_ref_unsafe(refs, refname, resolve_flags,\n-\t\t\t\t &oid, &flags);\n+\tref = refs_werrres_ref_unsafe(refs, refname, resolve_flags,\n+\t\t\t\t &oid, &flags, &ignore_errno);\n \tprintf(\"%s %s 0x%x\\n\", oid_to_hex(&oid), ref ? ref : \"(null)\", flags);\n \treturn ref ? 0 : 1;\n }\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438733","messageId":"patch-15.20-4b2a4dbe7d5-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 15/20] refs API: make refs_ref_exists() not set errno","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:27Z","receivedAt":"2021-10-14T00:07:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move refs_ref_exists from the legacy refs_resolve_ref_unsafe() to the\nnew refs_werrres_ref_unsafe(). I have read its callers and determined\nthat they don't care about errno being set, in particular:\n\n git grep -W -w -e refs_ref_exists -e ref_exists\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex c64ed6285a6..f2a56a216c3 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -308,7 +308,9 @@ int read_ref(const char *refname, struct object_id *oid)\n \n int refs_ref_exists(struct ref_store *refs, const char *refname)\n {\n-\treturn !!refs_resolve_ref_unsafe(refs, refname, RESOLVE_REF_READING, NULL, NULL);\n+\tint ignore_errno;\n+\treturn !!refs_werrres_ref_unsafe(refs, refname, RESOLVE_REF_READING,\n+\t\t\t\t\t NULL, NULL, &ignore_errno);\n }\n \n int ref_exists(const char *refname)\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438734","messageId":"patch-16.20-888b1884c29-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 16/20] refs API: make resolve_ref_unsafe() not set errno","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:28Z","receivedAt":"2021-10-14T00:07:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the resolve_ref_unsafe() wrapper function to use the underlying\nrefs_werrres_ref_unsafe() directly.\n\nFrom a reading of the callers I determined that the only one who cared\nabout errno was a sequencer.c caller added in e47c6cafcb5 (commit:\nmove print_commit_summary() to libgit, 2017-11-24), I'm migrating it\nto using refs_werrres_ref_unsafe() directly.\n\nThis adds another \"set errno\" instance, but in this case it's OK and\nidiomatic. We are setting it just before calling die_errno(). We could\nhave some hypothetical die_errno_var(&saved_errno, ...) here, but I\ndon't think it's worth it. The problem with errno is subtle action at\ndistance, not this sort of thing. We already use this pattern in a\ncouple of places in wrapper.c\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 6 ++++--\n sequencer.c | 10 ++++++++--\n 2 files changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex f2a56a216c3..4b0c1a30a18 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1784,8 +1784,10 @@ int refs_init_db(struct strbuf *err)\n const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n \t\t\t struct object_id *oid, int *flags)\n {\n-\treturn refs_resolve_ref_unsafe(get_main_ref_store(the_repository), refname,\n-\t\t\t\t resolve_flags, oid, flags);\n+\tint ignore_errno;\n+\n+\treturn refs_werrres_ref_unsafe(get_main_ref_store(the_repository), refname,\n+\t\t\t\t resolve_flags, oid, flags, &ignore_errno);\n }\n \n int resolve_gitlink_ref(const char *submodule, const char *refname,\ndiff --git a/sequencer.c b/sequencer.c\nindex 64b1f2e681c..7052f791410 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1281,6 +1281,8 @@ void print_commit_summary(struct repository *r,\n \tstruct pretty_print_context pctx = {0};\n \tstruct strbuf author_ident = STRBUF_INIT;\n \tstruct strbuf committer_ident = STRBUF_INIT;\n+\tstruct ref_store *refs;\n+\tint resolve_errno;\n \n \tcommit = lookup_commit(r, oid);\n \tif (!commit)\n@@ -1330,9 +1332,13 @@ void print_commit_summary(struct repository *r,\n \trev.diffopt.break_opt = 0;\n \tdiff_setup_done(&rev.diffopt);\n \n-\thead = resolve_ref_unsafe(\"HEAD\", 0, NULL, NULL);\n-\tif (!head)\n+\trefs = get_main_ref_store(the_repository);\n+\thead = refs_werrres_ref_unsafe(refs, \"HEAD\", 0, NULL, NULL,\n+\t\t\t\t &resolve_errno);\n+\tif (!head) {\n+\t\terrno = resolve_errno;\n \t\tdie_errno(_(\"unable to resolve HEAD after creating commit\"));\n+\t}\n \tif (!strcmp(head, \"HEAD\"))\n \t\thead = _(\"detached HEAD\");\n \telse\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438735","messageId":"patch-17.20-e2885f13c9b-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 17/20] refs API: make expand_ref() & repo_dwim_log() not set errno","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:29Z","receivedAt":"2021-10-14T00:07:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The use of these two is rather trivial, and it's easy to see none of\ntheir callers care about errno. So let's move them from\nrefs_resolve_ref_unsafe() to refs_resolve_ref_unsafe_with_errno(),\nthese were the last two callers, so we can get rid of that wrapper\nfunction.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 4b0c1a30a18..16f8220b108 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -653,13 +653,16 @@ int expand_ref(struct repository *repo, const char *str, int len,\n \t\tstruct object_id oid_from_ref;\n \t\tstruct object_id *this_result;\n \t\tint flag;\n+\t\tstruct ref_store *refs = get_main_ref_store(repo);\n+\t\tint ignore_errno;\n \n \t\tthis_result = refs_found ? &oid_from_ref : oid;\n \t\tstrbuf_reset(&fullref);\n \t\tstrbuf_addf(&fullref, *p, len, str);\n-\t\tr = refs_resolve_ref_unsafe(get_main_ref_store(repo),\n-\t\t\t\t\t fullref.buf, RESOLVE_REF_READING,\n-\t\t\t\t\t this_result, &flag);\n+\t\tr = refs_werrres_ref_unsafe(refs, fullref.buf,\n+\t\t\t\t\t RESOLVE_REF_READING,\n+\t\t\t\t\t this_result, &flag,\n+\t\t\t\t\t &ignore_errno);\n \t\tif (r) {\n \t\t\tif (!refs_found++)\n \t\t\t\t*ref = xstrdup(r);\n@@ -688,12 +691,14 @@ int repo_dwim_log(struct repository *r, const char *str, int len,\n \tfor (p = ref_rev_parse_rules; *p; p++) {\n \t\tstruct object_id hash;\n \t\tconst char *ref, *it;\n+\t\tint ignore_errno;\n \n \t\tstrbuf_reset(&path);\n \t\tstrbuf_addf(&path, *p, len, str);\n-\t\tref = refs_resolve_ref_unsafe(refs, path.buf,\n+\t\tref = refs_werrres_ref_unsafe(refs, path.buf,\n \t\t\t\t\t RESOLVE_REF_READING,\n-\t\t\t\t\t oid ? &hash : NULL, NULL);\n+\t\t\t\t\t oid ? &hash : NULL, NULL,\n+\t\t\t\t\t &ignore_errno);\n \t\tif (!ref)\n \t\t\tcontinue;\n \t\tif (refs_reflog_exists(refs, path.buf))\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438736","messageId":"patch-18.20-df50373a272-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 18/20] refs API: don't expose \"errno\" in run_transaction_hook()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:30Z","receivedAt":"2021-10-14T00:07:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In run_transaction_hook() we've checked errno since 67541597670 (refs:\nimplement reference transaction hook, 2020-06-19), let's reset errno\nafterwards to make sure nobody using refs.c directly or indirectly\nrelies on it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 16f8220b108..9aa41b55c7b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2095,8 +2095,11 @@ static int run_transaction_hook(struct ref_transaction *transaction,\n \t\t\t update->refname);\n \n \t\tif (write_in_full(proc.in, buf.buf, buf.len) < 0) {\n-\t\t\tif (errno != EPIPE)\n+\t\t\tif (errno != EPIPE) {\n+\t\t\t\t/* Don't leak errno outside this API */\n+\t\t\t\terrno = 0;\n \t\t\t\tret = -1;\n+\t\t\t}\n \t\t\tbreak;\n \t\t}\n \t}\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438737","messageId":"patch-20.20-54b18e3a719-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 20/20] refs API: post-migration API renaming [2/2]","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:32Z","receivedAt":"2021-10-14T00:07:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Rename the transitory refs_werrres_ref_unsafe() function to\nrefs_resolve_ref_unsafe(), now that all callers of the old function\nhave learned to pass in a \"failure_errno\" parameter.\n\nThe coccinelle semantic patch added in the preceding commit works, but\nI couldn't figure out how to get spatch(1) to re-flow these argument\nlists (and sometimes make lines way too long), so this rename was done\nwith:\n\n perl -pi -e 's/refs_werrres_ref_unsafe/refs_resolve_ref_unsafe/g' \\\n $(git grep -l refs_werrres_ref_unsafe -- '*.c')\n\nBut after that \"make contrib/coccinelle/refs.cocci.patch\" comes up\nempty, so the result would have been the same. Let's remove that\ntransitory semantic patch file, we won't need to retain it for any\nother in-flight changes, refs_werrres_ref_unsafe() only existed within\nthis patch series.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n contrib/coccinelle/refs.pending.cocci | 5 -----\n refs.c | 16 ++++++++--------\n refs.h | 2 --\n refs/files-backend.c | 16 ++++++++--------\n sequencer.c | 2 +-\n t/helper/test-ref-store.c | 2 +-\n worktree.c | 6 +++---\n 7 files changed, 21 insertions(+), 28 deletions(-)\n delete mode 100644 contrib/coccinelle/refs.pending.cocci\n\ndiff --git a/contrib/coccinelle/refs.pending.cocci b/contrib/coccinelle/refs.pending.cocci\ndeleted file mode 100644\nindex b33cb8a12aa..00000000000\n--- a/contrib/coccinelle/refs.pending.cocci\n+++ /dev/null\n@@ -1,5 +0,0 @@\n-@@\n-expression refs, refname, resolve_flags, oid, flags, failure_errno;\n-@@\n-- refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)\n-+ refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)\ndiff --git a/refs.c b/refs.c\nindex a19f156b66a..75c6eafa847 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -269,7 +269,7 @@ char *refs_resolve_refdup(struct ref_store *refs,\n \tconst char *result;\n \tint ignore_errno;\n \n-\tresult = refs_werrres_ref_unsafe(refs, refname, resolve_flags,\n+\tresult = refs_resolve_ref_unsafe(refs, refname, resolve_flags,\n \t\t\t\t\t oid, flags, &ignore_errno);\n \treturn xstrdup_or_null(result);\n }\n@@ -295,7 +295,7 @@ int read_ref_full(const char *refname, int resolve_flags, struct object_id *oid,\n \tint ignore_errno;\n \tstruct ref_store *refs = get_main_ref_store(the_repository);\n \n-\tif (refs_werrres_ref_unsafe(refs, refname, resolve_flags,\n+\tif (refs_resolve_ref_unsafe(refs, refname, resolve_flags,\n \t\t\t\t oid, flags, &ignore_errno))\n \t\treturn 0;\n \treturn -1;\n@@ -309,7 +309,7 @@ int read_ref(const char *refname, struct object_id *oid)\n int refs_ref_exists(struct ref_store *refs, const char *refname)\n {\n \tint ignore_errno;\n-\treturn !!refs_werrres_ref_unsafe(refs, refname, RESOLVE_REF_READING,\n+\treturn !!refs_resolve_ref_unsafe(refs, refname, RESOLVE_REF_READING,\n \t\t\t\t\t NULL, NULL, &ignore_errno);\n }\n \n@@ -659,7 +659,7 @@ int expand_ref(struct repository *repo, const char *str, int len,\n \t\tthis_result = refs_found ? &oid_from_ref : oid;\n \t\tstrbuf_reset(&fullref);\n \t\tstrbuf_addf(&fullref, *p, len, str);\n-\t\tr = refs_werrres_ref_unsafe(refs, fullref.buf,\n+\t\tr = refs_resolve_ref_unsafe(refs, fullref.buf,\n \t\t\t\t\t RESOLVE_REF_READING,\n \t\t\t\t\t this_result, &flag,\n \t\t\t\t\t &ignore_errno);\n@@ -695,7 +695,7 @@ int repo_dwim_log(struct repository *r, const char *str, int len,\n \n \t\tstrbuf_reset(&path);\n \t\tstrbuf_addf(&path, *p, len, str);\n-\t\tref = refs_werrres_ref_unsafe(refs, path.buf,\n+\t\tref = refs_resolve_ref_unsafe(refs, path.buf,\n \t\t\t\t\t RESOLVE_REF_READING,\n \t\t\t\t\t oid ? &hash : NULL, NULL,\n \t\t\t\t\t &ignore_errno);\n@@ -1382,7 +1382,7 @@ int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n \tint flag;\n \tint ignore_errno;\n \n-\tif (refs_werrres_ref_unsafe(refs, \"HEAD\", RESOLVE_REF_READING,\n+\tif (refs_resolve_ref_unsafe(refs, \"HEAD\", RESOLVE_REF_READING,\n \t\t\t\t &oid, &flag, &ignore_errno))\n \t\treturn fn(\"HEAD\", &oid, flag, cb_data);\n \n@@ -1778,7 +1778,7 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n {\n \tint ignore_errno;\n \n-\treturn refs_werrres_ref_unsafe(get_main_ref_store(the_repository), refname,\n+\treturn refs_resolve_ref_unsafe(get_main_ref_store(the_repository), refname,\n \t\t\t\t resolve_flags, oid, flags, &ignore_errno);\n }\n \n@@ -1794,7 +1794,7 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,\n \tif (!refs)\n \t\treturn -1;\n \n-\tif (!refs_werrres_ref_unsafe(refs, refname, 0, oid, &flags,\n+\tif (!refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags,\n \t\t\t\t &ignore_errno) || is_null_oid(oid))\n \t\treturn -1;\n \treturn 0;\ndiff --git a/refs.h b/refs.h\nindex d908a161c06..45c34e99e3a 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -68,8 +68,6 @@ struct worktree;\n #define RESOLVE_REF_NO_RECURSE 0x02\n #define RESOLVE_REF_ALLOW_BAD_NAME 0x04\n \n-#define refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno) \\\n-\trefs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)\n const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t\t\t const char *refname,\n \t\t\t\t int resolve_flags,\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex b3d4544dcf0..e9d754d5ad3 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -281,7 +281,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,\n \t\t\t\t\t\t\t refname.len));\n \t\t} else {\n \t\t\tint ignore_errno;\n-\t\t\tif (!refs_werrres_ref_unsafe(&refs->base,\n+\t\t\tif (!refs_resolve_ref_unsafe(&refs->base,\n \t\t\t\t\t\t refname.buf,\n \t\t\t\t\t\t RESOLVE_REF_READING,\n \t\t\t\t\t\t &oid, &flag, &ignore_errno)) {\n@@ -1034,7 +1034,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n \t\tgoto error_return;\n \t}\n \n-\tif (!refs_werrres_ref_unsafe(&refs->base, lock->ref_name, 0,\n+\tif (!refs_resolve_ref_unsafe(&refs->base, lock->ref_name, 0,\n \t\t\t\t &lock->old_oid, NULL, &ignore_errno))\n \t\toidclr(&lock->old_oid);\n \tgoto out;\n@@ -1410,7 +1410,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,\n \t\tgoto out;\n \t}\n \n-\tif (!refs_werrres_ref_unsafe(&refs->base, oldrefname,\n+\tif (!refs_resolve_ref_unsafe(&refs->base, oldrefname,\n \t\t\t\t RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n \t\t\t\t &orig_oid, &flag, &ignore_errno)) {\n \t\tret = error(\"refname %s not found\", oldrefname);\n@@ -1456,7 +1456,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,\n \t * the safety anyway; we want to delete the reference whatever\n \t * its current value.\n \t */\n-\tif (!copy && refs_werrres_ref_unsafe(&refs->base, newrefname,\n+\tif (!copy && refs_resolve_ref_unsafe(&refs->base, newrefname,\n \t\t\t\t\t RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n \t\t\t\t\t NULL, NULL, &ignore_errno) &&\n \t refs_delete_ref(&refs->base, NULL, newrefname,\n@@ -1825,7 +1825,7 @@ static int commit_ref_update(struct files_ref_store *refs,\n \t\tconst char *head_ref;\n \t\tint ignore_errno;\n \n-\t\thead_ref = refs_werrres_ref_unsafe(&refs->base, \"HEAD\",\n+\t\thead_ref = refs_resolve_ref_unsafe(&refs->base, \"HEAD\",\n \t\t\t\t\t\t RESOLVE_REF_READING,\n \t\t\t\t\t\t NULL, &head_flag,\n \t\t\t\t\t\t &ignore_errno);\n@@ -1875,7 +1875,7 @@ static void update_symref_reflog(struct files_ref_store *refs,\n \tint ignore_errno;\n \n \tif (logmsg &&\n-\t refs_werrres_ref_unsafe(&refs->base, target,\n+\t refs_resolve_ref_unsafe(&refs->base, target,\n \t\t\t\t RESOLVE_REF_READING, &new_oid, NULL,\n \t\t\t\t &ignore_errno) &&\n \t files_log_ref_write(refs, refname, &lock->old_oid,\n@@ -2163,7 +2163,7 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\tif (ends_with(diter->basename, \".lock\"))\n \t\t\tcontinue;\n \n-\t\tif (!refs_werrres_ref_unsafe(iter->ref_store,\n+\t\tif (!refs_resolve_ref_unsafe(iter->ref_store,\n \t\t\t\t\t diter->relative_path, 0,\n \t\t\t\t\t &iter->oid, &flags,\n \t\t\t\t\t &ignore_errno)) {\n@@ -2511,7 +2511,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t * to record and possibly check old_oid:\n \t\t\t */\n \t\t\tint ignore_errno;\n-\t\t\tif (!refs_werrres_ref_unsafe(&refs->base,\n+\t\t\tif (!refs_resolve_ref_unsafe(&refs->base,\n \t\t\t\t\t\t referent.buf, 0,\n \t\t\t\t\t\t &lock->old_oid, NULL,\n \t\t\t\t\t\t &ignore_errno)) {\ndiff --git a/sequencer.c b/sequencer.c\nindex 7052f791410..d848c5dc985 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1333,7 +1333,7 @@ void print_commit_summary(struct repository *r,\n \tdiff_setup_done(&rev.diffopt);\n \n \trefs = get_main_ref_store(the_repository);\n-\thead = refs_werrres_ref_unsafe(refs, \"HEAD\", 0, NULL, NULL,\n+\thead = refs_resolve_ref_unsafe(refs, \"HEAD\", 0, NULL, NULL,\n \t\t\t\t &resolve_errno);\n \tif (!head) {\n \t\terrno = resolve_errno;\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 2f91fb9b227..3986665037a 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -125,7 +125,7 @@ static int cmd_resolve_ref(struct ref_store *refs, const char **argv)\n \tconst char *ref;\n \tint ignore_errno;\n \n-\tref = refs_werrres_ref_unsafe(refs, refname, resolve_flags,\n+\tref = refs_resolve_ref_unsafe(refs, refname, resolve_flags,\n \t\t\t\t &oid, &flags, &ignore_errno);\n \tprintf(\"%s %s 0x%x\\n\", oid_to_hex(&oid), ref ? ref : \"(null)\", flags);\n \treturn ref ? 0 : 1;\ndiff --git a/worktree.c b/worktree.c\nindex 7d7cf058154..2c155b10150 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -30,7 +30,7 @@ static void add_head_info(struct worktree *wt)\n \tconst char *target;\n \tint ignore_errno;\n \n-\ttarget = refs_werrres_ref_unsafe(get_worktree_ref_store(wt),\n+\ttarget = refs_resolve_ref_unsafe(get_worktree_ref_store(wt),\n \t\t\t\t\t \"HEAD\",\n \t\t\t\t\t 0,\n \t\t\t\t\t &wt->head_oid, &flags,\n@@ -437,7 +437,7 @@ const struct worktree *find_shared_symref(const char *symref,\n \t\t}\n \n \t\trefs = get_worktree_ref_store(wt);\n-\t\tsymref_target = refs_werrres_ref_unsafe(refs, symref, 0,\n+\t\tsymref_target = refs_resolve_ref_unsafe(refs, symref, 0,\n \t\t\t\t\t\t\tNULL, &flags,\n \t\t\t\t\t\t\t&ignore_errno);\n \t\tif ((flags & REF_ISSYMREF) &&\n@@ -574,7 +574,7 @@ int other_head_refs(each_ref_fn fn, void *cb_data)\n \n \t\tstrbuf_reset(&refname);\n \t\tstrbuf_worktree_ref(wt, &refname, \"HEAD\");\n-\t\tif (refs_werrres_ref_unsafe(get_main_ref_store(the_repository),\n+\t\tif (refs_resolve_ref_unsafe(get_main_ref_store(the_repository),\n \t\t\t\t\t refname.buf,\n \t\t\t\t\t RESOLVE_REF_READING,\n \t\t\t\t\t &oid, &flag, &ignore_errno))\n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438738","messageId":"patch-19.20-4c80b05bf1d-20211013T235900Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH 19/20] refs API: post-migration API renaming [1/2]","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-14T00:06:31Z","receivedAt":"2021-10-14T00:07:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In preceding commits all callers of refs_resolve_ref_unsafe() were\nmigrated to the transitory refs_werrres_ref_unsafe() function.\n\nAs a first step in getting rid of it let's remove the old function\nfrom the public API (it went unused in a preceding commit).\n\nWe then provide both a coccinelle rule to do the rename, and a macro\nto avoid breaking the existing callers.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n contrib/coccinelle/refs.pending.cocci | 5 +++++\n refs.c | 15 +--------------\n refs.h | 22 +++++++++-------------\n 3 files changed, 15 insertions(+), 27 deletions(-)\n create mode 100644 contrib/coccinelle/refs.pending.cocci\n\ndiff --git a/contrib/coccinelle/refs.pending.cocci b/contrib/coccinelle/refs.pending.cocci\nnew file mode 100644\nindex 00000000000..b33cb8a12aa\n--- /dev/null\n+++ b/contrib/coccinelle/refs.pending.cocci\n@@ -0,0 +1,5 @@\n+@@\n+expression refs, refname, resolve_flags, oid, flags, failure_errno;\n+@@\n+- refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)\n++ refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)\ndiff --git a/refs.c b/refs.c\nindex 9aa41b55c7b..a19f156b66a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1668,7 +1668,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t\t\t\t\t type, failure_errno);\n }\n \n-const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n+const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t\t\t const char *refname,\n \t\t\t\t int resolve_flags,\n \t\t\t\t struct object_id *oid,\n@@ -1765,19 +1765,6 @@ const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n \treturn NULL;\n }\n \n-const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname,\n-\t\t\t\t int resolve_flags, struct object_id *oid,\n-\t\t\t\t int *flags)\n-{\n-\tint failure_errno = 0;\n-\tconst char *refn;\n-\trefn = refs_werrres_ref_unsafe(refs, refname, resolve_flags,\n-\t\t\t\t oid, flags, &failure_errno);\n-\tif (!refn)\n-\t\terrno = failure_errno;\n-\treturn refn;\n-}\n-\n /* backend functions */\n int refs_init_db(struct strbuf *err)\n {\ndiff --git a/refs.h b/refs.h\nindex 3938f99c902..d908a161c06 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -11,18 +11,6 @@ struct string_list;\n struct string_list_item;\n struct worktree;\n \n-/*\n- * Callers should not inspect \"errno\" on failure, but rather pass in a\n- * \"failure_errno\" parameter, on failure the \"errno\" will indicate the\n- * type of failure encountered, but not necessarily one that came from\n- * a syscall. We might have faked it up.\n- */\n-const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n-\t\t\t\t const char *refname,\n-\t\t\t\t int resolve_flags,\n-\t\t\t\t struct object_id *oid,\n-\t\t\t\t int *flags, int *failure_errno);\n-\n /*\n * Resolve a reference, recursively following symbolic refererences.\n *\n@@ -70,16 +58,24 @@ const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n * resolved. The function returns NULL for such ref names.\n * Caps and underscores refers to the special refs, such as HEAD,\n * FETCH_HEAD and friends, that all live outside of the refs/ directory.\n+ *\n+ * Callers should not inspect \"errno\" on failure, but rather pass in a\n+ * \"failure_errno\" parameter, on failure the \"errno\" will indicate the\n+ * type of failure encountered, but not necessarily one that came from\n+ * a syscall. We might have faked it up.\n */\n #define RESOLVE_REF_READING 0x01\n #define RESOLVE_REF_NO_RECURSE 0x02\n #define RESOLVE_REF_ALLOW_BAD_NAME 0x04\n \n+#define refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno) \\\n+\trefs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)\n const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t\t\t const char *refname,\n \t\t\t\t int resolve_flags,\n \t\t\t\t struct object_id *oid,\n-\t\t\t\t int *flags);\n+\t\t\t\t int *flags, int *failure_errno);\n+\n const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n \t\t\t struct object_id *oid, int *flags);\n \n-- \n2.33.1.1346.g48288c3c089\n\n"},{"id":"438747","messageId":"CAPig+cQ7OqYi3T9GHh36JAsYfyiW8RJsLaztnhcvs2iWjsp9tw@mail.gmail.com","threadId":"56705","inReplyTo":"patch-01.20-bea88e382c0-20211013T235900Z-avarab@gmail.com","subject":"Re: [PATCH 01/20] branch tests: test for errno propagating on failing read","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-10-14T01:57:36Z","receivedAt":"2021-10-14T01:57:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Oct 13, 2021 at 8:07 PM Ævar Arnfjörð Bjarmason\n wrote:\n> Add a test for \"git branch\" to cover the case where .git/refs is\n> symlinked. To check availability, refs_verify_refname_available() will\n> run refs_read_raw_ref() on each prefix, leading to a read() from\n> .git/refs (which is a directory).\n>\n> It would probably be more robust to re-issue the lstat() as a normal\n> stat(), in which case, we would fall back to the directory case, but\n> for now let's just test for the existing behavior as-is. This test\n> covers a regression in a commit that only ever made it to \"next\", see\n> [1].\n>\n> Signed-off-by: Han-Wen Nienhuys \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> @@ -731,6 +731,26 @@ test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog for\n> +test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '\n> + test_when_finished \"rm -rf subdir\" &&\n> + git init subdir &&\n> +\n> + (\n> + cd subdir &&\n> + for d in refs objects packed-refs\n> + do\n> + rm -rf .git/$d &&\n> + ln -s ../../.git/$d .git/$d\n> + done\n> + ) &&\n\nAmend the last line of the loop body:\n\n ln -s ../../.git/$d .git/$d || exit 1\n\n> + git --git-dir subdir/.git/ branch rename-src &&\n> + git rev-parse rename-src >expect &&\n> + git --git-dir subdir/.git/ branch -m rename-src rename-dest &&\n> + git rev-parse rename-dest >actual &&\n> + test_cmp expect actual &&\n> + git branch -D rename-dest\n> +'\n"},{"id":"438792","messageId":"xmqqh7djba6e.fsf@gitster.g","threadId":"56705","inReplyTo":"patch-06.20-3162bf28505-20211013T235900Z-avarab@gmail.com","subject":"Re: [PATCH 06/20] refs/files: remove \"name exist?\" check in lock_ref_oid_basic()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-14T20:53:13Z","receivedAt":"2021-10-14T20:53:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 0af6ee44552..0dd21b2e205 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -1013,16 +1013,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n> \tCALLOC_ARRAY(lock, 1);\n> \n> \tfiles_ref_path(refs, &ref_file, refname);\n> -\tif (!refs_resolve_ref_unsafe(&refs->base, refname,\n> -\t\t\t\t RESOLVE_REF_NO_RECURSE,\n> -\t\t\t\t &lock->old_oid, type)) {\n> -\t\tif (!refs_verify_refname_available(&refs->base, refname,\n> -\t\t\t\t\t\t NULL, NULL, err))\n> -\t\t\tstrbuf_addf(err, \"unable to resolve reference '%s': %s\",\n> -\t\t\t\t refname, strerror(errno));\n> -\n> -\t\tgoto error_return;\n> -\t}\n> \n> \t/*\n> \t * If the ref did not exist and we are creating it, make sure\n\nThis has the side effect of \"type\" not touched at all by this\nfunction, which in turn triggers \"type is used uninitialized?\"\nerror in files_reflog_expire() that calls this function.\n\n"},{"id":"438925","messageId":"patch-v2-04.21-988bf62b3f2-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 04/21] refs API: make parse_loose_ref_contents() not set errno","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:10Z","receivedAt":"2021-10-16T09:39:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Han-Wen Nienhuys \n\nChange the parse_loose_ref_contents() function to stop setting \"errno\"\nand failure, and to instead pass up a \"failure_errno\" via a\nparameter. This requires changing its callers to do the same.\n\nThe EINVAL error from parse_loose_ref_contents is used in files-backend\nto create a custom error message.\n\nIn untangling this we discovered a tricky edge case. The\nrefs_read_special_head() function was relying on\nparse_loose_ref_contents() setting EINVAL.\n\nBy converting it to use \"saved_errno\" we can migrate away from \"errno\"\nin this part of the code entirely, and do away with an existing\n\"save_errno\" pattern, its only purpose was to not clobber the \"errno\"\nwe previously needed at the end of files_read_raw_ref().\n\nLet's assert that we can do that by not having files_read_raw_ref()\nitself operate on *failure_errno in addition to passing it on. Instead\nwe'll assert that if we return non-zero we actually do set errno, thus\nassuring ourselves and callers that they can trust the resulting\n\"failure_errno\".\n\nSigned-off-by: Han-Wen Nienhuys \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 8 +++++---\n refs/files-backend.c | 31 ++++++++++++++++++++-----------\n refs/refs-internal.h | 6 ++++--\n 3 files changed, 29 insertions(+), 16 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 200c44e6963..bb09993c2f9 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1648,7 +1648,8 @@ int for_each_fullref_in_prefixes(const char *namespace,\n \n static int refs_read_special_head(struct ref_store *ref_store,\n \t\t\t\t const char *refname, struct object_id *oid,\n-\t\t\t\t struct strbuf *referent, unsigned int *type)\n+\t\t\t\t struct strbuf *referent, unsigned int *type,\n+\t\t\t\t int *failure_errno)\n {\n \tstruct strbuf full_path = STRBUF_INIT;\n \tstruct strbuf content = STRBUF_INIT;\n@@ -1658,7 +1659,8 @@ static int refs_read_special_head(struct ref_store *ref_store,\n \tif (strbuf_read_file(&content, full_path.buf, 0) < 0)\n \t\tgoto done;\n \n-\tresult = parse_loose_ref_contents(content.buf, oid, referent, type);\n+\tresult = parse_loose_ref_contents(content.buf, oid, referent, type,\n+\t\t\t\t\t failure_errno);\n \n done:\n \tstrbuf_release(&full_path);\n@@ -1673,7 +1675,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \tassert(failure_errno);\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\t\t\t\t type, failure_errno);\n \t}\n \n \treturn ref_store->be->read_raw_ref(ref_store, refname, oid, referent,\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 94c194665ed..c73ffd1ca33 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -355,6 +355,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \tint fd;\n \tint ret = -1;\n \tint remaining_retries = 3;\n+\tint myerr = 0;\n \n \t*type = 0;\n \tstrbuf_reset(&sb_path);\n@@ -382,11 +383,13 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \n \tif (lstat(path, &st) < 0) {\n \t\tint ignore_errno;\n-\t\tif (errno != ENOENT)\n+\t\tmyerr = errno;\n+\t\terrno = 0;\n+\t\tif (myerr != ENOENT)\n \t\t\tgoto out;\n \t\tif (refs_read_raw_ref(refs->packed_ref_store, refname, oid,\n \t\t\t\t referent, type, &ignore_errno)) {\n-\t\t\terrno = ENOENT;\n+\t\t\tmyerr = ENOENT;\n \t\t\tgoto out;\n \t\t}\n \t\tret = 0;\n@@ -397,7 +400,9 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \tif (S_ISLNK(st.st_mode)) {\n \t\tstrbuf_reset(&sb_contents);\n \t\tif (strbuf_readlink(&sb_contents, path, st.st_size) < 0) {\n-\t\t\tif (errno == ENOENT || errno == EINVAL)\n+\t\t\tmyerr = errno;\n+\t\t\terrno = 0;\n+\t\t\tif (myerr == ENOENT || myerr == EINVAL)\n \t\t\t\t/* inconsistent with lstat; retry */\n \t\t\t\tgoto stat_ref;\n \t\t\telse\n@@ -427,7 +432,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t\t */\n \t\tif (refs_read_raw_ref(refs->packed_ref_store, refname, oid,\n \t\t\t\t referent, type, &ignore_errno)) {\n-\t\t\terrno = EISDIR;\n+\t\t\tmyerr = EISDIR;\n \t\t\tgoto out;\n \t\t}\n \t\tret = 0;\n@@ -440,7 +445,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t */\n \tfd = open(path, O_RDONLY);\n \tif (fd < 0) {\n-\t\tif (errno == ENOENT && !S_ISLNK(st.st_mode))\n+\t\tmyerr = errno;\n+\t\tif (myerr == ENOENT && !S_ISLNK(st.st_mode))\n \t\t\t/* inconsistent with lstat; retry */\n \t\t\tgoto stat_ref;\n \t\telse\n@@ -448,26 +454,29 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t}\n \tstrbuf_reset(&sb_contents);\n \tif (strbuf_read(&sb_contents, fd, 256) < 0) {\n-\t\tint save_errno = errno;\n+\t\tmyerr = errno;\n \t\tclose(fd);\n-\t\terrno = save_errno;\n \t\tgoto out;\n \t}\n \tclose(fd);\n \tstrbuf_rtrim(&sb_contents);\n \tbuf = sb_contents.buf;\n \n-\tret = parse_loose_ref_contents(buf, oid, referent, type);\n+\tret = parse_loose_ref_contents(buf, oid, referent, type, &myerr);\n \n out:\n-\t*failure_errno = errno;\n+\tif (ret && !myerr)\n+\t\tBUG(\"returning non-zero %d, should have set myerr!\", ret);\n+\t*failure_errno = myerr;\n+\n \tstrbuf_release(&sb_path);\n \tstrbuf_release(&sb_contents);\n \treturn ret;\n }\n \n int parse_loose_ref_contents(const char *buf, 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 \tconst char *p;\n \tif (skip_prefix(buf, \"ref:\", &buf)) {\n@@ -486,7 +495,7 @@ int parse_loose_ref_contents(const char *buf, struct object_id *oid,\n \tif (parse_oid_hex(buf, oid, &p) ||\n \t (*p != '\\0' && !isspace(*p))) {\n \t\t*type |= REF_ISBROKEN;\n-\t\terrno = EINVAL;\n+\t\t*failure_errno = EINVAL;\n \t\treturn -1;\n \t}\n \treturn 0;\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex c87f1135e5b..121b8fdad08 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -706,10 +706,12 @@ struct ref_store {\n };\n \n /*\n- * Parse contents of a loose ref file.\n+ * Parse contents of a loose ref file. *failure_errno maybe be set to EINVAL for\n+ * invalid contents.\n */\n int parse_loose_ref_contents(const char *buf, 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 /*\n * Fill in the generic part of refs and add it to our collection of\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438926","messageId":"patch-v2-06.21-d3242f5f687-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 06/21] reflog tests: add --updateref tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:12Z","receivedAt":"2021-10-16T09:39:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add tests that cover blindspots in \"git reflog delete --updateref\"\nbehavior. Before this change removing the \"type & REF_ISSYMREF\" check\nadded in 5e6f003ca8a (reflog_expire(): ignore --updateref for symbolic\nreferences, 2015-03-03) would not fail any tests.\n\nThe \"--updateref\" option was added in 55f10565371 (git-reflog: add\noption --updateref to write the last reflog sha1 into the ref,\n2008-02-22) for use in git-stash.sh, see e25d5f9c82e (git-stash: add\nnew 'drop' subcommand, 2008-02-22).\n\nEven though the regression test I need is just the \"C\" case here,\nlet's test all these combinations for good measure. I started out\ndoing these as a for-loop, but I think this is more readable.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1417-reflog-updateref.sh | 65 +++++++++++++++++++++++++++++++++++++\n 1 file changed, 65 insertions(+)\n create mode 100755 t/t1417-reflog-updateref.sh\n\ndiff --git a/t/t1417-reflog-updateref.sh b/t/t1417-reflog-updateref.sh\nnew file mode 100755\nindex 00000000000..14f13b57c6d\n--- /dev/null\n+++ b/t/t1417-reflog-updateref.sh\n@@ -0,0 +1,65 @@\n+#!/bin/sh\n+\n+test_description='git reflog --updateref'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tgit init -b main repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit A &&\n+\t\ttest_commit B &&\n+\t\ttest_commit C &&\n+\n+\t\tcp .git/logs/HEAD HEAD.old &&\n+\t\tgit reset --hard HEAD~ &&\n+\t\tcp HEAD.old .git/logs/HEAD\n+\t)\n+'\n+\n+test_reflog_updateref () {\n+\texp=$1\n+\tshift\n+\targs=\"$@\"\n+\n+\ttest_expect_success REFFILES \"get '$exp' with '$args'\" '\n+\t\ttest_when_finished \"rm -rf copy\" &&\n+\t\tcp -R repo copy &&\n+\n+\t\t(\n+\t\t\tcd copy &&\n+\n+\t\t\t$args &&\n+\t\t\tgit rev-parse $exp >expect &&\n+\t\t\tgit rev-parse HEAD >actual &&\n+\n+\t\t\ttest_cmp expect actual\n+\t\t)\n+\t'\n+}\n+\n+test_reflog_updateref B git reflog delete --updateref HEAD@{0}\n+test_reflog_updateref B git reflog delete --updateref HEAD@{1}\n+test_reflog_updateref C git reflog delete --updateref main@{0}\n+test_reflog_updateref B git reflog delete --updateref main@{1}\n+test_reflog_updateref B git reflog delete --updateref --rewrite HEAD@{0}\n+test_reflog_updateref B git reflog delete --updateref --rewrite HEAD@{1}\n+test_reflog_updateref C git reflog delete --updateref --rewrite main@{0}\n+test_reflog_updateref B git reflog delete --updateref --rewrite main@{1}\n+test_reflog_updateref B test_must_fail git reflog expire HEAD@{0}\n+test_reflog_updateref B test_must_fail git reflog expire HEAD@{1}\n+test_reflog_updateref B test_must_fail git reflog expire main@{0}\n+test_reflog_updateref B test_must_fail git reflog expire main@{1}\n+test_reflog_updateref B test_must_fail git reflog expire --updateref HEAD@{0}\n+test_reflog_updateref B test_must_fail git reflog expire --updateref HEAD@{1}\n+test_reflog_updateref B test_must_fail git reflog expire --updateref main@{0}\n+test_reflog_updateref B test_must_fail git reflog expire --updateref main@{1}\n+test_reflog_updateref B test_must_fail git reflog expire --updateref --rewrite HEAD@{0}\n+test_reflog_updateref B test_must_fail git reflog expire --updateref --rewrite HEAD@{1}\n+test_reflog_updateref B test_must_fail git reflog expire --updateref --rewrite main@{0}\n+test_reflog_updateref B test_must_fail git reflog expire --updateref --rewrite main@{1}\n+\n+test_done\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438927","messageId":"patch-v2-03.21-8932b109087-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 03/21] refs API: make refs_read_raw_ref() not set errno","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:09Z","receivedAt":"2021-10-16T09:39:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Han-Wen Nienhuys \n\nAdd a \"failure_errno\" to refs_read_raw_ref(), his allows\nrefs_werrres_ref_unsafe() to pass along its \"failure_errno\", as a\nfirst step before its own callers are migrated to pass it further up\nthe chain.\n\nWe are leaving out out the refs_read_special_head() in\nrefs_read_raw_ref() for now, as noted in a subsequent commit moving it\nto \"failure_errno\" will require some special consideration.\n\nSigned-off-by: Han-Wen Nienhuys \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 24 ++++++++++++++++--------\n refs/files-backend.c | 10 ++++++----\n refs/packed-backend.c | 7 ++++---\n refs/refs-internal.h | 6 +++---\n 4 files changed, 29 insertions(+), 18 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ad56dbb0125..200c44e6963 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1666,17 +1666,18 @@ static int refs_read_special_head(struct ref_store *ref_store,\n \treturn result;\n }\n \n-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+int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,\n+\t\t struct object_id *oid, struct strbuf *referent,\n+\t\t unsigned int *type, int *failure_errno)\n {\n+\tassert(failure_errno);\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, &errno);\n+\t\t\t\t\t type, failure_errno);\n }\n \n const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n@@ -1720,9 +1721,8 @@ const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n \tfor (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) {\n \t\tunsigned int read_flags = 0;\n \n-\t\terrno = 0;\n-\t\tif (refs_read_raw_ref(refs, refname,\n-\t\t\t\t oid, &sb_refname, &read_flags)) {\n+\t\tif (refs_read_raw_ref(refs, refname, oid, &sb_refname,\n+\t\t\t\t &read_flags, failure_errno)) {\n \t\t\t*flags |= read_flags;\n \t\t\tif (errno)\n \t\t\t\t*failure_errno = errno;\n@@ -2240,6 +2240,13 @@ int refs_verify_refname_available(struct ref_store *refs,\n \n \tstrbuf_grow(&dirname, strlen(refname) + 1);\n \tfor (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) {\n+\t\t/*\n+\t\t * Just saying \"Is a directory\" when we e.g. can't\n+\t\t * lock some multi-level ref isn't very informative,\n+\t\t * the user won't be told *what* is a directory, so\n+\t\t * let's not use strerror() below.\n+\t\t */\n+\t\tint ignore_errno;\n \t\t/* Expand dirname to the new prefix, not including the trailing slash: */\n \t\tstrbuf_add(&dirname, refname + dirname.len, slash - refname - dirname.len);\n \n@@ -2251,7 +2258,8 @@ int refs_verify_refname_available(struct ref_store *refs,\n \t\tif (skip && string_list_has_string(skip, dirname.buf))\n \t\t\tcontinue;\n \n-\t\tif (!refs_read_raw_ref(refs, dirname.buf, &oid, &referent, &type)) {\n+\t\tif (!refs_read_raw_ref(refs, dirname.buf, &oid, &referent,\n+\t\t\t\t &type, &ignore_errno)) {\n \t\t\tstrbuf_addf(err, _(\"'%s' exists; cannot create '%s'\"),\n \t\t\t\t dirname.buf, refname);\n \t\t\tgoto cleanup;\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 6a6ead0b99b..94c194665ed 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -381,10 +381,11 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t\tgoto out;\n \n \tif (lstat(path, &st) < 0) {\n+\t\tint ignore_errno;\n \t\tif (errno != ENOENT)\n \t\t\tgoto out;\n-\t\tif (refs_read_raw_ref(refs->packed_ref_store, refname,\n-\t\t\t\t oid, referent, type)) {\n+\t\tif (refs_read_raw_ref(refs->packed_ref_store, refname, oid,\n+\t\t\t\t referent, type, &ignore_errno)) {\n \t\t\terrno = ENOENT;\n \t\t\tgoto out;\n \t\t}\n@@ -418,13 +419,14 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \n \t/* Is it a directory? */\n \tif (S_ISDIR(st.st_mode)) {\n+\t\tint ignore_errno;\n \t\t/*\n \t\t * Even though there is a directory where the loose\n \t\t * ref is supposed to be, there could still be a\n \t\t * packed ref:\n \t\t */\n-\t\tif (refs_read_raw_ref(refs->packed_ref_store, refname,\n-\t\t\t\t oid, referent, type)) {\n+\t\tif (refs_read_raw_ref(refs->packed_ref_store, refname, oid,\n+\t\t\t\t referent, type, &ignore_errno)) {\n \t\t\terrno = EISDIR;\n \t\t\tgoto out;\n \t\t}\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 47247a14917..52cdc94a26e 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1347,6 +1347,7 @@ int is_packed_transaction_needed(struct ref_store *ref_store,\n \tret = 0;\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n+\t\tint failure_errno;\n \t\tunsigned int type;\n \t\tstruct object_id oid;\n \n@@ -1357,9 +1358,9 @@ int is_packed_transaction_needed(struct ref_store *ref_store,\n \t\t\t */\n \t\t\tcontinue;\n \n-\t\tif (!refs_read_raw_ref(ref_store, update->refname,\n-\t\t\t\t &oid, &referent, &type) ||\n-\t\t errno != ENOENT) {\n+\t\tif (!refs_read_raw_ref(ref_store, update->refname, &oid,\n+\t\t\t\t &referent, &type, &failure_errno) ||\n+\t\t failure_errno != ENOENT) {\n \t\t\t/*\n \t\t\t * We have to actually delete that reference\n \t\t\t * -> this transaction is needed.\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 72746407fc3..c87f1135e5b 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -149,9 +149,9 @@ struct ref_update {\n \tconst char refname[FLEX_ARRAY];\n };\n \n-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+int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,\n+\t\t struct object_id *oid, struct strbuf *referent,\n+\t\t unsigned int *type, int *failure_errno);\n \n /*\n * Write an error to `err` and return a nonzero value iff the same\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438928","messageId":"patch-v2-01.21-1863c597c98-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 01/21] branch tests: test for errno propagating on failing read","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:07Z","receivedAt":"2021-10-16T09:39:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Han-Wen Nienhuys \n\nAdd a test for \"git branch\" to cover the case where .git/refs is\nsymlinked. To check availability, refs_verify_refname_available() will\nrun refs_read_raw_ref() on each prefix, leading to a read() from\n.git/refs (which is a directory).\n\nIt would probably be more robust to re-issue the lstat() as a normal\nstat(), in which case, we would fall back to the directory case, but\nfor now let's just test for the existing behavior as-is. This test\ncovers a regression in a commit that only ever made it to \"next\", see\n[1].\n\n1. http://lore.kernel.org/git/pull.1068.git.git.1629203489546.gitgitgadget@gmail.com\n\nSigned-off-by: Han-Wen Nienhuys \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t3200-branch.sh | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex e575ffb4ffb..53e0d094bb4 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -731,6 +731,28 @@ test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog for\n \ttest_must_fail git branch -m u v\n '\n \n+test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '\n+\ttest_when_finished \"rm -rf subdir\" &&\n+\tgit init --bare subdir &&\n+\n+\trm -rfv subdir/refs subdir/objects subdir/packed-refs &&\n+\tln -s ../.git/refs subdir/refs &&\n+\tln -s ../.git/objects subdir/objects &&\n+\tln -s ../.git/packed-refs subdir/packed-refs &&\n+\n+\tgit -C subdir rev-parse --absolute-git-dir >subdir.dir &&\n+\tgit rev-parse --absolute-git-dir >our.dir &&\n+\t! test_cmp subdir.dir our.dir &&\n+\n+\tgit -C subdir log &&\n+\tgit -C subdir branch rename-src &&\n+\tgit rev-parse rename-src >expect &&\n+\tgit -C subdir branch -m rename-src rename-dest &&\n+\tgit rev-parse rename-dest >actual &&\n+\ttest_cmp expect actual &&\n+\tgit branch -D rename-dest\n+'\n+\n test_expect_success 'test tracking setup via --track' '\n \tgit config remote.local.url . &&\n \tgit config remote.local.fetch refs/heads/*:refs/remotes/local/* &&\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438929","messageId":"patch-v2-08.21-b2532614430-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 08/21] refs API: remove refs_read_ref_full() wrapper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:14Z","receivedAt":"2021-10-16T09:39:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the refs_read_ref_full() wrapper in favor of migrating various\nrefs.c API users to the underlying refs_werrres_ref_unsafe() function.\n\nA careful reading of these callers shows that the callers of this\nfunction did not care about \"errno\", by moving away from the\nrefs_resolve_ref_unsafe() wrapper we can be sure that nothing relies\non it anymore.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 20 +++++++++-----------\n refs.h | 2 --\n refs/files-backend.c | 36 ++++++++++++++++++++++--------------\n worktree.c | 9 +++++----\n 4 files changed, 36 insertions(+), 31 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 44ddbb14f1d..80b85d244d2 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -290,20 +290,17 @@ struct ref_filter {\n \tvoid *cb_data;\n };\n \n-int refs_read_ref_full(struct ref_store *refs, const char *refname,\n-\t\t int resolve_flags, struct object_id *oid, int *flags)\n+int read_ref_full(const char *refname, int resolve_flags, struct object_id *oid, int *flags)\n {\n-\tif (refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags))\n+\tint ignore_errno;\n+\tstruct ref_store *refs = get_main_ref_store(the_repository);\n+\n+\tif (refs_werrres_ref_unsafe(refs, refname, resolve_flags,\n+\t\t\t\t oid, flags, &ignore_errno))\n \t\treturn 0;\n \treturn -1;\n }\n \n-int read_ref_full(const char *refname, int resolve_flags, struct object_id *oid, int *flags)\n-{\n-\treturn refs_read_ref_full(get_main_ref_store(the_repository), refname,\n-\t\t\t\t resolve_flags, oid, flags);\n-}\n-\n int read_ref(const char *refname, struct object_id *oid)\n {\n \treturn read_ref_full(refname, RESOLVE_REF_READING, oid, NULL);\n@@ -1376,9 +1373,10 @@ int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n {\n \tstruct object_id oid;\n \tint flag;\n+\tint ignore_errno;\n \n-\tif (!refs_read_ref_full(refs, \"HEAD\", RESOLVE_REF_READING,\n-\t\t\t\t&oid, &flag))\n+\tif (refs_werrres_ref_unsafe(refs, \"HEAD\", RESOLVE_REF_READING,\n+\t\t\t\t &oid, &flag, &ignore_errno))\n \t\treturn fn(\"HEAD\", &oid, flag, cb_data);\n \n \treturn 0;\ndiff --git a/refs.h b/refs.h\nindex c8afde6bb50..3938f99c902 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -89,8 +89,6 @@ char *refs_resolve_refdup(struct ref_store *refs,\n char *resolve_refdup(const char *refname, int resolve_flags,\n \t\t struct object_id *oid, int *flags);\n \n-int refs_read_ref_full(struct ref_store *refs, const char *refname,\n-\t\t int resolve_flags, struct object_id *oid, int *flags);\n int read_ref_full(const char *refname, int resolve_flags,\n \t\t struct object_id *oid, int *flags);\n int read_ref(const char *refname, struct object_id *oid);\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 16e78326381..482d04de03a 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1006,6 +1006,7 @@ 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 ignore_errno;\n \n \tfiles_assert_main_repository(refs, \"lock_ref_oid_basic\");\n \tassert(err);\n@@ -1032,9 +1033,8 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n \t\tgoto error_return;\n \t}\n \n-\tif (refs_read_ref_full(&refs->base, lock->ref_name,\n-\t\t\t 0,\n-\t\t\t &lock->old_oid, NULL))\n+\tif (!refs_werrres_ref_unsafe(&refs->base, lock->ref_name, 0,\n+\t\t\t\t &lock->old_oid, NULL, &ignore_errno))\n \t\toidclr(&lock->old_oid);\n \tgoto out;\n \n@@ -1397,6 +1397,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,\n \tstruct strbuf tmp_renamed_log = STRBUF_INIT;\n \tint log, ret;\n \tstruct strbuf err = STRBUF_INIT;\n+\tint ignore_errno;\n \n \tfiles_reflog_path(refs, &sb_oldref, oldrefname);\n \tfiles_reflog_path(refs, &sb_newref, newrefname);\n@@ -1454,9 +1455,9 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,\n \t * the safety anyway; we want to delete the reference whatever\n \t * its current value.\n \t */\n-\tif (!copy && !refs_read_ref_full(&refs->base, newrefname,\n-\t\t\t\tRESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n-\t\t\t\tNULL, NULL) &&\n+\tif (!copy && refs_werrres_ref_unsafe(&refs->base, newrefname,\n+\t\t\t\t\t RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n+\t\t\t\t\t NULL, NULL, &ignore_errno) &&\n \t refs_delete_ref(&refs->base, NULL, newrefname,\n \t\t\t NULL, REF_NO_DEREF)) {\n \t\tif (errno == EISDIR) {\n@@ -1868,9 +1869,12 @@ static void update_symref_reflog(struct files_ref_store *refs,\n {\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct object_id new_oid;\n+\tint ignore_errno;\n+\n \tif (logmsg &&\n-\t !refs_read_ref_full(&refs->base, target,\n-\t\t\t\tRESOLVE_REF_READING, &new_oid, NULL) &&\n+\t refs_werrres_ref_unsafe(&refs->base, target,\n+\t\t\t\t RESOLVE_REF_READING, &new_oid, NULL,\n+\t\t\t\t &ignore_errno) &&\n \t files_log_ref_write(refs, refname, &lock->old_oid,\n \t\t\t\t&new_oid, logmsg, 0, &err)) {\n \t\terror(\"%s\", err.buf);\n@@ -2144,6 +2148,7 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\t(struct files_reflog_iterator *)ref_iterator;\n \tstruct dir_iterator *diter = iter->dir_iterator;\n \tint ok;\n+\tint ignore_errno;\n \n \twhile ((ok = dir_iterator_advance(diter)) == ITER_OK) {\n \t\tint flags;\n@@ -2155,9 +2160,10 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\tif (ends_with(diter->basename, \".lock\"))\n \t\t\tcontinue;\n \n-\t\tif (refs_read_ref_full(iter->ref_store,\n-\t\t\t\t diter->relative_path, 0,\n-\t\t\t\t &iter->oid, &flags)) {\n+\t\tif (!refs_werrres_ref_unsafe(iter->ref_store,\n+\t\t\t\t\t diter->relative_path, 0,\n+\t\t\t\t\t &iter->oid, &flags,\n+\t\t\t\t\t &ignore_errno)) {\n \t\t\terror(\"bad ref for %s\", diter->path.buf);\n \t\t\tcontinue;\n \t\t}\n@@ -2501,9 +2507,11 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t * the transaction, so we have to read it here\n \t\t\t * to record and possibly check old_oid:\n \t\t\t */\n-\t\t\tif (refs_read_ref_full(&refs->base,\n-\t\t\t\t\t referent.buf, 0,\n-\t\t\t\t\t &lock->old_oid, NULL)) {\n+\t\t\tint ignore_errno;\n+\t\t\tif (!refs_werrres_ref_unsafe(&refs->base,\n+\t\t\t\t\t\t referent.buf, 0,\n+\t\t\t\t\t\t &lock->old_oid, NULL,\n+\t\t\t\t\t\t &ignore_errno)) {\n \t\t\t\tif (update->flags & REF_HAVE_OLD) {\n \t\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n \t\t\t\t\t\t \"error reading reference\",\ndiff --git a/worktree.c b/worktree.c\nindex 092a4f92ad2..cfffcdb62b3 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -563,16 +563,17 @@ int other_head_refs(each_ref_fn fn, void *cb_data)\n \t\tstruct worktree *wt = *p;\n \t\tstruct object_id oid;\n \t\tint flag;\n+\t\tint ignore_errno;\n \n \t\tif (wt->is_current)\n \t\t\tcontinue;\n \n \t\tstrbuf_reset(&refname);\n \t\tstrbuf_worktree_ref(wt, &refname, \"HEAD\");\n-\t\tif (!refs_read_ref_full(get_main_ref_store(the_repository),\n-\t\t\t\t\trefname.buf,\n-\t\t\t\t\tRESOLVE_REF_READING,\n-\t\t\t\t\t&oid, &flag))\n+\t\tif (refs_werrres_ref_unsafe(get_main_ref_store(the_repository),\n+\t\t\t\t\t refname.buf,\n+\t\t\t\t\t RESOLVE_REF_READING,\n+\t\t\t\t\t &oid, &flag, &ignore_errno))\n \t\t\tret = fn(refname.buf, &oid, flag, cb_data);\n \t\tif (ret)\n \t\t\tbreak;\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438930","messageId":"patch-v2-02.21-f6d784b4979-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 02/21] refs API: add a version of refs_resolve_ref_unsafe() with \"errno\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:08Z","receivedAt":"2021-10-16T09:39:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a new refs_werrres_ref_unsafe() function, which is like\nrefs_resolve_ref_unsafe() except that it explicitly saves away the\n\"errno\" to a passed-in parameter, the refs_resolve_ref_unsafe() then\nbecomes a wrapper for it.\n\nIn subsequent commits we'll migrate code over to it, before finally\nmaking \"refs_resolve_ref_unsafe()\" with an \"errno\" parameter the\ncanonical version, so this this function exists only so that we can\nincrementally migrate callers, it will be going away in a subsequent\ncommit.\n\nAs the added comment notes has a rather tortured name to be the same\nlength as \"refs_resolve_ref_unsafe\", to avoid churn as we won't need\nto re-indent the argument lists, similarly the documentation and\nstructure of it in refs.h is designed to minimize a diff in a\nsubsequent commit, where that documentation will be added to the new\nrefs_resolve_ref_unsafe().\n\nAt the end of this migration the \"meaningful errno\" TODO item left in\n76d70dc0c63 (refs.c: make resolve_ref_unsafe set errno to something\nmeaningful on error, 2014-06-20) will be resolved.\n\nAs can be seen from the use of refs_read_raw_ref() we'll also need to\nconvert some functions that the new refs_werrres_ref_unsafe() itself\ncalls to take this \"failure_errno\". That will be done in subsequent\ncommits.\n\nSigned-off-by: Han-Wen Nienhuys \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 36 +++++++++++++++++++++++++++---------\n refs.h | 12 ++++++++++++\n 2 files changed, 39 insertions(+), 9 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 7f019c2377e..ad56dbb0125 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1679,17 +1679,19 @@ int refs_read_raw_ref(struct ref_store *ref_store,\n \t\t\t\t\t type, &errno);\n }\n \n-/* This function needs to return a meaningful errno on failure */\n-const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n+const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n \t\t\t\t const char *refname,\n \t\t\t\t int resolve_flags,\n-\t\t\t\t struct object_id *oid, int *flags)\n+\t\t\t\t struct object_id *oid,\n+\t\t\t\t int *flags, int *failure_errno)\n {\n \tstatic struct strbuf sb_refname = STRBUF_INIT;\n \tstruct object_id unused_oid;\n \tint unused_flags;\n \tint symref_count;\n \n+\tassert(failure_errno);\n+\n \tif (!oid)\n \t\toid = &unused_oid;\n \tif (!flags)\n@@ -1700,7 +1702,7 @@ 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\t*failure_errno = EINVAL;\n \t\t\treturn NULL;\n \t\t}\n \n@@ -1718,9 +1720,12 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \tfor (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) {\n \t\tunsigned int read_flags = 0;\n \n+\t\terrno = 0;\n \t\tif (refs_read_raw_ref(refs, refname,\n \t\t\t\t oid, &sb_refname, &read_flags)) {\n \t\t\t*flags |= read_flags;\n+\t\t\tif (errno)\n+\t\t\t\t*failure_errno = errno;\n \n \t\t\t/* In reading mode, refs must eventually resolve */\n \t\t\tif (resolve_flags & RESOLVE_REF_READING)\n@@ -1731,9 +1736,9 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t\t * may show errors besides ENOENT if there are\n \t\t\t * similarly-named refs.\n \t\t\t */\n-\t\t\tif (errno != ENOENT &&\n-\t\t\t errno != EISDIR &&\n-\t\t\t errno != ENOTDIR)\n+\t\t\tif (*failure_errno != ENOENT &&\n+\t\t\t *failure_errno != EISDIR &&\n+\t\t\t *failure_errno != ENOTDIR)\n \t\t\t\treturn NULL;\n \n \t\t\toidclr(oid);\n@@ -1760,7 +1765,7 @@ 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\t*failure_errno = EINVAL;\n \t\t\t\treturn NULL;\n \t\t\t}\n \n@@ -1768,10 +1773,23 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t}\n \t}\n \n-\terrno = ELOOP;\n+\t*failure_errno = ELOOP;\n \treturn NULL;\n }\n \n+const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname,\n+\t\t\t\t int resolve_flags, struct object_id *oid,\n+\t\t\t\t int *flags)\n+{\n+\tint failure_errno = 0;\n+\tconst char *refn;\n+\trefn = refs_werrres_ref_unsafe(refs, refname, resolve_flags,\n+\t\t\t\t oid, flags, &failure_errno);\n+\tif (!refn)\n+\t\terrno = failure_errno;\n+\treturn refn;\n+}\n+\n /* backend functions */\n int refs_init_db(struct strbuf *err)\n {\ndiff --git a/refs.h b/refs.h\nindex d5099d4984e..c8afde6bb50 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -11,6 +11,18 @@ struct string_list;\n struct string_list_item;\n struct worktree;\n \n+/*\n+ * Callers should not inspect \"errno\" on failure, but rather pass in a\n+ * \"failure_errno\" parameter, on failure the \"errno\" will indicate the\n+ * type of failure encountered, but not necessarily one that came from\n+ * a syscall. We might have faked it up.\n+ */\n+const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n+\t\t\t\t const char *refname,\n+\t\t\t\t int resolve_flags,\n+\t\t\t\t struct object_id *oid,\n+\t\t\t\t int *flags, int *failure_errno);\n+\n /*\n * Resolve a reference, recursively following symbolic refererences.\n *\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438931","messageId":"patch-v2-05.21-8073f93a904-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 05/21] refs API: make refs_rename_ref_available() static","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:11Z","receivedAt":"2021-10-16T09:39:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move the refs_rename_ref_available() function into\n\"refs/files-backend.c\". It is file-backend specific.\n\nThis function was added in 5fe7d825da8 (refs.c: pass a list of names\nto skip to is_refname_available, 2014-05-01) as rename_ref_available()\nand was only ever used in this one file-backend specific codepath. So\nlet's move it there.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 19 -------------------\n refs/files-backend.c | 29 +++++++++++++++++++++++++++++\n refs/refs-internal.h | 14 --------------\n 3 files changed, 29 insertions(+), 33 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex bb09993c2f9..44ddbb14f1d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1372,25 +1372,6 @@ const char *find_descendant_ref(const char *dirname,\n \treturn NULL;\n }\n \n-int refs_rename_ref_available(struct ref_store *refs,\n-\t\t\t const char *old_refname,\n-\t\t\t const char *new_refname)\n-{\n-\tstruct string_list skip = STRING_LIST_INIT_NODUP;\n-\tstruct strbuf err = STRBUF_INIT;\n-\tint ok;\n-\n-\tstring_list_insert(&skip, old_refname);\n-\tok = !refs_verify_refname_available(refs, new_refname,\n-\t\t\t\t\t NULL, &skip, &err);\n-\tif (!ok)\n-\t\terror(\"%s\", err.buf);\n-\n-\tstring_list_clear(&skip, 0);\n-\tstrbuf_release(&err);\n-\treturn ok;\n-}\n-\n int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n {\n \tstruct object_id oid;\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex c73ffd1ca33..0af6ee44552 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1363,6 +1363,35 @@ static int commit_ref_update(struct files_ref_store *refs,\n \t\t\t const struct object_id *oid, const char *logmsg,\n \t\t\t struct strbuf *err);\n \n+/*\n+ * Check whether an attempt to rename old_refname to new_refname would\n+ * cause a D/F conflict with any existing reference (other than\n+ * possibly old_refname). If there would be a conflict, emit an error\n+ * message and return false; otherwise, return true.\n+ *\n+ * Note that this function is not safe against all races with other\n+ * processes (though rename_ref() catches some races that might get by\n+ * this check).\n+ */\n+static int refs_rename_ref_available(struct ref_store *refs,\n+\t\t\t const char *old_refname,\n+\t\t\t const char *new_refname)\n+{\n+\tstruct string_list skip = STRING_LIST_INIT_NODUP;\n+\tstruct strbuf err = STRBUF_INIT;\n+\tint ok;\n+\n+\tstring_list_insert(&skip, old_refname);\n+\tok = !refs_verify_refname_available(refs, new_refname,\n+\t\t\t\t\t NULL, &skip, &err);\n+\tif (!ok)\n+\t\terror(\"%s\", err.buf);\n+\n+\tstring_list_clear(&skip, 0);\n+\tstrbuf_release(&err);\n+\treturn ok;\n+}\n+\n static int files_copy_or_rename_ref(struct ref_store *ref_store,\n \t\t\t const char *oldrefname, const char *newrefname,\n \t\t\t const char *logmsg, int copy)\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 121b8fdad08..2ecd3dea9fb 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -228,20 +228,6 @@ const char *find_descendant_ref(const char *dirname,\n \t\t\t\tconst struct string_list *extras,\n \t\t\t\tconst struct string_list *skip);\n \n-/*\n- * Check whether an attempt to rename old_refname to new_refname would\n- * cause a D/F conflict with any existing reference (other than\n- * possibly old_refname). If there would be a conflict, emit an error\n- * message and return false; otherwise, return true.\n- *\n- * Note that this function is not safe against all races with other\n- * processes (though rename_ref() catches some races that might get by\n- * this check).\n- */\n-int refs_rename_ref_available(struct ref_store *refs,\n-\t\t\t const char *old_refname,\n-\t\t\t const char *new_refname);\n-\n /* We allow \"recursive\" symbolic refs. Only within reason, though */\n #define SYMREF_MAXDEPTH 5\n \n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438932","messageId":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com","subject":"[PATCH v2 00/21] refs: stop having the API set \"errno\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:06Z","receivedAt":"2021-10-16T09:40:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"See [1] for the v1 summary. There was a regression there with \"git\nreflog\"'s \"--updateref\" option, which had no meaningful tests in the\ntest suite (only one test in \"t1503-rev-parse-verify.sh\"). Test for\nit, and address Eric's comment about \"t3200-branch.sh\" by\nre-structuring that test away from a for-loop.\n\n1. https://lore.kernel.org/git/cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com/\n\nHan-Wen Nienhuys (3):\n branch tests: test for errno propagating on failing read\n refs API: make refs_read_raw_ref() not set errno\n refs API: make parse_loose_ref_contents() not set errno\n\nÆvar Arnfjörð Bjarmason (18):\n refs API: add a version of refs_resolve_ref_unsafe() with \"errno\"\n refs API: make refs_rename_ref_available() static\n reflog tests: add --updateref tests\n refs/files: remove \"name exist?\" check in lock_ref_oid_basic()\n refs API: remove refs_read_ref_full() wrapper\n refs API: make resolve_gitlink_ref() not set errno\n refs API: make loose_fill_ref_dir() not set errno\n refs API: make files_copy_or_rename_ref() et al not set errno\n refs API: ignore errno in worktree.c's add_head_info()\n refs API: ignore errno in worktree.c's find_shared_symref()\n refs tests: ignore ignore errno in test-ref-store helper\n refs API: make refs_resolve_refdup() not set errno\n refs API: make refs_ref_exists() not set errno\n refs API: make resolve_ref_unsafe() not set errno\n refs API: make expand_ref() & repo_dwim_log() not set errno\n refs API: don't expose \"errno\" in run_transaction_hook()\n refs API: post-migration API renaming [1/2]\n refs API: post-migration API renaming [2/2]\n\n refs.c | 124 +++++++++++++++--------------\n refs.h | 10 ++-\n refs/files-backend.c | 153 ++++++++++++++++++++++++------------\n refs/packed-backend.c | 7 +-\n refs/refs-internal.h | 26 ++----\n sequencer.c | 10 ++-\n t/helper/test-ref-store.c | 3 +-\n t/t1417-reflog-updateref.sh | 65 +++++++++++++++\n t/t3200-branch.sh | 22 ++++++\n worktree.c | 17 ++--\n 10 files changed, 294 insertions(+), 143 deletions(-)\n create mode 100755 t/t1417-reflog-updateref.sh\n\nRange-diff against v1:\n 1: bea88e382c0 ! 1: 1863c597c98 branch tests: test for errno propagating on failing read\n @@ t/t3200-branch.sh: test_expect_success SYMLINKS 'git branch -m u v should fail w\n \n +test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '\n +\ttest_when_finished \"rm -rf subdir\" &&\n -+\tgit init subdir &&\n ++\tgit init --bare subdir &&\n +\n -+\t(\n -+\t\tcd subdir &&\n -+\t\tfor d in refs objects packed-refs\n -+\t\tdo\n -+\t\t\trm -rf .git/$d &&\n -+\t\t\tln -s ../../.git/$d .git/$d\n -+\t\tdone\n -+\t) &&\n -+\tgit --git-dir subdir/.git/ branch rename-src &&\n ++\trm -rfv subdir/refs subdir/objects subdir/packed-refs &&\n ++\tln -s ../.git/refs subdir/refs &&\n ++\tln -s ../.git/objects subdir/objects &&\n ++\tln -s ../.git/packed-refs subdir/packed-refs &&\n ++\n ++\tgit -C subdir rev-parse --absolute-git-dir >subdir.dir &&\n ++\tgit rev-parse --absolute-git-dir >our.dir &&\n ++\t! test_cmp subdir.dir our.dir &&\n ++\n ++\tgit -C subdir log &&\n ++\tgit -C subdir branch rename-src &&\n +\tgit rev-parse rename-src >expect &&\n -+\tgit --git-dir subdir/.git/ branch -m rename-src rename-dest &&\n ++\tgit -C subdir branch -m rename-src rename-dest &&\n +\tgit rev-parse rename-dest >actual &&\n +\ttest_cmp expect actual &&\n +\tgit branch -D rename-dest\n 2: 46641111885 = 2: f6d784b4979 refs API: add a version of refs_resolve_ref_unsafe() with \"errno\"\n 3: a1a80715ffe = 3: 8932b109087 refs API: make refs_read_raw_ref() not set errno\n 4: 758c761abcf ! 4: 988bf62b3f2 refs API: make parse_loose_ref_contents() not set errno\n @@ refs/refs-internal.h: struct ref_store {\n \n /*\n * Fill in the generic part of refs and add it to our collection of\n -\n - ## t/t3200-branch.sh ##\n -@@ t/t3200-branch.sh: test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '\n - \t) &&\n - \tgit --git-dir subdir/.git/ branch rename-src &&\n - \tgit rev-parse rename-src >expect &&\n -+\t# Tests a BUG() assertion in files_read_raw_ref()\n - \tgit --git-dir subdir/.git/ branch -m rename-src rename-dest &&\n - \tgit rev-parse rename-dest >actual &&\n - \ttest_cmp expect actual &&\n 5: cdf17fa34a4 = 5: 8073f93a904 refs API: make refs_rename_ref_available() static\n -: ----------- > 6: d3242f5f687 reflog tests: add --updateref tests\n 6: 3162bf28505 ! 7: 7db3b446632 refs/files: remove \"name exist?\" check in lock_ref_oid_basic()\n @@ Commit message\n already, or something we're about to discover by trying to lock the\n ref with raceproof_create_file().\n \n + The one exception is the caller in files_reflog_expire(), who passes\n + us a \"type\" to find out if the reference is a symref or not. We can\n + move the that logic over to that caller, which can now defer its\n + discovery of whether or not the ref is a symref until it's needed. In\n + the preceding commit an exhaustive regression test was added for that\n + case in a new test in \"t1417-reflog-updateref.sh\".\n +\n The improved diagnostics here were added in\n 5b2d8d6f218 (lock_ref_sha1_basic(): improve diagnostics for ref D/F\n conflicts, 2015-05-11), and then much of the surrounding code went\n @@ Commit message\n reflog we're expiring exists.\n \n See ae35e16cd43 (reflog expire: don't lock reflogs using previously\n - seen OID, 2021-08-23) for the current state of that code.\n + seen OID, 2021-08-23) for the current state of that code, and\n + 5e6f003ca8a (reflog_expire(): ignore --updateref for symbolic\n + references, 2015-03-03) for the code we'd break if we only did a\n + \"update = !!ref\" here, which is covered by the aforementioned\n + regression test in \"t1417-reflog-updateref.sh\".\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## refs/files-backend.c ##\n +@@ refs/files-backend.c: static int create_reflock(const char *path, void *cb)\n + * Locks a ref returning the lock on success and NULL on failure.\n + */\n + static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n +-\t\t\t\t\t const char *refname, int *type,\n ++\t\t\t\t\t const char *refname,\n + \t\t\t\t\t struct strbuf *err)\n + {\n + \tstruct strbuf ref_file = STRBUF_INIT;\n @@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n \tCALLOC_ARRAY(lock, 1);\n \n @@ refs/files-backend.c: static int commit_ref_update(struct files_ref_store *refs,\n */\n static int refs_rename_ref_available(struct ref_store *refs,\n \t\t\t const char *old_refname,\n +@@ refs/files-backend.c: static int files_copy_or_rename_ref(struct ref_store *ref_store,\n + \n + \tlogmoved = log;\n + \n +-\tlock = lock_ref_oid_basic(refs, newrefname, NULL, &err);\n ++\tlock = lock_ref_oid_basic(refs, newrefname, &err);\n + \tif (!lock) {\n + \t\tif (copy)\n + \t\t\terror(\"unable to copy '%s' to '%s': %s\", oldrefname, newrefname, err.buf);\n +@@ refs/files-backend.c: static int files_copy_or_rename_ref(struct ref_store *ref_store,\n + \tgoto out;\n + \n + rollback:\n +-\tlock = lock_ref_oid_basic(refs, oldrefname, NULL, &err);\n ++\tlock = lock_ref_oid_basic(refs, oldrefname, &err);\n + \tif (!lock) {\n + \t\terror(\"unable to lock %s for rollback: %s\", oldrefname, err.buf);\n + \t\tstrbuf_release(&err);\n +@@ refs/files-backend.c: static int files_create_symref(struct ref_store *ref_store,\n + \tstruct ref_lock *lock;\n + \tint ret;\n + \n +-\tlock = lock_ref_oid_basic(refs, refname, NULL, &err);\n ++\tlock = lock_ref_oid_basic(refs, refname, &err);\n + \tif (!lock) {\n + \t\terror(\"%s\", err.buf);\n + \t\tstrbuf_release(&err);\n +@@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,\n + \tstruct strbuf log_file_sb = STRBUF_INIT;\n + \tchar *log_file;\n + \tint status = 0;\n +-\tint type;\n + \tstruct strbuf err = STRBUF_INIT;\n + \tconst struct object_id *oid;\n + \n +@@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,\n + \t * reference itself, plus we might need to update the\n + \t * reference if --updateref was specified:\n + \t */\n +-\tlock = lock_ref_oid_basic(refs, refname, &type, &err);\n ++\tlock = lock_ref_oid_basic(refs, refname, &err);\n + \tif (!lock) {\n + \t\terror(\"cannot lock ref '%s': %s\", refname, err.buf);\n + \t\tstrbuf_release(&err);\n +@@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,\n + \t\t * a reference if there are no remaining reflog\n + \t\t * entries.\n + \t\t */\n +-\t\tint update = (flags & EXPIRE_REFLOGS_UPDATE_REF) &&\n +-\t\t\t!(type & REF_ISSYMREF) &&\n +-\t\t\t!is_null_oid(&cb.last_kept_oid);\n ++\t\tint update = 0;\n ++\n ++\t\tif ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&\n ++\t\t !is_null_oid(&cb.last_kept_oid)) {\n ++\t\t\tint ignore_errno;\n ++\t\t\tint type;\n ++\t\t\tconst char *ref;\n ++\n ++\t\t\tref = refs_werrres_ref_unsafe(&refs->base, refname,\n ++\t\t\t\t\t\t RESOLVE_REF_NO_RECURSE,\n ++\t\t\t\t\t\t NULL, &type,\n ++\t\t\t\t\t\t &ignore_errno);\n ++\t\t\tupdate = !!(ref && !(type & REF_ISSYMREF));\n ++\t\t}\n + \n + \t\tif (close_lock_file_gently(&reflog_lock)) {\n + \t\t\tstatus |= error(\"couldn't write %s: %s\", log_file,\n 7: 288237b1900 = 8: b2532614430 refs API: remove refs_read_ref_full() wrapper\n 8: acb484ea547 = 9: 65beb51ae4e refs API: make resolve_gitlink_ref() not set errno\n 9: 4be84c9bf53 = 10: c99c59dcc57 refs API: make loose_fill_ref_dir() not set errno\n10: 8753210f9cc = 11: 22883846a6a refs API: make files_copy_or_rename_ref() et al not set errno\n11: 9fe85926140 = 12: b90e65abd7d refs API: ignore errno in worktree.c's add_head_info()\n12: 8d87db98041 = 13: 38f5f54938f refs API: ignore errno in worktree.c's find_shared_symref()\n13: 954633bcbb2 = 14: 1289d78e4f3 refs tests: ignore ignore errno in test-ref-store helper\n14: fbbc08d3ebd = 15: 152a831b8be refs API: make refs_resolve_refdup() not set errno\n15: 4b2a4dbe7d5 = 16: c4e87181121 refs API: make refs_ref_exists() not set errno\n16: 888b1884c29 = 17: cae56498b43 refs API: make resolve_ref_unsafe() not set errno\n17: e2885f13c9b = 18: 146b030a142 refs API: make expand_ref() & repo_dwim_log() not set errno\n18: df50373a272 = 19: 3d630872aff refs API: don't expose \"errno\" in run_transaction_hook()\n19: 4c80b05bf1d = 20: 56f855d32b7 refs API: post-migration API renaming [1/2]\n20: 54b18e3a719 ! 21: 0135fb5949f refs API: post-migration API renaming [2/2]\n @@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *ref\n \t\t\t\t\t\t referent.buf, 0,\n \t\t\t\t\t\t &lock->old_oid, NULL,\n \t\t\t\t\t\t &ignore_errno)) {\n +@@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,\n + \t\t\tint type;\n + \t\t\tconst char *ref;\n + \n +-\t\t\tref = refs_werrres_ref_unsafe(&refs->base, refname,\n ++\t\t\tref = refs_resolve_ref_unsafe(&refs->base, refname,\n + \t\t\t\t\t\t RESOLVE_REF_NO_RECURSE,\n + \t\t\t\t\t\t NULL, &type,\n + \t\t\t\t\t\t &ignore_errno);\n \n ## sequencer.c ##\n @@ sequencer.c: void print_commit_summary(struct repository *r,\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438933","messageId":"patch-v2-07.21-7db3b446632-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 07/21] refs/files: remove \"name exist?\" check in lock_ref_oid_basic()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:13Z","receivedAt":"2021-10-16T09:40:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In lock_ref_oid_basic() we'll happily lock a reference that doesn't\nexist yet. That's normal, and is how references are initially born,\nbut we don't need to retain checks here in lock_ref_oid_basic() about\nthe state of the ref, when what we're checking is either checked\nalready, or something we're about to discover by trying to lock the\nref with raceproof_create_file().\n\nThe one exception is the caller in files_reflog_expire(), who passes\nus a \"type\" to find out if the reference is a symref or not. We can\nmove the that logic over to that caller, which can now defer its\ndiscovery of whether or not the ref is a symref until it's needed. In\nthe preceding commit an exhaustive regression test was added for that\ncase in a new test in \"t1417-reflog-updateref.sh\".\n\nThe improved diagnostics here were added in\n5b2d8d6f218 (lock_ref_sha1_basic(): improve diagnostics for ref D/F\nconflicts, 2015-05-11), and then much of the surrounding code went\naway recently in my 245fbba46d6 (refs/files: remove unused \"errno ==\nEISDIR\" code, 2021-08-23).\n\nThe refs_resolve_ref_unsafe() code being removed here looks like it\nshould be tasked with doing that, but it's actually redundant to other\ncode.\n\nThe reason for that is as noted in 245fbba46d6 this once widely used\nfunction now only has a handful of callers left, which all handle this\ncase themselves.\n\nTo the extent that we're racy between their check and ours removing\nthis check actually improves the situation, as we'll be doing fewer\nthings between the not-under-lock initial check and acquiring the\nlock.\n\nWhy this is OK for all the remaining callers of lock_ref_oid_basic()\nis noted below. There are only two of those callers:\n\n* \"git branch -[cm] \":\n\n In files_copy_or_rename_ref() we'll call this when we copy or rename\n refs via rename_ref() and copy_ref(). but only after we've checked\n if the refname exists already via its own call to\n refs_resolve_ref_unsafe() and refs_rename_ref_available().\n\n As the updated comment to the latter here notes neither of those are\n actually needed. If we delete not only this code but also\n refs_rename_ref_available() we'll do just fine, we'll just emit a\n less friendly error message if e.g. \"git branch -m A B/C\" would have\n a D/F conflict with a \"B\" file.\n\n Actually we'd probably die before that in case reflogs for the\n branch existed, i.e. when the try to rename() or copy_file() the\n relevant reflog, since if we've got a D/F conflict with a branch\n name we'll probably also have the same with its reflogs (but not\n necessarily, we might have reflogs, but it might not).\n\n As some #leftoverbits that code seems buggy to me, i.e. the reflog\n \"protocol\" should be to get a lock on the main ref, and then perform\n ref and/or reflog operations. That code dates back to\n c976d415e53 (git-branch: add options and tests for branch renaming,\n 2006-11-28) and probably pre-dated the solidifying of that\n convention. But in any case, that edge case is not our bug or\n problem right now.\n\n* \"git reflog expire \":\n\n In files_reflog_expire() we'll call this without previous ref\n existence checking in files-backend.c, but that code is in turn\n called by code that's just finished checking if the refname whose\n reflog we're expiring exists.\n\n See ae35e16cd43 (reflog expire: don't lock reflogs using previously\n seen OID, 2021-08-23) for the current state of that code, and\n 5e6f003ca8a (reflog_expire(): ignore --updateref for symbolic\n references, 2015-03-03) for the code we'd break if we only did a\n \"update = !!ref\" here, which is covered by the aforementioned\n regression test in \"t1417-reflog-updateref.sh\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs/files-backend.c | 48 ++++++++++++++++++++++----------------------\n 1 file changed, 24 insertions(+), 24 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 0af6ee44552..16e78326381 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1001,7 +1001,7 @@ static int create_reflock(const char *path, void *cb)\n * Locks a ref returning the lock on success and NULL on failure.\n */\n static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n-\t\t\t\t\t const char *refname, int *type,\n+\t\t\t\t\t const char *refname,\n \t\t\t\t\t struct strbuf *err)\n {\n \tstruct strbuf ref_file = STRBUF_INIT;\n@@ -1013,16 +1013,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n \tCALLOC_ARRAY(lock, 1);\n \n \tfiles_ref_path(refs, &ref_file, refname);\n-\tif (!refs_resolve_ref_unsafe(&refs->base, refname,\n-\t\t\t\t RESOLVE_REF_NO_RECURSE,\n-\t\t\t\t &lock->old_oid, type)) {\n-\t\tif (!refs_verify_refname_available(&refs->base, refname,\n-\t\t\t\t\t\t NULL, NULL, err))\n-\t\t\tstrbuf_addf(err, \"unable to resolve reference '%s': %s\",\n-\t\t\t\t refname, strerror(errno));\n-\n-\t\tgoto error_return;\n-\t}\n \n \t/*\n \t * If the ref did not exist and we are creating it, make sure\n@@ -1364,14 +1354,14 @@ static int commit_ref_update(struct files_ref_store *refs,\n \t\t\t struct strbuf *err);\n \n /*\n- * Check whether an attempt to rename old_refname to new_refname would\n- * cause a D/F conflict with any existing reference (other than\n- * possibly old_refname). If there would be a conflict, emit an error\n+ * Emit a better error message than lockfile.c's\n+ * unable_to_lock_message() would in case there is a D/F conflict with\n+ * another existing reference. If there would be a conflict, emit an error\n * message and return false; otherwise, return true.\n *\n * Note that this function is not safe against all races with other\n- * processes (though rename_ref() catches some races that might get by\n- * this check).\n+ * processes, and that's not its job. We'll emit a more verbose error on D/f\n+ * conflicts if we get past it into lock_ref_oid_basic().\n */\n static int refs_rename_ref_available(struct ref_store *refs,\n \t\t\t const char *old_refname,\n@@ -1492,7 +1482,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,\n \n \tlogmoved = log;\n \n-\tlock = lock_ref_oid_basic(refs, newrefname, NULL, &err);\n+\tlock = lock_ref_oid_basic(refs, newrefname, &err);\n \tif (!lock) {\n \t\tif (copy)\n \t\t\terror(\"unable to copy '%s' to '%s': %s\", oldrefname, newrefname, err.buf);\n@@ -1514,7 +1504,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,\n \tgoto out;\n \n rollback:\n-\tlock = lock_ref_oid_basic(refs, oldrefname, NULL, &err);\n+\tlock = lock_ref_oid_basic(refs, oldrefname, &err);\n \tif (!lock) {\n \t\terror(\"unable to lock %s for rollback: %s\", oldrefname, err.buf);\n \t\tstrbuf_release(&err);\n@@ -1921,7 +1911,7 @@ static int files_create_symref(struct ref_store *ref_store,\n \tstruct ref_lock *lock;\n \tint ret;\n \n-\tlock = lock_ref_oid_basic(refs, refname, NULL, &err);\n+\tlock = lock_ref_oid_basic(refs, refname, &err);\n \tif (!lock) {\n \t\terror(\"%s\", err.buf);\n \t\tstrbuf_release(&err);\n@@ -3125,7 +3115,6 @@ static int files_reflog_expire(struct ref_store *ref_store,\n \tstruct strbuf log_file_sb = STRBUF_INIT;\n \tchar *log_file;\n \tint status = 0;\n-\tint type;\n \tstruct strbuf err = STRBUF_INIT;\n \tconst struct object_id *oid;\n \n@@ -3139,7 +3128,7 @@ static int files_reflog_expire(struct ref_store *ref_store,\n \t * reference itself, plus we might need to update the\n \t * reference if --updateref was specified:\n \t */\n-\tlock = lock_ref_oid_basic(refs, refname, &type, &err);\n+\tlock = lock_ref_oid_basic(refs, refname, &err);\n \tif (!lock) {\n \t\terror(\"cannot lock ref '%s': %s\", refname, err.buf);\n \t\tstrbuf_release(&err);\n@@ -3201,9 +3190,20 @@ static int files_reflog_expire(struct ref_store *ref_store,\n \t\t * a reference if there are no remaining reflog\n \t\t * entries.\n \t\t */\n-\t\tint update = (flags & EXPIRE_REFLOGS_UPDATE_REF) &&\n-\t\t\t!(type & REF_ISSYMREF) &&\n-\t\t\t!is_null_oid(&cb.last_kept_oid);\n+\t\tint update = 0;\n+\n+\t\tif ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&\n+\t\t !is_null_oid(&cb.last_kept_oid)) {\n+\t\t\tint ignore_errno;\n+\t\t\tint type;\n+\t\t\tconst char *ref;\n+\n+\t\t\tref = refs_werrres_ref_unsafe(&refs->base, refname,\n+\t\t\t\t\t\t RESOLVE_REF_NO_RECURSE,\n+\t\t\t\t\t\t NULL, &type,\n+\t\t\t\t\t\t &ignore_errno);\n+\t\t\tupdate = !!(ref && !(type & REF_ISSYMREF));\n+\t\t}\n \n \t\tif (close_lock_file_gently(&reflog_lock)) {\n \t\t\tstatus |= error(\"couldn't write %s: %s\", log_file,\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438934","messageId":"patch-v2-09.21-65beb51ae4e-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 09/21] refs API: make resolve_gitlink_ref() not set errno","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:15Z","receivedAt":"2021-10-16T09:40:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I have carefully read the upstream callers of resolve_gitlink_ref()\nand determined that they don't care about errno.\n\nSo let's move away from the errno-setting refs_resolve_ref_unsafe()\nwrapper to refs_werrres_ref_unsafe(), and explicitly ignore the errno\nit sets for us.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 80b85d244d2..dc6ed561492 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1791,14 +1791,15 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,\n {\n \tstruct ref_store *refs;\n \tint flags;\n+\tint ignore_errno;\n \n \trefs = get_submodule_ref_store(submodule);\n \n \tif (!refs)\n \t\treturn -1;\n \n-\tif (!refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags) ||\n-\t is_null_oid(oid))\n+\tif (!refs_werrres_ref_unsafe(refs, refname, 0, oid, &flags,\n+\t\t\t\t &ignore_errno) || is_null_oid(oid))\n \t\treturn -1;\n \treturn 0;\n }\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438935","messageId":"patch-v2-10.21-c99c59dcc57-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 10/21] refs API: make loose_fill_ref_dir() not set errno","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:16Z","receivedAt":"2021-10-16T09:40:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the refs_resolve_ref_unsafe() invoked in loose_fill_ref_dir()\nto a form that ignores errno. The only eventual caller of this\nfunction is create_ref_cache(), whose callers in turn don't have their\nfailure depend on any errno set here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs/files-backend.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 482d04de03a..759c21e88ae 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -280,10 +280,11 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,\n \t\t\t\t\t create_dir_entry(dir->cache, refname.buf,\n \t\t\t\t\t\t\t refname.len));\n \t\t} else {\n-\t\t\tif (!refs_resolve_ref_unsafe(&refs->base,\n+\t\t\tint ignore_errno;\n+\t\t\tif (!refs_werrres_ref_unsafe(&refs->base,\n \t\t\t\t\t\t refname.buf,\n \t\t\t\t\t\t RESOLVE_REF_READING,\n-\t\t\t\t\t\t &oid, &flag)) {\n+\t\t\t\t\t\t &oid, &flag, &ignore_errno)) {\n \t\t\t\toidclr(&oid);\n \t\t\t\tflag |= REF_ISBROKEN;\n \t\t\t} else if (is_null_oid(&oid)) {\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438936","messageId":"patch-v2-12.21-b90e65abd7d-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 12/21] refs API: ignore errno in worktree.c's add_head_info()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:18Z","receivedAt":"2021-10-16T09:40:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The static add_head_info() function is only used indirectly by callers\nof get_worktrees(), none of whom care about errno, and even if they\ndid having the faked-up one from refs_resolve_ref_unsafe() would only\nconfuse them if they used die_errno() et al. So let's explicitly\nignore it here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n worktree.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex cfffcdb62b3..fa988ee978f 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -28,11 +28,13 @@ static void add_head_info(struct worktree *wt)\n {\n \tint flags;\n \tconst char *target;\n+\tint ignore_errno;\n \n-\ttarget = refs_resolve_ref_unsafe(get_worktree_ref_store(wt),\n+\ttarget = refs_werrres_ref_unsafe(get_worktree_ref_store(wt),\n \t\t\t\t\t \"HEAD\",\n \t\t\t\t\t 0,\n-\t\t\t\t\t &wt->head_oid, &flags);\n+\t\t\t\t\t &wt->head_oid, &flags,\n+\t\t\t\t\t &ignore_errno);\n \tif (!target)\n \t\treturn;\n \n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438937","messageId":"patch-v2-11.21-22883846a6a-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 11/21] refs API: make files_copy_or_rename_ref() et al not set errno","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:17Z","receivedAt":"2021-10-16T09:40:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"None of the callers of rename_ref() and copy_ref() care about errno,\nand as seen in the context here we already emit our own non-errno\nusing error() in the case where we'd use it.\n\nSo let's have it explicitly ignore errno, and do the same in\ncommit_ref_update(), which is only used within other code in\nfiles_copy_or_rename_ref() itself which doesn't care about errno\neither.\n\nIt might actually be sensible to have the callers use errno if the\nfailure was filesystem-specific, and with the upcoming reftable\nbackend we don't want to rely on that sort of thing, so let's keep\nignoring that for now.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs/files-backend.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 759c21e88ae..6c854dda533 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1410,9 +1410,9 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,\n \t\tgoto out;\n \t}\n \n-\tif (!refs_resolve_ref_unsafe(&refs->base, oldrefname,\n+\tif (!refs_werrres_ref_unsafe(&refs->base, oldrefname,\n \t\t\t\t RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n-\t\t\t\t&orig_oid, &flag)) {\n+\t\t\t\t &orig_oid, &flag, &ignore_errno)) {\n \t\tret = error(\"refname %s not found\", oldrefname);\n \t\tgoto out;\n \t}\n@@ -1823,10 +1823,12 @@ static int commit_ref_update(struct files_ref_store *refs,\n \t\t */\n \t\tint head_flag;\n \t\tconst char *head_ref;\n+\t\tint ignore_errno;\n \n-\t\thead_ref = refs_resolve_ref_unsafe(&refs->base, \"HEAD\",\n+\t\thead_ref = refs_werrres_ref_unsafe(&refs->base, \"HEAD\",\n \t\t\t\t\t\t RESOLVE_REF_READING,\n-\t\t\t\t\t\t NULL, &head_flag);\n+\t\t\t\t\t\t NULL, &head_flag,\n+\t\t\t\t\t\t &ignore_errno);\n \t\tif (head_ref && (head_flag & REF_ISSYMREF) &&\n \t\t !strcmp(head_ref, lock->ref_name)) {\n \t\t\tstruct strbuf log_err = STRBUF_INIT;\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438938","messageId":"patch-v2-14.21-1289d78e4f3-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 14/21] refs tests: ignore ignore errno in test-ref-store helper","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:20Z","receivedAt":"2021-10-16T09:40:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The cmd_resolve_ref() function has always ignored errno on failure,\nbut let's do so explicitly when using the refs_resolve_ref_unsafe()\nfunction.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/helper/test-ref-store.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex b314b81a45b..2f91fb9b227 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -123,9 +123,10 @@ static int cmd_resolve_ref(struct ref_store *refs, const char **argv)\n \tint resolve_flags = arg_flags(*argv++, \"resolve-flags\");\n \tint flags;\n \tconst char *ref;\n+\tint ignore_errno;\n \n-\tref = refs_resolve_ref_unsafe(refs, refname, resolve_flags,\n-\t\t\t\t &oid, &flags);\n+\tref = refs_werrres_ref_unsafe(refs, refname, resolve_flags,\n+\t\t\t\t &oid, &flags, &ignore_errno);\n \tprintf(\"%s %s 0x%x\\n\", oid_to_hex(&oid), ref ? ref : \"(null)\", flags);\n \treturn ref ? 0 : 1;\n }\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438939","messageId":"patch-v2-17.21-cae56498b43-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 17/21] refs API: make resolve_ref_unsafe() not set errno","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:23Z","receivedAt":"2021-10-16T09:40:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the resolve_ref_unsafe() wrapper function to use the underlying\nrefs_werrres_ref_unsafe() directly.\n\nFrom a reading of the callers I determined that the only one who cared\nabout errno was a sequencer.c caller added in e47c6cafcb5 (commit:\nmove print_commit_summary() to libgit, 2017-11-24), I'm migrating it\nto using refs_werrres_ref_unsafe() directly.\n\nThis adds another \"set errno\" instance, but in this case it's OK and\nidiomatic. We are setting it just before calling die_errno(). We could\nhave some hypothetical die_errno_var(&saved_errno, ...) here, but I\ndon't think it's worth it. The problem with errno is subtle action at\ndistance, not this sort of thing. We already use this pattern in a\ncouple of places in wrapper.c\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 6 ++++--\n sequencer.c | 10 ++++++++--\n 2 files changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8d5a76fbf29..b563729782b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1785,8 +1785,10 @@ int refs_init_db(struct strbuf *err)\n const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n \t\t\t struct object_id *oid, int *flags)\n {\n-\treturn refs_resolve_ref_unsafe(get_main_ref_store(the_repository), refname,\n-\t\t\t\t resolve_flags, oid, flags);\n+\tint ignore_errno;\n+\n+\treturn refs_werrres_ref_unsafe(get_main_ref_store(the_repository), refname,\n+\t\t\t\t resolve_flags, oid, flags, &ignore_errno);\n }\n \n int resolve_gitlink_ref(const char *submodule, const char *refname,\ndiff --git a/sequencer.c b/sequencer.c\nindex fac0b5162f6..1223dc2d2bf 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1282,6 +1282,8 @@ void print_commit_summary(struct repository *r,\n \tstruct pretty_print_context pctx = {0};\n \tstruct strbuf author_ident = STRBUF_INIT;\n \tstruct strbuf committer_ident = STRBUF_INIT;\n+\tstruct ref_store *refs;\n+\tint resolve_errno;\n \n \tcommit = lookup_commit(r, oid);\n \tif (!commit)\n@@ -1331,9 +1333,13 @@ void print_commit_summary(struct repository *r,\n \trev.diffopt.break_opt = 0;\n \tdiff_setup_done(&rev.diffopt);\n \n-\thead = resolve_ref_unsafe(\"HEAD\", 0, NULL, NULL);\n-\tif (!head)\n+\trefs = get_main_ref_store(the_repository);\n+\thead = refs_werrres_ref_unsafe(refs, \"HEAD\", 0, NULL, NULL,\n+\t\t\t\t &resolve_errno);\n+\tif (!head) {\n+\t\terrno = resolve_errno;\n \t\tdie_errno(_(\"unable to resolve HEAD after creating commit\"));\n+\t}\n \tif (!strcmp(head, \"HEAD\"))\n \t\thead = _(\"detached HEAD\");\n \telse\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438940","messageId":"patch-v2-13.21-38f5f54938f-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 13/21] refs API: ignore errno in worktree.c's find_shared_symref()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:19Z","receivedAt":"2021-10-16T09:41:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"There are only handful of callers of find_shared_symref(), none of\nwhom care about errno, so let's migrate to the non-errno-propagating\nversion of refs_resolve_ref_unsafe() and explicitly ignore errno here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n worktree.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex fa988ee978f..7d7cf058154 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -420,6 +420,7 @@ const struct worktree *find_shared_symref(const char *symref,\n \t\tconst char *symref_target;\n \t\tstruct ref_store *refs;\n \t\tint flags;\n+\t\tint ignore_errno;\n \n \t\tif (wt->is_bare)\n \t\t\tcontinue;\n@@ -436,8 +437,9 @@ const struct worktree *find_shared_symref(const char *symref,\n \t\t}\n \n \t\trefs = get_worktree_ref_store(wt);\n-\t\tsymref_target = refs_resolve_ref_unsafe(refs, symref, 0,\n-\t\t\t\t\t\t\tNULL, &flags);\n+\t\tsymref_target = refs_werrres_ref_unsafe(refs, symref, 0,\n+\t\t\t\t\t\t\tNULL, &flags,\n+\t\t\t\t\t\t\t&ignore_errno);\n \t\tif ((flags & REF_ISSYMREF) &&\n \t\t symref_target && !strcmp(symref_target, target)) {\n \t\t\texisting = wt;\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438941","messageId":"patch-v2-16.21-c4e87181121-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 16/21] refs API: make refs_ref_exists() not set errno","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:22Z","receivedAt":"2021-10-16T09:41:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move refs_ref_exists from the legacy refs_resolve_ref_unsafe() to the\nnew refs_werrres_ref_unsafe(). I have read its callers and determined\nthat they don't care about errno being set, in particular:\n\n git grep -W -w -e refs_ref_exists -e ref_exists\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 09452b5e413..8d5a76fbf29 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -309,7 +309,9 @@ int read_ref(const char *refname, struct object_id *oid)\n \n int refs_ref_exists(struct ref_store *refs, const char *refname)\n {\n-\treturn !!refs_resolve_ref_unsafe(refs, refname, RESOLVE_REF_READING, NULL, NULL);\n+\tint ignore_errno;\n+\treturn !!refs_werrres_ref_unsafe(refs, refname, RESOLVE_REF_READING,\n+\t\t\t\t\t NULL, NULL, &ignore_errno);\n }\n \n int ref_exists(const char *refname)\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438942","messageId":"patch-v2-15.21-152a831b8be-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 15/21] refs API: make refs_resolve_refdup() not set errno","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:21Z","receivedAt":"2021-10-16T09:41:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move refs_resolve_refdup() from the legacy refs_resolve_ref_unsafe()\nto the new refs_werrres_ref_unsafe(). I have read its callers and\ndetermined that they don't care about errno being set.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex dc6ed561492..09452b5e413 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -268,9 +268,10 @@ char *refs_resolve_refdup(struct ref_store *refs,\n \t\t\t struct object_id *oid, int *flags)\n {\n \tconst char *result;\n+\tint ignore_errno;\n \n-\tresult = refs_resolve_ref_unsafe(refs, refname, resolve_flags,\n-\t\t\t\t\t oid, flags);\n+\tresult = refs_werrres_ref_unsafe(refs, refname, resolve_flags,\n+\t\t\t\t\t oid, flags, &ignore_errno);\n \treturn xstrdup_or_null(result);\n }\n \n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438943","messageId":"patch-v2-20.21-56f855d32b7-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 20/21] refs API: post-migration API renaming [1/2]","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:26Z","receivedAt":"2021-10-16T09:41:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In preceding commits all callers of refs_resolve_ref_unsafe() were\nmigrated to the transitory refs_werrres_ref_unsafe() function.\n\nAs a first step in getting rid of it let's remove the old function\nfrom the public API (it went unused in a preceding commit).\n\nWe then provide both a coccinelle rule to do the rename, and a macro\nto avoid breaking the existing callers.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n contrib/coccinelle/refs.pending.cocci | 5 +++++\n refs.c | 15 +--------------\n refs.h | 22 +++++++++-------------\n 3 files changed, 15 insertions(+), 27 deletions(-)\n create mode 100644 contrib/coccinelle/refs.pending.cocci\n\ndiff --git a/contrib/coccinelle/refs.pending.cocci b/contrib/coccinelle/refs.pending.cocci\nnew file mode 100644\nindex 00000000000..b33cb8a12aa\n--- /dev/null\n+++ b/contrib/coccinelle/refs.pending.cocci\n@@ -0,0 +1,5 @@\n+@@\n+expression refs, refname, resolve_flags, oid, flags, failure_errno;\n+@@\n+- refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)\n++ refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)\ndiff --git a/refs.c b/refs.c\nindex e90c59539b4..4e06745c97a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1669,7 +1669,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t\t\t\t\t type, failure_errno);\n }\n \n-const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n+const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t\t\t const char *refname,\n \t\t\t\t int resolve_flags,\n \t\t\t\t struct object_id *oid,\n@@ -1766,19 +1766,6 @@ const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n \treturn NULL;\n }\n \n-const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname,\n-\t\t\t\t int resolve_flags, struct object_id *oid,\n-\t\t\t\t int *flags)\n-{\n-\tint failure_errno = 0;\n-\tconst char *refn;\n-\trefn = refs_werrres_ref_unsafe(refs, refname, resolve_flags,\n-\t\t\t\t oid, flags, &failure_errno);\n-\tif (!refn)\n-\t\terrno = failure_errno;\n-\treturn refn;\n-}\n-\n /* backend functions */\n int refs_init_db(struct strbuf *err)\n {\ndiff --git a/refs.h b/refs.h\nindex 3938f99c902..d908a161c06 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -11,18 +11,6 @@ struct string_list;\n struct string_list_item;\n struct worktree;\n \n-/*\n- * Callers should not inspect \"errno\" on failure, but rather pass in a\n- * \"failure_errno\" parameter, on failure the \"errno\" will indicate the\n- * type of failure encountered, but not necessarily one that came from\n- * a syscall. We might have faked it up.\n- */\n-const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n-\t\t\t\t const char *refname,\n-\t\t\t\t int resolve_flags,\n-\t\t\t\t struct object_id *oid,\n-\t\t\t\t int *flags, int *failure_errno);\n-\n /*\n * Resolve a reference, recursively following symbolic refererences.\n *\n@@ -70,16 +58,24 @@ const char *refs_werrres_ref_unsafe(struct ref_store *refs,\n * resolved. The function returns NULL for such ref names.\n * Caps and underscores refers to the special refs, such as HEAD,\n * FETCH_HEAD and friends, that all live outside of the refs/ directory.\n+ *\n+ * Callers should not inspect \"errno\" on failure, but rather pass in a\n+ * \"failure_errno\" parameter, on failure the \"errno\" will indicate the\n+ * type of failure encountered, but not necessarily one that came from\n+ * a syscall. We might have faked it up.\n */\n #define RESOLVE_REF_READING 0x01\n #define RESOLVE_REF_NO_RECURSE 0x02\n #define RESOLVE_REF_ALLOW_BAD_NAME 0x04\n \n+#define refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno) \\\n+\trefs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)\n const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t\t\t const char *refname,\n \t\t\t\t int resolve_flags,\n \t\t\t\t struct object_id *oid,\n-\t\t\t\t int *flags);\n+\t\t\t\t int *flags, int *failure_errno);\n+\n const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n \t\t\t struct object_id *oid, int *flags);\n \n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438944","messageId":"patch-v2-18.21-146b030a142-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 18/21] refs API: make expand_ref() & repo_dwim_log() not set errno","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:24Z","receivedAt":"2021-10-16T09:41:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The use of these two is rather trivial, and it's easy to see none of\ntheir callers care about errno. So let's move them from\nrefs_resolve_ref_unsafe() to refs_resolve_ref_unsafe_with_errno(),\nthese were the last two callers, so we can get rid of that wrapper\nfunction.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex b563729782b..43fe9e6d89d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -654,13 +654,16 @@ int expand_ref(struct repository *repo, const char *str, int len,\n \t\tstruct object_id oid_from_ref;\n \t\tstruct object_id *this_result;\n \t\tint flag;\n+\t\tstruct ref_store *refs = get_main_ref_store(repo);\n+\t\tint ignore_errno;\n \n \t\tthis_result = refs_found ? &oid_from_ref : oid;\n \t\tstrbuf_reset(&fullref);\n \t\tstrbuf_addf(&fullref, *p, len, str);\n-\t\tr = refs_resolve_ref_unsafe(get_main_ref_store(repo),\n-\t\t\t\t\t fullref.buf, RESOLVE_REF_READING,\n-\t\t\t\t\t this_result, &flag);\n+\t\tr = refs_werrres_ref_unsafe(refs, fullref.buf,\n+\t\t\t\t\t RESOLVE_REF_READING,\n+\t\t\t\t\t this_result, &flag,\n+\t\t\t\t\t &ignore_errno);\n \t\tif (r) {\n \t\t\tif (!refs_found++)\n \t\t\t\t*ref = xstrdup(r);\n@@ -689,12 +692,14 @@ int repo_dwim_log(struct repository *r, const char *str, int len,\n \tfor (p = ref_rev_parse_rules; *p; p++) {\n \t\tstruct object_id hash;\n \t\tconst char *ref, *it;\n+\t\tint ignore_errno;\n \n \t\tstrbuf_reset(&path);\n \t\tstrbuf_addf(&path, *p, len, str);\n-\t\tref = refs_resolve_ref_unsafe(refs, path.buf,\n+\t\tref = refs_werrres_ref_unsafe(refs, path.buf,\n \t\t\t\t\t RESOLVE_REF_READING,\n-\t\t\t\t\t oid ? &hash : NULL, NULL);\n+\t\t\t\t\t oid ? &hash : NULL, NULL,\n+\t\t\t\t\t &ignore_errno);\n \t\tif (!ref)\n \t\t\tcontinue;\n \t\tif (refs_reflog_exists(refs, path.buf))\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438945","messageId":"patch-v2-19.21-3d630872aff-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 19/21] refs API: don't expose \"errno\" in run_transaction_hook()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:25Z","receivedAt":"2021-10-16T09:41:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In run_transaction_hook() we've checked errno since 67541597670 (refs:\nimplement reference transaction hook, 2020-06-19), let's reset errno\nafterwards to make sure nobody using refs.c directly or indirectly\nrelies on it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n refs.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex 43fe9e6d89d..e90c59539b4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2096,8 +2096,11 @@ static int run_transaction_hook(struct ref_transaction *transaction,\n \t\t\t update->refname);\n \n \t\tif (write_in_full(proc.in, buf.buf, buf.len) < 0) {\n-\t\t\tif (errno != EPIPE)\n+\t\t\tif (errno != EPIPE) {\n+\t\t\t\t/* Don't leak errno outside this API */\n+\t\t\t\terrno = 0;\n \t\t\t\tret = -1;\n+\t\t\t}\n \t\t\tbreak;\n \t\t}\n \t}\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"438946","messageId":"patch-v2-21.21-0135fb5949f-20211016T093845Z-avarab@gmail.com","threadId":"56705","inReplyTo":"cover-v2-00.21-00000000000-20211016T093845Z-avarab@gmail.com","subject":"[PATCH v2 21/21] refs API: post-migration API renaming [2/2]","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T09:39:27Z","receivedAt":"2021-10-16T09:41:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Rename the transitory refs_werrres_ref_unsafe() function to\nrefs_resolve_ref_unsafe(), now that all callers of the old function\nhave learned to pass in a \"failure_errno\" parameter.\n\nThe coccinelle semantic patch added in the preceding commit works, but\nI couldn't figure out how to get spatch(1) to re-flow these argument\nlists (and sometimes make lines way too long), so this rename was done\nwith:\n\n perl -pi -e 's/refs_werrres_ref_unsafe/refs_resolve_ref_unsafe/g' \\\n $(git grep -l refs_werrres_ref_unsafe -- '*.c')\n\nBut after that \"make contrib/coccinelle/refs.cocci.patch\" comes up\nempty, so the result would have been the same. Let's remove that\ntransitory semantic patch file, we won't need to retain it for any\nother in-flight changes, refs_werrres_ref_unsafe() only existed within\nthis patch series.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n contrib/coccinelle/refs.pending.cocci | 5 -----\n refs.c | 16 ++++++++--------\n refs.h | 2 --\n refs/files-backend.c | 18 +++++++++---------\n sequencer.c | 2 +-\n t/helper/test-ref-store.c | 2 +-\n worktree.c | 6 +++---\n 7 files changed, 22 insertions(+), 29 deletions(-)\n delete mode 100644 contrib/coccinelle/refs.pending.cocci\n\ndiff --git a/contrib/coccinelle/refs.pending.cocci b/contrib/coccinelle/refs.pending.cocci\ndeleted file mode 100644\nindex b33cb8a12aa..00000000000\n--- a/contrib/coccinelle/refs.pending.cocci\n+++ /dev/null\n@@ -1,5 +0,0 @@\n-@@\n-expression refs, refname, resolve_flags, oid, flags, failure_errno;\n-@@\n-- refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)\n-+ refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)\ndiff --git a/refs.c b/refs.c\nindex 4e06745c97a..cd9b8bb1141 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -270,7 +270,7 @@ char *refs_resolve_refdup(struct ref_store *refs,\n \tconst char *result;\n \tint ignore_errno;\n \n-\tresult = refs_werrres_ref_unsafe(refs, refname, resolve_flags,\n+\tresult = refs_resolve_ref_unsafe(refs, refname, resolve_flags,\n \t\t\t\t\t oid, flags, &ignore_errno);\n \treturn xstrdup_or_null(result);\n }\n@@ -296,7 +296,7 @@ int read_ref_full(const char *refname, int resolve_flags, struct object_id *oid,\n \tint ignore_errno;\n \tstruct ref_store *refs = get_main_ref_store(the_repository);\n \n-\tif (refs_werrres_ref_unsafe(refs, refname, resolve_flags,\n+\tif (refs_resolve_ref_unsafe(refs, refname, resolve_flags,\n \t\t\t\t oid, flags, &ignore_errno))\n \t\treturn 0;\n \treturn -1;\n@@ -310,7 +310,7 @@ int read_ref(const char *refname, struct object_id *oid)\n int refs_ref_exists(struct ref_store *refs, const char *refname)\n {\n \tint ignore_errno;\n-\treturn !!refs_werrres_ref_unsafe(refs, refname, RESOLVE_REF_READING,\n+\treturn !!refs_resolve_ref_unsafe(refs, refname, RESOLVE_REF_READING,\n \t\t\t\t\t NULL, NULL, &ignore_errno);\n }\n \n@@ -660,7 +660,7 @@ int expand_ref(struct repository *repo, const char *str, int len,\n \t\tthis_result = refs_found ? &oid_from_ref : oid;\n \t\tstrbuf_reset(&fullref);\n \t\tstrbuf_addf(&fullref, *p, len, str);\n-\t\tr = refs_werrres_ref_unsafe(refs, fullref.buf,\n+\t\tr = refs_resolve_ref_unsafe(refs, fullref.buf,\n \t\t\t\t\t RESOLVE_REF_READING,\n \t\t\t\t\t this_result, &flag,\n \t\t\t\t\t &ignore_errno);\n@@ -696,7 +696,7 @@ int repo_dwim_log(struct repository *r, const char *str, int len,\n \n \t\tstrbuf_reset(&path);\n \t\tstrbuf_addf(&path, *p, len, str);\n-\t\tref = refs_werrres_ref_unsafe(refs, path.buf,\n+\t\tref = refs_resolve_ref_unsafe(refs, path.buf,\n \t\t\t\t\t RESOLVE_REF_READING,\n \t\t\t\t\t oid ? &hash : NULL, NULL,\n \t\t\t\t\t &ignore_errno);\n@@ -1383,7 +1383,7 @@ int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n \tint flag;\n \tint ignore_errno;\n \n-\tif (refs_werrres_ref_unsafe(refs, \"HEAD\", RESOLVE_REF_READING,\n+\tif (refs_resolve_ref_unsafe(refs, \"HEAD\", RESOLVE_REF_READING,\n \t\t\t\t &oid, &flag, &ignore_errno))\n \t\treturn fn(\"HEAD\", &oid, flag, cb_data);\n \n@@ -1779,7 +1779,7 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,\n {\n \tint ignore_errno;\n \n-\treturn refs_werrres_ref_unsafe(get_main_ref_store(the_repository), refname,\n+\treturn refs_resolve_ref_unsafe(get_main_ref_store(the_repository), refname,\n \t\t\t\t resolve_flags, oid, flags, &ignore_errno);\n }\n \n@@ -1795,7 +1795,7 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,\n \tif (!refs)\n \t\treturn -1;\n \n-\tif (!refs_werrres_ref_unsafe(refs, refname, 0, oid, &flags,\n+\tif (!refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags,\n \t\t\t\t &ignore_errno) || is_null_oid(oid))\n \t\treturn -1;\n \treturn 0;\ndiff --git a/refs.h b/refs.h\nindex d908a161c06..45c34e99e3a 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -68,8 +68,6 @@ struct worktree;\n #define RESOLVE_REF_NO_RECURSE 0x02\n #define RESOLVE_REF_ALLOW_BAD_NAME 0x04\n \n-#define refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno) \\\n-\trefs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)\n const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t\t\t const char *refname,\n \t\t\t\t int resolve_flags,\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 6c854dda533..abb6091fcf5 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -281,7 +281,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,\n \t\t\t\t\t\t\t refname.len));\n \t\t} else {\n \t\t\tint ignore_errno;\n-\t\t\tif (!refs_werrres_ref_unsafe(&refs->base,\n+\t\t\tif (!refs_resolve_ref_unsafe(&refs->base,\n \t\t\t\t\t\t refname.buf,\n \t\t\t\t\t\t RESOLVE_REF_READING,\n \t\t\t\t\t\t &oid, &flag, &ignore_errno)) {\n@@ -1034,7 +1034,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,\n \t\tgoto error_return;\n \t}\n \n-\tif (!refs_werrres_ref_unsafe(&refs->base, lock->ref_name, 0,\n+\tif (!refs_resolve_ref_unsafe(&refs->base, lock->ref_name, 0,\n \t\t\t\t &lock->old_oid, NULL, &ignore_errno))\n \t\toidclr(&lock->old_oid);\n \tgoto out;\n@@ -1410,7 +1410,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,\n \t\tgoto out;\n \t}\n \n-\tif (!refs_werrres_ref_unsafe(&refs->base, oldrefname,\n+\tif (!refs_resolve_ref_unsafe(&refs->base, oldrefname,\n \t\t\t\t RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n \t\t\t\t &orig_oid, &flag, &ignore_errno)) {\n \t\tret = error(\"refname %s not found\", oldrefname);\n@@ -1456,7 +1456,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,\n \t * the safety anyway; we want to delete the reference whatever\n \t * its current value.\n \t */\n-\tif (!copy && refs_werrres_ref_unsafe(&refs->base, newrefname,\n+\tif (!copy && refs_resolve_ref_unsafe(&refs->base, newrefname,\n \t\t\t\t\t RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n \t\t\t\t\t NULL, NULL, &ignore_errno) &&\n \t refs_delete_ref(&refs->base, NULL, newrefname,\n@@ -1825,7 +1825,7 @@ static int commit_ref_update(struct files_ref_store *refs,\n \t\tconst char *head_ref;\n \t\tint ignore_errno;\n \n-\t\thead_ref = refs_werrres_ref_unsafe(&refs->base, \"HEAD\",\n+\t\thead_ref = refs_resolve_ref_unsafe(&refs->base, \"HEAD\",\n \t\t\t\t\t\t RESOLVE_REF_READING,\n \t\t\t\t\t\t NULL, &head_flag,\n \t\t\t\t\t\t &ignore_errno);\n@@ -1875,7 +1875,7 @@ static void update_symref_reflog(struct files_ref_store *refs,\n \tint ignore_errno;\n \n \tif (logmsg &&\n-\t refs_werrres_ref_unsafe(&refs->base, target,\n+\t refs_resolve_ref_unsafe(&refs->base, target,\n \t\t\t\t RESOLVE_REF_READING, &new_oid, NULL,\n \t\t\t\t &ignore_errno) &&\n \t files_log_ref_write(refs, refname, &lock->old_oid,\n@@ -2163,7 +2163,7 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\tif (ends_with(diter->basename, \".lock\"))\n \t\t\tcontinue;\n \n-\t\tif (!refs_werrres_ref_unsafe(iter->ref_store,\n+\t\tif (!refs_resolve_ref_unsafe(iter->ref_store,\n \t\t\t\t\t diter->relative_path, 0,\n \t\t\t\t\t &iter->oid, &flags,\n \t\t\t\t\t &ignore_errno)) {\n@@ -2511,7 +2511,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t * to record and possibly check old_oid:\n \t\t\t */\n \t\t\tint ignore_errno;\n-\t\t\tif (!refs_werrres_ref_unsafe(&refs->base,\n+\t\t\tif (!refs_resolve_ref_unsafe(&refs->base,\n \t\t\t\t\t\t referent.buf, 0,\n \t\t\t\t\t\t &lock->old_oid, NULL,\n \t\t\t\t\t\t &ignore_errno)) {\n@@ -3209,7 +3209,7 @@ static int files_reflog_expire(struct ref_store *ref_store,\n \t\t\tint type;\n \t\t\tconst char *ref;\n \n-\t\t\tref = refs_werrres_ref_unsafe(&refs->base, refname,\n+\t\t\tref = refs_resolve_ref_unsafe(&refs->base, refname,\n \t\t\t\t\t\t RESOLVE_REF_NO_RECURSE,\n \t\t\t\t\t\t NULL, &type,\n \t\t\t\t\t\t &ignore_errno);\ndiff --git a/sequencer.c b/sequencer.c\nindex 1223dc2d2bf..cf10f9a332e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1334,7 +1334,7 @@ void print_commit_summary(struct repository *r,\n \tdiff_setup_done(&rev.diffopt);\n \n \trefs = get_main_ref_store(the_repository);\n-\thead = refs_werrres_ref_unsafe(refs, \"HEAD\", 0, NULL, NULL,\n+\thead = refs_resolve_ref_unsafe(refs, \"HEAD\", 0, NULL, NULL,\n \t\t\t\t &resolve_errno);\n \tif (!head) {\n \t\terrno = resolve_errno;\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 2f91fb9b227..3986665037a 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -125,7 +125,7 @@ static int cmd_resolve_ref(struct ref_store *refs, const char **argv)\n \tconst char *ref;\n \tint ignore_errno;\n \n-\tref = refs_werrres_ref_unsafe(refs, refname, resolve_flags,\n+\tref = refs_resolve_ref_unsafe(refs, refname, resolve_flags,\n \t\t\t\t &oid, &flags, &ignore_errno);\n \tprintf(\"%s %s 0x%x\\n\", oid_to_hex(&oid), ref ? ref : \"(null)\", flags);\n \treturn ref ? 0 : 1;\ndiff --git a/worktree.c b/worktree.c\nindex 7d7cf058154..2c155b10150 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -30,7 +30,7 @@ static void add_head_info(struct worktree *wt)\n \tconst char *target;\n \tint ignore_errno;\n \n-\ttarget = refs_werrres_ref_unsafe(get_worktree_ref_store(wt),\n+\ttarget = refs_resolve_ref_unsafe(get_worktree_ref_store(wt),\n \t\t\t\t\t \"HEAD\",\n \t\t\t\t\t 0,\n \t\t\t\t\t &wt->head_oid, &flags,\n@@ -437,7 +437,7 @@ const struct worktree *find_shared_symref(const char *symref,\n \t\t}\n \n \t\trefs = get_worktree_ref_store(wt);\n-\t\tsymref_target = refs_werrres_ref_unsafe(refs, symref, 0,\n+\t\tsymref_target = refs_resolve_ref_unsafe(refs, symref, 0,\n \t\t\t\t\t\t\tNULL, &flags,\n \t\t\t\t\t\t\t&ignore_errno);\n \t\tif ((flags & REF_ISSYMREF) &&\n@@ -574,7 +574,7 @@ int other_head_refs(each_ref_fn fn, void *cb_data)\n \n \t\tstrbuf_reset(&refname);\n \t\tstrbuf_worktree_ref(wt, &refname, \"HEAD\");\n-\t\tif (refs_werrres_ref_unsafe(get_main_ref_store(the_repository),\n+\t\tif (refs_resolve_ref_unsafe(get_main_ref_store(the_repository),\n \t\t\t\t\t refname.buf,\n \t\t\t\t\t RESOLVE_REF_READING,\n \t\t\t\t\t &oid, &flag, &ignore_errno))\n-- \n2.33.1.1338.g20da966911a\n\n"}]}