mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Eric Sunshine <>
To: "Ævar Arnfjörð Bjarmason" <>
Cc: Eric Sunshine via GitGitGadget <>,,
	Johannes Schindelin <>
Subject: Re: [PATCH 3/3] t1509: facilitate repeated script invocations
Date: Mon, 5 Dec 2022 22:23:13 -0500	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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

  reply	other threads:[~2022-12-06  3:23 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-21  3:00 [PATCH 0/3] fix t1509-root-work-tree failure Eric Sunshine via GitGitGadget
2022-11-21  3:00 ` [PATCH 1/3] t1509: fix failing "root work tree" test due to owner-check Eric Sunshine via GitGitGadget
2022-12-08 11:49   ` Johannes Schindelin
2022-11-21  3:00 ` [PATCH 2/3] t1509: make "setup" test more robust Eric Sunshine via GitGitGadget
2022-12-08 11:49   ` Johannes Schindelin
2022-11-21  3:00 ` [PATCH 3/3] t1509: facilitate repeated script invocations Eric Sunshine via GitGitGadget
2022-12-06  2:42   ` Ævar Arnfjörð Bjarmason
2022-12-06  3:23     ` Eric Sunshine [this message]
2022-12-08 12:04       ` Johannes Schindelin
2022-12-08 13:14         ` "test_atexit" v.s. "test_when_finished" (was: [PATCH 3/3] t1509: facilitate repeated script invocations) Ævar Arnfjörð Bjarmason
2022-12-09  4:46           ` "test_atexit" v.s. "test_when_finished" Junio C Hamano
2022-12-05 18:21 ` [PATCH 0/3] fix t1509-root-work-tree failure Eric Sunshine
2022-12-08 12:10   ` Johannes Schindelin
2022-12-09  4:59     ` Eric Sunshine

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).