[PATCH 0/2] tweaks to refs/debug.c

[PATCH 0/2] tweaks to refs/debug.c

[Han-Wen Nienhuys via GitGitGadget]

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
-- 
gitgitgadget

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

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 refs/debug.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/refs/debug.c b/refs/debug.c
index 756d07c7247..cf6ad36fbb0 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -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;
 }
 
-- 
gitgitgadget

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

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

This is for consistency with the files backend.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 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;
 }
-- 
gitgitgadget

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

[Junio C Hamano]

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> 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?

> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  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

[Han-Wen Nienhuys]

On Wed, Dec 22, 2021 at 6:58 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Han-Wen Nienhuys <hanwen@google.com>
> >
> > 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

[PATCH v2 0/3] tweaks to refs/debug.c

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

-- 
gitgitgadget

[PATCH v2 1/3] refs: pass gitdir to packed_ref_store_create

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

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 <hanwen@google.com>
---
 refs/files-backend.c  | 5 ++---
 refs/packed-backend.c | 9 +++++----
 refs/packed-backend.h | 2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 90b671025a7..f1b66130dfb 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -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",
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 67152c664e2..caa1957252a 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -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;
 }
 
diff --git a/refs/packed-backend.h b/refs/packed-backend.h
index f61a73ec25b..9dd8a344c34 100644
--- a/refs/packed-backend.h
+++ b/refs/packed-backend.h
@@ -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);
 
 /*
-- 
gitgitgadget

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

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 refs/debug.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/refs/debug.c b/refs/debug.c
index 791423c6a7d..8a6bb157ee6 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -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;
 }
 
-- 
gitgitgadget

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

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 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(-)

diff --git a/refs.c b/refs.c
index 4c317955813..f91edb73075 100644
--- a/refs.c
+++ b/refs.c
@@ -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 */
diff --git a/refs/debug.c b/refs/debug.c
index 8a6bb157ee6..2b0771ca53b 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -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;
 }
 
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f1b66130dfb..5c6d49a267e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -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 =
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index caa1957252a..d91a2018f60 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -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;
 }
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 46a839539e3..7ff6fba4f0d 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -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.
-- 
gitgitgadget

Re: [PATCH v2 0/3] tweaks to refs/debug.c

[Junio C Hamano]

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

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