Re: [PATCH] hashmap: ensure hashmaps are reusable after hashmap_clear()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 29, 2025, 16:55 UTC
- Message-ID
- <xmqqwmb26h6x.fsf@gitster.g>
- In-Reply-To
- <pull.1911.git.1745941663160.gitgitgadget@gmail.com>
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 22 quoted lines
> From: Elijah Newren <newren@gmail.com> > > In the series merged at bf0a430f70b5 (Merge branch 'en/strmap', > 2020-11-21), strmap was built on top of hashmap and hashmap was extended > in a few ways to support strmap and be more generally useful. One of > the extensions was that hashmap_partial_clear() was introduced to allow > reuse of the hashmap without freeing the table. Peff believed that it > also made sense to introduce a hashmap_clear() which freed everything > while allowing reuse. > > I added hashmap_clear(), but in doing so, overlooked the fact that for > a hashmap to be reusable, it needs a defined cmpfn and data (the > HASHMAP_INIT macro requires these fields as parameters, for example). > So, if we want the hashmap to be reusable, we shouldn't zero out those > fields. We probably also shouldn't zero out do_count_items. (We could > zero out grow_at and shrink_at, but whether we zero those or not is > irrelevant as they'll be automatically updated whenever a new entry is > inserted.) > > Since clearing is associated with freeing map->table, and the only thing > required for consistency after freeing map->table is zeroing tablesize > and private_size, let's only zero those fields out.
Makes sense. Thanks for finding and fixing.
I do not think we want to patch all the way down to Git 2.30, ...
Show 18 quoted lines
> diff --git a/hashmap.c b/hashmap.c > index ee45ef00852..a711377853f 100644 > --- a/hashmap.c > +++ b/hashmap.c > @@ -205,8 +205,9 @@ void hashmap_clear_(struct hashmap *map, ssize_t entry_offset) > return; > if (entry_offset >= 0) /* called by hashmap_clear_and_free */ > free_individual_entries(map, entry_offset); > - free(map->table); > - memset(map, 0, sizeof(*map)); > + FREE_AND_NULL(map->table); > + map->tablesize = 0; > + map->private_size = 0; > } > > struct hashmap_entry *hashmap_get(const struct hashmap *map, > > base-commit: f65182a99e545d2f2bc22e6c1c2da192133b16a3
... but this part of the code has been fairly quiet and the patch applies very cleanly. So I'll apply on top of bf0a430f7 and merge the result---anybody maintaining Git for their LTS distro can then merge it to their favorite ancient maintenance track ;-)
Thanks.