| Message ID | 72da3b584490467c2492578a8125cbcfe05aad9a.1655018322.git.dyroneteng@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | trace2 output for bitmap decision path | expand |
On Sun, Jun 12, 2022 at 03:44:19PM +0800, Teng Long wrote: > @@ -323,7 +323,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git, > > if (fstat(fd, &st)) { > close(fd); > - return -1; > + return error(_("cannot stat bitmap file")); Since we are handling an error from fstat here, the errno variable contains useful information that we should include in the error via error_errno(). > @@ -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"); The other error strings are marked for translation, but this one is not. Was that intentional, or just a typo / oversight? > static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git *packfile) > @@ -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(_("cannot stat bitmap file")); Same note here about using error_errno() instead of just error(). Thanks, Taylor
On Mon, 13 Jun 2022 21:15:51 -0400, Taylor Blau wrote: > Since we are handling an error from fstat here, the errno variable > contains useful information that we should include in the error via > error_errno(). Appreciate for reminding this. Will fix. > The other error strings are marked for translation, but this one is not. > Was that intentional, or just a typo / oversight? It's an oversight. Will fix. > Same note here about using error_errno() instead of just error(). As same as the first one, will fix. Thanks.
diff --git a/pack-bitmap.c b/pack-bitmap.c index af0f41833e..5654eb7b8d 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -323,7 +323,7 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git, if (fstat(fd, &st)) { close(fd); - return -1; + return error(_("cannot stat bitmap file")); } if (bitmap_git->pack || bitmap_git->midx) { @@ -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"); } static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git *packfile) @@ -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(_("cannot stat bitmap file")); } if (bitmap_git->pack || bitmap_git->midx) { @@ -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")); } bitmap_git->pack = packfile; @@ -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")); } return 0;
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@gmail.com> --- pack-bitmap.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)