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

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

[SURA]

Hello everyone

OS: Linux Mint 22
git version: v2.48.1

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 seems that 59c35fa accidentally damaged upload-pack when optimizing
git for-each-ref

It seems that there is no simple fix except rolling back this commit?

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

[Taylor Blau]

(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:
> 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

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

[SURA]

Taylor Blau <me@ttaylorr.com> 于2025年2月28日周五 08:12写道：
>
> (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.)

So sorry, I used another email address to submit the patch, it is
indeed a bit early

> On Thu, Feb 27, 2025 at 03:24:07PM +0800, SURA wrote:
> > 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

I shouldn't have mentioned the '/' suffix, it's confusing

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.

I provide a reproducible step to make it clear

------

# create git repo
$ mkdir sura-repo && cd sura-repo
$ git init

# create one commit
$ echo "hello" > file-001
$ git add . && git commit -m "init repo"

# create some refs
$ git checkout -b sura
$ git checkout -b sura-001
$ git checkout -b sura-002
$ git checkout -b sura-003

# show refs
$ git for-each-ref
d0205e0d0a0a7a6d1a712afb3734ad3e88eeda1c commit refs/heads/master
d0205e0d0a0a7a6d1a712afb3734ad3e88eeda1c commit refs/heads/sura
d0205e0d0a0a7a6d1a712afb3734ad3e88eeda1c commit refs/heads/sura-001
d0205e0d0a0a7a6d1a712afb3734ad3e88eeda1c commit refs/heads/sura-002
d0205e0d0a0a7a6d1a712afb3734ad3e88eeda1c commit refs/heads/sura-003

# upload-pack, normal, hide 'refs/heads/sura'
$ git -c transfer.hiderefs=refs/heads/sura upload-pack .git
0103d0205e0d0a0a7a6d1a712afb3734ad3e88eeda1c HEADmulti_ack thin-pack
side-band side-band-64k ofs-delta shallow deepen-since deepen-not
deepen-relative no-progress include-tag multi_ack_detailed
symref=HEAD:refs/heads/sura object-format=sha1 agent=git/2.46.0
003fd0205e0d0a0a7a6d1a712afb3734ad3e88eeda1c refs/heads/master
0041d0205e0d0a0a7a6d1a712afb3734ad3e88eeda1c refs/heads/sura-001
0041d0205e0d0a0a7a6d1a712afb3734ad3e88eeda1c refs/heads/sura-002
0041d0205e0d0a0a7a6d1a712afb3734ad3e88eeda1c refs/heads/sura-003
0000

# gc make loose refs to packed refs
$ git gc

# then upload-pack
$ git -c transfer.hiderefs=refs/heads/sura upload-pack .git
0103d0205e0d0a0a7a6d1a712afb3734ad3e88eeda1c HEADmulti_ack thin-pack
side-band side-band-64k ofs-delta shallow deepen-since deepen-not
deepen-relative no-progress include-tag multi_ack_detailed
symref=HEAD:refs/heads/sura object-format=sha1 agent=git/2.46.0
003fd0205e0d0a0a7a6d1a712afb3734ad3e88eeda1c refs/heads/master
0000

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

[Jeff King]

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.

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.

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.

-Peff

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

[Jeff King]

On Tue, Mar 04, 2025 at 02:51:14AM -0500, Jeff King wrote:

> 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.

Oh, and of course it would be interesting to know how reftables behave
here, too, as I think they recently learned about exclusions.

-Peff

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

[Patrick Steinhardt]

On Tue, Mar 04, 2025 at 02:51:46AM -0500, Jeff King wrote:
> On Tue, Mar 04, 2025 at 02:51:14AM -0500, Jeff King wrote:
> 
> > 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.
> 
> Oh, and of course it would be interesting to know how reftables behave
> here, too, as I think they recently learned about exclusions.

Well, we have tests that explicitly verify that prefixes cause us to
exclude such refs in t1419, "overlapping exclude regions". So the
reftable backend is bug-compatible :)

Guess we'll need something similar to the following:

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 7e90e13f745..45462f2ce6d 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -594,6 +594,16 @@ static int should_exclude_current_ref(struct reftable_ref_iterator *iter)
 		if (cmp < 0)
 			return 0;
 
+		/*
+		 * The exclude pattern needs to either match exactly, or, if it
+		 * is a prefix of the given reference, it must be a containing
+		 * directory.
+		 */
+		if (iter->ref.refname[iter->exclude_patterns_strlen] &&
+		    pattern[iter->exclude_patterns_strlen - 1] != '/' &&
+		    iter->ref.refname[iter->exclude_patterns_strlen] != '/')
+			return 0;
+
 		/*
 		 * The reference shares a prefix with the exclude pattern and
 		 * shall thus be omitted. We skip all references that match the

Patrick

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

[Taylor Blau]

On Tue, Mar 04, 2025 at 12:38:50PM +0100, Patrick Steinhardt wrote:
> On Tue, Mar 04, 2025 at 02:51:46AM -0500, Jeff King wrote:
> > On Tue, Mar 04, 2025 at 02:51:14AM -0500, Jeff King wrote:
> >
> > > 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.
> >
> > Oh, and of course it would be interesting to know how reftables behave
> > here, too, as I think they recently learned about exclusions.
>
> Well, we have tests that explicitly verify that prefixes cause us to
> exclude such refs in t1419, "overlapping exclude regions". So the
> reftable backend is bug-compatible :)
>
> Guess we'll need something similar to the following:
>
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 7e90e13f745..45462f2ce6d 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -594,6 +594,16 @@ static int should_exclude_current_ref(struct reftable_ref_iterator *iter)
>  		if (cmp < 0)
>  			return 0;
>
> +		/*
> +		 * The exclude pattern needs to either match exactly, or, if it
> +		 * is a prefix of the given reference, it must be a containing
> +		 * directory.
> +		 */
> +		if (iter->ref.refname[iter->exclude_patterns_strlen] &&
> +		    pattern[iter->exclude_patterns_strlen - 1] != '/' &&
> +		    iter->ref.refname[iter->exclude_patterns_strlen] != '/')
> +			return 0;
> +
>  		/*
>  		 * The reference shares a prefix with the exclude pattern and
>  		 * shall thus be omitted. We skip all references that match the

Ah, makes sense. Thanks, both, for looking a little deeper. I can work
on a fix shortly...

Thanks,
Taylor

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

[Taylor Blau]

On Tue, Mar 04, 2025 at 11:40:17AM -0500, Taylor Blau wrote:
> Ah, makes sense. Thanks, both, for looking a little deeper. I can work
> on a fix shortly...

This took me a little longer than I had hoped, but I posted the patches
here:

    https://lore.kernel.org/git/cover.1741223981.git.me@ttaylorr.com/

Thanks,
Taylor

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

[Taylor Blau]

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

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> 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.

Somehow, today seems to be the day to read about "let's push as much
common things up from ref backends to the generic ref layer " for me
;-).