Re: [PATCHv3 2/3] patch-ids.c: use hashmap correctly

[Jeff King]

On Fri, Jun 30, 2017 at 12:14:06PM -0700, Stefan Beller wrote:

> As eluded to in the previous patch, the code in patch-ids.c is using
> the hashmaps API wrong.
> 
> Luckily we do not have a bug, as all hashmap functionality that we use
> here (hashmap_get) passes through the keydata.  If hashmap_get_next were
> to be used, a bug would occur as that passes NULL for the key_data.
> 
> So instead use the hashmap API correctly and provide the caller required
> data in the compare function via the first argument that always gets
> passed and was setup via the hashmap_init function.

Reviewing this a bit late, but it looks good to me. And I think the
explanation above nicely covers what is going on (and why it isn't a
bug).

-Peff

PS I think you meant "alluded" in the first sentence, unless you really
   were trying to escape the previous patch. :)

Re: [PATCHv3 2/3] patch-ids.c: use hashmap correctly

[Junio C Hamano]

Stefan Beller  writes:

> As eluded to in the previous patch, the code in patch-ids.c is using
> the hashmaps API wrong.
>
> Luckily we do not have a bug, as all hashmap functionality that we use
> here (hashmap_get) passes through the keydata.  If hashmap_get_next were
> to be used, a bug would occur as that passes NULL for the key_data.
>
> So instead use the hashmap API correctly and provide the caller required
> data in the compare function via the first argument that always gets
> passed and was setup via the hashmap_init function.
>
> Signed-off-by: Stefan Beller 
> ---
>  patch-ids.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)

Nicely explained.  Thanks for polishing the series.

>
> diff --git a/patch-ids.c b/patch-ids.c
> index 815c115811..b4166b0f38 100644
> --- a/patch-ids.c
> +++ b/patch-ids.c
> @@ -35,10 +35,10 @@ int commit_patch_id(struct commit *commit, struct 
> diff_options *options,
>   * the side of safety.  The actual value being negative does not have
>   * any significance; only that it is non-zero matters.
>   */
> -static int patch_id_cmp(const void *unused_cmp_data,
> +static int patch_id_cmp(struct diff_options *opt,
>   struct patch_id *a,
>   struct patch_id *b,
> - struct diff_options *opt)
> + const void *unused_keydata)
>  {
>   if (is_null_oid(>patch_id) &&
>   commit_patch_id(a->commit, opt, >patch_id, 0))
> @@ -59,7 +59,7 @@ int init_patch_ids(struct patch_ids *ids)
>   DIFF_OPT_SET(>diffopts, RECURSIVE);
>   diff_setup_done(>diffopts);
>   hashmap_init(>patches, (hashmap_cmp_fn)patch_id_cmp,
> -  NULL, 256);
> +  >diffopts, 256);
>   return 0;
>  }
>  
> @@ -95,7 +95,7 @@ struct patch_id *has_commit_patch_id(struct commit *commit,
>   if (init_patch_id_entry(, commit, ids))
>   return NULL;
>  
> - return hashmap_get(>patches, , >diffopts);
> + return hashmap_get(>patches, , NULL);
>  }
>  
>  struct patch_id *add_commit_patch_id(struct commit *commit,

[PATCHv3 2/3] patch-ids.c: use hashmap correctly

[Stefan Beller]

As eluded to in the previous patch, the code in patch-ids.c is using
the hashmaps API wrong.

Luckily we do not have a bug, as all hashmap functionality that we use
here (hashmap_get) passes through the keydata.  If hashmap_get_next were
to be used, a bug would occur as that passes NULL for the key_data.

So instead use the hashmap API correctly and provide the caller required
data in the compare function via the first argument that always gets
passed and was setup via the hashmap_init function.

Signed-off-by: Stefan Beller 
---
 patch-ids.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/patch-ids.c b/patch-ids.c
index 815c115811..b4166b0f38 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -35,10 +35,10 @@ int commit_patch_id(struct commit *commit, struct 
diff_options *options,
  * the side of safety.  The actual value being negative does not have
  * any significance; only that it is non-zero matters.
  */
-static int patch_id_cmp(const void *unused_cmp_data,
+static int patch_id_cmp(struct diff_options *opt,
struct patch_id *a,
struct patch_id *b,
-   struct diff_options *opt)
+   const void *unused_keydata)
 {
if (is_null_oid(>patch_id) &&
commit_patch_id(a->commit, opt, >patch_id, 0))
@@ -59,7 +59,7 @@ int init_patch_ids(struct patch_ids *ids)
DIFF_OPT_SET(>diffopts, RECURSIVE);
diff_setup_done(>diffopts);
hashmap_init(>patches, (hashmap_cmp_fn)patch_id_cmp,
-NULL, 256);
+>diffopts, 256);
return 0;
 }
 
@@ -95,7 +95,7 @@ struct patch_id *has_commit_patch_id(struct commit *commit,
if (init_patch_id_entry(, commit, ids))
return NULL;
 
-   return hashmap_get(>patches, , >diffopts);
+   return hashmap_get(>patches, , NULL);
 }
 
 struct patch_id *add_commit_patch_id(struct commit *commit,
-- 
2.13.0.31.g9b732c453e