* [PATCH v3 1/6] config.c: fix accuracy of line number in errors
2014-07-28 10:33 [PATCH v3 0/6] Rewrite `git_config()` using config-set API Tanay Abhra
@ 2014-07-28 10:33 ` Tanay Abhra
2014-07-28 10:33 ` [PATCH v3 2/6] add line number and file name info to `config_set` Tanay Abhra
` (5 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Tanay Abhra @ 2014-07-28 10:33 UTC (permalink / raw)
To: git; +Cc: Tanay Abhra, Ramkumar Ramachandra, Matthieu Moy
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 d3ad661..e5b7f10 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] 15+ messages in thread* [PATCH v3 2/6] add line number and file name info to `config_set`
2014-07-28 10:33 [PATCH v3 0/6] Rewrite `git_config()` using config-set API Tanay Abhra
2014-07-28 10:33 ` [PATCH v3 1/6] config.c: fix accuracy of line number in errors Tanay Abhra
@ 2014-07-28 10:33 ` Tanay Abhra
2014-07-28 10:33 ` [PATCH v3 3/6] rewrite git_config() to use the config-set API Tanay Abhra
` (4 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Tanay Abhra @ 2014-07-28 10:33 UTC (permalink / raw)
To: git; +Cc: Tanay Abhra, Ramkumar Ramachandra, Matthieu Moy
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 | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index e5b7f10..5499108 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;
}
@@ -1301,7 +1318,7 @@ 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;
--
1.9.0.GIT
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v3 3/6] rewrite git_config() to use the config-set API
2014-07-28 10:33 [PATCH v3 0/6] Rewrite `git_config()` using config-set API Tanay Abhra
2014-07-28 10:33 ` [PATCH v3 1/6] config.c: fix accuracy of line number in errors Tanay Abhra
2014-07-28 10:33 ` [PATCH v3 2/6] add line number and file name info to `config_set` Tanay Abhra
@ 2014-07-28 10:33 ` Tanay Abhra
2014-07-28 10:33 ` [PATCH v3 4/6] add a test for semantic errors in config files Tanay Abhra
` (3 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Tanay Abhra @ 2014-07-28 10:33 UTC (permalink / raw)
To: git; +Cc: Tanay Abhra, Ramkumar Ramachandra, Matthieu Moy
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 | 24 ++++++++++++++++++++++++
config.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++--------
2 files changed, 73 insertions(+), 8 deletions(-)
diff --git a/cache.h b/cache.h
index 7292aef..93bdbab 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,32 @@ 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 5499108..2ce3318 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,12 @@ 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);
+
+ ALLOC_GROW(cs->list.items, cs->list.nr + 1, 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 +1340,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)
@@ -1322,6 +1359,10 @@ void git_configset_clear(struct config_set *cs)
}
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)
@@ -1440,7 +1481,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] 15+ messages in thread* [PATCH v3 4/6] add a test for semantic errors in config files
2014-07-28 10:33 [PATCH v3 0/6] Rewrite `git_config()` using config-set API Tanay Abhra
` (2 preceding siblings ...)
2014-07-28 10:33 ` [PATCH v3 3/6] rewrite git_config() to use the config-set API Tanay Abhra
@ 2014-07-28 10:33 ` Tanay Abhra
2014-07-28 11:22 ` Matthieu Moy
2014-07-28 10:33 ` [PATCH v3 5/6] config: add `git_die_config()` to the config-set API Tanay Abhra
` (2 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Tanay Abhra @ 2014-07-28 10:33 UTC (permalink / raw)
To: git; +Cc: Tanay Abhra, Ramkumar Ramachandra, Matthieu Moy
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] 15+ messages in thread* Re: [PATCH v3 4/6] add a test for semantic errors in config files
2014-07-28 10:33 ` [PATCH v3 4/6] add a test for semantic errors in config files Tanay Abhra
@ 2014-07-28 11:22 ` Matthieu Moy
2014-07-28 11:27 ` Tanay Abhra
0 siblings, 1 reply; 15+ messages in thread
From: Matthieu Moy @ 2014-07-28 11:22 UTC (permalink / raw)
To: Tanay Abhra; +Cc: git, Ramkumar Ramachandra
Tanay Abhra <tanayabh@gmail.com> writes:
> +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
> +'
This is PATCH 4, and it tests a bug fixed in PATCH 1. It would have
eased review to group both patches, either
PATCH 1: introduce test_expect_failure test to demonstrate the failure
PATCH 2: fix the bug and change test_expect_failure to test_expect_success
Or putting both in the same patch.
I think the series is OK like this, my comment is just to be read as
"next time, here's how to do better".
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 4/6] add a test for semantic errors in config files
2014-07-28 11:22 ` Matthieu Moy
@ 2014-07-28 11:27 ` Tanay Abhra
2014-07-28 11:38 ` Matthieu Moy
0 siblings, 1 reply; 15+ messages in thread
From: Tanay Abhra @ 2014-07-28 11:27 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git, Ramkumar Ramachandra
On 7/28/2014 4:52 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> +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
>> +'
>
> This is PATCH 4, and it tests a bug fixed in PATCH 1. It would have
> eased review to group both patches, either
>
> PATCH 1: introduce test_expect_failure test to demonstrate the failure
Didn't Junio comment that he wouldn't recommend inserting a test_expect_failure
for new tests and then flipping them after in the series.
> PATCH 2: fix the bug and change test_expect_failure to test_expect_success
>
> Or putting both in the same patch.
>
Much better, thanks for the advice.
> I think the series is OK like this, my comment is just to be read as
> "next time, here's how to do better".
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 4/6] add a test for semantic errors in config files
2014-07-28 11:27 ` Tanay Abhra
@ 2014-07-28 11:38 ` Matthieu Moy
0 siblings, 0 replies; 15+ messages in thread
From: Matthieu Moy @ 2014-07-28 11:38 UTC (permalink / raw)
To: Tanay Abhra; +Cc: git, Ramkumar Ramachandra
Tanay Abhra <tanayabh@gmail.com> writes:
> On 7/28/2014 4:52 PM, Matthieu Moy wrote:
>> Tanay Abhra <tanayabh@gmail.com> writes:
>>
>>> +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
>>> +'
>>
>> This is PATCH 4, and it tests a bug fixed in PATCH 1. It would have
>> eased review to group both patches, either
>>
>> PATCH 1: introduce test_expect_failure test to demonstrate the failure
>
> Didn't Junio comment that he wouldn't recommend inserting a test_expect_failure
> for new tests and then flipping them after in the series.
No, he even said it was good practice:
http://thread.gmane.org/gmane.comp.version-control.git/254101/focus=254104
his point was to avoid breaking something and repairing in another patch
(which your initial series was doing because the test patch was coming
between "rewrite git_config() to use the config-set API" and "add line
number and file name info to `config_set`").
The situation is different when you have a pre-existing bug.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 5/6] config: add `git_die_config()` to the config-set API
2014-07-28 10:33 [PATCH v3 0/6] Rewrite `git_config()` using config-set API Tanay Abhra
` (3 preceding siblings ...)
2014-07-28 10:33 ` [PATCH v3 4/6] add a test for semantic errors in config files Tanay Abhra
@ 2014-07-28 10:33 ` Tanay Abhra
2014-07-28 10:55 ` Ramsay Jones
2014-07-28 11:14 ` Matthieu Moy
2014-07-28 10:33 ` [PATCH v3 6/6] add tests for `git_config_get_string_const()` Tanay Abhra
2014-07-28 11:24 ` [PATCH v3 0/6] Rewrite `git_config()` using config-set API Matthieu Moy
6 siblings, 2 replies; 15+ messages in thread
From: Tanay Abhra @ 2014-07-28 10:33 UTC (permalink / raw)
To: git; +Cc: Tanay Abhra, Ramkumar Ramachandra, Matthieu Moy
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>
---
Note: git_config_get_string() calls git_config_get_string_const(),
so no need to check for whether to die or not, as it is done by
git_config_get_string_const().
Documentation/technical/api-config.txt | 5 +++++
cache.h | 1 +
config.c | 24 ++++++++++++++++++++++--
3 files changed, 28 insertions(+), 2 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 93bdbab..8512225 100644
--- a/cache.h
+++ b/cache.h
@@ -1406,6 +1406,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 2ce3318..136ee9c 100644
--- a/config.c
+++ b/config.c
@@ -1505,8 +1505,12 @@ 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)
@@ -1547,10 +1551,26 @@ 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);
+ }
+
/*
* Find all the stuff for git_config_set() below.
*/
--
1.9.0.GIT
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3 5/6] config: add `git_die_config()` to the config-set API
2014-07-28 10:33 ` [PATCH v3 5/6] config: add `git_die_config()` to the config-set API Tanay Abhra
@ 2014-07-28 10:55 ` Ramsay Jones
2014-07-28 11:02 ` Tanay Abhra
2014-07-28 11:14 ` Matthieu Moy
1 sibling, 1 reply; 15+ messages in thread
From: Ramsay Jones @ 2014-07-28 10:55 UTC (permalink / raw)
To: Tanay Abhra, git; +Cc: Ramkumar Ramachandra, Matthieu Moy
On 28/07/14 11:33, Tanay Abhra wrote:
> 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>
> ---
> Note: git_config_get_string() calls git_config_get_string_const(),
> so no need to check for whether to die or not, as it is done by
> git_config_get_string_const().
>
> Documentation/technical/api-config.txt | 5 +++++
> cache.h | 1 +
> config.c | 24 ++++++++++++++++++++++--
> 3 files changed, 28 insertions(+), 2 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 93bdbab..8512225 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1406,6 +1406,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 2ce3318..136ee9c 100644
> --- a/config.c
> +++ b/config.c
> @@ -1505,8 +1505,12 @@ 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)
> @@ -1547,10 +1551,26 @@ 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);
How about including the 'key' string in the error message?
Similar comment applies to an earlier patch in this series.
> + }
> +
> /*
> * Find all the stuff for git_config_set() below.
> */
>
ATB,
Ramsay Jones
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 5/6] config: add `git_die_config()` to the config-set API
2014-07-28 10:55 ` Ramsay Jones
@ 2014-07-28 11:02 ` Tanay Abhra
2014-07-28 11:16 ` Matthieu Moy
0 siblings, 1 reply; 15+ messages in thread
From: Tanay Abhra @ 2014-07-28 11:02 UTC (permalink / raw)
To: Ramsay Jones, git; +Cc: Ramkumar Ramachandra, Matthieu Moy
On 7/28/2014 4:25 PM, Ramsay Jones wrote:
> On 28/07/14 11:33, Tanay Abhra wrote:
>> 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>
>> ---
>> Note: git_config_get_string() calls git_config_get_string_const(),
>> so no need to check for whether to die or not, as it is done by
>> git_config_get_string_const().
>>
>> Documentation/technical/api-config.txt | 5 +++++
>> cache.h | 1 +
>> config.c | 24 ++++++++++++++++++++++--
>> 3 files changed, 28 insertions(+), 2 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 93bdbab..8512225 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -1406,6 +1406,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 2ce3318..136ee9c 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -1505,8 +1505,12 @@ 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)
>> @@ -1547,10 +1551,26 @@ 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);
>
> How about including the 'key' string in the error message?
> Similar comment applies to an earlier patch in this series.
>
git_config_get_string & pathname() are already raising a
config_error_nonbool() with the error message,
"Missing value for 'key'".
For other cases, I was just following the previous error message,
but it can be done for the new git_config().
Thanks.
>> + }
>> +
>> /*
>> * Find all the stuff for git_config_set() below.
>> */
>>
>
> ATB,
> Ramsay Jones
>
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v3 5/6] config: add `git_die_config()` to the config-set API
2014-07-28 11:02 ` Tanay Abhra
@ 2014-07-28 11:16 ` Matthieu Moy
0 siblings, 0 replies; 15+ messages in thread
From: Matthieu Moy @ 2014-07-28 11:16 UTC (permalink / raw)
To: Tanay Abhra; +Cc: Ramsay Jones, git, Ramkumar Ramachandra
Tanay Abhra <tanayabh@gmail.com> writes:
> On 7/28/2014 4:25 PM, Ramsay Jones wrote:
>> On 28/07/14 11:33, Tanay Abhra wrote:
>>> + 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);
>>
>> How about including the 'key' string in the error message?
>> Similar comment applies to an earlier patch in this series.
>>
>
> git_config_get_string & pathname() are already raising a
> config_error_nonbool() with the error message,
> "Missing value for 'key'".
>
> For other cases, I was just following the previous error message,
> but it can be done for the new git_config().
It is good to follow the existing in this patch (better code, same
behavior), but I agree with Ramsay that including key in the error
message (for both branches of the if/then/else) in a further patch would
be good, and should be straightforward enough.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 5/6] config: add `git_die_config()` to the config-set API
2014-07-28 10:33 ` [PATCH v3 5/6] config: add `git_die_config()` to the config-set API Tanay Abhra
2014-07-28 10:55 ` Ramsay Jones
@ 2014-07-28 11:14 ` Matthieu Moy
1 sibling, 0 replies; 15+ messages in thread
From: Matthieu Moy @ 2014-07-28 11:14 UTC (permalink / raw)
To: Tanay Abhra; +Cc: git, Ramkumar Ramachandra
Tanay Abhra <tanayabh@gmail.com> writes:
> +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);
Missing space after ,.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 6/6] add tests for `git_config_get_string_const()`
2014-07-28 10:33 [PATCH v3 0/6] Rewrite `git_config()` using config-set API Tanay Abhra
` (4 preceding siblings ...)
2014-07-28 10:33 ` [PATCH v3 5/6] config: add `git_die_config()` to the config-set API Tanay Abhra
@ 2014-07-28 10:33 ` Tanay Abhra
2014-07-28 11:24 ` [PATCH v3 0/6] Rewrite `git_config()` using config-set API Matthieu Moy
6 siblings, 0 replies; 15+ messages in thread
From: Tanay Abhra @ 2014-07-28 10:33 UTC (permalink / raw)
To: git; +Cc: Tanay Abhra, Ramkumar Ramachandra, Matthieu Moy
Add tests for `git_config_get_string_const()`, 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] 15+ messages in thread* Re: [PATCH v3 0/6] Rewrite `git_config()` using config-set API
2014-07-28 10:33 [PATCH v3 0/6] Rewrite `git_config()` using config-set API Tanay Abhra
` (5 preceding siblings ...)
2014-07-28 10:33 ` [PATCH v3 6/6] add tests for `git_config_get_string_const()` Tanay Abhra
@ 2014-07-28 11:24 ` Matthieu Moy
6 siblings, 0 replies; 15+ messages in thread
From: Matthieu Moy @ 2014-07-28 11:24 UTC (permalink / raw)
To: Tanay Abhra; +Cc: git, Ramkumar Ramachandra
Tanay Abhra <tanayabh@gmail.com> writes:
> [PATCH V3]:All the suggestions in [3] applied. Built on top of [1].
Except for the minor style fix in PATCH 5, the series looks OK to me.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 15+ messages in thread