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

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 30, 2022, 18:42 UTC
Message-ID: <Y17FeGeWqEE7CWQS@nand.local>
In-Reply-To: <Y1rt+uOKwlP5PIrT@coredump.intra.peff.net>

On Thu, Oct 27, 2022 at 04:45:46PM -0400, Jeff King wrote:

Show 11 quoted lines

> 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. ;-)

Show 12 quoted lines

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

Show 12 quoted lines

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