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

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

From
JTJonathan Tan <jonathantanmy@google.com>
Date
Aug 2, 2021, 21:06 UTC
Message-ID
<20210802210644.3432544-1-jonathantanmy@google.com>
In-Reply-To
<20210731203415.618641-1-kim@eagain.st>
Show 25 quoted lines
> 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).

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

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

Show 23 quoted lines
> +	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.

Show 16 quoted lines
> +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.
Show 16 quoted lines
> 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.
Show 20 quoted lines
> 
> -		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/

Previous: Kim AltintopNext: Kim Altintop
Message 6 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.