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

[David Turner <dturner@twopensource.com>]

On Thu, 2015-02-26 at 01:29 +0530, karthik nayak wrote:
> 
> 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.

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

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