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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Jul 11 2022, Teng Long wrote:

> Since v5:
>
> 1. [1/7] New commit, pre-clean the issues of the strings.
> 2. [2/7] Optimize commit subject (the word "retrieve").
> 3. [4/7] New commit, do not ignore ENOENT silently when fail to open file. 
> 4. [5/7] Replace "stat" to "fstat" in output string and let "cleanup" to
>    return -1 instead of an unaccurate error tip as "cannot open".
> 5. [7/7] dump corresponding scope-name when "interesting" config is print to
>    trace2 log.

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.

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.

It would be a tad simpler to always talk about the *.pack, as we can
free() the bitamp filename right away, but arguably we shoudl do it the
other way around. After all we have issues with the bitmap file, let's
mention it (or mention a filename at all).

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.