On Mon, Aug 23, 2021 at 07:10:11PM +0200, Johannes Berg wrote: > If using --object-dir to point into a repo while the current > working dir is outside, such as > > git init /repo > git -C /repo ... # add some objects > cd /non-repo > git multi-pack-index --object-dir /repo/.git/objects/ write > > the binary will segfault trying to access the object-dir via > the repo it found, but that's not fully initialized. Fix it > to use the object_dir properly to clean up the *.rev files, > this avoids the crash and cleans up the *.rev files for the > now rewritten multi-pack-index properly. Thanks for splitting this up and clarifying the bug. This matches my understanding, and is careful to get the "--object-dir write" vs "write --object-dir" (where the former segfaults, and the latter triggers a BUG()) distinction right. This looks good to me, but I had one suggestion for an additional test we should consider before picking this up. > diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh > index 3d4d9f10c31b..665c6d64a0ab 100755 > --- a/t/t5319-multi-pack-index.sh > +++ b/t/t5319-multi-pack-index.sh > @@ -201,6 +201,21 @@ test_expect_success 'write midx with twelve packs' ' > > compare_results_with_midx "twelve packs" > > +test_expect_success 'multi-pack-index *.rev cleanup with --object-dir' ' > + git init objdir-test-repo && > + test_when_finished "rm -rf objdir-test-repo" && > + ( > + cd objdir-test-repo && > + test_commit base && > + git repack -d > + ) && > + 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), but this is absolutely the right thing to do in the meantime. > + 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. Thanks, Taylor
