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
