From: Junio C Hamano <gitster@pobox•com>
To: "Elijah Newren via GitGitGadget" <gitgitgadget@gmail•com>
Cc: git@vger•kernel.org, Elijah Newren <newren@gmail•com>
Subject: Re: [PATCH] hashmap: ensure hashmaps are reusable after hashmap_clear()
Date: Tue, 29 Apr 2025 09:55:18 -0700 [thread overview]
Message-ID: <xmqqwmb26h6x.fsf@gitster.g> (raw)
In-Reply-To: <pull.1911.git.1745941663160.gitgitgadget@gmail.com> (Elijah Newren via GitGitGadget's message of "Tue, 29 Apr 2025 15:47:43 +0000")
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail•com> writes:
> 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, ...
> 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.
prev parent reply other threads:[~2025-04-29 16:55 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-04-29 15:47 [PATCH] hashmap: ensure hashmaps are reusable after hashmap_clear() Elijah Newren via GitGitGadget
2025-04-29 16:55 ` Junio C Hamano [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqwmb26h6x.fsf@gitster.g \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitgitgadget@gmail$(echo .)com \
--cc=newren@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox