As Jeff pointed out on v1 the control flow around serve.c was more subtle than I expected, i.e. we might process N requests: http://lore.kernel.org/git/YMolS8iJAMgbbg9D@xxxxxxxxxxxxxxxxxxxxxxx This v2 has a 1-4/8 that's a trivially improved version of what Junio picked up for "seen", the only change is addressing Eric's feedback that the "struct strvec;" could also be removed. Then 5-6/8 is a new trivial refactoring of the various callback invocations into a rapper function, allowing us to add a trace2 region for the advertisements & command we run. After that 7-8/8 is a new approach of doing this "configure" callback, I was on the fence about whether it should be another series on top, but decided to submit it as one thing. Now instead of there being a "configure" we explicitly have a "startup_config" where we expect to stick all our config that doesn't change, won't be different depending on what "mode" or "command" we operate as, and in the case of upload-pack doesn't depend on anything in "upload_pack_data". As the diffstat shows it doesn't make the code shorter, but I think it makes it easier to read and maintain, as we now clearly separate the types of config we're reading. Ævar Arnfjörð Bjarmason (8): serve: mark has_capability() as static transport: rename "fetch" in transport_vtable to "fetch_refs" transport: use designated initializers serve: use designated initializers serve.c: add call_{advertise,command}() indirection serve.c: add trace2 regions for advertise & command serve: add support for a "startup" git_config() callback upload-pack.c: convert to new serve.c "startup" config cb ls-refs.c | 42 +++----- ls-refs.h | 1 + serve.c | 138 +++++++++++++++++++++---- serve.h | 4 - t/t5705-session-id-in-capabilities.sh | 16 ++- transport-helper.c | 18 ++-- transport-internal.h | 2 +- transport.c | 32 +++--- upload-pack.c | 139 +++++++++++++++----------- upload-pack.h | 2 + 10 files changed, 255 insertions(+), 139 deletions(-) Range-diff against v1: 1: 4f74d7d34c4 ! 1: fdb0c5f4df1 serve: mark has_capability() as static @@ serve.c: static int is_command(const char *key, struct protocol_capability **com ## serve.h ## @@ + #ifndef SERVE_H #define SERVE_H - struct strvec; +-struct strvec; -int has_capability(const struct strvec *keys, const char *capability, - const char **value); - 2: c6720d1bf33 = 2: 7b8101dca71 transport: rename "fetch" in transport_vtable to "fetch_refs" 3: 369efe0eed5 = 3: 766045e5f1d transport: use designated initializers 4: 8e97412d584 = 4: bd920de1629 serve: use designated initializers -: ----------- > 5: d0b02aa0c7d serve.c: add call_{advertise,command}() indirection -: ----------- > 6: baeee6539ad serve.c: add trace2 regions for advertise & command 5: c8625025125 ! 7: 0a4fb01ae38 serve: add support for a git_config() callback @@ Metadata Author: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## Commit message ## - serve: add support for a git_config() callback + serve: add support for a "startup" git_config() callback Since the introduction of serve.c we've added git_config() callbacks and other config reading for capabilities in the following commits: - e20b4192a37 (upload-pack: support hidden refs with protocol v2, 2018-12-18) - - 59e1205d167 (ls-refs: report unborn targets of symrefs, 2021-02-05) + - 08450ef7918 (upload-pack: clear filter_options for each v2 fetch command, 2020-05-08) - 6b5b6e422ee (serve: advertise session ID in v2 capabilities, 2020-11-11) + - 59e1205d167 (ls-refs: report unborn targets of symrefs, 2021-02-05) - This code can be simplified by simply adding support to serve.c itself - for reading config at the right time before the capability is - called. In particular the ensure_config_read() function being gone - makes this whole thing much simpler. + Of these 08450ef7918 fixed code that needed to read config on a + per-request basis, whereas most of the config reading just wants to + check if we've enabled one semi-static config variable or other. We'd + like to re-read that value eventually, but from request-to-request + it's OK if we retain the old one, and it isn't impacted by other + request data. - A behavior change here is that we're eagerly doing the configuration - for all our capabilities regardless of which one we end up serving, - whereas before we'd defer it until the point where we were processing - the specific command. + So let's support this common pattern as a "startup_config" callback, + making use of our recently added "call_{advertise,command}()" + functions. This allows us to simplify e.g. the "ensure_config_read()" + function added in 59e1205d167 (ls-refs: report unborn targets of + symrefs, 2021-02-05). - We could try to be more lazy here and only read the config if we find - out that we're serving "ls-refs" or "session-id", but it's just a fast - git_config() callback, I think in this case simplicity trumps a - premature optimization. + We could read all the config for all the protocol capabilities, but + let's do it one callback at a time in anticipation that some won't be + called at all, and that some might be more expensive than others in + the future. + + I'm not migrating over the code in the upload_pack_v2 function in + upload-pack.c yet, that case is more complex since it deals with both + v1 and v2. It will be dealt with in a code a subsequent commit. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> @@ ls-refs.c -static int config_read; -static int advertise_unborn; -static int allow_unborn; -- ++/* "unborn" is on by default if there's no lsrefs.unborn config */ ++static int advertise_unborn = 1; ++static int allow_unborn = 1; + -static void ensure_config_read(void) --{ ++int ls_refs_startup_config(const char *var, const char *value, void *data) + { - const char *str = NULL; - - if (config_read) @@ ls-refs.c - } else { - if (!strcmp(str, "advertise")) { - advertise_unborn = 1; -- allow_unborn = 1; -- } else if (!strcmp(str, "allow")) { -- allow_unborn = 1; -- } else if (!strcmp(str, "ignore")) { -- /* do nothing */ -- } else { -- die(_("invalid value '%s' for lsrefs.unborn"), str); -- } -- } -- config_read = 1; --} -+/* "unborn" is on by default if there's no lsrefs.unborn config */ -+static int advertise_unborn = 1; -+static int allow_unborn = 1; - - /* - * Check if one of the prefixes is a prefix of the ref. -@@ ls-refs.c: static void send_possibly_unborn_head(struct ls_refs_data *data) - strbuf_release(&namespaced); - } - --static int ls_refs_config(const char *var, const char *value, void *data) -+int ls_refs_configure(const char *var, const char *value, void *data) - { + if (!strcmp(var, "lsrefs.unborn")) { + if (!strcmp(value, "advertise")) { + /* Allowed and advertised by default */ + } else if (!strcmp(value, "allow")) { + advertise_unborn = 0; -+ allow_unborn = 1; + allow_unborn = 1; +- } else if (!strcmp(str, "allow")) { +- allow_unborn = 1; +- } else if (!strcmp(str, "ignore")) { +- /* do nothing */ + } else if (!strcmp(value, "ignore")) { + advertise_unborn = 0; + allow_unborn = 0; -+ } else { + } else { +- die(_("invalid value '%s' for lsrefs.unborn"), str); + die(_("invalid value '%s' for lsrefs.unborn"), value); -+ } -+ } -+ - /* - * We only serve fetches over v2 for now, so respect only "uploadpack" - * config. This may need to eventually be expanded to "receive", but we + } + } +- config_read = 1; ++ return 0; + } + + /* @@ ls-refs.c: int ls_refs(struct repository *r, struct strvec *keys, + memset(&data, 0, sizeof(data)); strvec_init(&data.prefixes); - -- ensure_config_read(); -- git_config(ls_refs_config, NULL); - +- ensure_config_read(); + git_config(ls_refs_config, NULL); + while (packet_reader_read(request) == PACKET_READ_NORMAL) { - const char *arg = request->line; - const char *out; @@ ls-refs.c: int ls_refs(struct repository *r, struct strvec *keys, int ls_refs_advertise(struct repository *r, struct strbuf *value) { @@ ls-refs.h: struct strvec; struct packet_reader; int ls_refs(struct repository *r, struct strvec *keys, struct packet_reader *request); -+int ls_refs_configure(const char *var, const char *value, void *data); ++int ls_refs_startup_config(const char *var, const char *value, void *data); int ls_refs_advertise(struct repository *r, struct strbuf *value); #endif /* LS_REFS_H */ @@ serve.c: static int object_format_advertise(struct repository *r, return 1; } -+static int session_id_configure(const char *var, const char *value, void *data) ++static int session_id_startup_config(const char *var, const char *value, void *data) +{ + if (!strcmp(var, "transfer.advertisesid")) + advertise_sid = git_config_bool(var, value); @@ serve.c: struct protocol_capability { const char *name; + /* -+ * A git_config() callback. If defined it'll be dispatched too -+ * sometime before the other functions are called, giving the -+ * capability a chance to configure itself. ++ * A git_config() callback that'll be called only once for the ++ * lifetime of the process, possibly over many different ++ * requests. Used for reading config that's expected to be ++ * static. ++ * ++ * The "command" or "advertise" callbacks themselves are ++ * expected to read config that needs to be more current than ++ * that, or which is dependent on request data. + */ -+ int (*configure)(const char *var, const char *value, void *data); ++ int (*startup_config)(const char *var, const char *value, void *data); ++ ++ /* ++ * A boolean to check if we've called our "startup_config" ++ * callback. ++ */ ++ int have_startup_config; + /* * Function queried to see if a capability should be advertised. @@ serve.c: static struct protocol_capability capabilities[] = { }, { .name = "ls-refs", -+ .configure = ls_refs_configure, ++ .startup_config = ls_refs_startup_config, .advertise = ls_refs_advertise, .command = ls_refs, }, @@ serve.c: static struct protocol_capability capabilities[] = { }, { .name = "session-id", -+ .configure = session_id_configure, ++ .startup_config = session_id_startup_config, .advertise = session_id_advertise, }, { @@ serve.c: static struct protocol_capability capabilities[] = { }, }; -+static int git_serve_config(const char *var, const char *value, void *data) ++static void read_startup_config(struct protocol_capability *command) +{ -+ int i; -+ for (i = 0; i < ARRAY_SIZE(capabilities); i++) { -+ struct protocol_capability *c = &capabilities[i]; -+ config_fn_t fn = c->configure; -+ int ret; -+ if (!fn) -+ continue; -+ ret = fn(var, value, data); -+ if (ret) -+ return ret; -+ } -+ return 0; ++ if (!command->startup_config) ++ return; ++ if (command->have_startup_config++) ++ return; ++ git_config(command->startup_config, NULL); +} + - static void advertise_capabilities(void) + static int call_advertise(struct protocol_capability *command, + struct repository *r, struct strbuf *value) { - struct strbuf capability = STRBUF_INIT; +@@ serve.c: static int call_advertise(struct protocol_capability *command, + struct strbuf sb = STRBUF_INIT; + const char *msg; + ++ read_startup_config(command); ++ + strbuf_addf(&sb, "advertise/%s", command->name); + trace2_region_enter("serve", sb.buf, r); + ret = command->advertise(r, value); +@@ serve.c: static int call_command(struct protocol_capability *command, + int ret; + struct strbuf sb = STRBUF_INIT; + ++ read_startup_config(command); ++ + strbuf_addf(&sb, "command/%s", command->name); + trace2_region_enter("serve", sb.buf, r); + ret = command->command(r, keys, request); @@ serve.c: static int process_request(void) /* Main serve loop for protocol version 2 */ void serve(struct serve_options *options) { - git_config_get_bool("transfer.advertisesid", &advertise_sid); -+ git_config(git_serve_config, NULL); - +- if (options->advertise_capabilities || !options->stateless_rpc) { /* serve by default supports v2 */ + packet_write_fmt(1, "version 2\n"); -: ----------- > 8: 9fd6138aa62 upload-pack.c: convert to new serve.c "startup" config cb -- 2.32.0.611.gd4a17395dfa
