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

From: Johannes Berg <johannes@sipsolutions.net>
Date: Aug 24, 2021, 07:50 UTC
Message-ID: <255fb1277db09f66e5cfddc6bbe34181effca3dc.camel@sipsolutions.net>
In-Reply-To: <YSQ7wVKbE2HTkEz0@nand.local>

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

Show 11 quoted lines

> 
> > +	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.

Show 11 quoted lines

> > +	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