Re: [PATCH v2] upload-pack.c: treat want-ref relative to namespace
flat view
From: Jonathan Tan <hidden>
Date: 2021-08-02 21:06:52
quoted hunk ↗ jump to hunk
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).
quoted hunk ↗ jump to hunk
+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.)
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
- 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/ (local) [2] https://lore.kernel.org/git/d0d42b3bb4cf755f122591e191354c53848f197d.1485381677.git.jonathantanmy@google.com/ (local)