Re: [PATCH v2 2/2] refs: add GIT_REF_URI to specify reference backend and directory
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 26, 2025, 16:17 UTC
- Message-ID
- <xmqq7bvcpy35.fsf@gitster.g>
- In-Reply-To
- <20251126-kn-alternate-ref-dir-v2-2-8b9f6f18f635@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
Show 7 quoted lines
> +`GIT_REF_URI`:: > + Specify which reference backend to be used along with its URI. Reference > + backends like the files, reftable backend use the $GIT_DIR as their URI. > ++ > +Expects the format `<ref_backend>://<URI-for-resource>`, where the > +_<ref_backend>_ specifies the reference backend and the _<URI-for-resource>_ > +specifies the URI used by the backend.
It is more like "<directory>" that specifies the local directory the backend is told to use to store its data. It feels way too broad for what the initial implementation achieves and what the design can potentially include, to say "URI-for-resource", I would think.
Show 33 quoted lines
> diff --git a/environment.h b/environment.h
> index 51898c99cd..9bc380bba4 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -42,6 +42,7 @@
> #define GIT_OPTIONAL_LOCKS_ENVIRONMENT "GIT_OPTIONAL_LOCKS"
> #define GIT_TEXT_DOMAIN_DIR_ENVIRONMENT "GIT_TEXTDOMAINDIR"
> #define GIT_ATTR_SOURCE_ENVIRONMENT "GIT_ATTR_SOURCE"
> +#define GIT_REF_URI_ENVIRONMENT "GIT_REF_URI"
>
> /*
> * Environment variable used to propagate the --no-advice global option to the
> diff --git a/refs.c b/refs.c
> index 23f46867f2..a7af228799 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2186,15 +2186,73 @@ static struct ref_store *get_ref_store_for_dir(struct repository *r,
> return maybe_debug_wrap_ref_store(dir, ref_store);
> }
>
> +static struct ref_store *get_ref_store_from_uri(struct repository *repo,
> + const char *uri)
> +{
> + struct string_list ref_backend_info = STRING_LIST_INIT_DUP;
> + enum ref_storage_format format;
> + struct ref_store *store = NULL;
> + char *format_string;
> + char *dir;
> +
> + if (!uri || !uri[0]) {
> + error("reference backend uri is empty");
> + goto cleanup;
> + }Equating !uri and !uri[0] and giving the same message would not help diagnosing an error, and not _("localizing") the message is of dubious value (after all, the message is not being given to somebody coming over the network, but meant to be given to the local user, right?).
If we remove the !uri[0] from the check, shouldn't the later check catch it as "invalid format" anyway, and print '%s' it to show that what was given was empty clearly enough?
Show 11 quoted lines
> + if (string_list_split(&ref_backend_info, uri, ":", 2) != 2) {
> + error("invalid reference backend uri format '%s'", uri);
> + goto cleanup;
> + }
> +
> + format_string = ref_backend_info.items[0].string;
> + if (!starts_with(ref_backend_info.items[1].string, "//")) {
> + error("invalid reference backend uri format '%s'", uri);
> + goto cleanup;
> + }
> + dir = ref_backend_info.items[1].string + 2;Two questions. (1) do we still want the double-slash after the colon? (2) if so, would it make it simpler to string-list-split using "://" as the separator?
> + format_string = ref_backend_info.items[0].string; > + dir = ref_backend_info.items[1].string + 2;
These two lines are fishy. Perhaps leftover from an earlier draft that did not have an error checking before the previous 5 lines were added?
> + if (!dir || !dir[0]) {
> + error("invalid path in uri '%s'", uri);
> + goto cleanup;
> + }At this point it is very unlikely for "dir" to be NULL, no? Even if the .string member after splitting were NULL, adding 2 to it would not leave it NULL.
Being defensive and checking for NULL is good, but then exactly the same question on "NULL vs an empty string" applies here.
Show 23 quoted lines
> struct ref_store *get_main_ref_store(struct repository *r)
> {
> + char *ref_uri;
> +
> if (r->refs_private)
> return r->refs_private;
>
> if (!r->gitdir)
> BUG("attempting to get main_ref_store outside of repository");
>
> - r->refs_private = get_ref_store_for_dir(r, r->gitdir, r->ref_storage_format);
> + ref_uri = getenv(GIT_REF_URI_ENVIRONMENT);
> + if (ref_uri) {
> + r->refs_private = get_ref_store_from_uri(r, ref_uri);
> + if (!r->refs_private)
> + die("failed to initialize ref store from URI: %s", ref_uri);
> +
> + } else {
> + r->refs_private = get_ref_store_for_dir(r, r->gitdir,
> + r->ref_storage_format);
> + }
> return r->refs_private;
> }If this mechanism is for consumption by "git refs migrate", is it possible to reduce the blast radius by giving the command a command line option to do an equivalent of this? I really am not happy with this environment variable that can change the behaviour of such a low level layer from unsuspecting programs that are not ready.
Instead of tweaking the behaviour of this function via environment that can affect any programs, can't we give these callers like "git refs migrate" with specific needs set_main_ref_store() function that takes a ref_store and a repository. Then they can use to call into get_ref_store_for_dir() to obtain a ref they need. "git refs migrate" already takes "--ref-format" variable, so all it needs is another "--ref-directory" command line option, right?
If the ability to set the ref backend location for arbitrary program proves to be useful, we _could_ give the same --ref-format and --ref-direcctory command line options to "git" itself (like "git -C there" runs any subcommand in the named directory), which does the the get_ref_store_for_dir() plus set_main_ref_store() dance, modelled after how "git refs migrate" does them.
Hmm?