threads / patch / 57577

patch, 5 partsrefs: designated init & missing debug in ps/fetch-mirror-optim

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

## tl;dr

11 messages between Mar 17, 2022 and Mar 21, 2022. Diffs are folded; open one to read it.

replies: 10people: 4as markdown or json

Ævar Arnfjörð Bjarmason· Mar 17, 2022, 17:27 UTC · lore

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
Ævar Arnfjörð Bjarmason· Mar 17, 2022, 17:27 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 3 files +78 −78

refs/debug.c, refs/files-backend.c, refs/packed-backend.c

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
Ævar Arnfjörð Bjarmason· Mar 17, 2022, 17:27 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 5 files +24 −23

refs/debug.c, refs/files-backend.c, refs/iterator.c, refs/packed-backend.c, refs/ref-cache.c

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
Ævar Arnfjörð Bjarmason· Mar 17, 2022, 17:27 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 3 files +7 −5

attr.c, notes-merge.c, object-file.c

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
Ævar Arnfjörð Bjarmason· Mar 17, 2022, 17:27 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to refs/packed-backend.c +9 −79
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
Junio C Hamano· Mar 17, 2022, 17:39 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 12 quoted lines
> -	.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.
Ævar Arnfjörð Bjarmason· Mar 17, 2022, 17:27 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 2 files +27 −13

refs.c, refs/debug.c

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
Han-Wen Nienhuys· Mar 17, 2022, 17:45 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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.
Ævar Arnfjörð Bjarmason· Mar 18, 2022, 12:40 UTC · re: Han-Wen Nienhuys · lore

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

On Thu, Mar 17 2022, Han-Wen Nienhuys wrote:
Show 9 quoted lines
> 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!
Patrick Steinhardt· Mar 21, 2022, 11:10 UTC · re: Han-Wen Nienhuys · lore

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

On Thu, Mar 17, 2022 at 06:45:13PM +0100, Han-Wen Nienhuys wrote:
Show 11 quoted lines
> 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
Junio C Hamano· Mar 21, 2022, 19:10 UTC · re: Patrick Steinhardt · lore

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

Patrick Steinhardt <ps@pks.im> writes:
Show 14 quoted lines
> 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).

← back to recent threads