| Message ID | 1a169d7b5ea4b6aec030a48f718dc2c4e922a9f4.1650547400.git.dyroneteng@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | trace2 output for bitmap decision path | expand |
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@gmail.com> > --- > 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.
On Thu, 21 Apr 2022 17:41:36 +0200, Ævar Arnfjörð Bjarmason wrote: > 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. Yes, I agree with you. I also think I have a willingness to make another patchset to solve the _() missing problems recently. > 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. Make sense. Will adjust the order in next path. > 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(). Yeah, I forgot about the "error()" already load the trace2 functions in. Will remove the redundant trace2_data_string() where it's obviously will return error(). > 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. Make sense. And I think it's a good case in "open_midx_bitmap_1()" to add related trace2_data_string() because there only a general error info in "cleanup:". Thanks.
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"); } if (bitmap_git->pack || bitmap_git->midx) { @@ -374,7 +374,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) @@ -399,7 +399,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git trace2_data_string("bitmap", the_repository, "stat bitmap file", "failed"); close(fd); - return -1; + return error("cannot stat bitmap file"); } if (bitmap_git->pack || bitmap_git->midx) { @@ -413,7 +413,7 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git if (!is_pack_valid(packfile)) { trace2_data_string("bitmap", the_repository, "packfile", "invalid"); close(fd); - return -1; + return error("packfile is invalid"); } bitmap_git->pack = packfile; @@ -430,7 +430,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(-)