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