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

From: Taylor Blau <me@ttaylorr.com>
Date: Feb 28, 2025, 00:12 UTC
Message-ID: <Z8D/aiqN5e/aRSn7@nand.local>
In-Reply-To: <CAD6AYr-ZC32VNfUfMB63H-rQRfTdV=VQfBm67i2mG+6GDCNxkQ@mail.gmail.com>

(There's a parallel discussion going on in [1], so it isn't entirely clear which thread to respond to. Since it seems a little premature to comment on the patch itself, I'll respond here.)

On Thu, Feb 27, 2025 at 03:24:07PM +0800, SURA wrote:

Show 7 quoted lines

> I found that packed refs are excluded by the transfer.hideRefs front
> match, while loose refs use full match (when transfer.hideRefs ends
> with '/', it is prefix match, which is normal)
>
> When the server uses git, after setting transfer.hideRefs, the
> references that the client can see before and after server repo gc are
> different

It's true that the low-level loose references iterator does not know how to handle excluded patterns, and that is by design. In the packed-refs case, we can skip over whole sections of the packed-refs file according to which patterns are excluded.

But in the loose references case, we haven't implemented anything like that to skip over, e.g. enumerating the contents of "$GIT_DIR/refs/heads/foo" when "refs/heads/foo/" is excluded. (As an aside, this is something that we could do, it just hasn't been implemented yet).

So in practice today the only way to exclude loose references according to some set of exclusion patterns would be to filter them out as we iterate over them. But that is the caller's responsibility, as we see in a handful of comments in refs.h which say "any pattern in 'exclude_patterns' [is] omitted on a best-effort basis".

So upload-pack / etc. will see all loose references, and it filters out the ones which it's supposed to hide via:

    upload_pack() -> for_each_namespaced_ref_1() -> send_ref() ->
    write_v0_ref() -> mark_our_ref() -> ref_is_hidden()

, where mark_our_ref() tosses out references that the low-level refs iterator gave back to it which match one of the excluded patterns.

And there we have ref_is_hidden() doing the following for each hidden pattern:

    if (subject &&
        skip_prefix(subject, match, &p) &&
        (!*p || *p == '/'))
     return !neg;

So if the reference either matches the pattern exactly, or matches up to a '/', then it is hidden and thus not advertised.

I have to imagine I'm missing something, but perhaps it would be useful if you could provide a reproduction script that demonstrates what you're seeing.

> It seems that 59c35fa accidentally damaged upload-pack when optimizing
> git for-each-ref

No. 59c35fac54 (refs/packed-backend.c: implement jump lists to avoid excluded pattern(s), 2023-07-10) predates any behavior changes in upload-pack, which were introduced later on in 18b6b1b5c5 (upload-pack.c: avoid enumerating hidden refs where possible, 2023-07-10).

Thanks, Taylor

[1]: https://lore.kernel.org/git/MA0P287MB06412DF70BCDA0D99641129FE4CD2@MA0P287MB0641.INDP287.PROD.OUTLOOK.COM/T/#t