Re: [PATCH v2 2/2] repack: die on incremental + write-bitmap-index

From: Jeff King <peff@peff.net>
Date: Dec 23, 2016, 22:20 UTC
Message-ID: <20161223222028.7zuwl2bktyyiatrl@sigill.intra.peff.net>
In-Reply-To: <1482522215-13401-2-git-send-email-dturner@twosigma.com>

On Fri, Dec 23, 2016 at 02:43:35PM -0500, David Turner wrote:

> The bitmap index only works for single packs, so requesting an
> incremental repack with bitmap indexes makes no sense.

OK. I doubt this will come up much after the git-gc change from the first patch. And it should already be printing a warning in this case anyway[1]. But I do not mind turning the warning into a hard failure; it may make things more obvious for the user. The only weird thing is that the bitmap option may come from the config, so this effectively means you can never run "git repack -d" in a repo with bitmap config. I'm not sure if anybody cares or not, with the new git-gc patch.

[1] Technically "repack -d" could actually create bitmaps (with no
    warning) in a repo that has no existing packs. But that's probably
    an uninteresting corner case (the user should say "-ad" there, and
    the "-a" ends up being a noop).

Show 6 quoted lines

> @@ -206,6 +212,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  	if (pack_kept_objects < 0)
>  		pack_kept_objects = write_bitmaps;
>  
> +	if (write_bitmaps && !(pack_everything & ALL_INTO_ONE))
> +		die(incremental_bitmap_conflict_error);

I double-checked that ALL_INTO_ONE covers both -A and -a, which I think should be sufficient here.

Show 10 quoted lines

> -test_expect_success 'incremental repack cannot create bitmaps' '
> +test_expect_success 'incremental repack fails when bitmaps are requested' '
>  	test_commit more-1 &&
>  	find .git/objects/pack -name "*.bitmap" >expect &&
> -	git repack -d &&
> +	test_must_fail git repack -d 2>err &&
> +	test_i18ngrep "Incremental repacks are incompatible with bitmap" err &&
>  	find .git/objects/pack -name "*.bitmap" >actual &&
>  	test_cmp expect actual
>  '

The final `find` is probably overkill now after we know the command failed and reported the error. But it does not hurt to be thorough. :)

-Peff