On Fri, Jan 17, 2025 at 05:01:21PM -0500, Eric Sunshine wrote: > On Fri, Jan 17, 2025 at 8:59 AM shejialuo <shejialuo@xxxxxxxxx> wrote: > > On Tue, Jan 07, 2025 at 08:33:56AM -0800, Karthik Nayak wrote: > > > shejialuo <shejialuo@xxxxxxxxx> 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
