[PATCH 0/6] t: mark "files"-backend specific tests

[PATCH 0/6] t: mark "files"-backend specific tests

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]

Hi,

this patch series amends some of our tests that exercise "files"-backend
specific logic to have the respective prereqs. The patch series is quite
unexciting and part of our final preparations to get the new "reftable"
backend upstream.

The series depends on ps/refstorage-extension as it makes use of the new
DEFAULT_REPO_FORMAT prereq introduced by that branch.

Patrick

Patrick Steinhardt (6):
  t1300: mark tests to require default repo format
  t1301: mark test for `core.sharedRepository` as reffiles specific
  t1302: make tests more robust with new extensions
  t1419: mark test suite as files-backend specific
  t5526: break test submodule differently
  t: mark tests regarding git-pack-refs(1) to be backend specific

 t/t1300-config.sh             |  8 ++++----
 t/t1301-shared-repo.sh        |  2 +-
 t/t1302-repo-version.sh       | 19 +++++++++++++++----
 t/t1409-avoid-packing-refs.sh |  6 ++++++
 t/t1419-exclude-refs.sh       |  6 ++++++
 t/t3210-pack-refs.sh          |  6 ++++++
 t/t5526-fetch-submodules.sh   |  2 +-
 7 files changed, 39 insertions(+), 10 deletions(-)

-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 1/6] t1300: mark tests to require default repo format

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]

The t1300 test suite exercises the git-config(1) tool. To do so we
overwrite ".git/config" to contain custom contents. While this is easy
enough to do, it may create problems when using a non-default repository
format because we also overwrite the repository format version as well
as any potential extensions.

Mark these tests with the DEFAULT_REPO_FORMAT prerequisite to avoid the
problem. An alternative would be to carry over mandatory config keys
into the rewritten config file. But the effort does not seem worth it
given that the system under test is git-config(1), which is at a lower
level than the repository format.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1300-config.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index f4e2752134..1e953a0fc2 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1098,7 +1098,7 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
 	test_must_fail git config --file=linktolinktonada --list
 '
 
-test_expect_success 'check split_cmdline return' "
+test_expect_success DEFAULT_REPO_FORMAT 'check split_cmdline return' "
 	git config alias.split-cmdline-fix 'echo \"' &&
 	test_must_fail git split-cmdline-fix &&
 	echo foo > foo &&
@@ -1156,7 +1156,7 @@ test_expect_success 'git -c works with aliases of builtins' '
 	test_cmp expect actual
 '
 
-test_expect_success 'aliases can be CamelCased' '
+test_expect_success DEFAULT_REPO_FORMAT 'aliases can be CamelCased' '
 	test_config alias.CamelCased "rev-parse HEAD" &&
 	git CamelCased >out &&
 	git rev-parse HEAD >expect &&
@@ -2051,7 +2051,7 @@ test_expect_success '--show-origin stdin with file include' '
 	test_cmp expect output
 '
 
-test_expect_success '--show-origin blob' '
+test_expect_success DEFAULT_REPO_FORMAT '--show-origin blob' '
 	blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") &&
 	cat >expect <<-EOF &&
 	blob:$blob	user.custom=true
@@ -2060,7 +2060,7 @@ test_expect_success '--show-origin blob' '
 	test_cmp expect output
 '
 
-test_expect_success '--show-origin blob ref' '
+test_expect_success DEFAULT_REPO_FORMAT '--show-origin blob ref' '
 	cat >expect <<-\EOF &&
 	blob:main:custom.conf	user.custom=true
 	EOF
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/6] t1301: mark test for `core.sharedRepository` as reffiles specific

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

In t1301 we verify whether reflog files written by the "files" ref
backend correctly honor permissions when "core.sharedRepository" is set.
The test logic is thus specific to the reffiles backend and will not
work with any other backends.

Mark the test accordingly with the REFFILES prereq.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1301-shared-repo.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index e5a0d65caa..8e2c01e760 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -137,7 +137,7 @@ test_expect_success POSIXPERM 'info/refs respects umask in unshared repo' '
 	test_cmp expect actual
 '
 
-test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
+test_expect_success REFFILES,POSIXPERM 'git reflog expire honors core.sharedRepository' '
 	umask 077 &&
 	git config core.sharedRepository group &&
 	git reflog expire --all &&
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/6] t1302: make tests more robust with new extensions

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2151 bytes --]

In t1302 we exercise logic around "core.repositoryFormatVersion" and
extensions. These tests are not particularly robust against extensions
like the newly introduced "refStorage" extension.

Refactor the tests to be more robust:

  - Check the DEFAULT_REPO_FORMAT prereq to determine the expected
    repository format version. This helps to ensure that we only need to
    update the prereq in a central place when new extensions are added.

  - Use a separate repository to rewrite ".git/config" to test
    combinations of the repository format version and extensions. This
    ensures that we don't break the main test repository.

  - Do not rewrite ".git/config" when exercising the "preciousObjects"
    extension.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1302-repo-version.sh | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
index 179474fa65..fb30c87e1b 100755
--- a/t/t1302-repo-version.sh
+++ b/t/t1302-repo-version.sh
@@ -28,7 +28,12 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'gitdir selection on normal repos' '
-	test_oid version >expect &&
+	if test_have_prereq DEFAULT_REPO_FORMAT
+	then
+		echo 0
+	else
+		echo 1
+	fi >expect &&
 	git config core.repositoryformatversion >actual &&
 	git -C test config core.repositoryformatversion >actual2 &&
 	test_cmp expect actual &&
@@ -79,8 +84,13 @@ mkconfig () {
 
 while read outcome version extensions; do
 	test_expect_success "$outcome version=$version $extensions" "
-		mkconfig $version $extensions >.git/config &&
-		check_${outcome}
+		test_when_finished 'rm -rf extensions' &&
+		git init extensions &&
+		(
+			cd extensions &&
+			mkconfig $version $extensions >.git/config &&
+			check_${outcome}
+		)
 	"
 done <<\EOF
 allow 0
@@ -94,7 +104,8 @@ allow 1 noop-v1
 EOF
 
 test_expect_success 'precious-objects allowed' '
-	mkconfig 1 preciousObjects >.git/config &&
+	git config core.repositoryformatversion 1 &&
+	git config extensions.preciousObjects 1 &&
 	check_allow
 '
 
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 4/6] t1419: mark test suite as files-backend specific

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1617 bytes --]

With 59c35fac54 (refs/packed-backend.c: implement jump lists to avoid
excluded pattern(s), 2023-07-10) we have implemented logic to handle
excluded refs more efficiently in the "packed" ref backend. This logic
allows us to skip emitting refs completely which we know to not be of
any interest to the caller, which can avoid quite some allocaitons and
object lookups.

This was wired up via a new `exclude_patterns` parameter passed to the
backend's ref iterator. The backend only needs to handle them on a best
effort basis though, and in fact we only handle it for the "packed-refs"
file, but not for loose references. Consequentially, all callers must
still filter emitted refs with those exclude patterns.

The result is that handling exclude patterns is completely optional in
the ref backend, and any future backends may or may not implement it.
Let's thus mark the test for t1419 to depend on the REFFILES prereq.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1419-exclude-refs.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/t1419-exclude-refs.sh b/t/t1419-exclude-refs.sh
index 5d8c86b657..1359574419 100755
--- a/t/t1419-exclude-refs.sh
+++ b/t/t1419-exclude-refs.sh
@@ -8,6 +8,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+if test_have_prereq !REFFILES
+then
+	skip_all='skipping `git for-each-ref --exclude` tests; need files backend'
+	test_done
+fi
+
 for_each_ref__exclude () {
 	GIT_TRACE2_PERF=1 test-tool ref-store main \
 		for-each-ref--exclude "$@" >actual.raw
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 5/6] t5526: break test submodule differently

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1873 bytes --]

In 10f5c52656 (submodule: avoid auto-discovery in
prepare_submodule_repo_env(), 2016-09-01) we fixed a bug when doing a
recursive fetch with submodule in the case where the submodule is broken
due to whatever reason. The test to exercise that the fix works breaks
the submodule by deleting its `HEAD` reference, which will cause us to
not detect the directory as a Git repository.

While this is perfectly fine in theory, this way of breaking the repo
becomes problematic with the current efforts to introduce another refdb
backend into Git. The new reftable backend has a stub HEAD file that
always contains "ref: refs/heads/.invalid" so that tools continue to be
able to detect such a repository. But as the reftable backend will never
delete this file even when asked to delete `HEAD` the current way to
delete the `HEAD` reference will stop working.

Adapt the code to instead delete the objects database. Going back with
this new way to cuase breakage confirms that it triggers the infinite
recursion just the same, and there are no equivalent ongoing efforts to
replace the object database with an alternate backend.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5526-fetch-submodules.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 7ab220fa31..5e566205ba 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '
 	git -C dst fetch --recurse-submodules &&
 
 	# Break the receiving submodule
-	test-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&
+	rm -r dst/sub/.git/objects &&
 
 	# NOTE: without the fix the following tests will recurse forever!
 	# They should terminate with an error.
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 6/6] t: mark tests regarding git-pack-refs(1) to be backend specific

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]

Both t1409 and t3210 exercise parts of git-pack-refs(1). Given that we
must check the on-disk files to verify whether the backend has indeed
packed refs as expected those test suites are deeply tied to the actual
backend that is in use.

Mark the test suites to depend on the REFFILES backend.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1409-avoid-packing-refs.sh | 6 ++++++
 t/t3210-pack-refs.sh          | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index f23c0152a8..7748973733 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -5,6 +5,12 @@ test_description='avoid rewriting packed-refs unnecessarily'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+if test_have_prereq !REFFILES
+then
+  skip_all='skipping files-backend specific pack-refs tests'
+  test_done
+fi
+
 # Add an identifying mark to the packed-refs file header line. This
 # shouldn't upset readers, and it should be omitted if the file is
 # ever rewritten.
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 7f4e98db7d..c0f1f9cfb7 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -15,6 +15,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+if test_have_prereq !REFFILES
+then
+  skip_all='skipping files-backend specific pack-refs tests'
+  test_done
+fi
+
 test_expect_success 'enable reflogs' '
 	git config core.logallrefupdates true
 '
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/6] t1300: mark tests to require default repo format

[Taylor Blau]

On Tue, Jan 09, 2024 at 01:17:04PM +0100, Patrick Steinhardt wrote:
> The t1300 test suite exercises the git-config(1) tool. To do so we
> overwrite ".git/config" to contain custom contents. While this is easy
> enough to do, it may create problems when using a non-default repository
> format because we also overwrite the repository format version as well
> as any potential extensions.
>
> Mark these tests with the DEFAULT_REPO_FORMAT prerequisite to avoid the
> problem. An alternative would be to carry over mandatory config keys
> into the rewritten config file. But the effort does not seem worth it
> given that the system under test is git-config(1), which is at a lower
> level than the repository format.

I think I am missing something obvious here ;-).

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t1300-config.sh | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index f4e2752134..1e953a0fc2 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -1098,7 +1098,7 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
>  	test_must_fail git config --file=linktolinktonada --list
>  '
>
> -test_expect_success 'check split_cmdline return' "
> +test_expect_success DEFAULT_REPO_FORMAT 'check split_cmdline return' "
>  	git config alias.split-cmdline-fix 'echo \"' &&
>  	test_must_fail git split-cmdline-fix &&
>  	echo foo > foo &&
> @@ -1156,7 +1156,7 @@ test_expect_success 'git -c works with aliases of builtins' '
>  	test_cmp expect actual
>  '

Looking at this first test, for example, I see two places where we
modify the configuration file:

  - git config alias.split-cmdline-fix 'echo \"'
  - git config branch.main.mergeoptions 'echo \"'

I think I am missing some detail about why we can't do this when we have
extensions enabled?

Thanks,
Taylor

Re: [PATCH 3/6] t1302: make tests more robust with new extensions

[Taylor Blau]

On Tue, Jan 09, 2024 at 01:17:12PM +0100, Patrick Steinhardt wrote:
> In t1302 we exercise logic around "core.repositoryFormatVersion" and
> extensions. These tests are not particularly robust against extensions
> like the newly introduced "refStorage" extension.
>
> Refactor the tests to be more robust:
>
>   - Check the DEFAULT_REPO_FORMAT prereq to determine the expected
>     repository format version. This helps to ensure that we only need to
>     update the prereq in a central place when new extensions are added.
>
>   - Use a separate repository to rewrite ".git/config" to test
>     combinations of the repository format version and extensions. This
>     ensures that we don't break the main test repository.
>
>   - Do not rewrite ".git/config" when exercising the "preciousObjects"
>     extension.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t1302-repo-version.sh | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
> index 179474fa65..fb30c87e1b 100755
> --- a/t/t1302-repo-version.sh
> +++ b/t/t1302-repo-version.sh
> @@ -28,7 +28,12 @@ test_expect_success 'setup' '
>  '
>
>  test_expect_success 'gitdir selection on normal repos' '
> -	test_oid version >expect &&
> +	if test_have_prereq DEFAULT_REPO_FORMAT
> +	then
> +		echo 0
> +	else
> +		echo 1
> +	fi >expect &&
>  	git config core.repositoryformatversion >actual &&
>  	git -C test config core.repositoryformatversion >actual2 &&
>  	test_cmp expect actual &&
> @@ -79,8 +84,13 @@ mkconfig () {
>
>  while read outcome version extensions; do
>  	test_expect_success "$outcome version=$version $extensions" "
> -		mkconfig $version $extensions >.git/config &&
> -		check_${outcome}
> +		test_when_finished 'rm -rf extensions' &&
> +		git init extensions &&
> +		(
> +			cd extensions &&
> +			mkconfig $version $extensions >.git/config &&
> +			check_${outcome}
> +		)
>  	"
>  done <<\EOF
>  allow 0
> @@ -94,7 +104,8 @@ allow 1 noop-v1
>  EOF
>
>  test_expect_success 'precious-objects allowed' '
> -	mkconfig 1 preciousObjects >.git/config &&
> +	git config core.repositoryformatversion 1 &&

I'm nit-picking, but it looks like core.repositoryformatversion is all
lower-case, whereas extensions.preciousObjects is camel-case. I don't
think it's a big deal either way, but I couldn't *not* mention it while
reading ;-)

> +	git config extensions.preciousObjects 1 &&
>  	check_allow
>  '
>
> --
> 2.43.GIT

Thanks,
Taylor

Re: [PATCH 5/6] t5526: break test submodule differently

[Eric Sunshine]

On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> In 10f5c52656 (submodule: avoid auto-discovery in
> prepare_submodule_repo_env(), 2016-09-01) we fixed a bug when doing a
> recursive fetch with submodule in the case where the submodule is broken
> due to whatever reason. The test to exercise that the fix works breaks
> the submodule by deleting its `HEAD` reference, which will cause us to
> not detect the directory as a Git repository.
>
> While this is perfectly fine in theory, this way of breaking the repo
> becomes problematic with the current efforts to introduce another refdb
> backend into Git. The new reftable backend has a stub HEAD file that
> always contains "ref: refs/heads/.invalid" so that tools continue to be
> able to detect such a repository. But as the reftable backend will never
> delete this file even when asked to delete `HEAD` the current way to
> delete the `HEAD` reference will stop working.

This patch is not the appropriate place to bikeshed (but since this is
the first time I've read or paid attention to it), if I saw "ref:
refs/heads/.invalid", the word ".invalid" would make me think
something was broken in my repository. Would it make sense to use some
less alarming word, such as perhaps ".placeholder", ".stand-in",
".synthesized" or even the name of the non-file-based backend in use?

> Adapt the code to instead delete the objects database. Going back with
> this new way to cuase breakage confirms that it triggers the infinite
> recursion just the same, and there are no equivalent ongoing efforts to
> replace the object database with an alternate backend.

s/cuase/cause/

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> @@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '
>         # Break the receiving submodule
> -       test-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&
> +       rm -r dst/sub/.git/objects &&

If there is no guarantee that .git/objects will exist when any
particular backend is in use, would it be more robust to use -f here,
as well?

    rm -rf dst/sub/.git/objects &&

Re: [PATCH 1/6] t1300: mark tests to require default repo format

[Eric Sunshine]

On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> The t1300 test suite exercises the git-config(1) tool. To do so we
> overwrite ".git/config" to contain custom contents. While this is easy
> enough to do, it may create problems when using a non-default repository
> format because we also overwrite the repository format version as well
> as any potential extensions.
>
> Mark these tests with the DEFAULT_REPO_FORMAT prerequisite to avoid the
> problem. An alternative would be to carry over mandatory config keys
> into the rewritten config file. But the effort does not seem worth it
> given that the system under test is git-config(1), which is at a lower
> level than the repository format.

If I'm understanding correctly, with the approach taken by this patch,
won't we undesirably lose some git-config test coverage if the
file-based backend is ever retired, or if tests specific to it are
ever disabled by default? As such, it seems like the alternative "fix"
you mention above would be preferable to ensure that coverage of
git-config doesn't get diluted.

Or am I misunderstanding something?

Re: [PATCH 4/6] t1419: mark test suite as files-backend specific

[Eric Sunshine]

On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> With 59c35fac54 (refs/packed-backend.c: implement jump lists to avoid
> excluded pattern(s), 2023-07-10) we have implemented logic to handle
> excluded refs more efficiently in the "packed" ref backend. This logic
> allows us to skip emitting refs completely which we know to not be of
> any interest to the caller, which can avoid quite some allocaitons and
> object lookups.

s/allocaitons/allocations/

> This was wired up via a new `exclude_patterns` parameter passed to the
> backend's ref iterator. The backend only needs to handle them on a best
> effort basis though, and in fact we only handle it for the "packed-refs"
> file, but not for loose references. Consequentially, all callers must
> still filter emitted refs with those exclude patterns.

s/Consequentially/Consequently/

> The result is that handling exclude patterns is completely optional in
> the ref backend, and any future backends may or may not implement it.
> Let's thus mark the test for t1419 to depend on the REFFILES prereq.

This change seems to be abusing the meaning of the REFFILES
prerequisite. Instead the above description argues for introduction of
a new prerequisite which indicates whether or not the backend honors
the exclude patterns. Or, am I misunderstanding this?

Re: [PATCH 1/6] t1300: mark tests to require default repo format

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3303 bytes --]

On Tue, Jan 09, 2024 at 01:41:57PM -0500, Taylor Blau wrote:
> On Tue, Jan 09, 2024 at 01:17:04PM +0100, Patrick Steinhardt wrote:
> > The t1300 test suite exercises the git-config(1) tool. To do so we
> > overwrite ".git/config" to contain custom contents. While this is easy
> > enough to do, it may create problems when using a non-default repository
> > format because we also overwrite the repository format version as well
> > as any potential extensions.
> >
> > Mark these tests with the DEFAULT_REPO_FORMAT prerequisite to avoid the
> > problem. An alternative would be to carry over mandatory config keys
> > into the rewritten config file. But the effort does not seem worth it
> > given that the system under test is git-config(1), which is at a lower
> > level than the repository format.
> 
> I think I am missing something obvious here ;-).
> 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  t/t1300-config.sh | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> > index f4e2752134..1e953a0fc2 100755
> > --- a/t/t1300-config.sh
> > +++ b/t/t1300-config.sh
> > @@ -1098,7 +1098,7 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
> >  	test_must_fail git config --file=linktolinktonada --list
> >  '
> >
> > -test_expect_success 'check split_cmdline return' "
> > +test_expect_success DEFAULT_REPO_FORMAT 'check split_cmdline return' "
> >  	git config alias.split-cmdline-fix 'echo \"' &&
> >  	test_must_fail git split-cmdline-fix &&
> >  	echo foo > foo &&
> > @@ -1156,7 +1156,7 @@ test_expect_success 'git -c works with aliases of builtins' '
> >  	test_cmp expect actual
> >  '
> 
> Looking at this first test, for example, I see two places where we
> modify the configuration file:
> 
>   - git config alias.split-cmdline-fix 'echo \"'
>   - git config branch.main.mergeoptions 'echo \"'
> 
> I think I am missing some detail about why we can't do this when we have
> extensions enabled?

The issue is not directly visible in the tests I'm amending here, but
happens in the setup code. What we do is to overwrite the repository's
config like this:

```
cat > .git/config << EOF
[beta] ; silly comment # another comment
noIndent= sillyValue ; 'nother silly comment

# empty line
		; comment
		haha   ="beta" # last silly comment
haha = hello
	haha = bello
[nextSection] noNewline = ouch
EOF
```

The problem here is that we drop any extensions that the repository has
been initialized with originally. This seems to work alright in the
context of SHA256 repositories. But with the reftable backend this
pattern will cause test failures because the discarded "refStorage"
extension will make us assume that the repostiory uses the "files"
backend instead of the "reftable" backend. And that starts to go
downhill quite fast when trying to read or write refs.

A "proper" fix for this issue would be to rewrite tests such that we
know to retain those extensions. But I'm just not sure whether that is
really worth it, mostly because the system under test is at a lower
level and thus shouldn't care about repository extensions. After all,
extensions build on top of our config code.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/6] t1300: mark tests to require default repo format

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1494 bytes --]

On Tue, Jan 09, 2024 at 02:35:29PM -0500, Eric Sunshine wrote:
> On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> > The t1300 test suite exercises the git-config(1) tool. To do so we
> > overwrite ".git/config" to contain custom contents. While this is easy
> > enough to do, it may create problems when using a non-default repository
> > format because we also overwrite the repository format version as well
> > as any potential extensions.
> >
> > Mark these tests with the DEFAULT_REPO_FORMAT prerequisite to avoid the
> > problem. An alternative would be to carry over mandatory config keys
> > into the rewritten config file. But the effort does not seem worth it
> > given that the system under test is git-config(1), which is at a lower
> > level than the repository format.
> 
> If I'm understanding correctly, with the approach taken by this patch,
> won't we undesirably lose some git-config test coverage if the
> file-based backend is ever retired, or if tests specific to it are
> ever disabled by default? As such, it seems like the alternative "fix"
> you mention above would be preferable to ensure that coverage of
> git-config doesn't get diluted.
> 
> Or am I misunderstanding something?

A valid remark indeed, even though this is thinking quite far into the
future. I'll investigate how much of a pain it would be to instead "do
the right thing" and retain the repositroy format version as well as
extensions.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/6] t1419: mark test suite as files-backend specific

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2756 bytes --]

On Tue, Jan 09, 2024 at 02:40:50PM -0500, Eric Sunshine wrote:
> On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> > With 59c35fac54 (refs/packed-backend.c: implement jump lists to avoid
> > excluded pattern(s), 2023-07-10) we have implemented logic to handle
> > excluded refs more efficiently in the "packed" ref backend. This logic
> > allows us to skip emitting refs completely which we know to not be of
> > any interest to the caller, which can avoid quite some allocaitons and
> > object lookups.
> 
> s/allocaitons/allocations/
> 
> > This was wired up via a new `exclude_patterns` parameter passed to the
> > backend's ref iterator. The backend only needs to handle them on a best
> > effort basis though, and in fact we only handle it for the "packed-refs"
> > file, but not for loose references. Consequentially, all callers must
> > still filter emitted refs with those exclude patterns.
> 
> s/Consequentially/Consequently/

Hum. I had the last time when you mentioned the in mind while writing
the commit message, but seemingly misremembered the outcome. So I now
looked things up in a dictionary, and both words seem to be used in
equivalent ways. As a non-native speaker, could you maybe elaborate a
bit to help me out? :)

> > The result is that handling exclude patterns is completely optional in
> > the ref backend, and any future backends may or may not implement it.
> > Let's thus mark the test for t1419 to depend on the REFFILES prereq.
> 
> This change seems to be abusing the meaning of the REFFILES
> prerequisite. Instead the above description argues for introduction of
> a new prerequisite which indicates whether or not the backend honors
> the exclude patterns. Or, am I misunderstanding this?

I wouldn't say that this is abuse. We know the logic is only implemented
by certain backends, and for the time being the only backend that does
is the "files" backend. Furthermore, no test outside of t1419 ever cares
for whether the backend knows to handle exclude patterns, so introducing
a separate prereq that simply maps to REFFILES doesn't really feel worth
it. If we ever implement this behaviour in the "reftable" backend, then
we can easily extend the prereq like follows:

```
if ! test_have_prereq REFFILES && ! test_have_prereq REFTABLE
then
       skip_all='skipping `git for-each-ref --exclude` tests; need files backend'
       test_done
fi
```

Now we could of course make the prereq clever and auto-detect whether
the ref backend supports excludes. But this has the downside that it
could lead to silent failures in case the exclude pattern handling ever
breaks because now the prereq would potentially evaluate to "false".

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/6] t5526: break test submodule differently

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3458 bytes --]

On Tue, Jan 09, 2024 at 02:23:55PM -0500, Eric Sunshine wrote:
> On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> > In 10f5c52656 (submodule: avoid auto-discovery in
> > prepare_submodule_repo_env(), 2016-09-01) we fixed a bug when doing a
> > recursive fetch with submodule in the case where the submodule is broken
> > due to whatever reason. The test to exercise that the fix works breaks
> > the submodule by deleting its `HEAD` reference, which will cause us to
> > not detect the directory as a Git repository.
> >
> > While this is perfectly fine in theory, this way of breaking the repo
> > becomes problematic with the current efforts to introduce another refdb
> > backend into Git. The new reftable backend has a stub HEAD file that
> > always contains "ref: refs/heads/.invalid" so that tools continue to be
> > able to detect such a repository. But as the reftable backend will never
> > delete this file even when asked to delete `HEAD` the current way to
> > delete the `HEAD` reference will stop working.
> 
> This patch is not the appropriate place to bikeshed (but since this is
> the first time I've read or paid attention to it), if I saw "ref:
> refs/heads/.invalid", the word ".invalid" would make me think
> something was broken in my repository. Would it make sense to use some
> less alarming word, such as perhaps ".placeholder", ".stand-in",
> ".synthesized" or even the name of the non-file-based backend in use?

Well, something _is_ broken in your repository in case Git ever tries to
read the "HEAD" placeholder in a reftable-enabled repository. But I
guess you rather come from the angle of using `cat HEAD` as a user. I do
agree that using a better hint could help users, but this detail has
already been recorded as such in "Documentation/technical/reftable.txt".

We can of course change this to be "ref: refs/heads/.reftable", but as
you already say this is something that should be discussed in a separate
thread.

> > Adapt the code to instead delete the objects database. Going back with
> > this new way to cuase breakage confirms that it triggers the infinite
> > recursion just the same, and there are no equivalent ongoing efforts to
> > replace the object database with an alternate backend.
> 
> s/cuase/cause/
> 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> > @@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '
> >         # Break the receiving submodule
> > -       test-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&
> > +       rm -r dst/sub/.git/objects &&
> 
> If there is no guarantee that .git/objects will exist when any
> particular backend is in use, would it be more robust to use -f here,
> as well?
> 
>     rm -rf dst/sub/.git/objects &&

`.git/objects` always exists in a healthy repository. If it doesn't,
then `is_git_directory()` would return a false-ish value and we wouldn't
recognize the repository as such. Or are you saying that this could
potentially change in the future if there was ever to be an alternate
ODB format? If so that is a valid remark, but the test would break
regardless of whether we use `-f` or not: if a missing ".git/objects"
directory does not lead to a corrupted repository then the whole premise
of the test is broken.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 0/6] t: mark "files"-backend specific tests

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5069 bytes --]

Hi,

this is the second version of my patch series that addresses tests which
are specific to the "files" backend. Changes compared to v1:

  - I've rewritten the patch addressing t1300 to not mark tests as
    repository format specific anymoreg. Instead, we now create separate
    repos for relevant tests where we are more careful to not discard
    extensions.

  - I've made the casing of config options more consistent.

  - I've extended some commit messages to hopefully explain better why
    I'm doing things the way I do them and fixed some typos.

Thanks for your feedback!

Patrick

Patrick Steinhardt (6):
  t1300: make tests more robust with non-default ref backends
  t1301: mark test for `core.sharedRepository` as reffiles specific
  t1302: make tests more robust with new extensions
  t1419: mark test suite as files-backend specific
  t5526: break test submodule differently
  t: mark tests regarding git-pack-refs(1) to be backend specific

 t/t1300-config.sh             | 74 +++++++++++++++++++++++------------
 t/t1301-shared-repo.sh        |  2 +-
 t/t1302-repo-version.sh       | 19 +++++++--
 t/t1409-avoid-packing-refs.sh |  6 +++
 t/t1419-exclude-refs.sh       |  6 +++
 t/t3210-pack-refs.sh          |  6 +++
 t/t5526-fetch-submodules.sh   |  2 +-
 7 files changed, 83 insertions(+), 32 deletions(-)

Range-diff against v1:
1:  ec1b5bdd17 < -:  ---------- t1300: mark tests to require default repo format
-:  ---------- > 1:  0552123fa3 t1300: make tests more robust with non-default ref backends
2:  68e308c200 = 2:  384250fec2 t1301: mark test for `core.sharedRepository` as reffiles specific
3:  9af1e418d4 ! 3:  1284b70fab t1302: make tests more robust with new extensions
    @@ t/t1302-repo-version.sh: allow 1 noop-v1
      
      test_expect_success 'precious-objects allowed' '
     -	mkconfig 1 preciousObjects >.git/config &&
    -+	git config core.repositoryformatversion 1 &&
    ++	git config core.repositoryFormatVersion 1 &&
     +	git config extensions.preciousObjects 1 &&
      	check_allow
      '
4:  d7c6b8b2a7 ! 4:  c6062b612c t1419: mark test suite as files-backend specific
    @@ Commit message
         excluded pattern(s), 2023-07-10) we have implemented logic to handle
         excluded refs more efficiently in the "packed" ref backend. This logic
         allows us to skip emitting refs completely which we know to not be of
    -    any interest to the caller, which can avoid quite some allocaitons and
    +    any interest to the caller, which can avoid quite some allocations and
         object lookups.
     
         This was wired up via a new `exclude_patterns` parameter passed to the
         backend's ref iterator. The backend only needs to handle them on a best
         effort basis though, and in fact we only handle it for the "packed-refs"
    -    file, but not for loose references. Consequentially, all callers must
    -    still filter emitted refs with those exclude patterns.
    +    file, but not for loose references. Consequently, all callers must still
    +    filter emitted refs with those exclude patterns.
     
         The result is that handling exclude patterns is completely optional in
         the ref backend, and any future backends may or may not implement it.
         Let's thus mark the test for t1419 to depend on the REFFILES prereq.
     
    +    An alternative would be to introduce a new prereq that tells us whether
    +    the backend under test supports exclude patterns or not. But this does
    +    feel a bit overblown:
    +
    +      - It would either map to the REFFILES prereq, in which case it feels
    +        overengineered because the prereq is only ever relevant to t1419.
    +
    +      - Otherwise, it could auto-detect whether the backend supports exclude
    +        patterns. But this could lead to silent failures in case the support
    +        for this feature breaks at any point in time.
    +
    +    It should thus be good enough to just use the REFFILES prereq for now.
    +    If future backends ever grow support for exclude patterns we can easily
    +    add their respective prereq as another condition for this test suite to
    +    execute.
    +
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## t/t1419-exclude-refs.sh ##
5:  51e494a50e ! 5:  ae08afc459 t5526: break test submodule differently
    @@ Commit message
         delete the `HEAD` reference will stop working.
     
         Adapt the code to instead delete the objects database. Going back with
    -    this new way to cuase breakage confirms that it triggers the infinite
    +    this new way to cause breakage confirms that it triggers the infinite
         recursion just the same, and there are no equivalent ongoing efforts to
         replace the object database with an alternate backend.
     
6:  a9620f329d = 6:  df648be535 t: mark tests regarding git-pack-refs(1) to be backend specific
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 1/6] t1300: make tests more robust with non-default ref backends

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4258 bytes --]

The t1300 test suite exercises the git-config(1) tool. To do so we
overwrite ".git/config" to contain custom contents. While this is easy
enough to do, it may create problems when using a non-default repository
format because we also overwrite the repository format version as well
as any potential extensions. With the upcoming "reftable" ref backend
the result is that we may try to access refs via the "files" backend
even though the repository has been initialized with the "reftable"
backend.

Refactor tests which access the refdb to be more robust by using their
own separate repositories, which allows us to be more careful and not
discard required extensions.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1300-config.sh | 74 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 48 insertions(+), 26 deletions(-)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index f4e2752134..53c3d65823 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1099,13 +1099,18 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
 '
 
 test_expect_success 'check split_cmdline return' "
-	git config alias.split-cmdline-fix 'echo \"' &&
-	test_must_fail git split-cmdline-fix &&
-	echo foo > foo &&
-	git add foo &&
-	git commit -m 'initial commit' &&
-	git config branch.main.mergeoptions 'echo \"' &&
-	test_must_fail git merge main
+	test_when_finished 'rm -rf repo' &&
+	git init repo &&
+	(
+		cd repo &&
+		git config alias.split-cmdline-fix 'echo \"' &&
+		test_must_fail git split-cmdline-fix &&
+		echo foo >foo &&
+		git add foo &&
+		git commit -m 'initial commit' &&
+		git config branch.main.mergeoptions 'echo \"' &&
+		test_must_fail git merge main
+	)
 "
 
 test_expect_success 'git -c "key=value" support' '
@@ -1157,10 +1162,16 @@ test_expect_success 'git -c works with aliases of builtins' '
 '
 
 test_expect_success 'aliases can be CamelCased' '
-	test_config alias.CamelCased "rev-parse HEAD" &&
-	git CamelCased >out &&
-	git rev-parse HEAD >expect &&
-	test_cmp expect out
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		git config alias.CamelCased "rev-parse HEAD" &&
+		git CamelCased >out &&
+		git rev-parse HEAD >expect &&
+		test_cmp expect out
+	)
 '
 
 test_expect_success 'git -c does not split values on equals' '
@@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '
 '
 
 test_expect_success 'set up custom config file' '
-	CUSTOM_CONFIG_FILE="custom.conf" &&
-	cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
+	cat >"custom.conf" <<-\EOF &&
 	[user]
 		custom = true
 	EOF
+	CUSTOM_CONFIG_FILE="$(test-tool path-utils real_path custom.conf)"
 '
 
 test_expect_success !MINGW 'set up custom config file with special name characters' '
@@ -2052,22 +2063,33 @@ test_expect_success '--show-origin stdin with file include' '
 '
 
 test_expect_success '--show-origin blob' '
-	blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") &&
-	cat >expect <<-EOF &&
-	blob:$blob	user.custom=true
-	EOF
-	git config --blob=$blob --show-origin --list >output &&
-	test_cmp expect output
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") &&
+		cat >expect <<-EOF &&
+		blob:$blob	user.custom=true
+		EOF
+		git config --blob=$blob --show-origin --list >output &&
+		test_cmp expect output
+	)
 '
 
 test_expect_success '--show-origin blob ref' '
-	cat >expect <<-\EOF &&
-	blob:main:custom.conf	user.custom=true
-	EOF
-	git add "$CUSTOM_CONFIG_FILE" &&
-	git commit -m "new config file" &&
-	git config --blob=main:"$CUSTOM_CONFIG_FILE" --show-origin --list >output &&
-	test_cmp expect output
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		cat >expect <<-\EOF &&
+		blob:main:custom.conf	user.custom=true
+		EOF
+		cp "$CUSTOM_CONFIG_FILE" custom.conf &&
+		git add custom.conf &&
+		git commit -m "new config file" &&
+		git config --blob=main:custom.conf --show-origin --list >output &&
+		test_cmp expect output
+	)
 '
 
 test_expect_success '--show-origin with --default' '
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 2/6] t1301: mark test for `core.sharedRepository` as reffiles specific

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

In t1301 we verify whether reflog files written by the "files" ref
backend correctly honor permissions when "core.sharedRepository" is set.
The test logic is thus specific to the reffiles backend and will not
work with any other backends.

Mark the test accordingly with the REFFILES prereq.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1301-shared-repo.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index e5a0d65caa..8e2c01e760 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -137,7 +137,7 @@ test_expect_success POSIXPERM 'info/refs respects umask in unshared repo' '
 	test_cmp expect actual
 '
 
-test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
+test_expect_success REFFILES,POSIXPERM 'git reflog expire honors core.sharedRepository' '
 	umask 077 &&
 	git config core.sharedRepository group &&
 	git reflog expire --all &&
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 3/6] t1302: make tests more robust with new extensions

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2151 bytes --]

In t1302 we exercise logic around "core.repositoryFormatVersion" and
extensions. These tests are not particularly robust against extensions
like the newly introduced "refStorage" extension.

Refactor the tests to be more robust:

  - Check the DEFAULT_REPO_FORMAT prereq to determine the expected
    repository format version. This helps to ensure that we only need to
    update the prereq in a central place when new extensions are added.

  - Use a separate repository to rewrite ".git/config" to test
    combinations of the repository format version and extensions. This
    ensures that we don't break the main test repository.

  - Do not rewrite ".git/config" when exercising the "preciousObjects"
    extension.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1302-repo-version.sh | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
index 179474fa65..7c680c91c4 100755
--- a/t/t1302-repo-version.sh
+++ b/t/t1302-repo-version.sh
@@ -28,7 +28,12 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'gitdir selection on normal repos' '
-	test_oid version >expect &&
+	if test_have_prereq DEFAULT_REPO_FORMAT
+	then
+		echo 0
+	else
+		echo 1
+	fi >expect &&
 	git config core.repositoryformatversion >actual &&
 	git -C test config core.repositoryformatversion >actual2 &&
 	test_cmp expect actual &&
@@ -79,8 +84,13 @@ mkconfig () {
 
 while read outcome version extensions; do
 	test_expect_success "$outcome version=$version $extensions" "
-		mkconfig $version $extensions >.git/config &&
-		check_${outcome}
+		test_when_finished 'rm -rf extensions' &&
+		git init extensions &&
+		(
+			cd extensions &&
+			mkconfig $version $extensions >.git/config &&
+			check_${outcome}
+		)
 	"
 done <<\EOF
 allow 0
@@ -94,7 +104,8 @@ allow 1 noop-v1
 EOF
 
 test_expect_success 'precious-objects allowed' '
-	mkconfig 1 preciousObjects >.git/config &&
+	git config core.repositoryFormatVersion 1 &&
+	git config extensions.preciousObjects 1 &&
 	check_allow
 '
 
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 4/6] t1419: mark test suite as files-backend specific

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2360 bytes --]

With 59c35fac54 (refs/packed-backend.c: implement jump lists to avoid
excluded pattern(s), 2023-07-10) we have implemented logic to handle
excluded refs more efficiently in the "packed" ref backend. This logic
allows us to skip emitting refs completely which we know to not be of
any interest to the caller, which can avoid quite some allocations and
object lookups.

This was wired up via a new `exclude_patterns` parameter passed to the
backend's ref iterator. The backend only needs to handle them on a best
effort basis though, and in fact we only handle it for the "packed-refs"
file, but not for loose references. Consequently, all callers must still
filter emitted refs with those exclude patterns.

The result is that handling exclude patterns is completely optional in
the ref backend, and any future backends may or may not implement it.
Let's thus mark the test for t1419 to depend on the REFFILES prereq.

An alternative would be to introduce a new prereq that tells us whether
the backend under test supports exclude patterns or not. But this does
feel a bit overblown:

  - It would either map to the REFFILES prereq, in which case it feels
    overengineered because the prereq is only ever relevant to t1419.

  - Otherwise, it could auto-detect whether the backend supports exclude
    patterns. But this could lead to silent failures in case the support
    for this feature breaks at any point in time.

It should thus be good enough to just use the REFFILES prereq for now.
If future backends ever grow support for exclude patterns we can easily
add their respective prereq as another condition for this test suite to
execute.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1419-exclude-refs.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/t1419-exclude-refs.sh b/t/t1419-exclude-refs.sh
index 5d8c86b657..1359574419 100755
--- a/t/t1419-exclude-refs.sh
+++ b/t/t1419-exclude-refs.sh
@@ -8,6 +8,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+if test_have_prereq !REFFILES
+then
+	skip_all='skipping `git for-each-ref --exclude` tests; need files backend'
+	test_done
+fi
+
 for_each_ref__exclude () {
 	GIT_TRACE2_PERF=1 test-tool ref-store main \
 		for-each-ref--exclude "$@" >actual.raw
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 5/6] t5526: break test submodule differently

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1873 bytes --]

In 10f5c52656 (submodule: avoid auto-discovery in
prepare_submodule_repo_env(), 2016-09-01) we fixed a bug when doing a
recursive fetch with submodule in the case where the submodule is broken
due to whatever reason. The test to exercise that the fix works breaks
the submodule by deleting its `HEAD` reference, which will cause us to
not detect the directory as a Git repository.

While this is perfectly fine in theory, this way of breaking the repo
becomes problematic with the current efforts to introduce another refdb
backend into Git. The new reftable backend has a stub HEAD file that
always contains "ref: refs/heads/.invalid" so that tools continue to be
able to detect such a repository. But as the reftable backend will never
delete this file even when asked to delete `HEAD` the current way to
delete the `HEAD` reference will stop working.

Adapt the code to instead delete the objects database. Going back with
this new way to cause breakage confirms that it triggers the infinite
recursion just the same, and there are no equivalent ongoing efforts to
replace the object database with an alternate backend.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5526-fetch-submodules.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 7ab220fa31..5e566205ba 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '
 	git -C dst fetch --recurse-submodules &&
 
 	# Break the receiving submodule
-	test-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&
+	rm -r dst/sub/.git/objects &&
 
 	# NOTE: without the fix the following tests will recurse forever!
 	# They should terminate with an error.
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 6/6] t: mark tests regarding git-pack-refs(1) to be backend specific

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]

Both t1409 and t3210 exercise parts of git-pack-refs(1). Given that we
must check the on-disk files to verify whether the backend has indeed
packed refs as expected those test suites are deeply tied to the actual
backend that is in use.

Mark the test suites to depend on the REFFILES backend.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1409-avoid-packing-refs.sh | 6 ++++++
 t/t3210-pack-refs.sh          | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index f23c0152a8..7748973733 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -5,6 +5,12 @@ test_description='avoid rewriting packed-refs unnecessarily'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+if test_have_prereq !REFFILES
+then
+  skip_all='skipping files-backend specific pack-refs tests'
+  test_done
+fi
+
 # Add an identifying mark to the packed-refs file header line. This
 # shouldn't upset readers, and it should be omitted if the file is
 # ever rewritten.
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 7f4e98db7d..c0f1f9cfb7 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -15,6 +15,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+if test_have_prereq !REFFILES
+then
+  skip_all='skipping files-backend specific pack-refs tests'
+  test_done
+fi
+
 test_expect_success 'enable reflogs' '
 	git config core.logallrefupdates true
 '
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/6] t1419: mark test suite as files-backend specific

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

On just this (non-technical) part...

>> > file, but not for loose references. Consequentially, all callers must
>> > still filter emitted refs with those exclude patterns.
>> 
>> s/Consequentially/Consequently/
>
> Hum. I had the last time when you mentioned the in mind while writing
> the commit message, but seemingly misremembered the outcome. So I now
> looked things up in a dictionary, and both words seem to be used in
> equivalent ways. As a non-native speaker, could you maybe elaborate a
> bit to help me out? :)

As a non-native, I often find this

  https://trends.google.com/trends/explore?q=consequentially,consequently&hl=en

fairly useful.

Re: [PATCH 4/6] t1419: mark test suite as files-backend specific

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]

On Wed, Jan 10, 2024 at 08:27:29AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> On just this (non-technical) part...
> 
> >> > file, but not for loose references. Consequentially, all callers must
> >> > still filter emitted refs with those exclude patterns.
> >> 
> >> s/Consequentially/Consequently/
> >
> > Hum. I had the last time when you mentioned the in mind while writing
> > the commit message, but seemingly misremembered the outcome. So I now
> > looked things up in a dictionary, and both words seem to be used in
> > equivalent ways. As a non-native speaker, could you maybe elaborate a
> > bit to help me out? :)
> 
> As a non-native, I often find this
> 
>   https://trends.google.com/trends/explore?q=consequentially,consequently&hl=en
> 
> fairly useful.

Good idea, thanks!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends

[Toon Claes]

Patrick Steinhardt <ps@pks.im> writes:

> [[PGP Signed Part:Undecided]]
> [1. text/plain]
> The t1300 test suite exercises the git-config(1) tool. To do so we
> overwrite ".git/config" to contain custom contents. While this is easy
> enough to do, it may create problems when using a non-default repository
> format because we also overwrite the repository format version as well
> as any potential extensions. With the upcoming "reftable" ref backend
> the result is that we may try to access refs via the "files" backend
> even though the repository has been initialized with the "reftable"
> backend.
>
> Refactor tests which access the refdb to be more robust by using their
> own separate repositories, which allows us to be more careful and not
> discard required extensions.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---

> @@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '
>  '
>
>  test_expect_success 'set up custom config file' '
> -	CUSTOM_CONFIG_FILE="custom.conf" &&
> -	cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
> +	cat >"custom.conf" <<-\EOF &&
>  	[user]
>  		custom = true
>  	EOF
> +	CUSTOM_CONFIG_FILE="$(test-tool path-utils real_path
>  custom.conf)"
>  '

From the commit message it was not clear to me this change was needed.
Do you think it's worth it to add something to the commit message
explaining you now need to copy the custom.conf into each seperate
repository?


--
Toon

Re: [PATCH v2 3/6] t1302: make tests more robust with new extensions

[Toon Claes]

Patrick Steinhardt <ps@pks.im> writes:

> [[PGP Signed Part:Undecided]]
> [1. text/plain]
> In t1302 we exercise logic around "core.repositoryFormatVersion" and
> extensions. These tests are not particularly robust against extensions
> like the newly introduced "refStorage" extension.
>
> Refactor the tests to be more robust:
>
>   - Check the DEFAULT_REPO_FORMAT prereq to determine the expected
>     repository format version. This helps to ensure that we only need to
>     update the prereq in a central place when new extensions are added.
>
>   - Use a separate repository to rewrite ".git/config" to test
>     combinations of the repository format version and extensions. This
>     ensures that we don't break the main test repository.
>
>   - Do not rewrite ".git/config" when exercising the "preciousObjects"
>     extension.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t1302-repo-version.sh | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
> index 179474fa65..7c680c91c4 100755
> --- a/t/t1302-repo-version.sh
> +++ b/t/t1302-repo-version.sh
> @@ -79,8 +84,13 @@ mkconfig () {
>
>  while read outcome version extensions; do
>  	test_expect_success "$outcome version=$version $extensions" "
> -		mkconfig $version $extensions >.git/config &&
> -		check_${outcome}
> +		test_when_finished 'rm -rf extensions' &&
> +		git init extensions &&
> +		(
> +			cd extensions &&
> +			mkconfig $version $extensions >.git/config &&

Why did you not remove the use of `mkconfig` here?

--
Toon

Re: [PATCH v2 3/6] t1302: make tests more robust with new extensions

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2308 bytes --]

On Tue, Jan 23, 2024 at 03:08:00PM +0100, Toon Claes wrote:
> 
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > [[PGP Signed Part:Undecided]]
> > [1. text/plain]
> > In t1302 we exercise logic around "core.repositoryFormatVersion" and
> > extensions. These tests are not particularly robust against extensions
> > like the newly introduced "refStorage" extension.
> >
> > Refactor the tests to be more robust:
> >
> >   - Check the DEFAULT_REPO_FORMAT prereq to determine the expected
> >     repository format version. This helps to ensure that we only need to
> >     update the prereq in a central place when new extensions are added.
> >
> >   - Use a separate repository to rewrite ".git/config" to test
> >     combinations of the repository format version and extensions. This
> >     ensures that we don't break the main test repository.
> >
> >   - Do not rewrite ".git/config" when exercising the "preciousObjects"
> >     extension.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  t/t1302-repo-version.sh | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
> > index 179474fa65..7c680c91c4 100755
> > --- a/t/t1302-repo-version.sh
> > +++ b/t/t1302-repo-version.sh
> > @@ -79,8 +84,13 @@ mkconfig () {
> >
> >  while read outcome version extensions; do
> >  	test_expect_success "$outcome version=$version $extensions" "
> > -		mkconfig $version $extensions >.git/config &&
> > -		check_${outcome}
> > +		test_when_finished 'rm -rf extensions' &&
> > +		git init extensions &&
> > +		(
> > +			cd extensions &&
> > +			mkconfig $version $extensions >.git/config &&
> 
> Why did you not remove the use of `mkconfig` here?

I guess you mean stop using `mkconfig` and instead use git-config(1) to
manually write only the repository format version as well as extensions?
The problem here is that the resulting configuration would be dependent
on some environment variables, like `GIT_TEST_DEFAULT_HASH` and the
refdb equivalent `GIT_TEST_DEFAULT_REF_FORMAT` which do end up in the
repo's configuration. So I think it's actually preferable to overwrite
the complete ".git/config" so that is exactly in the expected state.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]

On Tue, Jan 23, 2024 at 02:41:17PM +0100, Toon Claes wrote:
> 
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > [[PGP Signed Part:Undecided]]
> > [1. text/plain]
> > The t1300 test suite exercises the git-config(1) tool. To do so we
> > overwrite ".git/config" to contain custom contents. While this is easy
> > enough to do, it may create problems when using a non-default repository
> > format because we also overwrite the repository format version as well
> > as any potential extensions. With the upcoming "reftable" ref backend
> > the result is that we may try to access refs via the "files" backend
> > even though the repository has been initialized with the "reftable"
> > backend.
> >
> > Refactor tests which access the refdb to be more robust by using their
> > own separate repositories, which allows us to be more careful and not
> > discard required extensions.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> 
> > @@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '
> >  '
> >
> >  test_expect_success 'set up custom config file' '
> > -	CUSTOM_CONFIG_FILE="custom.conf" &&
> > -	cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
> > +	cat >"custom.conf" <<-\EOF &&
> >  	[user]
> >  		custom = true
> >  	EOF
> > +	CUSTOM_CONFIG_FILE="$(test-tool path-utils real_path
> >  custom.conf)"
> >  '
> 
> From the commit message it was not clear to me this change was needed.
> Do you think it's worth it to add something to the commit message
> explaining you now need to copy the custom.conf into each seperate
> repository?

Good point in fact. The problem here is that before, CUSTOM_CONFIG_FILE
was using a relative path that wouldn't be found when cd'ing into the
respective subrepositories. By using `path-utils real_path` we resolve
the relative path to the full path, and thus we can find the file
regardless of our shell's current working directory.

Not sure whether this is worth a reroll, but in case you or others think
that it is then I'm happy to add this explanation.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends

[Christian Couder]

On Wed, Jan 10, 2024 at 10:01 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> The t1300 test suite exercises the git-config(1) tool. To do so we
> overwrite ".git/config" to contain custom contents.

Here "we" means "tests in t1300" I guess.

> While this is easy
> enough to do, it may create problems when using a non-default repository
> format because we also overwrite the repository format version as well
> as any potential extensions.

But I am not sure that "we" in the above sentence is also "tests in
t1300". I think overwriting the repo format version and potential
extensions is done by other tests, right? Anyway it would be nice to
clarify this.

> With the upcoming "reftable" ref backend
> the result is that we may try to access refs via the "files" backend
> even though the repository has been initialized with the "reftable"
> backend.

Not sure here also what "we" is. When could refs be accessed via the
"files" backend even though the repo was initialized with the
"reftable" backend? Does this mean that some of the tests in t1300 try
to access refs via the "files" backend while we may want to run all
the tests using the reftable backend?

> Refactor tests which access the refdb to be more robust by using their
> own separate repositories, which allows us to be more careful and not
> discard required extensions.

Not sure what exactly is discarding extensions. Also robust is not
very clear. It would be better to give at least an example of how
things could fail.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t1300-config.sh | 74 ++++++++++++++++++++++++++++++-----------------
>  1 file changed, 48 insertions(+), 26 deletions(-)
>
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index f4e2752134..53c3d65823 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -1099,13 +1099,18 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
>  '
>
>  test_expect_success 'check split_cmdline return' "
> -       git config alias.split-cmdline-fix 'echo \"' &&
> -       test_must_fail git split-cmdline-fix &&
> -       echo foo > foo &&
> -       git add foo &&
> -       git commit -m 'initial commit' &&
> -       git config branch.main.mergeoptions 'echo \"' &&
> -       test_must_fail git merge main
> +       test_when_finished 'rm -rf repo' &&
> +       git init repo &&
> +       (
> +               cd repo &&
> +               git config alias.split-cmdline-fix 'echo \"' &&
> +               test_must_fail git split-cmdline-fix &&
> +               echo foo >foo &&
> +               git add foo &&
> +               git commit -m 'initial commit' &&
> +               git config branch.main.mergeoptions 'echo \"' &&
> +               test_must_fail git merge main
> +       )
>  "

Maybe, while at it, this test could be modernized to use single quotes
around the test code like:

test_expect_success 'check split_cmdline return' '
...
'

or is using double quotes still Ok?

>  test_expect_success 'git -c "key=value" support' '
> @@ -1157,10 +1162,16 @@ test_expect_success 'git -c works with aliases of builtins' '
>  '
>
>  test_expect_success 'aliases can be CamelCased' '
> -       test_config alias.CamelCased "rev-parse HEAD" &&
> -       git CamelCased >out &&
> -       git rev-parse HEAD >expect &&
> -       test_cmp expect out
> +       test_when_finished "rm -rf repo" &&
> +       git init repo &&
> +       (
> +               cd repo &&
> +               test_commit A &&
> +               git config alias.CamelCased "rev-parse HEAD" &&
> +               git CamelCased >out &&
> +               git rev-parse HEAD >expect &&
> +               test_cmp expect out
> +       )
>  '

Here single quotes are used for example.

>  test_expect_success 'git -c does not split values on equals' '
> @@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '
>  '
>
>  test_expect_success 'set up custom config file' '
> -       CUSTOM_CONFIG_FILE="custom.conf" &&
> -       cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
> +       cat >"custom.conf" <<-\EOF &&
>         [user]
>                 custom = true
>         EOF
> +       CUSTOM_CONFIG_FILE="$(test-tool path-utils real_path custom.conf)"
>  '

This looks like a test modernization, but maybe it's part of making
the tests more robust. Anyway it might be a good idea to either talk a
bit about that in the commit message or to move it to a preparatory
commit if it's a modernization and other modernizations could be made
in that preparatory commit.

Otherwise this patch looks good to me.

Re: [PATCH v2 3/6] t1302: make tests more robust with new extensions

[Christian Couder]

On Wed, Jan 10, 2024 at 10:02 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> In t1302 we exercise logic around "core.repositoryFormatVersion" and
> extensions. These tests are not particularly robust against extensions
> like the newly introduced "refStorage" extension.

Here also it's not very clear what robust means. Some examples of how
things could fail would perhaps help.

> Refactor the tests to be more robust:
>
>   - Check the DEFAULT_REPO_FORMAT prereq to determine the expected
>     repository format version. This helps to ensure that we only need to
>     update the prereq in a central place when new extensions are added.
>
>   - Use a separate repository to rewrite ".git/config" to test
>     combinations of the repository format version and extensions. This
>     ensures that we don't break the main test repository.
>
>   - Do not rewrite ".git/config" when exercising the "preciousObjects"
>     extension.

It would be nice to talk a bit about the failures that each of the
above changes could prevent.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t1302-repo-version.sh | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
> index 179474fa65..7c680c91c4 100755
> --- a/t/t1302-repo-version.sh
> +++ b/t/t1302-repo-version.sh
> @@ -28,7 +28,12 @@ test_expect_success 'setup' '
>  '
>
>  test_expect_success 'gitdir selection on normal repos' '
> -       test_oid version >expect &&
> +       if test_have_prereq DEFAULT_REPO_FORMAT
> +       then
> +               echo 0
> +       else
> +               echo 1
> +       fi >expect &&
>         git config core.repositoryformatversion >actual &&
>         git -C test config core.repositoryformatversion >actual2 &&
>         test_cmp expect actual &&
> @@ -79,8 +84,13 @@ mkconfig () {

Before that hunk there is:

test_expect_success 'setup' '
    test_oid_cache <<-\EOF &&
    version sha1:0
    version sha256:1
    EOF

and it seems to me that the above test_oid_cache call is not needed
anymore, if `test_oid version` is not used anymore.

Otherwise this looks good to me.

Re: [PATCH v2 0/6] t: mark "files"-backend specific tests

[Christian Couder]

Hi,

On Wed, Jan 10, 2024 at 10:01 AM Patrick Steinhardt <ps@pks.im> wrote:

> Patrick Steinhardt (6):
>   t1300: make tests more robust with non-default ref backends
>   t1301: mark test for `core.sharedRepository` as reffiles specific
>   t1302: make tests more robust with new extensions
>   t1419: mark test suite as files-backend specific
>   t5526: break test submodule differently
>   t: mark tests regarding git-pack-refs(1) to be backend specific

I took a look at these 6 patches and only had comments for patches 1/6
and 3/6 so I replied only to those. The rest looks good to me.

Thanks!

Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends

[Justin Tobler]

On Tue, Jan 23, 2024 at 9:22 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, Jan 23, 2024 at 02:41:17PM +0100, Toon Claes wrote:
> >
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> > > [[PGP Signed Part:Undecided]]
> > > [1. text/plain]
> > > The t1300 test suite exercises the git-config(1) tool. To do so we
> > > overwrite ".git/config" to contain custom contents. While this is easy
> > > enough to do, it may create problems when using a non-default repository
> > > format because we also overwrite the repository format version as well
> > > as any potential extensions. With the upcoming "reftable" ref backend
> > > the result is that we may try to access refs via the "files" backend
> > > even though the repository has been initialized with the "reftable"
> > > backend.
> > >
> > > Refactor tests which access the refdb to be more robust by using their
> > > own separate repositories, which allows us to be more careful and not
> > > discard required extensions.
> > >
> > > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > > ---
> >
> > > @@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '
> > >  '
> > >
> > >  test_expect_success 'set up custom config file' '
> > > -   CUSTOM_CONFIG_FILE="custom.conf" &&
> > > -   cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
> > > +   cat >"custom.conf" <<-\EOF &&
> > >     [user]
> > >             custom = true
> > >     EOF
> > > +   CUSTOM_CONFIG_FILE="$(test-tool path-utils real_path
> > >  custom.conf)"
> > >  '
> >
> > From the commit message it was not clear to me this change was needed.
> > Do you think it's worth it to add something to the commit message
> > explaining you now need to copy the custom.conf into each seperate
> > repository?
>
> Good point in fact. The problem here is that before, CUSTOM_CONFIG_FILE
> was using a relative path that wouldn't be found when cd'ing into the
> respective subrepositories. By using `path-utils real_path` we resolve
> the relative path to the full path, and thus we can find the file
> regardless of our shell's current working directory.
>
> Not sure whether this is worth a reroll, but in case you or others think
> that it is then I'm happy to add this explanation.

I also found it unclear why the CUSTOM_CONFIG_FILE change was needed.
I had assumed it was a refactor to make the tests more robust. It might be
nice to explain it in the commit message. :)

-Justin

[PATCH v3 0/6] t: mark "files"-backend specific tests

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 7632 bytes --]

Hi,

this is the third version of my patch series that addresses tests whihc
are specific to the "files" backend. Changes compared to v2:

  - Reworded some commit messages to hopefully explain better what is
    going on.

  - Refactored a test "while at it" to not use double quotes for the
    test body.

  - Removed the now-unneeded OID cache for one of our tests.

Thanks all for your comments!

Patrick

Patrick Steinhardt (6):
  t1300: make tests more robust with non-default ref backends
  t1301: mark test for `core.sharedRepository` as reffiles specific
  t1302: make tests more robust with new extensions
  t1419: mark test suite as files-backend specific
  t5526: break test submodule differently
  t: mark tests regarding git-pack-refs(1) to be backend specific

 t/t1300-config.sh             | 78 ++++++++++++++++++++++-------------
 t/t1301-shared-repo.sh        |  2 +-
 t/t1302-repo-version.sh       | 23 +++++++----
 t/t1409-avoid-packing-refs.sh |  6 +++
 t/t1419-exclude-refs.sh       |  6 +++
 t/t3210-pack-refs.sh          |  6 +++
 t/t5526-fetch-submodules.sh   |  2 +-
 7 files changed, 85 insertions(+), 38 deletions(-)

Range-diff against v2:
1:  0552123fa3 ! 1:  a57e57a7c3 t1300: make tests more robust with non-default ref backends
    @@ Metadata
      ## Commit message ##
         t1300: make tests more robust with non-default ref backends
     
    -    The t1300 test suite exercises the git-config(1) tool. To do so we
    -    overwrite ".git/config" to contain custom contents. While this is easy
    -    enough to do, it may create problems when using a non-default repository
    -    format because we also overwrite the repository format version as well
    -    as any potential extensions. With the upcoming "reftable" ref backend
    -    the result is that we may try to access refs via the "files" backend
    -    even though the repository has been initialized with the "reftable"
    -    backend.
    +    The t1300 test suite exercises the git-config(1) tool. To do so, the
    +    test overwrites ".git/config" to contain custom contents. While this is
    +    easy enough to do, it may create problems when using a non-default
    +    repository format because this causes us to overwrite the repository
    +    format version as well as any potential extensions. With the upcoming
    +    "reftable" ref backend the result is that Git would try to access refs
    +    via the "files" backend even though the repository has been initialized
    +    with the "reftable" backend, which will cause failures when trying to
    +    access any refs.
     
         Refactor tests which access the refdb to be more robust by using their
         own separate repositories, which allows us to be more careful and not
         discard required extensions.
     
    +    Note that we also have to touch up how the CUSTOM_CONFIG_FILE gets
    +    accessed. This environment variable contains the relative path to a
    +    custom config file which we're setting up. But because we are now using
    +    subrepositories, this relative path will not be found anymore because
    +    our working directory changes. This issue is addressed by storing the
    +    absolute path to the file in CUSTOM_CONFIG_FILE instead.
    +
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## t/t1300-config.sh ##
     @@ t/t1300-config.sh: test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
    + 	test_must_fail git config --file=linktolinktonada --list
      '
      
    - test_expect_success 'check split_cmdline return' "
    +-test_expect_success 'check split_cmdline return' "
     -	git config alias.split-cmdline-fix 'echo \"' &&
     -	test_must_fail git split-cmdline-fix &&
     -	echo foo > foo &&
    @@ t/t1300-config.sh: test_expect_success SYMLINKS 'symlink to nonexistent configur
     -	git commit -m 'initial commit' &&
     -	git config branch.main.mergeoptions 'echo \"' &&
     -	test_must_fail git merge main
    -+	test_when_finished 'rm -rf repo' &&
    +-"
    ++test_expect_success 'check split_cmdline return' '
    ++	test_when_finished "rm -rf repo" &&
     +	git init repo &&
     +	(
     +		cd repo &&
    -+		git config alias.split-cmdline-fix 'echo \"' &&
    ++		git config alias.split-cmdline-fix "echo \"" &&
     +		test_must_fail git split-cmdline-fix &&
     +		echo foo >foo &&
     +		git add foo &&
    -+		git commit -m 'initial commit' &&
    -+		git config branch.main.mergeoptions 'echo \"' &&
    ++		git commit -m "initial commit" &&
    ++		git config branch.main.mergeoptions "echo \"" &&
     +		test_must_fail git merge main
     +	)
    - "
    ++'
      
      test_expect_success 'git -c "key=value" support' '
    + 	cat >expect <<-\EOF &&
     @@ t/t1300-config.sh: test_expect_success 'git -c works with aliases of builtins' '
      '
      
2:  384250fec2 = 2:  fd6dd92c23 t1301: mark test for `core.sharedRepository` as reffiles specific
3:  1284b70fab ! 3:  ec90320ff1 t1302: make tests more robust with new extensions
    @@ Commit message
     
         In t1302 we exercise logic around "core.repositoryFormatVersion" and
         extensions. These tests are not particularly robust against extensions
    -    like the newly introduced "refStorage" extension.
    +    like the newly introduced "refStorage" extension as we tend to clobber
    +    the repository's config file. We thus overwrite any extensions that were
    +    set, which may render the repository inaccessible in case it has to be
    +    accessed with a non-default ref storage.
     
         Refactor the tests to be more robust:
     
           - Check the DEFAULT_REPO_FORMAT prereq to determine the expected
             repository format version. This helps to ensure that we only need to
             update the prereq in a central place when new extensions are added.
    +        Furthermore, this allows us to stop seeding the now-unneeded object
    +        ID cache that was only used to figure out the repository version.
     
           - Use a separate repository to rewrite ".git/config" to test
             combinations of the repository format version and extensions. This
    -        ensures that we don't break the main test repository.
    +        ensures that we don't break the main test repository. While we could
    +        rewrite these tests to not overwrite preexisting extensions, it
    +        feels cleaner like this so that we can test extensions standalone
    +        without interference from the environment.
     
           - Do not rewrite ".git/config" when exercising the "preciousObjects"
             extension.
    @@ Commit message
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## t/t1302-repo-version.sh ##
    +@@ t/t1302-repo-version.sh: TEST_PASSES_SANITIZE_LEAK=true
    + . ./test-lib.sh
    + 
    + test_expect_success 'setup' '
    +-	test_oid_cache <<-\EOF &&
    +-	version sha1:0
    +-	version sha256:1
    +-	EOF
    + 	cat >test.patch <<-\EOF &&
    + 	diff --git a/test.txt b/test.txt
    + 	new file mode 100644
     @@ t/t1302-repo-version.sh: test_expect_success 'setup' '
      '
      
4:  c6062b612c = 4:  d0d70c3f18 t1419: mark test suite as files-backend specific
5:  ae08afc459 = 5:  066c297189 t5526: break test submodule differently
6:  df648be535 = 6:  7b8921817b t: mark tests regarding git-pack-refs(1) to be backend specific
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 1/6] t1300: make tests more robust with non-default ref backends

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4893 bytes --]

The t1300 test suite exercises the git-config(1) tool. To do so, the
test overwrites ".git/config" to contain custom contents. While this is
easy enough to do, it may create problems when using a non-default
repository format because this causes us to overwrite the repository
format version as well as any potential extensions. With the upcoming
"reftable" ref backend the result is that Git would try to access refs
via the "files" backend even though the repository has been initialized
with the "reftable" backend, which will cause failures when trying to
access any refs.

Refactor tests which access the refdb to be more robust by using their
own separate repositories, which allows us to be more careful and not
discard required extensions.

Note that we also have to touch up how the CUSTOM_CONFIG_FILE gets
accessed. This environment variable contains the relative path to a
custom config file which we're setting up. But because we are now using
subrepositories, this relative path will not be found anymore because
our working directory changes. This issue is addressed by storing the
absolute path to the file in CUSTOM_CONFIG_FILE instead.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1300-config.sh | 78 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 50 insertions(+), 28 deletions(-)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index f4e2752134..31c3878687 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1098,15 +1098,20 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
 	test_must_fail git config --file=linktolinktonada --list
 '
 
-test_expect_success 'check split_cmdline return' "
-	git config alias.split-cmdline-fix 'echo \"' &&
-	test_must_fail git split-cmdline-fix &&
-	echo foo > foo &&
-	git add foo &&
-	git commit -m 'initial commit' &&
-	git config branch.main.mergeoptions 'echo \"' &&
-	test_must_fail git merge main
-"
+test_expect_success 'check split_cmdline return' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		git config alias.split-cmdline-fix "echo \"" &&
+		test_must_fail git split-cmdline-fix &&
+		echo foo >foo &&
+		git add foo &&
+		git commit -m "initial commit" &&
+		git config branch.main.mergeoptions "echo \"" &&
+		test_must_fail git merge main
+	)
+'
 
 test_expect_success 'git -c "key=value" support' '
 	cat >expect <<-\EOF &&
@@ -1157,10 +1162,16 @@ test_expect_success 'git -c works with aliases of builtins' '
 '
 
 test_expect_success 'aliases can be CamelCased' '
-	test_config alias.CamelCased "rev-parse HEAD" &&
-	git CamelCased >out &&
-	git rev-parse HEAD >expect &&
-	test_cmp expect out
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		git config alias.CamelCased "rev-parse HEAD" &&
+		git CamelCased >out &&
+		git rev-parse HEAD >expect &&
+		test_cmp expect out
+	)
 '
 
 test_expect_success 'git -c does not split values on equals' '
@@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '
 '
 
 test_expect_success 'set up custom config file' '
-	CUSTOM_CONFIG_FILE="custom.conf" &&
-	cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
+	cat >"custom.conf" <<-\EOF &&
 	[user]
 		custom = true
 	EOF
+	CUSTOM_CONFIG_FILE="$(test-tool path-utils real_path custom.conf)"
 '
 
 test_expect_success !MINGW 'set up custom config file with special name characters' '
@@ -2052,22 +2063,33 @@ test_expect_success '--show-origin stdin with file include' '
 '
 
 test_expect_success '--show-origin blob' '
-	blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") &&
-	cat >expect <<-EOF &&
-	blob:$blob	user.custom=true
-	EOF
-	git config --blob=$blob --show-origin --list >output &&
-	test_cmp expect output
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") &&
+		cat >expect <<-EOF &&
+		blob:$blob	user.custom=true
+		EOF
+		git config --blob=$blob --show-origin --list >output &&
+		test_cmp expect output
+	)
 '
 
 test_expect_success '--show-origin blob ref' '
-	cat >expect <<-\EOF &&
-	blob:main:custom.conf	user.custom=true
-	EOF
-	git add "$CUSTOM_CONFIG_FILE" &&
-	git commit -m "new config file" &&
-	git config --blob=main:"$CUSTOM_CONFIG_FILE" --show-origin --list >output &&
-	test_cmp expect output
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		cat >expect <<-\EOF &&
+		blob:main:custom.conf	user.custom=true
+		EOF
+		cp "$CUSTOM_CONFIG_FILE" custom.conf &&
+		git add custom.conf &&
+		git commit -m "new config file" &&
+		git config --blob=main:custom.conf --show-origin --list >output &&
+		test_cmp expect output
+	)
 '
 
 test_expect_success '--show-origin with --default' '
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 2/6] t1301: mark test for `core.sharedRepository` as reffiles specific

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

In t1301 we verify whether reflog files written by the "files" ref
backend correctly honor permissions when "core.sharedRepository" is set.
The test logic is thus specific to the reffiles backend and will not
work with any other backends.

Mark the test accordingly with the REFFILES prereq.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1301-shared-repo.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index e5a0d65caa..8e2c01e760 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -137,7 +137,7 @@ test_expect_success POSIXPERM 'info/refs respects umask in unshared repo' '
 	test_cmp expect actual
 '
 
-test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
+test_expect_success REFFILES,POSIXPERM 'git reflog expire honors core.sharedRepository' '
 	umask 077 &&
 	git config core.sharedRepository group &&
 	git reflog expire --all &&
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 3/6] t1302: make tests more robust with new extensions

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2977 bytes --]

In t1302 we exercise logic around "core.repositoryFormatVersion" and
extensions. These tests are not particularly robust against extensions
like the newly introduced "refStorage" extension as we tend to clobber
the repository's config file. We thus overwrite any extensions that were
set, which may render the repository inaccessible in case it has to be
accessed with a non-default ref storage.

Refactor the tests to be more robust:

  - Check the DEFAULT_REPO_FORMAT prereq to determine the expected
    repository format version. This helps to ensure that we only need to
    update the prereq in a central place when new extensions are added.
    Furthermore, this allows us to stop seeding the now-unneeded object
    ID cache that was only used to figure out the repository version.

  - Use a separate repository to rewrite ".git/config" to test
    combinations of the repository format version and extensions. This
    ensures that we don't break the main test repository. While we could
    rewrite these tests to not overwrite preexisting extensions, it
    feels cleaner like this so that we can test extensions standalone
    without interference from the environment.

  - Do not rewrite ".git/config" when exercising the "preciousObjects"
    extension.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1302-repo-version.sh | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
index 179474fa65..42caa0d297 100755
--- a/t/t1302-repo-version.sh
+++ b/t/t1302-repo-version.sh
@@ -9,10 +9,6 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-	test_oid_cache <<-\EOF &&
-	version sha1:0
-	version sha256:1
-	EOF
 	cat >test.patch <<-\EOF &&
 	diff --git a/test.txt b/test.txt
 	new file mode 100644
@@ -28,7 +24,12 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'gitdir selection on normal repos' '
-	test_oid version >expect &&
+	if test_have_prereq DEFAULT_REPO_FORMAT
+	then
+		echo 0
+	else
+		echo 1
+	fi >expect &&
 	git config core.repositoryformatversion >actual &&
 	git -C test config core.repositoryformatversion >actual2 &&
 	test_cmp expect actual &&
@@ -79,8 +80,13 @@ mkconfig () {
 
 while read outcome version extensions; do
 	test_expect_success "$outcome version=$version $extensions" "
-		mkconfig $version $extensions >.git/config &&
-		check_${outcome}
+		test_when_finished 'rm -rf extensions' &&
+		git init extensions &&
+		(
+			cd extensions &&
+			mkconfig $version $extensions >.git/config &&
+			check_${outcome}
+		)
 	"
 done <<\EOF
 allow 0
@@ -94,7 +100,8 @@ allow 1 noop-v1
 EOF
 
 test_expect_success 'precious-objects allowed' '
-	mkconfig 1 preciousObjects >.git/config &&
+	git config core.repositoryFormatVersion 1 &&
+	git config extensions.preciousObjects 1 &&
 	check_allow
 '
 
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 4/6] t1419: mark test suite as files-backend specific

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2360 bytes --]

With 59c35fac54 (refs/packed-backend.c: implement jump lists to avoid
excluded pattern(s), 2023-07-10) we have implemented logic to handle
excluded refs more efficiently in the "packed" ref backend. This logic
allows us to skip emitting refs completely which we know to not be of
any interest to the caller, which can avoid quite some allocations and
object lookups.

This was wired up via a new `exclude_patterns` parameter passed to the
backend's ref iterator. The backend only needs to handle them on a best
effort basis though, and in fact we only handle it for the "packed-refs"
file, but not for loose references. Consequently, all callers must still
filter emitted refs with those exclude patterns.

The result is that handling exclude patterns is completely optional in
the ref backend, and any future backends may or may not implement it.
Let's thus mark the test for t1419 to depend on the REFFILES prereq.

An alternative would be to introduce a new prereq that tells us whether
the backend under test supports exclude patterns or not. But this does
feel a bit overblown:

  - It would either map to the REFFILES prereq, in which case it feels
    overengineered because the prereq is only ever relevant to t1419.

  - Otherwise, it could auto-detect whether the backend supports exclude
    patterns. But this could lead to silent failures in case the support
    for this feature breaks at any point in time.

It should thus be good enough to just use the REFFILES prereq for now.
If future backends ever grow support for exclude patterns we can easily
add their respective prereq as another condition for this test suite to
execute.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1419-exclude-refs.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/t1419-exclude-refs.sh b/t/t1419-exclude-refs.sh
index 5d8c86b657..1359574419 100755
--- a/t/t1419-exclude-refs.sh
+++ b/t/t1419-exclude-refs.sh
@@ -8,6 +8,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+if test_have_prereq !REFFILES
+then
+	skip_all='skipping `git for-each-ref --exclude` tests; need files backend'
+	test_done
+fi
+
 for_each_ref__exclude () {
 	GIT_TRACE2_PERF=1 test-tool ref-store main \
 		for-each-ref--exclude "$@" >actual.raw
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 5/6] t5526: break test submodule differently

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1873 bytes --]

In 10f5c52656 (submodule: avoid auto-discovery in
prepare_submodule_repo_env(), 2016-09-01) we fixed a bug when doing a
recursive fetch with submodule in the case where the submodule is broken
due to whatever reason. The test to exercise that the fix works breaks
the submodule by deleting its `HEAD` reference, which will cause us to
not detect the directory as a Git repository.

While this is perfectly fine in theory, this way of breaking the repo
becomes problematic with the current efforts to introduce another refdb
backend into Git. The new reftable backend has a stub HEAD file that
always contains "ref: refs/heads/.invalid" so that tools continue to be
able to detect such a repository. But as the reftable backend will never
delete this file even when asked to delete `HEAD` the current way to
delete the `HEAD` reference will stop working.

Adapt the code to instead delete the objects database. Going back with
this new way to cause breakage confirms that it triggers the infinite
recursion just the same, and there are no equivalent ongoing efforts to
replace the object database with an alternate backend.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5526-fetch-submodules.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 7ab220fa31..5e566205ba 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '
 	git -C dst fetch --recurse-submodules &&
 
 	# Break the receiving submodule
-	test-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&
+	rm -r dst/sub/.git/objects &&
 
 	# NOTE: without the fix the following tests will recurse forever!
 	# They should terminate with an error.
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 6/6] t: mark tests regarding git-pack-refs(1) to be backend specific

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]

Both t1409 and t3210 exercise parts of git-pack-refs(1). Given that we
must check the on-disk files to verify whether the backend has indeed
packed refs as expected those test suites are deeply tied to the actual
backend that is in use.

Mark the test suites to depend on the REFFILES backend.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1409-avoid-packing-refs.sh | 6 ++++++
 t/t3210-pack-refs.sh          | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index f23c0152a8..7748973733 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -5,6 +5,12 @@ test_description='avoid rewriting packed-refs unnecessarily'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+if test_have_prereq !REFFILES
+then
+  skip_all='skipping files-backend specific pack-refs tests'
+  test_done
+fi
+
 # Add an identifying mark to the packed-refs file header line. This
 # shouldn't upset readers, and it should be omitted if the file is
 # ever rewritten.
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 7f4e98db7d..c0f1f9cfb7 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -15,6 +15,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+if test_have_prereq !REFFILES
+then
+  skip_all='skipping files-backend specific pack-refs tests'
+  test_done
+fi
+
 test_expect_success 'enable reflogs' '
 	git config core.logallrefupdates true
 '
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/6] t1302: make tests more robust with new extensions

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2623 bytes --]

On Tue, Jan 23, 2024 at 05:15:56PM +0100, Christian Couder wrote:
> On Wed, Jan 10, 2024 at 10:02 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > In t1302 we exercise logic around "core.repositoryFormatVersion" and
> > extensions. These tests are not particularly robust against extensions
> > like the newly introduced "refStorage" extension.
> 
> Here also it's not very clear what robust means. Some examples of how
> things could fail would perhaps help.
> 
> > Refactor the tests to be more robust:
> >
> >   - Check the DEFAULT_REPO_FORMAT prereq to determine the expected
> >     repository format version. This helps to ensure that we only need to
> >     update the prereq in a central place when new extensions are added.
> >
> >   - Use a separate repository to rewrite ".git/config" to test
> >     combinations of the repository format version and extensions. This
> >     ensures that we don't break the main test repository.
> >
> >   - Do not rewrite ".git/config" when exercising the "preciousObjects"
> >     extension.
> 
> It would be nice to talk a bit about the failures that each of the
> above changes could prevent.

They all fail in the same way: we fail to access the repository because
we delete the extension that tells us to use the reftable backend. I'll
add that explanation at the front.

> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  t/t1302-repo-version.sh | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
> > index 179474fa65..7c680c91c4 100755
> > --- a/t/t1302-repo-version.sh
> > +++ b/t/t1302-repo-version.sh
> > @@ -28,7 +28,12 @@ test_expect_success 'setup' '
> >  '
> >
> >  test_expect_success 'gitdir selection on normal repos' '
> > -       test_oid version >expect &&
> > +       if test_have_prereq DEFAULT_REPO_FORMAT
> > +       then
> > +               echo 0
> > +       else
> > +               echo 1
> > +       fi >expect &&
> >         git config core.repositoryformatversion >actual &&
> >         git -C test config core.repositoryformatversion >actual2 &&
> >         test_cmp expect actual &&
> > @@ -79,8 +84,13 @@ mkconfig () {
> 
> Before that hunk there is:
> 
> test_expect_success 'setup' '
>     test_oid_cache <<-\EOF &&
>     version sha1:0
>     version sha256:1
>     EOF
> 
> and it seems to me that the above test_oid_cache call is not needed
> anymore, if `test_oid version` is not used anymore.
> 
> Otherwise this looks good to me.

Good catch.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5000 bytes --]

On Tue, Jan 23, 2024 at 05:15:48PM +0100, Christian Couder wrote:
> On Wed, Jan 10, 2024 at 10:01 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > The t1300 test suite exercises the git-config(1) tool. To do so we
> > overwrite ".git/config" to contain custom contents.
> 
> Here "we" means "tests in t1300" I guess.
> 
> > While this is easy
> > enough to do, it may create problems when using a non-default repository
> > format because we also overwrite the repository format version as well
> > as any potential extensions.
> 
> But I am not sure that "we" in the above sentence is also "tests in
> t1300". I think overwriting the repo format version and potential
> extensions is done by other tests, right? Anyway it would be nice to
> clarify this.
> 
> > With the upcoming "reftable" ref backend
> > the result is that we may try to access refs via the "files" backend
> > even though the repository has been initialized with the "reftable"
> > backend.
> 
> Not sure here also what "we" is. When could refs be accessed via the
> "files" backend even though the repo was initialized with the
> "reftable" backend?

Yeah, I've rephrased all of these to sey "the tests" or something
similar.

> Does this mean that some of the tests in t1300 try to access refs via
> the "files" backend while we may want to run all the tests using the
> reftable backend?

Exactly. We overwrite the ".git/config", which contains the "refStorage"
extension that tells us to use the "reftable" backend. So the extension
is gone, and thus Git would fall back to use the "files" backend
instead, which will fail.

> > Refactor tests which access the refdb to be more robust by using their
> > own separate repositories, which allows us to be more careful and not
> > discard required extensions.
> 
> Not sure what exactly is discarding extensions. Also robust is not
> very clear. It would be better to give at least an example of how
> things could fail.

Hm. I don't really know how to phrase this better. The preceding
paragraph already explains why we're discarding the extension and what
the consequence is. I added a sentence saying ", which will cause
failures when trying to access any refs."

> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  t/t1300-config.sh | 74 ++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 48 insertions(+), 26 deletions(-)
> >
> > diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> > index f4e2752134..53c3d65823 100755
> > --- a/t/t1300-config.sh
> > +++ b/t/t1300-config.sh
> > @@ -1099,13 +1099,18 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
> >  '
> >
> >  test_expect_success 'check split_cmdline return' "
> > -       git config alias.split-cmdline-fix 'echo \"' &&
> > -       test_must_fail git split-cmdline-fix &&
> > -       echo foo > foo &&
> > -       git add foo &&
> > -       git commit -m 'initial commit' &&
> > -       git config branch.main.mergeoptions 'echo \"' &&
> > -       test_must_fail git merge main
> > +       test_when_finished 'rm -rf repo' &&
> > +       git init repo &&
> > +       (
> > +               cd repo &&
> > +               git config alias.split-cmdline-fix 'echo \"' &&
> > +               test_must_fail git split-cmdline-fix &&
> > +               echo foo >foo &&
> > +               git add foo &&
> > +               git commit -m 'initial commit' &&
> > +               git config branch.main.mergeoptions 'echo \"' &&
> > +               test_must_fail git merge main
> > +       )
> >  "
> 
> Maybe, while at it, this test could be modernized to use single quotes
> around the test code like:
> 
> test_expect_success 'check split_cmdline return' '
> ...
> '
> 
> or is using double quotes still Ok?

In general single quotes are preferable. This test is using quotes
internally, which I guess is the reason why we didn't. Happy to change
while at it.

[snip]
> >  test_expect_success 'git -c does not split values on equals' '
> > @@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '
> >  '
> >
> >  test_expect_success 'set up custom config file' '
> > -       CUSTOM_CONFIG_FILE="custom.conf" &&
> > -       cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
> > +       cat >"custom.conf" <<-\EOF &&
> >         [user]
> >                 custom = true
> >         EOF
> > +       CUSTOM_CONFIG_FILE="$(test-tool path-utils real_path custom.conf)"
> >  '
> 
> This looks like a test modernization, but maybe it's part of making
> the tests more robust. Anyway it might be a good idea to either talk a
> bit about that in the commit message or to move it to a preparatory
> commit if it's a modernization and other modernizations could be made
> in that preparatory commit.
> 
> Otherwise this patch looks good to me.

Yup, this has also been pointed out by others. Will mention in the
commit message.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends

[Christian Couder]

On Wed, Jan 24, 2024 at 9:52 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, Jan 23, 2024 at 05:15:48PM +0100, Christian Couder wrote:
> > On Wed, Jan 10, 2024 at 10:01 AM Patrick Steinhardt <ps@pks.im> wrote:
> > >
> > > The t1300 test suite exercises the git-config(1) tool. To do so we
> > > overwrite ".git/config" to contain custom contents.
> >
> > Here "we" means "tests in t1300" I guess.
> >
> > > While this is easy
> > > enough to do, it may create problems when using a non-default repository
> > > format because we also overwrite the repository format version as well
> > > as any potential extensions.
> >
> > But I am not sure that "we" in the above sentence is also "tests in
> > t1300". I think overwriting the repo format version and potential
> > extensions is done by other tests, right? Anyway it would be nice to
> > clarify this.
> >
> > > With the upcoming "reftable" ref backend
> > > the result is that we may try to access refs via the "files" backend
> > > even though the repository has been initialized with the "reftable"
> > > backend.
> >
> > Not sure here also what "we" is. When could refs be accessed via the
> > "files" backend even though the repo was initialized with the
> > "reftable" backend?
>
> Yeah, I've rephrased all of these to sey "the tests" or something
> similar.
>
> > Does this mean that some of the tests in t1300 try to access refs via
> > the "files" backend while we may want to run all the tests using the
> > reftable backend?
>
> Exactly. We overwrite the ".git/config", which contains the "refStorage"
> extension that tells us to use the "reftable" backend. So the extension
> is gone, and thus Git would fall back to use the "files" backend
> instead, which will fail.

Let's take a look at this test:

test_expect_success 'check split_cmdline return' "
    git config alias.split-cmdline-fix 'echo \"' &&
    test_must_fail git split-cmdline-fix &&
    echo foo > foo &&
    git add foo &&
    git commit -m 'initial commit' &&
    git config branch.main.mergeoptions 'echo \"' &&
    test_must_fail git merge main
"

I don't really see how it overwrites anything. When putting some debug
commands before and after that test, it looks like the config file
contains the following before that test:

---
[section "sub=section"]
       val1 = foo=bar
       val2 = foo\nbar
       val3 = \n\n
       val4 =
       val5
[section]
       val = foo \t  bar
---

and the following after that test:

---
[section "sub=section"]
       val1 = foo=bar
       val2 = foo\nbar
       val3 = \n\n
       val4 =
       val5
[section]
       val = foo \t  bar
[alias]
       split-cmdline-fix = echo \"
[branch "main"]
       mergeoptions = echo \"
---

So it doesn't look like it overwrites anything. To me it just adds
stuff at the end of the file.

> > > Refactor tests which access the refdb to be more robust by using their
> > > own separate repositories, which allows us to be more careful and not
> > > discard required extensions.
> >
> > Not sure what exactly is discarding extensions. Also robust is not
> > very clear. It would be better to give at least an example of how
> > things could fail.
>
> Hm. I don't really know how to phrase this better. The preceding
> paragraph already explains why we're discarding the extension and what
> the consequence is. I added a sentence saying ", which will cause
> failures when trying to access any refs."

To me the preceding paragraph said that we are overwriting the config
file, but I just don't see how for example the above test overwrites
anything. So maybe I am missing something obvious, or maybe you don't
quite mean "overwrite", but I don't see how the extension would be
discarded by the test which only seems to add stuff.

Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]

On Mon, Jan 29, 2024 at 11:32:53AM +0100, Christian Couder wrote:
> On Wed, Jan 24, 2024 at 9:52 AM Patrick Steinhardt <ps@pks.im> wrote:
> > On Tue, Jan 23, 2024 at 05:15:48PM +0100, Christian Couder wrote:
> > > On Wed, Jan 10, 2024 at 10:01 AM Patrick Steinhardt <ps@pks.im> wrote:
[snip]
> > Hm. I don't really know how to phrase this better. The preceding
> > paragraph already explains why we're discarding the extension and what
> > the consequence is. I added a sentence saying ", which will cause
> > failures when trying to access any refs."
> 
> To me the preceding paragraph said that we are overwriting the config
> file, but I just don't see how for example the above test overwrites
> anything. So maybe I am missing something obvious, or maybe you don't
> quite mean "overwrite", but I don't see how the extension would be
> discarded by the test which only seems to add stuff.

It happens before already, outside of any tests. See line 1036:

```
cat > .git/config <<\EOF
[section "sub=section"]
	val1 = foo=bar
	val2 = foo\nbar
	val3 = \n\n
	val4 =
	val5
EOF
```

Overall, I agree that this is rather hard to discover and that the tests
really could require a bigger refactoring to make them more independent
of each other.

I'll send another version that mentions this explicitly.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v4 0/6] t: mark "files"-backend specific tests

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3908 bytes --]

Hi,

this is the fourth version of my patch series that addresses tests which
are specific to the "files" backend. There is only a single change
compared to v3, which is an improved commit message for the first patch.

Thanks!

Patrick

Patrick Steinhardt (6):
  t1300: make tests more robust with non-default ref backends
  t1301: mark test for `core.sharedRepository` as reffiles specific
  t1302: make tests more robust with new extensions
  t1419: mark test suite as files-backend specific
  t5526: break test submodule differently
  t: mark tests regarding git-pack-refs(1) to be backend specific

 t/t1300-config.sh             | 78 ++++++++++++++++++++++-------------
 t/t1301-shared-repo.sh        |  2 +-
 t/t1302-repo-version.sh       | 23 +++++++----
 t/t1409-avoid-packing-refs.sh |  6 +++
 t/t1419-exclude-refs.sh       |  6 +++
 t/t3210-pack-refs.sh          |  6 +++
 t/t5526-fetch-submodules.sh   |  2 +-
 7 files changed, 85 insertions(+), 38 deletions(-)

Range-diff against v3:
1:  a57e57a7c3 ! 1:  80a74bbb56 t1300: make tests more robust with non-default ref backends
    @@ Commit message
         t1300: make tests more robust with non-default ref backends
     
         The t1300 test suite exercises the git-config(1) tool. To do so, the
    -    test overwrites ".git/config" to contain custom contents. While this is
    -    easy enough to do, it may create problems when using a non-default
    -    repository format because this causes us to overwrite the repository
    -    format version as well as any potential extensions. With the upcoming
    -    "reftable" ref backend the result is that Git would try to access refs
    -    via the "files" backend even though the repository has been initialized
    -    with the "reftable" backend, which will cause failures when trying to
    -    access any refs.
    +    test overwrites ".git/config" to contain custom contents in several
    +    places with code like the following:
     
    -    Refactor tests which access the refdb to be more robust by using their
    -    own separate repositories, which allows us to be more careful and not
    -    discard required extensions.
    +    ```
    +    cat > .git/config <<\EOF
    +    ...
    +    EOF
    +    ```
    +
    +    While this is easy enough to do, it may create problems when using a
    +    non-default repository format because this causes us to overwrite the
    +    repository format version as well as any potential extensions. With the
    +    upcoming "reftable" ref backend the result is that Git would try to
    +    access refs via the "files" backend even though the repository has been
    +    initialized with the "reftable" backend, which will cause failures when
    +    trying to access any refs.
    +
    +    Ideally, we would rewrite the whole test suite to not depend on state
    +    written by previous tests, but that would result in a lot of changes in
    +    this test suite. Instead, we only refactor tests which access the refdb
    +    to be more robust by using their own separate repositories, which allows
    +    us to be more careful and not discard required extensions.
     
         Note that we also have to touch up how the CUSTOM_CONFIG_FILE gets
         accessed. This environment variable contains the relative path to a
2:  fd6dd92c23 = 2:  4359d3ffa8 t1301: mark test for `core.sharedRepository` as reffiles specific
3:  ec90320ff1 = 3:  b72d85df60 t1302: make tests more robust with new extensions
4:  d0d70c3f18 = 4:  1faa8687ae t1419: mark test suite as files-backend specific
5:  066c297189 = 5:  4b95277e20 t5526: break test submodule differently
6:  7b8921817b = 6:  53aea8236d t: mark tests regarding git-pack-refs(1) to be backend specific

base-commit: b50a608ba20348cb3dfc16a696816d51780e3f0f
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v4 1/6] t1300: make tests more robust with non-default ref backends

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5169 bytes --]

The t1300 test suite exercises the git-config(1) tool. To do so, the
test overwrites ".git/config" to contain custom contents in several
places with code like the following:

```
cat > .git/config <<\EOF
...
EOF
```

While this is easy enough to do, it may create problems when using a
non-default repository format because this causes us to overwrite the
repository format version as well as any potential extensions. With the
upcoming "reftable" ref backend the result is that Git would try to
access refs via the "files" backend even though the repository has been
initialized with the "reftable" backend, which will cause failures when
trying to access any refs.

Ideally, we would rewrite the whole test suite to not depend on state
written by previous tests, but that would result in a lot of changes in
this test suite. Instead, we only refactor tests which access the refdb
to be more robust by using their own separate repositories, which allows
us to be more careful and not discard required extensions.

Note that we also have to touch up how the CUSTOM_CONFIG_FILE gets
accessed. This environment variable contains the relative path to a
custom config file which we're setting up. But because we are now using
subrepositories, this relative path will not be found anymore because
our working directory changes. This issue is addressed by storing the
absolute path to the file in CUSTOM_CONFIG_FILE instead.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1300-config.sh | 78 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 50 insertions(+), 28 deletions(-)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index f4e2752134..31c3878687 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1098,15 +1098,20 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
 	test_must_fail git config --file=linktolinktonada --list
 '
 
-test_expect_success 'check split_cmdline return' "
-	git config alias.split-cmdline-fix 'echo \"' &&
-	test_must_fail git split-cmdline-fix &&
-	echo foo > foo &&
-	git add foo &&
-	git commit -m 'initial commit' &&
-	git config branch.main.mergeoptions 'echo \"' &&
-	test_must_fail git merge main
-"
+test_expect_success 'check split_cmdline return' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		git config alias.split-cmdline-fix "echo \"" &&
+		test_must_fail git split-cmdline-fix &&
+		echo foo >foo &&
+		git add foo &&
+		git commit -m "initial commit" &&
+		git config branch.main.mergeoptions "echo \"" &&
+		test_must_fail git merge main
+	)
+'
 
 test_expect_success 'git -c "key=value" support' '
 	cat >expect <<-\EOF &&
@@ -1157,10 +1162,16 @@ test_expect_success 'git -c works with aliases of builtins' '
 '
 
 test_expect_success 'aliases can be CamelCased' '
-	test_config alias.CamelCased "rev-parse HEAD" &&
-	git CamelCased >out &&
-	git rev-parse HEAD >expect &&
-	test_cmp expect out
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit A &&
+		git config alias.CamelCased "rev-parse HEAD" &&
+		git CamelCased >out &&
+		git rev-parse HEAD >expect &&
+		test_cmp expect out
+	)
 '
 
 test_expect_success 'git -c does not split values on equals' '
@@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '
 '
 
 test_expect_success 'set up custom config file' '
-	CUSTOM_CONFIG_FILE="custom.conf" &&
-	cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
+	cat >"custom.conf" <<-\EOF &&
 	[user]
 		custom = true
 	EOF
+	CUSTOM_CONFIG_FILE="$(test-tool path-utils real_path custom.conf)"
 '
 
 test_expect_success !MINGW 'set up custom config file with special name characters' '
@@ -2052,22 +2063,33 @@ test_expect_success '--show-origin stdin with file include' '
 '
 
 test_expect_success '--show-origin blob' '
-	blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") &&
-	cat >expect <<-EOF &&
-	blob:$blob	user.custom=true
-	EOF
-	git config --blob=$blob --show-origin --list >output &&
-	test_cmp expect output
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		blob=$(git hash-object -w "$CUSTOM_CONFIG_FILE") &&
+		cat >expect <<-EOF &&
+		blob:$blob	user.custom=true
+		EOF
+		git config --blob=$blob --show-origin --list >output &&
+		test_cmp expect output
+	)
 '
 
 test_expect_success '--show-origin blob ref' '
-	cat >expect <<-\EOF &&
-	blob:main:custom.conf	user.custom=true
-	EOF
-	git add "$CUSTOM_CONFIG_FILE" &&
-	git commit -m "new config file" &&
-	git config --blob=main:"$CUSTOM_CONFIG_FILE" --show-origin --list >output &&
-	test_cmp expect output
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		cat >expect <<-\EOF &&
+		blob:main:custom.conf	user.custom=true
+		EOF
+		cp "$CUSTOM_CONFIG_FILE" custom.conf &&
+		git add custom.conf &&
+		git commit -m "new config file" &&
+		git config --blob=main:custom.conf --show-origin --list >output &&
+		test_cmp expect output
+	)
 '
 
 test_expect_success '--show-origin with --default' '
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v4 2/6] t1301: mark test for `core.sharedRepository` as reffiles specific

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

In t1301 we verify whether reflog files written by the "files" ref
backend correctly honor permissions when "core.sharedRepository" is set.
The test logic is thus specific to the reffiles backend and will not
work with any other backends.

Mark the test accordingly with the REFFILES prereq.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1301-shared-repo.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index e5a0d65caa..8e2c01e760 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -137,7 +137,7 @@ test_expect_success POSIXPERM 'info/refs respects umask in unshared repo' '
 	test_cmp expect actual
 '
 
-test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
+test_expect_success REFFILES,POSIXPERM 'git reflog expire honors core.sharedRepository' '
 	umask 077 &&
 	git config core.sharedRepository group &&
 	git reflog expire --all &&
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v4 3/6] t1302: make tests more robust with new extensions

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2977 bytes --]

In t1302 we exercise logic around "core.repositoryFormatVersion" and
extensions. These tests are not particularly robust against extensions
like the newly introduced "refStorage" extension as we tend to clobber
the repository's config file. We thus overwrite any extensions that were
set, which may render the repository inaccessible in case it has to be
accessed with a non-default ref storage.

Refactor the tests to be more robust:

  - Check the DEFAULT_REPO_FORMAT prereq to determine the expected
    repository format version. This helps to ensure that we only need to
    update the prereq in a central place when new extensions are added.
    Furthermore, this allows us to stop seeding the now-unneeded object
    ID cache that was only used to figure out the repository version.

  - Use a separate repository to rewrite ".git/config" to test
    combinations of the repository format version and extensions. This
    ensures that we don't break the main test repository. While we could
    rewrite these tests to not overwrite preexisting extensions, it
    feels cleaner like this so that we can test extensions standalone
    without interference from the environment.

  - Do not rewrite ".git/config" when exercising the "preciousObjects"
    extension.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1302-repo-version.sh | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
index 179474fa65..42caa0d297 100755
--- a/t/t1302-repo-version.sh
+++ b/t/t1302-repo-version.sh
@@ -9,10 +9,6 @@ TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-	test_oid_cache <<-\EOF &&
-	version sha1:0
-	version sha256:1
-	EOF
 	cat >test.patch <<-\EOF &&
 	diff --git a/test.txt b/test.txt
 	new file mode 100644
@@ -28,7 +24,12 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'gitdir selection on normal repos' '
-	test_oid version >expect &&
+	if test_have_prereq DEFAULT_REPO_FORMAT
+	then
+		echo 0
+	else
+		echo 1
+	fi >expect &&
 	git config core.repositoryformatversion >actual &&
 	git -C test config core.repositoryformatversion >actual2 &&
 	test_cmp expect actual &&
@@ -79,8 +80,13 @@ mkconfig () {
 
 while read outcome version extensions; do
 	test_expect_success "$outcome version=$version $extensions" "
-		mkconfig $version $extensions >.git/config &&
-		check_${outcome}
+		test_when_finished 'rm -rf extensions' &&
+		git init extensions &&
+		(
+			cd extensions &&
+			mkconfig $version $extensions >.git/config &&
+			check_${outcome}
+		)
 	"
 done <<\EOF
 allow 0
@@ -94,7 +100,8 @@ allow 1 noop-v1
 EOF
 
 test_expect_success 'precious-objects allowed' '
-	mkconfig 1 preciousObjects >.git/config &&
+	git config core.repositoryFormatVersion 1 &&
+	git config extensions.preciousObjects 1 &&
 	check_allow
 '
 
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v4 4/6] t1419: mark test suite as files-backend specific

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2360 bytes --]

With 59c35fac54 (refs/packed-backend.c: implement jump lists to avoid
excluded pattern(s), 2023-07-10) we have implemented logic to handle
excluded refs more efficiently in the "packed" ref backend. This logic
allows us to skip emitting refs completely which we know to not be of
any interest to the caller, which can avoid quite some allocations and
object lookups.

This was wired up via a new `exclude_patterns` parameter passed to the
backend's ref iterator. The backend only needs to handle them on a best
effort basis though, and in fact we only handle it for the "packed-refs"
file, but not for loose references. Consequently, all callers must still
filter emitted refs with those exclude patterns.

The result is that handling exclude patterns is completely optional in
the ref backend, and any future backends may or may not implement it.
Let's thus mark the test for t1419 to depend on the REFFILES prereq.

An alternative would be to introduce a new prereq that tells us whether
the backend under test supports exclude patterns or not. But this does
feel a bit overblown:

  - It would either map to the REFFILES prereq, in which case it feels
    overengineered because the prereq is only ever relevant to t1419.

  - Otherwise, it could auto-detect whether the backend supports exclude
    patterns. But this could lead to silent failures in case the support
    for this feature breaks at any point in time.

It should thus be good enough to just use the REFFILES prereq for now.
If future backends ever grow support for exclude patterns we can easily
add their respective prereq as another condition for this test suite to
execute.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1419-exclude-refs.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/t1419-exclude-refs.sh b/t/t1419-exclude-refs.sh
index 5d8c86b657..1359574419 100755
--- a/t/t1419-exclude-refs.sh
+++ b/t/t1419-exclude-refs.sh
@@ -8,6 +8,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+if test_have_prereq !REFFILES
+then
+	skip_all='skipping `git for-each-ref --exclude` tests; need files backend'
+	test_done
+fi
+
 for_each_ref__exclude () {
 	GIT_TRACE2_PERF=1 test-tool ref-store main \
 		for-each-ref--exclude "$@" >actual.raw
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v4 5/6] t5526: break test submodule differently

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1873 bytes --]

In 10f5c52656 (submodule: avoid auto-discovery in
prepare_submodule_repo_env(), 2016-09-01) we fixed a bug when doing a
recursive fetch with submodule in the case where the submodule is broken
due to whatever reason. The test to exercise that the fix works breaks
the submodule by deleting its `HEAD` reference, which will cause us to
not detect the directory as a Git repository.

While this is perfectly fine in theory, this way of breaking the repo
becomes problematic with the current efforts to introduce another refdb
backend into Git. The new reftable backend has a stub HEAD file that
always contains "ref: refs/heads/.invalid" so that tools continue to be
able to detect such a repository. But as the reftable backend will never
delete this file even when asked to delete `HEAD` the current way to
delete the `HEAD` reference will stop working.

Adapt the code to instead delete the objects database. Going back with
this new way to cause breakage confirms that it triggers the infinite
recursion just the same, and there are no equivalent ongoing efforts to
replace the object database with an alternate backend.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t5526-fetch-submodules.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 7ab220fa31..5e566205ba 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '
 	git -C dst fetch --recurse-submodules &&
 
 	# Break the receiving submodule
-	test-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&
+	rm -r dst/sub/.git/objects &&
 
 	# NOTE: without the fix the following tests will recurse forever!
 	# They should terminate with an error.
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v4 6/6] t: mark tests regarding git-pack-refs(1) to be backend specific

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]

Both t1409 and t3210 exercise parts of git-pack-refs(1). Given that we
must check the on-disk files to verify whether the backend has indeed
packed refs as expected those test suites are deeply tied to the actual
backend that is in use.

Mark the test suites to depend on the REFFILES backend.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t1409-avoid-packing-refs.sh | 6 ++++++
 t/t3210-pack-refs.sh          | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index f23c0152a8..7748973733 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -5,6 +5,12 @@ test_description='avoid rewriting packed-refs unnecessarily'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+if test_have_prereq !REFFILES
+then
+  skip_all='skipping files-backend specific pack-refs tests'
+  test_done
+fi
+
 # Add an identifying mark to the packed-refs file header line. This
 # shouldn't upset readers, and it should be omitted if the file is
 # ever rewritten.
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 7f4e98db7d..c0f1f9cfb7 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -15,6 +15,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+if test_have_prereq !REFFILES
+then
+  skip_all='skipping files-backend specific pack-refs tests'
+  test_done
+fi
+
 test_expect_success 'enable reflogs' '
 	git config core.logallrefupdates true
 '
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 1/6] t1300: make tests more robust with non-default ref backends

[Christian Couder]

On Mon, Jan 29, 2024 at 12:07 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> The t1300 test suite exercises the git-config(1) tool. To do so, the
> test overwrites ".git/config" to contain custom contents in several
> places with code like the following:
>
> ```
> cat > .git/config <<\EOF
> ...
> EOF
> ```

I thought about using a function that would overwrite a config file
safely as it would still copy the repository format version and other
extension information to the new config file, but a number of tests
even do `rm .git/config`, so it wouldn't be enough.

> While this is easy enough to do, it may create problems when using a
> non-default repository format because this causes us to overwrite the
> repository format version as well as any potential extensions. With the
> upcoming "reftable" ref backend the result is that Git would try to
> access refs via the "files" backend even though the repository has been
> initialized with the "reftable" backend, which will cause failures when
> trying to access any refs.
>
> Ideally, we would rewrite the whole test suite to not depend on state
> written by previous tests, but that would result in a lot of changes in
> this test suite.

I agree that the whole test script would need significant work.

> Instead, we only refactor tests which access the refdb
> to be more robust by using their own separate repositories, which allows
> us to be more careful and not discard required extensions.
>
> Note that we also have to touch up how the CUSTOM_CONFIG_FILE gets
> accessed. This environment variable contains the relative path to a
> custom config file which we're setting up. But because we are now using
> subrepositories, this relative path will not be found anymore because
> our working directory changes. This issue is addressed by storing the
> absolute path to the file in CUSTOM_CONFIG_FILE instead.

Re: [PATCH v4 0/6] t: mark "files"-backend specific tests

[Christian Couder]

On Mon, Jan 29, 2024 at 12:07 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> Hi,
>
> this is the fourth version of my patch series that addresses tests which
> are specific to the "files" backend. There is only a single change
> compared to v3, which is an improved commit message for the first patch.

I took another look at the patches in this and it looks good to me
now. Feel free to add my "Reviewed-by:"

Thanks!

Re: [PATCH v4 0/6] t: mark "files"-backend specific tests

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Mon, Jan 29, 2024 at 12:07 PM Patrick Steinhardt <ps@pks.im> wrote:
>>
>> Hi,
>>
>> this is the fourth version of my patch series that addresses tests which
>> are specific to the "files" backend. There is only a single change
>> compared to v3, which is an improved commit message for the first patch.
>
> I took another look at the patches in this and it looks good to me
> now. Feel free to add my "Reviewed-by:"

Thanks.