Re: [PATCH v5 3/4] object-file: emit corruption errors when detected
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 13, 2022, 01:51 UTC
- Message-ID
- <xmqqzgbsoyte.fsf@gitster.g>
- In-Reply-To
- <a229ea0b1122f55e91f98475cd7e508f4dd8501a.1670885252.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
Show 11 quoted lines
> diff --git a/object-file.c b/object-file.c
> index 429e3a746d..e0cef8b906 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1422,7 +1422,9 @@ static int loose_object_info(struct repository *r,
> struct object_info *oi, int flags)
> {
> int status = 0;
> + int fd;
> unsigned long mapsize;
> + const char *path = NULL;It may be OK to leave this uninitialized, as long as the contract of open_loose_object() is that a successful opening will report the path to the loose object file that was opened. Because ...
Show 10 quoted lines
> @@ -1454,7 +1455,13 @@ static int loose_object_info(struct repository *r,
> return 0;
> }
>
> - map = map_loose_object(r, oid, &mapsize);
> + fd = open_loose_object(r, oid, &path);
> + if (fd < 0) {
> + if (errno != ENOENT)
> + error_errno(_("unable to open loose object %s"), oid_to_hex(oid));
> + return -1;... we no longer refer to "path" which may not be populated here, and ...
> + }
... at this point, we know we successfully opened, and populated path.
Show 9 quoted lines
> + map = map_fd(fd, path, &mapsize); > if (!map) > return -1; > > @@ -1492,6 +1499,10 @@ static int loose_object_info(struct repository *r, > break; > } > > + if (status && path && (flags & OBJECT_INFO_DIE_IF_CORRUPT))
Also, here, "path" should be valid, as we have successfully opened the loose object file (otherwise we would have hit the early return that reports only the oid_to_hex(oid)).
> + die(_("loose object %s (stored in %s) is corrupt"),
> + oid_to_hex(oid), path);If we didn't have path and did not die, we'd end up ignoring DIE_IF_CORRUPT request and force the caller to handle non-zero status. But I do not think that should happen, because path would be valid here. No?