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

[Michael J Gruber <git@drmicha.warpmail.net>]

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