All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: kim@eagain.st
Cc: git@vger.kernel.org, bwilliams.eng@gmail.com, gitster@pobox.com,
	jonathantanmy@google.com
Subject: Re: [PATCH v2] upload-pack.c: treat want-ref relative to namespace
Date: Mon,  2 Aug 2021 14:06:44 -0700	[thread overview]
Message-ID: <20210802210644.3432544-1-jonathantanmy@google.com> (raw)
In-Reply-To: <20210731203415.618641-1-kim@eagain.st>

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

  reply	other threads:[~2021-08-02 21:06 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-30 13:59 [PATCH] " Kim Altintop
2021-07-30 14:04 ` Kim Altintop
2021-07-30 18:57 ` Junio C Hamano
2021-07-30 21:08   ` Kim Altintop
2021-07-31 20:36 ` [PATCH v2] " Kim Altintop
2021-08-02 21:06   ` Jonathan Tan [this message]
2021-08-04 20:36     ` Kim Altintop
2021-08-04 20:42   ` [PATCH v3] " Kim Altintop
2021-08-04 21:00     ` [PATCH v4] " Kim Altintop
2021-08-09 17:56       ` [PATCH 0/3] upload-pack: " Kim Altintop
2021-08-09 17:56         ` [PATCH 1/3] t5730: introduce fetch command helper Kim Altintop
2021-08-09 19:16           ` Junio C Hamano
2021-08-09 21:18             ` Kim Altintop
2021-08-09 19:40           ` Jonathan Nieder
2021-08-09 21:43             ` Junio C Hamano
2021-08-09 21:56             ` Kim Altintop
2021-08-09 22:03               ` Junio C Hamano
2021-08-09 23:01                 ` Jonathan Nieder
2021-08-10  9:44                   ` Kim Altintop
2021-08-09 17:57         ` [PATCH 2/3] upload-pack.c: treat want-ref relative to namespace Kim Altintop
2021-08-09 17:57         ` [PATCH 3/3] docs: clarify the interaction of transfer.hideRefs and namespaces Kim Altintop
2021-08-10  9:49           ` Kim Altintop
2021-08-13  6:23         ` [PATCH v6 0/3] upload-pack: treat want-ref relative to namespace Kim Altintop
2021-08-14 21:46           ` Johannes Schindelin
2021-08-15 17:59             ` Junio C Hamano
2021-08-15 19:35             ` Kim Altintop
2021-08-16 12:39               ` Johannes Schindelin
2021-08-13  6:23         ` [PATCH v6 1/3] t5730: introduce fetch command helper Kim Altintop
2021-08-13  6:23         ` [PATCH v6 2/3] upload-pack.c: treat want-ref relative to namespace Kim Altintop
2021-08-13  6:23         ` [PATCH v6 3/3] docs: clarify the interaction of transfer.hideRefs and namespaces Kim Altintop
2021-08-04 21:15     ` [PATCH v3] upload-pack.c: treat want-ref relative to namespace Junio C Hamano
2021-08-04 22:04       ` Kim Altintop
2021-08-04 22:17         ` Eric Sunshine
2021-08-04 22:17         ` Junio C Hamano
2021-08-04 22:23         ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210802210644.3432544-1-jonathantanmy@google.com \
    --to=jonathantanmy@google.com \
    --cc=bwilliams.eng@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=kim@eagain.st \
    --subject='Re: [PATCH v2] upload-pack.c: treat want-ref relative to namespace' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.