Re: [PATCH v2] config: Use parseopt.

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

[-- Attachment #1: Type: text/plain, Size: 2781 bytes --]

On Sun, Feb 15, 2009 at 2:33 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sun, 15 Feb 2009, Felipe Contreras wrote:
>
>> On Sun, Feb 15, 2009 at 1:26 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>> > Hi,
>> >
>> > On Sun, 15 Feb 2009, Felipe Contreras wrote:
>> >
>> >> On Sun, Feb 15, 2009 at 12:32 AM, Johannes Schindelin
>> >> <Johannes.Schindelin@gmx.de> wrote:
>> >> > Hi,
>> >> >
>> >> > On Sat, 14 Feb 2009, Felipe Contreras wrote:
>> >> >
>> >> >> On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin
>> >> >> <Johannes.Schindelin@gmx.de> wrote:
>> >> >>
>> >> >> > On Sat, 14 Feb 2009, Felipe Contreras wrote:
>> >> >> >
>> >> >> >> +     else if (actions & ACTION_EDIT) {
>> >> >> >> +             const char *config_filename;
>> >> >> >> +             if (config_exclusive_filename)
>> >> >> >> +                     config_filename = config_exclusive_filename;
>> >> >> >> +             else
>> >> >> >> +                     config_filename = git_path("config");
>> >> >> >
>> >> >> > Why not reuse config_exclusive_filename here?
>> >> >>
>> >> >> You mean:
>> >> >> if (!config_exclusive_filename)
>> >> >>   config_exclusive_filename = git_path("config");
>> >> >
>> >> > Yes.
>> >>
>> >> I'm not sure about this one. At least git_config should be moved before
>> >> that code, otherwise it will only try to read core.editor from the
>> >> exclusive_filename and that's not what we want.
>> >
>> > Ah!  I did not think about core.editor, of course, but that is what got
>> > you started with git-config, after all.
>> >
>> > However, the next line sets config_exclusive_filename, does it not?
>>
>> Huh? Which line?
>
> Ah, I misremembered.  Your current patch shows this:
>
> +       else if (actions & ACTION_EDIT) {
> +               const char *config_filename;
> +               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);
> +       }
>
> ... which makes me believe that the point about moving git_config() before
> setting config_exclusive_filename is rather weak :-)
>
> +       else if (actions & ACTION_EDIT) {
> +               git_config(git_default_config, NULL);
> +               launch_editor(config_exclusive_filename ?
> +                       config_exclusive_filename : git_path("config"),
> +                       NULL, NULL);
> +       }
>
> (This has whitespace issues, as I copy-pasted it using kterm's clipboard
> functions.)

I thought on that before but now it makes more sense to me. Attaching the patch.

-- 
Felipe Contreras

[-- Attachment #2: 0001-config-Cleanup-editor-action.patch --]
[-- Type: application/octet-stream, Size: 1142 bytes --]

From 4b3db3d13261849cd7d561dc20bddb375f1430c8 Mon Sep 17 00:00:00 2001
From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Sun, 15 Feb 2009 14:46:58 +0200
Subject: [PATCH] config: Cleanup editor action.

Copy-paste from Johannes Schindelin.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin-config.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin-config.c b/builtin-config.c
index 2c1ad71..bd7bac4 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -370,13 +370,10 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)
 	}
 	else if (actions & ACTION_EDIT) {
 		check_argc(argc, 0, 0);
-		const char *config_filename;
-		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);
+		launch_editor(config_exclusive_filename ?
+			      config_exclusive_filename : git_path("config"),
+			      NULL, NULL);
 	}
 	else if (actions & ACTION_ADD) {
 		check_argc(argc, 2, 2);
-- 
1.6.1.3