From: karthik nayak <karthik.188@gmail.com>
Date: Wed, 25 Feb 2015 19:59:41 GMT
Subject: Re: [PATCH 1/2] sha1_file: Add sha1_object_type_literally and export it.
Message-ID: <54EE29AD.4060802@gmail.com>
In-Reply-To: <1424888526.2968.3.camel@leckie>



On 02/25/2015 11:52 PM, David Turner wrote:
> On Wed, 2015-02-25 at 16:37 +0530, Karthik Nayak wrote:
>> +	unsigned long mapsize;
>> ...
>> +	map = map_sha1_file(sha1, &mapsize);
>
> I know this is a pre-existing issue, but I'm not sure "unsigned long" is
> the right type here.  Shouldn't it be a size_t?
I got the type from the function definition of map_sha1_file(), which is 
"void *map_sha1_file(const unsigned char *sha1, unsigned long *size)"
>
>> +	if (!map)
>> +		return -1;
>> +	if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)
>> +		status = error("unable to unpack %s header",
>> +			       sha1_to_hex(sha1));
>> +
>> +	for (i = 0; i < 32; i++) {
>
> This number should probably be a constant.
Do you want me to define it as a preprocessor directive?
>
>> +		if (hdr[i] == ' ') {
>> +			type[i] = '\0';
>> +			break;
>> +		}
>> +		type[i] = hdr[i];
>> +	}
>
> type might end up without a trailing \0 here in the case where hdr has
> no space in it.  Is this possible?
What's possible is when the object type name is greater than 32bytes 
"hdr" will not be able to hold the whole type, its a tradeoff, I guess I 
should put a null terminator at the end of "hdr". What do you suggest?
>
>