Re: [PATCH 03/10] packed-backend: check whether the "packed-refs" is regular

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Jan 17, 2025 at 8:59 AM shejialuo <shejialuo@gmail.com> wrote:
> On Tue, Jan 07, 2025 at 08:33:56AM -0800, Karthik Nayak wrote:
> > shejialuo <shejialuo@gmail.com> writes:
> > > +test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' '
> > > +   test_when_finished "rm -rf repo" &&
> > > +   git init repo &&
> > > +   cd repo &&
> >
> > This should be in a subshell, so that at the end we can actually remove
> > the repo. This seems to be applicable to most of the other tests in this
> > file too. Perhaps, we should clean it up as a precursor commit to this
> > series?
>
> I have searched the usage of "test_when_finished", and I don't know why
> we need to use subshell. Could you please explain this further here.

Karthik may have been thinking about operating systems, such as
Microsoft Windows, which won't allow a directory to be deleted if that
directory is in use. In this case, because the test cd's into "repo"
and never cd's elsewhere, the directory is still in use when
test_when_finished() tries to delete "repo".

However, there is an even more important reason to use a subshell, and
that is because a subshell ensures that the current working directory
is effectively restored to the path which was current before the cd
command. This is important since it guarantees that subsequent tests
will be run in the correct directory even if the preceding test bombed
out part way through. For example:

    test_expect_success 'foo' '
        git init repo &&
        cd repo &&
        ...some more commands... &&
        cd ..
    '

If one of the commands in "...some more commands..." fails, then the
`cd ..` will never be reached, and the current working directory will
remain "repo" rather than reverting to the path prior to the cd
command. Thus, any tests which follow this one in the script will end
up running in the wrong directory. The proper way to protect against
this is:

    test_expect_success 'foo' '
        git init repo &&
        (
            cd repo &&
            ...some more commands...
        )
    '

Exiting the subshell will correctly restore the current working
directory to the original path _regardless_ of whether the test
succeeds or fails somewhere in "...some more commands...". Using a
subshell also means that you don't have to manually restore the
working directory via `cd ..` or similar.