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

From: Johannes Berg <johannes@sipsolutions.net>
Date: Aug 23, 2021, 07:21 UTC
Message-ID: <4d65ef5b0a9e4104d763facc42d10a20557d054d.camel@sipsolutions.net>
In-Reply-To: <04ed58aa-94fa-010e-f4db-f41cd51876a5@gmail.com>

Hi Derrick,

Show 9 quoted lines

> > +test_expect_success 'multi-pack-index with --object-dir need not be in repo' '
> > +	p="$(pwd)" &&
> > +	rm -f $objdir/multi-pack-index &&
> > +	cd / &&
> > +	git multi-pack-index --object-dir="$p/$objdir" write &&
> > +	cd "$p"
> 
> Why are you using "cd /" here? 
>

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.

Could you instead init a new repo within the current directory and point the object-dir to that location?

I guess I could, but all the other stuff in here is already making a new repo in the current working dir, and already initializing it with objects, etc.

Doing it all over again seemed like a waste of time?

It could look something like this, (warning: I did not test this)

	git init other &&
	test_commit -C other first &&
	git multi-pack-index --object-dir=other/.git/objects write

Sure.

Actually, this won't work to test for the crash, I'd have to do something like

git init other test_commit -C other first && ( mkdir non-git && cd non-git && git multi-pack-index --object-dir=../other/.git/objects write )

or so.

And is the only post-condition you are checking that we do not crash?

Yes, I was assuming that it'd actually work at that point - maybe not the best assumption, it could (erroneously) exit with a 0 exit status but have done nothing.

> Or is there a specific result you are looking for? For

instance, we can double check that the MIDX was written:

	test_path_is_file other/.git/objects/pack/multi-pack-index

So I guess that would be a good idea.

but also you seem to be touching areas that delete files. Could we 'touch' some of those and then see them get deleted?

Ah, well, that's the underlying issue but I'm not sure we even ever get to that code? Then again, yes, the *.rev files should get removed, I'll see - not even sure I know how to get them to be generated in the first place, is that even supported already?

johannes