From: Matthieu Moy <Matthieu.Moy@grenoble-inp•fr>
To: Tanay Abhra <tanayabh@gmail•com>
Cc: git@vger•kernel.org, Ramkumar Ramachandra <artagnon@gmail•com>
Subject: Re: [PATCH v3 0/6] git_config callers rewritten with the new config cache API
Date: Mon, 21 Jul 2014 14:51:24 +0200 [thread overview]
Message-ID: <vpqegxeeuyb.fsf@anie.imag.fr> (raw)
In-Reply-To: <1405941145-12120-1-git-send-email-tanayabh@gmail.com> (Tanay Abhra's message of "Mon, 21 Jul 2014 04:12:19 -0700")
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/
next prev parent reply other threads:[~2014-07-21 12:51 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-21 11:12 [PATCH v3 0/6] git_config callers rewritten with the new config cache API Tanay Abhra
2014-07-21 11:12 ` [PATCH v3 1/6] alias.c: replace `git_config()` with `git_config_get_string()` Tanay Abhra
2014-07-21 12:52 ` Matthieu Moy
2014-07-21 13:43 ` Ramsay Jones
2014-07-31 17:13 ` Samuel Bronson
2014-07-21 11:12 ` [PATCH v3 2/6] branch.c: " Tanay Abhra
2014-07-21 17:59 ` Junio C Hamano
2014-07-21 18:06 ` Matthieu Moy
2014-07-21 18:11 ` Tanay Abhra
2014-07-21 11:12 ` [PATCH v3 3/6] imap-send.c: replace `git_config()` with `git_config_get_*()` family Tanay Abhra
2014-07-21 18:00 ` Junio C Hamano
2014-07-21 11:12 ` [PATCH v3 4/6] notes.c: replace `git_config()` with `git_config_get_value()` Tanay Abhra
2014-07-21 11:12 ` [PATCH v3 5/6] pager.c: " Tanay Abhra
2014-07-21 11:12 ` [PATCH v3 6/6] notes-util.c: " Tanay Abhra
2014-07-22 11:23 ` Jeff King
2014-07-21 11:44 ` [PATCH/RFC] rewrite `git_default_config()` using config-set API functions Tanay Abhra
2014-07-21 13:43 ` Matthieu Moy
2014-07-21 14:23 ` Tanay Abhra
2014-07-21 15:37 ` Matthieu Moy
2014-07-21 18:07 ` Junio C Hamano
2014-07-22 7:55 ` Matthieu Moy
2014-07-21 13:59 ` Ramsay Jones
2014-07-21 12:51 ` Matthieu Moy [this message]
2014-07-21 13:15 ` [PATCH v3 0/6] git_config callers rewritten with the new config cache API Tanay Abhra
2014-07-21 13:45 ` Matthieu Moy
2014-07-21 14:00 ` Tanay Abhra
2014-07-21 14:27 ` Matthieu Moy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=vpqegxeeuyb.fsf@anie.imag.fr \
--to=matthieu.moy@grenoble-inp$(echo .)fr \
--cc=artagnon@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=tanayabh@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox