All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kim Altintop <kim@eagain.st>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org, bwilliams.eng@gmail.com, gitster@pobox.com
Subject: Re: [PATCH v2] upload-pack.c: treat want-ref relative to namespace
Date: Wed, 04 Aug 2021 20:36:48 +0000	[thread overview]
Message-ID: <CDB0M5IP0WNJ.3I09ZETKF0VXS@schmidt> (raw)
In-Reply-To: <20210802210644.3432544-1-jonathantanmy@google.com>

On Mon Aug 2, 2021 at 11:06 PM CEST, Jonathan Tan wrote:
> > +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).

Ah, that answers the question raised by Junio in the first review. I'll revert
to using a subshell, as that seems clearer and is used throughout the file.

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

Hm no. I think I misread how this works.

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

That's a good suggestion. However, I'm having some difficulties finding just the
right amount of common code to extract due to

a. having to pass the namespace via env instead of --namespace (and the
   different position for the success / test_must_fail cases), and
b. having to rely on _persistent_ config for hideRefs, as opposed to being able
   to pass it via -c to upload-pack (ie. I'm worried about tests breaking if
   they get reordered)

So I'm not exactly happy with what I came up with for v3, but I'd also be
reluctant to add --namespace / -c support to test-tool as part of this patch.
Let me know what you think.


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

Yeap :/


  reply	other threads:[~2021-08-04 20:37 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
2021-08-04 20:36     ` Kim Altintop [this message]
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=CDB0M5IP0WNJ.3I09ZETKF0VXS@schmidt \
    --to=kim@eagain.st \
    --cc=bwilliams.eng@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jonathantanmy@google.com \
    --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.