Re: [PATCH 2/2 v7] pack-objects: use reachability bitmap index when generating non-stdout pack
From: Jeff King <hidden>
Date: 2016-08-19 01:12:59
Possibly related (same subject, not in this thread)
- 2016-09-12 · Re: [PATCH 2/2 v7] pack-objects: use reachability bitmap index when generating non-stdout pack · Junio C Hamano <hidden>
On Tue, Aug 09, 2016 at 10:32:17PM +0300, Kirill Smelkov wrote:
Subject: Re: [PATCH 2/2 v7] pack-objects: use reachability bitmap index when generating non-stdout pack
This is v7, but as I understand your numbering, it goes with v5 of patch 1/2 that I just reviewed (usually we just increment the version number on the whole series and treat it as a unit, even if some patches didn't change from version to version).
So we can teach pack-objects to use bitmap index for initial object counting phase when generating resultant pack file too: - if we care it is not activated under git-repack:
Do you mean "if we take care that it is not..." here? (I think you might just be getting tripped up in the English idioms; "care" means that we have a preference; "to take care" means that we are being careful).
- if we know bitmap index generation is not enabled for resultant pack: Current code has singleton bitmap_git so cannot work simultaneously with two bitmap indices.
Minor English fixes: The current code has a singleton bitmap_git, so it cannot work simultaneously with two bitmap indices.
- if we keep pack reuse enabled still only for "send-to-stdout" case: Because on pack reuse raw entries are directly written out to destination pack by write_reused_pack() bypassing needed for pack index generation bookkeeping done by regular codepath in write_one() and friends.
Ditto on English: On pack reuse raw entries are directly written out to the destination pack by write_reused_pack(), bypassing the need for pack index generation bookkeeping done by the regular code path in write_one() and friends. I think this is missing the implication. Why wouldn't we want to reuse in this case? Certainly we don't when doing a "careful" on-disk repack. I suspect the answer is that we cannot write a ".idx" off of the result of write_reused_pack(), and write-to-disk always includes the .idx.
More context:
http://marc.info/?t=146792101400001&r=1&w=2Can we turn this into a link to public-inbox? We have just been bit by all of our old links to gmane dying, and they cannot easily be replaced because they use a gmane-specific article number. public-inbox URLs use message-ids, which should be usable for other archives if public-inbox goes away.
quoted hunk ↗ jump to hunk
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index b1007f2..c92d7fc 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c
The code here looks fine.
quoted hunk ↗ jump to hunk
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh index a278d30..9602e9a 100755 --- a/t/t5310-pack-bitmaps.sh +++ b/t/t5310-pack-bitmaps.sh@@ -196,6 +196,18 @@ test_expect_success 'pack-objects respects --local (non-local bitmapped pack)' ' ! has_any packbitmap.objects 3b.objects ' +test_expect_success 'pack-objects to file can use bitmap' ' + # make sure we still have 1 bitmap index from previous tests + ls .git/objects/pack/ | grep bitmap >output && + test_line_count = 1 output && + # verify equivalent packs are generated with/without using bitmap index + packasha1=$(git pack-objects --no-use-bitmap-index --all packa </dev/null) && + packbsha1=$(git pack-objects --use-bitmap-index --all packb </dev/null) && + list_packed_objects <packa-$packasha1.idx >packa.objects && + list_packed_objects <packb-$packbsha1.idx >packb.objects && + test_cmp packa.objects packb.objects +'
Of course we can't know if bitmaps were actually used, or if they were turned off under the hood. But at least this exercises the code a bit. You could possibly add a perf test which shows off the improvement, but I don't think it's strictly necessary. -Peff