patchsend-pack.c: add config push.useBitmaps

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

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

s/disabled/disable/

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

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

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

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

You can instead write this as:

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

Just use "err" instead of "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

On Wed, Jun 15, 2022 at 09:09:35AM -0400, Derrick Stolee wrote:

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

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

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

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.

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

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

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

On Wed, Jun 15, 2022 at 09:47:58PM +0200, Ævar Arnfjörð Bjarmason wrote:

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.

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

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.

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

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

On 6/15/2022 11:36 PM, Kyle Zhao via GitGitGadget wrote:

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

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

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

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.

Not yet, I'll try it later.

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.

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

On Thu, Jun 16, 2022 at 03:11:09PM +0000, kylezhao(赵柯宇) wrote:

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

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

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.

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

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

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

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

Thanks.  This round looks more or less OK.

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.

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

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.

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.

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.

Likewise.

Lastly, some critique on the proposed log message.

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.

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

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.

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

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

Looking good.  Will queue.

Thanks.