* [PATCH v2 1/6] config.c: fix accuracy of line number in errors
2014-07-25 12:58 [PATCH v2 0/6] Rewrite `git_config()` using config-set API Tanay Abhra
@ 2014-07-25 12:58 ` Tanay Abhra
2014-07-25 12:58 ` [PATCH v2 2/6] add line number and file name info to `config_set` Tanay Abhra
` (4 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Tanay Abhra @ 2014-07-25 12:58 UTC (permalink / raw)
To: git; +Cc: Tanay Abhra, Ramkumar Ramachandra, Matthieu Moy, Junio C Hamano
From: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
If a callback returns a negative value to `git_config*()` family,
they call `die()` while printing the line number and the file name.
Currently the printed line number is off by one, thus printing the
wrong line number.
Make `linenr` point to the line we just parsed during the call
to callback to get accurate line number in error messages.
Commit-message-by: Tanay Abhra <tanayabh@gmail.com>
Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
config.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/config.c b/config.c
index 0d799e0..06257d9 100644
--- a/config.c
+++ b/config.c
@@ -244,6 +244,7 @@ static int get_next_char(void)
cf->linenr++;
if (c == EOF) {
cf->eof = 1;
+ cf->linenr++;
c = '\n';
}
return c;
@@ -319,6 +320,7 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)
{
int c;
char *value;
+ int ret;
/* Get the full name */
for (;;) {
@@ -341,7 +343,15 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)
if (!value)
return -1;
}
- return fn(name->buf, value, data);
+ /*
+ * We already consumed the \n, but we need linenr to point to
+ * the line we just parsed during the call to fn to get
+ * accurate line number in error messages.
+ */
+ cf->linenr--;
+ ret = fn(name->buf, value, data);
+ cf->linenr++;
+ return ret;
}
static int get_extended_base_var(struct strbuf *name, int c)
--
1.9.0.GIT
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 2/6] add line number and file name info to `config_set`
2014-07-25 12:58 [PATCH v2 0/6] Rewrite `git_config()` using config-set API Tanay Abhra
2014-07-25 12:58 ` [PATCH v2 1/6] config.c: fix accuracy of line number in errors Tanay Abhra
@ 2014-07-25 12:58 ` Tanay Abhra
2014-07-25 12:58 ` [PATCH v2 3/6] rewrite git_config() to use the config-set API Tanay Abhra
` (3 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Tanay Abhra @ 2014-07-25 12:58 UTC (permalink / raw)
To: git; +Cc: Tanay Abhra, Ramkumar Ramachandra, Matthieu Moy, Junio C Hamano
Store file name and line number for each key-value pair in the cache
during parsing of the configuration files.
Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
config.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/config.c b/config.c
index 06257d9..110f9a5 100644
--- a/config.c
+++ b/config.c
@@ -1232,6 +1232,11 @@ int git_config_with_options(config_fn_t fn, void *data,
return ret;
}
+struct key_value_info {
+ const char *filename;
+ int linenr;
+};
+
int git_config(config_fn_t fn, void *data)
{
return git_config_with_options(fn, data, NULL, 1);
@@ -1262,6 +1267,9 @@ static struct config_set_element *configset_find_element(struct config_set *cs,
static int configset_add_value(struct config_set *cs, const char *key, const char *value)
{
struct config_set_element *e;
+ struct string_list_item *si;
+ struct key_value_info *kv_info = xmalloc(sizeof(*kv_info));
+
e = configset_find_element(cs, key);
/*
* Since the keys are being fed by git_config*() callback mechanism, they
@@ -1274,7 +1282,16 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
string_list_init(&e->value_list, 1);
hashmap_add(&cs->config_hash, e);
}
- string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
+ si = string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
+ if (cf) {
+ kv_info->filename = strintern(cf->name);
+ kv_info->linenr = cf->linenr;
+ } else {
+ /* for values read from `git_config_from_parameters()` */
+ kv_info->filename = strintern("parameter");
+ kv_info->linenr = 0;
+ }
+ si->util = kv_info;
return 0;
}
--
1.9.0.GIT
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 3/6] rewrite git_config() to use the config-set API
2014-07-25 12:58 [PATCH v2 0/6] Rewrite `git_config()` using config-set API Tanay Abhra
2014-07-25 12:58 ` [PATCH v2 1/6] config.c: fix accuracy of line number in errors Tanay Abhra
2014-07-25 12:58 ` [PATCH v2 2/6] add line number and file name info to `config_set` Tanay Abhra
@ 2014-07-25 12:58 ` Tanay Abhra
2014-07-25 13:58 ` Matthieu Moy
2014-07-25 12:58 ` [PATCH v2 4/6] add a test for semantic errors in config files Tanay Abhra
` (2 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Tanay Abhra @ 2014-07-25 12:58 UTC (permalink / raw)
To: git; +Cc: Tanay Abhra, Ramkumar Ramachandra, Matthieu Moy, Junio C Hamano
Of all the functions in `git_config*()` family, `git_config()` has the
most invocations in the whole code base. Each `git_config()` invocation
causes config file rereads which can be avoided using the config-set API.
Use the config-set API to rewrite `git_config()` to use the config caching
layer to avoid config file rereads on each invocation during a git process
lifetime. First invocation constructs the cache, and after that for each
successive invocation, `git_config()` feeds values from the config cache
instead of rereading the configuration files.
Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
cache.h | 25 +++++++++++++++++++++++++
config.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
2 files changed, 76 insertions(+), 9 deletions(-)
diff --git a/cache.h b/cache.h
index 7292aef..11ded5a 100644
--- a/cache.h
+++ b/cache.h
@@ -8,6 +8,7 @@
#include "gettext.h"
#include "convert.h"
#include "trace.h"
+#include "string-list.h"
#include SHA1_HEADER
#ifndef git_SHA_CTX
@@ -1351,9 +1352,33 @@ extern int parse_config_key(const char *var,
const char **subsection, int *subsection_len,
const char **key);
+struct config_set_element {
+ struct hashmap_entry ent;
+ char *key;
+ struct string_list value_list;
+};
+
+struct configset_list_item {
+ struct config_set_element *e;
+ int value_index;
+};
+
+/*
+ * the contents of the list are ordered according to their
+ * position in the config files and order of parsing the files.
+ * (i.e. key-value pair at the last position of .git/config will
+ * be at the last item of the list)
+ */
+
+struct configset_list {
+ struct configset_list_item *items;
+ unsigned int nr, alloc;
+};
+
struct config_set {
struct hashmap config_hash;
int hash_initialized;
+ struct configset_list list;
};
extern void git_configset_init(struct config_set *cs);
diff --git a/config.c b/config.c
index 110f9a5..aa5c0ad 100644
--- a/config.c
+++ b/config.c
@@ -35,12 +35,6 @@ struct config_source {
long (*do_ftell)(struct config_source *c);
};
-struct config_set_element {
- struct hashmap_entry ent;
- char *key;
- struct string_list value_list;
-};
-
static struct config_source *cf;
static int zlib_compression_seen;
@@ -1237,11 +1231,44 @@ struct key_value_info {
int linenr;
};
-int git_config(config_fn_t fn, void *data)
+static int git_config_raw(config_fn_t fn, void *data)
{
return git_config_with_options(fn, data, NULL, 1);
}
+static int configset_iter(struct config_set *cs, config_fn_t fn, void *data)
+{
+ int i, value_index;
+ struct string_list *values;
+ struct config_set_element *entry;
+ struct configset_list *list = &cs->list;
+ struct key_value_info *kv_info;
+
+ for (i = 0; i < list->nr; i++) {
+ entry = list->items[i].e;
+ value_index = list->items[i].value_index;
+ values = &entry->value_list;
+ if (fn(entry->key, values->items[value_index].string, data) < 0) {
+ kv_info = values->items[value_index].util;
+ if (!kv_info->linenr)
+ die("unable to parse command-line config");
+ else
+ die("bad config file line %d in %s",
+ kv_info->linenr,
+ kv_info->filename);
+ }
+ }
+ return 0;
+}
+
+static void git_config_check_init(void);
+
+int git_config(config_fn_t fn, void *data)
+{
+ git_config_check_init();
+ return configset_iter(&the_config_set, fn, data);
+}
+
static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)
{
struct config_set_element k;
@@ -1268,6 +1295,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
{
struct config_set_element *e;
struct string_list_item *si;
+ struct configset_list_item *l_item;
struct key_value_info *kv_info = xmalloc(sizeof(*kv_info));
e = configset_find_element(cs, key);
@@ -1283,6 +1311,13 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
hashmap_add(&cs->config_hash, e);
}
si = string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
+
+ if (cs->list.nr + 1 > cs->list.alloc)
+ ALLOC_GROW(cs->list.items, cs->list.nr + 20, cs->list.alloc);
+ l_item = &cs->list.items[cs->list.nr++];
+ l_item->e = e;
+ l_item->value_index = e->value_list.nr - 1;
+
if (cf) {
kv_info->filename = strintern(cf->name);
kv_info->linenr = cf->linenr;
@@ -1306,6 +1341,9 @@ void git_configset_init(struct config_set *cs)
{
hashmap_init(&cs->config_hash, (hashmap_cmp_fn)config_set_element_cmp, 0);
cs->hash_initialized = 1;
+ cs->list.nr = 0;
+ cs->list.alloc = 0;
+ cs->list.items = NULL;
}
void git_configset_clear(struct config_set *cs)
@@ -1318,10 +1356,14 @@ void git_configset_clear(struct config_set *cs)
hashmap_iter_init(&cs->config_hash, &iter);
while ((entry = hashmap_iter_next(&iter))) {
free(entry->key);
- string_list_clear(&entry->value_list, 0);
+ string_list_clear(&entry->value_list, 1);
}
hashmap_free(&cs->config_hash, 1);
cs->hash_initialized = 0;
+ free(cs->list.items);
+ cs->list.nr = 0;
+ cs->list.alloc = 0;
+ cs->list.items = NULL;
}
static int config_set_callback(const char *key, const char *value, void *cb)
@@ -1448,7 +1490,7 @@ static void git_config_check_init(void)
if (the_config_set.hash_initialized)
return;
git_configset_init(&the_config_set);
- git_config(config_set_callback, &the_config_set);
+ git_config_raw(config_set_callback, &the_config_set);
}
void git_config_clear(void)
--
1.9.0.GIT
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 3/6] rewrite git_config() to use the config-set API
2014-07-25 12:58 ` [PATCH v2 3/6] rewrite git_config() to use the config-set API Tanay Abhra
@ 2014-07-25 13:58 ` Matthieu Moy
2014-07-25 14:07 ` Tanay Abhra
0 siblings, 1 reply; 12+ messages in thread
From: Matthieu Moy @ 2014-07-25 13:58 UTC (permalink / raw)
To: Tanay Abhra; +Cc: git, Ramkumar Ramachandra, Junio C Hamano
Tanay Abhra <tanayabh@gmail.com> writes:
> +struct config_set_element {
> + struct hashmap_entry ent;
> + char *key;
> + struct string_list value_list;
> +};
> +
> +struct configset_list_item {
> + struct config_set_element *e;
> + int value_index;
> +};
I originally wondered why you had two levels of pointers, but
config_set_element is not new, just moved. It's OK.
> +/*
> + * the contents of the list are ordered according to their
> + * position in the config files and order of parsing the files.
> + * (i.e. key-value pair at the last position of .git/config will
> + * be at the last item of the list)
> + */
> +
> +struct configset_list {
I wouldn't put a blank line between comment and decl if the comment
applies to the decl.
> -int git_config(config_fn_t fn, void *data)
> +static int git_config_raw(config_fn_t fn, void *data)
> {
> return git_config_with_options(fn, data, NULL, 1);
> }
I would have done this and the new git_config() as a separate patch, but
I do not mind strongly.
> static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)
> {
> struct config_set_element k;
> @@ -1268,6 +1295,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
> {
> struct config_set_element *e;
> struct string_list_item *si;
> + struct configset_list_item *l_item;
> struct key_value_info *kv_info = xmalloc(sizeof(*kv_info));
>
> e = configset_find_element(cs, key);
> @@ -1283,6 +1311,13 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
> hashmap_add(&cs->config_hash, e);
> }
> si = string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
> +
> + if (cs->list.nr + 1 > cs->list.alloc)
The "if" is already in ALLOC_GROW.
> + ALLOC_GROW(cs->list.items, cs->list.nr + 20, cs->list.alloc);
The 20 should be just 1 I guess. You're adding 1 element, and ALLOC_GROW
will take care of allocating more than 1 for you (see alloc_nr and
ALLOC_GROW's defs in cache.h).
> @@ -1318,10 +1356,14 @@ void git_configset_clear(struct config_set *cs)
> hashmap_iter_init(&cs->config_hash, &iter);
> while ((entry = hashmap_iter_next(&iter))) {
> free(entry->key);
> - string_list_clear(&entry->value_list, 0);
> + string_list_clear(&entry->value_list, 1);
Doesn't this change belong to PATCH 2/6 ?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 3/6] rewrite git_config() to use the config-set API
2014-07-25 13:58 ` Matthieu Moy
@ 2014-07-25 14:07 ` Tanay Abhra
0 siblings, 0 replies; 12+ messages in thread
From: Tanay Abhra @ 2014-07-25 14:07 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git, Ramkumar Ramachandra, Junio C Hamano
On 7/25/2014 7:28 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> +struct config_set_element {
>> + struct hashmap_entry ent;
>> + char *key;
>> + struct string_list value_list;
>> +};
>> +
>> +struct configset_list_item {
>> + struct config_set_element *e;
>> + int value_index;
>> +};
>
> I originally wondered why you had two levels of pointers, but
> config_set_element is not new, just moved. It's OK.
>
>> +/*
>> + * the contents of the list are ordered according to their
>> + * position in the config files and order of parsing the files.
>> + * (i.e. key-value pair at the last position of .git/config will
>> + * be at the last item of the list)
>> + */
>> +
>> +struct configset_list {
>
> I wouldn't put a blank line between comment and decl if the comment
> applies to the decl.
>
Noted.
>> -int git_config(config_fn_t fn, void *data)
>> +static int git_config_raw(config_fn_t fn, void *data)
>> {
>> return git_config_with_options(fn, data, NULL, 1);
>> }
>
> I would have done this and the new git_config() as a separate patch, but
> I do not mind strongly.
>
>> static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)
>> {
>> struct config_set_element k;
>> @@ -1268,6 +1295,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
>> {
>> struct config_set_element *e;
>> struct string_list_item *si;
>> + struct configset_list_item *l_item;
>> struct key_value_info *kv_info = xmalloc(sizeof(*kv_info));
>>
>> e = configset_find_element(cs, key);
>> @@ -1283,6 +1311,13 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
>> hashmap_add(&cs->config_hash, e);
>> }
>> si = string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
>> +
>> + if (cs->list.nr + 1 > cs->list.alloc)
>
> The "if" is already in ALLOC_GROW.
>
>> + ALLOC_GROW(cs->list.items, cs->list.nr + 20, cs->list.alloc);
>
> The 20 should be just 1 I guess. You're adding 1 element, and ALLOC_GROW
> will take care of allocating more than 1 for you (see alloc_nr and
> ALLOC_GROW's defs in cache.h).
>
Oh, you are are right, I thought alloc grew by one, not by a factor.
>> @@ -1318,10 +1356,14 @@ void git_configset_clear(struct config_set *cs)
>> hashmap_iter_init(&cs->config_hash, &iter);
>> while ((entry = hashmap_iter_next(&iter))) {
>> free(entry->key);
>> - string_list_clear(&entry->value_list, 0);
>> + string_list_clear(&entry->value_list, 1);
>
> Doesn't this change belong to PATCH 2/6 ?
>
Yup, you are right again.
Thanks.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 4/6] add a test for semantic errors in config files
2014-07-25 12:58 [PATCH v2 0/6] Rewrite `git_config()` using config-set API Tanay Abhra
` (2 preceding siblings ...)
2014-07-25 12:58 ` [PATCH v2 3/6] rewrite git_config() to use the config-set API Tanay Abhra
@ 2014-07-25 12:58 ` Tanay Abhra
2014-07-25 12:58 ` [PATCH v2 5/6] config: add `git_die_config()` to the config-set API Tanay Abhra
2014-07-25 12:58 ` [PATCH v2 6/6] Add tests for `git_config_get_string()` Tanay Abhra
5 siblings, 0 replies; 12+ messages in thread
From: Tanay Abhra @ 2014-07-25 12:58 UTC (permalink / raw)
To: git; +Cc: Tanay Abhra, Ramkumar Ramachandra, Matthieu Moy, Junio C Hamano
Semantic errors (for example, for alias.* variables NULL values are
not allowed) in configuration files cause a die printing the line
number and file name of the offending value.
Add a test documenting that such errors cause a die printing the
accurate line number and file name.
Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
t/t1308-config-set.sh | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 7fdf840..35c6ee2 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -197,4 +197,15 @@ test_expect_success 'proper error on error in custom config files' '
test_cmp expect actual
'
+test_expect_success 'check line errors for malformed values' '
+ mv .git/config .git/config.old &&
+ test_when_finished "mv .git/config.old .git/config" &&
+ cat >.git/config <<-\EOF &&
+ [alias]
+ br
+ EOF
+ test_expect_code 128 git br 2>result &&
+ grep "fatal: bad config file line 2 in .git/config" result
+'
+
test_done
--
1.9.0.GIT
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 5/6] config: add `git_die_config()` to the config-set API
2014-07-25 12:58 [PATCH v2 0/6] Rewrite `git_config()` using config-set API Tanay Abhra
` (3 preceding siblings ...)
2014-07-25 12:58 ` [PATCH v2 4/6] add a test for semantic errors in config files Tanay Abhra
@ 2014-07-25 12:58 ` Tanay Abhra
2014-07-25 14:03 ` Matthieu Moy
2014-07-25 12:58 ` [PATCH v2 6/6] Add tests for `git_config_get_string()` Tanay Abhra
5 siblings, 1 reply; 12+ messages in thread
From: Tanay Abhra @ 2014-07-25 12:58 UTC (permalink / raw)
To: git; +Cc: Tanay Abhra, Ramkumar Ramachandra, Matthieu Moy, Junio C Hamano
Add `git_die_config` that dies printing the line number and the file name
of the highest priority value for the configuration variable `key`.
It has usage in non-callback based config value retrieval where we can
raise an error and die if there is a semantic error.
For example,
if (!git_config_get_value(key, &value)) {
/* NULL values not allowed */
if (!value)
git_config_die(key);
else
/* do work */
}
Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
Documentation/technical/api-config.txt | 5 ++++
cache.h | 1 +
config.c | 44 ++++++++++++++++++++++++++--------
3 files changed, 40 insertions(+), 10 deletions(-)
diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index 815c1ee..e7ec7cc 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -155,6 +155,11 @@ as well as retrieval for the queried variable, including:
Similar to `git_config_get_string`, but expands `~` or `~user` into
the user's home directory when found at the beginning of the path.
+`void git_die_config(const char *key)`::
+
+ Dies printing the line number and the file name of the highest
+ priority value for the configuration variable `key`.
+
See test-config.c for usage examples.
Value Parsing Helpers
diff --git a/cache.h b/cache.h
index 11ded5a..5c3dd88 100644
--- a/cache.h
+++ b/cache.h
@@ -1407,6 +1407,7 @@ extern int git_config_get_bool(const char *key, int *dest);
extern int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest);
extern int git_config_get_maybe_bool(const char *key, int *dest);
extern int git_config_get_pathname(const char *key, const char **dest);
+extern void git_die_config(const char *key);
extern int committer_ident_sufficiently_given(void);
extern int author_ident_sufficiently_given(void);
diff --git a/config.c b/config.c
index aa5c0ad..1e49ae7 100644
--- a/config.c
+++ b/config.c
@@ -1403,11 +1403,12 @@ const struct string_list *git_configset_get_value_multi(struct config_set *cs, c
int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest)
{
- const char *value;
- if (!git_configset_get_value(cs, key, &value))
- return git_config_string(dest, key, value);
- else
- return 1;
+ int ret;
+ char *value;
+ ret = git_configset_get_string(cs, key, &value);
+ if (ret <= 0)
+ *dest = (const char*)value;
+ return ret;
}
int git_configset_get_string(struct config_set *cs, const char *key, char **dest)
@@ -1418,8 +1419,7 @@ int git_configset_get_string(struct config_set *cs, const char *key, char **dest
return config_error_nonbool(key);
*dest = xstrdup(value);
return 0;
- }
- else
+ } else
return 1;
}
@@ -1514,14 +1514,22 @@ const struct string_list *git_config_get_value_multi(const char *key)
int git_config_get_string_const(const char *key, const char **dest)
{
+ int ret;
git_config_check_init();
- return git_configset_get_string_const(&the_config_set, key, dest);
+ ret = git_configset_get_string_const(&the_config_set, key, dest);
+ if (ret < 0)
+ git_die_config(key);
+ return ret;
}
int git_config_get_string(const char *key, char **dest)
{
+ int ret;
git_config_check_init();
- return git_configset_get_string(&the_config_set, key, dest);
+ ret = git_configset_get_string(&the_config_set, key, dest);
+ if (ret < 0)
+ git_die_config(key);
+ return ret;
}
int git_config_get_int(const char *key, int *dest)
@@ -1556,8 +1564,24 @@ int git_config_get_maybe_bool(const char *key, int *dest)
int git_config_get_pathname(const char *key, const char **dest)
{
+ int ret;
git_config_check_init();
- return git_configset_get_pathname(&the_config_set, key, dest);
+ ret = git_configset_get_pathname(&the_config_set, key, dest);
+ if (ret < 0)
+ git_die_config(key);
+ return ret;
+}
+
+void git_die_config(const char *key)
+{
+ const struct string_list *values;
+ struct key_value_info *kv_info;
+ values = git_config_get_value_multi(key);
+ kv_info = values->items[values->nr - 1].util;
+ if (!kv_info->linenr)
+ die("unable to parse command-line config");
+ else
+ die("bad config file line %d in %s",kv_info->linenr, kv_info->filename);
}
/*
--
1.9.0.GIT
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 5/6] config: add `git_die_config()` to the config-set API
2014-07-25 12:58 ` [PATCH v2 5/6] config: add `git_die_config()` to the config-set API Tanay Abhra
@ 2014-07-25 14:03 ` Matthieu Moy
2014-07-25 14:12 ` Tanay Abhra
0 siblings, 1 reply; 12+ messages in thread
From: Matthieu Moy @ 2014-07-25 14:03 UTC (permalink / raw)
To: Tanay Abhra; +Cc: git, Ramkumar Ramachandra, Junio C Hamano
Tanay Abhra <tanayabh@gmail.com> writes:
> --- a/config.c
> +++ b/config.c
> @@ -1403,11 +1403,12 @@ const struct string_list *git_configset_get_value_multi(struct config_set *cs, c
>
> int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest)
> {
> - const char *value;
> - if (!git_configset_get_value(cs, key, &value))
> - return git_config_string(dest, key, value);
> - else
> - return 1;
> + int ret;
> + char *value;
> + ret = git_configset_get_string(cs, key, &value);
> + if (ret <= 0)
> + *dest = (const char*)value;
> + return ret;
> }
Isn't this a fixup meant for another series?
> int git_configset_get_string(struct config_set *cs, const char *key, char **dest)
> @@ -1418,8 +1419,7 @@ int git_configset_get_string(struct config_set *cs, const char *key, char **dest
> return config_error_nonbool(key);
> *dest = xstrdup(value);
> return 0;
> - }
> - else
> + } else
> return 1;
Useless churn.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 5/6] config: add `git_die_config()` to the config-set API
2014-07-25 14:03 ` Matthieu Moy
@ 2014-07-25 14:12 ` Tanay Abhra
2014-07-25 17:29 ` Junio C Hamano
0 siblings, 1 reply; 12+ messages in thread
From: Tanay Abhra @ 2014-07-25 14:12 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git, Ramkumar Ramachandra, Junio C Hamano
On 7/25/2014 7:33 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> --- a/config.c
>> +++ b/config.c
>> @@ -1403,11 +1403,12 @@ const struct string_list *git_configset_get_value_multi(struct config_set *cs, c
>>
>> int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest)
>> {
>> - const char *value;
>> - if (!git_configset_get_value(cs, key, &value))
>> - return git_config_string(dest, key, value);
>> - else
>> - return 1;
>> + int ret;
>> + char *value;
>> + ret = git_configset_get_string(cs, key, &value);
>> + if (ret <= 0)
>> + *dest = (const char*)value;
>> + return ret;
>> }
>
> Isn't this a fixup meant for another series?
>
Though v12 is in pu, Junio commented that git_configset_get_string_const() &
git_configset_get_string() can be done more concisely, I was trying to do
that but I failed.
>> int git_configset_get_string(struct config_set *cs, const char *key, char **dest)
>> @@ -1418,8 +1419,7 @@ int git_configset_get_string(struct config_set *cs, const char *key, char **dest
>> return config_error_nonbool(key);
>> *dest = xstrdup(value);
>> return 0;
>> - }
>> - else
>> + } else
>> return 1;
>
> Useless churn.
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 5/6] config: add `git_die_config()` to the config-set API
2014-07-25 14:12 ` Tanay Abhra
@ 2014-07-25 17:29 ` Junio C Hamano
0 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2014-07-25 17:29 UTC (permalink / raw)
To: Tanay Abhra; +Cc: Matthieu Moy, git, Ramkumar Ramachandra
Tanay Abhra <tanayabh@gmail.com> writes:
> On 7/25/2014 7:33 PM, Matthieu Moy wrote:
>> Tanay Abhra <tanayabh@gmail.com> writes:
>>
>>> --- a/config.c
>>> +++ b/config.c
>>> @@ -1403,11 +1403,12 @@ const struct string_list *git_configset_get_value_multi(struct config_set *cs, c
>>>
>>> int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest)
>>> {
>>> - const char *value;
>>> - if (!git_configset_get_value(cs, key, &value))
>>> - return git_config_string(dest, key, value);
>>> - else
>>> - return 1;
>>> + int ret;
>>> + char *value;
>>> + ret = git_configset_get_string(cs, key, &value);
>>> + if (ret <= 0)
>>> + *dest = (const char*)value;
>>> + return ret;
>>> }
>>
>> Isn't this a fixup meant for another series?
>>
>
> Though v12 is in pu, Junio commented that git_configset_get_string_const() &
> git_configset_get_string() can be done more concisely, I was trying to do
> that but I failed.
My comment on that version was not about conciseness. You had one
that called git_config_string() to let the callee do the nonbool
error handling and xstrdup() of the non-error return value, and the
other one that did exactly what a call to git_config_string() would
have done. That is being redundant, not just failing to be concise.
I was actually hoping that we would see just
int git_configset_get_string(struct config_set *cs, const char *key, char **dest)
{
return git_configset_get_string_const(cs, key, (const char **)dest);
}
with the implementation of _const() variant be the one from v12.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 6/6] Add tests for `git_config_get_string()`
2014-07-25 12:58 [PATCH v2 0/6] Rewrite `git_config()` using config-set API Tanay Abhra
` (4 preceding siblings ...)
2014-07-25 12:58 ` [PATCH v2 5/6] config: add `git_die_config()` to the config-set API Tanay Abhra
@ 2014-07-25 12:58 ` Tanay Abhra
5 siblings, 0 replies; 12+ messages in thread
From: Tanay Abhra @ 2014-07-25 12:58 UTC (permalink / raw)
To: git; +Cc: Tanay Abhra, Ramkumar Ramachandra, Matthieu Moy, Junio C Hamano
Add tests for `git_config_get_string()`, check whether it
dies printing the line number and the file name if a NULL
value is retrieved for the given key.
Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
t/t1308-config-set.sh | 9 +++++++++
test-config.c | 10 ++++++++++
2 files changed, 19 insertions(+)
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 35c6ee2..d7cdc6e 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -119,6 +119,15 @@ test_expect_success 'find integer value for a key' '
check_config get_int lamb.chop 65
'
+test_expect_success 'find string value for a key' '
+ check_config get_string case.baz hask
+'
+
+test_expect_success 'check line error when NULL string is queried' '
+ test_expect_code 128 test-config get_string case.foo 2>result &&
+ grep "fatal: bad config file line 7 in .git/config" result
+'
+
test_expect_success 'find integer if value is non parse-able' '
check_config expect_code 128 get_int lamb.head
'
diff --git a/test-config.c b/test-config.c
index 9dd1b22..6a77552 100644
--- a/test-config.c
+++ b/test-config.c
@@ -16,6 +16,8 @@
*
* get_bool -> print bool value for the entered key or die
*
+ * get_string -> print string value for the entered key or die
+ *
* configset_get_value -> returns value with the highest priority for the entered key
* from a config_set constructed from files entered as arguments.
*
@@ -84,6 +86,14 @@ int main(int argc, char **argv)
printf("Value not found for \"%s\"\n", argv[2]);
goto exit1;
}
+ } else if (argc == 3 && !strcmp(argv[1], "get_string")) {
+ if (!git_config_get_string_const(argv[2], &v)) {
+ printf("%s\n", v);
+ goto exit0;
+ } else {
+ printf("Value not found for \"%s\"\n", argv[2]);
+ goto exit1;
+ }
} else if (!strcmp(argv[1], "configset_get_value")) {
for (i = 3; i < argc; i++) {
int err;
--
1.9.0.GIT
^ permalink raw reply related [flat|nested] 12+ messages in thread