object-file: always set OI_LOOSE when reading object info

There are some early returns in `odb_source_loose_read_object_info()`
in cases where we don't have to open the loose object. These return
paths do not set `struct object_info::whence` to `OI_LOOSE` though, so
it becomes impossible for the caller to tell the format of such an
object.

The root cause of this really is that we have so many different return
paths in the function. As a consequence, it's harder than necessary to
make sure that all successful exit paths sot up the `whence` field as
expected.

Address this by refactoring the function to have a single exit path.
Like this, we can trivially set up the `whence` field when we exit
successfully from the function.

Note that we also:

  - Rename `status` to `ret` to match our usual coding style, but also
    to show that the old `status` variable is now always getting the
    expected value. Furthermore, the value is not initialized anymore,
    which has the consequence that most compilers will warn for exit
    paths where we forgot to set it.

  - Move the setup of scratch pointers closer to `parse_loose_header()`
    to show where it's needed.

  - Guard a couple of variables on cleanup so that they only get
    released in case they have been set up.

  - Reset `oi->delta_base_oid` towards the end of the function, together
    with all the other object info pointers.

Overall, all these changes result in a diff that is somewhat hard to
read. But the end result is significantly easier to read and reason
about, so I'd argue this one-time churn is worth it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/object-file.c b/object-file.c
index 6280e42f3412c363a7e2629f6a5a8e7aef4a9a33..e7e4c3348f9c1bb2bc909d13a41e1eb38a946743 100644 (file)
--- a/object-file.c
+++ b/object-file.c
@@ -416,19 +416,16 @@ int odb_source_loose_read_object_info(struct odb_source *source,
                                      const struct object_id *oid,
                                      struct object_info *oi, int flags)
 {
-       int status = 0;
+       int ret;
        int fd;
        unsigned long mapsize;
        const char *path;
-       void *map;
-       git_zstream stream;
+       void *map = NULL;
+       git_zstream stream, *stream_to_end = NULL;
        char hdr[MAX_HEADER_LEN];
        unsigned long size_scratch;
        enum object_type type_scratch;
 
-       if (oi && oi->delta_base_oid)
-               oidclr(oi->delta_base_oid, source->odb->repo->hash_algo);
-
        /*
         * If we don't care about type or size, then we don't
         * need to look inside the object at all. Note that we
@@ -439,71 +436,101 @@ int odb_source_loose_read_object_info(struct odb_source *source,
         */
        if (!oi || (!oi->typep && !oi->sizep && !oi->contentp)) {
                struct stat st;
-               if ((!oi || !oi->disk_sizep) && (flags & OBJECT_INFO_QUICK))
-                       return quick_has_loose(source->loose, oid) ? 0 : -1;
-               if (stat_loose_object(source->loose, oid, &st, &path) < 0)
-                       return -1;
+
+               if ((!oi || !oi->disk_sizep) && (flags & OBJECT_INFO_QUICK)) {
+                       ret = quick_has_loose(source->loose, oid) ? 0 : -1;
+                       goto out;
+               }
+
+               if (stat_loose_object(source->loose, oid, &st, &path) < 0) {
+                       ret = -1;
+                       goto out;
+               }
+
                if (oi && oi->disk_sizep)
                        *oi->disk_sizep = st.st_size;
-               return 0;
+
+               ret = 0;
+               goto out;
        }
 
        fd = open_loose_object(source->loose, oid, &path);
        if (fd < 0) {
                if (errno != ENOENT)
                        error_errno(_("unable to open loose object %s"), oid_to_hex(oid));
-               return -1;
+               ret = -1;
+               goto out;
        }
-       map = map_fd(fd, path, &mapsize);
-       if (!map)
-               return -1;
 
-       if (!oi->sizep)
-               oi->sizep = &size_scratch;
-       if (!oi->typep)
-               oi->typep = &type_scratch;
+       map = map_fd(fd, path, &mapsize);
+       if (!map) {
+               ret = -1;
+               goto out;
+       }
 
        if (oi->disk_sizep)
                *oi->disk_sizep = mapsize;
 
+       stream_to_end = &stream;
+
        switch (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr))) {
        case ULHR_OK:
-               if (parse_loose_header(hdr, oi) < 0)
-                       status = error(_("unable to parse %s header"), oid_to_hex(oid));
-               else if (*oi->typep < 0)
+               if (!oi->sizep)
+                       oi->sizep = &size_scratch;
+               if (!oi->typep)
+                       oi->typep = &type_scratch;
+
+               if (parse_loose_header(hdr, oi) < 0) {
+                       ret = error(_("unable to parse %s header"), oid_to_hex(oid));
+                       goto corrupt;
+               }
+
+               if (*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;
+               if (oi->contentp) {
+                       *oi->contentp = unpack_loose_rest(&stream, hdr, *oi->sizep, oid);
+                       if (!*oi->contentp) {
+                               ret = -1;
+                               goto corrupt;
+                       }
+               }
 
-               status = -1;
                break;
        case ULHR_BAD:
-               status = error(_("unable to unpack %s header"),
-                              oid_to_hex(oid));
-               break;
+               ret = error(_("unable to unpack %s header"),
+                           oid_to_hex(oid));
+               goto corrupt;
        case ULHR_TOO_LONG:
-               status = error(_("header for %s too long, exceeds %d bytes"),
-                              oid_to_hex(oid), MAX_HEADER_LEN);
-               break;
+               ret = error(_("header for %s too long, exceeds %d bytes"),
+                           oid_to_hex(oid), MAX_HEADER_LEN);
+               goto corrupt;
        }
 
-       if (status && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
+       ret = 0;
+
+corrupt:
+       if (ret && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
                die(_("loose object %s (stored in %s) is corrupt"),
                    oid_to_hex(oid), path);
 
-cleanup:
-       git_inflate_end(&stream);
-       munmap(map, mapsize);
-       if (oi->sizep == &size_scratch)
-               oi->sizep = NULL;
-       if (oi->typep == &type_scratch)
-               oi->typep = NULL;
-       oi->whence = OI_LOOSE;
-       return status;
+out:
+       if (stream_to_end)
+               git_inflate_end(stream_to_end);
+       if (map)
+               munmap(map, mapsize);
+       if (oi) {
+               if (oi->sizep == &size_scratch)
+                       oi->sizep = NULL;
+               if (oi->typep == &type_scratch)
+                       oi->typep = NULL;
+               if (oi->delta_base_oid)
+                       oidclr(oi->delta_base_oid, source->odb->repo->hash_algo);
+               if (!ret)
+                       oi->whence = OI_LOOSE;
+       }
+
+       return ret;
 }
 
 static void hash_object_body(const struct git_hash_algo *algo, struct git_hash_ctx *c,