On Sun, Feb 15, 2009 at 12:32 AM, Johannes Schindelin
<Johannes.Schindelin@xxxxxx> wrote:
> Hi,
>
> On Sat, 14 Feb 2009, Felipe Contreras wrote:
>
>> On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin
>> <Johannes.Schindelin@xxxxxx> wrote:
>>
>> > On Sat, 14 Feb 2009, Felipe Contreras wrote:
>> >
>> >> @@ -231,7 +264,7 @@ static int get_diff_color_found;
>> >> static int git_get_colorbool_config(const char *var, const char *value,
>> >> void *cb)
>> >> {
>> >> - if (!strcmp(var, get_color_slot)) {
>> >> + if (!strcmp(var, get_colorbool_slot)) {
>> >> get_colorbool_found =
>> >> git_config_colorbool(var, value, stdout_is_tty);
>> >> }
>> >
>> > Name changes like this make it harder to read the patch; can you separate
>> > that change out into its own patch?
>>
>> In that case I couldn't use OPT_STRING to store that value; I would
>> have to change --get-color* to use OPT_BOOLEAN or OPT_SET_INT and
>> store check the argc (since OPT_STRING isn't doing that anymore) and
>> save argv[1] to get_color_slot,
>
> What I meant was: have a patch renaming get_color_slot to
> get_colorbool_slot _before_ the (already large) parseoptification.
Done.
>> >> + die("unable to read config file %s: %s", file,
>> >> + strerror(errno));
>> >
>> > Do we really only want to die() in case we know the file name? AFAICT at
>> > this point we have no idea in which of the possibly three files the error
>> > occurred. And there need not be any errno set, for example when there was
>> > a parse error.
>>
>> Yes, there should be an error even if 'file' is not set. But if the
>> file is set what's wrong with that die command?
>
> I think we should die() in all cases, not just one, and we might want to
> skip the "file" parameter (and the "errno") parameter, as the file
> containing the error could be different from "file".
Done.
>> >> + else if (actions & ACTION_EDIT) {
>> >> + const char *config_filename;
>> >> + if (config_exclusive_filename)
>> >> + config_filename = config_exclusive_filename;
>> >> + else
>> >> + config_filename = git_path("config");
>> >
>> > Why not reuse config_exclusive_filename here?
>>
>> You mean:
>> if (!config_exclusive_filename)
>> config_exclusive_filename = git_path("config");
>
> Yes.
I'm not sure about this one. At least git_config should be moved
before that code, otherwise it will only try to read core.editor from
the exclusive_filename and that's not what we want.
If somebody adds some extra code that uses config_exclusive_filename
there would be issues. I would prefer to leave it like that since
that's how the code in config.c is handling config_exclusive_filename.
>> >> + else if (actions & ACTION_ADD) {
>> >> + check_argc(argc, 2, 2);
>> >
>> > BTW what about check_argc() in the previous two cases?
>>
>> You mean fail if -e or -l have extra arguments?
>
> Yes.
Done.
>> >> + return git_config_set_multivar(argv[0], value, "^$", 0);
>> >
>> > Now that I see this, there is another idea for a possible cleanup
>> > after the parseoptification: cmd_config() should not return -1, as
>> > that will be turned into the exit status. So maybe prefix the return
>> > value with "!!"?
>> >
>> > Or maybe even better: set a variable "ret" and at the end of
>> > cmd_config(), "return !!ret;"?
>>
>> Huh? So git commands don't return negative error values?
>
> AFAICT an exit status is supposed to be between 0 and 127.
Ok. Done.
--
Felipe Contreras
--
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
