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

On Fri Jul 30, 2021 at 8:57 PM CEST, Junio C Hamano wrote:

Kim Altintop [off-list ref] writes:

[..]

quoted

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.

quoted

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

quoted

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.

quoted

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