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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 6, 2022, 03:23 UTC
Message-ID: <CAPig+cSfvgu8XjvmmAkFWe1G1VDRgrcx5GjUhr4xSDqoJ4cZOA@mail.gmail.com>
In-Reply-To: <221206.86ilipckms.gmgdl@evledraar.gmail.com>

On Mon, Dec 5, 2022 at 9:48 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 34 quoted lines

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

Show 14 quoted 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.
>
> 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.