Re: [PATCH 2/3] repack: silence warnings when auto-enabled bitmaps cannot be built

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> @@ -3313,8 +3319,13 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  			    N_("do not hide commits by grafts"), 0),
>  		OPT_BOOL(0, "use-bitmap-index", &use_bitmap_index,
>  			 N_("use a bitmap index if available to speed up counting objects")),
> -		OPT_BOOL(0, "write-bitmap-index", &write_bitmap_index,
> -			 N_("write a bitmap index together with the pack index")),
> +		OPT_SET_INT(0, "write-bitmap-index", &write_bitmap_index,
> +			    N_("write a bitmap index together with the pack index"),
> +			    WRITE_BITMAP_TRUE),
> +		OPT_SET_INT_F(0, "write-bitmap-index-quiet",
> +			      &write_bitmap_index,
> +			      N_("write a bitmap index if possible"),
> +			      WRITE_BITMAP_QUIET, PARSE_OPT_HIDDEN),

The receiving end of this communication is pretty easy to follow.
I'd have named an option to trigger "if possible" behaviour after
that "if possible" phrase and not "quiet", but this is entirely
internal that it does not matter.

> diff --git a/builtin/repack.c b/builtin/repack.c
> index 30982ed2a2..db93ca3660 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -345,13 +345,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  		die(_("--keep-unreachable and -A are incompatible"));
>  
>  	if (write_bitmaps < 0) {
> -		write_bitmaps = (pack_everything & ALL_INTO_ONE) &&
> -				 is_bare_repository() &&
> -				 keep_pack_list.nr == 0 &&
> -				 !has_pack_keep_file();
> +		if (!(pack_everything & ALL_INTO_ONE) ||
> +		    !is_bare_repository() ||
> +		    keep_pack_list.nr != 0 ||
> +		    has_pack_keep_file())
> +			write_bitmaps = 0;

This side of communication is a bit harder to follow, but not
impossible ;-) We leave it "negative" to signal "the user did not
specify, but we enabled it by default" here.

>  	}
>  	if (pack_kept_objects < 0)
> -		pack_kept_objects = write_bitmaps;
> +		pack_kept_objects = !!write_bitmaps;

And non-zero write_bitmaps replaces "true" in the older world.

>  	if (write_bitmaps && !(pack_everything & ALL_INTO_ONE))

So this does not have to change.

>  		die(_(incremental_bitmap_conflict_error));
> @@ -375,8 +376,10 @@ 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");
> +	else if (write_bitmaps < 0)
> +		argv_array_push(&cmd.args, "--write-bitmap-index-quiet");

And "enabled by user" and "enabled by default" are mapped to the two
options.

All makes sense.

Thanks.