Re: [PATCH] buitin_config: return postitive status in get_value
On Mon, Jul 30, 2012 at 3:43 AM, Junio C Hamano gits...@pobox.com wrote: +. You use '--global' option without $HOME being properly set (ret=128), +. Any other errors (ret=7). To be pedantic, ret=128 is a result of die() and not setting $HOME is just one of them. There's also ret=129 for usage(), which is not documented. So maybe we can make it clear that we document some exit codes, but not all of them. -This command will fail (with exit code ret) if: +This command will fail with non-zero status if: This command will fail with non-zero status. Some important exit codes are: -- Duy -- 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] buitin_config: return postitive status in get_value
Nguyen Thai Ngoc Duy pclo...@gmail.com writes: On Mon, Jul 30, 2012 at 3:43 AM, Junio C Hamano gits...@pobox.com wrote: +. You use '--global' option without $HOME being properly set (ret=128), +. Any other errors (ret=7). To be pedantic, ret=128 is a result of die() and not setting $HOME is just one of them. There's also ret=129 for usage(), which is not documented. So maybe we can make it clear that we document some exit codes, but not all of them. -This command will fail (with exit code ret) if: +This command will fail with non-zero status if: This command will fail with non-zero status. Some important exit codes are: Sounds good; will squash in. -- 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] buitin_config: return postitive status in get_value
Nguyen Thai Ngoc Duy pclo...@gmail.com writes:
On Sat, Jul 28, 2012 at 04:18:49PM +0300, Nikolay Vladimirov wrote:
But the behavior now seems kind of strange, or maybe I'm missing something:
# git config foobar; echo $?
error: key does not contain a section: foobar
255
# git config foobar.info; echo $?
1
git version 1.7.11.2
I would generally expect the both to behave the same way.
Then the following patch may be better because it leaves other cases
untouched (I'm not saying that we should or should not do it though)
I personally think that the documentation unnecessarily exposes the
useless value assignment of the exit codes (the use of different
exit codes was done merely to aid debugging the git-config command
itself) by describing the then-current set of conditions, and should
be reduced to say 0 for success, non-zero for any error.
But if we really want to follow that documented subset of possible
conditions, I agree that your version to return 1 in this case,
together with a change to initialize ret to 7 and document it as
all other errors (ret=7), would make more sense. Other conditions
that have been added since that partial enumeration of the exit code
was done are regexp errors, which I think will get -1 from the same
function.
-- 8 --
diff --git a/builtin/config.c b/builtin/config.c
index 8cd08da..d048ebf 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -199,8 +199,10 @@ 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)) {
+ ret = 1;
goto free_strings;
+ }
}
if (regex_) {
-- 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
Re: [PATCH] buitin_config: return postitive status in get_value
Junio C Hamano gits...@pobox.com writes:
I personally think that the documentation unnecessarily exposes the
useless value assignment of the exit codes (the use of different
exit codes was done merely to aid debugging the git-config command
itself) by describing the then-current set of conditions, and should
be reduced to say 0 for success, non-zero for any error.
But if we really want to follow that documented subset of possible
conditions, I agree that your version to return 1 in this case,
together with a change to initialize ret to 7 and document it as
all other errors (ret=7), would make more sense. Other conditions
that have been added since that partial enumeration of the exit code
was done are regexp errors, which I think will get -1 from the same
function.
IOW, something like this.
Documentation/git-config.txt | 18 ++
builtin/config.c | 8 ++--
2 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 2d6ef32..b071d65 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -54,19 +54,21 @@ configuration file by default, and options '--system',
'--global',
'--file filename' can be used to tell the command to write to
that location (you can say '--local' but that is the default).
-This command will fail (with exit code ret) if:
+This command will fail with non-zero status if:
+. The section name or key name is invalid (ret=1),
+. No section name or key name was provided (ret=2),
. The config file is invalid (ret=3),
-. can not write to the config file (ret=4),
-. no section or name was provided (ret=2),
-. the section or key is invalid (ret=1),
-. you try to unset an option which does not exist (ret=5),
-. you try to unset/set an option for which multiple lines match (ret=5),
-. you try to use an invalid regexp (ret=6), or
-. you use '--global' option without $HOME being properly set (ret=128).
+. The config file cannot be written (ret=4),
+. You try to unset an option which does not exist (ret=5),
+. You try to unset/set an option for which multiple lines match (ret=5),
+. You try to use an invalid regexp (ret=6),
+. You use '--global' option without $HOME being properly set (ret=128),
+. Any other errors (ret=7).
On success, the command returns the exit code 0.
+
OPTIONS
---
diff --git a/builtin/config.c b/builtin/config.c
index 8cd08da..2318308 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -160,7 +160,7 @@ static int show_config(const char *key_, const char
*value_, void *cb)
static int get_value(const char *key_, const char *regex_)
{
- int ret = -1;
+ int ret = 7;
char *global = NULL, *xdg = NULL, *repo_config = NULL;
const char *system_wide = NULL, *local;
struct config_include_data inc = CONFIG_INCLUDE_INIT;
@@ -196,11 +196,14 @@ static int get_value(const char *key_, const char *regex_)
if (regcomp(key_regexp, key, REG_EXTENDED)) {
fprintf(stderr, Invalid key pattern: %s\n, key_);
free(key);
+ ret = 6;
goto free_strings;
}
} else {
- if (git_config_parse_key(key_, key, NULL))
+ if (git_config_parse_key(key_, key, NULL)) {
+ ret = 1;
goto free_strings;
+ }
}
if (regex_) {
@@ -212,6 +215,7 @@ static int get_value(const char *key_, const char *regex_)
regexp = (regex_t*)xmalloc(sizeof(regex_t));
if (regcomp(regexp, regex_, REG_EXTENDED)) {
fprintf(stderr, Invalid pattern: %s\n, regex_);
+ ret = 6;
goto free_strings;
}
}
--
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] buitin_config: return postitive status in get_value
On Sat, Jul 28, 2012 at 11:38:10PM -0700, Junio C Hamano wrote: Then the following patch may be better because it leaves other cases untouched (I'm not saying that we should or should not do it though) I personally think that the documentation unnecessarily exposes the useless value assignment of the exit codes (the use of different exit codes was done merely to aid debugging the git-config command itself) by describing the then-current set of conditions, and should be reduced to say 0 for success, non-zero for any error. We use ret=5 in the test suite to say unset this variable, but it's OK if it wasn't set in the first place but still fail on error. The only other one I can imagine that would be useful is you tried to get a variable but it did not exist, and there was no other error. Which is probably what ret=1 is attempting to do, though it also encompasses syntactically bogus keys. -Peff -- 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] buitin_config: return postitive status in get_value
On Sun, Jul 29, 2012 at 01:43:21PM -0700, Junio C Hamano wrote: But if we really want to follow that documented subset of possible conditions, I agree that your version to return 1 in this case, together with a change to initialize ret to 7 and document it as all other errors (ret=7), would make more sense. Other conditions that have been added since that partial enumeration of the exit code was done are regexp errors, which I think will get -1 from the same function. IOW, something like this. Documentation/git-config.txt | 18 ++ builtin/config.c | 8 ++-- 2 files changed, 16 insertions(+), 10 deletions(-) This looks right to me. Even if we are missing an error case, it is certainly going in the right direction. -Peff -- 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] buitin_config: return postitive status in get_value
Returning -1 instead of 1 results in wrong exit status(255) since
the output of get_value is passed to exit().
'git config missing_section' should now return proper exit status = 1,
as specified by the git config documentation.
Signed-off-by: Nikolai Vladimirov niko...@vladimiroff.com
---
builtin/config.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/config.c b/builtin/config.c
index 8cd08da..c262cbb 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -160,7 +160,7 @@ static int show_config(const char *key_, const char
*value_, void *cb)
static int get_value(const char *key_, const char *regex_)
{
- int ret = -1;
+ int ret = 1;
char *global = NULL, *xdg = NULL, *repo_config = NULL;
const char *system_wide = NULL, *local;
struct config_include_data inc = CONFIG_INCLUDE_INIT;
--
1.7.11.2
--
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] buitin_config: return postitive status in get_value
On Sat, Jul 28, 2012 at 6:42 PM, Nikolai Vladimirov niko...@vladimiroff.com wrote: Returning -1 instead of 1 results in wrong exit status(255) since the output of get_value is passed to exit(). 'git config missing_section' should now return proper exit status = 1, as specified by the git config documentation. I'm curious. Why is -1 (or 255) wrong? It was introduced in the first version of get_value in 4ddba79 (git-config-set: add more options - 2005-11-20). Back then it returned -1 when there's regex compile error to distinguish with 0 and 1 (but git-config-set.txt in the same commit did not get update about exit code). Maybe we should update document instead of the code. -- Duy -- 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] buitin_config: return postitive status in get_value
On Sat, Jul 28, 2012 at 04:18:49PM +0300, Nikolay Vladimirov wrote:
But the behavior now seems kind of strange, or maybe I'm missing something:
# git config foobar; echo $?
error: key does not contain a section: foobar
255
# git config foobar.info; echo $?
1
git version 1.7.11.2
I would generally expect the both to behave the same way.
Then the following patch may be better because it leaves other cases
untouched (I'm not saying that we should or should not do it though)
-- 8 --
diff --git a/builtin/config.c b/builtin/config.c
index 8cd08da..d048ebf 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -199,8 +199,10 @@ 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)) {
+ ret = 1;
goto free_strings;
+ }
}
if (regex_) {
-- 8 --
--
Duy
--
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
