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

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

[Felipe Contreras]

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(-)

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

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

[Johannes Schindelin]

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... ;-)

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

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

[Felipe Contreras]

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

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

[Johannes Schindelin]

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

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

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

[Felipe Contreras]

On Wed, Feb 4, 2009 at 1:05 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> 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.

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

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

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

[Felipe Contreras]

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(-)

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

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

[Johannes Schindelin]

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.

> 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

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

[Felipe Contreras]

On Wed, Feb 4, 2009 at 1:31 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> 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.

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

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

[Jeff King]

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

?

Other than that, I like the concept of this patch.

-Peff

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

[Felipe Contreras]

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

-- 
Felipe Contreras

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

[Jeff King]

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.

-Peff

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

[Johannes Schindelin]

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?

:-)

Ciao,
Dscho

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

[Felipe Contreras]

On Wed, Feb 4, 2009 at 5:18 PM, Jeff King <peff@peff.net> wrote:
> 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

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

[Felipe Contreras]

On Wed, Feb 4, 2009 at 5:20 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> 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

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

[Felipe Contreras]

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(-)

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

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

[Junio C Hamano]

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?

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

[Felipe Contreras]

On Thu, Feb 5, 2009 at 1:16 AM, Junio C Hamano <gitster@pobox.com> 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?

Nope. I guess I need to add git_config(git_default_config, NULL) for that.

-- 
Felipe Contreras

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

[Johannes Schindelin]

Hi,

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().

Ciao,
Dscho

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

[Johannes Schindelin]

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

Ciao,
Dscho

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

[Felipe Contreras]

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?

-- 
Felipe Contreras

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

[Johannes Schindelin]

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?

Ciao,
Dscho

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

[Felipe Contreras]

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

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

[Junio C Hamano]

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.

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

[Felipe Contreras]

On Sat, Feb 7, 2009 at 11:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Felipe Contreras]

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(-)

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