[PATCH 0/5] refs: designated init & missing debug in ps/fetch-mirror-optim

[PATCH 0/5] refs: designated init & missing debug in ps/fetch-mirror-optim

[Ævar Arnfjörð Bjarmason]

When I recently submitted a topic[1] to convert various things to
designated initializers Junio (presumably) ejected the refs changes
due to conflicts with ps/fetch-mirror-optim. The rest of the series
was down already in 20d34c07ea4 (Merge branch
'ab/c99-designated-initializers', 2022-03-06)

Now that that series has landed 1-3/5 are those previous patches
re-submitted (with a minor change for the new "read_symbolic_ref").

The 4/5 is then a deletion of packed-backend.c stub functions that I
think are not worth carrying anymore, and which has the fringe benefit
of squashing a noisy warning under SunCC.

5/5 is then a minor fix-up for ps/fetch-mirror-optim: It didn't define
a debug wrapper for refs_read_symbolic_ref(). Now that we do we don't
need an implementation that'll ever fall back on
refs_read_raw_ref(). It's now a stand-alone API function without a
fallback (which in effect it was before, sans debug backend).

Aside: It seems that the GIT_TRACE_REFS facility has been broken since
it was added, i.e. running the test site with e.g.:

    GIT_TRACE_REFS=/tmp/log.txt make test

Will fail, and not due to a test being confused about the logs. I
think something in how the iterators are wrapped is confusing it, but
that's a long-standing issue in any case...

1. https://lore.kernel.org/git/cover-00.12-00000000000-20220224T092805Z-avarab@gmail.com

Ævar Arnfjörð Bjarmason (5):
  refs: use designated initializers for "struct ref_storage_be"
  refs: use designated initializers for "struct ref_iterator_vtable"
  misc *.c: use designated initializers for struct assignments
  packed-backend: remove stub BUG(...) functions
  refs debug: add a wrapper for "read_symbolic_ref"

 attr.c                |   2 +-
 notes-merge.c         |   1 +
 object-file.c         |   9 +--
 refs.c                |  13 +----
 refs/debug.c          |  82 ++++++++++++++++++---------
 refs/files-backend.c  |  64 ++++++++++-----------
 refs/iterator.c       |  18 +++---
 refs/packed-backend.c | 128 ++++++++++--------------------------------
 refs/ref-cache.c      |   6 +-
 9 files changed, 135 insertions(+), 188 deletions(-)

-- 
2.35.1.1384.g7d2906948a1

[PATCH 1/5] refs: use designated initializers for "struct ref_storage_be"

[Ævar Arnfjörð Bjarmason]

Change the definition of the three refs backends we currently carry to
use designated initializers.

The "= NULL" assignments being retained here are redundant, and could
be removed, but let's keep them for clarity. All of these backends
define almost all fields, so we're not saving much in terms of line
count by omitting these, but e.g. for "refs_be_debug" it's immediately
apparent that we're omitting "init" when comparing its assignment to
the others.

This is a follow-up to similar work merged in bd4232fac33 (Merge
branch 'ab/struct-init', 2021-07-16), a4b9fb6a5cf (Merge branch
'ab/designated-initializers-more', 2021-10-18) and a30321b9eae (Merge
branch 'ab/designated-initializers' into
ab/designated-initializers-more, 2021-09-27).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/debug.c          | 52 +++++++++++++++++++++----------------------
 refs/files-backend.c  | 52 +++++++++++++++++++++----------------------
 refs/packed-backend.c | 52 +++++++++++++++++++++----------------------
 3 files changed, 78 insertions(+), 78 deletions(-)

diff --git a/refs/debug.c b/refs/debug.c
index c590d377200..b03a83258be 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -418,30 +418,30 @@ static int debug_reflog_expire(struct ref_store *ref_store, const char *refname,
 }
 
 struct ref_storage_be refs_be_debug = {
-	NULL,
-	"debug",
-	NULL,
-	debug_init_db,
-	debug_transaction_prepare,
-	debug_transaction_finish,
-	debug_transaction_abort,
-	debug_initial_transaction_commit,
-
-	debug_pack_refs,
-	debug_create_symref,
-	debug_delete_refs,
-	debug_rename_ref,
-	debug_copy_ref,
-
-	debug_ref_iterator_begin,
-	debug_read_raw_ref,
-	NULL,
-
-	debug_reflog_iterator_begin,
-	debug_for_each_reflog_ent,
-	debug_for_each_reflog_ent_reverse,
-	debug_reflog_exists,
-	debug_create_reflog,
-	debug_delete_reflog,
-	debug_reflog_expire,
+	.next = NULL,
+	.name = "debug",
+	.init = NULL,
+	.init_db = debug_init_db,
+	.transaction_prepare = debug_transaction_prepare,
+	.transaction_finish = debug_transaction_finish,
+	.transaction_abort = debug_transaction_abort,
+	.initial_transaction_commit = debug_initial_transaction_commit,
+
+	.pack_refs = debug_pack_refs,
+	.create_symref = debug_create_symref,
+	.delete_refs = debug_delete_refs,
+	.rename_ref = debug_rename_ref,
+	.copy_ref = debug_copy_ref,
+
+	.iterator_begin = debug_ref_iterator_begin,
+	.read_raw_ref = debug_read_raw_ref,
+	.read_symbolic_ref = NULL,
+
+	.reflog_iterator_begin = debug_reflog_iterator_begin,
+	.for_each_reflog_ent = debug_for_each_reflog_ent,
+	.for_each_reflog_ent_reverse = debug_for_each_reflog_ent_reverse,
+	.reflog_exists = debug_reflog_exists,
+	.create_reflog = debug_create_reflog,
+	.delete_reflog = debug_delete_reflog,
+	.reflog_expire = debug_reflog_expire,
 };
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0457ecdb42d..f95552f9263 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3291,30 +3291,30 @@ static int files_init_db(struct ref_store *ref_store, struct strbuf *err)
 }
 
 struct ref_storage_be refs_be_files = {
-	NULL,
-	"files",
-	files_ref_store_create,
-	files_init_db,
-	files_transaction_prepare,
-	files_transaction_finish,
-	files_transaction_abort,
-	files_initial_transaction_commit,
-
-	files_pack_refs,
-	files_create_symref,
-	files_delete_refs,
-	files_rename_ref,
-	files_copy_ref,
-
-	files_ref_iterator_begin,
-	files_read_raw_ref,
-	files_read_symbolic_ref,
-
-	files_reflog_iterator_begin,
-	files_for_each_reflog_ent,
-	files_for_each_reflog_ent_reverse,
-	files_reflog_exists,
-	files_create_reflog,
-	files_delete_reflog,
-	files_reflog_expire
+	.next = NULL,
+	.name = "files",
+	.init = files_ref_store_create,
+	.init_db = files_init_db,
+	.transaction_prepare = files_transaction_prepare,
+	.transaction_finish = files_transaction_finish,
+	.transaction_abort = files_transaction_abort,
+	.initial_transaction_commit = files_initial_transaction_commit,
+
+	.pack_refs = files_pack_refs,
+	.create_symref = files_create_symref,
+	.delete_refs = files_delete_refs,
+	.rename_ref = files_rename_ref,
+	.copy_ref = files_copy_ref,
+
+	.iterator_begin = files_ref_iterator_begin,
+	.read_raw_ref = files_read_raw_ref,
+	.read_symbolic_ref = files_read_symbolic_ref,
+
+	.reflog_iterator_begin = files_reflog_iterator_begin,
+	.for_each_reflog_ent = files_for_each_reflog_ent,
+	.for_each_reflog_ent_reverse = files_for_each_reflog_ent_reverse,
+	.reflog_exists = files_reflog_exists,
+	.create_reflog = files_create_reflog,
+	.delete_reflog = files_delete_reflog,
+	.reflog_expire = files_reflog_expire
 };
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index f56e2cc635b..47f01fa5c98 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1667,30 +1667,30 @@ static int packed_reflog_expire(struct ref_store *ref_store,
 }
 
 struct ref_storage_be refs_be_packed = {
-	NULL,
-	"packed",
-	packed_ref_store_create,
-	packed_init_db,
-	packed_transaction_prepare,
-	packed_transaction_finish,
-	packed_transaction_abort,
-	packed_initial_transaction_commit,
-
-	packed_pack_refs,
-	packed_create_symref,
-	packed_delete_refs,
-	packed_rename_ref,
-	packed_copy_ref,
-
-	packed_ref_iterator_begin,
-	packed_read_raw_ref,
-	NULL,
-
-	packed_reflog_iterator_begin,
-	packed_for_each_reflog_ent,
-	packed_for_each_reflog_ent_reverse,
-	packed_reflog_exists,
-	packed_create_reflog,
-	packed_delete_reflog,
-	packed_reflog_expire
+	.next = NULL,
+	.name = "packed",
+	.init = packed_ref_store_create,
+	.init_db = packed_init_db,
+	.transaction_prepare = packed_transaction_prepare,
+	.transaction_finish = packed_transaction_finish,
+	.transaction_abort = packed_transaction_abort,
+	.initial_transaction_commit = packed_initial_transaction_commit,
+
+	.pack_refs = packed_pack_refs,
+	.create_symref = packed_create_symref,
+	.delete_refs = packed_delete_refs,
+	.rename_ref = packed_rename_ref,
+	.copy_ref = packed_copy_ref,
+
+	.iterator_begin = packed_ref_iterator_begin,
+	.read_raw_ref = packed_read_raw_ref,
+	.read_symbolic_ref = NULL,
+
+	.reflog_iterator_begin = packed_reflog_iterator_begin,
+	.for_each_reflog_ent = packed_for_each_reflog_ent,
+	.for_each_reflog_ent_reverse = packed_for_each_reflog_ent_reverse,
+	.reflog_exists = packed_reflog_exists,
+	.create_reflog = packed_create_reflog,
+	.delete_reflog = packed_delete_reflog,
+	.reflog_expire = packed_reflog_expire
 };
-- 
2.35.1.1384.g7d2906948a1

[PATCH 2/5] refs: use designated initializers for "struct ref_iterator_vtable"

[Ævar Arnfjörð Bjarmason]

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/debug.c          |  5 +++--
 refs/files-backend.c  | 12 ++++++------
 refs/iterator.c       | 18 +++++++++---------
 refs/packed-backend.c |  6 +++---
 refs/ref-cache.c      |  6 +++---
 5 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/refs/debug.c b/refs/debug.c
index b03a83258be..b83b5817118 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -220,8 +220,9 @@ static int debug_ref_iterator_abort(struct ref_iterator *ref_iterator)
 }
 
 static struct ref_iterator_vtable debug_ref_iterator_vtable = {
-	debug_ref_iterator_advance, debug_ref_iterator_peel,
-	debug_ref_iterator_abort
+	.advance = debug_ref_iterator_advance,
+	.peel = debug_ref_iterator_peel,
+	.abort = debug_ref_iterator_abort,
 };
 
 static struct ref_iterator *
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f95552f9263..aa4e7182b6e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -822,9 +822,9 @@ static int files_ref_iterator_abort(struct ref_iterator *ref_iterator)
 }
 
 static struct ref_iterator_vtable files_ref_iterator_vtable = {
-	files_ref_iterator_advance,
-	files_ref_iterator_peel,
-	files_ref_iterator_abort
+	.advance = files_ref_iterator_advance,
+	.peel = files_ref_iterator_peel,
+	.abort = files_ref_iterator_abort,
 };
 
 static struct ref_iterator *files_ref_iterator_begin(
@@ -2231,9 +2231,9 @@ static int files_reflog_iterator_abort(struct ref_iterator *ref_iterator)
 }
 
 static struct ref_iterator_vtable files_reflog_iterator_vtable = {
-	files_reflog_iterator_advance,
-	files_reflog_iterator_peel,
-	files_reflog_iterator_abort
+	.advance = files_reflog_iterator_advance,
+	.peel = files_reflog_iterator_peel,
+	.abort = files_reflog_iterator_abort,
 };
 
 static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
diff --git a/refs/iterator.c b/refs/iterator.c
index a89d132d4fe..b2e56bae1c6 100644
--- a/refs/iterator.c
+++ b/refs/iterator.c
@@ -64,9 +64,9 @@ static int empty_ref_iterator_abort(struct ref_iterator *ref_iterator)
 }
 
 static struct ref_iterator_vtable empty_ref_iterator_vtable = {
-	empty_ref_iterator_advance,
-	empty_ref_iterator_peel,
-	empty_ref_iterator_abort
+	.advance = empty_ref_iterator_advance,
+	.peel = empty_ref_iterator_peel,
+	.abort = empty_ref_iterator_abort,
 };
 
 struct ref_iterator *empty_ref_iterator_begin(void)
@@ -201,9 +201,9 @@ static int merge_ref_iterator_abort(struct ref_iterator *ref_iterator)
 }
 
 static struct ref_iterator_vtable merge_ref_iterator_vtable = {
-	merge_ref_iterator_advance,
-	merge_ref_iterator_peel,
-	merge_ref_iterator_abort
+	.advance = merge_ref_iterator_advance,
+	.peel = merge_ref_iterator_peel,
+	.abort = merge_ref_iterator_abort,
 };
 
 struct ref_iterator *merge_ref_iterator_begin(
@@ -378,9 +378,9 @@ static int prefix_ref_iterator_abort(struct ref_iterator *ref_iterator)
 }
 
 static struct ref_iterator_vtable prefix_ref_iterator_vtable = {
-	prefix_ref_iterator_advance,
-	prefix_ref_iterator_peel,
-	prefix_ref_iterator_abort
+	.advance = prefix_ref_iterator_advance,
+	.peel = prefix_ref_iterator_peel,
+	.abort = prefix_ref_iterator_abort,
 };
 
 struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 47f01fa5c98..03002451f15 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -911,9 +911,9 @@ static int packed_ref_iterator_abort(struct ref_iterator *ref_iterator)
 }
 
 static struct ref_iterator_vtable packed_ref_iterator_vtable = {
-	packed_ref_iterator_advance,
-	packed_ref_iterator_peel,
-	packed_ref_iterator_abort
+	.advance = packed_ref_iterator_advance,
+	.peel = packed_ref_iterator_peel,
+	.abort = packed_ref_iterator_abort
 };
 
 static struct ref_iterator *packed_ref_iterator_begin(
diff --git a/refs/ref-cache.c b/refs/ref-cache.c
index be4aa5e0981..3080ef944d9 100644
--- a/refs/ref-cache.c
+++ b/refs/ref-cache.c
@@ -456,9 +456,9 @@ static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator)
 }
 
 static struct ref_iterator_vtable cache_ref_iterator_vtable = {
-	cache_ref_iterator_advance,
-	cache_ref_iterator_peel,
-	cache_ref_iterator_abort
+	.advance = cache_ref_iterator_advance,
+	.peel = cache_ref_iterator_peel,
+	.abort = cache_ref_iterator_abort
 };
 
 struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache,
-- 
2.35.1.1384.g7d2906948a1

[PATCH 3/5] misc *.c: use designated initializers for struct assignments

[Ævar Arnfjörð Bjarmason]

Change a few miscellaneous non-designated initializer assignments to
use designated initializers.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 attr.c        | 2 +-
 notes-merge.c | 1 +
 object-file.c | 9 +++++----
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/attr.c b/attr.c
index 79adaa50ea1..9ad12578cce 100644
--- a/attr.c
+++ b/attr.c
@@ -80,7 +80,7 @@ static int attr_hash_entry_cmp(const void *unused_cmp_data,
  * Access to this dictionary must be surrounded with a mutex.
  */
 static struct attr_hashmap g_attr_hashmap = {
-	HASHMAP_INIT(attr_hash_entry_cmp, NULL)
+	.map = HASHMAP_INIT(attr_hash_entry_cmp, NULL),
 };
 
 /*
diff --git a/notes-merge.c b/notes-merge.c
index 878b6c571b9..b4cc594a790 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -113,6 +113,7 @@ static struct notes_merge_pair *find_notes_merge_pair_pos(
 }
 
 static struct object_id uninitialized = {
+	.hash =
 	"\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff" \
 	"\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff"
 };
diff --git a/object-file.c b/object-file.c
index bdc5cbdd386..f0a75b3ff1e 100644
--- a/object-file.c
+++ b/object-file.c
@@ -274,10 +274,11 @@ static struct cached_object {
 static int cached_object_nr, cached_object_alloc;
 
 static struct cached_object empty_tree = {
-	{ EMPTY_TREE_SHA1_BIN_LITERAL },
-	OBJ_TREE,
-	"",
-	0
+	.oid = {
+		.hash = EMPTY_TREE_SHA1_BIN_LITERAL,
+	},
+	.type = OBJ_TREE,
+	.buf = "",
 };
 
 static struct cached_object *find_cached_object(const struct object_id *oid)
-- 
2.35.1.1384.g7d2906948a1

[PATCH 4/5] packed-backend: remove stub BUG(...) functions

[Ævar Arnfjörð Bjarmason]

Remove the stub BUG(...) functions previously used by the "struct
ref_storage_be refs_be_packed" backend.

We never call any functions in the packed backend by using it as a
"normal" primary ref store, instead we'll always initialize a "files"
backend ref-store.

It will then via the "packed_ref_store" member of "struct
files_ref_store" call selected functions in the "packed" backend, and
we'll in addition call others via wrappers in refs.c.

So while these would arguably give us *slightly* more meaningful error
messages we'll NULL the missing members in the initializer anyway, so
we'll reliably get a segfault if we're ever changing the backend and
having it call something it doesn't have.

So there's no need for this verbose boilerplate, and as shown in a
subsequent commit it might even lead to some confusion about the
packed backend being a "real" backend. Let's make it clear that it's
not.

As an aside, this also fixes a warning emitted by SunCC in at least
versions 12.5 and 12.6 of Oracle Developer Studio:

    "refs/packed-backend.c", line 1599: warning: Function has no return statement : packed_create_symref
    "refs/packed-backend.c", line 1606: warning: Function has no return statement : packed_rename_ref)
    "refs/packed-backend.c", line 1613: warning: Function has no return statement : packed_copy_ref
    "refs/packed-backend.c", line 1648: warning: Function has no return statement : packed_create_reflog

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs/packed-backend.c | 88 +++++--------------------------------------
 1 file changed, 9 insertions(+), 79 deletions(-)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 03002451f15..310c2a72026 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1591,81 +1591,11 @@ static int packed_pack_refs(struct ref_store *ref_store, unsigned int flags)
 	return 0;
 }
 
-static int packed_create_symref(struct ref_store *ref_store,
-			       const char *refname, const char *target,
-			       const char *logmsg)
-{
-	BUG("packed reference store does not support symrefs");
-}
-
-static int packed_rename_ref(struct ref_store *ref_store,
-			    const char *oldrefname, const char *newrefname,
-			    const char *logmsg)
-{
-	BUG("packed reference store does not support renaming references");
-}
-
-static int packed_copy_ref(struct ref_store *ref_store,
-			   const char *oldrefname, const char *newrefname,
-			   const char *logmsg)
-{
-	BUG("packed reference store does not support copying references");
-}
-
 static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_store)
 {
 	return empty_ref_iterator_begin();
 }
 
-static int packed_for_each_reflog_ent(struct ref_store *ref_store,
-				      const char *refname,
-				      each_reflog_ent_fn fn, void *cb_data)
-{
-	BUG("packed reference store does not support reflogs");
-	return 0;
-}
-
-static int packed_for_each_reflog_ent_reverse(struct ref_store *ref_store,
-					      const char *refname,
-					      each_reflog_ent_fn fn,
-					      void *cb_data)
-{
-	BUG("packed reference store does not support reflogs");
-	return 0;
-}
-
-static int packed_reflog_exists(struct ref_store *ref_store,
-			       const char *refname)
-{
-	BUG("packed reference store does not support reflogs");
-	return 0;
-}
-
-static int packed_create_reflog(struct ref_store *ref_store,
-				const char *refname, struct strbuf *err)
-{
-	BUG("packed reference store does not support reflogs");
-}
-
-static int packed_delete_reflog(struct ref_store *ref_store,
-			       const char *refname)
-{
-	BUG("packed reference store does not support reflogs");
-	return 0;
-}
-
-static int packed_reflog_expire(struct ref_store *ref_store,
-				const char *refname,
-				unsigned int flags,
-				reflog_expiry_prepare_fn prepare_fn,
-				reflog_expiry_should_prune_fn should_prune_fn,
-				reflog_expiry_cleanup_fn cleanup_fn,
-				void *policy_cb_data)
-{
-	BUG("packed reference store does not support reflogs");
-	return 0;
-}
-
 struct ref_storage_be refs_be_packed = {
 	.next = NULL,
 	.name = "packed",
@@ -1677,20 +1607,20 @@ struct ref_storage_be refs_be_packed = {
 	.initial_transaction_commit = packed_initial_transaction_commit,
 
 	.pack_refs = packed_pack_refs,
-	.create_symref = packed_create_symref,
+	.create_symref = NULL,
 	.delete_refs = packed_delete_refs,
-	.rename_ref = packed_rename_ref,
-	.copy_ref = packed_copy_ref,
+	.rename_ref = NULL,
+	.copy_ref = NULL,
 
 	.iterator_begin = packed_ref_iterator_begin,
 	.read_raw_ref = packed_read_raw_ref,
 	.read_symbolic_ref = NULL,
 
 	.reflog_iterator_begin = packed_reflog_iterator_begin,
-	.for_each_reflog_ent = packed_for_each_reflog_ent,
-	.for_each_reflog_ent_reverse = packed_for_each_reflog_ent_reverse,
-	.reflog_exists = packed_reflog_exists,
-	.create_reflog = packed_create_reflog,
-	.delete_reflog = packed_delete_reflog,
-	.reflog_expire = packed_reflog_expire
+	.for_each_reflog_ent = NULL,
+	.for_each_reflog_ent_reverse = NULL,
+	.reflog_exists = NULL,
+	.create_reflog = NULL,
+	.delete_reflog = NULL,
+	.reflog_expire = NULL,
 };
-- 
2.35.1.1384.g7d2906948a1

Re: [PATCH 4/5] packed-backend: remove stub BUG(...) functions

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> -	.for_each_reflog_ent = packed_for_each_reflog_ent,
> -	.for_each_reflog_ent_reverse = packed_for_each_reflog_ent_reverse,
> -	.reflog_exists = packed_reflog_exists,
> -	.create_reflog = packed_create_reflog,
> -	.delete_reflog = packed_delete_reflog,
> -	.reflog_expire = packed_reflog_expire
> +	.for_each_reflog_ent = NULL,
> +	.for_each_reflog_ent_reverse = NULL,
> +	.reflog_exists = NULL,
> +	.create_reflog = NULL,
> +	.delete_reflog = NULL,
> +	.reflog_expire = NULL,

Yup, the entry in vtable being set to NULL is a more direct sign the
readers would appreciate than a name natural for the method, whose
definition needs to be consulted before the readers can realize that
it is a "missing" method.

Very sensible.

Thanks.

[PATCH 5/5] refs debug: add a wrapper for "read_symbolic_ref"

[Ævar Arnfjörð Bjarmason]

In cd475b3b038 (refs: add ability for backends to special-case reading
of symbolic refs, 2022-03-01) when the "read_symbolic_ref" callback
was added we'd fall back on "refs_read_raw_ref" if there wasn't any
backend implementation of "read_symbolic_ref".

As discussed in the preceding commit this would only happen if we were
running the "debug" backend, e.g. in the "setup for ref completion"
test in t9902-completion.sh with:

    GIT_TRACE_REFS=1 git fetch --no-tags other

Let's improve the trace output, but and also eliminate the
now-redundant refs_read_raw_ref() fallback case. As noted in the
preceding commit the "packed" backend will never call
refs_read_symbolic_ref() (nor is it ever going to). For any future
backend such as reftable it's OK to ask that they either implement
this (or a wrapper) themselves.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 refs.c       | 13 +------------
 refs/debug.c | 27 ++++++++++++++++++++++++++-
 2 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/refs.c b/refs.c
index 0b79bdd7c37..1a964505f92 100644
--- a/refs.c
+++ b/refs.c
@@ -1676,18 +1676,7 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
 int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname,
 			   struct strbuf *referent)
 {
-	struct object_id oid;
-	int ret, failure_errno = 0;
-	unsigned int type = 0;
-
-	if (ref_store->be->read_symbolic_ref)
-		return ref_store->be->read_symbolic_ref(ref_store, refname, referent);
-
-	ret = refs_read_raw_ref(ref_store, refname, &oid, referent, &type, &failure_errno);
-	if (ret || !(type & REF_ISSYMREF))
-		return -1;
-
-	return 0;
+	return ref_store->be->read_symbolic_ref(ref_store, refname, referent);
 }
 
 const char *refs_resolve_ref_unsafe(struct ref_store *refs,
diff --git a/refs/debug.c b/refs/debug.c
index b83b5817118..eed8bc94b04 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -262,6 +262,24 @@ static int debug_read_raw_ref(struct ref_store *ref_store, const char *refname,
 	return res;
 }
 
+static int debug_read_symbolic_ref(struct ref_store *ref_store, const char *refname,
+				   struct strbuf *referent)
+{
+	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
+	struct ref_store *refs = drefs->refs;
+	int res;
+
+	res = refs->be->read_symbolic_ref(refs, refname, referent);
+	if (!res)
+		trace_printf_key(&trace_refs, "read_symbolic_ref: %s: (%s)\n",
+				 refname, referent->buf);
+	else
+		trace_printf_key(&trace_refs,
+				 "read_symbolic_ref: %s: %d\n", refname, res);
+	return res;
+
+}
+
 static struct ref_iterator *
 debug_reflog_iterator_begin(struct ref_store *ref_store)
 {
@@ -423,6 +441,13 @@ struct ref_storage_be refs_be_debug = {
 	.name = "debug",
 	.init = NULL,
 	.init_db = debug_init_db,
+
+	/*
+	 * None of these should be NULL. If the "files" backend (in
+	 * "struct ref_storage_be refs_be_files" in files-backend.c)
+	 * has a function we should also have a wrapper for it here.
+	 * Test the output with "GIT_TRACE_REFS=1".
+	 */
 	.transaction_prepare = debug_transaction_prepare,
 	.transaction_finish = debug_transaction_finish,
 	.transaction_abort = debug_transaction_abort,
@@ -436,7 +461,7 @@ struct ref_storage_be refs_be_debug = {
 
 	.iterator_begin = debug_ref_iterator_begin,
 	.read_raw_ref = debug_read_raw_ref,
-	.read_symbolic_ref = NULL,
+	.read_symbolic_ref = debug_read_symbolic_ref,
 
 	.reflog_iterator_begin = debug_reflog_iterator_begin,
 	.for_each_reflog_ent = debug_for_each_reflog_ent,
-- 
2.35.1.1384.g7d2906948a1

Re: [PATCH 0/5] refs: designated init & missing debug in ps/fetch-mirror-optim

[Han-Wen Nienhuys]

On Thu, Mar 17, 2022 at 6:27 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Aside: It seems that the GIT_TRACE_REFS facility has been broken since
> it was added, i.e. running the test site with e.g.:
>
>     GIT_TRACE_REFS=/tmp/log.txt make test

I wasn't aware that this is even possible. I've only used it with
GIT_TRACE_REFS=1

I looked over your patches and they LGTM.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.

Re: [PATCH 0/5] refs: designated init & missing debug in ps/fetch-mirror-optim

[Ævar Arnfjörð Bjarmason]

On Thu, Mar 17 2022, Han-Wen Nienhuys wrote:

> On Thu, Mar 17, 2022 at 6:27 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> Aside: It seems that the GIT_TRACE_REFS facility has been broken since
>> it was added, i.e. running the test site with e.g.:
>>
>>     GIT_TRACE_REFS=/tmp/log.txt make test
>
> I wasn't aware that this is even possible. I've only used it with
> GIT_TRACE_REFS=1

Yes, it's possible with all the GIT_TRACE* variables. To clarify the
failure has nothing to do with the "log to file" feature of it, the same
happens e.g. with this on master:

    GIT_TRACE_REFS=1 ./t1400-update-ref.sh -vx --run=1-46,55-169

I.e. here I skipped some tests that would fail because we emitted the
output on stderr, but this one fails due to the debug tracing and
transactions (I think, but not 100% sure) tripping over one another
somehow.

> I looked over your patches and they LGTM.

Thanks!

Re: [PATCH 0/5] refs: designated init & missing debug in ps/fetch-mirror-optim

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 535 bytes --]

On Thu, Mar 17, 2022 at 06:45:13PM +0100, Han-Wen Nienhuys wrote:
> On Thu, Mar 17, 2022 at 6:27 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> > Aside: It seems that the GIT_TRACE_REFS facility has been broken since
> > it was added, i.e. running the test site with e.g.:
> >
> >     GIT_TRACE_REFS=/tmp/log.txt make test
> 
> I wasn't aware that this is even possible. I've only used it with
> GIT_TRACE_REFS=1
> 
> I looked over your patches and they LGTM.

Seconded, the patches look good to me.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/5] refs: designated init & missing debug in ps/fetch-mirror-optim

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Thu, Mar 17, 2022 at 06:45:13PM +0100, Han-Wen Nienhuys wrote:
>> On Thu, Mar 17, 2022 at 6:27 PM Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> wrote:
>> > Aside: It seems that the GIT_TRACE_REFS facility has been broken since
>> > it was added, i.e. running the test site with e.g.:
>> >
>> >     GIT_TRACE_REFS=/tmp/log.txt make test
>> 
>> I wasn't aware that this is even possible. I've only used it with
>> GIT_TRACE_REFS=1
>> 
>> I looked over your patches and they LGTM.
>
> Seconded, the patches look good to me.

Thanks, all.  The topic will be in 'master' hopefully by the end of
the week (I'll be offline part of today, mostly tomorrow and also
Friday, so things may slow, though).