threads / patch / 17539

patchconfig: Add new option to open an editor.

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

## tl;dr

25 messages between Feb 3, 2009 and Feb 7, 2009. Diffs are folded; open one to read it.

replies: 24people: 4as markdown or json

Felipe Contreras· Feb 3, 2009, 22:40 UTC · lore

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.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-config.txt |    6 ++++++
 builtin-config.c             |   11 ++++++++++-
 2 files changed, 16 insertions(+), 1 deletions(-)
Show changes to 2 files +16 −1

Documentation/git-config.txt, builtin-config.c

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 19a8917..7d14007 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -22,6 +22,7 @@ SYNOPSIS
 'git config' [<file-option>] [-z|--null] -l | --list
 'git config' [<file-option>] --get-color name [default]
 'git config' [<file-option>] --get-colorbool name [stdout-is-tty]
+'git config' [<file-option>] -e | --edit
 
 DESCRIPTION
 -----------
@@ -157,6 +158,11 @@ See also <<FILES>>.
 	output.  The optional `default` parameter is used instead, if
 	there is no color configured for `name`.
 
+-e::
+--edit::
+	Opens an editor to modify the specified config file; either
+	'--system', '--global', or repository (default).
+
 [[FILES]]
 FILES
 -----
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 ]";
 
 static char *key;
 static regex_t *key_regexp;
@@ -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;
 		} else
 			break;
 		argc--;

-- 
1.6.1.2
Johannes Schindelin· Feb 3, 2009, 22:53 UTC · re: Felipe Contreras · lore

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

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...
Show 10 quoted lines
> 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... ;-)

Show 13 quoted lines
> @@ -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.

However, a test case would be nice...

Ciao, Dscho

Felipe Contreras· Feb 3, 2009, 22:56 UTC · re: Johannes Schindelin · lore

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

On Wed, Feb 4, 2009 at 12:53 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 24 quoted lines
> 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?

Show 17 quoted lines
>> @@ -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
Johannes Schindelin· Feb 3, 2009, 23:05 UTC · re: Felipe Contreras · lore

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

Hi,
On Wed, 4 Feb 2009, Felipe Contreras wrote:
Show 15 quoted lines
> On Wed, Feb 4, 2009 at 12:53 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > On Wed, 4 Feb 2009, Felipe Contreras wrote:
> >
> >>  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?
Yep... test-parse-options.c ;-)
Show 19 quoted lines
> >> @@ -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/ ?
And s/xstrdup(\(.*\))/\1/.
> > However, a test case would be nice...
> 
> What would the the test case check?

That 'GIT_CONFIG=bla GIT_EDITOR=echo git config -e' and 'GIT_DIR=blub GIT_EDITOR=echo git config -e' do the right thing. Maybe even --global, but that would also be a good test that "git config --global -e" does not fail when there is no original file.

Ciao, Dscho

Felipe Contreras· Feb 3, 2009, 23:25 UTC · re: Johannes Schindelin · lore

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

On Wed, Feb 4, 2009 at 1:05 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 21 quoted lines
> Hi,
>
> On Wed, 4 Feb 2009, Felipe Contreras wrote:
>
>> On Wed, Feb 4, 2009 at 12:53 AM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>
>> > On Wed, 4 Feb 2009, Felipe Contreras wrote:
>> >
>> >>  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?
>
> Yep... test-parse-options.c ;-)
Cool, I'll check that.
Show 21 quoted lines
>> >> @@ -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/ ?
>
> And s/xstrdup(\(.*\))/\1/.
Yeah, I understood that.
Show 8 quoted lines
>> > However, a test case would be nice...
>>
>> What would the the test case check?
>
> That 'GIT_CONFIG=bla GIT_EDITOR=echo git config -e' and 'GIT_DIR=blub
> GIT_EDITOR=echo git config -e' do the right thing.  Maybe even --global,
> but that would also be a good test that "git config --global -e" does not
> fail when there is no original file.

Hmm, I'm not sure what issues this test case would find. If there's a problem with launch_editor that's something other test case should find.

If there's no original file it's up to the editor to create one, if for some reason the editor fails at doing that it's a problem of the editor, and there's not much 'git config -e' could do except show an error, and that's what launch_editor would do. Same thing if the editor is wrong (GIT_EDITOR=blah).

I'll resend.
-- 
Felipe Contreras
Felipe Contreras· Feb 3, 2009, 23:26 UTC · re: Felipe Contreras · lore

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.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-config.txt |    6 ++++++
 builtin-config.c             |   10 +++++++++-
 2 files changed, 15 insertions(+), 1 deletions(-)
Show changes to 2 files +15 −1

Documentation/git-config.txt, builtin-config.c

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 19a8917..7d14007 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -22,6 +22,7 @@ SYNOPSIS
 'git config' [<file-option>] [-z|--null] -l | --list
 'git config' [<file-option>] --get-color name [default]
 'git config' [<file-option>] --get-colorbool name [stdout-is-tty]
+'git config' [<file-option>] -e | --edit
 
 DESCRIPTION
 -----------
@@ -157,6 +158,11 @@ See also <<FILES>>.
 	output.  The optional `default` parameter is used instead, if
 	there is no color configured for `name`.
 
+-e::
+--edit::
+	Opens an editor to modify the specified config file; either
+	'--system', '--global', or repository (default).
+
 [[FILES]]
 FILES
 -----
diff --git a/builtin-config.c b/builtin-config.c
index 1582673..0405242 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 ]";
 
 static char *key;
 static regex_t *key_regexp;
@@ -362,6 +362,14 @@ 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")) {
+			const char *config_filename;
+			if (config_exclusive_filename)
+				config_filename = config_exclusive_filename;
+			else
+				config_filename = git_path("config");
+			launch_editor(config_filename, NULL, NULL);
+			return 0;
 		} else
 			break;
 		argc--;

-- 
1.6.1.2
Johannes Schindelin· Feb 3, 2009, 23:31 UTC · re: Felipe Contreras · lore

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

Hi,
On Wed, 4 Feb 2009, Felipe Contreras wrote:
Show 19 quoted lines
> On Wed, Feb 4, 2009 at 1:05 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Wed, 4 Feb 2009, Felipe Contreras wrote:
> >
> >> On Wed, Feb 4, 2009 at 12:53 AM, Johannes Schindelin
> >> <Johannes.Schindelin@gmx.de> wrote:
> >>
> >> > However, a test case would be nice...
> >>
> >> What would the the test case check?
> >
> > That 'GIT_CONFIG=bla GIT_EDITOR=echo git config -e' and 'GIT_DIR=blub 
> > GIT_EDITOR=echo git config -e' do the right thing.  Maybe even 
> > --global, but that would also be a good test that "git config --global 
> > -e" does not fail when there is no original file.
> 
> Hmm, I'm not sure what issues this test case would find. If there's a 
> problem with launch_editor that's something other test case should find.

The purpose of the test case is not to find problems now, but ensure that what the patch is intended to do does not get broken by subsequent patches.

Show 5 quoted lines
> If there's no original file it's up to the editor to create one, if for 
> some reason the editor fails at doing that it's a problem of the editor, 
> and there's not much 'git config -e' could do except show an error, and 
> that's what launch_editor would do. Same thing if the editor is wrong 
> (GIT_EDITOR=blah).

I was more thinking about 'git config --global -e' complaining that it could not find a non-existant file _before_ launching the editor.

Likewise, GIT_EDITOR=echo was meant to output the filename, not to edit the file.

Ciao, Dscho

Felipe Contreras· Feb 3, 2009, 23:43 UTC · re: Johannes Schindelin · lore

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

On Wed, Feb 4, 2009 at 1:31 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 27 quoted lines
> Hi,
>
> On Wed, 4 Feb 2009, Felipe Contreras wrote:
>
>> On Wed, Feb 4, 2009 at 1:05 AM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>> >
>> > On Wed, 4 Feb 2009, Felipe Contreras wrote:
>> >
>> >> On Wed, Feb 4, 2009 at 12:53 AM, Johannes Schindelin
>> >> <Johannes.Schindelin@gmx.de> wrote:
>> >>
>> >> > However, a test case would be nice...
>> >>
>> >> What would the the test case check?
>> >
>> > That 'GIT_CONFIG=bla GIT_EDITOR=echo git config -e' and 'GIT_DIR=blub
>> > GIT_EDITOR=echo git config -e' do the right thing.  Maybe even
>> > --global, but that would also be a good test that "git config --global
>> > -e" does not fail when there is no original file.
>>
>> Hmm, I'm not sure what issues this test case would find. If there's a
>> problem with launch_editor that's something other test case should find.
>
> The purpose of the test case is not to find problems now, but ensure that
> what the patch is intended to do does not get broken by subsequent
> patches.
I know, I wonder what kinds of possible problems this test case would find.
Show 11 quoted lines
>> If there's no original file it's up to the editor to create one, if for
>> some reason the editor fails at doing that it's a problem of the editor,
>> and there's not much 'git config -e' could do except show an error, and
>> that's what launch_editor would do. Same thing if the editor is wrong
>> (GIT_EDITOR=blah).
>
> I was more thinking about 'git config --global -e' complaining that it
> could not find a non-existant file _before_ launching the editor.
>
> Likewise, GIT_EDITOR=echo was meant to output the filename, not to edit
> the file.

You mean if somebody for some reason decides to do some extra checking on the config_filename before executing launch_editor? I seriously doubt anything more would be needed for this --edit option. Can this patch be acked as it is?

-- 
Felipe Contreras
Jeff King· Feb 4, 2009, 14:53 UTC · re: Felipe Contreras · lore

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

On Wed, Feb 04, 2009 at 12:40:26AM +0200, Felipe Contreras wrote:
Show 10 quoted lines
> +		} 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;
>  		} else
With this patch, won't I get different behavior from:
  git config -e --global
versus
  git config --global -e
?
Other than that, I like the concept of this patch.
-Peff
Felipe Contreras· Feb 4, 2009, 15:03 UTC · re: Jeff King · lore

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

On Wed, Feb 4, 2009 at 4:53 PM, Jeff King <peff@peff.net> wrote:
Show 20 quoted lines
> On Wed, Feb 04, 2009 at 12:40:26AM +0200, Felipe Contreras wrote:
>
>> +             } 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;
>>               } else
>
> With this patch, won't I get different behavior from:
>
>  git config -e --global
>
> versus
>
>  git config --global -e
Just like you get different behavior from:
git config -l --global
and
git config --global -l
-- 
Felipe Contreras
Jeff King· Feb 4, 2009, 15:18 UTC · re: Felipe Contreras · lore

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

On Wed, Feb 04, 2009 at 05:03:22PM +0200, Felipe Contreras wrote:
Show 15 quoted lines
> > With this patch, won't I get different behavior from:
> >
> >  git config -e --global
> >
> > versus
> >
> >  git config --global -e
> 
> Just like you get different behavior from:
> 
> git config -l --global
> 
> and
> 
> git config --global -l

Ugh. Personally I consider such interfaces poorly designed. I understand that the general way "git config" works is to have "git config [options] [action]". And when "[action]" is a variable name, or a variable name with a value, it is easy to see what's going on. But when the action looks like an option, it is just confusing that their ordering is important.

However, the interface to "git config" is not going to change, so I think your following existing practice is reasonable here.

_But_ there is one important difference between your "-e" and "-l". In the "-l" case, we detect that there is extra trailing cruft that will be ignored and give a usage message. So "git config -l --global" complains, but "git config -e --global" silently ignores the second argument. I think you just need to add

  if (argc != 2)
    usage(git_config_set_usage);
as the "-l" code does.
-Peff
Felipe Contreras· Feb 4, 2009, 15:41 UTC · re: Jeff King · lore

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

On Wed, Feb 4, 2009 at 5:18 PM, Jeff King <peff@peff.net> wrote:
Show 38 quoted lines
> On Wed, Feb 04, 2009 at 05:03:22PM +0200, Felipe Contreras wrote:
>
>> > With this patch, won't I get different behavior from:
>> >
>> >  git config -e --global
>> >
>> > versus
>> >
>> >  git config --global -e
>>
>> Just like you get different behavior from:
>>
>> git config -l --global
>>
>> and
>>
>> git config --global -l
>
> Ugh. Personally I consider such interfaces poorly designed. I understand
> that the general way "git config" works is to have "git config [options]
> [action]". And when "[action]" is a variable name, or a variable name
> with a value, it is easy to see what's going on. But when the action
> looks like an option, it is just confusing that their ordering is
> important.
>
> However, the interface to "git config" is not going to change, so I
> think your following existing practice is reasonable here.
>
> _But_ there is one important difference between your "-e" and "-l". In
> the "-l" case, we detect that there is extra trailing cruft that will be
> ignored and give a usage message.  So "git config -l --global"
> complains, but "git config -e --global" silently ignores the second
> argument. I think you just need to add
>
>  if (argc != 2)
>    usage(git_config_set_usage);
>
> as the "-l" code does.
Oh, ok, will do.
-- 
Felipe Contreras
Felipe Contreras· Feb 4, 2009, 22:34 UTC · re: Felipe Contreras · lore

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

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.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-config.txt |    6 ++++++
 builtin-config.c             |   12 +++++++++++-
 2 files changed, 17 insertions(+), 1 deletions(-)
Show changes to 2 files +17 −1

Documentation/git-config.txt, builtin-config.c

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 19a8917..7d14007 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -22,6 +22,7 @@ SYNOPSIS
 'git config' [<file-option>] [-z|--null] -l | --list
 'git config' [<file-option>] --get-color name [default]
 'git config' [<file-option>] --get-colorbool name [stdout-is-tty]
+'git config' [<file-option>] -e | --edit
 
 DESCRIPTION
 -----------
@@ -157,6 +158,11 @@ See also <<FILES>>.
 	output.  The optional `default` parameter is used instead, if
 	there is no color configured for `name`.
 
+-e::
+--edit::
+	Opens an editor to modify the specified config file; either
+	'--system', '--global', or repository (default).
+
 [[FILES]]
 FILES
 -----
diff --git a/builtin-config.c b/builtin-config.c
index 1582673..4457b34 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 ]";
 
 static char *key;
 static regex_t *key_regexp;
@@ -362,6 +362,16 @@ 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")) {
+			const char *config_filename;
+			if (argc != 2)
+				usage(git_config_set_usage);
+			if (config_exclusive_filename)
+				config_filename = config_exclusive_filename;
+			else
+				config_filename = git_path("config");
+			launch_editor(config_filename, NULL, NULL);
+			return 0;
 		} else
 			break;
 		argc--;

-- 
1.6.1.2
Junio C Hamano· Feb 4, 2009, 23:16 UTC · re: Felipe Contreras · lore

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

Felipe Contreras <felipe.contreras@gmail.com> writes:
> 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.
Does this honor core.editor setting in existing configuration files?
Felipe Contreras· Feb 4, 2009, 23:39 UTC · re: Junio C Hamano · lore

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

On Thu, Feb 5, 2009 at 1:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> 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.
>
> Does this honor core.editor setting in existing configuration files?
Nope. I guess I need to add git_config(git_default_config, NULL) for that.
-- 
Felipe Contreras
Johannes Schindelin· Feb 4, 2009, 23:43 UTC · re: Junio C Hamano · lore

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

Hi,
On Wed, 4 Feb 2009, Junio C Hamano wrote:
Show 7 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > 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.
> 
> Does this honor core.editor setting in existing configuration files?
It should, as it uses launch_editor().

Ciao, Dscho

Johannes Schindelin· Feb 4, 2009, 23:49 UTC · re: Johannes Schindelin · lore

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

Hi,
On Thu, 5 Feb 2009, Johannes Schindelin wrote:
Show 11 quoted lines
> On Wed, 4 Feb 2009, Junio C Hamano wrote:
> 
> > Felipe Contreras <felipe.contreras@gmail.com> writes:
> > 
> > > 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.
> > 
> > Does this honor core.editor setting in existing configuration files?
> 
> It should, as it uses launch_editor().

Oh, I see that launch_editor() does not do its own git_config() parsing. Dunno if it should...

Ciao, Dscho

Felipe Contreras· Feb 7, 2009, 21:09 UTC · re: Johannes Schindelin · lore

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

On Thu, Feb 5, 2009 at 1:49 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 18 quoted lines
> Hi,
>
> On Thu, 5 Feb 2009, Johannes Schindelin wrote:
>
>> On Wed, 4 Feb 2009, Junio C Hamano wrote:
>>
>> > Felipe Contreras <felipe.contreras@gmail.com> writes:
>> >
>> > > 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.
>> >
>> > Does this honor core.editor setting in existing configuration files?
>>
>> It should, as it uses launch_editor().
>
> Oh, I see that launch_editor() does not do its own git_config() parsing.
> Dunno if it should...

I think it should, how expensive would it be to call git_config(git_default_config, NULL) when it has been loaded already?

-- 
Felipe Contreras
Johannes Schindelin· Feb 7, 2009, 21:14 UTC · re: Felipe Contreras · lore

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

Hi,
On Sat, 7 Feb 2009, Felipe Contreras wrote:
Show 23 quoted lines
> On Thu, Feb 5, 2009 at 1:49 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi,
> >
> > On Thu, 5 Feb 2009, Johannes Schindelin wrote:
> >
> >> On Wed, 4 Feb 2009, Junio C Hamano wrote:
> >>
> >> > Felipe Contreras <felipe.contreras@gmail.com> writes:
> >> >
> >> > > 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.
> >> >
> >> > Does this honor core.editor setting in existing configuration files?
> >>
> >> It should, as it uses launch_editor().
> >
> > Oh, I see that launch_editor() does not do its own git_config() parsing.
> > Dunno if it should...
> 
> I think it should, how expensive would it be to call
> git_config(git_default_config, NULL) when it has been loaded already?
We would not need the complete git_default_config(), would we?

Ciao, Dscho

Felipe Contreras· Feb 7, 2009, 21:15 UTC · re: Johannes Schindelin · lore

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

On Sat, Feb 7, 2009 at 11:14 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 29 quoted lines
> Hi,
>
> On Sat, 7 Feb 2009, Felipe Contreras wrote:
>
>> On Thu, Feb 5, 2009 at 1:49 AM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>> > Hi,
>> >
>> > On Thu, 5 Feb 2009, Johannes Schindelin wrote:
>> >
>> >> On Wed, 4 Feb 2009, Junio C Hamano wrote:
>> >>
>> >> > Felipe Contreras <felipe.contreras@gmail.com> writes:
>> >> >
>> >> > > 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.
>> >> >
>> >> > Does this honor core.editor setting in existing configuration files?
>> >>
>> >> It should, as it uses launch_editor().
>> >
>> > Oh, I see that launch_editor() does not do its own git_config() parsing.
>> > Dunno if it should...
>>
>> I think it should, how expensive would it be to call
>> git_config(git_default_config, NULL) when it has been loaded already?
>
> We would not need the complete git_default_config(), would we?
Nope, just core.editor. But I don't know how to load only that.
-- 
Felipe Contreras
Junio C Hamano· Feb 7, 2009, 21:34 UTC · re: Felipe Contreras · lore

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

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 6 quoted lines
>>> I think it should, how expensive would it be to call
>>> git_config(git_default_config, NULL) when it has been loaded already?
>>
>> We would not need the complete git_default_config(), would we?
>
> Nope, just core.editor. But I don't know how to load only that.

The most expensive part is to actually open and parse the files into tokens, not strcmp/prefixcmp the parsed tokens and flipping internal bits and storing value in const char * variables.

But you need to be careful about correctness issues. I do not want launch_editor() to run git_config(). The caller, other than the caller you happen to be interested in this thread, may already have read the config and overrode some of the bits with what was given from the command line. Calling git_config() overwrite the bits and will break these callers.

I think the right thing to do is to call git_config() immediately before you call launch_editor() in your patch.

Felipe Contreras· Feb 7, 2009, 21:50 UTC · re: Junio C Hamano · lore

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

On Sat, Feb 7, 2009 at 11:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 22 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>>> I think it should, how expensive would it be to call
>>>> git_config(git_default_config, NULL) when it has been loaded already?
>>>
>>> We would not need the complete git_default_config(), would we?
>>
>> Nope, just core.editor. But I don't know how to load only that.
>
> The most expensive part is to actually open and parse the files into
> tokens, not strcmp/prefixcmp the parsed tokens and flipping internal bits
> and storing value in const char * variables.
>
> But you need to be careful about correctness issues.  I do not want
> launch_editor() to run git_config().  The caller, other than the caller
> you happen to be interested in this thread, may already have read the
> config and overrode some of the bits with what was given from the command
> line.  Calling git_config() overwrite the bits and will break these
> callers.
>
> I think the right thing to do is to call git_config() immediately before
> you call launch_editor() in your patch.

Right, also for some reason the caller might actually call a specific editor and not the one configured, but that would require more changes to editor.c

For now I guess this patch should be fine (re-sending).
-- 
Felipe Contreras
Felipe Contreras· Feb 7, 2009, 21:53 UTC · re: Felipe Contreras · lore

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.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-config.txt |    6 ++++++
 builtin-config.c             |   13 ++++++++++++-
 2 files changed, 18 insertions(+), 1 deletions(-)
Show changes to 2 files +18 −1

Documentation/git-config.txt, builtin-config.c

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 19a8917..7d14007 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -22,6 +22,7 @@ SYNOPSIS
 'git config' [<file-option>] [-z|--null] -l | --list
 'git config' [<file-option>] --get-color name [default]
 'git config' [<file-option>] --get-colorbool name [stdout-is-tty]
+'git config' [<file-option>] -e | --edit
 
 DESCRIPTION
 -----------
@@ -157,6 +158,11 @@ See also <<FILES>>.
 	output.  The optional `default` parameter is used instead, if
 	there is no color configured for `name`.
 
+-e::
+--edit::
+	Opens an editor to modify the specified config file; either
+	'--system', '--global', or repository (default).
+
 [[FILES]]
 FILES
 -----
diff --git a/builtin-config.c b/builtin-config.c
index f710162..6937eaf 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 ]";
 
 static char *key;
 static regex_t *key_regexp;
@@ -362,6 +362,17 @@ 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")) {
+			const char *config_filename;
+			if (argc != 2)
+				usage(git_config_set_usage);
+			if (config_exclusive_filename)
+				config_filename = config_exclusive_filename;
+			else
+				config_filename = git_path("config");
+			git_config(git_default_config, NULL);
+			launch_editor(config_filename, NULL, NULL);
+			return 0;
 		} else
 			break;
 		argc--;

-- 
1.6.1.2
Johannes Schindelin· Feb 4, 2009, 15:20 UTC · re: Felipe Contreras · lore

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

Hi,
On Wed, 4 Feb 2009, Felipe Contreras wrote:
Show 29 quoted lines
> On Wed, Feb 4, 2009 at 4:53 PM, Jeff King <peff@peff.net> wrote:
> > On Wed, Feb 04, 2009 at 12:40:26AM +0200, Felipe Contreras wrote:
> >
> >> +             } 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;
> >>               } else
> >
> > With this patch, won't I get different behavior from:
> >
> >  git config -e --global
> >
> > versus
> >
> >  git config --global -e
> 
> Just like you get different behavior from:
> 
> git config -l --global
> 
> and
> 
> git config --global -l
... which will be fixed once you parseopt'ified builtin-config, right?
:-)

Ciao, Dscho

Felipe Contreras· Feb 4, 2009, 15:42 UTC · re: Johannes Schindelin · lore

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

On Wed, Feb 4, 2009 at 5:20 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 35 quoted lines
> Hi,
>
> On Wed, 4 Feb 2009, Felipe Contreras wrote:
>
>> On Wed, Feb 4, 2009 at 4:53 PM, Jeff King <peff@peff.net> wrote:
>> > On Wed, Feb 04, 2009 at 12:40:26AM +0200, Felipe Contreras wrote:
>> >
>> >> +             } 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;
>> >>               } else
>> >
>> > With this patch, won't I get different behavior from:
>> >
>> >  git config -e --global
>> >
>> > versus
>> >
>> >  git config --global -e
>>
>> Just like you get different behavior from:
>>
>> git config -l --global
>>
>> and
>>
>> git config --global -l
>
> ... which will be fixed once you parseopt'ified builtin-config, right?

I'm going to cleanup the big string, I still haven't looked at parseopt, if it's not too complicated I'll do it.

-- 
Felipe Contreras

← back to recent threads