git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [RFC/PATCH 0/2] Git config cache & special querying api utilizing the cache
@ 2014-05-26 17:33 Tanay Abhra
  2014-05-26 17:33 ` [RFC/PATCH 1/2] config: Add cache for config value querying Tanay Abhra
  2014-05-26 17:33 ` [RFC/PATCH 2/2] config: Add new query functions to the api Tanay Abhra
  0 siblings, 2 replies; 7+ messages in thread
From: Tanay Abhra @ 2014-05-26 17:33 UTC (permalink / raw
  To: git; +Cc: Tanay Abhra, Ramkumar Ramachandra, Matthieu Moy

Hi,

This is my first patch series for this year's GSoC. My project is
"Git Config API improvements". The link of my proposal is appended below [1].

The aim of this patch series is to generate a cache for querying values from
the config files in a non callback manner as the current method reads and
parses the config files every time a value is queried for.

The cache is generated from hooking the update_cache function to the current
parsing and callback mechanism in config.c. It is implemented as an hashmap
using the hashmap-api with variables and its corresponding values list as
its members. The values in the list are sorted in order of increasing priority.
The cache is initialised in git_config_early() as it is the first time a `git_config`
function is called during program startup. setup_git_directory_gently() calls
git_config_early() which in turn reads every config file (local, user and
global config files).

get_value() in config.c feeds variable and values into the callback function.
Using this function as a hook, we update the cache. Also, we add two new
functions to the config-api git_config_get_string() and
git_config_get_string_multi() for querying in a non callback manner from
the cache.

I have run the tests and debug the code and it works, but I have to add a
few things,

1. Invalidity check: if a config file is written into, update the cache.
   I am using git_config_set_multivar_in_file() as an update hook.

2. Metadata about the variables and values. I have added only the file
   from each variable value pair comes in an upcoming series.

What else should I add or implement ;is my approach right? 

[1] https://drive.google.com/file/d/0B4suZ-aHqDcnSUZJRXVTTnZUN1E/edit?usp=sharing

Cheers,
Tanay Abhra.

Tanay Abhra (2):
  config: Add cache for config value querying
  config: Add new query functions to the api

 Documentation/technical/api-config.txt |  19 ++++++
 cache.h                                |   2 +
 config.c                               | 105 +++++++++++++++++++++++++++++++++
 3 files changed, 126 insertions(+)

-- 
1.9.0.GIT

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC/PATCH 1/2] config: Add cache for config value querying
  2014-05-26 17:33 [RFC/PATCH 0/2] Git config cache & special querying api utilizing the cache Tanay Abhra
@ 2014-05-26 17:33 ` Tanay Abhra
  2014-05-26 20:02   ` Torsten Bögershausen
  2014-05-28  9:31   ` Eric Sunshine
  2014-05-26 17:33 ` [RFC/PATCH 2/2] config: Add new query functions to the api Tanay Abhra
  1 sibling, 2 replies; 7+ messages in thread
From: Tanay Abhra @ 2014-05-26 17:33 UTC (permalink / raw
  To: git; +Cc: Tanay Abhra, Ramkumar Ramachandra, Matthieu Moy

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
/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
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;
+
+struct config_cache_node {
+	struct hashmap_entry ent;
+	struct strbuf key;
+	struct string_list *value_list ;
+};
+
+static int hashmap_init_v = 0;
+
+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)
+{
+	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);
+}
+
+static struct config_cache_node *config_cache_find_entry(const char *key)
+{
+	struct hashmap_entry k;
+	hashmap_entry_init(&k, strihash(key));
+	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)
+		return NULL;
+	else
+		return (e->value_list);
+}
+
+
+static void config_cache_set_value(const char *key, const char *value)
+{
+	struct config_cache_node *e;
+
+	if (!value)
+		return;
+
+	e = config_cache_find_entry(key);
+	if (!e) {
+		e = malloc(sizeof(*e));
+		hashmap_entry_init(e, strihash(key));
+		strbuf_init(&(e->key), 1024);
+		strbuf_addstr(&(e->key),key);
+		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;
+		string_list_append(e->value_list, value);
+		hashmap_add(&config_cache, e);
+	}
+	else {
+		if (!unsorted_string_list_has_string((e->value_list), value))
+			string_list_append((e->value_list), value);
+	}
+}
+
+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 ;
+	return result;
+}
+
+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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [RFC/PATCH 2/2] config: Add new query functions to the api
  2014-05-26 17:33 [RFC/PATCH 0/2] Git config cache & special querying api utilizing the cache Tanay Abhra
  2014-05-26 17:33 ` [RFC/PATCH 1/2] config: Add cache for config value querying Tanay Abhra
@ 2014-05-26 17:33 ` Tanay Abhra
  2014-05-28  9:43   ` Eric Sunshine
  1 sibling, 1 reply; 7+ messages in thread
From: Tanay Abhra @ 2014-05-26 17:33 UTC (permalink / raw
  To: git; +Cc: Tanay Abhra, Ramkumar Ramachandra, Matthieu Moy

Add explanations for `git_config_get_string_multi` and `git_config_get_string`
which utilize the config cache for querying in an non callback manner for a
specific variable.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 Documentation/technical/api-config.txt | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index 230b3a0..33b5b90 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -77,6 +77,25 @@ To read a specific file in git-config format, use
 `git_config_from_file`. This takes the same callback and data parameters
 as `git_config`.
 
+Querying for specific variables
+-------------------------------
+
+For programs wanting to query for specific variables in a non callback
+manner, the config API provides two functions `git_config_get_string`
+and `git_config_get_string_multi`. They both take a single parameter,
+
+- a variable as the key string for which the corresponding values will
+  be retrieved and returned.
+
+They both read value from an internal cache generated previously from
+reading the config files. `git_config_get_string` returns the value with
+the highest priority(i.e. value in the repo config will be preferred over
+value in user wide config for the same variable).
+
+`git_config_get_string_multi` returns a `string_list` containing all the
+values for that particular variable, sorted in order of increasing
+priority.
+
 Value Parsing Helpers
 ---------------------
 
-- 
1.9.0.GIT

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC/PATCH 1/2] config: Add cache for config value querying
  2014-05-26 17:33 ` [RFC/PATCH 1/2] config: Add cache for config value querying Tanay Abhra
@ 2014-05-26 20:02   ` Torsten Bögershausen
  2014-05-27 13:54     ` Tanay Abhra
  2014-05-28  9:31   ` Eric Sunshine
  1 sibling, 1 reply; 7+ messages in thread
From: Torsten Bögershausen @ 2014-05-26 20:02 UTC (permalink / raw
  To: Tanay Abhra, git; +Cc: Ramkumar Ramachandra, Matthieu Moy

On 2014-05-26 19.33, Tanay Abhra wrote:
I like the idea.
Please allow some minor comments (and read them as questions rather then answers)
> Add an internal cache with the all variable value pairs read from the usual
"cache": The word "cache" is in Git often used for "index" 
"variable value" can be written as "key value"
"usual": I don't think we handle "unusual" config files,
(so can we drop the word usual ?)
I think the (important) first line can be written like this:

>Add a hash table with the all key-value pairs read from the
or
>Add a hash table to cache all key-value pairs read from the

> config files(repo specific .git/config, user wide ~/.gitconfig and the global
> /etc/gitconfig). Also, add two external functions `git_config_get_string` and
Can we drop "Also" ?
> @@ -37,6 +39,102 @@ static struct config_source *cf;
>  
>  static int zlib_compression_seen;
>  
> +struct hashmap config_cache;
> +
> +struct config_cache_node {
> +	struct hashmap_entry ent;
> +	struct strbuf key;
Do we need a whole strbuf for the key?
Or could a "char *key" work as well? 
(and/or could it be "const char *key" ?
> +	struct string_list *value_list ;



> +static struct string_list *config_cache_get_value(const char *key)
> +{
> +	struct config_cache_node *e = config_cache_find_entry(key);
why "e" ? Will "node" be easier to read ? Or entry ? 


> +static void config_cache_set_value(const char *key, const char *value)
> +{
> +	struct config_cache_node *e;
> +
> +	if (!value)
> +		return;
Hm, either NULL could mean "unset==remove" the value, (but we don't do that, do we?

Or it could mean a programming or runtime error?, Should there be a warning ?

> +
> +	e = config_cache_find_entry(key);
> +	if (!e) {
> +		e = malloc(sizeof(*e));
> +		hashmap_entry_init(e, strihash(key));
> +		strbuf_init(&(e->key), 1024);
> +		strbuf_addstr(&(e->key),key);
> +		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;
When malloc() is replaced by xcalloc()  the x = NULL and y = 0 can go away,
and the code is shorter and easier to read.


> +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 ;
We could get rid of the variable  "int num_values" by simply writing
result = values->items[values->nr-1].string;

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC/PATCH 1/2] config: Add cache for config value querying
  2014-05-26 20:02   ` Torsten Bögershausen
@ 2014-05-27 13:54     ` Tanay Abhra
  0 siblings, 0 replies; 7+ messages in thread
From: Tanay Abhra @ 2014-05-27 13:54 UTC (permalink / raw
  To: Torsten Bögershausen; +Cc: git, Ramkumar Ramachandra, Matthieu Moy

Hi,

On 05/26/2014 01:02 PM, Torsten Bögershausen wrote:
>> Add an internal cache with the all variable value pairs read from the usual
> "cache": The word "cache" is in Git often used for "index" 
Okay, point noted. I thought about choosing between "hashmap" and "cache" and chose
the later.
> "variable value" can be written as "key value"

I  had used the term "variable" to be consistent with the documentation
(api-config.txt). But I think "key" is much clearer.

> "usual": I don't think we handle "unusual" config files,
> (so can we drop the word usual ?)

Okay, noted.

> I think the (important) first line can be written like this:
> 
>> Add a hash table with the all key-value pairs read from the
> or
>> Add a hash table to cache all key-value pairs read from the
> 
>> config files(repo specific .git/config, user wide ~/.gitconfig and the global
>> /etc/gitconfig). Also, add two external functions `git_config_get_string` and
> Can we drop "Also" ?
>> @@ -37,6 +39,102 @@ static struct config_source *cf;
>>  
>>  static int zlib_compression_seen;
>>  
>> +struct hashmap config_cache;
>> +
>> +struct config_cache_node {
>> +	struct hashmap_entry ent;
>> +	struct strbuf key;
> Do we need a whole strbuf for the key?
> Or could a "char *key" work as well? 
> (and/or could it be "const char *key" ?

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.

>> +	struct string_list *value_list ;
> 
> 
> 
>> +static struct string_list *config_cache_get_value(const char *key)
>> +{
>> +	struct config_cache_node *e = config_cache_find_entry(key);
> why "e" ? Will "node" be easier to read ? Or entry ? 

Noted. Entry is much better.

>> +static void config_cache_set_value(const char *key, const char *value)
>> +{
>> +	struct config_cache_node *e;
>> +
>> +	if (!value)
>> +		return;
> Hm, either NULL could mean "unset==remove" the value, (but we don't do that, do we?
> 
> Or it could mean a programming or runtime error?, Should there be a warning ?

Nope. It is just a check to not save blank values for a key in the hashmap. Removal
functionality will come later. NULL==remove is implemented in
git_config_set_multivar_in_file(). We are not reading key value pairs from that, just
from git_config().
>> +
>> +	e = config_cache_find_entry(key);
>> +	if (!e) {
>> +		e = malloc(sizeof(*e));
>> +		hashmap_entry_init(e, strihash(key));
>> +		strbuf_init(&(e->key), 1024);
>> +		strbuf_addstr(&(e->key),key);
>> +		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;
> When malloc() is replaced by xcalloc()  the x = NULL and y = 0 can go away,
> and the code is shorter and easier to read.

Much better, thanks.

>> +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 ;
> We could get rid of the variable  "int num_values" by simply writing
> result = values->items[values->nr-1].string;
> 

Noted.


Cheers,
Tanay Abhra.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC/PATCH 1/2] config: Add cache for config value querying
  2014-05-26 17:33 ` [RFC/PATCH 1/2] config: Add cache for config value querying Tanay Abhra
  2014-05-26 20:02   ` Torsten Bögershausen
@ 2014-05-28  9:31   ` Eric Sunshine
  1 sibling, 0 replies; 7+ messages in thread
From: Eric Sunshine @ 2014-05-28  9:31 UTC (permalink / raw
  To: Tanay Abhra
  Cc: Git List, Ramkumar Ramachandra, Matthieu Moy,
	Torsten Bögershausen

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.

> 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.

> +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.)

> +{
> +       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()?

> +       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;

> +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.

> +               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.

> +       }
> +}
> +
> +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;

> +}
> +
> +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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC/PATCH 2/2] config: Add new query functions to the api
  2014-05-26 17:33 ` [RFC/PATCH 2/2] config: Add new query functions to the api Tanay Abhra
@ 2014-05-28  9:43   ` Eric Sunshine
  0 siblings, 0 replies; 7+ messages in thread
From: Eric Sunshine @ 2014-05-28  9:43 UTC (permalink / raw
  To: Tanay Abhra; +Cc: Git List, Ramkumar Ramachandra, Matthieu Moy

On Mon, May 26, 2014 at 1:33 PM, Tanay Abhra <tanayabh@gmail.com> wrote:
> Subject: config: Add new query functions to the api

This sounds as if you are adding new functions to the header file.
Saying "... to the api docs" would been clearer. Better:

    config: document new query functions

> Add explanations for `git_config_get_string_multi` and `git_config_get_string`
> which utilize the config cache for querying in an non callback manner for a

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

> specific variable.
>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>

One might expect these functions to be documented in the same patch
which introduces them. Since this patch is so small, it might make
sense just to squash it into patch 1.

More below.

> ---
>  Documentation/technical/api-config.txt | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
> index 230b3a0..33b5b90 100644
> --- a/Documentation/technical/api-config.txt
> +++ b/Documentation/technical/api-config.txt
> @@ -77,6 +77,25 @@ To read a specific file in git-config format, use
>  `git_config_from_file`. This takes the same callback and data parameters
>  as `git_config`.
>
> +Querying for specific variables
> +-------------------------------

All other section headers in this file capitalize each word.

> +For programs wanting to query for specific variables in a non callback

s/non-/

> +manner, the config API provides two functions `git_config_get_string`
> +and `git_config_get_string_multi`. They both take a single parameter,
> +
> +- a variable as the key string for which the corresponding values will
> +  be retrieved and returned.
> +
> +They both read value from an internal cache generated previously from

Minor observation: As this file is documenting the API, mentioning
private implementation details such, as the "internal cache", may be
frowned upon (though probably doesn't matter in practice since this
_is_ a technical document).

> +reading the config files. `git_config_get_string` returns the value with
> +the highest priority(i.e. value in the repo config will be preferred over
> +value in user wide config for the same variable).
> +
> +`git_config_get_string_multi` returns a `string_list` containing all the
> +values for that particular variable, sorted in order of increasing
> +priority.
> +
>  Value Parsing Helpers
>  ---------------------
>
> --
> 1.9.0.GIT

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2014-05-28  9:43 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-26 17:33 [RFC/PATCH 0/2] Git config cache & special querying api utilizing the cache Tanay Abhra
2014-05-26 17:33 ` [RFC/PATCH 1/2] config: Add cache for config value querying Tanay Abhra
2014-05-26 20:02   ` Torsten Bögershausen
2014-05-27 13:54     ` Tanay Abhra
2014-05-28  9:31   ` Eric Sunshine
2014-05-26 17:33 ` [RFC/PATCH 2/2] config: Add new query functions to the api Tanay Abhra
2014-05-28  9:43   ` Eric Sunshine

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).