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
> - 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()
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:
Can 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
> 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.
> 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.
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html