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

From: Jeff King <peff@peff.net>
Date: May 17, 2011, 12:38 UTC
Message-ID: <20110517123840.GB13176@sigill.intra.peff.net>
In-Reply-To: <42ab57ae5a2cb7d6860e43e7c3061f9c38cf1b99.1305632091.git.git@drmicha.warpmail.net>

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

Show 10 quoted lines

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

Show 10 quoted lines

>  	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