ref-in-want does not consider namespace

ref-in-want does not consider namespace

[Kim Altintop]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]

Hello,

I am experimenting with the ref-in-want feature using a custom client and plain
`git-upload-pack` on the server end. To my suprise, it turned out that fetching
from a namespaced repository requires the `want-ref` lines to specify the refs
in namespaced form. That is:

Say I have the ref `refs/namespaces/foo/refs/heads/main`. I launch upload-pack
with `--namespace=foo`. Running `ls-refs` with a namespace-relative

  `ref-prefix refs/heads/*`

will work as expected, but sendingr

  `want-ref refs/heads/main`

will result in

  `ERR unknown ref refs/heads/main`

Sending

  `want-ref refs/namespaces/foo/refs/heads/main`

works, and the corresponding `wanted-refs` section will contain the ref as given
(ie. including the namespace path).

Since my understanding of git namespaces is that they should be transparent to
the client, I find this surprising.

I dug up an early version of a patch introducing the feature, which did handle
namespaces [0], but what eventually landed (authored by Brandon Williams)
didn't.


Was this an oversight, or is there a specific reason for the behaviour?


Thanks,
Kim



[0]: https://lore.kernel.org/git/d0d42b3bb4cf755f122591e191354c53848f197d.1485381677.git.jonathantanmy@google.com/

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]

Re: ref-in-want does not consider namespace

[Kim Altintop]

> Running `ls-refs` with a namespace-relative
>
> `ref-prefix refs/heads/*`
>
> will work as expected

That turned out to not be entirely true: it does work as expected with a 2.31.1 install, but fails to select any refs with 2.20.1.

Re: ref-in-want does not consider namespace

[Jonathan Tan]

> > Running `ls-refs` with a namespace-relative
> >
> > `ref-prefix refs/heads/*`
> >
> > will work as expected
> 
> That turned out to not be entirely true: it does work as expected with a
> 2.31.1 install, but fails to select any refs with 2.20.1.

Your expectation that "want-ref refs/heads/main" should match
"refs/namespaces/foo/refs/heads/main" when you run upload-pack with
"--namespace=foo" makes sense to me. Just to clarify - does this mean
tha 2.31.1 works like you expect?

In any case, t5703 should contain a namespace test, but it doesn't seem
to. We could just use the one in my original patch [1] (thanks for
looking it up, by the way). Would you be able to contribute a patch? I
see that in [1], I had my own helpers to construct the input, but
"test-tool pkt-line" now exists (as you can see in t5703) so you can do
that instead.

[1] https://lore.kernel.org/git/d0d42b3bb4cf755f122591e191354c53848f197d.1485381677.git.jonathantanmy@google.com/

Re: ref-in-want does not consider namespace

[Kim Altintop]

Thanks Jonathan

> Your expectation that "want-ref refs/heads/main" should match
> "refs/namespaces/foo/refs/heads/main" when you run upload-pack with
> "--namespace=foo" makes sense to me. Just to clarify - does this mean
> tha 2.31.1 works like you expect?

"ref-prefix" works as expected in the newer version, but not "want-ref". I
haven't properly bisected, but the fix seems to coincide with b3970c7, although
I'm not sure if that was intentional.

> In any case, t5703 should contain a namespace test, but it doesn't seem
> to.

Yes I noticed that your original patch did contain a test for this.

> Would you be able to contribute a patch?

With some handholding, I'd be at least up for giving it a spin :)

One thing that is unclear to me is how "hideRefs" is supposed to interact with
"want-ref". At the moment, it seems like a client can "want-ref" _any_ ref, even
if it is hidden by "hideRefs". It might seem sensible to ignore hidden refs, but
then the situation could occur that the effective list of "want-ref"s is empty.

If that happens, and the client did not send any "want"s either, "upload-pack"
ignores the fetch command altogether and just waits for the next command. In
this case, the client can only recover by timing out the connection.

So should this be special-cased somehow and result in an error response?