Re: [PATCH 01/17] describe: fix accidental oid/hash type-punning

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 20, 2019, 16:32 UTC
Message-ID: <xmqq7e9gp68e.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190620074050.GA3713@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 7 quoted lines

> The find_commit_name() function passes an object_id.hash as the key of a
> hashmap. That ends up in commit_name_neq(), which then feeds it to
> oideq(). Which means we should actually be the whole "struct object_id".
>
> It works anyway because pointers to the two are interchangeable. And
> because we're going through a layer of void pointers, the compiler
> doesn't notice the type mismatch.

Wow. Good eyes. I wouldn't have noticed this (and for the reasons you stated, it is very tricky for any clever compiler to notice it).

Impressed.

> But it's worth cleaning up (especially since once we switch away from
> sha1hash() on the same line, accessing the hash member will look doubly
> out of place).

Yup.  Thanks.

Show 19 quoted lines

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/describe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/describe.c b/builtin/describe.c
> index 1409cedce2..0a5cde00a2 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -76,7 +76,7 @@ static int commit_name_neq(const void *unused_cmp_data,
>  
>  static inline struct commit_name *find_commit_name(const struct object_id *peeled)
>  {
> -	return hashmap_get_from_hash(&names, sha1hash(peeled->hash), peeled->hash);
> +	return hashmap_get_from_hash(&names, sha1hash(peeled->hash), peeled);
>  }
>  
>  static int replace_name(struct commit_name *e,