On Thu, Apr 21 2022, Teng Long wrote:
> In "open_pack_bitmap_1()" and "open_midx_bitmap_1()", it's better to
> return error() instead of "-1" when some unexpected error occurs like
> "stat bitmap file failed", "bitmap header is invalid" or "checksum
> mismatch", etc.
>
> There are places where we do not replace, such as when the bitmap
> does not exist (no bitmap in repository is allowed) or when another
> bitmap has already been opened (in which case it should be a warning
> rather than an error).
>
> Signed-off-by: Teng Long <dyroneteng@xxxxxxxxx>
> ---
> pack-bitmap.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index a1d06c4252..e0dcd06db3 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -328,7 +328,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
> trace2_data_string("midx", the_repository, "stat bitmap file",
> "failed");
> close(fd);
> - return -1;
> + return error("cannot stat bitmap file");
> }
First, I wondered if we were missing _(), but looking at other string in
the file they're not using that already, looks like these all should,
but we can fix this all up some other time in some i18n commit. It's
fine to keep this for now.
But more importantly: I think this should be your 4/5. I.e. just make
these an error() and you won't need to add e.g. this
trace2_data_string() for a failed stat.
You will be inside your trace2 region, so any failure to stat etc. will
be obvious from the structure of the data and the "error" event, no
reason to have an additional trace2_data_string().
Aside from that & as a general matter: Unless you have some use-case for
trace2 data in this detail I'd think that it would be better just to
skip logging it (except if we get it for free, such as with error()).
I.e. is this particular callsite really different from other places we
fail to stat() or open() something?
It's all a moot point with the region + error, but just something to
keep in mind.
