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

On Wed, Feb 4, 2009 at 1:05 AM, Johannes Schindelin
[off-list ref] wrote:

Hi,

On Wed, 4 Feb 2009, Felipe Contreras wrote:

quoted

On Wed, Feb 4, 2009 at 12:53 AM, Johannes Schindelin
[off-list ref] wrote:

quoted

On Wed, 4 Feb 2009, Felipe Contreras wrote:

quoted

 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.

quoted

quoted

quoted

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

quoted

quoted

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