Re: [PATCH v6 0/7] trace2: dump scope when print "interesting" config

From: Teng Long <dyroneteng@gmail.com>
Date: Jul 18, 2022, 08:36 UTC
Message-ID: <20220718083600.11803-1-dyroneteng@gmail.com>
In-Reply-To: <220711.867d4j7kuj.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.

Show 40 quoted lines

> 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.

Show 7 quoted lines

> 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.