Subject: Re: [PATCH v3 21/35] fetch-pack: perform a fetch using v2

From: Brandon Williams <[email-masked]>
Date: Tue, 27 Feb 2018 11:40:47 -0800

On 02/27, Stefan Beller wrote:
> > +
> > +       /* 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.

I'm sure there is more that can be micro optimized but I don't really
want to get distracted by redesigning this logic another time right now.

> 
> 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" ?

I'll update this.

> 
> 
> > +       }
> > +
> > +       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?

Yeah protocol-v2.txt should say all of that.

> Thanks,
> Stefan

-- 
Brandon Williams