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/
next prev parent 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] upload-pack.c: treat want-ref relative to namespace 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 \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).