| Message ID | 20210730135845.633234-1-kim@eagain.st (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | upload-pack.c: treat want-ref relative to namespace | expand |
> Please excuse my newbie ness.
Oops I'm sorry. `send-email` remembered this from a dry run.
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.
Kim Altintop <kim@eagain.st> writes: > When 'upload-pack' runs within the context of a git namespace, treat any > 'want-ref' lines the client sends as relative to that namespace. > > Also check if the wanted ref is hidden via 'hideRefs', and respond with > an error otherwise. It was previously possible to request any ref, but > note that this is still the case unless 'hideRefs' is in effect. > > Signed-off-by: Kim Altintop <kim@eagain.st> > --- Nicely described. I have a question on the last sentence, though. Do you mean that any ref can be requested when a namespace is in use, as long as 'hideRefs' is not in effect? What does "any ref" exactly mean---even thouse outside the given namespace (and if so how?) I wonder if the last sentence is making the description more confusing without adding any clarity. In other words, would this work as a replacement for the second paragraph, or does it say something different from what you wanted to say? Requests for any ref, even those that are marked to be hidden via the 'transfer.hideRefs' configuration, were allowed but it is problematic for such and such reasons. Respond with an error if a requested ref is to be hidden. I couldn't tell why you thought it was problematic, so left "for such and such reasons" to be filled in, but there still may be an issue. How does the error response look like? We shouldn't be saying "you requested for the hidden/x branch, but you are not allowed to do so, as that is hidden". To hide something, we should pretend that the thing does not exist, so that we can hide even the fact that we are hiding it. To help future readers of "git log" who find this change from you, we should clarify the "respond with an error" part of your proposed log message (e.g. "pretend that the wanted ref does not exist when it is hidden via the 'transfer.hiderefs' configuration" or something else). > +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 > +' I wonder why the last two are outside the subshell? IOW, you could have configured the newly created repository while you were still in there. > +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 > + done > + 0000 > + EOF > + > + GIT_NAMESPACE=ns && export GIT_NAMESPACE && > + test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in && > + check_output > +' Unless you mean to make all subsequent tests to be done inside the 'ns' namespace, and even when you do, you do not want to do this in order to keep each test as independent as possible (iow, make some of them skippable without affecting the later tests). Run the final test in a subshell, e.g. oid=$(git -C "$REPO" rev-parse c) && test-tool pkt-line pack >in <<-EOF && ... EOF ( export GIT_NAMESPACE=ns && test-tool ... >out <in ) && check_output or if the command you want to run with a custom environment variable is a single external executable like this case, do oid=$(git -C "$REPO" rev-parse c) && test-tool pkt-line pack >in <<-EOF && ... EOF GIT_NAMESPACE=ns test-tool ... >out <in && check_output That way, the environment will be kept clean without GIT_NAMESPACE outside the invocation of test-tool. Note that you cannot use this technique directly with test_must_fail which is *not* an external executable but is a shell function. test_must_fail env GIT_NAMESPACE=ns test-tool ... would be the way to write a step that must fail. > diff --git a/upload-pack.c b/upload-pack.c > index 297b76fcb4..008ac75125 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; > + const char *refname_nons; > if (skip_prefix(line, "want-ref ", &arg)) { Don't you receive the result in refname_nons here, as arg is no longer there? > struct object_id oid; > struct string_list_item *item; > struct object *o; > + struct strbuf refname = STRBUF_INIT; > > - 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.buf); > } 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. Thanks. > - 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); > if (!(o->flags & WANTED)) { > o->flags |= WANTED; > add_object_array(o, NULL, want_obj); > -- > 2.32.0
On Fri Jul 30, 2021 at 8:57 PM CEST, Junio C Hamano wrote: > > Kim Altintop <kim@eagain.st> writes: [..] > > Also check if the wanted ref is hidden via 'hideRefs', and respond with > > an error otherwise. It was previously possible to request any ref, but > > note that this is still the case unless 'hideRefs' is in effect. [..] > Nicely described. I have a question on the last sentence, though. > Do you mean that any ref can be requested when a namespace is in > use, as long as 'hideRefs' is not in effect? What does "any ref" > exactly mean---even thouse outside the given namespace (and if so > how?) Thank you. It seems like I got confused for a moment when writing the commit message. It's not possible to get a ref outside the namespace anymore. I removed that sentence. > > +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 > > +' > > I wonder why the last two are outside the subshell? IOW, you could > have configured the newly created repository while you were still in > there. To be honest, I don't know. I did that because the other repo setups in that file follow the same pattern, I suppose that qualifies as "cargo culting". Happy to remove the subshell, unless others point out that there is some specific reason for it. > Unless you mean to make all subsequent tests to be done inside the > 'ns' namespace, and even when you do, you do not want to do this > in order to keep each test as independent as possible (iow, make > some of them skippable without affecting the later tests). Run the > final test in a subshell, e.g. > > oid=$(git -C "$REPO" rev-parse c) && > test-tool pkt-line pack >in <<-EOF && > ... > EOF > > ( > export GIT_NAMESPACE=ns && > test-tool ... >out <in > ) && > check_output > > or if the command you want to run with a custom environment variable > is a single external executable like this case, do > > oid=$(git -C "$REPO" rev-parse c) && > test-tool pkt-line pack >in <<-EOF && > ... > EOF > GIT_NAMESPACE=ns test-tool ... >out <in && > check_output > > That way, the environment will be kept clean without GIT_NAMESPACE > outside the invocation of test-tool. > > Note that you cannot use this technique directly with test_must_fail > which is *not* an external executable but is a shell function. > > test_must_fail env GIT_NAMESPACE=ns test-tool ... > > would be the way to write a step that must fail. Ah thanks! I had tried ... GIT_NAMESPACE=ns test-tool ... >out <in but the linter complained about this without giving a hint as to what the fix would be. It turns out that "env" works, ie. ... env GIT_NAMESPACE=ns test-tool ... test_must_fail env GIT_NAMESPACE=ns test-tool ... > > > diff --git a/upload-pack.c b/upload-pack.c > > index 297b76fcb4..008ac75125 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; > > + const char *refname_nons; > > if (skip_prefix(line, "want-ref ", &arg)) { > > Don't you receive the result in refname_nons here, as arg is no > longer there? Ouch. Will fix. > > > struct object_id oid; > > struct string_list_item *item; > > struct object *o; > > + struct strbuf refname = STRBUF_INIT; > > > > - 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.buf); > > } > > 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 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?).
diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh index e9e471621d..9fb16848bc 100755 --- a/t/t5703-upload-pack-ref-in-want.sh +++ b/t/t5703-upload-pack-ref-in-want.sh @@ -298,6 +298,83 @@ 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 +' + +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 + done + 0000 + EOF + + GIT_NAMESPACE=ns && export GIT_NAMESPACE && + 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 + + GIT_NAMESPACE=ns && export GIT_NAMESPACE && + test_must_fail test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in && + grep "unknown ref" out +' + +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 + + GIT_NAMESPACE=ns && export GIT_NAMESPACE && + test_must_fail test-tool -C "$REPO" serve-v2 --stateless-rpc >out <in && + grep "unknown ref" out +' + . "$TEST_DIRECTORY"/lib-httpd.sh start_httpd diff --git a/upload-pack.c b/upload-pack.c index 297b76fcb4..008ac75125 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; + const char *refname_nons; if (skip_prefix(line, "want-ref ", &arg)) { struct object_id oid; struct string_list_item *item; struct object *o; + struct strbuf refname = STRBUF_INIT; - 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.buf); } - 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); if (!(o->flags & WANTED)) { o->flags |= WANTED; add_object_array(o, NULL, want_obj);
When 'upload-pack' runs within the context of a git namespace, treat any 'want-ref' lines the client sends as relative to that namespace. Also check if the wanted ref is hidden via 'hideRefs', and respond with an error otherwise. It was previously possible to request any ref, but note that this is still the case unless 'hideRefs' is in effect. Signed-off-by: Kim Altintop <kim@eagain.st> --- Please excuse my newbie ness. t/t5703-upload-pack-ref-in-want.sh | 77 ++++++++++++++++++++++++++++++ upload-pack.c | 15 +++--- 2 files changed, 86 insertions(+), 6 deletions(-) -- 2.32.0