diff mbox series

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

Message ID pull.1263.v4.git.1655492779228.gitgitgadget@gmail.com (mailing list archive)
State Accepted
Commit 82f67ee13fb25ebed1cd722c83de49a1ac588429
Headers show
Series [v4] send-pack.c: add config push.useBitmaps | expand

Commit Message

Kyle Zhao June 17, 2022, 7:06 p.m. UTC 
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(-)


base-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9

Comments

Junio C Hamano June 17, 2022, 9:32 p.m. UTC | #1 
"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.
diff mbox series

Patch

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