{"thread":{"id":"58013","subject":"[PATCH] send-pack.c: add config push.useBitmaps","startedAt":"2022-06-15T11:09:02Z","lastAt":"2022-06-17T21:32:10Z","messageCount":17,"participants":["Kyle Zhao via GitGitGadget","Derrick Stolee","Ævar Arnfjörð Bjarmason","Taylor Blau","kylezhao (赵柯宇)","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"457269","messageId":"pull.1263.git.1655291320433.gitgitgadget@gmail.com","threadId":"58013","inReplyTo":null,"subject":"[PATCH] send-pack.c: add config push.useBitmaps","fromName":"Kyle Zhao via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-15T11:08:40Z","receivedAt":"2022-06-15T11:09:02Z","isPatch":true,"sender":{"key":"goldofzky@gmail.com","avatar":"https://gravatar.com/avatar/aa4fa27f0c1bed9c986ed596415799518e03a6e71a0931308a4de87e166e183a?d=mp&s=160"},"body":"From: Kyle Zhao <kylezhao@tencent.com>\n\nThis allows you to disabled bitmaps for \"git push\". Default is false.\n\nBitmaps are designed to speed up the \"counting objects\" phase of\nsubsequent packs created for clones and fetches.\nBut in some cases, turning bitmaps on does horrible things for \"push\"\nperformance[1].\n\n[1]: https://lore.kernel.org/git/87zhoz8b9o.fsf@evledraar.gmail.com/\n\nSigned-off-by: Kyle Zhao <kylezhao@tencent.com>\n---\n    send-pack.c: add config push.useBitmaps\n    \n    This patch add config push.useBitmaps to prevent git push using bitmap.\n    \n    The origin discussion is here:\n    https://lore.kernel.org/git/b940e705fbe9454685757f2e3055e2ce@tencent.com/\n    \n    Thanks, Kyle\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1263%2Fkeyu98%2Fkz%2Fpush-usebitmps-config-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1263/keyu98/kz/push-usebitmps-config-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1263\n\n Documentation/config/push.txt |  4 ++++\n send-pack.c                   |  6 ++++++\n send-pack.h                   |  3 ++-\n t/t5516-fetch-push.sh         | 11 +++++++++++\n 4 files changed, 23 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/push.txt b/Documentation/config/push.txt\nindex e32801e6c91..d8fb0bd1414 100644\n--- a/Documentation/config/push.txt\n+++ b/Documentation/config/push.txt\n@@ -137,3 +137,7 @@ push.negotiate::\n \tserver attempt to find commits in common. If \"false\", Git will\n \trely solely on the server's ref advertisement to find commits\n \tin common.\n+\n+push.useBitmaps::\n+\tIf this config and `pack.useBitmaps` are both \"true\", git will\n+\tuse pack bitmaps (if available) when git push. Default is false.\n\\ No newline at end of file\ndiff --git a/send-pack.c b/send-pack.c\nindex bc0fcdbb000..d6091571caa 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -84,6 +84,8 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised,\n \t\tstrvec_push(&po.args, \"--progress\");\n \tif (is_repository_shallow(the_repository))\n \t\tstrvec_push(&po.args, \"--shallow\");\n+\tif (!args->use_bitmaps)\n+\t\tstrvec_push(&po.args, \"--no-use-bitmap-index\");\n \tpo.in = -1;\n \tpo.out = args->stateless_rpc ? -1 : fd;\n \tpo.git_cmd = 1;\n@@ -482,6 +484,7 @@ int send_pack(struct send_pack_args *args,\n \tint use_push_options = 0;\n \tint push_options_supported = 0;\n \tint object_format_supported = 0;\n+\tint use_bitmaps = 0;\n \tunsigned cmds_sent = 0;\n \tint ret;\n \tstruct async demux;\n@@ -497,6 +500,9 @@ int send_pack(struct send_pack_args *args,\n \tgit_config_get_bool(\"push.negotiate\", &push_negotiate);\n \tif (push_negotiate)\n \t\tget_commons_through_negotiation(args->url, remote_refs, &commons);\n+\tgit_config_get_bool(\"push.usebitmaps\", &use_bitmaps);\n+\tif (use_bitmaps)\n+\t\targs->use_bitmaps = 1;\n \n \tgit_config_get_bool(\"transfer.advertisesid\", &advertise_sid);\n \ndiff --git a/send-pack.h b/send-pack.h\nindex e148fcd9609..f7af1b0353e 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -26,7 +26,8 @@ struct send_pack_args {\n \t\t/* One of the SEND_PACK_PUSH_CERT_* constants. */\n \t\tpush_cert:2,\n \t\tstateless_rpc:1,\n-\t\tatomic:1;\n+\t\tatomic:1,\n+\t\tuse_bitmaps:1;\n \tconst struct string_list *push_options;\n };\n \ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex dedca106a7a..ee0b912a5e8 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1865,4 +1865,15 @@ test_expect_success 'push warns or fails when using username:password' '\n \ttest_line_count = 1 warnings\n '\n \n+test_expect_success 'push with config push.useBitmaps' '\n+\tmk_test testrepo heads/main &&\n+\tgit checkout main &&\n+\tGIT_TRACE=1 git push testrepo main:test >/dev/null 2>stderr &&\n+\tgrep \"no-use-bitmap-index\" stderr &&\n+\n+\tgit config push.useBitmaps true &&\n+\tGIT_TRACE=1 git push testrepo main:test2 >/dev/null 2>stderr &&\n+\t! grep \"no-use-bitmap-index\" stderr\n+'\n+\n test_done\n\nbase-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9\n-- \ngitgitgadget\n"},{"id":"457276","messageId":"57a7393d-add0-468a-a276-5241c2c84065@github.com","threadId":"58013","inReplyTo":"pull.1263.git.1655291320433.gitgitgadget@gmail.com","subject":"Re: [PATCH] send-pack.c: add config push.useBitmaps","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-15T13:09:35Z","receivedAt":"2022-06-15T13:09:39Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/15/22 7:08 AM, Kyle Zhao via GitGitGadget wrote:\n> From: Kyle Zhao <kylezhao@tencent.com>\n> \n> This allows you to disabled bitmaps for \"git push\". Default is false.\n\ns/disabled/disable/\n\n> Bitmaps are designed to speed up the \"counting objects\" phase of\n> subsequent packs created for clones and fetches.\n> But in some cases, turning bitmaps on does horrible things for \"push\"\n> performance[1].\n\nI would rephrase this message body as follows:\n\n  Reachability bitmaps are designed to speed up the \"counting objects\"\n  phase of generating a pack during a clone or fetch. They are not\n  optimized for Git clients sending a small topic branch via \"git push\".\n  In some cases (see [1]), using reachability bitmaps during \"git push\"\n  can cause significant performance regressions.\n\n  Add a new \"push.useBitmaps\" config option to disable reachability\n  bitmaps during \"git push\". This allows reachability bitmaps to still\n  be used in other areas, such as \"git rev-list --use-bitmap-index\".\n\n> [1]: https://lore.kernel.org/git/87zhoz8b9o.fsf@evledraar.gmail.com/\n\n> +\n> +push.useBitmaps::\n> +\tIf this config and `pack.useBitmaps` are both \"true\", git will\n> +\tuse pack bitmaps (if available) when git push. Default is false.\n\nRewording slightly:\n\n  If this config and `pack.useBitmaps` are both `true`, then Git will\n  use reachability bitmaps during `git push`, if available (disabled\n  by default).\n\n> \\ No newline at end of file\n\nPlease fix this missing newline.\n\nI'm glad that this references `pack.useBitmaps`. I wonder if that\nconfig is sufficient for your purposes: do you expect to use your\nbitmaps to generate pack-files in any other way than \"git push\"?\n\nThat is: are you serving remote requests from your repo with your\nbitmaps while also using \"git push\"? Or, are you using bitmaps\nonly for things like \"git rev-list --use-bitmap-index\"?\n\nI just want to compare this with `pack.useSparse` which targets\n\"git push\", but focuses entirely on the pack-creation part of the\noperation. Since the existence of reachability bitmaps overrides\nthe sparse algorithm, this does not affect Git servers (who should\nhave a reachability bitmap).\n\nI just want to be sure that using pack.useBitmaps=false would not\nbe sufficient for your situation (and probably most situations).\n\n\n> diff --git a/send-pack.c b/send-pack.c\n> index bc0fcdbb000..d6091571caa 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -84,6 +84,8 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised,\n>  \t\tstrvec_push(&po.args, \"--progress\");\n>  \tif (is_repository_shallow(the_repository))\n>  \t\tstrvec_push(&po.args, \"--shallow\");\n> +\tif (!args->use_bitmaps)\n> +\t\tstrvec_push(&po.args, \"--no-use-bitmap-index\");\n\nHere is where you specify the lower-level pack creation only\nwhen sending a pack. It is very focused. Good.\n\n> +\tint use_bitmaps = 0;\n\n> +\tgit_config_get_bool(\"push.usebitmaps\", &use_bitmaps);\n> +\tif (use_bitmaps)\n> +\t\targs->use_bitmaps = 1;\n\nYou can instead write this as:\n\n\tif (!git_config_get_bool(\"push.usebitmaps\", &use_bitmaps))\n\t\targs->use_bitmaps = use_bitmaps;\n\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index dedca106a7a..ee0b912a5e8 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -1865,4 +1865,15 @@ test_expect_success 'push warns or fails when using username:password' '\n>  \ttest_line_count = 1 warnings\n>  '\n>  \n> +test_expect_success 'push with config push.useBitmaps' '\n> +\tmk_test testrepo heads/main &&\n> +\tgit checkout main &&\n> +\tGIT_TRACE=1 git push testrepo main:test >/dev/null 2>stderr &&\n\nJust use \"err\" instead of \"stderr\".\n\n> +\tgrep \"no-use-bitmap-index\" stderr &&\n> +\n> +\tgit config push.useBitmaps true &&\n> +\tGIT_TRACE=1 git push testrepo main:test2 >/dev/null 2>stderr &&\n> +\t! grep \"no-use-bitmap-index\" stderr\n> +'\n\nI believe this test is correct, but it might be worth looking\ninto test_subcommand if you can determine the exact subcommand\narguments you are looking for.\n\nThanks,\n-Stolee\n"},{"id":"457310","messageId":"220615.86edzpy9m6.gmgdl@evledraar.gmail.com","threadId":"58013","inReplyTo":"pull.1263.git.1655291320433.gitgitgadget@gmail.com","subject":"Re: [PATCH] send-pack.c: add config push.useBitmaps","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-15T19:47:58Z","receivedAt":"2022-06-15T20:06:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 15 2022, Kyle Zhao via GitGitGadget wrote:\n\n[CC'd Taylor, who's worked a lot on bitmaps]\n\n> From: Kyle Zhao <kylezhao@tencent.com>\n>\n> This allows you to disabled bitmaps for \"git push\". Default is false.\n\nThanks for following up.\n\nRefresh my memory, we always use them if we find them now, correct?\nI.e. if repack.writebitmaps=true\n\nThis doesn't make it clear: Are we now going to ignore them for \"push\"\nby default, even if they exist? I.e. a change in the current default.\n\nI think I recall from the previous discussions that it was a bit of hit\n& miss, maybe we're confident that they're almost (or always?) bad for\n\"push\", but I think there *are* cases where they help.\n\n> Bitmaps are designed to speed up the \"counting objects\" phase of\n> subsequent packs created for clones and fetches.\n> But in some cases, turning bitmaps on does horrible things for \"push\"\n> performance[1].\n>\n> [1]: https://lore.kernel.org/git/87zhoz8b9o.fsf@evledraar.gmail.com/\n>\n> Signed-off-by: Kyle Zhao <kylezhao@tencent.com>\n> ---\n>     send-pack.c: add config push.useBitmaps\n>     \n>     This patch add config push.useBitmaps to prevent git push using bitmap.\n>     \n>     The origin discussion is here:\n>     https://lore.kernel.org/git/b940e705fbe9454685757f2e3055e2ce@tencent.com/\n>     \n>     Thanks, Kyle\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1263%2Fkeyu98%2Fkz%2Fpush-usebitmps-config-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1263/keyu98/kz/push-usebitmps-config-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1263\n>\n>  Documentation/config/push.txt |  4 ++++\n>  send-pack.c                   |  6 ++++++\n>  send-pack.h                   |  3 ++-\n>  t/t5516-fetch-push.sh         | 11 +++++++++++\n>  4 files changed, 23 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt\n> index e32801e6c91..d8fb0bd1414 100644\n> --- a/Documentation/config/push.txt\n> +++ b/Documentation/config/push.txt\n> @@ -137,3 +137,7 @@ push.negotiate::\n>  \tserver attempt to find commits in common. If \"false\", Git will\n>  \trely solely on the server's ref advertisement to find commits\n>  \tin common.\n> +\n> +push.useBitmaps::\n> +\tIf this config and `pack.useBitmaps` are both \"true\", git will\n> +\tuse pack bitmaps (if available) when git push. Default is false.\n> \\ No newline at end of file\n\n\"git diff\" is telling you something is wrong here :) hint: missing \\n.\n\n> diff --git a/send-pack.c b/send-pack.c\n> index bc0fcdbb000..d6091571caa 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -84,6 +84,8 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised,\n>  \t\tstrvec_push(&po.args, \"--progress\");\n>  \tif (is_repository_shallow(the_repository))\n>  \t\tstrvec_push(&po.args, \"--shallow\");\n> +\tif (!args->use_bitmaps)\n> +\t\tstrvec_push(&po.args, \"--no-use-bitmap-index\");\n>  \tpo.in = -1;\n>  \tpo.out = args->stateless_rpc ? -1 : fd;\n>  \tpo.git_cmd = 1;\n> @@ -482,6 +484,7 @@ int send_pack(struct send_pack_args *args,\n>  \tint use_push_options = 0;\n>  \tint push_options_supported = 0;\n>  \tint object_format_supported = 0;\n> +\tint use_bitmaps = 0;\n>  \tunsigned cmds_sent = 0;\n>  \tint ret;\n>  \tstruct async demux;\n> @@ -497,6 +500,9 @@ int send_pack(struct send_pack_args *args,\n>  \tgit_config_get_bool(\"push.negotiate\", &push_negotiate);\n>  \tif (push_negotiate)\n>  \t\tget_commons_through_negotiation(args->url, remote_refs, &commons);\n> +\tgit_config_get_bool(\"push.usebitmaps\", &use_bitmaps);\n> +\tif (use_bitmaps)\n> +\t\targs->use_bitmaps = 1;\n\n[A bit rambling, sorry]\n\nA bit off of a use of the API, can't we just do:\n\n\tgit_config_get_bool(\"push.usebitmaps\", &args->use_bitmaps);\n\nAnd drop the local variable? I.e. if we have a config variable we'll\nwrite the value to it.\n\nBut then again that goes with the suggested default, i.e. I think we\nshould probably use them by default, and provide an out, unless we're\n*really* sure they're useless for \"push\".\n\nIn this case I found the code a bit odd, which took me a moment to put\nmy finger on.\n\nIn builtin/send-pack.c we initialize \"struct send_pack_args\" in the file\nscope, so it's zero'd out.\n\nSo all parameters are 0'd by default.\n\nSo your new \"use_bitmaps\" is born false.\n\nThen when we get here you assign 0 to use_bitmaps, and only if it's true\ndo you use it.\n\nWhich to me is a couple of layers in to something that's less clear than\nit could be, i.e. we're making the hard assumption here that the default\nin the struct is false. So we should really just do:\n\n\tint tmp;\n\tif (!git_config_get_bool(\"push.usebitmaps\", &tmp))\n\t\targs->use_bitmaps = tmp;\n\nSo don't care what the default was before, we have an explicit config\nvariable we're going to use, if we saw push.usebitmaps let's use its\nvalue.\n\nThis way the default can flip, and this code downstream of that will\nstill do what's intended.\n\nFWIW that \"if\" is redundant, but it's the general idiom, but we *can* do\nit the way I suggested above, but I think it's clearer to go with the\nsecond form, which reads more obviously as \"if the variable exists, set\nit to ...\".\n\nFor config.c in particular the \"without the if\" works, but I think\nthat's relying on an implementation detail, i.e. we have a few other\nAPIs that \"zero out\" the parameter you put in as the first thing they\ndo. So if they also return \"I didn't error\" you want to use a temp\nvariable, and only assign it to your \"real\" variable if the return value\nwas \"OK\".\n\n>  \n>  \tgit_config_get_bool(\"transfer.advertisesid\", &advertise_sid);\n>  \n> diff --git a/send-pack.h b/send-pack.h\n> index e148fcd9609..f7af1b0353e 100644\n> --- a/send-pack.h\n> +++ b/send-pack.h\n> @@ -26,7 +26,8 @@ struct send_pack_args {\n>  \t\t/* One of the SEND_PACK_PUSH_CERT_* constants. */\n>  \t\tpush_cert:2,\n>  \t\tstateless_rpc:1,\n> -\t\tatomic:1;\n> +\t\tatomic:1,\n> +\t\tuse_bitmaps:1;\n>  \tconst struct string_list *push_options;\n>  };\n>  \n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index dedca106a7a..ee0b912a5e8 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -1865,4 +1865,15 @@ test_expect_success 'push warns or fails when using username:password' '\n>  \ttest_line_count = 1 warnings\n>  '\n>  \n> +test_expect_success 'push with config push.useBitmaps' '\n> +\tmk_test testrepo heads/main &&\n> +\tgit checkout main &&\n> +\tGIT_TRACE=1 git push testrepo main:test >/dev/null 2>stderr &&\n\nWe generally don't >/dev/null in tests, just emit the output, and if we\nrun with -v you'll see it.\n\nIn this case though you want just:\n\n    GIT_TRACE=\"$PWD/trace.log\" [...]\n\nWithout any redirection,\n\nAlso, you probably want GIT_TRACE2_EVENT=$PWD/trace.json, and see\n\"test_subcommand\". We have a handy helper for finding if we have trace2\nregions.\n\nI'm assuming we have some region for \"used bitmaps\" already, but I\ndidn't check...\n"},{"id":"457319","messageId":"YqpN9FnBmdfLQFF2@nand.local","threadId":"58013","inReplyTo":"57a7393d-add0-468a-a276-5241c2c84065@github.com","subject":"Re: [PATCH] send-pack.c: add config push.useBitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-15T21:24:04Z","receivedAt":"2022-06-15T21:24:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jun 15, 2022 at 09:09:35AM -0400, Derrick Stolee wrote:\n> I just want to be sure that using pack.useBitmaps=false would not\n> be sufficient for your situation (and probably most situations).\n\nI think the only other affected scenario on the client side would be\nrepacking. And in theory most clients are repacking in the background\nanyways, so any speed-ups from using bitmaps wouldn't be noticeable\nanyway.\n\nI think just relying on the existing pack.useBitmaps config should be\nsufficient here.\n\nThanks,\nTaylor\n"},{"id":"457320","messageId":"YqpP0b2zt0sWZD0B@nand.local","threadId":"58013","inReplyTo":"220615.86edzpy9m6.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] send-pack.c: add config push.useBitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-15T21:32:01Z","receivedAt":"2022-06-15T21:32:12Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jun 15, 2022 at 09:47:58PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Refresh my memory, we always use them if we find them now, correct?\n> I.e. if repack.writebitmaps=true\n\nKind of. `rev-list` has an opt-in `--use-bitmap-index` option, and\n`pack-objects` has `pack.useBitmaps` which controls whether or not\nbitmaps are read.\n\nSo `git rev-list --objects HEAD` won't use any bitmaps, even if they\nexist, but `git rev-list --objects --use-bitmap-index HEAD` will. There\nthere's a good reason not to use bitmaps by default, which is that they\n(effectively) randomize the ordering of your output.\n\n`pack-objects` behaves slightly differently, cf. its\n`use_bitmap_index{,default}` variables to see how that works.\n\n> This doesn't make it clear: Are we now going to ignore them for \"push\"\n> by default, even if they exist? I.e. a change in the current default.\n>\n> I think I recall from the previous discussions that it was a bit of hit\n> & miss, maybe we're confident that they're almost (or always?) bad for\n> \"push\", but I think there *are* cases where they help.\n\nYeah. In theory they should help most of the time as long as the bitmaps\nare kept reasonably up-to-date. There's a tradeoff between using bitmaps\nand how much walking between bitmap and ref-tips is required. Since\nevery object we encounter between the most recent bitmap and the thing\nwe're trying to generate a bitmap for has to incur extra overhead in\norder to find and mark its position in the bitmap being generated,\nthere's a certain point at which it would have been faster to just walk\ndown to the roots and avoid bitmaps altogether.\n\nBut I suspect that in this case the bitmaps are just simply stale, and\nthat a \"git repack -adb\" or more aggressive background maintenance would\nmake things quite a bit faster.\n\nThanks,\nTaylor\n"},{"id":"457338","messageId":"0c9a4b31a3884fad8b0b88c7892a7071@tencent.com","threadId":"58013","inReplyTo":"YqpP0b2zt0sWZD0B@nand.local","subject":"Re: [Internet]Re: [PATCH] send-pack.c: add config push.useBitmaps","fromName":"kylezhao (赵柯宇)","fromEmail":"kylezhao@tencent.com","sentAt":"2022-06-16T02:13:12Z","receivedAt":"2022-06-16T02:13:45Z","isPatch":true,"sender":{"key":"kylezhao@tencent.com","avatar":"https://avatars.githubusercontent.com/u/34176272?v=4"},"body":"> > 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 \n> > sufficient for your situation (and probably most situations).\n> \n> 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.\n> \n> I think just relying on the existing pack.useBitmaps config should be sufficient here.\n\nIn fact ,we also use \"git push\" on our server side. \nEach git repositories have multiple replicas on our servers to improve system disaster tolerance and read performance.\n\nFor example, a git repo will be distributed on multiple servers (like server-1, server-2, server-3).\nIf 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.\nAnd users can clone from all the server mentioned above.\nUnder such a process, our system works well most of the time.\n\nIf we set pack.useBitmaps=false, \"git upload-pack\" will also be affected.\nFor my situation, it would be sufficient when set both pack.useBitmaps=true and push.useBitmaps=false.\n\n> But I suspect that in this case the bitmaps are just simply stale, and\n> that a \"git repack -adb\" or more aggressive background maintenance would\n> make things quite a bit faster.\n\nIt doesn't seem to be the reason.\nI have already called  \"git repack -adb\" in this case[1] but that didn't seem to fix the push performance issue.\nYou can see that the git repo had only one pack at that time.\n\n[1] https://lore.kernel.org/git/b940e705fbe9454685757f2e3055e2ce@tencent.com/\n\nThanks,\n- Kyle"},{"id":"457339","messageId":"pull.1263.v2.git.1655350617442.gitgitgadget@gmail.com","threadId":"58013","inReplyTo":"pull.1263.git.1655291320433.gitgitgadget@gmail.com","subject":"[PATCH v2] send-pack.c: add config push.useBitmaps","fromName":"Kyle Zhao via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-16T03:36:57Z","receivedAt":"2022-06-16T03:37:06Z","isPatch":true,"sender":{"key":"goldofzky@gmail.com","avatar":"https://gravatar.com/avatar/aa4fa27f0c1bed9c986ed596415799518e03a6e71a0931308a4de87e166e183a?d=mp&s=160"},"body":"From: Kyle Zhao <kylezhao@tencent.com>\n\nThis allows you to disable bitmaps for \"git push\". Default is false.\n\nReachability bitmaps are designed to speed up the \"counting objects\"\nphase of generating a pack during a clone or fetch. They are not\noptimized for Git clients sending a small topic branch via \"git push\".\nIn some cases (see [1]), using reachability bitmaps during \"git push\"\ncan cause significant performance regressions.\n\nAdd a new \"push.useBitmaps\" config option to disable reachability\nbitmaps during \"git push\". This allows reachability bitmaps to still\nbe used in other areas, such as \"git rev-list --use-bitmap-index\".\n\n[1]: https://lore.kernel.org/git/87zhoz8b9o.fsf@evledraar.gmail.com/\n\nSigned-off-by: Kyle Zhao <kylezhao@tencent.com>\n---\n    send-pack.c: add config push.useBitmaps\n    \n    This patch add config push.useBitmaps to prevent git push using bitmap.\n    \n    The origin discussion is here:\n    https://lore.kernel.org/git/b940e705fbe9454685757f2e3055e2ce@tencent.com/\n    \n    Thanks, -Kyle\n    \n    Changes since v1:\n    \n     * changed the commit message\n     * modified and added missing \\n to push.txt\n     * used test_subcommand for test\n     * modified \"if\" statement for \"git_config_get_bool()\" in send-pack.c\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1263%2Fkeyu98%2Fkz%2Fpush-usebitmps-config-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1263/keyu98/kz/push-usebitmps-config-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1263\n\nRange-diff vs v1:\n\n 1:  000d033584b ! 1:  42e0b4845b2 send-pack.c: add config push.useBitmaps\n     @@ Metadata\n       ## Commit message ##\n          send-pack.c: add config push.useBitmaps\n      \n     -    This allows you to disabled bitmaps for \"git push\". Default is false.\n     +    This allows you to disable bitmaps for \"git push\". Default is false.\n      \n     -    Bitmaps are designed to speed up the \"counting objects\" phase of\n     -    subsequent packs created for clones and fetches.\n     -    But in some cases, turning bitmaps on does horrible things for \"push\"\n     -    performance[1].\n     +    Reachability bitmaps are designed to speed up the \"counting objects\"\n     +    phase of generating a pack during a clone or fetch. They are not\n     +    optimized for Git clients sending a small topic branch via \"git push\".\n     +    In some cases (see [1]), using reachability bitmaps during \"git push\"\n     +    can cause significant performance regressions.\n     +\n     +    Add a new \"push.useBitmaps\" config option to disable reachability\n     +    bitmaps during \"git push\". This allows reachability bitmaps to still\n     +    be used in other areas, such as \"git rev-list --use-bitmap-index\".\n      \n          [1]: https://lore.kernel.org/git/87zhoz8b9o.fsf@evledraar.gmail.com/\n      \n     @@ Documentation/config/push.txt: push.negotiate::\n       \tin common.\n      +\n      +push.useBitmaps::\n     -+\tIf this config and `pack.useBitmaps` are both \"true\", git will\n     -+\tuse pack bitmaps (if available) when git push. Default is false.\n     - \\ No newline at end of file\n     ++    If this config and `pack.useBitmaps` are both `true`, then Git will\n     ++    use reachability bitmaps during `git push`, if available (disabled\n     ++    by default).\n      \n       ## send-pack.c ##\n      @@ send-pack.c: static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised,\n     @@ send-pack.c: static int pack_objects(int fd, struct ref *refs, struct oid_array\n       \tpo.out = args->stateless_rpc ? -1 : fd;\n       \tpo.git_cmd = 1;\n      @@ send-pack.c: int send_pack(struct send_pack_args *args,\n     - \tint use_push_options = 0;\n     - \tint push_options_supported = 0;\n     - \tint object_format_supported = 0;\n     -+\tint use_bitmaps = 0;\n     - \tunsigned cmds_sent = 0;\n     - \tint ret;\n       \tstruct async demux;\n     + \tconst char *push_cert_nonce = NULL;\n     + \tstruct packet_reader reader;\n     ++\tint use_bitmaps;\n     + \n     + \tif (!remote_refs) {\n     + \t\tfprintf(stderr, \"No refs in common and none specified; doing nothing.\\n\"\n      @@ send-pack.c: int send_pack(struct send_pack_args *args,\n     - \tgit_config_get_bool(\"push.negotiate\", &push_negotiate);\n       \tif (push_negotiate)\n       \t\tget_commons_through_negotiation(args->url, remote_refs, &commons);\n     -+\tgit_config_get_bool(\"push.usebitmaps\", &use_bitmaps);\n     -+\tif (use_bitmaps)\n     -+\t\targs->use_bitmaps = 1;\n       \n     ++\tif (!git_config_get_bool(\"push.usebitmaps\", &use_bitmaps))\n     ++\t\targs->use_bitmaps = use_bitmaps;\n     ++\n       \tgit_config_get_bool(\"transfer.advertisesid\", &advertise_sid);\n       \n     + \t/* Does the other end support the reporting? */\n      \n       ## send-pack.h ##\n      @@ send-pack.h: struct send_pack_args {\n     @@ t/t5516-fetch-push.sh: test_expect_success 'push warns or fails when using usern\n      +test_expect_success 'push with config push.useBitmaps' '\n      +\tmk_test testrepo heads/main &&\n      +\tgit checkout main &&\n     -+\tGIT_TRACE=1 git push testrepo main:test >/dev/null 2>stderr &&\n     -+\tgrep \"no-use-bitmap-index\" stderr &&\n     ++\tGIT_TRACE2_EVENT=\"$PWD/default\" \\\n     ++\tgit push testrepo main:test &&\n     ++\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n     ++\t--thin --delta-base-offset -q --no-use-bitmap-index <default &&\n      +\n      +\tgit config push.useBitmaps true &&\n     -+\tGIT_TRACE=1 git push testrepo main:test2 >/dev/null 2>stderr &&\n     -+\t! grep \"no-use-bitmap-index\" stderr\n     ++\tGIT_TRACE2_EVENT=\"$PWD/true\" \\\n     ++\tgit push testrepo main:test2 &&\n     ++\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n     ++\t--thin --delta-base-offset -q <true &&\n     ++\n     ++\tgit config push.useBitmaps false &&\n     ++\tGIT_TRACE2_EVENT=\"$PWD/false\" \\\n     ++\tgit push testrepo main:test3 &&\n     ++\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n     ++\t--thin --delta-base-offset -q --no-use-bitmap-index <false\n      +'\n      +\n       test_done\n\n\n Documentation/config/push.txt |  5 +++++\n send-pack.c                   |  6 ++++++\n send-pack.h                   |  3 ++-\n t/t5516-fetch-push.sh         | 21 +++++++++++++++++++++\n 4 files changed, 34 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/push.txt b/Documentation/config/push.txt\nindex e32801e6c91..3f3ff66fe7c 100644\n--- a/Documentation/config/push.txt\n+++ b/Documentation/config/push.txt\n@@ -137,3 +137,8 @@ push.negotiate::\n \tserver attempt to find commits in common. If \"false\", Git will\n \trely solely on the server's ref advertisement to find commits\n \tin common.\n+\n+push.useBitmaps::\n+    If this config and `pack.useBitmaps` are both `true`, then Git will\n+    use reachability bitmaps during `git push`, if available (disabled\n+    by default).\ndiff --git a/send-pack.c b/send-pack.c\nindex bc0fcdbb000..627e79d7623 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -84,6 +84,8 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised,\n \t\tstrvec_push(&po.args, \"--progress\");\n \tif (is_repository_shallow(the_repository))\n \t\tstrvec_push(&po.args, \"--shallow\");\n+\tif (!args->use_bitmaps)\n+\t\tstrvec_push(&po.args, \"--no-use-bitmap-index\");\n \tpo.in = -1;\n \tpo.out = args->stateless_rpc ? -1 : fd;\n \tpo.git_cmd = 1;\n@@ -487,6 +489,7 @@ int send_pack(struct send_pack_args *args,\n \tstruct async demux;\n \tconst char *push_cert_nonce = NULL;\n \tstruct packet_reader reader;\n+\tint use_bitmaps;\n \n \tif (!remote_refs) {\n \t\tfprintf(stderr, \"No refs in common and none specified; doing nothing.\\n\"\n@@ -498,6 +501,9 @@ int send_pack(struct send_pack_args *args,\n \tif (push_negotiate)\n \t\tget_commons_through_negotiation(args->url, remote_refs, &commons);\n \n+\tif (!git_config_get_bool(\"push.usebitmaps\", &use_bitmaps))\n+\t\targs->use_bitmaps = use_bitmaps;\n+\n \tgit_config_get_bool(\"transfer.advertisesid\", &advertise_sid);\n \n \t/* Does the other end support the reporting? */\ndiff --git a/send-pack.h b/send-pack.h\nindex e148fcd9609..f7af1b0353e 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -26,7 +26,8 @@ struct send_pack_args {\n \t\t/* One of the SEND_PACK_PUSH_CERT_* constants. */\n \t\tpush_cert:2,\n \t\tstateless_rpc:1,\n-\t\tatomic:1;\n+\t\tatomic:1,\n+\t\tuse_bitmaps:1;\n \tconst struct string_list *push_options;\n };\n \ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex dedca106a7a..0d416d1474f 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1865,4 +1865,25 @@ test_expect_success 'push warns or fails when using username:password' '\n \ttest_line_count = 1 warnings\n '\n \n+test_expect_success 'push with config push.useBitmaps' '\n+\tmk_test testrepo heads/main &&\n+\tgit checkout main &&\n+\tGIT_TRACE2_EVENT=\"$PWD/default\" \\\n+\tgit push testrepo main:test &&\n+\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n+\t--thin --delta-base-offset -q --no-use-bitmap-index <default &&\n+\n+\tgit config push.useBitmaps true &&\n+\tGIT_TRACE2_EVENT=\"$PWD/true\" \\\n+\tgit push testrepo main:test2 &&\n+\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n+\t--thin --delta-base-offset -q <true &&\n+\n+\tgit config push.useBitmaps false &&\n+\tGIT_TRACE2_EVENT=\"$PWD/false\" \\\n+\tgit push testrepo main:test3 &&\n+\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n+\t--thin --delta-base-offset -q --no-use-bitmap-index <false\n+'\n+\n test_done\n\nbase-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9\n-- \ngitgitgadget\n"},{"id":"457366","messageId":"e2ea1475-ce21-ef52-7301-3ed074c40f9b@github.com","threadId":"58013","inReplyTo":"pull.1263.v2.git.1655350617442.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] send-pack.c: add config push.useBitmaps","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-16T13:02:23Z","receivedAt":"2022-06-16T13:02:32Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/15/2022 11:36 PM, Kyle Zhao via GitGitGadget wrote:\n> From: Kyle Zhao <kylezhao@tencent.com>\n> \n> This allows you to disable bitmaps for \"git push\". Default is false.\n> \n> Reachability bitmaps are designed to speed up the \"counting objects\"\n> phase of generating a pack during a clone or fetch. They are not\n> optimized for Git clients sending a small topic branch via \"git push\".\n> In some cases (see [1]), using reachability bitmaps during \"git push\"\n> can cause significant performance regressions.\n> \n> Add a new \"push.useBitmaps\" config option to disable reachability\n> bitmaps during \"git push\". This allows reachability bitmaps to still\n> be used in other areas, such as \"git rev-list --use-bitmap-index\".\n> \n> [1]: https://lore.kernel.org/git/87zhoz8b9o.fsf@evledraar.gmail.com/\n> \n> Signed-off-by: Kyle Zhao <kylezhao@tencent.com>\n> ---\n>     send-pack.c: add config push.useBitmaps\n>     \n>     This patch add config push.useBitmaps to prevent git push using bitmap.\n>     \n>     The origin discussion is here:\n>     https://lore.kernel.org/git/b940e705fbe9454685757f2e3055e2ce@tencent.com/\n>     \n>     Thanks, -Kyle\n>     \n>     Changes since v1:\n>     \n>      * changed the commit message\n>      * modified and added missing \\n to push.txt\n>      * used test_subcommand for test\n>      * modified \"if\" statement for \"git_config_get_bool()\" in send-pack.c\n\nThis version satisfies the recommended changes I had from the last\nversion. As long as we think there is a benefit to having this\nadditional knob over just pack.useBitmaps, then this is good to go.\n\nFor my part, I think having the additional knob is less complicated\nthan requiring all users in this situation to shift from \"push\" to\na \"fetch\" (from the other side of the connection).\n\nThanks,\n-Stolee\n"},{"id":"457378","messageId":"220616.86fsk4ww69.gmgdl@evledraar.gmail.com","threadId":"58013","inReplyTo":"pull.1263.v2.git.1655350617442.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] send-pack.c: add config push.useBitmaps","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-16T13:38:36Z","receivedAt":"2022-06-16T13:54:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jun 16 2022, Kyle Zhao via GitGitGadget wrote:\n\n> From: Kyle Zhao <kylezhao@tencent.com>\n>\n> This allows you to disable bitmaps for \"git push\". Default is false.\n>\n> Reachability bitmaps are designed to speed up the \"counting objects\"\n> phase of generating a pack during a clone or fetch. They are not\n> optimized for Git clients sending a small topic branch via \"git push\".\n> In some cases (see [1]), using reachability bitmaps during \"git push\"\n> can cause significant performance regressions.\n>\n> Add a new \"push.useBitmaps\" config option to disable reachability\n> bitmaps during \"git push\". This allows reachability bitmaps to still\n> be used in other areas, such as \"git rev-list --use-bitmap-index\".\n>\n> [1]: https://lore.kernel.org/git/87zhoz8b9o.fsf@evledraar.gmail.com/\n> [...]\n> +\tmk_test testrepo heads/main &&\n> +\tgit checkout main &&\n> +\tGIT_TRACE2_EVENT=\"$PWD/default\" \\\n> +\tgit push testrepo main:test &&\n> +\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n> +\t--thin --delta-base-offset -q --no-use-bitmap-index <default &&\n\nNit: We tend to indent these ase we wrap, so e.g.:\n\n\ttest_subcommand git ... \\\n\t\t--thin --delta [...]\n\nThe rest all looks good as far as the diff goes, if what we want to do\nis to disable this by default, and this isn't worth a re-roll in itself.\n\nBut I still think that completely disabling bitmaps might be premature\nhere, especially per Taylor's comment on v1 (which I understand to mean\nthat they should help some of the time, even with push).\n\nAnd since this is linking to that old thread I started in 2019 I really\nthink the commit message should be exploring and justifying why this\nmight be slower in all cases, or at least a sufficient number of cases\nto flip the default.\n\nif it's not are we throwing out a generally useful optimization (by\ndefault) due to some edge cases we've found?\n\nE.g. have you tried to follow-up on this comment by Jeff King?\nhttps://lore.kernel.org/git/20190523113031.GA17448@sigill.intra.peff.net/\n\nAt the time I didn't, because as noted in a follow-up I'd lost my test\ncase by the time I read that, but it seems you haven't, and have a\ncurrent test case.\n\nIf you apply that patch on that old git version at the time (as it\nprobably won't apply cleanly), does it make a difference for your case?\n\nTo be clear I'm *not* confident that this isn't the right thing to do as\nfar as the diff is concerned. But I *am* confident that the proposed\ncommit message isn't selling me on the idea.\n\nI think a really good start would be to come up with some benchmarks for\na few common cases, e.g. how do bitmaps do if we have a big repo but few\nrefs, how about a lot of refs? Does their stale-ness make a difference\nor not (per previous discussions) etc. etc.\n\nOr, if you don't have time for any of that I think a good change for now\nwould be to add the flag, but not flip the default, and say \"we don't\nhave enough data to say if bitmaps are overall worse for pushes, but\nhere's an opt-opt\".\n\nBut the change as it stands is effectively saying \"bitmaps on push\nhinder more than they help, and turning them on for push was a mistake\".\n\nMaybe that's true, but I don't think we've got any data to support\nthat. Even though I've got one of those anecdotes (and from occasional\ninvestigations of slow \"git push\" I'm pretty sure this would help my\nuse-cases more than it would harm them) the plural of anecdote isn't\ndata.\n\nI've only personally run into this with repos with a large number (>30k\nat least) number of refs, and where the bitmaps were in some state of\nstaleness (this usually being my working copy, where I rely on \"git gc\n--auto\").\n"},{"id":"457383","messageId":"be5afab25a5b44b3af797c0c8b75f5e6@tencent.com","threadId":"58013","inReplyTo":"220616.86fsk4ww69.gmgdl@evledraar.gmail.com","subject":"Re: [Internet]Re: [PATCH v2] send-pack.c: add config push.useBitmaps","fromName":"kylezhao (赵柯宇)","fromEmail":"kylezhao@tencent.com","sentAt":"2022-06-16T15:11:09Z","receivedAt":"2022-06-16T15:11:27Z","isPatch":true,"sender":{"key":"kylezhao@tencent.com","avatar":"https://avatars.githubusercontent.com/u/34176272?v=4"},"body":"> And since this is linking to that old thread I started in 2019 I really\n>think the commit message should be exploring and justifying why this\n> might be slower in all cases, or at least a sufficient number of cases\n> to flip the default.\n> \n> if it's not are we throwing out a generally useful optimization (by\n> default) due to some edge cases we've found?\n\nI'm not sure if it's an edge case.\nHowever, if a git repo has this problem, it's \"git push\" will always be very slow\ninstead of going fast and slow.\n\n> E.g. have you tried to follow-up on this comment by Jeff King?\n> https://lore.kernel.org/git/20190523113031.GA17448@sigill.intra.peff.net/\n\nNot yet, I'll try it later.\n\n> At the time I didn't, because as noted in a follow-up I'd lost my test\n> case by the time I read that, but it seems you haven't, and have a\n> current test case.\n\nI tried to find a test case in open-source projects, and finally found one.\n$ git clone https://github.com/JetBrains/intellij-community.git --bare\n$ cd intellij-community.git\n$ git repack -adb\n$ GIT_TRACE=1 git push . master:test1\n\nThen it would get stuck in \"git pack-objects\" for about 10s. (OS: Windows)\nAfter I deleted the bitmap, \"git push\" took less than 1s.\n\n\n> I think a really good start would be to come up with some benchmarks for\n> a few common cases, e.g. how do bitmaps do if we have a big repo but few\n> refs, how about a lot of refs? Does their stale-ness make a difference\n> or not (per previous discussions) etc. etc.\n\nAccording to my observation, the problem always occur on git repos which have \na lot of refs and objects.\nAlso, \"git push\"  takes much time on \"find _objects(..) for haves_bitmap\".\n\nHope the information above can help find the cause of the problem.\n\nThanks,\n- Kyle\n\n"},{"id":"457394","messageId":"xmqqfsk4tr3n.fsf@gitster.g","threadId":"58013","inReplyTo":"220616.86fsk4ww69.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] send-pack.c: add config push.useBitmaps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-16T18:12:28Z","receivedAt":"2022-06-16T18:12:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> +\tmk_test testrepo heads/main &&\n>> +\tgit checkout main &&\n>> +\tGIT_TRACE2_EVENT=\"$PWD/default\" \\\n>> +\tgit push testrepo main:test &&\n>> +\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n>> +\t--thin --delta-base-offset -q --no-use-bitmap-index <default &&\n>\n> Nit: We tend to indent these ase we wrap, so e.g.:\n>\n> \ttest_subcommand git ... \\\n> \t\t--thin --delta [...]\n>\n> The rest all looks good as far as the diff goes, if what we want to do\n> is to disable this by default, and this isn't worth a re-roll in itself.\n>\n> But I still think that completely disabling bitmaps might be premature\n> here, especially per Taylor's comment on v1 (which I understand to mean\n> that they should help some of the time, even with push).\n\nThe usual way to move is to move slowly and carefully.\n\nIt may well be the case that disabling bitmaps gives users a better\ndefault, but that is not even proven and is hard to prove.  How many\nusers of Git do we have?  Those silently using it happily will by\ndefinition complain here or elsewhere, and the complaints \"X is slow\nwith Y, so Y should be disabled when doing X\" we hear tend to be\nlouder than \"I am happily doing X with Y\".\n\nI have different problems with this patch, though.  It can use a bit\nmore honesty.  If you introduce a new knob and sell it as a knob\nthat allows disabling, be honest and keep its behaviour as\nadvertised.\n\nAs posted, IIUC, the patch does something quite different.  It\ndisables by default, and have a knob to allow it enabled again.\n\nSo, perhaps make it default on to keep the historical behaviour, and\ndocument it as \"setting it false may improve push performance without\naffecting use of the reachability bitmaps for other operations.\nDefault is true.\"\n\nThanks.\n"},{"id":"457416","messageId":"Yqua243d7FPqWxQH@coredump.intra.peff.net","threadId":"58013","inReplyTo":"220616.86fsk4ww69.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] send-pack.c: add config push.useBitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-16T21:04:27Z","receivedAt":"2022-06-16T21:04:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 16, 2022 at 03:38:36PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> But the change as it stands is effectively saying \"bitmaps on push\n> hinder more than they help, and turning them on for push was a mistake\".\n> \n> Maybe that's true, but I don't think we've got any data to support\n> that. Even though I've got one of those anecdotes (and from occasional\n> investigations of slow \"git push\" I'm pretty sure this would help my\n> use-cases more than it would harm them) the plural of anecdote isn't\n> data.\n\nYeah, I'm not convinced they hinder more than they help on net. And any\nproblems on pushing are also things that _could_ happen on fetching, so\nI think this is really a band-aid. But:\n\n  - even if the average is improved, it's reasonable to want to avoid\n    the most pathological cases\n\n  - it's reasonable for it to be the user's choice to make, and right\n    now the config isn't expressive enough to allow the choice they want\n\nMy biggest fear would be that the pathological cases are improved\n(perhaps with the approach of mine that you linked earlier; thanks for\nthat), but that people erroneously continue to think that turning off\npush.useBitmaps is a good idea due to inertia or superstition. But that\nseems like a minor problem that can be addressed later if need be.\n\n-Peff\n"},{"id":"457419","messageId":"Yqud+Y6LgkbmBOBj@nand.local","threadId":"58013","inReplyTo":"be5afab25a5b44b3af797c0c8b75f5e6@tencent.com","subject":"Re: [Internet]Re: [PATCH v2] send-pack.c: add config push.useBitmaps","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-16T21:17:45Z","receivedAt":"2022-06-16T21:17:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jun 16, 2022 at 03:11:09PM +0000, kylezhao(赵柯宇) wrote:\n> > At the time I didn't, because as noted in a follow-up I'd lost my test\n> > case by the time I read that, but it seems you haven't, and have a\n> > current test case.\n>\n> I tried to find a test case in open-source projects, and finally found one.\n> $ git clone https://github.com/JetBrains/intellij-community.git --bare\n> $ cd intellij-community.git\n> $ git repack -adb\n> $ GIT_TRACE=1 git push . master:test1\n\nI wouldn't expect this to push any objects at all, since you're pushing\nto a repository that already has all of the objects contained in\n`master`.\n\nA more representative test might be something like:\n\n    $ git clone https://github.com/JetBrains/intellij-community.git --bare\n    $ cd intellij-community.git\n    $ git repack -adb\n    $ git rev-parse HEAD >in\n    $ time git pack-objects --revs --stdout <in >/dev/null\n    # move the bitmap away so we don't use it\n    $ mv objects/pack/pack-*.bitmap{,.bak}\n    $ time git pack-objects --revs --stdout <in >/dev/null\n\nThanks,\nTaylor\n"},{"id":"457447","messageId":"pull.1263.v3.git.1655438361228.gitgitgadget@gmail.com","threadId":"58013","inReplyTo":"pull.1263.v2.git.1655350617442.gitgitgadget@gmail.com","subject":"[PATCH v3] send-pack.c: add config push.useBitmaps","fromName":"Kyle Zhao via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-17T03:59:20Z","receivedAt":"2022-06-17T03:59:31Z","isPatch":true,"sender":{"key":"goldofzky@gmail.com","avatar":"https://gravatar.com/avatar/aa4fa27f0c1bed9c986ed596415799518e03a6e71a0931308a4de87e166e183a?d=mp&s=160"},"body":"From: Kyle Zhao <kylezhao@tencent.com>\n\nThis allows you to disable bitmaps for \"git push\". Default is true.\n\nReachability bitmaps are designed to speed up the \"counting objects\"\nphase of generating a pack during a clone or fetch. They are not\noptimized for Git clients sending a small topic branch via \"git push\".\nIn some cases (see [1]), using reachability bitmaps during \"git push\"\ncan cause significant performance regressions.\n\nAdd a new \"push.useBitmaps\" config option to disable reachability\nbitmaps during \"git push\". This allows reachability bitmaps to still\nbe used in other areas, such as \"git rev-list --use-bitmap-index\".\n\n[1]: https://lore.kernel.org/git/87zhoz8b9o.fsf@evledraar.gmail.com/\n\nSigned-off-by: Kyle Zhao <kylezhao@tencent.com>\n---\n    send-pack.c: add config push.useBitmaps\n    \n    This patch add config push.useBitmaps to prevent git push using bitmap.\n    \n    The origin discussion is here:\n    https://lore.kernel.org/git/b940e705fbe9454685757f2e3055e2ce@tencent.com/\n    \n    Thanks, -Kyle\n    \n    Changes since v1:\n    \n     * changed the commit message\n     * modified and added missing \\n to push.txt\n     * used test_subcommand for test\n     * modified \"if\" statement for \"git_config_get_bool()\" in send-pack.c\n    \n    Changes since v2:\n    \n     * enable 'push.useBitmaps\" by default\n     * fix nit in t/t5516-fetch-push.sh\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1263%2Fkeyu98%2Fkz%2Fpush-usebitmps-config-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1263/keyu98/kz/push-usebitmps-config-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1263\n\nRange-diff vs v2:\n\n 1:  42e0b4845b2 ! 1:  a523cb52542 send-pack.c: add config push.useBitmaps\n     @@ Metadata\n       ## Commit message ##\n          send-pack.c: add config push.useBitmaps\n      \n     -    This allows you to disable bitmaps for \"git push\". Default is false.\n     +    This allows you to disable bitmaps for \"git push\". Default is true.\n      \n          Reachability bitmaps are designed to speed up the \"counting objects\"\n          phase of generating a pack during a clone or fetch. They are not\n     @@ Documentation/config/push.txt: push.negotiate::\n       \tin common.\n      +\n      +push.useBitmaps::\n     -+    If this config and `pack.useBitmaps` are both `true`, then Git will\n     -+    use reachability bitmaps during `git push`, if available (disabled\n     -+    by default).\n     ++\tIf this config and `pack.useBitmaps` are both `true`, then Git will\n     ++\tuse reachability bitmaps during `git push`, if available. If set to\n     ++\t`false`, may improve push performance without affecting use of the\n     ++\treachability bitmaps for other operations. Default is true.\n      \n       ## send-pack.c ##\n      @@ send-pack.c: static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised,\n       \t\tstrvec_push(&po.args, \"--progress\");\n       \tif (is_repository_shallow(the_repository))\n       \t\tstrvec_push(&po.args, \"--shallow\");\n     -+\tif (!args->use_bitmaps)\n     ++\tif (args->no_use_bitmaps)\n      +\t\tstrvec_push(&po.args, \"--no-use-bitmap-index\");\n       \tpo.in = -1;\n       \tpo.out = args->stateless_rpc ? -1 : fd;\n     @@ send-pack.c: int send_pack(struct send_pack_args *args,\n       \t\tget_commons_through_negotiation(args->url, remote_refs, &commons);\n       \n      +\tif (!git_config_get_bool(\"push.usebitmaps\", &use_bitmaps))\n     -+\t\targs->use_bitmaps = use_bitmaps;\n     ++\t\targs->no_use_bitmaps = !use_bitmaps;\n      +\n       \tgit_config_get_bool(\"transfer.advertisesid\", &advertise_sid);\n       \n     @@ send-pack.h: struct send_pack_args {\n       \t\tstateless_rpc:1,\n      -\t\tatomic:1;\n      +\t\tatomic:1,\n     -+\t\tuse_bitmaps:1;\n     ++\t\tno_use_bitmaps:1;\n       \tconst struct string_list *push_options;\n       };\n       \n     @@ t/t5516-fetch-push.sh: test_expect_success 'push warns or fails when using usern\n      +\tGIT_TRACE2_EVENT=\"$PWD/default\" \\\n      +\tgit push testrepo main:test &&\n      +\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n     -+\t--thin --delta-base-offset -q --no-use-bitmap-index <default &&\n     ++\t\t--thin --delta-base-offset -q <default &&\n      +\n      +\tgit config push.useBitmaps true &&\n      +\tGIT_TRACE2_EVENT=\"$PWD/true\" \\\n      +\tgit push testrepo main:test2 &&\n      +\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n     -+\t--thin --delta-base-offset -q <true &&\n     ++\t\t--thin --delta-base-offset -q <true &&\n      +\n      +\tgit config push.useBitmaps false &&\n      +\tGIT_TRACE2_EVENT=\"$PWD/false\" \\\n      +\tgit push testrepo main:test3 &&\n      +\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n     -+\t--thin --delta-base-offset -q --no-use-bitmap-index <false\n     ++\t\t--thin --delta-base-offset -q --no-use-bitmap-index <false\n      +'\n      +\n       test_done\n\n\n Documentation/config/push.txt |  6 ++++++\n send-pack.c                   |  6 ++++++\n send-pack.h                   |  3 ++-\n t/t5516-fetch-push.sh         | 21 +++++++++++++++++++++\n 4 files changed, 35 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/push.txt b/Documentation/config/push.txt\nindex e32801e6c91..f16ac9311db 100644\n--- a/Documentation/config/push.txt\n+++ b/Documentation/config/push.txt\n@@ -137,3 +137,9 @@ push.negotiate::\n \tserver attempt to find commits in common. If \"false\", Git will\n \trely solely on the server's ref advertisement to find commits\n \tin common.\n+\n+push.useBitmaps::\n+\tIf this config and `pack.useBitmaps` are both `true`, then Git will\n+\tuse reachability bitmaps during `git push`, if available. If set to\n+\t`false`, may improve push performance without affecting use of the\n+\treachability bitmaps for other operations. Default is true.\ndiff --git a/send-pack.c b/send-pack.c\nindex bc0fcdbb000..d18cde850ef 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -84,6 +84,8 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised,\n \t\tstrvec_push(&po.args, \"--progress\");\n \tif (is_repository_shallow(the_repository))\n \t\tstrvec_push(&po.args, \"--shallow\");\n+\tif (args->no_use_bitmaps)\n+\t\tstrvec_push(&po.args, \"--no-use-bitmap-index\");\n \tpo.in = -1;\n \tpo.out = args->stateless_rpc ? -1 : fd;\n \tpo.git_cmd = 1;\n@@ -487,6 +489,7 @@ int send_pack(struct send_pack_args *args,\n \tstruct async demux;\n \tconst char *push_cert_nonce = NULL;\n \tstruct packet_reader reader;\n+\tint use_bitmaps;\n \n \tif (!remote_refs) {\n \t\tfprintf(stderr, \"No refs in common and none specified; doing nothing.\\n\"\n@@ -498,6 +501,9 @@ int send_pack(struct send_pack_args *args,\n \tif (push_negotiate)\n \t\tget_commons_through_negotiation(args->url, remote_refs, &commons);\n \n+\tif (!git_config_get_bool(\"push.usebitmaps\", &use_bitmaps))\n+\t\targs->no_use_bitmaps = !use_bitmaps;\n+\n \tgit_config_get_bool(\"transfer.advertisesid\", &advertise_sid);\n \n \t/* Does the other end support the reporting? */\ndiff --git a/send-pack.h b/send-pack.h\nindex e148fcd9609..d5d6ff589d9 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -26,7 +26,8 @@ struct send_pack_args {\n \t\t/* One of the SEND_PACK_PUSH_CERT_* constants. */\n \t\tpush_cert:2,\n \t\tstateless_rpc:1,\n-\t\tatomic:1;\n+\t\tatomic:1,\n+\t\tno_use_bitmaps:1;\n \tconst struct string_list *push_options;\n };\n \ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex dedca106a7a..ffda830ba22 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1865,4 +1865,25 @@ test_expect_success 'push warns or fails when using username:password' '\n \ttest_line_count = 1 warnings\n '\n \n+test_expect_success 'push with config push.useBitmaps' '\n+\tmk_test testrepo heads/main &&\n+\tgit checkout main &&\n+\tGIT_TRACE2_EVENT=\"$PWD/default\" \\\n+\tgit push testrepo main:test &&\n+\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n+\t\t--thin --delta-base-offset -q <default &&\n+\n+\tgit config push.useBitmaps true &&\n+\tGIT_TRACE2_EVENT=\"$PWD/true\" \\\n+\tgit push testrepo main:test2 &&\n+\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n+\t\t--thin --delta-base-offset -q <true &&\n+\n+\tgit config push.useBitmaps false &&\n+\tGIT_TRACE2_EVENT=\"$PWD/false\" \\\n+\tgit push testrepo main:test3 &&\n+\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n+\t\t--thin --delta-base-offset -q --no-use-bitmap-index <false\n+'\n+\n test_done\n\nbase-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9\n-- \ngitgitgadget\n"},{"id":"457456","messageId":"xmqqtu8jns08.fsf@gitster.g","threadId":"58013","inReplyTo":"pull.1263.v3.git.1655438361228.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] send-pack.c: add config push.useBitmaps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-17T17:01:43Z","receivedAt":"2022-06-17T17:01:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle Zhao via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>  Documentation/config/push.txt |  6 ++++++\n>  send-pack.c                   |  6 ++++++\n>  send-pack.h                   |  3 ++-\n>  t/t5516-fetch-push.sh         | 21 +++++++++++++++++++++\n>  4 files changed, 35 insertions(+), 1 deletion(-)\n\nThanks.  This round looks more or less OK.\n\n> diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt\n> index e32801e6c91..f16ac9311db 100644\n> --- a/Documentation/config/push.txt\n> +++ b/Documentation/config/push.txt\n> @@ -137,3 +137,9 @@ push.negotiate::\n>  \tserver attempt to find commits in common. If \"false\", Git will\n>  \trely solely on the server's ref advertisement to find commits\n>  \tin common.\n> +\n> +push.useBitmaps::\n> +\tIf this config and `pack.useBitmaps` are both `true`, then Git will\n> +\tuse reachability bitmaps during `git push`, if available. If set to\n> +\t`false`, may improve push performance without affecting use of the\n> +\treachability bitmaps for other operations. Default is true.\n\nWhile nothing in the description is incorrect per-se, I somehow find\nit hard to follow.  \"git push\" uses reachability bitmaps when three\nconditions hold true at the same time (pack.useBitmaps that is by\ndefault true, push.useBitmaps that is by default true, and there\nexist reachability bitmaps to be used).  It is left unsaid why the\nuser needs to know about this configuration variable in order to\ntweak \"without affecting other operations\".\n\nI tried to come up with a version that I find easier to read (at\nleast to me).  I am not sure how successfull I was.\n\n    While git can be told to use reachability bitmaps in general\n    with \"pack.useBitmaps\" configuration, this variable can be used\n    to disable use of bitmaps only for \"git push\" by setting it to\n    false, without preventing other git operations from using\n    bitmaps.\n\n> diff --git a/send-pack.c b/send-pack.c\n> index bc0fcdbb000..d18cde850ef 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -84,6 +84,8 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised,\n>  \t\tstrvec_push(&po.args, \"--progress\");\n>  \tif (is_repository_shallow(the_repository))\n>  \t\tstrvec_push(&po.args, \"--shallow\");\n> +\tif (args->no_use_bitmaps)\n> +\t\tstrvec_push(&po.args, \"--no-use-bitmap-index\");\n\n\"disable_bitmaps\" might be a better name for the struct member, but OK.\n\n> @@ -498,6 +501,9 @@ int send_pack(struct send_pack_args *args,\n>  \tif (push_negotiate)\n>  \t\tget_commons_through_negotiation(args->url, remote_refs, &commons);\n>  \n> +\tif (!git_config_get_bool(\"push.usebitmaps\", &use_bitmaps))\n> +\t\targs->no_use_bitmaps = !use_bitmaps;\n\nBecause the \"--no-use-bitmap-index\" is explicitly given only when\nthe \".no_use_bitmaps\" member is true and we do not pass\n\"--use-bitmap-index\" merely because the member is false, the\nconfiguration can only disable, which matches the documented\ndesign.  Good.\n\n> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\n> index dedca106a7a..ffda830ba22 100755\n> --- a/t/t5516-fetch-push.sh\n> +++ b/t/t5516-fetch-push.sh\n> @@ -1865,4 +1865,25 @@ test_expect_success 'push warns or fails when using username:password' '\n>  \ttest_line_count = 1 warnings\n>  '\n>  \n> +test_expect_success 'push with config push.useBitmaps' '\n> +\tmk_test testrepo heads/main &&\n> +\tgit checkout main &&\n> +\tGIT_TRACE2_EVENT=\"$PWD/default\" \\\n> +\tgit push testrepo main:test &&\n> +\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n> +\t\t--thin --delta-base-offset -q <default &&\n\nWe do not pass \"--no-use-bitmap-index\" without configuration.\nGood. \n\nHow do we know this is the \"default\" state, by the way?  Because we\nread the tests before this, all 1860 lines?\n\nPerhaps\n\n\ttest_unconfig push.useBitmaps &&\n\nat the very beginning of the test would help future readers.  That\nway, they can immediately tell that the first test piece is without\nthe variable.\n\n> +\tgit config push.useBitmaps true &&\n> +\tGIT_TRACE2_EVENT=\"$PWD/true\" \\\n> +\tgit push testrepo main:test2 &&\n\nPerhaps use\n\n\ttest_config push.useBitmaps true\n\ninstead?  That way, after this test-expect-success finishes,\npush.useBitmaps variable will be wiped from the configuration\nvariable.  That is a good hygiene to follow to help future\ndevelopers who may need to add more tests after this one.\n\n> +\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n> +\t\t--thin --delta-base-offset -q <true &&\n> +\n> +\tgit config push.useBitmaps false &&\n\nLikewise.\n\n> +\tGIT_TRACE2_EVENT=\"$PWD/false\" \\\n> +\tgit push testrepo main:test3 &&\n> +\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n> +\t\t--thin --delta-base-offset -q --no-use-bitmap-index <false\n> +'\n> +\n>  test_done\n\n\nLastly, some critique on the proposed log message.\n\n> This allows you to disable bitmaps for \"git push\". Default is true.\n\nI find it more confusing to have this line at the beginning than\nwithout.  What the configuration variable does is explained later\nanyway, and our convention is not to start from such a \"conclusion\"\nbut ease the readers into the reasoning by starting with the\nexplanation of the status quo, which leads readers to realize why\nthe current system is lacking.\n\n> Reachability bitmaps are designed to speed up the \"counting objects\"\n> phase of generating a pack during a clone or fetch. They are not\n> optimized for Git clients sending a small topic branch via \"git push\".\n> In some cases (see [1]), using reachability bitmaps during \"git push\"\n> can cause significant performance regressions.\n\nAnd this paragraph brilliantly does its job.  A reader who did not\neven know what the reachability bitmaps are for is now prepared to\nfollow your logic why it is a good idea to special case \"git push\"\nbecause you explain that it is good for some things and not\nnecessarily good for others like \"git push\".\n\n> Add a new \"push.useBitmaps\" config option to disable reachability\n> bitmaps during \"git push\". This allows reachability bitmaps to still\n> be used in other areas, such as \"git rev-list --use-bitmap-index\".\n\nI had the same problem with this paragraph as the one in the\ndocumentation part of the patch.  Nothing in it is incorrect per-se,\nbut is roundabout way of saying what it wants to say.\n\nHere is my attempted rewrite.\n\n    Add a new \"push.useBitmaps\" configuration variable to allow users to\n    tell \"git push\" not to use bitmaps.  We already have \"pack.bitmaps\"\n    that controls the use of bitmaps, but a separate configuration\n    variable allows the reachability bitmaps to still be used in other\n    areas, such as \"git rev-list --use-bitmap-index\", while disabling it\n    only for \"git push\".\n\nThanks.\n"},{"id":"457458","messageId":"pull.1263.v4.git.1655492779228.gitgitgadget@gmail.com","threadId":"58013","inReplyTo":"pull.1263.v3.git.1655438361228.gitgitgadget@gmail.com","subject":"[PATCH v4] send-pack.c: add config push.useBitmaps","fromName":"Kyle Zhao via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-17T19:06:19Z","receivedAt":"2022-06-17T19:06:37Z","isPatch":true,"sender":{"key":"goldofzky@gmail.com","avatar":"https://gravatar.com/avatar/aa4fa27f0c1bed9c986ed596415799518e03a6e71a0931308a4de87e166e183a?d=mp&s=160"},"body":"From: Kyle Zhao <kylezhao@tencent.com>\n\nReachability bitmaps are designed to speed up the \"counting objects\"\nphase of generating a pack during a clone or fetch. They are not\noptimized for Git clients sending a small topic branch via \"git push\".\nIn some cases (see [1]), using reachability bitmaps during \"git push\"\ncan cause significant performance regressions.\n\nAdd a new \"push.useBitmaps\" configuration variable to allow users to\ntell \"git push\" not to use bitmaps. We already have \"pack.bitmaps\"\nthat controls the use of bitmaps, but a separate configuration variable\nallows the reachability bitmaps to still be used in other areas,\nsuch as \"git upload-pack\", while disabling it only for \"git push\".\n\n[1]: https://lore.kernel.org/git/87zhoz8b9o.fsf@evledraar.gmail.com/\n\nSigned-off-by: Kyle Zhao <kylezhao@tencent.com>\n---\n    send-pack.c: add config push.useBitmaps\n    \n    This patch add config push.useBitmaps to prevent git push using bitmap.\n    \n    The origin discussion is here:\n    https://lore.kernel.org/git/b940e705fbe9454685757f2e3055e2ce@tencent.com/\n    \n    Thanks, -Kyle\n    \n    Changes since v1:\n    \n     * changed the commit message\n     * modified and added missing \\n to push.txt\n     * used test_subcommand for test\n     * modified \"if\" statement for \"git_config_get_bool()\" in send-pack.c\n    \n    Changes since v2:\n    \n     * enable 'push.useBitmaps\" by default\n     * fix nit in t/t5516-fetch-push.sh\n    \n    Changes since v3:\n    \n     * changed the commit message\n     * s/no_use_bitmaps/disable_bitmaps in send-pack.h and send-pack.c\n     * I modified the document about \"push.useBitmaps\". When it is true, Git\n       will keep the historical behaviour. So I mainly introduced what\n       happens when it set to false.\n     * use test_config and test_unconfig for test.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1263%2Fkeyu98%2Fkz%2Fpush-usebitmps-config-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1263/keyu98/kz/push-usebitmps-config-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1263\n\nRange-diff vs v3:\n\n 1:  a523cb52542 ! 1:  9d35d926638 send-pack.c: add config push.useBitmaps\n     @@ Metadata\n       ## Commit message ##\n          send-pack.c: add config push.useBitmaps\n      \n     -    This allows you to disable bitmaps for \"git push\". Default is true.\n     -\n          Reachability bitmaps are designed to speed up the \"counting objects\"\n          phase of generating a pack during a clone or fetch. They are not\n          optimized for Git clients sending a small topic branch via \"git push\".\n          In some cases (see [1]), using reachability bitmaps during \"git push\"\n          can cause significant performance regressions.\n      \n     -    Add a new \"push.useBitmaps\" config option to disable reachability\n     -    bitmaps during \"git push\". This allows reachability bitmaps to still\n     -    be used in other areas, such as \"git rev-list --use-bitmap-index\".\n     +    Add a new \"push.useBitmaps\" configuration variable to allow users to\n     +    tell \"git push\" not to use bitmaps. We already have \"pack.bitmaps\"\n     +    that controls the use of bitmaps, but a separate configuration variable\n     +    allows the reachability bitmaps to still be used in other areas,\n     +    such as \"git upload-pack\", while disabling it only for \"git push\".\n      \n          [1]: https://lore.kernel.org/git/87zhoz8b9o.fsf@evledraar.gmail.com/\n      \n     @@ Documentation/config/push.txt: push.negotiate::\n       \tin common.\n      +\n      +push.useBitmaps::\n     -+\tIf this config and `pack.useBitmaps` are both `true`, then Git will\n     -+\tuse reachability bitmaps during `git push`, if available. If set to\n     -+\t`false`, may improve push performance without affecting use of the\n     -+\treachability bitmaps for other operations. Default is true.\n     ++\tIf set to \"false\", disable use of bitmaps for \"git push\" even if\n     ++\t`pack.useBitmaps` is \"true\", without preventing other git operations\n     ++\tfrom using bitmaps. Default is true.\n      \n       ## send-pack.c ##\n      @@ send-pack.c: static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised,\n       \t\tstrvec_push(&po.args, \"--progress\");\n       \tif (is_repository_shallow(the_repository))\n       \t\tstrvec_push(&po.args, \"--shallow\");\n     -+\tif (args->no_use_bitmaps)\n     ++\tif (args->disable_bitmaps)\n      +\t\tstrvec_push(&po.args, \"--no-use-bitmap-index\");\n       \tpo.in = -1;\n       \tpo.out = args->stateless_rpc ? -1 : fd;\n     @@ send-pack.c: int send_pack(struct send_pack_args *args,\n       \t\tget_commons_through_negotiation(args->url, remote_refs, &commons);\n       \n      +\tif (!git_config_get_bool(\"push.usebitmaps\", &use_bitmaps))\n     -+\t\targs->no_use_bitmaps = !use_bitmaps;\n     ++\t\targs->disable_bitmaps = !use_bitmaps;\n      +\n       \tgit_config_get_bool(\"transfer.advertisesid\", &advertise_sid);\n       \n     @@ send-pack.h: struct send_pack_args {\n       \t\tstateless_rpc:1,\n      -\t\tatomic:1;\n      +\t\tatomic:1,\n     -+\t\tno_use_bitmaps:1;\n     ++\t\tdisable_bitmaps:1;\n       \tconst struct string_list *push_options;\n       };\n       \n     @@ t/t5516-fetch-push.sh: test_expect_success 'push warns or fails when using usern\n      +test_expect_success 'push with config push.useBitmaps' '\n      +\tmk_test testrepo heads/main &&\n      +\tgit checkout main &&\n     ++\ttest_unconfig push.useBitmaps &&\n      +\tGIT_TRACE2_EVENT=\"$PWD/default\" \\\n      +\tgit push testrepo main:test &&\n      +\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n      +\t\t--thin --delta-base-offset -q <default &&\n      +\n     -+\tgit config push.useBitmaps true &&\n     ++\ttest_config push.useBitmaps true &&\n      +\tGIT_TRACE2_EVENT=\"$PWD/true\" \\\n      +\tgit push testrepo main:test2 &&\n      +\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n      +\t\t--thin --delta-base-offset -q <true &&\n      +\n     -+\tgit config push.useBitmaps false &&\n     ++\ttest_config push.useBitmaps false &&\n      +\tGIT_TRACE2_EVENT=\"$PWD/false\" \\\n      +\tgit push testrepo main:test3 &&\n      +\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n\n\n Documentation/config/push.txt |  5 +++++\n send-pack.c                   |  6 ++++++\n send-pack.h                   |  3 ++-\n t/t5516-fetch-push.sh         | 22 ++++++++++++++++++++++\n 4 files changed, 35 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/push.txt b/Documentation/config/push.txt\nindex e32801e6c91..7386fea225a 100644\n--- a/Documentation/config/push.txt\n+++ b/Documentation/config/push.txt\n@@ -137,3 +137,8 @@ push.negotiate::\n \tserver attempt to find commits in common. If \"false\", Git will\n \trely solely on the server's ref advertisement to find commits\n \tin common.\n+\n+push.useBitmaps::\n+\tIf set to \"false\", disable use of bitmaps for \"git push\" even if\n+\t`pack.useBitmaps` is \"true\", without preventing other git operations\n+\tfrom using bitmaps. Default is true.\ndiff --git a/send-pack.c b/send-pack.c\nindex bc0fcdbb000..662f7c2aeb9 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -84,6 +84,8 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised,\n \t\tstrvec_push(&po.args, \"--progress\");\n \tif (is_repository_shallow(the_repository))\n \t\tstrvec_push(&po.args, \"--shallow\");\n+\tif (args->disable_bitmaps)\n+\t\tstrvec_push(&po.args, \"--no-use-bitmap-index\");\n \tpo.in = -1;\n \tpo.out = args->stateless_rpc ? -1 : fd;\n \tpo.git_cmd = 1;\n@@ -487,6 +489,7 @@ int send_pack(struct send_pack_args *args,\n \tstruct async demux;\n \tconst char *push_cert_nonce = NULL;\n \tstruct packet_reader reader;\n+\tint use_bitmaps;\n \n \tif (!remote_refs) {\n \t\tfprintf(stderr, \"No refs in common and none specified; doing nothing.\\n\"\n@@ -498,6 +501,9 @@ int send_pack(struct send_pack_args *args,\n \tif (push_negotiate)\n \t\tget_commons_through_negotiation(args->url, remote_refs, &commons);\n \n+\tif (!git_config_get_bool(\"push.usebitmaps\", &use_bitmaps))\n+\t\targs->disable_bitmaps = !use_bitmaps;\n+\n \tgit_config_get_bool(\"transfer.advertisesid\", &advertise_sid);\n \n \t/* Does the other end support the reporting? */\ndiff --git a/send-pack.h b/send-pack.h\nindex e148fcd9609..7edb80596c7 100644\n--- a/send-pack.h\n+++ b/send-pack.h\n@@ -26,7 +26,8 @@ struct send_pack_args {\n \t\t/* One of the SEND_PACK_PUSH_CERT_* constants. */\n \t\tpush_cert:2,\n \t\tstateless_rpc:1,\n-\t\tatomic:1;\n+\t\tatomic:1,\n+\t\tdisable_bitmaps:1;\n \tconst struct string_list *push_options;\n };\n \ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex dedca106a7a..b3734dd2fe9 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1865,4 +1865,26 @@ test_expect_success 'push warns or fails when using username:password' '\n \ttest_line_count = 1 warnings\n '\n \n+test_expect_success 'push with config push.useBitmaps' '\n+\tmk_test testrepo heads/main &&\n+\tgit checkout main &&\n+\ttest_unconfig push.useBitmaps &&\n+\tGIT_TRACE2_EVENT=\"$PWD/default\" \\\n+\tgit push testrepo main:test &&\n+\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n+\t\t--thin --delta-base-offset -q <default &&\n+\n+\ttest_config push.useBitmaps true &&\n+\tGIT_TRACE2_EVENT=\"$PWD/true\" \\\n+\tgit push testrepo main:test2 &&\n+\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n+\t\t--thin --delta-base-offset -q <true &&\n+\n+\ttest_config push.useBitmaps false &&\n+\tGIT_TRACE2_EVENT=\"$PWD/false\" \\\n+\tgit push testrepo main:test3 &&\n+\ttest_subcommand git pack-objects --all-progress-implied --revs --stdout \\\n+\t\t--thin --delta-base-offset -q --no-use-bitmap-index <false\n+'\n+\n test_done\n\nbase-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9\n-- \ngitgitgadget\n"},{"id":"457463","messageId":"xmqq4k0jnfhn.fsf@gitster.g","threadId":"58013","inReplyTo":"pull.1263.v4.git.1655492779228.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] send-pack.c: add config push.useBitmaps","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-17T21:32:04Z","receivedAt":"2022-06-17T21:32:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle Zhao via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>     Changes since v3:\n>     \n>      * changed the commit message\n>      * s/no_use_bitmaps/disable_bitmaps in send-pack.h and send-pack.c\n>      * I modified the document about \"push.useBitmaps\". When it is true, Git\n>        will keep the historical behaviour. So I mainly introduced what\n>        happens when it set to false.\n>      * use test_config and test_unconfig for test.\n\nLooking good.  Will queue.\n\nThanks.\n"}]}