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

[shejialuo <shejialuo@gmail.com>]

On Fri, Jan 17, 2025 at 05:01:21PM -0500, Eric Sunshine wrote:
> 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.

Thanks for above detailed explanation. I somehow understand why there
would be so many "repo/repo/repo" when I execute the test. I have
thought that `test_expect_success` command will make the environment of
each test totally independent. I will improve this in the next version.


Thanks,
Jialuo