[PATCH v5 1/7] object-file: always set OI_LOOSE when reading object info
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Jan 12, 2026, 09:00 UTC
- Message-ID
- <20260112-b4-pks-odb-read-object-info-improvements-v5-1-9a6124e95bf2@pks.im>
- In-Reply-To
- <20260112-b4-pks-odb-read-object-info-improvements-v5-0-9a6124e95bf2@pks.im>
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> --- object-file.c | 115 ++++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 71 insertions(+), 44 deletions(-)
diff --git a/object-file.c b/object-file.c index 6280e42f34..e7e4c3348f 100644 --- 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,
-- 2.52.0.590.g1f87b77810.dirty