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

[Toon Claes <toon@iotcl.com>]

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

> Git allows setting a different object directory via
> 'GIT_OBJECT_DIRECTORY', but provides no equivalent for references.
> This asymmetry makes it difficult to test different reference backends
> or use alternative reference storage locations without modifying the
> repository structure.
>
> Add a new environment variable 'GIT_REF_URI' that specifies both the
> reference backend and directory path using a URI format:
>
>     <ref_backend>://<path>

I like this idea. This would allow us in the future to also do something
like:

    reftable+nfs://10.11.12.13/ref-dir

> When set, this variable is used to obtain the main reference store for
> all Git commands. The variable is checked in `get_main_ref_store()`
> when lazily assigning `repo->refs_private`. We cannot initialize this
> earlier in `repo_set_gitdir()` because the repository's hash algorithm
> isn't known at that point, and the reftable backend requires this
> information during initialization.
>
> When used with worktrees, the specified directory is treated as the
> reference directory for all worktree operations.
>
> Add a new test file 't1423-ref-backend.sh' to test this environment
> variable.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  Documentation/git.adoc |   8 ++++
>  environment.h          |   1 +
>  refs.c                 |  53 +++++++++++++++++++++++-
>  t/meson.build          |   1 +
>  t/t1423-ref-backend.sh | 109 +++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 171 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git.adoc b/Documentation/git.adoc
> index ce099e78b8..a1d1078f42 100644
> --- a/Documentation/git.adoc
> +++ b/Documentation/git.adoc
> @@ -584,6 +584,14 @@ double-quotes and respecting backslash escapes. E.g., the value
>  	repositories will be set to this value. The default is "files".
>  	See `--ref-format` in linkgit:git-init[1].
>  
> +`GIT_REF_URI`::
> +    Specify which reference backend and path to be used, if not specified the
> +    backend is inferred from the configuration and $GIT_DIR is used as the
> +    path.
> ++
> +Expects the format '<ref_backend>://<path>', where the 'backend' specifies the
> +reference backend and the 'path' specifies the directory used by the backend.
> +
>  Git Commits
>  ~~~~~~~~~~~
>  `GIT_AUTHOR_NAME`::
> 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..0922f08c9f 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2186,15 +2186,66 @@ 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");

I see no localization on any of the error() or die() messages. I think
it's worth to make them translatable.

> +		goto cleanup;
> +	}
> +
> +	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;
> +	dir = ref_backend_info.items[1].string + 2;

Length check before jumping to the third char would be adviced. Also I
think it's worth to check if the first two chars are "//".

-- 
Cheers,
Toon