{"thread":{"id":"57128","subject":"[PATCH 0/2] tweaks to refs/debug.c","startedAt":"2021-12-21T12:34:03Z","lastAt":"2021-12-22T21:54:34Z","messageCount":10,"participants":["Han-Wen Nienhuys via GitGitGadget","Junio C Hamano","Han-Wen Nienhuys"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"444604","messageId":"pull.1163.git.git.1640090038.gitgitgadget@gmail.com","threadId":"57128","inReplyTo":null,"subject":"[PATCH 0/2] tweaks to refs/debug.c","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-21T12:33:56Z","receivedAt":"2021-12-21T12:34:03Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"these are two small commits that helped me while debugging reftable support\nfor git.\n\nHan-Wen Nienhuys (2):\n  refs: print error message in debug output\n  refs: set the repo in debug_ref_store.base\n\n refs/debug.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\n\nbase-commit: e773545c7fe7eca21b134847f4fc2cbc9547fa14\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1163%2Fhanwen%2Fdebug-tweaks-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1163/hanwen/debug-tweaks-v1\nPull-Request: https://github.com/git/git/pull/1163\n-- \ngitgitgadget\n"},{"id":"444605","messageId":"177d84f8563341c688db8a5b14c043277cfc1106.1640090038.git.gitgitgadget@gmail.com","threadId":"57128","inReplyTo":"pull.1163.git.git.1640090038.gitgitgadget@gmail.com","subject":"[PATCH 1/2] refs: print error message in debug output","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-21T12:33:57Z","receivedAt":"2021-12-21T12:34:03Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n refs/debug.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 756d07c7247..cf6ad36fbb0 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -47,7 +47,8 @@ static int debug_transaction_prepare(struct ref_store *refs,\n \ttransaction->ref_store = drefs->refs;\n \tres = drefs->refs->be->transaction_prepare(drefs->refs, transaction,\n \t\t\t\t\t\t   err);\n-\ttrace_printf_key(&trace_refs, \"transaction_prepare: %d\\n\", res);\n+\ttrace_printf_key(&trace_refs, \"transaction_prepare: %d \\\"%s\\\"\\n\", res,\n+\t\t\t err->buf);\n \treturn res;\n }\n \n-- \ngitgitgadget\n\n"},{"id":"444606","messageId":"75e5392032dbdbdedf8a2b76a7098e4dc1133d82.1640090038.git.gitgitgadget@gmail.com","threadId":"57128","inReplyTo":"pull.1163.git.git.1640090038.gitgitgadget@gmail.com","subject":"[PATCH 2/2] refs: set the repo in debug_ref_store.base","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-21T12:33:58Z","receivedAt":"2021-12-21T12:34:03Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nThis is for consistency with the files backend.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n refs/debug.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/refs/debug.c b/refs/debug.c\nindex cf6ad36fbb0..136cfd7c700 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -26,6 +26,7 @@ struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_stor\n \tbe_copy->name = store->be->name;\n \ttrace_printf_key(&trace_refs, \"ref_store for %s\\n\", gitdir);\n \tres->refs = store;\n+\tres->base.repo = store->repo;\n \tbase_ref_store_init((struct ref_store *)res, be_copy);\n \treturn (struct ref_store *)res;\n }\n-- \ngitgitgadget\n"},{"id":"444746","messageId":"xmqqtuf1kwob.fsf@gitster.g","threadId":"57128","inReplyTo":"75e5392032dbdbdedf8a2b76a7098e4dc1133d82.1640090038.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] refs: set the repo in debug_ref_store.base","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-22T05:58:44Z","receivedAt":"2021-12-22T05:58:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Han-Wen Nienhuys via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Han-Wen Nienhuys <hanwen@google.com>\n>\n> This is for consistency with the files backend.\n\nHmmmm.  Could you explain what it exactly means?\n\nI can see that files_ref_store structure has the .repo member and\nfiles_ref_store_create() uses it to remember which repository the\nref store is for, but that is an implementation detail that is not\nexposed outside the files backend, isn't it?\n\nTo put it differently, what is broken with the current code that\nleaves the .repo member in refs->base uninitialized?  We are\npresumably helping the caller that wants to know the repository the\nref store belongs to via this pointer with this change---what is\nthat caller?\n\n> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>\n> ---\n>  refs/debug.c | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/refs/debug.c b/refs/debug.c\n> index cf6ad36fbb0..136cfd7c700 100644\n> --- a/refs/debug.c\n> +++ b/refs/debug.c\n> @@ -26,6 +26,7 @@ struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_stor\n>  \tbe_copy->name = store->be->name;\n>  \ttrace_printf_key(&trace_refs, \"ref_store for %s\\n\", gitdir);\n>  \tres->refs = store;\n> +\tres->base.repo = store->repo;\n>  \tbase_ref_store_init((struct ref_store *)res, be_copy);\n>  \treturn (struct ref_store *)res;\n>  }\n\nThanks.\n"},{"id":"444790","messageId":"pull.1163.v2.git.git.1640196714.gitgitgadget@gmail.com","threadId":"57128","inReplyTo":"pull.1163.git.git.1640090038.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] tweaks to refs/debug.c","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-22T18:11:51Z","receivedAt":"2021-12-22T18:11:58Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"these are two small commits that helped me while debugging reftable support\nfor git.\n\nHan-Wen Nienhuys (3):\n  refs: pass gitdir to packed_ref_store_create\n  refs: print error message in debug output\n  refs: centralize initialization of the base ref_store.\n\n refs.c                |  6 ++++--\n refs/debug.c          |  6 ++++--\n refs/files-backend.c  | 10 +++-------\n refs/packed-backend.c | 11 +++++------\n refs/packed-backend.h |  2 +-\n refs/refs-internal.h  |  4 ++--\n 6 files changed, 19 insertions(+), 20 deletions(-)\n\n\nbase-commit: 69a9c10c95e28df457e33b3c7400b16caf2e2962\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1163%2Fhanwen%2Fdebug-tweaks-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1163/hanwen/debug-tweaks-v2\nPull-Request: https://github.com/git/git/pull/1163\n\nRange-diff vs v1:\n\n -:  ----------- > 1:  bfebb5f08fe refs: pass gitdir to packed_ref_store_create\n 1:  177d84f8563 = 2:  b189f8661e2 refs: print error message in debug output\n 2:  75e5392032d < -:  ----------- refs: set the repo in debug_ref_store.base\n -:  ----------- > 3:  eea294b688f refs: centralize initialization of the base ref_store.\n\n-- \ngitgitgadget\n"},{"id":"444791","messageId":"bfebb5f08fed835cedfd5373ba98c6e38519f882.1640196714.git.gitgitgadget@gmail.com","threadId":"57128","inReplyTo":"pull.1163.v2.git.git.1640196714.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] refs: pass gitdir to packed_ref_store_create","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-22T18:11:52Z","receivedAt":"2021-12-22T18:12:02Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nThis is consistent with the calling convention for ref backend creation, and\navoids storing \".git/packed-refs\" (the name of a regular file) in a variable called\nref_store::gitdir.\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n refs/files-backend.c  | 5 ++---\n refs/packed-backend.c | 9 +++++----\n refs/packed-backend.h | 2 +-\n 3 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 90b671025a7..f1b66130dfb 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -93,9 +93,8 @@ static struct ref_store *files_ref_store_create(struct repository *repo,\n \n \tget_common_dir_noenv(&sb, gitdir);\n \trefs->gitcommondir = strbuf_detach(&sb, NULL);\n-\tstrbuf_addf(&sb, \"%s/packed-refs\", refs->gitcommondir);\n-\trefs->packed_ref_store = packed_ref_store_create(repo, sb.buf, flags);\n-\tstrbuf_release(&sb);\n+\trefs->packed_ref_store =\n+\t\tpacked_ref_store_create(repo, refs->gitcommondir, flags);\n \n \tchdir_notify_reparent(\"files-backend $GIT_DIR\", &refs->base.gitdir);\n \tchdir_notify_reparent(\"files-backend $GIT_COMMONDIR\",\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 67152c664e2..caa1957252a 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -194,20 +194,21 @@ static int release_snapshot(struct snapshot *snapshot)\n }\n \n struct ref_store *packed_ref_store_create(struct repository *repo,\n-\t\t\t\t\t  const char *path,\n+\t\t\t\t\t  const char *gitdir,\n \t\t\t\t\t  unsigned int store_flags)\n {\n \tstruct packed_ref_store *refs = xcalloc(1, sizeof(*refs));\n \tstruct ref_store *ref_store = (struct ref_store *)refs;\n+\tstruct strbuf sb = STRBUF_INIT;\n \n \tbase_ref_store_init(ref_store, &refs_be_packed);\n \tref_store->repo = repo;\n-\tref_store->gitdir = xstrdup(path);\n+\tref_store->gitdir = xstrdup(gitdir);\n \trefs->store_flags = store_flags;\n+\tstrbuf_addf(&sb, \"%s/packed-refs\", gitdir);\n+\trefs->path = strbuf_detach(&sb, NULL);\n \n-\trefs->path = xstrdup(path);\n \tchdir_notify_reparent(\"packed-refs\", &refs->path);\n-\n \treturn ref_store;\n }\n \ndiff --git a/refs/packed-backend.h b/refs/packed-backend.h\nindex f61a73ec25b..9dd8a344c34 100644\n--- a/refs/packed-backend.h\n+++ b/refs/packed-backend.h\n@@ -14,7 +14,7 @@ struct ref_transaction;\n  */\n \n struct ref_store *packed_ref_store_create(struct repository *repo,\n-\t\t\t\t\t  const char *path,\n+\t\t\t\t\t  const char *gitdir,\n \t\t\t\t\t  unsigned int store_flags);\n \n /*\n-- \ngitgitgadget\n\n"},{"id":"444792","messageId":"b189f8661e22a7e8d3f4a0dfd3ed32ae14b49e0b.1640196714.git.gitgitgadget@gmail.com","threadId":"57128","inReplyTo":"pull.1163.v2.git.git.1640196714.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] refs: print error message in debug output","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-22T18:11:53Z","receivedAt":"2021-12-22T18:12:04Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n refs/debug.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 791423c6a7d..8a6bb157ee6 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -47,7 +47,8 @@ static int debug_transaction_prepare(struct ref_store *refs,\n \ttransaction->ref_store = drefs->refs;\n \tres = drefs->refs->be->transaction_prepare(drefs->refs, transaction,\n \t\t\t\t\t\t   err);\n-\ttrace_printf_key(&trace_refs, \"transaction_prepare: %d\\n\", res);\n+\ttrace_printf_key(&trace_refs, \"transaction_prepare: %d \\\"%s\\\"\\n\", res,\n+\t\t\t err->buf);\n \treturn res;\n }\n \n-- \ngitgitgadget\n\n"},{"id":"444793","messageId":"eea294b688f984f9218db7c6d96a6b7c75d7873a.1640196714.git.gitgitgadget@gmail.com","threadId":"57128","inReplyTo":"pull.1163.v2.git.git.1640196714.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] refs: centralize initialization of the base ref_store.","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-22T18:11:54Z","receivedAt":"2021-12-22T18:12:05Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys <hanwen@google.com>\n\nSigned-off-by: Han-Wen Nienhuys <hanwen@google.com>\n---\n refs.c                | 6 ++++--\n refs/debug.c          | 3 ++-\n refs/files-backend.c  | 5 +----\n refs/packed-backend.c | 6 ++----\n refs/refs-internal.h  | 4 ++--\n 5 files changed, 11 insertions(+), 13 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 4c317955813..f91edb73075 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2007,10 +2007,12 @@ struct ref_store *get_worktree_ref_store(const struct worktree *wt)\n \treturn refs;\n }\n \n-void base_ref_store_init(struct ref_store *refs,\n-\t\t\t const struct ref_storage_be *be)\n+void base_ref_store_init(struct ref_store *refs, struct repository *repo,\n+\t\t\t const char *path, const struct ref_storage_be *be)\n {\n \trefs->be = be;\n+\trefs->repo = repo;\n+\trefs->gitdir = xstrdup(path);\n }\n \n /* backend functions */\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 8a6bb157ee6..2b0771ca53b 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -26,7 +26,8 @@ struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_stor\n \tbe_copy->name = store->be->name;\n \ttrace_printf_key(&trace_refs, \"ref_store for %s\\n\", gitdir);\n \tres->refs = store;\n-\tbase_ref_store_init((struct ref_store *)res, be_copy);\n+\tbase_ref_store_init((struct ref_store *)res, store->repo, gitdir,\n+\t\t\t    be_copy);\n \treturn (struct ref_store *)res;\n }\n \ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex f1b66130dfb..5c6d49a267e 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -86,11 +86,8 @@ static struct ref_store *files_ref_store_create(struct repository *repo,\n \tstruct ref_store *ref_store = (struct ref_store *)refs;\n \tstruct strbuf sb = STRBUF_INIT;\n \n-\tref_store->repo = repo;\n-\tref_store->gitdir = xstrdup(gitdir);\n-\tbase_ref_store_init(ref_store, &refs_be_files);\n+\tbase_ref_store_init(ref_store, repo, gitdir, &refs_be_files);\n \trefs->store_flags = flags;\n-\n \tget_common_dir_noenv(&sb, gitdir);\n \trefs->gitcommondir = strbuf_detach(&sb, NULL);\n \trefs->packed_ref_store =\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex caa1957252a..d91a2018f60 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -201,13 +201,11 @@ struct ref_store *packed_ref_store_create(struct repository *repo,\n \tstruct ref_store *ref_store = (struct ref_store *)refs;\n \tstruct strbuf sb = STRBUF_INIT;\n \n-\tbase_ref_store_init(ref_store, &refs_be_packed);\n-\tref_store->repo = repo;\n-\tref_store->gitdir = xstrdup(gitdir);\n+\tbase_ref_store_init(ref_store, repo, gitdir, &refs_be_packed);\n \trefs->store_flags = store_flags;\n+\n \tstrbuf_addf(&sb, \"%s/packed-refs\", gitdir);\n \trefs->path = strbuf_detach(&sb, NULL);\n-\n \tchdir_notify_reparent(\"packed-refs\", &refs->path);\n \treturn ref_store;\n }\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 46a839539e3..7ff6fba4f0d 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -710,8 +710,8 @@ int parse_loose_ref_contents(const char *buf, struct object_id *oid,\n  * Fill in the generic part of refs and add it to our collection of\n  * reference stores.\n  */\n-void base_ref_store_init(struct ref_store *refs,\n-\t\t\t const struct ref_storage_be *be);\n+void base_ref_store_init(struct ref_store *refs, struct repository *repo,\n+\t\t\t const char *path, const struct ref_storage_be *be);\n \n /*\n  * Support GIT_TRACE_REFS by optionally wrapping the given ref_store instance.\n-- \ngitgitgadget\n"},{"id":"444794","messageId":"CAFQ2z_M7S_gC2AfuPZptYAY3E2XmUMhQONq5iv90OJZHXWrq-Q@mail.gmail.com","threadId":"57128","inReplyTo":"xmqqtuf1kwob.fsf@gitster.g","subject":"Re: [PATCH 2/2] refs: set the repo in debug_ref_store.base","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-12-22T18:13:11Z","receivedAt":"2021-12-22T18:13:25Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Wed, Dec 22, 2021 at 6:58 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Han-Wen Nienhuys via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > From: Han-Wen Nienhuys <hanwen@google.com>\n> >\n> > This is for consistency with the files backend.\n>\n> Hmmmm.  Could you explain what it exactly means?\n>\n> I can see that files_ref_store structure has the .repo member and\n> files_ref_store_create() uses it to remember which repository the\n> ref store is for, but that is an implementation detail that is not\n> exposed outside the files backend, isn't it?\n>\n> To put it differently, what is broken with the current code that\n> leaves the .repo member in refs->base uninitialized?  We are\n> presumably helping the caller that wants to know the repository the\n> ref store belongs to via this pointer with this change---what is\n> that caller?\n\nIt's confusing for the base ref_store to have fields that are\nsometimes set and sometimes not. I sent an alternate take on this as\nv2; hope you like that better.\n\n(sorry, I forgot to update the cover letter.)\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"444829","messageId":"xmqq5yrgi9uw.fsf@gitster.g","threadId":"57128","inReplyTo":"pull.1163.v2.git.git.1640196714.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] tweaks to refs/debug.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-22T21:54:31Z","receivedAt":"2021-12-22T21:54:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Han-Wen Nienhuys via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> these are two small commits that helped me while debugging reftable support\n> for git.\n>\n> Han-Wen Nienhuys (3):\n>   refs: pass gitdir to packed_ref_store_create\n>   refs: print error message in debug output\n>   refs: centralize initialization of the base ref_store.\n\nWelcome to our \"we cannot count to three\" club ;-)\n\nAll three patches make sense to me.  Especially the [3/3] has become\nmuch easier to understand.\n\nThanks, will queue.\n"}]}