Re: [PATCH v2] config: Use parseopt.

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

On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> 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,

>> +     if (use_global_config) {
>> +             char *home = getenv("HOME");
>> +             if (home) {
>> +                     char *user_config = xstrdup(mkpath("%s/.gitconfig", home));
>> +                     config_exclusive_filename = user_config;
>
> In a subsequent patch, you might add a check only one of --global,
> --system or --file was given.

Yeap.

>> +     else if (given_config_file) {
>> +             if (!is_absolute_path(given_config_file) && file)
>> +                     file = prefix_filename(file, strlen(file),
>> +                                            given_config_file);
>> +             else
>> +                     file = given_config_file;
>> +             config_exclusive_filename = file;
>
> It took me a considerable amount of time to figure out that "file" is
> actually the "prefix"!  That cleanup would be nice to have before the
> parseopt patch, methinks, especially since the code is reindented, and
> thus hard to follow in the diff.

True.

I tried to minimize changes, so that code is exactly the same as
before (except vi didn't preserve the indent).

>> +     if (actions & ACTION_LIST) {
>> +             if (git_config(show_all_config, NULL) < 0 &&
>> +                 file && errno)
>
> Should this not be config_exclusive_filename?

Looks like that.

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

-- 
Felipe Contreras