Re: [PATCH v2 1/1] upload-pack: advertise capabilities when cloning empty repos

From

Jeff King <peff@peff.net>

Date

May 1, 2023, 22:40 UTC

Message-ID

<20230501224038.GA1174291@coredump.intra.peff.net>

In-Reply-To

<20230501170018.1410567-2-sandals@crustytoothpaste.net>

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:

> @@ -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;

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