Re: [PATCH v2] config: Use parseopt.

[Felipe Contreras <felipe.contreras@gmail.com>]

On Sun, Feb 15, 2009 at 12:32 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sat, 14 Feb 2009, Felipe Contreras wrote:
>
>> On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> 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