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

On Mon Aug 2, 2021 at 11:06 PM CEST, Jonathan Tan wrote:

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

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.

quoted

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

quoted

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

quoted

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