Teng Long <dyroneteng@xxxxxxxxx> 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;