[01/17] describe: fix accidental oid/hash type-punning
diff mbox series

Message ID 20190620074050.GA3713@sigill.intra.peff.net
State New
Headers show
Series
  • drop non-object_id hashing
Related show

Commit Message

Jeff King June 20, 2019, 7:40 a.m. UTC
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.

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).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/describe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Junio C Hamano June 20, 2019, 4:32 p.m. UTC | #1
Jeff King <peff@peff.net> writes:

> 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.

>
> 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,
Jeff King June 20, 2019, 6:25 p.m. UTC | #2
On Thu, Jun 20, 2019 at 09:32:49AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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.

It only looks impressive in retrospect. It became very obvious when
updating the reference to sha1hash(peeled->hash) in the same line, and
then scratching my head about why this one was looking at the hash
member. But history rewriting lets me re-order it to make myself look
good. Thanks, "rebase -i"! :)

-Peff

Patch
diff mbox series

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,