Re: [PATCH] multi-pack-index: fix --object-dir from outside repo

From: Derrick Stolee <stolee@gmail.com>
Date: Aug 23, 2021, 13:19 UTC
Message-ID: <414ed641-2bd3-1316-8189-ad542988d091@gmail.com>
In-Reply-To: <caafaf945ec43ba606b054bf4c4faa42e35a8db1.camel@sipsolutions.net>

On 8/23/2021 4:10 AM, Johannes Berg wrote:

Show 23 quoted lines

> On Mon, 2021-08-23 at 01:05 -0700, Junio C Hamano wrote:
>> Johannes Berg <johannes@sipsolutions.net> writes:
>>
>>> I just needed to go outside the current test git directory, the tests
>>> are running in a way that the current working directory is already the
>>> git tree I'm operating in.
>>>
>>>> Even if you mean to use "cd",
>>>> please do so within a sub-shell.
>>>
>>> I thought about it, but clearly all the tests are run in a sub-shell, so
>>> it didn't seem necessary? But happy to change, I don't really care
>>> either way.
>>
>> Please learn to care before you write your next test, then ;-)
> 
> Hey now, I'm fixing your segfaults ;-)
> 
>> These tests are not run in a sub-shell; they are eval'ed, so that
>> the assignment they make to variables can persist and affect the
>> next test piece.
> 
> Makes sense. FWIW, the test *did* restore the CWD so things worked,

This assumes that your test completes to run the second "cd".

> and
> subshells are actually ugly (need to import test-lib-functions.sh again
> if you want to use those), but I'll make it work somehow.

We just add subshells this way:

test_expect_success 'test name' '
	prep_step &&
	(
		# now in a subshell
		cd wherever &&
		do things
		# don't need to cd again
	) &&
	continue test
'

Show 7 quoted lines

> More importantly, how do you feel about the "cd /"?
>
> The tests are always run in a place where there's a parent git folder
> (even if it's git itself), so you cannot reproduce the segfault in a
> test without the "cd /", though I guess "cd /tmp" would also work or
> something, but "cd /" felt pretty safe, hopefully not many people have
> "/.git" on their system.

Don't leave the directory your test is set up to run in.

Git has a very large test suite full of examples to use for inspiration. If you do not see a pattern used within the test suite, then there is probably good reason to avoid that pattern.

Thanks, -Stolee