On Sat, Feb 14, 2009 at 1:40 PM, Johannes Schindelin
<Johannes.Schindelin@xxxxxx> 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
--
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
