upload-pack: use a strmap for want-ref lines

When the "ref-in-want" capability is advertised (which it is not by default), then upload-pack processes a "want-ref" line from the client by checking that the name is a valid ref and recording it in a string-list. In theory this list should grow no larger than the number of refs in the server-side repository. But since we don't do any de-duplication, a client which sends "want-ref refs/heads/foo" over and over will cause the array to grow without bound. We can fix this by switching to strmap, which efficiently detects duplicates. There are two client-visible changes here: 1. The "wanted-refs" response will now be in an apparently-random order (based on iterating the hashmap) rather than the order given by the client. The protocol documentation is quiet on ordering here. The current fetch-pack implementation is happy with any order, as it looks up each returned ref using a binary search in its local sorted list. JGit seems to implement want-ref on the server side, but has no client-side support. libgit2 doesn't support either side. It would obviously be possible to record the original order or to use the strmap as an auxiliary data structure. But if the client doesn't care, we may as well do the simplest thing. 2. We'll now reject duplicates explicitly as a protocol error. The client should never send them (and our current implementation, even when asked to "git fetch master:one master:two" will de-dup on the client side). If we wanted to be more forgiving, we could perhaps just throw away the duplicates. But then our "wanted-refs" response back to the client would omit the duplicates, and it's hard to say what a client that accidentally sent a duplicate would do with that. So I think we're better off to complain loudly before anybody accidentally writes such a client. Let's also add a note to the protocol documentation clarifying that duplicates are forbidden. As discussed above, this was already the intent, but it's not very explicit. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Feb 28, 2024 at 17:38 UTC b065063c570f8dfb25eccb5094fef7e89a1883f2
2 files changed +19 -14
Documentation/gitprotocol-v2.txt
+2 -1
@@ -346,7 +346,8 @@ the 'wanted-refs' section in the server's response as explained below.
346 want-ref <ref>
347 Indicates to the server that the client wants to retrieve a
348 particular ref, where <ref> is the full name of a ref on the
349 - server.
349 + server. It is a protocol error to send want-ref for the
350 + same ref more than once.
351
352 If the 'sideband-all' feature is advertised, the following argument can be
353 included in the client's request:
upload-pack.c
+17 -13
@@ -28,6 +28,7 @@
28 #include "shallow.h"
29 #include "write-or-die.h"
30 #include "json-writer.h"
31 +#include "strmap.h"
32
33 /* Remember to update object flag allocation in object.h */
34 #define THEY_HAVE (1u << 11)
@@ -61,7 +62,7 @@ struct upload_pack_data {
62 struct string_list symref; /* v0 only */
63 struct object_array want_obj;
64 struct object_array have_obj;
64 - struct string_list wanted_refs; /* v2 only */
65 + struct strmap wanted_refs; /* v2 only */
66 struct strvec hidden_refs;
67
68 struct object_array shallows;
@@ -120,7 +121,7 @@ struct upload_pack_data {
121 static void upload_pack_data_init(struct upload_pack_data *data)
122 {
123 struct string_list symref = STRING_LIST_INIT_DUP;
123 - struct string_list wanted_refs = STRING_LIST_INIT_DUP;
124 + struct strmap wanted_refs = STRMAP_INIT;
125 struct strvec hidden_refs = STRVEC_INIT;
126 struct object_array want_obj = OBJECT_ARRAY_INIT;
127 struct object_array have_obj = OBJECT_ARRAY_INIT;
@@ -153,7 +154,7 @@ static void upload_pack_data_init(struct upload_pack_data *data)
154 static void upload_pack_data_clear(struct upload_pack_data *data)
155 {
156 string_list_clear(&data->symref, 1);
156 - string_list_clear(&data->wanted_refs, 1);
157 + strmap_clear(&data->wanted_refs, 1);
158 strvec_clear(&data->hidden_refs);
159 object_array_clear(&data->want_obj);
160 object_array_clear(&data->have_obj);
@@ -1488,14 +1489,13 @@ static int parse_want(struct packet_writer *writer, const char *line,
1489 }
1490
1491 static int parse_want_ref(struct packet_writer *writer, const char *line,
1491 - struct string_list *wanted_refs,
1492 + struct strmap *wanted_refs,
1493 struct strvec *hidden_refs,
1494 struct object_array *want_obj)
1495 {
1496 const char *refname_nons;
1497 if (skip_prefix(line, "want-ref ", &refname_nons)) {
1498 struct object_id oid;
1498 - struct string_list_item *item;
1499 struct object *o = NULL;
1500 struct strbuf refname = STRBUF_INIT;
1501
@@ -1507,8 +1507,11 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,
1507 }
1508 strbuf_release(&refname);
1509
1510 - item = string_list_append(wanted_refs, refname_nons);
1511 - item->util = oiddup(&oid);
1510 + if (strmap_put(wanted_refs, refname_nons, oiddup(&oid))) {
1511 + packet_writer_error(writer, "duplicate want-ref %s",
1512 + refname_nons);
1513 + die("duplicate want-ref %s", refname_nons);
1514 + }
1515
1516 if (!starts_with(refname_nons, "refs/tags/")) {
1517 struct commit *commit = lookup_commit_in_graph(the_repository, &oid);
@@ -1551,7 +1554,7 @@ static void trace2_fetch_info(struct upload_pack_data *data)
1554 jw_object_begin(&jw, 0);
1555 jw_object_intmax(&jw, "haves", data->have_obj.nr);
1556 jw_object_intmax(&jw, "wants", data->want_obj.nr);
1554 - jw_object_intmax(&jw, "want-refs", data->wanted_refs.nr);
1557 + jw_object_intmax(&jw, "want-refs", strmap_get_size(&data->wanted_refs));
1558 jw_object_intmax(&jw, "depth", data->depth);
1559 jw_object_intmax(&jw, "shallows", data->shallows.nr);
1560 jw_object_bool(&jw, "deepen-since", data->deepen_since);
@@ -1705,17 +1708,18 @@ static int process_haves_and_send_acks(struct upload_pack_data *data)
1708
1709 static void send_wanted_ref_info(struct upload_pack_data *data)
1710 {
1708 - const struct string_list_item *item;
1711 + struct hashmap_iter iter;
1712 + const struct strmap_entry *e;
1713
1710 - if (!data->wanted_refs.nr)
1714 + if (strmap_empty(&data->wanted_refs))
1715 return;
1716
1717 packet_writer_write(&data->writer, "wanted-refs\n");
1718
1715 - for_each_string_list_item(item, &data->wanted_refs) {
1719 + strmap_for_each_entry(&data->wanted_refs, &iter, e) {
1720 packet_writer_write(&data->writer, "%s %s\n",
1717 - oid_to_hex(item->util),
1718 - item->string);
1721 + oid_to_hex(e->value),
1722 + e->key);
1723 }
1724
1725 packet_writer_delim(&data->writer);