git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 4, 2021, 21:15 UTC
Message-ID
<xmqq35rooqr5.fsf@gitster.g>
In-Reply-To
<20210804203829.661565-1-kim@eagain.st>
Kim Altintop <kim@eagain.st> writes:
Show 11 quoted lines
> 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 ().
Show 9 quoted lines
> +	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).

Show 16 quoted lines
> +
> +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?

Show 14 quoted lines
> +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?

Show 12 quoted lines
> +	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.
Show 25 quoted lines
> 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.
Show 14 quoted lines
>  		}
> +		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
Previous: Kim AltintopNext: Kim Altintop
Message 31 of 35 in “upload-pack.c: treat want-ref relative to namespace”
  1. upload-pack.c: treat want-ref relative to namespaceKim Altintop, Jul 30, 2021
  2. Kim AltintopJul 30, 2021
  3. Junio C HamanoJul 30, 2021
  4. Kim AltintopJul 30, 2021
  5. upload-pack.c: treat want-ref relative to namespaceKim Altintop, Jul 31, 2021
  6. Jonathan TanAug 2, 2021
  7. Kim AltintopAug 4, 2021
  8. upload-pack.c: treat want-ref relative to namespaceKim Altintop, Aug 4, 2021
  9. upload-pack.c: treat want-ref relative to namespaceKim Altintop, Aug 4, 2021
  10. 0/3 upload-pack: treat want-ref relative to namespaceKim Altintop, Aug 9, 2021
  11. 1/3 t5730: introduce fetch command helperKim Altintop, Aug 9, 2021
  12. Junio C HamanoAug 9, 2021
  13. Kim AltintopAug 9, 2021
  14. Jonathan NiederAug 9, 2021
  15. Junio C HamanoAug 9, 2021
  16. Kim AltintopAug 9, 2021
  17. Junio C HamanoAug 9, 2021
  18. Jonathan NiederAug 9, 2021
  19. Kim AltintopAug 10, 2021
  20. 2/3 upload-pack.c: treat want-ref relative to namespaceKim Altintop, Aug 9, 2021
  21. 3/3 docs: clarify the interaction of transfer.hideRefs and namespacesKim Altintop, Aug 9, 2021
  22. Kim AltintopAug 10, 2021
  23. 0/3 upload-pack: treat want-ref relative to namespaceKim Altintop, Aug 13, 2021
  24. Johannes SchindelinAug 14, 2021
  25. Junio C HamanoAug 15, 2021
  26. Kim AltintopAug 15, 2021
  27. Johannes SchindelinAug 16, 2021
  28. 1/3 t5730: introduce fetch command helperKim Altintop, Aug 13, 2021
  29. 2/3 upload-pack.c: treat want-ref relative to namespaceKim Altintop, Aug 13, 2021
  30. 3/3 docs: clarify the interaction of transfer.hideRefs and namespacesKim Altintop, Aug 13, 2021
  31. Junio C HamanoAug 4, 2021
  32. Kim AltintopAug 4, 2021
  33. Eric SunshineAug 4, 2021
  34. Junio C HamanoAug 4, 2021
  35. Junio C HamanoAug 4, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.