Re: [PATCHv2 1/2] hashmap.h: compare function has access to a data field

From: Stefan Beller <sbeller@google.com>
Date: Jun 30, 2017, 18:00 UTC
Message-ID: <CAGZ79kbWXuC2y97+nOsgvNhCCqg=2+VgOOvTbUn1LGpH4f0=PQ@mail.gmail.com>
In-Reply-To: <xmqqh8yxs7by.fsf@gitster.mtv.corp.google.com>

On Fri, Jun 30, 2017 at 10:39 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 31 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> diff --git a/patch-ids.c b/patch-ids.c
>> index 9c0ab9e67a..b9b2ebbad0 100644
>> --- a/patch-ids.c
>> +++ b/patch-ids.c
>> @@ -37,6 +37,7 @@ int commit_patch_id(struct commit *commit, struct diff_options *options,
>>   */
>>  static int patch_id_cmp(struct patch_id *a,
>>                       struct patch_id *b,
>> +                     const void *unused_keydata,
>>                       struct diff_options *opt)
>>  {
>>       if (is_null_oid(&a->patch_id) &&
>> @@ -57,7 +58,8 @@ int init_patch_ids(struct patch_ids *ids)
>>       ids->diffopts.detect_rename = 0;
>>       DIFF_OPT_SET(&ids->diffopts, RECURSIVE);
>>       diff_setup_done(&ids->diffopts);
>> -     hashmap_init(&ids->patches, (hashmap_cmp_fn)patch_id_cmp, 256);
>> +     hashmap_init(&ids->patches, (hashmap_cmp_fn)patch_id_cmp,
>> +                  &ids->diffopts, 256);
>>       return 0;
>>  }
>>
>> @@ -93,7 +95,7 @@ struct patch_id *has_commit_patch_id(struct commit *commit,
>>       if (init_patch_id_entry(&patch, commit, ids))
>>               return NULL;
>>
>> -     return hashmap_get(&ids->patches, &patch, &ids->diffopts);
>> +     return hashmap_get(&ids->patches, &patch, NULL);
>>  }

+cc Peff

Show 13 quoted lines

>
> This actually makes me wonder if we can demonstrate an existing
> breakage in tests.  The old code used to pass NULL to the diffopts,
> causing it to be passed down through commit_patch_id() function down
> to diff_tree_oid() or diff_root_tree_oid().  When the codepath
> triggers the issue that Peff warned about in his old article (was it
> about rehashing or something?)  that makes two entries compared
> (i.e. not using keydata, because we are not comparing an existing
> entry with a key and data only to see if that already exists in the
> hashmap), wouldn't that cause ll_diff_tree_oid() that is called from
> diff_tree_oid() to dereference NULL?
>
> Thanks.

I am at a loss here after re-reading your answer over and over, but I think you are asking if patch_id_cmp can break, as we have a callchain like

  patch_id_cmp
    commit_patch_id
      (diff_root_tree_oid)
        diff_tree_oid
          ll_diff_tree_oid

passing diff_options down there, and patch_id_cmp may have gotten NULL.

The answer is no, it was safe. (by accident?)

That is because we never use hashmap_get_next on the hashmap that uses patch_id_cmp as the compare function.

hashmap_get_next is the only function that does not pass on a keydata, any other has valid caller provided keydata.

Thanks, Stefan