Re: [PATCH] send-pack.c: add config push.useBitmaps

From: Derrick Stolee <derrickstolee@github.com>
Date: Jun 15, 2022, 13:09 UTC
Message-ID: <57a7393d-add0-468a-a276-5241c2c84065@github.com>
In-Reply-To: <pull.1263.git.1655291320433.gitgitgadget@gmail.com>

On 6/15/22 7:08 AM, Kyle Zhao via GitGitGadget wrote:

> From: Kyle Zhao <kylezhao@tencent.com>
> 
> This allows you to disabled bitmaps for "git push". Default is false.

s/disabled/disable/

> Bitmaps are designed to speed up the "counting objects" phase of
> subsequent packs created for clones and fetches.
> But in some cases, turning bitmaps on does horrible things for "push"
> performance[1].

I would rephrase this message body as follows:

  Reachability bitmaps are designed to speed up the "counting objects"
  phase of generating a pack during a clone or fetch. They are not
  optimized for Git clients sending a small topic branch via "git push".
  In some cases (see [1]), using reachability bitmaps during "git push"
  can cause significant performance regressions.

  Add a new "push.useBitmaps" config option to disable reachability
  bitmaps during "git push". This allows reachability bitmaps to still
  be used in other areas, such as "git rev-list --use-bitmap-index".

> [1]: https://lore.kernel.org/git/87zhoz8b9o.fsf@evledraar.gmail.com/

> +
> +push.useBitmaps::
> +	If this config and `pack.useBitmaps` are both "true", git will
> +	use pack bitmaps (if available) when git push. Default is false.

Rewording slightly:

  If this config and `pack.useBitmaps` are both `true`, then Git will
  use reachability bitmaps during `git push`, if available (disabled
  by default).

> \ No newline at end of file

Please fix this missing newline.

I'm glad that this references `pack.useBitmaps`. I wonder if that config is sufficient for your purposes: do you expect to use your bitmaps to generate pack-files in any other way than "git push"?

That is: are you serving remote requests from your repo with your bitmaps while also using "git push"? Or, are you using bitmaps only for things like "git rev-list --use-bitmap-index"?

I just want to compare this with `pack.useSparse` which targets "git push", but focuses entirely on the pack-creation part of the operation. Since the existence of reachability bitmaps overrides the sparse algorithm, this does not affect Git servers (who should have a reachability bitmap).

I just want to be sure that using pack.useBitmaps=false would not be sufficient for your situation (and probably most situations).

Show 10 quoted lines

> diff --git a/send-pack.c b/send-pack.c
> index bc0fcdbb000..d6091571caa 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -84,6 +84,8 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised,
>  		strvec_push(&po.args, "--progress");
>  	if (is_repository_shallow(the_repository))
>  		strvec_push(&po.args, "--shallow");
> +	if (!args->use_bitmaps)
> +		strvec_push(&po.args, "--no-use-bitmap-index");

Here is where you specify the lower-level pack creation only when sending a pack. It is very focused. Good.

> +	int use_bitmaps = 0;

> +	git_config_get_bool("push.usebitmaps", &use_bitmaps);
> +	if (use_bitmaps)
> +		args->use_bitmaps = 1;

You can instead write this as:

	if (!git_config_get_bool("push.usebitmaps", &use_bitmaps))
		args->use_bitmaps = use_bitmaps;

Show 12 quoted lines

> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index dedca106a7a..ee0b912a5e8 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1865,4 +1865,15 @@ test_expect_success 'push warns or fails when using username:password' '
>  	test_line_count = 1 warnings
>  '
>  
> +test_expect_success 'push with config push.useBitmaps' '
> +	mk_test testrepo heads/main &&
> +	git checkout main &&
> +	GIT_TRACE=1 git push testrepo main:test >/dev/null 2>stderr &&

Just use "err" instead of "stderr".

Show 6 quoted lines

> +	grep "no-use-bitmap-index" stderr &&
> +
> +	git config push.useBitmaps true &&
> +	GIT_TRACE=1 git push testrepo main:test2 >/dev/null 2>stderr &&
> +	! grep "no-use-bitmap-index" stderr
> +'

I believe this test is correct, but it might be worth looking into test_subcommand if you can determine the exact subcommand arguments you are looking for.

Thanks, -Stolee