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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 30, 2021, 18:57 UTC
Message-ID: <xmqqbl6j1vgh.fsf@gitster.g>
In-Reply-To: <20210730135845.633234-1-kim@eagain.st>

Kim Altintop <kim@eagain.st> writes:

Show 9 quoted lines

> When 'upload-pack' runs within the context of a git namespace, treat any
> 'want-ref' lines the client sends as relative to that namespace.
>
> 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.
>
> Signed-off-by: Kim Altintop <kim@eagain.st>
> ---

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?) I wonder if the last sentence is making the description more confusing without adding any clarity. In other words, would this work as a replacement for the second paragraph, or does it say something different from what you wanted to say?

    Requests for any ref, even those that are marked to be hidden
    via the 'transfer.hideRefs' configuration, were allowed but it
    is problematic for such and such reasons.  Respond with an error
    if a requested ref is to be hidden.

I couldn't tell why you thought it was problematic, so left "for such and such reasons" to be filled in, but there still may be an issue.

How does the error response look like? We shouldn't be saying "you requested for the hidden/x branch, but you are not allowed to do so, as that is hidden". To hide something, we should pretend that the thing does not exist, so that we can hide even the fact that we are hiding it.

To help future readers of "git log" who find this change from you, we should clarify the "respond with an error" part of your proposed log message (e.g. "pretend that the wanted ref does not exist when it is hidden via the 'transfer.hiderefs' configuration" or something else).

Show 17 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 &&
> +    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.

Show 22 quoted lines

> +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
> +	done
> +	0000
> +	EOF
> +
> +	GIT_NAMESPACE=ns && export GIT_NAMESPACE &&
> +	test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
> +	check_output
> +'

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.

Show 11 quoted lines

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

Show 14 quoted lines

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

Thanks.

Show 11 quoted lines

> -		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);
>  		if (!(o->flags & WANTED)) {
>  			o->flags |= WANTED;
>  			add_object_array(o, NULL, want_obj);
> --
> 2.32.0