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

On Wed Aug 4, 2021 at 11:15 PM CEST, Junio C Hamano wrote:

Kim Altintop [off-list ref] writes:

quoted

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

quoted

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

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

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.

quoted

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