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

On Fri, Jun 30, 2017 at 10:39 AM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

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