On Wed, Dec 09 2020, Han-Wen Nienhuys via GitGitGadget wrote:

From: Han-Wen Nienhuys <redacted>
[...]
 refs.c                                        |   27 +-
 refs.h                                        |    3 +
[...]
 refs/reftable-backend.c                       | 1435 +++++++++++++++++
[...]
+static int git_reftable_read_raw_ref(struct ref_store *ref_store,
+				     const char *refname, struct object_id *oid,
+				     struct strbuf *referent,
+				     unsigned int *type)
+{
+	struct git_reftable_ref_store *refs =
+		(struct git_reftable_ref_store *)ref_store;
+	struct reftable_stack *stack = stack_for(refs, refname);
+
+	struct reftable_ref_record ref = { NULL };
+	int err = 0;
+	if (refs->err < 0) {
+		return refs->err;
+	}
+
+	/* This is usually not needed, but Git doesn't signal to ref backend if
+	   a subprocess updated the ref DB.  So we always check.
+	*/
+	err = reftable_stack_reload(stack);
+	if (err) {
+		goto done;
+	}
+
+	err = reftable_stack_read_ref(stack, refname, &ref);
+	if (err > 0) {
+		errno = ENOENT;
+		err = -1;
+		goto done;
+	}
+	if (err < 0) {
+		errno = reftable_error_to_errno(err);
+		err = -1;
+		goto done;
+	}
+
+	if (ref.value_type == REFTABLE_REF_SYMREF) {
+		strbuf_reset(referent);
+		strbuf_addstr(referent, ref.value.symref);
+		*type |= REF_ISSYMREF;
+	} else if (reftable_ref_record_val1(&ref) != NULL) {
+		hashcpy(oid->hash, reftable_ref_record_val1(&ref));
+	} else {
+		*type |= REF_ISBROKEN;
+		errno = EINVAL;
+		err = -1;

I've been chasing down some edge cases in refs.c as part of another WIP
series I have and found that for this particular "errno" stuff we don't
have any test coverage. And with master & hanwen/reftable if I apply:

    diff --git a/refs.c b/refs.c
    index af0d000082..8ac57908ea 100644
    --- a/refs.c
    +++ b/refs.c
    @@ -1589,3 +1589,2 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
                        !refname_is_safe(refname)) {
    -                       errno = EINVAL;
                            return NULL;
    @@ -1649,3 +1648,2 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
                                !refname_is_safe(refname)) {
    -                               errno = EINVAL;
                                    return NULL;
    @@ -1657,3 +1655,2 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
     
    -       errno = ELOOP;
            return NULL;
    diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
    index e98d22c826..e09ae21965 100644
    --- a/refs/reftable-backend.c
    +++ b/refs/reftable-backend.c
    @@ -1343,3 +1343,2 @@ static int git_reftable_read_raw_ref(struct ref_store *ref_store,
            if (err < 0) {
    -               errno = reftable_error_to_errno(err);
                    err = -1;
    @@ -1355,3 +1354,2 @@ static int git_reftable_read_raw_ref(struct ref_store *ref_store,
                    *type |= REF_ISBROKEN;
    -               errno = EINVAL;
                    err = -1;

All tests pass on both.

Anyway, in the case of the code sitting on "master" I find that less
concerning. It's some code that's drifted apart over the years blah blah
blah. Boring details but for various reasons we don't use some/any of
that error handling logic anymore, or to the extent that we do I'll need
to improve the tests.

I must say I do find it a bit more concerning in the case of the
reftable series. I.e. the git_reftable_read_raw_ref() function seems
like a faithful copy of the general logic (at least this part) of the
refs_resolve_ref_unsafe() function in refs.c.

That I can remove error checking/handling from this place means existing
general logic was faithfully copied without checking that we have a test
for it, or adding one. Or perhaps given the history of this codebase
there's out-of-tree tests for it somewhere?