On Wed, Nov 14 2018, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>
>> Add a new core.checkCollisions setting. On by default, it can be set
>> to 'false' to disable the check for existing objects in sha1_object().
>> ...
>> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
>> index 2004e25da2..4a3508aa9f 100644
>> --- a/builtin/index-pack.c
>> +++ b/builtin/index-pack.c
>> @@ -791,23 +791,24 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,
>> {
>> void *new_data = NULL;
>> int collision_test_needed = 0;
>> + int do_coll_check = git_config_get_collision_check();
>>
>> assert(data || obj_entry);
>>
>> - if (startup_info->have_repository) {
>> + if (do_coll_check && startup_info->have_repository) {
>> read_lock();
>> collision_test_needed =
>> has_sha1_file_with_flags(oid->hash, OBJECT_INFO_QUICK);
>> read_unlock();
>> }
>>
>> - if (collision_test_needed && !data) {
>> + if (do_coll_check && collision_test_needed && !data) {
>> read_lock();
>> if (!check_collison(obj_entry))
>> collision_test_needed = 0;
>> read_unlock();
>> }
>> - if (collision_test_needed) {
>> + if (do_coll_check && collision_test_needed) {
>
> If I am reading the patch correctly, The latter two changes are
> totally unnecessary. c-t-needed is true only when dO-coll_check
> allowed the initial "do we even have that object?" check to kick in
> and never set otherwise.
You're right. I was trying to do this in a few different ways and didn't
simplify this part.
> I am not all that enthused to the idea of sending a wrong message to
> our users, i.e. it is sometimes OK to sacrifice the security of
> collision detection.
I think I've made the case in side-threads that given the performance
numbers and the danger of an actual SHA-1 collision this is something
other powerusers would be interested in having.
> A small change like this is easy to adjust to apply to the codebase,
> even after today's codebase undergoes extensive modifications; quite
> honestly, I'd prefer not having to worry about it so close to the
> pre-release feature freeze.
Yeah, let's definitely wait with this under 2.20. I sent this out more
because I re-rolled it for an internal deployment, and wanted to get
some thoughts on what the plan should be for queuing up these two
related (no collision detection && loose cache) features.
