Re: [PATCH v3] multi-pack-index: fix *.rev cleanups with --object-dir

On Mon, 2021-08-23 at 20:22 -0400, Taylor Blau wrote:

quoted

+	rev="objdir-test-repo/$objdir/pack/multi-pack-index-abcdef123456.rev" &&
+	touch $rev &&

This is the only non-obvious part of the patch, but is necessary because
there's no way to trigger the MIDX code to write a reverse index
(thankfully so, since this means that we're not affecting anybody in the
wild cleaning up .rev's that we shouldn't be).

It may be worth returning to this in the future when we have support for
MIDX bitmaps (which will trigger writing a .rev file)

No argument there, though it doesn't matter much for this test how you
arrive at a repo that has a .rev file.

quoted

+	nongit git multi-pack-index --object-dir="$(pwd)/objdir-test-repo/$objdir" write &&
+	test_path_is_file objdir-test-repo/$objdir/pack/multi-pack-index &&
+	test_path_is_missing $rev

Makes sense. There's no point in testing that we ignore a .rev file in
the outer repository, since we're using nongit to trigger this bug.

But it may be worth adding an additional test which doesn't use nongit,
and instead invokes 'git multi-pack-index' from a Git repository, but
points at another repo's object directory. That should give us some
confidence that we're not deleting .rev files that we shouldn't.

Maybe you can just send that as a separate follow-up patch? :)

I'm not _entirely_ sure what you'd want to test, you could do at least
these things:

 * test like this that the correct file is deleted, from another repo
   instead of nongit
 * additionally arrange the *other* repo to have a .rev file and check
   that it's *not* deleted?

But to me all of the three (including my test) seem quite equivalent, at
least as long as we assume that the code won't grow a "try to delete all
the .rev files anywhere I can find" thing :)

johannes