Re: [PATCH v2] config: Use parseopt.

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

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

> >> +     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");

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

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