[PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths

[PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

This patch tries to avoid the problem of leaking sensitive information that
could output the absolute path of the repository when try to open multiple
bitmaps. For example, in "alternates" scenario, where the repository
"alternate_repo" serves as alternate object stores for repository
"want_to_borrow" , and each of both has it's own bitmap file, then we run
`git rev-list --use-bitmap-index HEAD`, the output might be:

  $ cd want_to_borrow.git
  $ git rev-list --test-bitmap HEAD
  warning: ignoring extra bitmap file: /Users/tenglong.tl/Downloads/alternate_repo.git/.git/objects/pack/pack-bff67e2a7a154e6933afe61b3681726cf9bd8e8b.pack
  Bitmap v1 test (1 entries loaded)
  Found bitmap for 30f146d41a7a86930fae3f4a5e1f1c1f2bfacbba. 64 bits / 11030517 checksum
  Verifying bitmap entries: 100% (3/3), done.
  OK!

After apply this patch:

  $ git rev-list --test-bitmap HEAD
  warning: ignoring extra bitmap files
  Bitmap v1 test (1 entries loaded)
  Found bitmap for 30f146d41a7a86930fae3f4a5e1f1c1f2bfacbba. 64 bits / 11030517 checksum
  Verifying bitmap entries: 100% (3/3), done.
  OK!

Thanks.

Teng Long (1):
  pack-bitmap.c: avoid exposing absolute paths

 pack-bitmap.c                 | 8 ++++----
 t/t5310-pack-bitmaps.sh       | 2 +-
 t/t5326-multi-pack-bitmaps.sh | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.37.2.1.g1591e7ee52e

[PATCH 1/1] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

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 <moweng.xx@antgroup.com>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c                 | 8 ++++----
 t/t5310-pack-bitmaps.sh       | 2 +-
 t/t5326-multi-pack-bitmaps.sh | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 36134222d7a..5103d91d18a 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -331,8 +331,8 @@ 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 */
+		warning("ignoring extra midx bitmap files");
 		close(fd);
 		strbuf_release(&buf);
 		return -1;
@@ -402,8 +402,8 @@ 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 */
+		warning("ignoring extra bitmap files");
 		close(fd);
 		return -1;
 	}
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index f775fc1ce69..7cd6d79a022 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -421,7 +421,7 @@ test_expect_success 'complains about multiple pack bitmaps' '
 		test_line_count = 2 bitmaps &&
 
 		git rev-list --use-bitmap-index HEAD 2>err &&
-		grep "ignoring extra bitmap file" err
+		grep "ignoring extra bitmap files" err
 	)
 '
 
diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh
index 4fe57414c13..1786f28376a 100755
--- a/t/t5326-multi-pack-bitmaps.sh
+++ b/t/t5326-multi-pack-bitmaps.sh
@@ -303,7 +303,7 @@ test_expect_success 'graceful fallback when missing reverse index' '
 
 		GIT_TEST_MIDX_READ_RIDX=0 \
 			git rev-list --use-bitmap-index HEAD 2>err &&
-		! grep "ignoring extra bitmap file" err
+		grep "multi-pack bitmap is missing required reverse index" err
 	)
 '
 
-- 
2.37.2.1.g1591e7ee52e

Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths

[Junio C Hamano]

Teng Long <dyroneteng@gmail.com> writes:

> This patch tries to avoid the problem of leaking sensitive information that
> could output the absolute path of the repository when try to open multiple
> bitmaps. For example, in "alternates" scenario, where the repository
> "alternate_repo" serves as alternate object stores for repository
> "want_to_borrow" , and each of both has it's own bitmap file, then we run
> `git rev-list --use-bitmap-index HEAD`, the output might be:

>   $ cd want_to_borrow.git
>   $ git rev-list --test-bitmap HEAD
>   warning: ignoring extra bitmap file: /Users/tenglong.tl/Downloads/alternate_repo.git/.git/objects/pack/pack-bff67e2a7a154e6933afe61b3681726cf9bd8e8b.pack
>   Bitmap v1 test (1 entries loaded)
>   Found bitmap for 30f146d41a7a86930fae3f4a5e1f1c1f2bfacbba. 64 bits / 11030517 checksum
>   Verifying bitmap entries: 100% (3/3), done.
>   OK!
>
> After apply this patch:
>
>   $ git rev-list --test-bitmap HEAD
>   warning: ignoring extra bitmap files
>   Bitmap v1 test (1 entries loaded)
>   Found bitmap for 30f146d41a7a86930fae3f4a5e1f1c1f2bfacbba. 64 bits / 11030517 checksum
>   Verifying bitmap entries: 100% (3/3), done.
>   OK!

I am not sure why it is an improvement though.

Doesn't it make it harder to diagnose where the extra bitmap file
you do not need exists (so that you can check if you can/want
to/need to remove it)?

If the "ignoring extra" is a totally expected situation (e.g. it is
not suprising if we always ignore the bitmapfile in the alternate
when we have our own), perhaps we should squelch the warning in such
expected cases altogether (while warning other cases where we see
more bitmap files than we expect to see, which may be an anomaly
worth warning about), and that may be an improvement worth spending
development cycles on, but I am not sure about this one.

Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

Junio C Hamano <gitster@pobox.com> writes:

> Doesn't it make it harder to diagnose where the extra bitmap file
> you do not need exists (so that you can check if you can/want
> to/need to remove it)?

Yes, if we want that warnings for diagnosing we shouldn't remove the related
path but I think it should build on the security concerns. I don't mean that
to warning the packfile name in a absolute path is a danger, but when serving
a git server maybe we don't want to expose the real path information on server
to client users.

> If the "ignoring extra" is a totally expected situation (e.g. it is
> not suprising if we always ignore the bitmapfile in the alternate
> when we have our own), perhaps we should squelch the warning in such
> expected cases altogether (while warning other cases where we see
> more bitmap files than we expect to see, which may be an anomaly
> worth warning about), and that may be an improvement worth spending
> development cycles on, but I am not sure about this one.

That's exactly good suggestion. In my opinion, I think to avoid the sensitive
warning and the same time we keep some information to let the users know "Oh,
there are some extra existing bitmaps we just ignored then maybe can do some
optimization works", but I think just remove the total warning here is
reasonable also, i'm good with it.

Thanks.

Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths

[Taylor Blau]

On Mon, Aug 29, 2022 at 10:48:03AM +0800, Teng Long wrote:
> > If the "ignoring extra" is a totally expected situation (e.g. it is
> > not suprising if we always ignore the bitmapfile in the alternate
> > when we have our own), perhaps we should squelch the warning in such
> > expected cases altogether (while warning other cases where we see
> > more bitmap files than we expect to see, which may be an anomaly
> > worth warning about), and that may be an improvement worth spending
> > development cycles on, but I am not sure about this one.
>
> That's exactly good suggestion. In my opinion, I think to avoid the sensitive
> warning and the same time we keep some information to let the users know "Oh,
> there are some extra existing bitmaps we just ignored then maybe can do some
> optimization works", but I think just remove the total warning here is
> reasonable also, i'm good with it.

I think that it is somewhat of a step backwards to remove it entirely,
but let me qualify that a little bit.

At GitHub, we actually *do* remove this warning entirely:

--- >8 ---

From: Jeff King <peff@peff.net>
Subject: [PATCH] pack-bitmap: drop "ignoring extra bitmap" warning

For simplicity, the bitmap code only handles a single bitmap
file at a time. If you have two, it emits a warning.

This is usually the sign of a misconfiguration, but it can
also happen racily during maintenance. We create and install
the new bitmap file and then remove the old one. As a
result, users may see something like:

  $ git fetch
  ...
  remote: warning: ignoring extra bitmap file: /path/to/pack-xyz.bitmap

This is scary for them (even though the condition is totally
harmless), and it exposes a bunch of internal paths. Let's
just silence the warning entirely. It's possible that this
may make debugging some obscure case harder, but it seems
rather unlikely.
---
 pack-bitmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index d3fde3d3b28..283da33a648 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -298,7 +298,7 @@ static int open_pack_bitmap_1(struct packed_git *packfile)
 	}

 	if (bitmap_git.pack) {
-		warning("ignoring extra bitmap file: %s", packfile->pack_name);
+		/* ignore extra bitmap file; we can only handle one */
 		close(fd);
 		return -1;
 	}
--
2.38.0.16.g393fd4c6db

--- 8< ---

...and as the patch message notes, this is done mostly to prevent
confusion when racily fetching from GitHub, or due to some
misconfiguration.

And in that instance, I think the patch from Peff is right. If there is
a legitimate bug, we'd see it elsewhere and have sufficiently powerful
tools to investigate it. But the warning is useless and confusing to
users who don't have access to such tools.

For the general case of what ships in git.git, I *do* find this warning
useful. It's helpful when hacking on pack-bitmap.c to know if you've
messed up, and it's useful to see the filename of the affected
bitmap(s).

I think we could reasonably change the warning to

    warning(_("ignoring extra bitmap file: %s"),
            basename(packfile->pack_name));

since the rest of the path is obvious based on which repository you're
working in. So that would be a reasonable change to shorten up the
output a little bit.

You could also imagine adding a configuration knob here to control
whether or not the warning is shown, but I find that to be kind of
gross.

So I think that the warning is--in general--too useful to consider
removing it entirely, and that at most we should consider just emitting
the basename of the pack, but nothing else.

Thanks,
Taylor

Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths

[Ævar Arnfjörð Bjarmason]

On Wed, Oct 26 2022, Taylor Blau wrote:

> On Mon, Aug 29, 2022 at 10:48:03AM +0800, Teng Long wrote:
>> > If the "ignoring extra" is a totally expected situation (e.g. it is
>> > not suprising if we always ignore the bitmapfile in the alternate
>> > when we have our own), perhaps we should squelch the warning in such
>> > expected cases altogether (while warning other cases where we see
>> > more bitmap files than we expect to see, which may be an anomaly
>> > worth warning about), and that may be an improvement worth spending
>> > development cycles on, but I am not sure about this one.
>>
>> That's exactly good suggestion. In my opinion, I think to avoid the sensitive
>> warning and the same time we keep some information to let the users know "Oh,
>> there are some extra existing bitmaps we just ignored then maybe can do some
>> optimization works", but I think just remove the total warning here is
>> reasonable also, i'm good with it.
>
> I think that it is somewhat of a step backwards to remove it entirely,
> but let me qualify that a little bit.
>
> At GitHub, we actually *do* remove this warning entirely:

You at GitHub also added it entirely :) => fff42755efc (pack-bitmap: add
support for bitmap indexes, 2013-12-21).

Anyway, I'm fine with removing it. From skimming that commit it was
probably added for no particularly strong reason. But I found the
omission of "it was added in xyz commit" to be sometihng that could be
added to the commit message in this case, and....

> You could also imagine adding a configuration knob here to control
> whether or not the warning is shown, but I find that to be kind of
> gross.

FWIW I don't find that to be particularly gross. I think it's fine to
just delete it.

But isn't this a general sign that we should perhaps have different
output when "pack-objects" and the like is run "locally", v.s. when
we're running via some server process, and end up spewing a message out
that the user can't do anything about?

Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

"Ævar Arnfjörð Bjarmason" <avarab@gmail.com> writes:

> But isn't this a general sign that we should perhaps have different
> output when "pack-objects" and the like is run "locally", v.s. when
> we're running via some server process, and end up spewing a message out
> that the user can't do anything about?


I think that maybe we could just use "trace2" to print the detailed
informations we (git developers) care about for some diagnosing work 
and do not output the warnings to the client users.

Maybe it's a solution that satisfies both scenarios.

Thanks.

Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths

[Jeff King]

On Wed, Oct 26, 2022 at 05:42:51PM -0400, Taylor Blau wrote:

> At GitHub, we actually *do* remove this warning entirely:
> 
> --- >8 ---
> 
> From: Jeff King <peff@peff.net>
> Subject: [PATCH] pack-bitmap: drop "ignoring extra bitmap" warning

A blast from the past. You didn't include the Date field, but this was
from ~2017.

> For the general case of what ships in git.git, I *do* find this warning
> useful. It's helpful when hacking on pack-bitmap.c to know if you've
> messed up, and it's useful to see the filename of the affected
> bitmap(s).

It feels like you might be a special case, though. Most people are not
hacking on the bitmap code. :)

I wonder if you'd be better served by having good tracing for the bitmap
code. Then it could tell you lots of things, like which bitmap it
actually opened, which ones it ignored, etc. Of course you'd have to
remember to enable that tracing when you're working in the area.

The other thing that has always bugged me about this warning (and would
be true of tracing code, too) is that it inherently requires looking at
all of the packfiles. I.e., open_pack_bitmap() doesn't return when it
finds something; it keeps going just in case it finds another bitmap so
it can say "hey, there's an extra one!".

This complicates the code a little bit. But it's also inefficient. If we
have N packs, we'll do N open() calls, most of which will fail. Weirder,
though, is that we open the pack .idx first. So we're literally opening,
mapping, and checking the idx for every pack for no reason!

Now this might not be as bad as it seems:

  - in the long run, we might open those idx files anyway, if we have to
    access those packs. So it's really just overriding the lazy-open
    behavior.

  - in the worst case, the one bitmap file is at the end of the list, so
    you hit all N anyway. And this is actually pretty common, since we
    sort in reverse-chronological order, and the bitmap is usually on
    the oldest and biggest pack.

  - in general, having a lot of packs has bad performance for other
    reasons. So you'd generally want to keep N small-ish in the first
    place.

So it may not be worth worrying about. It does seem like it would be
easy to reorder open_pack_bitmap_1() to look for a bitmap file first and
only open the idx if it finds something.

It's possible we have a weird race-dependency on opening those idx files
early (certainly there's some subtlety with pack-objects keeping the
.pack files themselves open which prevents problems with a simultaneous
repack). But I think it would be OK; we don't commit to using a
particular .idx file in the general case, and one that goes away would
just cause us to reprepare_packed_git() and continue. For a particular
.bitmap, we do commit to having the matching .idx file, but obviously
we'd want to open and check that one before considering the bitmap to be
successfully opened.

> I think we could reasonably change the warning to
> 
>     warning(_("ignoring extra bitmap file: %s"),
>             basename(packfile->pack_name));
> 
> since the rest of the path is obvious based on which repository you're
> working in. So that would be a reasonable change to shorten up the
> output a little bit.

Yeah. If we are going to keep the warning, this makes much better sense.
Possibly we could even just skip_prefix() on the object directory,
though I think in practice it amounts to the same thing.

-Peff

Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths

[Taylor Blau]

On Thu, Oct 27, 2022 at 04:45:46PM -0400, Jeff King wrote:
> On Wed, Oct 26, 2022 at 05:42:51PM -0400, Taylor Blau wrote:
>
> > At GitHub, we actually *do* remove this warning entirely:
> >
> > --- >8 ---
> >
> > From: Jeff King <peff@peff.net>
> > Subject: [PATCH] pack-bitmap: drop "ignoring extra bitmap" warning
>
> A blast from the past. You didn't include the Date field, but this was
> from ~2017.

I'll blame you for that one, since this output comes from your
'git print-patch's script, which only prints out the From: and Subject:
headers. ;-)

> > For the general case of what ships in git.git, I *do* find this warning
> > useful. It's helpful when hacking on pack-bitmap.c to know if you've
> > messed up, and it's useful to see the filename of the affected
> > bitmap(s).
>
> It feels like you might be a special case, though. Most people are not
> hacking on the bitmap code. :)
>
> I wonder if you'd be better served by having good tracing for the bitmap
> code. Then it could tell you lots of things, like which bitmap it
> actually opened, which ones it ignored, etc. Of course you'd have to
> remember to enable that tracing when you're working in the area.

Yeah, I wouldn't be sad to see this get moved to the trace2 mechanism.
In fact, I almost prefer that approach, since server operators and
developers can still debug the bitmap machinery, but end-users of forges
like GitHub won't see the output.

Maybe that is the best approach forward. Teng Long: does that sound OK
to you?

> So it may not be worth worrying about. It does seem like it would be
> easy to reorder open_pack_bitmap_1() to look for a bitmap file first and
> only open the idx if it finds something.

Yeah, I had always thought that it was odd that we had to go through all
of the repository's possible bitmaps and try to open them one by one. It
might be interesting to have a file that lists the path of _the_
bitmap, which we consult first and then try and open that pack or MIDX
bitmap. But I dunno.

> > I think we could reasonably change the warning to
> >
> >     warning(_("ignoring extra bitmap file: %s"),
> >             basename(packfile->pack_name));
> >
> > since the rest of the path is obvious based on which repository you're
> > working in. So that would be a reasonable change to shorten up the
> > output a little bit.
>
> Yeah. If we are going to keep the warning, this makes much better sense.
> Possibly we could even just skip_prefix() on the object directory,
> though I think in practice it amounts to the same thing.

I agree, they end up the same in either approach you take.

Thanks,
Taylor

Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths Taylor Blau <me@ttaylorr.com> writes:

[Teng Long]

> Maybe that is the best approach forward. Teng Long: does that sound OK                                                                                                                                                                                                     > to you?                                                                                                                                                                                                                                                                     
Sorry, there are some problems on my Mac (I can't run coverage but it's ok now),                                                                                                                                                                                             I will read the context and reply again a little bit later.                                                                                                                                                                                                                  Thanks for Taylor Blau, Ævar Arnfjörð Bjarmason and Jeff King to supply the                                                                                                                                                                                                  discussions on this topic.                                                                                                                                                                                                                                                   

Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

Jeff King <peff@peff.net> writes：

> Now this might not be as bad as it seems:
>
>   - in the long run, we might open those idx files anyway, if we have to
>     access those packs. So it's really just overriding the lazy-open
>     behavior.

Sorry, can you explain it a bit more. When we might open idxes anyway? Do you
mean if the pack idx files will be opened sooner or later if a repo serves
git-upload-pack many times in the long run. So, the system-wide table or the
mmap space will not be wasted so much in practice.

>   - in the worst case, the one bitmap file is at the end of the list, so
>     you hit all N anyway. And this is actually pretty common, since we
>     sort in reverse-chronological order, and the bitmap is usually on
>     the oldest and biggest pack.

Yes, in "sort_pack()" I think.

> So it may not be worth worrying about. It does seem like it would be
> easy to reorder open_pack_bitmap_1() to look for a bitmap file first and
> only open the idx if it finds something.

I think it may be worthy if we have lots of packs and the bitmap is refer to
an older one, but I didn't make the test. At least, the scenario is common, I
agree with that, so maybe we could shuffle the sort order in "open_pack_bitmap()".

Thanks.

Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths

[Jeff King]

On Wed, Nov 02, 2022 at 01:37:48PM +0800, Teng Long wrote:

> Jeff King <peff@peff.net> writes：
> 
> > Now this might not be as bad as it seems:
> >
> >   - in the long run, we might open those idx files anyway, if we have to
> >     access those packs. So it's really just overriding the lazy-open
> >     behavior.
> 
> Sorry, can you explain it a bit more. When we might open idxes anyway? Do you
> mean if the pack idx files will be opened sooner or later if a repo serves
> git-upload-pack many times in the long run. So, the system-wide table or the
> mmap space will not be wasted so much in practice.

I mean that later in the process, if we need to find an object we may
open the .idx file to look for it. So by opening them all up front, we
_might_ just be doing work that would get done later.

But it's not guaranteed. Imagine you have 10,000 small packs, and one
big bitmapped pack. If you can serve the request from just the big pack,
then you'd never need to open those other .idx files at all. However,
the current code will open them anyway.

I care less about mmap space, and more that it's work (syscalls, and
examining the contents of the idx) to open each one. It's probably not
even measurable unless you have a ton of packs, though.

> > So it may not be worth worrying about. It does seem like it would be
> > easy to reorder open_pack_bitmap_1() to look for a bitmap file first and
> > only open the idx if it finds something.
> 
> I think it may be worthy if we have lots of packs and the bitmap is refer to
> an older one, but I didn't make the test. At least, the scenario is common, I
> agree with that, so maybe we could shuffle the sort order in "open_pack_bitmap()".

I don't mean the order in which we look at packs. I mean the order of
operations in open_pack_bitmap_1(), something like:

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 440407f1be..1df2f6c8b6 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -411,9 +411,6 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 	struct stat st;
 	char *bitmap_name;
 
-	if (open_pack_index(packfile))
-		return -1;
-
 	bitmap_name = pack_bitmap_filename(packfile);
 	fd = git_open(bitmap_name);
 
@@ -438,6 +435,10 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 		return -1;
 	}
 
+	/* now we know we have a plausible bitmap; make sure the idx is OK, too */
+	if (open_pack_index(packfile))
+		return -1;
+
 	if (!is_pack_valid(packfile)) {
 		close(fd);
 		return -1;

But we can further observe that the first thing is_pack_valid() will do
is open the idx file. :) So we can really just drop this line entirely,
I'd think.

BTW, another oddity I noticed in this function. We check:

   if (bitmap_git->pack || bitmap_git->midx) {
	   /* ignore extra bitmap file; we can only handle one */
	   ...
   }

but it's impossible for bitmap_git->midx to be set here. If we opened
the midx bitmap, we'll skip calling open_pack_bitmap() entirely.

-Peff

Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

Jeff King <peff@peff.net> writes:

> I mean that later in the process, if we need to find an object we may
> open the .idx file to look for it. So by opening them all up front, we
> _might_ just be doing work that would get done later.
>
> But it's not guaranteed. Imagine you have 10,000 small packs, and one
> big bitmapped pack. If you can serve the request from just the big pack,
> then you'd never need to open those other .idx files at all. However,
> the current code will open them anyway.
>
> I care less about mmap space, and more that it's work (syscalls, and
> examining the contents of the idx) to open each one. It's probably not
> even measurable unless you have a ton of packs, though.
>
> > > So it may not be worth worrying about. It does seem like it would be
> > > easy to reorder open_pack_bitmap_1() to look for a bitmap file first and
> > > only open the idx if it finds something.
> >
> > I think it may be worthy if we have lots of packs and the bitmap is refer to
> > an older one, but I didn't make the test. At least, the scenario is common, I
> > agree with that, so maybe we could shuffle the sort order in "open_pack_bitmap()".
>
> I don't mean the order in which we look at packs. I mean the order of
> operations in open_pack_bitmap_1(), something like:

Thank you for the explanation. Make sense.

I run a test under a repo with 3 packs and without bitmaps,it seems like now
will open every idx and failed at last:

➜  pack git:(master) git rev-list --test-bitmap HEAD
pack: /Users/tenglong.tl/Downloads/trace-test/.git/objects/pack/pack-c9fe9d2dc5d002d4a4b622626ffa282bcbccb7ee.pack
pack: /Users/tenglong.tl/Downloads/trace-test/.git/objects/pack/pack-08841c0c4c1fd176c354bdbd25c5a1b152ea95d0.pack
pack: /Users/tenglong.tl/Downloads/trace-test/.git/objects/pack/pack-3cea516b416961285fd8f519e12102b19bcf257e.pack
fatal: failed to load bitmap indexes

So we're now looping for packs first, then try to find the corresponded bitmap
of it. In that case, why can't we start the search from the bitmap files at
first? If this is possible, when we found the first bitmap file or an
appropriate one under some mechanism (biggest or newest maybe？I'm not deep into
it right now) then break the loop and open it.

> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 440407f1be..1df2f6c8b6 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -411,9 +411,6 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
>  	struct stat st;
>  	char *bitmap_name;
>
> -	if (open_pack_index(packfile))
> -		return -1;
> -
>  	bitmap_name = pack_bitmap_filename(packfile);
>  	fd = git_open(bitmap_name);
>
> @@ -438,6 +435,10 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
>  		return -1;
>  	}
>
> +	/* now we know we have a plausible bitmap; make sure the idx is OK, too */
> +	if (open_pack_index(packfile))
> +		return -1;
> +
>  	if (!is_pack_valid(packfile)) {
>  		close(fd);
>  		return -1;
>
> But we can further observe that the first thing is_pack_valid() will do
> is open the idx file. :) So we can really just drop this line entirely,
> I'd think.

I agree that and I think it could append to patch v3, maybe.

> BTW, another oddity I noticed in this function. We check:
>
>    if (bitmap_git->pack || bitmap_git->midx) {
> 	   /* ignore extra bitmap file; we can only handle one */
> 	   ...
>    }
>
> but it's impossible for bitmap_git->midx to be set here. If we opened
> the midx bitmap, we'll skip calling open_pack_bitmap() entirely.

Oh, I remember that and it's mentioned in another patchset at Tue, 29 Mar 2022:

https://public-inbox.org/git/20220329024949.62091-1-dyroneteng@gmail.com/

I agree with Taylor with
https://public-inbox.org/git/YkPGq0mDL4NG6D1o@nand.local/

But I'm ok if you think it should be solved.

Thank you very much for your help.

Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

Taylor Blau <me@ttaylorr.com> writes:

> --- >8 ---

What does ">8" means? 

It's like a Github internal patch between the ">8"
and "<8" marks, right?

> ...and as the patch message notes, this is done mostly to prevent
> confusion when racily fetching from GitHub, or due to some
> misconfiguration.

> And in that instance, I think the patch from Peff is right. If there is
> a legitimate bug, we'd see it elsewhere and have sufficiently powerful
> tools to investigate it. But the warning is useless and confusing to
> users who don't have access to such tools.

Yes, currently there will be only one bitmap to be process and other
ones will be skipped, so we should remove it I think because it's no
actual meanings for client uses, they do not care about what the server
skipped and hope to avoid the disturbing information.

> For the general case of what ships in git.git, I *do* find this warning
> ...

"ships in git.git" means as the git developers, right?

> useful. It's helpful when hacking on pack-bitmap.c to know if you've
> messed up, and it's useful to see the filename of the affected
> bitmap(s).

I agree, let things expose entirely, but it's a trade-off.

> You could also imagine adding a configuration knob here to control
> whether or not the warning is shown, but I find that to be kind of
> gross.
> 
> So I think that the warning is--in general--too useful to consider
> removing it entirely, and that at most we should consider just emitting
> the basename of the pack, but nothing else. 

I think it's no need for a new config knob here, I agree with you on this
point.

And keep the pack basename seems like definitly a better solution than
the current one and I have a other solution which is to replace "warn()"
to a trace2 api, so the developer of git or ships in git.git if I
understand it right ;-) could get the warning in trace2 output by:


➜  pack git:(master) GIT_TRACE2_PERF=1 git rev-list --test-bitmap HEAD                                                                                                                                                                                                    <<<
21:04:08.702523 common-main.c:49             | d0 | main                     | version      |     |           |           |              | 2.38.0
21:04:08.702791 common-main.c:50             | d0 | main                     | start        |     |  0.001922 |           |              | /usr/local/bin/git rev-list --test-bitmap HEAD
21:04:08.703553 git.c:461                    | d0 | main                     | cmd_name     |     |           |           |              | rev-list (rev-list)
21:04:08.704889 usage.c:86                   | d0 | main                     | error        |     |           |           |              | ignoring extra bitmap file: '/Users/tenglong.tl/Downloads/trace-test/.git/objects/pack/pack-3cea516b416961285fd8f519e12102b19bcf257e.pack'
warning: ignoring extra bitmap file: '/Users/tenglong.tl/Downloads/trace-test/.git/objects/pack/pack-3cea516b416961285fd8f519e12102b19bcf257e.pack'
Bitmap v1 test (2 entries loaded)
Found bitmap for '2c5959955b5e6167181d08eeb30ee4099b4a4c5b'. 64 bits / ca44d5df checksum
21:04:08.705305 progress.c:268               | d0 | main                     | region_enter | r0  |  0.004448 |           | progress     | label:Verifying bitmap entries
Verifying bitmap entries: 100% (6/6), done.
21:04:08.705814 progress.c:340               | d0 | main                     | data         | r0  |  0.004955 |  0.000507 | progress     | ..total_objects:6
21:04:08.705829 progress.c:346               | d0 | main                     | region_leave | r0  |  0.004972 |  0.000524 | progress     | label:Verifying bitmap entries
OK!
21:04:08.705883 git.c:721                    | d0 | main                     | exit         |     |  0.005026 |           |              | code:0
21:04:08.705899 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.005043 |           |              | code:0



I don't know which is better and I will go on reading the left replies.

Thanks for the hard working on this patchset, Taylor, really appreciate for that.

Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths

[Taylor Blau]

On Mon, Oct 31, 2022 at 09:13:56PM +0800, Teng Long wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > --- >8 ---
>
> What does ">8" means?
>
> It's like a Github internal patch between the ">8"
> and "<8" marks, right?

They are scissor marks. Tools like 'git am -c' will trim everything
before a scissors line when applying patches.

I used them to deliniate between the patch contents (which can be
applied) and the rest of my mail here (which can't).

> > For the general case of what ships in git.git, I *do* find this warning
> > ...
>
> "ships in git.git" means as the git developers, right?

Kind of, but not entirely. I meant "what is in Junio's tree": so, what
gets bundled by package maintainer, as well as the source that Git
developer build from.

Thanks,
Taylor

Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths

[Ævar Arnfjörð Bjarmason]

On Fri, Aug 26 2022, Teng Long wrote:

> This patch tries to avoid the problem of leaking sensitive information that
> could output the absolute path of the repository when try to open multiple
> bitmaps. For example, in "alternates" scenario, where the repository
> "alternate_repo" serves as alternate object stores for repository
> "want_to_borrow" , and each of both has it's own bitmap file, then we run
> `git rev-list --use-bitmap-index HEAD`, the output might be:
>
>   $ cd want_to_borrow.git
>   $ git rev-list --test-bitmap HEAD
>   warning: ignoring extra bitmap file: /Users/tenglong.tl/Downloads/alternate_repo.git/.git/objects/pack/pack-bff67e2a7a154e6933afe61b3681726cf9bd8e8b.pack
>   Bitmap v1 test (1 entries loaded)
>   Found bitmap for 30f146d41a7a86930fae3f4a5e1f1c1f2bfacbba. 64 bits / 11030517 checksum
>   Verifying bitmap entries: 100% (3/3), done.
>   OK!
>
> After apply this patch:
>
>   $ git rev-list --test-bitmap HEAD
>   warning: ignoring extra bitmap files
>   Bitmap v1 test (1 entries loaded)
>   Found bitmap for 30f146d41a7a86930fae3f4a5e1f1c1f2bfacbba. 64 bits / 11030517 checksum
>   Verifying bitmap entries: 100% (3/3), done.
>   OK!

I see that downthread of here there's discussion about keeping the
warning, adding tracing, etc. etc.

Maybe it's been brought up (I was skimming, sorry), but for the problem
you have isn't a narrow and acceptable solution to you to keep the
warning, but just don't print the absolute path?

I.e.:

	warning: ignoring extra bitmap file: /Users/tenglong.tl/Downloads/alternate_repo.git/.git/objects/pack/pack-bff67e2a7a154e6933afe61b3681726cf9bd8e8b.pack

To:

	warning: ignoring extra bitmap file: ../alternate_repo.git/.git/objects/pack/pack-bff67e2a7a154e6933afe61b3681726cf9bd8e8b.pack

Or would the relative path to the alternate also be sensitive?

We might also want to just remove this etc., but that's a different
question than "should we print these absolute paths?".

Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

"Ævar Arnfjörð Bjarmason" <avarab@gmail.com> writes：

> I see that downthread of here there's discussion about keeping the
> warning, adding tracing, etc. etc.
>
> Maybe it's been brought up (I was skimming, sorry), but for the problem
> you have isn't a narrow and acceptable solution to you to keep the
> warning, but just don't print the absolute path?
>
> I.e.:
>
> 	warning: ignoring extra bitmap file: /Users/tenglong.tl/Downloads/alternate_repo.git/.git/objects/pack/pack-bff67e2a7a154e6933afe61b3681726cf9bd8e8b.pack
>
> To:
>
> 	warning: ignoring extra bitmap file: ../alternate_repo.git/.git/objects/pack/pack-bff67e2a7a154e6933afe61b3681726cf9bd8e8b.pack
>
> Or would the relative path to the alternate also be sensitive?
>
> We might also want to just remove this etc., but that's a different
> question than "should we print these absolute paths?".

Oops, sorry for that mistake, and I just sent the patch v2, and the trace2
output might like:

19:33:24.360684 common-main.c:49             | d0 | main                     | version      |     |           |           |              | 2.37.2.1.g520f5131b7c.dirty
19:33:24.360963 common-main.c:50             | d0 | main                     | start        |     |  0.001820 |           |              | /usr/local/bin/git rev-list --test-bitmap HEAD
19:33:24.361487 git.c:461                    | d0 | main                     | cmd_name     |     |           |           |              | rev-list (rev-list)
19:33:24.364617 pack-bitmap.c:435            | d0 | main                     | data         | r0  |  0.005497 |  0.005497 | bitmap       | opened bitmap file:pack-c9fe9d2dc5d002d4a4b622626ffa282bcbccb7ee.pack
19:33:24.365635 pack-bitmap.c:409            | d0 | main                     | data         | r0  |  0.006518 |  0.006518 | bitmap       | ignoring extra bitmap file:pack-3cea516b416961285fd8f519e12102b19bcf257e.pack
Bitmap v1 test (2 entries loaded)
Found bitmap for 2c5959955b5e6167181d08eeb30ee4099b4a4c5b. 64 bits / ca44d5df checksum
19:33:24.365970 progress.c:268               | d0 | main                     | region_enter | r0  |  0.006853 |           | progress     | label:Verifying bitmap entries
Verifying bitmap entries: 100% (6/6), done.
19:33:24.366479 progress.c:340               | d0 | main                     | data         | r0  |  0.007362 |  0.000509 | progress     | ..total_objects:6
19:33:24.366493 progress.c:346               | d0 | main                     | region_leave | r0  |  0.007377 |  0.000524 | progress     | label:Verifying bitmap entries
OK!
19:33:24.366558 wrapper.c:621                | d0 | main                     | data         | r0  |  0.007442 |  0.007442 | fsync        | fsync/writeout-only:0
19:33:24.366572 wrapper.c:622                | d0 | main                     | data         | r0  |  0.007456 |  0.007456 | fsync        | fsync/hardware-flush:0
19:33:24.366578 git.c:720                    | d0 | main                     | exit         |     |  0.007463 |           |              | code:0
19:33:24.366592 trace2/tr2_tgt_perf.c:215    | d0 | main                     | atexit       |     |  0.007476 |           |              | code:0

In short, the "related path" will not appear in output now but the "basename"
(packname) instead. I think maybe it will help the experts like Taylor who
often hacking the bitmap code.

Thanks.

[PATCH v2 0/1] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

From: Teng Long <dyroneteng@gmail.com>

Diff since the first patch:

     * remove the "ignore extra bitmap file" warning
     * add "ignore extra bitmap file" in trace2 debugging output
     * modify case "t5310"

Thanks.

Teng Long (1):
  pack-bitmap.c: avoid exposing absolute paths

 pack-bitmap.c           | 12 ++++++++----
 t/t5310-pack-bitmaps.sh | 11 ++++++++---
 2 files changed, 16 insertions(+), 7 deletions(-)

Range-diff against v1:
1:  1591e7ee52e < -:  ----------- pack-bitmap.c: avoid exposing absolute paths
-:  ----------- > 1:  87a494e5ac0 pack-bitmap.c: avoid exposing absolute paths
-- 
2.37.2.1.g87a494e5ac0

[PATCH v2 1/1] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

From: Teng Long <dyroneteng@gmail.com>

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 <moweng.xx@antgroup.com>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c           | 12 ++++++++----
 t/t5310-pack-bitmaps.sh | 11 ++++++++---
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 36134222d7a..a8c76056637 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -331,8 +331,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", basename(buf.buf));
 		close(fd);
 		strbuf_release(&buf);
 		return -1;
@@ -402,8 +403,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", basename(packfile->pack_name));
 		close(fd);
 		return -1;
 	}
@@ -428,6 +430,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",
+			   basename(packfile->pack_name));
 	return 0;
 }
 
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index f775fc1ce69..614586b0181 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -397,7 +397,7 @@ test_expect_success 'pack.preferBitmapTips' '
 	)
 '
 
-test_expect_success 'complains about multiple pack bitmaps' '
+test_expect_success 'complains about multiple pack bitmaps when debugging by trace2' '
 	rm -fr repo &&
 	git init repo &&
 	test_when_finished "rm -fr repo" &&
@@ -420,8 +420,13 @@ test_expect_success 'complains about multiple pack bitmaps' '
 		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
+		ls -tr .git/objects/pack | grep -e "^pack-.*\.pack$" > sorted-packs &&
+		ignored_pack="$(cat sorted-packs | awk 'NR==1{print}')" &&
+		open_pack="$(cat sorted-packs | awk 'NR==2{print}')" &&
+
+		GIT_TRACE2_PERF=1 git rev-list --use-bitmap-index HEAD 2>err &&
+		grep "opened bitmap file:$opened_pack" err &&
+		grep "ignoring extra bitmap file:$ignored_pack" err
 	)
 '
 
-- 
2.37.2.1.g87a494e5ac0

Re: [PATCH v2 1/1] pack-bitmap.c: avoid exposing absolute paths

[Taylor Blau]

On Wed, Nov 02, 2022 at 08:56:05PM +0800, Teng Long wrote:
> From: Teng Long <dyroneteng@gmail.com>
>
> 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 <moweng.xx@antgroup.com>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  pack-bitmap.c           | 12 ++++++++----
>  t/t5310-pack-bitmaps.sh | 11 ++++++++---
>  2 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 36134222d7a..a8c76056637 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -331,8 +331,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);

OK... here we're getting rid of the user-visible warning, which makes
sense and is the point of this patch.

> +		/* ignore extra midx bitmap files; we can only handle one */
> +		trace2_data_string("bitmap", the_repository,
> +				   "ignoring extra midx bitmap file", basename(buf.buf));

But we replace it with a trace2_data_string() that only includes the
basename? I'd think that the point of moving this warning into
trace2-land is that we could emit the full path without worrying about
who sees it, since trace2 data is typically not plumbed through to
end-users.

IOW, I would have expected to see a patch something along the lines of:

> -		/* 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);

> @@ -402,8 +403,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", basename(packfile->pack_name));

Same note here.

> +	trace2_data_string("bitmap", the_repository, "opened bitmap file",
> +			   basename(packfile->pack_name));

If we get later bitmap-related information in the trace2 stream, we know
that we opened a bitmap. And at the moment we read a bitmap, there is
only one such bitmap in the repository.

I suppose that this is race-proof in the sense that if the bitmaps
change after reading, we can still usefully debug the trace2 stream
after the fact.

So even though my first reaction was that this was unnecessary, on
balance I do find it useful.

> -test_expect_success 'complains about multiple pack bitmaps' '
> +test_expect_success 'complains about multiple pack bitmaps when debugging by trace2' '
>  	rm -fr repo &&
>  	git init repo &&
>  	test_when_finished "rm -fr repo" &&
> @@ -420,8 +420,13 @@ test_expect_success 'complains about multiple pack bitmaps' '
>  		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
> +		ls -tr .git/objects/pack | grep -e "^pack-.*\.pack$" > sorted-packs &&
> +		ignored_pack="$(cat sorted-packs | awk 'NR==1{print}')" &&
> +		open_pack="$(cat sorted-packs | awk 'NR==2{print}')" &&

Hmmph. This test only passes if 'ls -tr' gives us the packs in order
that they are read by readdir(), which doesn't seem all that portable to
me. At the very least, it is tightly coupled to the implementation of
open_pack_bitmap() and friends.

> +		GIT_TRACE2_PERF=1 git rev-list --use-bitmap-index HEAD 2>err &&
> +		grep "opened bitmap file:$opened_pack" err &&
> +		grep "ignoring extra bitmap file:$ignored_pack" err

Do we necessarily care about which .bitmap is read and which isn't? The
existing test doesn't look too closely, which I think is fine (though as
the author of that test, I might be biased ;-)).

I would be equally happy to write:

> +		GIT_TRACE2_PERF=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&
> +		grep "opened bitmap" trace2.txt &&
> +		grep "ignoring extra bitmap" trace2.txt

Thanks,
Taylor

Re: [PATCH v2 1/1] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

Taylor Blau <me@ttaylorr.com> writes:

> OK... here we're getting rid of the user-visible warning, which makes
> sense and is the point of this patch.

Yes, my point is to avoid exposing some sensitive informations to end- users.

> But we replace it with a trace2_data_string() that only includes the
> basename? I'd think that the point of moving this warning into
> trace2-land is that we could emit the full path without worrying about
> who sees it, since trace2 data is typically not plumbed through to
> end-users.

I'm not sure if trace2 data will be given to end-users, at least as I understand
it as you, it's not. If so, your opinion is very reasonable.

So... maybe we could add a new configuration like "core.hideSensitive", and
there are some supported values , "loose", "normal " and "strict":

    loose: plumbing full information in trace2, even warning.
    normal: plumbing full information in trace2, but not warning.
    strict: plumbing part of information in trace2, but not warning

But it looks like heavy, maybe not worthy... So, currently I will remove
basename and print the pack with path if there are no new inputs here.

Thanks.

> If we get later bitmap-related information in the trace2 stream, we know
> that we opened a bitmap. And at the moment we read a bitmap, there is
> only one such bitmap in the repository.
>
> I suppose that this is race-proof in the sense that if the bitmaps
> change after reading, we can still usefully debug the trace2 stream
> after the fact.
>
> So even though my first reaction was that this was unnecessary, on
> balance I do find it useful.

Yes... I think it's useful as least for me to do some bitmap tests and
I think print a bit more related information in trace2 data might be OK.

> > -test_expect_success 'complains about multiple pack bitmaps' '
> > +test_expect_success 'complains about multiple pack bitmaps when debugging by trace2' '
> >  	rm -fr repo &&
> >  	git init repo &&
> >  	test_when_finished "rm -fr repo" &&
> > @@ -420,8 +420,13 @@ test_expect_success 'complains about multiple pack bitmaps' '
> >  		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
> > +		ls -tr .git/objects/pack | grep -e "^pack-.*\.pack$" > sorted-packs &&
> > +		ignored_pack="$(cat sorted-packs | awk 'NR==1{print}')" &&
> > +		open_pack="$(cat sorted-packs | awk 'NR==2{print}')" &&
>
> Hmmph. This test only passes if 'ls -tr' gives us the packs in order
> that they are read by readdir(), which doesn't seem all that portable to
> me. At the very least, it is tightly coupled to the implementation of
> open_pack_bitmap() and friends.

Yes, because the _order_ matters here I think originally. May you explain a
little more about _portable_ problem please?

> Do we necessarily care about which .bitmap is read and which isn't? The
> existing test doesn't look too closely, which I think is fine (though as
> the author of that test, I might be biased ;-)).
>
> I would be equally happy to write:
>
> > +		GIT_TRACE2_PERF=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&
> > +		grep "opened bitmap" trace2.txt &&
> > +		grep "ignoring extra bitmap" trace2.txt

Orz. Actually, I wrote it on the same way, but I think it maybe not so
sufficient for my patch. So...

But I think you are right afterI look other test cases, will fix.

Thanks.

Re: [PATCH v2 1/1] pack-bitmap.c: avoid exposing absolute paths

[Taylor Blau]

On Thu, Nov 03, 2022 at 05:35:32PM +0800, Teng Long wrote:
> I'm not sure if trace2 data will be given to end-users, at least as I understand
> it as you, it's not. If so, your opinion is very reasonable.
>
> So... maybe we could add a new configuration like "core.hideSensitive", and
> there are some supported values , "loose", "normal " and "strict":
>
>     loose: plumbing full information in trace2, even warning.
>     normal: plumbing full information in trace2, but not warning.
>     strict: plumbing part of information in trace2, but not warning
>
> But it looks like heavy, maybe not worthy... So, currently I will remove
> basename and print the pack with path if there are no new inputs here.

trace2 data isn't sent to users. So having read this after I took a look
at the updated round, I am glad that you pursued the approach that you
did :-).

> > > -test_expect_success 'complains about multiple pack bitmaps' '
> > > +test_expect_success 'complains about multiple pack bitmaps when debugging by trace2' '
> > >  	rm -fr repo &&
> > >  	git init repo &&
> > >  	test_when_finished "rm -fr repo" &&
> > > @@ -420,8 +420,13 @@ test_expect_success 'complains about multiple pack bitmaps' '
> > >  		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
> > > +		ls -tr .git/objects/pack | grep -e "^pack-.*\.pack$" > sorted-packs &&
> > > +		ignored_pack="$(cat sorted-packs | awk 'NR==1{print}')" &&
> > > +		open_pack="$(cat sorted-packs | awk 'NR==2{print}')" &&
> >
> > Hmmph. This test only passes if 'ls -tr' gives us the packs in order
> > that they are read by readdir(), which doesn't seem all that portable to
> > me. At the very least, it is tightly coupled to the implementation of
> > open_pack_bitmap() and friends.
>
> Yes, because the _order_ matters here I think originally. May you explain a
> little more about _portable_ problem please?

We're depending on the loop in open_pack_bitmap() seeing packs in an
order compatible with the output of 'ls -tr' here. In other words, for
this test to pass, we care very much that the pack we collected as
"$ignored_pack" is seen *after* the pack that we designate as
"$open_pack".

Does that help?

Thanks,
Taylor

Re: [PATCH v2 0/1] pack-bitmap.c: avoid exposing absolute paths

[Taylor Blau]

On Wed, Nov 02, 2022 at 08:56:04PM +0800, Teng Long wrote:
>  pack-bitmap.c           | 12 ++++++++----
>  t/t5310-pack-bitmaps.sh | 11 ++++++++---
>  2 files changed, 16 insertions(+), 7 deletions(-)

This is based on a somewhat out-of-date version of the pack-bitmap.c
code which has changed quite a bit since Abhradeep Chakraborty worked on
it over the summer.

Resolving the merge conflicts during am wasn't too hard, and my rerere
cache should save me from having to do it again. But if you don't mind
rebasing on the tip of 'master' before sending the next round, that
would be appreciated.

Thanks.

Taylor

Re: [PATCH v2 0/1] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

Taylor Blau <me@ttaylorr.com> writes:

> This is based on a somewhat out-of-date version of the pack-bitmap.c
> code which has changed quite a bit since Abhradeep Chakraborty worked on
> it over the summer.
>
> Resolving the merge conflicts during am wasn't too hard, and my rerere
> cache should save me from having to do it again. But if you don't mind
> rebasing on the tip of 'master' before sending the next round, that
> would be appreciated.

Sorry for that, I will do rebase on 'master' before sending v3.

Thanks.

[PATCH v3 0/2] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

From: Teng Long <dyroneteng@gmail.com>

Diff since v2:

1. trace2 log will include the path info of pack. not only "basename".
2. A new commit which removes unnecessary "open_pack_index()" calls in
"open_midx_bitmap_1()".
3. The patch v2 still base on 2.37, rebase on 'master' to prevent conflicts.

Thanks.

Teng Long (2):
  pack-bitmap.c: avoid exposing absolute paths
  pack-bitmap.c: remove unnecessary "open_pack_index()" calls

 pack-bitmap.c           | 15 ++++++++-------
 t/t5310-pack-bitmaps.sh |  5 +++--
 2 files changed, 11 insertions(+), 9 deletions(-)

Range-diff against v2:
1:  1591e7ee52 < -:  ---------- pack-bitmap.c: avoid exposing absolute paths
-:  ---------- > 1:  de941f58f9 pack-bitmap.c: avoid exposing absolute paths
-:  ---------- > 2:  7ac9b859f3 pack-bitmap.c: remove unnecessary "open_pack_index()" calls
-- 
2.38.1.383.g35840fde1dd

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

[Teng Long]

From: Teng Long <dyroneteng@gmail.com>

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 <moweng.xx@antgroup.com>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c           | 12 ++++++++----
 t/t5310-pack-bitmaps.sh |  5 +++--
 2 files changed, 11 insertions(+), 6 deletions(-)

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
 		)
 	'
 }
-- 
2.38.1.383.g35840fde1dd

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

[Taylor Blau]

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 v3 2/2] pack-bitmap.c: remove unnecessary "open_pack_index()" calls

[Teng Long]

From: Teng Long <dyroneteng@gmail.com>

Everytime when calling "open_pack_bitmap_1()", we will firstly call
"open_pack_index(packfile)" to check the index, then further check
again in "is_pack_valid()" before mmap the bitmap file. So, let's
remove the first check here.

The relate discussion:
https://public-inbox.org/git/Y2IiSU1L+bJPUioV@coredump.intra.peff.net/#t

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 9443b7adca..503c95f0b8 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -412,9 +412,6 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 	struct stat st;
 	char *bitmap_name;
 
-	if (open_pack_index(packfile))
-		return -1;
-
 	bitmap_name = pack_bitmap_filename(packfile);
 	fd = git_open(bitmap_name);
 
-- 
2.38.1.383.g35840fde1dd

Re: [PATCH v3 2/2] pack-bitmap.c: remove unnecessary "open_pack_index()" calls

[Taylor Blau]

On Fri, Nov 04, 2022 at 11:17:10AM +0800, Teng Long wrote:
> From: Teng Long <dyroneteng@gmail.com>
>
> Everytime when calling "open_pack_bitmap_1()", we will firstly call
> "open_pack_index(packfile)" to check the index, then further check
> again in "is_pack_valid()" before mmap the bitmap file. So, let's
> remove the first check here.
>
> The relate discussion:
> https://public-inbox.org/git/Y2IiSU1L+bJPUioV@coredump.intra.peff.net/#t

The related discussion is good, but I would have liked to see some of
the details make its way into the patch message for future readers.

Maybe something like:

    When trying to open a pack bitmap, we call open_pack_bitmap_1() in a
    loop, during which it tries to open up the pack index corresponding
    with each available pack.

    It's likely that we'll end up relying on objects in that pack later
    in the process (in which case we're doing the work of opening the
    pack index optimistically), but not guaranteed.

    For instance, consider a repository with a large number of small
    packs, and one large pack with a bitmap. If we see that bitmap pack
    last in our loop which calls open_pack_bitmap_1(), the current code
    will have opened *all* pack index files in the repository. If the
    request can be served out of the bitmapped pack alone, then the time
    spent opening these idx files was wasted.S

    Since open_pack_bitmap_1() calls is_pack_valid() later on (which in
    turns calls open_pack_index() itself), we can just drop the earlier
    call altogether.

Thanks,
Taylor

Re: [PATCH v3 0/2] pack-bitmap.c: avoid exposing absolute paths

[Taylor Blau]

On Fri, Nov 04, 2022 at 11:17:08AM +0800, Teng Long wrote:
> From: Teng Long <dyroneteng@gmail.com>
>
> Diff since v2:
>
> 1. trace2 log will include the path info of pack. not only "basename".
> 2. A new commit which removes unnecessary "open_pack_index()" calls in
> "open_midx_bitmap_1()".
> 3. The patch v2 still base on 2.37, rebase on 'master' to prevent conflicts.
>
> Thanks.

This version is looking pretty good, and I very much appreciate the
rebase onto 'master' in the meantime.

I left a couple of notes throughout which I think merit a small reroll.
Otherwise, this is looking just about ready to go.


Thanks,
Taylor

[PATCH v3 0/2] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

From: Teng Long <dyroneteng@gmail.com>

Diff since v2:

* remove unnecessary comments.
* use "GIT_TRACE2_EVENT" instead of "GIT_TRACE_PERF".
* improve commit message of [1/2].

Thanks.

Teng Long (2):
  pack-bitmap.c: remove unnecessary "open_pack_index()" calls
  pack-bitmap.c: avoid exposing absolute paths

 pack-bitmap.c           | 13 ++++++-------
 t/t5310-pack-bitmaps.sh |  5 +++--
 2 files changed, 9 insertions(+), 9 deletions(-)

Range-diff against v2:
2:  7ac9b859f3 ! 1:  aaeb021538 pack-bitmap.c: remove unnecessary "open_pack_index()" calls
    @@ Metadata
      ## Commit message ##
         pack-bitmap.c: remove unnecessary "open_pack_index()" calls
     
    -    Everytime when calling "open_pack_bitmap_1()", we will firstly call
    -    "open_pack_index(packfile)" to check the index, then further check
    -    again in "is_pack_valid()" before mmap the bitmap file. So, let's
    -    remove the first check here.
    +    When trying to open a pack bitmap, we call open_pack_bitmap_1() in a
    +    loop, during which it tries to open up the pack index corresponding
    +    with each available pack.
     
    -    The relate discussion:
    -    https://public-inbox.org/git/Y2IiSU1L+bJPUioV@coredump.intra.peff.net/#t
    +    It's likely that we'll end up relying on objects in that pack later
    +    in the process (in which case we're doing the work of opening the
    +    pack index optimistically), but not guaranteed.
    +
    +    For instance, consider a repository with a large number of small
    +    packs, and one large pack with a bitmap. If we see that bitmap pack
    +    last in our loop which calls open_pack_bitmap_1(), the current code
    +    will have opened *all* pack index files in the repository. If the
    +    request can be served out of the bitmapped pack alone, then the time
    +    spent opening these idx files was wasted.S
    +
    +    Since open_pack_bitmap_1() calls is_pack_valid() later on (which in
    +    turns calls open_pack_index() itself), we can just drop the earlier
    +    call altogether.
     
         Signed-off-by: Teng Long <dyroneteng@gmail.com>
     
1:  de941f58f9 ! 2:  9d5a491887 pack-bitmap.c: avoid exposing absolute paths
    @@ pack-bitmap.c: static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
      		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);
    @@ pack-bitmap.c: static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, st
      	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);
    @@ t/t5310-pack-bitmaps.sh: test_bitmap_cases () {
      
     -			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 &&
    ++			GIT_TRACE2_EVENT=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&
     +			grep "opened bitmap" trace2.txt &&
     +			grep "ignoring extra bitmap" trace2.txt
      		)
-- 
2.38.1.383.g7ac9b859f31.dirty

[PATCH v3 1/2] pack-bitmap.c: remove unnecessary "open_pack_index()" calls

[Teng Long]

From: Teng Long <dyroneteng@gmail.com>

When trying to open a pack bitmap, we call open_pack_bitmap_1() in a
loop, during which it tries to open up the pack index corresponding
with each available pack.

It's likely that we'll end up relying on objects in that pack later
in the process (in which case we're doing the work of opening the
pack index optimistically), but not guaranteed.

For instance, consider a repository with a large number of small
packs, and one large pack with a bitmap. If we see that bitmap pack
last in our loop which calls open_pack_bitmap_1(), the current code
will have opened *all* pack index files in the repository. If the
request can be served out of the bitmapped pack alone, then the time
spent opening these idx files was wasted.S

Since open_pack_bitmap_1() calls is_pack_valid() later on (which in
turns calls open_pack_index() itself), we can just drop the earlier
call altogether.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 440407f1be..982e286bac 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -411,9 +411,6 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 	struct stat st;
 	char *bitmap_name;
 
-	if (open_pack_index(packfile))
-		return -1;
-
 	bitmap_name = pack_bitmap_filename(packfile);
 	fd = git_open(bitmap_name);
 
-- 
2.38.1.383.g7ac9b859f31.dirty

Re: [PATCH v3 1/2] pack-bitmap.c: remove unnecessary "open_pack_index()" calls

[Jeff King]

On Thu, Nov 10, 2022 at 03:10:11PM +0800, Teng Long wrote:

> It's likely that we'll end up relying on objects in that pack later
> in the process (in which case we're doing the work of opening the
> pack index optimistically), but not guaranteed.
> 
> For instance, consider a repository with a large number of small
> packs, and one large pack with a bitmap. If we see that bitmap pack
> last in our loop which calls open_pack_bitmap_1(), the current code
> will have opened *all* pack index files in the repository. If the
> request can be served out of the bitmapped pack alone, then the time
> spent opening these idx files was wasted.S

By the way, I wondered if it was possible to measure a slowdown in this
case. It is, but you have to try pretty hard. Something like this:

  # one bitmapped pack
  git repack -adb

  # and then a bunch of other packs
  git rev-list HEAD |
  head -10000 |
  while read commit; do
    echo $commit | git pack-objects .git/objects/pack/pack
  done

  # make the bitmapped one newest, since otherwise our non-bitmap lookup
  # of the initial traversal commit causes us to open all the other
  # packs first!
  bitmap=$(echo .git/objects/pack/pack-*.bitmap)
  touch ${bitmap%.bitmap}.*

  hyperfine -L v old,new './git.{v} rev-list --count --use-bitmap-index HEAD'

where "new" and "old" are builds with and without this patch. I get:

  Benchmark 1: ./git.old rev-list --count --use-bitmap-index HEAD
    Time (mean ± σ):     117.9 ms ±   1.8 ms    [User: 26.9 ms, System: 90.0 ms]
    Range (min … max):   113.4 ms … 120.3 ms    25 runs
  
  Benchmark 2: ./git.new rev-list --count --use-bitmap-index HEAD
    Time (mean ± σ):      71.8 ms ±   2.6 ms    [User: 21.2 ms, System: 50.5 ms]
    Range (min … max):    67.0 ms …  75.1 ms    41 runs
  
  Summary
    './git.new rev-list --count --use-bitmap-index HEAD' ran
      1.64 ± 0.06 times faster than './git.old rev-list --count --use-bitmap-index HEAD'

which implies to me two things:

  - this probably isn't helping anybody much in the real world, as
    evidenced by the contortions I had to go through to set up the
    situation (and which would be made much better by repacking, which
    would also speed up non-bitmap operations).

  - it's worth doing anyway. Even if it only shaves off microseconds,
    the existing call is just pointless.

-Peff

Re: [PATCH v3 1/2] pack-bitmap.c: remove unnecessary "open_pack_index()" calls

[Taylor Blau]

On Mon, Nov 14, 2022 at 05:03:56PM -0500, Jeff King wrote:
> On Thu, Nov 10, 2022 at 03:10:11PM +0800, Teng Long wrote:
>
> > It's likely that we'll end up relying on objects in that pack later
> > in the process (in which case we're doing the work of opening the
> > pack index optimistically), but not guaranteed.
> >
> > For instance, consider a repository with a large number of small
> > packs, and one large pack with a bitmap. If we see that bitmap pack
> > last in our loop which calls open_pack_bitmap_1(), the current code
> > will have opened *all* pack index files in the repository. If the
> > request can be served out of the bitmapped pack alone, then the time
> > spent opening these idx files was wasted.S
>
> By the way, I wondered if it was possible to measure a slowdown in this
> case. It is, but you have to try pretty hard. Something like this:
>
>   # one bitmapped pack
>   git repack -adb
>
>   # and then a bunch of other packs
>   git rev-list HEAD |
>   head -10000 |
>   while read commit; do
>     echo $commit | git pack-objects .git/objects/pack/pack
>   done

OK, so with 10K packs, we see about a 1.6-fold improvement, which is
definitely substantial.

On a fresh clone of git.git, repeating your experiment with only 1K
packs (which is definitely a number of packs that GitHub sees in
under-maintained repositories), the runtime goes from 25.3ms -> 20.9ms
on my machine, or about a 1.2-fold improvement.

So definitely smaller, but even at 1/10th the number of packs from your
experiment, still noticeable.

>   - this probably isn't helping anybody much in the real world, as
>     evidenced by the contortions I had to go through to set up the
>     situation (and which would be made much better by repacking, which
>     would also speed up non-bitmap operations).

Per above, I'm not sure I totally agree ;-). 1K packs is definitely an
extreme amount of packs, but not out-of-this-world. It probably would
show up in carefully-picked graphs, but not in "overall git rev-list
time" or something as broad/noisy as that.

>   - it's worth doing anyway. Even if it only shaves off microseconds,
>     the existing call is just pointless.

Yes, definitely.

Thanks,
Taylor

Re: [PATCH v3 1/2] pack-bitmap.c: remove unnecessary "open_pack_index()" calls

[Jeff King]

On Mon, Nov 14, 2022 at 05:14:37PM -0500, Taylor Blau wrote:

> OK, so with 10K packs, we see about a 1.6-fold improvement, which is
> definitely substantial.
> 
> On a fresh clone of git.git, repeating your experiment with only 1K
> packs (which is definitely a number of packs that GitHub sees in
> under-maintained repositories), the runtime goes from 25.3ms -> 20.9ms
> on my machine, or about a 1.2-fold improvement.
> 
> So definitely smaller, but even at 1/10th the number of packs from your
> experiment, still noticeable.

Interesting. I had tried it initially with 1000 and the improvements were
much smaller. I just did it again, though, and got the same 20% speedup.
I'm not sure what I screwed up earlier (I may have been confused by the
timestamp/sorting issue; I only realized it was important midway through
looking into this).

> >   - this probably isn't helping anybody much in the real world, as
> >     evidenced by the contortions I had to go through to set up the
> >     situation (and which would be made much better by repacking, which
> >     would also speed up non-bitmap operations).
> 
> Per above, I'm not sure I totally agree ;-). 1K packs is definitely an
> extreme amount of packs, but not out-of-this-world. It probably would
> show up in carefully-picked graphs, but not in "overall git rev-list
> time" or something as broad/noisy as that.

Yeah, I agree that 1k is a lot more compelling. The big impractical
thing I think is that if the bitmapped pack is older (and it usually
is), then we'd often open all the other packs anyway:

  - if the start of the traversal is in the bitmapped pack, then we
    fruitlessly open each of the others looking for the object (since
    the bitmapped one will come last in the reverse-chronological
    sorting)

  - if it isn't in the bitmapped pack, then we'll end up opening all
    those other packs anyway to fill out the bitmap (since by definition
    it can't be included in the on-disk bitmaps)

So I'd be surprised if it ever mattered in the real world. Though again,
I think the new code is less surprising in general, and could matter if
we changed other things (e.g., if we prioritized lookups in a pack with
a .bitmap).

-Peff

Re: [PATCH v3 1/2] pack-bitmap.c: remove unnecessary "open_pack_index()" calls

[Taylor Blau]

On Mon, Nov 14, 2022 at 05:31:18PM -0500, Jeff King wrote:
> Yeah, I agree that 1k is a lot more compelling. The big impractical
> thing I think is that if the bitmapped pack is older (and it usually
> is), then we'd often open all the other packs anyway:
>
>   - if the start of the traversal is in the bitmapped pack, then we
>     fruitlessly open each of the others looking for the object (since
>     the bitmapped one will come last in the reverse-chronological
>     sorting)
>
>   - if it isn't in the bitmapped pack, then we'll end up opening all
>     those other packs anyway to fill out the bitmap (since by definition
>     it can't be included in the on-disk bitmaps)
>
> So I'd be surprised if it ever mattered in the real world. Though again,
> I think the new code is less surprising in general, and could matter if
> we changed other things (e.g., if we prioritized lookups in a pack with
> a .bitmap).

I completely agree. It's definitely worth doing purely based on the
principle of least-surprise. But the potential performance improvements
are just gravy on top ;-).

Thanks,
Taylor

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

[Teng Long]

From: Teng Long <dyroneteng@gmail.com>

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 <moweng.xx@antgroup.com>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c           | 10 ++++++----
 t/t5310-pack-bitmaps.sh |  5 +++--
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 982e286bac..aaa2d9a104 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -354,8 +354,8 @@ 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);
+		trace2_data_string("bitmap", the_repository,
+				   "ignoring extra midx bitmap file", buf.buf);
 		close(fd);
 		strbuf_release(&buf);
 		return -1;
@@ -429,8 +429,8 @@ 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);
+		trace2_data_string("bitmap", the_repository,
+				   "ignoring extra bitmap file", packfile->pack_name);
 		close(fd);
 		return -1;
 	}
@@ -455,6 +455,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..7d8dee41b0 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_EVENT=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&
+			grep "opened bitmap" trace2.txt &&
+			grep "ignoring extra bitmap" trace2.txt
 		)
 	'
 }
-- 
2.38.1.383.g7ac9b859f31.dirty

Re: [PATCH v3 0/2] pack-bitmap.c: avoid exposing absolute paths

[Taylor Blau]

On Thu, Nov 10, 2022 at 03:10:10PM +0800, Teng Long wrote:
> From: Teng Long <dyroneteng@gmail.com>
>
> Diff since v2:
>
> * remove unnecessary comments.
> * use "GIT_TRACE2_EVENT" instead of "GIT_TRACE_PERF".
> * improve commit message of [1/2].

Thanks, this version looks great. I'm very satisfied with where it ended
up, and I'm feeling comfortable with merging it down.

Thanks,
Taylor

Re: [PATCH v3 0/2] pack-bitmap.c: avoid exposing absolute paths

[Jeff King]

On Fri, Nov 11, 2022 at 05:26:39PM -0500, Taylor Blau wrote:

> On Thu, Nov 10, 2022 at 03:10:10PM +0800, Teng Long wrote:
> > From: Teng Long <dyroneteng@gmail.com>
> >
> > Diff since v2:
> >
> > * remove unnecessary comments.
> > * use "GIT_TRACE2_EVENT" instead of "GIT_TRACE_PERF".
> > * improve commit message of [1/2].
> 
> Thanks, this version looks great. I'm very satisfied with where it ended
> up, and I'm feeling comfortable with merging it down.

Me too. If we wanted to go further, there are two obvious next steps.

One, we can break out of the bitmap loop early if we're not tracing:

diff --git a/pack-bitmap.c b/pack-bitmap.c
index aaa2d9a104..3b6c2f804a 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -527,8 +527,15 @@ static int open_pack_bitmap(struct repository *r,
 	assert(!bitmap_git->map);
 
 	for (p = get_all_packs(r); p; p = p->next) {
-		if (open_pack_bitmap_1(bitmap_git, p) == 0)
+		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
 			ret = 0;
+			/*
+			 * The only reason to keep looking is to report
+			 * duplicates.
+			 */
+			if (!trace2_is_enabled())
+				break;
+		}
 	}
 
 	return ret;

I doubt this buys us much in practice. After patch 2, looking for extra
bitmaps is much cheaper. It's one open() call per pack (which will
return ENOENT normally) looking for a bitmap. And while it's only 2
lines of code, it does increase coupling of assumptions between the two
functions. So maybe not worth doing. I dunno.

And two, we could complain when there is both a midx and a pack bitmap,
like so:

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 3b6c2f804a..44a80ed8f2 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -524,8 +524,6 @@ static int open_pack_bitmap(struct repository *r,
 	struct packed_git *p;
 	int ret = -1;
 
-	assert(!bitmap_git->map);
-
 	for (p = get_all_packs(r); p; p = p->next) {
 		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
 			ret = 0;
@@ -559,11 +557,20 @@ static int open_midx_bitmap(struct repository *r,
 static int open_bitmap(struct repository *r,
 		       struct bitmap_index *bitmap_git)
 {
+	int found = 0;
+
 	assert(!bitmap_git->map);
 
-	if (!open_midx_bitmap(r, bitmap_git))
-		return 0;
-	return open_pack_bitmap(r, bitmap_git);
+	found = !open_midx_bitmap(r, bitmap_git);
+
+	/*
+	 * these will all be skipped if we opened a midx bitmap; but run it
+	 * anyway if tracing is enabled to report the duplicates
+	 */
+	if (!found || trace2_is_enabled())
+		found |= !open_pack_bitmap(r, bitmap_git);
+
+	return found ? 0 : -1;
 }
 
 struct bitmap_index *prepare_bitmap_git(struct repository *r)

But I'm not sure if that is even something we want. I know we retained
pack bitmaps as a quick recovery mechanism while test-deploying midx
bitmaps. OTOH, now that the feature is stable, I doubt anybody else
would ever do that.

It also suffers from the same coupling issues as the other.

So I don't know that either is worth pursuing (hence this message and
not fully prepared patches), but these are just the two leftover things
I noticed from the series, so I wanted to record them for posterity.

-Peff

Re: [PATCH v3 0/2] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

> Me too. If we wanted to go further, there are two obvious next steps.
>
> One, we can break out of the bitmap loop early if we're not tracing:

> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index aaa2d9a104..3b6c2f804a 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -527,8 +527,15 @@ static int open_pack_bitmap(struct repository *r,
>  	assert(!bitmap_git->map);
>
>  	for (p = get_all_packs(r); p; p = p->next) {
> -		if (open_pack_bitmap_1(bitmap_git, p) == 0)
> +		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
>  			ret = 0;
> +			/*
> +			 * The only reason to keep looking is to report
> +			 * duplicates.
> +			 */
> +			if (!trace2_is_enabled())
> +				break;
> +		}
>  	}
>
>  	return ret;
>
> I doubt this buys us much in practice. After patch 2, looking for extra
> bitmaps is much cheaper. It's one open() call per pack (which will
> return ENOENT normally) looking for a bitmap. And while it's only 2
> lines of code, it does increase coupling of assumptions between the two
> functions. So maybe not worth doing. I dunno.

I agree and I think it's reasonable.

So If I bring it into the patch how about the commit message:

Author: Jeff King <peff@peff.net>
Date:   Thu Nov 17 19:55:23 2022 +0800

    pack-bitmap.c: break out of the bitmap loop early if not tracing

    When we successfully open a bitmap, we will continue to try to open
    other packs, and when trace2 is enabled, we will report any subsequent
    bitmap ignored information in the log. So when we find that trace2 is
    not enabled, we can actually terminate the loop early.

    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Teng Long <dyroneteng@gmail.com>

> And two, we could complain when there is both a midx and a pack bitmap,
> like so:
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 3b6c2f804a..44a80ed8f2 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -524,8 +524,6 @@ static int open_pack_bitmap(struct repository *r,
>  	struct packed_git *p;
>  	int ret = -1;
>
> -	assert(!bitmap_git->map);
> -
>  	for (p = get_all_packs(r); p; p = p->next) {
>  		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
>  			ret = 0;
> @@ -559,11 +557,20 @@ static int open_midx_bitmap(struct repository *r,
>  static int open_bitmap(struct repository *r,
>  		       struct bitmap_index *bitmap_git)
>  {
> +	int found = 0;
> +
>  	assert(!bitmap_git->map);
>
> -	if (!open_midx_bitmap(r, bitmap_git))
> -		return 0;
> -	return open_pack_bitmap(r, bitmap_git);
> +	found = !open_midx_bitmap(r, bitmap_git);
> +
> +	/*
> +	 * these will all be skipped if we opened a midx bitmap; but run it
> +	 * anyway if tracing is enabled to report the duplicates
> +	 */
> +	if (!found || trace2_is_enabled())
> +		found |= !open_pack_bitmap(r, bitmap_git);
> +
> +	return found ? 0 : -1;
>  }
>
>  struct bitmap_index *prepare_bitmap_git(struct repository *r)
>
> But I'm not sure if that is even something we want. I know we retained
> pack bitmaps as a quick recovery mechanism while test-deploying midx
> bitmaps. OTOH, now that the feature is stable, I doubt anybody else
> would ever do that.
>
> It also suffers from the same coupling issues as the other.
>
> So I don't know that either is worth pursuing (hence this message and
> not fully prepared patches), but these are just the two leftover things
> I noticed from the series, so I wanted to record them for posterity.

Since this is an internal mechanism, and we are doing reminders in trace2, so
the diff looks good to me. How about the commit message if I need to take it:

Author: Jeff King <peff@peff.net>
Date:   Thu Nov 17 20:25:18 2022 +0800

    pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found

    We retained pack bitmaps as a quick recovery mechanism while
    test-deploying midx bitmaps. This is an internal mechanism, and we
    want to expose this rule externally through trace2 so that end users,
    repo-maintainers, and debuggers know what is happening in the process.

    Signed-off-by: Jeff King <peff@peff.net>
    Signed-off-by: Teng Long <dyroneteng@gmail.com>

Thank you Peff for providing such detailed suggestions for improvement.

Re: [PATCH v3 0/2] pack-bitmap.c: avoid exposing absolute paths

[Jeff King]

On Thu, Nov 17, 2022 at 10:19:33PM +0800, Teng Long wrote:

> > I doubt this buys us much in practice. After patch 2, looking for extra
> > bitmaps is much cheaper. It's one open() call per pack (which will
> > return ENOENT normally) looking for a bitmap. And while it's only 2
> > lines of code, it does increase coupling of assumptions between the two
> > functions. So maybe not worth doing. I dunno.
> 
> I agree and I think it's reasonable.
> 
> So If I bring it into the patch how about the commit message:
> [...]

Both the commit messages you proposed look accurate to me.

I was a bit skeptical on the second one (warning about skipping the
pack bitmap when a midx is present) just because we really did do that
intentionally at one point. But then I remembered that these are no
longer producing warnings, but trace output. So in that sense, nobody
will really be bothered by them.

I do wonder if there are people who have trace2 on all the time (for
performance tracing, telemetry, etc) who would find these to be junk in
their logs. IMHO the trace2 mechanism is a bit coarse grained in that we
can only check "is it on". One of the nice things about the original
trace facility is that we could stuff this behind GIT_TRACE_BITMAPS
which would really only be turned on if somebody wanted to debug
bitmaps.

-Peff

Re: [PATCH v3 0/2] pack-bitmap.c: avoid exposing absolute paths

[Taylor Blau]

On Thu, Nov 17, 2022 at 10:03:06AM -0500, Jeff King wrote:
> On Thu, Nov 17, 2022 at 10:19:33PM +0800, Teng Long wrote:
>
> > > I doubt this buys us much in practice. After patch 2, looking for extra
> > > bitmaps is much cheaper. It's one open() call per pack (which will
> > > return ENOENT normally) looking for a bitmap. And while it's only 2
> > > lines of code, it does increase coupling of assumptions between the two
> > > functions. So maybe not worth doing. I dunno.
> >
> > I agree and I think it's reasonable.
> >
> > So If I bring it into the patch how about the commit message:
> > [...]
>
> Both the commit messages you proposed look accurate to me.

Yep, ditto. Teng -- would you mind sending them as a short series to the
list so that I can pick them up? Otherwise, I can do it if you don't
have time or interest.

Thanks,
Taylor

Re: [PATCH v3 0/2] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

Taylor Blau <me@ttaylorr.com> writes:

> Yep, ditto. Teng -- would you mind sending them as a short series to the
> list so that I can pick them up? Otherwise, I can do it if you don't
> have time or interest.

Sorry, will send today.

Thanks

[PATCH v4 0/4] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

From: Teng Long <dyroneteng@gmail.com>

Sorry for relying late, Taylor..

Diff since v3:

I picked Peff's two new constructive patches[1] which I think are meaningful.
Peff also mentioned whether to design a "GIT_TRACE2_BITMAP" ENV to help
debugging bitmap, and I think it's very interesting but maybe it's a bigger
scale on current patchset, so I prefer to do it in another series.

1. https://public-inbox.org/git/Y3K%2F%2FkO3fxD7Pl3%2F@coredump.intra.peff.net/

Thanks.

Jeff King (2):
  pack-bitmap.c: break out of the bitmap loop early if not tracing
  pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found

Teng Long (2):
  pack-bitmap.c: remove unnecessary "open_pack_index()" calls
  pack-bitmap.c: avoid exposing absolute paths

 pack-bitmap.c           | 39 ++++++++++++++++++++++++++-------------
 t/t5310-pack-bitmaps.sh |  5 +++--
 2 files changed, 29 insertions(+), 15 deletions(-)

Range-diff against v3:
-:  ---------- > 1:  aaeb021538 pack-bitmap.c: remove unnecessary "open_pack_index()" calls
1:  de941f58f9 ! 2:  9d5a491887 pack-bitmap.c: avoid exposing absolute paths
    @@ pack-bitmap.c: static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
      		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);
    @@ pack-bitmap.c: static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, st
      	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);
    @@ t/t5310-pack-bitmaps.sh: test_bitmap_cases () {
      
     -			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 &&
    ++			GIT_TRACE2_EVENT=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&
     +			grep "opened bitmap" trace2.txt &&
     +			grep "ignoring extra bitmap" trace2.txt
      		)
-:  ---------- > 3:  f6c5b45bdc pack-bitmap.c: break out of the bitmap loop early if not tracing
-:  ---------- > 4:  2acaa3f097 pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found
-- 
2.38.1.383.g9d5a491887b

[PATCH v4 1/4] pack-bitmap.c: remove unnecessary "open_pack_index()" calls

[Teng Long]

From: Teng Long <dyroneteng@gmail.com>

When trying to open a pack bitmap, we call open_pack_bitmap_1() in a
loop, during which it tries to open up the pack index corresponding
with each available pack.

It's likely that we'll end up relying on objects in that pack later
in the process (in which case we're doing the work of opening the
pack index optimistically), but not guaranteed.

For instance, consider a repository with a large number of small
packs, and one large pack with a bitmap. If we see that bitmap pack
last in our loop which calls open_pack_bitmap_1(), the current code
will have opened *all* pack index files in the repository. If the
request can be served out of the bitmapped pack alone, then the time
spent opening these idx files was wasted.S

Since open_pack_bitmap_1() calls is_pack_valid() later on (which in
turns calls open_pack_index() itself), we can just drop the earlier
call altogether.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 440407f1be..982e286bac 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -411,9 +411,6 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 	struct stat st;
 	char *bitmap_name;
 
-	if (open_pack_index(packfile))
-		return -1;
-
 	bitmap_name = pack_bitmap_filename(packfile);
 	fd = git_open(bitmap_name);
 
-- 
2.38.1.383.g9d5a491887b

[PATCH v4 2/4] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

From: Teng Long <dyroneteng@gmail.com>

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 <moweng.xx@antgroup.com>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c           | 10 ++++++----
 t/t5310-pack-bitmaps.sh |  5 +++--
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 982e286bac..aaa2d9a104 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -354,8 +354,8 @@ 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);
+		trace2_data_string("bitmap", the_repository,
+				   "ignoring extra midx bitmap file", buf.buf);
 		close(fd);
 		strbuf_release(&buf);
 		return -1;
@@ -429,8 +429,8 @@ 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);
+		trace2_data_string("bitmap", the_repository,
+				   "ignoring extra bitmap file", packfile->pack_name);
 		close(fd);
 		return -1;
 	}
@@ -455,6 +455,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..7d8dee41b0 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_EVENT=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&
+			grep "opened bitmap" trace2.txt &&
+			grep "ignoring extra bitmap" trace2.txt
 		)
 	'
 }
-- 
2.38.1.383.g9d5a491887b

[PATCH v4 3/4] pack-bitmap.c: break out of the bitmap loop early if not tracing

[Teng Long]

From: Jeff King <peff@peff.net>

When we successfully open a bitmap, we will continue to try to open
other packs, and when trace2 is enabled, we will report any subsequent
bitmap ignored information in the log. So when we find that trace2 is
not enabled, we can actually terminate the loop early.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index aaa2d9a104..3b6c2f804a 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -527,8 +527,15 @@ static int open_pack_bitmap(struct repository *r,
 	assert(!bitmap_git->map);
 
 	for (p = get_all_packs(r); p; p = p->next) {
-		if (open_pack_bitmap_1(bitmap_git, p) == 0)
+		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
 			ret = 0;
+			/*
+			 * The only reason to keep looking is to report
+			 * duplicates.
+			 */
+			if (!trace2_is_enabled())
+				break;
+		}
 	}
 
 	return ret;
-- 
2.38.1.383.g9d5a491887b

Re: [PATCH v4 3/4] pack-bitmap.c: break out of the bitmap loop early if not tracing

[Junio C Hamano]

Teng Long <dyroneteng@gmail.com> writes:

> From: Jeff King <peff@peff.net>
>
> When we successfully open a bitmap, we will continue to try to open
> other packs, and when trace2 is enabled, we will report any subsequent
> bitmap ignored information in the log. So when we find that trace2 is
> not enabled, we can actually terminate the loop early.

The above took me a few reads to understand what it wants to say,
probably because the "and when trace2 is enabled" comes a bit too
late to explain why "try to open other" is done.  After reading it a
few times, here is what I think it wants to say:

    After opening a bitmap successfully, we try opening others only
    because we want to report that other bitmap files are ignored in
    the trace2 log.  When trace2 is not enabled, we do not have to
    do any of that.



>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Teng Long <dyroneteng@gmail.com>
> ---
>  pack-bitmap.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index aaa2d9a104..3b6c2f804a 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -527,8 +527,15 @@ static int open_pack_bitmap(struct repository *r,
>  	assert(!bitmap_git->map);
>  
>  	for (p = get_all_packs(r); p; p = p->next) {
> -		if (open_pack_bitmap_1(bitmap_git, p) == 0)
> +		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
>  			ret = 0;
> +			/*
> +			 * The only reason to keep looking is to report
> +			 * duplicates.
> +			 */
> +			if (!trace2_is_enabled())
> +				break;
> +		}
>  	}
>  
>  	return ret;

Re: [PATCH v4 3/4] pack-bitmap.c: break out of the bitmap loop early if not tracing

[Teng Long]

Junio C Hamano <gitster@pobox.com> writes:

> From: Jeff King <peff@peff.net>
>
> When we successfully open a bitmap, we will continue to try to open
> other packs, and when trace2 is enabled, we will report any subsequent
> bitmap ignored information in the log. So when we find that trace2 is
> not enabled, we can actually terminate the loop early.

> The above took me a few reads to understand what it wants to say,
> probably because the "and when trace2 is enabled" comes a bit too
> late to explain why "try to open other" is done.  After reading it a
> few times, here is what I think it wants to say:
>
>     After opening a bitmap successfully, we try opening others only
>     because we want to report that other bitmap files are ignored in
>     the trace2 log.  When trace2 is not enabled, we do not have to
>     do any of that.

Thanks for the reword, will apply in reroll.

[PATCH v4 4/4] pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found

[Teng Long]

From: Jeff King <peff@peff.net>

We retained pack bitmaps as a quick recovery mechanism while
test-deploying midx bitmaps. This is an internal mechanism, and we
want to expose this rule externally through trace2 so that end users,
repo-maintainers, and debuggers know what is happening in the process.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 3b6c2f804a..44a80ed8f2 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -524,8 +524,6 @@ static int open_pack_bitmap(struct repository *r,
 	struct packed_git *p;
 	int ret = -1;
 
-	assert(!bitmap_git->map);
-
 	for (p = get_all_packs(r); p; p = p->next) {
 		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
 			ret = 0;
@@ -559,11 +557,20 @@ static int open_midx_bitmap(struct repository *r,
 static int open_bitmap(struct repository *r,
 		       struct bitmap_index *bitmap_git)
 {
+	int found = 0;
+
 	assert(!bitmap_git->map);
 
-	if (!open_midx_bitmap(r, bitmap_git))
-		return 0;
-	return open_pack_bitmap(r, bitmap_git);
+	found = !open_midx_bitmap(r, bitmap_git);
+
+	/*
+	 * these will all be skipped if we opened a midx bitmap; but run it
+	 * anyway if tracing is enabled to report the duplicates
+	 */
+	if (!found || trace2_is_enabled())
+		found |= !open_pack_bitmap(r, bitmap_git);
+
+	return found ? 0 : -1;
 }
 
 struct bitmap_index *prepare_bitmap_git(struct repository *r)
-- 
2.38.1.383.g9d5a491887b

Re: [PATCH v4 4/4] pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found

[Jeff King]

On Mon, Nov 21, 2022 at 08:16:15PM +0800, Teng Long wrote:

> From: Jeff King <peff@peff.net>
> 
> We retained pack bitmaps as a quick recovery mechanism while
> test-deploying midx bitmaps. This is an internal mechanism, and we
> want to expose this rule externally through trace2 so that end users,
> repo-maintainers, and debuggers know what is happening in the process.

Re-reading this outside the context of the earlier thread, I think "this
rule" is a little vague. Maybe:

  When we find a midx bitmap, we do not bother checking for pack
  bitmaps, since we can use only one. But since we will warn of unused
  bitmaps via trace2, let's continue looking for pack bitmaps when
  tracing is enabled.

> @@ -559,11 +557,20 @@ static int open_midx_bitmap(struct repository *r,
>  static int open_bitmap(struct repository *r,
>  		       struct bitmap_index *bitmap_git)
>  {
> +	int found = 0;
> +
>  	assert(!bitmap_git->map);
>  
> -	if (!open_midx_bitmap(r, bitmap_git))
> -		return 0;
> -	return open_pack_bitmap(r, bitmap_git);
> +	found = !open_midx_bitmap(r, bitmap_git);

I think we can drop the initialization of "found = 0"; that value is
unused, since we'll always assign to it (I think my initial attempt had
setting it to 1 inside the conditional).

It's not hurting anything functionally, but it makes the code slightly
more confusing to read.

-Peff

Re: [PATCH v4 4/4] pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ...
> It's not hurting anything functionally, but it makes the code slightly
> more confusing to read.

Thanks for all these good suggestions.

As you pointed out, the first two seems to be identical to what
Taylor queued in 'next' already (so they do not have to be re-sent
but it is not a huge deal if they did---it would be a huge deal if
they were rewritten, though), and both of the two follow-on patches
make sense with suggested (minor) updates.  I'll expect a reroll
before queuing these two and mark the topic in 'next' to be waiting
for them.

Thanks, both.

Re: [PATCH v4 4/4] pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found

[Teng Long]

Junio C Hamano <gitster@pobox.com> write:

> Thanks for all these good suggestions.
>
> As you pointed out, the first two seems to be identical to what
> Taylor queued in 'next' already (so they do not have to be re-sent
> but it is not a huge deal if they did---it would be a huge deal if
> they were rewritten, though), and both of the two follow-on patches
> make sense with suggested (minor) updates.  I'll expect a reroll
> before queuing these two and mark the topic in 'next' to be waiting
> for them.

Sorry, Not online for a few days last week.
Will send a reroll today.

Thanks.

Re: [PATCH v4 4/4] pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found

[Teng Long]

Jeff King <peff@peff.net> writes:

> > From: Jeff King <peff@peff.net>
> >
> > We retained pack bitmaps as a quick recovery mechanism while
> > test-deploying midx bitmaps. This is an internal mechanism, and we
> > want to expose this rule externally through trace2 so that end users,
> > repo-maintainers, and debuggers know what is happening in the process.
>
> Re-reading this outside the context of the earlier thread, I think "this
> rule" is a little vague. Maybe:
>
>   When we find a midx bitmap, we do not bother checking for pack
>   bitmaps, since we can use only one. But since we will warn of unused
>   bitmaps via trace2, let's continue looking for pack bitmaps when
>   tracing is enabled.

Thanks. That's more clear and will be applies on next patch.

> > @@ -559,11 +557,20 @@ static int open_midx_bitmap(struct repository *r,
> >  static int open_bitmap(struct repository *r,
> >  		       struct bitmap_index *bitmap_git)
> >  {
> > +	int found = 0;
> > +
> >  	assert(!bitmap_git->map);
> >
> > -	if (!open_midx_bitmap(r, bitmap_git))
> > -		return 0;
> > -	return open_pack_bitmap(r, bitmap_git);
> > +	found = !open_midx_bitmap(r, bitmap_git);
>
> I think we can drop the initialization of "found = 0"; that value is
> unused, since we'll always assign to it (I think my initial attempt had
> setting it to 1 inside the conditional).
>
> It's not hurting anything functionally, but it makes the code slightly
> more confusing to read.

I agree it's not hurting here, it's OK for me to make the improvement
here. But I have a question, do we prefer to omit the initialization
in such scenarios if we make sure it will initialized correctly？

Thanks.

Re: [PATCH v4 4/4] pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found

[Jeff King]

On Mon, Nov 28, 2022 at 08:37:40PM +0800, Teng Long wrote:

> > > @@ -559,11 +557,20 @@ static int open_midx_bitmap(struct repository *r,
> > >  static int open_bitmap(struct repository *r,
> > >  		       struct bitmap_index *bitmap_git)
> > >  {
> > > +	int found = 0;
> > > +
> > >  	assert(!bitmap_git->map);
> > >
> > > -	if (!open_midx_bitmap(r, bitmap_git))
> > > -		return 0;
> > > -	return open_pack_bitmap(r, bitmap_git);
> > > +	found = !open_midx_bitmap(r, bitmap_git);
> >
> > I think we can drop the initialization of "found = 0"; that value is
> > unused, since we'll always assign to it (I think my initial attempt had
> > setting it to 1 inside the conditional).
> >
> > It's not hurting anything functionally, but it makes the code slightly
> > more confusing to read.
> 
> I agree it's not hurting here, it's OK for me to make the improvement
> here. But I have a question, do we prefer to omit the initialization
> in such scenarios if we make sure it will initialized correctly？

I don't know that we have a particular rule here, but I would say that
yes, if you know the initialization isn't used, then skip it. That
communicates the expectation to anybody reading the code. And if you're
wrong, the compiler will generally notice.

-Peff

Re: [PATCH v4 4/4] pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found

[Teng Long]

Jeff King <peff@peff.net> writes:

> > I agree it's not hurting here, it's OK for me to make the improvement
> > here. But I have a question, do we prefer to omit the initialization
> > in such scenarios if we make sure it will initialized correctly？
>
> I don't know that we have a particular rule here, but I would say that
> yes, if you know the initialization isn't used, then skip it. That
> communicates the expectation to anybody reading the code. And if you're
> wrong, the compiler will generally notice.

Thanks for the explanation!

Re: [PATCH v4 0/4] pack-bitmap.c: avoid exposing absolute paths

[Jeff King]

On Mon, Nov 21, 2022 at 08:16:11PM +0800, Teng Long wrote:

> Diff since v3:
> 
> I picked Peff's two new constructive patches[1] which I think are meaningful.
> Peff also mentioned whether to design a "GIT_TRACE2_BITMAP" ENV to help
> debugging bitmap, and I think it's very interesting but maybe it's a bigger
> scale on current patchset, so I prefer to do it in another series.

I think the first two are already in 'next', so they aren't eligible for
re-roll, though from the range diff it looks like those changes are
already there?

-Peff

Re: [PATCH v4 0/4] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

Jeff King <peff@peff.net> writes:

> > Diff since v3:
> >
> > I picked Peff's two new constructive patches[1] which I think are meaningful.
> > Peff also mentioned whether to design a "GIT_TRACE2_BITMAP" ENV to help
> > debugging bitmap, and I think it's very interesting but maybe it's a bigger
> > scale on current patchset, so I prefer to do it in another series.
>
> I think the first two are already in 'next', so they aren't eligible for
> re-roll, though from the range diff it looks like those changes are
> already there?

Oops, I created the wrong range-diff. I didn't check the 'next', but now
I think I shall make the reroll here because the new replies from Junio
and you.

Thanks.

[PATCH v5 0/4] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

From: Teng Long <dyroneteng@gmail.com>

Changes since v4:

* [3/4] Reroll the commit message.
* [4/4] Reroll the commit message and remove the initialization of 'found'.

Besides of 3/4 and 4/4, the 1/4 and 2/4 seem already in 'next' as
8ddc06631b184e6f43051f7ea8ac9bbc86817127 and
2aa84d5f3ea1966a81477ad31bee0136e39d3917 , but I send again here just for
the completeness of the patchset, I think it will be nice to merge Peff's
commits of there are no new suggestions.

Thanks.

Jeff King (2):
  pack-bitmap.c: break out of the bitmap loop early if not tracing
  pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found

Teng Long (2):
  pack-bitmap.c: remove unnecessary "open_pack_index()" calls
  pack-bitmap.c: avoid exposing absolute paths

 pack-bitmap.c           | 39 ++++++++++++++++++++++++++-------------
 t/t5310-pack-bitmaps.sh |  5 +++--
 2 files changed, 29 insertions(+), 15 deletions(-)

Range-diff against v4:
-:  ---------- > 1:  aaeb021538 pack-bitmap.c: remove unnecessary "open_pack_index()" calls
-:  ---------- > 2:  9d5a491887 pack-bitmap.c: avoid exposing absolute paths
1:  f6c5b45bdc ! 3:  22deec6aab pack-bitmap.c: break out of the bitmap loop early if not tracing
    @@ Metadata
      ## Commit message ##
         pack-bitmap.c: break out of the bitmap loop early if not tracing
     
    -    When we successfully open a bitmap, we will continue to try to open
    -    other packs, and when trace2 is enabled, we will report any subsequent
    -    bitmap ignored information in the log. So when we find that trace2 is
    -    not enabled, we can actually terminate the loop early.
    +    After opening a bitmap successfully, we try opening others only
    +    because we want to report that other bitmap files are ignored in
    +    the trace2 log.  When trace2 is not enabled, we do not have to
    +    do any of that.
     
         Signed-off-by: Jeff King <peff@peff.net>
         Signed-off-by: Teng Long <dyroneteng@gmail.com>
2:  2acaa3f097 ! 4:  28306b7e8d pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found
    @@ Metadata
      ## Commit message ##
         pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found
     
    -    We retained pack bitmaps as a quick recovery mechanism while
    -    test-deploying midx bitmaps. This is an internal mechanism, and we
    -    want to expose this rule externally through trace2 so that end users,
    -    repo-maintainers, and debuggers know what is happening in the process.
    +    When we find a midx bitmap, we do not bother checking for pack
    +    bitmaps, since we can use only one. But since we will warn of unused
    +    bitmaps via trace2, let's continue looking for pack bitmaps when
    +    tracing is enabled.
     
         Signed-off-by: Jeff King <peff@peff.net>
         Signed-off-by: Teng Long <dyroneteng@gmail.com>
    @@ pack-bitmap.c: static int open_midx_bitmap(struct repository *r,
      static int open_bitmap(struct repository *r,
      		       struct bitmap_index *bitmap_git)
      {
    -+	int found = 0;
    ++	int found;
     +
      	assert(!bitmap_git->map);
      
-- 
2.38.1.385.g28306b7e8d5

[PATCH v5 1/4] pack-bitmap.c: remove unnecessary "open_pack_index()" calls

[Teng Long]

From: Teng Long <dyroneteng@gmail.com>

When trying to open a pack bitmap, we call open_pack_bitmap_1() in a
loop, during which it tries to open up the pack index corresponding
with each available pack.

It's likely that we'll end up relying on objects in that pack later
in the process (in which case we're doing the work of opening the
pack index optimistically), but not guaranteed.

For instance, consider a repository with a large number of small
packs, and one large pack with a bitmap. If we see that bitmap pack
last in our loop which calls open_pack_bitmap_1(), the current code
will have opened *all* pack index files in the repository. If the
request can be served out of the bitmapped pack alone, then the time
spent opening these idx files was wasted.S

Since open_pack_bitmap_1() calls is_pack_valid() later on (which in
turns calls open_pack_index() itself), we can just drop the earlier
call altogether.

Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 440407f1be..982e286bac 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -411,9 +411,6 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git
 	struct stat st;
 	char *bitmap_name;
 
-	if (open_pack_index(packfile))
-		return -1;
-
 	bitmap_name = pack_bitmap_filename(packfile);
 	fd = git_open(bitmap_name);
 
-- 
2.38.1.385.g28306b7e8d5

[PATCH v5 2/4] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

From: Teng Long <dyroneteng@gmail.com>

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 <moweng.xx@antgroup.com>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c           | 10 ++++++----
 t/t5310-pack-bitmaps.sh |  5 +++--
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 982e286bac..aaa2d9a104 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -354,8 +354,8 @@ 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);
+		trace2_data_string("bitmap", the_repository,
+				   "ignoring extra midx bitmap file", buf.buf);
 		close(fd);
 		strbuf_release(&buf);
 		return -1;
@@ -429,8 +429,8 @@ 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);
+		trace2_data_string("bitmap", the_repository,
+				   "ignoring extra bitmap file", packfile->pack_name);
 		close(fd);
 		return -1;
 	}
@@ -455,6 +455,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..7d8dee41b0 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_EVENT=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&
+			grep "opened bitmap" trace2.txt &&
+			grep "ignoring extra bitmap" trace2.txt
 		)
 	'
 }
-- 
2.38.1.385.g28306b7e8d5

[PATCH v5 3/4] pack-bitmap.c: break out of the bitmap loop early if not tracing

[Teng Long]

From: Jeff King <peff@peff.net>

After opening a bitmap successfully, we try opening others only
because we want to report that other bitmap files are ignored in
the trace2 log.  When trace2 is not enabled, we do not have to
do any of that.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index aaa2d9a104..3b6c2f804a 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -527,8 +527,15 @@ static int open_pack_bitmap(struct repository *r,
 	assert(!bitmap_git->map);
 
 	for (p = get_all_packs(r); p; p = p->next) {
-		if (open_pack_bitmap_1(bitmap_git, p) == 0)
+		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
 			ret = 0;
+			/*
+			 * The only reason to keep looking is to report
+			 * duplicates.
+			 */
+			if (!trace2_is_enabled())
+				break;
+		}
 	}
 
 	return ret;
-- 
2.38.1.385.g28306b7e8d5

Re: [PATCH v5 3/4] pack-bitmap.c: break out of the bitmap loop early if not tracing

[Taylor Blau]

On Mon, Nov 28, 2022 at 10:09:52PM +0800, Teng Long wrote:
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index aaa2d9a104..3b6c2f804a 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -527,8 +527,15 @@ static int open_pack_bitmap(struct repository *r,
>  	assert(!bitmap_git->map);
>
>  	for (p = get_all_packs(r); p; p = p->next) {
> -		if (open_pack_bitmap_1(bitmap_git, p) == 0)
> +		if (open_pack_bitmap_1(bitmap_git, p) == 0) {

While we're here, we *could* change this line to read:

    if (!open_pack_bitmap_1(bitmap_git, p))

which more closely adheres to our conventions. But I don't think just
because we could do something necessarily means that we have to, so I'm
happy to leave it alone, too. It definitely does not merit a reroll on
its own.

Otherwise this patch looks quite reasonable.

Thanks,
Taylor

Re: [PATCH v5 3/4] pack-bitmap.c: break out of the bitmap loop early if not tracing

[Teng Long]

Taylor Blau <me@ttaylorr.com> writes:

> > diff --git a/pack-bitmap.c b/pack-bitmap.c
> > index aaa2d9a104..3b6c2f804a 100644
> > --- a/pack-bitmap.c
> > +++ b/pack-bitmap.c
> > @@ -527,8 +527,15 @@ static int open_pack_bitmap(struct repository *r,
> >  	assert(!bitmap_git->map);
> >
> >  	for (p = get_all_packs(r); p; p = p->next) {
> > -		if (open_pack_bitmap_1(bitmap_git, p) == 0)
> > +		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
>
> While we're here, we *could* change this line to read:
>
>     if (!open_pack_bitmap_1(bitmap_git, p))

Feeling a bit sad why I didn't notice here.

> which more closely adheres to our conventions. But I don't think just
> because we could do something necessarily means that we have to, so I'm
> happy to leave it alone, too. It definitely does not merit a reroll on
> its own.
>
> Otherwise this patch looks quite reasonable.

That's a good catch and thanks for your suggestion.

[PATCH v5 4/4] pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found

[Teng Long]

From: Jeff King <peff@peff.net>

When we find a midx bitmap, we do not bother checking for pack
bitmaps, since we can use only one. But since we will warn of unused
bitmaps via trace2, let's continue looking for pack bitmaps when
tracing is enabled.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Teng Long <dyroneteng@gmail.com>
---
 pack-bitmap.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 3b6c2f804a..d2a42abf28 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -524,8 +524,6 @@ static int open_pack_bitmap(struct repository *r,
 	struct packed_git *p;
 	int ret = -1;
 
-	assert(!bitmap_git->map);
-
 	for (p = get_all_packs(r); p; p = p->next) {
 		if (open_pack_bitmap_1(bitmap_git, p) == 0) {
 			ret = 0;
@@ -559,11 +557,20 @@ static int open_midx_bitmap(struct repository *r,
 static int open_bitmap(struct repository *r,
 		       struct bitmap_index *bitmap_git)
 {
+	int found;
+
 	assert(!bitmap_git->map);
 
-	if (!open_midx_bitmap(r, bitmap_git))
-		return 0;
-	return open_pack_bitmap(r, bitmap_git);
+	found = !open_midx_bitmap(r, bitmap_git);
+
+	/*
+	 * these will all be skipped if we opened a midx bitmap; but run it
+	 * anyway if tracing is enabled to report the duplicates
+	 */
+	if (!found || trace2_is_enabled())
+		found |= !open_pack_bitmap(r, bitmap_git);
+
+	return found ? 0 : -1;
 }
 
 struct bitmap_index *prepare_bitmap_git(struct repository *r)
-- 
2.38.1.385.g28306b7e8d5

Re: [PATCH v5 0/4] pack-bitmap.c: avoid exposing absolute paths

[Taylor Blau]

On Mon, Nov 28, 2022 at 10:09:49PM +0800, Teng Long wrote:
> From: Teng Long <dyroneteng@gmail.com>
>
> Changes since v4:
>
> * [3/4] Reroll the commit message.
> * [4/4] Reroll the commit message and remove the initialization of 'found'.
>
> Besides of 3/4 and 4/4, the 1/4 and 2/4 seem already in 'next' as
> 8ddc06631b184e6f43051f7ea8ac9bbc86817127 and
> 2aa84d5f3ea1966a81477ad31bee0136e39d3917 , but I send again here just for
> the completeness of the patchset, I think it will be nice to merge Peff's
> commits of there are no new suggestions.

As you note, the first couple of patches are already in 'next', so those
do not need to be queued again.

But the latter two patches look good, and are ready to go.

Thanks for continuing to work on this :-)..

Thanks,
Taylor

Re: [PATCH v5 0/4] pack-bitmap.c: avoid exposing absolute paths

[Teng Long]

Taylor Blau <me@ttaylorr.com> writes:

> > Besides of 3/4 and 4/4, the 1/4 and 2/4 seem already in 'next' as
> > 8ddc06631b184e6f43051f7ea8ac9bbc86817127 and
> > 2aa84d5f3ea1966a81477ad31bee0136e39d3917 , but I send again here just for
> > the completeness of the patchset, I think it will be nice to merge Peff's
> > commits of there are no new suggestions.
>
> As you note, the first couple of patches are already in 'next', so those
> do not need to be queued again.
>
> But the latter two patches look good, and are ready to go.
>
> Thanks for continuing to work on this :-)..

Thank you for your help, Taylor Blau.