[3/3] t1509: facilitate repeated script invocations

Commit Message

Eric Sunshine Nov. 21, 2022, 3 a.m. UTC

From: Eric Sunshine <[e-mail]>

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 <[e-mail]>
---
 t/t1509-root-work-tree.sh | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Ævar Arnfjörð Bjarmason Dec. 6, 2022, 2:42 a.m. UTC | #1

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

> From: Eric Sunshine <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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.

Eric Sunshine Dec. 6, 2022, 3:23 a.m. UTC | #2

On Mon, Dec 5, 2022 at 9:48 PM Ævar Arnfjörð Bjarmason <[e-mail]> 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 <[e-mail]>
> > ---
> > +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.

Johannes Schindelin Dec. 8, 2022, 12:04 p.m. UTC | #3

Hi,

On Mon, 5 Dec 2022, Eric Sunshine wrote:

> On Mon, Dec 5, 2022 at 9:48 PM Ævar Arnfjörð Bjarmason <[e-mail]> 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

Ævar Arnfjörð Bjarmason Dec. 8, 2022, 1:14 p.m. UTC | #4

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 <[e-mail]> 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".

Junio C Hamano Dec. 9, 2022, 4:46 a.m. UTC | #5

Ævar Arnfjörð Bjarmason <[e-mail]> 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.

Patch

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