Re: [PATCH] builtin/difftool: intialize some hashmap variables

From: Jeff King <peff@peff.net>
Date: Nov 11, 2024, 22:09 UTC
Message-ID: <20241111220959.GE5019@coredump.intra.peff.net>
In-Reply-To: <09bf8815-320b-494a-87c4-34bb0df08595@efficios.com>

On Mon, Nov 11, 2024 at 04:22:26PM -0500, Simon Marchi wrote:

Show 7 quoted lines

> > The fix makes sense. I wondered if this had been broken for a long time,
> > and if so, how we managed not to notice it. But it looks like it is a
> > recent problem, via 7f795a1715 (builtin/difftool: plug several trivial
> > memory leaks, 2024-09-26).
> 
> Are there tests for this specific scenario (no diff between the two
> versions)?

Presumably not, or they'd have been segfaulting. ;) So it may be worth adding some to t7800.

Show 8 quoted lines

> > I'm not sure if zero-initialization is being a little too familiar with
> > the hashmap internals, though.
> 
> Up to you.  In other C projects I worked on, it was typical that
> zero-ing an object would get it in a valid initial empty state, properly
> handled by the destruction functions.  This way, a big struct containing
> other objects could be initialized simply by zero-ing it, without having
> to initialize each component explicitly.

We often follow that rule in git.git, too, but have been increasingly moving to macro initializers. They make it easier if we ever need a non-zero state as an invariant (e.g., STRBUF_INIT always points to a dummy string). In this case, it does look like HASHMAP_INIT sets do_count_items.

It's not a bug in your program since we still hashmap_init() over the zero'd state, but it does feel a bit weird to me.

> Please let me know if you want a v2 or if you are just going to merge an
> updated version of this patch.

That's up to the maintainer. IMHO it's worth using HASHMAP_INIT, though, and perhaps adding a test.

-Peff