Re: [PATCH v4 5/5] Reftable support for git-core
- From
brian m. carlson <sandals@crustytoothpaste.net>
- Date
- Feb 6, 2020, 23:49 UTC
- Message-ID
- <20200206234921.GE6573@camp.crustytoothpaste.net>
- In-Reply-To
- <2786a6bf61d732570335f181540b0d1f7dd47b13.1581029756.git.gitgitgadget@gmail.com>
On 2020-02-06 at 22:55:56, Han-Wen Nienhuys via GitGitGadget wrote:
Show 7 quoted lines
> @@ -403,6 +417,10 @@ int init_db(const char *git_dir, const char *real_git_dir,
> git_config_set("receive.denyNonFastforwards", "true");
> }
>
> + if (flags & INIT_DB_REFTABLE) {
> + git_config_set("extensions.refStorage", "reftable");
> + }This does seem like the best way to do this. Can we get an addition to Documentation/technical/repository-version.txt that documents this extension and the values it takes? I presume "reftable" and "files" are options, but it would be nice it folks didn't have to dig through the code to find that out.
One side note: we typically omit the braces for a single-line if.
Show 26 quoted lines
> +
> if (!(flags & INIT_DB_QUIET)) {
> int len = strlen(git_dir);
>
> @@ -481,15 +499,18 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
> const char *template_dir = NULL;
> unsigned int flags = 0;
> const struct option init_db_options[] = {
> - OPT_STRING(0, "template", &template_dir, N_("template-directory"),
> - N_("directory from which templates will be used")),
> + OPT_STRING(0, "template", &template_dir,
> + N_("template-directory"),
> + N_("directory from which templates will be used")),
> OPT_SET_INT(0, "bare", &is_bare_repository_cfg,
> - N_("create a bare repository"), 1),
> + N_("create a bare repository"), 1),
> { OPTION_CALLBACK, 0, "shared", &init_shared_repository,
> - N_("permissions"),
> - N_("specify that the git repository is to be shared amongst several users"),
> - PARSE_OPT_OPTARG | PARSE_OPT_NONEG, shared_callback, 0},
> + N_("permissions"),
> + N_("specify that the git repository is to be shared amongst several users"),
> + PARSE_OPT_OPTARG | PARSE_OPT_NONEG, shared_callback, 0 },
> OPT_BIT('q', "quiet", &flags, N_("be quiet"), INIT_DB_QUIET),
> + OPT_BIT(0, "reftable", &flags, N_("use reftable"),
> + INIT_DB_REFTABLE),I wonder if this might be better as --refs-backend={files|reftable}. If reftable becomes the default at some point in the future, it would be easier to let people explicitly specify that they want a non-reftable version. If we learn support for a hypothetical reftable v2 in the future, maybe we'd want to let folks specify that instead.
Show 33 quoted lines
> OPT_STRING(0, "separate-git-dir", &real_git_dir, N_("gitdir"),
> N_("separate git dir from working tree")),
> OPT_END()
> diff --git a/cache.h b/cache.h
> index cbfaead23a..929a61e861 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -625,6 +625,7 @@ int path_inside_repo(const char *prefix, const char *path);
>
> #define INIT_DB_QUIET 0x0001
> #define INIT_DB_EXIST_OK 0x0002
> +#define INIT_DB_REFTABLE 0x0004
>
> int init_db(const char *git_dir, const char *real_git_dir,
> const char *template_dir, unsigned int flags);
> @@ -1041,6 +1042,7 @@ struct repository_format {
> int is_bare;
> int hash_algo;
> char *work_tree;
> + char *ref_storage;
> struct string_list unknown_extensions;
> };
>
> diff --git a/refs.c b/refs.c
> index 1ab0bb54d3..e4e5af8ea0 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -20,7 +20,7 @@
> /*
> * List of all available backends
> */
> -static struct ref_storage_be *refs_backends = &refs_be_files;
> +static struct ref_storage_be *refs_backends = &refs_be_reftable;I'm not sure why we're making this change. It doesn't look like the default is changing.
Show 14 quoted lines
> @@ -1913,7 +1916,7 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
> goto done;
>
> /* assume that add_submodule_odb() has been called */
> - refs = ref_store_init(submodule_sb.buf,
> + refs = ref_store_init(submodule_sb.buf, "files", /* XXX */
> REF_STORE_READ | REF_STORE_ODB);
> register_ref_store_map(&submodule_ref_stores, "submodule",
> refs, submodule);
> @@ -1927,6 +1930,7 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
>
> struct ref_store *get_worktree_ref_store(const struct worktree *wt)
> {
> + const char *format = "files"; /* XXX */If the question is whether we still want to default to "files" as the default, then yes, I think we do. Suddenly changing things would be a problem if for some reason someone needed to downgrade Git versions.
Since we have two instances of "files" above, it might be better to create a #define like DEFAULT_REF_BACKEND or some such.
-- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204