Re: [PATCHv2 1/2] hashmap.h: compare function has access to a data field
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 30, 2017, 17:39 UTC
- Message-ID
- <xmqqh8yxs7by.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170629235336.28460-2-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
Show 29 quoted lines
> 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.