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