Re: [PATCH v8 1/4] sha1_file.c: support reading from a loose object of unknown type
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 15, 2015, 20:21 UTC
- Message-ID
- <xmqqmw29jg78.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1429117143-4882-1-git-send-email-karthik.188@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
Show 19 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. > > Helped-by: Junio C Hamano <gitster@pobox.com> > Helped-by: Eric Sunshine <sunshine@sunshineco.com> > Signed-off-by: Karthik Nayak <karthik.188@gmail.com> > ---
I see that you made the type parsing to happen earlier than the previous round (which used to do the size first and then type).
Not a problem, though. Just something I noticed...
Show 30 quoted lines
> @@ -1614,27 +1642,40 @@ 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)
> +int parse_sha1_header_extended(const char *hdr, struct object_info *oi,
> + unsigned int flags)
> {
> - char type[10];
> - int i;
> + struct strbuf typename = STRBUF_INIT;
> unsigned long size;
> + int type;
>
> /*
> * The type can be at most ten bytes (including the
> * terminating '\0' that we add), and is followed by
> * a space.
> */
> - i = 0;
> for (;;) {
> char c = *hdr++;
> if (c == ' ')
> break;
> - type[i++] = c;
> - if (i >= sizeof(type))
> - return -1;
> + strbuf_addch(&typename, c);
> }
> - type[i] = 0;This _might_ have some performance impact in that strbuf_addch() involves strbuf_grow(*, 1), which does "does it overflow to increment sb->len by one?"; I would say it should be unmeasurable because the function is expected to be used only on loose objects and you shouldn't have very many of them without packing in your repository in the first place.
I guess Peff's c1822d4f (strbuf: add an optimized 1-character strbuf_grow, 2015-04-04) may want to teach strbuf_addch() to use his new strbuf_grow_ch(), and once that happens the performance worry would disappear without this code to be changed at all.
Show 6 quoted lines
> @@ -2541,7 +2596,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,
> * return value implicitly indicates whether the
> * object even exists.
> */
> - if (!oi->typep && !oi->sizep) {
> + if (!oi->typep && !oi->typename && !oi->sizep) {You didn't have this check in the earlier round, and this new one is correct, I think. Good eyes to spot this potential problem.
Thanks.