From: Karthik Nayak Date: Tue, 10 Feb 2026 10:09:16 GMT Subject: Re: [PATCH v5 2/4] refs: forward and use the reference storage payload Message-ID: In-Reply-To: Patrick Steinhardt writes: > On Mon, Feb 09, 2026 at 04:58:19PM +0100, Karthik Nayak wrote: >> An upcoming commit will add support for providing an URI via the >> 'extensions.refStorage' config. The URI will contain the reference >> backend and a corresponding payload. The payload can be then used for >> providing an alternate locations for the reference backend. >> >> To prepare for this, modify the existing backends to accept such an >> argument when initializing via the 'init()' function. Both the files >> and reftable backends will parse the information to be filesystem paths >> to store references. > > Maybe add: "to store references. Given that no callers pass any payload > yet this is essentially a no-op change for now." > Will add. >> diff --git a/refs.c b/refs.c >> index 36f3441632..d9df25d7c0 100644 >> --- a/refs.c >> +++ b/refs.c >> @@ -3425,3 +3426,33 @@ void refs_create_refdir_stubs(struct repository *repo, const char *refdir, >> >> strbuf_release(&path); >> } >> + >> +void refs_compute_filesystem_location(const char *gitdir, const char *payload, >> + bool *is_worktree, struct strbuf *refdir, >> + struct strbuf *ref_common_dir) >> +{ >> + struct strbuf sb = STRBUF_INIT; >> + >> + strbuf_addstr(refdir, gitdir); >> + *is_worktree = get_common_dir_noenv(ref_common_dir, gitdir); >> + >> + if (!payload) >> + return; > > I think you should add a comment here that explains why it's not > necessary to modify the `refdir` in case `*is_worktree`. I'd arguably > even move that code into `if (!payload)`, as we otherwise only set it to > reset it later. So: > > if (!payload) { > /* > * We can use `gitdir` as `refdir` without appending the > * worktree path because... > / > strbuf_addstr(refdir, gitdir); > } > >> + if (!is_absolute_path(payload)) { >> + strbuf_addf(&sb, "%s/%s", ref_common_dir->buf, payload); >> + strbuf_realpath(ref_common_dir, sb.buf, 1); >> + } else { >> + strbuf_realpath(ref_common_dir, payload, 1); >> + } >> + >> + strbuf_reset(refdir); > > And then you can drop this call to `strbuf_reset()`. > Fair enough, will do this. >> diff --git a/refs/files-backend.c b/refs/files-backend.c >> index 240d3c3b26..b192ce606d 100644 >> --- a/refs/files-backend.c >> +++ b/refs/files-backend.c >> @@ -106,19 +106,24 @@ static void clear_loose_ref_cache(struct files_ref_store *refs) >> * set of caches. >> */ >> static struct ref_store *files_ref_store_init(struct repository *repo, >> + const char *payload, >> const char *gitdir, >> unsigned int flags) >> { >> struct files_ref_store *refs = xcalloc(1, sizeof(*refs)); >> struct ref_store *ref_store = (struct ref_store *)refs; >> - struct strbuf sb = STRBUF_INIT; >> + struct strbuf ref_common_dir = STRBUF_INIT; >> + struct strbuf refdir = STRBUF_INIT; >> + bool is_worktree; >> + >> + refs_compute_filesystem_location(gitdir, payload, &is_worktree, &refdir, >> + &ref_common_dir); >> >> - base_ref_store_init(ref_store, repo, gitdir, &refs_be_files); >> + base_ref_store_init(ref_store, repo, refdir.buf, &refs_be_files); >> refs->store_flags = flags; >> - get_common_dir_noenv(&sb, gitdir); >> - refs->gitcommondir = strbuf_detach(&sb, NULL); >> + refs->gitcommondir = strbuf_detach(&ref_common_dir, NULL); >> refs->packed_ref_store = >> - packed_ref_store_init(repo, refs->gitcommondir, flags); >> + packed_ref_store_init(repo, payload, refs->gitcommondir, flags); > > It's a bit weird that we end up passing the payload even though we > unconditionally ignore it in `packed_ref_store_init()`. I'd argue that > we should either pass a `NULL` pointer as payload, or let the packed > backend call `refs_compute_filesystem_location()` itsefl. > I'm considering passing in a NULL and relying on the 'gitdir'. Mostly because the packed-refs backend always comes linked to the files-backend and as such should simply rely on the 'gitdir' provided by it. Let me know if you think we should go the other way. >> diff --git a/refs/packed-backend.c b/refs/packed-backend.c >> index 4ea0c12299..028fbc0585 100644 >> --- a/refs/packed-backend.c >> +++ b/refs/packed-backend.c >> @@ -212,6 +212,7 @@ static size_t snapshot_hexsz(const struct snapshot *snapshot) >> } >> >> struct ref_store *packed_ref_store_init(struct repository *repo, >> + const char *payload UNUSED, >> const char *gitdir, >> unsigned int store_flags) >> { > > And here we should probably explain why we don't have to respect the > payload. > Yeah, will add in a comment. >> diff --git a/refs/refs-internal.h b/refs/refs-internal.h >> index c7d2a6e50b..bd09b1280c 100644 >> --- a/refs/refs-internal.h >> +++ b/refs/refs-internal.h >> @@ -666,4 +667,18 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs >> unsigned int initial_transaction, >> struct strbuf *err); >> >> +/* >> + * Given a gitdir and the reference storage payload provided, retrieve the >> + * 'refdir' and 'ref_common_dir'. The former is where references should be >> + * stored for the current worktree, the latter is the common reference >> + * directory if working with a linked worktree. If working with the main >> + * worktree, both values will be the same. >> + * >> + * This is used by backends such as {files, reftable} which store references in >> + * dedicated filesystem paths. >> + */ > > I guess we can say "This is used by backends that store store files in > the repository directly." > > Patrick Makes sense. Thanks.