On Thu, Jun 10 2021, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
>
> refs/files-backend.c::lock_ref_oid_basic() tries to signal how it failed
> to its callers using errno.
>
> It is safe to stop setting errno here, because the callers of this
> file-scope static function are
>
> * files_copy_or_rename_ref()
> * files_create_symref()
> * files_reflog_expire()
>
> None of them looks at errno after seeing a negative return from
> lock_ref_oid_basic() to make any decision, and no caller of these three
> functions looks at errno after they signal a failure by returning a
> negative value. In particular,
>
> * files_copy_or_rename_ref() - here, calls are followed by error()
> (which performs I/O) or write_ref_to_lockfile() (which calls
> parse_object() which may perform I/O)
>
> * files_create_symref() - here, calls are followed by error() or
> create_symref_locked() (which performs I/O and does not inspect
> errno)
>
> * files_reflog_expire() - here, calls are followed by error() or
> refs_reflog_exists() (which calls a function in a vtable that is not
> documented to use and/or preserve errno)
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
> Reviewed-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx>
> ---
> refs/files-backend.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
This all looks good and well justified after the last commit (where we
even mentioned refs_verify_refname_available() explicitly), but...
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 677b7e4cdd2d..6aa0f5c41dd3 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -910,7 +910,6 @@ static int create_reflock(const char *path, void *cb)
>
> /*
> * Locks a ref returning the lock on success and NULL on failure.
> - * On failure errno is set to something meaningful.
> */
> static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
> const char *refname,
> @@ -922,7 +921,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
> {
> struct strbuf ref_file = STRBUF_INIT;
> struct ref_lock *lock;
> - int last_errno = 0;
> int mustexist = (old_oid && !is_null_oid(old_oid));
> int resolve_flags = RESOLVE_REF_NO_RECURSE;
> int resolved;
> @@ -949,7 +947,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
> * to remain.
> */
> if (remove_empty_directories(&ref_file)) {
> - last_errno = errno;
> if (!refs_verify_refname_available(
> &refs->base,
> refname, extras, skip, err))
> @@ -962,7 +959,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
> &lock->old_oid, type);
> }
> if (!resolved) {
> - last_errno = errno;
> + int last_errno = errno;
> if (last_errno != ENOTDIR ||
> !refs_verify_refname_available(&refs->base, refname,
> extras, skip, err))
...this particular change gives me some pause, because all the rest is
about squirreling away our own errno for our own caller (which it turns
out, we didn't need).
But in this case we're only guarding against
refs_verify_refname_available() possibly clobbering the errno we just
got on !resolved in refs_verify_refname_available().
So instead I'd expect either this on top:
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 6aa0f5c41dd..28aa4932529 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -959,12 +959,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 ||
+ if (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(errno));
goto error_return;
}
Or, if we are actually worried about the errno being reset as we report
it:
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 6aa0f5c41dd..8ee6af61f1a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -960,11 +960,20 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
}
if (!resolved) {
int last_errno = errno;
+ errno = 0;
if (last_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));
+ extras, skip, err)) {
+ if (errno)
+ strbuf_addf(err, "unable to resolve reference '%s': '%s', "
+ "also got '%s when reporting the error!",
+ refname, strerror(last_errno),
+ strerror(errno));
+ else
+ strbuf_addf(err, "unable to resolve reference '%s': %s",
+ refname, strerror(errno));
+
+ }
goto error_return;
}
I think in the end it doesn't matter much, we hit our primary error, so
we're only potentially losing another error on our way out the door.
It's more about clarity, the "last_errno" pattern signals "I'm about to
call something that'll reset the errno I care about", but it's not clear
if that's actually the case here, or if this is just leftover
boilerplate.
In any case running the tests with my second hunk with just a:
if (errno)
BUG("lost it?");
else
...
Passes all our tests. I don't think it should be the scope of this
series to give this code 100% test coverage, but (looking ahead) there's
no mention of /test/ anywhere in the commit messages/comments.
I think even if we keep your "last_errno" as-is here it would be useful
to at least say something like:
/*
* Just paranoia, we probably won't lose errno in
* refs_verify_refname_available().
*/
int last_errno = errno;
...
