Re: [PATCH v2 1/1] upload-pack: advertise capabilities when cloning empty repos
From: Jeff King <hidden>
Date: 2023-05-01 22:40:49
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2023-05-01 · Re: [PATCH v2 1/1] upload-pack: advertise capabilities when cloning empty repos · Junio C Hamano <hidden>
On Mon, May 01, 2023 at 05:00:18PM +0000, brian m. carlson wrote:
There is one minor issue to fix, though. When we call send_ref with namespaces, we would return NULL with the capabilities entry, which would cause a crash. Instead, let's make sure we don't try to strip the namespace if we're using our special capabilities entry.
Thanks, this hunk:
quoted hunk ↗ jump to hunk
@@ -1212,7 +1213,8 @@ static int send_ref(const char *refname, const struct object_id *oid, static const char *capabilities = "multi_ack thin-pack side-band" " side-band-64k ofs-delta shallow deepen-since deepen-not" " deepen-relative no-progress include-tag multi_ack_detailed"; - const char *refname_nons = strip_namespace(refname); + const char *refname_nons = !strcmp(refname, "capabilities^{}") ? + refname : strip_namespace(refname); struct object_id peeled; struct upload_pack_data *data = cb_data;
looks much better than sticking it in strip_namespace() as I did earlier. I did wonder about refactoring further:
diff --git a/upload-pack.c b/upload-pack.c
index d7b31d0527..e1d75d7c3c 100644
--- a/upload-pack.c
+++ b/upload-pack.c@@ -1207,19 +1207,17 @@ static void format_session_id(struct strbuf *buf, struct upload_pack_data *d) { strbuf_addf(buf, " session-id=%s", trace2_session_id()); } -static int send_ref(const char *refname, const struct object_id *oid, - int flag UNUSED, void *cb_data) +static void write_v0_ref(struct upload_pack_data *data, + const char *refname, const char *refname_nons, + const struct object_id *oid) { static const char *capabilities = "multi_ack thin-pack side-band" " side-band-64k ofs-delta shallow deepen-since deepen-not" " deepen-relative no-progress include-tag multi_ack_detailed"; - const char *refname_nons = !strcmp(refname, "capabilities^{}") ? - refname : strip_namespace(refname); struct object_id peeled; - struct upload_pack_data *data = cb_data; if (mark_our_ref(refname_nons, refname, oid, &data->hidden_refs)) - return 0; + return; if (capabilities) { struct strbuf symref_info = STRBUF_INIT;
@@ -1249,6 +1247,12 @@ static int send_ref(const char *refname, const struct object_id *oid, capabilities = NULL; if (!peel_iterated_oid(oid, &peeled)) packet_fwrite_fmt(stdout, "%s %s^{}\n", oid_to_hex(&peeled), refname_nons); +} + +static int send_ref(const char *refname, const struct object_id *oid, + int flag UNUSED, void *cb_data) +{ + write_v0_ref(cb_data, refname, strip_namespace(refname), oid); return 0; }
@@ -1382,8 +1386,10 @@ void upload_pack(const int advertise_refs, const int stateless_rpc, data.no_done = 1; head_ref_namespaced(send_ref, &data); for_each_namespaced_ref(send_ref, &data); - if (!data.sent_capabilities) - send_ref("capabilities^{}", null_oid(), 0, &data); + if (!data.sent_capabilities) { + const char *ref = "capabilities^{}"; + write_v0_ref(&data, ref, ref, null_oid()); + } /* * fflush stdout before calling advertise_shallow_grafts because send_ref * uses stdio.
which avoids doing an extra strcmp() on every ref. But probably it is not that big a deal either way.
Add several sets of tests for HTTP as well as for local clones.
This part puzzled me a bit. There's a local test in t5700, which is good. But it also gets HTTP tests. What do they offer versus the ones in t5551 (or vice versa)? -Peff