Re: [PATCH v3 8/8] fetch-pack: implement ref-in-want
From: Jonathan Nieder <hidden>
Date: 2018-06-22 23:01:25
Hi, Brandon Williams wrote:
Implement ref-in-want on the client side so that when a server supports the "ref-in-want" feature, a client will send "want-ref" lines for each reference the client wants to fetch. Signed-off-by: Brandon Williams <redacted> --- fetch-pack.c | 35 +++++++++++++++++++++++++++--- remote.c | 1 + remote.h | 1 + t/t5703-upload-pack-ref-in-want.sh | 4 ++-- 4 files changed, 36 insertions(+), 5 deletions(-)
This commit message doesn't tell me what ref-in-want is or is for. Could it include A. a pointer to Documentation/technical/protocol-v2.txt, or B. an example illustrating the effect e.g. using GIT_TRACE_PACKET or both? [...]
quoted hunk ↗ jump to hunk
--- a/fetch-pack.c +++ b/fetch-pack.c@@ -1102,9 +1102,10 @@ static void add_shallow_requests(struct strbuf *req_buf, static void add_wants(const struct ref *wants, struct strbuf *req_buf) { + int use_ref_in_want = server_supports_feature("fetch", "ref-in-want", 0); + for ( ; wants ; wants = wants->next) {
Not about this patch: it's kind of confusing that the iterator is called
'wants' even though it points into the middle of the list. I would even
be tempted to do
const struct ref *want;
for (want = wants; want; want = want->next) {
It wouldn't make sense to do in this patch, though.
[...]quoted hunk ↗ jump to hunk
@@ -1122,8 +1123,10 @@ static void add_wants(const struct ref *wants, struct strbuf *req_buf) continue; } - remote_hex = oid_to_hex(remote); - packet_buf_write(req_buf, "want %s\n", remote_hex); + if (!use_ref_in_want || wants->exact_oid) + packet_buf_write(req_buf, "want %s\n", oid_to_hex(remote)); + else + packet_buf_write(req_buf, "want-ref %s\n", wants->name);
Very neat. [...]
quoted hunk ↗ jump to hunk
@@ -1334,6 +1337,29 @@ static void receive_shallow_info(struct fetch_pack_args *args, args->deepen = 1; } +static void receive_wanted_refs(struct packet_reader *reader, struct ref *refs) +{ + process_section_header(reader, "wanted-refs", 0); + while (packet_reader_read(reader) == PACKET_READ_NORMAL) { + struct object_id oid; + const char *end; + struct ref *r = NULL; + + if (parse_oid_hex(reader->line, &oid, &end) || *end++ != ' ') + die("expected wanted-ref, got '%s'", reader->line); + + for (r = refs; r; r = r->next) { + if (!strcmp(end, r->name)) { + oidcpy(&r->old_oid, &oid); + break; + }
Stefan mentioned that the spec says
* The server MUST NOT send any refs which were not requested
using 'want-ref' lines.
Can client enforce that? If not, can the spec say SHOULD NOT for the
server and add a MUST describing appropriate client behavior?
+ } + } + + if (reader->status != PACKET_READ_DELIM)
The spec says
* This section is only included if the client has requested a
ref using a 'want-ref' line and if a packfile section is also
included in the response.
What should happen if the client already has all the relevant objects
(or in other words if there is no packfile to send in the packfile
section)? Is the idea that the client should already have known that
based on the ref advertisement? What if ref values change to put us
in that state between the ls-refs and fetch steps?
[...]quoted hunk ↗ jump to hunk
--- a/remote.c +++ b/remote.c@@ -1735,6 +1735,7 @@ int get_fetch_map(const struct ref *remote_refs, if (refspec->exact_sha1) { ref_map = alloc_ref(name); get_oid_hex(name, &ref_map->old_oid); + ref_map->exact_oid = 1;
Sensible. The alternative would be that we check whether the refname is oid-shaped at want-ref generation time, which would be unnecessarily complicated. [...]
quoted hunk ↗ jump to hunk
--- a/remote.h +++ b/remote.h@@ -73,6 +73,7 @@ struct ref {
Not about this patch: why is this in remote.h instead of ref.h?
force:1, forced_update:1, expect_old_sha1:1, + exact_oid:1, deletion:1;
Looks good, and we have room for plenty more bits there. [...]
quoted hunk ↗ jump to hunk
+++ b/t/t5703-upload-pack-ref-in-want.sh
Nice. Thanks for a pleasant read. Sincerely, Jonathan