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

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2746 bytes --]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

I didn't know this either. I was mostly talking about what you mentioned
below.

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

This is was a super nice explanation compared to my single sentence.
Thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]