On 02/26, Jonathan Nieder wrote:
> Brandon Williams wrote:
> > static char *server_capabilities;
> > +static struct argv_array server_capabilities_v2 = ARGV_ARRAY_INIT;
>
> Can a quick doc comment describe these and how they relate?
>
> Is only one of them set, based on which protocol version is in use?
> Should server_capabilities be renamed to server_capabilities_v1?
yes that's correct. I can rename it.
> > +{
> > + int ret = 1;
> > + int i = 0;
> > + struct object_id old_oid;
> > + struct ref *ref;
> > + struct string_list line_sections = STRING_LIST_INIT_DUP;
> > +
> > + if (string_list_split(&line_sections, line, ' ', -1) < 2) {
>
> Can there be a comment describing the expected format?
Yep I'll write a comment up.
> > +
> > + oidcpy(&ref->old_oid, &old_oid);
> > + **list = ref;
> > + *list = &ref->next;
> > +
> > + for (; i < line_sections.nr; i++) {
> > + const char *arg = line_sections.items[i].string;
> > + if (skip_prefix(arg, "symref-target:", &arg))
> > + ref->symref = xstrdup(arg);
>
> Using space-delimited fields in a single pkt-line means that
> - values cannot contains a space
> - total length is limited by the size of a pkt-line
>
> Given the context, I think that's fine. More generally it is tempting
> to use a pkt-line per field to avoid the trouble v1 had with
> capability lists crammed into a pkt-line, but I see why you used a
> pkt-line per ref to avoid having to have sections-within-a-section.
>
> My only potential worry is the length part: do we have an explicit
> limit on the length of a ref name? git-check-ref-format(1) doesn't
> mention one. A 32k ref name would be a bit ridiculous, though.
Yeah I think we're fine for now, mostly because we're out of luck with
the current protocol as it is.
>
> > +
> > + if (skip_prefix(arg, "peeled:", &arg)) {
> > + struct object_id peeled_oid;
> > + char *peeled_name;
> > + struct ref *peeled;
> > + if (get_oid_hex(arg, &peeled_oid)) {
> > + ret = 0;
> > + goto out;
> > + }
>
> Can this also check that there's no trailing garbage after the oid?
Yeah I do that.
>
> > +
> > + packet_delim(fd_out);
> > + /* When pushing we don't want to request the peeled tags */
>
> Can you say more about this? In theory it would be nice to have the
> peeled tags since they name commits whose history can be excluded from
> the pack.
I don't believe peeled refs are sent now in v0 for push.
>
> > + if (!for_push)
> > + packet_write_fmt(fd_out, "peel\n");
> > + packet_write_fmt(fd_out, "symrefs\n");
>
> Are symrefs useful during push?
They may be at a later point in time when you want to update a symref :)
>
> > + for (i = 0; ref_patterns && i < ref_patterns->argc; i++) {
> > + packet_write_fmt(fd_out, "ref-pattern %s\n",
> > + ref_patterns->argv[i]);
> > + }
>
> The exciting part.
>
> Why do these pkts end with \n? I would have expected the pkt-line
> framing to work to delimit them.
All pkts end with \n, that's just hows its been since v0. Though the
server isn't supposed to complain if they don't contain newlines.
>
> > + packet_flush(fd_out);
> > +
> > + /* Process response from server */
> > + while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
> > + if (!process_ref_v2(reader->line, &list))
> > + die("invalid ls-refs response: %s", reader->line);
> > + }
> > +
> > + if (reader->status != PACKET_READ_FLUSH)
> > + die("protocol error");
>
> Can this protocol error give more detail? When diagnosing an error in
> servers, proxies, or lower-level networking issues, informative protocol
> errors can be very helpful (similar to syntax errors from a compiler).
I'll update the error msg.
> [...]
> > --- a/remote.h
> > +++ b/remote.h
> > @@ -151,10 +151,14 @@ void free_refs(struct ref *ref);
> >
> > struct oid_array;
> > struct packet_reader;
> > +struct argv_array;
> > extern struct ref **get_remote_heads(struct packet_reader *reader,
> > struct ref **list, unsigned int flags,
> > struct oid_array *extra_have,
> > struct oid_array *shallow_points);
> > +extern struct ref **get_remote_refs(int fd_out, struct packet_reader
> > *reader,
> > + struct ref **list, int for_push,
> > + const struct argv_array *ref_patterns);
>
> What is the difference between get_remote_heads and get_remote_refs?
> A comment might help. (BTW, thanks for making the new saner name to
> replace get_remote_heads!)
I'll add a comment saying its used in v2 to retrieve a list of refs from
the remote.
--
Brandon Williams
