Re: The transfer.hideRefs of the upload-pack process does not work properly

[Taylor Blau <me@ttaylorr.com>]

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