{"thread":{"id":"56182","subject":"[PATCH] upload-pack.c: treat want-ref relative to namespace","startedAt":"2021-07-30T13:59:26Z","lastAt":"2021-08-16T12:40:45Z","messageCount":35,"participants":["Kim Altintop","Junio C Hamano","Jonathan Tan","Eric Sunshine","Jonathan Nieder","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"431570","messageId":"20210730135845.633234-1-kim@eagain.st","threadId":"56182","inReplyTo":null,"subject":"[PATCH] upload-pack.c: treat want-ref relative to namespace","fromName":"Kim Altintop","fromEmail":"kim@eagain.st","sentAt":"2021-07-30T13:59:16Z","receivedAt":"2021-07-30T13:59:26Z","isPatch":true,"sender":{"key":"kim@eagain.st","avatar":"https://avatars.githubusercontent.com/u/6163?v=4"},"body":"When 'upload-pack' runs within the context of a git namespace, treat any\n'want-ref' lines the client sends as relative to that namespace.\n\nAlso check if the wanted ref is hidden via 'hideRefs', and respond with\nan error otherwise. It was previously possible to request any ref, but\nnote that this is still the case unless 'hideRefs' is in effect.\n\nSigned-off-by: Kim Altintop \n---\n\nPlease excuse my newbie ness.\n\n t/t5703-upload-pack-ref-in-want.sh | 77 ++++++++++++++++++++++++++++++\n upload-pack.c | 15 +++---\n 2 files changed, 86 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh\nindex e9e471621d..9fb16848bc 100755\n--- a/t/t5703-upload-pack-ref-in-want.sh\n+++ b/t/t5703-upload-pack-ref-in-want.sh\n@@ -298,6 +298,83 @@ test_expect_success 'fetching with wildcard that matches multiple refs' '\n \tgrep \"want-ref refs/heads/o/bar\" log\n '\n\n+REPO=\"$(pwd)/repo-ns\"\n+\n+test_expect_success 'setup namespaced repo' '\n+\t(\n+\t\tgit init -b main \"$REPO\" &&\n+\t\tcd \"$REPO\" &&\n+\t\ttest_commit a &&\n+\t\ttest_commit b &&\n+\t\tgit checkout a &&\n+\t\ttest_commit c &&\n+\t\tgit checkout a &&\n+\t\ttest_commit d &&\n+\t\tgit update-ref refs/heads/ns-no b &&\n+\t\tgit update-ref refs/namespaces/ns/refs/heads/ns-yes c &&\n+\t\tgit update-ref refs/namespaces/ns/refs/heads/hidden d\n+\t) &&\n+ git -C \"$REPO\" config uploadpack.allowRefInWant true &&\n+ git -C \"$REPO\" config transfer.hideRefs refs/heads/hidden\n+'\n+\n+test_expect_success 'want-ref with namespaces' '\n+\toid=$(git -C \"$REPO\" rev-parse c) &&\n+\tcat >expected_refs <<-EOF &&\n+\t$oid refs/heads/ns-yes\n+\tEOF\n+\t>expected_commits &&\n+\n+\toid=$(git -C \"$REPO\" rev-parse c) &&\n+\ttest-tool pkt-line pack >in <<-EOF &&\n+\t$(write_command fetch)\n+\t0001\n+\tno-progress\n+\twant-ref refs/heads/ns-yes\n+\thave $oid\n+\tdone\n+\t0000\n+\tEOF\n+\n+\tGIT_NAMESPACE=ns && export GIT_NAMESPACE &&\n+\ttest-tool -C \"$REPO\" serve-v2 --stateless-rpc >out in <<-EOF &&\n+\t$(write_command fetch)\n+\t0001\n+\tno-progress\n+\twant-ref refs/heads/ns-no\n+\thave $oid\n+\tdone\n+\t0000\n+\tEOF\n+\n+\tGIT_NAMESPACE=ns && export GIT_NAMESPACE &&\n+\ttest_must_fail test-tool -C \"$REPO\" serve-v2 --stateless-rpc >out in <<-EOF &&\n+\t$(write_command fetch)\n+\t0001\n+\tno-progress\n+\twant-ref refs/heads/hidden\n+\thave $oid\n+\tdone\n+\t0000\n+\tEOF\n+\n+\tGIT_NAMESPACE=ns && export GIT_NAMESPACE &&\n+\ttest_must_fail test-tool -C \"$REPO\" serve-v2 --stateless-rpc >out util = oiddup(&oid);\n\n-\t\to = parse_object_or_die(&oid, arg);\n+\t\to = parse_object_or_die(&oid, refname);\n \t\tif (!(o->flags & WANTED)) {\n \t\t\to->flags |= WANTED;\n \t\t\tadd_object_array(o, NULL, want_obj);\n--\n2.32.0\n\n\n"},{"id":"431571","messageId":"CD6J50083UWZ.QYE3HJQ3D3B4@schmidt","threadId":"56182","inReplyTo":"20210730135845.633234-1-kim@eagain.st","subject":"Re: [PATCH] upload-pack.c: treat want-ref relative to namespace","fromName":"Kim Altintop","fromEmail":"kim@eagain.st","sentAt":"2021-07-30T14:04:26Z","receivedAt":"2021-07-30T14:04:52Z","isPatch":true,"sender":{"key":"kim@eagain.st","avatar":"https://avatars.githubusercontent.com/u/6163?v=4"},"body":"> Please excuse my newbie ness.\n\nOops I'm sorry. `send-email` remembered this from a dry run.\n\nJonathan: I took the test code from your original patch introducing ref-in-want,\nbut modified it substantially. Let me know if it is conventional to credit you\nanyway, and by which trailer.\n\n"},{"id":"431597","messageId":"xmqqbl6j1vgh.fsf@gitster.g","threadId":"56182","inReplyTo":"20210730135845.633234-1-kim@eagain.st","subject":"Re: [PATCH] upload-pack.c: treat want-ref relative to namespace","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-30T18:57:02Z","receivedAt":"2021-07-30T18:57:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kim Altintop writes:\n\n> When 'upload-pack' runs within the context of a git namespace, treat any\n> 'want-ref' lines the client sends as relative to that namespace.\n>\n> Also check if the wanted ref is hidden via 'hideRefs', and respond with\n> an error otherwise. It was previously possible to request any ref, but\n> note that this is still the case unless 'hideRefs' is in effect.\n>\n> Signed-off-by: Kim Altintop \n> ---\n\nNicely described. I have a question on the last sentence, though.\nDo you mean that any ref can be requested when a namespace is in\nuse, as long as 'hideRefs' is not in effect? What does \"any ref\"\nexactly mean---even thouse outside the given namespace (and if so\nhow?) I wonder if the last sentence is making the description more\nconfusing without adding any clarity. In other words, would this\nwork as a replacement for the second paragraph, or does it say\nsomething different from what you wanted to say?\n\n Requests for any ref, even those that are marked to be hidden\n via the 'transfer.hideRefs' configuration, were allowed but it\n is problematic for such and such reasons. Respond with an error\n if a requested ref is to be hidden.\n\nI couldn't tell why you thought it was problematic, so left \"for\nsuch and such reasons\" to be filled in, but there still may be an\nissue.\n\nHow does the error response look like? We shouldn't be saying \"you\nrequested for the hidden/x branch, but you are not allowed to do so,\nas that is hidden\". To hide something, we should pretend that the\nthing does not exist, so that we can hide even the fact that we are\nhiding it.\n\nTo help future readers of \"git log\" who find this change from you,\nwe should clarify the \"respond with an error\" part of your proposed\nlog message (e.g. \"pretend that the wanted ref does not exist when\nit is hidden via the 'transfer.hiderefs' configuration\" or something\nelse).\n\n> +test_expect_success 'setup namespaced repo' '\n> +\t(\n> +\t\tgit init -b main \"$REPO\" &&\n> +\t\tcd \"$REPO\" &&\n> +\t\ttest_commit a &&\n> +\t\ttest_commit b &&\n> +\t\tgit checkout a &&\n> +\t\ttest_commit c &&\n> +\t\tgit checkout a &&\n> +\t\ttest_commit d &&\n> +\t\tgit update-ref refs/heads/ns-no b &&\n> +\t\tgit update-ref refs/namespaces/ns/refs/heads/ns-yes c &&\n> +\t\tgit update-ref refs/namespaces/ns/refs/heads/hidden d\n> +\t) &&\n> + git -C \"$REPO\" config uploadpack.allowRefInWant true &&\n> + git -C \"$REPO\" config transfer.hideRefs refs/heads/hidden\n> +'\n\nI wonder why the last two are outside the subshell? IOW, you could\nhave configured the newly created repository while you were still in\nthere.\n\n> +test_expect_success 'want-ref with namespaces' '\n> +\toid=$(git -C \"$REPO\" rev-parse c) &&\n> +\tcat >expected_refs <<-EOF &&\n> +\t$oid refs/heads/ns-yes\n> +\tEOF\n> +\t>expected_commits &&\n> +\n> +\toid=$(git -C \"$REPO\" rev-parse c) &&\n> +\ttest-tool pkt-line pack >in <<-EOF &&\n> +\t$(write_command fetch)\n> +\t0001\n> +\tno-progress\n> +\twant-ref refs/heads/ns-yes\n> +\thave $oid\n> +\tdone\n> +\t0000\n> +\tEOF\n> +\n> +\tGIT_NAMESPACE=ns && export GIT_NAMESPACE &&\n> +\ttest-tool -C \"$REPO\" serve-v2 --stateless-rpc >out +\tcheck_output\n> +'\n\nUnless you mean to make all subsequent tests to be done inside the\n'ns' namespace, and even when you do, you do not want to do this\nin order to keep each test as independent as possible (iow, make\nsome of them skippable without affecting the later tests). Run the\nfinal test in a subshell, e.g.\n\n\toid=$(git -C \"$REPO\" rev-parse c) &&\n\ttest-tool pkt-line pack >in <<-EOF &&\n\t...\n\tEOF\n\n\t(\n \texport GIT_NAMESPACE=ns &&\n\t\ttest-tool ... >out in <<-EOF &&\n\t...\n\tEOF\n\tGIT_NAMESPACE=ns test-tool ... >out diff --git a/upload-pack.c b/upload-pack.c\n> index 297b76fcb4..008ac75125 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -1417,21 +1417,24 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,\n> \t\t\t struct string_list *wanted_refs,\n> \t\t\t struct object_array *want_obj)\n> {\n> -\tconst char *arg;\n> +\tconst char *refname_nons;\n> \tif (skip_prefix(line, \"want-ref \", &arg)) {\n\nDon't you receive the result in refname_nons here, as arg is no\nlonger there?\n\n> \t\tstruct object_id oid;\n> \t\tstruct string_list_item *item;\n> \t\tstruct object *o;\n> +\t\tstruct strbuf refname = STRBUF_INIT;\n>\n> -\t\tif (read_ref(arg, &oid)) {\n> -\t\t\tpacket_writer_error(writer, \"unknown ref %s\", arg);\n> -\t\t\tdie(\"unknown ref %s\", arg);\n> +\t\tstrbuf_addf(&refname, \"%s%s\", get_git_namespace(), refname_nons);\n> +\t\tif (ref_is_hidden(refname_nons, refname.buf) ||\n> +\t\t read_ref(refname.buf, &oid)) {\n> +\t\t\tpacket_writer_error(writer, \"unknown ref %s\", refname_nons);\n> +\t\t\tdie(\"unknown ref %s\", refname.buf);\n> \t\t}\n\nOK. Assuming that it makes sense for the hideRefs mechanism to kick\nin here (which I would prefer to hear from others who've worked with\nthis code, say Jonathan Tan?), the updated code makes sense.\n\nThanks.\n\n\n> -\t\titem = string_list_append(wanted_refs, arg);\n> +\t\titem = string_list_append(wanted_refs, refname_nons);\n> \t\titem->util = oiddup(&oid);\n>\n> -\t\to = parse_object_or_die(&oid, arg);\n> +\t\to = parse_object_or_die(&oid, refname);\n> \t\tif (!(o->flags & WANTED)) {\n> \t\t\to->flags |= WANTED;\n> \t\t\tadd_object_array(o, NULL, want_obj);\n> --\n> 2.32.0\n"},{"id":"431603","messageId":"CD6S5MUMEIW3.3ULMDKKSQ92UW@schmidt","threadId":"56182","inReplyTo":"xmqqbl6j1vgh.fsf@gitster.g","subject":"Re: [PATCH] upload-pack.c: treat want-ref relative to namespace","fromName":"Kim Altintop","fromEmail":"kim@eagain.st","sentAt":"2021-07-30T21:08:20Z","receivedAt":"2021-07-30T21:08:29Z","isPatch":true,"sender":{"key":"kim@eagain.st","avatar":"https://avatars.githubusercontent.com/u/6163?v=4"},"body":"On Fri Jul 30, 2021 at 8:57 PM CEST, Junio C Hamano wrote:\n>\n> Kim Altintop writes:\n[..]\n> > Also check if the wanted ref is hidden via 'hideRefs', and respond with\n> > an error otherwise. It was previously possible to request any ref, but\n> > note that this is still the case unless 'hideRefs' is in effect.\n[..]\n> Nicely described. I have a question on the last sentence, though.\n> Do you mean that any ref can be requested when a namespace is in\n> use, as long as 'hideRefs' is not in effect? What does \"any ref\"\n> exactly mean---even thouse outside the given namespace (and if so\n> how?)\n\nThank you. It seems like I got confused for a moment when writing the commit\nmessage. It's not possible to get a ref outside the namespace anymore. I removed\nthat sentence.\n\n> > +test_expect_success 'setup namespaced repo' '\n> > +\t(\n> > +\t\tgit init -b main \"$REPO\" &&\n> > +\t\tcd \"$REPO\" &&\n> > +\t\ttest_commit a &&\n> > +\t\ttest_commit b &&\n> > +\t\tgit checkout a &&\n> > +\t\ttest_commit c &&\n> > +\t\tgit checkout a &&\n> > +\t\ttest_commit d &&\n> > +\t\tgit update-ref refs/heads/ns-no b &&\n> > +\t\tgit update-ref refs/namespaces/ns/refs/heads/ns-yes c &&\n> > +\t\tgit update-ref refs/namespaces/ns/refs/heads/hidden d\n> > +\t) &&\n> > + git -C \"$REPO\" config uploadpack.allowRefInWant true &&\n> > + git -C \"$REPO\" config transfer.hideRefs refs/heads/hidden\n> > +'\n>\n> I wonder why the last two are outside the subshell? IOW, you could\n> have configured the newly created repository while you were still in\n> there.\n\nTo be honest, I don't know. I did that because the other repo setups in that\nfile follow the same pattern, I suppose that qualifies as \"cargo culting\". Happy\nto remove the subshell, unless others point out that there is some specific\nreason for it.\n\n> Unless you mean to make all subsequent tests to be done inside the\n> 'ns' namespace, and even when you do, you do not want to do this\n> in order to keep each test as independent as possible (iow, make\n> some of them skippable without affecting the later tests). Run the\n> final test in a subshell, e.g.\n>\n> oid=$(git -C \"$REPO\" rev-parse c) &&\n> test-tool pkt-line pack >in <<-EOF &&\n> ...\n> EOF\n>\n> (\n> export GIT_NAMESPACE=ns &&\n> test-tool ... >out ) &&\n> check_output\n>\n> or if the command you want to run with a custom environment variable\n> is a single external executable like this case, do\n>\n> oid=$(git -C \"$REPO\" rev-parse c) &&\n> test-tool pkt-line pack >in <<-EOF &&\n> ...\n> EOF\n> GIT_NAMESPACE=ns test-tool ... >out check_output\n>\n> That way, the environment will be kept clean without GIT_NAMESPACE\n> outside the invocation of test-tool.\n>\n> Note that you cannot use this technique directly with test_must_fail\n> which is *not* an external executable but is a shell function.\n>\n> test_must_fail env GIT_NAMESPACE=ns test-tool ...\n>\n> would be the way to write a step that must fail.\n\n\nAh thanks! I had tried\n\n...\nGIT_NAMESPACE=ns test-tool ... >out \n> > diff --git a/upload-pack.c b/upload-pack.c\n> > index 297b76fcb4..008ac75125 100644\n> > --- a/upload-pack.c\n> > +++ b/upload-pack.c\n> > @@ -1417,21 +1417,24 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,\n> > \t\t\t struct string_list *wanted_refs,\n> > \t\t\t struct object_array *want_obj)\n> > {\n> > -\tconst char *arg;\n> > +\tconst char *refname_nons;\n> > \tif (skip_prefix(line, \"want-ref \", &arg)) {\n>\n> Don't you receive the result in refname_nons here, as arg is no\n> longer there?\n\nOuch. Will fix.\n\n>\n> > \t\tstruct object_id oid;\n> > \t\tstruct string_list_item *item;\n> > \t\tstruct object *o;\n> > +\t\tstruct strbuf refname = STRBUF_INIT;\n> >\n> > -\t\tif (read_ref(arg, &oid)) {\n> > -\t\t\tpacket_writer_error(writer, \"unknown ref %s\", arg);\n> > -\t\t\tdie(\"unknown ref %s\", arg);\n> > +\t\tstrbuf_addf(&refname, \"%s%s\", get_git_namespace(), refname_nons);\n> > +\t\tif (ref_is_hidden(refname_nons, refname.buf) ||\n> > +\t\t read_ref(refname.buf, &oid)) {\n> > +\t\t\tpacket_writer_error(writer, \"unknown ref %s\", refname_nons);\n> > +\t\t\tdie(\"unknown ref %s\", refname.buf);\n> > \t\t}\n>\n> OK. Assuming that it makes sense for the hideRefs mechanism to kick\n> in here (which I would prefer to hear from others who've worked with\n> this code, say Jonathan Tan?), the updated code makes sense.\n\nI have also updated the code for the v2 to use refname_nons for any die() calls,\nas I realised that this may be transmitted to the client via sideband (is that\ncorrect?).\n\n"},{"id":"431640","messageId":"20210731203415.618641-1-kim@eagain.st","threadId":"56182","inReplyTo":"20210730135845.633234-1-kim@eagain.st","subject":"[PATCH v2] upload-pack.c: treat want-ref relative to namespace","fromName":"Kim Altintop","fromEmail":"kim@eagain.st","sentAt":"2021-07-31T20:36:59Z","receivedAt":"2021-07-31T20:37:10Z","isPatch":true,"sender":{"key":"kim@eagain.st","avatar":"https://avatars.githubusercontent.com/u/6163?v=4"},"body":"When 'upload-pack' runs within the context of a git namespace, treat any\n'want-ref' lines the client sends as relative to that namespace.\n\nAlso check if the wanted ref is hidden via 'hideRefs'. If it is hidden,\nrespond with an error as if the ref didn't exist.\n\nSigned-off-by: Kim Altintop \n---\n\nChanges from v1:\n\n * Amend commit message\n * upload-pack.c: fix variable renaming (how could this even work?)\n * upload-pack.c: hide namespace in all output, including die()\n * t5703: don't use subshell in repo setup\n * t5703: use \"env\" keyword to correctly scope GIT_NAMESPACE\n\n\n t/t5703-upload-pack-ref-in-want.sh | 72 ++++++++++++++++++++++++++++++\n upload-pack.c | 17 ++++---\n 2 files changed, 82 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh\nindex e9e471621d..96df3073d1 100755\n--- a/t/t5703-upload-pack-ref-in-want.sh\n+++ b/t/t5703-upload-pack-ref-in-want.sh\n@@ -298,6 +298,78 @@ test_expect_success 'fetching with wildcard that matches multiple refs' '\n \tgrep \"want-ref refs/heads/o/bar\" log\n '\n\n+REPO=\"$(pwd)/repo-ns\"\n+\n+test_expect_success 'setup namespaced repo' '\n+\tgit init -b main \"$REPO\" &&\n+\tcd \"$REPO\" &&\n+\ttest_commit a &&\n+\ttest_commit b &&\n+\tgit checkout a &&\n+\ttest_commit c &&\n+\tgit checkout a &&\n+\ttest_commit d &&\n+\tgit update-ref refs/heads/ns-no b &&\n+\tgit update-ref refs/namespaces/ns/refs/heads/ns-yes c &&\n+\tgit update-ref refs/namespaces/ns/refs/heads/hidden d &&\n+\tgit -C \"$REPO\" config uploadpack.allowRefInWant true &&\n+\tgit -C \"$REPO\" config transfer.hideRefs refs/heads/hidden\n+'\n+\n+test_expect_success 'want-ref with namespaces' '\n+\toid=$(git -C \"$REPO\" rev-parse c) &&\n+\tcat >expected_refs <<-EOF &&\n+\t$oid refs/heads/ns-yes\n+\tEOF\n+\t>expected_commits &&\n+\n+\toid=$(git -C \"$REPO\" rev-parse c) &&\n+\ttest-tool pkt-line pack >in <<-EOF &&\n+\t$(write_command fetch)\n+\t0001\n+\tno-progress\n+\twant-ref refs/heads/ns-yes\n+\thave $oid\n+\tdone\n+\t0000\n+\tEOF\n+\n+\tenv GIT_NAMESPACE=ns test-tool -C \"$REPO\" serve-v2 --stateless-rpc >out in <<-EOF &&\n+\t$(write_command fetch)\n+\t0001\n+\tno-progress\n+\twant-ref refs/heads/ns-no\n+\thave $oid\n+\tdone\n+\t0000\n+\tEOF\n+\n+\ttest_must_fail env GIT_NAMESPACE=ns test-tool -C \"$REPO\" serve-v2 --stateless-rpc >out in <<-EOF &&\n+\t$(write_command fetch)\n+\t0001\n+\tno-progress\n+\twant-ref refs/heads/hidden\n+\thave $oid\n+\tdone\n+\t0000\n+\tEOF\n+\n+\ttest_must_fail env GIT_NAMESPACE=ns test-tool -C \"$REPO\" serve-v2 --stateless-rpc >out util = oiddup(&oid);\n\n-\t\to = parse_object_or_die(&oid, arg);\n+\t\to = parse_object_or_die(&oid, refname_nons);\n \t\tif (!(o->flags & WANTED)) {\n \t\t\to->flags |= WANTED;\n \t\t\tadd_object_array(o, NULL, want_obj);\n--\n2.32.0\n\n\n"},{"id":"431759","messageId":"20210802210644.3432544-1-jonathantanmy@google.com","threadId":"56182","inReplyTo":"20210731203415.618641-1-kim@eagain.st","subject":"Re: [PATCH v2] upload-pack.c: treat want-ref relative to namespace","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-02T21:06:44Z","receivedAt":"2021-08-02T21:06:52Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh\n> index e9e471621d..96df3073d1 100755\n> --- a/t/t5703-upload-pack-ref-in-want.sh\n> +++ b/t/t5703-upload-pack-ref-in-want.sh\n> @@ -298,6 +298,78 @@ test_expect_success 'fetching with wildcard that matches multiple refs' '\n> \tgrep \"want-ref refs/heads/o/bar\" log\n> '\n> \n> +REPO=\"$(pwd)/repo-ns\"\n> +\n> +test_expect_success 'setup namespaced repo' '\n> +\tgit init -b main \"$REPO\" &&\n> +\tcd \"$REPO\" &&\n> +\ttest_commit a &&\n> +\ttest_commit b &&\n> +\tgit checkout a &&\n> +\ttest_commit c &&\n> +\tgit checkout a &&\n> +\ttest_commit d &&\n> +\tgit update-ref refs/heads/ns-no b &&\n> +\tgit update-ref refs/namespaces/ns/refs/heads/ns-yes c &&\n> +\tgit update-ref refs/namespaces/ns/refs/heads/hidden d &&\n> +\tgit -C \"$REPO\" config uploadpack.allowRefInWant true &&\n> +\tgit -C \"$REPO\" config transfer.hideRefs refs/heads/hidden\n> +'\n\nIf you're not using a subshell to set up the repo, you should add '-C\n\"$REPO\"' to all the \"git\" commands (like you do in the last 2 lines)\ninstead of \"cd\"-ing halfway through the test. The helper function\ntest_commit also has that facility ('test_commit -C \"$REPO\" a', for\nexample).\n\n> +test_expect_success 'want-ref with namespaces' '\n> +\toid=$(git -C \"$REPO\" rev-parse c) &&\n> +\tcat >expected_refs <<-EOF &&\n> +\t$oid refs/heads/ns-yes\n> +\tEOF\n> +\t>expected_commits &&\n> +\n> +\toid=$(git -C \"$REPO\" rev-parse c) &&\n> +\ttest-tool pkt-line pack >in <<-EOF &&\n> +\t$(write_command fetch)\n> +\t0001\n> +\tno-progress\n> +\twant-ref refs/heads/ns-yes\n> +\thave $oid\n\nDo we need this \"have\" line? I think we can just omit it, since what the\nclient has is irrelevant to the test. (Same for the other tests.)\n\n> +\tdone\n> +\t0000\n> +\tEOF\n> +\n> +\tenv GIT_NAMESPACE=ns test-tool -C \"$REPO\" serve-v2 --stateless-rpc >out +\tcheck_output\n> +'\n> +\n> +test_expect_success 'want-ref outside namespace' '\n> +\toid=$(git -C \"$REPO\" rev-parse c) &&\n> +\ttest-tool pkt-line pack >in <<-EOF &&\n> +\t$(write_command fetch)\n> +\t0001\n> +\tno-progress\n> +\twant-ref refs/heads/ns-no\n> +\thave $oid\n> +\tdone\n> +\t0000\n> +\tEOF\n> +\n> +\ttest_must_fail env GIT_NAMESPACE=ns test-tool -C \"$REPO\" serve-v2 --stateless-rpc >out +\tgrep \"unknown ref\" out\n> +'\n\nFor the failure tests, it's safer to write them in pairs - one that\nsucceeds and one that fails. Here, a typo in \"ns-no\" (e.g. if I wrote\n\"ns-noo\" instead) would cause the exact same result, but if we were to\nwrite a pair of tests, we wouldn't have this problem.\n\nTo do this, you can bundle the same code into a function and call them\nfrom both tests. E.g.:\n\n setup_want_ns_no () {\n (common code)\n }\n\n test_expect_success 'want-ref without namespace works...' '\n setup_want_ref_outside_namespace &&\n test-tool ... &&\n check_output\n '\n\n test_expect_success '...but, with namespace, does not work' '\n setup_want_ref_outside_namespace &&\n test_must_fail env GIT_NAMESPACE=ns ... &&\n grep \"unknown ref\" out\n '\n\nThe first test_expect_success does seem redundant, but I can't think of\na better way to ensure that the helper function (setup_want_ns_no in\nthis case) is written correctly.\n\n> +test_expect_success 'hideRefs with namespaces' '\n> +\toid=$(git -C \"$REPO\" rev-parse c) &&\n> +\ttest-tool pkt-line pack >in <<-EOF &&\n> +\t$(write_command fetch)\n> +\t0001\n> +\tno-progress\n> +\twant-ref refs/heads/hidden\n> +\thave $oid\n> +\tdone\n> +\t0000\n> +\tEOF\n> +\n> +\ttest_must_fail env GIT_NAMESPACE=ns test-tool -C \"$REPO\" serve-v2 --stateless-rpc >out +\tgrep \"unknown ref\" out\n> +'\n> +\n\nSame for this.\n\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 297b76fcb4..c897802f1c 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -1417,21 +1417,24 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,\n> \t\t\t struct string_list *wanted_refs,\n> \t\t\t struct object_array *want_obj)\n> {\n> -\tconst char *arg;\n> -\tif (skip_prefix(line, \"want-ref \", &arg)) {\n> +\tconst char *refname_nons;\n> +\tif (skip_prefix(line, \"want-ref \", &refname_nons)) {\n> \t\tstruct object_id oid;\n> \t\tstruct string_list_item *item;\n> \t\tstruct object *o;\n> +\t\tstruct strbuf refname = STRBUF_INIT;\n\n\"refname\" needs to be released somewhere.\n\n> \n> -\t\tif (read_ref(arg, &oid)) {\n> -\t\t\tpacket_writer_error(writer, \"unknown ref %s\", arg);\n> -\t\t\tdie(\"unknown ref %s\", arg);\n> +\t\tstrbuf_addf(&refname, \"%s%s\", get_git_namespace(), refname_nons);\n> +\t\tif (ref_is_hidden(refname_nons, refname.buf) ||\n> +\t\t read_ref(refname.buf, &oid)) {\n> +\t\t\tpacket_writer_error(writer, \"unknown ref %s\", refname_nons);\n> +\t\t\tdie(\"unknown ref %s\", refname_nons);\n> \t\t}\n> \n> -\t\titem = string_list_append(wanted_refs, arg);\n> +\t\titem = string_list_append(wanted_refs, refname_nons);\n> \t\titem->util = oiddup(&oid);\n> \n> -\t\to = parse_object_or_die(&oid, arg);\n> +\t\to = parse_object_or_die(&oid, refname_nons);\n> \t\tif (!(o->flags & WANTED)) {\n> \t\t\to->flags |= WANTED;\n> \t\t\tadd_object_array(o, NULL, want_obj);\n\nBesides my comments about the tests, I think that this patch looks good.\nJunio had a relevant question [1]:\n\n> OK. Assuming that it makes sense for the hideRefs mechanism to kick\n> in here (which I would prefer to hear from others who've worked with\n> this code, say Jonathan Tan?), the updated code makes sense.\n\nI think it makes sense here. I checked my old code that Kim linked [2],\nand in that patch I put it somewhere else (specifically, the part that\nprints out the \"wanted-ref\" lines), but it makes sense that it is\ndifferent. In my version, \"want-ref\" supports globs and is allowed to\nmatch 0 refs, but in Brandon's final version, \"want-ref\" does not\nsupport globs and must match the ref, so checking it upon parse (as is\ndone in this commit) is the most reasonable.\n\nQuestions from Kim:\n\n> Jonathan: I took the test code from your original patch introducing ref-in-want,\n> but modified it substantially. Let me know if it is conventional to credit you\n> anyway, and by which trailer.\n\nI don't think there's a convention, but you can use the \"Helped-by:\"\ntrailer if you want.\n\n> I have also updated the code for the v2 to use refname_nons for any die() calls,\n> as I realised that this may be transmitted to the client via sideband (is that\n> correct?).\n\nI believe that only packet_writer_error() output is sent via sideband,\nbut it still makes sense for the server-side output to print out the ref\nas read from the client verbatim.\n\n[1] https://lore.kernel.org/git/xmqqbl6j1vgh.fsf@gitster.g/\n[2] https://lore.kernel.org/git/d0d42b3bb4cf755f122591e191354c53848f197d.1485381677.git.jonathantanmy@google.com/\n"},{"id":"431976","messageId":"CDB0M5IP0WNJ.3I09ZETKF0VXS@schmidt","threadId":"56182","inReplyTo":"20210802210644.3432544-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] upload-pack.c: treat want-ref relative to namespace","fromName":"Kim Altintop","fromEmail":"kim@eagain.st","sentAt":"2021-08-04T20:36:48Z","receivedAt":"2021-08-04T20:37:30Z","isPatch":true,"sender":{"key":"kim@eagain.st","avatar":"https://avatars.githubusercontent.com/u/6163?v=4"},"body":"On Mon Aug 2, 2021 at 11:06 PM CEST, Jonathan Tan wrote:\n> > +test_expect_success 'setup namespaced repo' '\n> > +\tgit init -b main \"$REPO\" &&\n> > +\tcd \"$REPO\" &&\n> > +\ttest_commit a &&\n> > +\ttest_commit b &&\n> > +\tgit checkout a &&\n> > +\ttest_commit c &&\n> > +\tgit checkout a &&\n> > +\ttest_commit d &&\n> > +\tgit update-ref refs/heads/ns-no b &&\n> > +\tgit update-ref refs/namespaces/ns/refs/heads/ns-yes c &&\n> > +\tgit update-ref refs/namespaces/ns/refs/heads/hidden d &&\n> > +\tgit -C \"$REPO\" config uploadpack.allowRefInWant true &&\n> > +\tgit -C \"$REPO\" config transfer.hideRefs refs/heads/hidden\n> > +'\n>\n> If you're not using a subshell to set up the repo, you should add '-C\n> \"$REPO\"' to all the \"git\" commands (like you do in the last 2 lines)\n> instead of \"cd\"-ing halfway through the test. The helper function\n> test_commit also has that facility ('test_commit -C \"$REPO\" a', for\n> example).\n\nAh, that answers the question raised by Junio in the first review. I'll revert\nto using a subshell, as that seems clearer and is used throughout the file.\n\n> > +test_expect_success 'want-ref with namespaces' '\n> > +\toid=$(git -C \"$REPO\" rev-parse c) &&\n> > +\tcat >expected_refs <<-EOF &&\n> > +\t$oid refs/heads/ns-yes\n> > +\tEOF\n> > +\t>expected_commits &&\n> > +\n> > +\toid=$(git -C \"$REPO\" rev-parse c) &&\n> > +\ttest-tool pkt-line pack >in <<-EOF &&\n> > +\t$(write_command fetch)\n> > +\t0001\n> > +\tno-progress\n> > +\twant-ref refs/heads/ns-yes\n> > +\thave $oid\n>\n> Do we need this \"have\" line? I think we can just omit it, since what the\n> client has is irrelevant to the test. (Same for the other tests.)\n\nHm no. I think I misread how this works.\n\n> > +test_expect_success 'want-ref outside namespace' '\n> > +\toid=$(git -C \"$REPO\" rev-parse c) &&\n> > +\ttest-tool pkt-line pack >in <<-EOF &&\n> > +\t$(write_command fetch)\n> > +\t0001\n> > +\tno-progress\n> > +\twant-ref refs/heads/ns-no\n> > +\thave $oid\n> > +\tdone\n> > +\t0000\n> > +\tEOF\n> > +\n> > +\ttest_must_fail env GIT_NAMESPACE=ns test-tool -C \"$REPO\" serve-v2 --stateless-rpc >out > +\tgrep \"unknown ref\" out\n> > +'\n>\n> For the failure tests, it's safer to write them in pairs - one that\n> succeeds and one that fails. Here, a typo in \"ns-no\" (e.g. if I wrote\n> \"ns-noo\" instead) would cause the exact same result, but if we were to\n> write a pair of tests, we wouldn't have this problem.\n\nThat's a good suggestion. However, I'm having some difficulties finding just the\nright amount of common code to extract due to\n\na. having to pass the namespace via env instead of --namespace (and the\n different position for the success / test_must_fail cases), and\nb. having to rely on _persistent_ config for hideRefs, as opposed to being able\n to pass it via -c to upload-pack (ie. I'm worried about tests breaking if\n they get reordered)\n\nSo I'm not exactly happy with what I came up with for v3, but I'd also be\nreluctant to add --namespace / -c support to test-tool as part of this patch.\nLet me know what you think.\n\n\n> > diff --git a/upload-pack.c b/upload-pack.c\n> > index 297b76fcb4..c897802f1c 100644\n> > --- a/upload-pack.c\n> > +++ b/upload-pack.c\n> > @@ -1417,21 +1417,24 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,\n> > \t\t\t struct string_list *wanted_refs,\n> > \t\t\t struct object_array *want_obj)\n> > {\n> > -\tconst char *arg;\n> > -\tif (skip_prefix(line, \"want-ref \", &arg)) {\n> > +\tconst char *refname_nons;\n> > +\tif (skip_prefix(line, \"want-ref \", &refname_nons)) {\n> > \t\tstruct object_id oid;\n> > \t\tstruct string_list_item *item;\n> > \t\tstruct object *o;\n> > +\t\tstruct strbuf refname = STRBUF_INIT;\n>\n> \"refname\" needs to be released somewhere.\n\nYeap :/\n\n"},{"id":"431978","messageId":"20210804203829.661565-1-kim@eagain.st","threadId":"56182","inReplyTo":"20210731203415.618641-1-kim@eagain.st","subject":"[PATCH v3] upload-pack.c: treat want-ref relative to namespace","fromName":"Kim Altintop","fromEmail":"kim@eagain.st","sentAt":"2021-08-04T20:42:42Z","receivedAt":"2021-08-04T20:43:03Z","isPatch":true,"sender":{"key":"kim@eagain.st","avatar":"https://avatars.githubusercontent.com/u/6163?v=4"},"body":"When 'upload-pack' runs within the context of a git namespace, treat any\n'want-ref' lines the client sends as relative to that namespace.\n\nAlso check if the wanted ref is hidden via 'hideRefs'. If it is hidden,\nrespond with an error as if the ref didn't exist.\n\nHelped-by: Jonathan Tan \nSigned-off-by: Kim Altintop \n---\n\nChanges from v2:\n\n * upload-pack.c: release strbuf\n * t5730: revert to scoping to $REPO via subshell in setup\n * t5730: add \"cross-check\" tests as per review comments from Jonathan\n\n t/t5703-upload-pack-ref-in-want.sh | 128 +++++++++++++++++++++++++++++\n upload-pack.c | 18 ++--\n 2 files changed, 139 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh\nindex e9e471621d..4a828042bb 100755\n--- a/t/t5703-upload-pack-ref-in-want.sh\n+++ b/t/t5703-upload-pack-ref-in-want.sh\n@@ -298,6 +298,134 @@ test_expect_success 'fetching with wildcard that matches multiple refs' '\n \tgrep \"want-ref refs/heads/o/bar\" log\n '\n\n+REPO=\"$(pwd)/repo-ns\"\n+\n+write_fetch_want_ref() {\n+\tlocal wanted_ref=\"$1\"\n+\n+\twrite_command fetch\n+\techo \"0001\"\n+\techo \"no-progress\"\n+\techo \"want-ref $1\"\n+\techo \"done\"\n+\techo \"0000\"\n+}\n+\n+test_expect_success 'setup namespaced repo' '\n+\t(\n+\t\tgit init -b main \"$REPO\" &&\n+\t\tcd \"$REPO\" &&\n+\t\ttest_commit a &&\n+\t\ttest_commit b &&\n+\t\tgit checkout a &&\n+\t\ttest_commit c &&\n+\t\tgit checkout a &&\n+\t\ttest_commit d &&\n+\t\tgit update-ref refs/heads/ns-no b &&\n+\t\tgit update-ref refs/namespaces/ns/refs/heads/ns-yes c &&\n+\t\tgit update-ref refs/namespaces/ns/refs/heads/hidden d\n+\t) &&\n+\tgit -C \"$REPO\" config uploadpack.allowRefInWant true\n+'\n+\n+test_expect_success 'with namespace: want-ref is considered relative to namespace' '\n+\twanted_ref=refs/heads/ns-yes &&\n+\n+\toid=$(git -C \"$REPO\" rev-parse $wanted_ref) &&\n+\tcat >expected_refs <<-EOF &&\n+\t$oid $wanted_ref\n+\tEOF\n+\tgit -C \"$REPO\" rev-parse a $wanted_ref >expected_commits &&\n+\n+\ttest-tool pkt-line pack >in <<-EOF &&\n+\t$(write_fetch_want_ref $wanted_ref)\n+\tEOF\n+\n+\tenv GIT_NAMESPACE=ns test-tool -C \"$REPO\" serve-v2 --stateless-rpc >out in <<-EOF &&\n+\t$(write_fetch_want_ref $wanted_ref)\n+\tEOF\n+\n+\ttest_must_fail env GIT_NAMESPACE=ns \\\n+\t\ttest-tool -C \"$REPO\" serve-v2 --stateless-rpc >out expected_refs <<-EOF &&\n+\t$oid $wanted_ref\n+\tEOF\n+\tgit -C \"$REPO\" rev-parse a $wanted_ref >expected_commits &&\n+\n+\ttest-tool pkt-line pack >in <<-EOF &&\n+\t$(write_fetch_want_ref $wanted_ref)\n+\tEOF\n+\n+\ttest-tool -C \"$REPO\" serve-v2 --stateless-rpc >out in <<-EOF &&\n+\t$(write_fetch_want_ref $wanted_ref)\n+\tEOF\n+\n+\ttest_must_fail env GIT_NAMESPACE=ns \\\n+\t\ttest-tool -C \"$REPO\" serve-v2 --stateless-rpc >out