On Thu, Jun 10 2021, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
>
> lock_ref_oid_basic() in files-backend.c is the only caller of refs_resolve_ref()
> that needs error information to make logic decisions.
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
> Reviewed-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx>
> ---
> refs/files-backend.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 8f969c8f711f..5a430aabf623 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -924,6 +924,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
> int mustexist = (old_oid && !is_null_oid(old_oid));
> int resolve_flags = RESOLVE_REF_NO_RECURSE;
> int resolved;
> + int resolve_errno = 0;
>
> files_assert_main_repository(refs, "lock_ref_oid_basic");
> assert(err);
> @@ -936,10 +937,11 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
> resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
>
> files_ref_path(refs, &ref_file, refname);
> - resolved = !!refs_resolve_ref_unsafe(&refs->base,
> - refname, resolve_flags,
> - &lock->old_oid, type);
> - if (!resolved && errno == EISDIR) {
> + resolved = !!refs_resolve_ref_unsafe_with_errno(&refs->base, refname,
> + resolve_flags,
> + &lock->old_oid, type,
> + &resolve_errno);
> + if (!resolved && resolve_errno == EISDIR) {
> /*
> * we are trying to lock foo but we used to
> * have foo/bar which now does not exist;
> @@ -959,12 +961,11 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
> &lock->old_oid, type);
> }
> if (!resolved) {
> - int last_errno = errno;
> - if (last_errno != ENOTDIR ||
> - !refs_verify_refname_available(&refs->base, refname,
> - extras, skip, err))
> + if (resolve_errno != ENOTDIR ||
> + !refs_verify_refname_available(&refs->base, refname, extras,
> + skip, err))
> strbuf_addf(err, "unable to resolve reference '%s': %s",
> - refname, strerror(last_errno));
> + refname, strerror(resolve_errno));
>
> goto error_return;
> }
So, having read 4/8 and this I wonder why
refs_resolve_ref_unsafe_with_errno() is needed at all. It's just a
wrapper that sets errno to a variable you give it, but we could just
document that tha caller should check errno.
So far I haven't seen anything that suggests the below diff-on-top
wouldn't be OK (and all tests pass with it). It steps on the toes of
some of my earlier suggestions, but I'm doing these one-at-a-time.
In any case the comment I adjusted seems like something you should
adjust to. It looks like a TODO for having the sort of function you've
just implemented in refs_resolve_ref_unsafe_with_errno().
diff --git a/refs.c b/refs.c
index 64e2d55adcf..a07d852fcdc 100644
--- a/refs.c
+++ b/refs.c
@@ -1688,7 +1688,10 @@ int refs_read_raw_ref(struct ref_store *ref_store,
return result;
}
-/* This function needs to return a meaningful errno on failure */
+/*
+ * This function clears errno at the beginning. If it fails the errno
+ * will be meaningful.
+ */
const char *refs_resolve_ref_unsafe(struct ref_store *refs,
const char *refname,
int resolve_flags,
@@ -1698,6 +1701,7 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
struct object_id unused_oid;
int unused_flags;
int symref_count;
+ errno = 0;
if (!oid)
oid = &unused_oid;
@@ -1781,18 +1785,6 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
return NULL;
}
-const char *refs_resolve_ref_unsafe_with_errno(struct ref_store *refs,
- const char *refname,
- int resolve_flags,
- struct object_id *oid,
- int *flags, int *failure_errno)
-{
- const char *result = refs_resolve_ref_unsafe(refs, refname,
- resolve_flags, oid, flags);
- *failure_errno = errno;
- return result;
-}
-
/* backend functions */
int refs_init_db(struct strbuf *err)
{
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5a430aabf62..5a400e55cbf 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -924,7 +924,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
int mustexist = (old_oid && !is_null_oid(old_oid));
int resolve_flags = RESOLVE_REF_NO_RECURSE;
int resolved;
- int resolve_errno = 0;
files_assert_main_repository(refs, "lock_ref_oid_basic");
assert(err);
@@ -937,11 +936,10 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
files_ref_path(refs, &ref_file, refname);
- resolved = !!refs_resolve_ref_unsafe_with_errno(&refs->base, refname,
- resolve_flags,
- &lock->old_oid, type,
- &resolve_errno);
- if (!resolved && resolve_errno == EISDIR) {
+ resolved = !!refs_resolve_ref_unsafe(&refs->base, refname,
+ resolve_flags,
+ &lock->old_oid, type);
+ if (!resolved && errno == EISDIR) {
/*
* we are trying to lock foo but we used to
* have foo/bar which now does not exist;
@@ -961,11 +959,11 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
&lock->old_oid, type);
}
if (!resolved) {
- if (resolve_errno != ENOTDIR ||
+ if (errno != ENOTDIR ||
!refs_verify_refname_available(&refs->base, refname, extras,
skip, err))
strbuf_addf(err, "unable to resolve reference '%s': %s",
- refname, strerror(resolve_errno));
+ refname, strerror(errno));
goto error_return;
}
