Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> I left some minor comments, another thing I'd just like to note, but it
> largely pre-dates this series, so we can leave it for now is that I
> found our mismatching reporting of the *.bitmap and/or *.pack kind of odd.
Ok.
> I.e. this bit (a diff on top):
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 7e69093d5a8..8a8be5f6cad 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -381,23 +381,25 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
>
> bitmap_name = pack_bitmap_filename(packfile);
> fd = git_open(bitmap_name);
> + free(bitmap_name);
>
> if (fd < 0) {
> if (errno != ENOENT)
> - warning("'%s' cannot open '%s'", strerror(errno), bitmap_name);
> - free(bitmap_name);
> + warning_errno("cannot open bitmap for pack '%s'",
> + packfile->pack_name);
> return -1;
> }
> - free(bitmap_name);
>
> if (fstat(fd, &st)) {
> close(fd);
> - return error_errno(_("cannot fstat bitmap file"));
> + return error_errno(_("cannot fstat() bitmap file for pack '%s'"),
> + packfile->pack_name);
> }
>
> if (bitmap_git->pack || bitmap_git->midx) {
> /* ignore extra bitmap file; we can only handle one */
> warning(_("ignoring extra bitmap file: '%s'"), packfile->pack_name);
> + /* ^^ because here we refer to the pack file, not the bitmap file */
> close(fd);
> return -1;
> }
>
> As tha shows we sometimes talk about the path for the pack when
> something is wrong with the bitmap, but other times the *.bitmap
> associated with the pack.
I think the reason is, the single-pack-bitmap filename based on the pack
filename as "pack-$hash.bitmap" format and the multi-pack-bitmap filename based
on the multi-pack-index as "multi-pack-index-$hash.bitmap" format. So, if want
to open single-pack-bitmap, the corresponding packfile is needed. if want to
open multi-pack-bitmap, the multi-pack-index file is needed, then initialize
fields in "bitmap_index" like mmap, etc.
That'"s probably where these two methods and inputs come from and make the
"path" different:
static int open_pack_bitmap_1(struct bitmap_index *bitmap_git,
struct packed_git *packfile)
static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
struct multi_pack_index *midx)
By the way, I think maybe the "multi-pack-index" filename maybe could change
to "multi-pack-index-$hash.idx", it could map the current multi-pack-bitmap
filename "multi-pack-index-$sha.bitmap", and ".idx" could be consistent with
single-pack-index file. Maybe it's better I think, but it also should consider
the backend compatibility.
> Also a pre-existing issue: Why are we getting as far as opening a
> *.bitmap file, if we then find that we have "bitmap_git->pack ||
> bitmap_git->midx" already, and then ignore it? Maybe I've missed
> something...
>
> All of the same oddness applies to open_midx_bitmap_1, except with "path
> to the midx" in place of "*.pack" above.
I'm not quite sure but in the code I think it's purposed to get the "fd" first
by "open..()", then get the bitmap file size by "fstat()", finally "mmap" the
bitmap file for some struct data initialization work.
Thanks.
