Re: [PATCH v3 14/35] connect: request remote refs using v2

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

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

> > +
> > +	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.

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

> 
> > +
> > +	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.

> 
> > +	if (!for_push)
> > +		packet_write_fmt(fd_out, "peel\n");
> > +	packet_write_fmt(fd_out, "symrefs\n");
> 
> Are symrefs useful during push?

> 
> > +	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.

> 
> > +	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).

> [...]
> > --- 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!)