Re: [PATCH] config: Use parseopt.

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 14, 2009, 11:40 UTC
Message-ID: <alpine.DEB.1.00.0902141230250.10279@pacific.mpi-cbg.de>
In-Reply-To: <94a0d4530902140237o7d26ff4j1c7350d926d12c1a@mail.gmail.com>

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.

Show 13 quoted lines

> >> +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.

Show 25 quoted lines

> >> +     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