Re: [PATCH v3 0/6] git_config callers rewritten with the new config cache API
Tanay Abhra writes:
> On 7/21/2014 7:15 PM, Matthieu Moy wrote:
>> Tanay Abhra writes:
>>
>>> On 7/21/2014 6:21 PM, Matthieu Moy wrote:
2) Add a by-address parameter to git_configset_get_value that allows the
user to get the file and line information. In your previous patch,
that would mean returning a pointer to the corresponding struct
key_source.
>>>
>>> Will this extra complexity be good for "git_configset_get_value"?
>>> Instead can we provide a function like die_config(char *key)
>>> which prints
>>> die("bad config file line %d in %s", linenr, filename);?
>>
>> Where would you call this function, and where would you take linenr and
>> filename?
>>
>
> Usage can be like this,
>
> if(!git_config_get_value(k, &v)) {
> if (!v) {
> config_error_nonbool(k);
> die_config(k);
> /* die_config calls git_config_get_value_multi for 'k',
>* gets the string list with the util pointer containing
>* the linenr and the file name, dies printing the message.
>*/
> } else
> /* do work */
> }
OK, so you query the cache twice (which is OK, it's cheap and happens
just once before dying). That would work too.
--
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 v3 0/6] git_config callers rewritten with the new config cache API
On 7/21/2014 7:15 PM, Matthieu Moy wrote:
> Tanay Abhra writes:
>
>> On 7/21/2014 6:21 PM, Matthieu Moy wrote:
>>> 2) Add a by-address parameter to git_configset_get_value that allows the
>>>user to get the file and line information. In your previous patch,
>>>that would mean returning a pointer to the corresponding struct
>>>key_source.
>>
>> Will this extra complexity be good for "git_configset_get_value"?
>> Instead can we provide a function like die_config(char *key)
>> which prints
>> die("bad config file line %d in %s", linenr, filename);?
>
> Where would you call this function, and where would you take linenr and
> filename?
>
Usage can be like this,
if(!git_config_get_value(k, &v)) {
if (!v) {
config_error_nonbool(k);
die_config(k);
/* die_config calls git_config_get_value_multi for 'k',
* gets the string list with the util pointer containing
* the linenr and the file name, dies printing the message.
*/
} else
/* do work */
}
Above example works just like the current code. Currently the callbacks
does not have the access to the linenr and file name anyway.
--
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 v3 0/6] git_config callers rewritten with the new config cache API
Tanay Abhra writes:
> On 7/21/2014 6:21 PM, Matthieu Moy wrote:
>> 2) Add a by-address parameter to git_configset_get_value that allows the
>>user to get the file and line information. In your previous patch,
>>that would mean returning a pointer to the corresponding struct
>>key_source.
>
> Will this extra complexity be good for "git_configset_get_value"?
> Instead can we provide a function like die_config(char *key)
> which prints
> die("bad config file line %d in %s", linenr, filename);?
Where would you call this function, and where would you take linenr and
filename?
--
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 v3 0/6] git_config callers rewritten with the new config cache API
On 7/21/2014 6:21 PM, Matthieu Moy wrote:
> Tanay Abhra writes:
>
>> [PATCH v3]: Most of Eric's suggestions has been implemented. See [2] for
>> discussion.
>> Also, new helpers introduced in v7 of the config-set API series have
>> been used.
>> See [1] for the documentation of the new functions.
>>
>> This series builds on the top of 5def4132 in pu or topic[1] in the mailing
>> list
>> with name "git config cache & special querying API utilizing the cache".
>
> It's now called ta/config-set (see last "What's cooking in git.git").
>
Noted. More below.
>> All patches pass every test, but there is a catch, there is slight behaviour
>> change in most of them where originally the callback returns
>> config_error_nonbool() when it sees a NULL value for a key causing a die
>> specified in git_parse_source in config.c.
>>
>> The die also prints the file name and the line number as,
>>
>> "die("bad config file line %d in %s", cf->linenr, cf->name);"
>>
>> We lose the fine grained error checking when switching to this method.
>
> I think a first step would be something like this:
>
> --- a/config.c
> +++ b/config.c
> @@ -656,6 +656,15 @@ int git_config_string(const char **dest, const char
> *var, const char *value)
> return 0;
> }
>
> +// TODO: either make it static or export it properly
> +int git_config_string_or_die(const char **dest, const char *var, const char
> *value)
> +{
> + if (git_config_string(dest, var, value) < 0)
> + die("bad config file (TODO: file/line info)");
> + else
> + return 0;
> +}
> +
> int git_config_pathname(const char **dest, const char *var, const char
> *value)
> {
> if (!value)
> @@ -1336,7 +1345,7 @@ int git_configset_get_string(struct config_set *cs,
> const char *key, const char
> {
> const char *value;
> if (!git_configset_get_value(cs, key, &value))
> - return git_config_string(dest, key, value);
> + return git_config_string_or_die(dest, key, value);
> else
> return 1;
> }
>
> In the original API, git_config_string was called at parsing time, hence
> the file/line information was available through "cf". Here, we're
> querying the cache which doesn't have this information yet.
>
> I initially thought that managing properly file/line information would
> be just an addition, but this example shows that it is actually needed
> to be feature-complete wrt the old API. And I think we should be
> feature-complete (i.e. make the code cleaner without harming the user).
>
> So, I think it now makes sense to resurect your "file line info" patch:
>
> http://article.gmane.org/gmane.comp.version-control.git/253123
>
> Now that the series is properly reviewed, avoid modifying existing
> patches as much as possible, and add these file/line info on top of the
> existing.
>
> I think you need to:
>
> 1) Modify the hashmap data structure and the code that fills it in to
>store the file/line info (already done in your previous WIP patch).
>
> 2) Add a by-address parameter to git_configset_get_value that allows the
>user to get the file and line information. In your previous patch,
>that would mean returning a pointer to the corresponding struct
>key_source.
Will this extra complexity be good for "git_configset_get_value"?
Instead can we provide a function like die_config(char *key)
which prints
die("bad config file line %d in %s", linenr, filename);?
A variation would be die_config_multi(char *key, char *value)
for multi valued keys.
> 3) Pass this information to git_config_string_or_die, and die with the
>right message (with a helper like die_config(struct key_source *ks)
>that takes care of the formatting)
No need for passing if we use the above method. We will just call die_config()
inside it for NULL values
> 4) apply the same to git_config_get_.
>
> I'd actually add a step 0) before that: add a test that checks your
> behavior change. The test should pass without your patches, and fail
> with your current patch. Then, it should pass again once you completed
> the work.
>
Noted, I will add it.
> On a side note, re-reading your previous patch, I found this which
> sounds suspicious:
>
> + struct config_hash_entry *e;
> + struct string_list_item *si;
> + struct key_source *ks = xmalloc(sizeof(*e));
>
> Didn't you mean xmalloc(sizeof(*ks))?
>
Yikes, Thanks.
--
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 v3 0/6] git_config callers rewritten with the new config cache API
Tanay Abhra writes:
> [PATCH v3]: Most of Eric's suggestions has been implemented. See [2] for
> discussion.
> Also, new helpers introduced in v7 of the config-set API series have
> been used.
> See [1] for the documentation of the new functions.
>
> This series builds on the top of 5def4132 in pu or topic[1] in the mailing
> list
> with name "git config cache & special querying API utilizing the cache".
It's now called ta/config-set (see last "What's cooking in git.git").
> All patches pass every test, but there is a catch, there is slight behaviour
> change in most of them where originally the callback returns
> config_error_nonbool() when it sees a NULL value for a key causing a die
> specified in git_parse_source in config.c.
>
> The die also prints the file name and the line number as,
>
> "die("bad config file line %d in %s", cf->linenr, cf->name);"
>
> We lose the fine grained error checking when switching to this method.
I think a first step would be something like this:
--- a/config.c
+++ b/config.c
@@ -656,6 +656,15 @@ int git_config_string(const char **dest, const char *var,
const char *value)
return 0;
}
+// TODO: either make it static or export it properly
+int git_config_string_or_die(const char **dest, const char *var, const char
*value)
+{
+ if (git_config_string(dest, var, value) < 0)
+ die("bad config file (TODO: file/line info)");
+ else
+ return 0;
+}
+
int git_config_pathname(const char **dest, const char *var, const char *value)
{
if (!value)
@@ -1336,7 +1345,7 @@ int git_configset_get_string(struct config_set *cs, const
char *key, const char
{
const char *value;
if (!git_configset_get_value(cs, key, &value))
- return git_config_string(dest, key, value);
+ return git_config_string_or_die(dest, key, value);
else
return 1;
}
In the original API, git_config_string was called at parsing time, hence
the file/line information was available through "cf". Here, we're
querying the cache which doesn't have this information yet.
I initially thought that managing properly file/line information would
be just an addition, but this example shows that it is actually needed
to be feature-complete wrt the old API. And I think we should be
feature-complete (i.e. make the code cleaner without harming the user).
So, I think it now makes sense to resurect your "file line info" patch:
http://article.gmane.org/gmane.comp.version-control.git/253123
Now that the series is properly reviewed, avoid modifying existing
patches as much as possible, and add these file/line info on top of the
existing.
I think you need to:
1) Modify the hashmap data structure and the code that fills it in to
store the file/line info (already done in your previous WIP patch).
2) Add a by-address parameter to git_configset_get_value that allows the
user to get the file and line information. In your previous patch,
that would mean returning a pointer to the corresponding struct
key_source.
3) Pass this information to git_config_string_or_die, and die with the
right message (with a helper like die_config(struct key_source *ks)
that takes care of the formatting)
4) apply the same to git_config_get_.
I'd actually add a step 0) before that: add a test that checks your
behavior change. The test should pass without your patches, and fail
with your current patch. Then, it should pass again once you completed
the work.
On a side note, re-reading your previous patch, I found this which
sounds suspicious:
+ struct config_hash_entry *e;
+ struct string_list_item *si;
+ struct key_source *ks = xmalloc(sizeof(*e));
Didn't you mean xmalloc(sizeof(*ks))?
^^
--
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
[PATCH v3 0/6] git_config callers rewritten with the new config cache API
[PATCH v3]: Most of Eric's suggestions has been implemented. See [2] for
discussion.
Also, new helpers introduced in v7 of the config-set API series have
been used.
See [1] for the documentation of the new functions.
This series builds on the top of 5def4132 in pu or topic[1] in the mailing list
with name "git config cache & special querying API utilizing the cache".
All patches pass every test, but there is a catch, there is slight behaviour
change in most of them where originally the callback returns
config_error_nonbool() when it sees a NULL value for a key causing a die
specified in git_parse_source in config.c.
The die also prints the file name and the line number as,
"die("bad config file line %d in %s", cf->linenr, cf->name);"
We lose the fine grained error checking when switching to this method.
Still, I will try to correct this anomaly in my next series.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/253862
[2]: http://thread.gmane.org/gmane.comp.version-control.git/252334
Tanay Abhra (6):
alias.c | 27 +++
branch.c | 24
imap-send.c | 41 +++--
notes-utils.c | 33 -
notes.c | 21 +++--
pager.c | 40 +---
6 files changed, 62 insertions(+), 124 deletions(-)
--
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
