tweaks to refs/debug.c

PATCH Superseded Han-Wen Nienhuys via GitGitGadget 3 participants 10 messages Dec 21, 2021 – Dec 22, 2021

lore

Share & tools

lore

Han-Wen Nienhuys via GitGitGadget

author

2021-12-21 12:34 UTC #0 lore raw

cover tweaks to refs/debug.c

these are two small commits that helped me while debugging reftable support for git.

Han-Wen Nienhuys (2):
refs: print error message in debug output
refs: set the repo in debug_ref_store.base

 refs/debug.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

base-commit: e773545c7fe7eca21b134847f4fc2cbc9547fa14
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1163%2Fhanwen%2Fdebug-tweaks-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1163/hanwen/debug-tweaks-v1
Pull-Request: https://github.com/git/git/pull/1163

Signature · 1 line

gitgitgadget

Han-Wen Nienhuys via GitGitGadget

author

2021-12-21 12:34 UTC #1 lore raw

PATCH 1/2 refs: print error message in debug output

From: Han-Wen Nienhuys <[email protected]>

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 refs/debug.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

refs/debug.c +2 -1

index 756d07c7247..cf6ad36fbb0 100644
@@ -47,7 +47,8 @@ static int debug_transaction_prepare(struct ref_store *refs,
 	transaction->ref_store = drefs->refs;
 	res = drefs->refs->be->transaction_prepare(drefs->refs, transaction,
 						   err);
-	trace_printf_key(&trace_refs, "transaction_prepare: %d\n", res);
+	trace_printf_key(&trace_refs, "transaction_prepare: %d \"%s\"\n", res,
+			 err->buf);
 	return res;
 }

Signature · 1 line

gitgitgadget

Han-Wen Nienhuys via GitGitGadget

author

2021-12-21 12:34 UTC #2 lore raw

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

In reply to Han-Wen Nienhuys via GitGitGadget

From: Han-Wen Nienhuys <[email protected]>

This is for consistency with the files backend.

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 refs/debug.c | 1 +
 1 file changed, 1 insertion(+)

refs/debug.c +1 -0

index cf6ad36fbb0..136cfd7c700 100644
@@ -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;
 }

Signature · 1 line

gitgitgadget

Junio C Hamano

2021-12-22 05:58 UTC #3 lore raw

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

Quoted 3 lines · Han-Wen Nienhuys via GitGitGadget

From: Han-Wen Nienhuys <[email protected]>
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 10 lines

Signed-off-by Han-Wen Nienhuys <[email protected]>
---
 refs/debug.c | 1 +
 1 file changed, 1 insertion(+)
refs/debug.c +0 -0
index cf6ad36fbb0..136cfd7c700 100644

refs/debug.c struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_stor · line 32

Quoted 7 lines

refs/debug.c +1 -0

@@ -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.

Han-Wen Nienhuys

2021-12-22 18:13 UTC #4 lore raw

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

Quoted 14 lines · Junio C Hamano

Quoted 3 lines · Han-Wen Nienhuys via GitGitGadget
From: Han-Wen Nienhuys <[email protected]>
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?

Quoted 5 lines

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.)

Signature · 11 lines

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

Han-Wen Nienhuys via GitGitGadget

author

2021-12-22 18:11 UTC #5 lore raw

v2 cover tweaks to refs/debug.c

In reply to Han-Wen Nienhuys via GitGitGadget

these are two small commits that helped me while debugging reftable support for git.

Han-Wen Nienhuys (3):
refs: pass gitdir to packed_ref_store_create
refs: print error message in debug output
refs: centralize initialization of the base ref_store.

 refs.c                |  6 ++++--
 refs/debug.c          |  6 ++++--
 refs/files-backend.c  | 10 +++-------
 refs/packed-backend.c | 11 +++++------
 refs/packed-backend.h |  2 +-
 refs/refs-internal.h  |  4 ++--
 6 files changed, 19 insertions(+), 20 deletions(-)

base-commit: 69a9c10c95e28df457e33b3c7400b16caf2e2962
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1163%2Fhanwen%2Fdebug-tweaks-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1163/hanwen/debug-tweaks-v2
Pull-Request: https://github.com/git/git/pull/1163

Range-diff vs v1:

 -:  ----------- > 1:  bfebb5f08fe refs: pass gitdir to packed_ref_store_create
 1:  177d84f8563 = 2:  b189f8661e2 refs: print error message in debug output
 2:  75e5392032d < -:  ----------- refs: set the repo in debug_ref_store.base
 -:  ----------- > 3:  eea294b688f refs: centralize initialization of the base ref_store.

Signature · 1 line

gitgitgadget

Han-Wen Nienhuys via GitGitGadget

author

2021-12-22 18:12 UTC #6 lore raw

PATCH v2 1/3 refs: pass gitdir to packed_ref_store_create

From: Han-Wen Nienhuys <[email protected]>

This is consistent with the calling convention for ref backend creation, and avoids storing ".git/packed-refs" (the name of a regular file) in a variable called ref_store::gitdir.

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 refs/files-backend.c  | 5 ++---
 refs/packed-backend.c | 9 +++++----
 refs/packed-backend.h | 2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

refs/files-backend.c +2 -3

index 90b671025a7..f1b66130dfb 100644
@@ -93,9 +93,8 @@ static struct ref_store *files_ref_store_create(struct repository *repo,
 
 	get_common_dir_noenv(&sb, gitdir);
 	refs->gitcommondir = strbuf_detach(&sb, NULL);
-	strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir);
-	refs->packed_ref_store = packed_ref_store_create(repo, sb.buf, flags);
-	strbuf_release(&sb);
+	refs->packed_ref_store =
+		packed_ref_store_create(repo, refs->gitcommondir, flags);
 
 	chdir_notify_reparent("files-backend $GIT_DIR", &refs->base.gitdir);
 	chdir_notify_reparent("files-backend $GIT_COMMONDIR",

refs/packed-backend.c +5 -4

index 67152c664e2..caa1957252a 100644
@@ -194,20 +194,21 @@ static int release_snapshot(struct snapshot *snapshot)
 }
 
 struct ref_store *packed_ref_store_create(struct repository *repo,
-					  const char *path,
+					  const char *gitdir,
 					  unsigned int store_flags)
 {
 	struct packed_ref_store *refs = xcalloc(1, sizeof(*refs));
 	struct ref_store *ref_store = (struct ref_store *)refs;
+	struct strbuf sb = STRBUF_INIT;
 
 	base_ref_store_init(ref_store, &refs_be_packed);
 	ref_store->repo = repo;
-	ref_store->gitdir = xstrdup(path);
+	ref_store->gitdir = xstrdup(gitdir);
 	refs->store_flags = store_flags;
+	strbuf_addf(&sb, "%s/packed-refs", gitdir);
+	refs->path = strbuf_detach(&sb, NULL);
 
-	refs->path = xstrdup(path);
 	chdir_notify_reparent("packed-refs", &refs->path);
-
 	return ref_store;
 }

refs/packed-backend.h +1 -1

index f61a73ec25b..9dd8a344c34 100644
@@ -14,7 +14,7 @@ struct ref_transaction;
  */
 
 struct ref_store *packed_ref_store_create(struct repository *repo,
-					  const char *path,
+					  const char *gitdir,
 					  unsigned int store_flags);
 
 /*

Signature · 1 line

gitgitgadget

Han-Wen Nienhuys via GitGitGadget

author

2021-12-22 18:12 UTC #7 lore raw

PATCH v2 2/3 refs: print error message in debug output

In reply to Han-Wen Nienhuys via GitGitGadget

From: Han-Wen Nienhuys <[email protected]>

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 refs/debug.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

refs/debug.c +2 -1

index 791423c6a7d..8a6bb157ee6 100644
@@ -47,7 +47,8 @@ static int debug_transaction_prepare(struct ref_store *refs,
 	transaction->ref_store = drefs->refs;
 	res = drefs->refs->be->transaction_prepare(drefs->refs, transaction,
 						   err);
-	trace_printf_key(&trace_refs, "transaction_prepare: %d\n", res);
+	trace_printf_key(&trace_refs, "transaction_prepare: %d \"%s\"\n", res,
+			 err->buf);
 	return res;
 }

Signature · 1 line

gitgitgadget

Han-Wen Nienhuys via GitGitGadget

author

2021-12-22 18:12 UTC #8 lore raw

PATCH v2 3/3 refs: centralize initialization of the base ref_store.

In reply to Han-Wen Nienhuys via GitGitGadget

From: Han-Wen Nienhuys <[email protected]>

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 refs.c                | 6 ++++--
 refs/debug.c          | 3 ++-
 refs/files-backend.c  | 5 +----
 refs/packed-backend.c | 6 ++----
 refs/refs-internal.h  | 4 ++--
 5 files changed, 11 insertions(+), 13 deletions(-)

refs.c +4 -2

index 4c317955813..f91edb73075 100644
@@ -2007,10 +2007,12 @@ struct ref_store *get_worktree_ref_store(const struct worktree *wt)
 	return refs;
 }
 
-void base_ref_store_init(struct ref_store *refs,
-			 const struct ref_storage_be *be)
+void base_ref_store_init(struct ref_store *refs, struct repository *repo,
+			 const char *path, const struct ref_storage_be *be)
 {
 	refs->be = be;
+	refs->repo = repo;
+	refs->gitdir = xstrdup(path);
 }
 
 /* backend functions */

refs/debug.c +2 -1

index 8a6bb157ee6..2b0771ca53b 100644
@@ -26,7 +26,8 @@ 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;
-	base_ref_store_init((struct ref_store *)res, be_copy);
+	base_ref_store_init((struct ref_store *)res, store->repo, gitdir,
+			    be_copy);
 	return (struct ref_store *)res;
 }

refs/files-backend.c +1 -4

index f1b66130dfb..5c6d49a267e 100644
@@ -86,11 +86,8 @@ static struct ref_store *files_ref_store_create(struct repository *repo,
 	struct ref_store *ref_store = (struct ref_store *)refs;
 	struct strbuf sb = STRBUF_INIT;
 
-	ref_store->repo = repo;
-	ref_store->gitdir = xstrdup(gitdir);
-	base_ref_store_init(ref_store, &refs_be_files);
+	base_ref_store_init(ref_store, repo, gitdir, &refs_be_files);
 	refs->store_flags = flags;
-
 	get_common_dir_noenv(&sb, gitdir);
 	refs->gitcommondir = strbuf_detach(&sb, NULL);
 	refs->packed_ref_store =

refs/packed-backend.c +2 -4

index caa1957252a..d91a2018f60 100644
@@ -201,13 +201,11 @@ struct ref_store *packed_ref_store_create(struct repository *repo,
 	struct ref_store *ref_store = (struct ref_store *)refs;
 	struct strbuf sb = STRBUF_INIT;
 
-	base_ref_store_init(ref_store, &refs_be_packed);
-	ref_store->repo = repo;
-	ref_store->gitdir = xstrdup(gitdir);
+	base_ref_store_init(ref_store, repo, gitdir, &refs_be_packed);
 	refs->store_flags = store_flags;
+
 	strbuf_addf(&sb, "%s/packed-refs", gitdir);
 	refs->path = strbuf_detach(&sb, NULL);
-
 	chdir_notify_reparent("packed-refs", &refs->path);
 	return ref_store;
 }

refs/refs-internal.h +2 -2

index 46a839539e3..7ff6fba4f0d 100644
@@ -710,8 +710,8 @@ int parse_loose_ref_contents(const char *buf, struct object_id *oid,
  * Fill in the generic part of refs and add it to our collection of
  * reference stores.
  */
-void base_ref_store_init(struct ref_store *refs,
-			 const struct ref_storage_be *be);
+void base_ref_store_init(struct ref_store *refs, struct repository *repo,
+			 const char *path, const struct ref_storage_be *be);
 
 /*
  * Support GIT_TRACE_REFS by optionally wrapping the given ref_store instance.

Signature · 1 line

gitgitgadget

Junio C Hamano

2021-12-22 21:54 UTC #9 lore raw

In reply to Han-Wen Nienhuys via GitGitGadget

Quoted 7 lines · Han-Wen Nienhuys via GitGitGadget

these are two small commits that helped me while debugging reftable support for git.
Han-Wen Nienhuys (3):
refs: pass gitdir to packed_ref_store_create
refs: print error message in debug output
refs: centralize initialization of the base ref_store.

Welcome to our "we cannot count to three" club ;-)

All three patches make sense to me. Especially the [3/3] has become much easier to understand.

Thanks, will queue.