diff mbox series

[1/2] Revert "check_repository_format_gently(): refuse extensions for old repositories"

Message ID 20200716062429.GB3242764@google.com (mailing list archive)
State New, archived
Headers show
Series extensions.* fixes for 2.28 (Re: [PATCH] setup: warn about un-enabled extensions) | expand

Commit Message

Jonathan Nieder July 16, 2020, 6:24 a.m. UTC
This reverts commit 14c7fa269e42df4133edd9ae7763b678ed6594cd.

The core.repositoryFormatVersion field was introduced in ab9cb76f661
(Repository format version check., 2005-11-25), providing a welcome
bit of forward compatibility, thanks to some welcome analysis by
Martin Atukunda.  The semantics are simple: a repository with
core.repositoryFormatVersion set to 0 should be comprehensible by all
Git implementations in active use; and Git implementations should
error out early instead of trying to act on Git repositories with
higher core.repositoryFormatVersion values representing new formats
that they do not understand.

A new repository format did not need to be defined until 00a09d57eb8
(introduce "extensions" form of core.repositoryformatversion,
2015-06-23).  This provided a finer-grained extension mechanism for
Git repositories.  In a repository with core.repositoryFormatVersion
set to 1, Git implementations can act on "extensions.*" settings that
modify how a repository is interpreted.  In repository format version
1, unrecognized extensions settings cause Git to error out.

What happens if a user sets an extension setting but forgets to
increase the repository format version to 1?  The extension settings
were still recognized in that case; worse, unrecognized extensions
settings do *not* cause Git to error out.  So combining repository
format version 0 with extensions settings produces in some sense the
worst of both worlds.

To improve that situation, since 14c7fa269e4
(check_repository_format_gently(): refuse extensions for old
repositories, 2020-06-05) Git instead ignores extensions in v0 mode.
This way, v0 repositories get the historical (pre-2015) behavior and
maintain compatibility with Git implementations that do not know about
the v1 format.  Unfortunately, users had been using this sort of
configuration and this behavior change came to many as a surprise:

- users of "git config --worktree" that had followed its advice
  to enable extensions.worktreeConfig (without also increasing the
  repository format version) would find their worktree configuration
  no longer taking effect

- tools such as copybara[*] that had set extensions.partialClone in
  existing repositories (without also increasing the repository format
  version) would find that setting no longer taking effect

The behavior introduced in 14c7fa269e4 might be a good behavior if we
were traveling back in time to 2015, but we're far too late.  For some
reason I thought that it was what had been originally implemented and
that it had regressed.  Apologies for not doing my research when
14c7fa269e4 was under development.

Let's return to the behavior we've had since 2015: always act on
extensions.* settings, regardless of repository format version.  While
we're here, include some tests to describe the effect on the "upgrade
repository version" code path.

[*] https://github.com/google/copybara/commit/ca76c0b1e13c4e36448d12c2aba4a5d9d98fb6e7

Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 setup.c                  | 12 +++---------
 t/t0410-partial-clone.sh | 15 +++++++++++++--
 2 files changed, 16 insertions(+), 11 deletions(-)

Comments

Jeff King July 16, 2020, 10:56 a.m. UTC | #1
On Wed, Jul 15, 2020 at 11:24:29PM -0700, Jonathan Nieder wrote:

> The behavior introduced in 14c7fa269e4 might be a good behavior if we
> were traveling back in time to 2015, but we're far too late.  For some
> reason I thought that it was what had been originally implemented and
> that it had regressed.  Apologies for not doing my research when
> 14c7fa269e4 was under development.

Thanks for a good summary of the situation. I agree that the current
(well, pre-14c7fa269e4) behavior is a bug (mine) from 2015, and we
probably have to accept that state of affairs to some degree in order to
avoid breaking existing cases.

It is unfortunate that this means that somebody with version=0 and
extensions.preciousObjects is in danger of running a pre-2015 version of
Git and having that extension totally ignored, which could be a
data-safety issue. But the farther we get from 2015 the less likely that
is to be a problem (and the more likely somebody is to be depending on
the current behavior of v0+preciousObjects).

> Let's return to the behavior we've had since 2015: always act on
> extensions.* settings, regardless of repository format version.  While
> we're here, include some tests to describe the effect on the "upgrade
> repository version" code path.

So this makes sense to me as a first step.

-Peff
diff mbox series

Patch

diff --git a/setup.c b/setup.c
index dbac2eabe8f..87bf0112cf3 100644
--- a/setup.c
+++ b/setup.c
@@ -507,15 +507,9 @@  static int check_repository_format_gently(const char *gitdir, struct repository_
 		die("%s", err.buf);
 	}
 
-	if (candidate->version >= 1) {
-		repository_format_precious_objects = candidate->precious_objects;
-		set_repository_format_partial_clone(candidate->partial_clone);
-		repository_format_worktree_config = candidate->worktree_config;
-	} else {
-		repository_format_precious_objects = 0;
-		set_repository_format_partial_clone(NULL);
-		repository_format_worktree_config = 0;
-	}
+	repository_format_precious_objects = candidate->precious_objects;
+	set_repository_format_partial_clone(candidate->partial_clone);
+	repository_format_worktree_config = candidate->worktree_config;
 	string_list_clear(&candidate->unknown_extensions, 0);
 
 	if (repository_format_worktree_config) {
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index 463dc3a8be0..51d1eba6050 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -42,14 +42,25 @@  test_expect_success 'convert shallow clone to partial clone' '
 	test_cmp_config -C client 1 core.repositoryformatversion
 '
 
-test_expect_success 'convert shallow clone to partial clone must fail with any extension' '
+test_expect_success 'converting to partial clone fails with noop extension' '
 	rm -fr server client &&
 	test_create_repo server &&
 	test_commit -C server my_commit 1 &&
 	test_commit -C server my_commit2 1 &&
 	git clone --depth=1 "file://$(pwd)/server" client &&
 	test_cmp_config -C client 0 core.repositoryformatversion &&
-	git -C client config extensions.partialclone origin &&
+	git -C client config extensions.noop true &&
+	test_must_fail git -C client fetch --unshallow --filter="blob:none"
+'
+
+test_expect_success 'converting to partial clone fails with unrecognized extension' '
+	rm -fr server client &&
+	test_create_repo server &&
+	test_commit -C server my_commit 1 &&
+	test_commit -C server my_commit2 1 &&
+	git clone --depth=1 "file://$(pwd)/server" client &&
+	test_cmp_config -C client 0 core.repositoryformatversion &&
+	git -C client config extensions.nonsense true &&
 	test_must_fail git -C client fetch --unshallow --filter="blob:none"
 '