On Tue, Mar 04, 2025 at 02:51:13AM -0500, Jeff King wrote:
> On Fri, Feb 28, 2025 at 10:32:01AM +0800, SURA wrote:
>
> > My previous description was not clear enough. The early hiding
> > according to exclude_patterns in packed_ref_iterator_begin seems to be
> > designed for git for-each-ref's exclude. It is different from the
> > ref_hidden matching rule used by upload-pack.
>
> >From your reproduction, it looks like the issue is that for loose refs,
> asking for_each_ref() to exclude "refs/heads/foo" will not yield
> "refs/heads/foo/bar", but will yield "refs/heads/foo-bar".
>
> And that was true for packed-refs, too, before 59c35fac54
> (refs/packed-backend.c: implement jump lists to avoid excluded
> pattern(s), 2023-07-10). After that, packed-refs exclude both.
Thanks for the careful analysis. Since you and I co-wrote this feature
in the first place, naturally I agree with what you wrote here ;-).
> So probably the solution is for the jump list in 59c35fac54 to be
> pickier about finding its start/end points. It should insist on a
> trailing "/" (I think end-of-string would also be valid, but it may be
> easier to ignore that, and it is OK to err on the side of inclusion,
> since the caller is supposed to do their own filtering).
>
> Probably the logic needs to go into cmp_record_to_refname(), but I lack
> sufficient brain power at this time of night to even attempt a fix.
That is definitely one way to fix the issue, and the fix would look
something like the following:
--- 8< ---
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index a7b6f74b6e..b137641f9d 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -326,7 +326,8 @@ static int cmp_packed_ref_records(const void *v1, const void *v2,
* refname.
*/
static int cmp_record_to_refname(const char *rec, const char *refname,
- int start, const struct snapshot *snapshot)
+ int start, int strict,
+ const struct snapshot *snapshot)
{
const char *r1 = rec + snapshot_hexsz(snapshot) + 1;
const char *r2 = refname;
@@ -334,8 +335,11 @@ static int cmp_record_to_refname(const char *rec, const char *refname,
while (1) {
if (*r1 == '\n')
return *r2 ? -1 : 0;
- if (!*r2)
+ if (!*r2) {
+ if (strict && *r1 != '/')
+ return 1;
return start ? 1 : -1;
+ }
if (*r1 != *r2)
return (unsigned char)*r1 < (unsigned char)*r2 ? -1 : +1;
r1++;
--- >8 ---
I'm eliding some plumbing here to pass the "strict" flag through the
callers eventually all the way down to cmp_record_to_refname().
But I think this is equivalent to pretending like the excluded patterns
all end in a '/' character (if they weren't already like that to begin
with). So equivalently, you could do something like:
--- 8< ---
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index a7b6f74b6e..e4569519a1 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1024,6 +1024,7 @@ static void populate_excluded_jump_list(struct packed_ref_iterator *iter,
size_t i, j;
const char **pattern;
struct jump_list_entry *last_disjoint;
+ struct strbuf buf = STRBUF_INIT;
if (!excluded_patterns)
return;
@@ -1043,8 +1044,13 @@ static void populate_excluded_jump_list(struct packed_ref_iterator *iter,
if (has_glob_special(*pattern))
continue;
- start = find_reference_location(snapshot, *pattern, 0);
- end = find_reference_location_end(snapshot, *pattern, 0);
+ strbuf_reset(&buf);
+ strbuf_addstr(&buf, *pattern);
+ if (buf.len && buf.buf[buf.len - 1] != '/')
+ strbuf_addch(&buf, '/');
+
+ start = find_reference_location(snapshot, buf.buf, 0);
+ end = find_reference_location_end(snapshot, buf.buf, 0);
if (start == end)
continue; /* nothing to jump over */
@@ -1061,7 +1067,7 @@ static void populate_excluded_jump_list(struct packed_ref_iterator *iter,
* Every entry in exclude_patterns has a meta-character,
* nothing to do here.
*/
- return;
+ goto out;
}
QSORT(iter->jump, iter->jump_nr, jump_list_entry_cmp);
@@ -1095,6 +1101,9 @@ static void populate_excluded_jump_list(struct packed_ref_iterator *iter,
iter->jump_nr = j;
iter->jump_cur = 0;
+
+out:
+ strbuf_release(&buf);
}
static struct ref_iterator *packed_ref_iterator_begin(
--- >8 ---
But then we have to handle the reftable case too, which Patrick gave a
potential fix to below. But equally fine I think would be to push this
^^ logic up into refs.c::refs_ref_iterator_begin(), which would fix both
at the same time.
> The smallest reproduction for me is:
>
> git init
> git commit --allow-empty -m foo
> git pack-refs --all
> git -c transfer.hiderefs=refs/he upload-pack .
>
> which shows "refs/heads/main" (or "master") before 59c35fac54, but not
> after.
Thanks, this was a very clean reproduction that made it much easier to
diagnose what was going on here ;-).
Thanks,
Taylor
