From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 30 Jan 2026 12:58:23 GMT
Subject: Re: [PATCH 2/3] pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"
Message-ID: <aXyq7_yo2tVv7y38@pks.im>
In-Reply-To: <aXrGfGUJQ34JAmuz@nand.local>

On Wed, Jan 28, 2026 at 09:31:24PM -0500, Taylor Blau wrote:
> On Wed, Jan 28, 2026 at 09:49:21AM +0100, Patrick Steinhardt wrote:
> > The "pack.preferBitmapTips" configuration allows the user to specify
> > which references should be preferred when generating bitmaps. This
> > option is typically expected to be set to a reference prefix, like for
> > example "refs/heads/".
> >
> > It's not unreasonable though for a user to configure one specific
> > reference as preferred. But if they do, they'll hit a `BUG()`:
> >
> >     $ git -c pack.preferBitmapTips=refs/heads/main repack -adb
> >     BUG: ../refs/iterator.c:366: attempt to trim too many characters
> >     error: pack-objects died of signal 6
> 
> Oops. While we should definitely not BUG() here, I am not sure I
> understand the desired use-case of specifying a single reference as a
> value for pack.preferBitmapTips.

There is not really a desired use case here, I just happened to stumble
over this bug due to playing around with the feature.

> Looking at the implementation of bitmap_writer_select_commits(), we do
> not guarantee that *any* reference specified by pack.preferBitmapTips
> will receive a bitmap. That's because we don't necessarily enumerate the
> entire set of commits when determining which ones to bitmap.

Yeah, we don't indeed.

[snip]
> > One resulting weirdness is that two refs "refs/heads/base" and
> > "refs/heads/base-something" would now match if the user configured
> > "refs/heads/base" as bitmap tips. One could arguably change the
> > semantics of the configuration such that a string without a trailing
> > slash needs to be an exact reference match, whereas a string with a
> > trailing slash indicates a directory hierarchy. But such a change would
> > potentially cause regressions with dubious benefits, so this issue is
> > ignored for now.
> 
> (Setting aside the for_each_ref vs. for_each_fullref issue for a
> moment...)
> 
> Am I understanding this change correctly that doing something like -c
> pack.preferBitmapTips=refs/heads/foo would match both foo and foobar?
> 
> If so, I am not sure that that is a desirable interface, especially
> since we went the opposite direction in 10e8a9352bc (refs.c: stop
> matching non-directory prefixes in exclude patterns, 2025-03-06). Having
> the two behave inconsistently from one another feels somewhat awkward to
> me and may lead to unexpected results.

I don't have too much skin in the game, and as I wrote I agree that the
things are a bit nuanced here. I think there's two major ways to go from
here:

  - We can either fix the bug and say that we accept all references that
    start with the configured prefix. "refs/heads/main" _does_ start
    with the prefix "refs/heads/main", so it should match.

  - Or we can fix the bug by appending a slash to the configured prefix
    if it doesn't already have one.

The reason I picked the first option here is mostly because it allows
for more options rather than restricting options. The user has the
ability to both match hierarchies by appending a "/", and they can have
ref-prefix-matches by not doing so.

I'll adapt the commit message accordingly to document my thought
process, and...

> At the very least, if we do end up going in this direction (and I am not
> necessarily advocating that we do, since I would prefer a more
> consistent set of behavior), we should at minimum document it in
> git-config(1).

... will also adapt the documentation accordingly.

That being said, I'm also open to adapt my approach here.

Thanks!

Patrick