From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 09 Feb 2026 16:34:09 GMT
Subject: Re: [PATCH v5 2/4] refs: forward and use the reference storage payload
Message-ID: <aYoMdqDDbt-BArQQ@pks.im>
In-Reply-To: <20260209-kn-alternate-ref-dir-v5-2-740899834ceb@gmail.com>

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

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

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

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

> 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