| Message ID | 7ea9cced8ec79a8e39948a5e4b8dde6e9b54695a.1643150456.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 61fd31a17975ba27ef1d4a3f25bf55b92f5e7738 |
| Headers | show |
| Series | midx: prevent bitmap corruption when permuting pack order | expand |
On Tue, Jan 25 2022, Taylor Blau wrote: > +test_expect_failure 'changing the preferred pack does not corrupt bitmaps' ' > + rm -fr repo && > + git init repo && > + test_when_finished "rm -fr repo" && Nit: The initial "rm -fr" isn't needed here, and we should aim to have tests clean up after themselves, not needing to clean up after other tests. This appears to have been copy/pasted from the test you added in 54156af0d66 (t5326: test propagating hashcache values, 2021-09-17), which needlessly used that pattern, the tests you added preceding it follow the "clean up your own mess" pattern.
On Wed, Jan 26, 2022 at 04:01:20PM +0100, Ævar Arnfjörð Bjarmason wrote: > > On Tue, Jan 25 2022, Taylor Blau wrote: > > > +test_expect_failure 'changing the preferred pack does not corrupt bitmaps' ' > > + rm -fr repo && > > + git init repo && > > + test_when_finished "rm -fr repo" && > > Nit: The initial "rm -fr" isn't needed here, and we should aim to have > tests clean up after themselves, not needing to clean up after other > tests. > > This appears to have been copy/pasted from the test you added in > 54156af0d66 (t5326: test propagating hashcache values, 2021-09-17), > which needlessly used that pattern, the tests you added preceding it > follow the "clean up your own mess" pattern. I don't think I copy/pasted it, but just have it embedded deep into my muscle-memory from every time that I've written: rm -fr repo git init repo cd repo into $HOME/a.sh, which I use as a dumping ground for little tests. Yes, I agree it's useless here, but it is consistent with the style (for better or worse). It's fine if Junio wants to remove it when queueing, but I don't think it's a big enough deal to matter here (probably not in general, either, but I wouldn't be opposed to a concerted effort to clean these up across all of `t` if that's something you're interested in). Thanks, Taylor
diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh index e187f90f29..0ca2868b0b 100755 --- a/t/t5326-multi-pack-bitmaps.sh +++ b/t/t5326-multi-pack-bitmaps.sh @@ -395,4 +395,35 @@ test_expect_success 'hash-cache values are propagated from pack bitmaps' ' ) ' +test_expect_failure 'changing the preferred pack does not corrupt bitmaps' ' + rm -fr repo && + git init repo && + test_when_finished "rm -fr repo" && + ( + cd repo && + + test_commit A && + test_commit B && + + git rev-list --objects --no-object-names HEAD^ >A.objects && + git rev-list --objects --no-object-names HEAD^.. >B.objects && + + A=$(git pack-objects $objdir/pack/pack <A.objects) && + B=$(git pack-objects $objdir/pack/pack <B.objects) && + + cat >indexes <<-EOF && + pack-$A.idx + pack-$B.idx + EOF + + git multi-pack-index write --bitmap --stdin-packs \ + --preferred-pack=pack-$A.pack <indexes && + git rev-list --test-bitmap A && + + git multi-pack-index write --bitmap --stdin-packs \ + --preferred-pack=pack-$B.pack <indexes && + git rev-list --test-bitmap A + ) +' + test_done
This patch demonstrates a cause of bitmap corruption that can occur when the contents of the multi-pack index does not change, but the underlying object order does. In this example, we have a MIDX containing two packs, each with a distinct set of objects (pack A corresponds to the tree, blob, and commit from the first patch, and pack B corresponds to the second patch). First, a MIDX is written where the 'A' pack is preferred. As expected, the bitmaps generated there are in-tact. But then, we generate an identical MIDX with a different object order: this time preferring pack 'B'. Due to a bug which will be explained and fixed in the following commit, the MIDX is updated, but the .rev file is not, causing the .bitmap file to be read incorrectly. Specifically, the .bitmap file will contain correct data, but the auxiliary object order in the .rev file is stale, causing readers to get confused by reading the new bitmaps using the old object order. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/t5326-multi-pack-bitmaps.sh | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)