Re: [PATCH] blame.c: replace instance of !oidcmp for oideq

["René Scharfe" <l.s.r@web.de>]

Am 09.09.20 um 21:17 schrieb Jeff King:
> On Wed, Sep 09, 2020 at 03:13:46PM -0400, Jeff King wrote:
>
>> Which really _seems_ like a bug in coccinelle, unless I am missing
>> something. Because both of those parameters look like object_id pointers
>> (and the compiler would be complaining if it were not the case).  But I
>> also wonder if giving the specific types in the coccinelle rule is
>> buying us anything. If you passed two void pointers or ints or whatever
>> to !oidcmp(), we'd still want to rewrite it as oideq().

Right, using expressions for such a like-for-like transformation is safe
and practical in the sense that it won't break correct code, and broken
code will be flagged by the compiler.

>
> And indeed, just blindly swapping out "struct object_id" for
> "expression" in the coccinelle file (patch below), shows another spot
> that was missed:
>
> 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.

We better make sure our semantic patches are safe, otherwise we have to
check all conversions very carefully, and then we might be better off
doing them manually..

René