Re: [PATCH v3 0/6] git_config callers rewritten with the new config cache API

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Tanay Abhra <tanayabh@gmail.com> 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_<other than string>.

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/