* [PATCH 00/20] refs: stop having the API set "errno"
@ 2021-10-14 0:06 Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 01/20] branch tests: test for errno propagating on failing read Ævar Arnfjörð Bjarmason
` (20 more replies)
0 siblings, 21 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
This series changes the refs API to stop explicitly or implicitly
exposing "errno" to indicate its state, and instead moves to pushing
up an explicit "failure_errno" variable, or in other cases we migrate
callers entirely away from getting any errno values at all (because
they never cared).
This makes the eventual integration of the reftable backend much
easier to reason about, because we can be assured that the refs.c
function interface is everything we need to care about, and that some
callers e.g. don't expect the reftable backend to fake up certain
errno values in case of certain types of failures.
This topic replaces ab/refs-errno-cleanup, which was the result of
Junio peeling off the first 4 patches of this 10-part series:
https://lore.kernel.org/git/cover-v10-0.8-00000000000-20210823T114712Z-avarab@gmail.com/
It's been marked as "meh" for a while in What's Cooking, I think just
doing those first patches (or those 10) doesn't really show the
end-state benefit, so here's all of it.
In re-rolling this I found more cases where we could remove dead code
in the refs file backend, which made this easier, see in particular
06/20. Patches 08-18/20 are piecemeal moves away from setting "errno"
(which in most cases wasn't needed), and finally 19-20/20 removes a
transitory API function used in this series so that this migration
could be done incrementally.
Han-Wen Nienhuys (3):
branch tests: test for errno propagating on failing read
refs API: make refs_read_raw_ref() not set errno
refs API: make parse_loose_ref_contents() not set errno
Ævar Arnfjörð Bjarmason (17):
refs API: add a version of refs_resolve_ref_unsafe() with "errno"
refs API: make refs_rename_ref_available() static
refs/files: remove "name exist?" check in lock_ref_oid_basic()
refs API: remove refs_read_ref_full() wrapper
refs API: make resolve_gitlink_ref() not set errno
refs API: make loose_fill_ref_dir() not set errno
refs API: make files_copy_or_rename_ref() et al not set errno
refs API: ignore errno in worktree.c's add_head_info()
refs API: ignore errno in worktree.c's find_shared_symref()
refs tests: ignore ignore errno in test-ref-store helper
refs API: make refs_resolve_refdup() not set errno
refs API: make refs_ref_exists() not set errno
refs API: make resolve_ref_unsafe() not set errno
refs API: make expand_ref() & repo_dwim_log() not set errno
refs API: don't expose "errno" in run_transaction_hook()
refs API: post-migration API renaming [1/2]
refs API: post-migration API renaming [2/2]
refs.c | 124 +++++++++++++++++++------------------
refs.h | 10 ++-
refs/files-backend.c | 125 +++++++++++++++++++++++++-------------
refs/packed-backend.c | 7 ++-
refs/refs-internal.h | 26 +++-----
sequencer.c | 10 ++-
t/helper/test-ref-store.c | 3 +-
t/t3200-branch.sh | 21 +++++++
worktree.c | 17 ++++--
9 files changed, 209 insertions(+), 134 deletions(-)
Range-diff:
6: 6dae8b643ad = 1: bea88e382c0 branch tests: test for errno propagating on failing read
4: 05f3a346e3b ! 2: 46641111885 refs: make errno output explicit for read_raw_ref_fn
@@
## Metadata ##
-Author: Han-Wen Nienhuys <hanwen@google.com>
+Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## Commit message ##
- refs: make errno output explicit for read_raw_ref_fn
+ refs API: add a version of refs_resolve_ref_unsafe() with "errno"
- This makes it explicit how alternative ref backends should report errors in
- read_raw_ref_fn.
+ Add a new refs_werrres_ref_unsafe() function, which is like
+ refs_resolve_ref_unsafe() except that it explicitly saves away the
+ "errno" to a passed-in parameter, the refs_resolve_ref_unsafe() then
+ becomes a wrapper for it.
- read_raw_ref_fn needs to supply a credible errno for a number of cases. These
- are primarily:
+ In subsequent commits we'll migrate code over to it, before finally
+ making "refs_resolve_ref_unsafe()" with an "errno" parameter the
+ canonical version, so this this function exists only so that we can
+ incrementally migrate callers, it will be going away in a subsequent
+ commit.
- 1) The files backend calls read_raw_ref from lock_raw_ref, and uses the
- resulting error codes to create/remove directories as needed.
+ As the added comment notes has a rather tortured name to be the same
+ length as "refs_resolve_ref_unsafe", to avoid churn as we won't need
+ to re-indent the argument lists, similarly the documentation and
+ structure of it in refs.h is designed to minimize a diff in a
+ subsequent commit, where that documentation will be added to the new
+ refs_resolve_ref_unsafe().
- 2) ENOENT should be translated in a zero OID, optionally with REF_ISBROKEN set,
- returning the last successfully resolved symref. This is necessary so
- read_raw_ref("HEAD") on an empty repo returns refs/heads/main (or the default branch
- du-jour), and we know on which branch to create the first commit.
+ At the end of this migration the "meaningful errno" TODO item left in
+ 76d70dc0c63 (refs.c: make resolve_ref_unsafe set errno to something
+ meaningful on error, 2014-06-20) will be resolved.
- Make this information flow explicit by adding a failure_errno to the signature
- of read_raw_ref. All errnos from the files backend are still propagated
- unchanged, even though inspection suggests only ENOTDIR, EISDIR and ENOENT are
- relevant.
+ As can be seen from the use of refs_read_raw_ref() we'll also need to
+ convert some functions that the new refs_werrres_ref_unsafe() itself
+ calls to take this "failure_errno". That will be done in subsequent
+ commits.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## refs.c ##
@@ refs.c: int refs_read_raw_ref(struct ref_store *ref_store,
- }
-
- return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
-- type);
-+ type, &errno);
- }
-
- /* This function needs to return a meaningful errno on failure */
-
- ## refs/debug.c ##
-@@ refs/debug.c: debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,
-
- static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname,
- struct object_id *oid, struct strbuf *referent,
-- unsigned int *type)
-+ unsigned int *type, int *failure_errno)
- {
- struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
- int res = 0;
-
- oidcpy(oid, null_oid());
-- errno = 0;
- res = drefs->refs->be->read_raw_ref(drefs->refs, refname, oid, referent,
-- type);
-+ type, failure_errno);
-
- if (res == 0) {
- trace_printf_key(&trace_refs, "read_raw_ref: %s: %s (=> %s) type %x: %d\n",
-@@ refs/debug.c: static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname,
- } else {
- trace_printf_key(&trace_refs,
- "read_raw_ref: %s: %d (errno %d)\n", refname,
-- res, errno);
-+ res, *failure_errno);
- }
- return res;
- }
-
- ## refs/files-backend.c ##
-@@ refs/files-backend.c: static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
- return refs->loose;
+ type, &errno);
}
--static int files_read_raw_ref(struct ref_store *ref_store,
-- const char *refname, struct object_id *oid,
-- struct strbuf *referent, unsigned int *type)
-+static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
-+ struct object_id *oid, struct strbuf *referent,
-+ unsigned int *type, int *failure_errno)
+-/* This function needs to return a meaningful errno on failure */
+-const char *refs_resolve_ref_unsafe(struct ref_store *refs,
++const char *refs_werrres_ref_unsafe(struct ref_store *refs,
+ const char *refname,
+ int resolve_flags,
+- struct object_id *oid, int *flags)
++ struct object_id *oid,
++ int *flags, int *failure_errno)
{
- struct files_ref_store *refs =
- files_downcast(ref_store, REF_STORE_READ, "read_raw_ref");
-@@ refs/files-backend.c: static int files_read_raw_ref(struct ref_store *ref_store,
- struct stat st;
- int fd;
- int ret = -1;
-- int save_errno;
- int remaining_retries = 3;
-
- *type = 0;
-@@ refs/files-backend.c: static int files_read_raw_ref(struct ref_store *ref_store,
- ret = parse_loose_ref_contents(buf, oid, referent, type);
-
- out:
-- save_errno = errno;
-+ *failure_errno = errno;
- strbuf_release(&sb_path);
- strbuf_release(&sb_contents);
-- errno = save_errno;
- return ret;
- }
+ static struct strbuf sb_refname = STRBUF_INIT;
+ struct object_id unused_oid;
+ int unused_flags;
+ int symref_count;
+
++ assert(failure_errno);
++
+ if (!oid)
+ oid = &unused_oid;
+ if (!flags)
+@@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs,
+ if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+ if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
+ !refname_is_safe(refname)) {
+- errno = EINVAL;
++ *failure_errno = EINVAL;
+ return NULL;
+ }
-@@ refs/files-backend.c: static int lock_raw_ref(struct files_ref_store *refs,
- struct strbuf ref_file = STRBUF_INIT;
- int attempts_remaining = 3;
- int ret = TRANSACTION_GENERIC_ERROR;
-+ int failure_errno;
+@@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs,
+ for (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) {
+ unsigned int read_flags = 0;
- assert(err);
- files_assert_main_repository(refs, "lock_raw_ref");
-@@ refs/files-backend.c: static int lock_raw_ref(struct files_ref_store *refs,
- if (hold_lock_file_for_update_timeout(
- &lock->lk, ref_file.buf, LOCK_NO_DEREF,
- get_files_ref_lock_timeout_ms()) < 0) {
-- if (errno == ENOENT && --attempts_remaining > 0) {
-+ int myerr = errno;
+ errno = 0;
-+ if (myerr == ENOENT && --attempts_remaining > 0) {
- /*
- * Maybe somebody just deleted one of the
- * directories leading to ref_file. Try
-@@ refs/files-backend.c: static int lock_raw_ref(struct files_ref_store *refs,
+ if (refs_read_raw_ref(refs, refname,
+ oid, &sb_refname, &read_flags)) {
+ *flags |= read_flags;
++ if (errno)
++ *failure_errno = errno;
+
+ /* In reading mode, refs must eventually resolve */
+ if (resolve_flags & RESOLVE_REF_READING)
+@@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs,
+ * may show errors besides ENOENT if there are
+ * similarly-named refs.
*/
- goto retry;
- } else {
-- unable_to_lock_message(ref_file.buf, errno, err);
-+ unable_to_lock_message(ref_file.buf, myerr, err);
- goto error_return;
- }
- }
-@@ refs/files-backend.c: static int lock_raw_ref(struct files_ref_store *refs,
- * fear that its value will change.
- */
-
-- if (files_read_raw_ref(&refs->base, refname,
-- &lock->old_oid, referent, type)) {
-- if (errno == ENOENT) {
-+ if (files_read_raw_ref(&refs->base, refname, &lock->old_oid, referent,
-+ type, &failure_errno)) {
-+ if (failure_errno == ENOENT) {
- if (mustexist) {
- /* Garden variety missing reference. */
- strbuf_addf(err, "unable to resolve reference '%s'",
-@@ refs/files-backend.c: static int lock_raw_ref(struct files_ref_store *refs,
- * reference named "refs/foo/bar/baz".
- */
- }
-- } else if (errno == EISDIR) {
-+ } else if (failure_errno == EISDIR) {
- /*
- * There is a directory in the way. It might have
- * contained references that have been deleted. If
-@@ refs/files-backend.c: static int lock_raw_ref(struct files_ref_store *refs,
- goto error_return;
- }
+- if (errno != ENOENT &&
+- errno != EISDIR &&
+- errno != ENOTDIR)
++ if (*failure_errno != ENOENT &&
++ *failure_errno != EISDIR &&
++ *failure_errno != ENOTDIR)
+ return NULL;
+
+ oidclr(oid);
+@@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs,
+ if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+ if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
+ !refname_is_safe(refname)) {
+- errno = EINVAL;
++ *failure_errno = EINVAL;
+ return NULL;
}
-- } else if (errno == EINVAL && (*type & REF_ISBROKEN)) {
-+ } else if (failure_errno == EINVAL && (*type & REF_ISBROKEN)) {
- strbuf_addf(err, "unable to resolve reference '%s': "
- "reference broken", refname);
- goto error_return;
- } else {
- strbuf_addf(err, "unable to resolve reference '%s': %s",
-- refname, strerror(errno));
-+ refname, strerror(failure_errno));
- goto error_return;
+
+@@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs,
}
+ }
-
- ## refs/packed-backend.c ##
-@@ refs/packed-backend.c: static struct snapshot *get_snapshot(struct packed_ref_store *refs)
- return refs->snapshot;
+- errno = ELOOP;
++ *failure_errno = ELOOP;
+ return NULL;
}
--static int packed_read_raw_ref(struct ref_store *ref_store,
-- const char *refname, struct object_id *oid,
-- struct strbuf *referent, unsigned int *type)
-+static int packed_read_raw_ref(struct ref_store *ref_store, const char *refname,
-+ struct object_id *oid, struct strbuf *referent,
-+ unsigned int *type, int *failure_errno)
++const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname,
++ int resolve_flags, struct object_id *oid,
++ int *flags)
++{
++ int failure_errno = 0;
++ const char *refn;
++ refn = refs_werrres_ref_unsafe(refs, refname, resolve_flags,
++ oid, flags, &failure_errno);
++ if (!refn)
++ errno = failure_errno;
++ return refn;
++}
++
+ /* backend functions */
+ int refs_init_db(struct strbuf *err)
{
- struct packed_ref_store *refs =
- packed_downcast(ref_store, REF_STORE_READ, "read_raw_ref");
-@@ refs/packed-backend.c: static int packed_read_raw_ref(struct ref_store *ref_store,
-
- if (!rec) {
- /* refname is not a packed reference. */
-- errno = ENOENT;
-+ *failure_errno = ENOENT;
- return -1;
- }
-
- ## refs/refs-internal.h ##
-@@ refs/refs-internal.h: typedef int reflog_expire_fn(struct ref_store *ref_store,
- * properly-formatted or even safe reference name. NEITHER INPUT NOR
- * OUTPUT REFERENCE NAMES ARE VALIDATED WITHIN THIS FUNCTION.
- *
-- * Return 0 on success. If the ref doesn't exist, set errno to ENOENT
-- * and return -1. If the ref exists but is neither a symbolic ref nor
-- * an object ID, it is broken; set REF_ISBROKEN in type, and return -1
-- * (errno should not be ENOENT) If there is another error reading the
-- * ref, set errno appropriately and return -1.
-+ * Return 0 on success, or -1 on failure. If the ref exists but is neither a
-+ * symbolic ref nor an object ID, it is broken. In this case set REF_ISBROKEN in
-+ * type, and return -1 (failure_errno should not be ENOENT)
-+ *
-+ * failure_errno provides errno codes that are interpreted beyond error
-+ * reporting. The following error codes have special meaning:
-+ * * ENOENT: the ref doesn't exist
-+ * * EISDIR: ref name is a directory
-+ * * ENOTDIR: ref prefix is not a directory
+ ## refs.h ##
+@@ refs.h: struct string_list;
+ struct string_list_item;
+ struct worktree;
+
++/*
++ * Callers should not inspect "errno" on failure, but rather pass in a
++ * "failure_errno" parameter, on failure the "errno" will indicate the
++ * type of failure encountered, but not necessarily one that came from
++ * a syscall. We might have faked it up.
++ */
++const char *refs_werrres_ref_unsafe(struct ref_store *refs,
++ const char *refname,
++ int resolve_flags,
++ struct object_id *oid,
++ int *flags, int *failure_errno);
++
+ /*
+ * Resolve a reference, recursively following symbolic refererences.
*
- * Backend-specific flags might be set in type as well, regardless of
- * outcome.
-@@ refs/refs-internal.h: typedef int reflog_expire_fn(struct ref_store *ref_store,
- * - in all other cases, referent will be untouched, and therefore
- * refname will still be valid and unchanged.
- */
--typedef int read_raw_ref_fn(struct ref_store *ref_store,
-- const char *refname, struct object_id *oid,
-- struct strbuf *referent, unsigned int *type);
-+typedef int read_raw_ref_fn(struct ref_store *ref_store, const char *refname,
-+ struct object_id *oid, struct strbuf *referent,
-+ unsigned int *type, int *failure_errno);
-
- struct ref_storage_be {
- struct ref_storage_be *next;
5: fa9260f25fa ! 3: a1a80715ffe refs: add failure_errno to refs_read_raw_ref() signature
@@ Metadata
Author: Han-Wen Nienhuys <hanwen@google.com>
## Commit message ##
- refs: add failure_errno to refs_read_raw_ref() signature
+ refs API: make refs_read_raw_ref() not set errno
- This lets us use the explicit errno output parameter in refs_resolve_ref_unsafe.
-
- Some of our callers explicitly do not care about the errno, rather
- than understanding NULL let's have them declare that they don't care
- by passing in an "ignore_errno". There's only three of them, and using
- that pattern will make it more obvious that they want to throw away
- data, let's also add a comment to one of the callers about why we'd
- like to ignore the errno.
-
- Let's not extend that to refs_resolve_ref_unsafe() itself for now, it
- has a large set of legacy callers, so we're faking up the old "errno"
- behavior for it. We can convert those callers to
- refs_resolve_ref_unsafe_with_errno() later.
+ Add a "failure_errno" to refs_read_raw_ref(), his allows
+ refs_werrres_ref_unsafe() to pass along its "failure_errno", as a
+ first step before its own callers are migrated to pass it further up
+ the chain.
We are leaving out out the refs_read_special_head() in
- refs_read_raw_ref() for now, as noted in the next commit moving it to
- "failure_errno" will require some special consideration.
-
- We're intentionally mis-indenting the argument list of the new
- refs_resolve_ref_unsafe_with_errno(), it will be non-static in a
- subsequent commit, doing it this way makes that diff smaller.
+ refs_read_raw_ref() for now, as noted in a subsequent commit moving it
+ to "failure_errno" will require some special consideration.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
@@ refs.c: static int refs_read_special_head(struct ref_store *ref_store,
+ type, failure_errno);
}
--/* This function needs to return a meaningful errno on failure */
--const char *refs_resolve_ref_unsafe(struct ref_store *refs,
-- const char *refname,
-- int resolve_flags,
-- struct object_id *oid, int *flags)
-+static const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs,
-+ const char *refname,
-+ int resolve_flags,
-+ struct object_id *oid,
-+ int *flags, int *failure_errno)
- {
- static struct strbuf sb_refname = STRBUF_INIT;
- struct object_id unused_oid;
- int unused_flags;
- int symref_count;
-
-+ assert(failure_errno);
-+
- if (!oid)
- oid = &unused_oid;
- if (!flags)
-@@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs,
- if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
- if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
- !refname_is_safe(refname)) {
-- errno = EINVAL;
-+ *failure_errno = EINVAL;
- return NULL;
- }
-
-@@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs,
+ const char *refs_werrres_ref_unsafe(struct ref_store *refs,
+@@ refs.c: const char *refs_werrres_ref_unsafe(struct ref_store *refs,
for (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) {
unsigned int read_flags = 0;
+- errno = 0;
- if (refs_read_raw_ref(refs, refname,
- oid, &sb_refname, &read_flags)) {
+ if (refs_read_raw_ref(refs, refname, oid, &sb_refname,
+ &read_flags, failure_errno)) {
*flags |= read_flags;
-
- /* In reading mode, refs must eventually resolve */
-@@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs,
- * may show errors besides ENOENT if there are
- * similarly-named refs.
- */
-- if (errno != ENOENT &&
-- errno != EISDIR &&
-- errno != ENOTDIR)
-+ if (*failure_errno != ENOENT &&
-+ *failure_errno != EISDIR &&
-+ *failure_errno != ENOTDIR)
- return NULL;
-
- oidclr(oid);
-@@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs,
- if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
- if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
- !refname_is_safe(refname)) {
-- errno = EINVAL;
-+ *failure_errno = EINVAL;
- return NULL;
- }
-
-@@ refs.c: const char *refs_resolve_ref_unsafe(struct ref_store *refs,
- }
- }
-
-- errno = ELOOP;
-+ *failure_errno = ELOOP;
- return NULL;
- }
-
-+const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname,
-+ int resolve_flags, struct object_id *oid,
-+ int *flags)
-+{
-+ int failure_errno = 0;
-+ const char *refn;
-+ refn = refs_resolve_ref_unsafe_with_errno(refs, refname, resolve_flags,
-+ oid, flags, &failure_errno);
-+ if (!refn)
-+ /* For unmigrated legacy callers */
-+ errno = failure_errno;
-+ return refn;
-+}
-+
- /* backend functions */
- int refs_init_db(struct strbuf *err)
- {
+ if (errno)
+ *failure_errno = errno;
@@ refs.c: int refs_verify_refname_available(struct ref_store *refs,
strbuf_grow(&dirname, strlen(refname) + 1);
7: 18bf4a0e97c ! 4: 758c761abcf refs: explicitly return failure_errno from parse_loose_ref_contents
@@ Metadata
Author: Han-Wen Nienhuys <hanwen@google.com>
## Commit message ##
- refs: explicitly return failure_errno from parse_loose_ref_contents
+ refs API: make parse_loose_ref_contents() not set errno
+
+ Change the parse_loose_ref_contents() function to stop setting "errno"
+ and failure, and to instead pass up a "failure_errno" via a
+ parameter. This requires changing its callers to do the same.
The EINVAL error from parse_loose_ref_contents is used in files-backend
to create a custom error message.
1: f06b054e861 ! 5: cdf17fa34a4 refs file backend: move raceproof_create_file() here
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## Commit message ##
- refs file backend: move raceproof_create_file() here
+ refs API: make refs_rename_ref_available() static
- Move the raceproof_create_file() API added to cache.h and
- object-file.c in 177978f56ad (raceproof_create_file(): new function,
- 2017-01-06) to its only user, refs/files-backend.c.
+ Move the refs_rename_ref_available() function into
+ "refs/files-backend.c". It is file-backend specific.
- Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
+ This function was added in 5fe7d825da8 (refs.c: pass a list of names
+ to skip to is_refname_available, 2014-05-01) as rename_ref_available()
+ and was only ever used in this one file-backend specific codepath. So
+ let's move it there.
- ## cache.h ##
-@@ cache.h: enum scld_error safe_create_leading_directories(char *path);
- enum scld_error safe_create_leading_directories_const(const char *path);
- enum scld_error safe_create_leading_directories_no_share(char *path);
-
--/*
-- * Callback function for raceproof_create_file(). This function is
-- * expected to do something that makes dirname(path) permanent despite
-- * the fact that other processes might be cleaning up empty
-- * directories at the same time. Usually it will create a file named
-- * path, but alternatively it could create another file in that
-- * directory, or even chdir() into that directory. The function should
-- * return 0 if the action was completed successfully. On error, it
-- * should return a nonzero result and set errno.
-- * raceproof_create_file() treats two errno values specially:
-- *
-- * - ENOENT -- dirname(path) does not exist. In this case,
-- * raceproof_create_file() tries creating dirname(path)
-- * (and any parent directories, if necessary) and calls
-- * the function again.
-- *
-- * - EISDIR -- the file already exists and is a directory. In this
-- * case, raceproof_create_file() removes the directory if
-- * it is empty (and recursively any empty directories that
-- * it contains) and calls the function again.
-- *
-- * Any other errno causes raceproof_create_file() to fail with the
-- * callback's return value and errno.
-- *
-- * Obviously, this function should be OK with being called again if it
-- * fails with ENOENT or EISDIR. In other scenarios it will not be
-- * called again.
-- */
--typedef int create_file_fn(const char *path, void *cb);
--
--/*
-- * Create a file in dirname(path) by calling fn, creating leading
-- * directories if necessary. Retry a few times in case we are racing
-- * with another process that is trying to clean up the directory that
-- * contains path. See the documentation for create_file_fn for more
-- * details.
-- *
-- * Return the value and set the errno that resulted from the most
-- * recent call of fn. fn is always called at least once, and will be
-- * called more than once if it returns ENOENT or EISDIR.
-- */
--int raceproof_create_file(const char *path, create_file_fn fn, void *cb);
--
- int mkdir_in_gitdir(const char *path);
- char *expand_user_path(const char *path, int real_home);
- const char *enter_repo(const char *path, int strict);
+ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- ## object-file.c ##
-@@ object-file.c: enum scld_error safe_create_leading_directories_const(const char *path)
- return result;
+ ## refs.c ##
+@@ refs.c: const char *find_descendant_ref(const char *dirname,
+ return NULL;
}
--int raceproof_create_file(const char *path, create_file_fn fn, void *cb)
+-int refs_rename_ref_available(struct ref_store *refs,
+- const char *old_refname,
+- const char *new_refname)
-{
-- /*
-- * The number of times we will try to remove empty directories
-- * in the way of path. This is only 1 because if another
-- * process is racily creating directories that conflict with
-- * us, we don't want to fight against them.
-- */
-- int remove_directories_remaining = 1;
--
-- /*
-- * The number of times that we will try to create the
-- * directories containing path. We are willing to attempt this
-- * more than once, because another process could be trying to
-- * clean up empty directories at the same time as we are
-- * trying to create them.
-- */
-- int create_directories_remaining = 3;
--
-- /* A scratch copy of path, filled lazily if we need it: */
-- struct strbuf path_copy = STRBUF_INIT;
--
-- int ret, save_errno;
--
-- /* Sanity check: */
-- assert(*path);
+- struct string_list skip = STRING_LIST_INIT_NODUP;
+- struct strbuf err = STRBUF_INIT;
+- int ok;
-
--retry_fn:
-- ret = fn(path, cb);
-- save_errno = errno;
-- if (!ret)
-- goto out;
+- string_list_insert(&skip, old_refname);
+- ok = !refs_verify_refname_available(refs, new_refname,
+- NULL, &skip, &err);
+- if (!ok)
+- error("%s", err.buf);
-
-- if (errno == EISDIR && remove_directories_remaining-- > 0) {
-- /*
-- * A directory is in the way. Maybe it is empty; try
-- * to remove it:
-- */
-- if (!path_copy.len)
-- strbuf_addstr(&path_copy, path);
--
-- if (!remove_dir_recursively(&path_copy, REMOVE_DIR_EMPTY_ONLY))
-- goto retry_fn;
-- } else if (errno == ENOENT && create_directories_remaining-- > 0) {
-- /*
-- * Maybe the containing directory didn't exist, or
-- * maybe it was just deleted by a process that is
-- * racing with us to clean up empty directories. Try
-- * to create it:
-- */
-- enum scld_error scld_result;
--
-- if (!path_copy.len)
-- strbuf_addstr(&path_copy, path);
--
-- do {
-- scld_result = safe_create_leading_directories(path_copy.buf);
-- if (scld_result == SCLD_OK)
-- goto retry_fn;
-- } while (scld_result == SCLD_VANISHED && create_directories_remaining-- > 0);
-- }
--
--out:
-- strbuf_release(&path_copy);
-- errno = save_errno;
-- return ret;
+- string_list_clear(&skip, 0);
+- strbuf_release(&err);
+- return ok;
-}
-
- static void fill_loose_path(struct strbuf *buf, const struct object_id *oid)
+ int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
{
- int i;
+ struct object_id oid;
## refs/files-backend.c ##
-@@ refs/files-backend.c: static struct ref_iterator *files_ref_iterator_begin(
- return ref_iterator;
- }
+@@ refs/files-backend.c: static int commit_ref_update(struct files_ref_store *refs,
+ const struct object_id *oid, const char *logmsg,
+ struct strbuf *err);
+/*
-+ * Callback function for raceproof_create_file(). This function is
-+ * expected to do something that makes dirname(path) permanent despite
-+ * the fact that other processes might be cleaning up empty
-+ * directories at the same time. Usually it will create a file named
-+ * path, but alternatively it could create another file in that
-+ * directory, or even chdir() into that directory. The function should
-+ * return 0 if the action was completed successfully. On error, it
-+ * should return a nonzero result and set errno.
-+ * raceproof_create_file() treats two errno values specially:
++ * Check whether an attempt to rename old_refname to new_refname would
++ * cause a D/F conflict with any existing reference (other than
++ * possibly old_refname). If there would be a conflict, emit an error
++ * message and return false; otherwise, return true.
+ *
-+ * - ENOENT -- dirname(path) does not exist. In this case,
-+ * raceproof_create_file() tries creating dirname(path)
-+ * (and any parent directories, if necessary) and calls
-+ * the function again.
-+ *
-+ * - EISDIR -- the file already exists and is a directory. In this
-+ * case, raceproof_create_file() removes the directory if
-+ * it is empty (and recursively any empty directories that
-+ * it contains) and calls the function again.
-+ *
-+ * Any other errno causes raceproof_create_file() to fail with the
-+ * callback's return value and errno.
-+ *
-+ * Obviously, this function should be OK with being called again if it
-+ * fails with ENOENT or EISDIR. In other scenarios it will not be
-+ * called again.
-+ */
-+typedef int create_file_fn(const char *path, void *cb);
-+
-+/*
-+ * Create a file in dirname(path) by calling fn, creating leading
-+ * directories if necessary. Retry a few times in case we are racing
-+ * with another process that is trying to clean up the directory that
-+ * contains path. See the documentation for create_file_fn for more
-+ * details.
-+ *
-+ * Return the value and set the errno that resulted from the most
-+ * recent call of fn. fn is always called at least once, and will be
-+ * called more than once if it returns ENOENT or EISDIR.
++ * Note that this function is not safe against all races with other
++ * processes (though rename_ref() catches some races that might get by
++ * this check).
+ */
-+static int raceproof_create_file(const char *path, create_file_fn fn, void *cb)
++static int refs_rename_ref_available(struct ref_store *refs,
++ const char *old_refname,
++ const char *new_refname)
+{
-+ /*
-+ * The number of times we will try to remove empty directories
-+ * in the way of path. This is only 1 because if another
-+ * process is racily creating directories that conflict with
-+ * us, we don't want to fight against them.
-+ */
-+ int remove_directories_remaining = 1;
-+
-+ /*
-+ * The number of times that we will try to create the
-+ * directories containing path. We are willing to attempt this
-+ * more than once, because another process could be trying to
-+ * clean up empty directories at the same time as we are
-+ * trying to create them.
-+ */
-+ int create_directories_remaining = 3;
-+
-+ /* A scratch copy of path, filled lazily if we need it: */
-+ struct strbuf path_copy = STRBUF_INIT;
-+
-+ int ret, save_errno;
-+
-+ /* Sanity check: */
-+ assert(*path);
-+
-+retry_fn:
-+ ret = fn(path, cb);
-+ save_errno = errno;
-+ if (!ret)
-+ goto out;
++ struct string_list skip = STRING_LIST_INIT_NODUP;
++ struct strbuf err = STRBUF_INIT;
++ int ok;
+
-+ if (errno == EISDIR && remove_directories_remaining-- > 0) {
-+ /*
-+ * A directory is in the way. Maybe it is empty; try
-+ * to remove it:
-+ */
-+ if (!path_copy.len)
-+ strbuf_addstr(&path_copy, path);
++ string_list_insert(&skip, old_refname);
++ ok = !refs_verify_refname_available(refs, new_refname,
++ NULL, &skip, &err);
++ if (!ok)
++ error("%s", err.buf);
+
-+ if (!remove_dir_recursively(&path_copy, REMOVE_DIR_EMPTY_ONLY))
-+ goto retry_fn;
-+ } else if (errno == ENOENT && create_directories_remaining-- > 0) {
-+ /*
-+ * Maybe the containing directory didn't exist, or
-+ * maybe it was just deleted by a process that is
-+ * racing with us to clean up empty directories. Try
-+ * to create it:
-+ */
-+ enum scld_error scld_result;
-+
-+ if (!path_copy.len)
-+ strbuf_addstr(&path_copy, path);
-+
-+ do {
-+ scld_result = safe_create_leading_directories(path_copy.buf);
-+ if (scld_result == SCLD_OK)
-+ goto retry_fn;
-+ } while (scld_result == SCLD_VANISHED && create_directories_remaining-- > 0);
-+ }
-+
-+out:
-+ strbuf_release(&path_copy);
-+ errno = save_errno;
-+ return ret;
++ string_list_clear(&skip, 0);
++ strbuf_release(&err);
++ return ok;
+}
+
- static int remove_empty_directories(struct strbuf *path)
- {
- /*
+ static int files_copy_or_rename_ref(struct ref_store *ref_store,
+ const char *oldrefname, const char *newrefname,
+ const char *logmsg, int copy)
+
+ ## refs/refs-internal.h ##
+@@ refs/refs-internal.h: const char *find_descendant_ref(const char *dirname,
+ const struct string_list *extras,
+ const struct string_list *skip);
+
+-/*
+- * Check whether an attempt to rename old_refname to new_refname would
+- * cause a D/F conflict with any existing reference (other than
+- * possibly old_refname). If there would be a conflict, emit an error
+- * message and return false; otherwise, return true.
+- *
+- * Note that this function is not safe against all races with other
+- * processes (though rename_ref() catches some races that might get by
+- * this check).
+- */
+-int refs_rename_ref_available(struct ref_store *refs,
+- const char *old_refname,
+- const char *new_refname);
+-
+ /* We allow "recursive" symbolic refs. Only within reason, though */
+ #define SYMREF_MAXDEPTH 5
+
2: ba0f5f5fb0a < -: ----------- refs: remove EINVAL errno output from specification of read_raw_ref_fn
-: ----------- > 6: 3162bf28505 refs/files: remove "name exist?" check in lock_ref_oid_basic()
3: 2c4c30e8e06 ! 7: 288237b1900 refs/files-backend: stop setting errno from lock_ref_oid_basic
@@
## Metadata ##
-Author: Han-Wen Nienhuys <hanwen@google.com>
+Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## Commit message ##
- refs/files-backend: stop setting errno from lock_ref_oid_basic
+ refs API: remove refs_read_ref_full() wrapper
- refs/files-backend.c::lock_ref_oid_basic() tries to signal how it failed
- to its callers using errno.
+ Remove the refs_read_ref_full() wrapper in favor of migrating various
+ refs.c API users to the underlying refs_werrres_ref_unsafe() function.
- It is safe to stop setting errno here, because the callers of this
- file-scope static function are
+ A careful reading of these callers shows that the callers of this
+ function did not care about "errno", by moving away from the
+ refs_resolve_ref_unsafe() wrapper we can be sure that nothing relies
+ on it anymore.
- * files_copy_or_rename_ref()
- * files_create_symref()
- * files_reflog_expire()
-
- None of them looks at errno after seeing a negative return from
- lock_ref_oid_basic() to make any decision, and no caller of these three
- functions looks at errno after they signal a failure by returning a
- negative value. In particular,
-
- * files_copy_or_rename_ref() - here, calls are followed by error()
- (which performs I/O) or write_ref_to_lockfile() (which calls
- parse_object() which may perform I/O)
-
- * files_create_symref() - here, calls are followed by error() or
- create_symref_locked() (which performs I/O and does not inspect
- errno)
+ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- * files_reflog_expire() - here, calls are followed by error() or
- refs_reflog_exists() (which calls a function in a vtable that is not
- documented to use and/or preserve errno)
+ ## refs.c ##
+@@ refs.c: struct ref_filter {
+ void *cb_data;
+ };
+
+-int refs_read_ref_full(struct ref_store *refs, const char *refname,
+- int resolve_flags, struct object_id *oid, int *flags)
++int read_ref_full(const char *refname, int resolve_flags, struct object_id *oid, int *flags)
+ {
+- if (refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags))
++ int ignore_errno;
++ struct ref_store *refs = get_main_ref_store(the_repository);
++
++ if (refs_werrres_ref_unsafe(refs, refname, resolve_flags,
++ oid, flags, &ignore_errno))
+ return 0;
+ return -1;
+ }
+
+-int read_ref_full(const char *refname, int resolve_flags, struct object_id *oid, int *flags)
+-{
+- return refs_read_ref_full(get_main_ref_store(the_repository), refname,
+- resolve_flags, oid, flags);
+-}
+-
+ int read_ref(const char *refname, struct object_id *oid)
+ {
+ return read_ref_full(refname, RESOLVE_REF_READING, oid, NULL);
+@@ refs.c: int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
+ {
+ struct object_id oid;
+ int flag;
++ int ignore_errno;
+
+- if (!refs_read_ref_full(refs, "HEAD", RESOLVE_REF_READING,
+- &oid, &flag))
++ if (refs_werrres_ref_unsafe(refs, "HEAD", RESOLVE_REF_READING,
++ &oid, &flag, &ignore_errno))
+ return fn("HEAD", &oid, flag, cb_data);
+
+ return 0;
- Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
- Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
+ ## refs.h ##
+@@ refs.h: char *refs_resolve_refdup(struct ref_store *refs,
+ char *resolve_refdup(const char *refname, int resolve_flags,
+ struct object_id *oid, int *flags);
+
+-int refs_read_ref_full(struct ref_store *refs, const char *refname,
+- int resolve_flags, struct object_id *oid, int *flags);
+ int read_ref_full(const char *refname, int resolve_flags,
+ struct object_id *oid, int *flags);
+ int read_ref(const char *refname, struct object_id *oid);
## refs/files-backend.c ##
-@@ refs/files-backend.c: static int create_reflock(const char *path, void *cb)
-
- /*
- * Locks a ref returning the lock on success and NULL on failure.
-- * On failure errno is set to something meaningful.
- */
- static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
- const char *refname, int *type,
@@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
{
struct strbuf ref_file = STRBUF_INIT;
struct ref_lock *lock;
-- int last_errno = 0;
++ int ignore_errno;
files_assert_main_repository(refs, "lock_ref_oid_basic");
assert(err);
@@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
- if (!refs_resolve_ref_unsafe(&refs->base, refname,
- RESOLVE_REF_NO_RECURSE,
- &lock->old_oid, type)) {
-- last_errno = errno;
- if (!refs_verify_refname_available(&refs->base, refname,
- NULL, NULL, err))
- strbuf_addf(err, "unable to resolve reference '%s': %s",
-- refname, strerror(last_errno));
-+ refname, strerror(errno));
-
goto error_return;
}
-@@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
+
+- if (refs_read_ref_full(&refs->base, lock->ref_name,
+- 0,
+- &lock->old_oid, NULL))
++ if (!refs_werrres_ref_unsafe(&refs->base, lock->ref_name, 0,
++ &lock->old_oid, NULL, &ignore_errno))
+ oidclr(&lock->old_oid);
+ goto out;
+
+@@ refs/files-backend.c: static int files_copy_or_rename_ref(struct ref_store *ref_store,
+ struct strbuf tmp_renamed_log = STRBUF_INIT;
+ int log, ret;
+ struct strbuf err = STRBUF_INIT;
++ int ignore_errno;
+
+ files_reflog_path(refs, &sb_oldref, oldrefname);
+ files_reflog_path(refs, &sb_newref, newrefname);
+@@ refs/files-backend.c: static int files_copy_or_rename_ref(struct ref_store *ref_store,
+ * the safety anyway; we want to delete the reference whatever
+ * its current value.
*/
- if (is_null_oid(&lock->old_oid) &&
- refs_verify_refname_available(refs->packed_ref_store, refname,
-- NULL, NULL, err)) {
-- last_errno = ENOTDIR;
-+ NULL, NULL, err))
- goto error_return;
-- }
+- if (!copy && !refs_read_ref_full(&refs->base, newrefname,
+- RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
+- NULL, NULL) &&
++ if (!copy && refs_werrres_ref_unsafe(&refs->base, newrefname,
++ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
++ NULL, NULL, &ignore_errno) &&
+ refs_delete_ref(&refs->base, NULL, newrefname,
+ NULL, REF_NO_DEREF)) {
+ if (errno == EISDIR) {
+@@ refs/files-backend.c: static void update_symref_reflog(struct files_ref_store *refs,
+ {
+ struct strbuf err = STRBUF_INIT;
+ struct object_id new_oid;
++ int ignore_errno;
++
+ if (logmsg &&
+- !refs_read_ref_full(&refs->base, target,
+- RESOLVE_REF_READING, &new_oid, NULL) &&
++ refs_werrres_ref_unsafe(&refs->base, target,
++ RESOLVE_REF_READING, &new_oid, NULL,
++ &ignore_errno) &&
+ files_log_ref_write(refs, refname, &lock->old_oid,
+ &new_oid, logmsg, 0, &err)) {
+ error("%s", err.buf);
+@@ refs/files-backend.c: static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
+ (struct files_reflog_iterator *)ref_iterator;
+ struct dir_iterator *diter = iter->dir_iterator;
+ int ok;
++ int ignore_errno;
- lock->ref_name = xstrdup(refname);
+ while ((ok = dir_iterator_advance(diter)) == ITER_OK) {
+ int flags;
+@@ refs/files-backend.c: static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
+ if (ends_with(diter->basename, ".lock"))
+ continue;
- if (raceproof_create_file(ref_file.buf, create_reflock, &lock->lk)) {
-- last_errno = errno;
- unable_to_lock_message(ref_file.buf, errno, err);
- goto error_return;
- }
-@@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
+- if (refs_read_ref_full(iter->ref_store,
+- diter->relative_path, 0,
+- &iter->oid, &flags)) {
++ if (!refs_werrres_ref_unsafe(iter->ref_store,
++ diter->relative_path, 0,
++ &iter->oid, &flags,
++ &ignore_errno)) {
+ error("bad ref for %s", diter->path.buf);
+ continue;
+ }
+@@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *refs,
+ * the transaction, so we have to read it here
+ * to record and possibly check old_oid:
+ */
+- if (refs_read_ref_full(&refs->base,
+- referent.buf, 0,
+- &lock->old_oid, NULL)) {
++ int ignore_errno;
++ if (!refs_werrres_ref_unsafe(&refs->base,
++ referent.buf, 0,
++ &lock->old_oid, NULL,
++ &ignore_errno)) {
+ if (update->flags & REF_HAVE_OLD) {
+ strbuf_addf(err, "cannot lock ref '%s': "
+ "error reading reference",
+
+ ## worktree.c ##
+@@ worktree.c: int other_head_refs(each_ref_fn fn, void *cb_data)
+ struct worktree *wt = *p;
+ struct object_id oid;
+ int flag;
++ int ignore_errno;
- out:
- strbuf_release(&ref_file);
-- errno = last_errno;
- return lock;
- }
+ if (wt->is_current)
+ continue;
+ strbuf_reset(&refname);
+ strbuf_worktree_ref(wt, &refname, "HEAD");
+- if (!refs_read_ref_full(get_main_ref_store(the_repository),
+- refname.buf,
+- RESOLVE_REF_READING,
+- &oid, &flag))
++ if (refs_werrres_ref_unsafe(get_main_ref_store(the_repository),
++ refname.buf,
++ RESOLVE_REF_READING,
++ &oid, &flag, &ignore_errno))
+ ret = fn(refname.buf, &oid, flag, cb_data);
+ if (ret)
+ break;
-: ----------- > 8: acb484ea547 refs API: make resolve_gitlink_ref() not set errno
-: ----------- > 9: 4be84c9bf53 refs API: make loose_fill_ref_dir() not set errno
-: ----------- > 10: 8753210f9cc refs API: make files_copy_or_rename_ref() et al not set errno
-: ----------- > 11: 9fe85926140 refs API: ignore errno in worktree.c's add_head_info()
-: ----------- > 12: 8d87db98041 refs API: ignore errno in worktree.c's find_shared_symref()
-: ----------- > 13: 954633bcbb2 refs tests: ignore ignore errno in test-ref-store helper
-: ----------- > 14: fbbc08d3ebd refs API: make refs_resolve_refdup() not set errno
-: ----------- > 15: 4b2a4dbe7d5 refs API: make refs_ref_exists() not set errno
-: ----------- > 16: 888b1884c29 refs API: make resolve_ref_unsafe() not set errno
-: ----------- > 17: e2885f13c9b refs API: make expand_ref() & repo_dwim_log() not set errno
-: ----------- > 18: df50373a272 refs API: don't expose "errno" in run_transaction_hook()
8: 7d94a32af83 ! 19: 4c80b05bf1d refs: make errno output explicit for refs_resolve_ref_unsafe
@@
## Metadata ##
-Author: Han-Wen Nienhuys <hanwen@google.com>
+Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## Commit message ##
- refs: make errno output explicit for refs_resolve_ref_unsafe
+ refs API: post-migration API renaming [1/2]
- This introduces refs_resolve_ref_unsafe_with_errno(), which makes the API
- contract for the errno output explicit. The implementation still relies on
- the global errno variable to ensure no side effects of this refactoring.
+ In preceding commits all callers of refs_resolve_ref_unsafe() were
+ migrated to the transitory refs_werrres_ref_unsafe() function.
- lock_ref_oid_basic() in files-backend.c is the only caller of refs_resolve_ref()
- that needs error information to make logic decisions, so update that caller
+ As a first step in getting rid of it let's remove the old function
+ from the public API (it went unused in a preceding commit).
+
+ We then provide both a coccinelle rule to do the rename, and a macro
+ to avoid breaking the existing callers.
- Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
+ ## contrib/coccinelle/refs.pending.cocci (new) ##
+@@
++@@
++expression refs, refname, resolve_flags, oid, flags, failure_errno;
++@@
++- refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)
+++ refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)
+
## refs.c ##
@@ refs.c: int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
type, failure_errno);
}
--static const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs,
-+const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs,
- const char *refname,
- int resolve_flags,
- struct object_id *oid,
-
- ## refs.h ##
-@@ refs.h: const char *refs_resolve_ref_unsafe(struct ref_store *refs,
+-const char *refs_werrres_ref_unsafe(struct ref_store *refs,
++const char *refs_resolve_ref_unsafe(struct ref_store *refs,
+ const char *refname,
int resolve_flags,
struct object_id *oid,
- int *flags);
-+/**
-+ * refs_resolve_ref_unsafe_with_errno() is like
-+ * refs_resolve_ref_unsafe(), but provide access to errno code that
-+ * lead to a failure. We guarantee that errno is set to a meaningful
-+ * value on non-zero return.
-+ */
-+const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs,
-+ const char *refname,
-+ int resolve_flags,
-+ struct object_id *oid,
-+ int *flags, int *failure_errno);
- const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
- struct object_id *oid, int *flags);
+@@ refs.c: const char *refs_werrres_ref_unsafe(struct ref_store *refs,
+ return NULL;
+ }
-
- ## refs/files-backend.c ##
-@@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
+-const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname,
+- int resolve_flags, struct object_id *oid,
+- int *flags)
+-{
+- int failure_errno = 0;
+- const char *refn;
+- refn = refs_werrres_ref_unsafe(refs, refname, resolve_flags,
+- oid, flags, &failure_errno);
+- if (!refn)
+- errno = failure_errno;
+- return refn;
+-}
+-
+ /* backend functions */
+ int refs_init_db(struct strbuf *err)
{
- struct strbuf ref_file = STRBUF_INIT;
- struct ref_lock *lock;
-+ int resolve_errno = 0;
+
+ ## refs.h ##
+@@ refs.h: struct string_list;
+ struct string_list_item;
+ struct worktree;
- files_assert_main_repository(refs, "lock_ref_oid_basic");
- assert(err);
-@@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
- CALLOC_ARRAY(lock, 1);
+-/*
+- * Callers should not inspect "errno" on failure, but rather pass in a
+- * "failure_errno" parameter, on failure the "errno" will indicate the
+- * type of failure encountered, but not necessarily one that came from
+- * a syscall. We might have faked it up.
+- */
+-const char *refs_werrres_ref_unsafe(struct ref_store *refs,
+- const char *refname,
+- int resolve_flags,
+- struct object_id *oid,
+- int *flags, int *failure_errno);
+-
+ /*
+ * Resolve a reference, recursively following symbolic refererences.
+ *
+@@ refs.h: const char *refs_werrres_ref_unsafe(struct ref_store *refs,
+ * resolved. The function returns NULL for such ref names.
+ * Caps and underscores refers to the special refs, such as HEAD,
+ * FETCH_HEAD and friends, that all live outside of the refs/ directory.
++ *
++ * Callers should not inspect "errno" on failure, but rather pass in a
++ * "failure_errno" parameter, on failure the "errno" will indicate the
++ * type of failure encountered, but not necessarily one that came from
++ * a syscall. We might have faked it up.
+ */
+ #define RESOLVE_REF_READING 0x01
+ #define RESOLVE_REF_NO_RECURSE 0x02
+ #define RESOLVE_REF_ALLOW_BAD_NAME 0x04
- files_ref_path(refs, &ref_file, refname);
-- if (!refs_resolve_ref_unsafe(&refs->base, refname,
-- RESOLVE_REF_NO_RECURSE,
-- &lock->old_oid, type)) {
-+ if (!refs_resolve_ref_unsafe_with_errno(&refs->base, refname,
-+ RESOLVE_REF_NO_RECURSE,
-+ &lock->old_oid, type,
-+ &resolve_errno)) {
- if (!refs_verify_refname_available(&refs->base, refname,
- NULL, NULL, err))
- strbuf_addf(err, "unable to resolve reference '%s': %s",
-- refname, strerror(errno));
-+ refname, strerror(resolve_errno));
++#define refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno) \
++ refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)
+ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
+ const char *refname,
+ int resolve_flags,
+ struct object_id *oid,
+- int *flags);
++ int *flags, int *failure_errno);
++
+ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
+ struct object_id *oid, int *flags);
- goto error_return;
- }
-: ----------- > 20: 54b18e3a719 refs API: post-migration API renaming [2/2]
--
2.33.1.1346.g48288c3c089
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 01/20] branch tests: test for errno propagating on failing read
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-14 1:57 ` Eric Sunshine
2021-10-14 0:06 ` [PATCH 02/20] refs API: add a version of refs_resolve_ref_unsafe() with "errno" Ævar Arnfjörð Bjarmason
` (19 subsequent siblings)
20 siblings, 1 reply; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
From: Han-Wen Nienhuys <hanwen@google.com>
Add a test for "git branch" to cover the case where .git/refs is
symlinked. To check availability, refs_verify_refname_available() will
run refs_read_raw_ref() on each prefix, leading to a read() from
.git/refs (which is a directory).
It would probably be more robust to re-issue the lstat() as a normal
stat(), in which case, we would fall back to the directory case, but
for now let's just test for the existing behavior as-is. This test
covers a regression in a commit that only ever made it to "next", see
[1].
1. http://lore.kernel.org/git/pull.1068.git.git.1629203489546.gitgitgadget@gmail.com
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t3200-branch.sh | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e575ffb4ffb..b5242719159 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -731,6 +731,26 @@ test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog for
test_must_fail git branch -m u v
'
+test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '
+ test_when_finished "rm -rf subdir" &&
+ git init subdir &&
+
+ (
+ cd subdir &&
+ for d in refs objects packed-refs
+ do
+ rm -rf .git/$d &&
+ ln -s ../../.git/$d .git/$d
+ done
+ ) &&
+ git --git-dir subdir/.git/ branch rename-src &&
+ git rev-parse rename-src >expect &&
+ git --git-dir subdir/.git/ branch -m rename-src rename-dest &&
+ git rev-parse rename-dest >actual &&
+ test_cmp expect actual &&
+ git branch -D rename-dest
+'
+
test_expect_success 'test tracking setup via --track' '
git config remote.local.url . &&
git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 02/20] refs API: add a version of refs_resolve_ref_unsafe() with "errno"
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 01/20] branch tests: test for errno propagating on failing read Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 03/20] refs API: make refs_read_raw_ref() not set errno Ævar Arnfjörð Bjarmason
` (18 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
Add a new refs_werrres_ref_unsafe() function, which is like
refs_resolve_ref_unsafe() except that it explicitly saves away the
"errno" to a passed-in parameter, the refs_resolve_ref_unsafe() then
becomes a wrapper for it.
In subsequent commits we'll migrate code over to it, before finally
making "refs_resolve_ref_unsafe()" with an "errno" parameter the
canonical version, so this this function exists only so that we can
incrementally migrate callers, it will be going away in a subsequent
commit.
As the added comment notes has a rather tortured name to be the same
length as "refs_resolve_ref_unsafe", to avoid churn as we won't need
to re-indent the argument lists, similarly the documentation and
structure of it in refs.h is designed to minimize a diff in a
subsequent commit, where that documentation will be added to the new
refs_resolve_ref_unsafe().
At the end of this migration the "meaningful errno" TODO item left in
76d70dc0c63 (refs.c: make resolve_ref_unsafe set errno to something
meaningful on error, 2014-06-20) will be resolved.
As can be seen from the use of refs_read_raw_ref() we'll also need to
convert some functions that the new refs_werrres_ref_unsafe() itself
calls to take this "failure_errno". That will be done in subsequent
commits.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 36 +++++++++++++++++++++++++++---------
refs.h | 12 ++++++++++++
2 files changed, 39 insertions(+), 9 deletions(-)
diff --git a/refs.c b/refs.c
index 2166f37399d..46e3c785016 100644
--- a/refs.c
+++ b/refs.c
@@ -1678,17 +1678,19 @@ int refs_read_raw_ref(struct ref_store *ref_store,
type, &errno);
}
-/* This function needs to return a meaningful errno on failure */
-const char *refs_resolve_ref_unsafe(struct ref_store *refs,
+const char *refs_werrres_ref_unsafe(struct ref_store *refs,
const char *refname,
int resolve_flags,
- struct object_id *oid, int *flags)
+ struct object_id *oid,
+ int *flags, int *failure_errno)
{
static struct strbuf sb_refname = STRBUF_INIT;
struct object_id unused_oid;
int unused_flags;
int symref_count;
+ assert(failure_errno);
+
if (!oid)
oid = &unused_oid;
if (!flags)
@@ -1699,7 +1701,7 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
!refname_is_safe(refname)) {
- errno = EINVAL;
+ *failure_errno = EINVAL;
return NULL;
}
@@ -1717,9 +1719,12 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
for (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) {
unsigned int read_flags = 0;
+ errno = 0;
if (refs_read_raw_ref(refs, refname,
oid, &sb_refname, &read_flags)) {
*flags |= read_flags;
+ if (errno)
+ *failure_errno = errno;
/* In reading mode, refs must eventually resolve */
if (resolve_flags & RESOLVE_REF_READING)
@@ -1730,9 +1735,9 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
* may show errors besides ENOENT if there are
* similarly-named refs.
*/
- if (errno != ENOENT &&
- errno != EISDIR &&
- errno != ENOTDIR)
+ if (*failure_errno != ENOENT &&
+ *failure_errno != EISDIR &&
+ *failure_errno != ENOTDIR)
return NULL;
oidclr(oid);
@@ -1759,7 +1764,7 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
!refname_is_safe(refname)) {
- errno = EINVAL;
+ *failure_errno = EINVAL;
return NULL;
}
@@ -1767,10 +1772,23 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
}
}
- errno = ELOOP;
+ *failure_errno = ELOOP;
return NULL;
}
+const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname,
+ int resolve_flags, struct object_id *oid,
+ int *flags)
+{
+ int failure_errno = 0;
+ const char *refn;
+ refn = refs_werrres_ref_unsafe(refs, refname, resolve_flags,
+ oid, flags, &failure_errno);
+ if (!refn)
+ errno = failure_errno;
+ return refn;
+}
+
/* backend functions */
int refs_init_db(struct strbuf *err)
{
diff --git a/refs.h b/refs.h
index d5099d4984e..c8afde6bb50 100644
--- a/refs.h
+++ b/refs.h
@@ -11,6 +11,18 @@ struct string_list;
struct string_list_item;
struct worktree;
+/*
+ * Callers should not inspect "errno" on failure, but rather pass in a
+ * "failure_errno" parameter, on failure the "errno" will indicate the
+ * type of failure encountered, but not necessarily one that came from
+ * a syscall. We might have faked it up.
+ */
+const char *refs_werrres_ref_unsafe(struct ref_store *refs,
+ const char *refname,
+ int resolve_flags,
+ struct object_id *oid,
+ int *flags, int *failure_errno);
+
/*
* Resolve a reference, recursively following symbolic refererences.
*
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 03/20] refs API: make refs_read_raw_ref() not set errno
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 01/20] branch tests: test for errno propagating on failing read Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 02/20] refs API: add a version of refs_resolve_ref_unsafe() with "errno" Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 04/20] refs API: make parse_loose_ref_contents() " Ævar Arnfjörð Bjarmason
` (17 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
From: Han-Wen Nienhuys <hanwen@google.com>
Add a "failure_errno" to refs_read_raw_ref(), his allows
refs_werrres_ref_unsafe() to pass along its "failure_errno", as a
first step before its own callers are migrated to pass it further up
the chain.
We are leaving out out the refs_read_special_head() in
refs_read_raw_ref() for now, as noted in a subsequent commit moving it
to "failure_errno" will require some special consideration.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 24 ++++++++++++++++--------
refs/files-backend.c | 10 ++++++----
refs/packed-backend.c | 7 ++++---
refs/refs-internal.h | 6 +++---
4 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/refs.c b/refs.c
index 46e3c785016..acc5d5fa578 100644
--- a/refs.c
+++ b/refs.c
@@ -1665,17 +1665,18 @@ static int refs_read_special_head(struct ref_store *ref_store,
return result;
}
-int refs_read_raw_ref(struct ref_store *ref_store,
- const char *refname, struct object_id *oid,
- struct strbuf *referent, unsigned int *type)
+int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
+ struct object_id *oid, struct strbuf *referent,
+ unsigned int *type, int *failure_errno)
{
+ assert(failure_errno);
if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
return refs_read_special_head(ref_store, refname, oid, referent,
type);
}
return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
- type, &errno);
+ type, failure_errno);
}
const char *refs_werrres_ref_unsafe(struct ref_store *refs,
@@ -1719,9 +1720,8 @@ const char *refs_werrres_ref_unsafe(struct ref_store *refs,
for (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) {
unsigned int read_flags = 0;
- errno = 0;
- if (refs_read_raw_ref(refs, refname,
- oid, &sb_refname, &read_flags)) {
+ if (refs_read_raw_ref(refs, refname, oid, &sb_refname,
+ &read_flags, failure_errno)) {
*flags |= read_flags;
if (errno)
*failure_errno = errno;
@@ -2239,6 +2239,13 @@ int refs_verify_refname_available(struct ref_store *refs,
strbuf_grow(&dirname, strlen(refname) + 1);
for (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) {
+ /*
+ * Just saying "Is a directory" when we e.g. can't
+ * lock some multi-level ref isn't very informative,
+ * the user won't be told *what* is a directory, so
+ * let's not use strerror() below.
+ */
+ int ignore_errno;
/* Expand dirname to the new prefix, not including the trailing slash: */
strbuf_add(&dirname, refname + dirname.len, slash - refname - dirname.len);
@@ -2250,7 +2257,8 @@ int refs_verify_refname_available(struct ref_store *refs,
if (skip && string_list_has_string(skip, dirname.buf))
continue;
- if (!refs_read_raw_ref(refs, dirname.buf, &oid, &referent, &type)) {
+ if (!refs_read_raw_ref(refs, dirname.buf, &oid, &referent,
+ &type, &ignore_errno)) {
strbuf_addf(err, _("'%s' exists; cannot create '%s'"),
dirname.buf, refname);
goto cleanup;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 6a6ead0b99b..94c194665ed 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -381,10 +381,11 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
goto out;
if (lstat(path, &st) < 0) {
+ int ignore_errno;
if (errno != ENOENT)
goto out;
- if (refs_read_raw_ref(refs->packed_ref_store, refname,
- oid, referent, type)) {
+ if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
+ referent, type, &ignore_errno)) {
errno = ENOENT;
goto out;
}
@@ -418,13 +419,14 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
/* Is it a directory? */
if (S_ISDIR(st.st_mode)) {
+ int ignore_errno;
/*
* Even though there is a directory where the loose
* ref is supposed to be, there could still be a
* packed ref:
*/
- if (refs_read_raw_ref(refs->packed_ref_store, refname,
- oid, referent, type)) {
+ if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
+ referent, type, &ignore_errno)) {
errno = EISDIR;
goto out;
}
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 47247a14917..52cdc94a26e 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1347,6 +1347,7 @@ int is_packed_transaction_needed(struct ref_store *ref_store,
ret = 0;
for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = transaction->updates[i];
+ int failure_errno;
unsigned int type;
struct object_id oid;
@@ -1357,9 +1358,9 @@ int is_packed_transaction_needed(struct ref_store *ref_store,
*/
continue;
- if (!refs_read_raw_ref(ref_store, update->refname,
- &oid, &referent, &type) ||
- errno != ENOENT) {
+ if (!refs_read_raw_ref(ref_store, update->refname, &oid,
+ &referent, &type, &failure_errno) ||
+ failure_errno != ENOENT) {
/*
* We have to actually delete that reference
* -> this transaction is needed.
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 72746407fc3..c87f1135e5b 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -149,9 +149,9 @@ struct ref_update {
const char refname[FLEX_ARRAY];
};
-int refs_read_raw_ref(struct ref_store *ref_store,
- const char *refname, struct object_id *oid,
- struct strbuf *referent, unsigned int *type);
+int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
+ struct object_id *oid, struct strbuf *referent,
+ unsigned int *type, int *failure_errno);
/*
* Write an error to `err` and return a nonzero value iff the same
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 04/20] refs API: make parse_loose_ref_contents() not set errno
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-10-14 0:06 ` [PATCH 03/20] refs API: make refs_read_raw_ref() not set errno Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 05/20] refs API: make refs_rename_ref_available() static Ævar Arnfjörð Bjarmason
` (16 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
From: Han-Wen Nienhuys <hanwen@google.com>
Change the parse_loose_ref_contents() function to stop setting "errno"
and failure, and to instead pass up a "failure_errno" via a
parameter. This requires changing its callers to do the same.
The EINVAL error from parse_loose_ref_contents is used in files-backend
to create a custom error message.
In untangling this we discovered a tricky edge case. The
refs_read_special_head() function was relying on
parse_loose_ref_contents() setting EINVAL.
By converting it to use "saved_errno" we can migrate away from "errno"
in this part of the code entirely, and do away with an existing
"save_errno" pattern, its only purpose was to not clobber the "errno"
we previously needed at the end of files_read_raw_ref().
Let's assert that we can do that by not having files_read_raw_ref()
itself operate on *failure_errno in addition to passing it on. Instead
we'll assert that if we return non-zero we actually do set errno, thus
assuring ourselves and callers that they can trust the resulting
"failure_errno".
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 8 +++++---
refs/files-backend.c | 31 ++++++++++++++++++++-----------
refs/refs-internal.h | 6 ++++--
t/t3200-branch.sh | 1 +
4 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/refs.c b/refs.c
index acc5d5fa578..b1640b5d582 100644
--- a/refs.c
+++ b/refs.c
@@ -1647,7 +1647,8 @@ int for_each_fullref_in_prefixes(const char *namespace,
static int refs_read_special_head(struct ref_store *ref_store,
const char *refname, struct object_id *oid,
- struct strbuf *referent, unsigned int *type)
+ struct strbuf *referent, unsigned int *type,
+ int *failure_errno)
{
struct strbuf full_path = STRBUF_INIT;
struct strbuf content = STRBUF_INIT;
@@ -1657,7 +1658,8 @@ static int refs_read_special_head(struct ref_store *ref_store,
if (strbuf_read_file(&content, full_path.buf, 0) < 0)
goto done;
- result = parse_loose_ref_contents(content.buf, oid, referent, type);
+ result = parse_loose_ref_contents(content.buf, oid, referent, type,
+ failure_errno);
done:
strbuf_release(&full_path);
@@ -1672,7 +1674,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
assert(failure_errno);
if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
return refs_read_special_head(ref_store, refname, oid, referent,
- type);
+ type, failure_errno);
}
return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 94c194665ed..c73ffd1ca33 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -355,6 +355,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
int fd;
int ret = -1;
int remaining_retries = 3;
+ int myerr = 0;
*type = 0;
strbuf_reset(&sb_path);
@@ -382,11 +383,13 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
if (lstat(path, &st) < 0) {
int ignore_errno;
- if (errno != ENOENT)
+ myerr = errno;
+ errno = 0;
+ if (myerr != ENOENT)
goto out;
if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
referent, type, &ignore_errno)) {
- errno = ENOENT;
+ myerr = ENOENT;
goto out;
}
ret = 0;
@@ -397,7 +400,9 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
if (S_ISLNK(st.st_mode)) {
strbuf_reset(&sb_contents);
if (strbuf_readlink(&sb_contents, path, st.st_size) < 0) {
- if (errno == ENOENT || errno == EINVAL)
+ myerr = errno;
+ errno = 0;
+ if (myerr == ENOENT || myerr == EINVAL)
/* inconsistent with lstat; retry */
goto stat_ref;
else
@@ -427,7 +432,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
*/
if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
referent, type, &ignore_errno)) {
- errno = EISDIR;
+ myerr = EISDIR;
goto out;
}
ret = 0;
@@ -440,7 +445,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
*/
fd = open(path, O_RDONLY);
if (fd < 0) {
- if (errno == ENOENT && !S_ISLNK(st.st_mode))
+ myerr = errno;
+ if (myerr == ENOENT && !S_ISLNK(st.st_mode))
/* inconsistent with lstat; retry */
goto stat_ref;
else
@@ -448,26 +454,29 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
}
strbuf_reset(&sb_contents);
if (strbuf_read(&sb_contents, fd, 256) < 0) {
- int save_errno = errno;
+ myerr = errno;
close(fd);
- errno = save_errno;
goto out;
}
close(fd);
strbuf_rtrim(&sb_contents);
buf = sb_contents.buf;
- ret = parse_loose_ref_contents(buf, oid, referent, type);
+ ret = parse_loose_ref_contents(buf, oid, referent, type, &myerr);
out:
- *failure_errno = errno;
+ if (ret && !myerr)
+ BUG("returning non-zero %d, should have set myerr!", ret);
+ *failure_errno = myerr;
+
strbuf_release(&sb_path);
strbuf_release(&sb_contents);
return ret;
}
int parse_loose_ref_contents(const char *buf, struct object_id *oid,
- struct strbuf *referent, unsigned int *type)
+ struct strbuf *referent, unsigned int *type,
+ int *failure_errno)
{
const char *p;
if (skip_prefix(buf, "ref:", &buf)) {
@@ -486,7 +495,7 @@ int parse_loose_ref_contents(const char *buf, struct object_id *oid,
if (parse_oid_hex(buf, oid, &p) ||
(*p != '\0' && !isspace(*p))) {
*type |= REF_ISBROKEN;
- errno = EINVAL;
+ *failure_errno = EINVAL;
return -1;
}
return 0;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index c87f1135e5b..121b8fdad08 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -706,10 +706,12 @@ struct ref_store {
};
/*
- * Parse contents of a loose ref file.
+ * Parse contents of a loose ref file. *failure_errno maybe be set to EINVAL for
+ * invalid contents.
*/
int parse_loose_ref_contents(const char *buf, struct object_id *oid,
- struct strbuf *referent, unsigned int *type);
+ struct strbuf *referent, unsigned int *type,
+ int *failure_errno);
/*
* Fill in the generic part of refs and add it to our collection of
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index b5242719159..7486688d292 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -745,6 +745,7 @@ test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '
) &&
git --git-dir subdir/.git/ branch rename-src &&
git rev-parse rename-src >expect &&
+ # Tests a BUG() assertion in files_read_raw_ref()
git --git-dir subdir/.git/ branch -m rename-src rename-dest &&
git rev-parse rename-dest >actual &&
test_cmp expect actual &&
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 05/20] refs API: make refs_rename_ref_available() static
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-10-14 0:06 ` [PATCH 04/20] refs API: make parse_loose_ref_contents() " Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 06/20] refs/files: remove "name exist?" check in lock_ref_oid_basic() Ævar Arnfjörð Bjarmason
` (15 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
Move the refs_rename_ref_available() function into
"refs/files-backend.c". It is file-backend specific.
This function was added in 5fe7d825da8 (refs.c: pass a list of names
to skip to is_refname_available, 2014-05-01) as rename_ref_available()
and was only ever used in this one file-backend specific codepath. So
let's move it there.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 19 -------------------
refs/files-backend.c | 29 +++++++++++++++++++++++++++++
refs/refs-internal.h | 14 --------------
3 files changed, 29 insertions(+), 33 deletions(-)
diff --git a/refs.c b/refs.c
index b1640b5d582..4398a4c0257 100644
--- a/refs.c
+++ b/refs.c
@@ -1371,25 +1371,6 @@ const char *find_descendant_ref(const char *dirname,
return NULL;
}
-int refs_rename_ref_available(struct ref_store *refs,
- const char *old_refname,
- const char *new_refname)
-{
- struct string_list skip = STRING_LIST_INIT_NODUP;
- struct strbuf err = STRBUF_INIT;
- int ok;
-
- string_list_insert(&skip, old_refname);
- ok = !refs_verify_refname_available(refs, new_refname,
- NULL, &skip, &err);
- if (!ok)
- error("%s", err.buf);
-
- string_list_clear(&skip, 0);
- strbuf_release(&err);
- return ok;
-}
-
int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
{
struct object_id oid;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index c73ffd1ca33..0af6ee44552 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1363,6 +1363,35 @@ static int commit_ref_update(struct files_ref_store *refs,
const struct object_id *oid, const char *logmsg,
struct strbuf *err);
+/*
+ * Check whether an attempt to rename old_refname to new_refname would
+ * cause a D/F conflict with any existing reference (other than
+ * possibly old_refname). If there would be a conflict, emit an error
+ * message and return false; otherwise, return true.
+ *
+ * Note that this function is not safe against all races with other
+ * processes (though rename_ref() catches some races that might get by
+ * this check).
+ */
+static int refs_rename_ref_available(struct ref_store *refs,
+ const char *old_refname,
+ const char *new_refname)
+{
+ struct string_list skip = STRING_LIST_INIT_NODUP;
+ struct strbuf err = STRBUF_INIT;
+ int ok;
+
+ string_list_insert(&skip, old_refname);
+ ok = !refs_verify_refname_available(refs, new_refname,
+ NULL, &skip, &err);
+ if (!ok)
+ error("%s", err.buf);
+
+ string_list_clear(&skip, 0);
+ strbuf_release(&err);
+ return ok;
+}
+
static int files_copy_or_rename_ref(struct ref_store *ref_store,
const char *oldrefname, const char *newrefname,
const char *logmsg, int copy)
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 121b8fdad08..2ecd3dea9fb 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -228,20 +228,6 @@ const char *find_descendant_ref(const char *dirname,
const struct string_list *extras,
const struct string_list *skip);
-/*
- * Check whether an attempt to rename old_refname to new_refname would
- * cause a D/F conflict with any existing reference (other than
- * possibly old_refname). If there would be a conflict, emit an error
- * message and return false; otherwise, return true.
- *
- * Note that this function is not safe against all races with other
- * processes (though rename_ref() catches some races that might get by
- * this check).
- */
-int refs_rename_ref_available(struct ref_store *refs,
- const char *old_refname,
- const char *new_refname);
-
/* We allow "recursive" symbolic refs. Only within reason, though */
#define SYMREF_MAXDEPTH 5
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 06/20] refs/files: remove "name exist?" check in lock_ref_oid_basic()
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-10-14 0:06 ` [PATCH 05/20] refs API: make refs_rename_ref_available() static Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-14 20:53 ` Junio C Hamano
2021-10-14 0:06 ` [PATCH 07/20] refs API: remove refs_read_ref_full() wrapper Ævar Arnfjörð Bjarmason
` (14 subsequent siblings)
20 siblings, 1 reply; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
In lock_ref_oid_basic() we'll happily lock a reference that doesn't
exist yet. That's normal, and is how references are initially born,
but we don't need to retain checks here in lock_ref_oid_basic() about
the state of the ref, when what we're checking is either checked
already, or something we're about to discover by trying to lock the
ref with raceproof_create_file().
The improved diagnostics here were added in
5b2d8d6f218 (lock_ref_sha1_basic(): improve diagnostics for ref D/F
conflicts, 2015-05-11), and then much of the surrounding code went
away recently in my 245fbba46d6 (refs/files: remove unused "errno ==
EISDIR" code, 2021-08-23).
The refs_resolve_ref_unsafe() code being removed here looks like it
should be tasked with doing that, but it's actually redundant to other
code.
The reason for that is as noted in 245fbba46d6 this once widely used
function now only has a handful of callers left, which all handle this
case themselves.
To the extent that we're racy between their check and ours removing
this check actually improves the situation, as we'll be doing fewer
things between the not-under-lock initial check and acquiring the
lock.
Why this is OK for all the remaining callers of lock_ref_oid_basic()
is noted below. There are only two of those callers:
* "git branch -[cm] <oldbranch> <newbranch>":
In files_copy_or_rename_ref() we'll call this when we copy or rename
refs via rename_ref() and copy_ref(). but only after we've checked
if the refname exists already via its own call to
refs_resolve_ref_unsafe() and refs_rename_ref_available().
As the updated comment to the latter here notes neither of those are
actually needed. If we delete not only this code but also
refs_rename_ref_available() we'll do just fine, we'll just emit a
less friendly error message if e.g. "git branch -m A B/C" would have
a D/F conflict with a "B" file.
Actually we'd probably die before that in case reflogs for the
branch existed, i.e. when the try to rename() or copy_file() the
relevant reflog, since if we've got a D/F conflict with a branch
name we'll probably also have the same with its reflogs (but not
necessarily, we might have reflogs, but it might not).
As some #leftoverbits that code seems buggy to me, i.e. the reflog
"protocol" should be to get a lock on the main ref, and then perform
ref and/or reflog operations. That code dates back to
c976d415e53 (git-branch: add options and tests for branch renaming,
2006-11-28) and probably pre-dated the solidifying of that
convention. But in any case, that edge case is not our bug or
problem right now.
* "git reflog expire <ref>":
In files_reflog_expire() we'll call this without previous ref
existence checking in files-backend.c, but that code is in turn
called by code that's just finished checking if the refname whose
reflog we're expiring exists.
See ae35e16cd43 (reflog expire: don't lock reflogs using previously
seen OID, 2021-08-23) for the current state of that code.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs/files-backend.c | 20 +++++---------------
1 file changed, 5 insertions(+), 15 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0af6ee44552..0dd21b2e205 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1013,16 +1013,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
CALLOC_ARRAY(lock, 1);
files_ref_path(refs, &ref_file, refname);
- if (!refs_resolve_ref_unsafe(&refs->base, refname,
- RESOLVE_REF_NO_RECURSE,
- &lock->old_oid, type)) {
- if (!refs_verify_refname_available(&refs->base, refname,
- NULL, NULL, err))
- strbuf_addf(err, "unable to resolve reference '%s': %s",
- refname, strerror(errno));
-
- goto error_return;
- }
/*
* If the ref did not exist and we are creating it, make sure
@@ -1364,14 +1354,14 @@ static int commit_ref_update(struct files_ref_store *refs,
struct strbuf *err);
/*
- * Check whether an attempt to rename old_refname to new_refname would
- * cause a D/F conflict with any existing reference (other than
- * possibly old_refname). If there would be a conflict, emit an error
+ * Emit a better error message than lockfile.c's
+ * unable_to_lock_message() would in case there is a D/F conflict with
+ * another existing reference. If there would be a conflict, emit an error
* message and return false; otherwise, return true.
*
* Note that this function is not safe against all races with other
- * processes (though rename_ref() catches some races that might get by
- * this check).
+ * processes, and that's not its job. We'll emit a more verbose error on D/f
+ * conflicts if we get past it into lock_ref_oid_basic().
*/
static int refs_rename_ref_available(struct ref_store *refs,
const char *old_refname,
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 07/20] refs API: remove refs_read_ref_full() wrapper
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-10-14 0:06 ` [PATCH 06/20] refs/files: remove "name exist?" check in lock_ref_oid_basic() Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 08/20] refs API: make resolve_gitlink_ref() not set errno Ævar Arnfjörð Bjarmason
` (13 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
Remove the refs_read_ref_full() wrapper in favor of migrating various
refs.c API users to the underlying refs_werrres_ref_unsafe() function.
A careful reading of these callers shows that the callers of this
function did not care about "errno", by moving away from the
refs_resolve_ref_unsafe() wrapper we can be sure that nothing relies
on it anymore.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 20 +++++++++-----------
refs.h | 2 --
refs/files-backend.c | 36 ++++++++++++++++++++++--------------
worktree.c | 9 +++++----
4 files changed, 36 insertions(+), 31 deletions(-)
diff --git a/refs.c b/refs.c
index 4398a4c0257..08101a13318 100644
--- a/refs.c
+++ b/refs.c
@@ -289,20 +289,17 @@ struct ref_filter {
void *cb_data;
};
-int refs_read_ref_full(struct ref_store *refs, const char *refname,
- int resolve_flags, struct object_id *oid, int *flags)
+int read_ref_full(const char *refname, int resolve_flags, struct object_id *oid, int *flags)
{
- if (refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags))
+ int ignore_errno;
+ struct ref_store *refs = get_main_ref_store(the_repository);
+
+ if (refs_werrres_ref_unsafe(refs, refname, resolve_flags,
+ oid, flags, &ignore_errno))
return 0;
return -1;
}
-int read_ref_full(const char *refname, int resolve_flags, struct object_id *oid, int *flags)
-{
- return refs_read_ref_full(get_main_ref_store(the_repository), refname,
- resolve_flags, oid, flags);
-}
-
int read_ref(const char *refname, struct object_id *oid)
{
return read_ref_full(refname, RESOLVE_REF_READING, oid, NULL);
@@ -1375,9 +1372,10 @@ int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
{
struct object_id oid;
int flag;
+ int ignore_errno;
- if (!refs_read_ref_full(refs, "HEAD", RESOLVE_REF_READING,
- &oid, &flag))
+ if (refs_werrres_ref_unsafe(refs, "HEAD", RESOLVE_REF_READING,
+ &oid, &flag, &ignore_errno))
return fn("HEAD", &oid, flag, cb_data);
return 0;
diff --git a/refs.h b/refs.h
index c8afde6bb50..3938f99c902 100644
--- a/refs.h
+++ b/refs.h
@@ -89,8 +89,6 @@ char *refs_resolve_refdup(struct ref_store *refs,
char *resolve_refdup(const char *refname, int resolve_flags,
struct object_id *oid, int *flags);
-int refs_read_ref_full(struct ref_store *refs, const char *refname,
- int resolve_flags, struct object_id *oid, int *flags);
int read_ref_full(const char *refname, int resolve_flags,
struct object_id *oid, int *flags);
int read_ref(const char *refname, struct object_id *oid);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0dd21b2e205..be27823d099 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1006,6 +1006,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
{
struct strbuf ref_file = STRBUF_INIT;
struct ref_lock *lock;
+ int ignore_errno;
files_assert_main_repository(refs, "lock_ref_oid_basic");
assert(err);
@@ -1032,9 +1033,8 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
goto error_return;
}
- if (refs_read_ref_full(&refs->base, lock->ref_name,
- 0,
- &lock->old_oid, NULL))
+ if (!refs_werrres_ref_unsafe(&refs->base, lock->ref_name, 0,
+ &lock->old_oid, NULL, &ignore_errno))
oidclr(&lock->old_oid);
goto out;
@@ -1397,6 +1397,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
struct strbuf tmp_renamed_log = STRBUF_INIT;
int log, ret;
struct strbuf err = STRBUF_INIT;
+ int ignore_errno;
files_reflog_path(refs, &sb_oldref, oldrefname);
files_reflog_path(refs, &sb_newref, newrefname);
@@ -1454,9 +1455,9 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
* the safety anyway; we want to delete the reference whatever
* its current value.
*/
- if (!copy && !refs_read_ref_full(&refs->base, newrefname,
- RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
- NULL, NULL) &&
+ if (!copy && refs_werrres_ref_unsafe(&refs->base, newrefname,
+ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
+ NULL, NULL, &ignore_errno) &&
refs_delete_ref(&refs->base, NULL, newrefname,
NULL, REF_NO_DEREF)) {
if (errno == EISDIR) {
@@ -1868,9 +1869,12 @@ static void update_symref_reflog(struct files_ref_store *refs,
{
struct strbuf err = STRBUF_INIT;
struct object_id new_oid;
+ int ignore_errno;
+
if (logmsg &&
- !refs_read_ref_full(&refs->base, target,
- RESOLVE_REF_READING, &new_oid, NULL) &&
+ refs_werrres_ref_unsafe(&refs->base, target,
+ RESOLVE_REF_READING, &new_oid, NULL,
+ &ignore_errno) &&
files_log_ref_write(refs, refname, &lock->old_oid,
&new_oid, logmsg, 0, &err)) {
error("%s", err.buf);
@@ -2144,6 +2148,7 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
(struct files_reflog_iterator *)ref_iterator;
struct dir_iterator *diter = iter->dir_iterator;
int ok;
+ int ignore_errno;
while ((ok = dir_iterator_advance(diter)) == ITER_OK) {
int flags;
@@ -2155,9 +2160,10 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
if (ends_with(diter->basename, ".lock"))
continue;
- if (refs_read_ref_full(iter->ref_store,
- diter->relative_path, 0,
- &iter->oid, &flags)) {
+ if (!refs_werrres_ref_unsafe(iter->ref_store,
+ diter->relative_path, 0,
+ &iter->oid, &flags,
+ &ignore_errno)) {
error("bad ref for %s", diter->path.buf);
continue;
}
@@ -2501,9 +2507,11 @@ static int lock_ref_for_update(struct files_ref_store *refs,
* the transaction, so we have to read it here
* to record and possibly check old_oid:
*/
- if (refs_read_ref_full(&refs->base,
- referent.buf, 0,
- &lock->old_oid, NULL)) {
+ int ignore_errno;
+ if (!refs_werrres_ref_unsafe(&refs->base,
+ referent.buf, 0,
+ &lock->old_oid, NULL,
+ &ignore_errno)) {
if (update->flags & REF_HAVE_OLD) {
strbuf_addf(err, "cannot lock ref '%s': "
"error reading reference",
diff --git a/worktree.c b/worktree.c
index 092a4f92ad2..cfffcdb62b3 100644
--- a/worktree.c
+++ b/worktree.c
@@ -563,16 +563,17 @@ int other_head_refs(each_ref_fn fn, void *cb_data)
struct worktree *wt = *p;
struct object_id oid;
int flag;
+ int ignore_errno;
if (wt->is_current)
continue;
strbuf_reset(&refname);
strbuf_worktree_ref(wt, &refname, "HEAD");
- if (!refs_read_ref_full(get_main_ref_store(the_repository),
- refname.buf,
- RESOLVE_REF_READING,
- &oid, &flag))
+ if (refs_werrres_ref_unsafe(get_main_ref_store(the_repository),
+ refname.buf,
+ RESOLVE_REF_READING,
+ &oid, &flag, &ignore_errno))
ret = fn(refname.buf, &oid, flag, cb_data);
if (ret)
break;
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 08/20] refs API: make resolve_gitlink_ref() not set errno
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2021-10-14 0:06 ` [PATCH 07/20] refs API: remove refs_read_ref_full() wrapper Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 09/20] refs API: make loose_fill_ref_dir() " Ævar Arnfjörð Bjarmason
` (12 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
I have carefully read the upstream callers of resolve_gitlink_ref()
and determined that they don't care about errno.
So let's move away from the errno-setting refs_resolve_ref_unsafe()
wrapper to refs_werrres_ref_unsafe(), and explicitly ignore the errno
it sets for us.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 08101a13318..001f54845a7 100644
--- a/refs.c
+++ b/refs.c
@@ -1790,14 +1790,15 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,
{
struct ref_store *refs;
int flags;
+ int ignore_errno;
refs = get_submodule_ref_store(submodule);
if (!refs)
return -1;
- if (!refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags) ||
- is_null_oid(oid))
+ if (!refs_werrres_ref_unsafe(refs, refname, 0, oid, &flags,
+ &ignore_errno) || is_null_oid(oid))
return -1;
return 0;
}
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 09/20] refs API: make loose_fill_ref_dir() not set errno
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2021-10-14 0:06 ` [PATCH 08/20] refs API: make resolve_gitlink_ref() not set errno Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 10/20] refs API: make files_copy_or_rename_ref() et al " Ævar Arnfjörð Bjarmason
` (11 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
Change the refs_resolve_ref_unsafe() invoked in loose_fill_ref_dir()
to a form that ignores errno. The only eventual caller of this
function is create_ref_cache(), whose callers in turn don't have their
failure depend on any errno set here.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs/files-backend.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index be27823d099..1d27f915638 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -280,10 +280,11 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
create_dir_entry(dir->cache, refname.buf,
refname.len));
} else {
- if (!refs_resolve_ref_unsafe(&refs->base,
+ int ignore_errno;
+ if (!refs_werrres_ref_unsafe(&refs->base,
refname.buf,
RESOLVE_REF_READING,
- &oid, &flag)) {
+ &oid, &flag, &ignore_errno)) {
oidclr(&oid);
flag |= REF_ISBROKEN;
} else if (is_null_oid(&oid)) {
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 10/20] refs API: make files_copy_or_rename_ref() et al not set errno
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (8 preceding siblings ...)
2021-10-14 0:06 ` [PATCH 09/20] refs API: make loose_fill_ref_dir() " Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 11/20] refs API: ignore errno in worktree.c's add_head_info() Ævar Arnfjörð Bjarmason
` (10 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
None of the callers of rename_ref() and copy_ref() care about errno,
and as seen in the context here we already emit our own non-errno
using error() in the case where we'd use it.
So let's have it explicitly ignore errno, and do the same in
commit_ref_update(), which is only used within other code in
files_copy_or_rename_ref() itself which doesn't care about errno
either.
It might actually be sensible to have the callers use errno if the
failure was filesystem-specific, and with the upcoming reftable
backend we don't want to rely on that sort of thing, so let's keep
ignoring that for now.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs/files-backend.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1d27f915638..b3d4544dcf0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1410,9 +1410,9 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
goto out;
}
- if (!refs_resolve_ref_unsafe(&refs->base, oldrefname,
+ if (!refs_werrres_ref_unsafe(&refs->base, oldrefname,
RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
- &orig_oid, &flag)) {
+ &orig_oid, &flag, &ignore_errno)) {
ret = error("refname %s not found", oldrefname);
goto out;
}
@@ -1823,10 +1823,12 @@ static int commit_ref_update(struct files_ref_store *refs,
*/
int head_flag;
const char *head_ref;
+ int ignore_errno;
- head_ref = refs_resolve_ref_unsafe(&refs->base, "HEAD",
+ head_ref = refs_werrres_ref_unsafe(&refs->base, "HEAD",
RESOLVE_REF_READING,
- NULL, &head_flag);
+ NULL, &head_flag,
+ &ignore_errno);
if (head_ref && (head_flag & REF_ISSYMREF) &&
!strcmp(head_ref, lock->ref_name)) {
struct strbuf log_err = STRBUF_INIT;
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 11/20] refs API: ignore errno in worktree.c's add_head_info()
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (9 preceding siblings ...)
2021-10-14 0:06 ` [PATCH 10/20] refs API: make files_copy_or_rename_ref() et al " Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 12/20] refs API: ignore errno in worktree.c's find_shared_symref() Ævar Arnfjörð Bjarmason
` (9 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
The static add_head_info() function is only used indirectly by callers
of get_worktrees(), none of whom care about errno, and even if they
did having the faked-up one from refs_resolve_ref_unsafe() would only
confuse them if they used die_errno() et al. So let's explicitly
ignore it here.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
worktree.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/worktree.c b/worktree.c
index cfffcdb62b3..fa988ee978f 100644
--- a/worktree.c
+++ b/worktree.c
@@ -28,11 +28,13 @@ static void add_head_info(struct worktree *wt)
{
int flags;
const char *target;
+ int ignore_errno;
- target = refs_resolve_ref_unsafe(get_worktree_ref_store(wt),
+ target = refs_werrres_ref_unsafe(get_worktree_ref_store(wt),
"HEAD",
0,
- &wt->head_oid, &flags);
+ &wt->head_oid, &flags,
+ &ignore_errno);
if (!target)
return;
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 12/20] refs API: ignore errno in worktree.c's find_shared_symref()
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (10 preceding siblings ...)
2021-10-14 0:06 ` [PATCH 11/20] refs API: ignore errno in worktree.c's add_head_info() Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 13/20] refs tests: ignore ignore errno in test-ref-store helper Ævar Arnfjörð Bjarmason
` (8 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
There are only handful of callers of find_shared_symref(), none of
whom care about errno, so let's migrate to the non-errno-propagating
version of refs_resolve_ref_unsafe() and explicitly ignore errno here.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
worktree.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/worktree.c b/worktree.c
index fa988ee978f..7d7cf058154 100644
--- a/worktree.c
+++ b/worktree.c
@@ -420,6 +420,7 @@ const struct worktree *find_shared_symref(const char *symref,
const char *symref_target;
struct ref_store *refs;
int flags;
+ int ignore_errno;
if (wt->is_bare)
continue;
@@ -436,8 +437,9 @@ const struct worktree *find_shared_symref(const char *symref,
}
refs = get_worktree_ref_store(wt);
- symref_target = refs_resolve_ref_unsafe(refs, symref, 0,
- NULL, &flags);
+ symref_target = refs_werrres_ref_unsafe(refs, symref, 0,
+ NULL, &flags,
+ &ignore_errno);
if ((flags & REF_ISSYMREF) &&
symref_target && !strcmp(symref_target, target)) {
existing = wt;
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 13/20] refs tests: ignore ignore errno in test-ref-store helper
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (11 preceding siblings ...)
2021-10-14 0:06 ` [PATCH 12/20] refs API: ignore errno in worktree.c's find_shared_symref() Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 14/20] refs API: make refs_resolve_refdup() not set errno Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
The cmd_resolve_ref() function has always ignored errno on failure,
but let's do so explicitly when using the refs_resolve_ref_unsafe()
function.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/helper/test-ref-store.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index b314b81a45b..2f91fb9b227 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -123,9 +123,10 @@ static int cmd_resolve_ref(struct ref_store *refs, const char **argv)
int resolve_flags = arg_flags(*argv++, "resolve-flags");
int flags;
const char *ref;
+ int ignore_errno;
- ref = refs_resolve_ref_unsafe(refs, refname, resolve_flags,
- &oid, &flags);
+ ref = refs_werrres_ref_unsafe(refs, refname, resolve_flags,
+ &oid, &flags, &ignore_errno);
printf("%s %s 0x%x\n", oid_to_hex(&oid), ref ? ref : "(null)", flags);
return ref ? 0 : 1;
}
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 14/20] refs API: make refs_resolve_refdup() not set errno
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (12 preceding siblings ...)
2021-10-14 0:06 ` [PATCH 13/20] refs tests: ignore ignore errno in test-ref-store helper Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 15/20] refs API: make refs_ref_exists() " Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
Move refs_resolve_refdup() from the legacy refs_resolve_ref_unsafe()
to the new refs_werrres_ref_unsafe(). I have read its callers and
determined that they don't care about errno being set.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 001f54845a7..c64ed6285a6 100644
--- a/refs.c
+++ b/refs.c
@@ -267,9 +267,10 @@ char *refs_resolve_refdup(struct ref_store *refs,
struct object_id *oid, int *flags)
{
const char *result;
+ int ignore_errno;
- result = refs_resolve_ref_unsafe(refs, refname, resolve_flags,
- oid, flags);
+ result = refs_werrres_ref_unsafe(refs, refname, resolve_flags,
+ oid, flags, &ignore_errno);
return xstrdup_or_null(result);
}
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 15/20] refs API: make refs_ref_exists() not set errno
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (13 preceding siblings ...)
2021-10-14 0:06 ` [PATCH 14/20] refs API: make refs_resolve_refdup() not set errno Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 16/20] refs API: make resolve_ref_unsafe() " Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
Move refs_ref_exists from the legacy refs_resolve_ref_unsafe() to the
new refs_werrres_ref_unsafe(). I have read its callers and determined
that they don't care about errno being set, in particular:
git grep -W -w -e refs_ref_exists -e ref_exists
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index c64ed6285a6..f2a56a216c3 100644
--- a/refs.c
+++ b/refs.c
@@ -308,7 +308,9 @@ int read_ref(const char *refname, struct object_id *oid)
int refs_ref_exists(struct ref_store *refs, const char *refname)
{
- return !!refs_resolve_ref_unsafe(refs, refname, RESOLVE_REF_READING, NULL, NULL);
+ int ignore_errno;
+ return !!refs_werrres_ref_unsafe(refs, refname, RESOLVE_REF_READING,
+ NULL, NULL, &ignore_errno);
}
int ref_exists(const char *refname)
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 16/20] refs API: make resolve_ref_unsafe() not set errno
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (14 preceding siblings ...)
2021-10-14 0:06 ` [PATCH 15/20] refs API: make refs_ref_exists() " Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 17/20] refs API: make expand_ref() & repo_dwim_log() " Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
Change the resolve_ref_unsafe() wrapper function to use the underlying
refs_werrres_ref_unsafe() directly.
From a reading of the callers I determined that the only one who cared
about errno was a sequencer.c caller added in e47c6cafcb5 (commit:
move print_commit_summary() to libgit, 2017-11-24), I'm migrating it
to using refs_werrres_ref_unsafe() directly.
This adds another "set errno" instance, but in this case it's OK and
idiomatic. We are setting it just before calling die_errno(). We could
have some hypothetical die_errno_var(&saved_errno, ...) here, but I
don't think it's worth it. The problem with errno is subtle action at
distance, not this sort of thing. We already use this pattern in a
couple of places in wrapper.c
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 6 ++++--
sequencer.c | 10 ++++++++--
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index f2a56a216c3..4b0c1a30a18 100644
--- a/refs.c
+++ b/refs.c
@@ -1784,8 +1784,10 @@ int refs_init_db(struct strbuf *err)
const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
struct object_id *oid, int *flags)
{
- return refs_resolve_ref_unsafe(get_main_ref_store(the_repository), refname,
- resolve_flags, oid, flags);
+ int ignore_errno;
+
+ return refs_werrres_ref_unsafe(get_main_ref_store(the_repository), refname,
+ resolve_flags, oid, flags, &ignore_errno);
}
int resolve_gitlink_ref(const char *submodule, const char *refname,
diff --git a/sequencer.c b/sequencer.c
index 64b1f2e681c..7052f791410 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1281,6 +1281,8 @@ void print_commit_summary(struct repository *r,
struct pretty_print_context pctx = {0};
struct strbuf author_ident = STRBUF_INIT;
struct strbuf committer_ident = STRBUF_INIT;
+ struct ref_store *refs;
+ int resolve_errno;
commit = lookup_commit(r, oid);
if (!commit)
@@ -1330,9 +1332,13 @@ void print_commit_summary(struct repository *r,
rev.diffopt.break_opt = 0;
diff_setup_done(&rev.diffopt);
- head = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
- if (!head)
+ refs = get_main_ref_store(the_repository);
+ head = refs_werrres_ref_unsafe(refs, "HEAD", 0, NULL, NULL,
+ &resolve_errno);
+ if (!head) {
+ errno = resolve_errno;
die_errno(_("unable to resolve HEAD after creating commit"));
+ }
if (!strcmp(head, "HEAD"))
head = _("detached HEAD");
else
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 17/20] refs API: make expand_ref() & repo_dwim_log() not set errno
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (15 preceding siblings ...)
2021-10-14 0:06 ` [PATCH 16/20] refs API: make resolve_ref_unsafe() " Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 18/20] refs API: don't expose "errno" in run_transaction_hook() Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
The use of these two is rather trivial, and it's easy to see none of
their callers care about errno. So let's move them from
refs_resolve_ref_unsafe() to refs_resolve_ref_unsafe_with_errno(),
these were the last two callers, so we can get rid of that wrapper
function.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index 4b0c1a30a18..16f8220b108 100644
--- a/refs.c
+++ b/refs.c
@@ -653,13 +653,16 @@ int expand_ref(struct repository *repo, const char *str, int len,
struct object_id oid_from_ref;
struct object_id *this_result;
int flag;
+ struct ref_store *refs = get_main_ref_store(repo);
+ int ignore_errno;
this_result = refs_found ? &oid_from_ref : oid;
strbuf_reset(&fullref);
strbuf_addf(&fullref, *p, len, str);
- r = refs_resolve_ref_unsafe(get_main_ref_store(repo),
- fullref.buf, RESOLVE_REF_READING,
- this_result, &flag);
+ r = refs_werrres_ref_unsafe(refs, fullref.buf,
+ RESOLVE_REF_READING,
+ this_result, &flag,
+ &ignore_errno);
if (r) {
if (!refs_found++)
*ref = xstrdup(r);
@@ -688,12 +691,14 @@ int repo_dwim_log(struct repository *r, const char *str, int len,
for (p = ref_rev_parse_rules; *p; p++) {
struct object_id hash;
const char *ref, *it;
+ int ignore_errno;
strbuf_reset(&path);
strbuf_addf(&path, *p, len, str);
- ref = refs_resolve_ref_unsafe(refs, path.buf,
+ ref = refs_werrres_ref_unsafe(refs, path.buf,
RESOLVE_REF_READING,
- oid ? &hash : NULL, NULL);
+ oid ? &hash : NULL, NULL,
+ &ignore_errno);
if (!ref)
continue;
if (refs_reflog_exists(refs, path.buf))
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 18/20] refs API: don't expose "errno" in run_transaction_hook()
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (16 preceding siblings ...)
2021-10-14 0:06 ` [PATCH 17/20] refs API: make expand_ref() & repo_dwim_log() " Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 19/20] refs API: post-migration API renaming [1/2] Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
In run_transaction_hook() we've checked errno since 67541597670 (refs:
implement reference transaction hook, 2020-06-19), let's reset errno
afterwards to make sure nobody using refs.c directly or indirectly
relies on it.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 16f8220b108..9aa41b55c7b 100644
--- a/refs.c
+++ b/refs.c
@@ -2095,8 +2095,11 @@ static int run_transaction_hook(struct ref_transaction *transaction,
update->refname);
if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
- if (errno != EPIPE)
+ if (errno != EPIPE) {
+ /* Don't leak errno outside this API */
+ errno = 0;
ret = -1;
+ }
break;
}
}
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 19/20] refs API: post-migration API renaming [1/2]
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (17 preceding siblings ...)
2021-10-14 0:06 ` [PATCH 18/20] refs API: don't expose "errno" in run_transaction_hook() Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 20/20] refs API: post-migration API renaming [2/2] Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
In preceding commits all callers of refs_resolve_ref_unsafe() were
migrated to the transitory refs_werrres_ref_unsafe() function.
As a first step in getting rid of it let's remove the old function
from the public API (it went unused in a preceding commit).
We then provide both a coccinelle rule to do the rename, and a macro
to avoid breaking the existing callers.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
contrib/coccinelle/refs.pending.cocci | 5 +++++
refs.c | 15 +--------------
refs.h | 22 +++++++++-------------
3 files changed, 15 insertions(+), 27 deletions(-)
create mode 100644 contrib/coccinelle/refs.pending.cocci
diff --git a/contrib/coccinelle/refs.pending.cocci b/contrib/coccinelle/refs.pending.cocci
new file mode 100644
index 00000000000..b33cb8a12aa
--- /dev/null
+++ b/contrib/coccinelle/refs.pending.cocci
@@ -0,0 +1,5 @@
+@@
+expression refs, refname, resolve_flags, oid, flags, failure_errno;
+@@
+- refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)
++ refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)
diff --git a/refs.c b/refs.c
index 9aa41b55c7b..a19f156b66a 100644
--- a/refs.c
+++ b/refs.c
@@ -1668,7 +1668,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
type, failure_errno);
}
-const char *refs_werrres_ref_unsafe(struct ref_store *refs,
+const char *refs_resolve_ref_unsafe(struct ref_store *refs,
const char *refname,
int resolve_flags,
struct object_id *oid,
@@ -1765,19 +1765,6 @@ const char *refs_werrres_ref_unsafe(struct ref_store *refs,
return NULL;
}
-const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname,
- int resolve_flags, struct object_id *oid,
- int *flags)
-{
- int failure_errno = 0;
- const char *refn;
- refn = refs_werrres_ref_unsafe(refs, refname, resolve_flags,
- oid, flags, &failure_errno);
- if (!refn)
- errno = failure_errno;
- return refn;
-}
-
/* backend functions */
int refs_init_db(struct strbuf *err)
{
diff --git a/refs.h b/refs.h
index 3938f99c902..d908a161c06 100644
--- a/refs.h
+++ b/refs.h
@@ -11,18 +11,6 @@ struct string_list;
struct string_list_item;
struct worktree;
-/*
- * Callers should not inspect "errno" on failure, but rather pass in a
- * "failure_errno" parameter, on failure the "errno" will indicate the
- * type of failure encountered, but not necessarily one that came from
- * a syscall. We might have faked it up.
- */
-const char *refs_werrres_ref_unsafe(struct ref_store *refs,
- const char *refname,
- int resolve_flags,
- struct object_id *oid,
- int *flags, int *failure_errno);
-
/*
* Resolve a reference, recursively following symbolic refererences.
*
@@ -70,16 +58,24 @@ const char *refs_werrres_ref_unsafe(struct ref_store *refs,
* resolved. The function returns NULL for such ref names.
* Caps and underscores refers to the special refs, such as HEAD,
* FETCH_HEAD and friends, that all live outside of the refs/ directory.
+ *
+ * Callers should not inspect "errno" on failure, but rather pass in a
+ * "failure_errno" parameter, on failure the "errno" will indicate the
+ * type of failure encountered, but not necessarily one that came from
+ * a syscall. We might have faked it up.
*/
#define RESOLVE_REF_READING 0x01
#define RESOLVE_REF_NO_RECURSE 0x02
#define RESOLVE_REF_ALLOW_BAD_NAME 0x04
+#define refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno) \
+ refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)
const char *refs_resolve_ref_unsafe(struct ref_store *refs,
const char *refname,
int resolve_flags,
struct object_id *oid,
- int *flags);
+ int *flags, int *failure_errno);
+
const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
struct object_id *oid, int *flags);
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH 20/20] refs API: post-migration API renaming [2/2]
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (18 preceding siblings ...)
2021-10-14 0:06 ` [PATCH 19/20] refs API: post-migration API renaming [1/2] Ævar Arnfjörð Bjarmason
@ 2021-10-14 0:06 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-14 0:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
Rename the transitory refs_werrres_ref_unsafe() function to
refs_resolve_ref_unsafe(), now that all callers of the old function
have learned to pass in a "failure_errno" parameter.
The coccinelle semantic patch added in the preceding commit works, but
I couldn't figure out how to get spatch(1) to re-flow these argument
lists (and sometimes make lines way too long), so this rename was done
with:
perl -pi -e 's/refs_werrres_ref_unsafe/refs_resolve_ref_unsafe/g' \
$(git grep -l refs_werrres_ref_unsafe -- '*.c')
But after that "make contrib/coccinelle/refs.cocci.patch" comes up
empty, so the result would have been the same. Let's remove that
transitory semantic patch file, we won't need to retain it for any
other in-flight changes, refs_werrres_ref_unsafe() only existed within
this patch series.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
contrib/coccinelle/refs.pending.cocci | 5 -----
refs.c | 16 ++++++++--------
refs.h | 2 --
refs/files-backend.c | 16 ++++++++--------
sequencer.c | 2 +-
t/helper/test-ref-store.c | 2 +-
worktree.c | 6 +++---
7 files changed, 21 insertions(+), 28 deletions(-)
delete mode 100644 contrib/coccinelle/refs.pending.cocci
diff --git a/contrib/coccinelle/refs.pending.cocci b/contrib/coccinelle/refs.pending.cocci
deleted file mode 100644
index b33cb8a12aa..00000000000
--- a/contrib/coccinelle/refs.pending.cocci
+++ /dev/null
@@ -1,5 +0,0 @@
-@@
-expression refs, refname, resolve_flags, oid, flags, failure_errno;
-@@
-- refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)
-+ refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)
diff --git a/refs.c b/refs.c
index a19f156b66a..75c6eafa847 100644
--- a/refs.c
+++ b/refs.c
@@ -269,7 +269,7 @@ char *refs_resolve_refdup(struct ref_store *refs,
const char *result;
int ignore_errno;
- result = refs_werrres_ref_unsafe(refs, refname, resolve_flags,
+ result = refs_resolve_ref_unsafe(refs, refname, resolve_flags,
oid, flags, &ignore_errno);
return xstrdup_or_null(result);
}
@@ -295,7 +295,7 @@ int read_ref_full(const char *refname, int resolve_flags, struct object_id *oid,
int ignore_errno;
struct ref_store *refs = get_main_ref_store(the_repository);
- if (refs_werrres_ref_unsafe(refs, refname, resolve_flags,
+ if (refs_resolve_ref_unsafe(refs, refname, resolve_flags,
oid, flags, &ignore_errno))
return 0;
return -1;
@@ -309,7 +309,7 @@ int read_ref(const char *refname, struct object_id *oid)
int refs_ref_exists(struct ref_store *refs, const char *refname)
{
int ignore_errno;
- return !!refs_werrres_ref_unsafe(refs, refname, RESOLVE_REF_READING,
+ return !!refs_resolve_ref_unsafe(refs, refname, RESOLVE_REF_READING,
NULL, NULL, &ignore_errno);
}
@@ -659,7 +659,7 @@ int expand_ref(struct repository *repo, const char *str, int len,
this_result = refs_found ? &oid_from_ref : oid;
strbuf_reset(&fullref);
strbuf_addf(&fullref, *p, len, str);
- r = refs_werrres_ref_unsafe(refs, fullref.buf,
+ r = refs_resolve_ref_unsafe(refs, fullref.buf,
RESOLVE_REF_READING,
this_result, &flag,
&ignore_errno);
@@ -695,7 +695,7 @@ int repo_dwim_log(struct repository *r, const char *str, int len,
strbuf_reset(&path);
strbuf_addf(&path, *p, len, str);
- ref = refs_werrres_ref_unsafe(refs, path.buf,
+ ref = refs_resolve_ref_unsafe(refs, path.buf,
RESOLVE_REF_READING,
oid ? &hash : NULL, NULL,
&ignore_errno);
@@ -1382,7 +1382,7 @@ int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
int flag;
int ignore_errno;
- if (refs_werrres_ref_unsafe(refs, "HEAD", RESOLVE_REF_READING,
+ if (refs_resolve_ref_unsafe(refs, "HEAD", RESOLVE_REF_READING,
&oid, &flag, &ignore_errno))
return fn("HEAD", &oid, flag, cb_data);
@@ -1778,7 +1778,7 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
{
int ignore_errno;
- return refs_werrres_ref_unsafe(get_main_ref_store(the_repository), refname,
+ return refs_resolve_ref_unsafe(get_main_ref_store(the_repository), refname,
resolve_flags, oid, flags, &ignore_errno);
}
@@ -1794,7 +1794,7 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,
if (!refs)
return -1;
- if (!refs_werrres_ref_unsafe(refs, refname, 0, oid, &flags,
+ if (!refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags,
&ignore_errno) || is_null_oid(oid))
return -1;
return 0;
diff --git a/refs.h b/refs.h
index d908a161c06..45c34e99e3a 100644
--- a/refs.h
+++ b/refs.h
@@ -68,8 +68,6 @@ struct worktree;
#define RESOLVE_REF_NO_RECURSE 0x02
#define RESOLVE_REF_ALLOW_BAD_NAME 0x04
-#define refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno) \
- refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)
const char *refs_resolve_ref_unsafe(struct ref_store *refs,
const char *refname,
int resolve_flags,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index b3d4544dcf0..e9d754d5ad3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -281,7 +281,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
refname.len));
} else {
int ignore_errno;
- if (!refs_werrres_ref_unsafe(&refs->base,
+ if (!refs_resolve_ref_unsafe(&refs->base,
refname.buf,
RESOLVE_REF_READING,
&oid, &flag, &ignore_errno)) {
@@ -1034,7 +1034,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
goto error_return;
}
- if (!refs_werrres_ref_unsafe(&refs->base, lock->ref_name, 0,
+ if (!refs_resolve_ref_unsafe(&refs->base, lock->ref_name, 0,
&lock->old_oid, NULL, &ignore_errno))
oidclr(&lock->old_oid);
goto out;
@@ -1410,7 +1410,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
goto out;
}
- if (!refs_werrres_ref_unsafe(&refs->base, oldrefname,
+ if (!refs_resolve_ref_unsafe(&refs->base, oldrefname,
RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
&orig_oid, &flag, &ignore_errno)) {
ret = error("refname %s not found", oldrefname);
@@ -1456,7 +1456,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
* the safety anyway; we want to delete the reference whatever
* its current value.
*/
- if (!copy && refs_werrres_ref_unsafe(&refs->base, newrefname,
+ if (!copy && refs_resolve_ref_unsafe(&refs->base, newrefname,
RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
NULL, NULL, &ignore_errno) &&
refs_delete_ref(&refs->base, NULL, newrefname,
@@ -1825,7 +1825,7 @@ static int commit_ref_update(struct files_ref_store *refs,
const char *head_ref;
int ignore_errno;
- head_ref = refs_werrres_ref_unsafe(&refs->base, "HEAD",
+ head_ref = refs_resolve_ref_unsafe(&refs->base, "HEAD",
RESOLVE_REF_READING,
NULL, &head_flag,
&ignore_errno);
@@ -1875,7 +1875,7 @@ static void update_symref_reflog(struct files_ref_store *refs,
int ignore_errno;
if (logmsg &&
- refs_werrres_ref_unsafe(&refs->base, target,
+ refs_resolve_ref_unsafe(&refs->base, target,
RESOLVE_REF_READING, &new_oid, NULL,
&ignore_errno) &&
files_log_ref_write(refs, refname, &lock->old_oid,
@@ -2163,7 +2163,7 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
if (ends_with(diter->basename, ".lock"))
continue;
- if (!refs_werrres_ref_unsafe(iter->ref_store,
+ if (!refs_resolve_ref_unsafe(iter->ref_store,
diter->relative_path, 0,
&iter->oid, &flags,
&ignore_errno)) {
@@ -2511,7 +2511,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
* to record and possibly check old_oid:
*/
int ignore_errno;
- if (!refs_werrres_ref_unsafe(&refs->base,
+ if (!refs_resolve_ref_unsafe(&refs->base,
referent.buf, 0,
&lock->old_oid, NULL,
&ignore_errno)) {
diff --git a/sequencer.c b/sequencer.c
index 7052f791410..d848c5dc985 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1333,7 +1333,7 @@ void print_commit_summary(struct repository *r,
diff_setup_done(&rev.diffopt);
refs = get_main_ref_store(the_repository);
- head = refs_werrres_ref_unsafe(refs, "HEAD", 0, NULL, NULL,
+ head = refs_resolve_ref_unsafe(refs, "HEAD", 0, NULL, NULL,
&resolve_errno);
if (!head) {
errno = resolve_errno;
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 2f91fb9b227..3986665037a 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -125,7 +125,7 @@ static int cmd_resolve_ref(struct ref_store *refs, const char **argv)
const char *ref;
int ignore_errno;
- ref = refs_werrres_ref_unsafe(refs, refname, resolve_flags,
+ ref = refs_resolve_ref_unsafe(refs, refname, resolve_flags,
&oid, &flags, &ignore_errno);
printf("%s %s 0x%x\n", oid_to_hex(&oid), ref ? ref : "(null)", flags);
return ref ? 0 : 1;
diff --git a/worktree.c b/worktree.c
index 7d7cf058154..2c155b10150 100644
--- a/worktree.c
+++ b/worktree.c
@@ -30,7 +30,7 @@ static void add_head_info(struct worktree *wt)
const char *target;
int ignore_errno;
- target = refs_werrres_ref_unsafe(get_worktree_ref_store(wt),
+ target = refs_resolve_ref_unsafe(get_worktree_ref_store(wt),
"HEAD",
0,
&wt->head_oid, &flags,
@@ -437,7 +437,7 @@ const struct worktree *find_shared_symref(const char *symref,
}
refs = get_worktree_ref_store(wt);
- symref_target = refs_werrres_ref_unsafe(refs, symref, 0,
+ symref_target = refs_resolve_ref_unsafe(refs, symref, 0,
NULL, &flags,
&ignore_errno);
if ((flags & REF_ISSYMREF) &&
@@ -574,7 +574,7 @@ int other_head_refs(each_ref_fn fn, void *cb_data)
strbuf_reset(&refname);
strbuf_worktree_ref(wt, &refname, "HEAD");
- if (refs_werrres_ref_unsafe(get_main_ref_store(the_repository),
+ if (refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
refname.buf,
RESOLVE_REF_READING,
&oid, &flag, &ignore_errno))
--
2.33.1.1346.g48288c3c089
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH 01/20] branch tests: test for errno propagating on failing read
2021-10-14 0:06 ` [PATCH 01/20] branch tests: test for errno propagating on failing read Ævar Arnfjörð Bjarmason
@ 2021-10-14 1:57 ` Eric Sunshine
0 siblings, 0 replies; 45+ messages in thread
From: Eric Sunshine @ 2021-10-14 1:57 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git List, Junio C Hamano, Jeff King, Han-Wen Nienhuys
On Wed, Oct 13, 2021 at 8:07 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Add a test for "git branch" to cover the case where .git/refs is
> symlinked. To check availability, refs_verify_refname_available() will
> run refs_read_raw_ref() on each prefix, leading to a read() from
> .git/refs (which is a directory).
>
> It would probably be more robust to re-issue the lstat() as a normal
> stat(), in which case, we would fall back to the directory case, but
> for now let's just test for the existing behavior as-is. This test
> covers a regression in a commit that only ever made it to "next", see
> [1].
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> @@ -731,6 +731,26 @@ test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog for
> +test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '
> + test_when_finished "rm -rf subdir" &&
> + git init subdir &&
> +
> + (
> + cd subdir &&
> + for d in refs objects packed-refs
> + do
> + rm -rf .git/$d &&
> + ln -s ../../.git/$d .git/$d
> + done
> + ) &&
Amend the last line of the loop body:
ln -s ../../.git/$d .git/$d || exit 1
> + git --git-dir subdir/.git/ branch rename-src &&
> + git rev-parse rename-src >expect &&
> + git --git-dir subdir/.git/ branch -m rename-src rename-dest &&
> + git rev-parse rename-dest >actual &&
> + test_cmp expect actual &&
> + git branch -D rename-dest
> +'
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 06/20] refs/files: remove "name exist?" check in lock_ref_oid_basic()
2021-10-14 0:06 ` [PATCH 06/20] refs/files: remove "name exist?" check in lock_ref_oid_basic() Ævar Arnfjörð Bjarmason
@ 2021-10-14 20:53 ` Junio C Hamano
0 siblings, 0 replies; 45+ messages in thread
From: Junio C Hamano @ 2021-10-14 20:53 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King, Han-Wen Nienhuys
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 0af6ee44552..0dd21b2e205 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1013,16 +1013,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
> CALLOC_ARRAY(lock, 1);
>
> files_ref_path(refs, &ref_file, refname);
> - if (!refs_resolve_ref_unsafe(&refs->base, refname,
> - RESOLVE_REF_NO_RECURSE,
> - &lock->old_oid, type)) {
> - if (!refs_verify_refname_available(&refs->base, refname,
> - NULL, NULL, err))
> - strbuf_addf(err, "unable to resolve reference '%s': %s",
> - refname, strerror(errno));
> -
> - goto error_return;
> - }
>
> /*
> * If the ref did not exist and we are creating it, make sure
This has the side effect of "type" not touched at all by this
function, which in turn triggers "type is used uninitialized?"
error in files_reflog_expire() that calls this function.
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v2 00/21] refs: stop having the API set "errno"
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (19 preceding siblings ...)
2021-10-14 0:06 ` [PATCH 20/20] refs API: post-migration API renaming [2/2] Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 01/21] branch tests: test for errno propagating on failing read Ævar Arnfjörð Bjarmason
` (20 more replies)
20 siblings, 21 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
See [1] for the v1 summary. There was a regression there with "git
reflog"'s "--updateref" option, which had no meaningful tests in the
test suite (only one test in "t1503-rev-parse-verify.sh"). Test for
it, and address Eric's comment about "t3200-branch.sh" by
re-structuring that test away from a for-loop.
1. https://lore.kernel.org/git/cover-00.20-00000000000-20211013T235900Z-avarab@gmail.com/
Han-Wen Nienhuys (3):
branch tests: test for errno propagating on failing read
refs API: make refs_read_raw_ref() not set errno
refs API: make parse_loose_ref_contents() not set errno
Ævar Arnfjörð Bjarmason (18):
refs API: add a version of refs_resolve_ref_unsafe() with "errno"
refs API: make refs_rename_ref_available() static
reflog tests: add --updateref tests
refs/files: remove "name exist?" check in lock_ref_oid_basic()
refs API: remove refs_read_ref_full() wrapper
refs API: make resolve_gitlink_ref() not set errno
refs API: make loose_fill_ref_dir() not set errno
refs API: make files_copy_or_rename_ref() et al not set errno
refs API: ignore errno in worktree.c's add_head_info()
refs API: ignore errno in worktree.c's find_shared_symref()
refs tests: ignore ignore errno in test-ref-store helper
refs API: make refs_resolve_refdup() not set errno
refs API: make refs_ref_exists() not set errno
refs API: make resolve_ref_unsafe() not set errno
refs API: make expand_ref() & repo_dwim_log() not set errno
refs API: don't expose "errno" in run_transaction_hook()
refs API: post-migration API renaming [1/2]
refs API: post-migration API renaming [2/2]
refs.c | 124 +++++++++++++++--------------
refs.h | 10 ++-
refs/files-backend.c | 153 ++++++++++++++++++++++++------------
refs/packed-backend.c | 7 +-
refs/refs-internal.h | 26 ++----
sequencer.c | 10 ++-
t/helper/test-ref-store.c | 3 +-
t/t1417-reflog-updateref.sh | 65 +++++++++++++++
t/t3200-branch.sh | 22 ++++++
worktree.c | 17 ++--
10 files changed, 294 insertions(+), 143 deletions(-)
create mode 100755 t/t1417-reflog-updateref.sh
Range-diff against v1:
1: bea88e382c0 ! 1: 1863c597c98 branch tests: test for errno propagating on failing read
@@ t/t3200-branch.sh: test_expect_success SYMLINKS 'git branch -m u v should fail w
+test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '
+ test_when_finished "rm -rf subdir" &&
-+ git init subdir &&
++ git init --bare subdir &&
+
-+ (
-+ cd subdir &&
-+ for d in refs objects packed-refs
-+ do
-+ rm -rf .git/$d &&
-+ ln -s ../../.git/$d .git/$d
-+ done
-+ ) &&
-+ git --git-dir subdir/.git/ branch rename-src &&
++ rm -rfv subdir/refs subdir/objects subdir/packed-refs &&
++ ln -s ../.git/refs subdir/refs &&
++ ln -s ../.git/objects subdir/objects &&
++ ln -s ../.git/packed-refs subdir/packed-refs &&
++
++ git -C subdir rev-parse --absolute-git-dir >subdir.dir &&
++ git rev-parse --absolute-git-dir >our.dir &&
++ ! test_cmp subdir.dir our.dir &&
++
++ git -C subdir log &&
++ git -C subdir branch rename-src &&
+ git rev-parse rename-src >expect &&
-+ git --git-dir subdir/.git/ branch -m rename-src rename-dest &&
++ git -C subdir branch -m rename-src rename-dest &&
+ git rev-parse rename-dest >actual &&
+ test_cmp expect actual &&
+ git branch -D rename-dest
2: 46641111885 = 2: f6d784b4979 refs API: add a version of refs_resolve_ref_unsafe() with "errno"
3: a1a80715ffe = 3: 8932b109087 refs API: make refs_read_raw_ref() not set errno
4: 758c761abcf ! 4: 988bf62b3f2 refs API: make parse_loose_ref_contents() not set errno
@@ refs/refs-internal.h: struct ref_store {
/*
* Fill in the generic part of refs and add it to our collection of
-
- ## t/t3200-branch.sh ##
-@@ t/t3200-branch.sh: test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '
- ) &&
- git --git-dir subdir/.git/ branch rename-src &&
- git rev-parse rename-src >expect &&
-+ # Tests a BUG() assertion in files_read_raw_ref()
- git --git-dir subdir/.git/ branch -m rename-src rename-dest &&
- git rev-parse rename-dest >actual &&
- test_cmp expect actual &&
5: cdf17fa34a4 = 5: 8073f93a904 refs API: make refs_rename_ref_available() static
-: ----------- > 6: d3242f5f687 reflog tests: add --updateref tests
6: 3162bf28505 ! 7: 7db3b446632 refs/files: remove "name exist?" check in lock_ref_oid_basic()
@@ Commit message
already, or something we're about to discover by trying to lock the
ref with raceproof_create_file().
+ The one exception is the caller in files_reflog_expire(), who passes
+ us a "type" to find out if the reference is a symref or not. We can
+ move the that logic over to that caller, which can now defer its
+ discovery of whether or not the ref is a symref until it's needed. In
+ the preceding commit an exhaustive regression test was added for that
+ case in a new test in "t1417-reflog-updateref.sh".
+
The improved diagnostics here were added in
5b2d8d6f218 (lock_ref_sha1_basic(): improve diagnostics for ref D/F
conflicts, 2015-05-11), and then much of the surrounding code went
@@ Commit message
reflog we're expiring exists.
See ae35e16cd43 (reflog expire: don't lock reflogs using previously
- seen OID, 2021-08-23) for the current state of that code.
+ seen OID, 2021-08-23) for the current state of that code, and
+ 5e6f003ca8a (reflog_expire(): ignore --updateref for symbolic
+ references, 2015-03-03) for the code we'd break if we only did a
+ "update = !!ref" here, which is covered by the aforementioned
+ regression test in "t1417-reflog-updateref.sh".
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## refs/files-backend.c ##
+@@ refs/files-backend.c: static int create_reflock(const char *path, void *cb)
+ * Locks a ref returning the lock on success and NULL on failure.
+ */
+ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
+- const char *refname, int *type,
++ const char *refname,
+ struct strbuf *err)
+ {
+ struct strbuf ref_file = STRBUF_INIT;
@@ refs/files-backend.c: static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
CALLOC_ARRAY(lock, 1);
@@ refs/files-backend.c: static int commit_ref_update(struct files_ref_store *refs,
*/
static int refs_rename_ref_available(struct ref_store *refs,
const char *old_refname,
+@@ refs/files-backend.c: static int files_copy_or_rename_ref(struct ref_store *ref_store,
+
+ logmoved = log;
+
+- lock = lock_ref_oid_basic(refs, newrefname, NULL, &err);
++ lock = lock_ref_oid_basic(refs, newrefname, &err);
+ if (!lock) {
+ if (copy)
+ error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf);
+@@ refs/files-backend.c: static int files_copy_or_rename_ref(struct ref_store *ref_store,
+ goto out;
+
+ rollback:
+- lock = lock_ref_oid_basic(refs, oldrefname, NULL, &err);
++ lock = lock_ref_oid_basic(refs, oldrefname, &err);
+ if (!lock) {
+ error("unable to lock %s for rollback: %s", oldrefname, err.buf);
+ strbuf_release(&err);
+@@ refs/files-backend.c: static int files_create_symref(struct ref_store *ref_store,
+ struct ref_lock *lock;
+ int ret;
+
+- lock = lock_ref_oid_basic(refs, refname, NULL, &err);
++ lock = lock_ref_oid_basic(refs, refname, &err);
+ if (!lock) {
+ error("%s", err.buf);
+ strbuf_release(&err);
+@@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,
+ struct strbuf log_file_sb = STRBUF_INIT;
+ char *log_file;
+ int status = 0;
+- int type;
+ struct strbuf err = STRBUF_INIT;
+ const struct object_id *oid;
+
+@@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,
+ * reference itself, plus we might need to update the
+ * reference if --updateref was specified:
+ */
+- lock = lock_ref_oid_basic(refs, refname, &type, &err);
++ lock = lock_ref_oid_basic(refs, refname, &err);
+ if (!lock) {
+ error("cannot lock ref '%s': %s", refname, err.buf);
+ strbuf_release(&err);
+@@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,
+ * a reference if there are no remaining reflog
+ * entries.
+ */
+- int update = (flags & EXPIRE_REFLOGS_UPDATE_REF) &&
+- !(type & REF_ISSYMREF) &&
+- !is_null_oid(&cb.last_kept_oid);
++ int update = 0;
++
++ if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
++ !is_null_oid(&cb.last_kept_oid)) {
++ int ignore_errno;
++ int type;
++ const char *ref;
++
++ ref = refs_werrres_ref_unsafe(&refs->base, refname,
++ RESOLVE_REF_NO_RECURSE,
++ NULL, &type,
++ &ignore_errno);
++ update = !!(ref && !(type & REF_ISSYMREF));
++ }
+
+ if (close_lock_file_gently(&reflog_lock)) {
+ status |= error("couldn't write %s: %s", log_file,
7: 288237b1900 = 8: b2532614430 refs API: remove refs_read_ref_full() wrapper
8: acb484ea547 = 9: 65beb51ae4e refs API: make resolve_gitlink_ref() not set errno
9: 4be84c9bf53 = 10: c99c59dcc57 refs API: make loose_fill_ref_dir() not set errno
10: 8753210f9cc = 11: 22883846a6a refs API: make files_copy_or_rename_ref() et al not set errno
11: 9fe85926140 = 12: b90e65abd7d refs API: ignore errno in worktree.c's add_head_info()
12: 8d87db98041 = 13: 38f5f54938f refs API: ignore errno in worktree.c's find_shared_symref()
13: 954633bcbb2 = 14: 1289d78e4f3 refs tests: ignore ignore errno in test-ref-store helper
14: fbbc08d3ebd = 15: 152a831b8be refs API: make refs_resolve_refdup() not set errno
15: 4b2a4dbe7d5 = 16: c4e87181121 refs API: make refs_ref_exists() not set errno
16: 888b1884c29 = 17: cae56498b43 refs API: make resolve_ref_unsafe() not set errno
17: e2885f13c9b = 18: 146b030a142 refs API: make expand_ref() & repo_dwim_log() not set errno
18: df50373a272 = 19: 3d630872aff refs API: don't expose "errno" in run_transaction_hook()
19: 4c80b05bf1d = 20: 56f855d32b7 refs API: post-migration API renaming [1/2]
20: 54b18e3a719 ! 21: 0135fb5949f refs API: post-migration API renaming [2/2]
@@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *ref
referent.buf, 0,
&lock->old_oid, NULL,
&ignore_errno)) {
+@@ refs/files-backend.c: static int files_reflog_expire(struct ref_store *ref_store,
+ int type;
+ const char *ref;
+
+- ref = refs_werrres_ref_unsafe(&refs->base, refname,
++ ref = refs_resolve_ref_unsafe(&refs->base, refname,
+ RESOLVE_REF_NO_RECURSE,
+ NULL, &type,
+ &ignore_errno);
## sequencer.c ##
@@ sequencer.c: void print_commit_summary(struct repository *r,
--
2.33.1.1338.g20da966911a
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v2 01/21] branch tests: test for errno propagating on failing read
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 02/21] refs API: add a version of refs_resolve_ref_unsafe() with "errno" Ævar Arnfjörð Bjarmason
` (19 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
From: Han-Wen Nienhuys <hanwen@google.com>
Add a test for "git branch" to cover the case where .git/refs is
symlinked. To check availability, refs_verify_refname_available() will
run refs_read_raw_ref() on each prefix, leading to a read() from
.git/refs (which is a directory).
It would probably be more robust to re-issue the lstat() as a normal
stat(), in which case, we would fall back to the directory case, but
for now let's just test for the existing behavior as-is. This test
covers a regression in a commit that only ever made it to "next", see
[1].
1. http://lore.kernel.org/git/pull.1068.git.git.1629203489546.gitgitgadget@gmail.com
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t3200-branch.sh | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e575ffb4ffb..53e0d094bb4 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -731,6 +731,28 @@ test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog for
test_must_fail git branch -m u v
'
+test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '
+ test_when_finished "rm -rf subdir" &&
+ git init --bare subdir &&
+
+ rm -rfv subdir/refs subdir/objects subdir/packed-refs &&
+ ln -s ../.git/refs subdir/refs &&
+ ln -s ../.git/objects subdir/objects &&
+ ln -s ../.git/packed-refs subdir/packed-refs &&
+
+ git -C subdir rev-parse --absolute-git-dir >subdir.dir &&
+ git rev-parse --absolute-git-dir >our.dir &&
+ ! test_cmp subdir.dir our.dir &&
+
+ git -C subdir log &&
+ git -C subdir branch rename-src &&
+ git rev-parse rename-src >expect &&
+ git -C subdir branch -m rename-src rename-dest &&
+ git rev-parse rename-dest >actual &&
+ test_cmp expect actual &&
+ git branch -D rename-dest
+'
+
test_expect_success 'test tracking setup via --track' '
git config remote.local.url . &&
git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 02/21] refs API: add a version of refs_resolve_ref_unsafe() with "errno"
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 01/21] branch tests: test for errno propagating on failing read Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 03/21] refs API: make refs_read_raw_ref() not set errno Ævar Arnfjörð Bjarmason
` (18 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
Add a new refs_werrres_ref_unsafe() function, which is like
refs_resolve_ref_unsafe() except that it explicitly saves away the
"errno" to a passed-in parameter, the refs_resolve_ref_unsafe() then
becomes a wrapper for it.
In subsequent commits we'll migrate code over to it, before finally
making "refs_resolve_ref_unsafe()" with an "errno" parameter the
canonical version, so this this function exists only so that we can
incrementally migrate callers, it will be going away in a subsequent
commit.
As the added comment notes has a rather tortured name to be the same
length as "refs_resolve_ref_unsafe", to avoid churn as we won't need
to re-indent the argument lists, similarly the documentation and
structure of it in refs.h is designed to minimize a diff in a
subsequent commit, where that documentation will be added to the new
refs_resolve_ref_unsafe().
At the end of this migration the "meaningful errno" TODO item left in
76d70dc0c63 (refs.c: make resolve_ref_unsafe set errno to something
meaningful on error, 2014-06-20) will be resolved.
As can be seen from the use of refs_read_raw_ref() we'll also need to
convert some functions that the new refs_werrres_ref_unsafe() itself
calls to take this "failure_errno". That will be done in subsequent
commits.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 36 +++++++++++++++++++++++++++---------
refs.h | 12 ++++++++++++
2 files changed, 39 insertions(+), 9 deletions(-)
diff --git a/refs.c b/refs.c
index 7f019c2377e..ad56dbb0125 100644
--- a/refs.c
+++ b/refs.c
@@ -1679,17 +1679,19 @@ int refs_read_raw_ref(struct ref_store *ref_store,
type, &errno);
}
-/* This function needs to return a meaningful errno on failure */
-const char *refs_resolve_ref_unsafe(struct ref_store *refs,
+const char *refs_werrres_ref_unsafe(struct ref_store *refs,
const char *refname,
int resolve_flags,
- struct object_id *oid, int *flags)
+ struct object_id *oid,
+ int *flags, int *failure_errno)
{
static struct strbuf sb_refname = STRBUF_INIT;
struct object_id unused_oid;
int unused_flags;
int symref_count;
+ assert(failure_errno);
+
if (!oid)
oid = &unused_oid;
if (!flags)
@@ -1700,7 +1702,7 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
!refname_is_safe(refname)) {
- errno = EINVAL;
+ *failure_errno = EINVAL;
return NULL;
}
@@ -1718,9 +1720,12 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
for (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) {
unsigned int read_flags = 0;
+ errno = 0;
if (refs_read_raw_ref(refs, refname,
oid, &sb_refname, &read_flags)) {
*flags |= read_flags;
+ if (errno)
+ *failure_errno = errno;
/* In reading mode, refs must eventually resolve */
if (resolve_flags & RESOLVE_REF_READING)
@@ -1731,9 +1736,9 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
* may show errors besides ENOENT if there are
* similarly-named refs.
*/
- if (errno != ENOENT &&
- errno != EISDIR &&
- errno != ENOTDIR)
+ if (*failure_errno != ENOENT &&
+ *failure_errno != EISDIR &&
+ *failure_errno != ENOTDIR)
return NULL;
oidclr(oid);
@@ -1760,7 +1765,7 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
!refname_is_safe(refname)) {
- errno = EINVAL;
+ *failure_errno = EINVAL;
return NULL;
}
@@ -1768,10 +1773,23 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
}
}
- errno = ELOOP;
+ *failure_errno = ELOOP;
return NULL;
}
+const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname,
+ int resolve_flags, struct object_id *oid,
+ int *flags)
+{
+ int failure_errno = 0;
+ const char *refn;
+ refn = refs_werrres_ref_unsafe(refs, refname, resolve_flags,
+ oid, flags, &failure_errno);
+ if (!refn)
+ errno = failure_errno;
+ return refn;
+}
+
/* backend functions */
int refs_init_db(struct strbuf *err)
{
diff --git a/refs.h b/refs.h
index d5099d4984e..c8afde6bb50 100644
--- a/refs.h
+++ b/refs.h
@@ -11,6 +11,18 @@ struct string_list;
struct string_list_item;
struct worktree;
+/*
+ * Callers should not inspect "errno" on failure, but rather pass in a
+ * "failure_errno" parameter, on failure the "errno" will indicate the
+ * type of failure encountered, but not necessarily one that came from
+ * a syscall. We might have faked it up.
+ */
+const char *refs_werrres_ref_unsafe(struct ref_store *refs,
+ const char *refname,
+ int resolve_flags,
+ struct object_id *oid,
+ int *flags, int *failure_errno);
+
/*
* Resolve a reference, recursively following symbolic refererences.
*
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 03/21] refs API: make refs_read_raw_ref() not set errno
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 01/21] branch tests: test for errno propagating on failing read Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 02/21] refs API: add a version of refs_resolve_ref_unsafe() with "errno" Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 04/21] refs API: make parse_loose_ref_contents() " Ævar Arnfjörð Bjarmason
` (17 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
From: Han-Wen Nienhuys <hanwen@google.com>
Add a "failure_errno" to refs_read_raw_ref(), his allows
refs_werrres_ref_unsafe() to pass along its "failure_errno", as a
first step before its own callers are migrated to pass it further up
the chain.
We are leaving out out the refs_read_special_head() in
refs_read_raw_ref() for now, as noted in a subsequent commit moving it
to "failure_errno" will require some special consideration.
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 24 ++++++++++++++++--------
refs/files-backend.c | 10 ++++++----
refs/packed-backend.c | 7 ++++---
refs/refs-internal.h | 6 +++---
4 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/refs.c b/refs.c
index ad56dbb0125..200c44e6963 100644
--- a/refs.c
+++ b/refs.c
@@ -1666,17 +1666,18 @@ static int refs_read_special_head(struct ref_store *ref_store,
return result;
}
-int refs_read_raw_ref(struct ref_store *ref_store,
- const char *refname, struct object_id *oid,
- struct strbuf *referent, unsigned int *type)
+int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
+ struct object_id *oid, struct strbuf *referent,
+ unsigned int *type, int *failure_errno)
{
+ assert(failure_errno);
if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
return refs_read_special_head(ref_store, refname, oid, referent,
type);
}
return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
- type, &errno);
+ type, failure_errno);
}
const char *refs_werrres_ref_unsafe(struct ref_store *refs,
@@ -1720,9 +1721,8 @@ const char *refs_werrres_ref_unsafe(struct ref_store *refs,
for (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) {
unsigned int read_flags = 0;
- errno = 0;
- if (refs_read_raw_ref(refs, refname,
- oid, &sb_refname, &read_flags)) {
+ if (refs_read_raw_ref(refs, refname, oid, &sb_refname,
+ &read_flags, failure_errno)) {
*flags |= read_flags;
if (errno)
*failure_errno = errno;
@@ -2240,6 +2240,13 @@ int refs_verify_refname_available(struct ref_store *refs,
strbuf_grow(&dirname, strlen(refname) + 1);
for (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) {
+ /*
+ * Just saying "Is a directory" when we e.g. can't
+ * lock some multi-level ref isn't very informative,
+ * the user won't be told *what* is a directory, so
+ * let's not use strerror() below.
+ */
+ int ignore_errno;
/* Expand dirname to the new prefix, not including the trailing slash: */
strbuf_add(&dirname, refname + dirname.len, slash - refname - dirname.len);
@@ -2251,7 +2258,8 @@ int refs_verify_refname_available(struct ref_store *refs,
if (skip && string_list_has_string(skip, dirname.buf))
continue;
- if (!refs_read_raw_ref(refs, dirname.buf, &oid, &referent, &type)) {
+ if (!refs_read_raw_ref(refs, dirname.buf, &oid, &referent,
+ &type, &ignore_errno)) {
strbuf_addf(err, _("'%s' exists; cannot create '%s'"),
dirname.buf, refname);
goto cleanup;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 6a6ead0b99b..94c194665ed 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -381,10 +381,11 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
goto out;
if (lstat(path, &st) < 0) {
+ int ignore_errno;
if (errno != ENOENT)
goto out;
- if (refs_read_raw_ref(refs->packed_ref_store, refname,
- oid, referent, type)) {
+ if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
+ referent, type, &ignore_errno)) {
errno = ENOENT;
goto out;
}
@@ -418,13 +419,14 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
/* Is it a directory? */
if (S_ISDIR(st.st_mode)) {
+ int ignore_errno;
/*
* Even though there is a directory where the loose
* ref is supposed to be, there could still be a
* packed ref:
*/
- if (refs_read_raw_ref(refs->packed_ref_store, refname,
- oid, referent, type)) {
+ if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
+ referent, type, &ignore_errno)) {
errno = EISDIR;
goto out;
}
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 47247a14917..52cdc94a26e 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1347,6 +1347,7 @@ int is_packed_transaction_needed(struct ref_store *ref_store,
ret = 0;
for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = transaction->updates[i];
+ int failure_errno;
unsigned int type;
struct object_id oid;
@@ -1357,9 +1358,9 @@ int is_packed_transaction_needed(struct ref_store *ref_store,
*/
continue;
- if (!refs_read_raw_ref(ref_store, update->refname,
- &oid, &referent, &type) ||
- errno != ENOENT) {
+ if (!refs_read_raw_ref(ref_store, update->refname, &oid,
+ &referent, &type, &failure_errno) ||
+ failure_errno != ENOENT) {
/*
* We have to actually delete that reference
* -> this transaction is needed.
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 72746407fc3..c87f1135e5b 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -149,9 +149,9 @@ struct ref_update {
const char refname[FLEX_ARRAY];
};
-int refs_read_raw_ref(struct ref_store *ref_store,
- const char *refname, struct object_id *oid,
- struct strbuf *referent, unsigned int *type);
+int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
+ struct object_id *oid, struct strbuf *referent,
+ unsigned int *type, int *failure_errno);
/*
* Write an error to `err` and return a nonzero value iff the same
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 04/21] refs API: make parse_loose_ref_contents() not set errno
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-10-16 9:39 ` [PATCH v2 03/21] refs API: make refs_read_raw_ref() not set errno Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 05/21] refs API: make refs_rename_ref_available() static Ævar Arnfjörð Bjarmason
` (16 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
From: Han-Wen Nienhuys <hanwen@google.com>
Change the parse_loose_ref_contents() function to stop setting "errno"
and failure, and to instead pass up a "failure_errno" via a
parameter. This requires changing its callers to do the same.
The EINVAL error from parse_loose_ref_contents is used in files-backend
to create a custom error message.
In untangling this we discovered a tricky edge case. The
refs_read_special_head() function was relying on
parse_loose_ref_contents() setting EINVAL.
By converting it to use "saved_errno" we can migrate away from "errno"
in this part of the code entirely, and do away with an existing
"save_errno" pattern, its only purpose was to not clobber the "errno"
we previously needed at the end of files_read_raw_ref().
Let's assert that we can do that by not having files_read_raw_ref()
itself operate on *failure_errno in addition to passing it on. Instead
we'll assert that if we return non-zero we actually do set errno, thus
assuring ourselves and callers that they can trust the resulting
"failure_errno".
Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 8 +++++---
refs/files-backend.c | 31 ++++++++++++++++++++-----------
refs/refs-internal.h | 6 ++++--
3 files changed, 29 insertions(+), 16 deletions(-)
diff --git a/refs.c b/refs.c
index 200c44e6963..bb09993c2f9 100644
--- a/refs.c
+++ b/refs.c
@@ -1648,7 +1648,8 @@ int for_each_fullref_in_prefixes(const char *namespace,
static int refs_read_special_head(struct ref_store *ref_store,
const char *refname, struct object_id *oid,
- struct strbuf *referent, unsigned int *type)
+ struct strbuf *referent, unsigned int *type,
+ int *failure_errno)
{
struct strbuf full_path = STRBUF_INIT;
struct strbuf content = STRBUF_INIT;
@@ -1658,7 +1659,8 @@ static int refs_read_special_head(struct ref_store *ref_store,
if (strbuf_read_file(&content, full_path.buf, 0) < 0)
goto done;
- result = parse_loose_ref_contents(content.buf, oid, referent, type);
+ result = parse_loose_ref_contents(content.buf, oid, referent, type,
+ failure_errno);
done:
strbuf_release(&full_path);
@@ -1673,7 +1675,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
assert(failure_errno);
if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
return refs_read_special_head(ref_store, refname, oid, referent,
- type);
+ type, failure_errno);
}
return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 94c194665ed..c73ffd1ca33 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -355,6 +355,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
int fd;
int ret = -1;
int remaining_retries = 3;
+ int myerr = 0;
*type = 0;
strbuf_reset(&sb_path);
@@ -382,11 +383,13 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
if (lstat(path, &st) < 0) {
int ignore_errno;
- if (errno != ENOENT)
+ myerr = errno;
+ errno = 0;
+ if (myerr != ENOENT)
goto out;
if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
referent, type, &ignore_errno)) {
- errno = ENOENT;
+ myerr = ENOENT;
goto out;
}
ret = 0;
@@ -397,7 +400,9 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
if (S_ISLNK(st.st_mode)) {
strbuf_reset(&sb_contents);
if (strbuf_readlink(&sb_contents, path, st.st_size) < 0) {
- if (errno == ENOENT || errno == EINVAL)
+ myerr = errno;
+ errno = 0;
+ if (myerr == ENOENT || myerr == EINVAL)
/* inconsistent with lstat; retry */
goto stat_ref;
else
@@ -427,7 +432,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
*/
if (refs_read_raw_ref(refs->packed_ref_store, refname, oid,
referent, type, &ignore_errno)) {
- errno = EISDIR;
+ myerr = EISDIR;
goto out;
}
ret = 0;
@@ -440,7 +445,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
*/
fd = open(path, O_RDONLY);
if (fd < 0) {
- if (errno == ENOENT && !S_ISLNK(st.st_mode))
+ myerr = errno;
+ if (myerr == ENOENT && !S_ISLNK(st.st_mode))
/* inconsistent with lstat; retry */
goto stat_ref;
else
@@ -448,26 +454,29 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,
}
strbuf_reset(&sb_contents);
if (strbuf_read(&sb_contents, fd, 256) < 0) {
- int save_errno = errno;
+ myerr = errno;
close(fd);
- errno = save_errno;
goto out;
}
close(fd);
strbuf_rtrim(&sb_contents);
buf = sb_contents.buf;
- ret = parse_loose_ref_contents(buf, oid, referent, type);
+ ret = parse_loose_ref_contents(buf, oid, referent, type, &myerr);
out:
- *failure_errno = errno;
+ if (ret && !myerr)
+ BUG("returning non-zero %d, should have set myerr!", ret);
+ *failure_errno = myerr;
+
strbuf_release(&sb_path);
strbuf_release(&sb_contents);
return ret;
}
int parse_loose_ref_contents(const char *buf, struct object_id *oid,
- struct strbuf *referent, unsigned int *type)
+ struct strbuf *referent, unsigned int *type,
+ int *failure_errno)
{
const char *p;
if (skip_prefix(buf, "ref:", &buf)) {
@@ -486,7 +495,7 @@ int parse_loose_ref_contents(const char *buf, struct object_id *oid,
if (parse_oid_hex(buf, oid, &p) ||
(*p != '\0' && !isspace(*p))) {
*type |= REF_ISBROKEN;
- errno = EINVAL;
+ *failure_errno = EINVAL;
return -1;
}
return 0;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index c87f1135e5b..121b8fdad08 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -706,10 +706,12 @@ struct ref_store {
};
/*
- * Parse contents of a loose ref file.
+ * Parse contents of a loose ref file. *failure_errno maybe be set to EINVAL for
+ * invalid contents.
*/
int parse_loose_ref_contents(const char *buf, struct object_id *oid,
- struct strbuf *referent, unsigned int *type);
+ struct strbuf *referent, unsigned int *type,
+ int *failure_errno);
/*
* Fill in the generic part of refs and add it to our collection of
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 05/21] refs API: make refs_rename_ref_available() static
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-10-16 9:39 ` [PATCH v2 04/21] refs API: make parse_loose_ref_contents() " Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 06/21] reflog tests: add --updateref tests Ævar Arnfjörð Bjarmason
` (15 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
Move the refs_rename_ref_available() function into
"refs/files-backend.c". It is file-backend specific.
This function was added in 5fe7d825da8 (refs.c: pass a list of names
to skip to is_refname_available, 2014-05-01) as rename_ref_available()
and was only ever used in this one file-backend specific codepath. So
let's move it there.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 19 -------------------
refs/files-backend.c | 29 +++++++++++++++++++++++++++++
refs/refs-internal.h | 14 --------------
3 files changed, 29 insertions(+), 33 deletions(-)
diff --git a/refs.c b/refs.c
index bb09993c2f9..44ddbb14f1d 100644
--- a/refs.c
+++ b/refs.c
@@ -1372,25 +1372,6 @@ const char *find_descendant_ref(const char *dirname,
return NULL;
}
-int refs_rename_ref_available(struct ref_store *refs,
- const char *old_refname,
- const char *new_refname)
-{
- struct string_list skip = STRING_LIST_INIT_NODUP;
- struct strbuf err = STRBUF_INIT;
- int ok;
-
- string_list_insert(&skip, old_refname);
- ok = !refs_verify_refname_available(refs, new_refname,
- NULL, &skip, &err);
- if (!ok)
- error("%s", err.buf);
-
- string_list_clear(&skip, 0);
- strbuf_release(&err);
- return ok;
-}
-
int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
{
struct object_id oid;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index c73ffd1ca33..0af6ee44552 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1363,6 +1363,35 @@ static int commit_ref_update(struct files_ref_store *refs,
const struct object_id *oid, const char *logmsg,
struct strbuf *err);
+/*
+ * Check whether an attempt to rename old_refname to new_refname would
+ * cause a D/F conflict with any existing reference (other than
+ * possibly old_refname). If there would be a conflict, emit an error
+ * message and return false; otherwise, return true.
+ *
+ * Note that this function is not safe against all races with other
+ * processes (though rename_ref() catches some races that might get by
+ * this check).
+ */
+static int refs_rename_ref_available(struct ref_store *refs,
+ const char *old_refname,
+ const char *new_refname)
+{
+ struct string_list skip = STRING_LIST_INIT_NODUP;
+ struct strbuf err = STRBUF_INIT;
+ int ok;
+
+ string_list_insert(&skip, old_refname);
+ ok = !refs_verify_refname_available(refs, new_refname,
+ NULL, &skip, &err);
+ if (!ok)
+ error("%s", err.buf);
+
+ string_list_clear(&skip, 0);
+ strbuf_release(&err);
+ return ok;
+}
+
static int files_copy_or_rename_ref(struct ref_store *ref_store,
const char *oldrefname, const char *newrefname,
const char *logmsg, int copy)
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 121b8fdad08..2ecd3dea9fb 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -228,20 +228,6 @@ const char *find_descendant_ref(const char *dirname,
const struct string_list *extras,
const struct string_list *skip);
-/*
- * Check whether an attempt to rename old_refname to new_refname would
- * cause a D/F conflict with any existing reference (other than
- * possibly old_refname). If there would be a conflict, emit an error
- * message and return false; otherwise, return true.
- *
- * Note that this function is not safe against all races with other
- * processes (though rename_ref() catches some races that might get by
- * this check).
- */
-int refs_rename_ref_available(struct ref_store *refs,
- const char *old_refname,
- const char *new_refname);
-
/* We allow "recursive" symbolic refs. Only within reason, though */
#define SYMREF_MAXDEPTH 5
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 06/21] reflog tests: add --updateref tests
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-10-16 9:39 ` [PATCH v2 05/21] refs API: make refs_rename_ref_available() static Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 07/21] refs/files: remove "name exist?" check in lock_ref_oid_basic() Ævar Arnfjörð Bjarmason
` (14 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
Add tests that cover blindspots in "git reflog delete --updateref"
behavior. Before this change removing the "type & REF_ISSYMREF" check
added in 5e6f003ca8a (reflog_expire(): ignore --updateref for symbolic
references, 2015-03-03) would not fail any tests.
The "--updateref" option was added in 55f10565371 (git-reflog: add
option --updateref to write the last reflog sha1 into the ref,
2008-02-22) for use in git-stash.sh, see e25d5f9c82e (git-stash: add
new 'drop' subcommand, 2008-02-22).
Even though the regression test I need is just the "C" case here,
let's test all these combinations for good measure. I started out
doing these as a for-loop, but I think this is more readable.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t1417-reflog-updateref.sh | 65 +++++++++++++++++++++++++++++++++++++
1 file changed, 65 insertions(+)
create mode 100755 t/t1417-reflog-updateref.sh
diff --git a/t/t1417-reflog-updateref.sh b/t/t1417-reflog-updateref.sh
new file mode 100755
index 00000000000..14f13b57c6d
--- /dev/null
+++ b/t/t1417-reflog-updateref.sh
@@ -0,0 +1,65 @@
+#!/bin/sh
+
+test_description='git reflog --updateref'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ git init -b main repo &&
+ (
+ cd repo &&
+
+ test_commit A &&
+ test_commit B &&
+ test_commit C &&
+
+ cp .git/logs/HEAD HEAD.old &&
+ git reset --hard HEAD~ &&
+ cp HEAD.old .git/logs/HEAD
+ )
+'
+
+test_reflog_updateref () {
+ exp=$1
+ shift
+ args="$@"
+
+ test_expect_success REFFILES "get '$exp' with '$args'" '
+ test_when_finished "rm -rf copy" &&
+ cp -R repo copy &&
+
+ (
+ cd copy &&
+
+ $args &&
+ git rev-parse $exp >expect &&
+ git rev-parse HEAD >actual &&
+
+ test_cmp expect actual
+ )
+ '
+}
+
+test_reflog_updateref B git reflog delete --updateref HEAD@{0}
+test_reflog_updateref B git reflog delete --updateref HEAD@{1}
+test_reflog_updateref C git reflog delete --updateref main@{0}
+test_reflog_updateref B git reflog delete --updateref main@{1}
+test_reflog_updateref B git reflog delete --updateref --rewrite HEAD@{0}
+test_reflog_updateref B git reflog delete --updateref --rewrite HEAD@{1}
+test_reflog_updateref C git reflog delete --updateref --rewrite main@{0}
+test_reflog_updateref B git reflog delete --updateref --rewrite main@{1}
+test_reflog_updateref B test_must_fail git reflog expire HEAD@{0}
+test_reflog_updateref B test_must_fail git reflog expire HEAD@{1}
+test_reflog_updateref B test_must_fail git reflog expire main@{0}
+test_reflog_updateref B test_must_fail git reflog expire main@{1}
+test_reflog_updateref B test_must_fail git reflog expire --updateref HEAD@{0}
+test_reflog_updateref B test_must_fail git reflog expire --updateref HEAD@{1}
+test_reflog_updateref B test_must_fail git reflog expire --updateref main@{0}
+test_reflog_updateref B test_must_fail git reflog expire --updateref main@{1}
+test_reflog_updateref B test_must_fail git reflog expire --updateref --rewrite HEAD@{0}
+test_reflog_updateref B test_must_fail git reflog expire --updateref --rewrite HEAD@{1}
+test_reflog_updateref B test_must_fail git reflog expire --updateref --rewrite main@{0}
+test_reflog_updateref B test_must_fail git reflog expire --updateref --rewrite main@{1}
+
+test_done
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 07/21] refs/files: remove "name exist?" check in lock_ref_oid_basic()
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-10-16 9:39 ` [PATCH v2 06/21] reflog tests: add --updateref tests Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 08/21] refs API: remove refs_read_ref_full() wrapper Ævar Arnfjörð Bjarmason
` (13 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
In lock_ref_oid_basic() we'll happily lock a reference that doesn't
exist yet. That's normal, and is how references are initially born,
but we don't need to retain checks here in lock_ref_oid_basic() about
the state of the ref, when what we're checking is either checked
already, or something we're about to discover by trying to lock the
ref with raceproof_create_file().
The one exception is the caller in files_reflog_expire(), who passes
us a "type" to find out if the reference is a symref or not. We can
move the that logic over to that caller, which can now defer its
discovery of whether or not the ref is a symref until it's needed. In
the preceding commit an exhaustive regression test was added for that
case in a new test in "t1417-reflog-updateref.sh".
The improved diagnostics here were added in
5b2d8d6f218 (lock_ref_sha1_basic(): improve diagnostics for ref D/F
conflicts, 2015-05-11), and then much of the surrounding code went
away recently in my 245fbba46d6 (refs/files: remove unused "errno ==
EISDIR" code, 2021-08-23).
The refs_resolve_ref_unsafe() code being removed here looks like it
should be tasked with doing that, but it's actually redundant to other
code.
The reason for that is as noted in 245fbba46d6 this once widely used
function now only has a handful of callers left, which all handle this
case themselves.
To the extent that we're racy between their check and ours removing
this check actually improves the situation, as we'll be doing fewer
things between the not-under-lock initial check and acquiring the
lock.
Why this is OK for all the remaining callers of lock_ref_oid_basic()
is noted below. There are only two of those callers:
* "git branch -[cm] <oldbranch> <newbranch>":
In files_copy_or_rename_ref() we'll call this when we copy or rename
refs via rename_ref() and copy_ref(). but only after we've checked
if the refname exists already via its own call to
refs_resolve_ref_unsafe() and refs_rename_ref_available().
As the updated comment to the latter here notes neither of those are
actually needed. If we delete not only this code but also
refs_rename_ref_available() we'll do just fine, we'll just emit a
less friendly error message if e.g. "git branch -m A B/C" would have
a D/F conflict with a "B" file.
Actually we'd probably die before that in case reflogs for the
branch existed, i.e. when the try to rename() or copy_file() the
relevant reflog, since if we've got a D/F conflict with a branch
name we'll probably also have the same with its reflogs (but not
necessarily, we might have reflogs, but it might not).
As some #leftoverbits that code seems buggy to me, i.e. the reflog
"protocol" should be to get a lock on the main ref, and then perform
ref and/or reflog operations. That code dates back to
c976d415e53 (git-branch: add options and tests for branch renaming,
2006-11-28) and probably pre-dated the solidifying of that
convention. But in any case, that edge case is not our bug or
problem right now.
* "git reflog expire <ref>":
In files_reflog_expire() we'll call this without previous ref
existence checking in files-backend.c, but that code is in turn
called by code that's just finished checking if the refname whose
reflog we're expiring exists.
See ae35e16cd43 (reflog expire: don't lock reflogs using previously
seen OID, 2021-08-23) for the current state of that code, and
5e6f003ca8a (reflog_expire(): ignore --updateref for symbolic
references, 2015-03-03) for the code we'd break if we only did a
"update = !!ref" here, which is covered by the aforementioned
regression test in "t1417-reflog-updateref.sh".
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs/files-backend.c | 48 ++++++++++++++++++++++----------------------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0af6ee44552..16e78326381 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1001,7 +1001,7 @@ static int create_reflock(const char *path, void *cb)
* Locks a ref returning the lock on success and NULL on failure.
*/
static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
- const char *refname, int *type,
+ const char *refname,
struct strbuf *err)
{
struct strbuf ref_file = STRBUF_INIT;
@@ -1013,16 +1013,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
CALLOC_ARRAY(lock, 1);
files_ref_path(refs, &ref_file, refname);
- if (!refs_resolve_ref_unsafe(&refs->base, refname,
- RESOLVE_REF_NO_RECURSE,
- &lock->old_oid, type)) {
- if (!refs_verify_refname_available(&refs->base, refname,
- NULL, NULL, err))
- strbuf_addf(err, "unable to resolve reference '%s': %s",
- refname, strerror(errno));
-
- goto error_return;
- }
/*
* If the ref did not exist and we are creating it, make sure
@@ -1364,14 +1354,14 @@ static int commit_ref_update(struct files_ref_store *refs,
struct strbuf *err);
/*
- * Check whether an attempt to rename old_refname to new_refname would
- * cause a D/F conflict with any existing reference (other than
- * possibly old_refname). If there would be a conflict, emit an error
+ * Emit a better error message than lockfile.c's
+ * unable_to_lock_message() would in case there is a D/F conflict with
+ * another existing reference. If there would be a conflict, emit an error
* message and return false; otherwise, return true.
*
* Note that this function is not safe against all races with other
- * processes (though rename_ref() catches some races that might get by
- * this check).
+ * processes, and that's not its job. We'll emit a more verbose error on D/f
+ * conflicts if we get past it into lock_ref_oid_basic().
*/
static int refs_rename_ref_available(struct ref_store *refs,
const char *old_refname,
@@ -1492,7 +1482,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
logmoved = log;
- lock = lock_ref_oid_basic(refs, newrefname, NULL, &err);
+ lock = lock_ref_oid_basic(refs, newrefname, &err);
if (!lock) {
if (copy)
error("unable to copy '%s' to '%s': %s", oldrefname, newrefname, err.buf);
@@ -1514,7 +1504,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
goto out;
rollback:
- lock = lock_ref_oid_basic(refs, oldrefname, NULL, &err);
+ lock = lock_ref_oid_basic(refs, oldrefname, &err);
if (!lock) {
error("unable to lock %s for rollback: %s", oldrefname, err.buf);
strbuf_release(&err);
@@ -1921,7 +1911,7 @@ static int files_create_symref(struct ref_store *ref_store,
struct ref_lock *lock;
int ret;
- lock = lock_ref_oid_basic(refs, refname, NULL, &err);
+ lock = lock_ref_oid_basic(refs, refname, &err);
if (!lock) {
error("%s", err.buf);
strbuf_release(&err);
@@ -3125,7 +3115,6 @@ static int files_reflog_expire(struct ref_store *ref_store,
struct strbuf log_file_sb = STRBUF_INIT;
char *log_file;
int status = 0;
- int type;
struct strbuf err = STRBUF_INIT;
const struct object_id *oid;
@@ -3139,7 +3128,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
* reference itself, plus we might need to update the
* reference if --updateref was specified:
*/
- lock = lock_ref_oid_basic(refs, refname, &type, &err);
+ lock = lock_ref_oid_basic(refs, refname, &err);
if (!lock) {
error("cannot lock ref '%s': %s", refname, err.buf);
strbuf_release(&err);
@@ -3201,9 +3190,20 @@ static int files_reflog_expire(struct ref_store *ref_store,
* a reference if there are no remaining reflog
* entries.
*/
- int update = (flags & EXPIRE_REFLOGS_UPDATE_REF) &&
- !(type & REF_ISSYMREF) &&
- !is_null_oid(&cb.last_kept_oid);
+ int update = 0;
+
+ if ((flags & EXPIRE_REFLOGS_UPDATE_REF) &&
+ !is_null_oid(&cb.last_kept_oid)) {
+ int ignore_errno;
+ int type;
+ const char *ref;
+
+ ref = refs_werrres_ref_unsafe(&refs->base, refname,
+ RESOLVE_REF_NO_RECURSE,
+ NULL, &type,
+ &ignore_errno);
+ update = !!(ref && !(type & REF_ISSYMREF));
+ }
if (close_lock_file_gently(&reflog_lock)) {
status |= error("couldn't write %s: %s", log_file,
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 08/21] refs API: remove refs_read_ref_full() wrapper
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2021-10-16 9:39 ` [PATCH v2 07/21] refs/files: remove "name exist?" check in lock_ref_oid_basic() Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 09/21] refs API: make resolve_gitlink_ref() not set errno Ævar Arnfjörð Bjarmason
` (12 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
Remove the refs_read_ref_full() wrapper in favor of migrating various
refs.c API users to the underlying refs_werrres_ref_unsafe() function.
A careful reading of these callers shows that the callers of this
function did not care about "errno", by moving away from the
refs_resolve_ref_unsafe() wrapper we can be sure that nothing relies
on it anymore.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 20 +++++++++-----------
refs.h | 2 --
refs/files-backend.c | 36 ++++++++++++++++++++++--------------
worktree.c | 9 +++++----
4 files changed, 36 insertions(+), 31 deletions(-)
diff --git a/refs.c b/refs.c
index 44ddbb14f1d..80b85d244d2 100644
--- a/refs.c
+++ b/refs.c
@@ -290,20 +290,17 @@ struct ref_filter {
void *cb_data;
};
-int refs_read_ref_full(struct ref_store *refs, const char *refname,
- int resolve_flags, struct object_id *oid, int *flags)
+int read_ref_full(const char *refname, int resolve_flags, struct object_id *oid, int *flags)
{
- if (refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags))
+ int ignore_errno;
+ struct ref_store *refs = get_main_ref_store(the_repository);
+
+ if (refs_werrres_ref_unsafe(refs, refname, resolve_flags,
+ oid, flags, &ignore_errno))
return 0;
return -1;
}
-int read_ref_full(const char *refname, int resolve_flags, struct object_id *oid, int *flags)
-{
- return refs_read_ref_full(get_main_ref_store(the_repository), refname,
- resolve_flags, oid, flags);
-}
-
int read_ref(const char *refname, struct object_id *oid)
{
return read_ref_full(refname, RESOLVE_REF_READING, oid, NULL);
@@ -1376,9 +1373,10 @@ int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
{
struct object_id oid;
int flag;
+ int ignore_errno;
- if (!refs_read_ref_full(refs, "HEAD", RESOLVE_REF_READING,
- &oid, &flag))
+ if (refs_werrres_ref_unsafe(refs, "HEAD", RESOLVE_REF_READING,
+ &oid, &flag, &ignore_errno))
return fn("HEAD", &oid, flag, cb_data);
return 0;
diff --git a/refs.h b/refs.h
index c8afde6bb50..3938f99c902 100644
--- a/refs.h
+++ b/refs.h
@@ -89,8 +89,6 @@ char *refs_resolve_refdup(struct ref_store *refs,
char *resolve_refdup(const char *refname, int resolve_flags,
struct object_id *oid, int *flags);
-int refs_read_ref_full(struct ref_store *refs, const char *refname,
- int resolve_flags, struct object_id *oid, int *flags);
int read_ref_full(const char *refname, int resolve_flags,
struct object_id *oid, int *flags);
int read_ref(const char *refname, struct object_id *oid);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 16e78326381..482d04de03a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1006,6 +1006,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
{
struct strbuf ref_file = STRBUF_INIT;
struct ref_lock *lock;
+ int ignore_errno;
files_assert_main_repository(refs, "lock_ref_oid_basic");
assert(err);
@@ -1032,9 +1033,8 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
goto error_return;
}
- if (refs_read_ref_full(&refs->base, lock->ref_name,
- 0,
- &lock->old_oid, NULL))
+ if (!refs_werrres_ref_unsafe(&refs->base, lock->ref_name, 0,
+ &lock->old_oid, NULL, &ignore_errno))
oidclr(&lock->old_oid);
goto out;
@@ -1397,6 +1397,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
struct strbuf tmp_renamed_log = STRBUF_INIT;
int log, ret;
struct strbuf err = STRBUF_INIT;
+ int ignore_errno;
files_reflog_path(refs, &sb_oldref, oldrefname);
files_reflog_path(refs, &sb_newref, newrefname);
@@ -1454,9 +1455,9 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
* the safety anyway; we want to delete the reference whatever
* its current value.
*/
- if (!copy && !refs_read_ref_full(&refs->base, newrefname,
- RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
- NULL, NULL) &&
+ if (!copy && refs_werrres_ref_unsafe(&refs->base, newrefname,
+ RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
+ NULL, NULL, &ignore_errno) &&
refs_delete_ref(&refs->base, NULL, newrefname,
NULL, REF_NO_DEREF)) {
if (errno == EISDIR) {
@@ -1868,9 +1869,12 @@ static void update_symref_reflog(struct files_ref_store *refs,
{
struct strbuf err = STRBUF_INIT;
struct object_id new_oid;
+ int ignore_errno;
+
if (logmsg &&
- !refs_read_ref_full(&refs->base, target,
- RESOLVE_REF_READING, &new_oid, NULL) &&
+ refs_werrres_ref_unsafe(&refs->base, target,
+ RESOLVE_REF_READING, &new_oid, NULL,
+ &ignore_errno) &&
files_log_ref_write(refs, refname, &lock->old_oid,
&new_oid, logmsg, 0, &err)) {
error("%s", err.buf);
@@ -2144,6 +2148,7 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
(struct files_reflog_iterator *)ref_iterator;
struct dir_iterator *diter = iter->dir_iterator;
int ok;
+ int ignore_errno;
while ((ok = dir_iterator_advance(diter)) == ITER_OK) {
int flags;
@@ -2155,9 +2160,10 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
if (ends_with(diter->basename, ".lock"))
continue;
- if (refs_read_ref_full(iter->ref_store,
- diter->relative_path, 0,
- &iter->oid, &flags)) {
+ if (!refs_werrres_ref_unsafe(iter->ref_store,
+ diter->relative_path, 0,
+ &iter->oid, &flags,
+ &ignore_errno)) {
error("bad ref for %s", diter->path.buf);
continue;
}
@@ -2501,9 +2507,11 @@ static int lock_ref_for_update(struct files_ref_store *refs,
* the transaction, so we have to read it here
* to record and possibly check old_oid:
*/
- if (refs_read_ref_full(&refs->base,
- referent.buf, 0,
- &lock->old_oid, NULL)) {
+ int ignore_errno;
+ if (!refs_werrres_ref_unsafe(&refs->base,
+ referent.buf, 0,
+ &lock->old_oid, NULL,
+ &ignore_errno)) {
if (update->flags & REF_HAVE_OLD) {
strbuf_addf(err, "cannot lock ref '%s': "
"error reading reference",
diff --git a/worktree.c b/worktree.c
index 092a4f92ad2..cfffcdb62b3 100644
--- a/worktree.c
+++ b/worktree.c
@@ -563,16 +563,17 @@ int other_head_refs(each_ref_fn fn, void *cb_data)
struct worktree *wt = *p;
struct object_id oid;
int flag;
+ int ignore_errno;
if (wt->is_current)
continue;
strbuf_reset(&refname);
strbuf_worktree_ref(wt, &refname, "HEAD");
- if (!refs_read_ref_full(get_main_ref_store(the_repository),
- refname.buf,
- RESOLVE_REF_READING,
- &oid, &flag))
+ if (refs_werrres_ref_unsafe(get_main_ref_store(the_repository),
+ refname.buf,
+ RESOLVE_REF_READING,
+ &oid, &flag, &ignore_errno))
ret = fn(refname.buf, &oid, flag, cb_data);
if (ret)
break;
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 09/21] refs API: make resolve_gitlink_ref() not set errno
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2021-10-16 9:39 ` [PATCH v2 08/21] refs API: remove refs_read_ref_full() wrapper Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 10/21] refs API: make loose_fill_ref_dir() " Ævar Arnfjörð Bjarmason
` (11 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
I have carefully read the upstream callers of resolve_gitlink_ref()
and determined that they don't care about errno.
So let's move away from the errno-setting refs_resolve_ref_unsafe()
wrapper to refs_werrres_ref_unsafe(), and explicitly ignore the errno
it sets for us.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 80b85d244d2..dc6ed561492 100644
--- a/refs.c
+++ b/refs.c
@@ -1791,14 +1791,15 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,
{
struct ref_store *refs;
int flags;
+ int ignore_errno;
refs = get_submodule_ref_store(submodule);
if (!refs)
return -1;
- if (!refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags) ||
- is_null_oid(oid))
+ if (!refs_werrres_ref_unsafe(refs, refname, 0, oid, &flags,
+ &ignore_errno) || is_null_oid(oid))
return -1;
return 0;
}
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 10/21] refs API: make loose_fill_ref_dir() not set errno
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (8 preceding siblings ...)
2021-10-16 9:39 ` [PATCH v2 09/21] refs API: make resolve_gitlink_ref() not set errno Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 11/21] refs API: make files_copy_or_rename_ref() et al " Ævar Arnfjörð Bjarmason
` (10 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
Change the refs_resolve_ref_unsafe() invoked in loose_fill_ref_dir()
to a form that ignores errno. The only eventual caller of this
function is create_ref_cache(), whose callers in turn don't have their
failure depend on any errno set here.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs/files-backend.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 482d04de03a..759c21e88ae 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -280,10 +280,11 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
create_dir_entry(dir->cache, refname.buf,
refname.len));
} else {
- if (!refs_resolve_ref_unsafe(&refs->base,
+ int ignore_errno;
+ if (!refs_werrres_ref_unsafe(&refs->base,
refname.buf,
RESOLVE_REF_READING,
- &oid, &flag)) {
+ &oid, &flag, &ignore_errno)) {
oidclr(&oid);
flag |= REF_ISBROKEN;
} else if (is_null_oid(&oid)) {
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 11/21] refs API: make files_copy_or_rename_ref() et al not set errno
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (9 preceding siblings ...)
2021-10-16 9:39 ` [PATCH v2 10/21] refs API: make loose_fill_ref_dir() " Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 12/21] refs API: ignore errno in worktree.c's add_head_info() Ævar Arnfjörð Bjarmason
` (9 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
None of the callers of rename_ref() and copy_ref() care about errno,
and as seen in the context here we already emit our own non-errno
using error() in the case where we'd use it.
So let's have it explicitly ignore errno, and do the same in
commit_ref_update(), which is only used within other code in
files_copy_or_rename_ref() itself which doesn't care about errno
either.
It might actually be sensible to have the callers use errno if the
failure was filesystem-specific, and with the upcoming reftable
backend we don't want to rely on that sort of thing, so let's keep
ignoring that for now.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs/files-backend.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 759c21e88ae..6c854dda533 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1410,9 +1410,9 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
goto out;
}
- if (!refs_resolve_ref_unsafe(&refs->base, oldrefname,
+ if (!refs_werrres_ref_unsafe(&refs->base, oldrefname,
RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
- &orig_oid, &flag)) {
+ &orig_oid, &flag, &ignore_errno)) {
ret = error("refname %s not found", oldrefname);
goto out;
}
@@ -1823,10 +1823,12 @@ static int commit_ref_update(struct files_ref_store *refs,
*/
int head_flag;
const char *head_ref;
+ int ignore_errno;
- head_ref = refs_resolve_ref_unsafe(&refs->base, "HEAD",
+ head_ref = refs_werrres_ref_unsafe(&refs->base, "HEAD",
RESOLVE_REF_READING,
- NULL, &head_flag);
+ NULL, &head_flag,
+ &ignore_errno);
if (head_ref && (head_flag & REF_ISSYMREF) &&
!strcmp(head_ref, lock->ref_name)) {
struct strbuf log_err = STRBUF_INIT;
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 12/21] refs API: ignore errno in worktree.c's add_head_info()
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (10 preceding siblings ...)
2021-10-16 9:39 ` [PATCH v2 11/21] refs API: make files_copy_or_rename_ref() et al " Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 13/21] refs API: ignore errno in worktree.c's find_shared_symref() Ævar Arnfjörð Bjarmason
` (8 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
The static add_head_info() function is only used indirectly by callers
of get_worktrees(), none of whom care about errno, and even if they
did having the faked-up one from refs_resolve_ref_unsafe() would only
confuse them if they used die_errno() et al. So let's explicitly
ignore it here.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
worktree.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/worktree.c b/worktree.c
index cfffcdb62b3..fa988ee978f 100644
--- a/worktree.c
+++ b/worktree.c
@@ -28,11 +28,13 @@ static void add_head_info(struct worktree *wt)
{
int flags;
const char *target;
+ int ignore_errno;
- target = refs_resolve_ref_unsafe(get_worktree_ref_store(wt),
+ target = refs_werrres_ref_unsafe(get_worktree_ref_store(wt),
"HEAD",
0,
- &wt->head_oid, &flags);
+ &wt->head_oid, &flags,
+ &ignore_errno);
if (!target)
return;
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 13/21] refs API: ignore errno in worktree.c's find_shared_symref()
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (11 preceding siblings ...)
2021-10-16 9:39 ` [PATCH v2 12/21] refs API: ignore errno in worktree.c's add_head_info() Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 14/21] refs tests: ignore ignore errno in test-ref-store helper Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
There are only handful of callers of find_shared_symref(), none of
whom care about errno, so let's migrate to the non-errno-propagating
version of refs_resolve_ref_unsafe() and explicitly ignore errno here.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
worktree.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/worktree.c b/worktree.c
index fa988ee978f..7d7cf058154 100644
--- a/worktree.c
+++ b/worktree.c
@@ -420,6 +420,7 @@ const struct worktree *find_shared_symref(const char *symref,
const char *symref_target;
struct ref_store *refs;
int flags;
+ int ignore_errno;
if (wt->is_bare)
continue;
@@ -436,8 +437,9 @@ const struct worktree *find_shared_symref(const char *symref,
}
refs = get_worktree_ref_store(wt);
- symref_target = refs_resolve_ref_unsafe(refs, symref, 0,
- NULL, &flags);
+ symref_target = refs_werrres_ref_unsafe(refs, symref, 0,
+ NULL, &flags,
+ &ignore_errno);
if ((flags & REF_ISSYMREF) &&
symref_target && !strcmp(symref_target, target)) {
existing = wt;
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 14/21] refs tests: ignore ignore errno in test-ref-store helper
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (12 preceding siblings ...)
2021-10-16 9:39 ` [PATCH v2 13/21] refs API: ignore errno in worktree.c's find_shared_symref() Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 15/21] refs API: make refs_resolve_refdup() not set errno Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
The cmd_resolve_ref() function has always ignored errno on failure,
but let's do so explicitly when using the refs_resolve_ref_unsafe()
function.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/helper/test-ref-store.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index b314b81a45b..2f91fb9b227 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -123,9 +123,10 @@ static int cmd_resolve_ref(struct ref_store *refs, const char **argv)
int resolve_flags = arg_flags(*argv++, "resolve-flags");
int flags;
const char *ref;
+ int ignore_errno;
- ref = refs_resolve_ref_unsafe(refs, refname, resolve_flags,
- &oid, &flags);
+ ref = refs_werrres_ref_unsafe(refs, refname, resolve_flags,
+ &oid, &flags, &ignore_errno);
printf("%s %s 0x%x\n", oid_to_hex(&oid), ref ? ref : "(null)", flags);
return ref ? 0 : 1;
}
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 15/21] refs API: make refs_resolve_refdup() not set errno
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (13 preceding siblings ...)
2021-10-16 9:39 ` [PATCH v2 14/21] refs tests: ignore ignore errno in test-ref-store helper Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 16/21] refs API: make refs_ref_exists() " Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
Move refs_resolve_refdup() from the legacy refs_resolve_ref_unsafe()
to the new refs_werrres_ref_unsafe(). I have read its callers and
determined that they don't care about errno being set.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index dc6ed561492..09452b5e413 100644
--- a/refs.c
+++ b/refs.c
@@ -268,9 +268,10 @@ char *refs_resolve_refdup(struct ref_store *refs,
struct object_id *oid, int *flags)
{
const char *result;
+ int ignore_errno;
- result = refs_resolve_ref_unsafe(refs, refname, resolve_flags,
- oid, flags);
+ result = refs_werrres_ref_unsafe(refs, refname, resolve_flags,
+ oid, flags, &ignore_errno);
return xstrdup_or_null(result);
}
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 16/21] refs API: make refs_ref_exists() not set errno
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (14 preceding siblings ...)
2021-10-16 9:39 ` [PATCH v2 15/21] refs API: make refs_resolve_refdup() not set errno Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 17/21] refs API: make resolve_ref_unsafe() " Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
Move refs_ref_exists from the legacy refs_resolve_ref_unsafe() to the
new refs_werrres_ref_unsafe(). I have read its callers and determined
that they don't care about errno being set, in particular:
git grep -W -w -e refs_ref_exists -e ref_exists
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 09452b5e413..8d5a76fbf29 100644
--- a/refs.c
+++ b/refs.c
@@ -309,7 +309,9 @@ int read_ref(const char *refname, struct object_id *oid)
int refs_ref_exists(struct ref_store *refs, const char *refname)
{
- return !!refs_resolve_ref_unsafe(refs, refname, RESOLVE_REF_READING, NULL, NULL);
+ int ignore_errno;
+ return !!refs_werrres_ref_unsafe(refs, refname, RESOLVE_REF_READING,
+ NULL, NULL, &ignore_errno);
}
int ref_exists(const char *refname)
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 17/21] refs API: make resolve_ref_unsafe() not set errno
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (15 preceding siblings ...)
2021-10-16 9:39 ` [PATCH v2 16/21] refs API: make refs_ref_exists() " Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 18/21] refs API: make expand_ref() & repo_dwim_log() " Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
Change the resolve_ref_unsafe() wrapper function to use the underlying
refs_werrres_ref_unsafe() directly.
From a reading of the callers I determined that the only one who cared
about errno was a sequencer.c caller added in e47c6cafcb5 (commit:
move print_commit_summary() to libgit, 2017-11-24), I'm migrating it
to using refs_werrres_ref_unsafe() directly.
This adds another "set errno" instance, but in this case it's OK and
idiomatic. We are setting it just before calling die_errno(). We could
have some hypothetical die_errno_var(&saved_errno, ...) here, but I
don't think it's worth it. The problem with errno is subtle action at
distance, not this sort of thing. We already use this pattern in a
couple of places in wrapper.c
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 6 ++++--
sequencer.c | 10 ++++++++--
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 8d5a76fbf29..b563729782b 100644
--- a/refs.c
+++ b/refs.c
@@ -1785,8 +1785,10 @@ int refs_init_db(struct strbuf *err)
const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
struct object_id *oid, int *flags)
{
- return refs_resolve_ref_unsafe(get_main_ref_store(the_repository), refname,
- resolve_flags, oid, flags);
+ int ignore_errno;
+
+ return refs_werrres_ref_unsafe(get_main_ref_store(the_repository), refname,
+ resolve_flags, oid, flags, &ignore_errno);
}
int resolve_gitlink_ref(const char *submodule, const char *refname,
diff --git a/sequencer.c b/sequencer.c
index fac0b5162f6..1223dc2d2bf 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1282,6 +1282,8 @@ void print_commit_summary(struct repository *r,
struct pretty_print_context pctx = {0};
struct strbuf author_ident = STRBUF_INIT;
struct strbuf committer_ident = STRBUF_INIT;
+ struct ref_store *refs;
+ int resolve_errno;
commit = lookup_commit(r, oid);
if (!commit)
@@ -1331,9 +1333,13 @@ void print_commit_summary(struct repository *r,
rev.diffopt.break_opt = 0;
diff_setup_done(&rev.diffopt);
- head = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
- if (!head)
+ refs = get_main_ref_store(the_repository);
+ head = refs_werrres_ref_unsafe(refs, "HEAD", 0, NULL, NULL,
+ &resolve_errno);
+ if (!head) {
+ errno = resolve_errno;
die_errno(_("unable to resolve HEAD after creating commit"));
+ }
if (!strcmp(head, "HEAD"))
head = _("detached HEAD");
else
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 18/21] refs API: make expand_ref() & repo_dwim_log() not set errno
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (16 preceding siblings ...)
2021-10-16 9:39 ` [PATCH v2 17/21] refs API: make resolve_ref_unsafe() " Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 19/21] refs API: don't expose "errno" in run_transaction_hook() Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
The use of these two is rather trivial, and it's easy to see none of
their callers care about errno. So let's move them from
refs_resolve_ref_unsafe() to refs_resolve_ref_unsafe_with_errno(),
these were the last two callers, so we can get rid of that wrapper
function.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index b563729782b..43fe9e6d89d 100644
--- a/refs.c
+++ b/refs.c
@@ -654,13 +654,16 @@ int expand_ref(struct repository *repo, const char *str, int len,
struct object_id oid_from_ref;
struct object_id *this_result;
int flag;
+ struct ref_store *refs = get_main_ref_store(repo);
+ int ignore_errno;
this_result = refs_found ? &oid_from_ref : oid;
strbuf_reset(&fullref);
strbuf_addf(&fullref, *p, len, str);
- r = refs_resolve_ref_unsafe(get_main_ref_store(repo),
- fullref.buf, RESOLVE_REF_READING,
- this_result, &flag);
+ r = refs_werrres_ref_unsafe(refs, fullref.buf,
+ RESOLVE_REF_READING,
+ this_result, &flag,
+ &ignore_errno);
if (r) {
if (!refs_found++)
*ref = xstrdup(r);
@@ -689,12 +692,14 @@ int repo_dwim_log(struct repository *r, const char *str, int len,
for (p = ref_rev_parse_rules; *p; p++) {
struct object_id hash;
const char *ref, *it;
+ int ignore_errno;
strbuf_reset(&path);
strbuf_addf(&path, *p, len, str);
- ref = refs_resolve_ref_unsafe(refs, path.buf,
+ ref = refs_werrres_ref_unsafe(refs, path.buf,
RESOLVE_REF_READING,
- oid ? &hash : NULL, NULL);
+ oid ? &hash : NULL, NULL,
+ &ignore_errno);
if (!ref)
continue;
if (refs_reflog_exists(refs, path.buf))
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 19/21] refs API: don't expose "errno" in run_transaction_hook()
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (17 preceding siblings ...)
2021-10-16 9:39 ` [PATCH v2 18/21] refs API: make expand_ref() & repo_dwim_log() " Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 20/21] refs API: post-migration API renaming [1/2] Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 21/21] refs API: post-migration API renaming [2/2] Ævar Arnfjörð Bjarmason
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
In run_transaction_hook() we've checked errno since 67541597670 (refs:
implement reference transaction hook, 2020-06-19), let's reset errno
afterwards to make sure nobody using refs.c directly or indirectly
relies on it.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
refs.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 43fe9e6d89d..e90c59539b4 100644
--- a/refs.c
+++ b/refs.c
@@ -2096,8 +2096,11 @@ static int run_transaction_hook(struct ref_transaction *transaction,
update->refname);
if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
- if (errno != EPIPE)
+ if (errno != EPIPE) {
+ /* Don't leak errno outside this API */
+ errno = 0;
ret = -1;
+ }
break;
}
}
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 20/21] refs API: post-migration API renaming [1/2]
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (18 preceding siblings ...)
2021-10-16 9:39 ` [PATCH v2 19/21] refs API: don't expose "errno" in run_transaction_hook() Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 21/21] refs API: post-migration API renaming [2/2] Ævar Arnfjörð Bjarmason
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
In preceding commits all callers of refs_resolve_ref_unsafe() were
migrated to the transitory refs_werrres_ref_unsafe() function.
As a first step in getting rid of it let's remove the old function
from the public API (it went unused in a preceding commit).
We then provide both a coccinelle rule to do the rename, and a macro
to avoid breaking the existing callers.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
contrib/coccinelle/refs.pending.cocci | 5 +++++
refs.c | 15 +--------------
refs.h | 22 +++++++++-------------
3 files changed, 15 insertions(+), 27 deletions(-)
create mode 100644 contrib/coccinelle/refs.pending.cocci
diff --git a/contrib/coccinelle/refs.pending.cocci b/contrib/coccinelle/refs.pending.cocci
new file mode 100644
index 00000000000..b33cb8a12aa
--- /dev/null
+++ b/contrib/coccinelle/refs.pending.cocci
@@ -0,0 +1,5 @@
+@@
+expression refs, refname, resolve_flags, oid, flags, failure_errno;
+@@
+- refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)
++ refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)
diff --git a/refs.c b/refs.c
index e90c59539b4..4e06745c97a 100644
--- a/refs.c
+++ b/refs.c
@@ -1669,7 +1669,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
type, failure_errno);
}
-const char *refs_werrres_ref_unsafe(struct ref_store *refs,
+const char *refs_resolve_ref_unsafe(struct ref_store *refs,
const char *refname,
int resolve_flags,
struct object_id *oid,
@@ -1766,19 +1766,6 @@ const char *refs_werrres_ref_unsafe(struct ref_store *refs,
return NULL;
}
-const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname,
- int resolve_flags, struct object_id *oid,
- int *flags)
-{
- int failure_errno = 0;
- const char *refn;
- refn = refs_werrres_ref_unsafe(refs, refname, resolve_flags,
- oid, flags, &failure_errno);
- if (!refn)
- errno = failure_errno;
- return refn;
-}
-
/* backend functions */
int refs_init_db(struct strbuf *err)
{
diff --git a/refs.h b/refs.h
index 3938f99c902..d908a161c06 100644
--- a/refs.h
+++ b/refs.h
@@ -11,18 +11,6 @@ struct string_list;
struct string_list_item;
struct worktree;
-/*
- * Callers should not inspect "errno" on failure, but rather pass in a
- * "failure_errno" parameter, on failure the "errno" will indicate the
- * type of failure encountered, but not necessarily one that came from
- * a syscall. We might have faked it up.
- */
-const char *refs_werrres_ref_unsafe(struct ref_store *refs,
- const char *refname,
- int resolve_flags,
- struct object_id *oid,
- int *flags, int *failure_errno);
-
/*
* Resolve a reference, recursively following symbolic refererences.
*
@@ -70,16 +58,24 @@ const char *refs_werrres_ref_unsafe(struct ref_store *refs,
* resolved. The function returns NULL for such ref names.
* Caps and underscores refers to the special refs, such as HEAD,
* FETCH_HEAD and friends, that all live outside of the refs/ directory.
+ *
+ * Callers should not inspect "errno" on failure, but rather pass in a
+ * "failure_errno" parameter, on failure the "errno" will indicate the
+ * type of failure encountered, but not necessarily one that came from
+ * a syscall. We might have faked it up.
*/
#define RESOLVE_REF_READING 0x01
#define RESOLVE_REF_NO_RECURSE 0x02
#define RESOLVE_REF_ALLOW_BAD_NAME 0x04
+#define refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno) \
+ refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)
const char *refs_resolve_ref_unsafe(struct ref_store *refs,
const char *refname,
int resolve_flags,
struct object_id *oid,
- int *flags);
+ int *flags, int *failure_errno);
+
const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
struct object_id *oid, int *flags);
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v2 21/21] refs API: post-migration API renaming [2/2]
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
` (19 preceding siblings ...)
2021-10-16 9:39 ` [PATCH v2 20/21] refs API: post-migration API renaming [1/2] Ævar Arnfjörð Bjarmason
@ 2021-10-16 9:39 ` Ævar Arnfjörð Bjarmason
20 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-10-16 9:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Han-Wen Nienhuys,
Ævar Arnfjörð Bjarmason
Rename the transitory refs_werrres_ref_unsafe() function to
refs_resolve_ref_unsafe(), now that all callers of the old function
have learned to pass in a "failure_errno" parameter.
The coccinelle semantic patch added in the preceding commit works, but
I couldn't figure out how to get spatch(1) to re-flow these argument
lists (and sometimes make lines way too long), so this rename was done
with:
perl -pi -e 's/refs_werrres_ref_unsafe/refs_resolve_ref_unsafe/g' \
$(git grep -l refs_werrres_ref_unsafe -- '*.c')
But after that "make contrib/coccinelle/refs.cocci.patch" comes up
empty, so the result would have been the same. Let's remove that
transitory semantic patch file, we won't need to retain it for any
other in-flight changes, refs_werrres_ref_unsafe() only existed within
this patch series.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
contrib/coccinelle/refs.pending.cocci | 5 -----
refs.c | 16 ++++++++--------
refs.h | 2 --
refs/files-backend.c | 18 +++++++++---------
sequencer.c | 2 +-
t/helper/test-ref-store.c | 2 +-
worktree.c | 6 +++---
7 files changed, 22 insertions(+), 29 deletions(-)
delete mode 100644 contrib/coccinelle/refs.pending.cocci
diff --git a/contrib/coccinelle/refs.pending.cocci b/contrib/coccinelle/refs.pending.cocci
deleted file mode 100644
index b33cb8a12aa..00000000000
--- a/contrib/coccinelle/refs.pending.cocci
+++ /dev/null
@@ -1,5 +0,0 @@
-@@
-expression refs, refname, resolve_flags, oid, flags, failure_errno;
-@@
-- refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)
-+ refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)
diff --git a/refs.c b/refs.c
index 4e06745c97a..cd9b8bb1141 100644
--- a/refs.c
+++ b/refs.c
@@ -270,7 +270,7 @@ char *refs_resolve_refdup(struct ref_store *refs,
const char *result;
int ignore_errno;
- result = refs_werrres_ref_unsafe(refs, refname, resolve_flags,
+ result = refs_resolve_ref_unsafe(refs, refname, resolve_flags,
oid, flags, &ignore_errno);
return xstrdup_or_null(result);
}
@@ -296,7 +296,7 @@ int read_ref_full(const char *refname, int resolve_flags, struct object_id *oid,
int ignore_errno;
struct ref_store *refs = get_main_ref_store(the_repository);
- if (refs_werrres_ref_unsafe(refs, refname, resolve_flags,
+ if (refs_resolve_ref_unsafe(refs, refname, resolve_flags,
oid, flags, &ignore_errno))
return 0;
return -1;
@@ -310,7 +310,7 @@ int read_ref(const char *refname, struct object_id *oid)
int refs_ref_exists(struct ref_store *refs, const char *refname)
{
int ignore_errno;
- return !!refs_werrres_ref_unsafe(refs, refname, RESOLVE_REF_READING,
+ return !!refs_resolve_ref_unsafe(refs, refname, RESOLVE_REF_READING,
NULL, NULL, &ignore_errno);
}
@@ -660,7 +660,7 @@ int expand_ref(struct repository *repo, const char *str, int len,
this_result = refs_found ? &oid_from_ref : oid;
strbuf_reset(&fullref);
strbuf_addf(&fullref, *p, len, str);
- r = refs_werrres_ref_unsafe(refs, fullref.buf,
+ r = refs_resolve_ref_unsafe(refs, fullref.buf,
RESOLVE_REF_READING,
this_result, &flag,
&ignore_errno);
@@ -696,7 +696,7 @@ int repo_dwim_log(struct repository *r, const char *str, int len,
strbuf_reset(&path);
strbuf_addf(&path, *p, len, str);
- ref = refs_werrres_ref_unsafe(refs, path.buf,
+ ref = refs_resolve_ref_unsafe(refs, path.buf,
RESOLVE_REF_READING,
oid ? &hash : NULL, NULL,
&ignore_errno);
@@ -1383,7 +1383,7 @@ int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
int flag;
int ignore_errno;
- if (refs_werrres_ref_unsafe(refs, "HEAD", RESOLVE_REF_READING,
+ if (refs_resolve_ref_unsafe(refs, "HEAD", RESOLVE_REF_READING,
&oid, &flag, &ignore_errno))
return fn("HEAD", &oid, flag, cb_data);
@@ -1779,7 +1779,7 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
{
int ignore_errno;
- return refs_werrres_ref_unsafe(get_main_ref_store(the_repository), refname,
+ return refs_resolve_ref_unsafe(get_main_ref_store(the_repository), refname,
resolve_flags, oid, flags, &ignore_errno);
}
@@ -1795,7 +1795,7 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,
if (!refs)
return -1;
- if (!refs_werrres_ref_unsafe(refs, refname, 0, oid, &flags,
+ if (!refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags,
&ignore_errno) || is_null_oid(oid))
return -1;
return 0;
diff --git a/refs.h b/refs.h
index d908a161c06..45c34e99e3a 100644
--- a/refs.h
+++ b/refs.h
@@ -68,8 +68,6 @@ struct worktree;
#define RESOLVE_REF_NO_RECURSE 0x02
#define RESOLVE_REF_ALLOW_BAD_NAME 0x04
-#define refs_werrres_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno) \
- refs_resolve_ref_unsafe(refs, refname, resolve_flags, oid, flags, failure_errno)
const char *refs_resolve_ref_unsafe(struct ref_store *refs,
const char *refname,
int resolve_flags,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 6c854dda533..abb6091fcf5 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -281,7 +281,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
refname.len));
} else {
int ignore_errno;
- if (!refs_werrres_ref_unsafe(&refs->base,
+ if (!refs_resolve_ref_unsafe(&refs->base,
refname.buf,
RESOLVE_REF_READING,
&oid, &flag, &ignore_errno)) {
@@ -1034,7 +1034,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
goto error_return;
}
- if (!refs_werrres_ref_unsafe(&refs->base, lock->ref_name, 0,
+ if (!refs_resolve_ref_unsafe(&refs->base, lock->ref_name, 0,
&lock->old_oid, NULL, &ignore_errno))
oidclr(&lock->old_oid);
goto out;
@@ -1410,7 +1410,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
goto out;
}
- if (!refs_werrres_ref_unsafe(&refs->base, oldrefname,
+ if (!refs_resolve_ref_unsafe(&refs->base, oldrefname,
RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
&orig_oid, &flag, &ignore_errno)) {
ret = error("refname %s not found", oldrefname);
@@ -1456,7 +1456,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,
* the safety anyway; we want to delete the reference whatever
* its current value.
*/
- if (!copy && refs_werrres_ref_unsafe(&refs->base, newrefname,
+ if (!copy && refs_resolve_ref_unsafe(&refs->base, newrefname,
RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
NULL, NULL, &ignore_errno) &&
refs_delete_ref(&refs->base, NULL, newrefname,
@@ -1825,7 +1825,7 @@ static int commit_ref_update(struct files_ref_store *refs,
const char *head_ref;
int ignore_errno;
- head_ref = refs_werrres_ref_unsafe(&refs->base, "HEAD",
+ head_ref = refs_resolve_ref_unsafe(&refs->base, "HEAD",
RESOLVE_REF_READING,
NULL, &head_flag,
&ignore_errno);
@@ -1875,7 +1875,7 @@ static void update_symref_reflog(struct files_ref_store *refs,
int ignore_errno;
if (logmsg &&
- refs_werrres_ref_unsafe(&refs->base, target,
+ refs_resolve_ref_unsafe(&refs->base, target,
RESOLVE_REF_READING, &new_oid, NULL,
&ignore_errno) &&
files_log_ref_write(refs, refname, &lock->old_oid,
@@ -2163,7 +2163,7 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
if (ends_with(diter->basename, ".lock"))
continue;
- if (!refs_werrres_ref_unsafe(iter->ref_store,
+ if (!refs_resolve_ref_unsafe(iter->ref_store,
diter->relative_path, 0,
&iter->oid, &flags,
&ignore_errno)) {
@@ -2511,7 +2511,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
* to record and possibly check old_oid:
*/
int ignore_errno;
- if (!refs_werrres_ref_unsafe(&refs->base,
+ if (!refs_resolve_ref_unsafe(&refs->base,
referent.buf, 0,
&lock->old_oid, NULL,
&ignore_errno)) {
@@ -3209,7 +3209,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
int type;
const char *ref;
- ref = refs_werrres_ref_unsafe(&refs->base, refname,
+ ref = refs_resolve_ref_unsafe(&refs->base, refname,
RESOLVE_REF_NO_RECURSE,
NULL, &type,
&ignore_errno);
diff --git a/sequencer.c b/sequencer.c
index 1223dc2d2bf..cf10f9a332e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1334,7 +1334,7 @@ void print_commit_summary(struct repository *r,
diff_setup_done(&rev.diffopt);
refs = get_main_ref_store(the_repository);
- head = refs_werrres_ref_unsafe(refs, "HEAD", 0, NULL, NULL,
+ head = refs_resolve_ref_unsafe(refs, "HEAD", 0, NULL, NULL,
&resolve_errno);
if (!head) {
errno = resolve_errno;
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 2f91fb9b227..3986665037a 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -125,7 +125,7 @@ static int cmd_resolve_ref(struct ref_store *refs, const char **argv)
const char *ref;
int ignore_errno;
- ref = refs_werrres_ref_unsafe(refs, refname, resolve_flags,
+ ref = refs_resolve_ref_unsafe(refs, refname, resolve_flags,
&oid, &flags, &ignore_errno);
printf("%s %s 0x%x\n", oid_to_hex(&oid), ref ? ref : "(null)", flags);
return ref ? 0 : 1;
diff --git a/worktree.c b/worktree.c
index 7d7cf058154..2c155b10150 100644
--- a/worktree.c
+++ b/worktree.c
@@ -30,7 +30,7 @@ static void add_head_info(struct worktree *wt)
const char *target;
int ignore_errno;
- target = refs_werrres_ref_unsafe(get_worktree_ref_store(wt),
+ target = refs_resolve_ref_unsafe(get_worktree_ref_store(wt),
"HEAD",
0,
&wt->head_oid, &flags,
@@ -437,7 +437,7 @@ const struct worktree *find_shared_symref(const char *symref,
}
refs = get_worktree_ref_store(wt);
- symref_target = refs_werrres_ref_unsafe(refs, symref, 0,
+ symref_target = refs_resolve_ref_unsafe(refs, symref, 0,
NULL, &flags,
&ignore_errno);
if ((flags & REF_ISSYMREF) &&
@@ -574,7 +574,7 @@ int other_head_refs(each_ref_fn fn, void *cb_data)
strbuf_reset(&refname);
strbuf_worktree_ref(wt, &refname, "HEAD");
- if (refs_werrres_ref_unsafe(get_main_ref_store(the_repository),
+ if (refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
refname.buf,
RESOLVE_REF_READING,
&oid, &flag, &ignore_errno))
--
2.33.1.1338.g20da966911a
^ permalink raw reply related [flat|nested] 45+ messages in thread
end of thread, other threads:[~2021-10-16 9:41 UTC | newest]
Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-14 0:06 [PATCH 00/20] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 01/20] branch tests: test for errno propagating on failing read Ævar Arnfjörð Bjarmason
2021-10-14 1:57 ` Eric Sunshine
2021-10-14 0:06 ` [PATCH 02/20] refs API: add a version of refs_resolve_ref_unsafe() with "errno" Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 03/20] refs API: make refs_read_raw_ref() not set errno Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 04/20] refs API: make parse_loose_ref_contents() " Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 05/20] refs API: make refs_rename_ref_available() static Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 06/20] refs/files: remove "name exist?" check in lock_ref_oid_basic() Ævar Arnfjörð Bjarmason
2021-10-14 20:53 ` Junio C Hamano
2021-10-14 0:06 ` [PATCH 07/20] refs API: remove refs_read_ref_full() wrapper Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 08/20] refs API: make resolve_gitlink_ref() not set errno Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 09/20] refs API: make loose_fill_ref_dir() " Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 10/20] refs API: make files_copy_or_rename_ref() et al " Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 11/20] refs API: ignore errno in worktree.c's add_head_info() Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 12/20] refs API: ignore errno in worktree.c's find_shared_symref() Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 13/20] refs tests: ignore ignore errno in test-ref-store helper Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 14/20] refs API: make refs_resolve_refdup() not set errno Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 15/20] refs API: make refs_ref_exists() " Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 16/20] refs API: make resolve_ref_unsafe() " Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 17/20] refs API: make expand_ref() & repo_dwim_log() " Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 18/20] refs API: don't expose "errno" in run_transaction_hook() Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 19/20] refs API: post-migration API renaming [1/2] Ævar Arnfjörð Bjarmason
2021-10-14 0:06 ` [PATCH 20/20] refs API: post-migration API renaming [2/2] Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 00/21] refs: stop having the API set "errno" Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 01/21] branch tests: test for errno propagating on failing read Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 02/21] refs API: add a version of refs_resolve_ref_unsafe() with "errno" Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 03/21] refs API: make refs_read_raw_ref() not set errno Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 04/21] refs API: make parse_loose_ref_contents() " Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 05/21] refs API: make refs_rename_ref_available() static Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 06/21] reflog tests: add --updateref tests Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 07/21] refs/files: remove "name exist?" check in lock_ref_oid_basic() Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 08/21] refs API: remove refs_read_ref_full() wrapper Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 09/21] refs API: make resolve_gitlink_ref() not set errno Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 10/21] refs API: make loose_fill_ref_dir() " Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 11/21] refs API: make files_copy_or_rename_ref() et al " Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 12/21] refs API: ignore errno in worktree.c's add_head_info() Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 13/21] refs API: ignore errno in worktree.c's find_shared_symref() Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 14/21] refs tests: ignore ignore errno in test-ref-store helper Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 15/21] refs API: make refs_resolve_refdup() not set errno Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 16/21] refs API: make refs_ref_exists() " Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 17/21] refs API: make resolve_ref_unsafe() " Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 18/21] refs API: make expand_ref() & repo_dwim_log() " Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 19/21] refs API: don't expose "errno" in run_transaction_hook() Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 20/21] refs API: post-migration API renaming [1/2] Ævar Arnfjörð Bjarmason
2021-10-16 9:39 ` [PATCH v2 21/21] refs API: post-migration API renaming [2/2] Ævar Arnfjörð Bjarmason
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).