Re: The transfer.hideRefs of the upload-pack process does not work properly
From: Taylor Blau <hidden>
Date: 2025-03-05 23:12:57
Subsystem:
the rest · Maintainer:
Linus Torvalds
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:quoted
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.quoted
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