Re: [PATCH 2/2] repository: allow repository format upgrade with extensions
On Thu, Jul 16, 2020 at 07:00:08AM -0400, Jeff King wrote:
Show 27 quoted lines
> > > To avoid mistakes, continue to forbid repository format upgrades in v0
> > > repositories with an unrecognized extension. This way, a v0 user
> > > using a misspelled extension field gets a chance to correct the
> > > mistake before updating to the less forgiving v1 format.
> >
> > This needs to be managed carefully. When the next extension is
> > added to the codebase, that extension may be "known" to Git, but I
> > do not think it is a good idea to honor it in v0 repository, or
> > allow upgrading v0 repository to v1 with such an extension that
> > weren't "known" to Git. For example, a topic in flight adds
> > objectformat extension and I do not think it should be honored in v0
> > repository.
> >
> > Having said that, the approach is OK for now at the tip of tonight's
> > master, but the point is "known" vs "unknown" must be fixed right
> > with some means. E.g. tell people to throw the "new" extensions to
> > the list of "unknown extensions" in check_repo_format() when they
> > add new ones, or something.
>
> Yeah, I agree with this line of reasoning. I'd prefer to see it
> addressed now, so that we don't have to remember to do anything later.
> I.e., for this patch to put the existing known extensions into the
> "good" list for v0, locking it into place forever, and leaving the
> objectformat topic with nothing particular it needs to do.
>
> But in the name of -rc1 expediency, I'm also OK moving forward with this
> for now.
Hmm, this is actually a bit trickier than I expected because of the way the code is written. It's much easier to complain about extensions in a v0 repository than it is to ignore them. But I'm not sure if that isn't the right way to go anyway.
The patch I came up with is below (and goes on top of Jonathan's). Even if we decide this is the right direction, it can definitely happen post-v2.28.
-- >8 --
Subject: verify_repository_format(): complain about new extensions in v0 repo
We made the mistake in the past of respecting extensions.* even when the repository format version was set to 0. This is bad because forgetting to bump the repository version means that older versions of Git (which do not know about our extensions) won't complain. I.e., it's not a problem in itself, but it means your repository is in a state which does not give you the protection you think you're getting from older versions.
For compatibility reasons, we are stuck with that decision for existing extensions. However, we'd prefer not to extend the damage further. We can do that by catching any newly-added extensions and complaining about the repository format.
Note that this is a pretty heavy hammer: we'll refuse to work with the repository at all. A lesser option would be to ignore (possibly with a warning) any new extensions. But because of the way the extensions are handled, that puts the burden on each new extension that is added to remember to "undo" itself (because they are handled before we know for sure whether we are in a v1 repo or not, since we don't insist on a particular ordering of config entries).
So one option would be to rewrite that handling to record any new extensions (and their values) during the config parse, and then only after proceed to handle new ones only if we're in a v1 repository. But I'm not sure if it's worth the trouble:
- ignoring extensions is likely to end up with broken results anyway
(e.g., ignoring a proposed objectformat extension means parsing any
object data is likely to encounter errors) - this is a sign that whatever tool wrote the extension field is
broken. We may be better off notifying immediately and forcefully so
that such tools don't even appear to work accidentally.The only downside is that fixing the istuation is a little tricky, because programs like "git config" won't want to work with the repository. But:
git config --file=.git/config core.repositoryformatversion 1
should still suffice.
Signed-off-by: Jeff King <peff@peff.net>
---
cache.h | 2 +
setup.c | 96 ++++++++++++++++++++++++++++++++++-------
t/t1302-repo-version.sh | 3 ++
3 files changed, 85 insertions(+), 16 deletions(-)
Show changes to 3 files +85 −16
cache.h, setup.c, t/t1302-repo-version.sh
diff --git a/cache.h b/cache.h
index 654426460c..0290849c19 100644
--- a/cache.h
+++ b/cache.h
@@ -1044,6 +1044,7 @@ struct repository_format {
int hash_algo;
char *work_tree;
struct string_list unknown_extensions;
+ struct string_list v1_only_extensions;
};
/*
@@ -1057,6 +1058,7 @@ struct repository_format {
.is_bare = -1, \
.hash_algo = GIT_HASH_SHA1, \
.unknown_extensions = STRING_LIST_INIT_DUP, \
+ .v1_only_extensions = STRING_LIST_INIT_DUP, \
}
/*
diff --git a/setup.c b/setup.c
index 3a81307602..c1480b2b60 100644
--- a/setup.c
+++ b/setup.c
@@ -447,6 +447,54 @@ static int read_worktree_config(const char *var, const char *value, void *vdata)
return 0;
}
+enum extension_result {
+ EXTENSION_ERROR = -1, /* compatible with error(), etc */
+ EXTENSION_UNKNOWN = 0,
+ EXTENSION_OK = 1
+};
+
+/*
+ * Do not add new extensions to this function. It handles extensions which are
+ * respected even in v0-format repositories for historical compatibility.
+ */
+enum extension_result handle_extension_v0(const char *var,
+ const char *value,
+ const char *ext,
+ struct repository_format *data)
+{
+ if (!strcmp(ext, "noop")) {
+ return EXTENSION_OK;
+ } else if (!strcmp(ext, "preciousobjects")) {
+ data->precious_objects = git_config_bool(var, value);
+ return EXTENSION_OK;
+ } else if (!strcmp(ext, "partialclone")) {
+ if (!value)
+ return config_error_nonbool(var);
+ data->partial_clone = xstrdup(value);
+ return EXTENSION_OK;
+ } else if (!strcmp(ext, "worktreeconfig")) {
+ data->worktree_config = git_config_bool(var, value);
+ return EXTENSION_OK;
+ }
+
+ return EXTENSION_UNKNOWN;
+}
+
+/*
+ * Record any new extensions in this function.
+ */
+enum extension_result handle_extension(const char *var,
+ const char *value,
+ const char *ext,
+ struct repository_format *data)
+{
+ if (!strcmp(ext, "noop-v1")) {
+ return EXTENSION_OK;
+ }
+
+ return EXTENSION_UNKNOWN;
+}
+
static int check_repo_format(const char *var, const char *value, void *vdata)
{
struct repository_format *data = vdata;
@@ -455,23 +503,25 @@ static int check_repo_format(const char *var, const char *value, void *vdata)
if (strcmp(var, "core.repositoryformatversion") == 0)
data->version = git_config_int(var, value);
else if (skip_prefix(var, "extensions.", &ext)) {
- /*
- * record any known extensions here; otherwise,
- * we fall through to recording it as unknown, and
- * check_repository_format will complain
- */
- if (!strcmp(ext, "noop"))
- ;
- else if (!strcmp(ext, "preciousobjects"))
- data->precious_objects = git_config_bool(var, value);
- else if (!strcmp(ext, "partialclone")) {
- if (!value)
- return config_error_nonbool(var);
- data->partial_clone = xstrdup(value);
- } else if (!strcmp(ext, "worktreeconfig"))
- data->worktree_config = git_config_bool(var, value);
- else
+ switch (handle_extension_v0(var, value, ext, data)) {
+ case EXTENSION_ERROR:
+ return -1;
+ case EXTENSION_OK:
+ return 0;
+ case EXTENSION_UNKNOWN:
+ break;
+ }
+
+ switch (handle_extension(var, value, ext, data)) {
+ case EXTENSION_ERROR:
+ return -1;
+ case EXTENSION_OK:
+ string_list_append(&data->v1_only_extensions, ext);
+ return 0;
+ case EXTENSION_UNKNOWN:
string_list_append(&data->unknown_extensions, ext);
+ return 0;
+ }
}
return read_worktree_config(var, value, vdata);
@@ -510,6 +560,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
set_repository_format_partial_clone(candidate->partial_clone);
repository_format_worktree_config = candidate->worktree_config;
string_list_clear(&candidate->unknown_extensions, 0);
+ string_list_clear(&candidate->v1_only_extensions, 0);
if (repository_format_worktree_config) {
/*
@@ -588,6 +639,7 @@ int read_repository_format(struct repository_format *format, const char *path)
void clear_repository_format(struct repository_format *format)
{
string_list_clear(&format->unknown_extensions, 0);
+ string_list_clear(&format->v1_only_extensions, 0);
free(format->work_tree);
free(format->partial_clone);
init_repository_format(format);
@@ -613,6 +665,18 @@ int verify_repository_format(const struct repository_format *format,
return -1;
}
+ if (format->version == 0 && format->v1_only_extensions.nr) {
+ int i;
+
+ strbuf_addstr(err,
+ _("repo version is 0, but v1-only extensions found:"));
+
+ for (i = 0; i < format->v1_only_extensions.nr; i++)
+ strbuf_addf(err, "\n\t%s",
+ format->v1_only_extensions.items[i].string);
+ return -1;
+ }
+
return 0;
}
diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
index d60c042ce8..0acabb6d11 100755
--- a/t/t1302-repo-version.sh
+++ b/t/t1302-repo-version.sh
@@ -87,6 +87,9 @@ allow 1
allow 1 noop
abort 1 no-such-extension
allow 0 no-such-extension
+allow 0 noop
+abort 0 noop-v1
+allow 1 noop-v1
EOF
test_expect_success 'precious-objects allowed' '
--
2.28.0.rc0.424.g7d08728e23
Johannes Schindelin via GitGitGadget
