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

4 messages, 3 authors, 2021-08-23 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2021-08-23 08:05:44

Johannes Berg [off-list ref] 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.
quoted
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 ;-)

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.

Thanks.

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

From: Johannes Berg <johannes@sipsolutions.net>
Date: 2021-08-23 08:10:24

On Mon, 2021-08-23 at 01:05 -0700, Junio C Hamano wrote:
Johannes Berg [off-list ref] writes:
quoted
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.
quoted
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, 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.


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.

johannes

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

From: Derrick Stolee <hidden>
Date: 2021-08-23 13:19:28

On 8/23/2021 4:10 AM, Johannes Berg wrote:
On Mon, 2021-08-23 at 01:05 -0700, Junio C Hamano wrote:
quoted
Johannes Berg [off-list ref] writes:
quoted
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.
quoted
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 ;-)
quoted
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
'
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

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

From: Johannes Berg <johannes@sipsolutions.net>
Date: 2021-08-23 13:40:16

On Mon, 2021-08-23 at 09:19 -0400, Derrick Stolee wrote:
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
'
Sure. I know how to do subshells :)

My point was that inside the subshell you cannot do test_path_is_file
and similar, because the subshell didn't import the libs.
quoted
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.
I was specifically asking Junio ;-)

But realistically, if this is the requirement you want to impose, then
you _cannot_ test for the segfault within git's test suite. Your loss.

johannes

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help