On Mon, 2021-08-23 at 20:22 -0400, Taylor Blau wrote: > > > + 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. > > + 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
