Re: [PATCH] config: Use parseopt.

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

On Sat, Feb 14, 2009 at 1:40 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sat, 14 Feb 2009, Felipe Contreras wrote:
>
>> Then why are you asking?
>
> Out of curiosity, I guess, as it would happen to answer my curiosity as
> well.
>
>> This is more a "I would like to increase the chances of my patches
>> being accepted so I'd do some chores to gain the trust of some
>> developers", and Johannes Schindelin was pushing me to do this.
>
> Heh, I'll gladly take the blame for that!
>
> Note that in contrast to Junio, I think "git config" is a chimera between
> plumbing and porcelain, and would benefit tremendously from a nice help.

I agree on that.

>> >> +static int type_int, type_bool, type_bool_or_int;
>> >
>> > You can have either (no type specified, int, bool, bool-or-int) at the
>> > end.  Using three independent variables does not feel right.
>> >
>> > Hint: OPTION_SET_INT.
>>
>> That definitely makes things easier, it would have been nice to see an
>> example of this; I didn't knew it was there.
>>
>> The only problem is that --bool and --int would be possible in the
>> same command and there would be no way to output an error, but I guess
>> that's not a big problem.
>
> I think that is okay.
>
>> >> +     else if (do_add) {
>> >> +             if (argc > 2)
>> >> +                     die("Too many arguments.");
>> >> +             if (argc != 2)
>> >> +                     die("Need name value.");
>> >> +             value = normalize_value(argv[0], argv[1]);
>> >> +             return git_config_set_multivar(argv[0], value, "^$", 0);
>> >
>> > This part did not lose argc error checking, but...
>> >
>> >> +     }
>> >> +     else if (do_replace_all) {
>> >> +             value = normalize_value(argv[0], argv[1]);
>> >> +             return git_config_set_multivar(argv[0], value, (argc == 3 ? argv[2] : NULL), 1);
>> >
>> > You do not check argc here (nor in many "else if" below) to make sure you
>> > have sufficient number of arguments.  "git config --unset" is now allowed
>> > to segfault, and "git config --unset a b c d e f" can silently ignore
>> > excess arguments for example?
>>
>> Yes the arguments check need to be revised.
>>
>> My hope was somebody would review this and suggest a clever and
>> generic way of doing this. Perhaps a util function check_min_args, or
>> maybe something in parseopt that receives the number of args?
>
> Maybe a helper, yes.  Something like:
>
>        static void check_argc(int argc, int min, int max) {
>                if (argc >= min && argc <= max)
>                        return;
>                fprintf(stderr, "Wrong number of arguments: %d\n", argc);
>                usage_with_options(config_usage, config_options);
>        }
>
> Of course, this assumes that config_usage and config_options are global...

Cool.

I've sent a new patch with this helper (a bit modified), and all the
changes Junio suggested.

I still have a few doubts:

1) --list when no config file is given uses all the config files,
wouldn't it make sense to have a --repo option?

2) --get-colorbool prints "true" or "false" only when there are two
arguments, is that correct or should stdout_is_tty be used instead?

3) should the documentation be updated for the --get-color* options to
use 'slot' instead of 'name'?

-- 
Felipe Contreras