git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFC/PATCH 1/2] config: Add cache for config value querying

From
Eric Sunshine <sunshine@sunshineco.com>
Date
May 28, 2014, 09:31 UTC
Message-ID
<CAPig+cSvrJ0bjuHBaRpUYzi7tX+MEbhmCYv12oiTcexm3+Pd0g@mail.gmail.com>
In-Reply-To
<1401125601-18249-2-git-send-email-tanayabh@gmail.com>
On Mon, May 26, 2014 at 1:33 PM, Tanay Abhra <tanayabh@gmail.com> wrote:
> Add an internal cache with the all variable value pairs read from the usual
> config files(repo specific .git/config, user wide ~/.gitconfig and the global
s/(/ (/
> /etc/gitconfig). Also, add two external functions `git_config_get_string` and
> `git_config_get_string_multi` for querying in an non callback manner from the

s/an/a/ s/non/non-/

More below.
Show 39 quoted lines
> cache.
>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
>  cache.h  |   2 ++
>  config.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 107 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index 107ac61..2038150 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1271,6 +1271,8 @@ extern int check_repository_format_version(const char *var, const char *value, v
>  extern int git_env_bool(const char *, int);
>  extern int git_config_system(void);
>  extern int config_error_nonbool(const char *);
> +extern const char *git_config_get_string(const char *);
> +extern const struct string_list *git_config_get_string_multi(const char *);
>  #if defined(__GNUC__) && ! defined(__clang__)
>  #define config_error_nonbool(s) (config_error_nonbool(s), -1)
>  #endif
> diff --git a/config.c b/config.c
> index a30cb5c..f6515c2 100644
> --- a/config.c
> +++ b/config.c
> @@ -9,6 +9,8 @@
>  #include "exec_cmd.h"
>  #include "strbuf.h"
>  #include "quote.h"
> +#include "hashmap.h"
> +#include "string-list.h"
>
>  struct config_source {
>         struct config_source *prev;
> @@ -37,6 +39,102 @@ static struct config_source *cf;
>
>  static int zlib_compression_seen;
>
> +struct hashmap config_cache;
This should be 'static'.
> +struct config_cache_node {
> +       struct hashmap_entry ent;
> +       struct strbuf key;

Upon reading this, I had the same question as Torsten: Why isn't 'key' a plain 'char *' allocated when the entry is created? Your answer:

    To maintain consistency with config.c. config.c uses strbuf for
    both key and value throughout. I found the reason by git-blaming
    config.c. Key length is flexible so it would be better to use a
    api construct such as strbuf for it.

doesn't make sense. The existing callback-based code re-uses the strbuf for each key as it parses the file, however, you are just storing each key once in the hashmap and never altering it, thus a strbuf is overkill.

> +       struct string_list *value_list ;

Why isn't this just a plain 'struct string_list'? Why the extra indirection of a pointer?

Drop space before semicolon.
> +};
> +
> +static int hashmap_init_v = 0;
It's not obvious what "_v" is meant to convey.
Show 13 quoted lines
> +static int config_cache_node_cmp(const struct config_cache_node *e1,
> +                          const struct config_cache_node *e2, const char *key)
> +{
> +       return strcmp(e1->key.buf, key ? key : e2->key.buf);
> +}
> +
> +static int config_cache_node_cmp_icase(const struct config_cache_node *e1,
> +                                const struct config_cache_node *e2, const char* key)
> +{
> +       return strcasecmp(e1->key.buf, key ? key : e2->key.buf);
> +}
> +
> +static void config_cache_init(const int icase)

'const int' as a function argument is very unusual in this code base. (I found only a single instance of it in log-tree.[ch]:parse_decorate_color_config.)

Show 9 quoted lines
> +{
> +       hashmap_init(&config_cache, (hashmap_cmp_fn) (icase ? config_cache_node_cmp_icase
> +                       : config_cache_node_cmp), 0);
> +}
> +
> +static void config_cache_free(void)
> +{
> +       hashmap_free(&config_cache, 1);
> +}
Doesn't this leak the strbuf 'key' and string_list 'value_list'?
> +static struct config_cache_node *config_cache_find_entry(const char *key)
> +{
> +       struct hashmap_entry k;
> +       hashmap_entry_init(&k, strihash(key));

How does this unconditional use of case-insensitive strihash() interact with the 'icase' argument of config_cache_init()?

Show 7 quoted lines
> +       return hashmap_get(&config_cache, &k, key);
> +}
> +
> +static struct string_list *config_cache_get_value(const char *key)
> +{
> +       struct config_cache_node *e = config_cache_find_entry(key);
> +       if (e == NULL)
Rephrase: if (!e)
> +               return NULL;
> +       else
> +               return (e->value_list);
Unnecessary parentheses.
> +}
A purely subject rewrite of the above:
    return e ? e->value_list : NULL;
Show 6 quoted lines
> +static void config_cache_set_value(const char *key, const char *value)
> +{
> +       struct config_cache_node *e;
> +
> +       if (!value)
> +               return;

Again, I had the same questions as Torsten. ("Is this supposed to delete the entry?", "Is it a programmer error?", etc.) From a pure reading, it's not obvious why a NULL 'value' is handled this way. The intent might be clearer if the caller instead performs the check, and invokes config_cache_set_value() only if 'value' is non-NULL.

> +       e = config_cache_find_entry(key);
> +       if (!e) {
> +               e = malloc(sizeof(*e));
> +               hashmap_entry_init(e, strihash(key));

Same question as above regarding unconditional case-insensitive strihash() versus the 'icase' argument of config_cache_init().

> +               strbuf_init(&(e->key), 1024);

Why allocate such a large buffer for each key, especially when most keys likely are quite short. Since the key is never altered after being assigned, this seems like considerable waste.

Also, unnecessary parentheses.
> +               strbuf_addstr(&(e->key),key);
Unnecessary parentheses. Add space after comma.
Show 6 quoted lines
> +               e->value_list = malloc(sizeof(struct string_list));
> +               e->value_list->strdup_strings = 1;
> +               e->value_list->nr = 0;
> +               e->value_list->alloc = 0;
> +               e->value_list->items = NULL;
> +               e->value_list->cmp = NULL;

This string_list initialization code begs for a preparatory patch which adds a string_list_init() function (akin to strbuf_init()).

> +               string_list_append(e->value_list, value);
> +               hashmap_add(&config_cache, e);
> +       }
> +       else {
Cuddle the else with the braces: } else {
> +               if (!unsorted_string_list_has_string((e->value_list), value))
> +                       string_list_append((e->value_list), value);
Unnecessary parentheses (on both lines).

Since there is only an 'if' statement in this 'else' block, you could turn it into an 'else if', thus eliminating one level of indentation.

Show 13 quoted lines
> +       }
> +}
> +
> +extern const char *git_config_get_string(const char *name)
> +{
> +       struct string_list *values;
> +       int num_values;
> +       char *result;
> +       values = config_cache_get_value(name);
> +       if (!values)
> +               return NULL;
> +       num_values = values->nr;
> +       result = values->items[num_values-1].string ;
Add spaces around '-'. Drop space before semicolon.
> +       return result;

Why do you need the 'result' variable if you are returning it immediately after it's assigned?

    return values->items[num_values - 1].string;
Or, taking Torsten's recommendation into consideration to drop num_values:
    return values->items[values->nr - 1].string;
Show 33 quoted lines
> +}
> +
> +extern const struct string_list *git_config_get_string_multi(const char *key)
> +{
> +       return config_cache_get_value(key);
> +}
> +
>  static int config_file_fgetc(struct config_source *conf)
>  {
>         return fgetc(conf->u.file);
> @@ -333,6 +431,7 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)
>                 if (!value)
>                         return -1;
>         }
> +       config_cache_set_value(name->buf, value);
>         return fn(name->buf, value, data);
>  }
>
> @@ -1135,6 +1234,12 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
>         char *xdg_config = NULL;
>         char *user_config = NULL;
>
> +       int icase = 1;
> +       if (!hashmap_init_v) {
> +               config_cache_init(icase);
> +               hashmap_init_v = 1;
> +       }
> +
>         home_config_paths(&user_config, &xdg_config, "config");
>
>         if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {
> --
> 1.9.0.GIT
Previous: Tanay AbhraNext: Tanay Abhra
Message 5 of 7 in “Git config cache & special querying api utilizing the cache”
  1. 0/2 Git config cache & special querying api utilizing the cacheTanay Abhra, May 26, 2014
  2. 1/2 config: Add cache for config value queryingTanay Abhra, May 26, 2014
  3. Torsten BögershausenMay 26, 2014
  4. Tanay AbhraMay 27, 2014
  5. Eric SunshineMay 28, 2014
  6. 2/2 config: Add new query functions to the apiTanay Abhra, May 26, 2014
  7. Eric SunshineMay 28, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.