Re: [PATCH v2] config: Use parseopt.
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Feb 15, 2009, 12:33 UTC
- Message-ID
- <alpine.DEB.1.00.0902151329370.10279@pacific.mpi-cbg.de>
- In-Reply-To
- <94a0d4530902150407u1bc6669cr2584ff8f1a8b47af@mail.gmail.com>
Hi,
On Sun, 15 Feb 2009, Felipe Contreras wrote:
Show 42 quoted lines
> 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.)
Ciao, Dscho