On Tue, 6 Feb 2018 17:12:41 -0800 Brandon Williams <bmwill@xxxxxxxxxx> wrote: > In order to allow for code sharing with the server-side of fetch in > protocol-v2 convert upload-pack to be a builtin. > > Signed-off-by: Brandon Williams <bmwill@xxxxxxxxxx> As Stefan mentioned in [1], also mention in the commit message that this means that the "git-upload-pack" invocation gains additional capabilities (for example, invoking a pager for --help). Having said that, the main purpose of this patch seems to be to libify upload-pack, and the move to builtin is just a way of putting the program somewhere - we could have easily renamed upload-pack.c and created a new upload-pack.c containing the main(), preserving the non-builtin-ness of upload-pack, while still gaining the benefits of libifying upload-pack. If the community does want to make upload-pack a builtin, I would write the commit message this way: upload-pack: libify Libify upload-pack. The main() function is moved to builtin/upload-pack.c, thus making upload-pack a builtin. Note that this means that "git-upload-pack" gains functionality such as the ability to invoke a pager when passed "--help". And if not: upload-pack: libify Libify upload-pack by moving most of the functionality in upload-pack.c into a file upload-pack-lib.c (or some other name), to be used in subsequent patches. [1] https://public-inbox.org/git/CAGZ79kb2=uU0_K8wr27gNdNX-T+P+7gVdgc5EBdYc3zBobsR8w@xxxxxxxxxxxxxx/ > -static void upload_pack(void) > -{ > - struct string_list symref = STRING_LIST_INIT_DUP; > - > - head_ref_namespaced(find_symref, &symref); > - > - if (advertise_refs || !stateless_rpc) { > - reset_timeout(); > - head_ref_namespaced(send_ref, &symref); > - for_each_namespaced_ref(send_ref, &symref); > - advertise_shallow_grafts(1); > - packet_flush(1); > - } else { > - head_ref_namespaced(check_ref, NULL); > - for_each_namespaced_ref(check_ref, NULL); > - } > - string_list_clear(&symref, 1); > - if (advertise_refs) > - return; > - > - receive_needs(); > - if (want_obj.nr) { > - get_common_commits(); > - create_pack_file(); > - } > -} I see that this function had to be moved to the bottom because it now also needs to make use of functions like upload_pack_config() - that's fine. > +struct upload_pack_options { > + int stateless_rpc; > + int advertise_refs; > + unsigned int timeout; > + int daemon_mode; > +}; I would have expected "unsigned stateless_rpc : 1" etc., but I see that this makes it easier to use with OPT_BOOL (which needs us to pass it a pointer-to-int). As for what existing code does, files like fetch-pack and diff use "unsigned : 1", but they also process arguments without OPT_, so I don't think they are relevant. I think that we should decide if we're going to prefer "unsigned : 1" or "int" for flags in new code. Personally, I prefer "unsigned : 1" (despite the slight inconvenience in that argument parsers will need to declare their own temporary "int" and then assign its contents to the options struct) because of the stronger type, but I'm OK either way. Whatever the decision, I don't think it needs to block the review of this patch set.
