Re: [PATCH v2] send-pack.c: add config push.useBitmaps

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.