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

From: Clément Poulain <clement.poulain@ensimag.imag.fr>
Date: Jun 15, 2010, 09:29 UTC
Message-ID: <0091febb4a3832a6680a0fbc2209f841@ensimag.fr>
In-Reply-To: <7vfx0p9wlm.fsf@alter.siamese.dyndns.org>

On Mon, 14 Jun 2010 13:40:21 -0700, Junio C Hamano <gitster@pobox.com> wrote:

Show 23 quoted lines

> 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. Is it the same issue ? Or is it different because it's not a pointer cast?

Otherwise, we thought of reversing the conversion. That is to say, instead of casting "long *" in "size_t *" when calling textconv_object(), is it better to cast size_t in "unsigned long" in textconv_object():

	*buf_size = (unsigned long) fill_textconv(textconv, df, buf); ?