Skip to content

Commit

Permalink
Merge branch 'hn/refs-errno-cleanup' into seen
Browse files Browse the repository at this point in the history 
Futz with the way 'errno' is relied on in the refs API to carry the
failure modes up the callchain.

* hn/refs-errno-cleanup:
  refs: explicitly propagate errno from refs_read_raw_ref
  refs: stop setting EINVAL and ELOOP in symref resolution
  refs: clear errno return in refs_resolve_ref_unsafe()
  refs: add failure_errno to refs_read_raw_ref() signature
  refs: make errno output explicit for refs_resolve_ref_unsafe
  refs: make errno output explicit for read_raw_ref_fn
  refs/files-backend: stop setting errno from lock_ref_oid_basic
  refs: remove EINVAL specification from the errno sideband in read_raw_ref_fn
  • Loading branch information
@gitster
gitster committed May 7, 2021
2 parents a512e54 + 9b0b42b commit 2f05516
Show file tree
Hide file tree
Showing 7 changed files with 94 additions and 76 deletions.
46 changes: 28 additions & 18 deletions refs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1676,24 +1676,24 @@ 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)
{
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);
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)
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;
Expand All @@ -1706,11 +1706,11 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
flags = &unused_flags;

*flags = 0;
*failure_errno = 0;

if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) ||
!refname_is_safe(refname)) {
errno = EINVAL;
return NULL;
}

Expand All @@ -1727,11 +1727,14 @@ 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;
int read_failure = 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, &read_failure)) {
*flags |= read_flags;

*failure_errno = read_failure;

/* In reading mode, refs must eventually resolve */
if (resolve_flags & RESOLVE_REF_READING)
return NULL;
Expand All @@ -1741,9 +1744,8 @@ 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 (read_failure != ENOENT && read_failure != EISDIR &&
read_failure != ENOTDIR)
return NULL;

oidclr(oid);
Expand All @@ -1770,18 +1772,25 @@ 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;
return NULL;
}

*flags |= REF_ISBROKEN | REF_BAD_NAME;
}
}

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 ignore;
return refs_resolve_ref_unsafe_with_errno(refs, refname, resolve_flags,
oid, flags, &ignore);
}

/* backend functions */
int refs_init_db(struct strbuf *err)
{
Expand Down Expand Up @@ -2248,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, NULL)) {
strbuf_addf(err, _("'%s' exists; cannot create '%s'"),
dirname.buf, refname);
goto cleanup;
Expand Down
1 change: 1 addition & 0 deletions refs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
int resolve_flags,
struct object_id *oid,
int *flags);

const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
struct object_id *oid, int *flags);

Expand Down
4 changes: 2 additions & 2 deletions refs/debug.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -238,15 +238,15 @@ 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",
Expand Down
58 changes: 26 additions & 32 deletions refs/files-backend.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -341,9 +341,9 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)
return refs->loose;
}

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)
{
struct files_ref_store *refs =
files_downcast(ref_store, REF_STORE_READ, "read_raw_ref");
Expand All @@ -354,7 +354,6 @@ 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;
Expand Down Expand Up @@ -384,8 +383,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,
if (lstat(path, &st) < 0) {
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, NULL)) {
errno = ENOENT;
goto out;
}
Expand Down Expand Up @@ -424,8 +423,8 @@ static int files_read_raw_ref(struct ref_store *ref_store,
* 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, NULL)) {
errno = EISDIR;
goto out;
}
Expand Down Expand Up @@ -459,10 +458,10 @@ static int files_read_raw_ref(struct ref_store *ref_store,
ret = parse_loose_ref_contents(buf, oid, referent, type);

out:
save_errno = errno;
if (failure_errno)
*failure_errno = errno;
strbuf_release(&sb_path);
strbuf_release(&sb_contents);
errno = save_errno;
return ret;
}

Expand Down Expand Up @@ -541,6 +540,7 @@ 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 = 0;

assert(err);
files_assert_main_repository(refs, "lock_raw_ref");
Expand Down Expand Up @@ -629,9 +629,9 @@ 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'",
Expand All @@ -655,7 +655,7 @@ 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
Expand Down Expand Up @@ -693,13 +693,13 @@ static int lock_raw_ref(struct files_ref_store *refs,
goto error_return;
}
}
} 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;
}

Expand Down Expand Up @@ -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,
Expand All @@ -922,10 +921,10 @@ 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;
int resolve_errno;

files_assert_main_repository(refs, "lock_ref_oid_basic");
assert(err);
Expand All @@ -938,18 +937,18 @@ 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;
* it is normal for the empty directory 'foo'
* to remain.
*/
if (remove_empty_directories(&ref_file)) {
last_errno = errno;
if (!refs_verify_refname_available(
&refs->base,
refname, extras, skip, err))
Expand All @@ -962,12 +961,11 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
&lock->old_oid, type);
}
if (!resolved) {
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;
}
Expand All @@ -981,20 +979,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;
Expand All @@ -1005,7 +1000,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,

out:
strbuf_release(&ref_file);
errno = last_errno;
return lock;
}

Expand Down
16 changes: 9 additions & 7 deletions refs/packed-backend.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -724,9 +724,9 @@ static struct snapshot *get_snapshot(struct packed_ref_store *refs)
return refs->snapshot;
}

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)
{
struct packed_ref_store *refs =
packed_downcast(ref_store, REF_STORE_READ, "read_raw_ref");
Expand All @@ -739,7 +739,8 @@ static int packed_read_raw_ref(struct ref_store *ref_store,

if (!rec) {
/* refname is not a packed reference. */
errno = ENOENT;
if (failure_errno)
*failure_errno = ENOENT;
return -1;
}

Expand Down Expand Up @@ -1347,6 +1348,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;
unsigned int type;
struct object_id oid;

Expand All @@ -1357,9 +1359,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) ||
failure != ENOENT) {
/*
* We have to actually delete that reference
* -> this transaction is needed.
Expand Down

0 comments on commit 2f05516

Please sign in to comment.