Re: [PATCH v9 3/8] add line number and file name info to `config_set`
Junio C Hamano writes:
> Matthieu Moy writes:
>
>> Ramsay Jones writes:
>> ...
diff --git a/cache.h b/cache.h
...
+struct key_value_info {
+ const char *filename;
+ int linenr;
+};
+
>>>
>>> I haven't checked, but does this series now include a user for
>>> this struct outside of config.c? If not, then I think it would
>>> be better to leave the declaration in config.c until it is needed.
>>> (To make it easier to see if it is necessary in the context of the
>>> patch which will make use of it).
>>
>> I disagree: this patch series is essentially about introducing a new
>> API, and this struct declaration is part of the API.
>
> Hmm, is it? How would the customer of the API use it? die_config
> and friends may internally use the information recorded using the
> structure, but I had an impression that it is an implementation
> detail that does not need to be exposed to the customers of the API.
> Am I mistaken?
It does if you want to provide error message while iterating over the
string_list. Not the common case, but shouldn't be forbidden either.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 3/8] add line number and file name info to `config_set`
Matthieu Moy writes:
> Ramsay Jones writes:
> ...
>>> diff --git a/cache.h b/cache.h
>>> ...
>>> +struct key_value_info {
>>> + const char *filename;
>>> + int linenr;
>>> +};
>>> +
>>
>> I haven't checked, but does this series now include a user for
>> this struct outside of config.c? If not, then I think it would
>> be better to leave the declaration in config.c until it is needed.
>> (To make it easier to see if it is necessary in the context of the
>> patch which will make use of it).
>
> I disagree: this patch series is essentially about introducing a new
> API, and this struct declaration is part of the API.
Hmm, is it? How would the customer of the API use it? die_config
and friends may internally use the information recorded using the
structure, but I had an impression that it is an implementation
detail that does not need to be exposed to the customers of the API.
Am I mistaken?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 3/8] add line number and file name info to `config_set`
Ramsay Jones writes:
> On 07/08/14 12:59, Tanay Abhra wrote:
>> 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
>> ---
>> cache.h | 5 +
>> config.c | 16 ++--
>> 2 files changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/cache.h b/cache.h
>> index 7292aef..0b1bdfd 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -1383,6 +1383,11 @@ 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);
>>
>> +struct key_value_info {
>> +const char *filename;
>> +int linenr;
>> +};
>> +
>
> I haven't checked, but does this series now include a user for
> this struct outside of config.c? If not, then I think it would
> be better to leave the declaration in config.c until it is needed.
> (To make it easier to see if it is necessary in the context of the
> patch which will make use of it).
I disagree: this patch series is essentially about introducing a new
API, and this struct declaration is part of the API.
It seemed strange to me to see the code movement in the patch from two
versions of the series, but the patch itself does not move the code, it
just adds new code directly where it belongs.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 3/8] add line number and file name info to `config_set`
On 07/08/14 12:59, Tanay Abhra wrote:
> 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
> ---
> cache.h | 5 +
> config.c | 16 ++--
> 2 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 7292aef..0b1bdfd 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1383,6 +1383,11 @@ 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);
>
> +struct key_value_info {
> + const char *filename;
> + int linenr;
> +};
> +
I haven't checked, but does this series now include a user for
this struct outside of config.c? If not, then I think it would
be better to leave the declaration in config.c until it is needed.
(To make it easier to see if it is necessary in the context of the
patch which will make use of it).
ATB,
Ramsay Jones
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 3/8] add line number and file name info to `config_set`
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
---
cache.h | 5 +
config.c | 16 ++--
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index 7292aef..0b1bdfd 100644
--- a/cache.h
+++ b/cache.h
@@ -1383,6 +1383,11 @@ 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);
+struct key_value_info {
+ const char *filename;
+ int linenr;
+};
+
extern int committer_ident_sufficiently_given(void);
extern int author_ident_sufficiently_given(void);
diff --git a/config.c b/config.c
index bb4629e..e4d745e 100644
--- a/config.c
+++ b/config.c
@@ -1260,6 +1260,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
@@ -1272,7 +1275,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 = NULL;
+ kv_info->linenr = -1;
+ }
+ si->util = kv_info;
return 0;
}
@@ -1299,7 +1311,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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
