git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH/RFC] config: Give error message when not changing a multivar
@ 2011-05-17 11:34 Michael J Gruber
  2011-05-17 12:38 ` Jeff King
  0 siblings, 1 reply; 11+ messages in thread
From: Michael J Gruber @ 2011-05-17 11:34 UTC (permalink / raw)
  To: git

When trying to set a multivar with "git config var value", "git config"
issues

warning: remote.repoor.push has multiple values

leaving the user under the impression that the operation succeeded,
unless one checks the return value.

Instead, make it

warning: remote.repoor.push has multiple values
error: Use a regexp, --add or --set-all to change remote.repoor.push.

to be clear and helpful.

Note: The "warning" is raised through other code paths also so that it
needs to remain a warning for these (which do not raise the error). Only
the caller can determine how to go on from that.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/config.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 3e3c528..c438ef4 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -436,9 +436,13 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			      NULL, NULL);
 	}
 	else if (actions == ACTION_SET) {
+		int ret;
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set(argv[0], value);
+		ret = git_config_set(argv[0], value);
+		if (ret == 5)
+			error("Use a regexp, --add or --set-all to change %s.", argv[0]);
+		return ret;
 	}
 	else if (actions == ACTION_SET_ALL) {
 		check_argc(argc, 2, 3);
-- 
1.7.5.1.514.gd181fb

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH/RFC] config: Give error message when not changing a multivar
  2011-05-17 11:34 [PATCH/RFC] config: Give error message when not changing a multivar Michael J Gruber
@ 2011-05-17 12:38 ` Jeff King
  2011-05-17 14:03   ` Michael J Gruber
  0 siblings, 1 reply; 11+ messages in thread
From: Jeff King @ 2011-05-17 12:38 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git

On Tue, May 17, 2011 at 01:34:58PM +0200, Michael J Gruber wrote:

> Instead, make it
> 
> warning: remote.repoor.push has multiple values
> error: Use a regexp, --add or --set-all to change remote.repoor.push.
> 
> to be clear and helpful.
> 
> Note: The "warning" is raised through other code paths also so that it
> needs to remain a warning for these (which do not raise the error). Only
> the caller can determine how to go on from that.

Makes sense, and I think trying to change the "warning" text is not
worth the effort.

>  	else if (actions == ACTION_SET) {
> +		int ret;
>  		check_argc(argc, 2, 2);
>  		value = normalize_value(argv[0], argv[1]);
> -		return git_config_set(argv[0], value);
> +		ret = git_config_set(argv[0], value);
> +		if (ret == 5)
> +			error("Use a regexp, --add or --set-all to change %s.", argv[0]);
> +		return ret;
>  	}

What the heck is this 5? In fact, what in the world is going on with the
return values from git_config_set_multivar? It looks like we can return
3, 4, 5, or 6 to return various errors, but they're not documented
anywhere. Or we can return 2, propagated from parse_key. Or we can
return -1 (negative!) if the lock doesn't work.

I think the last one is a straight-out error. For the other ones, we
should probably document them in git-config(1). And it would be nice to
at least have some symbolic constants in the code.

None of these problems is introduced by your patch, of course. But it
might be nice to at least do the symbolic constants while we're looking
at this so that your patch can use them.

-Peff

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH/RFC] config: Give error message when not changing a multivar
  2011-05-17 12:38 ` Jeff King
@ 2011-05-17 14:03   ` Michael J Gruber
  2011-05-17 14:07     ` Jeff King
  0 siblings, 1 reply; 11+ messages in thread
From: Michael J Gruber @ 2011-05-17 14:03 UTC (permalink / raw)
  To: Jeff King; +Cc: git

Jeff King venit, vidit, dixit 17.05.2011 14:38:
> On Tue, May 17, 2011 at 01:34:58PM +0200, Michael J Gruber wrote:
> 
>> Instead, make it
>>
>> warning: remote.repoor.push has multiple values
>> error: Use a regexp, --add or --set-all to change remote.repoor.push.
>>
>> to be clear and helpful.
>>
>> Note: The "warning" is raised through other code paths also so that it
>> needs to remain a warning for these (which do not raise the error). Only
>> the caller can determine how to go on from that.
> 
> Makes sense, and I think trying to change the "warning" text is not
> worth the effort.
> 
>>  	else if (actions == ACTION_SET) {
>> +		int ret;
>>  		check_argc(argc, 2, 2);
>>  		value = normalize_value(argv[0], argv[1]);
>> -		return git_config_set(argv[0], value);
>> +		ret = git_config_set(argv[0], value);
>> +		if (ret == 5)
>> +			error("Use a regexp, --add or --set-all to change %s.", argv[0]);
>> +		return ret;
>>  	}
> 
> What the heck is this 5? In fact, what in the world is going on with the
> return values from git_config_set_multivar? It looks like we can return
> 3, 4, 5, or 6 to return various errors, but they're not documented
> anywhere. Or we can return 2, propagated from parse_key. Or we can
> return -1 (negative!) if the lock doesn't work.
> 
> I think the last one is a straight-out error. For the other ones, we
> should probably document them in git-config(1). And it would be nice to
> at least have some symbolic constants in the code.
> 
> None of these problems is introduced by your patch, of course. But it
> might be nice to at least do the symbolic constants while we're looking
> at this so that your patch can use them.

OK, makes sense, I just wanted to request for comments early enough -
someone might think we should remove the warning from the present site
and make all callers deal with the return codes. But I don't think it's
worth it, and we have other places where we warn first, then error out.

Michael

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH/RFC] config: Give error message when not changing a multivar
  2011-05-17 14:03   ` Michael J Gruber
@ 2011-05-17 14:07     ` Jeff King
  2011-05-17 15:38       ` [PATCH 1/2] config: define and document exit codes Michael J Gruber
  0 siblings, 1 reply; 11+ messages in thread
From: Jeff King @ 2011-05-17 14:07 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git

On Tue, May 17, 2011 at 04:03:00PM +0200, Michael J Gruber wrote:

> >> warning: remote.repoor.push has multiple values
> >> error: Use a regexp, --add or --set-all to change remote.repoor.push.
> [...]
> OK, makes sense, I just wanted to request for comments early enough -
> someone might think we should remove the warning from the present site
> and make all callers deal with the return codes. But I don't think it's
> worth it, and we have other places where we warn first, then error out.

You could maybe make it a little more clear what is going on with:

  warning: remote.repoor.push has multiple values
  error: cannot overwrite multiple values with a single value
         Use a regexp, --add, or --set-all to change remote.repoor.push

Then the error stands on its own, but the warning is not a bad thing to
have, also.

-Peff

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] config: define and document exit codes
  2011-05-17 14:07     ` Jeff King
@ 2011-05-17 15:38       ` Michael J Gruber
  2011-05-17 15:38         ` [PATCH 2/2] config: Give error message when not changing a multivar Michael J Gruber
                           ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Michael J Gruber @ 2011-05-17 15:38 UTC (permalink / raw)
  To: git; +Cc: Jeff King

The return codes of git_config_set() and friends are magic numbers right
in the source. #define them in cache.h where the functions are declared,
and use the constants in the source.

Also, mention the resulting exit codes of "git config" in its man page
(and complete the list).

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-config.txt |   18 ++++++++++--------
 cache.h                      |   10 ++++++++++
 config.c                     |   20 ++++++++++----------
 3 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 8804de3..e7ecf5d 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -50,16 +50,18 @@ The default is to assume the config file of the current repository,
 .git/config unless defined otherwise with GIT_DIR and GIT_CONFIG
 (see <<FILES>>).
 
-This command will fail if:
-
-. The config file is invalid,
-. Can not write to the config file,
-. no section was provided,
-. the section or key is invalid,
-. you try to unset an option which does not exist,
-. you try to unset/set an option for which multiple lines match, or
-. you use '--global' option without $HOME being properly set.
-
+This command will fail (with exit code ret) if:
+
+. 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).
+
+On success, the command returns the exit code 0.
 
 OPTIONS
 -------
diff --git a/cache.h b/cache.h
index 2225c3f..57a98d2 100644
--- a/cache.h
+++ b/cache.h
@@ -1018,6 +1018,16 @@ extern const char *packed_object_info_detail(struct packed_git *, off_t, unsigne
 /* Dumb servers support */
 extern int update_server_info(int);
 
+/* git_config_parse_key() returns these negated: */
+#define CONFIG_INVALID_KEY 1
+#define CONFIG_NO_SECTION_OR_NAME 2
+/* git_config_set(), git_config_set_multivar() return the above or these: */
+#define CONFIG_NO_LOCK -1
+#define CONFIG_INVALID_FILE 3
+#define CONFIG_NO_WRITE 4
+#define CONFIG_NOTHING_SET 5
+#define CONFIG_INVALID_PATTERN 6
+
 typedef int (*config_fn_t)(const char *, const char *, void *);
 extern int git_default_config(const char *, const char *, void *);
 extern int git_config_from_file(config_fn_t fn, const char *, void *);
diff --git a/config.c b/config.c
index 671c8df..9d36848 100644
--- a/config.c
+++ b/config.c
@@ -1123,12 +1123,12 @@ 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);
-		return -2;
+		return -CONFIG_NO_SECTION_OR_NAME;
 	}
 
 	if (!last_dot[1]) {
 		error("key does not contain variable name: %s", key);
-		return -2;
+		return -CONFIG_NO_SECTION_OR_NAME;
 	}
 
 	baselen = last_dot - key;
@@ -1165,7 +1165,7 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 
 out_free_ret_1:
 	free(*store_key);
-	return -1;
+	return -CONFIG_INVALID_KEY;
 }
 
 /*
@@ -1221,7 +1221,7 @@ int git_config_set_multivar(const char *key, const char *value,
 	if (fd < 0) {
 		error("could not lock config file %s: %s", config_filename, strerror(errno));
 		free(store.key);
-		ret = -1;
+		ret = CONFIG_NO_LOCK;
 		goto out_free;
 	}
 
@@ -1235,12 +1235,12 @@ int git_config_set_multivar(const char *key, const char *value,
 		if ( ENOENT != errno ) {
 			error("opening %s: %s", config_filename,
 			      strerror(errno));
-			ret = 3; /* same as "invalid config file" */
+			ret = CONFIG_INVALID_FILE; /* same as "invalid config file" */
 			goto out_free;
 		}
 		/* if nothing to unset, error out */
 		if (value == NULL) {
-			ret = 5;
+			ret = CONFIG_NOTHING_SET;
 			goto out_free;
 		}
 
@@ -1268,7 +1268,7 @@ int git_config_set_multivar(const char *key, const char *value,
 					REG_EXTENDED)) {
 				error("invalid pattern: %s", value_regex);
 				free(store.value_regex);
-				ret = 6;
+				ret = CONFIG_INVALID_PATTERN;
 				goto out_free;
 			}
 		}
@@ -1290,7 +1290,7 @@ int git_config_set_multivar(const char *key, const char *value,
 				regfree(store.value_regex);
 				free(store.value_regex);
 			}
-			ret = 3;
+			ret = CONFIG_INVALID_FILE;
 			goto out_free;
 		}
 
@@ -1303,7 +1303,7 @@ int git_config_set_multivar(const char *key, const char *value,
 		/* if nothing to unset, or too many matches, error out */
 		if ((store.seen == 0 && value == NULL) ||
 				(store.seen > 1 && multi_replace == 0)) {
-			ret = 5;
+			ret = CONFIG_NOTHING_SET;
 			goto out_free;
 		}
 
@@ -1364,7 +1364,7 @@ int git_config_set_multivar(const char *key, const char *value,
 
 	if (commit_lock_file(lock) < 0) {
 		error("could not commit config file %s", config_filename);
-		ret = 4;
+		ret = CONFIG_NO_WRITE;
 		goto out_free;
 	}
 
-- 
1.7.5.1.514.gd181fb

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] config: Give error message when not changing a multivar
  2011-05-17 15:38       ` [PATCH 1/2] config: define and document exit codes Michael J Gruber
@ 2011-05-17 15:38         ` Michael J Gruber
  2011-05-17 15:46         ` [PATCH 1/2] config: define and document exit codes Thiago Farina
                           ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: Michael J Gruber @ 2011-05-17 15:38 UTC (permalink / raw)
  To: git; +Cc: Jeff King

When trying to set a multivar with "git config var value", "git config"
issues

warning: remote.repoor.push has multiple values

leaving the user under the impression that the operation succeeded,
unless one checks the return value.

Instead, make it

warning: remote.repoor.push has multiple values
error: cannot overwrite multiple values with a single value
       Use a regexp, --add or --set-all to change remote.repoor.push.

to be clear and helpful.

Note: The "warning" is raised through other code paths also so that it
needs to remain a warning for these (which do not raise the error). Only
the caller can determine how to go on from that.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/config.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 3e3c528..211e118 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -436,9 +436,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			      NULL, NULL);
 	}
 	else if (actions == ACTION_SET) {
+		int ret;
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1]);
-		return git_config_set(argv[0], value);
+		ret = git_config_set(argv[0], value);
+		if (ret == CONFIG_NOTHING_SET)
+			error("cannot overwrite multiple values with a single value\n"
+			"       Use a regexp, --add or --set-all to change %s.", argv[0]);
+		return ret;
 	}
 	else if (actions == ACTION_SET_ALL) {
 		check_argc(argc, 2, 3);
-- 
1.7.5.1.514.gd181fb

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] config: define and document exit codes
  2011-05-17 15:38       ` [PATCH 1/2] config: define and document exit codes Michael J Gruber
  2011-05-17 15:38         ` [PATCH 2/2] config: Give error message when not changing a multivar Michael J Gruber
@ 2011-05-17 15:46         ` Thiago Farina
  2011-05-17 15:49           ` Michael J Gruber
  2011-05-18  8:21         ` Jeff King
  2011-05-18 15:41         ` Junio C Hamano
  3 siblings, 1 reply; 11+ messages in thread
From: Thiago Farina @ 2011-05-17 15:46 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Jeff King

On Tue, May 17, 2011 at 12:38 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> The return codes of git_config_set() and friends are magic numbers right
> in the source. #define them in cache.h where the functions are declared,

Why defining them on cache.h? Just because the functions are declared
there? Is this a good reason? I think this pollute even more the
cache.h and these constants are not used outside of config.c. So I'd
move them back onto config.c. Maybe it's fine as is though.

> and use the constants in the source.
>
> Also, mention the resulting exit codes of "git config" in its man page
> (and complete the list).
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  Documentation/git-config.txt |   18 ++++++++++--------
>  cache.h                      |   10 ++++++++++
>  config.c                     |   20 ++++++++++----------
>  3 files changed, 30 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 8804de3..e7ecf5d 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -50,16 +50,18 @@ The default is to assume the config file of the current repository,
>  .git/config unless defined otherwise with GIT_DIR and GIT_CONFIG
>  (see <<FILES>>).
>
> -This command will fail if:
> -
> -. The config file is invalid,
> -. Can not write to the config file,
> -. no section was provided,
> -. the section or key is invalid,
> -. you try to unset an option which does not exist,
> -. you try to unset/set an option for which multiple lines match, or
> -. you use '--global' option without $HOME being properly set.
> -
> +This command will fail (with exit code ret) if:
> +
> +. 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).
> +
> +On success, the command returns the exit code 0.
>
>  OPTIONS
>  -------
> diff --git a/cache.h b/cache.h
> index 2225c3f..57a98d2 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1018,6 +1018,16 @@ extern const char *packed_object_info_detail(struct packed_git *, off_t, unsigne
>  /* Dumb servers support */
>  extern int update_server_info(int);
>
> +/* git_config_parse_key() returns these negated: */
> +#define CONFIG_INVALID_KEY 1
> +#define CONFIG_NO_SECTION_OR_NAME 2
> +/* git_config_set(), git_config_set_multivar() return the above or these: */
> +#define CONFIG_NO_LOCK -1
> +#define CONFIG_INVALID_FILE 3
> +#define CONFIG_NO_WRITE 4
> +#define CONFIG_NOTHING_SET 5
> +#define CONFIG_INVALID_PATTERN 6
> +
>  typedef int (*config_fn_t)(const char *, const char *, void *);
>  extern int git_default_config(const char *, const char *, void *);
>  extern int git_config_from_file(config_fn_t fn, const char *, void *);
> diff --git a/config.c b/config.c
> index 671c8df..9d36848 100644
> --- a/config.c
> +++ b/config.c
> @@ -1123,12 +1123,12 @@ 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);
> -               return -2;
> +               return -CONFIG_NO_SECTION_OR_NAME;
>        }
>
>        if (!last_dot[1]) {
>                error("key does not contain variable name: %s", key);
> -               return -2;
> +               return -CONFIG_NO_SECTION_OR_NAME;
>        }
>
>        baselen = last_dot - key;
> @@ -1165,7 +1165,7 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
>
>  out_free_ret_1:
>        free(*store_key);
> -       return -1;
> +       return -CONFIG_INVALID_KEY;
>  }
>
>  /*
> @@ -1221,7 +1221,7 @@ int git_config_set_multivar(const char *key, const char *value,
>        if (fd < 0) {
>                error("could not lock config file %s: %s", config_filename, strerror(errno));
>                free(store.key);
> -               ret = -1;
> +               ret = CONFIG_NO_LOCK;
>                goto out_free;
>        }
>
> @@ -1235,12 +1235,12 @@ int git_config_set_multivar(const char *key, const char *value,
>                if ( ENOENT != errno ) {
>                        error("opening %s: %s", config_filename,
>                              strerror(errno));
> -                       ret = 3; /* same as "invalid config file" */
> +                       ret = CONFIG_INVALID_FILE; /* same as "invalid config file" */
>                        goto out_free;
>                }
>                /* if nothing to unset, error out */
>                if (value == NULL) {
> -                       ret = 5;
> +                       ret = CONFIG_NOTHING_SET;
>                        goto out_free;
>                }
>
> @@ -1268,7 +1268,7 @@ int git_config_set_multivar(const char *key, const char *value,
>                                        REG_EXTENDED)) {
>                                error("invalid pattern: %s", value_regex);
>                                free(store.value_regex);
> -                               ret = 6;
> +                               ret = CONFIG_INVALID_PATTERN;
>                                goto out_free;
>                        }
>                }
> @@ -1290,7 +1290,7 @@ int git_config_set_multivar(const char *key, const char *value,
>                                regfree(store.value_regex);
>                                free(store.value_regex);
>                        }
> -                       ret = 3;
> +                       ret = CONFIG_INVALID_FILE;
>                        goto out_free;
>                }
>
> @@ -1303,7 +1303,7 @@ int git_config_set_multivar(const char *key, const char *value,
>                /* if nothing to unset, or too many matches, error out */
>                if ((store.seen == 0 && value == NULL) ||
>                                (store.seen > 1 && multi_replace == 0)) {
> -                       ret = 5;
> +                       ret = CONFIG_NOTHING_SET;
>                        goto out_free;
>                }
>
> @@ -1364,7 +1364,7 @@ int git_config_set_multivar(const char *key, const char *value,
>
>        if (commit_lock_file(lock) < 0) {
>                error("could not commit config file %s", config_filename);
> -               ret = 4;
> +               ret = CONFIG_NO_WRITE;
>                goto out_free;
>        }
>
> --
> 1.7.5.1.514.gd181fb
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] config: define and document exit codes
  2011-05-17 15:46         ` [PATCH 1/2] config: define and document exit codes Thiago Farina
@ 2011-05-17 15:49           ` Michael J Gruber
  0 siblings, 0 replies; 11+ messages in thread
From: Michael J Gruber @ 2011-05-17 15:49 UTC (permalink / raw)
  To: Thiago Farina; +Cc: git, Jeff King

Thiago Farina venit, vidit, dixit 17.05.2011 17:46:
> On Tue, May 17, 2011 at 12:38 PM, Michael J Gruber
> <git@drmicha.warpmail.net> wrote:
>> The return codes of git_config_set() and friends are magic numbers right
>> in the source. #define them in cache.h where the functions are declared,
> 
> Why defining them on cache.h? Just because the functions are declared
> there? Is this a good reason? I think this pollute even more the
> cache.h and these constants are not used outside of config.c. So I'd
> move them back onto config.c. Maybe it's fine as is though.

Because, potentially, they are used by callers of these external
functions. It's the interface declaration. And in fact, 2/2 creates such
a use.

Michael

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] config: define and document exit codes
  2011-05-17 15:38       ` [PATCH 1/2] config: define and document exit codes Michael J Gruber
  2011-05-17 15:38         ` [PATCH 2/2] config: Give error message when not changing a multivar Michael J Gruber
  2011-05-17 15:46         ` [PATCH 1/2] config: define and document exit codes Thiago Farina
@ 2011-05-18  8:21         ` Jeff King
  2011-05-18 15:41         ` Junio C Hamano
  3 siblings, 0 replies; 11+ messages in thread
From: Jeff King @ 2011-05-18  8:21 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git

On Tue, May 17, 2011 at 05:38:52PM +0200, Michael J Gruber wrote:

> The return codes of git_config_set() and friends are magic numbers right
> in the source. #define them in cache.h where the functions are declared,
> and use the constants in the source.
> 
> Also, mention the resulting exit codes of "git config" in its man page
> (and complete the list).

This version (and the accompanying 2/2) both look sane to me. Thanks for
taking time to clean up.

-Peff

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] config: define and document exit codes
  2011-05-17 15:38       ` [PATCH 1/2] config: define and document exit codes Michael J Gruber
                           ` (2 preceding siblings ...)
  2011-05-18  8:21         ` Jeff King
@ 2011-05-18 15:41         ` Junio C Hamano
  2011-05-18 18:49           ` Michael J Gruber
  3 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2011-05-18 15:41 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Jeff King

Michael J Gruber <git@drmicha.warpmail.net> writes:

> +This command will fail (with exit code ret) if:
> +
> +. 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).

I wonder if you want to sort this in the order of return codes.

> +/* git_config_parse_key() returns these negated: */
> +#define CONFIG_INVALID_KEY 1
> +#define CONFIG_NO_SECTION_OR_NAME 2
> +/* git_config_set(), git_config_set_multivar() return the above or these: */
> +#define CONFIG_NO_LOCK -1
> +#define CONFIG_INVALID_FILE 3
> +#define CONFIG_NO_WRITE 4
> +#define CONFIG_NOTHING_SET 5
> +#define CONFIG_INVALID_PATTERN 6

Symbols "CONFIG_FOO" looks too much like they are about the feature set
chosen when compiling git, at least to me. But I do not think of a better
naming scheme ("CONFIG_ERR_FOO" may be a slight improvement but not good
enough improvement for the price we pay by having such long symbol names).

By the way, Are you still interested in "diff --stat-count" topic, or have
you abandoned it?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] config: define and document exit codes
  2011-05-18 15:41         ` Junio C Hamano
@ 2011-05-18 18:49           ` Michael J Gruber
  0 siblings, 0 replies; 11+ messages in thread
From: Michael J Gruber @ 2011-05-18 18:49 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King

Junio C Hamano venit, vidit, dixit 18.05.2011 17:41:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> +This command will fail (with exit code ret) if:
>> +
>> +. 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).
> 
> I wonder if you want to sort this in the order of return codes.

Well, if I wanted to... ;)

I don't mind resorting.

>> +/* git_config_parse_key() returns these negated: */
>> +#define CONFIG_INVALID_KEY 1
>> +#define CONFIG_NO_SECTION_OR_NAME 2
>> +/* git_config_set(), git_config_set_multivar() return the above or these: */
>> +#define CONFIG_NO_LOCK -1
>> +#define CONFIG_INVALID_FILE 3
>> +#define CONFIG_NO_WRITE 4
>> +#define CONFIG_NOTHING_SET 5
>> +#define CONFIG_INVALID_PATTERN 6
> 
> Symbols "CONFIG_FOO" looks too much like they are about the feature set
> chosen when compiling git, at least to me. But I do not think of a better
> naming scheme ("CONFIG_ERR_FOO" may be a slight improvement but not good
> enough improvement for the price we pay by having such long symbol names).

Well, we could do CONFIG_ENOLOCK and such. I didn't find a good sample
to follow. Thinking more about it, using CONFIG_ENOPARSE would be nice,
though I can't decide between 1, 2 or 3 for that...

> By the way, Are you still interested in "diff --stat-count" topic, or have
> you abandoned it?

Yes. I've reworked it a bit but there are three goals:

- count right
- determine the correct maximal file name length
- don't sacrifice efficiency

The middle one is problematic even before my patch (that first loop does
not drop all items that the latter one drops).

Maybe the third one is no real problem so that I could simply go through
the list twice.

Oh, and I've checked that fmt-merge-msg does this:

- output at most <count> items
- if there are more, output "..."

So, there's progress, though not visible yet.

Michael

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2011-05-18 18:49 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-17 11:34 [PATCH/RFC] config: Give error message when not changing a multivar Michael J Gruber
2011-05-17 12:38 ` Jeff King
2011-05-17 14:03   ` Michael J Gruber
2011-05-17 14:07     ` Jeff King
2011-05-17 15:38       ` [PATCH 1/2] config: define and document exit codes Michael J Gruber
2011-05-17 15:38         ` [PATCH 2/2] config: Give error message when not changing a multivar Michael J Gruber
2011-05-17 15:46         ` [PATCH 1/2] config: define and document exit codes Thiago Farina
2011-05-17 15:49           ` Michael J Gruber
2011-05-18  8:21         ` Jeff King
2011-05-18 15:41         ` Junio C Hamano
2011-05-18 18:49           ` Michael J Gruber

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).