From c4716086d8653ff2761b5f8b0452c97c24e117ed Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Feb 2023 01:37:22 -0500 Subject: ls-refs: drop config caching The code for the v2 ls-refs command has an ensure_config_read() function that tries to read the lsrefs.unborn config only once and caches it in some static global variables. There's no real need for this caching. In any given process we'd only need the value twice (once to decide whether to advertise, and once if somebody runs the command). And since the config code already has its own cache, each access is only incurring a hash lookup and string comparison anyway. Since the values we set are going to be specific to the_repository, the globals we set are a mild anti-pattern. In practice it's not a bug (yet) since the server-side v2 code only handles a single repository anyway. But it doesn't hurt to take a small step in the right direction and model a good approach. Note that we currently set two booleans: advertise_unborn and allow_unborn. But we can get away with a single value, since "advertise" naturally implies "allow". That lets us just convert this to a function with a return value. Note that we still always read from the_repository; we'll deal with that in a follow-on patch. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- ls-refs.c | 34 ++++++++++++---------------------- 1 file changed, 12 insertions(+), 22 deletions(-) (limited to 'ls-refs.c') diff --git a/ls-refs.c b/ls-refs.c index 697d4beb8d..4863813655 100644 --- a/ls-refs.c +++ b/ls-refs.c @@ -8,38 +8,32 @@ #include "config.h" #include "string-list.h" -static int config_read; -static int advertise_unborn; -static int allow_unborn; - -static void ensure_config_read(void) +static enum { + UNBORN_IGNORE = 0, + UNBORN_ALLOW, + UNBORN_ADVERTISE /* implies ALLOW */ +} unborn_config(void) { const char *str = NULL; - if (config_read) - return; - if (repo_config_get_string_tmp(the_repository, "lsrefs.unborn", &str)) { /* * If there is no such config, advertise and allow it by * default. */ - advertise_unborn = 1; - allow_unborn = 1; + return UNBORN_ADVERTISE; } else { if (!strcmp(str, "advertise")) { - advertise_unborn = 1; - allow_unborn = 1; + return UNBORN_ADVERTISE; } else if (!strcmp(str, "allow")) { - allow_unborn = 1; + return UNBORN_ALLOW; } else if (!strcmp(str, "ignore")) { - /* do nothing */ + return UNBORN_IGNORE; } else { die(_("invalid value for '%s': '%s'"), "lsrefs.unborn", str); } } - config_read = 1; } /* @@ -159,7 +153,6 @@ int ls_refs(struct repository *r, struct packet_reader *request) strbuf_init(&data.buf, 0); string_list_init_dup(&data.hidden_refs); - ensure_config_read(); git_config(ls_refs_config, &data); while (packet_reader_read(request) == PACKET_READ_NORMAL) { @@ -175,7 +168,7 @@ int ls_refs(struct repository *r, struct packet_reader *request) strvec_push(&data.prefixes, out); } else if (!strcmp("unborn", arg)) - data.unborn = allow_unborn; + data.unborn = !!unborn_config(); else die(_("unexpected line: '%s'"), arg); } @@ -206,11 +199,8 @@ int ls_refs(struct repository *r, struct packet_reader *request) int ls_refs_advertise(struct repository *r, struct strbuf *value) { - if (value) { - ensure_config_read(); - if (advertise_unborn) - strbuf_addstr(value, "unborn"); - } + if (value && unborn_config() == UNBORN_ADVERTISE) + strbuf_addstr(value, "unborn"); return 1; } -- cgit v1.2.3 From 4b4e75dd4f1dac0c25bded7466b0cc20c9649efb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Feb 2023 01:38:10 -0500 Subject: serve: use repository pointer to get config A few of the v2 "serve" callbacks ignore their repository parameter and read config using the_repository (either directly or implicitly by calling wrapper functions). This isn't a bug since the server code only handles a single main repository anyway (and indeed, if you look at the callers, these repository parameters will always be the_repository). But in the long run we want to get rid of the_repository, so let's take a tiny step in that direction. As a bonus, this silences some -Wunused-parameter warnings. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- ls-refs.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'ls-refs.c') diff --git a/ls-refs.c b/ls-refs.c index 4863813655..bd9ab2c348 100644 --- a/ls-refs.c +++ b/ls-refs.c @@ -12,11 +12,11 @@ static enum { UNBORN_IGNORE = 0, UNBORN_ALLOW, UNBORN_ADVERTISE /* implies ALLOW */ -} unborn_config(void) +} unborn_config(struct repository *r) { const char *str = NULL; - if (repo_config_get_string_tmp(the_repository, "lsrefs.unborn", &str)) { + if (repo_config_get_string_tmp(r, "lsrefs.unborn", &str)) { /* * If there is no such config, advertise and allow it by * default. @@ -168,7 +168,7 @@ int ls_refs(struct repository *r, struct packet_reader *request) strvec_push(&data.prefixes, out); } else if (!strcmp("unborn", arg)) - data.unborn = !!unborn_config(); + data.unborn = !!unborn_config(r); else die(_("unexpected line: '%s'"), arg); } @@ -199,7 +199,7 @@ int ls_refs(struct repository *r, struct packet_reader *request) int ls_refs_advertise(struct repository *r, struct strbuf *value) { - if (value && unborn_config() == UNBORN_ADVERTISE) + if (value && unborn_config(r) == UNBORN_ADVERTISE) strbuf_addstr(value, "unborn"); return 1; -- cgit v1.2.3