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 :).
> @@ -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.
> @@ -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.
> 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
