Re: [PATCH v2] upload-pack.c: treat want-ref relative to namespace
- From
Kim Altintop <kim@eagain.st>
- Date
- Aug 4, 2021, 20:36 UTC
- Message-ID
- <CDB0M5IP0WNJ.3I09ZETKF0VXS@schmidt>
- In-Reply-To
- <20210802210644.3432544-1-jonathantanmy@google.com>
On Mon Aug 2, 2021 at 11:06 PM CEST, Jonathan Tan wrote:
Show 21 quoted lines
> > +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.
Show 17 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.)
Hm no. I think I misread how this works.
Show 20 quoted lines
> > +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.
Show 18 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.Yeap :/