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

Hi Derrick,

quoted

+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