Re: [PATCH v10 1/4] sha1_file: support reading from a loose object of unknown type

[karthik nayak <karthik.188@gmail.com>]

On 05/05/2015 05:04 AM, Eric Sunshine wrote:
> On Sun, May 3, 2015 at 10:29 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Update sha1_loose_object_info() to optionally allow it to read
>> from a loose object file of unknown/bogus type; as the function
>> usually returns the type of the object it read in the form of enum
>> for known types, add an optional "typename" field to receive the
>> name of the type in textual form and a flag to indicate the reading
>> of a loose object file of unknown/bogus type.
>>
>> Add parse_sha1_header_extended() which acts as a wrapper around
>> parse_sha1_header() allowing more information to be obtained.
>>
>> Add unpack_sha1_header_to_strbuf() to unpack sha1 headers of
>> unknown/corrupt objects which have a unknown sha1 header size to
>> a strbuf structure. This was written by Junio C Hamano but tested
>> by me.
>> ---
>> diff --git a/cache.h b/cache.h
>> index 3d3244b..38419c3 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -1564,6 +1564,33 @@ int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long ma
>> +static int unpack_sha1_header_to_strbuf(git_zstream *stream, unsigned char *map,
>> +                                       unsigned long mapsize, void *buffer,
>> +                                       unsigned long bufsiz, struct strbuf *header)
>> +{
>> +       unsigned char *cp;
>> +       int status;
>> +
>> +       status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);
>> +
>> +       /*
>> +        * Check if entire header is unpacked in the first iteration.
>> +        */
>
> Nit: You could save some precious vertical screen real-estate by using
> one-line /* comment style */.
>
>> +       if (memchr(buffer, '\0', stream->next_out - (unsigned char *)buffer))
>> +               return 0;
>> +
>> @@ -1614,27 +1641,38 @@ static void *unpack_sha1_rest(git_zstream *stream, void *buffer, unsigned long s
>>    * too permissive for what we want to check. So do an anal
>>    * object header parse by hand.
>>    */
>> -int parse_sha1_header(const char *hdr, unsigned long *sizep)
>> +static int parse_sha1_header_extended(const char *hdr, struct object_info *oi,
>> +                              unsigned int flags)
>>   {
>> [...]
>> +
>> +       type = type_from_string_gently(type_buf, type_len, 1);
>> +       if (oi->typename)
>> +               strbuf_add(oi->typename, type_buf, type_len);
>> +       /*
>> +        * Set type to 0 if its an unknown object and
>> +        * we're obtaining the type using '--allow-unkown-type'
>> +        * option.
>> +        */
>> +       if ((flags & LOOKUP_UNKNOWN_OBJECT) && (type < 0))
>> +               type = 0;
>
> The comment says exactly what the code already says, thus it adds no
> value. A better comment would explain _why_ type is set to 0 under
> these conditions.
>
>> +       else if (type < 0)
>> +               die("invalid object type");
>> +       if (oi->typep)
>> +               *oi->typep = type;
>>
>>          /*
>>           * The length must follow immediately, and be in canonical
>> @@ -1652,12 +1690,24 @@ int parse_sha1_header(const char *hdr, unsigned long *sizep)
>>                          size = size * 10 + c;
>>                  }
>>          }
>> -       *sizep = size;
>> +
>> +       if (oi->sizep)
>> +               *oi->sizep = size;
>>
>>          /*
>>           * The length must be followed by a zero byte
>>           */
>
> Nit: Save precious vertical screen real-estate with one-line /*
> comment style */.
>
>> -       return *hdr ? -1 : type_from_string(type);
>> +       return *hdr ? -1 : type;
>> +}
>> +

Thanks for your suggestions Eric.