Subject: Re: [PATCH v4 13/15] Reftable support for git-core

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Thu, 21 Jan 2021 16:55:31 +0100

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

> From: Han-Wen Nienhuys <[email-masked]>
> [...]
>  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?