fix t1509-root-work-tree failure

From: Eric Sunshine <[email protected]>

When 8959555cee (setup_git_directory(): add an owner check for the top-level directory, 2022-03-02) tightened security surrounding directory ownership, it neglected to adjust t1509-root-work-tree.sh to take the new restriction into account. As a result, since the root directory / is typically not owned by the user running the test (indeed, t1509 refuses to run as root), the ownership check added by 8959555cee kicks in and causes the test to fail:

fatal: detected dubious ownership in repository at '/' To add an exception for this directory, call:

git config --global --add safe.directory /

This problem went unnoticed for so long because t1509 is rarely run since it requires setting up a chroot environment or a sacrificial virtual machine in which / can be made writable and polluted by any user.

Signed-off-by Eric Sunshine <[email protected]>

---
 t/t1509-root-work-tree.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

t/t1509-root-work-tree.sh +2 -2

index 553a3f601ba..eb57fe7e19f 100755
@@ -221,7 +221,8 @@ test_expect_success 'setup' '
 	rm -rf /.git &&
 	echo "Initialized empty Git repository in /.git/" > expected &&
 	git init > result &&
-	test_cmp expected result
+	test_cmp expected result &&
+	git config --global --add safe.directory /
 '
 
 test_vars 'auto gitdir, root' ".git" "/" ""
--

gitgitgadget

From: Eric Sunshine <[email protected]>

One of the t1509 setup tests is very particular about the output it expects from git init, and fails if the output differs even slightly which can happen easily if the script is run multiple times since it doesn't do a good job of cleaning up after itself (i.e. it leaves detritus in the root directory /). One bit of cruft in particular (/HEAD) makes the test fail since its presence causes git init to alter its output; rather than reporting "Initialized empty Git repository", it instead reports "Reinitialized existing Git repository" when /HEAD is present. Address this problem by making the test do a more careful job of crafting its intended initial state.

Signed-off-by Eric Sunshine <[email protected]>

---
 t/t1509-root-work-tree.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t1509-root-work-tree.sh +1 -2

index eb57fe7e19f..d0417626280 100755
@@ -243,7 +243,7 @@ say "auto bare gitdir"
 # DESTROYYYYY!!!!!
 test_expect_success 'setup' '
 	rm -rf /refs /objects /info /hooks &&
-	rm -f /expected /ls.expected /me /result &&
+	rm -f /HEAD /expected /ls.expected /me /result &&
 	cd / &&
 	echo "Initialized empty Git repository in /" > expected &&
 	git init --bare > result &&
--

gitgitgadget

From: Eric Sunshine <[email protected]>

t1509-root-work-tree.sh, which tests behavior of a Git repository located at the root / directory, refuses to run if it detects the presence of an existing repository at /. This safeguard ensures that it won't clobber a legitimate repository at that location. However, because t1509 does a poor job of cleaning up after itself, it runs afoul of its own safety check on subsequent runs, which makes it painful to run the script repeatedly since each run requires manual cleanup of detritus from the previous run.

Address this shortcoming by making t1509 clean up after itself as its last action. This is safe since the script can only make it to this cleanup action if it did not find a legitimate repository at / in the first place, so the resources cleaned up here can only have been created by the script itself.

Signed-off-by Eric Sunshine <[email protected]>

---
 t/t1509-root-work-tree.sh | 5 +++++
 1 file changed, 5 insertions(+)

t/t1509-root-work-tree.sh +0 -0

index d0417626280..c799f5b6aca 100755

Quoted 17 lines · Eric Sunshine via GitGitGadget

t1509-root-work-tree.sh, which tests behavior of a Git repository located at the root / directory, refuses to run if it detects the presence of an existing repository at /. This safeguard ensures that it won't clobber a legitimate repository at that location. However, because t1509 does a poor job of cleaning up after itself, it runs afoul of its own safety check on subsequent runs, which makes it painful to run the script repeatedly since each run requires manual cleanup of detritus from the previous run.

Address this shortcoming by making t1509 clean up after itself as its last action. This is safe since the script can only make it to this cleanup action if it did not find a legitimate repository at / in the first place, so the resources cleaned up here can only have been created by the script itself.

Signed-off-by Eric Sunshine <[email protected]>

---

t/t1509-root-work-tree.sh test_expect_success 'go to /foo' 'cd /foo' · line 262

Quoted 4 lines

t/t1509-root-work-tree.sh +4 -0

+test_expect_success 'cleanup root' '
+     rm -rf /.git /refs /objects /info /hooks /branches /foo &&
+     rm -f /HEAD /config /description /expected /ls.expected /me /result
+'

Perhaps it would be nice to split this into a function in an earlier step, as this duplicates what you patched in 2/3. E.g.:

        cleanup_root_git_bare() {
                rm -rf /.git
        }
        cleanup_root_git() {
                rm -f /HEAD /config /description /expected /ls.expected /me /result
        }

Then all 3 resulting users could call some combination of those.

This is an existing wart, but I also wondered why the "expected", "result" etc. was needed. Either we could make the tests creating those do a "test_when_finished" removal of it, or better yet just create those in the trash directory.

At this point we've cd'd to /, but there doesn't seem to be a reason we couldn't use our original trash directory for our own state.

The "description" we could then git rid of with "git init --template=".

We could even get rid of the need to maintain "HEAD" etc. by init-ing a repo in the trash directory, copying its contents to "/", and then we'd know exactly what we needed to remove afterwards. I.e. just a mirror of the structure we copied from our just init-ed repo.

But all that's a digression for this series, which I think is good enough as-is. I just wondered why we had some of these odd looking patterns.

Quoted 10 lines · Ævar Arnfjörð Bjarmason

Quoted 9 lines · Eric Sunshine via GitGitGadget

t1509-root-work-tree.sh, which tests behavior of a Git repository located at the root / directory, refuses to run if it detects the presence of an existing repository at /. This safeguard ensures that it won't clobber a legitimate repository at that location. However, because t1509 does a poor job of cleaning up after itself, it runs afoul of its own safety check on subsequent runs, which makes it painful to run the script repeatedly since each run requires manual cleanup of detritus from the previous run.

Quoted 5 lines

Quoted 5 lines

Address this shortcoming by making t1509 clean up after itself as its last action. This is safe since the script can only make it to this cleanup action if it did not find a legitimate repository at / in the first place, so the resources cleaned up here can only have been created by the script itself.

Quoted 4 lines

This is an existing wart, but I also wondered why the "expected", "result" etc. was needed. Either we could make the tests creating those do a "test_when_finished" removal of it, or better yet just create those in the trash directory.

Quoted 7 lines · Eric Sunshine

Quoted 6 lines · Ævar Arnfjörð Bjarmason

On Mon, Nov 21 2022, Eric Sunshine via GitGitGadget wrote:
[...]
This is an existing wart, but I also wondered why the "expected", "result" etc. was needed. Either we could make the tests creating those do a "test_when_finished" removal of it, or better yet just create those in the trash directory.

An even better suggestion would be to use test_atexit, of course.

On failure we'll skip the cleanup for the current test that just failed, but we're not distracted by scratch files from earlier tests, those would have already been cleaned up if they used the same "test_when_finished" pattern.

The t1509-root-work-tree script started failing earlier this year but went unnoticed because the script is rarely run since it requires setting up a chroot environment or a sacrificial virtual machine. This patch series fixes the failure and makes it a bit easier to run the script repeatedly without it tripping over itself.

Eric Sunshine (3):
t1509: fix failing "root work tree" test due to owner-check
t1509: make "setup" test more robust
t1509: facilitate repeated script invocations

Quoted 6 lines · Eric Sunshine via GitGitGadget

The t1509-root-work-tree script started failing earlier this year but went unnoticed because the script is rarely run since it requires setting up a chroot environment or a sacrificial virtual machine. This patch series fixes the failure and makes it a bit easier to run the script repeatedly without it tripping over itself.

Quoted 4 lines

Eric Sunshine (3):
t1509: fix failing "root work tree" test due to owner-check
t1509: make "setup" test more robust
t1509: facilitate repeated script invocations

Ping?

Quoted 8 lines · Eric Sunshine

Quoted 5 lines · Eric Sunshine via GitGitGadget

The t1509-root-work-tree script started failing earlier this year but went unnoticed because the script is rarely run since it requires setting up a chroot environment or a sacrificial virtual machine. This patch series fixes the failure and makes it a bit easier to run the script repeatedly without it tripping over itself.

Quoted 1 line

Ping?

Thank you for the ping. I did not have much time for the Git mailing list as of late (too much Git for Windows stuff going on).

The patch series looks good to me, with or without the test_atexit change I suggested.