Re: [PATCH v4 3/6] rewrite git_config() to use the config-set API
diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh index cd04543..741e080 100755 --- a/t/t4055-diff-context.sh +++ b/t/t4055-diff-context.sh @@ -79,7 +79,7 @@ test_expect_success 'non-integer config parsing' ' test_expect_success 'negative integer config parsing' ' git config diff.context -1 test_must_fail git diff 2output - test_i18ngrep bad config file output + test_i18ngrep bad config variable output ' test_expect_success '-U0 is valid, so is diff.context=0' ' This was a minor fixup with only a changed word, so I didn't flip it and fix after in the series as you said yesterday. Dunno if it's alright. -- 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 v4 3/6] rewrite git_config() to use the config-set API
Tanay Abhra tanay...@gmail.com writes: diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh index cd04543..741e080 100755 --- a/t/t4055-diff-context.sh +++ b/t/t4055-diff-context.sh @@ -79,7 +79,7 @@ test_expect_success 'non-integer config parsing' ' test_expect_success 'negative integer config parsing' ' git config diff.context -1 test_must_fail git diff 2output -test_i18ngrep bad config file output +test_i18ngrep bad config variable output ' test_expect_success '-U0 is valid, so is diff.context=0' ' This was a minor fixup with only a changed word, so I didn't flip it and fix after in the series as you said yesterday. Dunno if it's alright. You did right. It's not a new test, but an existing one that needs update together with your code update. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/6] rewrite git_config() to use the config-set API
Tanay Abhra tanay...@gmail.com writes:
+static int 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);
+ }
+ }
+ return 0;
+}
configset_iter unconditionnally returns 0 (or it dies). Since it is more
or less the equivalent of the old git_config(), I understand why we
never encounter the situation where git_config() would return -1 (syntax
error, weird permission issue = cannot happen when reading from
memory).
But then, do we really want this return value, and not just return void?
+static void git_config_check_init(void);
+
+int git_config(config_fn_t fn, void *data)
+{
+ git_config_check_init();
+ return configset_iter(the_config_set, fn, data);
+}
Here too, git_config now unconditionnally returns 0.
Most callers of git_config already ignore the return value. Actually,
there's only one exception in branch.c, but git still compiles with
this:
diff --git a/branch.c b/branch.c
index 660097b..92c3ff2 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 40b4ef3..23bfc67 100644
--- a/cache.h
+++ b/cache.h
@@ -1266,7 +1266,7 @@ extern int git_default_config(const char *, const char *,
void *);
extern int git_config_from_file(config_fn_t fn, const char *, void *);
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 0346681..3d033c9 100644
--- a/config.c
+++ b/config.c
@@ -1223,9 +1223,9 @@ 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);
+ git_config_with_options(fn, data, NULL, 1);
}
static struct config_set_element *configset_find_element(struct config_set
*cs, const char *key)
I do not see any difference between the git_config() call in branch.c
and the others.
So, I think it's time to make it official that git_config() does not
return an error code, and make it return void. I would do that in a
patch before the git_config() - git_config_raw() rewrite.
My preference would be to get the return value from
git_config_with_options and die() if it's negative, but I can also live
with a solution where the return value from git_config_with_options() is
ignored. It's the same discussion we already had about the call to
git_config() in git_config_check_init() actually, but I now think a
die() statement should be within git_config(), not after, so that every
callers benefit from it.
In any case, doing this in a separate patch means the commit message
(and possibly a comment next to the git_config() call) should explain
the situation clearly and justify the choice.
The current situation looks like someone tried to get good error
recovery, but the error code is lost in the way between
git_config_with_options() and the caller of git_config(), without a
clear justification of why an error code was once returned, nor a
justification of why it was later ignored.
So, in summary, my advice (but not the only option) would be: take my
patch above, add a die() statement and a comment, write a good commit
message and insert this before this patch.
static struct
Re: [PATCH v4 3/6] rewrite git_config() to use the config-set API
On 7/29/2014 6:10 PM, Matthieu Moy wrote:
Tanay Abhra tanay...@gmail.com writes:
configset_iter unconditionnally returns 0 (or it dies). Since it is more
or less the equivalent of the old git_config(), I understand why we
never encounter the situation where git_config() would return -1 (syntax
error, weird permission issue = cannot happen when reading from
memory).
But then, do we really want this return value, and not just return void?
Sounds sane to me.
+static void git_config_check_init(void);
+
+int git_config(config_fn_t fn, void *data)
+{
+git_config_check_init();
+return configset_iter(the_config_set, fn, data);
+}
Here too, git_config now unconditionnally returns 0.
Most callers of git_config already ignore the return value. Actually,
there's only one exception in branch.c, but git still compiles with
this:
branch.c is in my git_config() rewrite patch so it should not be a problem
in the future even if it was the case.
So, I think it's time to make it official that git_config() does not
return an error code, and make it return void. I would do that in a
patch before the git_config() - git_config_raw() rewrite.
My preference would be to get the return value from
git_config_with_options and die() if it's negative, but I can also live
Doesn't git_config_with_options() only return positive values, we checked it
pretty intensively last time.
with a solution where the return value from git_config_with_options() is
ignored. It's the same discussion we already had about the call to
git_config() in git_config_check_init() actually, but I now think a
die() statement should be within git_config(), not after, so that every
callers benefit from it.
The above patch works like that, doesn't it?
In any case, doing this in a separate patch means the commit message
(and possibly a comment next to the git_config() call) should explain
the situation clearly and justify the choice.
The choice being not to return a error code for git_config()?
I am pretty much confused by now.
The current situation looks like someone tried to get good error
recovery, but the error code is lost in the way between
git_config_with_options() and the caller of git_config(), without a
clear justification of why an error code was once returned, nor a
justification of why it was later ignored.
So, in summary, my advice (but not the only option) would be: take my
patch above, add a die() statement and a comment, write a good commit
Where can the die() statement be inserted? Again, I am confused.
Only thing which sounds reasonable to me is to rewrite existing git_config()
as void first. Other than that, it went over my head.
message and insert this before this patch.
static struct config_set_element *configset_find_element(struct config_set
*cs, const char *key)
{
struct config_set_element k;
@@ -1268,6 +1296,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);
@@ -1283,6 +1312,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;
I would spell this
l_item = cs-list.items[cs-list.nr];
l_item-e = e;
l_item-value_index = e-value_list.nr;
cs-list.nr++;
to avoid having to wonder why the - 1 is needed. But I'm OK with the
current code.
Yup, you are right. 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 v4 3/6] rewrite git_config() to use the config-set API
Tanay Abhra tanay...@gmail.com writes:
On 7/29/2014 6:10 PM, Matthieu Moy wrote:
So, I think it's time to make it official that git_config() does not
return an error code, and make it return void. I would do that in a
patch before the git_config() - git_config_raw() rewrite.
My preference would be to get the return value from
git_config_with_options and die() if it's negative, but I can also live
Doesn't git_config_with_options() only return positive values, we checked it
pretty intensively last time.
In normal cases, yes.
But the value comes from lines like
if (git_config_system() !access_or_die(git_etc_gitconfig(), R_OK,
0)) {
ret += git_config_from_file(fn, git_etc_gitconfig(),
data);
found += 1;
}
and git_config_from_file returns either 0 or -1.
So, either we consider that git_config_from_file always returns 0, and
the ret += part is dead code that should be removed as it only
confuses the reader, or we consider cases where git_config_from_file
returns -1, and we should do something with ret.
As we already discussed, return -1 is possible in case of race
condition between access_or_die() and git_config_from_file(). Very, very
unlikely in practice, but may happen in theory. That's why I suggest to
die() in these cases: the user will never see it in practice, but it
guarantees that we won't try to proceed if such case happen.
My point is not to improve the behavior, but to improve the code, by
documenting properly where the error code is lost in the path from
git_parse_source() to the caller of git_config().
We wouldn't have such discussion if the code was clear. I spent quite
some time trying to understand why an error code could be returned by
e.g. git_config_early(), and I'd like future readers to avoid wasting
such time.
Where can the die() statement be inserted? Again, I am confused.
I mean, changing the corresponding hunk to this:
--- a/config.c
+++ b/config.c
@@ -1223,9 +1223,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 user's
configuration);
}
static struct config_set_element *configset_find_element(struct config_set
*cs, const char *key)
with a solution where the return value from git_config_with_options() is
ignored. It's the same discussion we already had about the call to
git_config() in git_config_check_init() actually, but I now think a
die() statement should be within git_config(), not after, so that every
callers benefit from it.
The above patch works like that, doesn't it?
Except, it ignores the return code silently.
If you chose not to use a die() here, then ignoring the return value
must be justified, or readers of the code will just assume a programming
error, and will be tempted to repair the code by not ignoring the return
value. If so, there is no point in counting errors in git_config_early()
anymore, and a cleanup patch should be applied, something like:
--- a/config.c
+++ b/config.c
@@ -1147,30 +1147,30 @@ int git_config_system(void)
int git_config_early(config_fn_t fn, void *data, const char *repo_config)
{
- int ret = 0, found = 0;
+ int found = 0;
char *xdg_config = NULL;
char *user_config = NULL;
home_config_paths(user_config, xdg_config, config);
if (git_config_system() !access_or_die(git_etc_gitconfig(), R_OK,
0)) {
- ret += git_config_from_file(fn, git_etc_gitconfig(),
+ git_config_from_file(fn, git_etc_gitconfig(),
data);
found += 1;
}
if (xdg_config !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {
- ret += git_config_from_file(fn, xdg_config, data);
+ git_config_from_file(fn, xdg_config, data);
found += 1;
}
if (user_config !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))
{
- ret += git_config_from_file(fn, user_config, data);
+ git_config_from_file(fn, user_config, data);
found += 1;
}
if (repo_config !access_or_die(repo_config,
Re: [PATCH v4 3/6] rewrite git_config() to use the config-set API
On 7/29/2014 7:33 PM, Matthieu Moy wrote:
Tanay Abhra tanay...@gmail.com writes:
On 7/29/2014 6:10 PM, Matthieu Moy wrote:
So, I think it's time to make it official that git_config() does not
return an error code, and make it return void. I would do that in a
patch before the git_config() - git_config_raw() rewrite.
My preference would be to get the return value from
git_config_with_options and die() if it's negative, but I can also live
Doesn't git_config_with_options() only return positive values, we checked it
pretty intensively last time.
In normal cases, yes.
But the value comes from lines like
if (git_config_system() !access_or_die(git_etc_gitconfig(), R_OK,
0)) {
ret += git_config_from_file(fn, git_etc_gitconfig(),
data);
found += 1;
}
and git_config_from_file returns either 0 or -1.
So, either we consider that git_config_from_file always returns 0, and
the ret += part is dead code that should be removed as it only
confuses the reader, or we consider cases where git_config_from_file
returns -1, and we should do something with ret.
As we already discussed, return -1 is possible in case of race
condition between access_or_die() and git_config_from_file(). Very, very
unlikely in practice, but may happen in theory.
This time I checked the entire blame history of the functions. You are right,
the return values are remnants of an earlier time.
The git_config() return value had no significance whatsoever for the majority
of the project existence.
git_config_with_options() return value is only checked for git config --list
, that also is redundant since doing something like this,
diff --git a/config.c b/config.c
index 058505c..63f1d30 100644
--- a/config.c
+++ b/config.c
@@ -1169,7 +1169,7 @@ int git_config_early(config_fn_t fn, void *data, const
char *repo_config)
free(xdg_config);
free(user_config);
- return ret == 0 ? found : ret;
+ return found;
}
still passes all the tests.
Also I tried every trick in the book, to make it return a negative value, but I
failed.
So the only case left for a negative return value is what you said, race
condition. I checked
about it and you are right, quoting from the Linux programming interface,
The time gap between a call to access() and a subsequent operation on a file
means
that there is no guarantee that the information returned by access() will still
be true
at the time of the later operation (no matter how brief the interval). This
situation
could lead to security holes in some application designs.
time-ofcheck, time-of-use race condition
That's why I suggest to
die() in these cases: the user will never see it in practice, but it
guarantees that we won't try to proceed if such case happen.
My point is not to improve the behavior, but to improve the code, by
documenting properly where the error code is lost in the path from
git_parse_source() to the caller of git_config().
We wouldn't have such discussion if the code was clear. I spent quite
some time trying to understand why an error code could be returned by
e.g. git_config_early(), and I'd like future readers to avoid wasting
such time.
Where can the die() statement be inserted? Again, I am confused.
I mean, changing the corresponding hunk to this:
--- a/config.c
+++ b/config.c
@@ -1223,9 +1223,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 user's
configuration);
}
Sounds reasonable, will apply in the next series.
Somewhat validation for git_config_with_options() return value is given in
1f2baa78c6,
quoted below for review.
config: treat non-existent config files as empty
The git_config() function signals error by returning -1 in
two instances:
1. An actual error occurs in opening a config file (parse
errors cause an immediate die).
2. Of the three possible config files, none was found.
However, this second case is often not an error at all; it
simply means that the user has no configuration (they are
outside a repo, and they have no ~/.gitconfig file). This
can lead
