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

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 26, 2022, 21:42 UTC
Message-ID: <Y1mp23NHB0qzKsPR@nand.local>
In-Reply-To: <20220829024803.47496-1-tenglong.tl@alibaba-inc.com>

On Mon, Aug 29, 2022 at 10:48:03AM +0800, Teng Long wrote:

Show 13 quoted lines

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