Re: [PATCH v6 14/22] object-file.c: stop dying in parse_loose_header()
From: Taylor Blau <hidden>
Date: 2021-09-17 02:32:38
On Tue, Sep 07, 2021 at 12:58:09PM +0200, Ævar Arnfjörð Bjarmason wrote:
It thus makes sense to further libify the interface so that it stops calling die() when it encounters OBJ_BAD, and instead rely on its callers to check the populated "oi->typep".
Hmm. I thought we got rid of this behavior in a previous commit? Perhaps I'm thinking of something else, but I would certainly appreciate a clarification :).
quoted hunk ↗ jump to hunk
@@ -1369,15 +1367,6 @@ int parse_loose_header(const char *hdr, type = type_from_string_gently(type_buf, type_len, 1); if (oi->type_name) strbuf_add(oi->type_name, type_buf, type_len); - /* - * Set type to 0 if its an unknown object and - * we're obtaining the type using '--allow-unknown-type' - * option. - */ - if ((flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE) && (type < 0)) - type = 0; - else if (type < 0) - die(_("invalid object type"));
Good, this part moved to loose_object_info() as you said it would.
quoted hunk ↗ jump to hunk
@@ -1463,18 +1460,20 @@ static int loose_object_info(struct repository *r, status = error(_("unable to unpack %s header"), oid_to_hex(oid)); } - - if (status < 0) { - /* Do nothing */ - } else if (hdrbuf.len) { - if ((status = parse_loose_header(hdrbuf.buf, oi, flags)) < 0) - status = error(_("unable to parse %s header with --allow-unknown-type"), - oid_to_hex(oid)); - } else if ((status = parse_loose_header(hdr, oi, flags)) < 0) { - status = error(_("unable to parse %s header"), oid_to_hex(oid)); + if (!status) { + if (!parse_loose_header(hdrbuf.len ? hdrbuf.buf : hdr, oi)) + /* + * oi->{sizep,typep} are meaningless unless + * parse_loose_header() returns >= 0. + */
This double negative is a little confusing. Clearer to say:
"oi->{size,type}p is meaningless if parse_loose_header() returns < 0"?
But I was also a little confused to see that the expression we are
checking here is just that parse_loose_header() returned zero. What
about other positive values?
I think we should either update the comment to say "unless it returns
zero" or the conditional expression to check for >= 0.
quoted hunk ↗ jump to hunk
diff --git a/object-store.h b/object-store.h index 4064710ae29..584bf5556af 100644 --- a/object-store.h +++ b/object-store.h@@ -500,8 +500,17 @@ int for_each_packed_object(each_packed_object_fn, void *, int unpack_loose_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz, struct strbuf *hdrbuf); -int parse_loose_header(const char *hdr, struct object_info *oi, - unsigned int flags); + +/** + * parse_loose_header() parses the starting "<type> <len>\0" of an + * object. If it doesn't follow that format -1 is returned. To check + * the validity of the <type> populate the "typep" in the "struct + * object_info". It will be OBJ_BAD if the object type is unknown. The + * parsed <len> can be retrieved via "oi->sizep", and from there + * passed to unpack_loose_rest(). + */ +int parse_loose_header(const char *hdr, struct object_info *oi);
OK, I guess this must be what I was confused about earlier (that I thought we didn't support reading typep if returning OBJ_BAD). But it seems odd to me that we would get rid of it elsewhere, yet continue using this pattern here. Or am I mistaken that the two are different? Thanks, Taylor