Re: [PATCH v6 14/22] object-file.c: stop dying in parse_loose_header()
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Sep 17, 2021, 02:32 UTC
- Message-ID
- <YUP+Ni1tFKolLZd4@nand.local>
- In-Reply-To
- <patch-v6-14.22-7c9819d37c5-20210907T104559Z-avarab@gmail.com>
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 :).
Show 13 quoted lines
> @@ -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.
Show 19 quoted lines
> @@ -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.
Show 20 quoted lines
> 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