From: karthik nayak <karthik.188@gmail.com>
Date: Mon, 20 Apr 2015 18:43:30 GMT
Subject: Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type
Message-ID: <553548D2.7010904@gmail.com>
In-Reply-To: <xmqq4moepijp.fsf@gitster.dls.corp.google.com>



On 04/18/2015 02:40 AM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Fri, Apr 17, 2015 at 09:21:31AM -0700, Junio C Hamano wrote:
>>
>>> Jeff King <peff@peff.net> writes:
>>>
>>>> If there _is_ a performance implication to worry about here, I think it
>>>> would be that we are doing an extra malloc/free.
>>>
>>> Thanks for reminding me; yes, that also worried me.
>>
>> As an aside, I worried about the extra allocation for reading the header
>> in the first place. But it looks like we only do this on the --literally
>> code path (and otherwise use the normal unpack_sha1_header).  Still, I
>> wonder if we could make this work automagically.  That is, speculatively
>> unpack the first N bytes, assuming we hit the end-of-header. If not,
>> then go to a strbuf as the slow path. Then it would be fine to cover all
>> cases; the normal ones would be fast, and only ridiculous things would
>> incur the extra allocation.
>
> Yes, that was what I was hoping to see eventually ;-)
>

Sorry for the delay, So after reading what Jeff said I tried to 
implement it, this might not be a final version of the change, more of a 
RFC version. What do you'll think?

@@ -1564,6 +1564,36 @@ int unpack_sha1_header(git_zstream *stream, 
unsigned char *map, unsigned long ma
         return git_inflate(stream, 0);
  }

+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;
+       int i = 0;
+
+       status = unpack_sha1_header(stream, map, mapsize, buffer, bufsiz);
+
+       for (cp = buffer; cp < stream->next_out; cp++)
+               if (!*cp) {
+                       /* Found the NUL at the end of the header */
+                       return 0;
+               }
+
+       strbuf_add(header, buffer, stream->next_out - (unsigned char 
*)buffer);
+       do {
+               status = git_inflate(stream, 0);
+               strbuf_add(header, buffer, stream->next_out - (unsigned 
char *)buffer);
+               for (cp = buffer; cp < stream->next_out; cp++)
+                       if (!*cp)
+                               /* Found the NUL at the end of the header */
+                               return 0;
+               stream->next_out = buffer;
+               stream->avail_out = bufsiz;
+       } while (status != Z_STREAM_END);
+       return -1;
+}
+


@@ -2555,17 +2610,24 @@ static int sha1_loose_object_info(const unsigned 
char *sha1,
                 return -1;
         if (oi->disk_sizep)
                 *oi->disk_sizep = mapsize;
-       if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)
+       if ((flags & LOOKUP_LITERALLY)) {
+               if (unpack_sha1_header_to_strbuf(&stream, map, mapsize, 
hdr, sizeof(hdr), &hdrbuf) < 0)
+                       status = error("unable to unpack %s header with 
--literally",
+                                      sha1_to_hex(sha1));
+       } else if (unpack_sha1_header(&stream, map, mapsize, hdr, 
sizeof(hdr)) < 0)
                 status = error("unable to unpack %s header",
                                sha1_to_hex(sha1));
-       else if ((status = parse_sha1_header(hdr, &size)) < 0)
+       if (hdrbuf.len) {
+               if ((status = parse_sha1_header_extended(hdrbuf.buf, oi, 
flags)) < 0)
+                       status = error("unable to parse %s header with 
--literally",
+                                      sha1_to_hex(sha1));
+       } else if ((status = parse_sha1_header_extended(hdr, oi, flags)) 
< 0)
                 status = error("unable to parse %s header", 
sha1_to_hex(sha1));


and