Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string
Tanay Abhra tanay...@gmail.com writes:
On 6/30/2014 7:04 PM, Karsten Blees wrote:
Am 29.06.2014 13:01, schrieb Eric Sunshine:
On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra tanay...@gmail.com wrote:
On 6/25/2014 1:24 PM, Eric Sunshine wrote:
On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra tanay...@gmail.com wrote:
Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
notes-utils.c | 31 +++
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/notes-utils.c b/notes-utils.c
index a0b1d7b..fdc9912 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const
char *v)
return NULL;
}
-static int notes_rewrite_config(const char *k, const char *v, void *cb)
+static void notes_rewrite_config(struct notes_rewrite_cfg *c)
{
- struct notes_rewrite_cfg *c = cb;
- if (starts_with(k, notes.rewrite.) !strcmp(k+14, c-cmd)) {
- c-enabled = git_config_bool(k, v);
- return 0;
- } else if (!c-mode_from_env !strcmp(k, notes.rewritemode))
{
+ struct strbuf key = STRBUF_INIT;
+ const char *v;
+ strbuf_addf(key, notes.rewrite.%s, c-cmd);
+
+ if (!git_config_get_string(key.buf, v))
+ c-enabled = git_config_bool(key.buf, v);
+
+ if (!c-mode_from_env
!git_config_get_string(notes.rewritemode, v)) {
if (!v)
- return config_error_nonbool(k);
+ config_error_nonbool(notes.rewritemode);
There's a behavior change here. In the original code, the callback
function would return -1, which would cause the program to die() if
the config.c:die_on_error flag was set. The new code merely emits an
error.
Is this change serious enough? Can I ignore it?
IMO its better to Fail Fast than continue with some invalid config (which
may lead to more severe errors such as data corruption / data loss).
Noted but, what I am trying to do with the rewrite is emit an error and
not set the value if the value found is a NULL. The only change is that
program will not crash in this case and warn the user not set a NULL value for
a non boolean key.
This won't lead to severe errors as the value will not be set if found value
is a NULL.
The change probably makes sense, but as much as possible, keep
refactoring patches and patches introducing a semantic change separate.
It's much easier to review, and helps user digging history and finding
one of the commits.
--
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: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string
Am 29.06.2014 13:01, schrieb Eric Sunshine:
On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra tanay...@gmail.com wrote:
On 6/25/2014 1:24 PM, Eric Sunshine wrote:
On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra tanay...@gmail.com wrote:
Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
notes-utils.c | 31 +++
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/notes-utils.c b/notes-utils.c
index a0b1d7b..fdc9912 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const
char *v)
return NULL;
}
-static int notes_rewrite_config(const char *k, const char *v, void *cb)
+static void notes_rewrite_config(struct notes_rewrite_cfg *c)
{
- struct notes_rewrite_cfg *c = cb;
- if (starts_with(k, notes.rewrite.) !strcmp(k+14, c-cmd)) {
- c-enabled = git_config_bool(k, v);
- return 0;
- } else if (!c-mode_from_env !strcmp(k, notes.rewritemode)) {
+ struct strbuf key = STRBUF_INIT;
+ const char *v;
+ strbuf_addf(key, notes.rewrite.%s, c-cmd);
+
+ if (!git_config_get_string(key.buf, v))
+ c-enabled = git_config_bool(key.buf, v);
+
+ if (!c-mode_from_env
!git_config_get_string(notes.rewritemode, v)) {
if (!v)
- return config_error_nonbool(k);
+ config_error_nonbool(notes.rewritemode);
There's a behavior change here. In the original code, the callback
function would return -1, which would cause the program to die() if
the config.c:die_on_error flag was set. The new code merely emits an
error.
Is this change serious enough? Can I ignore it?
IMO its better to Fail Fast than continue with some invalid config (which
may lead to more severe errors such as data corruption / data loss).
I don't know. Even within this single function there is no consistency
about whether such problems should die() or just emit a message and
continue. For instance:
- if notes.rewritemode is bool, it die()s.
- if notes.rewritemode doesn't specify a recognized mode, it
error()s but continues
I think this would also die in git_parse_source():
...
if (get_value(fn, data, var) 0)
break;
}
if (cf-die_on_error)
die(bad config file line %d in %s, cf-linenr, cf-name);
...
(AFAICT, die_on_error is always true, except if invoked via 'git-config
--blob', which isn't used anywhere...)
This, however, raises another issue: switching to the config cache looses
file/line-precise error reporting for semantic errors. I don't know if
this feature is important enough to do something about it, though. A
message of the form Key 'xyz' is bad should usually enable a user to
locate the problematic file and line.
--
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: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string
On Mon, Jun 30, 2014 at 9:34 AM, Karsten Blees karsten.bl...@gmail.com wrote:
Am 29.06.2014 13:01, schrieb Eric Sunshine:
On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra tanay...@gmail.com wrote:
On 6/25/2014 1:24 PM, Eric Sunshine wrote:
On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra tanay...@gmail.com wrote:
Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
notes-utils.c | 31 +++
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/notes-utils.c b/notes-utils.c
index a0b1d7b..fdc9912 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const
char *v)
return NULL;
}
-static int notes_rewrite_config(const char *k, const char *v, void *cb)
+static void notes_rewrite_config(struct notes_rewrite_cfg *c)
{
- struct notes_rewrite_cfg *c = cb;
- if (starts_with(k, notes.rewrite.) !strcmp(k+14, c-cmd)) {
- c-enabled = git_config_bool(k, v);
- return 0;
- } else if (!c-mode_from_env !strcmp(k, notes.rewritemode)) {
+ struct strbuf key = STRBUF_INIT;
+ const char *v;
+ strbuf_addf(key, notes.rewrite.%s, c-cmd);
+
+ if (!git_config_get_string(key.buf, v))
+ c-enabled = git_config_bool(key.buf, v);
+
+ if (!c-mode_from_env
!git_config_get_string(notes.rewritemode, v)) {
if (!v)
- return config_error_nonbool(k);
+ config_error_nonbool(notes.rewritemode);
There's a behavior change here. In the original code, the callback
function would return -1, which would cause the program to die() if
the config.c:die_on_error flag was set. The new code merely emits an
error.
Is this change serious enough? Can I ignore it?
IMO its better to Fail Fast than continue with some invalid config (which
may lead to more severe errors such as data corruption / data loss).
I don't know. Even within this single function there is no consistency
about whether such problems should die() or just emit a message and
continue. For instance:
- if notes.rewritemode is bool, it die()s.
- if notes.rewritemode doesn't specify a recognized mode, it
error()s but continues
I think this would also die in git_parse_source():
...
if (get_value(fn, data, var) 0)
break;
}
if (cf-die_on_error)
die(bad config file line %d in %s, cf-linenr, cf-name);
...
One would expect so, but notes-utils.c:notes_rewrite_config() is
actually doing this:
if (!c-combine) {
error(_(Bad notes.rewriteMode value: '%s'), v);
return 1;
}
Rather than returning the -1 result of error(), which would make
git_parse_source() die(), it's explicitly returning 1, which
get_parse_source() ignores.
(AFAICT, die_on_error is always true, except if invoked via 'git-config
--blob', which isn't used anywhere...)
This, however, raises another issue: switching to the config cache looses
file/line-precise error reporting for semantic errors. I don't know if
this feature is important enough to do something about it, though. A
message of the form Key 'xyz' is bad should usually enable a user to
locate the problematic file and line.
--
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: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string
On 6/30/2014 7:04 PM, Karsten Blees wrote:
Am 29.06.2014 13:01, schrieb Eric Sunshine:
On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra tanay...@gmail.com wrote:
On 6/25/2014 1:24 PM, Eric Sunshine wrote:
On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra tanay...@gmail.com wrote:
Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
notes-utils.c | 31 +++
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/notes-utils.c b/notes-utils.c
index a0b1d7b..fdc9912 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const
char *v)
return NULL;
}
-static int notes_rewrite_config(const char *k, const char *v, void *cb)
+static void notes_rewrite_config(struct notes_rewrite_cfg *c)
{
- struct notes_rewrite_cfg *c = cb;
- if (starts_with(k, notes.rewrite.) !strcmp(k+14, c-cmd)) {
- c-enabled = git_config_bool(k, v);
- return 0;
- } else if (!c-mode_from_env !strcmp(k, notes.rewritemode)) {
+ struct strbuf key = STRBUF_INIT;
+ const char *v;
+ strbuf_addf(key, notes.rewrite.%s, c-cmd);
+
+ if (!git_config_get_string(key.buf, v))
+ c-enabled = git_config_bool(key.buf, v);
+
+ if (!c-mode_from_env
!git_config_get_string(notes.rewritemode, v)) {
if (!v)
- return config_error_nonbool(k);
+ config_error_nonbool(notes.rewritemode);
There's a behavior change here. In the original code, the callback
function would return -1, which would cause the program to die() if
the config.c:die_on_error flag was set. The new code merely emits an
error.
Is this change serious enough? Can I ignore it?
IMO its better to Fail Fast than continue with some invalid config (which
may lead to more severe errors such as data corruption / data loss).
Noted but, what I am trying to do with the rewrite is emit an error and
not set the value if the value found is a NULL. The only change is that
program will not crash in this case and warn the user not set a NULL value for
a non boolean key.
This won't lead to severe errors as the value will not be set if found value
is a NULL.
I don't know. Even within this single function there is no consistency
about whether such problems should die() or just emit a message and
continue. For instance:
- if notes.rewritemode is bool, it die()s.
- if notes.rewritemode doesn't specify a recognized mode, it
error()s but continues
I think this would also die in git_parse_source():
...
if (get_value(fn, data, var) 0)
break;
}
if (cf-die_on_error)
die(bad config file line %d in %s, cf-linenr, cf-name);
...
(AFAICT, die_on_error is always true, except if invoked via 'git-config
--blob', which isn't used anywhere...)
Noted.
This, however, raises another issue: switching to the config cache looses
file/line-precise error reporting for semantic errors. I don't know if
this feature is important enough to do something about it, though. A
message of the form Key 'xyz' is bad should usually enable a user to
locate the problematic file and line.
Hmn, but during the config cache construction we parse key-value pairs through
git_config() which still warns users about semantic errors. This happened
yesterday only when I was writing tests for the new API.
Thanks for the review.
Cheers,
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: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string
Am 30.06.2014 16:32, schrieb Eric Sunshine:
On Mon, Jun 30, 2014 at 9:34 AM, Karsten Blees karsten.bl...@gmail.com
wrote:
Am 29.06.2014 13:01, schrieb Eric Sunshine:
On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra tanay...@gmail.com wrote:
On 6/25/2014 1:24 PM, Eric Sunshine wrote:
On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra tanay...@gmail.com wrote:
Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
notes-utils.c | 31 +++
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/notes-utils.c b/notes-utils.c
index a0b1d7b..fdc9912 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const
char *v)
return NULL;
}
-static int notes_rewrite_config(const char *k, const char *v, void *cb)
+static void notes_rewrite_config(struct notes_rewrite_cfg *c)
{
- struct notes_rewrite_cfg *c = cb;
- if (starts_with(k, notes.rewrite.) !strcmp(k+14, c-cmd)) {
- c-enabled = git_config_bool(k, v);
- return 0;
- } else if (!c-mode_from_env !strcmp(k, notes.rewritemode))
{
+ struct strbuf key = STRBUF_INIT;
+ const char *v;
+ strbuf_addf(key, notes.rewrite.%s, c-cmd);
+
+ if (!git_config_get_string(key.buf, v))
+ c-enabled = git_config_bool(key.buf, v);
+
+ if (!c-mode_from_env
!git_config_get_string(notes.rewritemode, v)) {
if (!v)
- return config_error_nonbool(k);
+ config_error_nonbool(notes.rewritemode);
There's a behavior change here. In the original code, the callback
function would return -1, which would cause the program to die() if
the config.c:die_on_error flag was set. The new code merely emits an
error.
Is this change serious enough? Can I ignore it?
IMO its better to Fail Fast than continue with some invalid config (which
may lead to more severe errors such as data corruption / data loss).
I don't know. Even within this single function there is no consistency
about whether such problems should die() or just emit a message and
continue. For instance:
- if notes.rewritemode is bool, it die()s.
- if notes.rewritemode doesn't specify a recognized mode, it
error()s but continues
I think this would also die in git_parse_source():
...
if (get_value(fn, data, var) 0)
break;
}
if (cf-die_on_error)
die(bad config file line %d in %s, cf-linenr, cf-name);
...
One would expect so, but notes-utils.c:notes_rewrite_config() is
actually doing this:
if (!c-combine) {
error(_(Bad notes.rewriteMode value: '%s'), v);
return 1;
}
Rather than returning the -1 result of error(), which would make
git_parse_source() die(), it's explicitly returning 1, which
get_parse_source() ignores.
Ahh...I missed the ' 0', sorry.
--
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: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string
Am 30.06.2014 16:39, schrieb Tanay Abhra:
On 6/30/2014 7:04 PM, Karsten Blees wrote:
Am 29.06.2014 13:01, schrieb Eric Sunshine:
On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra tanay...@gmail.com wrote:
On 6/25/2014 1:24 PM, Eric Sunshine wrote:
On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra tanay...@gmail.com wrote:
Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
notes-utils.c | 31 +++
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/notes-utils.c b/notes-utils.c
index a0b1d7b..fdc9912 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const
char *v)
return NULL;
}
-static int notes_rewrite_config(const char *k, const char *v, void *cb)
+static void notes_rewrite_config(struct notes_rewrite_cfg *c)
{
- struct notes_rewrite_cfg *c = cb;
- if (starts_with(k, notes.rewrite.) !strcmp(k+14, c-cmd)) {
- c-enabled = git_config_bool(k, v);
- return 0;
- } else if (!c-mode_from_env !strcmp(k, notes.rewritemode))
{
+ struct strbuf key = STRBUF_INIT;
+ const char *v;
+ strbuf_addf(key, notes.rewrite.%s, c-cmd);
+
+ if (!git_config_get_string(key.buf, v))
+ c-enabled = git_config_bool(key.buf, v);
+
+ if (!c-mode_from_env
!git_config_get_string(notes.rewritemode, v)) {
if (!v)
- return config_error_nonbool(k);
+ config_error_nonbool(notes.rewritemode);
There's a behavior change here. In the original code, the callback
function would return -1, which would cause the program to die() if
the config.c:die_on_error flag was set. The new code merely emits an
error.
Is this change serious enough? Can I ignore it?
IMO its better to Fail Fast than continue with some invalid config (which
may lead to more severe errors such as data corruption / data loss).
Noted but, what I am trying to do with the rewrite is emit an error and
not set the value if the value found is a NULL.
If you don't set the value and continue, git will proceed with the variable's
default setting.
Which may not be too harmful in some cases, but if a user changes:
gc.pruneexpire=4.weeks.ago
to
gc.pruneexpire=4.monhts.ago
(note the typo), the next git-gc will warn the user and then happily throw
away data that the user intended to keep (default is 2.weeks.ago).
Thus I think git should die() if it encounters an invalid config setting.
This, however, raises another issue: switching to the config cache looses
file/line-precise error reporting for semantic errors. I don't know if
this feature is important enough to do something about it, though. A
message of the form Key 'xyz' is bad should usually enable a user to
locate the problematic file and line.
Hmn, but during the config cache construction we parse key-value pairs through
git_config() which still warns users about semantic errors.
If I'm not mistaken you only detect _syntax_ errors when loading the file (i.e.
whether the config file is structurally correct).
The semantic value and correctness of a key (e.g. whether its a boolean or an
int or a string that denotes a known merge algorithm) is only checked when it is
accessed via git_config_get_type. And at this point, file:line information
is already lost.
With the callback approach, both syntactic (structure) and semantic (meaning)
errors were checked at load time, resulting in
die(bad config file line %d in %s, cf-linenr, cf-name);
if the callback returned -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
Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string
On 6/30/2014 9:26 PM, Karsten Blees wrote:
Am 30.06.2014 16:39, schrieb Tanay Abhra:
On 6/30/2014 7:04 PM, Karsten Blees wrote:
Am 29.06.2014 13:01, schrieb Eric Sunshine:
On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra tanay...@gmail.com wrote:
On 6/25/2014 1:24 PM, Eric Sunshine wrote:
On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra tanay...@gmail.com wrote:
Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
notes-utils.c | 31 +++
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/notes-utils.c b/notes-utils.c
index a0b1d7b..fdc9912 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -68,22 +68,23 @@ static combine_notes_fn
parse_combine_notes_fn(const char *v)
return NULL;
}
-static int notes_rewrite_config(const char *k, const char *v, void *cb)
+static void notes_rewrite_config(struct notes_rewrite_cfg *c)
{
- struct notes_rewrite_cfg *c = cb;
- if (starts_with(k, notes.rewrite.) !strcmp(k+14, c-cmd)) {
- c-enabled = git_config_bool(k, v);
- return 0;
- } else if (!c-mode_from_env !strcmp(k,
notes.rewritemode)) {
+ struct strbuf key = STRBUF_INIT;
+ const char *v;
+ strbuf_addf(key, notes.rewrite.%s, c-cmd);
+
+ if (!git_config_get_string(key.buf, v))
+ c-enabled = git_config_bool(key.buf, v);
+
+ if (!c-mode_from_env
!git_config_get_string(notes.rewritemode, v)) {
if (!v)
- return config_error_nonbool(k);
+ config_error_nonbool(notes.rewritemode);
There's a behavior change here. In the original code, the callback
function would return -1, which would cause the program to die() if
the config.c:die_on_error flag was set. The new code merely emits an
error.
Is this change serious enough? Can I ignore it?
IMO its better to Fail Fast than continue with some invalid config (which
may lead to more severe errors such as data corruption / data loss).
Noted but, what I am trying to do with the rewrite is emit an error and
not set the value if the value found is a NULL.
If you don't set the value and continue, git will proceed with the variable's
default setting.
Which may not be too harmful in some cases, but if a user changes:
gc.pruneexpire=4.weeks.ago
to
gc.pruneexpire=4.monhts.ago
(note the typo), the next git-gc will warn the user and then happily throw
away data that the user intended to keep (default is 2.weeks.ago).
Thus I think git should die() if it encounters an invalid config setting.
Okay, point noted.
This, however, raises another issue: switching to the config cache looses
file/line-precise error reporting for semantic errors. I don't know if
this feature is important enough to do something about it, though. A
message of the form Key 'xyz' is bad should usually enable a user to
locate the problematic file and line.
Hmn, but during the config cache construction we parse key-value pairs
through
git_config() which still warns users about semantic errors.
If I'm not mistaken you only detect _syntax_ errors when loading the file
(i.e.
whether the config file is structurally correct).
The semantic value and correctness of a key (e.g. whether its a boolean or an
int or a string that denotes a known merge algorithm) is only checked when it
is
accessed via git_config_get_type. And at this point, file:line
information
is already lost.
With the callback approach, both syntactic (structure) and semantic (meaning)
errors were checked at load time, resulting in
die(bad config file line %d in %s, cf-linenr, cf-name);
if the callback returned -1.
Yup, you are right, we check only syntax error when loading the file for the
cache.
I could save the filename:linenr when the loading the file for future error
reporting. 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: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string
Karsten Blees karsten.bl...@gmail.com writes: Which may not be too harmful in some cases, but if a user changes: gc.pruneexpire=4.weeks.ago to gc.pruneexpire=4.monhts.ago (note the typo), the next git-gc will warn the user and then happily throw away data that the user intended to keep (default is 2.weeks.ago). Thus I think git should die() if it encounters an invalid config setting. Yes but not at the parsing time. Only when we are about to _use_ the value for pruneexpire as a time duration we should die of the error. Diagnosing an error early is a separate matter, but if the operation does not care about the typo we shouldn't die. -- 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: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string
On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra tanay...@gmail.com wrote:
On 6/25/2014 1:24 PM, Eric Sunshine wrote:
On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra tanay...@gmail.com wrote:
Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
notes-utils.c | 31 +++
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/notes-utils.c b/notes-utils.c
index a0b1d7b..fdc9912 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const
char *v)
return NULL;
}
-static int notes_rewrite_config(const char *k, const char *v, void *cb)
+static void notes_rewrite_config(struct notes_rewrite_cfg *c)
{
- struct notes_rewrite_cfg *c = cb;
- if (starts_with(k, notes.rewrite.) !strcmp(k+14, c-cmd)) {
- c-enabled = git_config_bool(k, v);
- return 0;
- } else if (!c-mode_from_env !strcmp(k, notes.rewritemode)) {
+ struct strbuf key = STRBUF_INIT;
+ const char *v;
+ strbuf_addf(key, notes.rewrite.%s, c-cmd);
+
+ if (!git_config_get_string(key.buf, v))
+ c-enabled = git_config_bool(key.buf, v);
+
+ if (!c-mode_from_env
!git_config_get_string(notes.rewritemode, v)) {
if (!v)
- return config_error_nonbool(k);
+ config_error_nonbool(notes.rewritemode);
There's a behavior change here. In the original code, the callback
function would return -1, which would cause the program to die() if
the config.c:die_on_error flag was set. The new code merely emits an
error.
Is this change serious enough? Can I ignore it?
I don't know. Even within this single function there is no consistency
about whether such problems should die() or just emit a message and
continue. For instance:
- if notes.rewritemode is bool, it die()s.
- if notes.rewritemode doesn't specify a recognized mode, it
error()s but continues
- if notes.rewriteref doesn't start with refs/notes/, it warning()s
and continues
It would be nice to hear an opinion from someone more invested in the
config system.
c-combine = parse_combine_notes_fn(v);
Worse: Though you correctly emit an error when 'v' is NULL, you then
(incorrectly) invoke parse_combine_notes_fn() with that NULL value,
which will result in a crash.
Noted.
- if (!c-combine) {
+ if (!c-combine)
error(_(Bad notes.rewriteMode value: '%s'), v);
- return 1;
- }
- return 0;
- } else if (!c-refs_from_env !strcmp(k, notes.rewriteref)) {
+ }
+ if (!c-refs_from_env !git_config_get_string(notes.rewriteref,
v)) {
/* note that a refs/ prefix is implied in the
* underlying for_each_glob_ref */
if (starts_with(v, refs/notes/))
@@ -91,10 +92,8 @@ static int notes_rewrite_config(const char *k, const
char *v, void *cb)
else
warning(_(Refusing to rewrite notes in %s
(outside of refs/notes/)), v);
- return 0;
}
-
- return 0;
+ strbuf_release(key);
It would be better to release the strbuf immediately after its final
use rather than waiting until the end of function. Not only does that
reduce cognitive load on people reading the code, but it also reduces
likelihood of 'key' being leaked if some future programmer inserts an
early 'return' into the function for some reason.
Noted. Thanks.
}
@@ -123,7 +122,7 @@ struct notes_rewrite_cfg
*init_copy_notes_for_rewrite(const char *cmd)
c-refs_from_env = 1;
string_list_add_refs_from_colon_sep(c-refs,
rewrite_refs_env);
}
- git_config(notes_rewrite_config, c);
+ notes_rewrite_config(c);
if (!c-enabled || !c-refs-nr) {
string_list_clear(c-refs, 0);
free(c-refs);
--
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: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string
On 6/25/2014 1:24 PM, Eric Sunshine wrote:
On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra tanay...@gmail.com wrote:
Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
notes-utils.c | 31 +++
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/notes-utils.c b/notes-utils.c
index a0b1d7b..fdc9912 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const
char *v)
return NULL;
}
-static int notes_rewrite_config(const char *k, const char *v, void *cb)
+static void notes_rewrite_config(struct notes_rewrite_cfg *c)
{
- struct notes_rewrite_cfg *c = cb;
- if (starts_with(k, notes.rewrite.) !strcmp(k+14, c-cmd)) {
- c-enabled = git_config_bool(k, v);
- return 0;
- } else if (!c-mode_from_env !strcmp(k, notes.rewritemode)) {
+ struct strbuf key = STRBUF_INIT;
+ const char *v;
+ strbuf_addf(key, notes.rewrite.%s, c-cmd);
+
+ if (!git_config_get_string(key.buf, v))
+ c-enabled = git_config_bool(key.buf, v);
+
+ if (!c-mode_from_env !git_config_get_string(notes.rewritemode,
v)) {
if (!v)
- return config_error_nonbool(k);
+ config_error_nonbool(notes.rewritemode);
There's a behavior change here. In the original code, the callback
function would return -1, which would cause the program to die() if
the config.c:die_on_error flag was set. The new code merely emits an
error.
Is this change serious enough? Can I ignore it?
c-combine = parse_combine_notes_fn(v);
Worse: Though you correctly emit an error when 'v' is NULL, you then
(incorrectly) invoke parse_combine_notes_fn() with that NULL value,
which will result in a crash.
Noted.
- if (!c-combine) {
+ if (!c-combine)
error(_(Bad notes.rewriteMode value: '%s'), v);
- return 1;
- }
- return 0;
- } else if (!c-refs_from_env !strcmp(k, notes.rewriteref)) {
+ }
+ if (!c-refs_from_env !git_config_get_string(notes.rewriteref,
v)) {
/* note that a refs/ prefix is implied in the
* underlying for_each_glob_ref */
if (starts_with(v, refs/notes/))
@@ -91,10 +92,8 @@ static int notes_rewrite_config(const char *k, const char
*v, void *cb)
else
warning(_(Refusing to rewrite notes in %s
(outside of refs/notes/)), v);
- return 0;
}
-
- return 0;
+ strbuf_release(key);
It would be better to release the strbuf immediately after its final
use rather than waiting until the end of function. Not only does that
reduce cognitive load on people reading the code, but it also reduces
likelihood of 'key' being leaked if some future programmer inserts an
early 'return' into the function for some reason.
Noted. Thanks.
}
@@ -123,7 +122,7 @@ struct notes_rewrite_cfg
*init_copy_notes_for_rewrite(const char *cmd)
c-refs_from_env = 1;
string_list_add_refs_from_colon_sep(c-refs,
rewrite_refs_env);
}
- git_config(notes_rewrite_config, c);
+ notes_rewrite_config(c);
if (!c-enabled || !c-refs-nr) {
string_list_clear(c-refs, 0);
free(c-refs);
--
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: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string
On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra tanay...@gmail.com wrote:
Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
notes-utils.c | 31 +++
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/notes-utils.c b/notes-utils.c
index a0b1d7b..fdc9912 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char
*v)
return NULL;
}
-static int notes_rewrite_config(const char *k, const char *v, void *cb)
+static void notes_rewrite_config(struct notes_rewrite_cfg *c)
{
- struct notes_rewrite_cfg *c = cb;
- if (starts_with(k, notes.rewrite.) !strcmp(k+14, c-cmd)) {
- c-enabled = git_config_bool(k, v);
- return 0;
- } else if (!c-mode_from_env !strcmp(k, notes.rewritemode)) {
+ struct strbuf key = STRBUF_INIT;
+ const char *v;
+ strbuf_addf(key, notes.rewrite.%s, c-cmd);
+
+ if (!git_config_get_string(key.buf, v))
+ c-enabled = git_config_bool(key.buf, v);
+
+ if (!c-mode_from_env !git_config_get_string(notes.rewritemode,
v)) {
if (!v)
- return config_error_nonbool(k);
+ config_error_nonbool(notes.rewritemode);
There's a behavior change here. In the original code, the callback
function would return -1, which would cause the program to die() if
the config.c:die_on_error flag was set. The new code merely emits an
error.
c-combine = parse_combine_notes_fn(v);
Worse: Though you correctly emit an error when 'v' is NULL, you then
(incorrectly) invoke parse_combine_notes_fn() with that NULL value,
which will result in a crash.
- if (!c-combine) {
+ if (!c-combine)
error(_(Bad notes.rewriteMode value: '%s'), v);
- return 1;
- }
- return 0;
- } else if (!c-refs_from_env !strcmp(k, notes.rewriteref)) {
+ }
+ if (!c-refs_from_env !git_config_get_string(notes.rewriteref,
v)) {
/* note that a refs/ prefix is implied in the
* underlying for_each_glob_ref */
if (starts_with(v, refs/notes/))
@@ -91,10 +92,8 @@ static int notes_rewrite_config(const char *k, const char
*v, void *cb)
else
warning(_(Refusing to rewrite notes in %s
(outside of refs/notes/)), v);
- return 0;
}
-
- return 0;
+ strbuf_release(key);
It would be better to release the strbuf immediately after its final
use rather than waiting until the end of function. Not only does that
reduce cognitive load on people reading the code, but it also reduces
likelihood of 'key' being leaked if some future programmer inserts an
early 'return' into the function for some reason.
}
@@ -123,7 +122,7 @@ struct notes_rewrite_cfg
*init_copy_notes_for_rewrite(const char *cmd)
c-refs_from_env = 1;
string_list_add_refs_from_colon_sep(c-refs,
rewrite_refs_env);
}
- git_config(notes_rewrite_config, c);
+ notes_rewrite_config(c);
if (!c-enabled || !c-refs-nr) {
string_list_clear(c-refs, 0);
free(c-refs);
--
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
[RFC/PATCH] notes-util.c: replace git_config with git_config_get_string
Use git_config_get_string instead of git_config to take advantage of
the config hash-table api which provides a cleaner control flow.
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
notes-utils.c | 31 +++
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/notes-utils.c b/notes-utils.c
index a0b1d7b..fdc9912 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const char
*v)
return NULL;
}
-static int notes_rewrite_config(const char *k, const char *v, void *cb)
+static void notes_rewrite_config(struct notes_rewrite_cfg *c)
{
- struct notes_rewrite_cfg *c = cb;
- if (starts_with(k, notes.rewrite.) !strcmp(k+14, c-cmd)) {
- c-enabled = git_config_bool(k, v);
- return 0;
- } else if (!c-mode_from_env !strcmp(k, notes.rewritemode)) {
+ struct strbuf key = STRBUF_INIT;
+ const char *v;
+ strbuf_addf(key, notes.rewrite.%s, c-cmd);
+
+ if (!git_config_get_string(key.buf, v))
+ c-enabled = git_config_bool(key.buf, v);
+
+ if (!c-mode_from_env !git_config_get_string(notes.rewritemode,
v)) {
if (!v)
- return config_error_nonbool(k);
+ config_error_nonbool(notes.rewritemode);
c-combine = parse_combine_notes_fn(v);
- if (!c-combine) {
+ if (!c-combine)
error(_(Bad notes.rewriteMode value: '%s'), v);
- return 1;
- }
- return 0;
- } else if (!c-refs_from_env !strcmp(k, notes.rewriteref)) {
+ }
+ if (!c-refs_from_env !git_config_get_string(notes.rewriteref,
v)) {
/* note that a refs/ prefix is implied in the
* underlying for_each_glob_ref */
if (starts_with(v, refs/notes/))
@@ -91,10 +92,8 @@ static int notes_rewrite_config(const char *k, const char
*v, void *cb)
else
warning(_(Refusing to rewrite notes in %s
(outside of refs/notes/)), v);
- return 0;
}
-
- return 0;
+ strbuf_release(key);
}
@@ -123,7 +122,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const
char *cmd)
c-refs_from_env = 1;
string_list_add_refs_from_colon_sep(c-refs, rewrite_refs_env);
}
- git_config(notes_rewrite_config, c);
+ notes_rewrite_config(c);
if (!c-enabled || !c-refs-nr) {
string_list_clear(c-refs, 0);
free(c-refs);
--
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
