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

[Kim Altintop <kim@eagain.st>]

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.