From: Felipe Contreras Date: Sun, 15 Feb 2009 12:51:45 GMT Subject: Re: [PATCH v2] config: Use parseopt. Message-ID: <94a0d4530902150451w2467bee2q225643f15ffffac1@mail.gmail.com> In-Reply-To: On Sun, Feb 15, 2009 at 2:33 PM, Johannes Schindelin wrote: > Hi, > > On Sun, 15 Feb 2009, Felipe Contreras wrote: > >> On Sun, Feb 15, 2009 at 1:26 PM, Johannes Schindelin >> wrote: >> > Hi, >> > >> > On Sun, 15 Feb 2009, Felipe Contreras wrote: >> > >> >> On Sun, Feb 15, 2009 at 12:32 AM, Johannes Schindelin >> >> wrote: >> >> > Hi, >> >> > >> >> > On Sat, 14 Feb 2009, Felipe Contreras wrote: >> >> > >> >> >> On Sat, Feb 14, 2009 at 9:59 PM, Johannes Schindelin >> >> >> 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 From 4b3db3d13261849cd7d561dc20bddb375f1430c8 Mon Sep 17 00:00:00 2001 From: Felipe Contreras 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 --- 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