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

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

Teng Long <dyroneteng@gmail.com> writes:

>  	if (fstat(fd, &st)) {
>  		close(fd);
> -		return -1;
> +		return error_errno(_("cannot stat bitmap file"));

"stat" -> "fstat" perhaps?

Not a new problem, but before this hunk, we have

	fd = git_open(...);
	if (fd < 0)
		return -1;

where it probably is legitimate to ignore ENOENT silently.  In
practice, it may be OK to treat a file that exists but cannot be
opened for whatever reason, but I do not think it would hurt to
report such a rare anomaly with a warning, e.g.

	if (fd < 0) {
		if (errno != ENOENT)
			warning("'%s' cannot open '%s'",
				strerror(errno), bitmap_name);
		free(bitmap_name);
		return -1;
	}

or something like that perhaps.

> @@ -361,7 +361,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>  	bitmap_git->map_pos = 0;
>  	bitmap_git->map = NULL;
>  	bitmap_git->midx = NULL;
> -	return -1;
> +	return error(_("cannot open midx bitmap file"));

This is not exactly "cannot open".  We opened the file, and found
there was something we do not like in it.  If we failed to find midx
lacks revindex chunk, we already would have given a warning, and the
error is not just misleading but is redundant.  load_bitmap_header()
also gives an error() on its own.

I think this patch aims for a good end result, but needs more work.
At least, checksum mismatch that goes to cleanup also needs to issue
its own error() and then we can remove this "cannot open" at the end.

> @@ -382,7 +382,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 stat bitmap file"));

"stat" -> "fstat"

> @@ -394,7 +394,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
>  
>  	if (!is_pack_valid(packfile)) {
>  		close(fd);
> -		return -1;
> +		return error(_("packfile is invalid"));

Same "sometimes redundant" comment applies here, but not due to this
part of the code but due to the helpers called from is_pack_valid().

Namely, packfile.c::open_packed_git_1() is mostly chatty, but is
silent upon "unable to open" and "unable to fstat" (which I think is
safe to make chatty as well---we do not want to special case ENOENT
in open_packed_git(), so "cannot open because it is not there" is an
error).

And then, this "invalid" will become redundant and fuzzier message
than what is_pack_valid() would have already given, so you can leave
it to silently return -1 here.

> @@ -409,7 +409,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
>  		bitmap_git->map_size = 0;
>  		bitmap_git->map_pos = 0;
>  		bitmap_git->pack = NULL;
> -		return -1;
> +		return error(_("bitmap header is invalid"));

Having shown how to analize these kind of things twice in the above,
I would not bother checking myself, but you should look at the
existing error messages from load_bitmap_header() and see if this
extra message should really be here.  I suspect not.

>  	}
>  
>  	return 0;