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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 6, 2022, 02:42 UTC
Message-ID: <221206.86ilipckms.gmgdl@evledraar.gmail.com>
In-Reply-To: <97ada2a1202190776ce3989d3841dd47e2702316.1668999621.git.gitgitgadget@gmail.com>

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

Show 34 quoted lines

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