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.
> >> +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...
> Also, I think the code would be easier to maintain with parseopt.
I agree.
Thanks,
Dscho
--
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
