Re: [PATCH v5 3/4] object-file: emit corruption errors when detected

[Junio C Hamano <gitster@pobox.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

> 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 ...

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

> +	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?