On 6/15/22 7:08 AM, Kyle Zhao via GitGitGadget wrote: > From: Kyle Zhao <kylezhao@xxxxxxxxxxx> > > This allows you to disabled bitmaps for "git push". Default is false. s/disabled/disable/ > Bitmaps are designed to speed up the "counting objects" phase of > subsequent packs created for clones and fetches. > But in some cases, turning bitmaps on does horrible things for "push" > performance[1]. I would rephrase this message body as follows: Reachability bitmaps are designed to speed up the "counting objects" phase of generating a pack during a clone or fetch. They are not optimized for Git clients sending a small topic branch via "git push". In some cases (see [1]), using reachability bitmaps during "git push" can cause significant performance regressions. Add a new "push.useBitmaps" config option to disable reachability bitmaps during "git push". This allows reachability bitmaps to still be used in other areas, such as "git rev-list --use-bitmap-index". > [1]: https://lore.kernel.org/git/87zhoz8b9o.fsf@xxxxxxxxxxxxxxxxxxx/ > + > +push.useBitmaps:: > + If this config and `pack.useBitmaps` are both "true", git will > + use pack bitmaps (if available) when git push. Default is false. Rewording slightly: If this config and `pack.useBitmaps` are both `true`, then Git will use reachability bitmaps during `git push`, if available (disabled by default). > \ No newline at end of file Please fix this missing newline. I'm glad that this references `pack.useBitmaps`. I wonder if that config is sufficient for your purposes: do you expect to use your bitmaps to generate pack-files in any other way than "git push"? That is: are you serving remote requests from your repo with your bitmaps while also using "git push"? Or, are you using bitmaps only for things like "git rev-list --use-bitmap-index"? I just want to compare this with `pack.useSparse` which targets "git push", but focuses entirely on the pack-creation part of the operation. Since the existence of reachability bitmaps overrides the sparse algorithm, this does not affect Git servers (who should have a reachability bitmap). I just want to be sure that using pack.useBitmaps=false would not be sufficient for your situation (and probably most situations). > diff --git a/send-pack.c b/send-pack.c > index bc0fcdbb000..d6091571caa 100644 > --- a/send-pack.c > +++ b/send-pack.c > @@ -84,6 +84,8 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *advertised, > strvec_push(&po.args, "--progress"); > if (is_repository_shallow(the_repository)) > strvec_push(&po.args, "--shallow"); > + if (!args->use_bitmaps) > + strvec_push(&po.args, "--no-use-bitmap-index"); Here is where you specify the lower-level pack creation only when sending a pack. It is very focused. Good. > + int use_bitmaps = 0; > + git_config_get_bool("push.usebitmaps", &use_bitmaps); > + if (use_bitmaps) > + args->use_bitmaps = 1; You can instead write this as: if (!git_config_get_bool("push.usebitmaps", &use_bitmaps)) args->use_bitmaps = use_bitmaps; > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh > index dedca106a7a..ee0b912a5e8 100755 > --- a/t/t5516-fetch-push.sh > +++ b/t/t5516-fetch-push.sh > @@ -1865,4 +1865,15 @@ test_expect_success 'push warns or fails when using username:password' ' > test_line_count = 1 warnings > ' > > +test_expect_success 'push with config push.useBitmaps' ' > + mk_test testrepo heads/main && > + git checkout main && > + GIT_TRACE=1 git push testrepo main:test >/dev/null 2>stderr && Just use "err" instead of "stderr". > + grep "no-use-bitmap-index" stderr && > + > + git config push.useBitmaps true && > + GIT_TRACE=1 git push testrepo main:test2 >/dev/null 2>stderr && > + ! grep "no-use-bitmap-index" stderr > +' I believe this test is correct, but it might be worth looking into test_subcommand if you can determine the exact subcommand arguments you are looking for. Thanks, -Stolee
