Re: [PATCH] pack-objects: warn on split packs disabling bitmaps
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 27, 2016, 22:56 UTC
- Message-ID
- <xmqqfuu67j9t.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20160427215324.GA22165@dcvr.yhbt.net>
Eric Wong <normalperson@yhbt.net> writes:
Show 12 quoted lines
> It can be tempting for a server admin to want a stable set of
> long-lived packs for dumb clients; but also want to enable
> bitmaps to serve smart clients more quickly.
>
> Unfortunately, such a configuration is impossible;
> so at least warn users of this incompatibility since
> commit 21134714787a02a37da15424d72c0119b2b8ed71
> ("pack-objects: turn off bitmaps when we split packs").
>
> Tested the warning by inspecting the output of:
>
> make -C t t5310-pack-bitmaps.sh GIT_TEST_OPTS=-vWhile I do not think the update in this patch is wrong, this makes me wonder what happens to a server admin who wants a stable set of long-lived objects in a pack, and other objects in another pack that is frequently recreated, by first packing objects needed for the latest released version into a single pack and marking it with .keep and then running "git repack" to create the second pack for the remainder.
There is no automatic split involved, so we would get a bitmap file for each of these two packs. Would that pose a problem? The pack with the newer part of the history would not satisfy "all of the reachable objects are in the same pack" condition.
Show 14 quoted lines
> diff --git a/Documentation/config.txt b/Documentation/config.txt > index 42d2b50..ec31170 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -2156,8 +2156,10 @@ pack.packSizeLimit:: > The maximum size of a pack. This setting only affects > packing to a file when repacking, i.e. the git:// protocol > is unaffected. It can be overridden by the `--max-pack-size` > - option of linkgit:git-repack[1]. The minimum size allowed is > - limited to 1 MiB. The default is unlimited. > + option of linkgit:git-repack[1]. Reaching this limit prevents > + pack bitmaps from being written when `repack.writeBitmaps` > + is configured. The minimum size allowed is limited to 1 MiB. > + The default is unlimited.
This is not wrong per-se, but I find the additional text overly verbose. The resulting text has at least three issues:
- This sets maximum. It does not say what happens when the maximum
is reached ("packing fails" is a valid expectation). We should
spell out that when the maximum is reached, we will create
multiple packfiles.- It is not "reaching this limit" that prevents bitmaps from being written. It is "we will create multiple packfiles" that does.
- Even when repack.writeBitmaps is not configured, but bitmap is requested via the command line option "repack -b", bitmap generation is disabled once we end up creating multiple packfiles.
Show 9 quoted lines
> @@ -2557,8 +2559,9 @@ repack.writeBitmaps:: > objects to disk (e.g., when `git repack -a` is run). This > index can speed up the "counting objects" phase of subsequent > packs created for clones and fetches, at the cost of some disk > - space and extra time spent on the initial repack. Defaults to > - false. > + space and extra time spent on the initial repack. This has > + no effect if `pack.packSizeLimit` is configured and reached. > + Defaults to false.
This has the opposite issue as the third point above. We have to ignore repack.writeBitmaps when we end up splitting a pack, regardless of the reason why we split was pack.packSizeLimit or by an explicit request from the command line.
Also to future-proof these two paragraphs (and those that are touched by later parts of this patch), it may even be a good idea to omit the mention of packsizelimit altogether. We may invent a new and different reason why we end up producing multiple packfiles, other than packsizelimit. What this patch wants to make readers aware is that when multiple packs are generated, bitmap generation is disabled. Other details (e.g. how the user asks us to create multiple packs, either via command line or configuration, or how the use asks us to generate bitmaps) are of lessor importance.
Show 11 quoted lines
> diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt > index bbea529..19cdcd0 100644 > --- a/Documentation/git-pack-objects.txt > +++ b/Documentation/git-pack-objects.txt > @@ -110,7 +110,8 @@ base-name:: > --max-pack-size=<n>:: > Maximum size of each output pack file. The size can be suffixed with > "k", "m", or "g". The minimum size allowed is limited to 1 MiB. > - If specified, multiple packfiles may be created. > + If specified, multiple packfiles may be created, which also > + prevents the creation of a bitmap index.
This is a good update, judging with the yardstick I set in the previous paragraph in this review.
Show 14 quoted lines
> The default is unlimited, unless the config variable > `pack.packSizeLimit` is set. > > diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt > index af230d0..2065812 100644 > --- a/Documentation/git-repack.txt > +++ b/Documentation/git-repack.txt > @@ -106,7 +106,8 @@ other objects in that pack they already have locally. > --max-pack-size=<n>:: > Maximum size of each output pack file. The size can be suffixed with > "k", "m", or "g". The minimum size allowed is limited to 1 MiB. > - If specified, multiple packfiles may be created. > + If specified, multiple packfiles may be created, causing > + `--write-bitmap-index` and `repack.writeBitmaps` to be ignored.
And this is not. Just say "bitmap index is not created".
Show 8 quoted lines
> @@ -115,7 +116,9 @@ other objects in that pack they already have locally. > Write a reachability bitmap index as part of the repack. This > only makes sense when used with `-a` or `-A`, as the bitmaps > must be able to refer to all reachable objects. This option > - overrides the setting of `pack.writeBitmaps`. > + overrides the setting of `repack.writeBitmaps`. This option > + has no effect if a multiple packfiles are created due to > + reaching `pack.packSizeLimit` or `--max-pack-size`.
Dropping "due to ..." makes it perfect.