Re: [PATCH v6 5/7] pack-bitmap.c: using error() instead of silently returning -1

[Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>]

On Mon, Jul 11 2022, Teng Long wrote:

Good to report errno now, however...

> Signed-off-by: Teng Long <dyroneteng@xxxxxxxxx>
> ---
>  pack-bitmap.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 319eb721d8..fbe3f58aff 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -327,7 +327,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>  
>  	if (fstat(fd, &st)) {
>  		close(fd);
> -		return -1;
> +		return error_errno(_("cannot fstat bitmap file"));

This is a logic error, as fstat() failed, but you're reproting errno()
after our call to close(), at which point it's anyone's guess what
"errno" is. It's only meaningful immediately after a system call.

So either:

    error_errno(....);
    close(fd);
    return -1;

Or even better:

    error_errno(...)
    if (close(fd))
        warning_errno("cannot close() bitmap file");
    return -1;

Although that last one may be too pedantic.

>  	}
>  
>  	if (bitmap_git->pack || bitmap_git->midx) {
> @@ -350,8 +350,10 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>  	if (load_bitmap_header(bitmap_git) < 0)
>  		goto cleanup;
>  
> -	if (!hasheq(get_midx_checksum(bitmap_git->midx), bitmap_git->checksum))
> +	if (!hasheq(get_midx_checksum(bitmap_git->midx), bitmap_git->checksum)) {
> +		error(_("checksum doesn't match in MIDX and bitmap"));
>  		goto cleanup;
> +	}
>  
>  	if (load_midx_revindex(bitmap_git->midx) < 0) {
>  		warning(_("multi-pack bitmap is missing required reverse index"));
> @@ -390,7 +392,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
>  
>  	if (fstat(fd, &st)) {
>  		close(fd);
> -		return -1;
> +		return error_errno(_("cannot fstat bitmap file"));

Same issue here.