threads / patch / 42070

patchreplace --edit: respect core.editor

Subject: [PATCH] replace --edit: respect core.editor

## tl;dr

10 messages between Apr 19, 2016 and Apr 20, 2016. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Johannes Schindelin· Apr 19, 2016, 14:37 UTC · lore
We simply need to read the config, is all.
This fixes https://github.com/git-for-windows/git/issues/733
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/replace.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to builtin/replace.c +1 −0
diff --git a/builtin/replace.c b/builtin/replace.c
index 748c6ca..02b13f6 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -475,6 +475,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 		return replace_object(argv[0], argv[1], force);
 
 	case MODE_EDIT:
+		git_config(git_default_config, NULL);
 		if (argc != 1)
 			usage_msg_opt("-e needs exactly one argument",
 				      git_replace_usage, options);

-- 
2.8.1.206.g8b39b4a
Junio C Hamano· Apr 19, 2016, 16:22 UTC · re: Johannes Schindelin · lore

Re: [PATCH] replace --edit: respect core.editor

"git blame -L475,6 builtin/replace.c" points at b892bb45 (replace: add --edit option, 2014-04-26) and the commit log message names two people who can review this change, so that is what I am doing here.

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 21 quoted lines
> We simply need to read the config, is all.
>
> This fixes https://github.com/git-for-windows/git/issues/733
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/replace.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index 748c6ca..02b13f6 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -475,6 +475,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
>  		return replace_object(argv[0], argv[1], force);
>  
>  	case MODE_EDIT:
> +		git_config(git_default_config, NULL);
>  		if (argc != 1)
>  			usage_msg_opt("-e needs exactly one argument",
>  				      git_replace_usage, options);
The placement of git_config() makes me wonder why.

I can understand "we only know edit mode needs config, and we know it will never affect other modes to have the new call here", and it would be good for an emergency patch for ancient maintenance track that will not get any other changes or enhancements. I do not think it is a sound reasoning to maintain the codefor the longer term, though.

Jeff King· Apr 20, 2016, 03:53 UTC · re: Junio C Hamano · lore

Re: [PATCH] replace --edit: respect core.editor

On Tue, Apr 19, 2016 at 09:22:37AM -0700, Junio C Hamano wrote:
Show 32 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > We simply need to read the config, is all.
> >
> > This fixes https://github.com/git-for-windows/git/issues/733
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin/replace.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/builtin/replace.c b/builtin/replace.c
> > index 748c6ca..02b13f6 100644
> > --- a/builtin/replace.c
> > +++ b/builtin/replace.c
> > @@ -475,6 +475,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
> >  		return replace_object(argv[0], argv[1], force);
> >  
> >  	case MODE_EDIT:
> > +		git_config(git_default_config, NULL);
> >  		if (argc != 1)
> >  			usage_msg_opt("-e needs exactly one argument",
> >  				      git_replace_usage, options);
> 
> The placement of git_config() makes me wonder why.
> 
> I can understand "we only know edit mode needs config, and we know
> it will never affect other modes to have the new call here", and it
> would be good for an emergency patch for ancient maintenance track
> that will not get any other changes or enhancements.  I do not think
> it is a sound reasoning to maintain the codefor the longer term,
> though.

Yeah. I agree the patch here is not wrong, but I would prefer to just have git-replace load the config when it starts. It's _possible_ that something might break or misbehave, but IMHO any program which breaks when git_default_config() is run is probably in need of fixing.

And I cannot recall any reason we did not read config when "--edit" was added; we just didn't think of it.

-Peff
Christian Couder· Apr 20, 2016, 05:51 UTC · re: Jeff King · lore

Re: [PATCH] replace --edit: respect core.editor

On Wed, Apr 20, 2016 at 5:53 AM, Jeff King <peff@peff.net> wrote:
Show 13 quoted lines
> On Tue, Apr 19, 2016 at 09:22:37AM -0700, Junio C Hamano wrote:
>
>> I can understand "we only know edit mode needs config, and we know
>> it will never affect other modes to have the new call here", and it
>> would be good for an emergency patch for ancient maintenance track
>> that will not get any other changes or enhancements.  I do not think
>> it is a sound reasoning to maintain the codefor the longer term,
>> though.
>
> Yeah. I agree the patch here is not wrong, but I would prefer to just
> have git-replace load the config when it starts. It's _possible_ that
> something might break or misbehave, but IMHO any program which breaks
> when git_default_config() is run is probably in need of fixing.
I agree.
> And I cannot recall any reason we did not read config when "--edit"
> was added; we just didn't think of it.
Yeah I think so.

Thanks, Christian.

Johannes Schindelin· Apr 20, 2016, 06:37 UTC · re: Christian Couder · lore

Re: [PATCH] replace --edit: respect core.editor

Hi Christian & Peff,
On Wed, 20 Apr 2016, Christian Couder wrote:
Show 16 quoted lines
> On Wed, Apr 20, 2016 at 5:53 AM, Jeff King <peff@peff.net> wrote:
> > On Tue, Apr 19, 2016 at 09:22:37AM -0700, Junio C Hamano wrote:
> >
> >> I can understand "we only know edit mode needs config, and we know it
> >> will never affect other modes to have the new call here", and it
> >> would be good for an emergency patch for ancient maintenance track
> >> that will not get any other changes or enhancements.  I do not think
> >> it is a sound reasoning to maintain the codefor the longer term,
> >> though.
> >
> > Yeah. I agree the patch here is not wrong, but I would prefer to just
> > have git-replace load the config when it starts. It's _possible_ that
> > something might break or misbehave, but IMHO any program which breaks
> > when git_default_config() is run is probably in need of fixing.
> 
> I agree.

Okay. I tried to err on the side of caution (side effects? Ever heard of side effects? ;-))

v2 coming.

Ciao, Dscho

Johannes Schindelin· Apr 20, 2016, 06:33 UTC · re: Junio C Hamano · lore

Re: [PATCH] replace --edit: respect core.editor

Hi Junio,
On Tue, 19 Apr 2016, Junio C Hamano wrote:
> "git blame -L475,6 builtin/replace.c" points at b892bb45 (replace: add
> --edit option, 2014-04-26) and the commit log message names two people
> who can review this change, so that is what I am doing here.

D'oh. Sorry. Dscho

Junio C Hamano· Apr 20, 2016, 15:26 UTC · re: Johannes Schindelin · lore

Re: [PATCH] replace --edit: respect core.editor

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 10 quoted lines
> Hi Junio,
>
> On Tue, 19 Apr 2016, Junio C Hamano wrote:
>
>> "git blame -L475,6 builtin/replace.c" points at b892bb45 (replace: add
>> --edit option, 2014-04-26) and the commit log message names two people
>> who can review this change, so that is what I am doing here.
>
> D'oh. Sorry.
> Dscho

Heh, no need to be sorry, that is what maintainers do. If anything, both of us need to thank them for commenting ;-)

Johannes Schindelin· Apr 20, 2016, 06:38 UTC · re: Johannes Schindelin · lore

[PATCH v2] replace --edit: respect core.editor

We simply need to read the config, is all.
This fixes https://github.com/git-for-windows/git/issues/733
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/replace.c | 1 +
 1 file changed, 1 insertion(+)
Interdiff vs v1:
 diff --git a/builtin/replace.c b/builtin/replace.c
 index 02b13f6..b58c714 100644
 --- a/builtin/replace.c
 +++ b/builtin/replace.c
 @@ -440,6 +440,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
  	};
  
  	check_replace_refs = 0;
 +	git_config(git_default_config, NULL);
  
  	argc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);
  
 @@ -475,7 +476,6 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
  		return replace_object(argv[0], argv[1], force);
  
  	case MODE_EDIT:
 -		git_config(git_default_config, NULL);
  		if (argc != 1)
  			usage_msg_opt("-e needs exactly one argument",
  				      git_replace_usage, options);
Show changes to builtin/replace.c +1 −0
diff --git a/builtin/replace.c b/builtin/replace.c
index 748c6ca..b58c714 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -440,6 +440,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 	};
 
 	check_replace_refs = 0;
+	git_config(git_default_config, NULL);
 
 	argc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);
 

-- 
2.8.1.207.g7b140d3
Jeff King· Apr 20, 2016, 06:53 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2] replace --edit: respect core.editor

On Wed, Apr 20, 2016 at 08:38:03AM +0200, Johannes Schindelin wrote:
Show 5 quoted lines
> We simply need to read the config, is all.
> 
> This fixes https://github.com/git-for-windows/git/issues/733
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Looks good to me. Thanks.
-Peff
Junio C Hamano· Apr 20, 2016, 15:29 UTC · re: Jeff King · lore

Re: [PATCH v2] replace --edit: respect core.editor

Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> On Wed, Apr 20, 2016 at 08:38:03AM +0200, Johannes Schindelin wrote:
>
>> We simply need to read the config, is all.
>> 
>> This fixes https://github.com/git-for-windows/git/issues/733
>> 
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Looks good to me. Thanks.

Yup, I think the new placement is at the most logical place, just before parse options. I looked at other codepaths and I do not see a reason why they shouldn't read the configuration variables.

Thanks, both.

← back to recent threads