Re: [PATCH] repack: enable bitmaps by default on bare repos

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Mar 12 2019, Eric Wong wrote:

> Jeff King <peff@peff.net> 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@80x24.org>
> ---
>  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.