On Tue, Feb 6, 2018 at 5:12 PM, Brandon Williams [off-list ref] wrote:

The comment might convey too much redundant information
instead of helping the reader gain more understanding. ;)

+
+       /* Add all of the common commits we've found in previous rounds */
+       add_common(&req_buf, common);

nit:
Maybe s/add_common/add_common_haves/ or
add_previous_haves ?

+
+       /* Add initial haves */
+       ret = add_haves(&req_buf, in_vain);

I like the shortness and conciseness of this send_fetch_request
function as it makes clear what is happening over the wire, however
I wonder if we can improve on that, still.

The functions 'add_common' and 'add_haves' seem like they do the
same (sending haves), except for different sets of oids.

So I would imagine that a structure like

  {
    struct set haves = compute_haves_from(in_vain, common, ...);
    struct set wants = compute_wants&wants);

    request_capabilities(args)
    send_haves(&haves);
    send_wants(&wants);
    flush();
  }

That way we would have an even more concise way of writing
one request, and factoring out the business logic. (Coming up
with the "right" haves is a heuristic that we plan on changing in
the future, so we'd want to have that encapsulated into one function
that computes all the haves?

+
+/*
+ * Processes a section header in a server's response and checks if it matches
+ * `section`.  If the value of `peek` is 1, the header line will be peeked (and
+ * not consumed); if 0, the line will be consumed and the function will die if
+ * the section header doesn't match what was expected.
+ */
+static int process_section_header(struct packet_reader *reader,
+                                 const char *section, int peek)
+{
+       int ret;
+
+       if (packet_reader_peek(reader) != PACKET_READ_NORMAL)
+               die("error reading packet");
+
+       ret = !strcmp(reader->line, section);
+
+       if (!peek) {
+               if (!ret)
+                       die("expected '%s', received '%s'",
+                           section, reader->line);
+               packet_reader_read(reader);
+       }
+
+       return ret;
+}
+
+static int process_acks(struct packet_reader *reader, struct oidset *common)
+{
+       /* received */
+       int received_ready = 0;
+       int received_ack = 0;
+
+       process_section_header(reader, "acknowledgments", 0);
+       while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
+               const char *arg;
+
+               if (!strcmp(reader->line, "NAK"))
+                       continue;
+
+               if (skip_prefix(reader->line, "ACK ", &arg)) {
+                       struct object_id oid;
+                       if (!get_oid_hex(arg, &oid)) {
+                               struct commit *commit;
+                               oidset_insert(common, &oid);
+                               commit = lookup_commit(&oid);
+                               mark_common(commit, 0, 1);
+                       }
+                       continue;
+               }
+
+               if (!strcmp(reader->line, "ready")) {
+                       clear_prio_queue(&rev_list);
+                       received_ready = 1;
+                       continue;
+               }
+
+               die(_("git fetch-pack: expected ACK/NAK, got '%s'"), reader->line);

This is slightly misleading, it could also expect "ready" ?

+       }
+
+       if (reader->status != PACKET_READ_FLUSH &&
+           reader->status != PACKET_READ_DELIM)
+               die("Error during processing acks: %d", reader->status);

Why is this not translated unlike the one 5 lines prior to this?
Do we expect these conditions to come up due to different
root causes?

+static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
+                                   int fd[2],
+                                   const struct ref *orig_ref,
+                                   struct ref **sought, int nr_sought,
+                                   char **pack_lockfile)
+{
+       struct ref *ref = copy_ref_list(orig_ref);
+       enum fetch_state state = FETCH_CHECK_LOCAL;
+       struct oidset common = OIDSET_INIT;
+       struct packet_reader reader;
+       int in_vain = 0;
+       packet_reader_init(&reader, fd[0], NULL, 0,
+                          PACKET_READ_CHOMP_NEWLINE);
+
+       while (state != FETCH_DONE) {
+               switch (state) {
+               case FETCH_CHECK_LOCAL:
+                       sort_ref_list(&ref, ref_compare_name);
+                       QSORT(sought, nr_sought, cmp_ref_by_name);
+
+                       /* v2 supports these by default */

Is there a doc that says what is all on by default?

Thanks,
Stefan