On 23/06/14 11:11, Tanay Abhra wrote:
[snip]
quoted hunk
diff --git a/config.c b/config.c
index a1aef1c..6200f36 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,141 @@ 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_add_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_add_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;
+}
[I have not been following this series at all (sorry I haven't had
the time to spare), so take these comments with a very big pinch of
salt! ie just ignore me if it's already been discussed etc. ;-) ]
The 'git config' command can be used to read arbitrary files (so long
as they conform to the config syntax). For example, see the --file and
--blob options to git-config. At present, I think only scripted commands
use this facility (eg git-submodule). Noting the singleton config_cache,
what happens when git-submodule becomes a C builtin, or indeed any other
C builtin wants to take advantage of the new code when processing a non-
standard config file?
+
+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, 1);
+ }
+ 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 = 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_add_value(const char *key, const char *value)
+{
+ struct hashmap *config_cache;
+ struct config_cache_entry *e;
+ struct string_list_item *item;
+ int *boolean_null_flag;
+
+ config_cache = get_config_cache();
+ e = config_cache_find_entry(key);
+
+ boolean_null_flag = xcalloc(1, sizeof(*boolean_null_flag));
+
+ if (!e) {
+ e = xmalloc(sizeof(*e));
+ hashmap_entry_init(e, strhash(key));
+ e->key = xstrdup(key);
config_cache_find_entry() searches for (and hashes the) normalized_key.
Should you not be entering the normalized key here?
+ string_list_init_dup(&e->value_list);
+ hashmap_add(config_cache, e);
+ }
+ /*
+ * If the variable had no value specified, the value will be NULL
+ * (typically this means it should be interpreted as boolean true).
+ * For such values, silently convert them to "BOOLEAN_NULL" to
+ * store in hashmap and flag that string_list_item as 1.
+ */
+ if (value == NULL) {
+ value = "BOOLEAN_NULL";
+ *boolean_null_flag = 1;
+ }
+ item = string_list_append(&e->value_list, value);
+ item->util = boolean_null_flag;
+
+ return 0;
+}
+
[snip]
ATB,
Ramsay Jones
On 06/23/2014 07:57 AM, Ramsay Jones wrote:
On 23/06/14 11:11, Tanay Abhra wrote:
quoted
diff --git a/config.c b/config.c
index a1aef1c..6200f36 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,141 @@ 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_add_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_add_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;
+}
[I have not been following this series at all (sorry I haven't had
the time to spare), so take these comments with a very big pinch of
salt! ie just ignore me if it's already been discussed etc. ;-) ]
The 'git config' command can be used to read arbitrary files (so long
as they conform to the config syntax). For example, see the --file and
--blob options to git-config. At present, I think only scripted commands
use this facility (eg git-submodule). Noting the singleton config_cache,
what happens when git-submodule becomes a C builtin, or indeed any other
C builtin wants to take advantage of the new code when processing a non-
standard config file?
This series was mainly to replace git_config() invocations around the codebase.
There are currently 111 git_config() invocations, each of which causes a file
reread whenever called. git_config() only feeds values from the standard config
files(i.e repo, user and global config).
For reading config values from specific files or blobs, there are three functions
git_config_with_options, git_config_from_file & git_config_from_blob which can be
easily used inside a C builtin or anywhere in the code.
The bulk of git_config_api calls are only for git_config(). For example,
git_config_from_file() has three hits only in entire codebase,
git_config_with_options() has 5 hits, so I concentrated on generating a cache
for the usual config files only. For other files, the callers can fall back on older
API functions like I had mentioned above.
Forgive me if I inferred your question incorrectly. More below.
quoted
+
+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, 1);
+ }
+ 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 = 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_add_value(const char *key, const char *value)
+{
+ struct hashmap *config_cache;
+ struct config_cache_entry *e;
+ struct string_list_item *item;
+ int *boolean_null_flag;
+
+ config_cache = get_config_cache();
+ e = config_cache_find_entry(key);
+
+ boolean_null_flag = xcalloc(1, sizeof(*boolean_null_flag));
+
+ if (!e) {
+ e = xmalloc(sizeof(*e));
+ hashmap_entry_init(e, strhash(key));
+ e->key = xstrdup(key);
config_cache_find_entry() searches for (and hashes the) normalized_key.
Should you not be entering the normalized key here?
config_cache_add_value() is fed key-values pairs through the git_config()
callback mechanism, which normalises the key beforehand, so no need for
renormalising.
Thanks for the review. :)
Cheers,
Tanay Abhra.
On 23/06/14 17:20, Tanay Abhra wrote:
On 06/23/2014 07:57 AM, Ramsay Jones wrote:
quoted
On 23/06/14 11:11, Tanay Abhra wrote:
[snip]
quoted
quoted
+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;
+}
[I have not been following this series at all (sorry I haven't had
the time to spare), so take these comments with a very big pinch of
salt! ie just ignore me if it's already been discussed etc. ;-) ]
The 'git config' command can be used to read arbitrary files (so long
as they conform to the config syntax). For example, see the --file and
--blob options to git-config. At present, I think only scripted commands
use this facility (eg git-submodule). Noting the singleton config_cache,
what happens when git-submodule becomes a C builtin, or indeed any other
C builtin wants to take advantage of the new code when processing a non-
standard config file?
This series was mainly to replace git_config() invocations around the codebase.
There are currently 111 git_config() invocations, each of which causes a file
reread whenever called. git_config() only feeds values from the standard config
files(i.e repo, user and global config).
For reading config values from specific files or blobs, there are three functions
git_config_with_options, git_config_from_file & git_config_from_blob which can be
easily used inside a C builtin or anywhere in the code.
The bulk of git_config_api calls are only for git_config(). For example,
git_config_from_file() has three hits only in entire codebase,
git_config_with_options() has 5 hits, so I concentrated on generating a cache
for the usual config files only. For other files, the callers can fall back on older
API functions like I had mentioned above.
Forgive me if I inferred your question incorrectly. More below.
Hmm, maybe. The "... take advantage of the new code" refers to the
possibility (or otherwise) of re-using your work to update these
"older API" functions to the new API style. (also, see Junio's response).
[In order to do this, I would have expected to see one hash table
for each file/blob, so the singleton object took me by surprise.]
An "out of scope for this project" is a perfectly acceptable
response (*particularly* since it is very late in the day to be
bringing this up!).
quoted
quoted
+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 = 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_add_value(const char *key, const char *value)
+{
+ struct hashmap *config_cache;
+ struct config_cache_entry *e;
+ struct string_list_item *item;
+ int *boolean_null_flag;
+
+ config_cache = get_config_cache();
+ e = config_cache_find_entry(key);
+
+ boolean_null_flag = xcalloc(1, sizeof(*boolean_null_flag));
+
+ if (!e) {
+ e = xmalloc(sizeof(*e));
+ hashmap_entry_init(e, strhash(key));
+ e->key = xstrdup(key);
config_cache_find_entry() searches for (and hashes the) normalized_key.
Should you not be entering the normalized key here?
config_cache_add_value() is fed key-values pairs through the git_config()
callback mechanism, which normalises the key beforehand, so no need for
renormalising.
Ah, yes, I forgot that the parsing code does a tolower() at various
places while accumulating the key string. So the (potentially) non-
normalized keys come from the user via the new API functions and,
rather than putting code to normalize the key in each of those,
just do it once in config_cache_find_entry(). (Although, you could
possibly do that in config_cache_get_value()). OK.
Hmm, maybe add a short comment to that effect? dunno.
ATB,
Ramsay Jones
On Mon, Jun 23, 2014 at 6:11 AM, Tanay Abhra [off-list ref] wrote:
Add different usage examples for 'git_config_get_string' and
`git_config_get_string_multi`. They will serve as documentation
on how to query for config values in a non-callback manner.
This is a good start, but it's not fully what Matthieu was suggesting
when he said that you should prove to other developers, by way of
reproducible tests, that your changes work. What he meant, was that
you should write a test-config program which exposes (as a runnable
command) the new config C API you've added, and then write tests which
exercise that API and implementation exhaustively.
For example, take a look at test-string-list.c and
t/t0063-string-list.sh. The C program does no checking itself. It
merely exposes the C API via command-line arguments, such as "split",
"filter", etc. The test script then employs that program to perform
the actual testing in a reproducible and (hopefully) exhaustive
fashion. Because t/t0063-string-list.sh is part of the test suite, the
string-list tests are run regularly by many developers. It's not just
something that someone might remember to run once in a while.
Contrary to your commit message and the comment in the program itself,
the purpose of test-config is not to serve as documentation or to
provide examples of usage. (Real documentation is better suited for
those purposes.) Instead, test-config should exist in support of a
real test script in t/ which is run regularly. The new script you add
to t/ should exercise the exposed C API as exhaustively as possible.
This means checking each possible state: for instance, (1) when a key
is absent, (2) when a value is boolean (NULL), (3) one non-boolean
(non-NULL) value, (4) multiple values, etc. Moreover, it should check
expected success _and_ expected failure modes. Check not only that it
returns expected values, but that it fails when appropriate.
More below.
quoted hunk
Signed-off-by: Tanay Abhra <redacted>
---
.gitignore | 1 +
Makefile | 1 +
test-config.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 95 insertions(+)
create mode 100644 test-config.c
diff --git a/.gitignore b/.gitignore
index 42294e5..7677533 100644
--- a/.gitignore
+++ b/.gitignore
@@ -177,6 +177,7 @@
/gitweb/static/gitweb.min.*
/test-chmtime
/test-ctype
+/test-config
/test-date
/test-delta
/test-dump-cache-tree
diff --git a/Makefile b/Makefile
index 07ea105..9544efb 100644
--- a/Makefile
+++ b/Makefile
@@ -549,6 +549,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
TEST_PROGRAMS_NEED_X += test-chmtime
TEST_PROGRAMS_NEED_X += test-ctype
+TEST_PROGRAMS_NEED_X += test-config
TEST_PROGRAMS_NEED_X += test-date
TEST_PROGRAMS_NEED_X += test-delta
TEST_PROGRAMS_NEED_X += test-dump-cache-tree
diff --git a/test-config.c b/test-config.c
new file mode 100644
index 0000000..ff24cb8
--- /dev/null
+++ b/test-config.c
@@ -0,0 +1,93 @@
+#include "cache.h"
+#include "hashmap.h"
+#include "string-list.h"
+
+/*
+ * This program gives examples on how to use non-callback based query
+ * functions like git_config_get_string & git_config_get_string_multi.
See above regarding the true purpose of test-config. A more accurate
description might be something like this:
This program exposes the C API of the configuration mechanism
as a set of simple commands in order to facilitate testing.
+ *
+ * Reads stdin and prints result of command to stdout:
+ *
+ * print_all -> prints all the key-value pairs contained in the hashmap
+ * also checks if all entries in the hashmap matches with
+ * the content of config files
Some comments:
This doesn't just print all values. It's doing validation on its own,
so the name 'print_all' is misleading.
This isn't part of the C API which you are exposing, so such a command
is somewhat out of place here. A script in t/ could perform similar
functionality via the exposed API.
Having said that, it probably doesn't hurt to do such internal
integrity checking, however, a better command name is warranted.
Perhaps 'check_integrity'. Moreover, a command which checks internal
integrity need not verbosely print all items. More useful, upon
success, would be to emit nothing at all or a simple "ok" or such. On
failure, emit enough information about detected problems to allow them
to be tracked down and fixed.
+ *
+ * get_value -> prints the value with highest priority for the entered key
+ *
+ * get_all_values -> prints all values for the given key in increasing order
+ * of priority
Since you're exposing the C API, it might make sense to name these
after the C functions. For instance, "get_string" and
"get_string_multi". (This can become important as the API is
expanded.)
+ * Examples:
+ *
+ * To print the value with highest priority for key "foo.bAr Baz.rock":
+ * test-config get_value "foo.bAr Baz.rock"
+ *
+ */
+
+static const char *v;
+static const struct string_list *strptr;
+static int i;
+static int *flag;
These variables are not shared between different functions in this
file, so they don't need to be global. Instead, you should declare
them as local variables where needed.
+static int config_cache_callback(const char *key, const char *value, void *unused)
This name doesn't convey much. Since this is actually implementing
functionality of the 'show_all' command, perhaps show_all_cb() would
be better. Even better, given the comments above,
check_integrity_cb().
+{
+ strptr = git_config_get_string_multi(key);
+ if (strptr) {
+ for (i = 0; i < strptr->nr; i++)
+ {
Style: { on same line as 'for'
+ v = strptr->items[i].string;
+ flag = strptr->items[i].util;
+ /* NULL values are flagged as 1 */
+ if (*flag == 1)
+ printf("%s\n", key);
+ else if (*flag == 0)
+ printf("%s=%s\n", key, v);
+ /* key-value pair printed so flag them as done */
+ *flag = -1;
Curious: It's not clear what the purpose of this -1 is. Is the
callback being invoked multiple times with the same key/value pair?
+ }
+ return 0;
+ } else {
+ printf("%s\n", "Config hashmap inconsistent\n");
Spelling out the actual inconsistency, rather than making a only
general statement, could help a programmer diagnose the problem.
+ return -1;
+ }
+}
+
+ int main(int argc, char **argv)
+{
+ if (argc == 2 && !strcmp(argv[1], "print_all")) {
+ git_config(config_cache_callback, NULL);
+ return 0;
+ } else if (argc == 3 && !strcmp(argv[1], "get_value")) {
+ /* enter key in canonical form enclosed in quotes */
What does this comment mean? Is it telling us what the code is doing
or is it an instruction to the person typing at the shell? The bit
about "canonical form" is something which you can document in the
comment block at the top of this file.
+ if (!git_config_get_string(argv[2], &v)) {
+ printf("%s\n", v);
What happens if this is a boolean (NULL) value? Behavior of '%s' on a
NULL value is undefined. Better would be to emit some special,
recognizable token when NULL.
+ return 0;
+ } else {
+ printf("%s\n", "Value not found for the entered key\n");
Although the key was specified on the command-line, this diagnostic
can be more helpful by mentioning which key was not found.
+ return -1;
+ }
+ } else if (argc == 3 && !strcmp(argv[1], "get_all_values")) {
+ /* enter key in canonical form enclosed in quotes */
Ditto regarding comment.
+ strptr = git_config_get_string_multi(argv[2]);
+ if (strptr) {
+ for (i = 0; i < strptr->nr; i++)
+ {
Style: { on same line as 'for'
+ v = strptr->items[i].string;
+ flag = strptr->items[i].util;
+ /* prints NULL values as "-" */
Rather than "-", it might make more sense to choose a more unique and
identifiable representation which can be easily and reliably
recognized by the test script.
+ if (*flag)
+ printf("%s ", "-");
+ else
+ printf("%s ", v);
+ }
+ printf("\n");
Printing all items of a multi-part config value on a single line
delimited by spaces may not be the best output format. The values
themselves may contain whitespace. Taking into consideration that this
output should be easily consumable by a test script, it probably makes
more sees to print each value on its own line.
+ return 0;
+ } else {
+ printf("%s\n", "Value not found for the entered key\n");
To be helpful, say which key was not found.
+ return -1;
+ }
+ }
+
+ fprintf(stderr, "%s: unknown function name: %s\n", argv[0],
+ argv[1] ? argv[1] : "(there was none)");
This is a bit of a lie. This diagnostic will be triggered, not only
for an unrecognized function, but also if the number of arguments to a
function was incorrect.
+ return 1;
+}
--
1.9.0.GIT
Am 23.06.2014 12:11, schrieb Tanay Abhra:
[...]
+
+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);
[I didn't follow all previous discussion, so feel free to ignore me if this has been discussed already]
Is it really necessary to normalize keys on each lookup? The current git config code simply does a 'strcmp("lower-case-key", var)' so normalization shouldn't be necessary for the majority of callers. If a caller uses a non-constant key (e.g. as passed to 'git config --get <name>'), it would be the caller's responsibility to normalize.
[...]
+int git_config_get_string(const char *key, const char **value)
I would have expected '..._get_string' to return a string. If the return value indicates whether something was found, it should probably be called '..._find_...'?
In the end, you want typed config functions that do type conversion and handle parse errors, at least for the common types bool/int/string... Thus the generic function that returns unparsed data should probably be called '..._value', not '..._string'?
A typical pull-style config API will also let you specify default values, e.g.:
const char *git_config_get_string(const char *key, const char *default_value)
{
const char *value;
if (!git_config_find_value(key, &value))
return default_value;
if (!value)
config_error_nonbool();
return value;
}
int git_config_get_bool(const char *key, int default_value)
{
const char *value;
if (!git_config_find_value(key, &value))
return default_value;
return git_config_bool(key, value);
}