[PATCH v2 0/2] Git config cache & special querying api utilizing the cache

[PATCH v2 0/2] Git config cache & special querying api utilizing the cache

[Tanay Abhra]

Hi,

[PATCH v2]:Changed the string_list to a struct instead of pointer to a struct.
	Added string-list initilization functions.
	Minor mistakes corrected acoording to review comments[4]. Thanks to
	Eric and Matthieu for their review.

[PATCH V1]:Most of the invaluable suggestions by Eric Sunshine, Torsten Bogershausen and
	Jeff King has been implemented[1]. Complete rewrite of config_cache*() family
	using git_config() as hook as suggested by Jeff. Thanks for the review.

[RFC V2]: Improved according to the suggestions by Eric Sunshine and Torsten Bogershausen.
	Added cache invalidation when config file is changed.[2]
	I am using git_config_set_multivar_in_file() as an update hook.

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

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 the first time when any of the new query functions is
called. It is invalidated by using git_config_set_multivar_in_file() as an
update hook.

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.

[1] http://marc.info/?t=140172066200006&r=1&w=2
[2] http://git.661346.n2.nabble.com/RFC-PATCH-0-2-Git-config-cache-amp-special-querying-api-utilizing-the-cache-td7611691.html
[3] https://drive.google.com/file/d/0B4suZ-aHqDcnSUZJRXVTTnZUN1E/edit?usp=sharing
[4] http://thread.gmane.org/gmane.comp.version-control.git/251073/focus=251369

Cheers,
Tanay Abhra.


Tanay Abhra (2):
  string-list: Add string_list initializer helper functions
  config: Add hashtable for config parsing & retrieval

 Documentation/technical/api-config.txt |  17 +++++
 cache.h                                |   2 +
 config.c                               | 123 +++++++++++++++++++++++++++++++++
 string-list.c                          |  18 +++++
 string-list.h                          |   3 +
 5 files changed, 163 insertions(+)

-- 
1.9.0.GIT

[PATCH v2 1/2] string-list: Add string_list initializer helper functions

[Tanay Abhra]

When a compound construct like a string_list within another
struct is used, the default initializer macros are useless.
For such cases add helper functions for string_list
initialization for both DUP and NODUP modes.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 string-list.c | 18 ++++++++++++++++++
 string-list.h |  3 +++
 2 files changed, 21 insertions(+)

diff --git a/string-list.c b/string-list.c
index aabb25e..8c3a4eb 100644
--- a/string-list.c
+++ b/string-list.c
@@ -1,6 +1,24 @@
 #include "cache.h"
 #include "string-list.h"
 
+void string_list_init_nodup(struct string_list *list)
+{
+	list->items = NULL;
+	list->nr = 0;
+	list->alloc = 0;
+	list->strdup_strings = 0;
+	list->cmp = NULL;
+}
+
+void string_list_init_dup(struct string_list *list)
+{
+	list->items = NULL;
+	list->nr = 0;
+	list->alloc = 0;
+	list->strdup_strings = 1;
+	list->cmp = NULL;
+}
+
 /* if there is no exact match, point to the index where the entry could be
  * inserted */
 static int get_entry_index(const struct string_list *list, const char *string,
diff --git a/string-list.h b/string-list.h
index de6769c..8ae3376 100644
--- a/string-list.h
+++ b/string-list.h
@@ -18,6 +18,9 @@ struct string_list {
 #define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0 }
 #define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1 }
 
+void string_list_init_nodup(struct string_list *list);
+void string_list_init_dup(struct string_list *list);
+
 void print_string_list(const struct string_list *p, const char *text);
 void string_list_clear(struct string_list *list, int free_util);
 
-- 
1.9.0.GIT

[PATCH v2 2/2] config: Add hashtable for config parsing & retrieval

[Tanay Abhra]

Add a hash table to cache all key-value pairs read from config files
(repo specific .git/config, user wide ~/.gitconfig and the global
/etc/gitconfig). Add two external functions `git_config_get_string` and
`git_config_get_string_multi` for querying in a non-callback manner from the
hash table.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 Documentation/technical/api-config.txt |  17 +++++
 cache.h                                |   2 +
 config.c                               | 123 +++++++++++++++++++++++++++++++++
 3 files changed, 142 insertions(+)

diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index 230b3a0..23e8ae3 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -77,6 +77,23 @@ 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 key string in canonical flat form for which the corresponding values
+will be retrieved and returned.
+
+They both read values 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 key, sorted in order of increasing priority.
+
 Value Parsing Helpers
 ---------------------
 
diff --git a/cache.h b/cache.h
index 1e4b4f0..7343692 100644
--- a/cache.h
+++ b/cache.h
@@ -1295,6 +1295,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__)
 #define config_error_nonbool(s) (config_error_nonbool(s), const_error())
 #endif
diff --git a/config.c b/config.c
index 3c3e314..2fd3656 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,120 @@ static struct config_source *cf;
 
 static int zlib_compression_seen;
 
+struct config_cache_entry {
+	struct hashmap_entry ent;
+	char *key;
+	struct string_list value_list;
+};
+
+static int hashmap_initialized;
+
+static int config_cache_set_value(const char *key, const char *value);
+
+static int config_cache_entry_cmp(const struct config_cache_entry *e1,
+				 const struct config_cache_entry *e2, const void *unused)
+{
+	return strcmp(e1->key, e2->key);
+}
+
+static void config_cache_init(struct hashmap *config_cache)
+{
+	hashmap_init(config_cache, (hashmap_cmp_fn)config_cache_entry_cmp, 0);
+}
+
+static int config_cache_callback(const char *key, const char *value, void *unused)
+{
+	config_cache_set_value(key, value);
+	return 0;
+}
+
+static struct hashmap *get_config_cache(void)
+{
+	static struct hashmap config_cache;
+	if (!hashmap_initialized) {
+		config_cache_init(&config_cache);
+		hashmap_initialized = 1;
+		git_config(config_cache_callback, NULL);
+	}
+	return &config_cache;
+}
+
+static void config_cache_free(void)
+{
+	struct hashmap *config_cache;
+	struct config_cache_entry *entry;
+	struct hashmap_iter iter;
+	config_cache = get_config_cache();
+	hashmap_iter_init(config_cache, &iter);
+	while ((entry = hashmap_iter_next(&iter))) {
+		free(entry->key);
+		string_list_clear(&entry->value_list, 0);
+	}
+	hashmap_free(config_cache, 1);
+	hashmap_initialized = 0;
+}
+
+static struct config_cache_entry *config_cache_find_entry(const char *key)
+{
+	struct hashmap *config_cache;
+	struct config_cache_entry k;
+	struct config_cache_entry *found_entry;
+	char *normalized_key;
+	int ret;
+	config_cache = get_config_cache();
+	ret = git_config_parse_key(key, &normalized_key, NULL);
+
+	if (ret)
+		return NULL;
+
+	hashmap_entry_init(&k, strhash(normalized_key));
+	k.key = (char *)normalized_key;
+	found_entry = hashmap_get(config_cache, &k, NULL);
+	free(normalized_key);
+	return found_entry;
+}
+
+static struct string_list *config_cache_get_value(const char *key)
+{
+	struct config_cache_entry *e = config_cache_find_entry(key);
+	return e ? &e->value_list : NULL;
+}
+
+static int config_cache_set_value(const char *key, const char *value)
+{
+	struct hashmap *config_cache;
+	struct config_cache_entry *e;
+
+	config_cache = get_config_cache();
+	e = config_cache_find_entry(key);
+	if (!e) {
+		e = xmalloc(sizeof(*e));
+		hashmap_entry_init(e, strhash(key));
+		e->key = xstrdup(key);
+		string_list_init_dup(&e->value_list);
+		string_list_append(&e->value_list, value);
+		hashmap_add(config_cache, e);
+	} else {
+		string_list_append(&e->value_list, value);
+	}
+	return 0;
+}
+
+const char *git_config_get_string(const char *key)
+{
+	struct string_list *values;
+	values = config_cache_get_value(key);
+	if (!values)
+		return NULL;
+	assert(values->nr > 0);
+	return values->items[values->nr - 1].string;
+}
+
+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);
@@ -1714,6 +1830,13 @@ int git_config_set_multivar_in_file(const char *config_filename,
 	lock = NULL;
 	ret = 0;
 
+	/*
+	 *contents of config file has changed, so invalidate the
+	 *config cache used by non-callback based query functions.
+	 */
+	if (hashmap_initialized)
+		config_cache_free();
+
 out_free:
 	if (lock)
 		rollback_lock_file(lock);
-- 
1.9.0.GIT

Re: [PATCH v2 2/2] config: Add hashtable for config parsing & retrieval

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> Add a hash table to cache all key-value pairs read from config files
> (repo specific .git/config, user wide ~/.gitconfig and the global
> /etc/gitconfig). Add two external functions `git_config_get_string` and
> `git_config_get_string_multi` for querying in a non-callback manner from the
> hash table.

This describes rather well _what_ your patch does, but the most
important part of a commit message is to justify _why_ the change is
good, and why the way you implemented it is good.

Think of it as an way to convince reviewers to accept your patch.

> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
>  Documentation/technical/api-config.txt |  17 +++++
>  cache.h                                |   2 +
>  config.c                               | 123 +++++++++++++++++++++++++++++++++
>  3 files changed, 142 insertions(+)

I'm still concerned about the fact that there is no test. At this point,
git_config_get_string_multi and git_config_get_string are basically dead
code.

I'm sure you did experiments to test these functions, run them through
valgrind, ... Now, you need to let other people reproduce these
experiments. "other people" can be reviewers, now, or anyone looking for
bugs or regressions in the future.

> +static int config_cache_callback(const char *key, const char *value, void *unused)
> +{
> +	config_cache_set_value(key, value);
> +	return 0;
> +}
> +
> +static struct hashmap *get_config_cache(void)
> +{
> +	static struct hashmap config_cache;
> +	if (!hashmap_initialized) {
> +		config_cache_init(&config_cache);
> +		hashmap_initialized = 1;
> +		git_config(config_cache_callback, NULL);
> +	}
> +	return &config_cache;
> +}

Good, just enough code for the job :-).

> +static struct config_cache_entry *config_cache_find_entry(const char *key)
> +{
> +	struct hashmap *config_cache;
> +	struct config_cache_entry k;
> +	struct config_cache_entry *found_entry;
> +	char *normalized_key;
> +	int ret;
> +	config_cache = get_config_cache();
> +	ret = git_config_parse_key(key, &normalized_key, NULL);
> +
> +	if (ret)
> +		return NULL;
> +
> +	hashmap_entry_init(&k, strhash(normalized_key));
> +	k.key = (char *)normalized_key;

No need to cast.

> +static int config_cache_set_value(const char *key, const char *value)
> +{
> +	struct hashmap *config_cache;
> +	struct config_cache_entry *e;
> +
> +	config_cache = get_config_cache();
> +	e = config_cache_find_entry(key);
> +	if (!e) {
> +		e = xmalloc(sizeof(*e));
> +		hashmap_entry_init(e, strhash(key));
> +		e->key = xstrdup(key);
> +		string_list_init_dup(&e->value_list);
> +		string_list_append(&e->value_list, value);
> +		hashmap_add(config_cache, e);
> +	} else {
> +		string_list_append(&e->value_list, value);
> +	}
> +	return 0;
> +}

I find the function name a bit confusing, as it does not "set" in the
sense "override any previous value". Wouldn't this be better named
config_cache_add_value? Or perhaps a comment would help.

> @@ -1714,6 +1830,13 @@ int git_config_set_multivar_in_file(const char *config_filename,
>  	lock = NULL;
>  	ret = 0;
>  
> +	/*
> +	 *contents of config file has changed, so invalidate the
> +	 *config cache used by non-callback based query functions.
> +	 */

Spaces after stars:

/*
 * Contents of config file has changed, so invalidate the
 * config cache used by non-callback based query functions.
 */

(I think the "s" of "contents" should be dropped too).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 2/2] config: Add hashtable for config parsing & retrieval

[Tanay Abhra]

On 06/16/2014 10:11 AM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> Add a hash table to cache all key-value pairs read from config files
>> (repo specific .git/config, user wide ~/.gitconfig and the global
>> /etc/gitconfig). Add two external functions `git_config_get_string` and
>> `git_config_get_string_multi` for querying in a non-callback manner from the
>> hash table.
> 
> This describes rather well _what_ your patch does, but the most
> important part of a commit message is to justify _why_ the change is
> good, and why the way you implemented it is good.
> 
> Think of it as an way to convince reviewers to accept your patch.

Okay, but isn't the content of the cover letter is doing that for now.
Should I put some part of it in here?

>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>> ---
>>  Documentation/technical/api-config.txt |  17 +++++
>>  cache.h                                |   2 +
>>  config.c                               | 123 +++++++++++++++++++++++++++++++++
>>  3 files changed, 142 insertions(+)
> 
> I'm still concerned about the fact that there is no test. At this point,
> git_config_get_string_multi and git_config_get_string are basically dead
> code.
> 
> I'm sure you did experiments to test these functions, run them through
> valgrind, ... Now, you need to let other people reproduce these
> experiments. "other people" can be reviewers, now, or anyone looking for
> bugs or regressions in the future.
>

Yeah, I have run the experiments. I will add a test file for it. I should have
appended it to this series only, my fault. :) A stray observation, Git has very less
unit tests, compared to the comprehensive test directory for commands.

>> +static struct config_cache_entry *config_cache_find_entry(const char *key)
>> +{
>> +	struct hashmap *config_cache;
>> +	struct config_cache_entry k;
>> +	struct config_cache_entry *found_entry;
>> +	char *normalized_key;
>> +	int ret;
>> +	config_cache = get_config_cache();
>> +	ret = git_config_parse_key(key, &normalized_key, NULL);
>> +
>> +	if (ret)
>> +		return NULL;
>> +
>> +	hashmap_entry_init(&k, strhash(normalized_key));
>> +	k.key = (char *)normalized_key;
> 
> No need to cast.
> 

Noted.

>> +static int config_cache_set_value(const char *key, const char *value)
>> +{
>> +	struct hashmap *config_cache;
>> +	struct config_cache_entry *e;
>> +
>> +	config_cache = get_config_cache();
>> +	e = config_cache_find_entry(key);
>> +	if (!e) {
>> +		e = xmalloc(sizeof(*e));
>> +		hashmap_entry_init(e, strhash(key));
>> +		e->key = xstrdup(key);
>> +		string_list_init_dup(&e->value_list);
>> +		string_list_append(&e->value_list, value);
>> +		hashmap_add(config_cache, e);
>> +	} else {
>> +		string_list_append(&e->value_list, value);
>> +	}
>> +	return 0;
>> +}
> 
> I find the function name a bit confusing, as it does not "set" in the
> sense "override any previous value". Wouldn't this be better named
> config_cache_add_value? Or perhaps a comment would help.
>

Noted.

>> @@ -1714,6 +1830,13 @@ int git_config_set_multivar_in_file(const char *config_filename,
>>  	lock = NULL;
>>  	ret = 0;
>>  
>> +	/*
>> +	 *contents of config file has changed, so invalidate the
>> +	 *config cache used by non-callback based query functions.
>> +	 */
> 
> Spaces after stars:
> 
> /*
>  * Contents of config file has changed, so invalidate the
>  * config cache used by non-callback based query functions.
>  */
> 
> (I think the "s" of "contents" should be dropped too).
> 

Noted. Thanks for the review.

Re: [PATCH v2 2/2] config: Add hashtable for config parsing & retrieval

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 06/16/2014 10:11 AM, Matthieu Moy wrote:
>> Tanay Abhra <tanayabh@gmail.com> writes:
>> 
>>> Add a hash table to cache all key-value pairs read from config files
>>> (repo specific .git/config, user wide ~/.gitconfig and the global
>>> /etc/gitconfig). Add two external functions `git_config_get_string` and
>>> `git_config_get_string_multi` for querying in a non-callback manner from the
>>> hash table.
>> 
>> This describes rather well _what_ your patch does, but the most
>> important part of a commit message is to justify _why_ the change is
>> good, and why the way you implemented it is good.
>> 
>> Think of it as an way to convince reviewers to accept your patch.
>
> Okay, but isn't the content of the cover letter is doing that for now.

The cover letter won't be part of the Git history, while the commit
messages are.

Imagine someone finding your functions in the code and wondering "wtf
is this code doing here?". "git blame" will point this person to your
commit message, but digging the mail archives is one big extra step
(that essentially no one will make).

> Yeah, I have run the experiments. I will add a test file for it. I should have
> appended it to this series only, my fault. :) A stray observation, Git has very less
> unit tests, compared to the comprehensive test directory for commands.

Yes. But in most cases, code written in a commit is directly reachable
from the command-line UI, and can be tested this way.

(I do believe that Git would benefit from more unit-testing, but that's
another topic).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 1/2] string-list: Add string_list initializer helper functions

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> When a compound construct like a string_list within another
> struct is used, the default initializer macros are useless.
> For such cases add helper functions for string_list
> initialization for both DUP and NODUP modes.
>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---


Sorry, but I do not understand the above "useless".  Do you mean to
say that xyzzy below cannot be initialized that way?

 git.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/git.c b/git.c
index d261575..17714d1 100644
--- a/git.c
+++ b/git.c
@@ -595,11 +595,24 @@ static int run_argv(int *argcp, const char ***argv)
 }
 
 
+#include "string-list.h"
+
 int main(int argc, char **av)
 {
 	const char **argv = (const char **) av;
 	const char *cmd;
 
+	struct compound {
+		int frotz;
+		struct string_list nitfol;
+	} xyzzy = {
+		314,
+		STRING_LIST_INIT_DUP,
+	};
+	printf("dup-strings is set to %s\n",
+	       xyzzy.nitfol.strdup_strings ? "true" : "false");
+	return 0;
+
 	startup_info = &git_startup_info;
 
 	cmd = git_extract_argv0_path(argv[0]);

Re: [PATCH v2 2/2] config: Add hashtable for config parsing & retrieval

[Jeff King]

On Mon, Jun 16, 2014 at 01:27:12AM -0700, Tanay Abhra wrote:

> +static int config_cache_callback(const char *key, const char *value, void *unused)
> +{
> +	config_cache_set_value(key, value);
> +	return 0;
> +}

This gets the normalized key, which is good; you should be able to just
hash/lookup values without further munging as long as they, too, are
normalized (you may want to note that in the documentaiton for
config_cache_find_entry and friends).

The value may be a string value, or NULL if it is a true boolean. That
value gets passed to...

> +static int config_cache_set_value(const char *key, const char *value)
> +{
> +	struct hashmap *config_cache;
> +	struct config_cache_entry *e;
> +
> +	config_cache = get_config_cache();
> +	e = config_cache_find_entry(key);
> +	if (!e) {
> +		e = xmalloc(sizeof(*e));
> +		hashmap_entry_init(e, strhash(key));
> +		e->key = xstrdup(key);
> +		string_list_init_dup(&e->value_list);
> +		string_list_append(&e->value_list, value);
> +		hashmap_add(config_cache, e);
> +	} else {
> +		string_list_append(&e->value_list, value);
> +	}
> +	return 0;
> +}

...this function, which then passes it to string_list_append. Which is
going to call xstrdup() on it, which will then segfault.

You need some mechanism to store entries that are NULL. It may be enough
to silently convert them into the string "true" inside the cached
storage. But there may be callers who treat NULL specially (e.g., a
tri-state true/false/auto that treats a bare boolean as "auto"); you'd
need to check.

The other alternative is to use something besides string_list that can
handle a NULL.  You may also need to give some thought to how such NULLs
would be handled by git_config_get_string() (since a NULL there also
means "not found").

-Peff

Re: [PATCH v2 2/2] config: Add hashtable for config parsing & retrieval

[Jeff King]

On Tue, Jun 17, 2014 at 01:34:36AM -0400, Jeff King wrote:

> You need some mechanism to store entries that are NULL. It may be enough
> to silently convert them into the string "true" inside the cached
> storage. But there may be callers who treat NULL specially (e.g., a
> tri-state true/false/auto that treats a bare boolean as "auto"); you'd
> need to check.
> 
> The other alternative is to use something besides string_list that can
> handle a NULL.  You may also need to give some thought to how such NULLs
> would be handled by git_config_get_string() (since a NULL there also
> means "not found").

After reading your other patch and thinking about config_error_nonbool,
I don't think it would be right to silently convert this to "true".

E.g., imagine config like:

  [alias]
  foo

If we interpret alias.foo as a boolean, it means "true". But since
alias.* variables are not booleans, the current alias code will produce
an error (with config_error_nonbool). Silently converting it to true in
the config layer would mean the alias code sees only "true", and rather
than generating an error, it would run the command "true".

-Peff

Re: [PATCH v2 1/2] string-list: Add string_list initializer helper functions

[Tanay Abhra]

On 06/16/2014 03:59 PM, Junio C Hamano wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> When a compound construct like a string_list within another
>> struct is used, the default initializer macros are useless.
>> For such cases add helper functions for string_list
>> initialization for both DUP and NODUP modes.
>>
>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>> ---
> 
> 
> Sorry, but I do not understand the above "useless".  Do you mean to
> say that xyzzy below cannot be initialized that way?
> 
>  git.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/git.c b/git.c
> index d261575..17714d1 100644
> --- a/git.c
> +++ b/git.c
> @@ -595,11 +595,24 @@ static int run_argv(int *argcp, const char ***argv)
>  }
>  
>  
> +#include "string-list.h"
> +
>  int main(int argc, char **av)
>  {
>  	const char **argv = (const char **) av;
>  	const char *cmd;
>  
> +	struct compound {
> +		int frotz;
> +		struct string_list nitfol;
> +	} xyzzy = {
> +		314,
> +		STRING_LIST_INIT_DUP,
> +	};
> +	printf("dup-strings is set to %s\n",
> +	       xyzzy.nitfol.strdup_strings ? "true" : "false");
> +	return 0;
> +
>  	startup_info = &git_startup_info;
>  
>  	cmd = git_extract_argv0_path(argv[0]);
> 

I was actually explaining for cases like below,

+struct config_cache_entry {
+	struct hashmap_entry ent;
+	char *key;
+	struct string_list value_list;
+};

+static int config_cache_set_value(const char *key, const char *value)
+{
+	struct hashmap *config_cache;
+	struct config_cache_entry *e;
+
+	config_cache = get_config_cache();
+	e = config_cache_find_entry(key);
+	if (!e) {
+		e = xmalloc(sizeof(*e));
+		hashmap_entry_init(e, strhash(key));
+		e->key = xstrdup(key);
+		string_list_init_dup(&e->value_list);
+		string_list_append(&e->value_list, value);
+		hashmap_add(config_cache, e);
+	} else {
+		string_list_append(&e->value_list, value);
+	}
+	return 0;
+}

Here even if we use an initialization list as you have shown above, I would have to
check contents of 'struct hashmap_entry', thus totally breaking the encapsulation
that the string_list macro was providing. There may not be default values for
'struct hashmap_entry' as it may be using internal init function.

Also, I have to dynamically allocate the config_cache_entry struct, thus the
initialization list such as above cannot be used. Two previous reviewers of the
patch suggested I put a preparatory patch with string_list_init functions because
the default macros will be useless in my case.

Is there another way out?

Cheers,
Tanay abhra.

Re: [PATCH v2 1/2] string-list: Add string_list initializer helper functions

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 06/16/2014 03:59 PM, Junio C Hamano wrote:
>> Tanay Abhra <tanayabh@gmail.com> writes:
>> 
>>> When a compound construct like a string_list within another
>>> struct is used, the default initializer macros are useless.
>>> For such cases add helper functions for string_list
>>> initialization for both DUP and NODUP modes.
>>>
>>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>>> ---
>> 
>> 
>> Sorry, but I do not understand the above "useless".  Do you mean to
>> say that xyzzy below cannot be initialized that way?
>>  ...
> I was actually explaining for cases like below,
> ...
> +		string_list_init_dup(&e->value_list);

If that is what you wanted to refer to, I would have to say
"useless" is placing a stress on a wrong place.  (I do not see
anything wrong with your new code; it was just the way it was
explained in the proposed log message was misleading).

Structure initialisers are not something you can assign to a
variable anyway, and calling them "useless" is like complaining how
unwieldty hammers are on screws.  "Hammers are useless on screws"
may not be technically wrong per-se, but the readers won't be helped
by hearing it very much.

Instead, you would want to explain what your new invention, a
screwdriver, is and how it is intended to be used.

We of course have precedences for this kind of thing.  STRBUF_INIT
is for definition-time initialisation and strbuf_init() is to
initialise an uninitialised piece of memory to be used as a strbuf.

I tend to think it was a long-time misdesign of string-list API to
have the STRING_LIST_INIT* definition-time initialisers without
having runtime string_list_init*() initialisers, and it is a good
idea to add them to complete the API.

If I were writing the log message for this, I would just say:

	The string-list API has STRING_LIST_INIT_* macros to be used
	to define variables with initialisers, but lacks functions
	to initialise an uninitialised piece of memory to be used as
	a string-list at the run-time.

        Introduce string_list_init_{dup,nodup}() functions for that.

or something.