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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html