[PATCH/RFC] config: Give error message when not changing a multivar

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 <redacted>
---
 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

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

Jeff King venit, vidit, dixit 17.05.2011 14:38:

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

quoted

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.

quoted

 	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

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

quoted

quoted

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

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 <redacted>
---
 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

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 <redacted>
---
 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

On Tue, May 17, 2011 at 12:38 PM, Michael J Gruber
[off-list ref] 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.

Thiago Farina venit, vidit, dixit 17.05.2011 17:46:

On Tue, May 17, 2011 at 12:38 PM, Michael J Gruber
[off-list ref] wrote:

quoted

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

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

Michael J Gruber [off-list ref] 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?

Junio C Hamano venit, vidit, dixit 18.05.2011 17:41:

Michael J Gruber [off-list ref] writes:

quoted

+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.

quoted

+/* 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