| Message ID | pull.1263.v3.git.1655438361228.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] send-pack.c: add config push.useBitmaps | expand |
"Kyle Zhao via GitGitGadget" <gitgitgadget@gmail.com> writes: > Documentation/config/push.txt | 6 ++++++ > send-pack.c | 6 ++++++ > send-pack.h | 3 ++- > t/t5516-fetch-push.sh | 21 +++++++++++++++++++++ > 4 files changed, 35 insertions(+), 1 deletion(-) Thanks. This round looks more or less OK. > diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt > index e32801e6c91..f16ac9311db 100644 > --- a/Documentation/config/push.txt > +++ b/Documentation/config/push.txt > @@ -137,3 +137,9 @@ push.negotiate:: > server attempt to find commits in common. If "false", Git will > rely solely on the server's ref advertisement to find commits > in common. > + > +push.useBitmaps:: > + If this config and `pack.useBitmaps` are both `true`, then Git will > + use reachability bitmaps during `git push`, if available. If set to > + `false`, may improve push performance without affecting use of the > + reachability bitmaps for other operations. Default is true. While nothing in the description is incorrect per-se, I somehow find it hard to follow. "git push" uses reachability bitmaps when three conditions hold true at the same time (pack.useBitmaps that is by default true, push.useBitmaps that is by default true, and there exist reachability bitmaps to be used). It is left unsaid why the user needs to know about this configuration variable in order to tweak "without affecting other operations". I tried to come up with a version that I find easier to read (at least to me). I am not sure how successfull I was. While git can be told to use reachability bitmaps in general with "pack.useBitmaps" configuration, this variable can be used to disable use of bitmaps only for "git push" by setting it to false, without preventing other git operations from using bitmaps. > diff --git a/send-pack.c b/send-pack.c > index bc0fcdbb000..d18cde850ef 100644 > --- a/send-pack.c > +++ b/send-pack.c > @@ -84,6 +84,8 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised, > strvec_push(&po.args, "--progress"); > if (is_repository_shallow(the_repository)) > strvec_push(&po.args, "--shallow"); > + if (args->no_use_bitmaps) > + strvec_push(&po.args, "--no-use-bitmap-index"); "disable_bitmaps" might be a better name for the struct member, but OK. > @@ -498,6 +501,9 @@ int send_pack(struct send_pack_args *args, > if (push_negotiate) > get_commons_through_negotiation(args->url, remote_refs, &commons); > > + if (!git_config_get_bool("push.usebitmaps", &use_bitmaps)) > + args->no_use_bitmaps = !use_bitmaps; Because the "--no-use-bitmap-index" is explicitly given only when the ".no_use_bitmaps" member is true and we do not pass "--use-bitmap-index" merely because the member is false, the configuration can only disable, which matches the documented design. Good. > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh > index dedca106a7a..ffda830ba22 100755 > --- a/t/t5516-fetch-push.sh > +++ b/t/t5516-fetch-push.sh > @@ -1865,4 +1865,25 @@ test_expect_success 'push warns or fails when using username:password' ' > test_line_count = 1 warnings > ' > > +test_expect_success 'push with config push.useBitmaps' ' > + mk_test testrepo heads/main && > + git checkout main && > + GIT_TRACE2_EVENT="$PWD/default" \ > + git push testrepo main:test && > + test_subcommand git pack-objects --all-progress-implied --revs --stdout \ > + --thin --delta-base-offset -q <default && We do not pass "--no-use-bitmap-index" without configuration. Good. How do we know this is the "default" state, by the way? Because we read the tests before this, all 1860 lines? Perhaps test_unconfig push.useBitmaps && at the very beginning of the test would help future readers. That way, they can immediately tell that the first test piece is without the variable. > + git config push.useBitmaps true && > + GIT_TRACE2_EVENT="$PWD/true" \ > + git push testrepo main:test2 && Perhaps use test_config push.useBitmaps true instead? That way, after this test-expect-success finishes, push.useBitmaps variable will be wiped from the configuration variable. That is a good hygiene to follow to help future developers who may need to add more tests after this one. > + test_subcommand git pack-objects --all-progress-implied --revs --stdout \ > + --thin --delta-base-offset -q <true && > + > + git config push.useBitmaps false && Likewise. > + GIT_TRACE2_EVENT="$PWD/false" \ > + git push testrepo main:test3 && > + test_subcommand git pack-objects --all-progress-implied --revs --stdout \ > + --thin --delta-base-offset -q --no-use-bitmap-index <false > +' > + > test_done Lastly, some critique on the proposed log message. > This allows you to disable bitmaps for "git push". Default is true. I find it more confusing to have this line at the beginning than without. What the configuration variable does is explained later anyway, and our convention is not to start from such a "conclusion" but ease the readers into the reasoning by starting with the explanation of the status quo, which leads readers to realize why the current system is lacking. > Reachability bitmaps are designed to speed up the "counting objects" > phase of generating a pack during a clone or fetch. They are not > optimized for Git clients sending a small topic branch via "git push". > In some cases (see [1]), using reachability bitmaps during "git push" > can cause significant performance regressions. And this paragraph brilliantly does its job. A reader who did not even know what the reachability bitmaps are for is now prepared to follow your logic why it is a good idea to special case "git push" because you explain that it is good for some things and not necessarily good for others like "git push". > Add a new "push.useBitmaps" config option to disable reachability > bitmaps during "git push". This allows reachability bitmaps to still > be used in other areas, such as "git rev-list --use-bitmap-index". I had the same problem with this paragraph as the one in the documentation part of the patch. Nothing in it is incorrect per-se, but is roundabout way of saying what it wants to say. Here is my attempted rewrite. Add a new "push.useBitmaps" configuration variable to allow users to tell "git push" not to use bitmaps. We already have "pack.bitmaps" that controls the use of bitmaps, but a separate configuration variable allows the reachability bitmaps to still be used in other areas, such as "git rev-list --use-bitmap-index", while disabling it only for "git push". Thanks.
diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt index e32801e6c91..f16ac9311db 100644 --- a/Documentation/config/push.txt +++ b/Documentation/config/push.txt @@ -137,3 +137,9 @@ push.negotiate:: server attempt to find commits in common. If "false", Git will rely solely on the server's ref advertisement to find commits in common. + +push.useBitmaps:: + If this config and `pack.useBitmaps` are both `true`, then Git will + use reachability bitmaps during `git push`, if available. If set to + `false`, may improve push performance without affecting use of the + reachability bitmaps for other operations. Default is true. diff --git a/send-pack.c b/send-pack.c index bc0fcdbb000..d18cde850ef 100644 --- a/send-pack.c +++ b/send-pack.c @@ -84,6 +84,8 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised, strvec_push(&po.args, "--progress"); if (is_repository_shallow(the_repository)) strvec_push(&po.args, "--shallow"); + if (args->no_use_bitmaps) + strvec_push(&po.args, "--no-use-bitmap-index"); po.in = -1; po.out = args->stateless_rpc ? -1 : fd; po.git_cmd = 1; @@ -487,6 +489,7 @@ int send_pack(struct send_pack_args *args, struct async demux; const char *push_cert_nonce = NULL; struct packet_reader reader; + int use_bitmaps; if (!remote_refs) { fprintf(stderr, "No refs in common and none specified; doing nothing.\n" @@ -498,6 +501,9 @@ int send_pack(struct send_pack_args *args, if (push_negotiate) get_commons_through_negotiation(args->url, remote_refs, &commons); + if (!git_config_get_bool("push.usebitmaps", &use_bitmaps)) + args->no_use_bitmaps = !use_bitmaps; + git_config_get_bool("transfer.advertisesid", &advertise_sid); /* Does the other end support the reporting? */ diff --git a/send-pack.h b/send-pack.h index e148fcd9609..d5d6ff589d9 100644 --- a/send-pack.h +++ b/send-pack.h @@ -26,7 +26,8 @@ struct send_pack_args { /* One of the SEND_PACK_PUSH_CERT_* constants. */ push_cert:2, stateless_rpc:1, - atomic:1; + atomic:1, + no_use_bitmaps:1; const struct string_list *push_options; }; diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index dedca106a7a..ffda830ba22 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1865,4 +1865,25 @@ test_expect_success 'push warns or fails when using username:password' ' test_line_count = 1 warnings ' +test_expect_success 'push with config push.useBitmaps' ' + mk_test testrepo heads/main && + git checkout main && + GIT_TRACE2_EVENT="$PWD/default" \ + git push testrepo main:test && + test_subcommand git pack-objects --all-progress-implied --revs --stdout \ + --thin --delta-base-offset -q <default && + + git config push.useBitmaps true && + GIT_TRACE2_EVENT="$PWD/true" \ + git push testrepo main:test2 && + test_subcommand git pack-objects --all-progress-implied --revs --stdout \ + --thin --delta-base-offset -q <true && + + git config push.useBitmaps false && + GIT_TRACE2_EVENT="$PWD/false" \ + git push testrepo main:test3 && + test_subcommand git pack-objects --all-progress-implied --revs --stdout \ + --thin --delta-base-offset -q --no-use-bitmap-index <false +' + test_done