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

[Junio C Hamano <gitster@pobox.com>]

Kim Altintop <kim@eagain.st> writes:

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

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

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

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

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


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