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

From: Jeff King <peff@peff.net>
Date: Nov 17, 2022, 15:03 UTC
Message-ID: <Y3ZNKhr3kMZLmQnh@coredump.intra.peff.net>
In-Reply-To: <20221117141933.73790-1-tenglong.tl@alibaba-inc.com>

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

Show 10 quoted lines

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