Re: [PATCH 1/2] sha1_file: Add sha1_object_type_literally and export it.

From: David Turner <dturner@twopensource.com>
Date: Feb 25, 2015, 20:15 UTC
Message-ID: <1424895352.2968.8.camel@leckie>
In-Reply-To: <54EE29AD.4060802@gmail.com>

On Thu, 2015-02-26 at 01:29 +0530, karthik nayak wrote:

Show 11 quoted lines

> 
> 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)"

Yep. That's why I describe it as a pre-existing issue - it's not your fault that the type is funny. But it might be worth cleaning up while you're in here. Or it might not -- maybe I'm totally off base and the type is correct.

Show 11 quoted lines

> >
> >> +	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?

Sounds good to me.

Show 12 quoted lines

> >> +		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?

Is it an error for the object type name to be > 32 bytes? If there is no max length, then maybe you should use the strbuf API for this. Silently truncating is probably the wrong thing to do, and leaving it unterminated is dangerous unless callers know how to handle that case. If it is an error, then you should return an error code.