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

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

[Kyle Zhao via GitGitGadget]

From: Kyle Zhao <kylezhao@tencent.com>

This allows you to disabled bitmaps for "git push". Default is false.

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

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

Signed-off-by: Kyle Zhao <kylezhao@tencent.com>
---
    send-pack.c: add config push.useBitmaps
    
    This patch add config push.useBitmaps to prevent git push using bitmap.
    
    The origin discussion is here:
    https://lore.kernel.org/git/b940e705fbe9454685757f2e3055e2ce@tencent.com/
    
    Thanks, Kyle

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1263%2Fkeyu98%2Fkz%2Fpush-usebitmps-config-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1263/keyu98/kz/push-usebitmps-config-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1263

 Documentation/config/push.txt |  4 ++++
 send-pack.c                   |  6 ++++++
 send-pack.h                   |  3 ++-
 t/t5516-fetch-push.sh         | 11 +++++++++++
 4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt
index e32801e6c91..d8fb0bd1414 100644
--- a/Documentation/config/push.txt
+++ b/Documentation/config/push.txt
@@ -137,3 +137,7 @@ push.negotiate::
 	server attempt to find commits in common. If "false", Git will
 	rely solely on the server's ref advertisement to find commits
 	in common.
+
+push.useBitmaps::
+	If this config and `pack.useBitmaps` are both "true", git will
+	use pack bitmaps (if available) when git push. Default is false.
\ No newline at end of file
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");
 	po.in = -1;
 	po.out = args->stateless_rpc ? -1 : fd;
 	po.git_cmd = 1;
@@ -482,6 +484,7 @@ int send_pack(struct send_pack_args *args,
 	int use_push_options = 0;
 	int push_options_supported = 0;
 	int object_format_supported = 0;
+	int use_bitmaps = 0;
 	unsigned cmds_sent = 0;
 	int ret;
 	struct async demux;
@@ -497,6 +500,9 @@ int send_pack(struct send_pack_args *args,
 	git_config_get_bool("push.negotiate", &push_negotiate);
 	if (push_negotiate)
 		get_commons_through_negotiation(args->url, remote_refs, &commons);
+	git_config_get_bool("push.usebitmaps", &use_bitmaps);
+	if (use_bitmaps)
+		args->use_bitmaps = 1;
 
 	git_config_get_bool("transfer.advertisesid", &advertise_sid);
 
diff --git a/send-pack.h b/send-pack.h
index e148fcd9609..f7af1b0353e 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -26,7 +26,8 @@ struct send_pack_args {
 		/* One of the SEND_PACK_PUSH_CERT_* constants. */
 		push_cert:2,
 		stateless_rpc:1,
-		atomic:1;
+		atomic:1,
+		use_bitmaps:1;
 	const struct string_list *push_options;
 };
 
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 &&
+	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
+'
+
 test_done

base-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9
-- 
gitgitgadget

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

[Derrick Stolee]

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


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

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

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

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

[Taylor Blau]

On Wed, Jun 15, 2022 at 09:09:35AM -0400, Derrick Stolee wrote:
> I just want to be sure that using pack.useBitmaps=false would not
> be sufficient for your situation (and probably most situations).

I think the only other affected scenario on the client side would be
repacking. And in theory most clients are repacking in the background
anyways, so any speed-ups from using bitmaps wouldn't be noticeable
anyway.

I think just relying on the existing pack.useBitmaps config should be
sufficient here.

Thanks,
Taylor

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

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 15 2022, Kyle Zhao via GitGitGadget wrote:

[CC'd Taylor, who's worked a lot on bitmaps]

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

Thanks for following up.

Refresh my memory, we always use them if we find them now, correct?
I.e. if repack.writebitmaps=true

This doesn't make it clear: Are we now going to ignore them for "push"
by default, even if they exist? I.e. a change in the current default.

I think I recall from the previous discussions that it was a bit of hit
& miss, maybe we're confident that they're almost (or always?) bad for
"push", but I think there *are* cases where they help.

> 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].
>
> [1]: https://lore.kernel.org/git/87zhoz8b9o.fsf@evledraar.gmail.com/
>
> Signed-off-by: Kyle Zhao <kylezhao@tencent.com>
> ---
>     send-pack.c: add config push.useBitmaps
>     
>     This patch add config push.useBitmaps to prevent git push using bitmap.
>     
>     The origin discussion is here:
>     https://lore.kernel.org/git/b940e705fbe9454685757f2e3055e2ce@tencent.com/
>     
>     Thanks, Kyle
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1263%2Fkeyu98%2Fkz%2Fpush-usebitmps-config-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1263/keyu98/kz/push-usebitmps-config-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1263
>
>  Documentation/config/push.txt |  4 ++++
>  send-pack.c                   |  6 ++++++
>  send-pack.h                   |  3 ++-
>  t/t5516-fetch-push.sh         | 11 +++++++++++
>  4 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt
> index e32801e6c91..d8fb0bd1414 100644
> --- a/Documentation/config/push.txt
> +++ b/Documentation/config/push.txt
> @@ -137,3 +137,7 @@ push.negotiate::
>  	server attempt to find commits in common. If "false", Git will
>  	rely solely on the server's ref advertisement to find commits
>  	in common.
> +
> +push.useBitmaps::
> +	If this config and `pack.useBitmaps` are both "true", git will
> +	use pack bitmaps (if available) when git push. Default is false.
> \ No newline at end of file

"git diff" is telling you something is wrong here :) hint: missing \n.

> 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");
>  	po.in = -1;
>  	po.out = args->stateless_rpc ? -1 : fd;
>  	po.git_cmd = 1;
> @@ -482,6 +484,7 @@ int send_pack(struct send_pack_args *args,
>  	int use_push_options = 0;
>  	int push_options_supported = 0;
>  	int object_format_supported = 0;
> +	int use_bitmaps = 0;
>  	unsigned cmds_sent = 0;
>  	int ret;
>  	struct async demux;
> @@ -497,6 +500,9 @@ int send_pack(struct send_pack_args *args,
>  	git_config_get_bool("push.negotiate", &push_negotiate);
>  	if (push_negotiate)
>  		get_commons_through_negotiation(args->url, remote_refs, &commons);
> +	git_config_get_bool("push.usebitmaps", &use_bitmaps);
> +	if (use_bitmaps)
> +		args->use_bitmaps = 1;

[A bit rambling, sorry]

A bit off of a use of the API, can't we just do:

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

And drop the local variable? I.e. if we have a config variable we'll
write the value to it.

But then again that goes with the suggested default, i.e. I think we
should probably use them by default, and provide an out, unless we're
*really* sure they're useless for "push".

In this case I found the code a bit odd, which took me a moment to put
my finger on.

In builtin/send-pack.c we initialize "struct send_pack_args" in the file
scope, so it's zero'd out.

So all parameters are 0'd by default.

So your new "use_bitmaps" is born false.

Then when we get here you assign 0 to use_bitmaps, and only if it's true
do you use it.

Which to me is a couple of layers in to something that's less clear than
it could be, i.e. we're making the hard assumption here that the default
in the struct is false. So we should really just do:

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

So don't care what the default was before, we have an explicit config
variable we're going to use, if we saw push.usebitmaps let's use its
value.

This way the default can flip, and this code downstream of that will
still do what's intended.

FWIW that "if" is redundant, but it's the general idiom, but we *can* do
it the way I suggested above, but I think it's clearer to go with the
second form, which reads more obviously as "if the variable exists, set
it to ...".

For config.c in particular the "without the if" works, but I think
that's relying on an implementation detail, i.e. we have a few other
APIs that "zero out" the parameter you put in as the first thing they
do. So if they also return "I didn't error" you want to use a temp
variable, and only assign it to your "real" variable if the return value
was "OK".

>  
>  	git_config_get_bool("transfer.advertisesid", &advertise_sid);
>  
> diff --git a/send-pack.h b/send-pack.h
> index e148fcd9609..f7af1b0353e 100644
> --- a/send-pack.h
> +++ b/send-pack.h
> @@ -26,7 +26,8 @@ struct send_pack_args {
>  		/* One of the SEND_PACK_PUSH_CERT_* constants. */
>  		push_cert:2,
>  		stateless_rpc:1,
> -		atomic:1;
> +		atomic:1,
> +		use_bitmaps:1;
>  	const struct string_list *push_options;
>  };
>  
> 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 &&

We generally don't >/dev/null in tests, just emit the output, and if we
run with -v you'll see it.

In this case though you want just:

    GIT_TRACE="$PWD/trace.log" [...]

Without any redirection,

Also, you probably want GIT_TRACE2_EVENT=$PWD/trace.json, and see
"test_subcommand". We have a handy helper for finding if we have trace2
regions.

I'm assuming we have some region for "used bitmaps" already, but I
didn't check...

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

[Taylor Blau]

On Wed, Jun 15, 2022 at 09:47:58PM +0200, Ævar Arnfjörð Bjarmason wrote:
> Refresh my memory, we always use them if we find them now, correct?
> I.e. if repack.writebitmaps=true

Kind of. `rev-list` has an opt-in `--use-bitmap-index` option, and
`pack-objects` has `pack.useBitmaps` which controls whether or not
bitmaps are read.

So `git rev-list --objects HEAD` won't use any bitmaps, even if they
exist, but `git rev-list --objects --use-bitmap-index HEAD` will. There
there's a good reason not to use bitmaps by default, which is that they
(effectively) randomize the ordering of your output.

`pack-objects` behaves slightly differently, cf. its
`use_bitmap_index{,default}` variables to see how that works.

> This doesn't make it clear: Are we now going to ignore them for "push"
> by default, even if they exist? I.e. a change in the current default.
>
> I think I recall from the previous discussions that it was a bit of hit
> & miss, maybe we're confident that they're almost (or always?) bad for
> "push", but I think there *are* cases where they help.

Yeah. In theory they should help most of the time as long as the bitmaps
are kept reasonably up-to-date. There's a tradeoff between using bitmaps
and how much walking between bitmap and ref-tips is required. Since
every object we encounter between the most recent bitmap and the thing
we're trying to generate a bitmap for has to incur extra overhead in
order to find and mark its position in the bitmap being generated,
there's a certain point at which it would have been faster to just walk
down to the roots and avoid bitmaps altogether.

But I suspect that in this case the bitmaps are just simply stale, and
that a "git repack -adb" or more aggressive background maintenance would
make things quite a bit faster.

Thanks,
Taylor

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

[kylezhao(赵柯宇)]

> > 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 > > I just want to be sure that using pack.useBitmaps=false would not be 
> > sufficient for your situation (and probably most situations).
> 
> I think the only other affected scenario on the client side would be repacking. And in theory most clients are repacking in the background anyways, so any speed-ups from using bitmaps wouldn't be noticeable anyway.
> 
> I think just relying on the existing pack.useBitmaps config should be sufficient here.

In fact ,we also use "git push" on our server side. 
Each git repositories have multiple replicas on our servers to improve system disaster tolerance and read performance.

For example, a git repo will be distributed on multiple servers (like server-1, server-2, server-3).
If user pushes the pack to server-1, then server-1 will call "git push" to transfer the objects data to server-2 and server-3.
And users can clone from all the server mentioned above.
Under such a process, our system works well most of the time.

If we set pack.useBitmaps=false, "git upload-pack" will also be affected.
For my situation, it would be sufficient when set both pack.useBitmaps=true and push.useBitmaps=false.

> But I suspect that in this case the bitmaps are just simply stale, and
> that a "git repack -adb" or more aggressive background maintenance would
> make things quite a bit faster.

It doesn't seem to be the reason.
I have already called  "git repack -adb" in this case[1] but that didn't seem to fix the push performance issue.
You can see that the git repo had only one pack at that time.

[1] https://lore.kernel.org/git/b940e705fbe9454685757f2e3055e2ce@tencent.com/

Thanks,
- Kyle

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

[Kyle Zhao via GitGitGadget]

From: Kyle Zhao <kylezhao@tencent.com>

This allows you to disable bitmaps for "git push". Default is false.

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/

Signed-off-by: Kyle Zhao <kylezhao@tencent.com>
---
    send-pack.c: add config push.useBitmaps
    
    This patch add config push.useBitmaps to prevent git push using bitmap.
    
    The origin discussion is here:
    https://lore.kernel.org/git/b940e705fbe9454685757f2e3055e2ce@tencent.com/
    
    Thanks, -Kyle
    
    Changes since v1:
    
     * changed the commit message
     * modified and added missing \n to push.txt
     * used test_subcommand for test
     * modified "if" statement for "git_config_get_bool()" in send-pack.c

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1263%2Fkeyu98%2Fkz%2Fpush-usebitmps-config-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1263/keyu98/kz/push-usebitmps-config-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1263

Range-diff vs v1:

 1:  000d033584b ! 1:  42e0b4845b2 send-pack.c: add config push.useBitmaps
     @@ Metadata
       ## Commit message ##
          send-pack.c: add config push.useBitmaps
      
     -    This allows you to disabled bitmaps for "git push". Default is false.
     +    This allows you to disable bitmaps for "git push". Default is false.
      
     -    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].
     +    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/
      
     @@ Documentation/config/push.txt: push.negotiate::
       	in common.
      +
      +push.useBitmaps::
     -+	If this config and `pack.useBitmaps` are both "true", git will
     -+	use pack bitmaps (if available) when git push. Default is false.
     - \ No newline at end of file
     ++    If this config and `pack.useBitmaps` are both `true`, then Git will
     ++    use reachability bitmaps during `git push`, if available (disabled
     ++    by default).
      
       ## send-pack.c ##
      @@ send-pack.c: static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised,
     @@ send-pack.c: static int pack_objects(int fd, struct ref *refs, struct oid_array
       	po.out = args->stateless_rpc ? -1 : fd;
       	po.git_cmd = 1;
      @@ send-pack.c: int send_pack(struct send_pack_args *args,
     - 	int use_push_options = 0;
     - 	int push_options_supported = 0;
     - 	int object_format_supported = 0;
     -+	int use_bitmaps = 0;
     - 	unsigned cmds_sent = 0;
     - 	int ret;
       	struct async demux;
     + 	const char *push_cert_nonce = NULL;
     + 	struct packet_reader reader;
     ++	int use_bitmaps;
     + 
     + 	if (!remote_refs) {
     + 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
      @@ send-pack.c: int send_pack(struct send_pack_args *args,
     - 	git_config_get_bool("push.negotiate", &push_negotiate);
       	if (push_negotiate)
       		get_commons_through_negotiation(args->url, remote_refs, &commons);
     -+	git_config_get_bool("push.usebitmaps", &use_bitmaps);
     -+	if (use_bitmaps)
     -+		args->use_bitmaps = 1;
       
     ++	if (!git_config_get_bool("push.usebitmaps", &use_bitmaps))
     ++		args->use_bitmaps = use_bitmaps;
     ++
       	git_config_get_bool("transfer.advertisesid", &advertise_sid);
       
     + 	/* Does the other end support the reporting? */
      
       ## send-pack.h ##
      @@ send-pack.h: struct send_pack_args {
     @@ t/t5516-fetch-push.sh: test_expect_success 'push warns or fails when using usern
      +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 &&
     -+	grep "no-use-bitmap-index" stderr &&
     ++	GIT_TRACE2_EVENT="$PWD/default" \
     ++	git push testrepo main:test &&
     ++	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
     ++	--thin --delta-base-offset -q --no-use-bitmap-index <default &&
      +
      +	git config push.useBitmaps true &&
     -+	GIT_TRACE=1 git push testrepo main:test2 >/dev/null 2>stderr &&
     -+	! grep "no-use-bitmap-index" stderr
     ++	GIT_TRACE2_EVENT="$PWD/true" \
     ++	git push testrepo main:test2 &&
     ++	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
     ++	--thin --delta-base-offset -q <true &&
     ++
     ++	git config push.useBitmaps false &&
     ++	GIT_TRACE2_EVENT="$PWD/false" \
     ++	git push testrepo main:test3 &&
     ++	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
     ++	--thin --delta-base-offset -q --no-use-bitmap-index <false
      +'
      +
       test_done


 Documentation/config/push.txt |  5 +++++
 send-pack.c                   |  6 ++++++
 send-pack.h                   |  3 ++-
 t/t5516-fetch-push.sh         | 21 +++++++++++++++++++++
 4 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt
index e32801e6c91..3f3ff66fe7c 100644
--- a/Documentation/config/push.txt
+++ b/Documentation/config/push.txt
@@ -137,3 +137,8 @@ push.negotiate::
 	server attempt to find commits in common. If "false", Git will
 	rely solely on the server's ref advertisement to find commits
 	in common.
+
+push.useBitmaps::
+    If this config and `pack.useBitmaps` are both `true`, then Git will
+    use reachability bitmaps during `git push`, if available (disabled
+    by default).
diff --git a/send-pack.c b/send-pack.c
index bc0fcdbb000..627e79d7623 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");
 	po.in = -1;
 	po.out = args->stateless_rpc ? -1 : fd;
 	po.git_cmd = 1;
@@ -487,6 +489,7 @@ int send_pack(struct send_pack_args *args,
 	struct async demux;
 	const char *push_cert_nonce = NULL;
 	struct packet_reader reader;
+	int use_bitmaps;
 
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
@@ -498,6 +501,9 @@ int send_pack(struct send_pack_args *args,
 	if (push_negotiate)
 		get_commons_through_negotiation(args->url, remote_refs, &commons);
 
+	if (!git_config_get_bool("push.usebitmaps", &use_bitmaps))
+		args->use_bitmaps = use_bitmaps;
+
 	git_config_get_bool("transfer.advertisesid", &advertise_sid);
 
 	/* Does the other end support the reporting? */
diff --git a/send-pack.h b/send-pack.h
index e148fcd9609..f7af1b0353e 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -26,7 +26,8 @@ struct send_pack_args {
 		/* One of the SEND_PACK_PUSH_CERT_* constants. */
 		push_cert:2,
 		stateless_rpc:1,
-		atomic:1;
+		atomic:1,
+		use_bitmaps:1;
 	const struct string_list *push_options;
 };
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index dedca106a7a..0d416d1474f 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1865,4 +1865,25 @@ 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_TRACE2_EVENT="$PWD/default" \
+	git push testrepo main:test &&
+	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
+	--thin --delta-base-offset -q --no-use-bitmap-index <default &&
+
+	git config push.useBitmaps true &&
+	GIT_TRACE2_EVENT="$PWD/true" \
+	git push testrepo main:test2 &&
+	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
+	--thin --delta-base-offset -q <true &&
+
+	git config push.useBitmaps false &&
+	GIT_TRACE2_EVENT="$PWD/false" \
+	git push testrepo main:test3 &&
+	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
+	--thin --delta-base-offset -q --no-use-bitmap-index <false
+'
+
 test_done

base-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9
-- 
gitgitgadget

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

[Derrick Stolee]

On 6/15/2022 11:36 PM, Kyle Zhao via GitGitGadget wrote:
> From: Kyle Zhao <kylezhao@tencent.com>
> 
> This allows you to disable bitmaps for "git push". Default is false.
> 
> 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/
> 
> Signed-off-by: Kyle Zhao <kylezhao@tencent.com>
> ---
>     send-pack.c: add config push.useBitmaps
>     
>     This patch add config push.useBitmaps to prevent git push using bitmap.
>     
>     The origin discussion is here:
>     https://lore.kernel.org/git/b940e705fbe9454685757f2e3055e2ce@tencent.com/
>     
>     Thanks, -Kyle
>     
>     Changes since v1:
>     
>      * changed the commit message
>      * modified and added missing \n to push.txt
>      * used test_subcommand for test
>      * modified "if" statement for "git_config_get_bool()" in send-pack.c

This version satisfies the recommended changes I had from the last
version. As long as we think there is a benefit to having this
additional knob over just pack.useBitmaps, then this is good to go.

For my part, I think having the additional knob is less complicated
than requiring all users in this situation to shift from "push" to
a "fetch" (from the other side of the connection).

Thanks,
-Stolee

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

[Ævar Arnfjörð Bjarmason]

On Thu, Jun 16 2022, Kyle Zhao via GitGitGadget wrote:

> From: Kyle Zhao <kylezhao@tencent.com>
>
> This allows you to disable bitmaps for "git push". Default is false.
>
> 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/
> [...]
> +	mk_test testrepo heads/main &&
> +	git checkout main &&
> +	GIT_TRACE2_EVENT="$PWD/default" \
> +	git push testrepo main:test &&
> +	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
> +	--thin --delta-base-offset -q --no-use-bitmap-index <default &&

Nit: We tend to indent these ase we wrap, so e.g.:

	test_subcommand git ... \
		--thin --delta [...]

The rest all looks good as far as the diff goes, if what we want to do
is to disable this by default, and this isn't worth a re-roll in itself.

But I still think that completely disabling bitmaps might be premature
here, especially per Taylor's comment on v1 (which I understand to mean
that they should help some of the time, even with push).

And since this is linking to that old thread I started in 2019 I really
think the commit message should be exploring and justifying why this
might be slower in all cases, or at least a sufficient number of cases
to flip the default.

if it's not are we throwing out a generally useful optimization (by
default) due to some edge cases we've found?

E.g. have you tried to follow-up on this comment by Jeff King?
https://lore.kernel.org/git/20190523113031.GA17448@sigill.intra.peff.net/

At the time I didn't, because as noted in a follow-up I'd lost my test
case by the time I read that, but it seems you haven't, and have a
current test case.

If you apply that patch on that old git version at the time (as it
probably won't apply cleanly), does it make a difference for your case?

To be clear I'm *not* confident that this isn't the right thing to do as
far as the diff is concerned. But I *am* confident that the proposed
commit message isn't selling me on the idea.

I think a really good start would be to come up with some benchmarks for
a few common cases, e.g. how do bitmaps do if we have a big repo but few
refs, how about a lot of refs? Does their stale-ness make a difference
or not (per previous discussions) etc. etc.

Or, if you don't have time for any of that I think a good change for now
would be to add the flag, but not flip the default, and say "we don't
have enough data to say if bitmaps are overall worse for pushes, but
here's an opt-opt".

But the change as it stands is effectively saying "bitmaps on push
hinder more than they help, and turning them on for push was a mistake".

Maybe that's true, but I don't think we've got any data to support
that. Even though I've got one of those anecdotes (and from occasional
investigations of slow "git push" I'm pretty sure this would help my
use-cases more than it would harm them) the plural of anecdote isn't
data.

I've only personally run into this with repos with a large number (>30k
at least) number of refs, and where the bitmaps were in some state of
staleness (this usually being my working copy, where I rely on "git gc
--auto").

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

[kylezhao(赵柯宇)]

> And since this is linking to that old thread I started in 2019 I really
>think the commit message should be exploring and justifying why this
> might be slower in all cases, or at least a sufficient number of cases
> to flip the default.
> 
> if it's not are we throwing out a generally useful optimization (by
> default) due to some edge cases we've found?

I'm not sure if it's an edge case.
However, if a git repo has this problem, it's "git push" will always be very slow
instead of going fast and slow.

> E.g. have you tried to follow-up on this comment by Jeff King?
> https://lore.kernel.org/git/20190523113031.GA17448@sigill.intra.peff.net/

Not yet, I'll try it later.

> At the time I didn't, because as noted in a follow-up I'd lost my test
> case by the time I read that, but it seems you haven't, and have a
> current test case.

I tried to find a test case in open-source projects, and finally found one.
$ git clone https://github.com/JetBrains/intellij-community.git --bare
$ cd intellij-community.git
$ git repack -adb
$ GIT_TRACE=1 git push . master:test1

Then it would get stuck in "git pack-objects" for about 10s. (OS: Windows)
After I deleted the bitmap, "git push" took less than 1s.


> I think a really good start would be to come up with some benchmarks for
> a few common cases, e.g. how do bitmaps do if we have a big repo but few
> refs, how about a lot of refs? Does their stale-ness make a difference
> or not (per previous discussions) etc. etc.

According to my observation, the problem always occur on git repos which have 
a lot of refs and objects.
Also, "git push"  takes much time on "find _objects(..) for haves_bitmap".

Hope the information above can help find the cause of the problem.

Thanks,
- Kyle

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

[Taylor Blau]

On Thu, Jun 16, 2022 at 03:11:09PM +0000, kylezhao(赵柯宇) wrote:
> > At the time I didn't, because as noted in a follow-up I'd lost my test
> > case by the time I read that, but it seems you haven't, and have a
> > current test case.
>
> I tried to find a test case in open-source projects, and finally found one.
> $ git clone https://github.com/JetBrains/intellij-community.git --bare
> $ cd intellij-community.git
> $ git repack -adb
> $ GIT_TRACE=1 git push . master:test1

I wouldn't expect this to push any objects at all, since you're pushing
to a repository that already has all of the objects contained in
`master`.

A more representative test might be something like:

    $ git clone https://github.com/JetBrains/intellij-community.git --bare
    $ cd intellij-community.git
    $ git repack -adb
    $ git rev-parse HEAD >in
    $ time git pack-objects --revs --stdout <in >/dev/null
    # move the bitmap away so we don't use it
    $ mv objects/pack/pack-*.bitmap{,.bak}
    $ time git pack-objects --revs --stdout <in >/dev/null

Thanks,
Taylor

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> +	mk_test testrepo heads/main &&
>> +	git checkout main &&
>> +	GIT_TRACE2_EVENT="$PWD/default" \
>> +	git push testrepo main:test &&
>> +	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
>> +	--thin --delta-base-offset -q --no-use-bitmap-index <default &&
>
> Nit: We tend to indent these ase we wrap, so e.g.:
>
> 	test_subcommand git ... \
> 		--thin --delta [...]
>
> The rest all looks good as far as the diff goes, if what we want to do
> is to disable this by default, and this isn't worth a re-roll in itself.
>
> But I still think that completely disabling bitmaps might be premature
> here, especially per Taylor's comment on v1 (which I understand to mean
> that they should help some of the time, even with push).

The usual way to move is to move slowly and carefully.

It may well be the case that disabling bitmaps gives users a better
default, but that is not even proven and is hard to prove.  How many
users of Git do we have?  Those silently using it happily will by
definition complain here or elsewhere, and the complaints "X is slow
with Y, so Y should be disabled when doing X" we hear tend to be
louder than "I am happily doing X with Y".

I have different problems with this patch, though.  It can use a bit
more honesty.  If you introduce a new knob and sell it as a knob
that allows disabling, be honest and keep its behaviour as
advertised.

As posted, IIUC, the patch does something quite different.  It
disables by default, and have a knob to allow it enabled again.

So, perhaps make it default on to keep the historical behaviour, and
document it as "setting it false may improve push performance without
affecting use of the reachability bitmaps for other operations.
Default is true."

Thanks.

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

[Jeff King]

On Thu, Jun 16, 2022 at 03:38:36PM +0200, Ævar Arnfjörð Bjarmason wrote:

> But the change as it stands is effectively saying "bitmaps on push
> hinder more than they help, and turning them on for push was a mistake".
> 
> Maybe that's true, but I don't think we've got any data to support
> that. Even though I've got one of those anecdotes (and from occasional
> investigations of slow "git push" I'm pretty sure this would help my
> use-cases more than it would harm them) the plural of anecdote isn't
> data.

Yeah, I'm not convinced they hinder more than they help on net. And any
problems on pushing are also things that _could_ happen on fetching, so
I think this is really a band-aid. But:

  - even if the average is improved, it's reasonable to want to avoid
    the most pathological cases

  - it's reasonable for it to be the user's choice to make, and right
    now the config isn't expressive enough to allow the choice they want

My biggest fear would be that the pathological cases are improved
(perhaps with the approach of mine that you linked earlier; thanks for
that), but that people erroneously continue to think that turning off
push.useBitmaps is a good idea due to inertia or superstition. But that
seems like a minor problem that can be addressed later if need be.

-Peff

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

[Kyle Zhao via GitGitGadget]

From: Kyle Zhao <kylezhao@tencent.com>

This allows you to disable bitmaps for "git push". Default is true.

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/

Signed-off-by: Kyle Zhao <kylezhao@tencent.com>
---
    send-pack.c: add config push.useBitmaps
    
    This patch add config push.useBitmaps to prevent git push using bitmap.
    
    The origin discussion is here:
    https://lore.kernel.org/git/b940e705fbe9454685757f2e3055e2ce@tencent.com/
    
    Thanks, -Kyle
    
    Changes since v1:
    
     * changed the commit message
     * modified and added missing \n to push.txt
     * used test_subcommand for test
     * modified "if" statement for "git_config_get_bool()" in send-pack.c
    
    Changes since v2:
    
     * enable 'push.useBitmaps" by default
     * fix nit in t/t5516-fetch-push.sh

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1263%2Fkeyu98%2Fkz%2Fpush-usebitmps-config-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1263/keyu98/kz/push-usebitmps-config-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1263

Range-diff vs v2:

 1:  42e0b4845b2 ! 1:  a523cb52542 send-pack.c: add config push.useBitmaps
     @@ Metadata
       ## Commit message ##
          send-pack.c: add config push.useBitmaps
      
     -    This allows you to disable bitmaps for "git push". Default is false.
     +    This allows you to disable bitmaps for "git push". Default is true.
      
          Reachability bitmaps are designed to speed up the "counting objects"
          phase of generating a pack during a clone or fetch. They are not
     @@ Documentation/config/push.txt: push.negotiate::
       	in common.
      +
      +push.useBitmaps::
     -+    If this config and `pack.useBitmaps` are both `true`, then Git will
     -+    use reachability bitmaps during `git push`, if available (disabled
     -+    by default).
     ++	If this config and `pack.useBitmaps` are both `true`, then Git will
     ++	use reachability bitmaps during `git push`, if available. If set to
     ++	`false`, may improve push performance without affecting use of the
     ++	reachability bitmaps for other operations. Default is true.
      
       ## send-pack.c ##
      @@ send-pack.c: 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)
     ++	if (args->no_use_bitmaps)
      +		strvec_push(&po.args, "--no-use-bitmap-index");
       	po.in = -1;
       	po.out = args->stateless_rpc ? -1 : fd;
     @@ send-pack.c: int send_pack(struct send_pack_args *args,
       		get_commons_through_negotiation(args->url, remote_refs, &commons);
       
      +	if (!git_config_get_bool("push.usebitmaps", &use_bitmaps))
     -+		args->use_bitmaps = use_bitmaps;
     ++		args->no_use_bitmaps = !use_bitmaps;
      +
       	git_config_get_bool("transfer.advertisesid", &advertise_sid);
       
     @@ send-pack.h: struct send_pack_args {
       		stateless_rpc:1,
      -		atomic:1;
      +		atomic:1,
     -+		use_bitmaps:1;
     ++		no_use_bitmaps:1;
       	const struct string_list *push_options;
       };
       
     @@ t/t5516-fetch-push.sh: test_expect_success 'push warns or fails when using usern
      +	GIT_TRACE2_EVENT="$PWD/default" \
      +	git push testrepo main:test &&
      +	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
     -+	--thin --delta-base-offset -q --no-use-bitmap-index <default &&
     ++		--thin --delta-base-offset -q <default &&
      +
      +	git config push.useBitmaps true &&
      +	GIT_TRACE2_EVENT="$PWD/true" \
      +	git push testrepo main:test2 &&
      +	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
     -+	--thin --delta-base-offset -q <true &&
     ++		--thin --delta-base-offset -q <true &&
      +
      +	git config push.useBitmaps false &&
      +	GIT_TRACE2_EVENT="$PWD/false" \
      +	git push testrepo main:test3 &&
      +	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
     -+	--thin --delta-base-offset -q --no-use-bitmap-index <false
     ++		--thin --delta-base-offset -q --no-use-bitmap-index <false
      +'
      +
       test_done


 Documentation/config/push.txt |  6 ++++++
 send-pack.c                   |  6 ++++++
 send-pack.h                   |  3 ++-
 t/t5516-fetch-push.sh         | 21 +++++++++++++++++++++
 4 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt
index e32801e6c91..f16ac9311db 100644
--- a/Documentation/config/push.txt
+++ b/Documentation/config/push.txt
@@ -137,3 +137,9 @@ push.negotiate::
 	server attempt to find commits in common. If "false", Git will
 	rely solely on the server's ref advertisement to find commits
 	in common.
+
+push.useBitmaps::
+	If this config and `pack.useBitmaps` are both `true`, then Git will
+	use reachability bitmaps during `git push`, if available. If set to
+	`false`, may improve push performance without affecting use of the
+	reachability bitmaps for other operations. Default is true.
diff --git a/send-pack.c b/send-pack.c
index bc0fcdbb000..d18cde850ef 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->no_use_bitmaps)
+		strvec_push(&po.args, "--no-use-bitmap-index");
 	po.in = -1;
 	po.out = args->stateless_rpc ? -1 : fd;
 	po.git_cmd = 1;
@@ -487,6 +489,7 @@ int send_pack(struct send_pack_args *args,
 	struct async demux;
 	const char *push_cert_nonce = NULL;
 	struct packet_reader reader;
+	int use_bitmaps;
 
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
@@ -498,6 +501,9 @@ int send_pack(struct send_pack_args *args,
 	if (push_negotiate)
 		get_commons_through_negotiation(args->url, remote_refs, &commons);
 
+	if (!git_config_get_bool("push.usebitmaps", &use_bitmaps))
+		args->no_use_bitmaps = !use_bitmaps;
+
 	git_config_get_bool("transfer.advertisesid", &advertise_sid);
 
 	/* Does the other end support the reporting? */
diff --git a/send-pack.h b/send-pack.h
index e148fcd9609..d5d6ff589d9 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -26,7 +26,8 @@ struct send_pack_args {
 		/* One of the SEND_PACK_PUSH_CERT_* constants. */
 		push_cert:2,
 		stateless_rpc:1,
-		atomic:1;
+		atomic:1,
+		no_use_bitmaps:1;
 	const struct string_list *push_options;
 };
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index dedca106a7a..ffda830ba22 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1865,4 +1865,25 @@ 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_TRACE2_EVENT="$PWD/default" \
+	git push testrepo main:test &&
+	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
+		--thin --delta-base-offset -q <default &&
+
+	git config push.useBitmaps true &&
+	GIT_TRACE2_EVENT="$PWD/true" \
+	git push testrepo main:test2 &&
+	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
+		--thin --delta-base-offset -q <true &&
+
+	git config push.useBitmaps false &&
+	GIT_TRACE2_EVENT="$PWD/false" \
+	git push testrepo main:test3 &&
+	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
+		--thin --delta-base-offset -q --no-use-bitmap-index <false
+'
+
 test_done

base-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9
-- 
gitgitgadget

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

[Junio C Hamano]

"Kyle Zhao via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  Documentation/config/push.txt |  6 ++++++
>  send-pack.c                   |  6 ++++++
>  send-pack.h                   |  3 ++-
>  t/t5516-fetch-push.sh         | 21 +++++++++++++++++++++
>  4 files changed, 35 insertions(+), 1 deletion(-)

Thanks.  This round looks more or less OK.

> diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt
> index e32801e6c91..f16ac9311db 100644
> --- a/Documentation/config/push.txt
> +++ b/Documentation/config/push.txt
> @@ -137,3 +137,9 @@ push.negotiate::
>  	server attempt to find commits in common. If "false", Git will
>  	rely solely on the server's ref advertisement to find commits
>  	in common.
> +
> +push.useBitmaps::
> +	If this config and `pack.useBitmaps` are both `true`, then Git will
> +	use reachability bitmaps during `git push`, if available. If set to
> +	`false`, may improve push performance without affecting use of the
> +	reachability bitmaps for other operations. Default is true.

While nothing in the description is incorrect per-se, I somehow find
it hard to follow.  "git push" uses reachability bitmaps when three
conditions hold true at the same time (pack.useBitmaps that is by
default true, push.useBitmaps that is by default true, and there
exist reachability bitmaps to be used).  It is left unsaid why the
user needs to know about this configuration variable in order to
tweak "without affecting other operations".

I tried to come up with a version that I find easier to read (at
least to me).  I am not sure how successfull I was.

    While git can be told to use reachability bitmaps in general
    with "pack.useBitmaps" configuration, this variable can be used
    to disable use of bitmaps only for "git push" by setting it to
    false, without preventing other git operations from using
    bitmaps.

> diff --git a/send-pack.c b/send-pack.c
> index bc0fcdbb000..d18cde850ef 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->no_use_bitmaps)
> +		strvec_push(&po.args, "--no-use-bitmap-index");

"disable_bitmaps" might be a better name for the struct member, but OK.

> @@ -498,6 +501,9 @@ int send_pack(struct send_pack_args *args,
>  	if (push_negotiate)
>  		get_commons_through_negotiation(args->url, remote_refs, &commons);
>  
> +	if (!git_config_get_bool("push.usebitmaps", &use_bitmaps))
> +		args->no_use_bitmaps = !use_bitmaps;

Because the "--no-use-bitmap-index" is explicitly given only when
the ".no_use_bitmaps" member is true and we do not pass
"--use-bitmap-index" merely because the member is false, the
configuration can only disable, which matches the documented
design.  Good.

> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index dedca106a7a..ffda830ba22 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1865,4 +1865,25 @@ 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_TRACE2_EVENT="$PWD/default" \
> +	git push testrepo main:test &&
> +	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
> +		--thin --delta-base-offset -q <default &&

We do not pass "--no-use-bitmap-index" without configuration.
Good. 

How do we know this is the "default" state, by the way?  Because we
read the tests before this, all 1860 lines?

Perhaps

	test_unconfig push.useBitmaps &&

at the very beginning of the test would help future readers.  That
way, they can immediately tell that the first test piece is without
the variable.

> +	git config push.useBitmaps true &&
> +	GIT_TRACE2_EVENT="$PWD/true" \
> +	git push testrepo main:test2 &&

Perhaps use

	test_config push.useBitmaps true

instead?  That way, after this test-expect-success finishes,
push.useBitmaps variable will be wiped from the configuration
variable.  That is a good hygiene to follow to help future
developers who may need to add more tests after this one.

> +	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
> +		--thin --delta-base-offset -q <true &&
> +
> +	git config push.useBitmaps false &&

Likewise.

> +	GIT_TRACE2_EVENT="$PWD/false" \
> +	git push testrepo main:test3 &&
> +	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
> +		--thin --delta-base-offset -q --no-use-bitmap-index <false
> +'
> +
>  test_done


Lastly, some critique on the proposed log message.

> This allows you to disable bitmaps for "git push". Default is true.

I find it more confusing to have this line at the beginning than
without.  What the configuration variable does is explained later
anyway, and our convention is not to start from such a "conclusion"
but ease the readers into the reasoning by starting with the
explanation of the status quo, which leads readers to realize why
the current system is lacking.

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

And this paragraph brilliantly does its job.  A reader who did not
even know what the reachability bitmaps are for is now prepared to
follow your logic why it is a good idea to special case "git push"
because you explain that it is good for some things and not
necessarily good for others like "git push".

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

I had the same problem with this paragraph as the one in the
documentation part of the patch.  Nothing in it is incorrect per-se,
but is roundabout way of saying what it wants to say.

Here is my attempted rewrite.

    Add a new "push.useBitmaps" configuration variable to allow users to
    tell "git push" not to use bitmaps.  We already have "pack.bitmaps"
    that controls the use of bitmaps, but a separate configuration
    variable allows the reachability bitmaps to still be used in other
    areas, such as "git rev-list --use-bitmap-index", while disabling it
    only for "git push".

Thanks.

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

[Kyle Zhao via GitGitGadget]

From: Kyle Zhao <kylezhao@tencent.com>

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" configuration variable to allow users to
tell "git push" not to use bitmaps. We already have "pack.bitmaps"
that controls the use of bitmaps, but a separate configuration variable
allows the reachability bitmaps to still be used in other areas,
such as "git upload-pack", while disabling it only for "git push".

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

Signed-off-by: Kyle Zhao <kylezhao@tencent.com>
---
    send-pack.c: add config push.useBitmaps
    
    This patch add config push.useBitmaps to prevent git push using bitmap.
    
    The origin discussion is here:
    https://lore.kernel.org/git/b940e705fbe9454685757f2e3055e2ce@tencent.com/
    
    Thanks, -Kyle
    
    Changes since v1:
    
     * changed the commit message
     * modified and added missing \n to push.txt
     * used test_subcommand for test
     * modified "if" statement for "git_config_get_bool()" in send-pack.c
    
    Changes since v2:
    
     * enable 'push.useBitmaps" by default
     * fix nit in t/t5516-fetch-push.sh
    
    Changes since v3:
    
     * changed the commit message
     * s/no_use_bitmaps/disable_bitmaps in send-pack.h and send-pack.c
     * I modified the document about "push.useBitmaps". When it is true, Git
       will keep the historical behaviour. So I mainly introduced what
       happens when it set to false.
     * use test_config and test_unconfig for test.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1263%2Fkeyu98%2Fkz%2Fpush-usebitmps-config-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1263/keyu98/kz/push-usebitmps-config-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1263

Range-diff vs v3:

 1:  a523cb52542 ! 1:  9d35d926638 send-pack.c: add config push.useBitmaps
     @@ Metadata
       ## Commit message ##
          send-pack.c: add config push.useBitmaps
      
     -    This allows you to disable bitmaps for "git push". Default is true.
     -
          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".
     +    Add a new "push.useBitmaps" configuration variable to allow users to
     +    tell "git push" not to use bitmaps. We already have "pack.bitmaps"
     +    that controls the use of bitmaps, but a separate configuration variable
     +    allows the reachability bitmaps to still be used in other areas,
     +    such as "git upload-pack", while disabling it only for "git push".
      
          [1]: https://lore.kernel.org/git/87zhoz8b9o.fsf@evledraar.gmail.com/
      
     @@ Documentation/config/push.txt: push.negotiate::
       	in common.
      +
      +push.useBitmaps::
     -+	If this config and `pack.useBitmaps` are both `true`, then Git will
     -+	use reachability bitmaps during `git push`, if available. If set to
     -+	`false`, may improve push performance without affecting use of the
     -+	reachability bitmaps for other operations. Default is true.
     ++	If set to "false", disable use of bitmaps for "git push" even if
     ++	`pack.useBitmaps` is "true", without preventing other git operations
     ++	from using bitmaps. Default is true.
      
       ## send-pack.c ##
      @@ send-pack.c: 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->no_use_bitmaps)
     ++	if (args->disable_bitmaps)
      +		strvec_push(&po.args, "--no-use-bitmap-index");
       	po.in = -1;
       	po.out = args->stateless_rpc ? -1 : fd;
     @@ send-pack.c: int send_pack(struct send_pack_args *args,
       		get_commons_through_negotiation(args->url, remote_refs, &commons);
       
      +	if (!git_config_get_bool("push.usebitmaps", &use_bitmaps))
     -+		args->no_use_bitmaps = !use_bitmaps;
     ++		args->disable_bitmaps = !use_bitmaps;
      +
       	git_config_get_bool("transfer.advertisesid", &advertise_sid);
       
     @@ send-pack.h: struct send_pack_args {
       		stateless_rpc:1,
      -		atomic:1;
      +		atomic:1,
     -+		no_use_bitmaps:1;
     ++		disable_bitmaps:1;
       	const struct string_list *push_options;
       };
       
     @@ t/t5516-fetch-push.sh: test_expect_success 'push warns or fails when using usern
      +test_expect_success 'push with config push.useBitmaps' '
      +	mk_test testrepo heads/main &&
      +	git checkout main &&
     ++	test_unconfig push.useBitmaps &&
      +	GIT_TRACE2_EVENT="$PWD/default" \
      +	git push testrepo main:test &&
      +	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
      +		--thin --delta-base-offset -q <default &&
      +
     -+	git config push.useBitmaps true &&
     ++	test_config push.useBitmaps true &&
      +	GIT_TRACE2_EVENT="$PWD/true" \
      +	git push testrepo main:test2 &&
      +	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
      +		--thin --delta-base-offset -q <true &&
      +
     -+	git config push.useBitmaps false &&
     ++	test_config push.useBitmaps false &&
      +	GIT_TRACE2_EVENT="$PWD/false" \
      +	git push testrepo main:test3 &&
      +	test_subcommand git pack-objects --all-progress-implied --revs --stdout \


 Documentation/config/push.txt |  5 +++++
 send-pack.c                   |  6 ++++++
 send-pack.h                   |  3 ++-
 t/t5516-fetch-push.sh         | 22 ++++++++++++++++++++++
 4 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt
index e32801e6c91..7386fea225a 100644
--- a/Documentation/config/push.txt
+++ b/Documentation/config/push.txt
@@ -137,3 +137,8 @@ push.negotiate::
 	server attempt to find commits in common. If "false", Git will
 	rely solely on the server's ref advertisement to find commits
 	in common.
+
+push.useBitmaps::
+	If set to "false", disable use of bitmaps for "git push" even if
+	`pack.useBitmaps` is "true", without preventing other git operations
+	from using bitmaps. Default is true.
diff --git a/send-pack.c b/send-pack.c
index bc0fcdbb000..662f7c2aeb9 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->disable_bitmaps)
+		strvec_push(&po.args, "--no-use-bitmap-index");
 	po.in = -1;
 	po.out = args->stateless_rpc ? -1 : fd;
 	po.git_cmd = 1;
@@ -487,6 +489,7 @@ int send_pack(struct send_pack_args *args,
 	struct async demux;
 	const char *push_cert_nonce = NULL;
 	struct packet_reader reader;
+	int use_bitmaps;
 
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
@@ -498,6 +501,9 @@ int send_pack(struct send_pack_args *args,
 	if (push_negotiate)
 		get_commons_through_negotiation(args->url, remote_refs, &commons);
 
+	if (!git_config_get_bool("push.usebitmaps", &use_bitmaps))
+		args->disable_bitmaps = !use_bitmaps;
+
 	git_config_get_bool("transfer.advertisesid", &advertise_sid);
 
 	/* Does the other end support the reporting? */
diff --git a/send-pack.h b/send-pack.h
index e148fcd9609..7edb80596c7 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -26,7 +26,8 @@ struct send_pack_args {
 		/* One of the SEND_PACK_PUSH_CERT_* constants. */
 		push_cert:2,
 		stateless_rpc:1,
-		atomic:1;
+		atomic:1,
+		disable_bitmaps:1;
 	const struct string_list *push_options;
 };
 
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index dedca106a7a..b3734dd2fe9 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1865,4 +1865,26 @@ 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 &&
+	test_unconfig push.useBitmaps &&
+	GIT_TRACE2_EVENT="$PWD/default" \
+	git push testrepo main:test &&
+	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
+		--thin --delta-base-offset -q <default &&
+
+	test_config push.useBitmaps true &&
+	GIT_TRACE2_EVENT="$PWD/true" \
+	git push testrepo main:test2 &&
+	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
+		--thin --delta-base-offset -q <true &&
+
+	test_config push.useBitmaps false &&
+	GIT_TRACE2_EVENT="$PWD/false" \
+	git push testrepo main:test3 &&
+	test_subcommand git pack-objects --all-progress-implied --revs --stdout \
+		--thin --delta-base-offset -q --no-use-bitmap-index <false
+'
+
 test_done

base-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9
-- 
gitgitgadget

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

[Junio C Hamano]

"Kyle Zhao via GitGitGadget" <gitgitgadget@gmail.com> writes:

>     Changes since v3:
>     
>      * changed the commit message
>      * s/no_use_bitmaps/disable_bitmaps in send-pack.h and send-pack.c
>      * I modified the document about "push.useBitmaps". When it is true, Git
>        will keep the historical behaviour. So I mainly introduced what
>        happens when it set to false.
>      * use test_config and test_unconfig for test.

Looking good.  Will queue.

Thanks.