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

[Junio C Hamano <gitster@pobox.com>]

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);
>  }

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.