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

[Stefan Beller <sbeller@google.com>]

On Fri, Jun 30, 2017 at 10:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

>
> 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