{"thread":{"id":"60704","subject":"[PATCH 0/6] t: mark \"files\"-backend specific tests","startedAt":"2024-01-09T12:17:04Z","lastAt":"2024-01-29T20:38:04Z","messageCount":54,"participants":["Patrick Steinhardt","Taylor Blau","Eric Sunshine","Junio C Hamano","Toon Claes","Christian Couder","Justin Tobler"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"486426","messageId":"cover.1704802213.git.ps@pks.im","threadId":"60704","inReplyTo":null,"subject":"[PATCH 0/6] t: mark \"files\"-backend specific tests","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-09T12:17:00Z","receivedAt":"2024-01-09T12:17:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series amends some of our tests that exercise \"files\"-backend\nspecific logic to have the respective prereqs. The patch series is quite\nunexciting and part of our final preparations to get the new \"reftable\"\nbackend upstream.\n\nThe series depends on ps/refstorage-extension as it makes use of the new\nDEFAULT_REPO_FORMAT prereq introduced by that branch.\n\nPatrick\n\nPatrick Steinhardt (6):\n t1300: mark tests to require default repo format\n t1301: mark test for `core.sharedRepository` as reffiles specific\n t1302: make tests more robust with new extensions\n t1419: mark test suite as files-backend specific\n t5526: break test submodule differently\n t: mark tests regarding git-pack-refs(1) to be backend specific\n\n t/t1300-config.sh | 8 ++++----\n t/t1301-shared-repo.sh | 2 +-\n t/t1302-repo-version.sh | 19 +++++++++++++++----\n t/t1409-avoid-packing-refs.sh | 6 ++++++\n t/t1419-exclude-refs.sh | 6 ++++++\n t/t3210-pack-refs.sh | 6 ++++++\n t/t5526-fetch-submodules.sh | 2 +-\n 7 files changed, 39 insertions(+), 10 deletions(-)\n\n-- \n2.43.GIT\n\n"},{"id":"486427","messageId":"ec1b5bdd176e6a3f093b76b732fd9e960a7880ca.1704802213.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1704802213.git.ps@pks.im","subject":"[PATCH 1/6] t1300: mark tests to require default repo format","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-09T12:17:04Z","receivedAt":"2024-01-09T12:17:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The t1300 test suite exercises the git-config(1) tool. To do so we\noverwrite \".git/config\" to contain custom contents. While this is easy\nenough to do, it may create problems when using a non-default repository\nformat because we also overwrite the repository format version as well\nas any potential extensions.\n\nMark these tests with the DEFAULT_REPO_FORMAT prerequisite to avoid the\nproblem. An alternative would be to carry over mandatory config keys\ninto the rewritten config file. But the effort does not seem worth it\ngiven that the system under test is git-config(1), which is at a lower\nlevel than the repository format.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1300-config.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex f4e2752134..1e953a0fc2 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1098,7 +1098,7 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '\n \ttest_must_fail git config --file=linktolinktonada --list\n '\n \n-test_expect_success 'check split_cmdline return' \"\n+test_expect_success DEFAULT_REPO_FORMAT 'check split_cmdline return' \"\n \tgit config alias.split-cmdline-fix 'echo \\\"' &&\n \ttest_must_fail git split-cmdline-fix &&\n \techo foo > foo &&\n@@ -1156,7 +1156,7 @@ test_expect_success 'git -c works with aliases of builtins' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'aliases can be CamelCased' '\n+test_expect_success DEFAULT_REPO_FORMAT 'aliases can be CamelCased' '\n \ttest_config alias.CamelCased \"rev-parse HEAD\" &&\n \tgit CamelCased >out &&\n \tgit rev-parse HEAD >expect &&\n@@ -2051,7 +2051,7 @@ test_expect_success '--show-origin stdin with file include' '\n \ttest_cmp expect output\n '\n \n-test_expect_success '--show-origin blob' '\n+test_expect_success DEFAULT_REPO_FORMAT '--show-origin blob' '\n \tblob=$(git hash-object -w \"$CUSTOM_CONFIG_FILE\") &&\n \tcat >expect <<-EOF &&\n \tblob:$blob\tuser.custom=true\n@@ -2060,7 +2060,7 @@ test_expect_success '--show-origin blob' '\n \ttest_cmp expect output\n '\n \n-test_expect_success '--show-origin blob ref' '\n+test_expect_success DEFAULT_REPO_FORMAT '--show-origin blob ref' '\n \tcat >expect <<-\\EOF &&\n \tblob:main:custom.conf\tuser.custom=true\n \tEOF\n-- \n2.43.GIT\n\n"},{"id":"486428","messageId":"68e308c200ca5a8b0add1a2cf855d78febd2bf36.1704802213.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1704802213.git.ps@pks.im","subject":"[PATCH 2/6] t1301: mark test for `core.sharedRepository` as reffiles specific","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-09T12:17:08Z","receivedAt":"2024-01-09T12:17:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In t1301 we verify whether reflog files written by the \"files\" ref\nbackend correctly honor permissions when \"core.sharedRepository\" is set.\nThe test logic is thus specific to the reffiles backend and will not\nwork with any other backends.\n\nMark the test accordingly with the REFFILES prereq.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1301-shared-repo.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex e5a0d65caa..8e2c01e760 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -137,7 +137,7 @@ test_expect_success POSIXPERM 'info/refs respects umask in unshared repo' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '\n+test_expect_success REFFILES,POSIXPERM 'git reflog expire honors core.sharedRepository' '\n \tumask 077 &&\n \tgit config core.sharedRepository group &&\n \tgit reflog expire --all &&\n-- \n2.43.GIT\n\n"},{"id":"486429","messageId":"9af1e418d47730f503dabb271d30c848bf74fa0b.1704802213.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1704802213.git.ps@pks.im","subject":"[PATCH 3/6] t1302: make tests more robust with new extensions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-09T12:17:12Z","receivedAt":"2024-01-09T12:17:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In t1302 we exercise logic around \"core.repositoryFormatVersion\" and\nextensions. These tests are not particularly robust against extensions\nlike the newly introduced \"refStorage\" extension.\n\nRefactor the tests to be more robust:\n\n - Check the DEFAULT_REPO_FORMAT prereq to determine the expected\n repository format version. This helps to ensure that we only need to\n update the prereq in a central place when new extensions are added.\n\n - Use a separate repository to rewrite \".git/config\" to test\n combinations of the repository format version and extensions. This\n ensures that we don't break the main test repository.\n\n - Do not rewrite \".git/config\" when exercising the \"preciousObjects\"\n extension.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1302-repo-version.sh | 19 +++++++++++++++----\n 1 file changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh\nindex 179474fa65..fb30c87e1b 100755\n--- a/t/t1302-repo-version.sh\n+++ b/t/t1302-repo-version.sh\n@@ -28,7 +28,12 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'gitdir selection on normal repos' '\n-\ttest_oid version >expect &&\n+\tif test_have_prereq DEFAULT_REPO_FORMAT\n+\tthen\n+\t\techo 0\n+\telse\n+\t\techo 1\n+\tfi >expect &&\n \tgit config core.repositoryformatversion >actual &&\n \tgit -C test config core.repositoryformatversion >actual2 &&\n \ttest_cmp expect actual &&\n@@ -79,8 +84,13 @@ mkconfig () {\n \n while read outcome version extensions; do\n \ttest_expect_success \"$outcome version=$version $extensions\" \"\n-\t\tmkconfig $version $extensions >.git/config &&\n-\t\tcheck_${outcome}\n+\t\ttest_when_finished 'rm -rf extensions' &&\n+\t\tgit init extensions &&\n+\t\t(\n+\t\t\tcd extensions &&\n+\t\t\tmkconfig $version $extensions >.git/config &&\n+\t\t\tcheck_${outcome}\n+\t\t)\n \t\"\n done <<\\EOF\n allow 0\n@@ -94,7 +104,8 @@ allow 1 noop-v1\n EOF\n \n test_expect_success 'precious-objects allowed' '\n-\tmkconfig 1 preciousObjects >.git/config &&\n+\tgit config core.repositoryformatversion 1 &&\n+\tgit config extensions.preciousObjects 1 &&\n \tcheck_allow\n '\n \n-- \n2.43.GIT\n\n"},{"id":"486430","messageId":"d7c6b8b2a7b3b4d776f06ce577bdbdbaff66f225.1704802213.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1704802213.git.ps@pks.im","subject":"[PATCH 4/6] t1419: mark test suite as files-backend specific","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-09T12:17:16Z","receivedAt":"2024-01-09T12:17:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"With 59c35fac54 (refs/packed-backend.c: implement jump lists to avoid\nexcluded pattern(s), 2023-07-10) we have implemented logic to handle\nexcluded refs more efficiently in the \"packed\" ref backend. This logic\nallows us to skip emitting refs completely which we know to not be of\nany interest to the caller, which can avoid quite some allocaitons and\nobject lookups.\n\nThis was wired up via a new `exclude_patterns` parameter passed to the\nbackend's ref iterator. The backend only needs to handle them on a best\neffort basis though, and in fact we only handle it for the \"packed-refs\"\nfile, but not for loose references. Consequentially, all callers must\nstill filter emitted refs with those exclude patterns.\n\nThe result is that handling exclude patterns is completely optional in\nthe ref backend, and any future backends may or may not implement it.\nLet's thus mark the test for t1419 to depend on the REFFILES prereq.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1419-exclude-refs.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t1419-exclude-refs.sh b/t/t1419-exclude-refs.sh\nindex 5d8c86b657..1359574419 100755\n--- a/t/t1419-exclude-refs.sh\n+++ b/t/t1419-exclude-refs.sh\n@@ -8,6 +8,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n+if test_have_prereq !REFFILES\n+then\n+\tskip_all='skipping `git for-each-ref --exclude` tests; need files backend'\n+\ttest_done\n+fi\n+\n for_each_ref__exclude () {\n \tGIT_TRACE2_PERF=1 test-tool ref-store main \\\n \t\tfor-each-ref--exclude \"$@\" >actual.raw\n-- \n2.43.GIT\n\n"},{"id":"486431","messageId":"51e494a50e4416ed0cbfd3c474ffcaf8b72e6ef4.1704802213.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1704802213.git.ps@pks.im","subject":"[PATCH 5/6] t5526: break test submodule differently","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-09T12:17:21Z","receivedAt":"2024-01-09T12:17:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 10f5c52656 (submodule: avoid auto-discovery in\nprepare_submodule_repo_env(), 2016-09-01) we fixed a bug when doing a\nrecursive fetch with submodule in the case where the submodule is broken\ndue to whatever reason. The test to exercise that the fix works breaks\nthe submodule by deleting its `HEAD` reference, which will cause us to\nnot detect the directory as a Git repository.\n\nWhile this is perfectly fine in theory, this way of breaking the repo\nbecomes problematic with the current efforts to introduce another refdb\nbackend into Git. The new reftable backend has a stub HEAD file that\nalways contains \"ref: refs/heads/.invalid\" so that tools continue to be\nable to detect such a repository. But as the reftable backend will never\ndelete this file even when asked to delete `HEAD` the current way to\ndelete the `HEAD` reference will stop working.\n\nAdapt the code to instead delete the objects database. Going back with\nthis new way to cuase breakage confirms that it triggers the infinite\nrecursion just the same, and there are no equivalent ongoing efforts to\nreplace the object database with an alternate backend.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5526-fetch-submodules.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 7ab220fa31..5e566205ba 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '\n \tgit -C dst fetch --recurse-submodules &&\n \n \t# Break the receiving submodule\n-\ttest-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&\n+\trm -r dst/sub/.git/objects &&\n \n \t# NOTE: without the fix the following tests will recurse forever!\n \t# They should terminate with an error.\n-- \n2.43.GIT\n\n"},{"id":"486432","messageId":"a9620f329d22c772b96737a5571d85a274dbf6f2.1704802213.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1704802213.git.ps@pks.im","subject":"[PATCH 6/6] t: mark tests regarding git-pack-refs(1) to be backend specific","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-09T12:17:25Z","receivedAt":"2024-01-09T12:17:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Both t1409 and t3210 exercise parts of git-pack-refs(1). Given that we\nmust check the on-disk files to verify whether the backend has indeed\npacked refs as expected those test suites are deeply tied to the actual\nbackend that is in use.\n\nMark the test suites to depend on the REFFILES backend.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1409-avoid-packing-refs.sh | 6 ++++++\n t/t3210-pack-refs.sh | 6 ++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh\nindex f23c0152a8..7748973733 100755\n--- a/t/t1409-avoid-packing-refs.sh\n+++ b/t/t1409-avoid-packing-refs.sh\n@@ -5,6 +5,12 @@ test_description='avoid rewriting packed-refs unnecessarily'\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n+if test_have_prereq !REFFILES\n+then\n+ skip_all='skipping files-backend specific pack-refs tests'\n+ test_done\n+fi\n+\n # Add an identifying mark to the packed-refs file header line. This\n # shouldn't upset readers, and it should be omitted if the file is\n # ever rewritten.\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex 7f4e98db7d..c0f1f9cfb7 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -15,6 +15,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n+if test_have_prereq !REFFILES\n+then\n+ skip_all='skipping files-backend specific pack-refs tests'\n+ test_done\n+fi\n+\n test_expect_success 'enable reflogs' '\n \tgit config core.logallrefupdates true\n '\n-- \n2.43.GIT\n\n"},{"id":"486461","messageId":"ZZ2TdYlcrLN9zckR@nand.local","threadId":"60704","inReplyTo":"ec1b5bdd176e6a3f093b76b732fd9e960a7880ca.1704802213.git.ps@pks.im","subject":"Re: [PATCH 1/6] t1300: mark tests to require default repo format","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-09T18:41:57Z","receivedAt":"2024-01-09T18:41:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 09, 2024 at 01:17:04PM +0100, Patrick Steinhardt wrote:\n> The t1300 test suite exercises the git-config(1) tool. To do so we\n> overwrite \".git/config\" to contain custom contents. While this is easy\n> enough to do, it may create problems when using a non-default repository\n> format because we also overwrite the repository format version as well\n> as any potential extensions.\n>\n> Mark these tests with the DEFAULT_REPO_FORMAT prerequisite to avoid the\n> problem. An alternative would be to carry over mandatory config keys\n> into the rewritten config file. But the effort does not seem worth it\n> given that the system under test is git-config(1), which is at a lower\n> level than the repository format.\n\nI think I am missing something obvious here ;-).\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> t/t1300-config.sh | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> index f4e2752134..1e953a0fc2 100755\n> --- a/t/t1300-config.sh\n> +++ b/t/t1300-config.sh\n> @@ -1098,7 +1098,7 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '\n> \ttest_must_fail git config --file=linktolinktonada --list\n> '\n>\n> -test_expect_success 'check split_cmdline return' \"\n> +test_expect_success DEFAULT_REPO_FORMAT 'check split_cmdline return' \"\n> \tgit config alias.split-cmdline-fix 'echo \\\"' &&\n> \ttest_must_fail git split-cmdline-fix &&\n> \techo foo > foo &&\n> @@ -1156,7 +1156,7 @@ test_expect_success 'git -c works with aliases of builtins' '\n> \ttest_cmp expect actual\n> '\n\nLooking at this first test, for example, I see two places where we\nmodify the configuration file:\n\n - git config alias.split-cmdline-fix 'echo \\\"'\n - git config branch.main.mergeoptions 'echo \\\"'\n\nI think I am missing some detail about why we can't do this when we have\nextensions enabled?\n\nThanks,\nTaylor\n"},{"id":"486462","messageId":"ZZ2T3G+igA6zAIKO@nand.local","threadId":"60704","inReplyTo":"9af1e418d47730f503dabb271d30c848bf74fa0b.1704802213.git.ps@pks.im","subject":"Re: [PATCH 3/6] t1302: make tests more robust with new extensions","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-01-09T18:43:40Z","receivedAt":"2024-01-09T18:43:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jan 09, 2024 at 01:17:12PM +0100, Patrick Steinhardt wrote:\n> In t1302 we exercise logic around \"core.repositoryFormatVersion\" and\n> extensions. These tests are not particularly robust against extensions\n> like the newly introduced \"refStorage\" extension.\n>\n> Refactor the tests to be more robust:\n>\n> - Check the DEFAULT_REPO_FORMAT prereq to determine the expected\n> repository format version. This helps to ensure that we only need to\n> update the prereq in a central place when new extensions are added.\n>\n> - Use a separate repository to rewrite \".git/config\" to test\n> combinations of the repository format version and extensions. This\n> ensures that we don't break the main test repository.\n>\n> - Do not rewrite \".git/config\" when exercising the \"preciousObjects\"\n> extension.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> t/t1302-repo-version.sh | 19 +++++++++++++++----\n> 1 file changed, 15 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh\n> index 179474fa65..fb30c87e1b 100755\n> --- a/t/t1302-repo-version.sh\n> +++ b/t/t1302-repo-version.sh\n> @@ -28,7 +28,12 @@ test_expect_success 'setup' '\n> '\n>\n> test_expect_success 'gitdir selection on normal repos' '\n> -\ttest_oid version >expect &&\n> +\tif test_have_prereq DEFAULT_REPO_FORMAT\n> +\tthen\n> +\t\techo 0\n> +\telse\n> +\t\techo 1\n> +\tfi >expect &&\n> \tgit config core.repositoryformatversion >actual &&\n> \tgit -C test config core.repositoryformatversion >actual2 &&\n> \ttest_cmp expect actual &&\n> @@ -79,8 +84,13 @@ mkconfig () {\n>\n> while read outcome version extensions; do\n> \ttest_expect_success \"$outcome version=$version $extensions\" \"\n> -\t\tmkconfig $version $extensions >.git/config &&\n> -\t\tcheck_${outcome}\n> +\t\ttest_when_finished 'rm -rf extensions' &&\n> +\t\tgit init extensions &&\n> +\t\t(\n> +\t\t\tcd extensions &&\n> +\t\t\tmkconfig $version $extensions >.git/config &&\n> +\t\t\tcheck_${outcome}\n> +\t\t)\n> \t\"\n> done <<\\EOF\n> allow 0\n> @@ -94,7 +104,8 @@ allow 1 noop-v1\n> EOF\n>\n> test_expect_success 'precious-objects allowed' '\n> -\tmkconfig 1 preciousObjects >.git/config &&\n> +\tgit config core.repositoryformatversion 1 &&\n\nI'm nit-picking, but it looks like core.repositoryformatversion is all\nlower-case, whereas extensions.preciousObjects is camel-case. I don't\nthink it's a big deal either way, but I couldn't *not* mention it while\nreading ;-)\n\n> +\tgit config extensions.preciousObjects 1 &&\n> \tcheck_allow\n> '\n>\n> --\n> 2.43.GIT\n\nThanks,\nTaylor\n"},{"id":"486463","messageId":"CAPig+cTB5OH1hCD-EagxNAcaw1=RR7yCeeZ_AzeqHtFTGxT-0g@mail.gmail.com","threadId":"60704","inReplyTo":"51e494a50e4416ed0cbfd3c474ffcaf8b72e6ef4.1704802213.git.ps@pks.im","subject":"Re: [PATCH 5/6] t5526: break test submodule differently","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-01-09T19:23:55Z","receivedAt":"2024-01-09T19:24:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt wrote:\n> In 10f5c52656 (submodule: avoid auto-discovery in\n> prepare_submodule_repo_env(), 2016-09-01) we fixed a bug when doing a\n> recursive fetch with submodule in the case where the submodule is broken\n> due to whatever reason. The test to exercise that the fix works breaks\n> the submodule by deleting its `HEAD` reference, which will cause us to\n> not detect the directory as a Git repository.\n>\n> While this is perfectly fine in theory, this way of breaking the repo\n> becomes problematic with the current efforts to introduce another refdb\n> backend into Git. The new reftable backend has a stub HEAD file that\n> always contains \"ref: refs/heads/.invalid\" so that tools continue to be\n> able to detect such a repository. But as the reftable backend will never\n> delete this file even when asked to delete `HEAD` the current way to\n> delete the `HEAD` reference will stop working.\n\nThis patch is not the appropriate place to bikeshed (but since this is\nthe first time I've read or paid attention to it), if I saw \"ref:\nrefs/heads/.invalid\", the word \".invalid\" would make me think\nsomething was broken in my repository. Would it make sense to use some\nless alarming word, such as perhaps \".placeholder\", \".stand-in\",\n\".synthesized\" or even the name of the non-file-based backend in use?\n\n> Adapt the code to instead delete the objects database. Going back with\n> this new way to cuase breakage confirms that it triggers the infinite\n> recursion just the same, and there are no equivalent ongoing efforts to\n> replace the object database with an alternate backend.\n\ns/cuase/cause/\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> @@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '\n> # Break the receiving submodule\n> - test-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&\n> + rm -r dst/sub/.git/objects &&\n\nIf there is no guarantee that .git/objects will exist when any\nparticular backend is in use, would it be more robust to use -f here,\nas well?\n\n rm -rf dst/sub/.git/objects &&\n"},{"id":"486465","messageId":"CAPig+cRdDSMACzB6mEfwbijLHHSJuQ_Tk8ggNkvFxEd1aSqw2A@mail.gmail.com","threadId":"60704","inReplyTo":"ec1b5bdd176e6a3f093b76b732fd9e960a7880ca.1704802213.git.ps@pks.im","subject":"Re: [PATCH 1/6] t1300: mark tests to require default repo format","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-01-09T19:35:29Z","receivedAt":"2024-01-09T19:35:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt wrote:\n> The t1300 test suite exercises the git-config(1) tool. To do so we\n> overwrite \".git/config\" to contain custom contents. While this is easy\n> enough to do, it may create problems when using a non-default repository\n> format because we also overwrite the repository format version as well\n> as any potential extensions.\n>\n> Mark these tests with the DEFAULT_REPO_FORMAT prerequisite to avoid the\n> problem. An alternative would be to carry over mandatory config keys\n> into the rewritten config file. But the effort does not seem worth it\n> given that the system under test is git-config(1), which is at a lower\n> level than the repository format.\n\nIf I'm understanding correctly, with the approach taken by this patch,\nwon't we undesirably lose some git-config test coverage if the\nfile-based backend is ever retired, or if tests specific to it are\never disabled by default? As such, it seems like the alternative \"fix\"\nyou mention above would be preferable to ensure that coverage of\ngit-config doesn't get diluted.\n\nOr am I misunderstanding something?\n"},{"id":"486466","messageId":"CAPig+cTAiEFu9p1nRe9LC3mxyPmfQ9m4r7aQUj_9OC8pSbwbig@mail.gmail.com","threadId":"60704","inReplyTo":"d7c6b8b2a7b3b4d776f06ce577bdbdbaff66f225.1704802213.git.ps@pks.im","subject":"Re: [PATCH 4/6] t1419: mark test suite as files-backend specific","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-01-09T19:40:50Z","receivedAt":"2024-01-09T19:41:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt wrote:\n> With 59c35fac54 (refs/packed-backend.c: implement jump lists to avoid\n> excluded pattern(s), 2023-07-10) we have implemented logic to handle\n> excluded refs more efficiently in the \"packed\" ref backend. This logic\n> allows us to skip emitting refs completely which we know to not be of\n> any interest to the caller, which can avoid quite some allocaitons and\n> object lookups.\n\ns/allocaitons/allocations/\n\n> This was wired up via a new `exclude_patterns` parameter passed to the\n> backend's ref iterator. The backend only needs to handle them on a best\n> effort basis though, and in fact we only handle it for the \"packed-refs\"\n> file, but not for loose references. Consequentially, all callers must\n> still filter emitted refs with those exclude patterns.\n\ns/Consequentially/Consequently/\n\n> The result is that handling exclude patterns is completely optional in\n> the ref backend, and any future backends may or may not implement it.\n> Let's thus mark the test for t1419 to depend on the REFFILES prereq.\n\nThis change seems to be abusing the meaning of the REFFILES\nprerequisite. Instead the above description argues for introduction of\na new prerequisite which indicates whether or not the backend honors\nthe exclude patterns. Or, am I misunderstanding this?\n"},{"id":"486501","messageId":"ZZ5EBojTbQ4pyGx-@tanuki","threadId":"60704","inReplyTo":"ZZ2TdYlcrLN9zckR@nand.local","subject":"Re: [PATCH 1/6] t1300: mark tests to require default repo format","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-10T07:15:18Z","receivedAt":"2024-01-10T07:15:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 09, 2024 at 01:41:57PM -0500, Taylor Blau wrote:\n> On Tue, Jan 09, 2024 at 01:17:04PM +0100, Patrick Steinhardt wrote:\n> > The t1300 test suite exercises the git-config(1) tool. To do so we\n> > overwrite \".git/config\" to contain custom contents. While this is easy\n> > enough to do, it may create problems when using a non-default repository\n> > format because we also overwrite the repository format version as well\n> > as any potential extensions.\n> >\n> > Mark these tests with the DEFAULT_REPO_FORMAT prerequisite to avoid the\n> > problem. An alternative would be to carry over mandatory config keys\n> > into the rewritten config file. But the effort does not seem worth it\n> > given that the system under test is git-config(1), which is at a lower\n> > level than the repository format.\n> \n> I think I am missing something obvious here ;-).\n> \n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > t/t1300-config.sh | 8 ++++----\n> > 1 file changed, 4 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> > index f4e2752134..1e953a0fc2 100755\n> > --- a/t/t1300-config.sh\n> > +++ b/t/t1300-config.sh\n> > @@ -1098,7 +1098,7 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '\n> > \ttest_must_fail git config --file=linktolinktonada --list\n> > '\n> >\n> > -test_expect_success 'check split_cmdline return' \"\n> > +test_expect_success DEFAULT_REPO_FORMAT 'check split_cmdline return' \"\n> > \tgit config alias.split-cmdline-fix 'echo \\\"' &&\n> > \ttest_must_fail git split-cmdline-fix &&\n> > \techo foo > foo &&\n> > @@ -1156,7 +1156,7 @@ test_expect_success 'git -c works with aliases of builtins' '\n> > \ttest_cmp expect actual\n> > '\n> \n> Looking at this first test, for example, I see two places where we\n> modify the configuration file:\n> \n> - git config alias.split-cmdline-fix 'echo \\\"'\n> - git config branch.main.mergeoptions 'echo \\\"'\n> \n> I think I am missing some detail about why we can't do this when we have\n> extensions enabled?\n\nThe issue is not directly visible in the tests I'm amending here, but\nhappens in the setup code. What we do is to overwrite the repository's\nconfig like this:\n\n```\ncat > .git/config << EOF\n[beta] ; silly comment # another comment\nnoIndent= sillyValue ; 'nother silly comment\n\n# empty line\n\t\t; comment\n\t\thaha =\"beta\" # last silly comment\nhaha = hello\n\thaha = bello\n[nextSection] noNewline = ouch\nEOF\n```\n\nThe problem here is that we drop any extensions that the repository has\nbeen initialized with originally. This seems to work alright in the\ncontext of SHA256 repositories. But with the reftable backend this\npattern will cause test failures because the discarded \"refStorage\"\nextension will make us assume that the repostiory uses the \"files\"\nbackend instead of the \"reftable\" backend. And that starts to go\ndownhill quite fast when trying to read or write refs.\n\nA \"proper\" fix for this issue would be to rewrite tests such that we\nknow to retain those extensions. But I'm just not sure whether that is\nreally worth it, mostly because the system under test is at a lower\nlevel and thus shouldn't care about repository extensions. After all,\nextensions build on top of our config code.\n\nPatrick\n"},{"id":"486502","messageId":"ZZ5EnxtMg0wLyAm9@tanuki","threadId":"60704","inReplyTo":"CAPig+cRdDSMACzB6mEfwbijLHHSJuQ_Tk8ggNkvFxEd1aSqw2A@mail.gmail.com","subject":"Re: [PATCH 1/6] t1300: mark tests to require default repo format","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-10T07:17:51Z","receivedAt":"2024-01-10T07:17:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 09, 2024 at 02:35:29PM -0500, Eric Sunshine wrote:\n> On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt wrote:\n> > The t1300 test suite exercises the git-config(1) tool. To do so we\n> > overwrite \".git/config\" to contain custom contents. While this is easy\n> > enough to do, it may create problems when using a non-default repository\n> > format because we also overwrite the repository format version as well\n> > as any potential extensions.\n> >\n> > Mark these tests with the DEFAULT_REPO_FORMAT prerequisite to avoid the\n> > problem. An alternative would be to carry over mandatory config keys\n> > into the rewritten config file. But the effort does not seem worth it\n> > given that the system under test is git-config(1), which is at a lower\n> > level than the repository format.\n> \n> If I'm understanding correctly, with the approach taken by this patch,\n> won't we undesirably lose some git-config test coverage if the\n> file-based backend is ever retired, or if tests specific to it are\n> ever disabled by default? As such, it seems like the alternative \"fix\"\n> you mention above would be preferable to ensure that coverage of\n> git-config doesn't get diluted.\n> \n> Or am I misunderstanding something?\n\nA valid remark indeed, even though this is thinking quite far into the\nfuture. I'll investigate how much of a pain it would be to instead \"do\nthe right thing\" and retain the repositroy format version as well as\nextensions.\n\nPatrick\n"},{"id":"486503","messageId":"ZZ5HrY76O1x8QABG@tanuki","threadId":"60704","inReplyTo":"CAPig+cTAiEFu9p1nRe9LC3mxyPmfQ9m4r7aQUj_9OC8pSbwbig@mail.gmail.com","subject":"Re: [PATCH 4/6] t1419: mark test suite as files-backend specific","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-10T07:30:53Z","receivedAt":"2024-01-10T07:30:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 09, 2024 at 02:40:50PM -0500, Eric Sunshine wrote:\n> On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt wrote:\n> > With 59c35fac54 (refs/packed-backend.c: implement jump lists to avoid\n> > excluded pattern(s), 2023-07-10) we have implemented logic to handle\n> > excluded refs more efficiently in the \"packed\" ref backend. This logic\n> > allows us to skip emitting refs completely which we know to not be of\n> > any interest to the caller, which can avoid quite some allocaitons and\n> > object lookups.\n> \n> s/allocaitons/allocations/\n> \n> > This was wired up via a new `exclude_patterns` parameter passed to the\n> > backend's ref iterator. The backend only needs to handle them on a best\n> > effort basis though, and in fact we only handle it for the \"packed-refs\"\n> > file, but not for loose references. Consequentially, all callers must\n> > still filter emitted refs with those exclude patterns.\n> \n> s/Consequentially/Consequently/\n\nHum. I had the last time when you mentioned the in mind while writing\nthe commit message, but seemingly misremembered the outcome. So I now\nlooked things up in a dictionary, and both words seem to be used in\nequivalent ways. As a non-native speaker, could you maybe elaborate a\nbit to help me out? :)\n\n> > The result is that handling exclude patterns is completely optional in\n> > the ref backend, and any future backends may or may not implement it.\n> > Let's thus mark the test for t1419 to depend on the REFFILES prereq.\n> \n> This change seems to be abusing the meaning of the REFFILES\n> prerequisite. Instead the above description argues for introduction of\n> a new prerequisite which indicates whether or not the backend honors\n> the exclude patterns. Or, am I misunderstanding this?\n\nI wouldn't say that this is abuse. We know the logic is only implemented\nby certain backends, and for the time being the only backend that does\nis the \"files\" backend. Furthermore, no test outside of t1419 ever cares\nfor whether the backend knows to handle exclude patterns, so introducing\na separate prereq that simply maps to REFFILES doesn't really feel worth\nit. If we ever implement this behaviour in the \"reftable\" backend, then\nwe can easily extend the prereq like follows:\n\n```\nif ! test_have_prereq REFFILES && ! test_have_prereq REFTABLE\nthen\n skip_all='skipping `git for-each-ref --exclude` tests; need files backend'\n test_done\nfi\n```\n\nNow we could of course make the prereq clever and auto-detect whether\nthe ref backend supports excludes. But this has the downside that it\ncould lead to silent failures in case the exclude pattern handling ever\nbreaks because now the prereq would potentially evaluate to \"false\".\n\nPatrick\n"},{"id":"486504","messageId":"ZZ5KJX4tC0UIX00j@tanuki","threadId":"60704","inReplyTo":"CAPig+cTB5OH1hCD-EagxNAcaw1=RR7yCeeZ_AzeqHtFTGxT-0g@mail.gmail.com","subject":"Re: [PATCH 5/6] t5526: break test submodule differently","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-10T07:41:25Z","receivedAt":"2024-01-10T07:41:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 09, 2024 at 02:23:55PM -0500, Eric Sunshine wrote:\n> On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt wrote:\n> > In 10f5c52656 (submodule: avoid auto-discovery in\n> > prepare_submodule_repo_env(), 2016-09-01) we fixed a bug when doing a\n> > recursive fetch with submodule in the case where the submodule is broken\n> > due to whatever reason. The test to exercise that the fix works breaks\n> > the submodule by deleting its `HEAD` reference, which will cause us to\n> > not detect the directory as a Git repository.\n> >\n> > While this is perfectly fine in theory, this way of breaking the repo\n> > becomes problematic with the current efforts to introduce another refdb\n> > backend into Git. The new reftable backend has a stub HEAD file that\n> > always contains \"ref: refs/heads/.invalid\" so that tools continue to be\n> > able to detect such a repository. But as the reftable backend will never\n> > delete this file even when asked to delete `HEAD` the current way to\n> > delete the `HEAD` reference will stop working.\n> \n> This patch is not the appropriate place to bikeshed (but since this is\n> the first time I've read or paid attention to it), if I saw \"ref:\n> refs/heads/.invalid\", the word \".invalid\" would make me think\n> something was broken in my repository. Would it make sense to use some\n> less alarming word, such as perhaps \".placeholder\", \".stand-in\",\n> \".synthesized\" or even the name of the non-file-based backend in use?\n\nWell, something _is_ broken in your repository in case Git ever tries to\nread the \"HEAD\" placeholder in a reftable-enabled repository. But I\nguess you rather come from the angle of using `cat HEAD` as a user. I do\nagree that using a better hint could help users, but this detail has\nalready been recorded as such in \"Documentation/technical/reftable.txt\".\n\nWe can of course change this to be \"ref: refs/heads/.reftable\", but as\nyou already say this is something that should be discussed in a separate\nthread.\n\n> > Adapt the code to instead delete the objects database. Going back with\n> > this new way to cuase breakage confirms that it triggers the infinite\n> > recursion just the same, and there are no equivalent ongoing efforts to\n> > replace the object database with an alternate backend.\n> \n> s/cuase/cause/\n> \n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> > @@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '\n> > # Break the receiving submodule\n> > - test-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&\n> > + rm -r dst/sub/.git/objects &&\n> \n> If there is no guarantee that .git/objects will exist when any\n> particular backend is in use, would it be more robust to use -f here,\n> as well?\n> \n> rm -rf dst/sub/.git/objects &&\n\n`.git/objects` always exists in a healthy repository. If it doesn't,\nthen `is_git_directory()` would return a false-ish value and we wouldn't\nrecognize the repository as such. Or are you saying that this could\npotentially change in the future if there was ever to be an alternate\nODB format? If so that is a valid remark, but the test would break\nregardless of whether we use `-f` or not: if a missing \".git/objects\"\ndirectory does not lead to a corrupted repository then the whole premise\nof the test is broken.\n\nPatrick\n"},{"id":"486506","messageId":"cover.1704877233.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1704802213.git.ps@pks.im","subject":"[PATCH v2 0/6] t: mark \"files\"-backend specific tests","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-10T09:01:34Z","receivedAt":"2024-01-10T09:01:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series that addresses tests which\nare specific to the \"files\" backend. Changes compared to v1:\n\n - I've rewritten the patch addressing t1300 to not mark tests as\n repository format specific anymoreg. Instead, we now create separate\n repos for relevant tests where we are more careful to not discard\n extensions.\n\n - I've made the casing of config options more consistent.\n\n - I've extended some commit messages to hopefully explain better why\n I'm doing things the way I do them and fixed some typos.\n\nThanks for your feedback!\n\nPatrick\n\nPatrick Steinhardt (6):\n t1300: make tests more robust with non-default ref backends\n t1301: mark test for `core.sharedRepository` as reffiles specific\n t1302: make tests more robust with new extensions\n t1419: mark test suite as files-backend specific\n t5526: break test submodule differently\n t: mark tests regarding git-pack-refs(1) to be backend specific\n\n t/t1300-config.sh | 74 +++++++++++++++++++++++------------\n t/t1301-shared-repo.sh | 2 +-\n t/t1302-repo-version.sh | 19 +++++++--\n t/t1409-avoid-packing-refs.sh | 6 +++\n t/t1419-exclude-refs.sh | 6 +++\n t/t3210-pack-refs.sh | 6 +++\n t/t5526-fetch-submodules.sh | 2 +-\n 7 files changed, 83 insertions(+), 32 deletions(-)\n\nRange-diff against v1:\n1: ec1b5bdd17 < -: ---------- t1300: mark tests to require default repo format\n-: ---------- > 1: 0552123fa3 t1300: make tests more robust with non-default ref backends\n2: 68e308c200 = 2: 384250fec2 t1301: mark test for `core.sharedRepository` as reffiles specific\n3: 9af1e418d4 ! 3: 1284b70fab t1302: make tests more robust with new extensions\n @@ t/t1302-repo-version.sh: allow 1 noop-v1\n \n test_expect_success 'precious-objects allowed' '\n -\tmkconfig 1 preciousObjects >.git/config &&\n -+\tgit config core.repositoryformatversion 1 &&\n ++\tgit config core.repositoryFormatVersion 1 &&\n +\tgit config extensions.preciousObjects 1 &&\n \tcheck_allow\n '\n4: d7c6b8b2a7 ! 4: c6062b612c t1419: mark test suite as files-backend specific\n @@ Commit message\n excluded pattern(s), 2023-07-10) we have implemented logic to handle\n excluded refs more efficiently in the \"packed\" ref backend. This logic\n allows us to skip emitting refs completely which we know to not be of\n - any interest to the caller, which can avoid quite some allocaitons and\n + any interest to the caller, which can avoid quite some allocations and\n object lookups.\n \n This was wired up via a new `exclude_patterns` parameter passed to the\n backend's ref iterator. The backend only needs to handle them on a best\n effort basis though, and in fact we only handle it for the \"packed-refs\"\n - file, but not for loose references. Consequentially, all callers must\n - still filter emitted refs with those exclude patterns.\n + file, but not for loose references. Consequently, all callers must still\n + filter emitted refs with those exclude patterns.\n \n The result is that handling exclude patterns is completely optional in\n the ref backend, and any future backends may or may not implement it.\n Let's thus mark the test for t1419 to depend on the REFFILES prereq.\n \n + An alternative would be to introduce a new prereq that tells us whether\n + the backend under test supports exclude patterns or not. But this does\n + feel a bit overblown:\n +\n + - It would either map to the REFFILES prereq, in which case it feels\n + overengineered because the prereq is only ever relevant to t1419.\n +\n + - Otherwise, it could auto-detect whether the backend supports exclude\n + patterns. But this could lead to silent failures in case the support\n + for this feature breaks at any point in time.\n +\n + It should thus be good enough to just use the REFFILES prereq for now.\n + If future backends ever grow support for exclude patterns we can easily\n + add their respective prereq as another condition for this test suite to\n + execute.\n +\n Signed-off-by: Patrick Steinhardt \n \n ## t/t1419-exclude-refs.sh ##\n5: 51e494a50e ! 5: ae08afc459 t5526: break test submodule differently\n @@ Commit message\n delete the `HEAD` reference will stop working.\n \n Adapt the code to instead delete the objects database. Going back with\n - this new way to cuase breakage confirms that it triggers the infinite\n + this new way to cause breakage confirms that it triggers the infinite\n recursion just the same, and there are no equivalent ongoing efforts to\n replace the object database with an alternate backend.\n \n6: a9620f329d = 6: df648be535 t: mark tests regarding git-pack-refs(1) to be backend specific\n-- \n2.43.GIT\n\n"},{"id":"486507","messageId":"0552123fa30243d6d8d6b378991651dd6ade7de3.1704877233.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1704877233.git.ps@pks.im","subject":"[PATCH v2 1/6] t1300: make tests more robust with non-default ref backends","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-10T09:01:39Z","receivedAt":"2024-01-10T09:01:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The t1300 test suite exercises the git-config(1) tool. To do so we\noverwrite \".git/config\" to contain custom contents. While this is easy\nenough to do, it may create problems when using a non-default repository\nformat because we also overwrite the repository format version as well\nas any potential extensions. With the upcoming \"reftable\" ref backend\nthe result is that we may try to access refs via the \"files\" backend\neven though the repository has been initialized with the \"reftable\"\nbackend.\n\nRefactor tests which access the refdb to be more robust by using their\nown separate repositories, which allows us to be more careful and not\ndiscard required extensions.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1300-config.sh | 74 ++++++++++++++++++++++++++++++-----------------\n 1 file changed, 48 insertions(+), 26 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex f4e2752134..53c3d65823 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1099,13 +1099,18 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '\n '\n \n test_expect_success 'check split_cmdline return' \"\n-\tgit config alias.split-cmdline-fix 'echo \\\"' &&\n-\ttest_must_fail git split-cmdline-fix &&\n-\techo foo > foo &&\n-\tgit add foo &&\n-\tgit commit -m 'initial commit' &&\n-\tgit config branch.main.mergeoptions 'echo \\\"' &&\n-\ttest_must_fail git merge main\n+\ttest_when_finished 'rm -rf repo' &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config alias.split-cmdline-fix 'echo \\\"' &&\n+\t\ttest_must_fail git split-cmdline-fix &&\n+\t\techo foo >foo &&\n+\t\tgit add foo &&\n+\t\tgit commit -m 'initial commit' &&\n+\t\tgit config branch.main.mergeoptions 'echo \\\"' &&\n+\t\ttest_must_fail git merge main\n+\t)\n \"\n \n test_expect_success 'git -c \"key=value\" support' '\n@@ -1157,10 +1162,16 @@ test_expect_success 'git -c works with aliases of builtins' '\n '\n \n test_expect_success 'aliases can be CamelCased' '\n-\ttest_config alias.CamelCased \"rev-parse HEAD\" &&\n-\tgit CamelCased >out &&\n-\tgit rev-parse HEAD >expect &&\n-\ttest_cmp expect out\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A &&\n+\t\tgit config alias.CamelCased \"rev-parse HEAD\" &&\n+\t\tgit CamelCased >out &&\n+\t\tgit rev-parse HEAD >expect &&\n+\t\ttest_cmp expect out\n+\t)\n '\n \n test_expect_success 'git -c does not split values on equals' '\n@@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '\n '\n \n test_expect_success 'set up custom config file' '\n-\tCUSTOM_CONFIG_FILE=\"custom.conf\" &&\n-\tcat >\"$CUSTOM_CONFIG_FILE\" <<-\\EOF\n+\tcat >\"custom.conf\" <<-\\EOF &&\n \t[user]\n \t\tcustom = true\n \tEOF\n+\tCUSTOM_CONFIG_FILE=\"$(test-tool path-utils real_path custom.conf)\"\n '\n \n test_expect_success !MINGW 'set up custom config file with special name characters' '\n@@ -2052,22 +2063,33 @@ test_expect_success '--show-origin stdin with file include' '\n '\n \n test_expect_success '--show-origin blob' '\n-\tblob=$(git hash-object -w \"$CUSTOM_CONFIG_FILE\") &&\n-\tcat >expect <<-EOF &&\n-\tblob:$blob\tuser.custom=true\n-\tEOF\n-\tgit config --blob=$blob --show-origin --list >output &&\n-\ttest_cmp expect output\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tblob=$(git hash-object -w \"$CUSTOM_CONFIG_FILE\") &&\n+\t\tcat >expect <<-EOF &&\n+\t\tblob:$blob\tuser.custom=true\n+\t\tEOF\n+\t\tgit config --blob=$blob --show-origin --list >output &&\n+\t\ttest_cmp expect output\n+\t)\n '\n \n test_expect_success '--show-origin blob ref' '\n-\tcat >expect <<-\\EOF &&\n-\tblob:main:custom.conf\tuser.custom=true\n-\tEOF\n-\tgit add \"$CUSTOM_CONFIG_FILE\" &&\n-\tgit commit -m \"new config file\" &&\n-\tgit config --blob=main:\"$CUSTOM_CONFIG_FILE\" --show-origin --list >output &&\n-\ttest_cmp expect output\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tcat >expect <<-\\EOF &&\n+\t\tblob:main:custom.conf\tuser.custom=true\n+\t\tEOF\n+\t\tcp \"$CUSTOM_CONFIG_FILE\" custom.conf &&\n+\t\tgit add custom.conf &&\n+\t\tgit commit -m \"new config file\" &&\n+\t\tgit config --blob=main:custom.conf --show-origin --list >output &&\n+\t\ttest_cmp expect output\n+\t)\n '\n \n test_expect_success '--show-origin with --default' '\n-- \n2.43.GIT\n\n"},{"id":"486508","messageId":"384250fec217a806cc0f9930034f04ba2ee29201.1704877233.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1704877233.git.ps@pks.im","subject":"[PATCH v2 2/6] t1301: mark test for `core.sharedRepository` as reffiles specific","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-10T09:01:43Z","receivedAt":"2024-01-10T09:01:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In t1301 we verify whether reflog files written by the \"files\" ref\nbackend correctly honor permissions when \"core.sharedRepository\" is set.\nThe test logic is thus specific to the reffiles backend and will not\nwork with any other backends.\n\nMark the test accordingly with the REFFILES prereq.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1301-shared-repo.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex e5a0d65caa..8e2c01e760 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -137,7 +137,7 @@ test_expect_success POSIXPERM 'info/refs respects umask in unshared repo' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '\n+test_expect_success REFFILES,POSIXPERM 'git reflog expire honors core.sharedRepository' '\n \tumask 077 &&\n \tgit config core.sharedRepository group &&\n \tgit reflog expire --all &&\n-- \n2.43.GIT\n\n"},{"id":"486509","messageId":"1284b70fab6dd85114cb24fc5c7b6c49e35eb135.1704877233.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1704877233.git.ps@pks.im","subject":"[PATCH v2 3/6] t1302: make tests more robust with new extensions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-10T09:01:47Z","receivedAt":"2024-01-10T09:01:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In t1302 we exercise logic around \"core.repositoryFormatVersion\" and\nextensions. These tests are not particularly robust against extensions\nlike the newly introduced \"refStorage\" extension.\n\nRefactor the tests to be more robust:\n\n - Check the DEFAULT_REPO_FORMAT prereq to determine the expected\n repository format version. This helps to ensure that we only need to\n update the prereq in a central place when new extensions are added.\n\n - Use a separate repository to rewrite \".git/config\" to test\n combinations of the repository format version and extensions. This\n ensures that we don't break the main test repository.\n\n - Do not rewrite \".git/config\" when exercising the \"preciousObjects\"\n extension.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1302-repo-version.sh | 19 +++++++++++++++----\n 1 file changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh\nindex 179474fa65..7c680c91c4 100755\n--- a/t/t1302-repo-version.sh\n+++ b/t/t1302-repo-version.sh\n@@ -28,7 +28,12 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'gitdir selection on normal repos' '\n-\ttest_oid version >expect &&\n+\tif test_have_prereq DEFAULT_REPO_FORMAT\n+\tthen\n+\t\techo 0\n+\telse\n+\t\techo 1\n+\tfi >expect &&\n \tgit config core.repositoryformatversion >actual &&\n \tgit -C test config core.repositoryformatversion >actual2 &&\n \ttest_cmp expect actual &&\n@@ -79,8 +84,13 @@ mkconfig () {\n \n while read outcome version extensions; do\n \ttest_expect_success \"$outcome version=$version $extensions\" \"\n-\t\tmkconfig $version $extensions >.git/config &&\n-\t\tcheck_${outcome}\n+\t\ttest_when_finished 'rm -rf extensions' &&\n+\t\tgit init extensions &&\n+\t\t(\n+\t\t\tcd extensions &&\n+\t\t\tmkconfig $version $extensions >.git/config &&\n+\t\t\tcheck_${outcome}\n+\t\t)\n \t\"\n done <<\\EOF\n allow 0\n@@ -94,7 +104,8 @@ allow 1 noop-v1\n EOF\n \n test_expect_success 'precious-objects allowed' '\n-\tmkconfig 1 preciousObjects >.git/config &&\n+\tgit config core.repositoryFormatVersion 1 &&\n+\tgit config extensions.preciousObjects 1 &&\n \tcheck_allow\n '\n \n-- \n2.43.GIT\n\n"},{"id":"486510","messageId":"c6062b612ce5793a716bfaec8082a5ab992cdd75.1704877233.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1704877233.git.ps@pks.im","subject":"[PATCH v2 4/6] t1419: mark test suite as files-backend specific","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-10T09:01:51Z","receivedAt":"2024-01-10T09:01:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"With 59c35fac54 (refs/packed-backend.c: implement jump lists to avoid\nexcluded pattern(s), 2023-07-10) we have implemented logic to handle\nexcluded refs more efficiently in the \"packed\" ref backend. This logic\nallows us to skip emitting refs completely which we know to not be of\nany interest to the caller, which can avoid quite some allocations and\nobject lookups.\n\nThis was wired up via a new `exclude_patterns` parameter passed to the\nbackend's ref iterator. The backend only needs to handle them on a best\neffort basis though, and in fact we only handle it for the \"packed-refs\"\nfile, but not for loose references. Consequently, all callers must still\nfilter emitted refs with those exclude patterns.\n\nThe result is that handling exclude patterns is completely optional in\nthe ref backend, and any future backends may or may not implement it.\nLet's thus mark the test for t1419 to depend on the REFFILES prereq.\n\nAn alternative would be to introduce a new prereq that tells us whether\nthe backend under test supports exclude patterns or not. But this does\nfeel a bit overblown:\n\n - It would either map to the REFFILES prereq, in which case it feels\n overengineered because the prereq is only ever relevant to t1419.\n\n - Otherwise, it could auto-detect whether the backend supports exclude\n patterns. But this could lead to silent failures in case the support\n for this feature breaks at any point in time.\n\nIt should thus be good enough to just use the REFFILES prereq for now.\nIf future backends ever grow support for exclude patterns we can easily\nadd their respective prereq as another condition for this test suite to\nexecute.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1419-exclude-refs.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t1419-exclude-refs.sh b/t/t1419-exclude-refs.sh\nindex 5d8c86b657..1359574419 100755\n--- a/t/t1419-exclude-refs.sh\n+++ b/t/t1419-exclude-refs.sh\n@@ -8,6 +8,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n+if test_have_prereq !REFFILES\n+then\n+\tskip_all='skipping `git for-each-ref --exclude` tests; need files backend'\n+\ttest_done\n+fi\n+\n for_each_ref__exclude () {\n \tGIT_TRACE2_PERF=1 test-tool ref-store main \\\n \t\tfor-each-ref--exclude \"$@\" >actual.raw\n-- \n2.43.GIT\n\n"},{"id":"486511","messageId":"ae08afc459229aaefdd166b5e55724b2a98c0d1f.1704877233.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1704877233.git.ps@pks.im","subject":"[PATCH v2 5/6] t5526: break test submodule differently","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-10T09:01:55Z","receivedAt":"2024-01-10T09:01:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 10f5c52656 (submodule: avoid auto-discovery in\nprepare_submodule_repo_env(), 2016-09-01) we fixed a bug when doing a\nrecursive fetch with submodule in the case where the submodule is broken\ndue to whatever reason. The test to exercise that the fix works breaks\nthe submodule by deleting its `HEAD` reference, which will cause us to\nnot detect the directory as a Git repository.\n\nWhile this is perfectly fine in theory, this way of breaking the repo\nbecomes problematic with the current efforts to introduce another refdb\nbackend into Git. The new reftable backend has a stub HEAD file that\nalways contains \"ref: refs/heads/.invalid\" so that tools continue to be\nable to detect such a repository. But as the reftable backend will never\ndelete this file even when asked to delete `HEAD` the current way to\ndelete the `HEAD` reference will stop working.\n\nAdapt the code to instead delete the objects database. Going back with\nthis new way to cause breakage confirms that it triggers the infinite\nrecursion just the same, and there are no equivalent ongoing efforts to\nreplace the object database with an alternate backend.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5526-fetch-submodules.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 7ab220fa31..5e566205ba 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '\n \tgit -C dst fetch --recurse-submodules &&\n \n \t# Break the receiving submodule\n-\ttest-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&\n+\trm -r dst/sub/.git/objects &&\n \n \t# NOTE: without the fix the following tests will recurse forever!\n \t# They should terminate with an error.\n-- \n2.43.GIT\n\n"},{"id":"486512","messageId":"df648be535737551ce7b6d6680841c7ac9ec44f3.1704877233.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1704877233.git.ps@pks.im","subject":"[PATCH v2 6/6] t: mark tests regarding git-pack-refs(1) to be backend specific","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-10T09:01:59Z","receivedAt":"2024-01-10T09:02:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Both t1409 and t3210 exercise parts of git-pack-refs(1). Given that we\nmust check the on-disk files to verify whether the backend has indeed\npacked refs as expected those test suites are deeply tied to the actual\nbackend that is in use.\n\nMark the test suites to depend on the REFFILES backend.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1409-avoid-packing-refs.sh | 6 ++++++\n t/t3210-pack-refs.sh | 6 ++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh\nindex f23c0152a8..7748973733 100755\n--- a/t/t1409-avoid-packing-refs.sh\n+++ b/t/t1409-avoid-packing-refs.sh\n@@ -5,6 +5,12 @@ test_description='avoid rewriting packed-refs unnecessarily'\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n+if test_have_prereq !REFFILES\n+then\n+ skip_all='skipping files-backend specific pack-refs tests'\n+ test_done\n+fi\n+\n # Add an identifying mark to the packed-refs file header line. This\n # shouldn't upset readers, and it should be omitted if the file is\n # ever rewritten.\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex 7f4e98db7d..c0f1f9cfb7 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -15,6 +15,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n+if test_have_prereq !REFFILES\n+then\n+ skip_all='skipping files-backend specific pack-refs tests'\n+ test_done\n+fi\n+\n test_expect_success 'enable reflogs' '\n \tgit config core.logallrefupdates true\n '\n-- \n2.43.GIT\n\n"},{"id":"486536","messageId":"xmqqcyu9duha.fsf@gitster.g","threadId":"60704","inReplyTo":"ZZ5HrY76O1x8QABG@tanuki","subject":"Re: [PATCH 4/6] t1419: mark test suite as files-backend specific","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-10T16:27:29Z","receivedAt":"2024-01-10T16:27:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\nOn just this (non-technical) part...\n\n>> > file, but not for loose references. Consequentially, all callers must\n>> > still filter emitted refs with those exclude patterns.\n>> \n>> s/Consequentially/Consequently/\n>\n> Hum. I had the last time when you mentioned the in mind while writing\n> the commit message, but seemingly misremembered the outcome. So I now\n> looked things up in a dictionary, and both words seem to be used in\n> equivalent ways. As a non-native speaker, could you maybe elaborate a\n> bit to help me out? :)\n\nAs a non-native, I often find this\n\n https://trends.google.com/trends/explore?q=consequentially,consequently&hl=en\n\nfairly useful.\n"},{"id":"486593","messageId":"ZZ92_PdDwPTgaJ4f@tanuki","threadId":"60704","inReplyTo":"xmqqcyu9duha.fsf@gitster.g","subject":"Re: [PATCH 4/6] t1419: mark test suite as files-backend specific","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-11T05:05:00Z","receivedAt":"2024-01-11T05:05:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jan 10, 2024 at 08:27:29AM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> On just this (non-technical) part...\n> \n> >> > file, but not for loose references. Consequentially, all callers must\n> >> > still filter emitted refs with those exclude patterns.\n> >> \n> >> s/Consequentially/Consequently/\n> >\n> > Hum. I had the last time when you mentioned the in mind while writing\n> > the commit message, but seemingly misremembered the outcome. So I now\n> > looked things up in a dictionary, and both words seem to be used in\n> > equivalent ways. As a non-native speaker, could you maybe elaborate a\n> > bit to help me out? :)\n> \n> As a non-native, I often find this\n> \n> https://trends.google.com/trends/explore?q=consequentially,consequently&hl=en\n> \n> fairly useful.\n\nGood idea, thanks!\n\nPatrick\n"},{"id":"487253","messageId":"87ede8qhw9.fsf@iotcl.com","threadId":"60704","inReplyTo":"0552123fa30243d6d8d6b378991651dd6ade7de3.1704877233.git.ps@pks.im","subject":"Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2024-01-23T13:41:17Z","receivedAt":"2024-01-23T13:51:52Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"\nPatrick Steinhardt writes:\n\n> [[PGP Signed Part:Undecided]]\n> [1. text/plain]\n> The t1300 test suite exercises the git-config(1) tool. To do so we\n> overwrite \".git/config\" to contain custom contents. While this is easy\n> enough to do, it may create problems when using a non-default repository\n> format because we also overwrite the repository format version as well\n> as any potential extensions. With the upcoming \"reftable\" ref backend\n> the result is that we may try to access refs via the \"files\" backend\n> even though the repository has been initialized with the \"reftable\"\n> backend.\n>\n> Refactor tests which access the refdb to be more robust by using their\n> own separate repositories, which allows us to be more careful and not\n> discard required extensions.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n\n> @@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '\n> '\n>\n> test_expect_success 'set up custom config file' '\n> -\tCUSTOM_CONFIG_FILE=\"custom.conf\" &&\n> -\tcat >\"$CUSTOM_CONFIG_FILE\" <<-\\EOF\n> +\tcat >\"custom.conf\" <<-\\EOF &&\n> \t[user]\n> \t\tcustom = true\n> \tEOF\n> +\tCUSTOM_CONFIG_FILE=\"$(test-tool path-utils real_path\n> custom.conf)\"\n> '\n\nFrom the commit message it was not clear to me this change was needed.\nDo you think it's worth it to add something to the commit message\nexplaining you now need to copy the custom.conf into each seperate\nrepository?\n\n\n--\nToon\n"},{"id":"487254","messageId":"87a5owqgzi.fsf@iotcl.com","threadId":"60704","inReplyTo":"1284b70fab6dd85114cb24fc5c7b6c49e35eb135.1704877233.git.ps@pks.im","subject":"Re: [PATCH v2 3/6] t1302: make tests more robust with new extensions","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2024-01-23T14:08:00Z","receivedAt":"2024-01-23T14:11:31Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"\nPatrick Steinhardt writes:\n\n> [[PGP Signed Part:Undecided]]\n> [1. text/plain]\n> In t1302 we exercise logic around \"core.repositoryFormatVersion\" and\n> extensions. These tests are not particularly robust against extensions\n> like the newly introduced \"refStorage\" extension.\n>\n> Refactor the tests to be more robust:\n>\n> - Check the DEFAULT_REPO_FORMAT prereq to determine the expected\n> repository format version. This helps to ensure that we only need to\n> update the prereq in a central place when new extensions are added.\n>\n> - Use a separate repository to rewrite \".git/config\" to test\n> combinations of the repository format version and extensions. This\n> ensures that we don't break the main test repository.\n>\n> - Do not rewrite \".git/config\" when exercising the \"preciousObjects\"\n> extension.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> t/t1302-repo-version.sh | 19 +++++++++++++++----\n> 1 file changed, 15 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh\n> index 179474fa65..7c680c91c4 100755\n> --- a/t/t1302-repo-version.sh\n> +++ b/t/t1302-repo-version.sh\n> @@ -79,8 +84,13 @@ mkconfig () {\n>\n> while read outcome version extensions; do\n> \ttest_expect_success \"$outcome version=$version $extensions\" \"\n> -\t\tmkconfig $version $extensions >.git/config &&\n> -\t\tcheck_${outcome}\n> +\t\ttest_when_finished 'rm -rf extensions' &&\n> +\t\tgit init extensions &&\n> +\t\t(\n> +\t\t\tcd extensions &&\n> +\t\t\tmkconfig $version $extensions >.git/config &&\n\nWhy did you not remove the use of `mkconfig` here?\n\n--\nToon\n"},{"id":"487260","messageId":"Za_Y1tP8-LEiNjrD@tanuki","threadId":"60704","inReplyTo":"87a5owqgzi.fsf@iotcl.com","subject":"Re: [PATCH v2 3/6] t1302: make tests more robust with new extensions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-23T15:18:46Z","receivedAt":"2024-01-23T15:18:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 23, 2024 at 03:08:00PM +0100, Toon Claes wrote:\n> \n> Patrick Steinhardt writes:\n> \n> > [[PGP Signed Part:Undecided]]\n> > [1. text/plain]\n> > In t1302 we exercise logic around \"core.repositoryFormatVersion\" and\n> > extensions. These tests are not particularly robust against extensions\n> > like the newly introduced \"refStorage\" extension.\n> >\n> > Refactor the tests to be more robust:\n> >\n> > - Check the DEFAULT_REPO_FORMAT prereq to determine the expected\n> > repository format version. This helps to ensure that we only need to\n> > update the prereq in a central place when new extensions are added.\n> >\n> > - Use a separate repository to rewrite \".git/config\" to test\n> > combinations of the repository format version and extensions. This\n> > ensures that we don't break the main test repository.\n> >\n> > - Do not rewrite \".git/config\" when exercising the \"preciousObjects\"\n> > extension.\n> >\n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > t/t1302-repo-version.sh | 19 +++++++++++++++----\n> > 1 file changed, 15 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh\n> > index 179474fa65..7c680c91c4 100755\n> > --- a/t/t1302-repo-version.sh\n> > +++ b/t/t1302-repo-version.sh\n> > @@ -79,8 +84,13 @@ mkconfig () {\n> >\n> > while read outcome version extensions; do\n> > \ttest_expect_success \"$outcome version=$version $extensions\" \"\n> > -\t\tmkconfig $version $extensions >.git/config &&\n> > -\t\tcheck_${outcome}\n> > +\t\ttest_when_finished 'rm -rf extensions' &&\n> > +\t\tgit init extensions &&\n> > +\t\t(\n> > +\t\t\tcd extensions &&\n> > +\t\t\tmkconfig $version $extensions >.git/config &&\n> \n> Why did you not remove the use of `mkconfig` here?\n\nI guess you mean stop using `mkconfig` and instead use git-config(1) to\nmanually write only the repository format version as well as extensions?\nThe problem here is that the resulting configuration would be dependent\non some environment variables, like `GIT_TEST_DEFAULT_HASH` and the\nrefdb equivalent `GIT_TEST_DEFAULT_REF_FORMAT` which do end up in the\nrepo's configuration. So I think it's actually preferable to overwrite\nthe complete \".git/config\" so that is exactly in the expected state.\n\nPatrick\n"},{"id":"487261","messageId":"Za_ZrtRMEmVHu_o5@tanuki","threadId":"60704","inReplyTo":"87ede8qhw9.fsf@iotcl.com","subject":"Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-23T15:22:22Z","receivedAt":"2024-01-23T15:22:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 23, 2024 at 02:41:17PM +0100, Toon Claes wrote:\n> \n> Patrick Steinhardt writes:\n> \n> > [[PGP Signed Part:Undecided]]\n> > [1. text/plain]\n> > The t1300 test suite exercises the git-config(1) tool. To do so we\n> > overwrite \".git/config\" to contain custom contents. While this is easy\n> > enough to do, it may create problems when using a non-default repository\n> > format because we also overwrite the repository format version as well\n> > as any potential extensions. With the upcoming \"reftable\" ref backend\n> > the result is that we may try to access refs via the \"files\" backend\n> > even though the repository has been initialized with the \"reftable\"\n> > backend.\n> >\n> > Refactor tests which access the refdb to be more robust by using their\n> > own separate repositories, which allows us to be more careful and not\n> > discard required extensions.\n> >\n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> \n> > @@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '\n> > '\n> >\n> > test_expect_success 'set up custom config file' '\n> > -\tCUSTOM_CONFIG_FILE=\"custom.conf\" &&\n> > -\tcat >\"$CUSTOM_CONFIG_FILE\" <<-\\EOF\n> > +\tcat >\"custom.conf\" <<-\\EOF &&\n> > \t[user]\n> > \t\tcustom = true\n> > \tEOF\n> > +\tCUSTOM_CONFIG_FILE=\"$(test-tool path-utils real_path\n> > custom.conf)\"\n> > '\n> \n> From the commit message it was not clear to me this change was needed.\n> Do you think it's worth it to add something to the commit message\n> explaining you now need to copy the custom.conf into each seperate\n> repository?\n\nGood point in fact. The problem here is that before, CUSTOM_CONFIG_FILE\nwas using a relative path that wouldn't be found when cd'ing into the\nrespective subrepositories. By using `path-utils real_path` we resolve\nthe relative path to the full path, and thus we can find the file\nregardless of our shell's current working directory.\n\nNot sure whether this is worth a reroll, but in case you or others think\nthat it is then I'm happy to add this explanation.\n\nPatrick\n"},{"id":"487263","messageId":"CAP8UFD0p-OqYuTrB5m2uq7BRFko887bKszOLtP5peP-A79g=BA@mail.gmail.com","threadId":"60704","inReplyTo":"0552123fa30243d6d8d6b378991651dd6ade7de3.1704877233.git.ps@pks.im","subject":"Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-01-23T16:15:48Z","receivedAt":"2024-01-23T16:16:03Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jan 10, 2024 at 10:01 AM Patrick Steinhardt wrote:\n>\n> The t1300 test suite exercises the git-config(1) tool. To do so we\n> overwrite \".git/config\" to contain custom contents.\n\nHere \"we\" means \"tests in t1300\" I guess.\n\n> While this is easy\n> enough to do, it may create problems when using a non-default repository\n> format because we also overwrite the repository format version as well\n> as any potential extensions.\n\nBut I am not sure that \"we\" in the above sentence is also \"tests in\nt1300\". I think overwriting the repo format version and potential\nextensions is done by other tests, right? Anyway it would be nice to\nclarify this.\n\n> With the upcoming \"reftable\" ref backend\n> the result is that we may try to access refs via the \"files\" backend\n> even though the repository has been initialized with the \"reftable\"\n> backend.\n\nNot sure here also what \"we\" is. When could refs be accessed via the\n\"files\" backend even though the repo was initialized with the\n\"reftable\" backend? Does this mean that some of the tests in t1300 try\nto access refs via the \"files\" backend while we may want to run all\nthe tests using the reftable backend?\n\n> Refactor tests which access the refdb to be more robust by using their\n> own separate repositories, which allows us to be more careful and not\n> discard required extensions.\n\nNot sure what exactly is discarding extensions. Also robust is not\nvery clear. It would be better to give at least an example of how\nthings could fail.\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> t/t1300-config.sh | 74 ++++++++++++++++++++++++++++++-----------------\n> 1 file changed, 48 insertions(+), 26 deletions(-)\n>\n> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> index f4e2752134..53c3d65823 100755\n> --- a/t/t1300-config.sh\n> +++ b/t/t1300-config.sh\n> @@ -1099,13 +1099,18 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '\n> '\n>\n> test_expect_success 'check split_cmdline return' \"\n> - git config alias.split-cmdline-fix 'echo \\\"' &&\n> - test_must_fail git split-cmdline-fix &&\n> - echo foo > foo &&\n> - git add foo &&\n> - git commit -m 'initial commit' &&\n> - git config branch.main.mergeoptions 'echo \\\"' &&\n> - test_must_fail git merge main\n> + test_when_finished 'rm -rf repo' &&\n> + git init repo &&\n> + (\n> + cd repo &&\n> + git config alias.split-cmdline-fix 'echo \\\"' &&\n> + test_must_fail git split-cmdline-fix &&\n> + echo foo >foo &&\n> + git add foo &&\n> + git commit -m 'initial commit' &&\n> + git config branch.main.mergeoptions 'echo \\\"' &&\n> + test_must_fail git merge main\n> + )\n> \"\n\nMaybe, while at it, this test could be modernized to use single quotes\naround the test code like:\n\ntest_expect_success 'check split_cmdline return' '\n...\n'\n\nor is using double quotes still Ok?\n\n> test_expect_success 'git -c \"key=value\" support' '\n> @@ -1157,10 +1162,16 @@ test_expect_success 'git -c works with aliases of builtins' '\n> '\n>\n> test_expect_success 'aliases can be CamelCased' '\n> - test_config alias.CamelCased \"rev-parse HEAD\" &&\n> - git CamelCased >out &&\n> - git rev-parse HEAD >expect &&\n> - test_cmp expect out\n> + test_when_finished \"rm -rf repo\" &&\n> + git init repo &&\n> + (\n> + cd repo &&\n> + test_commit A &&\n> + git config alias.CamelCased \"rev-parse HEAD\" &&\n> + git CamelCased >out &&\n> + git rev-parse HEAD >expect &&\n> + test_cmp expect out\n> + )\n> '\n\nHere single quotes are used for example.\n\n> test_expect_success 'git -c does not split values on equals' '\n> @@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '\n> '\n>\n> test_expect_success 'set up custom config file' '\n> - CUSTOM_CONFIG_FILE=\"custom.conf\" &&\n> - cat >\"$CUSTOM_CONFIG_FILE\" <<-\\EOF\n> + cat >\"custom.conf\" <<-\\EOF &&\n> [user]\n> custom = true\n> EOF\n> + CUSTOM_CONFIG_FILE=\"$(test-tool path-utils real_path custom.conf)\"\n> '\n\nThis looks like a test modernization, but maybe it's part of making\nthe tests more robust. Anyway it might be a good idea to either talk a\nbit about that in the commit message or to move it to a preparatory\ncommit if it's a modernization and other modernizations could be made\nin that preparatory commit.\n\nOtherwise this patch looks good to me.\n"},{"id":"487264","messageId":"CAP8UFD0yh4k4p7zcM+ZF90oWWTUMoGVpd3ajWWOV5pk++YKerw@mail.gmail.com","threadId":"60704","inReplyTo":"1284b70fab6dd85114cb24fc5c7b6c49e35eb135.1704877233.git.ps@pks.im","subject":"Re: [PATCH v2 3/6] t1302: make tests more robust with new extensions","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-01-23T16:15:56Z","receivedAt":"2024-01-23T16:16:10Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jan 10, 2024 at 10:02 AM Patrick Steinhardt wrote:\n>\n> In t1302 we exercise logic around \"core.repositoryFormatVersion\" and\n> extensions. These tests are not particularly robust against extensions\n> like the newly introduced \"refStorage\" extension.\n\nHere also it's not very clear what robust means. Some examples of how\nthings could fail would perhaps help.\n\n> Refactor the tests to be more robust:\n>\n> - Check the DEFAULT_REPO_FORMAT prereq to determine the expected\n> repository format version. This helps to ensure that we only need to\n> update the prereq in a central place when new extensions are added.\n>\n> - Use a separate repository to rewrite \".git/config\" to test\n> combinations of the repository format version and extensions. This\n> ensures that we don't break the main test repository.\n>\n> - Do not rewrite \".git/config\" when exercising the \"preciousObjects\"\n> extension.\n\nIt would be nice to talk a bit about the failures that each of the\nabove changes could prevent.\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> t/t1302-repo-version.sh | 19 +++++++++++++++----\n> 1 file changed, 15 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh\n> index 179474fa65..7c680c91c4 100755\n> --- a/t/t1302-repo-version.sh\n> +++ b/t/t1302-repo-version.sh\n> @@ -28,7 +28,12 @@ test_expect_success 'setup' '\n> '\n>\n> test_expect_success 'gitdir selection on normal repos' '\n> - test_oid version >expect &&\n> + if test_have_prereq DEFAULT_REPO_FORMAT\n> + then\n> + echo 0\n> + else\n> + echo 1\n> + fi >expect &&\n> git config core.repositoryformatversion >actual &&\n> git -C test config core.repositoryformatversion >actual2 &&\n> test_cmp expect actual &&\n> @@ -79,8 +84,13 @@ mkconfig () {\n\nBefore that hunk there is:\n\ntest_expect_success 'setup' '\n test_oid_cache <<-\\EOF &&\n version sha1:0\n version sha256:1\n EOF\n\nand it seems to me that the above test_oid_cache call is not needed\nanymore, if `test_oid version` is not used anymore.\n\nOtherwise this looks good to me.\n"},{"id":"487265","messageId":"CAP8UFD3orZNdaxUUxQTa6ZHDQtZLdmWvf9y0GGgaQadw1zoJUw@mail.gmail.com","threadId":"60704","inReplyTo":"cover.1704877233.git.ps@pks.im","subject":"Re: [PATCH v2 0/6] t: mark \"files\"-backend specific tests","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-01-23T16:20:37Z","receivedAt":"2024-01-23T16:20:51Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Wed, Jan 10, 2024 at 10:01 AM Patrick Steinhardt wrote:\n\n> Patrick Steinhardt (6):\n> t1300: make tests more robust with non-default ref backends\n> t1301: mark test for `core.sharedRepository` as reffiles specific\n> t1302: make tests more robust with new extensions\n> t1419: mark test suite as files-backend specific\n> t5526: break test submodule differently\n> t: mark tests regarding git-pack-refs(1) to be backend specific\n\nI took a look at these 6 patches and only had comments for patches 1/6\nand 3/6 so I replied only to those. The rest looks good to me.\n\nThanks!\n"},{"id":"487269","messageId":"CAGAWz+43bjotkJ3UxGgsageYuWt4g=95uBM-sy4dqyBVCJLToA@mail.gmail.com","threadId":"60704","inReplyTo":"Za_ZrtRMEmVHu_o5@tanuki","subject":"Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-01-23T16:43:10Z","receivedAt":"2024-01-23T16:43:24Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On Tue, Jan 23, 2024 at 9:22 AM Patrick Steinhardt wrote:\n>\n> On Tue, Jan 23, 2024 at 02:41:17PM +0100, Toon Claes wrote:\n> >\n> > Patrick Steinhardt writes:\n> >\n> > > [[PGP Signed Part:Undecided]]\n> > > [1. text/plain]\n> > > The t1300 test suite exercises the git-config(1) tool. To do so we\n> > > overwrite \".git/config\" to contain custom contents. While this is easy\n> > > enough to do, it may create problems when using a non-default repository\n> > > format because we also overwrite the repository format version as well\n> > > as any potential extensions. With the upcoming \"reftable\" ref backend\n> > > the result is that we may try to access refs via the \"files\" backend\n> > > even though the repository has been initialized with the \"reftable\"\n> > > backend.\n> > >\n> > > Refactor tests which access the refdb to be more robust by using their\n> > > own separate repositories, which allows us to be more careful and not\n> > > discard required extensions.\n> > >\n> > > Signed-off-by: Patrick Steinhardt \n> > > ---\n> >\n> > > @@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '\n> > > '\n> > >\n> > > test_expect_success 'set up custom config file' '\n> > > - CUSTOM_CONFIG_FILE=\"custom.conf\" &&\n> > > - cat >\"$CUSTOM_CONFIG_FILE\" <<-\\EOF\n> > > + cat >\"custom.conf\" <<-\\EOF &&\n> > > [user]\n> > > custom = true\n> > > EOF\n> > > + CUSTOM_CONFIG_FILE=\"$(test-tool path-utils real_path\n> > > custom.conf)\"\n> > > '\n> >\n> > From the commit message it was not clear to me this change was needed.\n> > Do you think it's worth it to add something to the commit message\n> > explaining you now need to copy the custom.conf into each seperate\n> > repository?\n>\n> Good point in fact. The problem here is that before, CUSTOM_CONFIG_FILE\n> was using a relative path that wouldn't be found when cd'ing into the\n> respective subrepositories. By using `path-utils real_path` we resolve\n> the relative path to the full path, and thus we can find the file\n> regardless of our shell's current working directory.\n>\n> Not sure whether this is worth a reroll, but in case you or others think\n> that it is then I'm happy to add this explanation.\n\nI also found it unclear why the CUSTOM_CONFIG_FILE change was needed.\nI had assumed it was a refactor to make the tests more robust. It might be\nnice to explain it in the commit message. :)\n\n-Justin\n"},{"id":"487311","messageId":"cover.1706085756.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1704802213.git.ps@pks.im","subject":"[PATCH v3 0/6] t: mark \"files\"-backend specific tests","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-24T08:45:01Z","receivedAt":"2024-01-24T08:45:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the third version of my patch series that addresses tests whihc\nare specific to the \"files\" backend. Changes compared to v2:\n\n - Reworded some commit messages to hopefully explain better what is\n going on.\n\n - Refactored a test \"while at it\" to not use double quotes for the\n test body.\n\n - Removed the now-unneeded OID cache for one of our tests.\n\nThanks all for your comments!\n\nPatrick\n\nPatrick Steinhardt (6):\n t1300: make tests more robust with non-default ref backends\n t1301: mark test for `core.sharedRepository` as reffiles specific\n t1302: make tests more robust with new extensions\n t1419: mark test suite as files-backend specific\n t5526: break test submodule differently\n t: mark tests regarding git-pack-refs(1) to be backend specific\n\n t/t1300-config.sh | 78 ++++++++++++++++++++++-------------\n t/t1301-shared-repo.sh | 2 +-\n t/t1302-repo-version.sh | 23 +++++++----\n t/t1409-avoid-packing-refs.sh | 6 +++\n t/t1419-exclude-refs.sh | 6 +++\n t/t3210-pack-refs.sh | 6 +++\n t/t5526-fetch-submodules.sh | 2 +-\n 7 files changed, 85 insertions(+), 38 deletions(-)\n\nRange-diff against v2:\n1: 0552123fa3 ! 1: a57e57a7c3 t1300: make tests more robust with non-default ref backends\n @@ Metadata\n ## Commit message ##\n t1300: make tests more robust with non-default ref backends\n \n - The t1300 test suite exercises the git-config(1) tool. To do so we\n - overwrite \".git/config\" to contain custom contents. While this is easy\n - enough to do, it may create problems when using a non-default repository\n - format because we also overwrite the repository format version as well\n - as any potential extensions. With the upcoming \"reftable\" ref backend\n - the result is that we may try to access refs via the \"files\" backend\n - even though the repository has been initialized with the \"reftable\"\n - backend.\n + The t1300 test suite exercises the git-config(1) tool. To do so, the\n + test overwrites \".git/config\" to contain custom contents. While this is\n + easy enough to do, it may create problems when using a non-default\n + repository format because this causes us to overwrite the repository\n + format version as well as any potential extensions. With the upcoming\n + \"reftable\" ref backend the result is that Git would try to access refs\n + via the \"files\" backend even though the repository has been initialized\n + with the \"reftable\" backend, which will cause failures when trying to\n + access any refs.\n \n Refactor tests which access the refdb to be more robust by using their\n own separate repositories, which allows us to be more careful and not\n discard required extensions.\n \n + Note that we also have to touch up how the CUSTOM_CONFIG_FILE gets\n + accessed. This environment variable contains the relative path to a\n + custom config file which we're setting up. But because we are now using\n + subrepositories, this relative path will not be found anymore because\n + our working directory changes. This issue is addressed by storing the\n + absolute path to the file in CUSTOM_CONFIG_FILE instead.\n +\n Signed-off-by: Patrick Steinhardt \n \n ## t/t1300-config.sh ##\n @@ t/t1300-config.sh: test_expect_success SYMLINKS 'symlink to nonexistent configuration' '\n + \ttest_must_fail git config --file=linktolinktonada --list\n '\n \n - test_expect_success 'check split_cmdline return' \"\n +-test_expect_success 'check split_cmdline return' \"\n -\tgit config alias.split-cmdline-fix 'echo \\\"' &&\n -\ttest_must_fail git split-cmdline-fix &&\n -\techo foo > foo &&\n @@ t/t1300-config.sh: test_expect_success SYMLINKS 'symlink to nonexistent configur\n -\tgit commit -m 'initial commit' &&\n -\tgit config branch.main.mergeoptions 'echo \\\"' &&\n -\ttest_must_fail git merge main\n -+\ttest_when_finished 'rm -rf repo' &&\n +-\"\n ++test_expect_success 'check split_cmdline return' '\n ++\ttest_when_finished \"rm -rf repo\" &&\n +\tgit init repo &&\n +\t(\n +\t\tcd repo &&\n -+\t\tgit config alias.split-cmdline-fix 'echo \\\"' &&\n ++\t\tgit config alias.split-cmdline-fix \"echo \\\"\" &&\n +\t\ttest_must_fail git split-cmdline-fix &&\n +\t\techo foo >foo &&\n +\t\tgit add foo &&\n -+\t\tgit commit -m 'initial commit' &&\n -+\t\tgit config branch.main.mergeoptions 'echo \\\"' &&\n ++\t\tgit commit -m \"initial commit\" &&\n ++\t\tgit config branch.main.mergeoptions \"echo \\\"\" &&\n +\t\ttest_must_fail git merge main\n +\t)\n - \"\n ++'\n \n test_expect_success 'git -c \"key=value\" support' '\n + \tcat >expect <<-\\EOF &&\n @@ t/t1300-config.sh: test_expect_success 'git -c works with aliases of builtins' '\n '\n \n2: 384250fec2 = 2: fd6dd92c23 t1301: mark test for `core.sharedRepository` as reffiles specific\n3: 1284b70fab ! 3: ec90320ff1 t1302: make tests more robust with new extensions\n @@ Commit message\n \n In t1302 we exercise logic around \"core.repositoryFormatVersion\" and\n extensions. These tests are not particularly robust against extensions\n - like the newly introduced \"refStorage\" extension.\n + like the newly introduced \"refStorage\" extension as we tend to clobber\n + the repository's config file. We thus overwrite any extensions that were\n + set, which may render the repository inaccessible in case it has to be\n + accessed with a non-default ref storage.\n \n Refactor the tests to be more robust:\n \n - Check the DEFAULT_REPO_FORMAT prereq to determine the expected\n repository format version. This helps to ensure that we only need to\n update the prereq in a central place when new extensions are added.\n + Furthermore, this allows us to stop seeding the now-unneeded object\n + ID cache that was only used to figure out the repository version.\n \n - Use a separate repository to rewrite \".git/config\" to test\n combinations of the repository format version and extensions. This\n - ensures that we don't break the main test repository.\n + ensures that we don't break the main test repository. While we could\n + rewrite these tests to not overwrite preexisting extensions, it\n + feels cleaner like this so that we can test extensions standalone\n + without interference from the environment.\n \n - Do not rewrite \".git/config\" when exercising the \"preciousObjects\"\n extension.\n @@ Commit message\n Signed-off-by: Patrick Steinhardt \n \n ## t/t1302-repo-version.sh ##\n +@@ t/t1302-repo-version.sh: TEST_PASSES_SANITIZE_LEAK=true\n + . ./test-lib.sh\n + \n + test_expect_success 'setup' '\n +-\ttest_oid_cache <<-\\EOF &&\n +-\tversion sha1:0\n +-\tversion sha256:1\n +-\tEOF\n + \tcat >test.patch <<-\\EOF &&\n + \tdiff --git a/test.txt b/test.txt\n + \tnew file mode 100644\n @@ t/t1302-repo-version.sh: test_expect_success 'setup' '\n '\n \n4: c6062b612c = 4: d0d70c3f18 t1419: mark test suite as files-backend specific\n5: ae08afc459 = 5: 066c297189 t5526: break test submodule differently\n6: df648be535 = 6: 7b8921817b t: mark tests regarding git-pack-refs(1) to be backend specific\n-- \n2.43.GIT\n\n"},{"id":"487312","messageId":"a57e57a7c33ff3b2664ccfe305af4d7208a74901.1706085756.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1706085756.git.ps@pks.im","subject":"[PATCH v3 1/6] t1300: make tests more robust with non-default ref backends","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-24T08:45:05Z","receivedAt":"2024-01-24T08:45:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The t1300 test suite exercises the git-config(1) tool. To do so, the\ntest overwrites \".git/config\" to contain custom contents. While this is\neasy enough to do, it may create problems when using a non-default\nrepository format because this causes us to overwrite the repository\nformat version as well as any potential extensions. With the upcoming\n\"reftable\" ref backend the result is that Git would try to access refs\nvia the \"files\" backend even though the repository has been initialized\nwith the \"reftable\" backend, which will cause failures when trying to\naccess any refs.\n\nRefactor tests which access the refdb to be more robust by using their\nown separate repositories, which allows us to be more careful and not\ndiscard required extensions.\n\nNote that we also have to touch up how the CUSTOM_CONFIG_FILE gets\naccessed. This environment variable contains the relative path to a\ncustom config file which we're setting up. But because we are now using\nsubrepositories, this relative path will not be found anymore because\nour working directory changes. This issue is addressed by storing the\nabsolute path to the file in CUSTOM_CONFIG_FILE instead.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1300-config.sh | 78 ++++++++++++++++++++++++++++++-----------------\n 1 file changed, 50 insertions(+), 28 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex f4e2752134..31c3878687 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1098,15 +1098,20 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '\n \ttest_must_fail git config --file=linktolinktonada --list\n '\n \n-test_expect_success 'check split_cmdline return' \"\n-\tgit config alias.split-cmdline-fix 'echo \\\"' &&\n-\ttest_must_fail git split-cmdline-fix &&\n-\techo foo > foo &&\n-\tgit add foo &&\n-\tgit commit -m 'initial commit' &&\n-\tgit config branch.main.mergeoptions 'echo \\\"' &&\n-\ttest_must_fail git merge main\n-\"\n+test_expect_success 'check split_cmdline return' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config alias.split-cmdline-fix \"echo \\\"\" &&\n+\t\ttest_must_fail git split-cmdline-fix &&\n+\t\techo foo >foo &&\n+\t\tgit add foo &&\n+\t\tgit commit -m \"initial commit\" &&\n+\t\tgit config branch.main.mergeoptions \"echo \\\"\" &&\n+\t\ttest_must_fail git merge main\n+\t)\n+'\n \n test_expect_success 'git -c \"key=value\" support' '\n \tcat >expect <<-\\EOF &&\n@@ -1157,10 +1162,16 @@ test_expect_success 'git -c works with aliases of builtins' '\n '\n \n test_expect_success 'aliases can be CamelCased' '\n-\ttest_config alias.CamelCased \"rev-parse HEAD\" &&\n-\tgit CamelCased >out &&\n-\tgit rev-parse HEAD >expect &&\n-\ttest_cmp expect out\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A &&\n+\t\tgit config alias.CamelCased \"rev-parse HEAD\" &&\n+\t\tgit CamelCased >out &&\n+\t\tgit rev-parse HEAD >expect &&\n+\t\ttest_cmp expect out\n+\t)\n '\n \n test_expect_success 'git -c does not split values on equals' '\n@@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '\n '\n \n test_expect_success 'set up custom config file' '\n-\tCUSTOM_CONFIG_FILE=\"custom.conf\" &&\n-\tcat >\"$CUSTOM_CONFIG_FILE\" <<-\\EOF\n+\tcat >\"custom.conf\" <<-\\EOF &&\n \t[user]\n \t\tcustom = true\n \tEOF\n+\tCUSTOM_CONFIG_FILE=\"$(test-tool path-utils real_path custom.conf)\"\n '\n \n test_expect_success !MINGW 'set up custom config file with special name characters' '\n@@ -2052,22 +2063,33 @@ test_expect_success '--show-origin stdin with file include' '\n '\n \n test_expect_success '--show-origin blob' '\n-\tblob=$(git hash-object -w \"$CUSTOM_CONFIG_FILE\") &&\n-\tcat >expect <<-EOF &&\n-\tblob:$blob\tuser.custom=true\n-\tEOF\n-\tgit config --blob=$blob --show-origin --list >output &&\n-\ttest_cmp expect output\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tblob=$(git hash-object -w \"$CUSTOM_CONFIG_FILE\") &&\n+\t\tcat >expect <<-EOF &&\n+\t\tblob:$blob\tuser.custom=true\n+\t\tEOF\n+\t\tgit config --blob=$blob --show-origin --list >output &&\n+\t\ttest_cmp expect output\n+\t)\n '\n \n test_expect_success '--show-origin blob ref' '\n-\tcat >expect <<-\\EOF &&\n-\tblob:main:custom.conf\tuser.custom=true\n-\tEOF\n-\tgit add \"$CUSTOM_CONFIG_FILE\" &&\n-\tgit commit -m \"new config file\" &&\n-\tgit config --blob=main:\"$CUSTOM_CONFIG_FILE\" --show-origin --list >output &&\n-\ttest_cmp expect output\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tcat >expect <<-\\EOF &&\n+\t\tblob:main:custom.conf\tuser.custom=true\n+\t\tEOF\n+\t\tcp \"$CUSTOM_CONFIG_FILE\" custom.conf &&\n+\t\tgit add custom.conf &&\n+\t\tgit commit -m \"new config file\" &&\n+\t\tgit config --blob=main:custom.conf --show-origin --list >output &&\n+\t\ttest_cmp expect output\n+\t)\n '\n \n test_expect_success '--show-origin with --default' '\n-- \n2.43.GIT\n\n"},{"id":"487313","messageId":"fd6dd92c23dddb47bafd81d063c0d00b91f43204.1706085756.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1706085756.git.ps@pks.im","subject":"[PATCH v3 2/6] t1301: mark test for `core.sharedRepository` as reffiles specific","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-24T08:45:09Z","receivedAt":"2024-01-24T08:45:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In t1301 we verify whether reflog files written by the \"files\" ref\nbackend correctly honor permissions when \"core.sharedRepository\" is set.\nThe test logic is thus specific to the reffiles backend and will not\nwork with any other backends.\n\nMark the test accordingly with the REFFILES prereq.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1301-shared-repo.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex e5a0d65caa..8e2c01e760 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -137,7 +137,7 @@ test_expect_success POSIXPERM 'info/refs respects umask in unshared repo' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '\n+test_expect_success REFFILES,POSIXPERM 'git reflog expire honors core.sharedRepository' '\n \tumask 077 &&\n \tgit config core.sharedRepository group &&\n \tgit reflog expire --all &&\n-- \n2.43.GIT\n\n"},{"id":"487314","messageId":"ec90320ff1d5c0a6e5657ef77096c90d816f0513.1706085756.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1706085756.git.ps@pks.im","subject":"[PATCH v3 3/6] t1302: make tests more robust with new extensions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-24T08:45:13Z","receivedAt":"2024-01-24T08:45:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In t1302 we exercise logic around \"core.repositoryFormatVersion\" and\nextensions. These tests are not particularly robust against extensions\nlike the newly introduced \"refStorage\" extension as we tend to clobber\nthe repository's config file. We thus overwrite any extensions that were\nset, which may render the repository inaccessible in case it has to be\naccessed with a non-default ref storage.\n\nRefactor the tests to be more robust:\n\n - Check the DEFAULT_REPO_FORMAT prereq to determine the expected\n repository format version. This helps to ensure that we only need to\n update the prereq in a central place when new extensions are added.\n Furthermore, this allows us to stop seeding the now-unneeded object\n ID cache that was only used to figure out the repository version.\n\n - Use a separate repository to rewrite \".git/config\" to test\n combinations of the repository format version and extensions. This\n ensures that we don't break the main test repository. While we could\n rewrite these tests to not overwrite preexisting extensions, it\n feels cleaner like this so that we can test extensions standalone\n without interference from the environment.\n\n - Do not rewrite \".git/config\" when exercising the \"preciousObjects\"\n extension.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1302-repo-version.sh | 23 +++++++++++++++--------\n 1 file changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh\nindex 179474fa65..42caa0d297 100755\n--- a/t/t1302-repo-version.sh\n+++ b/t/t1302-repo-version.sh\n@@ -9,10 +9,6 @@ TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n-\ttest_oid_cache <<-\\EOF &&\n-\tversion sha1:0\n-\tversion sha256:1\n-\tEOF\n \tcat >test.patch <<-\\EOF &&\n \tdiff --git a/test.txt b/test.txt\n \tnew file mode 100644\n@@ -28,7 +24,12 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'gitdir selection on normal repos' '\n-\ttest_oid version >expect &&\n+\tif test_have_prereq DEFAULT_REPO_FORMAT\n+\tthen\n+\t\techo 0\n+\telse\n+\t\techo 1\n+\tfi >expect &&\n \tgit config core.repositoryformatversion >actual &&\n \tgit -C test config core.repositoryformatversion >actual2 &&\n \ttest_cmp expect actual &&\n@@ -79,8 +80,13 @@ mkconfig () {\n \n while read outcome version extensions; do\n \ttest_expect_success \"$outcome version=$version $extensions\" \"\n-\t\tmkconfig $version $extensions >.git/config &&\n-\t\tcheck_${outcome}\n+\t\ttest_when_finished 'rm -rf extensions' &&\n+\t\tgit init extensions &&\n+\t\t(\n+\t\t\tcd extensions &&\n+\t\t\tmkconfig $version $extensions >.git/config &&\n+\t\t\tcheck_${outcome}\n+\t\t)\n \t\"\n done <<\\EOF\n allow 0\n@@ -94,7 +100,8 @@ allow 1 noop-v1\n EOF\n \n test_expect_success 'precious-objects allowed' '\n-\tmkconfig 1 preciousObjects >.git/config &&\n+\tgit config core.repositoryFormatVersion 1 &&\n+\tgit config extensions.preciousObjects 1 &&\n \tcheck_allow\n '\n \n-- \n2.43.GIT\n\n"},{"id":"487315","messageId":"d0d70c3f18d79e9f24f912a4944ccb4c862223fc.1706085756.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1706085756.git.ps@pks.im","subject":"[PATCH v3 4/6] t1419: mark test suite as files-backend specific","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-24T08:45:18Z","receivedAt":"2024-01-24T08:45:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"With 59c35fac54 (refs/packed-backend.c: implement jump lists to avoid\nexcluded pattern(s), 2023-07-10) we have implemented logic to handle\nexcluded refs more efficiently in the \"packed\" ref backend. This logic\nallows us to skip emitting refs completely which we know to not be of\nany interest to the caller, which can avoid quite some allocations and\nobject lookups.\n\nThis was wired up via a new `exclude_patterns` parameter passed to the\nbackend's ref iterator. The backend only needs to handle them on a best\neffort basis though, and in fact we only handle it for the \"packed-refs\"\nfile, but not for loose references. Consequently, all callers must still\nfilter emitted refs with those exclude patterns.\n\nThe result is that handling exclude patterns is completely optional in\nthe ref backend, and any future backends may or may not implement it.\nLet's thus mark the test for t1419 to depend on the REFFILES prereq.\n\nAn alternative would be to introduce a new prereq that tells us whether\nthe backend under test supports exclude patterns or not. But this does\nfeel a bit overblown:\n\n - It would either map to the REFFILES prereq, in which case it feels\n overengineered because the prereq is only ever relevant to t1419.\n\n - Otherwise, it could auto-detect whether the backend supports exclude\n patterns. But this could lead to silent failures in case the support\n for this feature breaks at any point in time.\n\nIt should thus be good enough to just use the REFFILES prereq for now.\nIf future backends ever grow support for exclude patterns we can easily\nadd their respective prereq as another condition for this test suite to\nexecute.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1419-exclude-refs.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t1419-exclude-refs.sh b/t/t1419-exclude-refs.sh\nindex 5d8c86b657..1359574419 100755\n--- a/t/t1419-exclude-refs.sh\n+++ b/t/t1419-exclude-refs.sh\n@@ -8,6 +8,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n+if test_have_prereq !REFFILES\n+then\n+\tskip_all='skipping `git for-each-ref --exclude` tests; need files backend'\n+\ttest_done\n+fi\n+\n for_each_ref__exclude () {\n \tGIT_TRACE2_PERF=1 test-tool ref-store main \\\n \t\tfor-each-ref--exclude \"$@\" >actual.raw\n-- \n2.43.GIT\n\n"},{"id":"487316","messageId":"066c297189f6e09ee9f0defa9998932d8072c854.1706085756.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1706085756.git.ps@pks.im","subject":"[PATCH v3 5/6] t5526: break test submodule differently","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-24T08:45:22Z","receivedAt":"2024-01-24T08:45:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 10f5c52656 (submodule: avoid auto-discovery in\nprepare_submodule_repo_env(), 2016-09-01) we fixed a bug when doing a\nrecursive fetch with submodule in the case where the submodule is broken\ndue to whatever reason. The test to exercise that the fix works breaks\nthe submodule by deleting its `HEAD` reference, which will cause us to\nnot detect the directory as a Git repository.\n\nWhile this is perfectly fine in theory, this way of breaking the repo\nbecomes problematic with the current efforts to introduce another refdb\nbackend into Git. The new reftable backend has a stub HEAD file that\nalways contains \"ref: refs/heads/.invalid\" so that tools continue to be\nable to detect such a repository. But as the reftable backend will never\ndelete this file even when asked to delete `HEAD` the current way to\ndelete the `HEAD` reference will stop working.\n\nAdapt the code to instead delete the objects database. Going back with\nthis new way to cause breakage confirms that it triggers the infinite\nrecursion just the same, and there are no equivalent ongoing efforts to\nreplace the object database with an alternate backend.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5526-fetch-submodules.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 7ab220fa31..5e566205ba 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '\n \tgit -C dst fetch --recurse-submodules &&\n \n \t# Break the receiving submodule\n-\ttest-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&\n+\trm -r dst/sub/.git/objects &&\n \n \t# NOTE: without the fix the following tests will recurse forever!\n \t# They should terminate with an error.\n-- \n2.43.GIT\n\n"},{"id":"487317","messageId":"7b8921817b14df7fe3346f89c84369df7403ddb0.1706085756.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1706085756.git.ps@pks.im","subject":"[PATCH v3 6/6] t: mark tests regarding git-pack-refs(1) to be backend specific","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-24T08:45:26Z","receivedAt":"2024-01-24T08:45:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Both t1409 and t3210 exercise parts of git-pack-refs(1). Given that we\nmust check the on-disk files to verify whether the backend has indeed\npacked refs as expected those test suites are deeply tied to the actual\nbackend that is in use.\n\nMark the test suites to depend on the REFFILES backend.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1409-avoid-packing-refs.sh | 6 ++++++\n t/t3210-pack-refs.sh | 6 ++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh\nindex f23c0152a8..7748973733 100755\n--- a/t/t1409-avoid-packing-refs.sh\n+++ b/t/t1409-avoid-packing-refs.sh\n@@ -5,6 +5,12 @@ test_description='avoid rewriting packed-refs unnecessarily'\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n+if test_have_prereq !REFFILES\n+then\n+ skip_all='skipping files-backend specific pack-refs tests'\n+ test_done\n+fi\n+\n # Add an identifying mark to the packed-refs file header line. This\n # shouldn't upset readers, and it should be omitted if the file is\n # ever rewritten.\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex 7f4e98db7d..c0f1f9cfb7 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -15,6 +15,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n+if test_have_prereq !REFFILES\n+then\n+ skip_all='skipping files-backend specific pack-refs tests'\n+ test_done\n+fi\n+\n test_expect_success 'enable reflogs' '\n \tgit config core.logallrefupdates true\n '\n-- \n2.43.GIT\n\n"},{"id":"487319","messageId":"ZbDP27b_L8G2Dd49@tanuki","threadId":"60704","inReplyTo":"CAP8UFD0yh4k4p7zcM+ZF90oWWTUMoGVpd3ajWWOV5pk++YKerw@mail.gmail.com","subject":"Re: [PATCH v2 3/6] t1302: make tests more robust with new extensions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-24T08:52:43Z","receivedAt":"2024-01-24T08:52:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 23, 2024 at 05:15:56PM +0100, Christian Couder wrote:\n> On Wed, Jan 10, 2024 at 10:02 AM Patrick Steinhardt wrote:\n> >\n> > In t1302 we exercise logic around \"core.repositoryFormatVersion\" and\n> > extensions. These tests are not particularly robust against extensions\n> > like the newly introduced \"refStorage\" extension.\n> \n> Here also it's not very clear what robust means. Some examples of how\n> things could fail would perhaps help.\n> \n> > Refactor the tests to be more robust:\n> >\n> > - Check the DEFAULT_REPO_FORMAT prereq to determine the expected\n> > repository format version. This helps to ensure that we only need to\n> > update the prereq in a central place when new extensions are added.\n> >\n> > - Use a separate repository to rewrite \".git/config\" to test\n> > combinations of the repository format version and extensions. This\n> > ensures that we don't break the main test repository.\n> >\n> > - Do not rewrite \".git/config\" when exercising the \"preciousObjects\"\n> > extension.\n> \n> It would be nice to talk a bit about the failures that each of the\n> above changes could prevent.\n\nThey all fail in the same way: we fail to access the repository because\nwe delete the extension that tells us to use the reftable backend. I'll\nadd that explanation at the front.\n\n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > t/t1302-repo-version.sh | 19 +++++++++++++++----\n> > 1 file changed, 15 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh\n> > index 179474fa65..7c680c91c4 100755\n> > --- a/t/t1302-repo-version.sh\n> > +++ b/t/t1302-repo-version.sh\n> > @@ -28,7 +28,12 @@ test_expect_success 'setup' '\n> > '\n> >\n> > test_expect_success 'gitdir selection on normal repos' '\n> > - test_oid version >expect &&\n> > + if test_have_prereq DEFAULT_REPO_FORMAT\n> > + then\n> > + echo 0\n> > + else\n> > + echo 1\n> > + fi >expect &&\n> > git config core.repositoryformatversion >actual &&\n> > git -C test config core.repositoryformatversion >actual2 &&\n> > test_cmp expect actual &&\n> > @@ -79,8 +84,13 @@ mkconfig () {\n> \n> Before that hunk there is:\n> \n> test_expect_success 'setup' '\n> test_oid_cache <<-\\EOF &&\n> version sha1:0\n> version sha256:1\n> EOF\n> \n> and it seems to me that the above test_oid_cache call is not needed\n> anymore, if `test_oid version` is not used anymore.\n> \n> Otherwise this looks good to me.\n\nGood catch.\n\nPatrick\n"},{"id":"487320","messageId":"ZbDP4ntRqrxX08yk@tanuki","threadId":"60704","inReplyTo":"CAP8UFD0p-OqYuTrB5m2uq7BRFko887bKszOLtP5peP-A79g=BA@mail.gmail.com","subject":"Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-24T08:52:50Z","receivedAt":"2024-01-24T08:52:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 23, 2024 at 05:15:48PM +0100, Christian Couder wrote:\n> On Wed, Jan 10, 2024 at 10:01 AM Patrick Steinhardt wrote:\n> >\n> > The t1300 test suite exercises the git-config(1) tool. To do so we\n> > overwrite \".git/config\" to contain custom contents.\n> \n> Here \"we\" means \"tests in t1300\" I guess.\n> \n> > While this is easy\n> > enough to do, it may create problems when using a non-default repository\n> > format because we also overwrite the repository format version as well\n> > as any potential extensions.\n> \n> But I am not sure that \"we\" in the above sentence is also \"tests in\n> t1300\". I think overwriting the repo format version and potential\n> extensions is done by other tests, right? Anyway it would be nice to\n> clarify this.\n> \n> > With the upcoming \"reftable\" ref backend\n> > the result is that we may try to access refs via the \"files\" backend\n> > even though the repository has been initialized with the \"reftable\"\n> > backend.\n> \n> Not sure here also what \"we\" is. When could refs be accessed via the\n> \"files\" backend even though the repo was initialized with the\n> \"reftable\" backend?\n\nYeah, I've rephrased all of these to sey \"the tests\" or something\nsimilar.\n\n> Does this mean that some of the tests in t1300 try to access refs via\n> the \"files\" backend while we may want to run all the tests using the\n> reftable backend?\n\nExactly. We overwrite the \".git/config\", which contains the \"refStorage\"\nextension that tells us to use the \"reftable\" backend. So the extension\nis gone, and thus Git would fall back to use the \"files\" backend\ninstead, which will fail.\n\n> > Refactor tests which access the refdb to be more robust by using their\n> > own separate repositories, which allows us to be more careful and not\n> > discard required extensions.\n> \n> Not sure what exactly is discarding extensions. Also robust is not\n> very clear. It would be better to give at least an example of how\n> things could fail.\n\nHm. I don't really know how to phrase this better. The preceding\nparagraph already explains why we're discarding the extension and what\nthe consequence is. I added a sentence saying \", which will cause\nfailures when trying to access any refs.\"\n\n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > t/t1300-config.sh | 74 ++++++++++++++++++++++++++++++-----------------\n> > 1 file changed, 48 insertions(+), 26 deletions(-)\n> >\n> > diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> > index f4e2752134..53c3d65823 100755\n> > --- a/t/t1300-config.sh\n> > +++ b/t/t1300-config.sh\n> > @@ -1099,13 +1099,18 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '\n> > '\n> >\n> > test_expect_success 'check split_cmdline return' \"\n> > - git config alias.split-cmdline-fix 'echo \\\"' &&\n> > - test_must_fail git split-cmdline-fix &&\n> > - echo foo > foo &&\n> > - git add foo &&\n> > - git commit -m 'initial commit' &&\n> > - git config branch.main.mergeoptions 'echo \\\"' &&\n> > - test_must_fail git merge main\n> > + test_when_finished 'rm -rf repo' &&\n> > + git init repo &&\n> > + (\n> > + cd repo &&\n> > + git config alias.split-cmdline-fix 'echo \\\"' &&\n> > + test_must_fail git split-cmdline-fix &&\n> > + echo foo >foo &&\n> > + git add foo &&\n> > + git commit -m 'initial commit' &&\n> > + git config branch.main.mergeoptions 'echo \\\"' &&\n> > + test_must_fail git merge main\n> > + )\n> > \"\n> \n> Maybe, while at it, this test could be modernized to use single quotes\n> around the test code like:\n> \n> test_expect_success 'check split_cmdline return' '\n> ...\n> '\n> \n> or is using double quotes still Ok?\n\nIn general single quotes are preferable. This test is using quotes\ninternally, which I guess is the reason why we didn't. Happy to change\nwhile at it.\n\n[snip]\n> > test_expect_success 'git -c does not split values on equals' '\n> > @@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '\n> > '\n> >\n> > test_expect_success 'set up custom config file' '\n> > - CUSTOM_CONFIG_FILE=\"custom.conf\" &&\n> > - cat >\"$CUSTOM_CONFIG_FILE\" <<-\\EOF\n> > + cat >\"custom.conf\" <<-\\EOF &&\n> > [user]\n> > custom = true\n> > EOF\n> > + CUSTOM_CONFIG_FILE=\"$(test-tool path-utils real_path custom.conf)\"\n> > '\n> \n> This looks like a test modernization, but maybe it's part of making\n> the tests more robust. Anyway it might be a good idea to either talk a\n> bit about that in the commit message or to move it to a preparatory\n> commit if it's a modernization and other modernizations could be made\n> in that preparatory commit.\n> \n> Otherwise this patch looks good to me.\n\nYup, this has also been pointed out by others. Will mention in the\ncommit message.\n\nPatrick\n"},{"id":"487496","messageId":"CAP8UFD2ZoV74jx3B_adwYENGtp9fCH_oVjW4QJXTZQJ0=_aeEA@mail.gmail.com","threadId":"60704","inReplyTo":"ZbDP4ntRqrxX08yk@tanuki","subject":"Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-01-29T10:32:53Z","receivedAt":"2024-01-29T10:33:07Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jan 24, 2024 at 9:52 AM Patrick Steinhardt wrote:\n>\n> On Tue, Jan 23, 2024 at 05:15:48PM +0100, Christian Couder wrote:\n> > On Wed, Jan 10, 2024 at 10:01 AM Patrick Steinhardt wrote:\n> > >\n> > > The t1300 test suite exercises the git-config(1) tool. To do so we\n> > > overwrite \".git/config\" to contain custom contents.\n> >\n> > Here \"we\" means \"tests in t1300\" I guess.\n> >\n> > > While this is easy\n> > > enough to do, it may create problems when using a non-default repository\n> > > format because we also overwrite the repository format version as well\n> > > as any potential extensions.\n> >\n> > But I am not sure that \"we\" in the above sentence is also \"tests in\n> > t1300\". I think overwriting the repo format version and potential\n> > extensions is done by other tests, right? Anyway it would be nice to\n> > clarify this.\n> >\n> > > With the upcoming \"reftable\" ref backend\n> > > the result is that we may try to access refs via the \"files\" backend\n> > > even though the repository has been initialized with the \"reftable\"\n> > > backend.\n> >\n> > Not sure here also what \"we\" is. When could refs be accessed via the\n> > \"files\" backend even though the repo was initialized with the\n> > \"reftable\" backend?\n>\n> Yeah, I've rephrased all of these to sey \"the tests\" or something\n> similar.\n>\n> > Does this mean that some of the tests in t1300 try to access refs via\n> > the \"files\" backend while we may want to run all the tests using the\n> > reftable backend?\n>\n> Exactly. We overwrite the \".git/config\", which contains the \"refStorage\"\n> extension that tells us to use the \"reftable\" backend. So the extension\n> is gone, and thus Git would fall back to use the \"files\" backend\n> instead, which will fail.\n\nLet's take a look at this test:\n\ntest_expect_success 'check split_cmdline return' \"\n git config alias.split-cmdline-fix 'echo \\\"' &&\n test_must_fail git split-cmdline-fix &&\n echo foo > foo &&\n git add foo &&\n git commit -m 'initial commit' &&\n git config branch.main.mergeoptions 'echo \\\"' &&\n test_must_fail git merge main\n\"\n\nI don't really see how it overwrites anything. When putting some debug\ncommands before and after that test, it looks like the config file\ncontains the following before that test:\n\n---\n[section \"sub=section\"]\n val1 = foo=bar\n val2 = foo\\nbar\n val3 = \\n\\n\n val4 =\n val5\n[section]\n val = foo \\t bar\n---\n\nand the following after that test:\n\n---\n[section \"sub=section\"]\n val1 = foo=bar\n val2 = foo\\nbar\n val3 = \\n\\n\n val4 =\n val5\n[section]\n val = foo \\t bar\n[alias]\n split-cmdline-fix = echo \\\"\n[branch \"main\"]\n mergeoptions = echo \\\"\n---\n\nSo it doesn't look like it overwrites anything. To me it just adds\nstuff at the end of the file.\n\n> > > Refactor tests which access the refdb to be more robust by using their\n> > > own separate repositories, which allows us to be more careful and not\n> > > discard required extensions.\n> >\n> > Not sure what exactly is discarding extensions. Also robust is not\n> > very clear. It would be better to give at least an example of how\n> > things could fail.\n>\n> Hm. I don't really know how to phrase this better. The preceding\n> paragraph already explains why we're discarding the extension and what\n> the consequence is. I added a sentence saying \", which will cause\n> failures when trying to access any refs.\"\n\nTo me the preceding paragraph said that we are overwriting the config\nfile, but I just don't see how for example the above test overwrites\nanything. So maybe I am missing something obvious, or maybe you don't\nquite mean \"overwrite\", but I don't see how the extension would be\ndiscarded by the test which only seems to add stuff.\n"},{"id":"487497","messageId":"ZbeCw6DgIp3h5gMN@tanuki","threadId":"60704","inReplyTo":"CAP8UFD2ZoV74jx3B_adwYENGtp9fCH_oVjW4QJXTZQJ0=_aeEA@mail.gmail.com","subject":"Re: [PATCH v2 1/6] t1300: make tests more robust with non-default ref backends","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-29T10:49:39Z","receivedAt":"2024-01-29T10:49:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jan 29, 2024 at 11:32:53AM +0100, Christian Couder wrote:\n> On Wed, Jan 24, 2024 at 9:52 AM Patrick Steinhardt wrote:\n> > On Tue, Jan 23, 2024 at 05:15:48PM +0100, Christian Couder wrote:\n> > > On Wed, Jan 10, 2024 at 10:01 AM Patrick Steinhardt wrote:\n[snip]\n> > Hm. I don't really know how to phrase this better. The preceding\n> > paragraph already explains why we're discarding the extension and what\n> > the consequence is. I added a sentence saying \", which will cause\n> > failures when trying to access any refs.\"\n> \n> To me the preceding paragraph said that we are overwriting the config\n> file, but I just don't see how for example the above test overwrites\n> anything. So maybe I am missing something obvious, or maybe you don't\n> quite mean \"overwrite\", but I don't see how the extension would be\n> discarded by the test which only seems to add stuff.\n\nIt happens before already, outside of any tests. See line 1036:\n\n```\ncat > .git/config <<\\EOF\n[section \"sub=section\"]\n\tval1 = foo=bar\n\tval2 = foo\\nbar\n\tval3 = \\n\\n\n\tval4 =\n\tval5\nEOF\n```\n\nOverall, I agree that this is rather hard to discover and that the tests\nreally could require a bigger refactoring to make them more independent\nof each other.\n\nI'll send another version that mentions this explicitly.\n\nPatrick\n"},{"id":"487499","messageId":"cover.1706525813.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1704802213.git.ps@pks.im","subject":"[PATCH v4 0/6] t: mark \"files\"-backend specific tests","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-29T11:07:13Z","receivedAt":"2024-01-29T11:07:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the fourth version of my patch series that addresses tests which\nare specific to the \"files\" backend. There is only a single change\ncompared to v3, which is an improved commit message for the first patch.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (6):\n t1300: make tests more robust with non-default ref backends\n t1301: mark test for `core.sharedRepository` as reffiles specific\n t1302: make tests more robust with new extensions\n t1419: mark test suite as files-backend specific\n t5526: break test submodule differently\n t: mark tests regarding git-pack-refs(1) to be backend specific\n\n t/t1300-config.sh | 78 ++++++++++++++++++++++-------------\n t/t1301-shared-repo.sh | 2 +-\n t/t1302-repo-version.sh | 23 +++++++----\n t/t1409-avoid-packing-refs.sh | 6 +++\n t/t1419-exclude-refs.sh | 6 +++\n t/t3210-pack-refs.sh | 6 +++\n t/t5526-fetch-submodules.sh | 2 +-\n 7 files changed, 85 insertions(+), 38 deletions(-)\n\nRange-diff against v3:\n1: a57e57a7c3 ! 1: 80a74bbb56 t1300: make tests more robust with non-default ref backends\n @@ Commit message\n t1300: make tests more robust with non-default ref backends\n \n The t1300 test suite exercises the git-config(1) tool. To do so, the\n - test overwrites \".git/config\" to contain custom contents. While this is\n - easy enough to do, it may create problems when using a non-default\n - repository format because this causes us to overwrite the repository\n - format version as well as any potential extensions. With the upcoming\n - \"reftable\" ref backend the result is that Git would try to access refs\n - via the \"files\" backend even though the repository has been initialized\n - with the \"reftable\" backend, which will cause failures when trying to\n - access any refs.\n + test overwrites \".git/config\" to contain custom contents in several\n + places with code like the following:\n \n - Refactor tests which access the refdb to be more robust by using their\n - own separate repositories, which allows us to be more careful and not\n - discard required extensions.\n + ```\n + cat > .git/config <<\\EOF\n + ...\n + EOF\n + ```\n +\n + While this is easy enough to do, it may create problems when using a\n + non-default repository format because this causes us to overwrite the\n + repository format version as well as any potential extensions. With the\n + upcoming \"reftable\" ref backend the result is that Git would try to\n + access refs via the \"files\" backend even though the repository has been\n + initialized with the \"reftable\" backend, which will cause failures when\n + trying to access any refs.\n +\n + Ideally, we would rewrite the whole test suite to not depend on state\n + written by previous tests, but that would result in a lot of changes in\n + this test suite. Instead, we only refactor tests which access the refdb\n + to be more robust by using their own separate repositories, which allows\n + us to be more careful and not discard required extensions.\n \n Note that we also have to touch up how the CUSTOM_CONFIG_FILE gets\n accessed. This environment variable contains the relative path to a\n2: fd6dd92c23 = 2: 4359d3ffa8 t1301: mark test for `core.sharedRepository` as reffiles specific\n3: ec90320ff1 = 3: b72d85df60 t1302: make tests more robust with new extensions\n4: d0d70c3f18 = 4: 1faa8687ae t1419: mark test suite as files-backend specific\n5: 066c297189 = 5: 4b95277e20 t5526: break test submodule differently\n6: 7b8921817b = 6: 53aea8236d t: mark tests regarding git-pack-refs(1) to be backend specific\n\nbase-commit: b50a608ba20348cb3dfc16a696816d51780e3f0f\n-- \n2.43.GIT\n\n"},{"id":"487500","messageId":"80a74bbb567de165a8dadf0664167140e4bf0504.1706525813.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1706525813.git.ps@pks.im","subject":"[PATCH v4 1/6] t1300: make tests more robust with non-default ref backends","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-29T11:07:17Z","receivedAt":"2024-01-29T11:07:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The t1300 test suite exercises the git-config(1) tool. To do so, the\ntest overwrites \".git/config\" to contain custom contents in several\nplaces with code like the following:\n\n```\ncat > .git/config <<\\EOF\n...\nEOF\n```\n\nWhile this is easy enough to do, it may create problems when using a\nnon-default repository format because this causes us to overwrite the\nrepository format version as well as any potential extensions. With the\nupcoming \"reftable\" ref backend the result is that Git would try to\naccess refs via the \"files\" backend even though the repository has been\ninitialized with the \"reftable\" backend, which will cause failures when\ntrying to access any refs.\n\nIdeally, we would rewrite the whole test suite to not depend on state\nwritten by previous tests, but that would result in a lot of changes in\nthis test suite. Instead, we only refactor tests which access the refdb\nto be more robust by using their own separate repositories, which allows\nus to be more careful and not discard required extensions.\n\nNote that we also have to touch up how the CUSTOM_CONFIG_FILE gets\naccessed. This environment variable contains the relative path to a\ncustom config file which we're setting up. But because we are now using\nsubrepositories, this relative path will not be found anymore because\nour working directory changes. This issue is addressed by storing the\nabsolute path to the file in CUSTOM_CONFIG_FILE instead.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1300-config.sh | 78 ++++++++++++++++++++++++++++++-----------------\n 1 file changed, 50 insertions(+), 28 deletions(-)\n\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex f4e2752134..31c3878687 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1098,15 +1098,20 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '\n \ttest_must_fail git config --file=linktolinktonada --list\n '\n \n-test_expect_success 'check split_cmdline return' \"\n-\tgit config alias.split-cmdline-fix 'echo \\\"' &&\n-\ttest_must_fail git split-cmdline-fix &&\n-\techo foo > foo &&\n-\tgit add foo &&\n-\tgit commit -m 'initial commit' &&\n-\tgit config branch.main.mergeoptions 'echo \\\"' &&\n-\ttest_must_fail git merge main\n-\"\n+test_expect_success 'check split_cmdline return' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit config alias.split-cmdline-fix \"echo \\\"\" &&\n+\t\ttest_must_fail git split-cmdline-fix &&\n+\t\techo foo >foo &&\n+\t\tgit add foo &&\n+\t\tgit commit -m \"initial commit\" &&\n+\t\tgit config branch.main.mergeoptions \"echo \\\"\" &&\n+\t\ttest_must_fail git merge main\n+\t)\n+'\n \n test_expect_success 'git -c \"key=value\" support' '\n \tcat >expect <<-\\EOF &&\n@@ -1157,10 +1162,16 @@ test_expect_success 'git -c works with aliases of builtins' '\n '\n \n test_expect_success 'aliases can be CamelCased' '\n-\ttest_config alias.CamelCased \"rev-parse HEAD\" &&\n-\tgit CamelCased >out &&\n-\tgit rev-parse HEAD >expect &&\n-\ttest_cmp expect out\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A &&\n+\t\tgit config alias.CamelCased \"rev-parse HEAD\" &&\n+\t\tgit CamelCased >out &&\n+\t\tgit rev-parse HEAD >expect &&\n+\t\ttest_cmp expect out\n+\t)\n '\n \n test_expect_success 'git -c does not split values on equals' '\n@@ -2009,11 +2020,11 @@ test_expect_success '--show-origin getting a single key' '\n '\n \n test_expect_success 'set up custom config file' '\n-\tCUSTOM_CONFIG_FILE=\"custom.conf\" &&\n-\tcat >\"$CUSTOM_CONFIG_FILE\" <<-\\EOF\n+\tcat >\"custom.conf\" <<-\\EOF &&\n \t[user]\n \t\tcustom = true\n \tEOF\n+\tCUSTOM_CONFIG_FILE=\"$(test-tool path-utils real_path custom.conf)\"\n '\n \n test_expect_success !MINGW 'set up custom config file with special name characters' '\n@@ -2052,22 +2063,33 @@ test_expect_success '--show-origin stdin with file include' '\n '\n \n test_expect_success '--show-origin blob' '\n-\tblob=$(git hash-object -w \"$CUSTOM_CONFIG_FILE\") &&\n-\tcat >expect <<-EOF &&\n-\tblob:$blob\tuser.custom=true\n-\tEOF\n-\tgit config --blob=$blob --show-origin --list >output &&\n-\ttest_cmp expect output\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tblob=$(git hash-object -w \"$CUSTOM_CONFIG_FILE\") &&\n+\t\tcat >expect <<-EOF &&\n+\t\tblob:$blob\tuser.custom=true\n+\t\tEOF\n+\t\tgit config --blob=$blob --show-origin --list >output &&\n+\t\ttest_cmp expect output\n+\t)\n '\n \n test_expect_success '--show-origin blob ref' '\n-\tcat >expect <<-\\EOF &&\n-\tblob:main:custom.conf\tuser.custom=true\n-\tEOF\n-\tgit add \"$CUSTOM_CONFIG_FILE\" &&\n-\tgit commit -m \"new config file\" &&\n-\tgit config --blob=main:\"$CUSTOM_CONFIG_FILE\" --show-origin --list >output &&\n-\ttest_cmp expect output\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tcat >expect <<-\\EOF &&\n+\t\tblob:main:custom.conf\tuser.custom=true\n+\t\tEOF\n+\t\tcp \"$CUSTOM_CONFIG_FILE\" custom.conf &&\n+\t\tgit add custom.conf &&\n+\t\tgit commit -m \"new config file\" &&\n+\t\tgit config --blob=main:custom.conf --show-origin --list >output &&\n+\t\ttest_cmp expect output\n+\t)\n '\n \n test_expect_success '--show-origin with --default' '\n-- \n2.43.GIT\n\n"},{"id":"487501","messageId":"4359d3ffa816541cb68212714fa5d08e51138203.1706525813.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1706525813.git.ps@pks.im","subject":"[PATCH v4 2/6] t1301: mark test for `core.sharedRepository` as reffiles specific","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-29T11:07:22Z","receivedAt":"2024-01-29T11:07:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In t1301 we verify whether reflog files written by the \"files\" ref\nbackend correctly honor permissions when \"core.sharedRepository\" is set.\nThe test logic is thus specific to the reffiles backend and will not\nwork with any other backends.\n\nMark the test accordingly with the REFFILES prereq.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1301-shared-repo.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex e5a0d65caa..8e2c01e760 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -137,7 +137,7 @@ test_expect_success POSIXPERM 'info/refs respects umask in unshared repo' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '\n+test_expect_success REFFILES,POSIXPERM 'git reflog expire honors core.sharedRepository' '\n \tumask 077 &&\n \tgit config core.sharedRepository group &&\n \tgit reflog expire --all &&\n-- \n2.43.GIT\n\n"},{"id":"487502","messageId":"b72d85df60b1f92e427e31b44cf08c1bf025e11f.1706525813.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1706525813.git.ps@pks.im","subject":"[PATCH v4 3/6] t1302: make tests more robust with new extensions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-29T11:07:26Z","receivedAt":"2024-01-29T11:07:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In t1302 we exercise logic around \"core.repositoryFormatVersion\" and\nextensions. These tests are not particularly robust against extensions\nlike the newly introduced \"refStorage\" extension as we tend to clobber\nthe repository's config file. We thus overwrite any extensions that were\nset, which may render the repository inaccessible in case it has to be\naccessed with a non-default ref storage.\n\nRefactor the tests to be more robust:\n\n - Check the DEFAULT_REPO_FORMAT prereq to determine the expected\n repository format version. This helps to ensure that we only need to\n update the prereq in a central place when new extensions are added.\n Furthermore, this allows us to stop seeding the now-unneeded object\n ID cache that was only used to figure out the repository version.\n\n - Use a separate repository to rewrite \".git/config\" to test\n combinations of the repository format version and extensions. This\n ensures that we don't break the main test repository. While we could\n rewrite these tests to not overwrite preexisting extensions, it\n feels cleaner like this so that we can test extensions standalone\n without interference from the environment.\n\n - Do not rewrite \".git/config\" when exercising the \"preciousObjects\"\n extension.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1302-repo-version.sh | 23 +++++++++++++++--------\n 1 file changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh\nindex 179474fa65..42caa0d297 100755\n--- a/t/t1302-repo-version.sh\n+++ b/t/t1302-repo-version.sh\n@@ -9,10 +9,6 @@ TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n-\ttest_oid_cache <<-\\EOF &&\n-\tversion sha1:0\n-\tversion sha256:1\n-\tEOF\n \tcat >test.patch <<-\\EOF &&\n \tdiff --git a/test.txt b/test.txt\n \tnew file mode 100644\n@@ -28,7 +24,12 @@ test_expect_success 'setup' '\n '\n \n test_expect_success 'gitdir selection on normal repos' '\n-\ttest_oid version >expect &&\n+\tif test_have_prereq DEFAULT_REPO_FORMAT\n+\tthen\n+\t\techo 0\n+\telse\n+\t\techo 1\n+\tfi >expect &&\n \tgit config core.repositoryformatversion >actual &&\n \tgit -C test config core.repositoryformatversion >actual2 &&\n \ttest_cmp expect actual &&\n@@ -79,8 +80,13 @@ mkconfig () {\n \n while read outcome version extensions; do\n \ttest_expect_success \"$outcome version=$version $extensions\" \"\n-\t\tmkconfig $version $extensions >.git/config &&\n-\t\tcheck_${outcome}\n+\t\ttest_when_finished 'rm -rf extensions' &&\n+\t\tgit init extensions &&\n+\t\t(\n+\t\t\tcd extensions &&\n+\t\t\tmkconfig $version $extensions >.git/config &&\n+\t\t\tcheck_${outcome}\n+\t\t)\n \t\"\n done <<\\EOF\n allow 0\n@@ -94,7 +100,8 @@ allow 1 noop-v1\n EOF\n \n test_expect_success 'precious-objects allowed' '\n-\tmkconfig 1 preciousObjects >.git/config &&\n+\tgit config core.repositoryFormatVersion 1 &&\n+\tgit config extensions.preciousObjects 1 &&\n \tcheck_allow\n '\n \n-- \n2.43.GIT\n\n"},{"id":"487503","messageId":"1faa8687ae201a426e603cd456534775e314108e.1706525813.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1706525813.git.ps@pks.im","subject":"[PATCH v4 4/6] t1419: mark test suite as files-backend specific","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-29T11:07:30Z","receivedAt":"2024-01-29T11:07:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"With 59c35fac54 (refs/packed-backend.c: implement jump lists to avoid\nexcluded pattern(s), 2023-07-10) we have implemented logic to handle\nexcluded refs more efficiently in the \"packed\" ref backend. This logic\nallows us to skip emitting refs completely which we know to not be of\nany interest to the caller, which can avoid quite some allocations and\nobject lookups.\n\nThis was wired up via a new `exclude_patterns` parameter passed to the\nbackend's ref iterator. The backend only needs to handle them on a best\neffort basis though, and in fact we only handle it for the \"packed-refs\"\nfile, but not for loose references. Consequently, all callers must still\nfilter emitted refs with those exclude patterns.\n\nThe result is that handling exclude patterns is completely optional in\nthe ref backend, and any future backends may or may not implement it.\nLet's thus mark the test for t1419 to depend on the REFFILES prereq.\n\nAn alternative would be to introduce a new prereq that tells us whether\nthe backend under test supports exclude patterns or not. But this does\nfeel a bit overblown:\n\n - It would either map to the REFFILES prereq, in which case it feels\n overengineered because the prereq is only ever relevant to t1419.\n\n - Otherwise, it could auto-detect whether the backend supports exclude\n patterns. But this could lead to silent failures in case the support\n for this feature breaks at any point in time.\n\nIt should thus be good enough to just use the REFFILES prereq for now.\nIf future backends ever grow support for exclude patterns we can easily\nadd their respective prereq as another condition for this test suite to\nexecute.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1419-exclude-refs.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t1419-exclude-refs.sh b/t/t1419-exclude-refs.sh\nindex 5d8c86b657..1359574419 100755\n--- a/t/t1419-exclude-refs.sh\n+++ b/t/t1419-exclude-refs.sh\n@@ -8,6 +8,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n+if test_have_prereq !REFFILES\n+then\n+\tskip_all='skipping `git for-each-ref --exclude` tests; need files backend'\n+\ttest_done\n+fi\n+\n for_each_ref__exclude () {\n \tGIT_TRACE2_PERF=1 test-tool ref-store main \\\n \t\tfor-each-ref--exclude \"$@\" >actual.raw\n-- \n2.43.GIT\n\n"},{"id":"487504","messageId":"4b95277e2004ed2c60274ec99cee547129bd1eda.1706525813.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1706525813.git.ps@pks.im","subject":"[PATCH v4 5/6] t5526: break test submodule differently","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-29T11:07:34Z","receivedAt":"2024-01-29T11:07:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 10f5c52656 (submodule: avoid auto-discovery in\nprepare_submodule_repo_env(), 2016-09-01) we fixed a bug when doing a\nrecursive fetch with submodule in the case where the submodule is broken\ndue to whatever reason. The test to exercise that the fix works breaks\nthe submodule by deleting its `HEAD` reference, which will cause us to\nnot detect the directory as a Git repository.\n\nWhile this is perfectly fine in theory, this way of breaking the repo\nbecomes problematic with the current efforts to introduce another refdb\nbackend into Git. The new reftable backend has a stub HEAD file that\nalways contains \"ref: refs/heads/.invalid\" so that tools continue to be\nable to detect such a repository. But as the reftable backend will never\ndelete this file even when asked to delete `HEAD` the current way to\ndelete the `HEAD` reference will stop working.\n\nAdapt the code to instead delete the objects database. Going back with\nthis new way to cause breakage confirms that it triggers the infinite\nrecursion just the same, and there are no equivalent ongoing efforts to\nreplace the object database with an alternate backend.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t5526-fetch-submodules.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 7ab220fa31..5e566205ba 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '\n \tgit -C dst fetch --recurse-submodules &&\n \n \t# Break the receiving submodule\n-\ttest-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&\n+\trm -r dst/sub/.git/objects &&\n \n \t# NOTE: without the fix the following tests will recurse forever!\n \t# They should terminate with an error.\n-- \n2.43.GIT\n\n"},{"id":"487505","messageId":"53aea8236dbc515951c25b27cde65d948523cde6.1706525813.git.ps@pks.im","threadId":"60704","inReplyTo":"cover.1706525813.git.ps@pks.im","subject":"[PATCH v4 6/6] t: mark tests regarding git-pack-refs(1) to be backend specific","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-29T11:07:38Z","receivedAt":"2024-01-29T11:07:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Both t1409 and t3210 exercise parts of git-pack-refs(1). Given that we\nmust check the on-disk files to verify whether the backend has indeed\npacked refs as expected those test suites are deeply tied to the actual\nbackend that is in use.\n\nMark the test suites to depend on the REFFILES backend.\n\nSigned-off-by: Patrick Steinhardt \n---\n t/t1409-avoid-packing-refs.sh | 6 ++++++\n t/t3210-pack-refs.sh | 6 ++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh\nindex f23c0152a8..7748973733 100755\n--- a/t/t1409-avoid-packing-refs.sh\n+++ b/t/t1409-avoid-packing-refs.sh\n@@ -5,6 +5,12 @@ test_description='avoid rewriting packed-refs unnecessarily'\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n+if test_have_prereq !REFFILES\n+then\n+ skip_all='skipping files-backend specific pack-refs tests'\n+ test_done\n+fi\n+\n # Add an identifying mark to the packed-refs file header line. This\n # shouldn't upset readers, and it should be omitted if the file is\n # ever rewritten.\ndiff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh\nindex 7f4e98db7d..c0f1f9cfb7 100755\n--- a/t/t3210-pack-refs.sh\n+++ b/t/t3210-pack-refs.sh\n@@ -15,6 +15,12 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n+if test_have_prereq !REFFILES\n+then\n+ skip_all='skipping files-backend specific pack-refs tests'\n+ test_done\n+fi\n+\n test_expect_success 'enable reflogs' '\n \tgit config core.logallrefupdates true\n '\n-- \n2.43.GIT\n\n"},{"id":"487516","messageId":"CAP8UFD1=yAhv5QKDCYPfTu0=qLCyRUGt3W9y1D9XGjvJbv8wLQ@mail.gmail.com","threadId":"60704","inReplyTo":"80a74bbb567de165a8dadf0664167140e4bf0504.1706525813.git.ps@pks.im","subject":"Re: [PATCH v4 1/6] t1300: make tests more robust with non-default ref backends","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-01-29T12:00:31Z","receivedAt":"2024-01-29T12:00:45Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jan 29, 2024 at 12:07 PM Patrick Steinhardt wrote:\n>\n> The t1300 test suite exercises the git-config(1) tool. To do so, the\n> test overwrites \".git/config\" to contain custom contents in several\n> places with code like the following:\n>\n> ```\n> cat > .git/config <<\\EOF\n> ...\n> EOF\n> ```\n\nI thought about using a function that would overwrite a config file\nsafely as it would still copy the repository format version and other\nextension information to the new config file, but a number of tests\neven do `rm .git/config`, so it wouldn't be enough.\n\n> While this is easy enough to do, it may create problems when using a\n> non-default repository format because this causes us to overwrite the\n> repository format version as well as any potential extensions. With the\n> upcoming \"reftable\" ref backend the result is that Git would try to\n> access refs via the \"files\" backend even though the repository has been\n> initialized with the \"reftable\" backend, which will cause failures when\n> trying to access any refs.\n>\n> Ideally, we would rewrite the whole test suite to not depend on state\n> written by previous tests, but that would result in a lot of changes in\n> this test suite.\n\nI agree that the whole test script would need significant work.\n\n> Instead, we only refactor tests which access the refdb\n> to be more robust by using their own separate repositories, which allows\n> us to be more careful and not discard required extensions.\n>\n> Note that we also have to touch up how the CUSTOM_CONFIG_FILE gets\n> accessed. This environment variable contains the relative path to a\n> custom config file which we're setting up. But because we are now using\n> subrepositories, this relative path will not be found anymore because\n> our working directory changes. This issue is addressed by storing the\n> absolute path to the file in CUSTOM_CONFIG_FILE instead.\n"},{"id":"487517","messageId":"CAP8UFD3xWW-WvLPnHNVyJbbXDvs85TTq-wVVZ-qnJLSdqtR4bg@mail.gmail.com","threadId":"60704","inReplyTo":"cover.1706525813.git.ps@pks.im","subject":"Re: [PATCH v4 0/6] t: mark \"files\"-backend specific tests","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-01-29T12:03:13Z","receivedAt":"2024-01-29T12:03:27Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jan 29, 2024 at 12:07 PM Patrick Steinhardt wrote:\n>\n> Hi,\n>\n> this is the fourth version of my patch series that addresses tests which\n> are specific to the \"files\" backend. There is only a single change\n> compared to v3, which is an improved commit message for the first patch.\n\nI took another look at the patches in this and it looks good to me\nnow. Feel free to add my \"Reviewed-by:\"\n\nThanks!\n"},{"id":"487549","messageId":"xmqqttmvho46.fsf@gitster.g","threadId":"60704","inReplyTo":"CAP8UFD3xWW-WvLPnHNVyJbbXDvs85TTq-wVVZ-qnJLSdqtR4bg@mail.gmail.com","subject":"Re: [PATCH v4 0/6] t: mark \"files\"-backend specific tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-29T20:38:01Z","receivedAt":"2024-01-29T20:38:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Mon, Jan 29, 2024 at 12:07 PM Patrick Steinhardt wrote:\n>>\n>> Hi,\n>>\n>> this is the fourth version of my patch series that addresses tests which\n>> are specific to the \"files\" backend. There is only a single change\n>> compared to v3, which is an improved commit message for the first patch.\n>\n> I took another look at the patches in this and it looks good to me\n> now. Feel free to add my \"Reviewed-by:\"\n\nThanks.\n"}]}