[PATCH] upload-pack.c: treat want-ref relative to namespace

[PATCH] upload-pack.c: treat want-ref relative to namespace

[Kim Altintop]

When 'upload-pack' runs within the context of a git namespace, treat any
'want-ref' lines the client sends as relative to that namespace.

Also check if the wanted ref is hidden via 'hideRefs', and respond with
an error otherwise. It was previously possible to request any ref, but
note that this is still the case unless 'hideRefs' is in effect.

Signed-off-by: Kim Altintop <kim@eagain.st>
---

Please excuse my newbie ness.

 t/t5703-upload-pack-ref-in-want.sh | 77 ++++++++++++++++++++++++++++++
 upload-pack.c                      | 15 +++---
 2 files changed, 86 insertions(+), 6 deletions(-)

diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index e9e471621d..9fb16848bc 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -298,6 +298,83 @@ test_expect_success 'fetching with wildcard that matches multiple refs' '
 	grep "want-ref refs/heads/o/bar" log
 '

+REPO="$(pwd)/repo-ns"
+
+test_expect_success 'setup namespaced repo' '
+	(
+		git init -b main "$REPO" &&
+		cd "$REPO" &&
+		test_commit a &&
+		test_commit b &&
+		git checkout a &&
+		test_commit c &&
+		git checkout a &&
+		test_commit d &&
+		git update-ref refs/heads/ns-no b &&
+		git update-ref refs/namespaces/ns/refs/heads/ns-yes c &&
+		git update-ref refs/namespaces/ns/refs/heads/hidden d
+	) &&
+    git -C "$REPO" config uploadpack.allowRefInWant true &&
+    git -C "$REPO" config transfer.hideRefs refs/heads/hidden
+'
+
+test_expect_success 'want-ref with namespaces' '
+	oid=$(git -C "$REPO" rev-parse c) &&
+	cat >expected_refs <<-EOF &&
+	$oid refs/heads/ns-yes
+	EOF
+	>expected_commits &&
+
+	oid=$(git -C "$REPO" rev-parse c) &&
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_command fetch)
+	0001
+	no-progress
+	want-ref refs/heads/ns-yes
+	have $oid
+	done
+	0000
+	EOF
+
+	GIT_NAMESPACE=ns && export GIT_NAMESPACE &&
+	test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	check_output
+'
+
+test_expect_success 'want-ref outside namespace' '
+	oid=$(git -C "$REPO" rev-parse c) &&
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_command fetch)
+	0001
+	no-progress
+	want-ref refs/heads/ns-no
+	have $oid
+	done
+	0000
+	EOF
+
+	GIT_NAMESPACE=ns && export GIT_NAMESPACE &&
+	test_must_fail test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	grep "unknown ref" out
+'
+
+test_expect_success 'hideRefs with namespaces' '
+	oid=$(git -C "$REPO" rev-parse c) &&
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_command fetch)
+	0001
+	no-progress
+	want-ref refs/heads/hidden
+	have $oid
+	done
+	0000
+	EOF
+
+	GIT_NAMESPACE=ns && export GIT_NAMESPACE &&
+	test_must_fail test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	grep "unknown ref" out
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd

diff --git a/upload-pack.c b/upload-pack.c
index 297b76fcb4..008ac75125 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1417,21 +1417,24 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,
 			  struct string_list *wanted_refs,
 			  struct object_array *want_obj)
 {
-	const char *arg;
+	const char *refname_nons;
 	if (skip_prefix(line, "want-ref ", &arg)) {
 		struct object_id oid;
 		struct string_list_item *item;
 		struct object *o;
+		struct strbuf refname = STRBUF_INIT;

-		if (read_ref(arg, &oid)) {
-			packet_writer_error(writer, "unknown ref %s", arg);
-			die("unknown ref %s", arg);
+		strbuf_addf(&refname, "%s%s", get_git_namespace(), refname_nons);
+		if (ref_is_hidden(refname_nons, refname.buf) ||
+		    read_ref(refname.buf, &oid)) {
+			packet_writer_error(writer, "unknown ref %s", refname_nons);
+			die("unknown ref %s", refname.buf);
 		}

-		item = string_list_append(wanted_refs, arg);
+		item = string_list_append(wanted_refs, refname_nons);
 		item->util = oiddup(&oid);

-		o = parse_object_or_die(&oid, arg);
+		o = parse_object_or_die(&oid, refname);
 		if (!(o->flags & WANTED)) {
 			o->flags |= WANTED;
 			add_object_array(o, NULL, want_obj);
--
2.32.0

Re: [PATCH] upload-pack.c: treat want-ref relative to namespace

[Kim Altintop]

> Please excuse my newbie ness.

Oops I'm sorry. `send-email` remembered this from a dry run.

Jonathan: I took the test code from your original patch introducing ref-in-want,
but modified it substantially. Let me know if it is conventional to credit you
anyway, and by which trailer.

Re: [PATCH] upload-pack.c: treat want-ref relative to namespace

[Junio C Hamano]

Kim Altintop <kim@eagain.st> writes:

> When 'upload-pack' runs within the context of a git namespace, treat any
> 'want-ref' lines the client sends as relative to that namespace.
>
> Also check if the wanted ref is hidden via 'hideRefs', and respond with
> an error otherwise. It was previously possible to request any ref, but
> note that this is still the case unless 'hideRefs' is in effect.
>
> Signed-off-by: Kim Altintop <kim@eagain.st>
> ---

Nicely described.  I have a question on the last sentence, though.
Do you mean that any ref can be requested when a namespace is in
use, as long as 'hideRefs' is not in effect?  What does "any ref"
exactly mean---even thouse outside the given namespace (and if so
how?)  I wonder if the last sentence is making the description more
confusing without adding any clarity.  In other words, would this
work as a replacement for the second paragraph, or does it say
something different from what you wanted to say?

    Requests for any ref, even those that are marked to be hidden
    via the 'transfer.hideRefs' configuration, were allowed but it
    is problematic for such and such reasons.  Respond with an error
    if a requested ref is to be hidden.

I couldn't tell why you thought it was problematic, so left "for
such and such reasons" to be filled in, but there still may be an
issue.

How does the error response look like?  We shouldn't be saying "you
requested for the hidden/x branch, but you are not allowed to do so,
as that is hidden".  To hide something, we should pretend that the
thing does not exist, so that we can hide even the fact that we are
hiding it.

To help future readers of "git log" who find this change from you,
we should clarify the "respond with an error" part of your proposed
log message (e.g. "pretend that the wanted ref does not exist when
it is hidden via the 'transfer.hiderefs' configuration" or something
else).

> +test_expect_success 'setup namespaced repo' '
> +	(
> +		git init -b main "$REPO" &&
> +		cd "$REPO" &&
> +		test_commit a &&
> +		test_commit b &&
> +		git checkout a &&
> +		test_commit c &&
> +		git checkout a &&
> +		test_commit d &&
> +		git update-ref refs/heads/ns-no b &&
> +		git update-ref refs/namespaces/ns/refs/heads/ns-yes c &&
> +		git update-ref refs/namespaces/ns/refs/heads/hidden d
> +	) &&
> +    git -C "$REPO" config uploadpack.allowRefInWant true &&
> +    git -C "$REPO" config transfer.hideRefs refs/heads/hidden
> +'

I wonder why the last two are outside the subshell?  IOW, you could
have configured the newly created repository while you were still in
there.

> +test_expect_success 'want-ref with namespaces' '
> +	oid=$(git -C "$REPO" rev-parse c) &&
> +	cat >expected_refs <<-EOF &&
> +	$oid refs/heads/ns-yes
> +	EOF
> +	>expected_commits &&
> +
> +	oid=$(git -C "$REPO" rev-parse c) &&
> +	test-tool pkt-line pack >in <<-EOF &&
> +	$(write_command fetch)
> +	0001
> +	no-progress
> +	want-ref refs/heads/ns-yes
> +	have $oid
> +	done
> +	0000
> +	EOF
> +
> +	GIT_NAMESPACE=ns && export GIT_NAMESPACE &&
> +	test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
> +	check_output
> +'

Unless you mean to make all subsequent tests to be done inside the
'ns' namespace, and even when you do, you do not want to do this
in order to keep each test as independent as possible (iow, make
some of them skippable without affecting the later tests).  Run the
final test in a subshell, e.g.

	oid=$(git -C "$REPO" rev-parse c) &&
	test-tool pkt-line pack >in <<-EOF &&
	...
	EOF

	(
        	export GIT_NAMESPACE=ns &&
		test-tool ... >out <in
	) &&
	check_output

or if the command you want to run with a custom environment variable
is a single external executable like this case, do

	oid=$(git -C "$REPO" rev-parse c) &&
	test-tool pkt-line pack >in <<-EOF &&
	...
	EOF
	GIT_NAMESPACE=ns test-tool ... >out <in &&
	check_output

That way, the environment will be kept clean without GIT_NAMESPACE
outside the invocation of test-tool.

Note that you cannot use this technique directly with test_must_fail
which is *not* an external executable but is a shell function.

	test_must_fail env GIT_NAMESPACE=ns test-tool ...

would be the way to write a step that must fail.

> diff --git a/upload-pack.c b/upload-pack.c
> index 297b76fcb4..008ac75125 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1417,21 +1417,24 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,
>  			  struct string_list *wanted_refs,
>  			  struct object_array *want_obj)
>  {
> -	const char *arg;
> +	const char *refname_nons;
>  	if (skip_prefix(line, "want-ref ", &arg)) {

Don't you receive the result in refname_nons here, as arg is no
longer there?

>  		struct object_id oid;
>  		struct string_list_item *item;
>  		struct object *o;
> +		struct strbuf refname = STRBUF_INIT;
>
> -		if (read_ref(arg, &oid)) {
> -			packet_writer_error(writer, "unknown ref %s", arg);
> -			die("unknown ref %s", arg);
> +		strbuf_addf(&refname, "%s%s", get_git_namespace(), refname_nons);
> +		if (ref_is_hidden(refname_nons, refname.buf) ||
> +		    read_ref(refname.buf, &oid)) {
> +			packet_writer_error(writer, "unknown ref %s", refname_nons);
> +			die("unknown ref %s", refname.buf);
>  		}

OK.  Assuming that it makes sense for the hideRefs mechanism to kick
in here (which I would prefer to hear from others who've worked with
this code, say Jonathan Tan?), the updated code makes sense.

Thanks.


> -		item = string_list_append(wanted_refs, arg);
> +		item = string_list_append(wanted_refs, refname_nons);
>  		item->util = oiddup(&oid);
>
> -		o = parse_object_or_die(&oid, arg);
> +		o = parse_object_or_die(&oid, refname);
>  		if (!(o->flags & WANTED)) {
>  			o->flags |= WANTED;
>  			add_object_array(o, NULL, want_obj);
> --
> 2.32.0

Re: [PATCH] upload-pack.c: treat want-ref relative to namespace

[Kim Altintop]

On Fri Jul 30, 2021 at 8:57 PM CEST, Junio C Hamano wrote:
>
> Kim Altintop <kim@eagain.st> writes:
[..]
> > Also check if the wanted ref is hidden via 'hideRefs', and respond with
> > an error otherwise. It was previously possible to request any ref, but
> > note that this is still the case unless 'hideRefs' is in effect.
[..]
> Nicely described. I have a question on the last sentence, though.
> Do you mean that any ref can be requested when a namespace is in
> use, as long as 'hideRefs' is not in effect? What does "any ref"
> exactly mean---even thouse outside the given namespace (and if so
> how?)

Thank you. It seems like I got confused for a moment when writing the commit
message. It's not possible to get a ref outside the namespace anymore. I removed
that sentence.

> > +test_expect_success 'setup namespaced repo' '
> > +	(
> > +		git init -b main "$REPO" &&
> > +		cd "$REPO" &&
> > +		test_commit a &&
> > +		test_commit b &&
> > +		git checkout a &&
> > +		test_commit c &&
> > +		git checkout a &&
> > +		test_commit d &&
> > +		git update-ref refs/heads/ns-no b &&
> > +		git update-ref refs/namespaces/ns/refs/heads/ns-yes c &&
> > +		git update-ref refs/namespaces/ns/refs/heads/hidden d
> > +	) &&
> > +    git -C "$REPO" config uploadpack.allowRefInWant true &&
> > +    git -C "$REPO" config transfer.hideRefs refs/heads/hidden
> > +'
>
> I wonder why the last two are outside the subshell? IOW, you could
> have configured the newly created repository while you were still in
> there.

To be honest, I don't know. I did that because the other repo setups in that
file follow the same pattern, I suppose that qualifies as "cargo culting". Happy
to remove the subshell, unless others point out that there is some specific
reason for it.

> Unless you mean to make all subsequent tests to be done inside the
> 'ns' namespace, and even when you do, you do not want to do this
> in order to keep each test as independent as possible (iow, make
> some of them skippable without affecting the later tests). Run the
> final test in a subshell, e.g.
>
> oid=$(git -C "$REPO" rev-parse c) &&
> test-tool pkt-line pack >in <<-EOF &&
> ...
> EOF
>
> (
> export GIT_NAMESPACE=ns &&
> test-tool ... >out <in
> ) &&
> check_output
>
> or if the command you want to run with a custom environment variable
> is a single external executable like this case, do
>
> oid=$(git -C "$REPO" rev-parse c) &&
> test-tool pkt-line pack >in <<-EOF &&
> ...
> EOF
> GIT_NAMESPACE=ns test-tool ... >out <in &&
> check_output
>
> That way, the environment will be kept clean without GIT_NAMESPACE
> outside the invocation of test-tool.
>
> Note that you cannot use this technique directly with test_must_fail
> which is *not* an external executable but is a shell function.
>
> test_must_fail env GIT_NAMESPACE=ns test-tool ...
>
> would be the way to write a step that must fail.


Ah thanks! I had tried

...
GIT_NAMESPACE=ns test-tool ... >out <in


but the linter complained about this without giving a hint as to what the fix
would be. It turns out that "env" works, ie.

...
env GIT_NAMESPACE=ns test-tool ...
test_must_fail env GIT_NAMESPACE=ns test-tool ...


>
> > diff --git a/upload-pack.c b/upload-pack.c
> > index 297b76fcb4..008ac75125 100644
> > --- a/upload-pack.c
> > +++ b/upload-pack.c
> > @@ -1417,21 +1417,24 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,
> >  			  struct string_list *wanted_refs,
> >  			  struct object_array *want_obj)
> >  {
> > -	const char *arg;
> > +	const char *refname_nons;
> >  	if (skip_prefix(line, "want-ref ", &arg)) {
>
> Don't you receive the result in refname_nons here, as arg is no
> longer there?

Ouch. Will fix.

>
> >  		struct object_id oid;
> >  		struct string_list_item *item;
> >  		struct object *o;
> > +		struct strbuf refname = STRBUF_INIT;
> >
> > -		if (read_ref(arg, &oid)) {
> > -			packet_writer_error(writer, "unknown ref %s", arg);
> > -			die("unknown ref %s", arg);
> > +		strbuf_addf(&refname, "%s%s", get_git_namespace(), refname_nons);
> > +		if (ref_is_hidden(refname_nons, refname.buf) ||
> > +		    read_ref(refname.buf, &oid)) {
> > +			packet_writer_error(writer, "unknown ref %s", refname_nons);
> > +			die("unknown ref %s", refname.buf);
> >  		}
>
> OK. Assuming that it makes sense for the hideRefs mechanism to kick
> in here (which I would prefer to hear from others who've worked with
> this code, say Jonathan Tan?), the updated code makes sense.

I have also updated the code for the v2 to use refname_nons for any die() calls,
as I realised that this may be transmitted to the client via sideband (is that
correct?).

[PATCH v2] upload-pack.c: treat want-ref relative to namespace

[Kim Altintop]

When 'upload-pack' runs within the context of a git namespace, treat any
'want-ref' lines the client sends as relative to that namespace.

Also check if the wanted ref is hidden via 'hideRefs'. If it is hidden,
respond with an error as if the ref didn't exist.

Signed-off-by: Kim Altintop <kim@eagain.st>
---

Changes from v1:

  * Amend commit message
  * upload-pack.c: fix variable renaming (how could this even work?)
  * upload-pack.c: hide namespace in all output, including die()
  * t5703: don't use subshell in repo setup
  * t5703: use "env" keyword to correctly scope GIT_NAMESPACE


 t/t5703-upload-pack-ref-in-want.sh | 72 ++++++++++++++++++++++++++++++
 upload-pack.c                      | 17 ++++---
 2 files changed, 82 insertions(+), 7 deletions(-)

diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index e9e471621d..96df3073d1 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -298,6 +298,78 @@ test_expect_success 'fetching with wildcard that matches multiple refs' '
 	grep "want-ref refs/heads/o/bar" log
 '

+REPO="$(pwd)/repo-ns"
+
+test_expect_success 'setup namespaced repo' '
+	git init -b main "$REPO" &&
+	cd "$REPO" &&
+	test_commit a &&
+	test_commit b &&
+	git checkout a &&
+	test_commit c &&
+	git checkout a &&
+	test_commit d &&
+	git update-ref refs/heads/ns-no b &&
+	git update-ref refs/namespaces/ns/refs/heads/ns-yes c &&
+	git update-ref refs/namespaces/ns/refs/heads/hidden d &&
+	git -C "$REPO" config uploadpack.allowRefInWant true &&
+	git -C "$REPO" config transfer.hideRefs refs/heads/hidden
+'
+
+test_expect_success 'want-ref with namespaces' '
+	oid=$(git -C "$REPO" rev-parse c) &&
+	cat >expected_refs <<-EOF &&
+	$oid refs/heads/ns-yes
+	EOF
+	>expected_commits &&
+
+	oid=$(git -C "$REPO" rev-parse c) &&
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_command fetch)
+	0001
+	no-progress
+	want-ref refs/heads/ns-yes
+	have $oid
+	done
+	0000
+	EOF
+
+	env GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	check_output
+'
+
+test_expect_success 'want-ref outside namespace' '
+	oid=$(git -C "$REPO" rev-parse c) &&
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_command fetch)
+	0001
+	no-progress
+	want-ref refs/heads/ns-no
+	have $oid
+	done
+	0000
+	EOF
+
+	test_must_fail env GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	grep "unknown ref" out
+'
+
+test_expect_success 'hideRefs with namespaces' '
+	oid=$(git -C "$REPO" rev-parse c) &&
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_command fetch)
+	0001
+	no-progress
+	want-ref refs/heads/hidden
+	have $oid
+	done
+	0000
+	EOF
+
+	test_must_fail env GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	grep "unknown ref" out
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd

diff --git a/upload-pack.c b/upload-pack.c
index 297b76fcb4..c897802f1c 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1417,21 +1417,24 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,
 			  struct string_list *wanted_refs,
 			  struct object_array *want_obj)
 {
-	const char *arg;
-	if (skip_prefix(line, "want-ref ", &arg)) {
+	const char *refname_nons;
+	if (skip_prefix(line, "want-ref ", &refname_nons)) {
 		struct object_id oid;
 		struct string_list_item *item;
 		struct object *o;
+		struct strbuf refname = STRBUF_INIT;

-		if (read_ref(arg, &oid)) {
-			packet_writer_error(writer, "unknown ref %s", arg);
-			die("unknown ref %s", arg);
+		strbuf_addf(&refname, "%s%s", get_git_namespace(), refname_nons);
+		if (ref_is_hidden(refname_nons, refname.buf) ||
+		    read_ref(refname.buf, &oid)) {
+			packet_writer_error(writer, "unknown ref %s", refname_nons);
+			die("unknown ref %s", refname_nons);
 		}

-		item = string_list_append(wanted_refs, arg);
+		item = string_list_append(wanted_refs, refname_nons);
 		item->util = oiddup(&oid);

-		o = parse_object_or_die(&oid, arg);
+		o = parse_object_or_die(&oid, refname_nons);
 		if (!(o->flags & WANTED)) {
 			o->flags |= WANTED;
 			add_object_array(o, NULL, want_obj);
--
2.32.0

Re: [PATCH v2] upload-pack.c: treat want-ref relative to namespace

[Jonathan Tan]

> diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
> index e9e471621d..96df3073d1 100755
> --- a/t/t5703-upload-pack-ref-in-want.sh
> +++ b/t/t5703-upload-pack-ref-in-want.sh
> @@ -298,6 +298,78 @@ test_expect_success 'fetching with wildcard that matches multiple refs' '
>  	grep "want-ref refs/heads/o/bar" log
>  '
> 
> +REPO="$(pwd)/repo-ns"
> +
> +test_expect_success 'setup namespaced repo' '
> +	git init -b main "$REPO" &&
> +	cd "$REPO" &&
> +	test_commit a &&
> +	test_commit b &&
> +	git checkout a &&
> +	test_commit c &&
> +	git checkout a &&
> +	test_commit d &&
> +	git update-ref refs/heads/ns-no b &&
> +	git update-ref refs/namespaces/ns/refs/heads/ns-yes c &&
> +	git update-ref refs/namespaces/ns/refs/heads/hidden d &&
> +	git -C "$REPO" config uploadpack.allowRefInWant true &&
> +	git -C "$REPO" config transfer.hideRefs refs/heads/hidden
> +'

If you're not using a subshell to set up the repo, you should add '-C
"$REPO"' to all the "git" commands (like you do in the last 2 lines)
instead of "cd"-ing halfway through the test. The helper function
test_commit also has that facility ('test_commit -C "$REPO" a', for
example).

> +test_expect_success 'want-ref with namespaces' '
> +	oid=$(git -C "$REPO" rev-parse c) &&
> +	cat >expected_refs <<-EOF &&
> +	$oid refs/heads/ns-yes
> +	EOF
> +	>expected_commits &&
> +
> +	oid=$(git -C "$REPO" rev-parse c) &&
> +	test-tool pkt-line pack >in <<-EOF &&
> +	$(write_command fetch)
> +	0001
> +	no-progress
> +	want-ref refs/heads/ns-yes
> +	have $oid

Do we need this "have" line? I think we can just omit it, since what the
client has is irrelevant to the test. (Same for the other tests.)

> +	done
> +	0000
> +	EOF
> +
> +	env GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
> +	check_output
> +'
> +
> +test_expect_success 'want-ref outside namespace' '
> +	oid=$(git -C "$REPO" rev-parse c) &&
> +	test-tool pkt-line pack >in <<-EOF &&
> +	$(write_command fetch)
> +	0001
> +	no-progress
> +	want-ref refs/heads/ns-no
> +	have $oid
> +	done
> +	0000
> +	EOF
> +
> +	test_must_fail env GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
> +	grep "unknown ref" out
> +'

For the failure tests, it's safer to write them in pairs - one that
succeeds and one that fails. Here, a typo in "ns-no" (e.g. if I wrote
"ns-noo" instead) would cause the exact same result, but if we were to
write a pair of tests, we wouldn't have this problem.

To do this, you can bundle the same code into a function and call them
from both tests. E.g.:

  setup_want_ns_no () {
    (common code)
  }

  test_expect_success 'want-ref without namespace works...' '
    setup_want_ref_outside_namespace &&
    test-tool ... &&
    check_output
  '

  test_expect_success '...but, with namespace, does not work' '
    setup_want_ref_outside_namespace &&
    test_must_fail env GIT_NAMESPACE=ns ... &&
    grep "unknown ref" out
  '

The first test_expect_success does seem redundant, but I can't think of
a better way to ensure that the helper function (setup_want_ns_no in
this case) is written correctly.

> +test_expect_success 'hideRefs with namespaces' '
> +	oid=$(git -C "$REPO" rev-parse c) &&
> +	test-tool pkt-line pack >in <<-EOF &&
> +	$(write_command fetch)
> +	0001
> +	no-progress
> +	want-ref refs/heads/hidden
> +	have $oid
> +	done
> +	0000
> +	EOF
> +
> +	test_must_fail env GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
> +	grep "unknown ref" out
> +'
> +

Same for this.

> diff --git a/upload-pack.c b/upload-pack.c
> index 297b76fcb4..c897802f1c 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1417,21 +1417,24 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,
>  			  struct string_list *wanted_refs,
>  			  struct object_array *want_obj)
>  {
> -	const char *arg;
> -	if (skip_prefix(line, "want-ref ", &arg)) {
> +	const char *refname_nons;
> +	if (skip_prefix(line, "want-ref ", &refname_nons)) {
>  		struct object_id oid;
>  		struct string_list_item *item;
>  		struct object *o;
> +		struct strbuf refname = STRBUF_INIT;

"refname" needs to be released somewhere.

> 
> -		if (read_ref(arg, &oid)) {
> -			packet_writer_error(writer, "unknown ref %s", arg);
> -			die("unknown ref %s", arg);
> +		strbuf_addf(&refname, "%s%s", get_git_namespace(), refname_nons);
> +		if (ref_is_hidden(refname_nons, refname.buf) ||
> +		    read_ref(refname.buf, &oid)) {
> +			packet_writer_error(writer, "unknown ref %s", refname_nons);
> +			die("unknown ref %s", refname_nons);
>  		}
> 
> -		item = string_list_append(wanted_refs, arg);
> +		item = string_list_append(wanted_refs, refname_nons);
>  		item->util = oiddup(&oid);
> 
> -		o = parse_object_or_die(&oid, arg);
> +		o = parse_object_or_die(&oid, refname_nons);
>  		if (!(o->flags & WANTED)) {
>  			o->flags |= WANTED;
>  			add_object_array(o, NULL, want_obj);

Besides my comments about the tests, I think that this patch looks good.
Junio had a relevant question [1]:

> OK.  Assuming that it makes sense for the hideRefs mechanism to kick
> in here (which I would prefer to hear from others who've worked with
> this code, say Jonathan Tan?), the updated code makes sense.

I think it makes sense here. I checked my old code that Kim linked [2],
and in that patch I put it somewhere else (specifically, the part that
prints out the "wanted-ref" lines), but it makes sense that it is
different. In my version, "want-ref" supports globs and is allowed to
match 0 refs, but in Brandon's final version, "want-ref" does not
support globs and must match the ref, so checking it upon parse (as is
done in this commit) is the most reasonable.

Questions from Kim:

> Jonathan: I took the test code from your original patch introducing ref-in-want,
> but modified it substantially. Let me know if it is conventional to credit you
> anyway, and by which trailer.

I don't think there's a convention, but you can use the "Helped-by:"
trailer if you want.

> I have also updated the code for the v2 to use refname_nons for any die() calls,
> as I realised that this may be transmitted to the client via sideband (is that
> correct?).

I believe that only packet_writer_error() output is sent via sideband,
but it still makes sense for the server-side output to print out the ref
as read from the client verbatim.

[1] https://lore.kernel.org/git/xmqqbl6j1vgh.fsf@gitster.g/
[2] https://lore.kernel.org/git/d0d42b3bb4cf755f122591e191354c53848f197d.1485381677.git.jonathantanmy@google.com/

Re: [PATCH v2] upload-pack.c: treat want-ref relative to namespace

[Kim Altintop]

On Mon Aug 2, 2021 at 11:06 PM CEST, Jonathan Tan wrote:
> > +test_expect_success 'setup namespaced repo' '
> > +	git init -b main "$REPO" &&
> > +	cd "$REPO" &&
> > +	test_commit a &&
> > +	test_commit b &&
> > +	git checkout a &&
> > +	test_commit c &&
> > +	git checkout a &&
> > +	test_commit d &&
> > +	git update-ref refs/heads/ns-no b &&
> > +	git update-ref refs/namespaces/ns/refs/heads/ns-yes c &&
> > +	git update-ref refs/namespaces/ns/refs/heads/hidden d &&
> > +	git -C "$REPO" config uploadpack.allowRefInWant true &&
> > +	git -C "$REPO" config transfer.hideRefs refs/heads/hidden
> > +'
>
> If you're not using a subshell to set up the repo, you should add '-C
> "$REPO"' to all the "git" commands (like you do in the last 2 lines)
> instead of "cd"-ing halfway through the test. The helper function
> test_commit also has that facility ('test_commit -C "$REPO" a', for
> example).

Ah, that answers the question raised by Junio in the first review. I'll revert
to using a subshell, as that seems clearer and is used throughout the file.

> > +test_expect_success 'want-ref with namespaces' '
> > +	oid=$(git -C "$REPO" rev-parse c) &&
> > +	cat >expected_refs <<-EOF &&
> > +	$oid refs/heads/ns-yes
> > +	EOF
> > +	>expected_commits &&
> > +
> > +	oid=$(git -C "$REPO" rev-parse c) &&
> > +	test-tool pkt-line pack >in <<-EOF &&
> > +	$(write_command fetch)
> > +	0001
> > +	no-progress
> > +	want-ref refs/heads/ns-yes
> > +	have $oid
>
> Do we need this "have" line? I think we can just omit it, since what the
> client has is irrelevant to the test. (Same for the other tests.)

Hm no. I think I misread how this works.

> > +test_expect_success 'want-ref outside namespace' '
> > +	oid=$(git -C "$REPO" rev-parse c) &&
> > +	test-tool pkt-line pack >in <<-EOF &&
> > +	$(write_command fetch)
> > +	0001
> > +	no-progress
> > +	want-ref refs/heads/ns-no
> > +	have $oid
> > +	done
> > +	0000
> > +	EOF
> > +
> > +	test_must_fail env GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
> > +	grep "unknown ref" out
> > +'
>
> For the failure tests, it's safer to write them in pairs - one that
> succeeds and one that fails. Here, a typo in "ns-no" (e.g. if I wrote
> "ns-noo" instead) would cause the exact same result, but if we were to
> write a pair of tests, we wouldn't have this problem.

That's a good suggestion. However, I'm having some difficulties finding just the
right amount of common code to extract due to

a. having to pass the namespace via env instead of --namespace (and the
   different position for the success / test_must_fail cases), and
b. having to rely on _persistent_ config for hideRefs, as opposed to being able
   to pass it via -c to upload-pack (ie. I'm worried about tests breaking if
   they get reordered)

So I'm not exactly happy with what I came up with for v3, but I'd also be
reluctant to add --namespace / -c support to test-tool as part of this patch.
Let me know what you think.


> > diff --git a/upload-pack.c b/upload-pack.c
> > index 297b76fcb4..c897802f1c 100644
> > --- a/upload-pack.c
> > +++ b/upload-pack.c
> > @@ -1417,21 +1417,24 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,
> >  			  struct string_list *wanted_refs,
> >  			  struct object_array *want_obj)
> >  {
> > -	const char *arg;
> > -	if (skip_prefix(line, "want-ref ", &arg)) {
> > +	const char *refname_nons;
> > +	if (skip_prefix(line, "want-ref ", &refname_nons)) {
> >  		struct object_id oid;
> >  		struct string_list_item *item;
> >  		struct object *o;
> > +		struct strbuf refname = STRBUF_INIT;
>
> "refname" needs to be released somewhere.

Yeap :/

[PATCH v3] upload-pack.c: treat want-ref relative to namespace

[Kim Altintop]

When 'upload-pack' runs within the context of a git namespace, treat any
'want-ref' lines the client sends as relative to that namespace.

Also check if the wanted ref is hidden via 'hideRefs'. If it is hidden,
respond with an error as if the ref didn't exist.

Helped-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Kim Altintop <kim@eagain.st>
---

Changes from v2:

    * upload-pack.c: release strbuf
    * t5730: revert to scoping to $REPO via subshell in setup
    * t5730: add "cross-check" tests as per review comments from Jonathan

 t/t5703-upload-pack-ref-in-want.sh | 128 +++++++++++++++++++++++++++++
 upload-pack.c                      |  18 ++--
 2 files changed, 139 insertions(+), 7 deletions(-)

diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index e9e471621d..4a828042bb 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -298,6 +298,134 @@ test_expect_success 'fetching with wildcard that matches multiple refs' '
 	grep "want-ref refs/heads/o/bar" log
 '

+REPO="$(pwd)/repo-ns"
+
+write_fetch_want_ref() {
+	local wanted_ref="$1"
+
+	write_command fetch
+	echo "0001"
+	echo "no-progress"
+	echo "want-ref $1"
+	echo "done"
+	echo "0000"
+}
+
+test_expect_success 'setup namespaced repo' '
+	(
+		git init -b main "$REPO" &&
+		cd "$REPO" &&
+		test_commit a &&
+		test_commit b &&
+		git checkout a &&
+		test_commit c &&
+		git checkout a &&
+		test_commit d &&
+		git update-ref refs/heads/ns-no b &&
+		git update-ref refs/namespaces/ns/refs/heads/ns-yes c &&
+		git update-ref refs/namespaces/ns/refs/heads/hidden d
+	) &&
+	git -C "$REPO" config uploadpack.allowRefInWant true
+'
+
+test_expect_success 'with namespace: want-ref is considered relative to namespace' '
+	wanted_ref=refs/heads/ns-yes &&
+
+	oid=$(git -C "$REPO" rev-parse $wanted_ref) &&
+	cat >expected_refs <<-EOF &&
+	$oid $wanted_ref
+	EOF
+	git -C "$REPO" rev-parse a $wanted_ref >expected_commits &&
+
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_fetch_want_ref $wanted_ref)
+	EOF
+
+	env GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	check_output
+'
+
+test_expect_success 'with namespace: want-ref outside namespace is unknown' '
+	wanted_ref=refs/heads/ns-no &&
+
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_fetch_want_ref $wanted_ref)
+	EOF
+
+	test_must_fail env GIT_NAMESPACE=ns \
+		test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	grep "unknown ref" out
+'
+
+# Cross-check refs/heads/ns-no indeed exists
+test_expect_success 'without namespace: want-ref outside namespace succeeds' '
+	wanted_ref=refs/heads/ns-no &&
+
+	oid=$(git -C "$REPO" rev-parse $wanted_ref) &&
+	cat >expected_refs <<-EOF &&
+	$oid $wanted_ref
+	EOF
+	git -C "$REPO" rev-parse a $wanted_ref >expected_commits &&
+
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_fetch_want_ref $wanted_ref)
+	EOF
+
+	test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	check_output
+'
+
+test_expect_success 'with namespace: hideRefs is matched, relative to namespace' '
+	wanted_ref=refs/heads/hidden &&
+	git -C "$REPO" config transfer.hideRefs $wanted_ref &&
+
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_fetch_want_ref $wanted_ref)
+	EOF
+
+	test_must_fail env GIT_NAMESPACE=ns \
+		test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	grep "unknown ref" out
+'
+
+# Cross-check refs/heads/hidden indeed exists
+test_expect_success 'with namespace: want-ref succeeds if hideRefs is removed' '
+	wanted_ref=refs/heads/hidden &&
+	git -C "$REPO" config --unset transfer.hideRefs $wanted_ref &&
+
+	oid=$(git -C "$REPO" rev-parse $wanted_ref) &&
+	cat >expected_refs <<-EOF &&
+	$oid $wanted_ref
+	EOF
+	git -C "$REPO" rev-parse a $wanted_ref >expected_commits &&
+
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_fetch_want_ref $wanted_ref)
+	EOF
+
+	env GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	check_output
+'
+
+test_expect_success 'without namespace: relative hideRefs does not match' '
+	wanted_ref=refs/namespaces/ns/refs/heads/hidden &&
+	git -C "$REPO" config transfer.hideRefs refs/heads/hidden &&
+
+	oid=$(git -C "$REPO" rev-parse $wanted_ref) &&
+	cat >expected_refs <<-EOF &&
+	$oid $wanted_ref
+	EOF
+	git -C "$REPO" rev-parse a $wanted_ref >expected_commits &&
+
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_fetch_want_ref $wanted_ref)
+	EOF
+
+	env GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	check_output
+'
+
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd

diff --git a/upload-pack.c b/upload-pack.c
index 297b76fcb4..6ce07231d3 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1417,21 +1417,25 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,
 			  struct string_list *wanted_refs,
 			  struct object_array *want_obj)
 {
-	const char *arg;
-	if (skip_prefix(line, "want-ref ", &arg)) {
+	const char *refname_nons;
+	if (skip_prefix(line, "want-ref ", &refname_nons)) {
 		struct object_id oid;
 		struct string_list_item *item;
 		struct object *o;
+		struct strbuf refname = STRBUF_INIT;

-		if (read_ref(arg, &oid)) {
-			packet_writer_error(writer, "unknown ref %s", arg);
-			die("unknown ref %s", arg);
+		strbuf_addf(&refname, "%s%s", get_git_namespace(), refname_nons);
+		if (ref_is_hidden(refname_nons, refname.buf) ||
+		    read_ref(refname.buf, &oid)) {
+			packet_writer_error(writer, "unknown ref %s", refname_nons);
+			die("unknown ref %s", refname_nons);
 		}
+		strbuf_release(&refname);

-		item = string_list_append(wanted_refs, arg);
+		item = string_list_append(wanted_refs, refname_nons);
 		item->util = oiddup(&oid);

-		o = parse_object_or_die(&oid, arg);
+		o = parse_object_or_die(&oid, refname_nons);
 		if (!(o->flags & WANTED)) {
 			o->flags |= WANTED;
 			add_object_array(o, NULL, want_obj);
--
2.32.0

[PATCH v4] upload-pack.c: treat want-ref relative to namespace

[Kim Altintop]

When 'upload-pack' runs within the context of a git namespace, treat any
'want-ref' lines the client sends as relative to that namespace.

Also check if the wanted ref is hidden via 'hideRefs'. If it is hidden,
respond with an error as if the ref didn't exist.

Helped-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Kim Altintop <kim@eagain.st>
---

Changes from v3:

    * t5703: make tests actually pass :)


 t/t5703-upload-pack-ref-in-want.sh | 140 +++++++++++++++++++++++++++++
 upload-pack.c                      |  18 ++--
 2 files changed, 151 insertions(+), 7 deletions(-)

diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index e9e471621d..4da86d771b 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -298,6 +298,146 @@ test_expect_success 'fetching with wildcard that matches multiple refs' '
 	grep "want-ref refs/heads/o/bar" log
 '

+REPO="$(pwd)/repo-ns"
+
+write_fetch_want_ref() {
+	local wanted_ref="$1"
+
+	write_command fetch
+	echo "0001"
+	echo "no-progress"
+	echo "want-ref $1"
+	echo "done"
+	echo "0000"
+}
+
+test_expect_success 'setup namespaced repo' '
+	(
+		git init -b main "$REPO" &&
+		cd "$REPO" &&
+		test_commit a &&
+		test_commit b &&
+		git checkout a &&
+		test_commit c &&
+		git checkout a &&
+		test_commit d &&
+		git update-ref refs/heads/ns-no b &&
+		git update-ref refs/namespaces/ns/refs/heads/ns-yes c &&
+		git update-ref refs/namespaces/ns/refs/heads/hidden d
+	) &&
+	git -C "$REPO" config uploadpack.allowRefInWant true
+'
+
+test_expect_success 'with namespace: want-ref is considered relative to namespace' '
+	wanted_ref=refs/heads/ns-yes &&
+
+	oid=$(git -C "$REPO" rev-parse "refs/namespaces/ns/$wanted_ref") &&
+	cat >expected_refs <<-EOF &&
+	$oid $wanted_ref
+	EOF
+	cat >expected_commits <<-EOF &&
+	$oid
+	$(git -C "$REPO" rev-parse a)
+	EOF
+
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_fetch_want_ref $wanted_ref)
+	EOF
+
+	env GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	check_output
+'
+
+test_expect_success 'with namespace: want-ref outside namespace is unknown' '
+	wanted_ref=refs/heads/ns-no &&
+
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_fetch_want_ref $wanted_ref)
+	EOF
+
+	test_must_fail env GIT_NAMESPACE=ns \
+		test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	grep "unknown ref" out
+'
+
+# Cross-check refs/heads/ns-no indeed exists
+test_expect_success 'without namespace: want-ref outside namespace succeeds' '
+	wanted_ref=refs/heads/ns-no &&
+
+	oid=$(git -C "$REPO" rev-parse $wanted_ref) &&
+	cat >expected_refs <<-EOF &&
+	$oid $wanted_ref
+	EOF
+	cat >expected_commits <<-EOF &&
+	$oid
+	$(git -C "$REPO" rev-parse a)
+	EOF
+
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_fetch_want_ref $wanted_ref)
+	EOF
+
+	test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	check_output
+'
+
+test_expect_success 'with namespace: hideRefs is matched, relative to namespace' '
+	wanted_ref=refs/heads/hidden &&
+	git -C "$REPO" config transfer.hideRefs $wanted_ref &&
+
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_fetch_want_ref $wanted_ref)
+	EOF
+
+	test_must_fail env GIT_NAMESPACE=ns \
+		test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	grep "unknown ref" out
+'
+
+# Cross-check refs/heads/hidden indeed exists
+test_expect_success 'with namespace: want-ref succeeds if hideRefs is removed' '
+	wanted_ref=refs/heads/hidden &&
+	git -C "$REPO" config --unset transfer.hideRefs $wanted_ref &&
+
+	oid=$(git -C "$REPO" rev-parse "refs/namespaces/ns/$wanted_ref") &&
+	cat >expected_refs <<-EOF &&
+	$oid $wanted_ref
+	EOF
+	cat >expected_commits <<-EOF &&
+	$oid
+	$(git -C "$REPO" rev-parse a)
+	EOF
+
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_fetch_want_ref $wanted_ref)
+	EOF
+
+	env GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	check_output
+'
+
+test_expect_success 'without namespace: relative hideRefs does not match' '
+	wanted_ref=refs/namespaces/ns/refs/heads/hidden &&
+	git -C "$REPO" config transfer.hideRefs refs/heads/hidden &&
+
+	oid=$(git -C "$REPO" rev-parse $wanted_ref) &&
+	cat >expected_refs <<-EOF &&
+	$oid $wanted_ref
+	EOF
+	cat >expected_commits <<-EOF &&
+	$oid
+	$(git -C "$REPO" rev-parse a)
+	EOF
+
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_fetch_want_ref $wanted_ref)
+	EOF
+
+	test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	check_output
+'
+
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd

diff --git a/upload-pack.c b/upload-pack.c
index 297b76fcb4..6ce07231d3 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1417,21 +1417,25 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,
 			  struct string_list *wanted_refs,
 			  struct object_array *want_obj)
 {
-	const char *arg;
-	if (skip_prefix(line, "want-ref ", &arg)) {
+	const char *refname_nons;
+	if (skip_prefix(line, "want-ref ", &refname_nons)) {
 		struct object_id oid;
 		struct string_list_item *item;
 		struct object *o;
+		struct strbuf refname = STRBUF_INIT;

-		if (read_ref(arg, &oid)) {
-			packet_writer_error(writer, "unknown ref %s", arg);
-			die("unknown ref %s", arg);
+		strbuf_addf(&refname, "%s%s", get_git_namespace(), refname_nons);
+		if (ref_is_hidden(refname_nons, refname.buf) ||
+		    read_ref(refname.buf, &oid)) {
+			packet_writer_error(writer, "unknown ref %s", refname_nons);
+			die("unknown ref %s", refname_nons);
 		}
+		strbuf_release(&refname);

-		item = string_list_append(wanted_refs, arg);
+		item = string_list_append(wanted_refs, refname_nons);
 		item->util = oiddup(&oid);

-		o = parse_object_or_die(&oid, arg);
+		o = parse_object_or_die(&oid, refname_nons);
 		if (!(o->flags & WANTED)) {
 			o->flags |= WANTED;
 			add_object_array(o, NULL, want_obj);
--
2.32.0

Re: [PATCH v3] upload-pack.c: treat want-ref relative to namespace

[Junio C Hamano]

Kim Altintop <kim@eagain.st> writes:

> diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
> index e9e471621d..4a828042bb 100755
> --- a/t/t5703-upload-pack-ref-in-want.sh
> +++ b/t/t5703-upload-pack-ref-in-want.sh
> @@ -298,6 +298,134 @@ test_expect_success 'fetching with wildcard that matches multiple refs' '
>  	grep "want-ref refs/heads/o/bar" log
>  '
>
> +REPO="$(pwd)/repo-ns"
> +
> +write_fetch_want_ref() {

Style; missing SP before ().

> +	local wanted_ref="$1"
> +
> +	write_command fetch
> +	echo "0001"
> +	echo "no-progress"
> +	echo "want-ref $1"
> +	echo "done"
> +	echo "0000"
> +}

All other helper shell functions in this file are defined near the
top.  If we compare them with this, we notice that this does not
check for any errors.  We should string these steps together with
"&&".

Also, having this in the middle looks a bit out of place.  If this
helper is useful only in the tests that are being added by this
patch, that may be OK, but we may want to have a preliminary
clean-up step before the main patch that adds this helper function
near the top (perhaps after "write_command" is defined), and use it
in existing tests.  It seems that 'invalid want-ref line', 'basic
want-ref', and 'multiple want-ref lines' tests among others may
benefit from a slightly expanded variant, something along the lines
of ...

    write_fetch_command () {
	write_command fetch &&
	echo "0001" &&
	echo "no-progress" || return
        # want-ref ...
	# --
	# want ...
	# --
	# have ...
	# --
        while :
	do
		case $# in 0) break ;; esac &&
		case "$1" in --) shift; break ;; esac &&
		echo "want-ref $1" &&
		shift || return
	done &&
        while :
	do
		case $# in 0) break ;; esac &&
		case "$1" in --) shift; break ;; esac &&
		echo "want $1" &&
		shift || return
	done &&
        while :
	do
		case $# in 0) break ;; esac &&
		case "$1" in --) shift; break ;; esac &&
		echo "have $1" &&
		shift || return
	done &&
	echo "done" &&
	echo "0000"
    }

and with something like that, an existing test like this one:

test_expect_success 'mix want and want-ref' '
	oid=$(git rev-parse f) &&
	cat >expected_refs <<-EOF &&
	$oid refs/heads/main
	EOF
	git rev-parse e f >expected_commits &&

	test-tool pkt-line pack >in <<-EOF &&
	$(write_command fetch)
	0001
	no-progress
	want-ref refs/heads/main
	want $(git rev-parse e)
	have $(git rev-parse a)
	done
	0000
	EOF

	test-tool serve-v2 --stateless-rpc >out <in &&
	check_output
'

may become

test_expect_success 'mix want and want-ref' '
	oid=$(git rev-parse f) &&
	cat >expected_refs <<-EOF &&
	$oid refs/heads/main
	EOF
	git rev-parse e f >expected_commits &&

	test-tool pkt-line pack >in <<-EOF &&
	$(write_fetch_command
		refs/heads/main
		--
		$(git rev-parse e)
		--
		$(git rev-parse a)
	)
	EOF

	test-tool serve-v2 --stateless-rpc >out <in &&
	check_output
'

And after preparing a reusable helper like that, we can add
namespace tests using the same helper in the main patch (so it would
become an at least 2-patch series).

> +
> +test_expect_success 'setup namespaced repo' '
> +	(
> +		git init -b main "$REPO" &&
> +		cd "$REPO" &&
> +		test_commit a &&
> +		test_commit b &&
> +		git checkout a &&
> +		test_commit c &&
> +		git checkout a &&
> +		test_commit d &&
> +		git update-ref refs/heads/ns-no b &&
> +		git update-ref refs/namespaces/ns/refs/heads/ns-yes c &&
> +		git update-ref refs/namespaces/ns/refs/heads/hidden d
> +	) &&
> +	git -C "$REPO" config uploadpack.allowRefInWant true

I do not see a reason why the last step wants to be done outside the
subshell, which we would be using anyway.  Does it clarify something?

> +test_expect_success 'with namespace: want-ref is considered relative to namespace' '
> +	wanted_ref=refs/heads/ns-yes &&
> +
> +	oid=$(git -C "$REPO" rev-parse $wanted_ref) &&
> +	cat >expected_refs <<-EOF &&
> +	$oid $wanted_ref
> +	EOF
> +	git -C "$REPO" rev-parse a $wanted_ref >expected_commits &&
> +
> +	test-tool pkt-line pack >in <<-EOF &&
> +	$(write_fetch_want_ref $wanted_ref)
> +	EOF
> +
> +	env GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&

I am not sure why we want "env" in front (it does not hurt, but it
should be unnecessary, as test-tool is a plain-vanilla binary
executable, not a shell function).  Is this a workaround for a buggy
test linter or something?

> +	check_output
> +'
> +
> +test_expect_success 'with namespace: want-ref outside namespace is unknown' '
> +	wanted_ref=refs/heads/ns-no &&
> +
> +	test-tool pkt-line pack >in <<-EOF &&
> +	$(write_fetch_want_ref $wanted_ref)
> +	EOF
> +
> +	test_must_fail env GIT_NAMESPACE=ns \
> +		test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&

Note that this one does need "env" here.

> diff --git a/upload-pack.c b/upload-pack.c
> index 297b76fcb4..6ce07231d3 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1417,21 +1417,25 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,
>  			  struct string_list *wanted_refs,
>  			  struct object_array *want_obj)
>  {
> -	const char *arg;
> -	if (skip_prefix(line, "want-ref ", &arg)) {
> +	const char *refname_nons;
> +	if (skip_prefix(line, "want-ref ", &refname_nons)) {
>  		struct object_id oid;
>  		struct string_list_item *item;
>  		struct object *o;
> +		struct strbuf refname = STRBUF_INIT;
>
> -		if (read_ref(arg, &oid)) {
> -			packet_writer_error(writer, "unknown ref %s", arg);
> -			die("unknown ref %s", arg);
> +		strbuf_addf(&refname, "%s%s", get_git_namespace(), refname_nons);
> +		if (ref_is_hidden(refname_nons, refname.buf) ||
> +		    read_ref(refname.buf, &oid)) {
> +			packet_writer_error(writer, "unknown ref %s", refname_nons);
> +			die("unknown ref %s", refname_nons);

Looks good.

>  		}
> +		strbuf_release(&refname);
>
> -		item = string_list_append(wanted_refs, arg);
> +		item = string_list_append(wanted_refs, refname_nons);
>  		item->util = oiddup(&oid);
>
> -		o = parse_object_or_die(&oid, arg);
> +		o = parse_object_or_die(&oid, refname_nons);
>  		if (!(o->flags & WANTED)) {
>  			o->flags |= WANTED;
>  			add_object_array(o, NULL, want_obj);
> --
> 2.32.0

Re: [PATCH v3] upload-pack.c: treat want-ref relative to namespace

[Kim Altintop]

On Wed Aug 4, 2021 at 11:15 PM CEST, Junio C Hamano wrote:
>
> Kim Altintop <kim@eagain.st> writes:
>
> > diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
> > index e9e471621d..4a828042bb 100755
> > --- a/t/t5703-upload-pack-ref-in-want.sh
> > +++ b/t/t5703-upload-pack-ref-in-want.sh
> > @@ -298,6 +298,134 @@ test_expect_success 'fetching with wildcard that matches multiple refs' '
> >  	grep "want-ref refs/heads/o/bar" log
> >  '
> >
> > +REPO="$(pwd)/repo-ns"
> > +
> > +write_fetch_want_ref() {
>
> Style; missing SP before ().
>
> > +	local wanted_ref="$1"
> > +
> > +	write_command fetch
> > +	echo "0001"
> > +	echo "no-progress"
> > +	echo "want-ref $1"
> > +	echo "done"
> > +	echo "0000"
> > +}
>
> All other helper shell functions in this file are defined near the
> top. If we compare them with this, we notice that this does not
> check for any errors. We should string these steps together with
> "&&".
>
> Also, having this in the middle looks a bit out of place. If this
> helper is useful only in the tests that are being added by this
> patch, that may be OK, but we may want to have a preliminary
> clean-up step before the main patch that adds this helper function
> near the top (perhaps after "write_command" is defined), and use it
> in existing tests. It seems that 'invalid want-ref line', 'basic
> want-ref', and 'multiple want-ref lines' tests among others may
> benefit from a slightly expanded variant, something along the lines
> of ...
>
> write_fetch_command () {
> write_command fetch &&
> echo "0001" &&
> echo "no-progress" || return
> # want-ref ...
> # --
> # want ...
> # --
> # have ...
> # --
> while :
> do
> case $# in 0) break ;; esac &&
> case "$1" in --) shift; break ;; esac &&
> echo "want-ref $1" &&
> shift || return
> done &&
> while :
> do
> case $# in 0) break ;; esac &&
> case "$1" in --) shift; break ;; esac &&
> echo "want $1" &&
> shift || return
> done &&
> while :
> do
> case $# in 0) break ;; esac &&
> case "$1" in --) shift; break ;; esac &&
> echo "have $1" &&
> shift || return
> done &&
> echo "done" &&
> echo "0000"
> }
>
> and with something like that, an existing test like this one:
>
> test_expect_success 'mix want and want-ref' '
> oid=$(git rev-parse f) &&
> cat >expected_refs <<-EOF &&
> $oid refs/heads/main
> EOF
> git rev-parse e f >expected_commits &&
>
> test-tool pkt-line pack >in <<-EOF &&
> $(write_command fetch)
> 0001
> no-progress
> want-ref refs/heads/main
> want $(git rev-parse e)
> have $(git rev-parse a)
> done
> 0000
> EOF
>
> test-tool serve-v2 --stateless-rpc >out <in &&
> check_output
> '
>
> may become
>
> test_expect_success 'mix want and want-ref' '
> oid=$(git rev-parse f) &&
> cat >expected_refs <<-EOF &&
> $oid refs/heads/main
> EOF
> git rev-parse e f >expected_commits &&
>
> test-tool pkt-line pack >in <<-EOF &&
> $(write_fetch_command
> refs/heads/main
> --
> $(git rev-parse e)
> --
> $(git rev-parse a)
> )
> EOF
>
> test-tool serve-v2 --stateless-rpc >out <in &&
> check_output
> '
>
> And after preparing a reusable helper like that, we can add
> namespace tests using the same helper in the main patch (so it would
> become an at least 2-patch series).


Ok got it. I did not want to come forward with a grand refactoring of that test
in my very first patch, but since reviewers seem to think it's in order I'll
give it a spin.


> > +
> > +test_expect_success 'setup namespaced repo' '
> > +	(
> > +		git init -b main "$REPO" &&
> > +		cd "$REPO" &&
> > +		test_commit a &&
> > +		test_commit b &&
> > +		git checkout a &&
> > +		test_commit c &&
> > +		git checkout a &&
> > +		test_commit d &&
> > +		git update-ref refs/heads/ns-no b &&
> > +		git update-ref refs/namespaces/ns/refs/heads/ns-yes c &&
> > +		git update-ref refs/namespaces/ns/refs/heads/hidden d
> > +	) &&
> > +	git -C "$REPO" config uploadpack.allowRefInWant true
>
> I do not see a reason why the last step wants to be done outside the
> subshell, which we would be using anyway. Does it clarify something?

I agree, but was sticking to the style that was already there. I'll then revise
the other setup steps as part of the refactor.


> > +test_expect_success 'with namespace: want-ref is considered relative to namespace' '
> > +	wanted_ref=refs/heads/ns-yes &&
> > +
> > +	oid=$(git -C "$REPO" rev-parse $wanted_ref) &&
> > +	cat >expected_refs <<-EOF &&
> > +	$oid $wanted_ref
> > +	EOF
> > +	git -C "$REPO" rev-parse a $wanted_ref >expected_commits &&
> > +
> > +	test-tool pkt-line pack >in <<-EOF &&
> > +	$(write_fetch_want_ref $wanted_ref)
> > +	EOF
> > +
> > +	env GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
>
> I am not sure why we want "env" in front (it does not hurt, but it
> should be unnecessary, as test-tool is a plain-vanilla binary
> executable, not a shell function). Is this a workaround for a buggy
> test linter or something?

The linter did indeed ask me to write `GIT_NAMESPACE=ns && export GIT_NAMESPACE
&& test-tool ...` in v1 of the patch, but now it doesn't... nevermind, I must
have held something the wrong way.

Re: [PATCH v3] upload-pack.c: treat want-ref relative to namespace

[Eric Sunshine]

On Wed, Aug 4, 2021 at 6:04 PM Kim Altintop <kim@eagain.st> wrote:
> On Wed Aug 4, 2021 at 11:15 PM CEST, Junio C Hamano wrote:
> > Kim Altintop <kim@eagain.st> writes:
> > > +   env GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
> >
> > I am not sure why we want "env" in front (it does not hurt, but it
> > should be unnecessary, as test-tool is a plain-vanilla binary
> > executable, not a shell function). Is this a workaround for a buggy
> > test linter or something?
>
> The linter did indeed ask me to write `GIT_NAMESPACE=ns && export GIT_NAMESPACE
> && test-tool ...` in v1 of the patch, but now it doesn't... nevermind, I must
> have held something the wrong way.

The linter will complain about unportable:

    export FOO=bar

and ask you to write it as:

    FOO=bar && export FOO

which is probably what you encountered.

It will also complain about

    FOO=bar some-command

which should be rewritten as:

    env FOO=bar some-command

if, and only if, `some-command` is a shell function. If not a shell
function, then:

    FOO=bar some-command

is just fine.

Re: [PATCH v3] upload-pack.c: treat want-ref relative to namespace

[Junio C Hamano]

Kim Altintop <kim@eagain.st> writes:

>> > +	) &&
>> > +	git -C "$REPO" config uploadpack.allowRefInWant true
>>
>> I do not see a reason why the last step wants to be done outside the
>> subshell, which we would be using anyway. Does it clarify something?
>
> I agree, but was sticking to the style that was already there. I'll then revise
> the other setup steps as part of the refactor.

Touching existing tests only for this, unlike the creation of a
helper out of existing test to improve reuse, does not improve
anything.  I was curious if there was a good reason to create in a
subshell and configure in a separate step, and "others do the same"
is good enough to satisfy the curiousity.

Thanks.

Re: [PATCH v3] upload-pack.c: treat want-ref relative to namespace

[Junio C Hamano]

Kim Altintop <kim@eagain.st> writes:

> ...
>> test-tool pkt-line pack >in <<-EOF &&
>> $(write_fetch_command
>> refs/heads/main
>> --
>> $(git rev-parse e)
>> --
>> $(git rev-parse a)
>> )

Sorry, I missed all the necessary backslashes at the end of these lines.

[PATCH 0/3] upload-pack: treat want-ref relative to namespace

[Kim Altintop]

Fifth reroll of [0].

The patch has been split into three commits, where the first is a small
refactoring of the test file, the second is the original patch + tests, and the
last one attempts to express the `transfer.hideRefs` behaviour more accurately
in the docs.

Thanks for your help and patience!

This version of the patch is also available at [1].

[0]: https://lore.kernel.org/git/20210804205951.668140-1-kim@eagain.st/
[1]: https://github.com/kim/git/tree/ka/namespaced-want-ref-v5

Kim Altintop (3):
  t5730: introduce fetch command helper
  upload-pack.c: treat want-ref relative to namespace
  docs: clarify the interaction of transfer.hideRefs and namespaces

 Documentation/config/transfer.txt  |  17 ++-
 t/t5703-upload-pack-ref-in-want.sh | 236 +++++++++++++++++++++++++----
 upload-pack.c                      |  18 ++-
 3 files changed, 224 insertions(+), 47 deletions(-)

--
2.32.0

[PATCH 1/3] t5730: introduce fetch command helper

[Kim Altintop]

Assembling a "raw" fetch command to be fed directly to "test-tool serve-v2"
is extracted into a test helper.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kim Altintop <kim@eagain.st>
---
 t/t5703-upload-pack-ref-in-want.sh | 107 ++++++++++++++++++++---------
 1 file changed, 74 insertions(+), 33 deletions(-)

diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index e9e471621d..cd4744b016 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -40,6 +40,54 @@ write_command () {
 	fi
 }

+# Write a complete fetch command to stdout, suitable for use with `test-tool
+# pkt-line`. "want-ref", "want", and "have" values can be given in this order,
+# with sections separated by "--".
+#
+# Examples:
+#
+# write_fetch_command refs/heads/main
+#
+# write_fetch_command \
+#	refs/heads/main \
+#	-- \
+#	-- \
+#	$(git rev-parse x)
+#
+# write_fetch_command \
+#	--
+#	$(git rev-parse a) \
+#	--
+#	$(git rev-parse b)
+write_fetch_command () {
+	write_command fetch &&
+	echo "0001" &&
+	echo "no-progress" || return
+    while :
+	do
+		case $# in 0) break ;; esac &&
+		case "$1" in --) shift; break ;; esac &&
+		echo "want-ref $1" &&
+		shift || return
+	done &&
+    while :
+	do
+		case $# in 0) break ;; esac &&
+		case "$1" in --) shift; break ;; esac &&
+		echo "want $1" &&
+		shift || return
+	done &&
+    while :
+	do
+		case $# in 0) break ;; esac &&
+		case "$1" in --) shift; break ;; esac &&
+		echo "have $1" &&
+		shift || return
+	done &&
+	echo "done" &&
+	echo "0000"
+}
+
 # c(o/foo) d(o/bar)
 #        \ /
 #         b   e(baz)  f(main)
@@ -97,15 +145,13 @@ test_expect_success 'basic want-ref' '
 	EOF
 	git rev-parse f >expected_commits &&

-	oid=$(git rev-parse a) &&
 	test-tool pkt-line pack >in <<-EOF &&
-	$(write_command fetch)
-	0001
-	no-progress
-	want-ref refs/heads/main
-	have $oid
-	done
-	0000
+	$(write_fetch_command \
+		refs/heads/main \
+		-- \
+		-- \
+		$(git rev-parse a) \
+	)
 	EOF

 	test-tool serve-v2 --stateless-rpc >out <in &&
@@ -121,16 +167,14 @@ test_expect_success 'multiple want-ref lines' '
 	EOF
 	git rev-parse c d >expected_commits &&

-	oid=$(git rev-parse b) &&
 	test-tool pkt-line pack >in <<-EOF &&
-	$(write_command fetch)
-	0001
-	no-progress
-	want-ref refs/heads/o/foo
-	want-ref refs/heads/o/bar
-	have $oid
-	done
-	0000
+	$(write_fetch_command \
+		refs/heads/o/foo \
+		refs/heads/o/bar \
+		-- \
+		-- \
+		$(git rev-parse b) \
+	)
 	EOF

 	test-tool serve-v2 --stateless-rpc >out <in &&
@@ -145,14 +189,13 @@ test_expect_success 'mix want and want-ref' '
 	git rev-parse e f >expected_commits &&

 	test-tool pkt-line pack >in <<-EOF &&
-	$(write_command fetch)
-	0001
-	no-progress
-	want-ref refs/heads/main
-	want $(git rev-parse e)
-	have $(git rev-parse a)
-	done
-	0000
+	$(write_fetch_command \
+		refs/heads/main \
+		-- \
+		$(git rev-parse e) \
+		-- \
+		$(git rev-parse a) \
+	)
 	EOF

 	test-tool serve-v2 --stateless-rpc >out <in &&
@@ -166,15 +209,13 @@ test_expect_success 'want-ref with ref we already have commit for' '
 	EOF
 	>expected_commits &&

-	oid=$(git rev-parse c) &&
 	test-tool pkt-line pack >in <<-EOF &&
-	$(write_command fetch)
-	0001
-	no-progress
-	want-ref refs/heads/o/foo
-	have $oid
-	done
-	0000
+	$(write_fetch_command \
+		refs/heads/o/foo \
+		-- \
+		-- \
+		$(git rev-parse c) \
+	)
 	EOF

 	test-tool serve-v2 --stateless-rpc >out <in &&
--
2.32.0

[PATCH 2/3] upload-pack.c: treat want-ref relative to namespace

[Kim Altintop]

When 'upload-pack' runs within the context of a git namespace, treat any
'want-ref' lines the client sends as relative to that namespace.

Also check if the wanted ref is hidden via 'hideRefs'. If it is hidden,
respond with an error as if the ref didn't exist.

Helped-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Kim Altintop <kim@eagain.st>
---
 t/t5703-upload-pack-ref-in-want.sh | 129 +++++++++++++++++++++++++++++
 upload-pack.c                      |  18 ++--
 2 files changed, 140 insertions(+), 7 deletions(-)

diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index cd4744b016..3e1e0b8fe1 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -339,6 +339,135 @@ test_expect_success 'fetching with wildcard that matches multiple refs' '
 	grep "want-ref refs/heads/o/bar" log
 '

+REPO="$(pwd)/repo-ns"
+
+test_expect_success 'setup namespaced repo' '
+	(
+		git init -b main "$REPO" &&
+		cd "$REPO" &&
+		test_commit a &&
+		test_commit b &&
+		git checkout a &&
+		test_commit c &&
+		git checkout a &&
+		test_commit d &&
+		git update-ref refs/heads/ns-no b &&
+		git update-ref refs/namespaces/ns/refs/heads/ns-yes c &&
+		git update-ref refs/namespaces/ns/refs/heads/hidden d
+	) &&
+	git -C "$REPO" config uploadpack.allowRefInWant true
+'
+
+test_expect_success 'with namespace: want-ref is considered relative to namespace' '
+	wanted_ref=refs/heads/ns-yes &&
+
+	oid=$(git -C "$REPO" rev-parse "refs/namespaces/ns/$wanted_ref") &&
+	cat >expected_refs <<-EOF &&
+	$oid $wanted_ref
+	EOF
+	cat >expected_commits <<-EOF &&
+	$oid
+	$(git -C "$REPO" rev-parse a)
+	EOF
+
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_fetch_command $wanted_ref)
+	EOF
+
+	GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	check_output
+'
+
+test_expect_success 'with namespace: want-ref outside namespace is unknown' '
+	wanted_ref=refs/heads/ns-no &&
+
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_fetch_command $wanted_ref)
+	EOF
+
+	test_must_fail env GIT_NAMESPACE=ns \
+		test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	grep "unknown ref" out
+'
+
+# Cross-check refs/heads/ns-no indeed exists
+test_expect_success 'without namespace: want-ref outside namespace succeeds' '
+	wanted_ref=refs/heads/ns-no &&
+
+	oid=$(git -C "$REPO" rev-parse $wanted_ref) &&
+	cat >expected_refs <<-EOF &&
+	$oid $wanted_ref
+	EOF
+	cat >expected_commits <<-EOF &&
+	$oid
+	$(git -C "$REPO" rev-parse a)
+	EOF
+
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_fetch_command $wanted_ref)
+	EOF
+
+	test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	check_output
+'
+
+test_expect_success 'with namespace: hideRefs is matched, relative to namespace' '
+	wanted_ref=refs/heads/hidden &&
+	git -C "$REPO" config transfer.hideRefs $wanted_ref &&
+
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_fetch_command $wanted_ref)
+	EOF
+
+	test_must_fail env GIT_NAMESPACE=ns \
+		test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	grep "unknown ref" out
+'
+
+# Cross-check refs/heads/hidden indeed exists
+test_expect_success 'with namespace: want-ref succeeds if hideRefs is removed' '
+	wanted_ref=refs/heads/hidden &&
+	git -C "$REPO" config --unset transfer.hideRefs $wanted_ref &&
+
+	oid=$(git -C "$REPO" rev-parse "refs/namespaces/ns/$wanted_ref") &&
+	cat >expected_refs <<-EOF &&
+	$oid $wanted_ref
+	EOF
+	cat >expected_commits <<-EOF &&
+	$oid
+	$(git -C "$REPO" rev-parse a)
+	EOF
+
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_fetch_command $wanted_ref)
+	EOF
+
+	GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	check_output
+'
+
+test_expect_success 'without namespace: relative hideRefs does not match' '
+	wanted_ref=refs/namespaces/ns/refs/heads/hidden &&
+	git -C "$REPO" config transfer.hideRefs refs/heads/hidden &&
+
+	oid=$(git -C "$REPO" rev-parse $wanted_ref) &&
+	cat >expected_refs <<-EOF &&
+	$oid $wanted_ref
+	EOF
+	cat >expected_commits <<-EOF &&
+	$oid
+	$(git -C "$REPO" rev-parse a)
+	EOF
+
+	test-tool pkt-line pack >in <<-EOF &&
+	$(write_fetch_command $wanted_ref)
+	EOF
+
+	test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	check_output
+'
+
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd

diff --git a/upload-pack.c b/upload-pack.c
index 297b76fcb4..6ce07231d3 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1417,21 +1417,25 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,
 			  struct string_list *wanted_refs,
 			  struct object_array *want_obj)
 {
-	const char *arg;
-	if (skip_prefix(line, "want-ref ", &arg)) {
+	const char *refname_nons;
+	if (skip_prefix(line, "want-ref ", &refname_nons)) {
 		struct object_id oid;
 		struct string_list_item *item;
 		struct object *o;
+		struct strbuf refname = STRBUF_INIT;

-		if (read_ref(arg, &oid)) {
-			packet_writer_error(writer, "unknown ref %s", arg);
-			die("unknown ref %s", arg);
+		strbuf_addf(&refname, "%s%s", get_git_namespace(), refname_nons);
+		if (ref_is_hidden(refname_nons, refname.buf) ||
+		    read_ref(refname.buf, &oid)) {
+			packet_writer_error(writer, "unknown ref %s", refname_nons);
+			die("unknown ref %s", refname_nons);
 		}
+		strbuf_release(&refname);

-		item = string_list_append(wanted_refs, arg);
+		item = string_list_append(wanted_refs, refname_nons);
 		item->util = oiddup(&oid);

-		o = parse_object_or_die(&oid, arg);
+		o = parse_object_or_die(&oid, refname_nons);
 		if (!(o->flags & WANTED)) {
 			o->flags |= WANTED;
 			add_object_array(o, NULL, want_obj);
--
2.32.0

[PATCH 3/3] docs: clarify the interaction of transfer.hideRefs and namespaces

[Kim Altintop]

Expand the section about namespaces in the documentation of
`transfer.hideRefs` to point out the subtle differences between
`upload-pack` and `receive-pack`.

9bedd82017 (upload-pack.c: treat want-ref relative to namespace,
2021-07-30) taught `upload-pack` to reject `want-ref`s for hidden refs,
which is now documented.

Signed-off-by: Kim Altintop <kim@eagain.st>
---
 Documentation/config/transfer.txt | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt
index 505126a780..09ebb399ce 100644
--- a/Documentation/config/transfer.txt
+++ b/Documentation/config/transfer.txt
@@ -52,13 +52,16 @@ If you have multiple hideRefs values, later entries override earlier ones
 (and entries in more-specific config files override less-specific ones).
 +
 If a namespace is in use, the namespace prefix is stripped from each
-reference before it is matched against `transfer.hiderefs` patterns.
-For example, if `refs/heads/master` is specified in `transfer.hideRefs` and
-the current namespace is `foo`, then `refs/namespaces/foo/refs/heads/master`
-is omitted from the advertisements but `refs/heads/master` and
-`refs/namespaces/bar/refs/heads/master` are still advertised as so-called
-"have" lines. In order to match refs before stripping, add a `^` in front of
-the ref name. If you combine `!` and `^`, `!` must be specified first.
+reference before it is matched against `transfer.hiderefs` patterns. For
+example, if `refs/heads/master` is specified in `transfer.hideRefs` and the
+current namespace is `foo`, then `refs/namespaces/foo/refs/heads/master` is
+omitted from the advertisements. If `uploadpack.allowRefInWant` is set,
+`upload-pack` will treat `want-ref refs/heads/master` in a protocol v2
+`fetch` command as if `refs/heads/master` was unknown. Note, however, that
+`receive-pack` will still advertise the object id `refs/heads/master` is
+pointing to, but will conceil the name of the ref. In order to match refs
+before stripping, add a `^` in front of the ref name. If you combine `!` and
+`^`, `!` must be specified first.
 +
 Even if you hide refs, a client may still be able to steal the target
 objects via the techniques described in the "SECURITY" section of the
--
2.32.0

Re: [PATCH 1/3] t5730: introduce fetch command helper

[Junio C Hamano]

Kim Altintop <kim@eagain.st> writes:

> Assembling a "raw" fetch command to be fed directly to "test-tool serve-v2"
> is extracted into a test helper.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Kim Altintop <kim@eagain.st>
> ---
>  t/t5703-upload-pack-ref-in-want.sh | 107 ++++++++++++++++++++---------
>  1 file changed, 74 insertions(+), 33 deletions(-)
>
> diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
> index e9e471621d..cd4744b016 100755
> --- a/t/t5703-upload-pack-ref-in-want.sh
> +++ b/t/t5703-upload-pack-ref-in-want.sh
> @@ -40,6 +40,54 @@ write_command () {
>  	fi
>  }
>
> +# Write a complete fetch command to stdout, suitable for use with `test-tool
> +# pkt-line`. "want-ref", "want", and "have" values can be given in this order,
> +# with sections separated by "--".
> +#
> +# Examples:
> +#
> +# write_fetch_command refs/heads/main
> +#
> +# write_fetch_command \
> +#	refs/heads/main \
> +#	-- \
> +#	-- \
> +#	$(git rev-parse x)
> +#
> +# write_fetch_command \
> +#	--
> +#	$(git rev-parse a) \
> +#	--
> +#	$(git rev-parse b)

Have a blank line here (or a line with "#" and nothing else) and it
would become easier to read.

> +write_fetch_command () {
> +	write_command fetch &&
> +	echo "0001" &&
> +	echo "no-progress" || return

The "while :" in this helper function are indented with 4 spaces,
not a single tab.

> +    while :
> +	do
> +		case $# in 0) break ;; esac &&
> +		case "$1" in --) shift; break ;; esac &&
> +		echo "want-ref $1" &&
> +		shift || return
> +	done &&
> +    while :
> +	do
> +		case $# in 0) break ;; esac &&
> +		case "$1" in --) shift; break ;; esac &&
> +		echo "want $1" &&
> +		shift || return
> +	done &&
> +    while :
> +	do
> +		case $# in 0) break ;; esac &&
> +		case "$1" in --) shift; break ;; esac &&
> +		echo "have $1" &&
> +		shift || return
> +	done &&
> +	echo "done" &&
> +	echo "0000"
> +}

The error checking of the helper function seems to be reasonable,
but ...

>  # c(o/foo) d(o/bar)
>  #        \ /
>  #         b   e(baz)  f(main)
> @@ -97,15 +145,13 @@ test_expect_success 'basic want-ref' '
>  	EOF
>  	git rev-parse f >expected_commits &&
>
>  	test-tool pkt-line pack >in <<-EOF &&
> +	$(write_fetch_command \
> +		refs/heads/main \
> +		-- \
> +		-- \
> +		$(git rev-parse a) \
> +	)
>  	EOF
>  	test-tool serve-v2 --stateless-rpc >out <in &&

... the code that uses the helper needs rewriting to make use of
it.  A failure in $(write_fetch_command) here will not cause the
caller to stop here.  If we had any "git" command used in the
helper, I would recommand restructuring the caller to do something
like:

    write_fetch_command >pkt-src \
	refs/heads/main \
	-- \
	-- \
	$(git rev-parse a) &&
    test-tool pkt-line pack <pkt-src >in &&
    test-tool serve-v2 --stateless-rpc >out <in &&

but it probably may not be necessary (we only use "echo" there, and
it probably is not worth worrying about 'echo' failing).

The call to $(git rev-parse a) might fail when rev-parse gets
broken, and I think the rewritten version would catch such a
breakage while the one inside $(write_fetch_command) in the here-doc
would not be.

Re: [PATCH 1/3] t5730: introduce fetch command helper

[Jonathan Nieder]

Hi,

Kim Altintop wrote:

> Assembling a "raw" fetch command to be fed directly to "test-tool serve-v2"
> is extracted into a test helper.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Kim Altintop <kim@eagain.st>
> ---
>  t/t5703-upload-pack-ref-in-want.sh | 107 ++++++++++++++++++++---------
>  1 file changed, 74 insertions(+), 33 deletions(-)

Thanks!  Interesting.

> diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
> index e9e471621d..cd4744b016 100755
> --- a/t/t5703-upload-pack-ref-in-want.sh
> +++ b/t/t5703-upload-pack-ref-in-want.sh
> @@ -40,6 +40,54 @@ write_command () {
>  	fi
>  }
> 
> +# Write a complete fetch command to stdout, suitable for use with `test-tool
> +# pkt-line`. "want-ref", "want", and "have" values can be given in this order,
> +# with sections separated by "--".
> +#
> +# Examples:
> +#
> +# write_fetch_command refs/heads/main
> +#
> +# write_fetch_command \
> +#	refs/heads/main \
> +#	-- \
> +#	-- \
> +#	$(git rev-parse x)
> +#
> +# write_fetch_command \
> +#	--
> +#	$(git rev-parse a) \
> +#	--
> +#	$(git rev-parse b)
> +write_fetch_command () {

Hm, for comparison let me see what this looks like without the helper:
after some prior step

	object_format=$(test_oid algo) &&	# probably just once in a setup step
	x=$(git rev-parse x) &&

we can write

	cat <<-EOF &&
		command=fetch
		object-format=$object_format
		0001
		no-progress
		want-ref refs/heads/main
		have $x
		done
		0000
	EOF

I find that a little _easier_ to read than a write_fetch_command call,
because I don't have to chase the definition and x is labeled as a
'have'.

Is there some additional motivation for this helper?

> +	write_command fetch &&
> +	echo "0001" &&
> +	echo "no-progress" || return
> +    while :

Whitespace seems off.  Junio covers this in his review so I'll ignore
other instances.

[...]
> @@ -97,15 +145,13 @@ test_expect_success 'basic want-ref' '
>  	EOF
>  	git rev-parse f >expected_commits &&
> 
> -	oid=$(git rev-parse a) &&
>  	test-tool pkt-line pack >in <<-EOF &&
> -	$(write_command fetch)
> -	0001
> -	no-progress
> -	want-ref refs/heads/main
> -	have $oid
> -	done
> -	0000
> +	$(write_fetch_command \
> +		refs/heads/main \
> +		-- \
> +		-- \
> +		$(git rev-parse a) \
> +	)
>  	EOF
> 
>  	test-tool serve-v2 --stateless-rpc >out <in &&
> @@ -121,16 +167,14 @@ test_expect_success 'multiple want-ref lines' '
>  	EOF
>  	git rev-parse c d >expected_commits &&
> 
> -	oid=$(git rev-parse b) &&
>  	test-tool pkt-line pack >in <<-EOF &&
> -	$(write_command fetch)
> -	0001
> -	no-progress
> -	want-ref refs/heads/o/foo
> -	want-ref refs/heads/o/bar
> -	have $oid
> -	done
> -	0000
> +	$(write_fetch_command \
> +		refs/heads/o/foo \
> +		refs/heads/o/bar \
> +		-- \
> +		-- \
> +		$(git rev-parse b) \
> +	)
>  	EOF

Here the entirety of the input to "test-tool pkt-line pack" is the
entirety of the output from write_fetch_command, which would suggest
either

 a. making write_fetch_command pipe its output to "test-tool pkt-line
    pack", or

 b. using a pipe instead of a command substitution, like
    "write_fetch_command ... | test-tool pkt-line pack >in"

(although as mentioned above, I think it's simpler to inline the
write_fetch_command and even the write_command as well).

Thanks and hope that helps,
Jonathan

Re: [PATCH 1/3] t5730: introduce fetch command helper

[Kim Altintop]

On Mon Aug 9, 2021 at 9:16 PM CEST, Junio C Hamano wrote:
> >
> > +# Write a complete fetch command to stdout, suitable for use with `test-tool
> > +# pkt-line`. "want-ref", "want", and "have" values can be given in this order,
> > +# with sections separated by "--".
> > +#
> > +# Examples:
> > +#
> > +# write_fetch_command refs/heads/main
> > +#
> > +# write_fetch_command \
> > +#	refs/heads/main \
> > +#	-- \
> > +#	-- \
> > +#	$(git rev-parse x)
> > +#
> > +# write_fetch_command \
> > +#	--
> > +#	$(git rev-parse a) \
> > +#	--
> > +#	$(git rev-parse b)
>
> Have a blank line here (or a line with "#" and nothing else) and it
> would become easier to read.

Noted.

>
> > +write_fetch_command () {
> > +	write_command fetch &&
> > +	echo "0001" &&
> > +	echo "no-progress" || return
>
> The "while :" in this helper function are indented with 4 spaces,
> not a single tab.

Noted, sorry.

I can't seem to be able teach `git diff --check` to exit non-zero on whitespace
errors.

> >  # c(o/foo) d(o/bar)
> >  #        \ /
> >  #         b   e(baz)  f(main)
> > @@ -97,15 +145,13 @@ test_expect_success 'basic want-ref' '
> >  	EOF
> >  	git rev-parse f >expected_commits &&
> >
> >  	test-tool pkt-line pack >in <<-EOF &&
> > +	$(write_fetch_command \
> > +		refs/heads/main \
> > +		-- \
> > +		-- \
> > +		$(git rev-parse a) \
> > +	)
> >  	EOF
> >  	test-tool serve-v2 --stateless-rpc >out <in &&
>
> ... the code that uses the helper needs rewriting to make use of
> it. A failure in $(write_fetch_command) here will not cause the
> caller to stop here. If we had any "git" command used in the
> helper, I would recommand restructuring the caller to do something
> like:
>
> write_fetch_command >pkt-src \
> 	refs/heads/main \
> 	-- \
> 	-- \
> 	$(git rev-parse a) &&
> test-tool pkt-line pack <pkt-src >in &&
> test-tool serve-v2 --stateless-rpc >out <in &&
>
> but it probably may not be necessary (we only use "echo" there, and
> it probably is not worth worrying about 'echo' failing).
>
> The call to $(git rev-parse a) might fail when rev-parse gets
> broken, and I think the rewritten version would catch such a
> breakage while the one inside $(write_fetch_command) in the here-doc
> would not be.

Heh, this is turning into an exercise to forget everything about bash :/

I didn't know that an error in a nested command substitution would not cause the
outer one to fail. As we can't use a pipe either (as Jonathan Nieder suggests),
I think a redirection like this is indeed necessary. The only other option I
could think of would be assigning rev-parse to a variable, which arguably is
less readable:

    have=$(git rew-parse a) &&
    test-tool pkt-line pack >in <<-EOF &&
    $(write_fetch_command \
	    refs/heads/main \
	    -- \
	    -- \
	    $have)
    EOF


Thanks

Re: [PATCH 1/3] t5730: introduce fetch command helper

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Here the entirety of the input to "test-tool pkt-line pack" is the
> entirety of the output from write_fetch_command, which would suggest
> either
>
>  a. making write_fetch_command pipe its output to "test-tool pkt-line
>     pack", or
>
>  b. using a pipe instead of a command substitution, like
>     "write_fetch_command ... | test-tool pkt-line pack >in"
>
> (although as mentioned above, I think it's simpler to inline the
> write_fetch_command and even the write_command as well).

Yeah, write_command was not saving all that much to begin with.  I
was hoping that there were a lot more commonality that we can save
some typing.

Re: [PATCH 1/3] t5730: introduce fetch command helper

[Kim Altintop]

Thanks for chiming in!

On Mon Aug 9, 2021 at 9:40 PM CEST, Jonathan Nieder wrote:
> Hm, for comparison let me see what this looks like without the helper:
> after some prior step
>
>	 object_format=$(test_oid algo) && # probably just once in a setup step
>	 x=$(git rev-parse x) &&
>
> we can write
>
>	 cat <<-EOF &&
>	 command=fetch
>	 object-format=$object_format
>	 0001
>	 no-progress
>	 want-ref refs/heads/main
>	 have $x
>	 done
>	 0000
>	 EOF
>
> I find that a little _easier_ to read than a write_fetch_command call,
> because I don't have to chase the definition and x is labeled as a
> 'have'.
>
> Is there some additional motivation for this helper?

It was suggested in earlier review rounds. I think it does improve readability
as quite some lines need to be repeated everywhere a fetch command is assembled.
I agree though that not having some sort of "named arguments" is a bit
detrimental.

>
> >  	test-tool serve-v2 --stateless-rpc >out <in &&
> > @@ -121,16 +167,14 @@ test_expect_success 'multiple want-ref lines' '
> >  	EOF
> >  	git rev-parse c d >expected_commits &&
> >
> > -	oid=$(git rev-parse b) &&
> >  	test-tool pkt-line pack >in <<-EOF &&
> > -	$(write_command fetch)
> > -	0001
> > -	no-progress
> > -	want-ref refs/heads/o/foo
> > -	want-ref refs/heads/o/bar
> > -	have $oid
> > -	done
> > -	0000
> > +	$(write_fetch_command \
> > +		refs/heads/o/foo \
> > +		refs/heads/o/bar \
> > +		-- \
> > +		-- \
> > +		$(git rev-parse b) \
> > +	)
> >  	EOF
>
> Here the entirety of the input to "test-tool pkt-line pack" is the
> entirety of the output from write_fetch_command, which would suggest
> either
>
> a. making write_fetch_command pipe its output to "test-tool pkt-line
> pack", or
>
> b. using a pipe instead of a command substitution, like
> "write_fetch_command ... | test-tool pkt-line pack >in"
>
> (although as mentioned above, I think it's simpler to inline the
> write_fetch_command and even the write_command as well).

Yes, although I believe a pipe cannot be used as we don't have bash's `set -o
pipefail` (ie. the exit status will always be the status of the last command in
the pipe, even if an earlier one failed).

Perhaps an alternative would be:

	write_fetch_command () {
		write_command fetch &&
		echo "0001" &&
		echo "no-progress" &&
		cat /dev/stdin &&
		echo "done" &&
		echo "0000"
 	}


Which would then be called like so:

	write_fetch_command >pkt_cmd <<-EOF &&
	want-ref refs/heads/main
	have $(git rev-parse a)
	EOF
	test-tool pkt-line pack <pkt_cmd >in &&
	test-tool serve-v2 --stateless-rpc >out <in &&


I'm not sure how portable that is, though. Maybe using `while read -r` instead
of `cat /dev/stdin`?

Re: [PATCH 1/3] t5730: introduce fetch command helper

[Junio C Hamano]

Kim Altintop <kim@eagain.st> writes:

> Perhaps an alternative would be:
>
> 	write_fetch_command () {
> 		write_command fetch &&
> 		echo "0001" &&
> 		echo "no-progress" &&
> 		cat /dev/stdin &&
> 		echo "done" &&
> 		echo "0000"
>  	}
>
>
> Which would then be called like so:
>
> 	write_fetch_command >pkt_cmd <<-EOF &&
> 	want-ref refs/heads/main
> 	have $(git rev-parse a)
> 	EOF
> 	test-tool pkt-line pack <pkt_cmd >in &&
> 	test-tool serve-v2 --stateless-rpc >out <in &&
>
>
> I'm not sure how portable that is, though. Maybe using `while read -r` instead
> of `cat /dev/stdin`?

If you drop /dev/stdin, the result would be emimently portable.
"cat" without any argument reads from the standard input stream
and copies it to the standard output stream.

Re: [PATCH 1/3] t5730: introduce fetch command helper

[Jonathan Nieder]

Junio C Hamano wrote:
> Kim Altintop <kim@eagain.st> writes:

>> Perhaps an alternative would be:
>>
>> 	write_fetch_command () {
>> 		write_command fetch &&
>> 		echo "0001" &&
>> 		echo "no-progress" &&
>> 		cat /dev/stdin &&
>> 		echo "done" &&
>> 		echo "0000"
>>  	}
>>
>>
>> Which would then be called like so:
>>
>> 	write_fetch_command >pkt_cmd <<-EOF &&
>> 	want-ref refs/heads/main
>> 	have $(git rev-parse a)
>> 	EOF
>> 	test-tool pkt-line pack <pkt_cmd >in &&
>> 	test-tool serve-v2 --stateless-rpc >out <in &&
>>
>>
>> I'm not sure how portable that is, though. Maybe using `while read -r` instead
>> of `cat /dev/stdin`?
>
> If you drop /dev/stdin, the result would be emimently portable.
> "cat" without any argument reads from the standard input stream
> and copies it to the standard output stream.

Yep, with that tweak (using "cat" instead of "cat /dev/stdin") this
looks like a pleasant enough helper.

Jonathan

Re: [PATCH 1/3] t5730: introduce fetch command helper

[Kim Altintop]

Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Junio C Hamano wrote:
> > Kim Altintop <kim@eagain.st> writes:
>
> >> Perhaps an alternative would be:
> >>
> >> 	write_fetch_command () {
> >> 		write_command fetch &&
> >> 		echo "0001" &&
> >> 		echo "no-progress" &&
> >> 		cat /dev/stdin &&
> >> 		echo "done" &&
> >> 		echo "0000"
> >>  	}
> >>
> >>
> >> Which would then be called like so:
> >>
> >> 	write_fetch_command >pkt_cmd <<-EOF &&
> >> 	want-ref refs/heads/main
> >> 	have $(git rev-parse a)
> >> 	EOF
> >> 	test-tool pkt-line pack <pkt_cmd >in &&
> >> 	test-tool serve-v2 --stateless-rpc >out <in &&
> >>
> >>
> >> I'm not sure how portable that is, though. Maybe using `while read -r` instead
> >> of `cat /dev/stdin`?
> >
> > If you drop /dev/stdin, the result would be emimently portable.
> > "cat" without any argument reads from the standard input stream
> > and copies it to the standard output stream.
>
> Yep, with that tweak (using "cat" instead of "cat /dev/stdin") this
> looks like a pleasant enough helper.

Great, I'll reroll like this then. Thanks!

Re: [PATCH 3/3] docs: clarify the interaction of transfer.hideRefs and namespaces

[Kim Altintop]

> diff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt
> index 505126a780..09ebb399ce 100644
> --- a/Documentation/config/transfer.txt
> +++ b/Documentation/config/transfer.txt
> @@ -52,13 +52,16 @@ If you have multiple hideRefs values, later entries override earlier ones
>  (and entries in more-specific config files override less-specific ones).
>  +
>  If a namespace is in use, the namespace prefix is stripped from each
> -reference before it is matched against `transfer.hiderefs` patterns.
> -For example, if `refs/heads/master` is specified in `transfer.hideRefs` and
> -the current namespace is `foo`, then `refs/namespaces/foo/refs/heads/master`
> -is omitted from the advertisements but `refs/heads/master` and
> -`refs/namespaces/bar/refs/heads/master` are still advertised as so-called
> -"have" lines. In order to match refs before stripping, add a `^` in front of
> -the ref name. If you combine `!` and `^`, `!` must be specified first.
> +reference before it is matched against `transfer.hiderefs` patterns. For
> +example, if `refs/heads/master` is specified in `transfer.hideRefs` and the
> +current namespace is `foo`, then `refs/namespaces/foo/refs/heads/master` is
> +omitted from the advertisements. If `uploadpack.allowRefInWant` is set,
> +`upload-pack` will treat `want-ref refs/heads/master` in a protocol v2
> +`fetch` command as if `refs/heads/master` was unknown. Note, however, that
> +`receive-pack` will still advertise the object id `refs/heads/master` is
> +pointing to, but will conceil the name of the ref. In order to match refs
> +before stripping, add a `^` in front of the ref name. If you combine `!` and
> +`^`, `!` must be specified first.
>  +
>  Even if you hide refs, a client may still be able to steal the target
>  objects via the techniques described in the "SECURITY" section of the

I'd appreciate some feedback on this one before rerolling. Having looked at the
code many times recently, it makes sense to me, but that could be different for
someone with less fresh memory. Thanks!

[PATCH v6 0/3] upload-pack: treat want-ref relative to namespace

[Kim Altintop]

Round six:

* simplifies the test helper in 1/3 as per review discussion
* rephrases the doc change in 3/3 to make it read less dense
* 2/3 only updates the tests to use the revised helper

CC'ing Johannes Schindelin as suggested by git-contacts.


Published-As: https://github.com/kim/git/tree/ka/namespaced-want-ref-v6

Kim Altintop (3):
  t5730: introduce fetch command helper
  upload-pack.c: treat want-ref relative to namespace
  docs: clarify the interaction of transfer.hideRefs and namespaces

 Documentation/config/transfer.txt  |  14 +-
 t/t5703-upload-pack-ref-in-want.sh | 208 ++++++++++++++++++++++++-----
 upload-pack.c                      |  18 ++-
 3 files changed, 192 insertions(+), 48 deletions(-)

--
2.32.0

[PATCH v6 1/3] t5730: introduce fetch command helper

[Kim Altintop]

Assembling a "raw" fetch command to be fed directly to "test-tool serve-v2"
is extracted into a test helper.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kim Altintop <kim@eagain.st>
---
 t/t5703-upload-pack-ref-in-want.sh | 73 +++++++++++++++---------------
 1 file changed, 37 insertions(+), 36 deletions(-)

diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index e9e471621d..3dad21ff45 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -40,6 +40,30 @@ write_command () {
 	fi
 }

+# Write a complete fetch command to stdout, suitable for use with `test-tool
+# pkt-line`. "want-ref", "want", and "have" lines are read from stdin.
+#
+# Examples:
+#
+# write_fetch_command <<-EOF
+# want-ref refs/heads/main
+# have $(git rev-parse a)
+# EOF
+#
+# write_fetch_command <<-EOF
+# want $(git rev-parse b)
+# have $(git rev-parse a)
+# EOF
+#
+write_fetch_command () {
+	write_command fetch &&
+	echo "0001" &&
+	echo "no-progress" &&
+	cat &&
+	echo "done" &&
+	echo "0000"
+}
+
 # c(o/foo) d(o/bar)
 #        \ /
 #         b   e(baz)  f(main)
@@ -77,15 +101,11 @@ test_expect_success 'config controls ref-in-want advertisement' '
 '

 test_expect_success 'invalid want-ref line' '
-	test-tool pkt-line pack >in <<-EOF &&
-	$(write_command fetch)
-	0001
-	no-progress
+	write_fetch_command >pkt <<-EOF &&
 	want-ref refs/heads/non-existent
-	done
-	0000
 	EOF

+	test-tool pkt-line pack <pkt >in &&
 	test_must_fail test-tool serve-v2 --stateless-rpc 2>out <in &&
 	grep "unknown ref" out
 '
@@ -97,16 +117,11 @@ test_expect_success 'basic want-ref' '
 	EOF
 	git rev-parse f >expected_commits &&

-	oid=$(git rev-parse a) &&
-	test-tool pkt-line pack >in <<-EOF &&
-	$(write_command fetch)
-	0001
-	no-progress
+	write_fetch_command >pkt <<-EOF &&
 	want-ref refs/heads/main
-	have $oid
-	done
-	0000
+	have $(git rev-parse a)
 	EOF
+	test-tool pkt-line pack <pkt >in &&

 	test-tool serve-v2 --stateless-rpc >out <in &&
 	check_output
@@ -121,17 +136,12 @@ test_expect_success 'multiple want-ref lines' '
 	EOF
 	git rev-parse c d >expected_commits &&

-	oid=$(git rev-parse b) &&
-	test-tool pkt-line pack >in <<-EOF &&
-	$(write_command fetch)
-	0001
-	no-progress
+	write_fetch_command >pkt <<-EOF &&
 	want-ref refs/heads/o/foo
 	want-ref refs/heads/o/bar
-	have $oid
-	done
-	0000
+	have $(git rev-parse b)
 	EOF
+	test-tool pkt-line pack <pkt >in &&

 	test-tool serve-v2 --stateless-rpc >out <in &&
 	check_output
@@ -144,16 +154,12 @@ test_expect_success 'mix want and want-ref' '
 	EOF
 	git rev-parse e f >expected_commits &&

-	test-tool pkt-line pack >in <<-EOF &&
-	$(write_command fetch)
-	0001
-	no-progress
+	write_fetch_command >pkt <<-EOF &&
 	want-ref refs/heads/main
 	want $(git rev-parse e)
 	have $(git rev-parse a)
-	done
-	0000
 	EOF
+	test-tool pkt-line pack <pkt >in &&

 	test-tool serve-v2 --stateless-rpc >out <in &&
 	check_output
@@ -166,16 +172,11 @@ test_expect_success 'want-ref with ref we already have commit for' '
 	EOF
 	>expected_commits &&

-	oid=$(git rev-parse c) &&
-	test-tool pkt-line pack >in <<-EOF &&
-	$(write_command fetch)
-	0001
-	no-progress
+	write_fetch_command >pkt <<-EOF &&
 	want-ref refs/heads/o/foo
-	have $oid
-	done
-	0000
+	have $(git rev-parse c)
 	EOF
+	test-tool pkt-line pack <pkt >in &&

 	test-tool serve-v2 --stateless-rpc >out <in &&
 	check_output
--
2.32.0

[PATCH v6 2/3] upload-pack.c: treat want-ref relative to namespace

[Kim Altintop]

When 'upload-pack' runs within the context of a git namespace, treat any
'want-ref' lines the client sends as relative to that namespace.

Also check if the wanted ref is hidden via 'hideRefs'. If it is hidden,
respond with an error as if the ref didn't exist.

Helped-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Kim Altintop <kim@eagain.st>
---
 t/t5703-upload-pack-ref-in-want.sh | 135 +++++++++++++++++++++++++++++
 upload-pack.c                      |  18 ++--
 2 files changed, 146 insertions(+), 7 deletions(-)

diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index 3dad21ff45..220098523a 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -299,6 +299,141 @@ test_expect_success 'fetching with wildcard that matches multiple refs' '
 	grep "want-ref refs/heads/o/bar" log
 '

+REPO="$(pwd)/repo-ns"
+
+test_expect_success 'setup namespaced repo' '
+	(
+		git init -b main "$REPO" &&
+		cd "$REPO" &&
+		test_commit a &&
+		test_commit b &&
+		git checkout a &&
+		test_commit c &&
+		git checkout a &&
+		test_commit d &&
+		git update-ref refs/heads/ns-no b &&
+		git update-ref refs/namespaces/ns/refs/heads/ns-yes c &&
+		git update-ref refs/namespaces/ns/refs/heads/hidden d
+	) &&
+	git -C "$REPO" config uploadpack.allowRefInWant true
+'
+
+test_expect_success 'with namespace: want-ref is considered relative to namespace' '
+	wanted_ref=refs/heads/ns-yes &&
+
+	oid=$(git -C "$REPO" rev-parse "refs/namespaces/ns/$wanted_ref") &&
+	cat >expected_refs <<-EOF &&
+	$oid $wanted_ref
+	EOF
+	cat >expected_commits <<-EOF &&
+	$oid
+	$(git -C "$REPO" rev-parse a)
+	EOF
+
+	write_fetch_command >pkt <<-EOF &&
+	want-ref $wanted_ref
+	EOF
+	test-tool pkt-line pack <pkt >in &&
+
+	GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	check_output
+'
+
+test_expect_success 'with namespace: want-ref outside namespace is unknown' '
+	wanted_ref=refs/heads/ns-no &&
+
+	write_fetch_command >pkt <<-EOF &&
+	want-ref $wanted_ref
+	EOF
+	test-tool pkt-line pack <pkt >in &&
+
+	test_must_fail env GIT_NAMESPACE=ns \
+		test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	grep "unknown ref" out
+'
+
+# Cross-check refs/heads/ns-no indeed exists
+test_expect_success 'without namespace: want-ref outside namespace succeeds' '
+	wanted_ref=refs/heads/ns-no &&
+
+	oid=$(git -C "$REPO" rev-parse $wanted_ref) &&
+	cat >expected_refs <<-EOF &&
+	$oid $wanted_ref
+	EOF
+	cat >expected_commits <<-EOF &&
+	$oid
+	$(git -C "$REPO" rev-parse a)
+	EOF
+
+	write_fetch_command >pkt <<-EOF &&
+	want-ref $wanted_ref
+	EOF
+	test-tool pkt-line pack <pkt >in &&
+
+	test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	check_output
+'
+
+test_expect_success 'with namespace: hideRefs is matched, relative to namespace' '
+	wanted_ref=refs/heads/hidden &&
+	git -C "$REPO" config transfer.hideRefs $wanted_ref &&
+
+	write_fetch_command >pkt <<-EOF &&
+	want-ref $wanted_ref
+	EOF
+	test-tool pkt-line pack <pkt >in &&
+
+	test_must_fail env GIT_NAMESPACE=ns \
+		test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	grep "unknown ref" out
+'
+
+# Cross-check refs/heads/hidden indeed exists
+test_expect_success 'with namespace: want-ref succeeds if hideRefs is removed' '
+	wanted_ref=refs/heads/hidden &&
+	git -C "$REPO" config --unset transfer.hideRefs $wanted_ref &&
+
+	oid=$(git -C "$REPO" rev-parse "refs/namespaces/ns/$wanted_ref") &&
+	cat >expected_refs <<-EOF &&
+	$oid $wanted_ref
+	EOF
+	cat >expected_commits <<-EOF &&
+	$oid
+	$(git -C "$REPO" rev-parse a)
+	EOF
+
+	write_fetch_command >pkt <<-EOF &&
+	want-ref $wanted_ref
+	EOF
+	test-tool pkt-line pack <pkt >in &&
+
+	GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	check_output
+'
+
+test_expect_success 'without namespace: relative hideRefs does not match' '
+	wanted_ref=refs/namespaces/ns/refs/heads/hidden &&
+	git -C "$REPO" config transfer.hideRefs refs/heads/hidden &&
+
+	oid=$(git -C "$REPO" rev-parse $wanted_ref) &&
+	cat >expected_refs <<-EOF &&
+	$oid $wanted_ref
+	EOF
+	cat >expected_commits <<-EOF &&
+	$oid
+	$(git -C "$REPO" rev-parse a)
+	EOF
+
+	write_fetch_command >pkt <<-EOF &&
+	want-ref $wanted_ref
+	EOF
+	test-tool pkt-line pack <pkt >in &&
+
+	test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
+	check_output
+'
+
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd

diff --git a/upload-pack.c b/upload-pack.c
index 297b76fcb4..6ce07231d3 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -1417,21 +1417,25 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,
 			  struct string_list *wanted_refs,
 			  struct object_array *want_obj)
 {
-	const char *arg;
-	if (skip_prefix(line, "want-ref ", &arg)) {
+	const char *refname_nons;
+	if (skip_prefix(line, "want-ref ", &refname_nons)) {
 		struct object_id oid;
 		struct string_list_item *item;
 		struct object *o;
+		struct strbuf refname = STRBUF_INIT;

-		if (read_ref(arg, &oid)) {
-			packet_writer_error(writer, "unknown ref %s", arg);
-			die("unknown ref %s", arg);
+		strbuf_addf(&refname, "%s%s", get_git_namespace(), refname_nons);
+		if (ref_is_hidden(refname_nons, refname.buf) ||
+		    read_ref(refname.buf, &oid)) {
+			packet_writer_error(writer, "unknown ref %s", refname_nons);
+			die("unknown ref %s", refname_nons);
 		}
+		strbuf_release(&refname);

-		item = string_list_append(wanted_refs, arg);
+		item = string_list_append(wanted_refs, refname_nons);
 		item->util = oiddup(&oid);

-		o = parse_object_or_die(&oid, arg);
+		o = parse_object_or_die(&oid, refname_nons);
 		if (!(o->flags & WANTED)) {
 			o->flags |= WANTED;
 			add_object_array(o, NULL, want_obj);
--
2.32.0

[PATCH v6 3/3] docs: clarify the interaction of transfer.hideRefs and namespaces

[Kim Altintop]

Expand the section about namespaces in the documentation of
`transfer.hideRefs` to point out the subtle differences between
`upload-pack` and `receive-pack`.

ffcfb68176 (upload-pack.c: treat want-ref relative to namespace,
2021-07-30) taught `upload-pack` to reject `want-ref`s for hidden refs,
which is now mentioned. It is clarified that at no point the name of a
hidden ref is revealed, but the object id it points to may.

Signed-off-by: Kim Altintop <kim@eagain.st>
---
 Documentation/config/transfer.txt | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt
index 505126a780..b49429eb4d 100644
--- a/Documentation/config/transfer.txt
+++ b/Documentation/config/transfer.txt
@@ -52,13 +52,17 @@ If you have multiple hideRefs values, later entries override earlier ones
 (and entries in more-specific config files override less-specific ones).
 +
 If a namespace is in use, the namespace prefix is stripped from each
-reference before it is matched against `transfer.hiderefs` patterns.
+reference before it is matched against `transfer.hiderefs` patterns. In
+order to match refs before stripping, add a `^` in front of the ref name. If
+you combine `!` and `^`, `!` must be specified first.
++
 For example, if `refs/heads/master` is specified in `transfer.hideRefs` and
 the current namespace is `foo`, then `refs/namespaces/foo/refs/heads/master`
-is omitted from the advertisements but `refs/heads/master` and
-`refs/namespaces/bar/refs/heads/master` are still advertised as so-called
-"have" lines. In order to match refs before stripping, add a `^` in front of
-the ref name. If you combine `!` and `^`, `!` must be specified first.
+is omitted from the advertisements. If `uploadpack.allowRefInWant` is set,
+`upload-pack` will treat `want-ref refs/heads/master` in a protocol v2
+`fetch` command as if `refs/namespaces/foo/refs/heads/master` did not exist.
+`receive-pack`, on the other hand, will still advertise the object id the
+ref is pointing to without mentioning its name (a so-called ".have" line).
 +
 Even if you hide refs, a client may still be able to steal the target
 objects via the techniques described in the "SECURITY" section of the
--
2.32.0

Re: [PATCH v6 0/3] upload-pack: treat want-ref relative to namespace

[Johannes Schindelin]

Hi Kim,

On Fri, 13 Aug 2021, Kim Altintop wrote:

> CC'ing Johannes Schindelin as suggested by git-contacts.

`git-contacts` wouldn't know that there are better experts on the
namespace matter.

My only comment is that I would find the diff to `upload-pack.c` much
easier to parse if the `arg` variable hadn't been renamed.

Ciao,
Johannes

Re: [PATCH v6 0/3] upload-pack: treat want-ref relative to namespace

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> My only comment is that I would find the diff to `upload-pack.c` much
> easier to parse if the `arg` variable hadn't been renamed.

If we only look at the patch text, perhaps.

What is left in the resulting file is what people will be reading in
the coming years, and because instead of just one string (whose name
did not matter and a short-and-sweet arg was a perfect name for it),
we now have to deal with two strings (one is the full and real name,
and the other is the name without namespace prefix) we need to be
clear which is which, the rename is an essential part of the patch
to keep the result easier to understand.

When I apply a patch to my tree to review it (e-mailed patch review
is merely an efficient way to transmit the change---I do not expect
that I can always review all non-trivial patches without stepping
out of my MUA).  My review, after noticing typos, style glitches and
design issues, of code and its correctness will begin after that.

"git show" can be driven with different options and in some cases
use of certaion options helps seeing what is going on quite well.
"--diff-algorithm" shifts which preimage lines match which postimage
lines and sometimes helps reduce the clutter greatly. "--word-diff"
is a great tool to see where in reflowed documentation patch actual
changes are. "--color-moved" lets reviewer skip pure code movement,
various options to "ignore space" sometimes helps declutter the
patch. etc.

I wonder if we already have an option (or if there isn't, if we can
design such a new option cleanly), that would help in this case.
Essentially, what you want is an "I know what used to be arg is
renamed to refname_nons in this patch; take advantage of that
knowledge and show a diff that is less cluttered" option.

Thanks for a comment, anyway.

Re: [PATCH v6 0/3] upload-pack: treat want-ref relative to namespace

[Kim Altintop]

Hi Johannes,

thanks for your response. This is my very first patch to git.git, and a lot of
it is learning about the conventions and expectations towards contributors, as
well as getting familiar with the tooling while trying to avoid silly mistakes.
So please bear with me.

On Sat, 14 Aug 2021 Johannes Schindelin wrote:

> > CC'ing Johannes Schindelin as suggested by git-contacts.
>
> `git-contacts` wouldn't know that there are better experts on the
> namespace matter.

I can see now that this could come across weird. I should've written:

"CC'ing Johannes Schindelin, who started to turn up in `git-contacts` output,
although I couldn't quite infer why. I haven't received any feedback about the
documentation change yet, and didn't have much success trying to find reviewers
by inspecting the history (parts of the file where moved). I am assuming that
`git-contacts` is better than me at this, and Johannes' name shows up because of
touching the documentation. Johannes: feel free to ignore if this assumption is
wrong."

With this said, if you have any suggestions about finding reviewers for specific
parts of a patch, or who are the experts on a more cross-cutting topic, I would
appreciate if you'd share them!

> My only comment is that I would find the diff to `upload-pack.c` much
> easier to parse if the `arg` variable hadn't been renamed.

Can you explain why? Just because the diff would be smaller? I can see that in a
larger patch it might have been preferable to put the rename into a separate
commit, but in a hunk-sized change it seemed fine. It is also that this
particular naming ("refname_nons") is used in other places in upload-pack.c, so
it seemed obvious that, if I introduce namespace handling where it was
previously missing, the terminology (if you will) should be the same.

From you comment, it seems like the proposer of a patch should assume that the
reviewers only look at the diff as sent in the email, and not any context.
Junio's response suggests something else, but I guess it's fair that if someone
feels like they got CC'ed by mistake, they're not going to spend too much time.

So my question from above stands: are there better ways to find the right people
to CC, especially for newbies?

Thanks,
Kim

Re: [PATCH v6 0/3] upload-pack: treat want-ref relative to namespace

[Johannes Schindelin]

Hi Kim,

On Sun, 15 Aug 2021, Kim Altintop wrote:

> On Sat, 14 Aug 2021 Johannes Schindelin wrote:
>
> > My only comment is that I would find the diff to `upload-pack.c` much
> > easier to parse if the `arg` variable hadn't been renamed.
>
> Can you explain why?

Yes. I prefer patches to be really obvious. That way, it is really easy to
spot bugs.

In this instance, the same patch that introduces a conditional block
_also_ renames an involved variable.

To satisfy myself that the patch does what is intended, I therefore have
to virtually split the patch into the rename part and the
added-conditional-block part.

It would be easier for me if the modifications were presented as two
separate patches. And I could imagine that I am not alone in this: you
yourself might also have an easier time looking at the commits in six
months from now if those concerns are separated into their own commit.

> Just because the diff would be smaller? I can see that in a larger patch
> it might have been preferable to put the rename into a separate commit,
> but in a hunk-sized change it seemed fine. It is also that this
> particular naming ("refname_nons") is used in other places in
> upload-pack.c, so it seemed obvious that, if I introduce namespace
> handling where it was previously missing, the terminology (if you will)
> should be the same.
>
> From you comment, it seems like the proposer of a patch should assume
> that the reviewers only look at the diff as sent in the email, and not
> any context. Junio's response suggests something else, but I guess it's
> fair that if someone feels like they got CC'ed by mistake, they're not
> going to spend too much time.

In this instance, I indeed did not spend more time than on reviewing the
patch, simply because I am (currently, at least) not all that familiar
with the `upload-pack.c` machinery. I probably touched it in the past, but
for the moment, all I can comment on is the shape of the patch series,
which is what I did.

> So my question from above stands: are there better ways to find the
> right people to CC, especially for newbies?

When I look for reviewers in projects other than the ones where I know the
usual reviewers' special areas of interest, I like to pick a function at
the center of my contribution, then look at `git log -L
:<function>:<file>` and try to figure out who was the last person to
implement non-stylistic changes on that function. This has worked
relatively well for me, in the past. Maybe it can help you here, too?

Ciao,
Dscho