Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: >> + 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 --no-use-bitmap-index <default && > > Nit: We tend to indent these ase we wrap, so e.g.: > > test_subcommand git ... \ > --thin --delta [...] > > The rest all looks good as far as the diff goes, if what we want to do > is to disable this by default, and this isn't worth a re-roll in itself. > > But I still think that completely disabling bitmaps might be premature > here, especially per Taylor's comment on v1 (which I understand to mean > that they should help some of the time, even with push). The usual way to move is to move slowly and carefully. It may well be the case that disabling bitmaps gives users a better default, but that is not even proven and is hard to prove. How many users of Git do we have? Those silently using it happily will by definition complain here or elsewhere, and the complaints "X is slow with Y, so Y should be disabled when doing X" we hear tend to be louder than "I am happily doing X with Y". I have different problems with this patch, though. It can use a bit more honesty. If you introduce a new knob and sell it as a knob that allows disabling, be honest and keep its behaviour as advertised. As posted, IIUC, the patch does something quite different. It disables by default, and have a knob to allow it enabled again. So, perhaps make it default on to keep the historical behaviour, and document it as "setting it false may improve push performance without affecting use of the reachability bitmaps for other operations. Default is true." Thanks.
