Re: [PATCH v3 14/35] connect: request remote refs using v2
Jeff Kingwrites: >> struct strs {...}; >> >> void strs_init(struct strs *); >> void strs_push(struct strs *, const char *); >> void strs_pushf(struct strs *, const char *fmt, ...); >> void strs_pushl(struct strs *, ...); >> void strs_pushv(struct strs *, const char **); >> void strs_pop(struct strs *); >> void strs_clear(struct strs *); >> const char **strs_detach(struct strs *); >> >> ...is short, feels pretty natural, and doesn't require understanding >> "v" for "vector". > > Not bad. The "v" carries the information that it _is_ a NULL-terminated > vector and not some other list-like structure (and so is suitable for > feeding to execv, etc). But that may just be obvious from looking at its > uses and documentation. And with "v", it probably is obvious without looking at its uses and documentation, so... ;-)
Re: [PATCH v3 14/35] connect: request remote refs using v2
On Tue, Feb 27, 2018 at 05:10:09PM -0500, Eric Sunshine wrote: > > That would be fine with me. Though I would love it if we could find a > > shorter name for the associated functions. For example, > > argv_array_pushf() can make lines quite long, and something like > > argv_pushf() is easier to read (in my opinion). And that might work > > because "argv" is pretty unique by itself, but "string" is not. > > > > Some one-word name like "strarray" might work, though I find that is not > > quite catchy. I guess "strv" is short if you assume that people know the > > "v" suffix means "vector". > > struct strs {...}; > > void strs_init(struct strs *); > void strs_push(struct strs *, const char *); > void strs_pushf(struct strs *, const char *fmt, ...); > void strs_pushl(struct strs *, ...); > void strs_pushv(struct strs *, const char **); > void strs_pop(struct strs *); > void strs_clear(struct strs *); > const char **strs_detach(struct strs *); > > ...is short, feels pretty natural, and doesn't require understanding > "v" for "vector". Not bad. The "v" carries the information that it _is_ a NULL-terminated vector and not some other list-like structure (and so is suitable for feeding to execv, etc). But that may just be obvious from looking at its uses and documentation. -Peff
Re: [PATCH v3 14/35] connect: request remote refs using v2
On Tue, Feb 27, 2018 at 5:04 PM, Jeff Kingwrote: > On Tue, Feb 27, 2018 at 01:58:00PM -0800, Junio C Hamano wrote: >> So are we looking for a natural name to call an array of trings? I >> personally do not mind argv_array too much, but perhaps we can call >> it a string_array and then everybody will be happy? > > That would be fine with me. Though I would love it if we could find a > shorter name for the associated functions. For example, > argv_array_pushf() can make lines quite long, and something like > argv_pushf() is easier to read (in my opinion). And that might work > because "argv" is pretty unique by itself, but "string" is not. > > Some one-word name like "strarray" might work, though I find that is not > quite catchy. I guess "strv" is short if you assume that people know the > "v" suffix means "vector". struct strs {...}; void strs_init(struct strs *); void strs_push(struct strs *, const char *); void strs_pushf(struct strs *, const char *fmt, ...); void strs_pushl(struct strs *, ...); void strs_pushv(struct strs *, const char **); void strs_pop(struct strs *); void strs_clear(struct strs *); const char **strs_detach(struct strs *); ...is short, feels pretty natural, and doesn't require understanding "v" for "vector".
Re: [PATCH v3 14/35] connect: request remote refs using v2
On Tue, Feb 27, 2018 at 01:58:00PM -0800, Junio C Hamano wrote: > Jonathan Niederwrites: > > > Jonathan Tan wrote: > >> On Thu, 22 Feb 2018 13:26:58 -0500 > >> Jeff King wrote: > > > >>> I agree that it shouldn't matter much here. But if the name argv_array > >>> is standing in the way of using it, I think we should consider giving it > >>> a more general name. I picked that not to evoke "this must be arguments" > >>> but "this is terminated by a single NULL". > > [...] > >> This sounds reasonable - I withdraw my comment about using struct > >> string_list. > > > > Marking with #leftoverbits as a reminder to think about what such a > > more general name would be (or what kind of docs to put in > > argv-array.h) and make it so the next time I do a search for that > > keyword. > > So are we looking for a natural name to call an array of trings? I > personally do not mind argv_array too much, but perhaps we can call > it a string_array and then everybody will be happy? That would be fine with me. Though I would love it if we could find a shorter name for the associated functions. For example, argv_array_pushf() can make lines quite long, and something like argv_pushf() is easier to read (in my opinion). And that might work because "argv" is pretty unique by itself, but "string" is not. Some one-word name like "strarray" might work, though I find that is not quite catchy. I guess "strv" is short if you assume that people know the "v" suffix means "vector". It may not be worth worrying too much about, though. We already have 24-character monstrosities like string_list_append_nodup(). ;) -Peff
Re: [PATCH v3 14/35] connect: request remote refs using v2
Jonathan Niederwrites: > Jonathan Tan wrote: >> On Thu, 22 Feb 2018 13:26:58 -0500 >> Jeff King wrote: > >>> I agree that it shouldn't matter much here. But if the name argv_array >>> is standing in the way of using it, I think we should consider giving it >>> a more general name. I picked that not to evoke "this must be arguments" >>> but "this is terminated by a single NULL". > [...] >> This sounds reasonable - I withdraw my comment about using struct >> string_list. > > Marking with #leftoverbits as a reminder to think about what such a > more general name would be (or what kind of docs to put in > argv-array.h) and make it so the next time I do a search for that > keyword. So are we looking for a natural name to call an array of trings? I personally do not mind argv_array too much, but perhaps we can call it a string_array and then everybody will be happy?
Re: [PATCH v3 14/35] connect: request remote refs using v2
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(_sections, line, ' ', -1) < 2) { > > Can there be a comment describing the expected format? Yep I'll write a comment up. > > + > > + oidcpy(>old_oid, _oid); > > + **list = ref; > > + *list = >next; > > + > > + for (; i < line_sections.nr; i++) { > > + const char *arg = line_sections.items[i].string; > > + if (skip_prefix(arg, "symref-target:", )) > > + 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:", )) { > > + struct object_id peeled_oid; > > + char *peeled_name; > > + struct ref *peeled; > > + if (get_oid_hex(arg, _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, )) > > + 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
Re: [PATCH v3 14/35] connect: request remote refs using v2
Brandon Williams wrote: > Teach the client to be able to request a remote's refs using protocol > v2. This is done by having a client issue a 'ls-refs' request to a v2 > server. Yay, ls-remote support! [...] > --- a/builtin/upload-pack.c > +++ b/builtin/upload-pack.c > @@ -5,6 +5,7 @@ > #include "parse-options.h" > #include "protocol.h" > #include "upload-pack.h" > +#include "serve.h" nit, no change needed in this patch: What is a good logical order for the #includes here? Bonus points if there's a tool to make it happen automatically. Asking since adding #includes like this at the end tends to result in a harder-to-read list of #includes, sometimes with duplicates, and often producing conflicts when multiple patches in flight add a #include to the same file. [...] > @@ -48,11 +50,9 @@ int cmd_upload_pack(int argc, const char **argv, const > char *prefix) > > switch (determine_protocol_version_server()) { > case protocol_v2: > - /* > - * fetch support for protocol v2 has not been implemented yet, > - * so ignore the request to use v2 and fallback to using v0. > - */ > - upload_pack(); > + serve_opts.advertise_capabilities = opts.advertise_refs; > + serve_opts.stateless_rpc = opts.stateless_rpc; > + serve(_opts); Interesting! daemon.c has its own (file-local) serve() function; can one of the two be renamed? I actually like both names: serve() is a traditional name for the function a server calls when it's done setting up and is ready to serve. But the clash is confusing. [...] > +++ b/connect.c > @@ -12,9 +12,11 @@ > #include "sha1-array.h" > #include "transport.h" > #include "strbuf.h" > +#include "version.h" > #include "protocol.h" > > 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? > static const char *parse_feature_value(const char *, const char *, int *); > > static int check_ref(const char *name, unsigned int flags) > @@ -62,6 +64,33 @@ static void die_initial_contact(int unexpected) > "and the repository exists.")); > } > > +/* Checks if the server supports the capability 'c' */ > +int server_supports_v2(const char *c, int die_on_error) > +{ > + int i; > + > + for (i = 0; i < server_capabilities_v2.argc; i++) { > + const char *out; > + if (skip_prefix(server_capabilities_v2.argv[i], c, ) && > + (!*out || *out == '=')) > + return 1; > + } > + > + if (die_on_error) > + die("server doesn't support '%s'", c); > + > + return 0; > +} Nice. > + > +static void process_capabilities_v2(struct packet_reader *reader) > +{ > + while (packet_reader_read(reader) == PACKET_READ_NORMAL) > + argv_array_push(_capabilities_v2, reader->line); > + > + if (reader->status != PACKET_READ_FLUSH) > + die("protocol error"); Can this say more? E.g. "expected flush after capabilities, got "? [...] > @@ -85,7 +114,7 @@ enum protocol_version discover_version(struct > packet_reader *reader) > /* Maybe process capabilities here, at least for v2 */ Is this comment out of date now? > switch (version) { > case protocol_v2: > - die("support for protocol v2 not implemented yet"); > + process_capabilities_v2(reader); > break; > case protocol_v1: > /* Read the peeked version line */ > @@ -293,6 +322,98 @@ struct ref **get_remote_heads(struct packet_reader > *reader, > return list; > } > > +static int process_ref_v2(const char *line, struct ref ***list) What does the return value represent? Could it return the more typical 0 on success, -1 on error? > +{ > + 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(_sections, line, ' ', -1) < 2) { Can there be a comment describing the expected format? > + ret = 0; > + goto out; > + } > + > + if (get_oid_hex(line_sections.items[i++].string, _oid)) { > + ret = 0; > + goto out; > + } > + > + ref = alloc_ref(line_sections.items[i++].string); Ref names cannot contains a space, so this is safe. Good. > + > + oidcpy(>old_oid, _oid); > + **list = ref; > + *list = >next; > + > + for (; i < line_sections.nr; i++) { > + const char *arg = line_sections.items[i].string; > + if (skip_prefix(arg, "symref-target:", )) > + ref->symref = xstrdup(arg); Using space-delimited fields in a single pkt-line means that - values
Re: [PATCH v3 14/35] connect: request remote refs using v2
Jonathan Tan wrote: > On Thu, 22 Feb 2018 13:26:58 -0500 > Jeff Kingwrote: >> I agree that it shouldn't matter much here. But if the name argv_array >> is standing in the way of using it, I think we should consider giving it >> a more general name. I picked that not to evoke "this must be arguments" >> but "this is terminated by a single NULL". [...] > This sounds reasonable - I withdraw my comment about using struct > string_list. Marking with #leftoverbits as a reminder to think about what such a more general name would be (or what kind of docs to put in argv-array.h) and make it so the next time I do a search for that keyword. Thanks, Jonathan
Re: [PATCH v3 14/35] connect: request remote refs using v2
On Thu, 22 Feb 2018 13:26:58 -0500 Jeff Kingwrote: > On Thu, Feb 22, 2018 at 10:19:22AM -0800, Brandon Williams wrote: > > > On 02/21, Jonathan Tan wrote: > > > On Tue, 6 Feb 2018 17:12:51 -0800 > > > Brandon Williams wrote: > > > > > > > +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); > > > > > > I haven't looked at the rest of this patch in detail, but the type of > > > ref_patterns is probably better as struct string_list, since this is not > > > a true argument array (e.g. with flags starting with --). Same comment > > > for the next few patches that deal with ref patterns. > > > > Its just a list of strings which don't require having a util pointer > > hanging around so actually using an argv_array would be more memory > > efficient than a string_list. But either way I don't think it matters > > much. > > I agree that it shouldn't matter much here. But if the name argv_array > is standing in the way of using it, I think we should consider giving it > a more general name. I picked that not to evoke "this must be arguments" > but "this is terminated by a single NULL". > > In general I think it should be the preferred structure for string > lists, just because it actually converts for free to the "other" common > format (whereas you can never pass string_list.items to a function that > doesn't know about string lists). This sounds reasonable - I withdraw my comment about using struct string_list.
Re: [PATCH v3 14/35] connect: request remote refs using v2
On Thu, Feb 22, 2018 at 10:19:22AM -0800, Brandon Williams wrote: > On 02/21, Jonathan Tan wrote: > > On Tue, 6 Feb 2018 17:12:51 -0800 > > Brandon Williamswrote: > > > > > +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); > > > > I haven't looked at the rest of this patch in detail, but the type of > > ref_patterns is probably better as struct string_list, since this is not > > a true argument array (e.g. with flags starting with --). Same comment > > for the next few patches that deal with ref patterns. > > Its just a list of strings which don't require having a util pointer > hanging around so actually using an argv_array would be more memory > efficient than a string_list. But either way I don't think it matters > much. I agree that it shouldn't matter much here. But if the name argv_array is standing in the way of using it, I think we should consider giving it a more general name. I picked that not to evoke "this must be arguments" but "this is terminated by a single NULL". In general I think it should be the preferred structure for string lists, just because it actually converts for free to the "other" common format (whereas you can never pass string_list.items to a function that doesn't know about string lists). -Peff
Re: [PATCH v3 14/35] connect: request remote refs using v2
On 02/21, Jonathan Tan wrote: > On Tue, 6 Feb 2018 17:12:51 -0800 > Brandon Williamswrote: > > > +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); > > I haven't looked at the rest of this patch in detail, but the type of > ref_patterns is probably better as struct string_list, since this is not > a true argument array (e.g. with flags starting with --). Same comment > for the next few patches that deal with ref patterns. Its just a list of strings which don't require having a util pointer hanging around so actually using an argv_array would be more memory efficient than a string_list. But either way I don't think it matters much. -- Brandon Williams
Re: [PATCH v3 14/35] connect: request remote refs using v2
On Tue, 6 Feb 2018 17:12:51 -0800 Brandon Williamswrote: > +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); I haven't looked at the rest of this patch in detail, but the type of ref_patterns is probably better as struct string_list, since this is not a true argument array (e.g. with flags starting with --). Same comment for the next few patches that deal with ref patterns.