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