From 28a592e4f4870bdd444675b7240920d0879a9c1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 5 Aug 2021 03:25:38 +0200 Subject: serve.[ch]: don't pass "struct strvec *keys" to commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The serve.c API added in ed10cb952d3 (serve: introduce git-serve, 2018-03-15) was passing in the raw capabilities "keys", but nothing downstream of it ever used them. Let's remove that code because it's not needed. If we do end up needing to pass information about the advertisement in the future it'll make more sense to have serve.c parse the capabilities keys and pass the result of its parsing, rather than expecting expecting its API users to parse the same keys again. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- upload-pack.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'upload-pack.c') diff --git a/upload-pack.c b/upload-pack.c index 297b76fcb4..ed60a9abd6 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1655,8 +1655,7 @@ enum fetch_state { FETCH_DONE, }; -int upload_pack_v2(struct repository *r, struct strvec *keys, - struct packet_reader *request) +int upload_pack_v2(struct repository *r, struct packet_reader *request) { enum fetch_state state = FETCH_PROCESS_ARGS; struct upload_pack_data data; -- cgit v1.2.3 From f234da80197b3115cd3c280e98d3393a884a9327 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 5 Aug 2021 03:25:42 +0200 Subject: serve.[ch]: remove "serve_options", split up --advertise-refs code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "advertise capabilities" mode of serve.c added in ed10cb952d3 (serve: introduce git-serve, 2018-03-15) is only used by the http-backend.c to call {upload,receive}-pack with the --advertise-refs parameter. See 42526b478e3 (Add stateless RPC options to upload-pack, receive-pack, 2009-10-30). Let's just make cmd_upload_pack() take the two (v2) or three (v2) parameters the the v2/v1 servicing functions need directly, and pass those in via the function signature. The logic of whether daemon mode is implied by the timeout belongs in the v1 function (only used there). Once we split up the "advertise v2 refs" from "serve v2 request" it becomes clear that v2 never cared about those in combination. The only time it mattered was for v1 to emit its ref advertisement, in that case we wanted to emit the smart-http-only "no-done" capability. Since we only do that in the --advertise-refs codepath let's just have it set "do_done" itself in v1's upload_pack() just before send_ref(), at that point --advertise-refs and --stateless-rpc in combination are redundant (the only user is get_info_refs() in http-backend.c), so we can just pass in --advertise-refs only. Since we need to touch all the serve() and advertise_capabilities() codepaths let's rename them to less clever and obvious names, it's been suggested numerous times, the latest of which is [1]'s suggestion for protocol_v2_serve_loop(). Let's go with that. 1. https://lore.kernel.org/git/CAFQ2z_NyGb8rju5CKzmo6KhZXD0Dp21u-BbyCb2aNxLEoSPRJw@mail.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- upload-pack.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) (limited to 'upload-pack.c') diff --git a/upload-pack.c b/upload-pack.c index ed60a9abd6..fc38f04d98 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1214,7 +1214,7 @@ static int send_ref(const char *refname, const struct object_id *oid, " allow-tip-sha1-in-want" : "", (data->allow_uor & ALLOW_REACHABLE_SHA1) ? " allow-reachable-sha1-in-want" : "", - data->stateless_rpc ? " no-done" : "", + data->no_done ? " no-done" : "", symref_info.buf, data->allow_filter ? " filter" : "", session_id.buf, @@ -1329,7 +1329,8 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data) return parse_hide_refs_config(var, value, "uploadpack"); } -void upload_pack(struct upload_pack_options *options) +void upload_pack(const int advertise_refs, const int stateless_rpc, + const int timeout) { struct packet_reader reader; struct upload_pack_data data; @@ -1338,14 +1339,17 @@ void upload_pack(struct upload_pack_options *options) git_config(upload_pack_config, &data); - data.stateless_rpc = options->stateless_rpc; - data.daemon_mode = options->daemon_mode; - data.timeout = options->timeout; + data.stateless_rpc = stateless_rpc; + data.timeout = timeout; + if (data.timeout) + data.daemon_mode = 1; head_ref_namespaced(find_symref, &data.symref); - if (options->advertise_refs || !data.stateless_rpc) { + if (advertise_refs || !data.stateless_rpc) { reset_timeout(data.timeout); + if (advertise_refs) + data.no_done = 1; head_ref_namespaced(send_ref, &data); for_each_namespaced_ref(send_ref, &data); advertise_shallow_grafts(1); @@ -1355,7 +1359,7 @@ void upload_pack(struct upload_pack_options *options) for_each_namespaced_ref(check_ref, NULL); } - if (!options->advertise_refs) { + if (!advertise_refs) { packet_reader_init(&reader, 0, NULL, 0, PACKET_READ_CHOMP_NEWLINE | PACKET_READ_DIE_ON_ERR_PACKET); -- cgit v1.2.3 From 39551406539e6ea87f89f619f7f0800e887e9b57 Mon Sep 17 00:00:00 2001 From: Kim Altintop Date: Fri, 13 Aug 2021 06:23:50 +0000 Subject: upload-pack.c: treat want-ref relative to namespace 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'. If it is hidden, respond with an error as if the ref didn't exist. Helped-by: Jonathan Tan Signed-off-by: Kim Altintop Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano --- upload-pack.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) (limited to 'upload-pack.c') diff --git a/upload-pack.c b/upload-pack.c index 297b76fcb4..6ce07231d3 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1417,21 +1417,25 @@ 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; - 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); } + strbuf_release(&refname); - 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); -- cgit v1.2.3 From 70afef5cdf29b5159f18df1b93722055f78740f8 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 1 Sep 2021 14:54:42 +0200 Subject: upload-pack: use stdio in send_ref callbacks In both protocol v0 and v2, upload-pack writes one pktline packet per advertised ref to stdout. That means one or two write(2) syscalls per ref. This is problematic if these writes become network sends with high overhead. This commit changes both send_ref callbacks to use buffered IO using stdio. To give an example of the impact: I set up a single-threaded loop that calls ls-remote (with HTTP and protocol v2) on a local GitLab instance, on a repository with 11K refs. When I switch from Git v2.32.0 to this patch, I see a 40% reduction in CPU time for Git, and 65% for Gitaly (GitLab's Git RPC service). So using buffered IO not only saves syscalls in upload-pack, it also saves time in things that consume upload-pack's output. Helped-by: Jeff King Signed-off-by: Jacob Vosmaer Signed-off-by: Junio C Hamano --- upload-pack.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) (limited to 'upload-pack.c') diff --git a/upload-pack.c b/upload-pack.c index 297b76fcb4..0ed377b1fb 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1207,7 +1207,7 @@ static int send_ref(const char *refname, const struct object_id *oid, format_symref_info(&symref_info, &data->symref); format_session_id(&session_id, data); - packet_write_fmt(1, "%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\n", + packet_fwrite_fmt(stdout, "%s %s%c%s%s%s%s%s%s%s object-format=%s agent=%s\n", oid_to_hex(oid), refname_nons, 0, capabilities, (data->allow_uor & ALLOW_TIP_SHA1) ? @@ -1223,11 +1223,11 @@ static int send_ref(const char *refname, const struct object_id *oid, strbuf_release(&symref_info); strbuf_release(&session_id); } else { - packet_write_fmt(1, "%s %s\n", oid_to_hex(oid), refname_nons); + packet_fwrite_fmt(stdout, "%s %s\n", oid_to_hex(oid), refname_nons); } capabilities = NULL; if (!peel_iterated_oid(oid, &peeled)) - packet_write_fmt(1, "%s %s^{}\n", oid_to_hex(&peeled), refname_nons); + packet_fwrite_fmt(stdout, "%s %s^{}\n", oid_to_hex(&peeled), refname_nons); return 0; } @@ -1348,6 +1348,11 @@ void upload_pack(struct upload_pack_options *options) reset_timeout(data.timeout); head_ref_namespaced(send_ref, &data); for_each_namespaced_ref(send_ref, &data); + /* + * fflush stdout before calling advertise_shallow_grafts because send_ref + * uses stdio. + */ + fflush_or_die(stdout); advertise_shallow_grafts(1); packet_flush(1); } else { -- cgit v1.2.3