Re: [PATCH v2 2/3] textconv: support for blame

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 15, 2010, 15:00 UTC
Message-ID: <7vfx0o8hop.fsf@alter.siamese.dyndns.org>
In-Reply-To: <0091febb4a3832a6680a0fbc2209f841@ensimag.fr>

Clément Poulain <clement.poulain@ensimag.imag.fr> writes:

Show 33 quoted lines

> On Mon, 14 Jun 2010 13:40:21 -0700, Junio C Hamano <gitster@pobox.com>
> wrote:
>> Axel Bonnet <axel.bonnet@ensimag.imag.fr> writes:
>> 
>>> @@ -86,16 +87,49 @@ struct origin {
>>> ...
>>> +static void fill_origin_blob(struct diff_options *opt,
>>> +			     struct origin *o, mmfile_t *file)
>>>  {
>>>  	if (!o->file.ptr) {
>>>  		enum object_type type;
>>>  		num_read_blob++;
>>> -		file->ptr = read_sha1_file(o->blob_sha1, &type,
>>> -					   (unsigned long *)(&(file->size)));
>>> +
>>> +		if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&
>>> +		    textconv_object(o->path, o->blob_sha1, &file->ptr,
>>> +				    (size_t *) &file->size))
>> 
>> This cast is not correct, as there is no guarantee that your size_t and
>> typeof(mmfile_t.size) are compatible.  Depending on the gcc version, you
>> would get "dereferencing type-punned pointer will break strict-aliasing
>> rules" error.
>> 
>> The same issue exists in Clément's patch to builtin/cat-file.c.
>
> We did this way because we found a similar cast in prep_temp_blob(),
> diff.c:
>
> 	if (convert_to_working_tree(path,
> 			(const char *)blob, (size_t)size, &buf)) {
>
> where size is an unsigned long.

That is a completely different kind of cast that is sane. The function takes a _value_ of type size_t.

Your cast is "This function wants to store a value to a _memory location_ that is supposed to hold a value of type size_t, and I know &(file->size) is not such a location (it is to hold a value of type 'unsigned long'); please pretend that these two distinct pointers are compatible", which is a big no-no.