[PATCH v2 0/4] Fix misuse of `refs_for_each_ref_in()`

Hi,

this small patch series fixes a bug I have discovered where configuring "pack.preferBitmapTips" to an exact branch will cause Git to `BUG()`.

The root cause of this bug is misuse of `refs_for_each_ref_in()`: this function accepts a prefix to yield refs for, and then strips the prefix for each ref. Consequently, if passed an exact refname, then stripping the prefix would make us end up with an empty refname, and that is not supposed to happen.

There was one other caller that got it wrong, too, and which is also fixed in this patch series.

Changes in v2:
  - Explain my thought process against why I chose to also allow exact
    ref matches in the second commit and clarify the documentation a
    bit. As said, I'm very open to changing this if my spelled-out
    thoughts are not convincing.
  - Apply Peff's patch to further simplify code.
  - Link to v1: https://lore.kernel.org/r/20260128-b4-pks-fix-for-each-ref-in-misuse-v1-0-deccae3ea725@pks.im

Thanks!

Patrick

---
Jeff King (1):
      bisect: simplify string_list memory handling

Patrick Steinhardt (3):
      pack-bitmap: deduplicate logic to iterate over preferred bitmap tips
      pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"
      bisect: fix misuse of `refs_for_each_ref_in()`

 Documentation/config/pack.adoc |  7 +++----
 bisect.c                       | 16 +++++++---------
 builtin/pack-objects.c         | 19 ++-----------------
 pack-bitmap.c                  | 18 +++++++++++++++++-
 pack-bitmap.h                  |  9 ++++++++-
 repack-midx.c                  | 14 +++-----------
 t/t5310-pack-bitmaps.sh        | 35 +++++++++++++++++++++++++++++++++++
 t/t5319-multi-pack-index.sh    | 36 ++++++++++++++++++++++++++++++++++++
 8 files changed, 111 insertions(+), 43 deletions(-)

Range-diff versus v1:

1:  dcdc4a5aa2 ! 1:  2a2558bb22 pack-bitmap: deduplicate logic to iterate over preferred bitmap tips
    @@ Commit message
     
         We have two locations that iterate over the preferred bitmap tips as
         configured by the user via "pack.preferBitmapTips". Both of these
    -    callsites are subtly wrong and can lead to a `BUG()`, which we'll fix in
    -    a subsequent commit.
    +    callsites are subtly wrong: when the preferred bitmap tips contain an
    +    exact refname match, then we will hit a `BUG()`.
     
    -    Prepare for this fix by unifying the two callsites into a new
    +    Prepare for the fix by unifying the two callsites into a new
         `for_each_preferred_bitmap_tip()` function.
     
         This removes the last callsite of `bitmap_preferred_tips()` outside of
         "pack-bitmap.c". As such, convert the function to be local to that file
    -    only.
    +    only. Note that the function is still used by a second caller, so we
    +    cannot just inline it.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
2:  5b14a8a680 ! 2:  5b4a33a0bd pack-bitmap: fix bug with exact ref match in "pack.preferBitmapTips"
    @@ Commit message
             lead to a somewhat weird result.
     
         Neither of these feel like viable options, so calling `BUG()` feels like
    -    a sensible way out.
    -
    -    The root cause really is that we try to trim the whole refname. We can
    -    thus easily fix the bug itself by calling `refs_for_each_fullref_in()`
    -    instead. This function behaves the same as `refs_for_each_ref_in()`,
    -    except that it doesn't strip the prefix. Consequently, it correctly
    -    yields also exact refnames.
    -
    -    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.
    +    a sensible way out. The root cause ultimately is that we even try to
    +    trim the whole refname in the first place. There are two possible ways
    +    to fix this issue:
    +
    +      - We can fix the bug by using `refs_for_each_fullref_in()` instead,
    +        which does not strip the prefix at all. Consequently, we would now
    +        start to accept all references that start with the configured
    +        prefix, including exact matches. So if we had "refs/heads/main", we
    +        would both match "refs/heads/main" and "refs/heads/main-branch".
    +
    +      - Or we can fix the bug by appending a slash to the prefix if it
    +        doesn't already have one. This would mean that we only match
    +        ref hierarchies that start with this prefix.
    +
    +    The first fix leaves the user with strictly _more_ configuration
    +    options: they can have prefix matches by not appending a slash to the
    +    configuration, and they can have ref hierarchy matches by appending one.
    +
    +    Apply this fix and clarify the documentation accordingly.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    + ## Documentation/config/pack.adoc ##
    +@@ Documentation/config/pack.adoc: pack.usePathWalk::
    + 
    + pack.preferBitmapTips::
    + 	When selecting which commits will receive bitmaps, prefer a
    +-	commit at the tip of any reference that is a suffix of any value
    +-	of this configuration over any other commits in the "selection
    +-	window".
    ++	commmit at the tip of a reference that matches any of the
    ++	configured prefixes.
    + +
    +-Note that setting this configuration to `refs/foo` does not mean that
    ++Note that setting this configuration to `refs/foo/` does not mean that
    + the commits at the tips of `refs/foo/bar` and `refs/foo/baz` will
    + necessarily be selected. This is because commits are selected for
    + bitmaps from within a series of windows of variable length.
    +
      ## pack-bitmap.c ##
     @@ pack-bitmap.c: void for_each_preferred_bitmap_tip(struct repository *repo,
      		return;
3:  2e1bfb0894 = 3:  1579608b04 bisect: fix misuse of `refs_for_each_ref_in()`
-:  ---------- > 4:  99a80bdb37 bisect: simplify string_list memory handling

--- base-commit: ea717645d199f6f1b66058886475db3e8c9330e9 change-id: 20260128-b4-pks-fix-for-each-ref-in-misuse-96ae62e313a5