Re: [ANNOUNCE] Git v2.16.0-rc2 - breakages in t1308 and 1404
On Fri, Jan 12, 2018 at 5:27 PM, Junio C Hamano <gits...@pobox.com> wrote: > > "Randall S. Becker" <rsbec...@nexbridge.com> writes: > > > Sadly, fixing the "except" thing causes the test to break now. > > That is exactly what I wanted to say. If you want to "fix" it, > you'd need to figure out what the author of the "except" thing > wanted to test, adjust the args given to test-config (it cannot be > the same as the test-config invocation of the previous test), and > then fix the typo s/except/expect/. Changing the typo alone *will* > of course make the test fail, because then the file with the > corrected name, i.e. "expect", has bogus lines that does not match > how the current invocation of "test-config" command is expected to > output. Hi Guys, I was the original author of the test, I am sorry about the typo. I will submit a patch fixing the test. The fix can be checked at https://github.com/git/git/pull/451. 'configset_get_value' will be changed to 'configset_get_value_multi' since the test expects a list of values instead of a single value. Thanks, Tanay Abhra
[PATCH v2] add a flag to supress errors in git_config_parse_key()
`git_config_parse_key()` is used to sanitize the input key.
Some callers of the function like `git_config_set_multivar_in_file()`
get the pre-sanitized key directly from the user so it becomes
necessary to raise an error specifying what went wrong when the entered
key is syntactically malformed.
Other callers like `configset_find_element()` get their keys from
the git itself so a return value signifying error would be enough.
The error output shown to the user is useless and confusing in this
case so add a flag to suppress errors in such cases.
Helped-by: Junio C Hamano gits...@pobox.com
Helped-by: Jeff King p...@peff.net
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
Hi Jeff,
I went through Junio's config guideline patch series
and the whole thread of underscore bug report and I also think
that pager.*.command is the right path to go.
If you want to relax the syntactic requirement (such as add '_' to
the current set of allowed chacters), I can work upon it but most of the
comments point that moving towards pager.*.command would be better.
p.s: I hope that I got the unsigned flag suggestion by Junio correctly.
-Tanay
builtin/config.c | 2 +-
cache.h | 4 +++-
config.c | 20 +---
t/t7006-pager.sh | 9 +
4 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index d32c532..326d3d3 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -200,7 +200,7 @@ static int get_value(const char *key_, const char *regex_)
goto free_strings;
}
} else {
- if (git_config_parse_key(key_, key, NULL)) {
+ if (git_config_parse_key(key_, key, NULL, 0)) {
ret = CONFIG_INVALID_KEY;
goto free_strings;
}
diff --git a/cache.h b/cache.h
index f704af5..9073ee2 100644
--- a/cache.h
+++ b/cache.h
@@ -1329,6 +1329,8 @@ extern int update_server_info(int);
#define CONFIG_REGEX_NONE ((void *)1)
+#define CONFIG_ERROR_QUIET 0x0001
+
struct git_config_source {
unsigned int use_stdin:1;
const char *file;
@@ -1358,7 +1360,7 @@ extern int git_config_string(const char **, const char *,
const char *);
extern int git_config_pathname(const char **, const char *, const char *);
extern int git_config_set_in_file(const char *, const char *, const char *);
extern int git_config_set(const char *, const char *);
-extern int git_config_parse_key(const char *, char **, int *);
+extern int git_config_parse_key(const char *, char **, int *, unsigned int);
extern int git_config_set_multivar(const char *, const char *, const char *,
int);
extern int git_config_set_multivar_in_file(const char *, const char *, const
char *, const char *, int);
extern int git_config_rename_section(const char *, const char *);
diff --git a/config.c b/config.c
index e5e64dc..7e23bb9 100644
--- a/config.c
+++ b/config.c
@@ -1309,7 +1309,7 @@ static struct config_set_element
*configset_find_element(struct config_set *cs,
* `key` may come from the user, so normalize it before using it
* for querying entries from the hashmap.
*/
- ret = git_config_parse_key(key, normalized_key, NULL);
+ ret = git_config_parse_key(key, normalized_key, NULL,
CONFIG_ERROR_QUIET);
if (ret)
return NULL;
@@ -1842,8 +1842,10 @@ int git_config_set(const char *key, const char *value)
* lowercase section and variable name
* baselen - pointer to int which will hold the length of the
* section + subsection part, can be NULL
+ * flags - toggle whether the function raises an error on a syntactically
+ * malformed key
*/
-int git_config_parse_key(const char *key, char **store_key, int *baselen_)
+int git_config_parse_key(const char *key, char **store_key, int *baselen_,
unsigned int flags)
{
int i, dot, baselen;
const char *last_dot = strrchr(key, '.');
@@ -1854,12 +1856,14 @@ int git_config_parse_key(const char *key, char
**store_key, int *baselen_)
*/
if (last_dot == NULL || last_dot == key) {
- error(key does not contain a section: %s, key);
+ if (!flags)
+ error(key does not contain a section: %s, key);
return -CONFIG_NO_SECTION_OR_NAME;
}
if (!last_dot[1]) {
- error(key does not contain variable name: %s, key);
+ if (!flags)
+ error(key does not contain variable name: %s, key);
return -CONFIG_NO_SECTION_OR_NAME;
}
@@ -1881,12 +1885,14 @@ int git_config_parse_key(const char *key, char
**store_key, int *baselen_)
if (!dot || i baselen) {
if (!iskeychar(c) ||
(i == baselen + 1 !isalpha(c))) {
- error(invalid key: %s, key
[PATCH] config: add show_err flag to git_config_parse_key()
`git_config_parse_key()` is used to sanitize the input key.
Some callers of the function like `git_config_set_multivar_in_file()`
get the per-sanitized key directly from the user so it becomes
necessary to raise an error specifying what went wrong when the entered
key is defective.
Other callers like `configset_find_element()` get their keys from
the git itself so a return value signifying error would be enough.
The error output shown to the user is useless and confusing in this
case so add a show_err flag to suppress errors in such cases.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
Hi,
I just saw your mail late in the night (I didn't had net for a week).
This patch just squelches the error message, I will take a better
look tomorrow morning.
-Tanay
builtin/config.c | 2 +-
cache.h | 2 +-
config.c | 19 ---
3 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 15a7bea..d5070d7 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -200,7 +200,7 @@ static int get_value(const char *key_, const char *regex_)
goto free_strings;
}
} else {
- if (git_config_parse_key(key_, key, NULL)) {
+ if (git_config_parse_key(key_, key, NULL, 1)) {
ret = CONFIG_INVALID_KEY;
goto free_strings;
}
diff --git a/cache.h b/cache.h
index f704af5..1c0914d 100644
--- a/cache.h
+++ b/cache.h
@@ -1358,7 +1358,7 @@ extern int git_config_string(const char **, const char *,
const char *);
extern int git_config_pathname(const char **, const char *, const char *);
extern int git_config_set_in_file(const char *, const char *, const char *);
extern int git_config_set(const char *, const char *);
-extern int git_config_parse_key(const char *, char **, int *);
+extern int git_config_parse_key(const char *, char **, int *, int);
extern int git_config_set_multivar(const char *, const char *, const char *,
int);
extern int git_config_set_multivar_in_file(const char *, const char *, const
char *, const char *, int);
extern int git_config_rename_section(const char *, const char *);
diff --git a/config.c b/config.c
index 752e2e2..074a671 100644
--- a/config.c
+++ b/config.c
@@ -1309,7 +1309,7 @@ static struct config_set_element
*configset_find_element(struct config_set *cs,
* `key` may come from the user, so normalize it before using it
* for querying entries from the hashmap.
*/
- ret = git_config_parse_key(key, normalized_key, NULL);
+ ret = git_config_parse_key(key, normalized_key, NULL, 0);
if (ret)
return NULL;
@@ -1842,8 +1842,9 @@ int git_config_set(const char *key, const char *value)
* lowercase section and variable name
* baselen - pointer to int which will hold the length of the
* section + subsection part, can be NULL
+ * show_err - toggle whether the function raises an error on a defective key
*/
-int git_config_parse_key(const char *key, char **store_key, int *baselen_)
+int git_config_parse_key(const char *key, char **store_key, int *baselen_, int
show_err)
{
int i, dot, baselen;
const char *last_dot = strrchr(key, '.');
@@ -1854,12 +1855,14 @@ int git_config_parse_key(const char *key, char
**store_key, int *baselen_)
*/
if (last_dot == NULL || last_dot == key) {
- error(key does not contain a section: %s, key);
+ if (show_err)
+ error(key does not contain a section: %s, key);
return -CONFIG_NO_SECTION_OR_NAME;
}
if (!last_dot[1]) {
- error(key does not contain variable name: %s, key);
+ if (show_err)
+ error(key does not contain variable name: %s, key);
return -CONFIG_NO_SECTION_OR_NAME;
}
@@ -1881,12 +1884,14 @@ int git_config_parse_key(const char *key, char
**store_key, int *baselen_)
if (!dot || i baselen) {
if (!iskeychar(c) ||
(i == baselen + 1 !isalpha(c))) {
- error(invalid key: %s, key);
+ if (show_err)
+ error(invalid key: %s, key);
goto out_free_ret_1;
}
c = tolower(c);
} else if (c == '\n') {
- error(invalid key (newline): %s, key);
+ if (show_err)
+ error(invalid key (newline): %s, key);
goto out_free_ret_1;
}
(*store_key)[i] = c;
@@ -1936,7 +1941,7 @@ int git_config_set_multivar_in_file(const char
*config_filename,
char *filename_buf = NULL;
/* parse-key returns negative; flip the sign
[PATCH] config: add show_err flag to git_config_parse_key()
`git_config_parse_key()` is used to sanitize the input key.
Some callers of the function like `git_config_set_multivar_in_file()`
get the pre-sanitized key directly from the user so it becomes
necessary to raise an error specifying what went wrong when the entered
key is defective.
Other callers like `configset_find_element()` get their keys from
the git itself so a return value signifying error would be enough.
The error output shown to the user is useless and confusing in this
case so add a show_err flag to suppress errors in such cases.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
Hi,
You were right, one of the functions was calling git_config_parse_key()
which was leaking errors to the console. git_config_parse_key() was
meant for sanitizing user provided keys only but it was being used
internally in a place where only a return value would be enough.
Thanks for bringing this to our attention.
Cheers,
Tanay Abhra.
builtin/config.c | 2 +-
cache.h | 2 +-
config.c | 19 ---
3 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 8cc2604..51635dc 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -200,7 +200,7 @@ static int get_value(const char *key_, const char *regex_)
goto free_strings;
}
} else {
-if (git_config_parse_key(key_, key, NULL)) {
+if (git_config_parse_key(key_, key, NULL, 1)) {
ret = CONFIG_INVALID_KEY;
goto free_strings;
}
diff --git a/cache.h b/cache.h
index 99ed096..8129590 100644
--- a/cache.h
+++ b/cache.h
@@ -1362,7 +1362,7 @@ extern int git_config_string(const char **,
const char *, const char *);
extern int git_config_pathname(const char **, const char *, const char *);
extern int git_config_set_in_file(const char *, const char *, const char *);
extern int git_config_set(const char *, const char *);
-extern int git_config_parse_key(const char *, char **, int *);
+extern int git_config_parse_key(const char *, char **, int *, int);
extern int git_config_set_multivar(const char *, const char *, const
char *, int);
extern int git_config_set_multivar_in_file(const char *, const char
*, const char *, const char *, int);
extern int git_config_rename_section(const char *, const char *);
diff --git a/config.c b/config.c
index 15a2983..eb9058c 100644
--- a/config.c
+++ b/config.c
@@ -1299,7 +1299,7 @@ static struct config_set_element
*configset_find_element(struct config_set *cs,
* `key` may come from the user, so normalize it before using it
* for querying entries from the hashmap.
*/
-ret = git_config_parse_key(key, normalized_key, NULL);
+ret = git_config_parse_key(key, normalized_key, NULL, 0);
if (ret)
return NULL;
@@ -1832,8 +1832,9 @@ int git_config_set(const char *key, const char *value)
* lowercase section and variable name
* baselen - pointer to int which will hold the length of the
* section + subsection part, can be NULL
+ * show_err - toggle whether the function raises an error on a defective key
*/
-int git_config_parse_key(const char *key, char **store_key, int *baselen_)
+int git_config_parse_key(const char *key, char **store_key, int
*baselen_, int show_err)
{
int i, dot, baselen;
const char *last_dot = strrchr(key, '.');
@@ -1844,12 +1845,14 @@ int git_config_parse_key(const char *key, char
**store_key, int *baselen_)
*/
if (last_dot == NULL || last_dot == key) {
-error(key does not contain a section: %s, key);
+if (show_err)
+error(key does not contain a section: %s, key);
return -CONFIG_NO_SECTION_OR_NAME;
}
if (!last_dot[1]) {
-error(key does not contain variable name: %s, key);
+if (show_err)
+error(key does not contain variable name: %s, key);
return -CONFIG_NO_SECTION_OR_NAME;
}
@@ -1871,12 +1874,14 @@ int git_config_parse_key(const char *key, char
**store_key, int *baselen_)
if (!dot || i baselen) {
if (!iskeychar(c) ||
(i == baselen + 1 !isalpha(c))) {
-error(invalid key: %s, key);
+if (show_err)
+error(invalid key: %s, key);
goto out_free_ret_1;
}
c = tolower(c);
} else if (c == '\n') {
-error(invalid key (newline): %s, key);
+if (show_err)
+error(invalid key (newline): %s, key);
goto out_free_ret_1;
}
(*store_key)[i] = c;
@@ -1926,7 +1931,7 @@ int git_config_set_multivar_in_file(const char
*config_filename,
char *filename_buf = NULL;
/* parse-key returns negative; flip the sign to feed exit(3) */
-ret = 0 - git_config_parse_key(key, store.key, store.baselen);
+ret = 0 - git_config_parse_key(key, store.key, store.baselen, 1);
if (ret)
goto out_free
Re: [PATCH] config: add show_err flag to git_config_parse_key()
--- Hi, You were right, one of the functions was calling git_config_parse_key() which was leaking errors to the console. git_config_parse_key() was meant for sanitizing user provided keys only but it was being used internally in a place where only a return value would be enough. Thanks for bringing this to our attention. Cheers, Tanay Abhra. Who are *you* in the above, and what was the bug report about (if it was a bug report)? Perhaps summarize it in a form of a few new tests in t/t13XX series is in order? Thanks. Sorry about that, I am behind a firewall and had to use the gmail web interface. The patches are butchered, I will send new ones with a proper connection tomorrow. The original bug report is at [1]. http://thread.gmane.org/gmane.comp.version-control.git/258886 -- 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: [bug] [UX] `stash save --untracked` produces a stash that *looks* empty
On 10/5/2014 10:58 PM, Alberto Scotto wrote:
Hi all,
I've just found that:
- given you have an empty staging area
- and you have only untracked files in your working dir
- when you do `git stash --untracked`
- then `git stash show` gives you an empty output = stash looks empty
My first thought was oh god, my files are lost!
Second thought: Jeez I found a bug in git! cool!
Then I found that actually `git stash apply` restores the apparently lost
files
So I think it's a UX issue.
It cost me a few lost files already, as I thought an empty stash? uhm..
can't remember what/when I stashed.. whatever.. let's just delete it and
clean up a little bit this mess of stashes.
Here are the reproducible steps:
1. create new fresh git repo in $REPO_DIR
2. create a couple of files/dirs and commit
3. edit src/MyClass.java and commit
4. create dir src/new-dir with one file inside
5. edit file.txt and stage it
6. stash = stashes staged changes; only untracked files are left
7. stash -u = stashes untracked changes = working dir is clean
8. stash list
9. git stash show -p = empty output
10. git stash apply (restore stashed untracked files)
Hi,
I think problem lies with show_stash() which just shows the
diff between working tree and the base tree, it ignores the
untracked files. A quick and dirty fix can be to just show
the diff between the untracked files and a NULL commit.
Here's the patch, it works all right but can be implemented
much better. I will try to find a better approach tomorrow.
diff --git a/git-stash.sh b/git-stash.sh
index d4cf818..7088584 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -304,6 +304,8 @@ show_stash () {
assert_stash_like $@
git diff ${FLAGS:---stat} $b_commit $w_commit
+ empty_tree=$(git hash-object -t tree /dev/null)
+ git diff ${FLAGS:---stat} ${empty_tree} $u_commit
}
#
Cheers,
Tanay
--
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: [BUG?] `config branch.autosetuprebase true` breaks `rev-parse --is-inside-work-tree`
On 10/8/2014 11:05 PM, Junio C Hamano wrote: Richard Hartmann richih.mailingl...@gmail.com writes: So this is not a real bug report, more of a is this intended this way? richih@titanium ~/git_test % git rev-parse --is-inside-work-tree error: Malformed value for branch.autosetuprebase fatal: bad config file line 8 in .git/config richih@titanium ~/git_test % cat .git/config ... [branch] autosetuprebase = true It does not seem to be limited to rev-parse but having a malformed configuration for that variable would break everything Git, which certainly is not how it is supposed to work. It also seems that the breakage dates back very far into the past (I checked 1.7.0 and it seems to be broken the same way). The same breakage exists for branch.autosetupmerge, I think, e.g. [branch] autosetupmerge = garbage In config.c, git_default_branch_config() must be corrected to set git_branch_track and autorebase to BRANCH_TRACK_MALFORMED and AUTOREBASE_MALFORMED and the users of these two variables must be fixed to deal with the malformed in the configuration cases, I think. The error should happen only in the codepath where we need the value, and no other places. Supporting Junio's claim, there is a function called git_default_config() which checks and sets a whole load of config values which may or maynot be relevant to the codepath that called it. (branch.autosetuprebase is a part of it) So an error may occur printing a seemingly unrelated config value as the malformed variable as happened in your case. There are currently 72 callers of git_default_config() in the codebase, so a malformed config value breaks most of git commands. The only path to correct this behavior would be either correct the config variable in the file or we could decouple the huge monolithic function that git_default_config() has become and use the git_config_get_value() in the code paths that really need them. This part is doable, albeit slowly. All the config variables in git_default_config() can be rewritten using the new non callback based functions easily as demonstrated in an earlier RFC patch. -- 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/RFC 5/5] add tests for checking the behaviour of unset.variable
On 10/4/2014 1:42 AM, Junio C Hamano wrote: Matthieu Moy matthieu@grenoble-inp.fr writes: Junio C Hamano gits...@pobox.com writes: Well, the normal use-case for unset.variable is to put it in a local config file, to unset a variable set in another, lower-priority file. I agree that is one major use case. This common use-case works with the command-line git config, and it would be a pity to forbid the common use-case because of a particular, unusual case. Either you are being incoherent or I am not reading you right. If you said If this common use-case worked with the command-line 'git config', it would be nice, but it would be a pity because it does not, I would understand. I think you missed the another in my sentence above. The normal use-case is to have foo.bar and unset.variable=foo.bar in different files. In this case, you do not care about the position in file. I didn't miss anything. The reason you want to have unset foo.bar in your .git/config is to override a foo.bar = z you have in another place, e.g. ~/.gitconfig; which would let you sanely do another foo.bar = x in .git/config for multi-value foo.bar, *if* the unset comes before foo.bar. But what if you have this in your .git/config file [core] bare = false ... standard stuff left by git init ... [foo] bar = x before you add unset foo.bar there? And that is not a contribed example. The likely reason why you want to resort to unset foo.bar is that you found that you get an unwanted foo.bar=z in addition to desired foo.bar=z in a repository that has the above in .git/config, and at that point you would want to say I want to unset the outside influence. And there is no [unset] variable = foo.bar in there yet; without some special casing, if you treated unset.variable as if it were just another ordinary variable, it would go after you define your own foo.bar=x, which is not what you want. I have made some conclusions after reading the whole thread, 1 Add some tests for this use case which seems the most appropriate use case for this feature, (Copied from Junio's mail) - Define [xyzzy] frotz 1 in $HOME/.gitconfig (I think $HOME defaults to your trash directory). - Verify that git config xyzzy.frotz gives 1. - Define [unset] variable = xyzzy.frotz in .git/config (it is OK to use git config unset.variable xyzzy.frotz here). - Verify that git config xyzzy.frotz does not find anything. - Define [xyzzy] frotz 2 in .git/config (again, it is OK to use git config xyzzy.frotz 2 here). - Verify that git config xyzzy.frotz gives 2. 2 Leave the internal implementation as it is, as it helps in manually writing unset.variable in its appropriate place by using an editor, i.e this use case, [unset] variable = ... nullify some /etc/gitconfig values ... [include] path = ~/.gitcommon [unset] variable = ... nullify some ~/.gitcommon values ... [xyzzy] frotz = nitfol 3 Special case unset.variable, so that git config unset.variable foo.baz pastes it on the top of the file. The implementation should be trivial, but, Junio had said in an earlier mail that he doesn't think this approach would do much good. Other than this approach, Junio had suggested to append it after the last mention of foo.baz, but I don't think if it would be worth it, but still I will cook up the code for it. 4 Change the name to config.unset or something. I will make the above changes in the next revision. 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/RFC 5/5] add tests for checking the behaviour of unset.variable
On 10/7/2014 12:58 AM, Junio C Hamano wrote: The point is to preventgit config --add foo.baz anothervalue starting from --- --- --- [foo] bar = some [unset] variable = foo.baz --- --- --- from adding foo.baz next to existing foo.bar. We would want to end up with --- --- --- [foo] bar = some [unset] variable = foo.baz [foo] baz = anothervalue --- --- --- So When dealing with foo.baz, ignore everything above the last unset.variable that unsets foo.baz is what I meant (and I think that is how I wrote). Yes, that was what I inferred too, I should have worded it more carefully, 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
[PATCH/RFC 0/5] add unset.variable for unsetting previously set variables
Hi, This series aims to add a method to filter previously set variables. The patch series can be best described by the 3/5 log message which I have pasted below verbatim. Add a new config variable unset.variable which unsets previously set variables. It affects `git_config()` and `git_config_get_*()` family of functions. It removes the matching variables from the `configset` which were added previously. Those matching variables which come after the unset.variable in parsing order will not be deleted and will be left untouched. It affects the result of git config -l and similar calls. It may be used in cases where the user can not access the config files, for example, the system wide config files may be only accessible to the system administrator. We can unset an unwanted variable declared in the system config file by using unset.variable in a local config file. for example, /etc/gitconfig may look like this, [foo] bar = baz in the repo config file, we will write, [unset] variable = foo.bar to unset foo.bar previously declared in system wide config file. Now, I have some points of contention which I like to clarify, 1 The name of the variable, I could not decide between unset.variable and config.unset, or may be some other name would be more appropriate. 2 It affects both the C git_config() calls and, git config shell invocations. Due to this some variables may be absent from the git config -l result which might confuse the user. 3 I also have an another implementation for this series which just marks the config variables instead of deleting them from the configset. This can be used to provide two versions of git_config(), one with filtered variables other without it. 4 While hacking on this series, I saw that git_config_int() does not print the file name of the invalid variable when values are fed by the configset. I will correct this regression in another patch. Cheers, Tanay Documentation/config.txt | 12 +++ config.c | 93 +--- t/t1300-repo-config.sh | 56 - 3 files changed, 139 insertions(+), 22 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
[PATCH/RFC 4/5] document the new unset.variable variable
Helped-by: Matthieu Moy matthieu@imag.fr Signed-off-by: Tanay Abhra tanay...@gmail.com --- Documentation/config.txt | 12 1 file changed, 12 insertions(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index 3b5b24a..7f36d35 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2382,6 +2382,18 @@ transfer.unpackLimit:: not set, the value of this variable is used instead. The default value is 100. +unset.variable:: + This variable can be used to unset previously set variables + which had been already declared in files of lower priority + or declared before in the same file. It does not unset + matching variables declared after its position in the file + or in files of higher priority. It can be used to unset + pesky variables declared in files which the user might not + be able to open due to not having the required security + privileges, for example, system wide configuration file + `/etc/gitconfig` which may be accessible to the system + administrator only. + uploadarchive.allowUnreachable:: If true, allow clients to use `git archive --remote` to request any tree, whether reachable from the ref tips or not. See the -- 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
[PATCH/RFC 2/5] make git_config_with_options() to use a configset
Make git_config_with_options() to use a configset to feed values
in the callback function. This change gives us the power to filter
variables we feed to the callback using custom constraints.
A slight behaviour change, git_config_int() loses the ability to
print the file name of the invalid variable while dying.
Helped-by: Matthieu Moy matthieu@imag.fr
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
config.c | 21 +++--
t/t1300-repo-config.sh | 2 +-
2 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/config.c b/config.c
index cb474b2..09cf009 100644
--- a/config.c
+++ b/config.c
@@ -1214,7 +1214,7 @@ int git_config_early(config_fn_t fn, void *data, const
char *repo_config)
return ret == 0 ? found : ret;
}
-int git_config_with_options(config_fn_t fn, void *data,
+static int git_config_with_options_raw(config_fn_t fn, void *data,
struct git_config_source *config_source,
int respect_includes)
{
@@ -1247,9 +1247,26 @@ int git_config_with_options(config_fn_t fn, void *data,
return ret;
}
+static int config_set_callback(const char *key, const char *value, void *cb);
+
+int git_config_with_options(config_fn_t fn, void *data,
+ struct git_config_source *config_source,
+ int respect_includes)
+{
+ int ret;
+ struct config_set options_config;
+ git_configset_init(options_config);
+ ret = git_config_with_options_raw(config_set_callback, options_config,
+ config_source, respect_includes);
+ if (ret = 0)
+ configset_iter(options_config, fn, data);
+ git_configset_clear(options_config);
+ return ret;
+}
+
static void git_config_raw(config_fn_t fn, void *data)
{
- if (git_config_with_options(fn, data, NULL, 1) 0)
+ if (git_config_with_options_raw(fn, data, NULL, 1) 0)
/*
* git_config_with_options() normally returns only
* positive values, as most errors are fatal, and
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 938fc8b..ce5ea01 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -678,7 +678,7 @@ test_expect_success 'invalid unit' '
git config aninvalid.unit actual
test_cmp expect actual
cat expect -\EOF
- fatal: bad numeric config value '\''1auto'\'' for
'\''aninvalid.unit'\'' in .git/config: invalid unit
+ fatal: bad numeric config value '\''1auto'\'' for
'\''aninvalid.unit'\'': invalid unit
EOF
test_must_fail git config --int --get aninvalid.unit 2actual
test_i18ncmp expect actual
--
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
[PATCH/RFC 3/5] add unset.variable for unsetting previously set variables
Add a new config variable unset.variable which unsets previously set
variables. It affects `git_config()` and `git_config_get_*()` family
of functions. It removes the matching variables from the `configset`
which were added previously. Those matching variables which come after
the unset.variable in parsing order will not be deleted and will
be left untouched.
It affects the result of git config -l and similar calls.
It may be used in cases where the user can not access the config files,
for example, the system wide config files may be only accessible to
the system administrator. We can unset an unwanted variable declared in
the system config file by using unset.variable in a local config file.
for example, /etc/gitconfig may look like this,
[foo]
bar = baz
in the repo config file, we will write,
[unset]
variable = foo.bar
to unset foo.bar previously declared in system wide config file.
Helped-by: Matthieu Moy matthieu@imag.fr
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
config.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/config.c b/config.c
index 09cf009..a80832d 100644
--- a/config.c
+++ b/config.c
@@ -1311,6 +1311,38 @@ static struct config_set_element
*configset_find_element(struct config_set *cs,
return found_entry;
}
+static void delete_config_variable(struct config_set *cs, const char *key,
const char *value)
+{
+ char *normalized_value;
+ struct config_set_element *e = NULL;
+ int ret, current = 0, updated = 0;
+ struct configset_list *list = cs-list;
+ /*
+* if we find a key value pair with key as unset.variable, unset all
variables
+* in the configset with keys equivalent to the value in
unset.variable.
+* unsetting a variable means that the variable is permanently deleted
from the
+* configset.
+*/
+ ret = git_config_parse_key(value, normalized_value, NULL);
+ if (!ret) {
+ /* first remove matching variables from the configset_list */
+ while (current list-nr) {
+ if (!strcmp(list-items[current].e-key,
normalized_value))
+ current++;
+ else
+ list-items[updated++] = list-items[current++];
+ }
+ list-nr = updated;
+ /* then delete the matching entry from the configset hashmap */
+ e = configset_find_element(cs, normalized_value);
+ if (e) {
+ free(e-key);
+ string_list_clear(e-value_list, 1);
+ hashmap_remove(cs-config_hash, e, NULL);
+ }
+ }
+}
+
static int configset_add_value(struct config_set *cs, const char *key, const
char *value)
{
struct config_set_element *e;
@@ -1331,6 +1363,8 @@ 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);
+ if (!strcmp(key, unset.variable))
+ delete_config_variable(cs, key, value);
ALLOC_GROW(cs-list.items, cs-list.nr + 1, cs-list.alloc);
l_item = cs-list.items[cs-list.nr++];
--
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
[PATCH/RFC 5/5] add tests for checking the behaviour of unset.variable
Helped-by: Matthieu Moy matthieu@imag.fr Signed-off-by: Tanay Abhra tanay...@gmail.com --- t/t1300-repo-config.sh | 54 ++ 1 file changed, 54 insertions(+) diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index ce5ea01..f75c001 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -1179,4 +1179,58 @@ test_expect_success POSIXPERM,PERL 'preserves existing permissions' ' die q(badrename) if ((stat(q(.git/config)))[2] 0) != 0600 ' +test_expect_success 'unset.variable unsets all previous matching keys' ' + cat .git/config -\EOF + [alias] + checkconfig = -c foo.check=baz config foo.check + checkconfig = -c foo.check=bar config foo.check + [unset] + variable = alias.checkconfig + EOF + + test_expect_code 1 git checkconfig +' + +test_expect_success 'unset.variable does not touch all matching keys after it' ' + cat .git/config -\EOF + [alias] + checkconfig = -c foo.check=foo config foo.check + [unset] + variable = alias.checkconfig + [alias] + checkconfig = -c foo.check=baz config foo.check + checkconfig = -c foo.check=bar config foo.check + EOF + + cat expect -\EOF + bar + EOF + + test_expect_code 0 git checkconfig actual + test_cmp expect actual +' + +test_expect_success 'document how unset.variable will behave in shell scripts' ' + rm -f .git/config + cat expect -\EOF + EOF + git config foo.bar boz1 + git config --add foo.bar boz2 + git config unset.variable foo.bar + git config --add foo.bar boz3 + test_must_fail git config --get-all foo.bar actual + test_cmp expect actual +' + +test_expect_success 'unset.variable declared after in shell scripts' ' + rm -f .git/config + cat expect -\EOF + EOF + git config foo.bar boz1 + git config --add foo.bar boz2 + git config unset.variable foo.bar + test_must_fail git config --get-all foo.bar actual + test_cmp expect actual +' + test_done -- 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
[PATCH/RFC 1/5] config.c : move configset_iter() to an appropriate position
Move configset_iter() to an appropriate position where it
can be called by git_config_*() family without putting
a forward declaration for it.
Helped-by: Matthieu Moy matthieu@imag.fr
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
config.c | 38 +++---
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/config.c b/config.c
index a677eb6..cb474b2 100644
--- a/config.c
+++ b/config.c
@@ -1150,6 +1150,25 @@ int git_config_system(void)
return !git_env_bool(GIT_CONFIG_NOSYSTEM, 0);
}
+static void 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;
+ git_die_config_linenr(entry-key, kv_info-filename,
kv_info-linenr);
+ }
+ }
+}
+
int git_config_early(config_fn_t fn, void *data, const char *repo_config)
{
int ret = 0, found = 0;
@@ -1245,25 +1264,6 @@ static void git_config_raw(config_fn_t fn, void *data)
die(_(unknown error occured while reading the configuration
files));
}
-static void 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;
- git_die_config_linenr(entry-key, kv_info-filename,
kv_info-linenr);
- }
- }
-}
-
static void git_config_check_init(void);
void git_config(config_fn_t fn, void *data)
--
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
Re: [PATCH/RFC 5/5] add tests for checking the behaviour of unset.variable
On 10/3/2014 1:39 AM, Junio C Hamano wrote: Tanay Abhra tanay...@gmail.com writes: +test_expect_success 'document how unset.variable will behave in shell scripts' ' +rm -f .git/config +cat expect -\EOF +EOF +git config foo.bar boz1 +git config --add foo.bar boz2 +git config unset.variable foo.bar +git config --add foo.bar boz3 +test_must_fail git config --get-all foo.bar actual You make foo.bar a multi-valued one, then you unset it, so I would imagine that the value given after that, 'boz3', would be the only value foo.bar has. Why should --get-all fail? I am having a hard time imagining how this behaviour can make any sense. git config -add appends the value to a existing header, after these two commands have executed the config file would look like, git config foo.bar boz1 git config --add foo.bar boz2 [foo] bar = boz1 bar = boz2 After git config unset.variable foo.bar, [foo] bar = boz1 bar = boz2 [unset] variable = foo.bar Now the tricky part, git config --add foo.bar boz3 append to the existing header, [foo] bar = boz1 bar = boz2 bar = boz3 [unset] variable = foo.bar Since unset.variable unsets all previous set values in parsing order, git config --get-all foo.bar gives us nothing in result. -- 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/RFC 5/5] add tests for checking the behaviour of unset.variable
On 10/3/2014 1:53 AM, Junio C Hamano wrote: Tanay Abhra tanay...@gmail.com writes: On 10/3/2014 1:39 AM, Junio C Hamano wrote: Tanay Abhra tanay...@gmail.com writes: +test_expect_success 'document how unset.variable will behave in shell scripts' ' + rm -f .git/config + cat expect -\EOF + EOF + git config foo.bar boz1 + git config --add foo.bar boz2 + git config unset.variable foo.bar + git config --add foo.bar boz3 + test_must_fail git config --get-all foo.bar actual You make foo.bar a multi-valued one, then you unset it, so I would imagine that the value given after that, 'boz3', would be the only value foo.bar has. Why should --get-all fail? I am having a hard time imagining how this behaviour can make any sense. git config -add appends the value to a existing header, after these two commands have executed the config file would look like, ... I *know* how it is implemented before the changes of this series. And if the original implementation of add is left as-is, I can imagine how such a behaviour that is unintuitive to end-users can arise. I was and am having a hard time how this behaviour can make any sense from an end-user's point of view. That's what I was trying to document. I think that it makes no sense to use the feature as I had shown above. I had envisioned unset.variable to be explicitly typed in on the appropriate place as can be seen in the first two tests but Matthieu had suggested to add tests using git config too. This is when I had discovered these inconsistencies. I can think of two solutions, one leave it as it is and advertise it to be explicitly typed in the config files at the appropriate position or to change the behavior of unset.variable to unset all matching variables in that file, before and after. We could also change git config --add to append at the end of the file regardless the variable exists or not. Which course of action do you think would be best? -- 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 v2 1/2] document irregular config --add behaviour for empty and NULL values
If we have a config file like, [foo] baz bar = and we try something like, git config --add foo.baz roll, Git will segfault. Moreover, for git config --add foo.bar roll, it will overwrite the original value instead of appending after the existing empty value. Document these deficiencies in form of a test. Helped-by: Junio C Hamano gits...@pobox.com Signed-off-by: Tanay Abhra tanay...@gmail.com --- Sorry for this very late reply. I was stuck in a flood affected region with no internet connectivity for the past week. I am safe now. :) FWIW, here is the reroll with a set bit in the store struct and an exported global. I could have done the reroll as you have done, but Jeff had mentioned that he liked the version with a bit flag more. But you can choose the version that seems better to you. t/t1303-wacky-config.sh | 20 1 file changed, 20 insertions(+) diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh index 3a2c819..e5c0f07 100755 --- a/t/t1303-wacky-config.sh +++ b/t/t1303-wacky-config.sh @@ -111,4 +111,24 @@ test_expect_success 'unset many entries' ' test_must_fail git config section.key ' +test_expect_failure '--add appends new value after existing empty value' ' + cat expect -\EOF + + + fool + roll + EOF + cp .git/config .git/config.old + test_when_finished mv .git/config.old .git/config + cat .git/config -\EOF + [foo] + baz + baz = + baz = fool + EOF + git config --add foo.baz roll + git config --get-all foo.baz output + test_cmp expect output +' + test_done -- 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
[PATCH v2 2/2] make config --add behave correctly for empty and NULL values
The problem lies with the regexp used for simulating --add in
`git_config_set_multivar_in_file()`, ^$, which in ideal case should
not match with any string but is true for empty strings. Instead use a
sentinel value CONFIG_REGEX_NONE to say we do not want to replace any
existing entry and use it in the implementation of git config --add.
For removing the segfault add a check for NULL values in `matches()` in
config.c.
Helped-by: Junio C Hamano gits...@pobox.com
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
builtin/config.c| 2 +-
cache.h | 2 ++
config.c| 21 +
t/t1303-wacky-config.sh | 2 +-
4 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index aba7135..195664b 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -611,7 +611,7 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
check_argc(argc, 2, 2);
value = normalize_value(argv[0], argv[1]);
return git_config_set_multivar_in_file(given_config_source.file,
- argv[0], value, ^$, 0);
+ argv[0], value,
CONFIG_REGEX_NONE, 0);
}
else if (actions == ACTION_REPLACE_ALL) {
check_write();
diff --git a/cache.h b/cache.h
index dfa1a56..a09217d 100644
--- a/cache.h
+++ b/cache.h
@@ -1284,6 +1284,8 @@ extern int update_server_info(int);
#define CONFIG_INVALID_PATTERN 6
#define CONFIG_GENERIC_ERROR 7
+extern const char CONFIG_REGEX_NONE[];
+
struct git_config_source {
unsigned int use_stdin:1;
const char *file;
diff --git a/config.c b/config.c
index 83c913a..20476e0 100644
--- a/config.c
+++ b/config.c
@@ -46,6 +46,8 @@ static int zlib_compression_seen;
*/
static struct config_set the_config_set;
+const char CONFIG_REGEX_NONE[] = a^;
+
static int config_file_fgetc(struct config_source *conf)
{
return fgetc(conf-u.file);
@@ -1607,14 +1609,20 @@ static struct {
unsigned int offset_alloc;
enum { START, SECTION_SEEN, SECTION_END_SEEN, KEY_SEEN } state;
int seen;
+ unsigned value_never_matches:1;
} store;
static int matches(const char *key, const char *value)
{
- return !strcmp(key, store.key)
- (store.value_regex == NULL ||
-(store.do_not_match ^
- !regexec(store.value_regex, value, 0, NULL, 0)));
+ if (strcmp(key, store.key))
+ return 0; /* not ours */
+ if (!store.value_regex)
+ return 1; /* always matches */
+ if (store.value_never_matches)
+ return 0; /* never matches */
+
+ return store.do_not_match ^
+ (value !regexec(store.value_regex, value, 0, NULL, 0));
}
static int store_aux(const char *key, const char *value, void *cb)
@@ -1876,6 +1884,8 @@ out_free_ret_1:
/*
* If value==NULL, unset in (remove from) config,
* if value_regex!=NULL, disregard key/value pairs where value does not match.
+ * if value_regex==CONFIG_REGEX_NONE, do not match any existing values
+ * (only add a new one)
* if multi_replace==0, nothing, or only one matching key/value is replaced,
* else all matching key/values (regardless how many) are removed,
* before the new pair is written.
@@ -1966,6 +1976,9 @@ int git_config_set_multivar_in_file(const char
*config_filename,
} else
store.do_not_match = 0;
+ if (value_regex == CONFIG_REGEX_NONE)
+ store.value_never_matches = 1;
+
store.value_regex = (regex_t*)xmalloc(sizeof(regex_t));
if (regcomp(store.value_regex, value_regex,
REG_EXTENDED)) {
diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
index e5c0f07..3b92083 100755
--- a/t/t1303-wacky-config.sh
+++ b/t/t1303-wacky-config.sh
@@ -111,7 +111,7 @@ test_expect_success 'unset many entries' '
test_must_fail git config section.key
'
-test_expect_failure '--add appends new value after existing empty value' '
+test_expect_success '--add appends new value after existing empty value' '
cat expect -\EOF
--
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
[PATCH] make config --add behave correctly for empty and NULL values
Currently if we have a config file like,
[foo]
baz
bar =
and we try something like, git config --add foo.baz roll, Git will
segfault. Moreover, for git config --add foo.bar roll, it will
overwrite the original value instead of appending after the existing
empty value.
The problem lies with the regexp used for simulating --add in
`git_config_set_multivar_in_file()`, ^$, which in ideal case should
not match with any string but is true for empty strings. Instead use a
regexp like a^ which can not be true for any string, empty or not.
For removing the segfault add a check for NULL values in `matches()` in
config.c.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
builtin/config.c| 2 +-
config.c| 2 +-
t/t1303-wacky-config.sh | 20
3 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index fcd8474..b9e7dce 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -586,7 +586,7 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
check_argc(argc, 2, 2);
value = normalize_value(argv[0], argv[1]);
return git_config_set_multivar_in_file(given_config_source.file,
- argv[0], value, ^$, 0);
+ argv[0], value, a^, 0);
}
else if (actions == ACTION_REPLACE_ALL) {
check_write();
diff --git a/config.c b/config.c
index 058505c..67a7729 100644
--- a/config.c
+++ b/config.c
@@ -1231,7 +1231,7 @@ static int matches(const char *key, const char *value)
return !strcmp(key, store.key)
(store.value_regex == NULL ||
(store.do_not_match ^
- !regexec(store.value_regex, value, 0, NULL, 0)));
+ (value !regexec(store.value_regex, value, 0, NULL, 0;
}
static int store_aux(const char *key, const char *value, void *cb)
diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
index 3a2c819..3b92083 100755
--- a/t/t1303-wacky-config.sh
+++ b/t/t1303-wacky-config.sh
@@ -111,4 +111,24 @@ test_expect_success 'unset many entries' '
test_must_fail git config section.key
'
+test_expect_success '--add appends new value after existing empty value' '
+ cat expect -\EOF
+
+
+ fool
+ roll
+ EOF
+ cp .git/config .git/config.old
+ test_when_finished mv .git/config.old .git/config
+ cat .git/config -\EOF
+ [foo]
+ baz
+ baz =
+ baz = fool
+ EOF
+ git config --add foo.baz roll
+ git config --get-all foo.baz output
+ test_cmp expect output
+'
+
test_done
--
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
[PATCH 3/4] merge-recursive.c: replace `git_config()` with `git_config_get_int()`
Use `git_config_get_int()` instead of `git_config()` to take advantage
of the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
merge-recursive.c | 22 ++
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 1d332b8..8ab944c 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2026,22 +2026,12 @@ int merge_recursive_generic(struct merge_options *o,
return clean ? 0 : 1;
}
-static int merge_recursive_config(const char *var, const char *value, void *cb)
+static void merge_recursive_config(struct merge_options *o)
{
- struct merge_options *o = cb;
- if (!strcmp(var, merge.verbosity)) {
- o-verbosity = git_config_int(var, value);
- return 0;
- }
- if (!strcmp(var, diff.renamelimit)) {
- o-diff_rename_limit = git_config_int(var, value);
- return 0;
- }
- if (!strcmp(var, merge.renamelimit)) {
- o-merge_rename_limit = git_config_int(var, value);
- return 0;
- }
- return git_xmerge_config(var, value, cb);
+ git_config_get_int(merge.verbosity, o-verbosity);
+ git_config_get_int(diff.renamelimit, o-diff_rename_limit);
+ git_config_get_int(merge.renamelimit, o-merge_rename_limit);
+ git_config(git_xmerge_config, NULL);
}
void init_merge_options(struct merge_options *o)
@@ -2052,7 +2042,7 @@ void init_merge_options(struct merge_options *o)
o-diff_rename_limit = -1;
o-merge_rename_limit = -1;
o-renormalize = 0;
- git_config(merge_recursive_config, o);
+ merge_recursive_config(o);
if (getenv(GIT_MERGE_VERBOSITY))
o-verbosity =
strtol(getenv(GIT_MERGE_VERBOSITY), NULL, 10);
--
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
[PATCH 1/4] fast-import.c: replace `git_config()` with `git_config_get_*()` family
Use `git_config_get_*()` family instead of `git_config()` to take
advantage of the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
fast-import.c | 42 +++---
1 file changed, 19 insertions(+), 23 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index d73f58c..eca5ed4 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3274,36 +3274,32 @@ static void parse_option(const char *option)
die(This version of fast-import does not support option: %s, option);
}
-static int git_pack_config(const char *k, const char *v, void *cb)
+static void git_pack_config(void)
{
- if (!strcmp(k, pack.depth)) {
- max_depth = git_config_int(k, v);
+ int indexversion_value;
+ unsigned long packsizelimit_value;
+
+ if (!git_config_get_ulong(pack.depth, max_depth)) {
if (max_depth MAX_DEPTH)
max_depth = MAX_DEPTH;
- return 0;
}
- if (!strcmp(k, pack.compression)) {
- int level = git_config_int(k, v);
- if (level == -1)
- level = Z_DEFAULT_COMPRESSION;
- else if (level 0 || level Z_BEST_COMPRESSION)
- die(bad pack compression level %d, level);
- pack_compression_level = level;
+ if (!git_config_get_int(pack.compression, pack_compression_level)) {
+ if (pack_compression_level == -1)
+ pack_compression_level = Z_DEFAULT_COMPRESSION;
+ else if (pack_compression_level 0 ||
+pack_compression_level Z_BEST_COMPRESSION)
+ die(bad pack compression level %d,
pack_compression_level);
pack_compression_seen = 1;
- return 0;
}
- if (!strcmp(k, pack.indexversion)) {
- pack_idx_opts.version = git_config_int(k, v);
+ if (!git_config_get_int(pack.indexversion, indexversion_value)) {
+ pack_idx_opts.version = indexversion_value;
if (pack_idx_opts.version 2)
- die(bad pack.indexversion=%PRIu32,
- pack_idx_opts.version);
- return 0;
+ die(bad pack.indexversion=%PRIu32,
pack_idx_opts.version);
}
- if (!strcmp(k, pack.packsizelimit)) {
- max_packsize = git_config_ulong(k, v);
- return 0;
- }
- return git_default_config(k, v, cb);
+ if (!git_config_get_ulong(pack.packsizelimit, packsizelimit_value))
+ max_packsize = packsizelimit_value;
+
+ git_config(git_default_config, NULL);
}
static const char fast_import_usage[] =
@@ -3356,7 +3352,7 @@ int main(int argc, char **argv)
setup_git_directory();
reset_pack_idx_option(pack_idx_opts);
- git_config(git_pack_config, NULL);
+ git_pack_config();
if (!pack_compression_seen core_compression_seen)
pack_compression_level = core_compression_level;
--
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
[PATCH 2/4] ll-merge.c: refactor `read_merge_config()` to use `git_config_string()`
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
ll-merge.c | 23 ++-
1 file changed, 6 insertions(+), 17 deletions(-)
diff --git a/ll-merge.c b/ll-merge.c
index fb61ea6..8ea03e5 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -225,11 +225,8 @@ static int read_merge_config(const char *var, const char
*value, void *cb)
const char *key, *name;
int namelen;
- if (!strcmp(var, merge.default)) {
- if (value)
- default_ll_merge = xstrdup(value);
- return 0;
- }
+ if (!strcmp(var, merge.default))
+ return git_config_string(default_ll_merge, var, value);
/*
* We are not interested in anything but merge.name.variable;
@@ -254,12 +251,8 @@ static int read_merge_config(const char *var, const char
*value, void *cb)
ll_user_merge_tail = (fn-next);
}
- if (!strcmp(name, key)) {
- if (!value)
- return error(%s: lacks value, var);
- fn-description = xstrdup(value);
- return 0;
- }
+ if (!strcmp(name, key))
+ return git_config_string(fn-description, var, value);
if (!strcmp(driver, key)) {
if (!value)
@@ -285,12 +278,8 @@ static int read_merge_config(const char *var, const char
*value, void *cb)
return 0;
}
- if (!strcmp(recursive, key)) {
- if (!value)
- return error(%s: lacks value, var);
- fn-recursive = xstrdup(value);
- return 0;
- }
+ if (!strcmp(recursive, key))
+ return git_config_string(fn-recursive, var, value);
return 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
[PATCH 4/4] builtin/apply.c: replace `git_config()` with `git_config_get_string_const()`
Use `git_config_get_string_const()` instead of `git_config()` to take
advantage of the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
builtin/apply.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index be2b4ce..66acf32 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4269,13 +4269,11 @@ static int apply_patch(int fd, const char *filename,
int options)
return 0;
}
-static int git_apply_config(const char *var, const char *value, void *cb)
+static void git_apply_config(void)
{
- if (!strcmp(var, apply.whitespace))
- return git_config_string(apply_default_whitespace, var, value);
- else if (!strcmp(var, apply.ignorewhitespace))
- return git_config_string(apply_default_ignorewhitespace, var,
value);
- return git_default_config(var, value, cb);
+ git_config_get_string_const(apply.whitespace,
apply_default_whitespace);
+ git_config_get_string_const(apply.ignorewhitespace,
apply_default_ignorewhitespace);
+ git_config(git_default_config, NULL);
}
static int option_parse_exclude(const struct option *opt,
@@ -4423,7 +4421,7 @@ int cmd_apply(int argc, const char **argv, const char
*prefix_)
prefix = prefix_;
prefix_length = prefix ? strlen(prefix) : 0;
- git_config(git_apply_config, NULL);
+ git_apply_config();
if (apply_default_whitespace)
parse_whitespace_option(apply_default_whitespace);
if (apply_default_ignorewhitespace)
--
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
Re: [PATCH 1/4] fast-import.c: replace `git_config()` with `git_config_get_*()` family
if (pack_idx_opts.version 2) -die(bad pack.indexversion=%PRIu32, -pack_idx_opts.version); -return 0; +die(bad pack.indexversion=%PRIu32, pack_idx_opts.version); One more opportunity for git_die_config()? Yup, I had thought of changing that and above to git_die_config(), but didn't do it, will send a revised patch. Not that it's terribly important, but I think it's good that your refactoring also brings a few end-users benefits. It will help you show I have been rewriting callers and the law of diminishing returns kicked in. I had rewritten some other call sites but I didn't see them bringing any benefits (cleaner control flow, less lines, bugs eliminated), so I left them out. off when you tell your friends what you did this summer (not I did useless code churn ;-) ), and helps everybody see the benefits of your GSoC ;-). -- 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 v2 1/5] fast-import.c: replace `git_config()` with `git_config_get_*()` family
Use `git_config_get_*()` family instead of `git_config()` to take
advantage of the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
fast-import.c | 44 +---
1 file changed, 21 insertions(+), 23 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index d73f58c..34e780d 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3274,36 +3274,34 @@ static void parse_option(const char *option)
die(This version of fast-import does not support option: %s, option);
}
-static int git_pack_config(const char *k, const char *v, void *cb)
+static void git_pack_config(void)
{
- if (!strcmp(k, pack.depth)) {
- max_depth = git_config_int(k, v);
+ int indexversion_value;
+ unsigned long packsizelimit_value;
+
+ if (!git_config_get_ulong(pack.depth, max_depth)) {
if (max_depth MAX_DEPTH)
max_depth = MAX_DEPTH;
- return 0;
}
- if (!strcmp(k, pack.compression)) {
- int level = git_config_int(k, v);
- if (level == -1)
- level = Z_DEFAULT_COMPRESSION;
- else if (level 0 || level Z_BEST_COMPRESSION)
- die(bad pack compression level %d, level);
- pack_compression_level = level;
+ if (!git_config_get_int(pack.compression, pack_compression_level)) {
+ if (pack_compression_level == -1)
+ pack_compression_level = Z_DEFAULT_COMPRESSION;
+ else if (pack_compression_level 0 ||
+pack_compression_level Z_BEST_COMPRESSION)
+ git_die_config(pack.compression,
+ bad pack compression level %d,
pack_compression_level);
pack_compression_seen = 1;
- return 0;
}
- if (!strcmp(k, pack.indexversion)) {
- pack_idx_opts.version = git_config_int(k, v);
+ if (!git_config_get_int(pack.indexversion, indexversion_value)) {
+ pack_idx_opts.version = indexversion_value;
if (pack_idx_opts.version 2)
- die(bad pack.indexversion=%PRIu32,
- pack_idx_opts.version);
- return 0;
+ git_die_config(pack.indexversion,
+ bad pack.indexversion=%PRIu32,
pack_idx_opts.version);
}
- if (!strcmp(k, pack.packsizelimit)) {
- max_packsize = git_config_ulong(k, v);
- return 0;
- }
- return git_default_config(k, v, cb);
+ if (!git_config_get_ulong(pack.packsizelimit, packsizelimit_value))
+ max_packsize = packsizelimit_value;
+
+ git_config(git_default_config, NULL);
}
static const char fast_import_usage[] =
@@ -3356,7 +3354,7 @@ int main(int argc, char **argv)
setup_git_directory();
reset_pack_idx_option(pack_idx_opts);
- git_config(git_pack_config, NULL);
+ git_pack_config();
if (!pack_compression_seen core_compression_seen)
pack_compression_level = core_compression_level;
--
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
[PATCH v2 2/4] ll-merge.c: refactor `read_merge_config()` to use `git_config_string()`
There is one slight behavior change, previously merge.default
silently ignored a NULL value and didn't raise any error. But,
in the same function, all other values raise an error on a NULL
value. So to conform with other call sites in Git, a NULL value
for merge.default raises an error.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
We cannot easily use the new config-set API here, because
much of the function is dedicated to processing
merge.name.variable which does not easily translate to
the new API. If it were for variables like,
merge.summary, merge.tool, and merge.verbosity, we
could use the new API.
ll-merge.c | 23 ++-
1 file changed, 6 insertions(+), 17 deletions(-)
diff --git a/ll-merge.c b/ll-merge.c
index fb61ea6..8ea03e5 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -225,11 +225,8 @@ static int read_merge_config(const char *var, const char
*value, void *cb)
const char *key, *name;
int namelen;
- if (!strcmp(var, merge.default)) {
- if (value)
- default_ll_merge = xstrdup(value);
- return 0;
- }
+ if (!strcmp(var, merge.default))
+ return git_config_string(default_ll_merge, var, value);
/*
* We are not interested in anything but merge.name.variable;
@@ -254,12 +251,8 @@ static int read_merge_config(const char *var, const char
*value, void *cb)
ll_user_merge_tail = (fn-next);
}
- if (!strcmp(name, key)) {
- if (!value)
- return error(%s: lacks value, var);
- fn-description = xstrdup(value);
- return 0;
- }
+ if (!strcmp(name, key))
+ return git_config_string(fn-description, var, value);
if (!strcmp(driver, key)) {
if (!value)
@@ -285,12 +278,8 @@ static int read_merge_config(const char *var, const char
*value, void *cb)
return 0;
}
- if (!strcmp(recursive, key)) {
- if (!value)
- return error(%s: lacks value, var);
- fn-recursive = xstrdup(value);
- return 0;
- }
+ if (!strcmp(recursive, key))
+ return git_config_string(fn-recursive, var, value);
return 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
[PATCH/RFC v2 1/2] git_default_config() rewritten using the config-set API
git_default_config() now uses config-set API functions to query for
values.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
Sorry, for the short log message, I will explain why.
The git_default_config() rewrite is 100% complete, the only
problem remains is the call sites; there are too many of them.
Some are called from callback functions which pass the remaining
variables to git_default_config() which they do not have any use for.
Those call sites can remain as they are, because git_default_config()
is a single call function now, and is guarded by a sentinel value.
So after one call, it would just return immediately instead of going on
checking.
For callers like git_config(git_default_config, NULL) (there are 38 of them),
we can leave them as they are or rewrite them as I have illustrated in the
next attached patch.
I will take this series out of RFC as soon as we have decided what to do with
the call sites.
Cheers,
Tanay Abhra.
advice.c | 17 ++--
advice.h | 2 +-
cache.h | 2 +-
config.c | 346 ++-
ident.c | 17 ++--
5 files changed, 136 insertions(+), 248 deletions(-)
diff --git a/advice.c b/advice.c
index 9b42033..34e1ccc 100644
--- a/advice.c
+++ b/advice.c
@@ -59,22 +59,17 @@ void advise(const char *advice, ...)
strbuf_release(buf);
}
-int git_default_advice_config(const char *var, const char *value)
+void git_default_advice_config(void)
{
- const char *k;
+ struct strbuf var = STRBUF_INIT;
int i;
- if (!skip_prefix(var, advice., k))
- return 0;
-
for (i = 0; i ARRAY_SIZE(advice_config); i++) {
- if (strcmp(k, advice_config[i].name))
- continue;
- *advice_config[i].preference = git_config_bool(var, value);
- return 0;
+ strbuf_addf(var, advice.%s, advice_config[i].name);
+ git_config_get_bool(var.buf, advice_config[i].preference);
+ strbuf_reset(var);
}
-
- return 0;
+ strbuf_release(var);
}
int error_resolve_conflict(const char *me)
diff --git a/advice.h b/advice.h
index 5ecc6c1..5bfe46c 100644
--- a/advice.h
+++ b/advice.h
@@ -19,7 +19,7 @@ extern int advice_set_upstream_failure;
extern int advice_object_name_warning;
extern int advice_rm_hints;
-int git_default_advice_config(const char *var, const char *value);
+void git_default_advice_config(void);
__attribute__((format (printf, 1, 2)))
void advise(const char *advice, ...);
int error_resolve_conflict(const char *me);
diff --git a/cache.h b/cache.h
index 2693a37..fa28b40 100644
--- a/cache.h
+++ b/cache.h
@@ -1065,7 +1065,7 @@ extern const char *fmt_name(const char *name, const char
*email);
extern const char *ident_default_email(void);
extern const char *git_editor(void);
extern const char *git_pager(int stdout_is_tty);
-extern int git_ident_config(const char *, const char *, void *);
+extern void git_ident_config(void);
struct ident_split {
const char *name_begin;
diff --git a/config.c b/config.c
index 427850a..36b9124 100644
--- a/config.c
+++ b/config.c
@@ -46,6 +46,8 @@ static int zlib_compression_seen;
*/
static struct config_set the_config_set;
+static int default_config_loaded;
+
static int config_file_fgetc(struct config_source *conf)
{
return fgetc(conf-u.file);
@@ -670,147 +672,91 @@ int git_config_pathname(const char **dest, const char
*var, const char *value)
return 0;
}
-static int git_default_core_config(const char *var, const char *value)
+static void git_default_core_config(void)
{
+ const char *value = NULL;
+ unsigned long v_l = 0;
+ int abbrev;
+ const char *comment;
+
/* This needs a better name */
- if (!strcmp(var, core.filemode)) {
- trust_executable_bit = git_config_bool(var, value);
- return 0;
- }
- if (!strcmp(var, core.trustctime)) {
- trust_ctime = git_config_bool(var, value);
- return 0;
- }
- if (!strcmp(var, core.checkstat)) {
+ git_config_get_bool(core.filemode, trust_executable_bit);
+ git_config_get_bool(core.trustctime, trust_ctime);
+
+ if (!git_config_get_value(core.checkstat, value)) {
if (!strcasecmp(value, default))
check_stat = 1;
else if (!strcasecmp(value, minimal))
check_stat = 0;
}
- if (!strcmp(var, core.quotepath)) {
- quote_path_fully = git_config_bool(var, value);
- return 0;
- }
+ git_config_get_bool(core.quotepath, quote_path_fully);
+ git_config_get_bool(core.symlinks, has_symlinks);
+ git_config_get_bool(core.ignorecase, ignore_case);
+ git_config_get_pathname(core.attributesfile, git_attributes_file);
+ git_config_get_bool(core.bare, is_bare_repository_cfg);
+ git_config_get_bool
[PATCH/RFC v2 2/2] use the new git_default_config()
If we change the signature to void git_default_config(void),
we would have to use a patch like this to change the call sites
of the function. This patch is just for illustrative purpose,
I couldn't finalize if this was unnecessary code cruft or
a valid approach.
---
builtin/check-attr.c | 2 +-
builtin/check-ignore.c | 2 +-
builtin/check-mailmap.c | 2 +-
builtin/checkout-index.c | 2 +-
builtin/clone.c | 2 +-
builtin/config.c | 2 +-
builtin/describe.c | 2 +-
builtin/fast-export.c| 2 +-
builtin/for-each-ref.c | 2 +-
builtin/hash-object.c| 2 +-
builtin/init-db.c| 2 +-
builtin/ls-files.c | 2 +-
builtin/ls-tree.c| 2 +-
builtin/merge-base.c | 2 +-
builtin/mv.c | 2 +-
builtin/name-rev.c | 2 +-
builtin/notes.c | 2 +-
builtin/push.c | 2 +-
builtin/read-tree.c | 2 +-
builtin/reset.c | 2 +-
builtin/rev-list.c | 2 +-
builtin/rev-parse.c | 2 +-
builtin/revert.c | 4 ++--
builtin/rm.c | 2 +-
builtin/shortlog.c | 2 +-
builtin/stripspace.c | 2 +-
builtin/symbolic-ref.c | 2 +-
builtin/unpack-file.c| 2 +-
builtin/unpack-objects.c | 2 +-
builtin/update-index.c | 2 +-
builtin/update-ref.c | 2 +-
builtin/update-server-info.c | 2 +-
builtin/var.c| 2 +-
builtin/verify-pack.c| 2 +-
builtin/write-tree.c | 2 +-
http-fetch.c | 2 +-
pager.c | 2 +-
37 files changed, 38 insertions(+), 38 deletions(-)
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 5600ec3..e2d7826 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -105,7 +105,7 @@ int cmd_check_attr(int argc, const char **argv, const char
*prefix)
if (!is_bare_repository())
setup_work_tree();
- git_config(git_default_config, NULL);
+ git_default_config();
argc = parse_options(argc, argv, prefix, check_attr_options,
check_attr_usage, PARSE_OPT_KEEP_DASHDASH);
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 594463a..c14c977 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -144,7 +144,7 @@ int cmd_check_ignore(int argc, const char **argv, const
char *prefix)
int num_ignored;
struct dir_struct dir;
- git_config(git_default_config, NULL);
+ git_default_config();
argc = parse_options(argc, argv, prefix, check_ignore_options,
check_ignore_usage, 0);
diff --git a/builtin/check-mailmap.c b/builtin/check-mailmap.c
index 8f4d809..f9d0de6 100644
--- a/builtin/check-mailmap.c
+++ b/builtin/check-mailmap.c
@@ -40,7 +40,7 @@ int cmd_check_mailmap(int argc, const char **argv, const char
*prefix)
int i;
struct string_list mailmap = STRING_LIST_INIT_NODUP;
- git_config(git_default_config, NULL);
+ git_default_config();
argc = parse_options(argc, argv, prefix, check_mailmap_options,
check_mailmap_usage, 0);
if (argc == 0 !use_stdin)
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 05edd9e..197a987 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -213,7 +213,7 @@ int cmd_checkout_index(int argc, const char **argv, const
char *prefix)
if (argc == 2 !strcmp(argv[1], -h))
usage_with_options(builtin_checkout_index_usage,
builtin_checkout_index_options);
- git_config(git_default_config, NULL);
+ git_default_config();
state.base_dir = ;
prefix_length = prefix ? strlen(prefix) : 0;
diff --git a/builtin/clone.c b/builtin/clone.c
index bbd169c..bcfd322 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -864,7 +864,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
init_db(option_template, INIT_DB_QUIET);
write_config(option_config);
- git_config(git_default_config, NULL);
+ git_default_config();
if (option_bare) {
if (option_mirror)
diff --git a/builtin/config.c b/builtin/config.c
index fcd8474..eed430d 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -558,7 +558,7 @@ int cmd_config(int argc, const char **argv, const char
*prefix)
die(editing stdin is not supported);
if (given_config_source.blob)
die(editing blobs is not supported);
- git_config(git_default_config, NULL);
+ git_default_config();
launch_editor(given_config_source.file ?
given_config_source.file : git_path(config),
NULL, NULL);
diff --git a/builtin/describe.c b/builtin/describe.c
index
Re: [PATCH v9 5/8] config: add `git_die_config()` to the config-set API
On 8/8/2014 12:25 AM, Junio C Hamano wrote: Tanay Abhra tanay...@gmail.com writes: diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt index 21f280c..0d8b99b 100644 --- a/Documentation/technical/api-config.txt +++ b/Documentation/technical/api-config.txt @@ -155,6 +155,19 @@ 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. +`git_die_config(const char *key, const char *err, ...)`:: + +First prints the error message specified by the caller in `err` and then +dies printing the line number and the file name of the highest priority +value for the configuration variable `key`. Reviewed with a wider context, I notice that this entry alone lacks the return type. I am assuming that this is just an oversight, and adding 'void ' in front of the filename to match the next entry is simple enough. Yikes, yes, you are right, it's just an oversight. I will send an amended patch. -- 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 v8 0/8] Rewrite `git_config()` using config-set API
On 8/8/2014 2:01 AM, Junio C Hamano wrote:
Matthieu Moy matthieu@grenoble-inp.fr writes:
Why is this needed? Are you now using key_value_info outside config.c?
Or is it a leftover from a previous experiment?
Has this been resolved in the new round?
Tanay explained in another subthread why this was needed. For callers
iterating over the string_list who want to get the file/line info, they
need to be able to cast the void * pointer to struct key_value_info *.
For callers that want to see all the multi-values, it would be
preferrable for the iterator to pass the filename and the linenumber
to the callback function, instead of exposing its implementation
detail as a single string list and telling them to pick it apart,
no?
Not a very convincing argument, but OK for now in the sense that we
can fix it later if we wanted to before it gets too late.
(cc to Ramsay)
The discussion in both threads (v8 and v9), boils down to this,
is the `key_value_info` struct really required to be declared public or should
be
just an implementation detail. I will give you the context,
The usage of the above mentioned struct is only required for
git_config_get_value_multi(). With the public struct, the code flow would look
like,
-- 8 --
diff --git a/notes.c b/notes.c
index 5fe691d..b7ab115 100644
--- a/notes.c
+++ b/notes.c
@@ -961,19 +961,6 @@ void string_list_add_refs_from_colon_sep(struct
string_list *list,
free(globs_copy);
}
-static int notes_display_config(const char *k, const char *v, void *cb)
-{
- int *load_refs = cb;
-
- if (*load_refs !strcmp(k, notes.displayref)) {
- if (!v)
- config_error_nonbool(k);
- string_list_add_refs_by_glob(display_notes_refs, v);
- }
-
- return 0;
-}
-
const char *default_notes_ref(void)
{
const char *notes_ref = NULL;
@@ -1041,7 +1028,9 @@ struct notes_tree **load_notes_trees(struct string_list
*refs)
void init_display_notes(struct display_notes_opt *opt)
{
char *display_ref_env;
- int load_config_refs = 0;
+ const struct string_list *values;
+ struct key_value_info *kv_info;
+ int load_config_refs = 0, i;
display_notes_refs.strdup_strings = 1;
assert(!display_notes_trees);
@@ -1058,7 +1047,21 @@ void init_display_notes(struct display_notes_opt *opt)
load_config_refs = 1;
}
- git_config(notes_display_config, load_config_refs);
+ if (load_config_refs) {
+ values = git_config_get_value_multi(notes.displayref);
+ if (values) {
+ for (i = 0; i values-nr; i++) {
+ if (!values-items[i].string) {
+ kv_info = values-items[i].util;
+
config_error_nonbool(notes.displayref);
+
git_die_config_linenr(notes.displayref, kv_info-filename, kv_info-linenr);
+ }
+ else
+
string_list_add_refs_by_glob(display_notes_refs,
+
values-items[i].string);
+ }
+ }
+ }
if (opt) {
struct string_list_item *item;
-- 8 --
We cannot use git_die_config() here because it is applicable to the last
value for a given variable.
Alternative solution to the problem can be a helper function like this,
git_die_config_index(key, value_index, err_msg, ...) which needs the
value_index for a multi valued one,
+ values = git_config_get_value_multi(notes.displayref);
+ if (values) {
+ for (i = 0; i values-nr; i++) {
+ if (!values-items[i].string)
+
git_die_config_linenr(notes.displayref, i, no null values allowed for
:'%s', notes.displayref);
+ else ; /* do stuff */
+ }
A callback iterator which supplies the linenr and filename to the callback
function is not helpful, because there are many variable checks in a
git_config() call where multi valued and single valued both reside, so we cannot
use a callback iterator without adding more code cruft.
What do you think, which way seems least obtrusive, or is there an another way
out?
--
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 0/8] Rewrite `git_config()` using config-set API
[Patch v9]: Changed the grep statements in patch 7/8 and 8/8. [Patch v8]: git_die_config now allows custom error messages. new tests are now not too reliant on specific strings. [Patch v7]: style nit corrected. (1/8) is Matthieu's translation patch. git_die_config_linenr() helper function added. Diff between v6 and v7 appended for review. [Patch v6]: Added _() to error messages. Diff between v6 and v4 at the bottom. [PATCH v5]: New patch added (3/7). git_config() now returns void. [PATCH v4]: One style nit corrected, also added key to error messages. [PATCH V3]:All the suggestions in [3] applied. Built on top of [1]. [PATCH V2]: All the suggestions in [2] incorporated. git_config() now follows correct parsing order. Reordered the patches. Removed xfuncname patch as it was unnecssary. This series builds on the top of topic[1] in the mailing list with name git config cache special querying API utilizing the cache or (ta/config-set in pu). This series aims to do these three things, * Use the config-set API to rewrite git_config(). * Solve any legacy bugs in the previous system while at it. * To be feature complete compared to the previous git_config() implementation, which I think it is now. (added the line number and file name info just for completeness) Also, I haven't yet checked the exact improvements but still as a teaser, git status now only rereads the configuration files twice instead of four times. [1]: http://thread.gmane.org/gmane.comp.version-control.git/254286 [2]: http://thread.gmane.org/gmane.comp.version-control.git/254101 [3]: http://thread.gmane.org/gmane.comp.version-control.git/254211 Matthieu Moy (1): config.c: mark error and warnings strings for translation Tanay Abhra (7): config.c: fix accuracy of line number in errors add line number and file name info to `config_set` change `git_config()` return value to void config: add `git_die_config()` to the config-set API rewrite git_config() to use the config-set API add a test for semantic errors in config files add tests for `git_config_get_string_const()` Documentation/technical/api-config.txt | 13 +++ branch.c | 5 +- cache.h| 34 +++- config.c | 152 +++-- t/t1308-config-set.sh | 21 + t/t4055-diff-context.sh| 2 +- test-config.c | 10 +++ 7 files changed, 207 insertions(+), 30 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
[PATCH v9 1/8] config.c: mark error and warnings strings for translation
From: Matthieu Moy matthieu@imag.fr
Signed-off-by: Matthieu Moy matthieu@imag.fr
---
config.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/config.c b/config.c
index a191328..34940fd 100644
--- a/config.c
+++ b/config.c
@@ -457,9 +457,9 @@ static int git_parse_source(config_fn_t fn, void *data)
break;
}
if (cf-die_on_error)
- die(bad config file line %d in %s, cf-linenr, cf-name);
+ die(_(bad config file line %d in %s), cf-linenr, cf-name);
else
- return error(bad config file line %d in %s, cf-linenr,
cf-name);
+ return error(_(bad config file line %d in %s), cf-linenr,
cf-name);
}
static int parse_unit_factor(const char *end, uintmax_t *val)
@@ -575,9 +575,9 @@ static void die_bad_number(const char *name, const char
*value)
value = ;
if (cf cf-name)
- die(bad numeric config value '%s' for '%s' in %s: %s,
+ die(_(bad numeric config value '%s' for '%s' in %s: %s),
value, name, cf-name, reason);
- die(bad numeric config value '%s' for '%s': %s, value, name, reason);
+ die(_(bad numeric config value '%s' for '%s': %s), value, name,
reason);
}
int git_config_int(const char *name, const char *value)
@@ -662,7 +662,7 @@ int git_config_pathname(const char **dest, const char *var,
const char *value)
return config_error_nonbool(var);
*dest = expand_user_path(value);
if (!*dest)
- die(Failed to expand user dir in: '%s', value);
+ die(_(failed to expand user dir in: '%s'), value);
return 0;
}
@@ -740,7 +740,7 @@ static int git_default_core_config(const char *var, const
char *value)
if (level == -1)
level = Z_DEFAULT_COMPRESSION;
else if (level 0 || level Z_BEST_COMPRESSION)
- die(bad zlib compression level %d, level);
+ die(_(bad zlib compression level %d), level);
zlib_compression_level = level;
zlib_compression_seen = 1;
return 0;
@@ -751,7 +751,7 @@ static int git_default_core_config(const char *var, const
char *value)
if (level == -1)
level = Z_DEFAULT_COMPRESSION;
else if (level 0 || level Z_BEST_COMPRESSION)
- die(bad zlib compression level %d, level);
+ die(_(bad zlib compression level %d), level);
core_compression_level = level;
core_compression_seen = 1;
if (!zlib_compression_seen)
@@ -873,7 +873,7 @@ static int git_default_core_config(const char *var, const
char *value)
else if (!strcmp(value, link))
object_creation_mode = OBJECT_CREATION_USES_HARDLINKS;
else
- die(Invalid mode for object creation: %s, value);
+ die(_(invalid mode for object creation: %s), value);
return 0;
}
@@ -1173,7 +1173,7 @@ int git_config_early(config_fn_t fn, void *data, const
char *repo_config)
switch (git_config_from_parameters(fn, data)) {
case -1: /* error */
- die(unable to parse command-line config);
+ die(_(unable to parse command-line config));
break;
case 0: /* found nothing */
break;
@@ -1514,7 +1514,7 @@ static int store_aux(const char *key, const char *value,
void *cb)
case KEY_SEEN:
if (matches(key, value)) {
if (store.seen == 1 store.multi_replace == 0) {
- warning(%s has multiple values, key);
+ warning(_(%s has multiple values), key);
}
ALLOC_GROW(store.offset, store.seen + 1,
--
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
[PATCH v9 5/8] config: add `git_die_config()` to the config-set API
Add `git_die_config` that dies printing the line number and the file name
of the highest priority value for the configuration variable `key`. A custom
error message is also printed before dying, specified by the caller, which can
be skipped if `err` argument is set to NULL.
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)){
if (!strcmp(value, foo))
git_config_die(key, value: `%s` is illegal, value);
else
/* do work */
}
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
Documentation/technical/api-config.txt | 13
cache.h| 3 +++
config.c | 39 --
3 files changed, 53 insertions(+), 2 deletions(-)
diff --git a/Documentation/technical/api-config.txt
b/Documentation/technical/api-config.txt
index 21f280c..0d8b99b 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -155,6 +155,19 @@ 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.
+`git_die_config(const char *key, const char *err, ...)`::
+
+ First prints the error message specified by the caller in `err` and then
+ dies printing the line number and the file name of the highest priority
+ value for the configuration variable `key`.
+
+`void git_die_config_linenr(const char *key, const char *filename, int
linenr)`::
+
+ Helper function which formats the die error message according to the
+ parameters entered. Used by `git_die_config()`. It can be used by
callers
+ handling `git_config_get_value_multi()` to print the correct error
message
+ for the desired value.
+
See test-config.c for usage examples.
Value Parsing Helpers
diff --git a/cache.h b/cache.h
index f11ce41..89a0d51 100644
--- a/cache.h
+++ b/cache.h
@@ -1388,6 +1388,9 @@ struct key_value_info {
int linenr;
};
+extern NORETURN void git_die_config(const char *key, const char *err, ...)
__attribute__((format(printf, 2, 3)));
+extern NORETURN void git_die_config_linenr(const char *key, 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 4cefa25..5ae9ab0 100644
--- a/config.c
+++ b/config.c
@@ -1469,8 +1469,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, NULL);
+ return ret;
}
int git_config_get_string(const char *key, char **dest)
@@ -1511,8 +1515,39 @@ 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, NULL);
+ return ret;
+}
+
+NORETURN
+void git_die_config_linenr(const char *key, const char *filename, int linenr)
+{
+ if (!filename)
+ die(_(unable to parse '%s' from command-line config), key);
+ else
+ die(_(bad config variable '%s' in file '%s' at line %d),
+ key, filename, linenr);
+}
+
+NORETURN __attribute__((format(printf, 2, 3)))
+void git_die_config(const char *key, const char *err, ...)
+{
+ const struct string_list *values;
+ struct key_value_info *kv_info;
+
+ if (err) {
+ va_list params;
+ va_start(params, err);
+ vreportf(error: , err, params);
+ va_end(params);
+ }
+ values = git_config_get_value_multi(key);
+ kv_info = values-items[values-nr - 1].util;
+ git_die_config_linenr(key, kv_info-filename, kv_info-linenr);
}
/*
--
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
[PATCH v9 2/8] config.c: fix accuracy of line number in errors
From: Matthieu Moy matthieu@imag.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 tanay...@gmail.com
Signed-off-by: Tanay Abhra tanay...@gmail.com
Signed-off-by: Matthieu Moy matthieu@imag.fr
---
config.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/config.c b/config.c
index 34940fd..bb4629e 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
--
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 4/8] change `git_config()` return value to void
Currently `git_config()` returns an integer signifying an error code.
During rewrites of the function most of the code was shifted to
`git_config_with_options()`. `git_config_with_options()` normally
returns positive values if its `config_source` parameter is set as NULL,
as most errors are fatal, and non-fatal potential errors are guarded
by if statements that are entered only when no error is possible.
Still a negative value can be returned in case of race condition between
`access_or_die()` `git_config_from_file()`. Also, all callers of
`git_config()` ignore the return value except for one case in branch.c.
Change `git_config()` return value to void and make it die if it receives
a negative value from `git_config_with_options()`.
Original-patch-by: Matthieu Moy matthieu@imag.fr
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
branch.c | 5 +
cache.h | 2 +-
config.c | 16 ++--
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/branch.c b/branch.c
index 46e8aa8..735767d 100644
--- a/branch.c
+++ b/branch.c
@@ -161,10 +161,7 @@ int read_branch_desc(struct strbuf *buf, const char
*branch_name)
strbuf_addf(name, branch.%s.description, branch_name);
cb.config_name = name.buf;
cb.value = NULL;
- if (git_config(read_branch_desc_cb, cb) 0) {
- strbuf_release(name);
- return -1;
- }
+ git_config(read_branch_desc_cb, cb);
if (cb.value)
strbuf_addstr(buf, cb.value);
strbuf_release(name);
diff --git a/cache.h b/cache.h
index 0b1bdfd..f11ce41 100644
--- a/cache.h
+++ b/cache.h
@@ -1294,7 +1294,7 @@ extern int git_config_from_buf(config_fn_t fn, const char
*name,
const char *buf, size_t len, void *data);
extern void git_config_push_parameter(const char *text);
extern int git_config_from_parameters(config_fn_t fn, void *data);
-extern int git_config(config_fn_t fn, void *);
+extern void git_config(config_fn_t fn, void *);
extern int git_config_with_options(config_fn_t fn, void *,
struct git_config_source *config_source,
int respect_includes);
diff --git a/config.c b/config.c
index e4d745e..4cefa25 100644
--- a/config.c
+++ b/config.c
@@ -1230,9 +1230,21 @@ int git_config_with_options(config_fn_t fn, void *data,
return ret;
}
-int git_config(config_fn_t fn, void *data)
+void git_config(config_fn_t fn, void *data)
{
- return git_config_with_options(fn, data, NULL, 1);
+ if (git_config_with_options(fn, data, NULL, 1) 0)
+ /*
+* git_config_with_options() normally returns only
+* positive values, as most errors are fatal, and
+* non-fatal potential errors are guarded by if
+* statements that are entered only when no error is
+* possible.
+*
+* If we ever encounter a non-fatal error, it means
+* something went really wrong and we should stop
+* immediately.
+*/
+ die(_(unknown error occured while reading the configuration
files));
}
static struct config_set_element *configset_find_element(struct config_set
*cs, const char *key)
--
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
[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 tanay...@gmail.com
---
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
[PATCH v9 8/8] add tests for `git_config_get_string_const()`
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 tanay...@gmail.com
---
t/t1308-config-set.sh | 10 ++
test-config.c | 10 ++
2 files changed, 20 insertions(+)
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 9cc678d..ea0bce2 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -119,6 +119,16 @@ 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
+ check_config expect_code 1 get_string case.ba Value not found for
\case.ba\
+'
+
+test_expect_success 'check line error when NULL string is queried' '
+ test_expect_code 128 test-config get_string case.foo 2result
+ test_i18ngrep fatal: .*case\.foo.*\.git/config.*line 7 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
--
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 6/8] rewrite git_config() to use the config-set API
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 tanay...@gmail.com
---
cache.h | 24 +++
config.c| 51 +
t/t4055-diff-context.sh | 2 +-
3 files changed, 68 insertions(+), 9 deletions(-)
diff --git a/cache.h b/cache.h
index 89a0d51..2693a37 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 5ae9ab0..427850a 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;
@@ -1230,7 +1224,7 @@ int git_config_with_options(config_fn_t fn, void *data,
return ret;
}
-void git_config(config_fn_t fn, void *data)
+static void git_config_raw(config_fn_t fn, void *data)
{
if (git_config_with_options(fn, data, NULL, 1) 0)
/*
@@ -1247,6 +1241,33 @@ void git_config(config_fn_t fn, void *data)
die(_(unknown error occured while reading the configuration
files));
}
+static void 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;
+ git_die_config_linenr(entry-key, kv_info-filename,
kv_info-linenr);
+ }
+ }
+}
+
+static void git_config_check_init(void);
+
+void git_config(config_fn_t fn, void *data)
+{
+ git_config_check_init();
+ 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;
@@ -1273,6 +1294,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);
@@ -1288,6 +1310,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;
@@ -1311,6 +1339,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
[PATCH v9 7/8] add a test for semantic errors in config files
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 tanay...@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..9cc678d 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 2result + test_i18ngrep fatal: .*alias\.br.*\.git/config.*line 2 result +' + test_done -- 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
[PATCH v2 05/11] fetchpack.c: replace `git_config()` with `git_config_get_*()` family
Use `git_config_get_*()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
fetch-pack.c | 35 ---
1 file changed, 8 insertions(+), 27 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index b8a58fa..a13e9db 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -869,34 +869,15 @@ static struct ref *do_fetch_pack(struct fetch_pack_args
*args,
return ref;
}
-static int fetch_pack_config(const char *var, const char *value, void *cb)
+static void fetch_pack_config(void)
{
- if (strcmp(var, fetch.unpacklimit) == 0) {
- fetch_unpack_limit = git_config_int(var, value);
- return 0;
- }
-
- if (strcmp(var, transfer.unpacklimit) == 0) {
- transfer_unpack_limit = git_config_int(var, value);
- return 0;
- }
-
- if (strcmp(var, repack.usedeltabaseoffset) == 0) {
- prefer_ofs_delta = git_config_bool(var, value);
- return 0;
- }
-
- if (!strcmp(var, fetch.fsckobjects)) {
- fetch_fsck_objects = git_config_bool(var, value);
- return 0;
- }
-
- if (!strcmp(var, transfer.fsckobjects)) {
- transfer_fsck_objects = git_config_bool(var, value);
- return 0;
- }
+ git_config_get_int(fetch.unpacklimit, fetch_unpack_limit);
+ git_config_get_int(transfer.unpacklimit, transfer_unpack_limit);
+ git_config_get_bool(repack.usedeltabaseoffset, prefer_ofs_delta);
+ git_config_get_bool(fetch.fsckobjects, fetch_fsck_objects);
+ git_config_get_bool(transfer.fsckobjects, transfer_fsck_objects);
- return git_default_config(var, value, cb);
+ git_config(git_default_config, NULL);
}
static void fetch_pack_setup(void)
@@ -904,7 +885,7 @@ static void fetch_pack_setup(void)
static int did_setup;
if (did_setup)
return;
- git_config(fetch_pack_config, NULL);
+ fetch_pack_config();
if (0 = transfer_unpack_limit)
unpack_limit = transfer_unpack_limit;
else if (0 = fetch_unpack_limit)
--
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
[PATCH v2 06/11] rerere.c: replace `git_config()` with `git_config_get_*()` family
Use `git_config_get_*()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
rerere.c | 43 ---
1 file changed, 12 insertions(+), 31 deletions(-)
diff --git a/rerere.c b/rerere.c
index d84b495..20b18ad 100644
--- a/rerere.c
+++ b/rerere.c
@@ -573,15 +573,11 @@ static int do_plain_rerere(struct string_list *rr, int fd)
return write_rr(rr, fd);
}
-static int git_rerere_config(const char *var, const char *value, void *cb)
+static void git_rerere_config(void)
{
- if (!strcmp(var, rerere.enabled))
- rerere_enabled = git_config_bool(var, value);
- else if (!strcmp(var, rerere.autoupdate))
- rerere_autoupdate = git_config_bool(var, value);
- else
- return git_default_config(var, value, cb);
- return 0;
+ git_config_get_bool(rerere.enabled, rerere_enabled);
+ git_config_get_bool(rerere.autoupdate, rerere_autoupdate);
+ git_config(git_default_config, NULL);
}
static int is_rerere_enabled(void)
@@ -606,7 +602,7 @@ int setup_rerere(struct string_list *merge_rr, int flags)
{
int fd;
- git_config(git_rerere_config, NULL);
+ git_rerere_config();
if (!is_rerere_enabled())
return -1;
@@ -699,24 +695,6 @@ static void unlink_rr_item(const char *name)
rmdir(git_path(rr-cache/%s, name));
}
-struct rerere_gc_config_cb {
- int cutoff_noresolve;
- int cutoff_resolve;
-};
-
-static int git_rerere_gc_config(const char *var, const char *value, void *cb)
-{
- struct rerere_gc_config_cb *cf = cb;
-
- if (!strcmp(var, gc.rerereresolved))
- cf-cutoff_resolve = git_config_int(var, value);
- else if (!strcmp(var, gc.rerereunresolved))
- cf-cutoff_noresolve = git_config_int(var, value);
- else
- return git_default_config(var, value, cb);
- return 0;
-}
-
void rerere_gc(struct string_list *rr)
{
struct string_list to_remove = STRING_LIST_INIT_DUP;
@@ -724,9 +702,12 @@ void rerere_gc(struct string_list *rr)
struct dirent *e;
int i, cutoff;
time_t now = time(NULL), then;
- struct rerere_gc_config_cb cf = { 15, 60 };
+ int cutoff_noresolve = 15;
+ int cutoff_resolve = 60;
- git_config(git_rerere_gc_config, cf);
+ git_config_get_int(gc.rerereresolved, cutoff_resolve);
+ git_config_get_int(gc.rerereunresolved, cutoff_noresolve);
+ git_config(git_default_config, NULL);
dir = opendir(git_path(rr-cache));
if (!dir)
die_errno(unable to open rr-cache directory);
@@ -736,12 +717,12 @@ void rerere_gc(struct string_list *rr)
then = rerere_last_used_at(e-d_name);
if (then) {
- cutoff = cf.cutoff_resolve;
+ cutoff = cutoff_resolve;
} else {
then = rerere_created_at(e-d_name);
if (!then)
continue;
- cutoff = cf.cutoff_noresolve;
+ cutoff = cutoff_noresolve;
}
if (then now - cutoff * 86400)
string_list_append(to_remove, e-d_name);
--
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
[PATCH v2 08/11] pager.c: replace `git_config()` with `git_config_get_value()`
Use `git_config_get_value()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
pager.c | 40 +---
1 file changed, 13 insertions(+), 27 deletions(-)
diff --git a/pager.c b/pager.c
index 8b5cbc5..b7eb7e7 100644
--- a/pager.c
+++ b/pager.c
@@ -6,12 +6,6 @@
#define DEFAULT_PAGER less
#endif
-struct pager_config {
- const char *cmd;
- int want;
- char *value;
-};
-
/*
* This is split up from the rest of git so that we can do
* something different on Windows.
@@ -155,30 +149,22 @@ int decimal_width(int number)
return width;
}
-static int pager_command_config(const char *var, const char *value, void *data)
+/* returns 0 for no pager, 1 for use pager, and -1 for not specified */
+int check_pager_config(const char *cmd)
{
- struct pager_config *c = data;
- if (starts_with(var, pager.) !strcmp(var + 6, c-cmd)) {
- int b = git_config_maybe_bool(var, value);
+ int want = -1;
+ struct strbuf key = STRBUF_INIT;
+ const char *value = NULL;
+ strbuf_addf(key, pager.%s, cmd);
+ if (!git_config_get_value(key.buf, value)) {
+ int b = git_config_maybe_bool(key.buf, value);
if (b = 0)
- c-want = b;
+ want = b;
else {
- c-want = 1;
- c-value = xstrdup(value);
+ want = 1;
+ pager_program = xstrdup(value);
}
}
- return 0;
-}
-
-/* returns 0 for no pager, 1 for use pager, and -1 for not specified */
-int check_pager_config(const char *cmd)
-{
- struct pager_config c;
- c.cmd = cmd;
- c.want = -1;
- c.value = NULL;
- git_config(pager_command_config, c);
- if (c.value)
- pager_program = c.value;
- return c.want;
+ strbuf_release(key);
+ return want;
}
--
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
[PATCH v2 04/11] archive.c: replace `git_config()` with `git_config_get_bool()` family
Use `git_config_get_bool()` family instead of `git_config()` to take advantage
of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
archive.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/archive.c b/archive.c
index 3fc0fb2..952a659 100644
--- a/archive.c
+++ b/archive.c
@@ -402,14 +402,6 @@ static int parse_archive_args(int argc, const char **argv,
return argc;
}
-static int git_default_archive_config(const char *var, const char *value,
- void *cb)
-{
- if (!strcmp(var, uploadarchive.allowunreachable))
- remote_allow_unreachable = git_config_bool(var, value);
- return git_default_config(var, value, cb);
-}
-
int write_archive(int argc, const char **argv, const char *prefix,
int setup_prefix, const char *name_hint, int remote)
{
@@ -420,7 +412,9 @@ int write_archive(int argc, const char **argv, const char
*prefix,
if (setup_prefix prefix == NULL)
prefix = setup_git_directory_gently(nongit);
- git_config(git_default_archive_config, NULL);
+ git_config_get_bool(uploadarchive.allowunreachable,
remote_allow_unreachable);
+ git_config(git_default_config, NULL);
+
init_tar_archiver();
init_zip_archiver();
--
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
[PATCH v2 10/11] alias.c: replace `git_config()` with `git_config_get_string()`
Use `git_config_get_string()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
alias.c | 25 ++---
1 file changed, 6 insertions(+), 19 deletions(-)
diff --git a/alias.c b/alias.c
index 758c867..6aa164a 100644
--- a/alias.c
+++ b/alias.c
@@ -1,26 +1,13 @@
#include cache.h
-static const char *alias_key;
-static char *alias_val;
-
-static int alias_lookup_cb(const char *k, const char *v, void *cb)
-{
- const char *name;
- if (skip_prefix(k, alias., name) !strcmp(name, alias_key)) {
- if (!v)
- return config_error_nonbool(k);
- alias_val = xstrdup(v);
- return 0;
- }
- return 0;
-}
-
char *alias_lookup(const char *alias)
{
- alias_key = alias;
- alias_val = NULL;
- git_config(alias_lookup_cb, NULL);
- return alias_val;
+ char *v = NULL;
+ struct strbuf key = STRBUF_INIT;
+ strbuf_addf(key, alias.%s, alias);
+ git_config_get_string(key.buf, v);
+ strbuf_release(key);
+ return v;
}
#define SPLIT_CMDLINE_BAD_ENDING 1
--
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
[PATCH v2 01/11] daemon.c: replace `git_config()` with `git_config_get_bool()` family
Use `git_config_get_bool()` family instead of `git_config()` to take advantage
of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
daemon.c | 26 --
1 file changed, 4 insertions(+), 22 deletions(-)
diff --git a/daemon.c b/daemon.c
index e6b51ed..6f78b61 100644
--- a/daemon.c
+++ b/daemon.c
@@ -230,23 +230,6 @@ struct daemon_service {
int overridable;
};
-static struct daemon_service *service_looking_at;
-static int service_enabled;
-
-static int git_daemon_config(const char *var, const char *value, void *cb)
-{
- const char *service;
-
- if (skip_prefix(var, daemon., service)
- !strcmp(service, service_looking_at-config_name)) {
- service_enabled = git_config_bool(var, value);
- return 0;
- }
-
- /* we are not interested in parsing any other configuration here */
- return 0;
-}
-
static int daemon_error(const char *dir, const char *msg)
{
if (!informative_errors)
@@ -324,6 +307,7 @@ static int run_service(const char *dir, struct
daemon_service *service)
{
const char *path;
int enabled = service-enabled;
+ struct strbuf var = STRBUF_INIT;
loginfo(Request %s for '%s', service-name, dir);
@@ -354,11 +338,9 @@ static int run_service(const char *dir, struct
daemon_service *service)
}
if (service-overridable) {
- service_looking_at = service;
- service_enabled = -1;
- git_config(git_daemon_config, NULL);
- if (0 = service_enabled)
- enabled = service_enabled;
+ strbuf_addf(var, daemon.%s, service-config_name);
+ git_config_get_bool(var.buf, enabled);
+ strbuf_release(var);
}
if (!enabled) {
logerror('%s': service not enabled for '%s',
--
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
[PATCH v2 00/11] git_config callers rewritten with the new config-set API
[v2]: git_die_config() messages changed. Diff between v1 and v2 is at the
bottom.
The ta/config-set API is more or less solidified.
This series builds on the top of 4c715ebb in pu (ta/config-set). On top of it,
it also requires series [1] (Rewrite `git_config()` using config-set API) for
proper error checking.
This series is the first batch of patches which rewrites the existing callers
using a non-callback approach.
This series aims to,
* rewrite the existing callers, as you can see from the diff stat the bew API
provides a much concise and clear control flow.
* stress test the new API, see if any corner cases or deficiencies arise or not.
The series passes all the tests, only thing to watch is that the config
variables
that have been rewritten are single valued only. Though I have tried my best to
ascertain it, still mistakes may arise.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/254633/
Tanay Abhra (11):
daemon.c: replace `git_config()` with `git_config_get_bool()` family
http-backend.c: replace `git_config()` with `git_config_get_bool()`
family
read-cache.c: replace `git_config()` with `git_config_get_*()` family
archive.c: replace `git_config()` with `git_config_get_bool()` family
fetchpack.c: replace `git_config()` with `git_config_get_*()` family
rerere.c: replace `git_config()` with `git_config_get_*()` family
builtin/gc.c: replace `git_config()` with `git_config_get_*()` family
pager.c: replace `git_config()` with `git_config_get_value()`
imap-send.c: replace `git_config()` with `git_config_get_*()` family
alias.c: replace `git_config()` with `git_config_get_string()`
branch.c: replace `git_config()` with `git_config_get_string()
alias.c| 25 ++--
archive.c | 12 +++-
branch.c | 27 +++---
builtin/gc.c | 51 -
daemon.c | 26 -
fetch-pack.c | 35 --
http-backend.c | 31 --
imap-send.c| 60 +-
pager.c| 40 +--
read-cache.c | 14 +++---
rerere.c | 43 -
11 files changed, 114 insertions(+), 250 deletions(-)
--
1.9.0.GIT
-- 8 --
diff --git a/builtin/gc.c b/builtin/gc.c
index 4612ef5..5173657 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -76,8 +76,8 @@ static void gc_config(void)
if (strcmp(prune_expire, now)) {
unsigned long now = approxidate(now);
if (approxidate(prune_expire) = now) {
- error(_(Invalid %s: '%s'), gc.pruneexpire, prune_expire);
- git_die_config(gc.pruneexpire);
+ git_die_config(gc.pruneexpire, _(Invalid gc.pruneexpire: '%s'),
+ prune_expire);
}
}
}
diff --git a/daemon.c b/daemon.c
index fb16664..6f78b61 100644
--- a/daemon.c
+++ b/daemon.c
@@ -342,7 +342,6 @@ static int run_service(const char *dir, struct
daemon_service *service)
git_config_get_bool(var.buf, enabled);
strbuf_release(var);
}
-
if (!enabled) {
logerror('%s': service not enabled for '%s',
service-name, path);
diff --git a/imap-send.c b/imap-send.c
index 586bdd8..618d75b 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1336,8 +1336,7 @@ static void git_imap_config(void)
if (!git_config_get_value(imap.host, val)) {
if (!val) {
- config_error_nonbool(imap.host);
- git_die_config(imap.host);
+ git_die_config(imap.host, Missing value for 'imap.host');
} else {
if (starts_with(val, imap:))
val += 5;
-- 8 --
--
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 v2 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family
Use `git_config_get_bool()` family instead of `git_config()` to take advantage
of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
http-backend.c | 31 ---
1 file changed, 12 insertions(+), 19 deletions(-)
diff --git a/http-backend.c b/http-backend.c
index 80790bb..106ca6b 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -219,29 +219,22 @@ static void get_idx_file(char *name)
send_local_file(application/x-git-packed-objects-toc, name);
}
-static int http_config(const char *var, const char *value, void *cb)
+static void http_config(void)
{
- const char *p;
+ int i, value = 0;
+ struct strbuf var = STRBUF_INIT;
- if (!strcmp(var, http.getanyfile)) {
- getanyfile = git_config_bool(var, value);
- return 0;
- }
+ git_config_get_bool(http.getanyfile, getanyfile);
- if (skip_prefix(var, http., p)) {
- int i;
-
- for (i = 0; i ARRAY_SIZE(rpc_service); i++) {
- struct rpc_service *svc = rpc_service[i];
- if (!strcmp(p, svc-config_name)) {
- svc-enabled = git_config_bool(var, value);
- return 0;
- }
- }
+ for (i = 0; i ARRAY_SIZE(rpc_service); i++) {
+ struct rpc_service *svc = rpc_service[i];
+ strbuf_addf(var, http.%s, svc-config_name);
+ if (!git_config_get_bool(var.buf, value))
+ svc-enabled = value;
+ strbuf_reset(var);
}
- /* we are not interested in parsing any other configuration here */
- return 0;
+ strbuf_release(var);
}
static struct rpc_service *select_service(const char *name)
@@ -627,7 +620,7 @@ int main(int argc, char **argv)
access(git-daemon-export-ok, F_OK) )
not_found(Repository not exported: '%s', dir);
- git_config(http_config, NULL);
+ http_config();
cmd-imp(cmd_arg);
return 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
[PATCH v2 03/11] read-cache.c: replace `git_config()` with `git_config_get_*()` family
Use `git_config_get_*()` family instead of `git_config()` to take
advantage of the config-set API which provides a cleaner control flow.
Use an intermediate value, as `version` can not be used directly in
git_config_get_int() due to incompatible type.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
read-cache.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index 5d3c8bd..acb132d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1238,24 +1238,16 @@ static struct cache_entry *refresh_cache_entry(struct
cache_entry *ce,
#define INDEX_FORMAT_DEFAULT 3
-static int index_format_config(const char *var, const char *value, void *cb)
-{
- unsigned int *version = cb;
- if (!strcmp(var, index.version)) {
- *version = git_config_int(var, value);
- return 0;
- }
- return 1;
-}
-
static unsigned int get_index_format_default(void)
{
char *envversion = getenv(GIT_INDEX_VERSION);
char *endp;
+ int value;
unsigned int version = INDEX_FORMAT_DEFAULT;
if (!envversion) {
- git_config(index_format_config, version);
+ if (!git_config_get_int(index.version, value))
+ version = value;
if (version INDEX_FORMAT_LB || INDEX_FORMAT_UB version) {
warning(_(index.version set, but the value is
invalid.\n
Using version %i), INDEX_FORMAT_DEFAULT);
--
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
[PATCH v2 09/11] imap-send.c: replace `git_config()` with `git_config_get_*()` family
Use `git_config_get_*()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
imap-send.c | 60 ++--
1 file changed, 26 insertions(+), 34 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 524fbab..618d75b 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1326,43 +1326,35 @@ static int split_msg(struct strbuf *all_msgs, struct
strbuf *msg, int *ofs)
static char *imap_folder;
-static int git_imap_config(const char *key, const char *val, void *cb)
+static void git_imap_config(void)
{
- if (!skip_prefix(key, imap., key))
- return 0;
+ const char *val = NULL;
+
+ git_config_get_bool(imap.sslverify, server.ssl_verify);
+ git_config_get_bool(imap.preformattedhtml, server.use_html);
+ git_config_get_string(imap.folder, imap_folder);
- /* check booleans first, and barf on others */
- if (!strcmp(sslverify, key))
- server.ssl_verify = git_config_bool(key, val);
- else if (!strcmp(preformattedhtml, key))
- server.use_html = git_config_bool(key, val);
- else if (!val)
- return config_error_nonbool(key);
-
- if (!strcmp(folder, key)) {
- imap_folder = xstrdup(val);
- } else if (!strcmp(host, key)) {
- if (starts_with(val, imap:))
- val += 5;
- else if (starts_with(val, imaps:)) {
- val += 6;
- server.use_ssl = 1;
+ if (!git_config_get_value(imap.host, val)) {
+ if (!val) {
+ git_die_config(imap.host, Missing value for
'imap.host');
+ } else {
+ if (starts_with(val, imap:))
+ val += 5;
+ else if (starts_with(val, imaps:)) {
+ val += 6;
+ server.use_ssl = 1;
+ }
+ if (starts_with(val, //))
+ val += 2;
+ server.host = xstrdup(val);
}
- if (starts_with(val, //))
- val += 2;
- server.host = xstrdup(val);
- } else if (!strcmp(user, key))
- server.user = xstrdup(val);
- else if (!strcmp(pass, key))
- server.pass = xstrdup(val);
- else if (!strcmp(port, key))
- server.port = git_config_int(key, val);
- else if (!strcmp(tunnel, key))
- server.tunnel = xstrdup(val);
- else if (!strcmp(authmethod, key))
- server.auth_method = xstrdup(val);
+ }
- return 0;
+ git_config_get_string(imap.user, server.user);
+ git_config_get_string(imap.pass, server.pass);
+ git_config_get_int(imap.port, server.port);
+ git_config_get_string(imap.tunnel, server.tunnel);
+ git_config_get_string(imap.authmethod, server.auth_method);
}
int main(int argc, char **argv)
@@ -1383,7 +1375,7 @@ int main(int argc, char **argv)
usage(imap_send_usage);
setup_git_directory_gently(nongit_ok);
- git_config(git_imap_config, NULL);
+ git_imap_config();
if (!server.port)
server.port = server.use_ssl ? 993 : 143;
--
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
[PATCH v2 07/11] builtin/gc.c: replace `git_config()` with `git_config_get_*()` family
Use `git_config_get_*()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
builtin/gc.c | 51 ---
1 file changed, 20 insertions(+), 31 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index 8d219d8..ced1456 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -55,44 +55,33 @@ static void remove_pidfile_on_signal(int signo)
raise(signo);
}
-static int gc_config(const char *var, const char *value, void *cb)
+static void gc_config(void)
{
- if (!strcmp(var, gc.packrefs)) {
+ const char *value;
+
+ if (!git_config_get_value(gc.packrefs, value)) {
if (value !strcmp(value, notbare))
pack_refs = -1;
else
- pack_refs = git_config_bool(var, value);
- return 0;
- }
- if (!strcmp(var, gc.aggressivewindow)) {
- aggressive_window = git_config_int(var, value);
- return 0;
- }
- if (!strcmp(var, gc.aggressivedepth)) {
- aggressive_depth = git_config_int(var, value);
- return 0;
- }
- if (!strcmp(var, gc.auto)) {
- gc_auto_threshold = git_config_int(var, value);
- return 0;
- }
- if (!strcmp(var, gc.autopacklimit)) {
- gc_auto_pack_limit = git_config_int(var, value);
- return 0;
+ pack_refs = git_config_bool(gc.packrefs, value);
}
- if (!strcmp(var, gc.autodetach)) {
- detach_auto = git_config_bool(var, value);
- return 0;
- }
- if (!strcmp(var, gc.pruneexpire)) {
- if (value strcmp(value, now)) {
+
+ git_config_get_int(gc.aggressivewindow, aggressive_window);
+ git_config_get_int(gc.aggressivedepth, aggressive_depth);
+ git_config_get_int(gc.auto, gc_auto_threshold);
+ git_config_get_int(gc.autopacklimit, gc_auto_pack_limit);
+ git_config_get_bool(gc.autodetach, detach_auto);
+
+ if (!git_config_get_string_const(gc.pruneexpire, prune_expire)) {
+ if (strcmp(prune_expire, now)) {
unsigned long now = approxidate(now);
- if (approxidate(value) = now)
- return error(_(Invalid %s: '%s'), var, value);
+ if (approxidate(prune_expire) = now) {
+ git_die_config(gc.pruneexpire, _(Invalid
gc.pruneexpire: '%s'),
+ prune_expire);
+ }
}
- return git_config_string(prune_expire, var, value);
}
- return git_default_config(var, value, cb);
+ git_config(git_default_config, NULL);
}
static int too_many_loose_objects(void)
@@ -301,7 +290,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
argv_array_pushl(prune, prune, --expire, NULL );
argv_array_pushl(rerere, rerere, gc, NULL);
- git_config(gc_config, NULL);
+ gc_config();
if (pack_refs 0)
pack_refs = !is_bare_repository();
--
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
[PATCH v2 11/11] branch.c: replace `git_config()` with `git_config_get_string()
Use `git_config_get_string()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
branch.c | 27 +++
1 file changed, 7 insertions(+), 20 deletions(-)
diff --git a/branch.c b/branch.c
index 735767d..df6b120 100644
--- a/branch.c
+++ b/branch.c
@@ -140,30 +140,17 @@ static int setup_tracking(const char *new_ref, const char
*orig_ref,
return 0;
}
-struct branch_desc_cb {
- const char *config_name;
- const char *value;
-};
-
-static int read_branch_desc_cb(const char *var, const char *value, void *cb)
-{
- struct branch_desc_cb *desc = cb;
- if (strcmp(desc-config_name, var))
- return 0;
- free((char *)desc-value);
- return git_config_string(desc-value, var, value);
-}
-
int read_branch_desc(struct strbuf *buf, const char *branch_name)
{
- struct branch_desc_cb cb;
+ char *v = NULL;
struct strbuf name = STRBUF_INIT;
strbuf_addf(name, branch.%s.description, branch_name);
- cb.config_name = name.buf;
- cb.value = NULL;
- git_config(read_branch_desc_cb, cb);
- if (cb.value)
- strbuf_addstr(buf, cb.value);
+ if (git_config_get_string(name.buf, v)) {
+ strbuf_release(name);
+ return -1;
+ }
+ strbuf_addstr(buf, v);
+ free(v);
strbuf_release(name);
return 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
[PATCH v3 11/11] branch.c: replace `git_config()` with `git_config_get_string()
Use `git_config_get_string()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow. While we are at
it, return -1 if we find no value for the queried variable. Original code
returned 0 for all cases, which was checked by `add_branch_desc()` in
fmt-merge-msg.c resulting in addition of a spurious newline to the `out`
strbuf. Now, the newline addition is skipped as -1 is returned to the caller
if no value is found.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
v3: Changed the commit message to a more appropriate one.
branch.c | 27 +++
1 file changed, 7 insertions(+), 20 deletions(-)
diff --git a/branch.c b/branch.c
index 735767d..df6b120 100644
--- a/branch.c
+++ b/branch.c
@@ -140,30 +140,17 @@ static int setup_tracking(const char *new_ref, const char
*orig_ref,
return 0;
}
-struct branch_desc_cb {
- const char *config_name;
- const char *value;
-};
-
-static int read_branch_desc_cb(const char *var, const char *value, void *cb)
-{
- struct branch_desc_cb *desc = cb;
- if (strcmp(desc-config_name, var))
- return 0;
- free((char *)desc-value);
- return git_config_string(desc-value, var, value);
-}
-
int read_branch_desc(struct strbuf *buf, const char *branch_name)
{
- struct branch_desc_cb cb;
+ char *v = NULL;
struct strbuf name = STRBUF_INIT;
strbuf_addf(name, branch.%s.description, branch_name);
- cb.config_name = name.buf;
- cb.value = NULL;
- git_config(read_branch_desc_cb, cb);
- if (cb.value)
- strbuf_addstr(buf, cb.value);
+ if (git_config_get_string(name.buf, v)) {
+ strbuf_release(name);
+ return -1;
+ }
+ strbuf_addstr(buf, v);
+ free(v);
strbuf_release(name);
return 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
[PATCH v8 0/8] Rewrite `git_config()` using config-set API
[Patch v8]: git_die_config now allows custom error messages.
new tests are now not too reliant on specific strings. Diff
between v7 v8 is appended at the bottom. Thanks to Junio
Matthieu for their suggestions.
[Patch v7]: style nit corrected. (1/8) is Matthieu's translation patch.
git_die_config_linenr() helper function added. Diff between v6
and v7 appended for review.
[Patch v6]: Added _() to error messages.
Diff between v6 and v4 at the bottom.
[PATCH v5]: New patch added (3/7). git_config() now returns void.
[PATCH v4]: One style nit corrected, also added key to error messages.
[PATCH V3]:All the suggestions in [3] applied. Built on top of [1].
[PATCH V2]: All the suggestions in [2] incorporated. git_config() now follows
correct parsing order. Reordered the patches. Removed xfuncname patch
as it was unnecssary.
This series builds on the top of topic[1] in the mailing list with name
git config cache special querying API utilizing the cache.
This series aims to do these three things,
* Use the config-set API to rewrite git_config().
* Solve any legacy bugs in the previous system while at it.
* To be feature complete compared to the previous git_config() implementation,
which I think it is now. (added the line number and file name info just
for
completeness)
Also, I haven't yet checked the exact improvements but still as a teaser,
git status now only rereads the configuration files twice instead of four
times.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/254286
[2]: http://thread.gmane.org/gmane.comp.version-control.git/254101
[3]: http://thread.gmane.org/gmane.comp.version-control.git/254211
Matthieu Moy (1):
config.c: mark error and warnings strings for translation
Tanay Abhra (7):
config.c: fix accuracy of line number in errors
add line number and file name info to `config_set`
change `git_config()` return value to void
config: add `git_die_config()` to the config-set API
rewrite git_config() to use the config-set API
add a test for semantic errors in config files
add tests for `git_config_get_string_const()`
Documentation/technical/api-config.txt | 13 +++
branch.c | 5 +-
cache.h| 34 +++-
config.c | 152 +++--
t/t1308-config-set.sh | 21 +
t/t4055-diff-context.sh| 2 +-
test-config.c | 10 +++
7 files changed, 207 insertions(+), 30 deletions(-)
--
1.9.0.GIT
-- 8 --
diff --git a/Documentation/technical/api-config.txt
b/Documentation/technical/api-config.txt
index d7d0cf9..0d8b99b 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -155,10 +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)`::
+`git_die_config(const char *key, const char *err, ...)`::
- Dies printing the line number and the file name of the highest
- priority value for the configuration variable `key`.
+ First prints the error message specified by the caller in `err` and then
+ dies printing the line number and the file name of the highest priority
+ value for the configuration variable `key`.
`void git_die_config_linenr(const char *key, const char *filename, int
linenr)`::
diff --git a/cache.h b/cache.h
index ff17889..2693a37 100644
--- a/cache.h
+++ b/cache.h
@@ -1406,8 +1406,14 @@ 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 void git_die_config_linenr(const char *key, const char *filename, int
linenr);
+
+struct key_value_info {
+ const char *filename;
+ int linenr;
+};
+
+extern NORETURN void git_die_config(const char *key, const char *err, ...)
__attribute__((format(printf, 2, 3)));
+extern NORETURN void git_die_config_linenr(const char *key, 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 cf9124f..427850a 100644
--- a/config.c
+++ b/config.c
@@ -1224,11 +1224,6 @@ int git_config_with_options(config_fn_t fn, void *data,
return ret;
}
-struct key_value_info {
- const char *filename;
- int linenr;
-};
-
static void git_config_raw(config_fn_t fn, void *data)
{
if (git_config_with_options(fn, data, NULL, 1) 0)
@@ -1326,8 +1321,8 @@ static int
[PATCH v8 4/8] change `git_config()` return value to void
Currently `git_config()` returns an integer signifying an error code.
During rewrites of the function most of the code was shifted to
`git_config_with_options()`. `git_config_with_options()` normally
returns positive values if its `config_source` parameter is set as NULL,
as most errors are fatal, and non-fatal potential errors are guarded
by if statements that are entered only when no error is possible.
Still a negative value can be returned in case of race condition between
`access_or_die()` `git_config_from_file()`. Also, all callers of
`git_config()` ignore the return value except for one case in branch.c.
Change `git_config()` return value to void and make it die if it receives
a negative value from `git_config_with_options()`.
Original-patch-by: Matthieu Moy matthieu@imag.fr
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
branch.c | 5 +
cache.h | 2 +-
config.c | 16 ++--
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/branch.c b/branch.c
index 46e8aa8..735767d 100644
--- a/branch.c
+++ b/branch.c
@@ -161,10 +161,7 @@ int read_branch_desc(struct strbuf *buf, const char
*branch_name)
strbuf_addf(name, branch.%s.description, branch_name);
cb.config_name = name.buf;
cb.value = NULL;
- if (git_config(read_branch_desc_cb, cb) 0) {
- strbuf_release(name);
- return -1;
- }
+ git_config(read_branch_desc_cb, cb);
if (cb.value)
strbuf_addstr(buf, cb.value);
strbuf_release(name);
diff --git a/cache.h b/cache.h
index 0b1bdfd..f11ce41 100644
--- a/cache.h
+++ b/cache.h
@@ -1294,7 +1294,7 @@ extern int git_config_from_buf(config_fn_t fn, const char
*name,
const char *buf, size_t len, void *data);
extern void git_config_push_parameter(const char *text);
extern int git_config_from_parameters(config_fn_t fn, void *data);
-extern int git_config(config_fn_t fn, void *);
+extern void git_config(config_fn_t fn, void *);
extern int git_config_with_options(config_fn_t fn, void *,
struct git_config_source *config_source,
int respect_includes);
diff --git a/config.c b/config.c
index e4d745e..4cefa25 100644
--- a/config.c
+++ b/config.c
@@ -1230,9 +1230,21 @@ int git_config_with_options(config_fn_t fn, void *data,
return ret;
}
-int git_config(config_fn_t fn, void *data)
+void git_config(config_fn_t fn, void *data)
{
- return git_config_with_options(fn, data, NULL, 1);
+ if (git_config_with_options(fn, data, NULL, 1) 0)
+ /*
+* git_config_with_options() normally returns only
+* positive values, as most errors are fatal, and
+* non-fatal potential errors are guarded by if
+* statements that are entered only when no error is
+* possible.
+*
+* If we ever encounter a non-fatal error, it means
+* something went really wrong and we should stop
+* immediately.
+*/
+ die(_(unknown error occured while reading the configuration
files));
}
static struct config_set_element *configset_find_element(struct config_set
*cs, const char *key)
--
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
[PATCH v8 2/8] config.c: fix accuracy of line number in errors
From: Matthieu Moy matthieu@imag.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 tanay...@gmail.com
Signed-off-by: Tanay Abhra tanay...@gmail.com
Signed-off-by: Matthieu Moy matthieu@imag.fr
---
config.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/config.c b/config.c
index 34940fd..bb4629e 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
--
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 v8 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 tanay...@gmail.com
---
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
[PATCH v8 1/8] config.c: mark error and warnings strings for translation
From: Matthieu Moy matthieu@imag.fr
Signed-off-by: Matthieu Moy matthieu@imag.fr
---
config.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/config.c b/config.c
index a191328..34940fd 100644
--- a/config.c
+++ b/config.c
@@ -457,9 +457,9 @@ static int git_parse_source(config_fn_t fn, void *data)
break;
}
if (cf-die_on_error)
- die(bad config file line %d in %s, cf-linenr, cf-name);
+ die(_(bad config file line %d in %s), cf-linenr, cf-name);
else
- return error(bad config file line %d in %s, cf-linenr,
cf-name);
+ return error(_(bad config file line %d in %s), cf-linenr,
cf-name);
}
static int parse_unit_factor(const char *end, uintmax_t *val)
@@ -575,9 +575,9 @@ static void die_bad_number(const char *name, const char
*value)
value = ;
if (cf cf-name)
- die(bad numeric config value '%s' for '%s' in %s: %s,
+ die(_(bad numeric config value '%s' for '%s' in %s: %s),
value, name, cf-name, reason);
- die(bad numeric config value '%s' for '%s': %s, value, name, reason);
+ die(_(bad numeric config value '%s' for '%s': %s), value, name,
reason);
}
int git_config_int(const char *name, const char *value)
@@ -662,7 +662,7 @@ int git_config_pathname(const char **dest, const char *var,
const char *value)
return config_error_nonbool(var);
*dest = expand_user_path(value);
if (!*dest)
- die(Failed to expand user dir in: '%s', value);
+ die(_(failed to expand user dir in: '%s'), value);
return 0;
}
@@ -740,7 +740,7 @@ static int git_default_core_config(const char *var, const
char *value)
if (level == -1)
level = Z_DEFAULT_COMPRESSION;
else if (level 0 || level Z_BEST_COMPRESSION)
- die(bad zlib compression level %d, level);
+ die(_(bad zlib compression level %d), level);
zlib_compression_level = level;
zlib_compression_seen = 1;
return 0;
@@ -751,7 +751,7 @@ static int git_default_core_config(const char *var, const
char *value)
if (level == -1)
level = Z_DEFAULT_COMPRESSION;
else if (level 0 || level Z_BEST_COMPRESSION)
- die(bad zlib compression level %d, level);
+ die(_(bad zlib compression level %d), level);
core_compression_level = level;
core_compression_seen = 1;
if (!zlib_compression_seen)
@@ -873,7 +873,7 @@ static int git_default_core_config(const char *var, const
char *value)
else if (!strcmp(value, link))
object_creation_mode = OBJECT_CREATION_USES_HARDLINKS;
else
- die(Invalid mode for object creation: %s, value);
+ die(_(invalid mode for object creation: %s), value);
return 0;
}
@@ -1173,7 +1173,7 @@ int git_config_early(config_fn_t fn, void *data, const
char *repo_config)
switch (git_config_from_parameters(fn, data)) {
case -1: /* error */
- die(unable to parse command-line config);
+ die(_(unable to parse command-line config));
break;
case 0: /* found nothing */
break;
@@ -1514,7 +1514,7 @@ static int store_aux(const char *key, const char *value,
void *cb)
case KEY_SEEN:
if (matches(key, value)) {
if (store.seen == 1 store.multi_replace == 0) {
- warning(%s has multiple values, key);
+ warning(_(%s has multiple values), key);
}
ALLOC_GROW(store.offset, store.seen + 1,
--
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
[PATCH v8 5/8] config: add `git_die_config()` to the config-set API
Add `git_die_config` that dies printing the line number and the file name
of the highest priority value for the configuration variable `key`. A custom
error message is also printed before dying, specified by the caller, which can
be skipped if `err` argument is set to NULL.
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)){
if (!strcmp(value, foo))
git_config_die(key, value: `%s` is illegal, value);
else
/* do work */
}
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
Documentation/technical/api-config.txt | 13
cache.h| 3 +++
config.c | 39 --
3 files changed, 53 insertions(+), 2 deletions(-)
diff --git a/Documentation/technical/api-config.txt
b/Documentation/technical/api-config.txt
index 21f280c..0d8b99b 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -155,6 +155,19 @@ 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.
+`git_die_config(const char *key, const char *err, ...)`::
+
+ First prints the error message specified by the caller in `err` and then
+ dies printing the line number and the file name of the highest priority
+ value for the configuration variable `key`.
+
+`void git_die_config_linenr(const char *key, const char *filename, int
linenr)`::
+
+ Helper function which formats the die error message according to the
+ parameters entered. Used by `git_die_config()`. It can be used by
callers
+ handling `git_config_get_value_multi()` to print the correct error
message
+ for the desired value.
+
See test-config.c for usage examples.
Value Parsing Helpers
diff --git a/cache.h b/cache.h
index f11ce41..89a0d51 100644
--- a/cache.h
+++ b/cache.h
@@ -1388,6 +1388,9 @@ struct key_value_info {
int linenr;
};
+extern NORETURN void git_die_config(const char *key, const char *err, ...)
__attribute__((format(printf, 2, 3)));
+extern NORETURN void git_die_config_linenr(const char *key, 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 4cefa25..5ae9ab0 100644
--- a/config.c
+++ b/config.c
@@ -1469,8 +1469,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, NULL);
+ return ret;
}
int git_config_get_string(const char *key, char **dest)
@@ -1511,8 +1515,39 @@ 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, NULL);
+ return ret;
+}
+
+NORETURN
+void git_die_config_linenr(const char *key, const char *filename, int linenr)
+{
+ if (!filename)
+ die(_(unable to parse '%s' from command-line config), key);
+ else
+ die(_(bad config variable '%s' in file '%s' at line %d),
+ key, filename, linenr);
+}
+
+NORETURN __attribute__((format(printf, 2, 3)))
+void git_die_config(const char *key, const char *err, ...)
+{
+ const struct string_list *values;
+ struct key_value_info *kv_info;
+
+ if (err) {
+ va_list params;
+ va_start(params, err);
+ vreportf(error: , err, params);
+ va_end(params);
+ }
+ values = git_config_get_value_multi(key);
+ kv_info = values-items[values-nr - 1].util;
+ git_die_config_linenr(key, kv_info-filename, kv_info-linenr);
}
/*
--
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
[PATCH v8 6/8] rewrite git_config() to use the config-set API
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 tanay...@gmail.com
---
cache.h | 24 +++
config.c| 51 +
t/t4055-diff-context.sh | 2 +-
3 files changed, 68 insertions(+), 9 deletions(-)
diff --git a/cache.h b/cache.h
index 89a0d51..2693a37 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 5ae9ab0..427850a 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;
@@ -1230,7 +1224,7 @@ int git_config_with_options(config_fn_t fn, void *data,
return ret;
}
-void git_config(config_fn_t fn, void *data)
+static void git_config_raw(config_fn_t fn, void *data)
{
if (git_config_with_options(fn, data, NULL, 1) 0)
/*
@@ -1247,6 +1241,33 @@ void git_config(config_fn_t fn, void *data)
die(_(unknown error occured while reading the configuration
files));
}
+static void 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;
+ git_die_config_linenr(entry-key, kv_info-filename,
kv_info-linenr);
+ }
+ }
+}
+
+static void git_config_check_init(void);
+
+void git_config(config_fn_t fn, void *data)
+{
+ git_config_check_init();
+ 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;
@@ -1273,6 +1294,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);
@@ -1288,6 +1310,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;
@@ -1311,6 +1339,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
[PATCH v8 8/8] add tests for `git_config_get_string_const()`
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 tanay...@gmail.com
---
t/t1308-config-set.sh | 10 ++
test-config.c | 10 ++
2 files changed, 20 insertions(+)
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 5ea0aa4..ecc757a 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -119,6 +119,16 @@ 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
+ check_config expect_code 1 get_string case.ba Value not found for
\case.ba\
+'
+
+test_expect_success 'check line error when NULL string is queried' '
+ test_expect_code 128 test-config get_string case.foo 2result
+ grep line 7.*.git/config\|.git/config.*line 7 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
--
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 v8 7/8] add a test for semantic errors in config files
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 tanay...@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..5ea0aa4 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 2result + grep line 2.*.git/config\|.git/config.*line 2 result +' + test_done -- 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
Re: [PATCH 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family
On 8/5/2014 12:29 AM, Eric Sunshine wrote:
On Mon, Aug 4, 2014 at 2:33 PM, Tanay Abhra tanay...@gmail.com wrote:
Use `git_config_get_bool()` family instead of `git_config()` to take
advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
http-backend.c | 31 ---
1 file changed, 12 insertions(+), 19 deletions(-)
diff --git a/http-backend.c b/http-backend.c
index 80790bb..106ca6b 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -219,29 +219,22 @@ static void get_idx_file(char *name)
send_local_file(application/x-git-packed-objects-toc, name);
}
-static int http_config(const char *var, const char *value, void *cb)
+static void http_config(void)
{
- const char *p;
+ int i, value = 0;
+ struct strbuf var = STRBUF_INIT;
- if (!strcmp(var, http.getanyfile)) {
- getanyfile = git_config_bool(var, value);
- return 0;
- }
+ git_config_get_bool(http.getanyfile, getanyfile);
- if (skip_prefix(var, http., p)) {
- int i;
-
- for (i = 0; i ARRAY_SIZE(rpc_service); i++) {
- struct rpc_service *svc = rpc_service[i];
- if (!strcmp(p, svc-config_name)) {
- svc-enabled = git_config_bool(var, value);
- return 0;
- }
- }
+ for (i = 0; i ARRAY_SIZE(rpc_service); i++) {
+ struct rpc_service *svc = rpc_service[i];
+ strbuf_addf(var, http.%s, svc-config_name);
+ if (!git_config_get_bool(var.buf, value))
+ svc-enabled = value;
+ strbuf_reset(var);
}
There is a behavior change here. The original code set svc-enabled to
the first matching rpc_service[] entry, whereas the new code sets it
to the last matching entry. Is this change intentional?
I was preparing the reroll and I saw that I had missed your mail.
I think that I haven't changed the behaviour, the original one is
written in callback form so it has to go through the array every time for each
new value.
When there are multiple entries for a service say,
http.receivepack = 1
http.receivepack = 0
the old code would have at overwritten the previous entry with the new value.
The new code just populates the whole array of `rpc_service` in one go, choosing
the last matching value for each entry. For reviewing purpose the original
array is
this,
struct rpc_service {
const char *name;
const char *config_name;
signed enabled : 2;
};
static struct rpc_service rpc_service[] = {
{ upload-pack, uploadpack, 1 },
{ receive-pack, receivepack, -1 },
};
What do you think, am I interpreting it wrong? Thanks.
- /* we are not interested in parsing any other configuration here */
- return 0;
+ strbuf_release(var);
}
static struct rpc_service *select_service(const char *name)
@@ -627,7 +620,7 @@ int main(int argc, char **argv)
access(git-daemon-export-ok, F_OK) )
not_found(Repository not exported: '%s', dir);
- git_config(http_config, NULL);
+ http_config();
cmd-imp(cmd_arg);
return 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
Re: [PATCH v8 8/8] add tests for `git_config_get_string_const()`
On 8/6/2014 9:02 PM, Matthieu Moy wrote: Tanay Abhra tanay...@gmail.com writes: +test_expect_success 'find string value for a key' ' +check_config get_string case.baz hask +check_config expect_code 1 get_string case.ba Value not found for \case.ba\ +' + +test_expect_success 'check line error when NULL string is queried' ' +test_expect_code 128 test-config get_string case.foo 2result +grep line 7.*.git/config\|.git/config.*line 7 result +' This is still dependant on the locale (line is translated). You need to use test_i18ngrep instead of grep here (see its definition and comment in t/test-lib.sh). Oh, and I was searching t/README for a hint. Maybe we should add a line there to hint future readers. Thanks. :) I don't think you need these two alternatives OTOH. BTW, Junio, I don't understand your remark This test is too tight (the full string) in the previous iteration. Can you elaborate? I think he meant we must search for the relevant snippets instead of the whole string. -- 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] cache.h: add missing NORETURN on git_die_config*()
On 8/5/2014 7:27 PM, Ramsay Jones wrote: Commit 3a2a9527 (config: add `git_die_config()` to the config-set API, 01-08-2014) added git_die_config() and git_die_config_linenr() functions, but forgot to include the NORETURN attribute in their declarations. Sparse complains like so: SP config.c config.c:1567:6: error: symbol 'git_die_config_linenr' redeclared \ with different type (originally declared at cache.h:1419) \ - different modifiers config.c:1579:6: error: symbol 'git_die_config' redeclared with \ different type (originally declared at cache.h:1418) \ - different modifiers Suppress the sparse errors by adding NORETURN to the function declarations in the cache.h header file. Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk --- Hi Tanay, If you need to re-roll the patches in the 'ta/config-set-1' branch, could you please squash this into the relevant patch. Thanks! Sorry, my bad. I will squash it in the relevant patch for my next re roll. Thanks, Tanay Abhra. -- 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 v7 5/8] config: add `git_die_config()` to the config-set API
On 8/5/2014 1:34 AM, Matthieu Moy wrote:
Tanay Abhra tanay...@gmail.com writes:
I was aping the old git_config() system, it also does exactly what you
described
above. for example, builtin/gc.c line 91,
if (!strcmp(var, gc.pruneexpire)) {
if (value strcmp(value, now)) {
unsigned long now = approxidate(now);
if (approxidate(value) = now)
return error(_(Invalid %s: '%s'), var, value);
}
would print,
error: Invalid gc.pruneexpire: 'value'
fatal: bad config variable 'gc.pruneexpire' at file line 15 in
.git/config
It's good to do at least as well as the old system, but I agree with
Junio that it's suboptimal.
Having a single API call to replace
error('%s' must be between 1 and 3);
git_config_die(key);
with stg like:
git_config_die(key, '%s' must be between 1 and 3);
Matthieu, I have finished the new version, but instead of flooding the mailing
list with
a series again, I wanted to confirm if the new git_config_die() is alright.
NORETURN __attribute__((format(printf, 2, 3)))
void git_die_config(const char *key, const char *err, ...)
{
const struct string_list *values;
struct key_value_info *kv_info;
if (err) {
va_list params;
va_start(params, err);
vreportf(error: , err, params);
va_end(params);
}
values = git_config_get_value_multi(key);
kv_info = values-items[values-nr - 1].util;
git_die_config_linenr(key, kv_info-filename, kv_info-linenr);
}
Currently works like the old git_config() error reporting path. If err is set
to NULL,
it would print no error message and just the die message. If given something
like,
git_config_die(key, value '%s' is not allowed, value);
it would print,
error: value '3' is not allowed
fatal: bad config variable 'core.frotz' at file line 15 in .git/config
Cheers,
Tanay Abhra.
in Junio's example would allow git_config_die to format the error
message the way it likes (i.e. it can be the same as before when you
introduce it, and improve afterwards).
I've never been disturbed by the quality of Git's error messages wrt
config files (it's not a compiler!), so having good quality messages is
not a high priority IMHO, but having a good API that as a side effect
can produce good error messages is important. If changing the error
messages requires rewritting all callers later, then we've missed the
point doing the refactoring now.
--
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] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure
`git_pretty_formats_config()` continues without checking git_config_string's
return value which can lead to a SEGFAULT. Instead return -1 when
git_config_string fails signalling `git_config()` to die printing the location
of the erroneous variable.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
pretty.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/pretty.c b/pretty.c
index 3a1da6f..72dbf55 100644
--- a/pretty.c
+++ b/pretty.c
@@ -65,7 +65,9 @@ static int git_pretty_formats_config(const char *var, const
char *value, void *c
commit_format-name = xstrdup(name);
commit_format-format = CMIT_FMT_USERFORMAT;
- git_config_string(fmt, var, value);
+ if (git_config_string(fmt, var, value))
+ return -1;
+
if (starts_with(fmt, format:) || starts_with(fmt, tformat:)) {
commit_format-is_tformat = fmt[0] == 't';
fmt = strchr(fmt, ':') + 1;
--
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
[PATCH 00/11] git_config callers rewritten with the new config-set API
The ta/config-set API is more or less solidified. This series builds on the top of 4c715ebb in pu (ta/config-set). On top of it, it also requires series [1] (Rewrite `git_config()` using config-set API) for proper error checking. This series is the first batch of patches which rewrites the existing callers using a non-callback approach. This series aims to, * rewrite the existing callers, as you can see from the diff stat the bew API provides a much concise and clear control flow. * stress test the new API, see if any corner cases or deficiencies arise or not. The series passes all the tests, only thing to watch is that the config variables that have been rewritten are single valued only. Though I have tried my best to ascertain it, still mistakes may arise. p.s: I haven't decided yet about whether to introduce a new helper set, for example git_config_get_value_fmt() which would behave like strbuf_addf(). It will probably come in a later series. [1]: http://thread.gmane.org/gmane.comp.version-control.git/254633/ Tanay Abhra (11): alias.c| 25 ++-- archive.c | 12 +++- branch.c | 27 +++--- builtin/gc.c | 51 +++- daemon.c | 27 +- fetch-pack.c | 35 - http-backend.c | 31 - imap-send.c| 61 ++ pager.c| 40 +- read-cache.c | 14 +++--- rerere.c | 43 - 11 files changed, 116 insertions(+), 250 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
[PATCH 02/11] http-backend.c: replace `git_config()` with `git_config_get_bool()` family
Use `git_config_get_bool()` family instead of `git_config()` to take advantage
of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
http-backend.c | 31 ---
1 file changed, 12 insertions(+), 19 deletions(-)
diff --git a/http-backend.c b/http-backend.c
index 80790bb..106ca6b 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -219,29 +219,22 @@ static void get_idx_file(char *name)
send_local_file(application/x-git-packed-objects-toc, name);
}
-static int http_config(const char *var, const char *value, void *cb)
+static void http_config(void)
{
- const char *p;
+ int i, value = 0;
+ struct strbuf var = STRBUF_INIT;
- if (!strcmp(var, http.getanyfile)) {
- getanyfile = git_config_bool(var, value);
- return 0;
- }
+ git_config_get_bool(http.getanyfile, getanyfile);
- if (skip_prefix(var, http., p)) {
- int i;
-
- for (i = 0; i ARRAY_SIZE(rpc_service); i++) {
- struct rpc_service *svc = rpc_service[i];
- if (!strcmp(p, svc-config_name)) {
- svc-enabled = git_config_bool(var, value);
- return 0;
- }
- }
+ for (i = 0; i ARRAY_SIZE(rpc_service); i++) {
+ struct rpc_service *svc = rpc_service[i];
+ strbuf_addf(var, http.%s, svc-config_name);
+ if (!git_config_get_bool(var.buf, value))
+ svc-enabled = value;
+ strbuf_reset(var);
}
- /* we are not interested in parsing any other configuration here */
- return 0;
+ strbuf_release(var);
}
static struct rpc_service *select_service(const char *name)
@@ -627,7 +620,7 @@ int main(int argc, char **argv)
access(git-daemon-export-ok, F_OK) )
not_found(Repository not exported: '%s', dir);
- git_config(http_config, NULL);
+ http_config();
cmd-imp(cmd_arg);
return 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
[PATCH 06/11] rerere.c: replace `git_config()` with `git_config_get_*()` family
Use `git_config_get_*()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
rerere.c | 43 ---
1 file changed, 12 insertions(+), 31 deletions(-)
diff --git a/rerere.c b/rerere.c
index d84b495..20b18ad 100644
--- a/rerere.c
+++ b/rerere.c
@@ -573,15 +573,11 @@ static int do_plain_rerere(struct string_list *rr, int fd)
return write_rr(rr, fd);
}
-static int git_rerere_config(const char *var, const char *value, void *cb)
+static void git_rerere_config(void)
{
- if (!strcmp(var, rerere.enabled))
- rerere_enabled = git_config_bool(var, value);
- else if (!strcmp(var, rerere.autoupdate))
- rerere_autoupdate = git_config_bool(var, value);
- else
- return git_default_config(var, value, cb);
- return 0;
+ git_config_get_bool(rerere.enabled, rerere_enabled);
+ git_config_get_bool(rerere.autoupdate, rerere_autoupdate);
+ git_config(git_default_config, NULL);
}
static int is_rerere_enabled(void)
@@ -606,7 +602,7 @@ int setup_rerere(struct string_list *merge_rr, int flags)
{
int fd;
- git_config(git_rerere_config, NULL);
+ git_rerere_config();
if (!is_rerere_enabled())
return -1;
@@ -699,24 +695,6 @@ static void unlink_rr_item(const char *name)
rmdir(git_path(rr-cache/%s, name));
}
-struct rerere_gc_config_cb {
- int cutoff_noresolve;
- int cutoff_resolve;
-};
-
-static int git_rerere_gc_config(const char *var, const char *value, void *cb)
-{
- struct rerere_gc_config_cb *cf = cb;
-
- if (!strcmp(var, gc.rerereresolved))
- cf-cutoff_resolve = git_config_int(var, value);
- else if (!strcmp(var, gc.rerereunresolved))
- cf-cutoff_noresolve = git_config_int(var, value);
- else
- return git_default_config(var, value, cb);
- return 0;
-}
-
void rerere_gc(struct string_list *rr)
{
struct string_list to_remove = STRING_LIST_INIT_DUP;
@@ -724,9 +702,12 @@ void rerere_gc(struct string_list *rr)
struct dirent *e;
int i, cutoff;
time_t now = time(NULL), then;
- struct rerere_gc_config_cb cf = { 15, 60 };
+ int cutoff_noresolve = 15;
+ int cutoff_resolve = 60;
- git_config(git_rerere_gc_config, cf);
+ git_config_get_int(gc.rerereresolved, cutoff_resolve);
+ git_config_get_int(gc.rerereunresolved, cutoff_noresolve);
+ git_config(git_default_config, NULL);
dir = opendir(git_path(rr-cache));
if (!dir)
die_errno(unable to open rr-cache directory);
@@ -736,12 +717,12 @@ void rerere_gc(struct string_list *rr)
then = rerere_last_used_at(e-d_name);
if (then) {
- cutoff = cf.cutoff_resolve;
+ cutoff = cutoff_resolve;
} else {
then = rerere_created_at(e-d_name);
if (!then)
continue;
- cutoff = cf.cutoff_noresolve;
+ cutoff = cutoff_noresolve;
}
if (then now - cutoff * 86400)
string_list_append(to_remove, e-d_name);
--
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
[PATCH 05/11] fetchpack.c: replace `git_config()` with `git_config_get_*()` family
Use `git_config_get_*()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
fetch-pack.c | 35 ---
1 file changed, 8 insertions(+), 27 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index b8a58fa..a13e9db 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -869,34 +869,15 @@ static struct ref *do_fetch_pack(struct fetch_pack_args
*args,
return ref;
}
-static int fetch_pack_config(const char *var, const char *value, void *cb)
+static void fetch_pack_config(void)
{
- if (strcmp(var, fetch.unpacklimit) == 0) {
- fetch_unpack_limit = git_config_int(var, value);
- return 0;
- }
-
- if (strcmp(var, transfer.unpacklimit) == 0) {
- transfer_unpack_limit = git_config_int(var, value);
- return 0;
- }
-
- if (strcmp(var, repack.usedeltabaseoffset) == 0) {
- prefer_ofs_delta = git_config_bool(var, value);
- return 0;
- }
-
- if (!strcmp(var, fetch.fsckobjects)) {
- fetch_fsck_objects = git_config_bool(var, value);
- return 0;
- }
-
- if (!strcmp(var, transfer.fsckobjects)) {
- transfer_fsck_objects = git_config_bool(var, value);
- return 0;
- }
+ git_config_get_int(fetch.unpacklimit, fetch_unpack_limit);
+ git_config_get_int(transfer.unpacklimit, transfer_unpack_limit);
+ git_config_get_bool(repack.usedeltabaseoffset, prefer_ofs_delta);
+ git_config_get_bool(fetch.fsckobjects, fetch_fsck_objects);
+ git_config_get_bool(transfer.fsckobjects, transfer_fsck_objects);
- return git_default_config(var, value, cb);
+ git_config(git_default_config, NULL);
}
static void fetch_pack_setup(void)
@@ -904,7 +885,7 @@ static void fetch_pack_setup(void)
static int did_setup;
if (did_setup)
return;
- git_config(fetch_pack_config, NULL);
+ fetch_pack_config();
if (0 = transfer_unpack_limit)
unpack_limit = transfer_unpack_limit;
else if (0 = fetch_unpack_limit)
--
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
[PATCH 04/11] archive.c: replace `git_config()` with `git_config_get_bool()` family
Use `git_config_get_bool()` family instead of `git_config()` to take advantage
of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
archive.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/archive.c b/archive.c
index 3fc0fb2..952a659 100644
--- a/archive.c
+++ b/archive.c
@@ -402,14 +402,6 @@ static int parse_archive_args(int argc, const char **argv,
return argc;
}
-static int git_default_archive_config(const char *var, const char *value,
- void *cb)
-{
- if (!strcmp(var, uploadarchive.allowunreachable))
- remote_allow_unreachable = git_config_bool(var, value);
- return git_default_config(var, value, cb);
-}
-
int write_archive(int argc, const char **argv, const char *prefix,
int setup_prefix, const char *name_hint, int remote)
{
@@ -420,7 +412,9 @@ int write_archive(int argc, const char **argv, const char
*prefix,
if (setup_prefix prefix == NULL)
prefix = setup_git_directory_gently(nongit);
- git_config(git_default_archive_config, NULL);
+ git_config_get_bool(uploadarchive.allowunreachable,
remote_allow_unreachable);
+ git_config(git_default_config, NULL);
+
init_tar_archiver();
init_zip_archiver();
--
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
[PATCH 03/11] read-cache.c: replace `git_config()` with `git_config_get_*()` family
Use `git_config_get_*()` family instead of `git_config()` to take
advantage of the config-set API which provides a cleaner control flow.
Use an intermediate value, as `version` can not be used directly in
git_config_get_int() due to incompatible type.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
read-cache.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index 5d3c8bd..acb132d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1238,24 +1238,16 @@ static struct cache_entry *refresh_cache_entry(struct
cache_entry *ce,
#define INDEX_FORMAT_DEFAULT 3
-static int index_format_config(const char *var, const char *value, void *cb)
-{
- unsigned int *version = cb;
- if (!strcmp(var, index.version)) {
- *version = git_config_int(var, value);
- return 0;
- }
- return 1;
-}
-
static unsigned int get_index_format_default(void)
{
char *envversion = getenv(GIT_INDEX_VERSION);
char *endp;
+ int value;
unsigned int version = INDEX_FORMAT_DEFAULT;
if (!envversion) {
- git_config(index_format_config, version);
+ if (!git_config_get_int(index.version, value))
+ version = value;
if (version INDEX_FORMAT_LB || INDEX_FORMAT_UB version) {
warning(_(index.version set, but the value is
invalid.\n
Using version %i), INDEX_FORMAT_DEFAULT);
--
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
[PATCH 01/11] daemon.c: replace `git_config()` with `git_config_get_bool()` family
Use `git_config_get_bool()` family instead of `git_config()` to take advantage
of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
daemon.c | 27 +--
1 file changed, 5 insertions(+), 22 deletions(-)
diff --git a/daemon.c b/daemon.c
index e6b51ed..fb16664 100644
--- a/daemon.c
+++ b/daemon.c
@@ -230,23 +230,6 @@ struct daemon_service {
int overridable;
};
-static struct daemon_service *service_looking_at;
-static int service_enabled;
-
-static int git_daemon_config(const char *var, const char *value, void *cb)
-{
- const char *service;
-
- if (skip_prefix(var, daemon., service)
- !strcmp(service, service_looking_at-config_name)) {
- service_enabled = git_config_bool(var, value);
- return 0;
- }
-
- /* we are not interested in parsing any other configuration here */
- return 0;
-}
-
static int daemon_error(const char *dir, const char *msg)
{
if (!informative_errors)
@@ -324,6 +307,7 @@ static int run_service(const char *dir, struct
daemon_service *service)
{
const char *path;
int enabled = service-enabled;
+ struct strbuf var = STRBUF_INIT;
loginfo(Request %s for '%s', service-name, dir);
@@ -354,12 +338,11 @@ static int run_service(const char *dir, struct
daemon_service *service)
}
if (service-overridable) {
- service_looking_at = service;
- service_enabled = -1;
- git_config(git_daemon_config, NULL);
- if (0 = service_enabled)
- enabled = service_enabled;
+ strbuf_addf(var, daemon.%s, service-config_name);
+ git_config_get_bool(var.buf, enabled);
+ strbuf_release(var);
}
+
if (!enabled) {
logerror('%s': service not enabled for '%s',
service-name, path);
--
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
[PATCH 07/11] builtin/gc.c: replace `git_config()` with `git_config_get_*()` family
Use `git_config_get_*()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
builtin/gc.c | 51 ---
1 file changed, 20 insertions(+), 31 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index 8d219d8..4612ef5 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -55,44 +55,33 @@ static void remove_pidfile_on_signal(int signo)
raise(signo);
}
-static int gc_config(const char *var, const char *value, void *cb)
+static void gc_config(void)
{
- if (!strcmp(var, gc.packrefs)) {
+ const char *value;
+
+ if (!git_config_get_value(gc.packrefs, value)) {
if (value !strcmp(value, notbare))
pack_refs = -1;
else
- pack_refs = git_config_bool(var, value);
- return 0;
- }
- if (!strcmp(var, gc.aggressivewindow)) {
- aggressive_window = git_config_int(var, value);
- return 0;
- }
- if (!strcmp(var, gc.aggressivedepth)) {
- aggressive_depth = git_config_int(var, value);
- return 0;
- }
- if (!strcmp(var, gc.auto)) {
- gc_auto_threshold = git_config_int(var, value);
- return 0;
- }
- if (!strcmp(var, gc.autopacklimit)) {
- gc_auto_pack_limit = git_config_int(var, value);
- return 0;
+ pack_refs = git_config_bool(gc.packrefs, value);
}
- if (!strcmp(var, gc.autodetach)) {
- detach_auto = git_config_bool(var, value);
- return 0;
- }
- if (!strcmp(var, gc.pruneexpire)) {
- if (value strcmp(value, now)) {
+
+ git_config_get_int(gc.aggressivewindow, aggressive_window);
+ git_config_get_int(gc.aggressivedepth, aggressive_depth);
+ git_config_get_int(gc.auto, gc_auto_threshold);
+ git_config_get_int(gc.autopacklimit, gc_auto_pack_limit);
+ git_config_get_bool(gc.autodetach, detach_auto);
+
+ if (!git_config_get_string_const(gc.pruneexpire, prune_expire)) {
+ if (strcmp(prune_expire, now)) {
unsigned long now = approxidate(now);
- if (approxidate(value) = now)
- return error(_(Invalid %s: '%s'), var, value);
+ if (approxidate(prune_expire) = now) {
+ error(_(Invalid %s: '%s'), gc.pruneexpire,
prune_expire);
+ git_die_config(gc.pruneexpire);
+ }
}
- return git_config_string(prune_expire, var, value);
}
- return git_default_config(var, value, cb);
+ git_config(git_default_config, NULL);
}
static int too_many_loose_objects(void)
@@ -301,7 +290,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
argv_array_pushl(prune, prune, --expire, NULL );
argv_array_pushl(rerere, rerere, gc, NULL);
- git_config(gc_config, NULL);
+ gc_config();
if (pack_refs 0)
pack_refs = !is_bare_repository();
--
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
[PATCH 08/11] pager.c: replace `git_config()` with `git_config_get_value()`
Use `git_config_get_value()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
pager.c | 40 +---
1 file changed, 13 insertions(+), 27 deletions(-)
diff --git a/pager.c b/pager.c
index 8b5cbc5..b7eb7e7 100644
--- a/pager.c
+++ b/pager.c
@@ -6,12 +6,6 @@
#define DEFAULT_PAGER less
#endif
-struct pager_config {
- const char *cmd;
- int want;
- char *value;
-};
-
/*
* This is split up from the rest of git so that we can do
* something different on Windows.
@@ -155,30 +149,22 @@ int decimal_width(int number)
return width;
}
-static int pager_command_config(const char *var, const char *value, void *data)
+/* returns 0 for no pager, 1 for use pager, and -1 for not specified */
+int check_pager_config(const char *cmd)
{
- struct pager_config *c = data;
- if (starts_with(var, pager.) !strcmp(var + 6, c-cmd)) {
- int b = git_config_maybe_bool(var, value);
+ int want = -1;
+ struct strbuf key = STRBUF_INIT;
+ const char *value = NULL;
+ strbuf_addf(key, pager.%s, cmd);
+ if (!git_config_get_value(key.buf, value)) {
+ int b = git_config_maybe_bool(key.buf, value);
if (b = 0)
- c-want = b;
+ want = b;
else {
- c-want = 1;
- c-value = xstrdup(value);
+ want = 1;
+ pager_program = xstrdup(value);
}
}
- return 0;
-}
-
-/* returns 0 for no pager, 1 for use pager, and -1 for not specified */
-int check_pager_config(const char *cmd)
-{
- struct pager_config c;
- c.cmd = cmd;
- c.want = -1;
- c.value = NULL;
- git_config(pager_command_config, c);
- if (c.value)
- pager_program = c.value;
- return c.want;
+ strbuf_release(key);
+ return want;
}
--
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
[PATCH 11/11] branch.c: replace `git_config()` with `git_config_get_string()
Use `git_config_get_string()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
branch.c | 27 +++
1 file changed, 7 insertions(+), 20 deletions(-)
diff --git a/branch.c b/branch.c
index 735767d..df6b120 100644
--- a/branch.c
+++ b/branch.c
@@ -140,30 +140,17 @@ static int setup_tracking(const char *new_ref, const char
*orig_ref,
return 0;
}
-struct branch_desc_cb {
- const char *config_name;
- const char *value;
-};
-
-static int read_branch_desc_cb(const char *var, const char *value, void *cb)
-{
- struct branch_desc_cb *desc = cb;
- if (strcmp(desc-config_name, var))
- return 0;
- free((char *)desc-value);
- return git_config_string(desc-value, var, value);
-}
-
int read_branch_desc(struct strbuf *buf, const char *branch_name)
{
- struct branch_desc_cb cb;
+ char *v = NULL;
struct strbuf name = STRBUF_INIT;
strbuf_addf(name, branch.%s.description, branch_name);
- cb.config_name = name.buf;
- cb.value = NULL;
- git_config(read_branch_desc_cb, cb);
- if (cb.value)
- strbuf_addstr(buf, cb.value);
+ if (git_config_get_string(name.buf, v)) {
+ strbuf_release(name);
+ return -1;
+ }
+ strbuf_addstr(buf, v);
+ free(v);
strbuf_release(name);
return 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
[PATCH 10/11] alias.c: replace `git_config()` with `git_config_get_string()`
Use `git_config_get_string()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
alias.c | 25 ++---
1 file changed, 6 insertions(+), 19 deletions(-)
diff --git a/alias.c b/alias.c
index 758c867..6aa164a 100644
--- a/alias.c
+++ b/alias.c
@@ -1,26 +1,13 @@
#include cache.h
-static const char *alias_key;
-static char *alias_val;
-
-static int alias_lookup_cb(const char *k, const char *v, void *cb)
-{
- const char *name;
- if (skip_prefix(k, alias., name) !strcmp(name, alias_key)) {
- if (!v)
- return config_error_nonbool(k);
- alias_val = xstrdup(v);
- return 0;
- }
- return 0;
-}
-
char *alias_lookup(const char *alias)
{
- alias_key = alias;
- alias_val = NULL;
- git_config(alias_lookup_cb, NULL);
- return alias_val;
+ char *v = NULL;
+ struct strbuf key = STRBUF_INIT;
+ strbuf_addf(key, alias.%s, alias);
+ git_config_get_string(key.buf, v);
+ strbuf_release(key);
+ return v;
}
#define SPLIT_CMDLINE_BAD_ENDING 1
--
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
[PATCH 09/11] imap-send.c: replace `git_config()` with `git_config_get_*()` family
Use `git_config_get_*()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
imap-send.c | 61 +++--
1 file changed, 27 insertions(+), 34 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 524fbab..586bdd8 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1326,43 +1326,36 @@ static int split_msg(struct strbuf *all_msgs, struct
strbuf *msg, int *ofs)
static char *imap_folder;
-static int git_imap_config(const char *key, const char *val, void *cb)
+static void git_imap_config(void)
{
- if (!skip_prefix(key, imap., key))
- return 0;
+ const char *val = NULL;
+
+ git_config_get_bool(imap.sslverify, server.ssl_verify);
+ git_config_get_bool(imap.preformattedhtml, server.use_html);
+ git_config_get_string(imap.folder, imap_folder);
- /* check booleans first, and barf on others */
- if (!strcmp(sslverify, key))
- server.ssl_verify = git_config_bool(key, val);
- else if (!strcmp(preformattedhtml, key))
- server.use_html = git_config_bool(key, val);
- else if (!val)
- return config_error_nonbool(key);
-
- if (!strcmp(folder, key)) {
- imap_folder = xstrdup(val);
- } else if (!strcmp(host, key)) {
- if (starts_with(val, imap:))
- val += 5;
- else if (starts_with(val, imaps:)) {
- val += 6;
- server.use_ssl = 1;
+ if (!git_config_get_value(imap.host, val)) {
+ if (!val) {
+ config_error_nonbool(imap.host);
+ git_die_config(imap.host);
+ } else {
+ if (starts_with(val, imap:))
+ val += 5;
+ else if (starts_with(val, imaps:)) {
+ val += 6;
+ server.use_ssl = 1;
+ }
+ if (starts_with(val, //))
+ val += 2;
+ server.host = xstrdup(val);
}
- if (starts_with(val, //))
- val += 2;
- server.host = xstrdup(val);
- } else if (!strcmp(user, key))
- server.user = xstrdup(val);
- else if (!strcmp(pass, key))
- server.pass = xstrdup(val);
- else if (!strcmp(port, key))
- server.port = git_config_int(key, val);
- else if (!strcmp(tunnel, key))
- server.tunnel = xstrdup(val);
- else if (!strcmp(authmethod, key))
- server.auth_method = xstrdup(val);
+ }
- return 0;
+ git_config_get_string(imap.user, server.user);
+ git_config_get_string(imap.pass, server.pass);
+ git_config_get_int(imap.port, server.port);
+ git_config_get_string(imap.tunnel, server.tunnel);
+ git_config_get_string(imap.authmethod, server.auth_method);
}
int main(int argc, char **argv)
@@ -1383,7 +1376,7 @@ int main(int argc, char **argv)
usage(imap_send_usage);
setup_git_directory_gently(nongit_ok);
- git_config(git_imap_config, NULL);
+ git_imap_config();
if (!server.port)
server.port = server.use_ssl ? 993 : 143;
--
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
Re: [PATCH v7 5/8] config: add `git_die_config()` to the config-set API
On 8/4/2014 11:37 PM, Junio C Hamano wrote:
Tanay Abhra tanay...@gmail.com writes:
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 */
}
It feels a bit unnatural at the API level that this does not take
'value'; I do understand that it is not a big deal in the error code
path to locate again the value from the configuration using the key,
but still.
But, we don't have a use for value as it is not denoted in the error
string, that is why I left it out.
It feels even more unnatural that the caller cannot say _how_ it
finds the value offending by not taking any message. For one
particular callchain, e.g. git_config_get_string() that eventually
calls git_config_string() which will show an error message via
config_error_nonbool(), you may not want any extra message, but for
new callers that wants to make sure value falls within a supported
range, this forces it to write
if (!git_config_get_int(key, num)) {
if (!(0 num num 4)) {
error('%s' must be between 1 and 3);
git_config_die(key);
}
/* otherwise work */
}
and then the error message would say something like:
error: 'core.frotz' must be between 1 and 3
fatal: bad config variable 'core.frotz' at file line 15 in .git/config
which sounds somewhat backwards, at least to me.
I was aping the old git_config() system, it also does exactly what you described
above. for example, builtin/gc.c line 91,
if (!strcmp(var, gc.pruneexpire)) {
if (value strcmp(value, now)) {
unsigned long now = approxidate(now);
if (approxidate(value) = now)
return error(_(Invalid %s: '%s'), var, value);
}
would print,
error: Invalid gc.pruneexpire: 'value'
fatal: bad config variable 'gc.pruneexpire' at file line 15 in
.git/config
or imap-send.c line 1340,
if (!strcmp(sslverify, key))
server.ssl_verify = git_config_bool(key, val);
else if (!strcmp(preformattedhtml, key))
server.use_html = git_config_bool(key, val);
else if (!val)
return config_error_nonbool(key);
again would cause a error die, message combo as above. There are many
examples like that.
We can easily take a custom error message but again I was just aping the old
system.
+NORETURN
+void git_die_config_linenr(const char *key, const char *filename, int
linenr)
+{
+if (!linenr)
+die(_(unable to parse '%s' from command-line config), key);
Do we have existing code that says we signal that it is from the
command line by setting linenr to zero already? Otherwise I would
have thought filename == NULL would be a more sensible convention.
Otherwise OK.
Noted. Next reroll will have filename as the convention.
+else
+die(_(bad config variable '%s' at file line %d in %s),
At least, quote the last '%s'.
Noted. 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 v6 6/7] config: add `git_die_config()` to the config-set API
On 8/1/2014 12:11 AM, Matthieu Moy wrote:
Tanay Abhra tanay...@gmail.com writes:
On 7/31/2014 11:39 PM, Matthieu Moy wrote:
This is the part I find weird. You're calling git_die_config_exact() on
the first boggus value, and git_die_config_exact() will notify an error
at the line of the last boggus value.
Hmn, we may have some confusion here. I meant the implementation of
git_config_exact() to look like this,
void git_die_config_exact(const char *key, const char *value)
{
int i;
const struct string_list *values;
struct key_value_info *kv_info;
values = git_config_get_value_multi(key);
for (i = 0; i values.nr; i++) {
kv_info = values-items[i].util;
/* A null check will be here also */
if (!strcmp(value, values-items[i].string)) {
if (!kv_info-linenr)
die(_(unable to parse '%s' from command-line
config), key);
else
die(_(bad config variable '%s' at file line %d
in %s),
key,
kv_info-linenr,
kv_info-filename);
}
}
}
The above code would print the info on first bogus value.
OK, I got confused because git_die_config() errors out at the first
error. So, your version works, but I do not see any added value in this
extra complexity.
To be cleary, my version would be
NORETURN static /* not sure about the static */
void git_die_config_linenr(const char *key,
const char *filename, int linenr)
{
if (!linenr)
die(_(unable to parse '%s' from command-line config), key);
else
die(_(bad config variable '%s' at file line %d in %s),
key,
linenr,
filename);
}
(hmm, I actually do not need the value, it's not printed)
NORETURN
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;
git_die_config_linenr(key, kv_info-linenr, kv_info-filename);
}
(we forgot the NORETURN in previous version BTW. It should be there in
both functions here and in the .h file)
In my version, git_die_config uses git_die_config_linenr, and there's no
issue with first/last boggus value. Callers of git_die_config_linenr
have all the information to call it directly.
There are 3 code path that leads to the final die() calls, and having
this little helper allows making sure the messages are the same for
every callers, by construction (as opposed to cut-and-pasting).
Really, I don't see any reason to do anything more complex.
Thanks, I am sending your version with the reroll.
Also, for clarity the multi value use case would look like,
struct key_value_info *kv_info;
if (load_config_refs) {
values = git_config_get_value_multi(notes.displayref);
if (values) {
for (i = 0; i values-nr; i++) {
if (!values-items[i].string) {
config_error_nonbool(notes.displayref);
kv_info = values-items[i].util;
git_die_config_linenr(notes.displayref,
kv_info-filename,
kv_info-linenr);
}
else
string_list_add_refs_by_glob(display_notes_refs,
values-items[i].string);
}
}
}
with my function it would have looked like,
if (load_config_refs) {
values = git_config_get_value_multi(notes.displayref);
if (values) {
for (i = 0; i values-nr; i++) {
if (!values-items[i].string) {
config_error_nonbool(notes.displayref);
git_die_config_exact(notes.displayref, values-items[i].string);
}
else
string_list_add_refs_by_glob(display_notes_refs,
values-items[i].string);
}
}
}
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
[PATCH v7 5/8] config: add `git_die_config()` to the config-set API
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 tanay...@gmail.com
---
Documentation/technical/api-config.txt | 12
cache.h| 2 ++
config.c | 34 --
3 files changed, 46 insertions(+), 2 deletions(-)
diff --git a/Documentation/technical/api-config.txt
b/Documentation/technical/api-config.txt
index 815c1ee..d6a2c39 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -155,6 +155,18 @@ 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`.
+
+`void git_die_config_linenr(const char *key, const char *filename, int
linenr)`::
+
+ Helper function which formats the die error message according to the
+ parameters entered. Used by `git_die_config()`. It can be used by
callers
+ handling `git_config_get_value_multi()` to print the correct error
message
+ for the desired value.
+
See test-config.c for usage examples.
Value Parsing Helpers
diff --git a/cache.h b/cache.h
index c61919d..9ec2f4e 100644
--- a/cache.h
+++ b/cache.h
@@ -1382,6 +1382,8 @@ 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 void git_die_config_linenr(const char *key, 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 f923b1c..ddbfe4d 100644
--- a/config.c
+++ b/config.c
@@ -1474,8 +1474,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)
@@ -1516,8 +1520,34 @@ 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;
+}
+
+NORETURN
+void git_die_config_linenr(const char *key, const char *filename, int linenr)
+{
+ if (!linenr)
+ die(_(unable to parse '%s' from command-line config), key);
+ else
+ die(_(bad config variable '%s' at file line %d in %s),
+ key,
+ linenr,
+ filename);
+}
+
+NORETURN
+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;
+ git_die_config_linenr(key, kv_info-filename, kv_info-linenr);
}
/*
--
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
[PATCH v7 2/8] config.c: fix accuracy of line number in errors
From: Matthieu Moy matthieu@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 tanay...@gmail.com
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
config.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/config.c b/config.c
index 34940fd..bb4629e 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
--
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 v7 0/8] Rewrite `git_config()` using config-set API
[Patch v7]: style nit corrected. (1/8) is Matthieu's translation patch. git_die_config_linenr() helper function added. Diff between v6 and v7 appended for review. [Patch v6]: Added _() to error messages. Diff between v6 and v4 at the bottom. [PATCH v5]: New patch added (3/7). git_config() now returns void. [PATCH v4]: One style nit corrected, also added key to error messages. [PATCH V3]:All the suggestions in [3] applied. Built on top of [1]. [PATCH V2]: All the suggestions in [2] incorporated. git_config() now follows correct parsing order. Reordered the patches. Removed xfuncname patch as it was unnecssary. This series builds on the top of topic[1] in the mailing list with name git config cache special querying API utilizing the cache. This series aims to do these three things, * Use the config-set API to rewrite git_config(). * Solve any legacy bugs in the previous system while at it. * To be feature complete compared to the previous git_config() implementation, which I think it is now. (added the line number and file name info just for completeness) Also, I haven't yet checked the exact improvements but still as a teaser, git status now only rereads the configuration files twice instead of four times. [1]: http://thread.gmane.org/gmane.comp.version-control.git/254286 [2]: http://thread.gmane.org/gmane.comp.version-control.git/254101 [3]: http://thread.gmane.org/gmane.comp.version-control.git/254211 Matthieu Moy (1): config.c: mark error and warnings strings for translation Tanay Abhra (7): config.c: fix accuracy of line number in errors add line number and file name info to `config_set` change `git_config()` return value to void config: add `git_die_config()` to the config-set API rewrite git_config() to use the config-set API add a test for semantic errors in config files add tests for `git_config_get_string_const()` Documentation/technical/api-config.txt | 12 +++ branch.c | 5 +- cache.h| 28 +- config.c | 152 +++-- t/t1308-config-set.sh | 21 + t/t4055-diff-context.sh| 2 +- test-config.c | 10 +++ 7 files changed, 200 insertions(+), 30 deletions(-) -- 1.9.0.GIT -- 8 -- diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt index e7ec7cc..d6a2c39 100644 --- a/Documentation/technical/api-config.txt +++ b/Documentation/technical/api-config.txt @@ -160,6 +160,13 @@ as well as retrieval for the queried variable, including: Dies printing the line number and the file name of the highest priority value for the configuration variable `key`. +`void git_die_config_linenr(const char *key, const char *filename, int linenr)`:: + + Helper function which formats the die error message according to the + parameters entered. Used by `git_die_config()`. It can be used by callers + handling `git_config_get_value_multi()` to print the correct error message + for the desired value. + See test-config.c for usage examples. Value Parsing Helpers diff --git a/cache.h b/cache.h index 243f618..ff17889 100644 --- a/cache.h +++ b/cache.h @@ -1407,6 +1407,7 @@ 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 void git_die_config_linenr(const char *key, 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 15fcd91..cf9124f 100644 --- a/config.c +++ b/config.c @@ -461,9 +461,9 @@ static int git_parse_source(config_fn_t fn, void *data) break; } if (cf-die_on_error) - die(bad config file line %d in %s, cf-linenr, cf-name); + die(_(bad config file line %d in %s), cf-linenr, cf-name); else - return error(bad config file line %d in %s, cf-linenr, cf-name); + return error(_(bad config file line %d in %s), cf-linenr, cf-name); } static int parse_unit_factor(const char *end, uintmax_t *val) @@ -579,9 +579,9 @@ static void die_bad_number(const char *name, const char *value) value = ; if (cf cf-name) - die(bad numeric config value '%s' for '%s' in %s: %s, + die(_(bad numeric config value '%s' for '%s' in %s: %s), value, name, cf-name, reason); - die(bad numeric config value '%s' for '%s': %s, value, name, reason); + die(_(bad numeric config value '%s' for '%s': %s), value, name, reason); } int git_config_int(const char
[PATCH v7 1/8] config.c: mark error and warnings strings for translation
From: Matthieu Moy matthieu@imag.fr
Signed-off-by: Matthieu Moy matthieu@imag.fr
---
config.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/config.c b/config.c
index a191328..34940fd 100644
--- a/config.c
+++ b/config.c
@@ -457,9 +457,9 @@ static int git_parse_source(config_fn_t fn, void *data)
break;
}
if (cf-die_on_error)
- die(bad config file line %d in %s, cf-linenr, cf-name);
+ die(_(bad config file line %d in %s), cf-linenr, cf-name);
else
- return error(bad config file line %d in %s, cf-linenr,
cf-name);
+ return error(_(bad config file line %d in %s), cf-linenr,
cf-name);
}
static int parse_unit_factor(const char *end, uintmax_t *val)
@@ -575,9 +575,9 @@ static void die_bad_number(const char *name, const char
*value)
value = ;
if (cf cf-name)
- die(bad numeric config value '%s' for '%s' in %s: %s,
+ die(_(bad numeric config value '%s' for '%s' in %s: %s),
value, name, cf-name, reason);
- die(bad numeric config value '%s' for '%s': %s, value, name, reason);
+ die(_(bad numeric config value '%s' for '%s': %s), value, name,
reason);
}
int git_config_int(const char *name, const char *value)
@@ -662,7 +662,7 @@ int git_config_pathname(const char **dest, const char *var,
const char *value)
return config_error_nonbool(var);
*dest = expand_user_path(value);
if (!*dest)
- die(Failed to expand user dir in: '%s', value);
+ die(_(failed to expand user dir in: '%s'), value);
return 0;
}
@@ -740,7 +740,7 @@ static int git_default_core_config(const char *var, const
char *value)
if (level == -1)
level = Z_DEFAULT_COMPRESSION;
else if (level 0 || level Z_BEST_COMPRESSION)
- die(bad zlib compression level %d, level);
+ die(_(bad zlib compression level %d), level);
zlib_compression_level = level;
zlib_compression_seen = 1;
return 0;
@@ -751,7 +751,7 @@ static int git_default_core_config(const char *var, const
char *value)
if (level == -1)
level = Z_DEFAULT_COMPRESSION;
else if (level 0 || level Z_BEST_COMPRESSION)
- die(bad zlib compression level %d, level);
+ die(_(bad zlib compression level %d), level);
core_compression_level = level;
core_compression_seen = 1;
if (!zlib_compression_seen)
@@ -873,7 +873,7 @@ static int git_default_core_config(const char *var, const
char *value)
else if (!strcmp(value, link))
object_creation_mode = OBJECT_CREATION_USES_HARDLINKS;
else
- die(Invalid mode for object creation: %s, value);
+ die(_(invalid mode for object creation: %s), value);
return 0;
}
@@ -1173,7 +1173,7 @@ int git_config_early(config_fn_t fn, void *data, const
char *repo_config)
switch (git_config_from_parameters(fn, data)) {
case -1: /* error */
- die(unable to parse command-line config);
+ die(_(unable to parse command-line config));
break;
case 0: /* found nothing */
break;
@@ -1514,7 +1514,7 @@ static int store_aux(const char *key, const char *value,
void *cb)
case KEY_SEEN:
if (matches(key, value)) {
if (store.seen == 1 store.multi_replace == 0) {
- warning(%s has multiple values, key);
+ warning(_(%s has multiple values), key);
}
ALLOC_GROW(store.offset, store.seen + 1,
--
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
[PATCH v7 6/8] rewrite git_config() to use the config-set API
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 tanay...@gmail.com
---
cache.h | 24 +++
config.c| 51 +
t/t4055-diff-context.sh | 2 +-
3 files changed, 68 insertions(+), 9 deletions(-)
diff --git a/cache.h b/cache.h
index 9ec2f4e..ff17889 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 ddbfe4d..cf9124f 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;
@@ -1235,7 +1229,7 @@ struct key_value_info {
int linenr;
};
-void git_config(config_fn_t fn, void *data)
+static void git_config_raw(config_fn_t fn, void *data)
{
if (git_config_with_options(fn, data, NULL, 1) 0)
/*
@@ -1252,6 +1246,33 @@ void git_config(config_fn_t fn, void *data)
die(_(unknown error occured while reading the configuration
files));
}
+static void 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;
+ git_die_config_linenr(entry-key, kv_info-filename,
kv_info-linenr);
+ }
+ }
+}
+
+static void git_config_check_init(void);
+
+void git_config(config_fn_t fn, void *data)
+{
+ git_config_check_init();
+ 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;
@@ -1278,6 +1299,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);
@@ -1293,6 +1315,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;
@@ -1316,6 +1344,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
[PATCH v7 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 tanay...@gmail.com
---
config.c | 21 +++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index bb4629e..e685b66 100644
--- a/config.c
+++ b/config.c
@@ -1230,6 +1230,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);
@@ -1260,6 +1265,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 +1280,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;
}
@@ -1299,7 +1316,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
[PATCH v7 4/8] change `git_config()` return value to void
Currently `git_config()` returns an integer signifying an error code.
During rewrites of the function most of the code was shifted to
`git_config_with_options()`. `git_config_with_options()` normally
returns positive values if its `config_source` parameter is set as NULL,
as most errors are fatal, and non-fatal potential errors are guarded
by if statements that are entered only when no error is possible.
Still a negative value can be returned in case of race condition between
`access_or_die()` `git_config_from_file()`. Also, all callers of
`git_config()` ignore the return value except for one case in branch.c.
Change `git_config()` return value to void and make it die if it receives
a negative value from `git_config_with_options()`.
Original-patch-by: Matthieu Moy matthieu@imag.fr
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
branch.c | 5 +
cache.h | 2 +-
config.c | 16 ++--
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/branch.c b/branch.c
index 46e8aa8..735767d 100644
--- a/branch.c
+++ b/branch.c
@@ -161,10 +161,7 @@ int read_branch_desc(struct strbuf *buf, const char
*branch_name)
strbuf_addf(name, branch.%s.description, branch_name);
cb.config_name = name.buf;
cb.value = NULL;
- if (git_config(read_branch_desc_cb, cb) 0) {
- strbuf_release(name);
- return -1;
- }
+ git_config(read_branch_desc_cb, cb);
if (cb.value)
strbuf_addstr(buf, cb.value);
strbuf_release(name);
diff --git a/cache.h b/cache.h
index 7292aef..c61919d 100644
--- a/cache.h
+++ b/cache.h
@@ -1294,7 +1294,7 @@ extern int git_config_from_buf(config_fn_t fn, const char
*name,
const char *buf, size_t len, void *data);
extern void git_config_push_parameter(const char *text);
extern int git_config_from_parameters(config_fn_t fn, void *data);
-extern int git_config(config_fn_t fn, void *);
+extern void git_config(config_fn_t fn, void *);
extern int git_config_with_options(config_fn_t fn, void *,
struct git_config_source *config_source,
int respect_includes);
diff --git a/config.c b/config.c
index e685b66..f923b1c 100644
--- a/config.c
+++ b/config.c
@@ -1235,9 +1235,21 @@ struct key_value_info {
int linenr;
};
-int git_config(config_fn_t fn, void *data)
+void git_config(config_fn_t fn, void *data)
{
- return git_config_with_options(fn, data, NULL, 1);
+ if (git_config_with_options(fn, data, NULL, 1) 0)
+ /*
+* git_config_with_options() normally returns only
+* positive values, as most errors are fatal, and
+* non-fatal potential errors are guarded by if
+* statements that are entered only when no error is
+* possible.
+*
+* If we ever encounter a non-fatal error, it means
+* something went really wrong and we should stop
+* immediately.
+*/
+ die(_(unknown error occured while reading the configuration
files));
}
static struct config_set_element *configset_find_element(struct config_set
*cs, const char *key)
--
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
[PATCH v7 8/8] add tests for `git_config_get_string_const()`
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 tanay...@gmail.com
---
t/t1308-config-set.sh | 10 ++
test-config.c | 10 ++
2 files changed, 20 insertions(+)
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index e2f9d0b..f012dd6 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -119,6 +119,16 @@ 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
+ check_config expect_code 1 get_string case.ba Value not found for
\case.ba\
+'
+
+test_expect_success 'check line error when NULL string is queried' '
+ test_expect_code 128 test-config get_string case.foo 2result
+ grep fatal: bad config variable '\''case.foo'\'' at 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
--
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 v7 7/8] add a test for semantic errors in config files
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 tanay...@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..e2f9d0b 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 2result + grep fatal: bad config variable '\''alias.br'\'' at file line 2 in .git/config result +' + test_done -- 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
[PATCH v5 3/7] change `git_config()` return value to void
Currently `git_config()` returns an integer signifying an error code.
During a previous rewrite of the function most of the code was shifted
to `git_config_with_options()`. `git_config_with_options()` normally
returns positive values if its `config_source` parameter is set as NULL,
as most errors are fatal, and non-fatal potential errors are guarded
by if statements that are entered only when no error is possible.
Still a negative value can be returned in case of race condition between
`access_or_die()` `git_config_from_file()`. Also, all callers of
`git_config()` ignore the return value except for one case in branch.c.
Change `git_config()` return value to void and make it die if it receives
a negative return value from `git_config_with_options()`.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
branch.c | 5 +
cache.h | 2 +-
config.c | 16 ++--
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/branch.c b/branch.c
index 46e8aa8..735767d 100644
--- a/branch.c
+++ b/branch.c
@@ -161,10 +161,7 @@ int read_branch_desc(struct strbuf *buf, const char
*branch_name)
strbuf_addf(name, branch.%s.description, branch_name);
cb.config_name = name.buf;
cb.value = NULL;
- if (git_config(read_branch_desc_cb, cb) 0) {
- strbuf_release(name);
- return -1;
- }
+ git_config(read_branch_desc_cb, cb);
if (cb.value)
strbuf_addstr(buf, cb.value);
strbuf_release(name);
diff --git a/cache.h b/cache.h
index 7292aef..c61919d 100644
--- a/cache.h
+++ b/cache.h
@@ -1294,7 +1294,7 @@ extern int git_config_from_buf(config_fn_t fn, const char
*name,
const char *buf, size_t len, void *data);
extern void git_config_push_parameter(const char *text);
extern int git_config_from_parameters(config_fn_t fn, void *data);
-extern int git_config(config_fn_t fn, void *);
+extern void git_config(config_fn_t fn, void *);
extern int git_config_with_options(config_fn_t fn, void *,
struct git_config_source *config_source,
int respect_includes);
diff --git a/config.c b/config.c
index 4a15383..c0faaca 100644
--- a/config.c
+++ b/config.c
@@ -1235,9 +1235,21 @@ struct key_value_info {
int linenr;
};
-int git_config(config_fn_t fn, void *data)
+void git_config(config_fn_t fn, void *data)
{
- return git_config_with_options(fn, data, NULL, 1);
+ if (git_config_with_options(fn, data, NULL, 1) 0)
+ /*
+* git_config_with_options() normally returns only
+* positive values, as most errors are fatal, and
+* non-fatal potential errors are guarded by if
+* statements that are entered only when no error is
+* possible.
+*
+* If we ever encounter a non-fatal error, it means
+* something went really wrong and we should stop
+* immediately.
+*/
+ die(Unknown error occured while reading the configuration
files);
}
static struct config_set_element *configset_find_element(struct config_set
*cs, const char *key)
--
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
[PATCH v5 1/7] config.c: fix accuracy of line number in errors
From: Matthieu Moy matthieu@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 tanay...@gmail.com
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
config.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/config.c b/config.c
index a191328..ed5fc8e 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
--
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 7/7] add tests for `git_config_get_string_const()`
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 tanay...@gmail.com
---
t/t1308-config-set.sh | 10 ++
test-config.c | 10 ++
2 files changed, 20 insertions(+)
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index e2f9d0b..f012dd6 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -119,6 +119,16 @@ 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
+ check_config expect_code 1 get_string case.ba Value not found for
\case.ba\
+'
+
+test_expect_success 'check line error when NULL string is queried' '
+ test_expect_code 128 test-config get_string case.foo 2result
+ grep fatal: bad config variable '\''case.foo'\'' at 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
--
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 v5 5/7] add a test for semantic errors in config files
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 tanay...@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..e2f9d0b 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 2result + grep fatal: bad config variable '\''alias.br'\'' at file line 2 in .git/config result +' + test_done -- 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
[PATCH v5 2/7] 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 tanay...@gmail.com
---
config.c | 21 +++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index ed5fc8e..4a15383 100644
--- a/config.c
+++ b/config.c
@@ -1230,6 +1230,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);
@@ -1260,6 +1265,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 +1280,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;
}
@@ -1299,7 +1316,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
[PATCH 6/7] config: add `git_die_config()` to the config-set API
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 tanay...@gmail.com
---
Documentation/technical/api-config.txt | 5 +
cache.h| 1 +
config.c | 27 +--
3 files changed, 31 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 4d3c6bd..243f618 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 4937515..03369b8 100644
--- a/config.c
+++ b/config.c
@@ -1515,8 +1515,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)
@@ -1557,10 +1561,29 @@ 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 '%s' from command-line config, key);
+ else
+ die(bad config variable '%s' at file line %d in %s,
+ key,
+ kv_info-linenr,
+ kv_info-filename);
+ }
+
/*
* Find all the stuff for git_config_set() below.
*/
--
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
[PATCH v5 4/7] rewrite git_config() to use the config-set API
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 tanay...@gmail.com
---
cache.h | 24 +
config.c| 57 ++---
t/t4055-diff-context.sh | 2 +-
3 files changed, 74 insertions(+), 9 deletions(-)
diff --git a/cache.h b/cache.h
index c61919d..4d3c6bd 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 c0faaca..4937515 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;
@@ -1235,7 +1229,7 @@ struct key_value_info {
int linenr;
};
-void git_config(config_fn_t fn, void *data)
+static void git_config_raw(config_fn_t fn, void *data)
{
if (git_config_with_options(fn, data, NULL, 1) 0)
/*
@@ -1252,6 +1246,39 @@ void git_config(config_fn_t fn, void *data)
die(Unknown error occured while reading the configuration
files);
}
+static void 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 '%s' from command-line
config, entry-key);
+ else
+ die(bad config variable '%s' at file line %d
in %s,
+ entry-key,
+ kv_info-linenr,
+ kv_info-filename);
+ }
+ }
+}
+
+static void git_config_check_init(void);
+
+void git_config(config_fn_t fn, void *data)
+{
+ git_config_check_init();
+ 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;
@@ -1278,6 +1305,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);
@@ -1293,6 +1321,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
[PATCH v5 0/7] Rewrite `git_config()` using config-set API
[PATCH v5]: New patch added (3/7). git_config() now returns void. [PATCH v4]: One style nit corrected, also added key to error messages. Diff with v3 at the bottom for easy review. [PATCH V3]:All the suggestions in [3] applied. Built on top of [1]. [PATCH V2]: All the suggestions in [2] incorporated. git_config() now follows correct parsing order. Reordered the patches. Removed xfuncname patch as it was unnecssary. This series builds on the top of topic[1] in the mailing list with name git config cache special querying API utilizing the cache. This series aims to do these three things, * Use the config-set API to rewrite git_config(). * Solve any legacy bugs in the previous system while at it. * To be feature complete compared to the previous git_config() implementation, which I think it is now. (added the line number and file name info just for completeness) Also, I haven't yet checked the exact improvements but still as a teaser, git status now only rereads the configuration files twice instead of four times. [1]: http://thread.gmane.org/gmane.comp.version-control.git/254286 [2]: http://thread.gmane.org/gmane.comp.version-control.git/254101 [3]: http://thread.gmane.org/gmane.comp.version-control.git/254211 Tanay Abhra (7): config.c: fix accuracy of line number in errors add line number and file name info to `config_set` change `git_config()` return value to void rewrite git_config() to use the config-set API add a test for semantic errors in config files config: add `git_die_config()` to the config-set API add tests for `git_config_get_string_const()` Documentation/technical/api-config.txt | 5 ++ branch.c | 5 +- cache.h| 27 ++- config.c | 131 + t/t1308-config-set.sh | 21 ++ t/t4055-diff-context.sh| 2 +- test-config.c | 10 +++ 7 files changed, 181 insertions(+), 20 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
Re: [PATCH v5 3/7] change `git_config()` return value to void
On 7/31/2014 4:52 PM, Matthieu Moy wrote:
Tanay Abhra tanay...@gmail.com writes:
Signed-off-by: Tanay Abhra tanay...@gmail.com
I think I deserve a bit of credit here ;-).
Yes, but to show credit would I have to write from you or
signed-off-by? :)
{
-return git_config_with_options(fn, data, NULL, 1);
+if (git_config_with_options(fn, data, NULL, 1) 0)
+/*
+ * git_config_with_options() normally returns only
+ * positive values, as most errors are fatal, and
+ * non-fatal potential errors are guarded by if
+ * statements that are entered only when no error is
+ * possible.
+ *
+ * If we ever encounter a non-fatal error, it means
+ * something went really wrong and we should stop
+ * immediately.
+ */
+die(Unknown error occured while reading the configuration
files);
}
My bad, but this should be die(_(...));, so that the message can be
translated. Not really serious since it's not really meant to be seen by
the user, though.
Noted. Though there are not many cases till I have read where error messages
are marked
for translation.
--
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] config.c: mark error and warnings strings for translation
On 7/31/2014 5:01 PM, Matthieu Moy wrote:
Signed-off-by: Matthieu Moy matthieu@imag.fr
---
Noticed while reviewing Tanay's patches, but this one is independant
from his series, both syntactically and semantically.
config.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/config.c b/config.c
index a191328..76eeb63 100644
--- a/config.c
+++ b/config.c
@@ -457,9 +457,9 @@ static int git_parse_source(config_fn_t fn, void *data)
break;
}
if (cf-die_on_error)
- die(bad config file line %d in %s, cf-linenr, cf-name);
+ die(_(bad config file line %d in %s), cf-linenr, cf-name);
else
- return error(bad config file line %d in %s, cf-linenr,
cf-name);
+ return error(_(bad config file line %d in %s), cf-linenr,
cf-name);
Can I package your patch with mine in which I am going to change the
error message to print the variable name also?
}
static int parse_unit_factor(const char *end, uintmax_t *val)
@@ -575,9 +575,9 @@ static void die_bad_number(const char *name, const char
*value)
value = ;
if (cf cf-name)
- die(bad numeric config value '%s' for '%s' in %s: %s,
+ die(_(bad numeric config value '%s' for '%s' in %s: %s),
value, name, cf-name, reason);
- die(bad numeric config value '%s' for '%s': %s, value, name, reason);
+ die(_(bad numeric config value '%s' for '%s': %s), value, name,
reason);
}
int git_config_int(const char *name, const char *value)
@@ -662,7 +662,7 @@ int git_config_pathname(const char **dest, const char
*var, const char *value)
return config_error_nonbool(var);
*dest = expand_user_path(value);
if (!*dest)
- die(Failed to expand user dir in: '%s', value);
+ die(_(Failed to expand user dir in: '%s'), value);
nit : error messages start with a small letter. same case below for Invalid.
;)
return 0;
}
@@ -740,7 +740,7 @@ static int git_default_core_config(const char *var, const
char *value)
if (level == -1)
level = Z_DEFAULT_COMPRESSION;
else if (level 0 || level Z_BEST_COMPRESSION)
- die(bad zlib compression level %d, level);
+ die(_(bad zlib compression level %d), level);
zlib_compression_level = level;
zlib_compression_seen = 1;
return 0;
@@ -751,7 +751,7 @@ static int git_default_core_config(const char *var, const
char *value)
if (level == -1)
level = Z_DEFAULT_COMPRESSION;
else if (level 0 || level Z_BEST_COMPRESSION)
- die(bad zlib compression level %d, level);
+ die(_(bad zlib compression level %d), level);
core_compression_level = level;
core_compression_seen = 1;
if (!zlib_compression_seen)
@@ -873,7 +873,7 @@ static int git_default_core_config(const char *var, const
char *value)
else if (!strcmp(value, link))
object_creation_mode = OBJECT_CREATION_USES_HARDLINKS;
else
- die(Invalid mode for object creation: %s, value);
+ die(_(Invalid mode for object creation: %s), value);
return 0;
}
@@ -1173,7 +1173,7 @@ int git_config_early(config_fn_t fn, void *data, const
char *repo_config)
switch (git_config_from_parameters(fn, data)) {
case -1: /* error */
- die(unable to parse command-line config);
+ die(_(unable to parse command-line config));
break;
case 0: /* found nothing */
break;
@@ -1514,7 +1514,7 @@ static int store_aux(const char *key, const char
*value, void *cb)
case KEY_SEEN:
if (matches(key, value)) {
if (store.seen == 1 store.multi_replace == 0) {
- warning(%s has multiple values, key);
+ warning(_(%s has multiple values), key);
}
ALLOC_GROW(store.offset, store.seen + 1,
--
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
