object-file.c: stop dying in parse_loose_header()

Make parse_loose_header() return error codes and data instead of
invoking die() by itself.

For now we'll move the relevant die() call to loose_object_info() and
read_loose_object() to keep this change smaller. In a subsequent
commit we'll make read_loose_object() return an error code instead of
dying. We should also address the "allow_unknown" case (should be
moved to builtin/cat-file.c), but for now I'll be leaving it.

For making parse_loose_header() not die() change its prototype to
accept a "struct object_info *" instead of the "unsigned long *sizep"
it accepted before. Its callers can now check the populated populated
"oi->typep".

Because of this we don't need to pass in the "unsigned int flags"
which we used for OBJECT_INFO_ALLOW_UNKNOWN_TYPE, we can instead do
that check in loose_object_info().

This also refactors some confusing control flow around the "status"
variable. In some cases we set it to the return value of "error()",
i.e. -1, and later checked if "status < 0" was true.

Since 93cff9a978e (sha1_loose_object_info: return error for corrupted
objects, 2017-04-01) the return value of loose_object_info() (then
named sha1_loose_object_info()) had been a "status" variable that be
any negative value, as we were expecting to return the "enum
object_type".

The only negative type happens to be OBJ_BAD, but the code still
assumed that more might be added. This was then used later in
e.g. c84a1f3ed4d (sha1_file: refactor read_object, 2017-06-21). Now
that parse_loose_header() will return 0 on success instead of the
type (which it'll stick into the "struct object_info") we don't need
to conflate these two cases in its callers.

Since parse_loose_header() doesn't need to return an arbitrary
"status" we only need to treat its "ret < 0" specially, but can
idiomatically overwrite it with our own error() return. This along
with having made unpack_loose_header() return an "enum
unpack_loose_header_result" in an earlier commit means that we can
move the previously nested if/else cases mostly into the "ULHR_OK"
branch of the "switch" statement.

We should be less silent if we reach that "status = -1" branch, which
happens if we've got trailing garbage in loose objects, see
f6371f92104 (sha1_file: add read_loose_object() function, 2017-01-13)
for a better way to handle it. For now let's punt on it, a subsequent
commit will address that edge case.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/cache.h b/cache.h
index e7d0cc3d3b4d20e0cee678373cc19282050e26d8..1181304f3f7d9f736e3e825ef47a5c6699239b74 100644 (file)
--- a/cache.h
+++ b/cache.h
@@ -1332,9 +1332,16 @@ enum unpack_loose_header_result unpack_loose_header(git_zstream *stream,
                                                    unsigned long bufsiz,
                                                    struct strbuf *hdrbuf);
 
+/**
+ * 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().
+ */
 struct object_info;
-int parse_loose_header(const char *hdr, struct object_info *oi,
-                      unsigned int flags);
+int parse_loose_header(const char *hdr, struct object_info *oi);
 
 int check_object_signature(struct repository *r, const struct object_id *oid,
                           void *buf, unsigned long size, const char *type);

diff --git a/object-file.c b/object-file.c
index 8abeb9ace87d82d3a404235ebf2445e670daeef1..e24fc4555d03ec8a96d9a10c221a960ade81152a 100644 (file)
--- a/object-file.c
+++ b/object-file.c
@@ -1324,8 +1324,7 @@ static void *unpack_loose_rest(git_zstream *stream,
  * too permissive for what we want to check. So do an anal
  * object header parse by hand.
  */
-int parse_loose_header(const char *hdr, struct object_info *oi,
-                      unsigned int flags)
+int parse_loose_header(const char *hdr, struct object_info *oi)
 {
        const char *type_buf = hdr;
        unsigned long size;
@@ -1347,15 +1346,6 @@ int parse_loose_header(const char *hdr, struct object_info *oi,
        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"));
        if (oi->typep)
                *oi->typep = type;
 
@@ -1382,7 +1372,14 @@ int parse_loose_header(const char *hdr, struct object_info *oi,
        /*
         * The length must be followed by a zero byte
         */
-       return *hdr ? -1 : type;
+       if (*hdr)
+               return -1;
+
+       /*
+        * The format is valid, but the type may still be bogus. The
+        * Caller needs to check its oi->typep.
+        */
+       return 0;
 }
 
 static int loose_object_info(struct repository *r,
@@ -1396,6 +1393,7 @@ static int loose_object_info(struct repository *r,
        char hdr[MAX_HEADER_LEN];
        struct strbuf hdrbuf = STRBUF_INIT;
        unsigned long size_scratch;
+       enum object_type type_scratch;
        int allow_unknown = flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE;
 
        if (oi->delta_base_oid)
@@ -1427,6 +1425,8 @@ static int loose_object_info(struct repository *r,
 
        if (!oi->sizep)
                oi->sizep = &size_scratch;
+       if (!oi->typep)
+               oi->typep = &type_scratch;
 
        if (oi->disk_sizep)
                *oi->disk_sizep = mapsize;
@@ -1434,6 +1434,18 @@ static int loose_object_info(struct repository *r,
        switch (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),
                                    allow_unknown ? &hdrbuf : NULL)) {
        case ULHR_OK:
+               if (parse_loose_header(hdrbuf.len ? hdrbuf.buf : hdr, oi) < 0)
+                       status = error(_("unable to parse %s header"), oid_to_hex(oid));
+               else if (!allow_unknown && *oi->typep < 0)
+                       die(_("invalid object type"));
+
+               if (!oi->contentp)
+                       break;
+               *oi->contentp = unpack_loose_rest(&stream, hdr, *oi->sizep, oid);
+               if (*oi->contentp)
+                       goto cleanup;
+
+               status = -1;
                break;
        case ULHR_BAD:
                status = error(_("unable to unpack %s header"),
@@ -1445,31 +1457,16 @@ static int loose_object_info(struct repository *r,
                break;
        }
 
-       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 >= 0 && oi->contentp) {
-               *oi->contentp = unpack_loose_rest(&stream, hdr,
-                                                 *oi->sizep, oid);
-               if (!*oi->contentp) {
-                       git_inflate_end(&stream);
-                       status = -1;
-               }
-       } else
-               git_inflate_end(&stream);
-
+       git_inflate_end(&stream);
+cleanup:
        munmap(map, mapsize);
        if (oi->sizep == &size_scratch)
                oi->sizep = NULL;
        strbuf_release(&hdrbuf);
+       if (oi->typep == &type_scratch)
+               oi->typep = NULL;
        oi->whence = OI_LOOSE;
-       return (status < 0) ? status : 0;
+       return status;
 }
 
 int obj_read_use_lock = 0;
@@ -2533,6 +2530,7 @@ int read_loose_object(const char *path,
        git_zstream stream;
        char hdr[MAX_HEADER_LEN];
        struct object_info oi = OBJECT_INFO_INIT;
+       oi.typep = type;
        oi.sizep = size;
 
        *contents = NULL;
@@ -2549,12 +2547,13 @@ int read_loose_object(const char *path,
                goto out;
        }
 
-       *type = parse_loose_header(hdr, &oi, 0);
-       if (*type < 0) {
+       if (parse_loose_header(hdr, &oi) < 0) {
                error(_("unable to parse header of %s"), path);
                git_inflate_end(&stream);
                goto out;
        }
+       if (*type < 0)
+               die(_("invalid object type"));
 
        if (*type == OBJ_BLOB && *size > big_file_threshold) {
                if (check_stream_oid(&stream, hdr, *size, path, expected_oid) < 0)

diff --git a/streaming.c b/streaming.c
index bd89c50e7b3b8069334daad6870b64eb72f13a77..fe54665d86e72a3400a6f60f610a5264bc0f0667 100644 (file)
--- a/streaming.c
+++ b/streaming.c
@@ -225,6 +225,7 @@ static int open_istream_loose(struct git_istream *st, struct repository *r,
 {
        struct object_info oi = OBJECT_INFO_INIT;
        oi.sizep = &st->size;
+       oi.typep = type;
 
        st->u.loose.mapped = map_loose_object(r, oid, &st->u.loose.mapsize);
        if (!st->u.loose.mapped)
@@ -238,7 +239,7 @@ static int open_istream_loose(struct git_istream *st, struct repository *r,
        case ULHR_TOO_LONG:
                goto error;
        }
-       if (parse_loose_header(st->u.loose.hdr, &oi, 0) < 0)
+       if (parse_loose_header(st->u.loose.hdr, &oi) < 0 || *type < 0)
                goto error;
 
        st->u.loose.hdr_used = strlen(st->u.loose.hdr) + 1;