Re: [PATCH] config: Add new option to open an editor.

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

On Wed, Feb 4, 2009 at 12:53 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Wed, 4 Feb 2009, Felipe Contreras wrote:
>
>> The idea was originated by discussion about usability of manually
>> editing the config file in 'special needs' systems such as Windows. Now
>> the user can forget a bit about where the config files actually are.
>
> Cute...
>
>> diff --git a/builtin-config.c b/builtin-config.c
>> index f710162..b0a86b1 100644
>> --- a/builtin-config.c
>> +++ b/builtin-config.c
>> @@ -3,7 +3,7 @@
>>  #include "color.h"
>>
>>  static const char git_config_set_usage[] =
>> -"git config [ --global | --system | [ -f | --file ] config-file ] [ --bool | --int | --bool-or-int ] [ -z | --null ] [--get | --get-all | --get-regexp | --replace-all | --add | --unset | --unset-all] name [value [value_regex]] | --rename-section old_name new_name | --remove-section name | --list | --get-color var [default] | --get-colorbool name [stdout-is-tty]";
>> +"git config [ --global | --system | [ -f | --file ] config-file ] [ --bool | --int | --bool-or-int ] [ -z | --null ] [--get | --get-all | --get-regexp | --replace-all | --add | --unset | --unset-all] name [value [value_regex]] | --rename-section old_name new_name | --remove-section name | --list | --get-color var [default] | --get-colorbool name [stdout-is-tty] | --edit | -e ]";
>
> This line is getting way too long...  (Not nit-picking your current patch,
> but maybe you could provide another patch to break the line, while you're
> at builtin-config.c already.  Maybe even parseopt'ifying it... ;-)

Yeah, I'll send another patch to clean that string. Is there any
example of that parseopt thing?

>> @@ -362,6 +362,15 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>>                       return get_color(argc-2, argv+2);
>>               } else if (!strcmp(argv[1], "--get-colorbool")) {
>>                       return get_colorbool(argc-2, argv+2);
>> +             } else if (!strcmp(argv[1], "--edit") || !strcmp(argv[1], "-e")) {
>> +                     char *config_filename;
>> +                     if (config_exclusive_filename)
>> +                             config_filename = xstrdup(config_exclusive_filename);
>> +                     else
>> +                             config_filename = git_pathdup("config");
>> +                     launch_editor(config_filename, NULL, NULL);
>> +                     free(config_filename);
>> +                     return 0;
>
> Does launch_editor() not take a 'const char *' on purpose?  IOW you do not
> need to xstrdup() the filename.  You do not even need git_pathdup(), as
> launch_editor() does not use git_path() itself.

So, s/git_pathdup/git_path/ ?

> However, a test case would be nice...

What would the the test case check?

-- 
Felipe Contreras