Re: [PATCH 2/2] refs: set the repo in debug_ref_store.base

2 messages, 2 authors, 2021-12-22 · open the first message on its own page

Re: [PATCH 2/2] refs: set the repo in debug_ref_store.base

From: Junio C Hamano <hidden>
Date: 2021-12-22 05:58:48

"Han-Wen Nienhuys via GitGitGadget" [off-list ref] writes:
From: Han-Wen Nienhuys <redacted>

This is for consistency with the files backend.
Hmmmm.  Could you explain what it exactly means?

I can see that files_ref_store structure has the .repo member and
files_ref_store_create() uses it to remember which repository the
ref store is for, but that is an implementation detail that is not
exposed outside the files backend, isn't it?

To put it differently, what is broken with the current code that
leaves the .repo member in refs->base uninitialized?  We are
presumably helping the caller that wants to know the repository the
ref store belongs to via this pointer with this change---what is
that caller?
quoted hunk
Signed-off-by: Han-Wen Nienhuys <redacted>
---
 refs/debug.c | 1 +
 1 file changed, 1 insertion(+)
diff --git a/refs/debug.c b/refs/debug.c
index cf6ad36fbb0..136cfd7c700 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -26,6 +26,7 @@ struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_stor
 	be_copy->name = store->be->name;
 	trace_printf_key(&trace_refs, "ref_store for %s\n", gitdir);
 	res->refs = store;
+	res->base.repo = store->repo;
 	base_ref_store_init((struct ref_store *)res, be_copy);
 	return (struct ref_store *)res;
 }
Thanks.

Re: [PATCH 2/2] refs: set the repo in debug_ref_store.base

From: Han-Wen Nienhuys <hidden>
Date: 2021-12-22 18:13:25

On Wed, Dec 22, 2021 at 6:58 AM Junio C Hamano [off-list ref] wrote:
"Han-Wen Nienhuys via GitGitGadget" [off-list ref] writes:
quoted
From: Han-Wen Nienhuys <redacted>

This is for consistency with the files backend.
Hmmmm.  Could you explain what it exactly means?

I can see that files_ref_store structure has the .repo member and
files_ref_store_create() uses it to remember which repository the
ref store is for, but that is an implementation detail that is not
exposed outside the files backend, isn't it?

To put it differently, what is broken with the current code that
leaves the .repo member in refs->base uninitialized?  We are
presumably helping the caller that wants to know the repository the
ref store belongs to via this pointer with this change---what is
that caller?
It's confusing for the base ref_store to have fields that are
sometimes set and sometimes not. I sent an alternate take on this as
v2; hope you like that better.

(sorry, I forgot to update the cover letter.)

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

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help