From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 09 Sep 2020 20:03:06 GMT
Subject: Re: [PATCH] blame.c: replace instance of !oidcmp for oideq
Message-ID: <xmqqa6xyvhtx.fsf@gitster.c.googlers.com>
In-Reply-To: <0773b560-b456-fc88-42d7-f214246ddd1b@web.de>

René Scharfe <l.s.r@web.de> writes:

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

;-)  A good counter-example.

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

Yes, that is a sensible suggestion.