| Message ID | cbe09a48036c0befafa0f26f72d188dc765f5b7b.1623329869.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refs: cleanup errno sideband ref related functions | expand |
On Thu, Jun 10 2021, Han-Wen Nienhuys via GitGitGadget wrote: > From: Han-Wen Nienhuys <hanwen@google.com> > > 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@google.com> > Reviewed-by: Jonathan Tan <jonathantanmy@google.com> > --- > 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; ...
On Thu, Jul 1, 2021 at 1:30 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > } > > 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(). This comes from 5b2d8d6f2184381b76c13504a2f5ec8a62cd584e .. invoke verify_refname_available() to try to generate a more helpful error message. That function might not detect an error. For example, some non-reference file might be blocking the deletion of an otherwise-empty directory tree, or there might be a race with another process that just deleted the offending reference. In such cases, generate the strerror-based error message like before. I think we want to keep this behavior, hence I think this should be kept as is. I'll add a comment.
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)) @@ -981,20 +978,17 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, if (is_null_oid(&lock->old_oid) && refs_verify_refname_available(refs->packed_ref_store, refname, extras, skip, err)) { - last_errno = ENOTDIR; goto error_return; } lock->ref_name = xstrdup(refname); 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; } if (verify_lock(&refs->base, lock, old_oid, mustexist, err)) { - last_errno = errno; goto error_return; } goto out; @@ -1005,7 +999,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, out: strbuf_release(&ref_file); - errno = last_errno; return lock; }