On Tue, Mar 12 2019, Eric Wong wrote:
> Jeff King <peff@xxxxxxxx> wrote:
>> On Sat, Mar 09, 2019 at 02:49:44AM +0000, Eric Wong wrote:
>> > It would make life easier for people new to hosting git servers
>> > (and hopefully reduce centralization :)
>>
>> I do think they're a net win for people hosting git servers. But if
>> that's the goal, I think at most you'd want to make bitmaps the default
>> for bare repos. They're really not much help for normal end-user repos
>> at this point.
>
> Fair enough, hopefully this can make life easier for admins
> new to hosting git:
>
> ----------8<---------
> Subject: [PATCH] repack: enable bitmaps by default on bare repos
>
> A typical use case for bare repos is for serving clones and
> fetches to clients. Enable bitmaps by default on bare repos to
> make it easier for admins to host git repos in a performant way.
>
> Signed-off-by: Eric Wong <e@xxxxxxxxx>
> ---
> builtin/repack.c | 16 ++++++++++------
> t/t7700-repack.sh | 14 +++++++++++++-
> 2 files changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 67f8978043..5d4758b515 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -14,7 +14,7 @@
>
> static int delta_base_offset = 1;
> static int pack_kept_objects = -1;
> -static int write_bitmaps;
> +static int write_bitmaps = -1;
> static int use_delta_islands;
> static char *packdir, *packtmp;
>
> @@ -343,11 +343,15 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE)))
> die(_("--keep-unreachable and -A are incompatible"));
>
> + if (!(pack_everything & ALL_INTO_ONE)) {
> + if (write_bitmaps > 0)
> + die(_(incremental_bitmap_conflict_error));
> + } else if (write_bitmaps < 0) {
> + write_bitmaps = is_bare_repository();
> + }
> +
> if (pack_kept_objects < 0)
> - pack_kept_objects = write_bitmaps;
> -
> - if (write_bitmaps && !(pack_everything & ALL_INTO_ONE))
> - die(_(incremental_bitmap_conflict_error));
> + pack_kept_objects = write_bitmaps > 0;
>
> packdir = mkpathdup("%s/pack", get_object_directory());
> packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, (int)getpid());
> @@ -368,7 +372,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> argv_array_push(&cmd.args, "--indexed-objects");
> if (repository_format_partial_clone)
> argv_array_push(&cmd.args, "--exclude-promisor-objects");
> - if (write_bitmaps)
> + if (write_bitmaps > 0)
> argv_array_push(&cmd.args, "--write-bitmap-index");
> if (use_delta_islands)
> argv_array_push(&cmd.args, "--delta-islands");
> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> index 6162e2a8e6..3e0b5c40e4 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -221,5 +221,17 @@ test_expect_success 'repack --keep-pack' '
> )
> '
>
> +test_expect_success 'bitmaps are created by default in bare repos' '
> + git clone --bare .git bare.git &&
> + cd bare.git &&
> + mkdir old &&
> + mv objects/pack/* old &&
> + pack=$(ls old/*.pack) &&
> + test_path_is_file "$pack" &&
> + git unpack-objects -q <"$pack" &&
> + git repack -ad &&
> + bitmap=$(ls objects/pack/*.bitmap) &&
> + test_path_is_file "$bitmap"
> +'
> +
> test_done
> -
Looks sensible in principle, but now the git-repack and git-config docs
talking about repack.writeBitmaps are out-of-date. A v2 should update
those.
Also worth testing that -c repack.writeBitmaps=false on a bare
repository still overrides it, even though glancing at the code it looks
like that case is handled, but without being tested for.
