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

[Jonathan Tan <jonathantanmy@google.com>]

> diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
> index e9e471621d..96df3073d1 100755
> --- a/t/t5703-upload-pack-ref-in-want.sh
> +++ b/t/t5703-upload-pack-ref-in-want.sh
> @@ -298,6 +298,78 @@ test_expect_success 'fetching with wildcard that matches multiple refs' '
>  	grep "want-ref refs/heads/o/bar" log
>  '
> 
> +REPO="$(pwd)/repo-ns"
> +
> +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
> +'

If you're not using a subshell to set up the repo, you should add '-C
"$REPO"' to all the "git" commands (like you do in the last 2 lines)
instead of "cd"-ing halfway through the test. The helper function
test_commit also has that facility ('test_commit -C "$REPO" a', for
example).

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

Do we need this "have" line? I think we can just omit it, since what the
client has is irrelevant to the test. (Same for the other tests.)

> +	done
> +	0000
> +	EOF
> +
> +	env GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
> +	check_output
> +'
> +
> +test_expect_success 'want-ref outside namespace' '
> +	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-no
> +	have $oid
> +	done
> +	0000
> +	EOF
> +
> +	test_must_fail env GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
> +	grep "unknown ref" out
> +'

For the failure tests, it's safer to write them in pairs - one that
succeeds and one that fails. Here, a typo in "ns-no" (e.g. if I wrote
"ns-noo" instead) would cause the exact same result, but if we were to
write a pair of tests, we wouldn't have this problem.

To do this, you can bundle the same code into a function and call them
from both tests. E.g.:

  setup_want_ns_no () {
    (common code)
  }

  test_expect_success 'want-ref without namespace works...' '
    setup_want_ref_outside_namespace &&
    test-tool ... &&
    check_output
  '

  test_expect_success '...but, with namespace, does not work' '
    setup_want_ref_outside_namespace &&
    test_must_fail env GIT_NAMESPACE=ns ... &&
    grep "unknown ref" out
  '

The first test_expect_success does seem redundant, but I can't think of
a better way to ensure that the helper function (setup_want_ns_no in
this case) is written correctly.

> +test_expect_success 'hideRefs with namespaces' '
> +	oid=$(git -C "$REPO" rev-parse c) &&
> +	test-tool pkt-line pack >in <<-EOF &&
> +	$(write_command fetch)
> +	0001
> +	no-progress
> +	want-ref refs/heads/hidden
> +	have $oid
> +	done
> +	0000
> +	EOF
> +
> +	test_must_fail env GIT_NAMESPACE=ns test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in &&
> +	grep "unknown ref" out
> +'
> +

Same for this.

> diff --git a/upload-pack.c b/upload-pack.c
> index 297b76fcb4..c897802f1c 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;
> -	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;

"refname" needs to be released somewhere.

> 
> -		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);
>  		}
> 
> -		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);

Besides my comments about the tests, I think that this patch looks good.
Junio had a relevant question [1]:

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

I think it makes sense here. I checked my old code that Kim linked [2],
and in that patch I put it somewhere else (specifically, the part that
prints out the "wanted-ref" lines), but it makes sense that it is
different. In my version, "want-ref" supports globs and is allowed to
match 0 refs, but in Brandon's final version, "want-ref" does not
support globs and must match the ref, so checking it upon parse (as is
done in this commit) is the most reasonable.

Questions from Kim:

> Jonathan: I took the test code from your original patch introducing ref-in-want,
> but modified it substantially. Let me know if it is conventional to credit you
> anyway, and by which trailer.

I don't think there's a convention, but you can use the "Helped-by:"
trailer if you want.

> I have also updated the code for the v2 to use refname_nons for any die() calls,
> as I realised that this may be transmitted to the client via sideband (is that
> correct?).

I believe that only packet_writer_error() output is sent via sideband,
but it still makes sense for the server-side output to print out the ref
as read from the client verbatim.

[1] https://lore.kernel.org/git/xmqqbl6j1vgh.fsf@gitster.g/
[2] https://lore.kernel.org/git/d0d42b3bb4cf755f122591e191354c53848f197d.1485381677.git.jonathantanmy@google.com/