Re: [PATCH v3] index-pack: add ability to disable SHA-1 collision check
From: Junio C Hamano <hidden>
Date: 2018-11-14 07:09:26
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted hunk
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. 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. 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.