{"thread":{"id":"42070","subject":"[PATCH] replace --edit: respect core.editor","startedAt":"2016-04-19T14:37:00Z","lastAt":"2016-04-20T15:29:18Z","messageCount":10,"participants":["Johannes Schindelin","Junio C Hamano","Jeff King","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"283789","messageId":"909769abaff1babdab77625bebd04e2013c6e344.1461076425.git.johannes.schindelin@gmx.de","threadId":"42070","inReplyTo":null,"subject":"[PATCH] replace --edit: respect core.editor","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-19T14:37:00Z","receivedAt":"2016-04-19T14:37:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"We simply need to read the config, is all.\n\nThis fixes https://github.com/git-for-windows/git/issues/733\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/replace.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 748c6ca..02b13f6 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -475,6 +475,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \t\treturn replace_object(argv[0], argv[1], force);\n \n \tcase MODE_EDIT:\n+\t\tgit_config(git_default_config, NULL);\n \t\tif (argc != 1)\n \t\t\tusage_msg_opt(\"-e needs exactly one argument\",\n \t\t\t\t git_replace_usage, options);\n-- \n2.8.1.206.g8b39b4a\n"},{"id":"283800","messageId":"xmqqoa9536wi.fsf@gitster.mtv.corp.google.com","threadId":"42070","inReplyTo":"909769abaff1babdab77625bebd04e2013c6e344.1461076425.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] replace --edit: respect core.editor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-19T16:22:37Z","receivedAt":"2016-04-19T16:22:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git blame -L475,6 builtin/replace.c\" points at b892bb45 (replace:\nadd --edit option, 2014-04-26) and the commit log message names two\npeople who can review this change, so that is what I am doing here.\n\nJohannes Schindelin writes:\n\n> We simply need to read the config, is all.\n>\n> This fixes https://github.com/git-for-windows/git/issues/733\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/replace.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index 748c6ca..02b13f6 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -475,6 +475,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n> \t\treturn replace_object(argv[0], argv[1], force);\n> \n> \tcase MODE_EDIT:\n> +\t\tgit_config(git_default_config, NULL);\n> \t\tif (argc != 1)\n> \t\t\tusage_msg_opt(\"-e needs exactly one argument\",\n> \t\t\t\t git_replace_usage, options);\n\nThe placement of git_config() makes me wonder why.\n\nI can understand \"we only know edit mode needs config, and we know\nit will never affect other modes to have the new call here\", and it\nwould be good for an emergency patch for ancient maintenance track\nthat will not get any other changes or enhancements. I do not think\nit is a sound reasoning to maintain the codefor the longer term,\nthough.\n"},{"id":"283901","messageId":"20160420035353.GA31890@sigill.intra.peff.net","threadId":"42070","inReplyTo":"xmqqoa9536wi.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] replace --edit: respect core.editor","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-20T03:53:54Z","receivedAt":"2016-04-20T03:53:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 19, 2016 at 09:22:37AM -0700, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > We simply need to read the config, is all.\n> >\n> > This fixes https://github.com/git-for-windows/git/issues/733\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > builtin/replace.c | 1 +\n> > 1 file changed, 1 insertion(+)\n> >\n> > diff --git a/builtin/replace.c b/builtin/replace.c\n> > index 748c6ca..02b13f6 100644\n> > --- a/builtin/replace.c\n> > +++ b/builtin/replace.c\n> > @@ -475,6 +475,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n> > \t\treturn replace_object(argv[0], argv[1], force);\n> > \n> > \tcase MODE_EDIT:\n> > +\t\tgit_config(git_default_config, NULL);\n> > \t\tif (argc != 1)\n> > \t\t\tusage_msg_opt(\"-e needs exactly one argument\",\n> > \t\t\t\t git_replace_usage, options);\n> \n> The placement of git_config() makes me wonder why.\n> \n> I can understand \"we only know edit mode needs config, and we know\n> it will never affect other modes to have the new call here\", and it\n> would be good for an emergency patch for ancient maintenance track\n> that will not get any other changes or enhancements. I do not think\n> it is a sound reasoning to maintain the codefor the longer term,\n> though.\n\nYeah. I agree the patch here is not wrong, but I would prefer to just\nhave git-replace load the config when it starts. It's _possible_ that\nsomething might break or misbehave, but IMHO any program which breaks\nwhen git_default_config() is run is probably in need of fixing.\n\nAnd I cannot recall any reason we did not read config when \"--edit\"\nwas added; we just didn't think of it.\n\n-Peff\n"},{"id":"283905","messageId":"CAP8UFD189+AcLstEx4s_2ZYWp6UOtDb=vWGYhCpJayM8ma5hxw@mail.gmail.com","threadId":"42070","inReplyTo":"20160420035353.GA31890@sigill.intra.peff.net","subject":"Re: [PATCH] replace --edit: respect core.editor","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-04-20T05:51:43Z","receivedAt":"2016-04-20T05:51:43Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Apr 20, 2016 at 5:53 AM, Jeff King wrote:\n> On Tue, Apr 19, 2016 at 09:22:37AM -0700, Junio C Hamano wrote:\n>\n>> I can understand \"we only know edit mode needs config, and we know\n>> it will never affect other modes to have the new call here\", and it\n>> would be good for an emergency patch for ancient maintenance track\n>> that will not get any other changes or enhancements. I do not think\n>> it is a sound reasoning to maintain the codefor the longer term,\n>> though.\n>\n> Yeah. I agree the patch here is not wrong, but I would prefer to just\n> have git-replace load the config when it starts. It's _possible_ that\n> something might break or misbehave, but IMHO any program which breaks\n> when git_default_config() is run is probably in need of fixing.\n\nI agree.\n\n> And I cannot recall any reason we did not read config when \"--edit\"\n> was added; we just didn't think of it.\n\nYeah I think so.\n\nThanks,\nChristian.\n"},{"id":"283907","messageId":"alpine.DEB.2.20.1604200832430.2826@virtualbox","threadId":"42070","inReplyTo":"xmqqoa9536wi.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] replace --edit: respect core.editor","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-20T06:33:10Z","receivedAt":"2016-04-20T06:33:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 19 Apr 2016, Junio C Hamano wrote:\n\n> \"git blame -L475,6 builtin/replace.c\" points at b892bb45 (replace: add\n> --edit option, 2014-04-26) and the commit log message names two people\n> who can review this change, so that is what I am doing here.\n\nD'oh. Sorry.\nDscho\n"},{"id":"283908","messageId":"alpine.DEB.2.20.1604200836050.2826@virtualbox","threadId":"42070","inReplyTo":"CAP8UFD189+AcLstEx4s_2ZYWp6UOtDb=vWGYhCpJayM8ma5hxw@mail.gmail.com","subject":"Re: [PATCH] replace --edit: respect core.editor","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-20T06:37:21Z","receivedAt":"2016-04-20T06:37:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Christian & Peff,\n\nOn Wed, 20 Apr 2016, Christian Couder wrote:\n\n> On Wed, Apr 20, 2016 at 5:53 AM, Jeff King wrote:\n> > On Tue, Apr 19, 2016 at 09:22:37AM -0700, Junio C Hamano wrote:\n> >\n> >> I can understand \"we only know edit mode needs config, and we know it\n> >> will never affect other modes to have the new call here\", and it\n> >> would be good for an emergency patch for ancient maintenance track\n> >> that will not get any other changes or enhancements. I do not think\n> >> it is a sound reasoning to maintain the codefor the longer term,\n> >> though.\n> >\n> > Yeah. I agree the patch here is not wrong, but I would prefer to just\n> > have git-replace load the config when it starts. It's _possible_ that\n> > something might break or misbehave, but IMHO any program which breaks\n> > when git_default_config() is run is probably in need of fixing.\n> \n> I agree.\n\nOkay. I tried to err on the side of caution (side effects? Ever heard of\nside effects? ;-))\n\nv2 coming.\n\nCiao,\nDscho\n"},{"id":"283909","messageId":"a7b889d525f99fecfb9d4a890fd2a5d4bf3001e0.1461134258.git.johannes.schindelin@gmx.de","threadId":"42070","inReplyTo":"909769abaff1babdab77625bebd04e2013c6e344.1461076425.git.johannes.schindelin@gmx.de","subject":"[PATCH v2] replace --edit: respect core.editor","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-20T06:38:03Z","receivedAt":"2016-04-20T06:38:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"We simply need to read the config, is all.\n\nThis fixes https://github.com/git-for-windows/git/issues/733\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/replace.c | 1 +\n 1 file changed, 1 insertion(+)\nInterdiff vs v1:\n\n diff --git a/builtin/replace.c b/builtin/replace.c\n index 02b13f6..b58c714 100644\n --- a/builtin/replace.c\n +++ b/builtin/replace.c\n @@ -440,6 +440,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \t};\n \n \tcheck_replace_refs = 0;\n +\tgit_config(git_default_config, NULL);\n \n \targc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);\n \n @@ -475,7 +476,6 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \t\treturn replace_object(argv[0], argv[1], force);\n \n \tcase MODE_EDIT:\n -\t\tgit_config(git_default_config, NULL);\n \t\tif (argc != 1)\n \t\t\tusage_msg_opt(\"-e needs exactly one argument\",\n \t\t\t\t git_replace_usage, options);\n\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 748c6ca..b58c714 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -440,6 +440,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \t};\n \n \tcheck_replace_refs = 0;\n+\tgit_config(git_default_config, NULL);\n \n \targc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);\n \n-- \n2.8.1.207.g7b140d3\n"},{"id":"283910","messageId":"20160420065349.GA14490@sigill.intra.peff.net","threadId":"42070","inReplyTo":"a7b889d525f99fecfb9d4a890fd2a5d4bf3001e0.1461134258.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2] replace --edit: respect core.editor","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-20T06:53:49Z","receivedAt":"2016-04-20T06:53:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 20, 2016 at 08:38:03AM +0200, Johannes Schindelin wrote:\n\n> We simply need to read the config, is all.\n> \n> This fixes https://github.com/git-for-windows/git/issues/733\n> \n> Signed-off-by: Johannes Schindelin \n\nLooks good to me. Thanks.\n\n-Peff\n"},{"id":"283949","messageId":"xmqq7ffsxpvh.fsf@gitster.mtv.corp.google.com","threadId":"42070","inReplyTo":"alpine.DEB.2.20.1604200832430.2826@virtualbox","subject":"Re: [PATCH] replace --edit: respect core.editor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-20T15:26:58Z","receivedAt":"2016-04-20T15:26:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Junio,\n>\n> On Tue, 19 Apr 2016, Junio C Hamano wrote:\n>\n>> \"git blame -L475,6 builtin/replace.c\" points at b892bb45 (replace: add\n>> --edit option, 2014-04-26) and the commit log message names two people\n>> who can review this change, so that is what I am doing here.\n>\n> D'oh. Sorry.\n> Dscho\n\nHeh, no need to be sorry, that is what maintainers do. If anything,\nboth of us need to thank them for commenting ;-)\n"},{"id":"283950","messageId":"xmqq37qgxprl.fsf@gitster.mtv.corp.google.com","threadId":"42070","inReplyTo":"20160420065349.GA14490@sigill.intra.peff.net","subject":"Re: [PATCH v2] replace --edit: respect core.editor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-20T15:29:18Z","receivedAt":"2016-04-20T15:29:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Apr 20, 2016 at 08:38:03AM +0200, Johannes Schindelin wrote:\n>\n>> We simply need to read the config, is all.\n>> \n>> This fixes https://github.com/git-for-windows/git/issues/733\n>> \n>> Signed-off-by: Johannes Schindelin \n>\n> Looks good to me. Thanks.\n\nYup, I think the new placement is at the most logical place, just\nbefore parse options. I looked at other codepaths and I do not see\na reason why they shouldn't read the configuration variables.\n\nThanks, both.\n"}]}