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

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.

Reply via email to