[PATCH 0/3] fix t1509-root-work-tree failure

[PATCH 0/3] fix t1509-root-work-tree failure

[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.

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

 t/t1509-root-work-tree.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)


base-commit: a0789512c5a4ae7da935cd2e419f253cb3cb4ce7
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1425%2Fsunshineco%2Ft1509fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1425/sunshineco/t1509fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1425
-- 
gitgitgadget

[PATCH 1/3] t1509: fix failing "root work tree" test due to owner-check

[Eric Sunshine via GitGitGadget]

From: Eric Sunshine <sunshine@sunshineco.com>

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 <sunshine@sunshineco.com>
---
 t/t1509-root-work-tree.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t1509-root-work-tree.sh b/t/t1509-root-work-tree.sh
index 553a3f601ba..eb57fe7e19f 100755
--- a/t/t1509-root-work-tree.sh
+++ b/t/t1509-root-work-tree.sh
@@ -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

Re: [PATCH 1/3] t1509: fix failing "root work tree" test due to owner-check

[Johannes Schindelin]

Hi Eric,

On Mon, 21 Nov 2022, Eric Sunshine via GitGitGadget wrote:

> From: Eric Sunshine <sunshine@sunshineco.com>
>
> 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.

ACK, this is the right thing to do.

Thanks for working on it,
Johannes

>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  t/t1509-root-work-tree.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/t1509-root-work-tree.sh b/t/t1509-root-work-tree.sh
> index 553a3f601ba..eb57fe7e19f 100755
> --- a/t/t1509-root-work-tree.sh
> +++ b/t/t1509-root-work-tree.sh
> @@ -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
>
>

[PATCH 2/3] t1509: make "setup" test more robust

[Eric Sunshine via GitGitGadget]

From: Eric Sunshine <sunshine@sunshineco.com>

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 <sunshine@sunshineco.com>
---
 t/t1509-root-work-tree.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1509-root-work-tree.sh b/t/t1509-root-work-tree.sh
index eb57fe7e19f..d0417626280 100755
--- a/t/t1509-root-work-tree.sh
+++ b/t/t1509-root-work-tree.sh
@@ -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

Re: [PATCH 2/3] t1509: make "setup" test more robust

[Johannes Schindelin]

Hi Eric,

On Mon, 21 Nov 2022, Eric Sunshine via GitGitGadget wrote:

> From: Eric Sunshine <sunshine@sunshineco.com>
>
> 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.

Good explanation, and the patch is obviously correct.

ACK,
Johannes

>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  t/t1509-root-work-tree.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t1509-root-work-tree.sh b/t/t1509-root-work-tree.sh
> index eb57fe7e19f..d0417626280 100755
> --- a/t/t1509-root-work-tree.sh
> +++ b/t/t1509-root-work-tree.sh
> @@ -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
>
>

[PATCH 3/3] t1509: facilitate repeated script invocations

[Eric Sunshine via GitGitGadget]

From: Eric Sunshine <sunshine@sunshineco.com>

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 <sunshine@sunshineco.com>
---
 t/t1509-root-work-tree.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/t1509-root-work-tree.sh b/t/t1509-root-work-tree.sh
index d0417626280..c799f5b6aca 100755
--- a/t/t1509-root-work-tree.sh
+++ b/t/t1509-root-work-tree.sh
@@ -256,4 +256,9 @@ test_expect_success 'go to /foo' 'cd /foo'
 
 test_vars 'auto gitdir, root' "/" "" ""
 
+test_expect_success 'cleanup root' '
+	rm -rf /.git /refs /objects /info /hooks /branches /foo &&
+	rm -f /HEAD /config /description /expected /ls.expected /me /result
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH 3/3] t1509: facilitate repeated script invocations

[Ævar Arnfjörð Bjarmason]

On Mon, Nov 21 2022, Eric Sunshine via GitGitGadget wrote:

> From: Eric Sunshine <sunshine@sunshineco.com>
>
> 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 <sunshine@sunshineco.com>
> ---
>  t/t1509-root-work-tree.sh | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/t/t1509-root-work-tree.sh b/t/t1509-root-work-tree.sh
> index d0417626280..c799f5b6aca 100755
> --- a/t/t1509-root-work-tree.sh
> +++ b/t/t1509-root-work-tree.sh
> @@ -256,4 +256,9 @@ test_expect_success 'go to /foo' 'cd /foo'
>  
>  test_vars 'auto gitdir, root' "/" "" ""
>  
> +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.

Re: [PATCH 3/3] t1509: facilitate repeated script invocations

[Eric Sunshine]

On Mon, Dec 5, 2022 at 9:48 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Mon, Nov 21 2022, Eric Sunshine via GitGitGadget wrote:
> > 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 <sunshine@sunshineco.com>
> > ---
> > +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.

I did something like that originally but decided against it in the
end, and went with the simpler "just clean up everything we created"
despite the bit of duplicated cleanup code. After all, this is only a
tiny bit of duplication in a script filled with much worse: for
instance, the `test_foobar_root`, `test_foobar_foo`, and
`test_foobar_foobar` functions are filled with copy/paste code -- not
to mention having rather poor names. So, considering that the script
is probably in need of a major overhaul and modernization at some
point anyhow[1], and because I simply wanted to get the script back
into a working state, I opted for minimal changes.

[1]: That's assuming anyone even cares enough to clean this script up.
It's clearly neglected; the breakage addressed by this series has gone
unnoticed for many months.

> 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.

Fodder for an eventual overhaul, I suppose.

> 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.

Thanks for reading through the patches.

Re: [PATCH 3/3] t1509: facilitate repeated script invocations

[Johannes Schindelin]

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

Hi,

On Mon, 5 Dec 2022, Eric Sunshine wrote:

> On Mon, Dec 5, 2022 at 9:48 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> > On Mon, Nov 21 2022, Eric Sunshine via GitGitGadget wrote:
> > > 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.

Makes sense.

> > 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.

Ciao,
Johannes

"test_atexit" v.s. "test_when_finished" (was: [PATCH 3/3] t1509: facilitate repeated script invocations)

[Ævar Arnfjörð Bjarmason]

On Thu, Dec 08 2022, Johannes Schindelin wrote:

> On Mon, 5 Dec 2022, Eric Sunshine wrote:
>
>> On Mon, Dec 5, 2022 at 9:48 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> > 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.

Why?

For assets that are only needed within a given test we prefer cleaning
them up with "test_when_finished", there's legitimate uses for
"test_atexit", but those are for global state.

In this case (and again, we're discussing the #leftoverbits if someone
wants to poke at this again) the tests in question could relatively
easily be changed to do the creation and cleanup of the files that are
"test_cmp"'d (or similar) within the lifetime of individual tests
("test_when_finished"), rather than the lifetime of the script
("test_atexit").

A good reason for why we do it way is that it has a nice interaction
with "--immediate --debug".

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.

If you use "test_atexit" to do that all subsequent tests need to deal
with the sum of your scratch files, until they're cleaned up in one big
operation at the end.

It not only makes that debugging case harder, but also to write tests,
as you'll need to contend with more unwanted global state in your test
playground the further down the test file you are.

So I think what you're recommending here is an anti-pattern for the
common case.

There *are* cases where we really do need the "global cleanup",
e.g. tests that spawn the apache httpd use "test_atexit" rather than
"test_when_finished", we don't want to have to start/stop the httpd for each test.

We should leave "test_atexit" for those sorts of cases, not routine
per-test scratch file creation.

I semi-regularly run into cases where a stale "httpd" is left running in
the background from such tests (and not after I kill -9'd a test), so I
suspect we also have tricky races in that are, that probably aren't
improved by "test_atexit".

Re: "test_atexit" v.s. "test_when_finished"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> 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.

Yup.

A big benefit of using test_when_finished is that the knowledge of
what cruft needs to be cleaned is isolated to the exact test piece
that would create the cruft.  Instead of test_when_finished, We
could use the other convention to clear what others may have left
behind to give yourself a clean slate, but that requires you to be
aware of what other tests that came before you did, which will
change over time and will add to the maintenance burden.  And to
some degree, the same downside is shared by the approach to use
test_atexit.

Re: [PATCH 0/3] fix t1509-root-work-tree failure

[Eric Sunshine]

On Sun, Nov 20, 2022 at 10:00 PM Eric Sunshine via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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

Ping?

Re: [PATCH 0/3] fix t1509-root-work-tree failure

[Johannes Schindelin]

Hi Eric,

On Mon, 5 Dec 2022, Eric Sunshine wrote:

> On Sun, Nov 20, 2022 at 10:00 PM Eric Sunshine via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > 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
>
> 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.

Thank you,
Johannes

Re: [PATCH 0/3] fix t1509-root-work-tree failure

[Eric Sunshine]

On Thu, Dec 8, 2022 at 7:10 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Mon, 5 Dec 2022, Eric Sunshine wrote:
> > On Sun, Nov 20, 2022 at 10:00 PM Eric Sunshine via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > > 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.
> >
> > 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.

Thanks for looking over the series. Taking downstream discussion[1][2]
into account regarding test_atexit(), I think I'll let the series
stand as-is.

In the long run, t1509 may need an overhaul, but the current series is
intended to be the minimal possible change (patch [1/3]) to get the
script working again, plus very minor "fixes" (patches [2/3] and
[3/3]) to make it a bit friendlier for the next person who has to
debug a failure in the script.

[1]: https://lore.kernel.org/git/221208.86fsdq6nci.gmgdl@evledraar.gmail.com/
[2]: https://lore.kernel.org/git/xmqq5yel18xf.fsf@gitster.g/