[v3,1/2] pack-bitmap.c: avoid exposing absolute paths

Commit Message

Teng Long Nov. 4, 2022, 3:17 a.m. UTC

From: Teng Long <[e-mail]>

In "open_midx_bitmap_1()" and "open_pack_bitmap_1()", when we find that
there are multiple bitmaps, we will only open the first one and then
leave warnings about the remaining pack information, the information
will contain the absolute path of the repository, for example in a
alternates usage scenario. So let's hide this kind of potentially
sensitive information in this commit.

Found-by: XingXin <[e-mail]>
Signed-off-by: Teng Long <[e-mail]>
---
 pack-bitmap.c           | 12 ++++++++----
 t/t5310-pack-bitmaps.sh |  5 +++--
 2 files changed, 11 insertions(+), 6 deletions(-)

Comments

Taylor Blau Nov. 4, 2022, 10:11 p.m. UTC | #1

On Fri, Nov 04, 2022 at 11:17:09AM +0800, Teng Long wrote:
> @@ -354,8 +354,9 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
>  	if (bitmap_git->pack || bitmap_git->midx) {
>  		struct strbuf buf = STRBUF_INIT;
>  		get_midx_filename(&buf, midx->object_dir);
> -		/* ignore extra bitmap file; we can only handle one */
> -		warning(_("ignoring extra bitmap file: '%s'"), buf.buf);
> +		/* ignore extra midx bitmap files; we can only handle one */
> +		trace2_data_string("bitmap", the_repository,
> +				   "ignoring extra midx bitmap file", buf.buf);

This is looking good, though I think we *could* drop the comment here.
It's as redundant in the new version as it was in the old (i.e., we say
that we ignore extra bitmap files in a comment, and then issue a warning
with basically the same message).

But I don't feel strongly about it, so I think this patch is fine with
or without that comment.

> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index 6d693eef82..0b422c8a63 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -428,8 +428,9 @@ test_bitmap_cases () {
>  			test_line_count = 2 packs &&
>  			test_line_count = 2 bitmaps &&
>
> -			git rev-list --use-bitmap-index HEAD 2>err &&
> -			grep "ignoring extra bitmap file" err
> +			GIT_TRACE2_PERF=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&

Do we need to clean up an existing trace2.txt here? Looks like not, so
OK. Is there a reason we need GIT_TRACE2_PERF and not GIT_TRACE2_EVENT
like the rest of the tests in t5310?

> +			grep "opened bitmap" trace2.txt &&
> +			grep "ignoring extra bitmap" trace2.txt

These two look good.

Thanks,
Taylor

Patch

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 440407f1be..9443b7adca 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -354,8 +354,9 @@  static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 	if (bitmap_git->pack || bitmap_git->midx) {
 		struct strbuf buf = STRBUF_INIT;
 		get_midx_filename(&buf, midx->object_dir);
-		/* ignore extra bitmap file; we can only handle one */
-		warning(_("ignoring extra bitmap file: '%s'"), buf.buf);
+		/* ignore extra midx bitmap files; we can only handle one */
+		trace2_data_string("bitmap", the_repository,
+				   "ignoring extra midx bitmap file", buf.buf);
 		close(fd);
 		strbuf_release(&buf);
 		return -1;
@@ -432,8 +433,9 @@  static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 	}
 
 	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);
+		/* ignore extra bitmap files; we can only handle one */
+		trace2_data_string("bitmap", the_repository,
+				   "ignoring extra bitmap file", packfile->pack_name);
 		close(fd);
 		return -1;
 	}
@@ -458,6 +460,8 @@  static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 		return -1;
 	}
 
+	trace2_data_string("bitmap", the_repository, "opened bitmap file",
+			   packfile->pack_name);
 	return 0;
 }
 
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 6d693eef82..0b422c8a63 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -428,8 +428,9 @@  test_bitmap_cases () {
 			test_line_count = 2 packs &&
 			test_line_count = 2 bitmaps &&
 
-			git rev-list --use-bitmap-index HEAD 2>err &&
-			grep "ignoring extra bitmap file" err
+			GIT_TRACE2_PERF=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&
+			grep "opened bitmap" trace2.txt &&
+			grep "ignoring extra bitmap" trace2.txt
 		)
 	'
 }