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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 4, 2015, 23:34 UTC
Message-ID: <CAPig+cTitnC7zvWJEwdo2cMWNE+3ODjd++81bYdg-qTc0bzgog@mail.gmail.com>
In-Reply-To: <1430663402-26717-1-git-send-email-karthik.188@gmail.com>

On Sun, May 3, 2015 at 10:29 AM, Karthik Nayak <karthik.188@gmail.com> wrote:

Show 32 quoted lines

> 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 */.

Show 23 quoted lines

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

Show 19 quoted lines

> +       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;
> +}
> +