From: Jeff King <peff@peff.net>
Date: Wed, 09 Sep 2020 19:58:03 GMT
Subject: Re: [PATCH] blame.c: replace instance of !oidcmp for oideq
Message-ID: <20200909195803.GA2515827@coredump.intra.peff.net>
In-Reply-To: <0773b560-b456-fc88-42d7-f214246ddd1b@web.de>

On Wed, Sep 09, 2020 at 09:54:55PM +0200, René Scharfe wrote:

> > diff -u -p a/packfile.c b/packfile.c
> > --- a/packfile.c
> > +++ b/packfile.c
> > @@ -735,7 +735,7 @@ struct packed_git *add_packed_git(const
> >  	p->mtime = st.st_mtime;
> >  	if (path_len < the_hash_algo->hexsz ||
> >  	    get_sha1_hex(path + path_len - the_hash_algo->hexsz, p->hash))
> > -		hashclr(p->hash);
> > +		oidclr(p);
> >  	return p;
> >  }
> >
> >
> > Maybe it's worth being looser in our cocci patch definitions. I'm having
> > trouble thinking of a downside...
> 
> For transformations that change the type as in the example above we
> should insist on getting the right one, otherwise we might introduce
> bugs -- like in the example above.  p points to a struct packed_git and
> not to a struct object_id, so this introduces a type mismatch.

Heh. You'd think that I would have applied that patch and run "make". Or
even read it carefully.

Thanks for pointing that out. I guess now we have a real example of a
downside (the compiler _would_ still catch it, but it means "make
coccicheck" is useless if it's repeatedly suggesting a bad
transformation).

-Peff