Re: [PATCH v2 2/2] refs: add GIT_REF_URI to specify reference backend and directory

[Junio C Hamano <gitster@pobox.com>]

Karthik Nayak <karthik.188@gmail.com> writes:

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

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

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

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