Re: [PATCH v4 13/15] Reftable support for git-core
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jan 21, 2021, 15:55 UTC
- Message-ID
- <871ree8eto.fsf@evledraar.gmail.com>
- In-Reply-To
- <d57023d9f13d178dc95d7a74751923b80d1a5939.1607522429.git.gitgitgadget@gmail.com>
On Wed, Dec 09 2020, Han-Wen Nienhuys via GitGitGadget wrote:
Show 52 quoted lines
> From: Han-Wen Nienhuys <hanwen@google.com>
> [...]
> 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?