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

From: Jeff King <peff@peff.net>
Date: Mar 12, 2019, 10:49 UTC
Message-ID: <20190312104954.GA2023@sigill.intra.peff.net>
In-Reply-To: <20190312031303.5tutut7zzvxne5dw@dcvr>

On Tue, Mar 12, 2019 at 03:13:03AM +0000, Eric Wong wrote:

Show 14 quoted lines

> > 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.

OK. I still think of bitmaps as something that might need manual care and feeding, but I think that may be leftover superstition. I can't offhand think of any real downsides to this.

>  static int delta_base_offset = 1;
>  static int pack_kept_objects = -1;
> -static int write_bitmaps;
> +static int write_bitmaps = -1;

So we'll have "-1" be "not decided yet". Makes sense.

Show 10 quoted lines

> @@ -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();
> +	}

Might it be easier here to always resolve "-1" into a 0/1? I.e., like:

  if (write_bitmaps < 0)
	write_bitmaps = (pack_everything & ALL_INTO_ONE) && is_bare_repository();

and then the rest of the logic can stay the same, and does not need to be modified to handle "write_bitmaps < 0"?

> +test_expect_success 'bitmaps are created by default in bare repos' '
> +	git clone --bare .git bare.git &&
> +	cd bare.git &&

Please don't "cd" outside of a subshell, since it impacts further tests that are added.

> +	mkdir old &&
> +	mv objects/pack/* old &&
> +	pack=$(ls old/*.pack) &&

Are we sure we have just done $pack here? Our repo came from a local-disk clone, which would have just hard-linked whatever was in the source repo. So we're subtly relying on the state that other tests have left.

I'm not sure what we're trying to accomplish with this unpacking, though. Running "git repack -ad" should generate bitmaps whether the objects were already in a single pack or not. So I think this test can just be:

  git clone --bare . bare.git &&
  git -C bare.git repack -ad &&
  bitmap=$(ls objects/pack/*.bitmap)
  test_path_is_file "$bitmap"

I do agree with Ævar it might also be worth testing that disabling bitmaps explicitly still works. And also that repacking _without_ "-a" (i.e., an incremental) does not complain about being unable to generate bitmaps.

-Peff