Re: [PATCH v3] index-pack: add ability to disable SHA-1 collision check
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 14, 2018, 07:09 UTC
- Message-ID
- <xmqqftw4nnj6.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20181113201910.11518-1-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 32 quoted lines
> 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.