{"thread":{"id":"56954","subject":"Update to Git 2.34.0 breaks application","startedAt":"2021-11-22T08:42:49Z","lastAt":"2021-12-02T14:48:58Z","messageCount":25,"participants":["Alexander Veit","Phillip Wood","Carlo Marcelo Arenas Belón","Junio C Hamano","Carlo Arenas","Johannes Schindelin","Chris Torek"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"441923","messageId":"ee302c98-da27-da43-e684-c7ec8b225360@gmx.net","threadId":"56954","inReplyTo":null,"subject":"Update to Git 2.34.0 breaks application","fromName":"Alexander Veit","fromEmail":"alexander.veit@gmx.net","sentAt":"2021-11-22T08:42:37Z","receivedAt":"2021-11-22T08:42:49Z","isPatch":false,"sender":{"key":"alexander.veit@gmx.net","avatar":"https://gravatar.com/avatar/6c8f105bf5e4c0df37bc436c17b060f60d2a6bc34945eca584b77b73b5e05eb8?d=mp&s=160"},"body":"After an update from Git 2.25.1 to 2.34.0 the Java application that uses the Eclipse JGit library freezes.\n\nStrace suggests that the JVM receives a SIGTTOU from the child process \"git config --system --edit\" created with java.lang.ProcessBuilder.\n\nUpstream[1] has identified 3d411afa[2] as the possible commit that introduced the problem.\n\nThe problem does not occur on all Linux systems.\n\n\n-Alex\n\nEnvironment:\nOS: Linux Mint 20\nKernel: 5.4.0-90-generic\nGit 2.34.0 from http://ppa.launchpad.net/git-core/ppa/ubuntu\nJava: OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.10+9, mixed mode)\n\n\nRef.:\n[1] https://bugs.eclipse.org/bugs/show_bug.cgi?id=577358\n[2] https://github.com/git/git/commit/3d411afabc9a96f41d47c07d6af6edda3d29ec92#diff-01b59b6a71e42b9c1251ffbf76a1119b965be087a78538e80e01f9239c8e5880\n"},{"id":"442026","messageId":"04ab7301-ea34-476c-eae4-4044fef74b91@gmail.com","threadId":"56954","inReplyTo":"ee302c98-da27-da43-e684-c7ec8b225360@gmx.net","subject":"Re: Update to Git 2.34.0 breaks application","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-11-22T21:43:51Z","receivedAt":"2021-11-22T21:43:59Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alex\n\nThanks for the report\n\nOn 22/11/2021 08:42, Alexander Veit wrote:\n> After an update from Git 2.25.1 to 2.34.0 the Java application that uses \n> the Eclipse JGit library freezes.\n> \n> Strace suggests that the JVM receives a SIGTTOU from the child process \n> \"git config --system --edit\" created with java.lang.ProcessBuilder.\n> \n> Upstream[1] has identified 3d411afa[2] as the possible commit that \n> introduced the problem.\n> \n> The problem does not occur on all Linux systems.\n\nI think the problem is possibly that git is calling tcsetattr() from a \nbackground process group[1]. A possible fix would be to call tcgetpgrp() \nafter opening /dev/tty to see if git is in the foreground process group.\n\nBest Wishes\n\nPhillip\n\n[1] https://www.man7.org/linux/man-pages/man3/tcsetattr.3p.html\n\n> -Alex\n> \n> Environment:\n> OS: Linux Mint 20\n> Kernel: 5.4.0-90-generic\n> Git 2.34.0 from http://ppa.launchpad.net/git-core/ppa/ubuntu\n> Java: OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.10+9, mixed mode)\n> \n> \n> Ref.:\n> [1] https://bugs.eclipse.org/bugs/show_bug.cgi?id=577358\n> [2] \n> https://github.com/git/git/commit/3d411afabc9a96f41d47c07d6af6edda3d29ec92#diff-01b59b6a71e42b9c1251ffbf76a1119b965be087a78538e80e01f9239c8e5880 \n> \n"},{"id":"442039","messageId":"20211122222850.674-1-carenas@gmail.com","threadId":"56954","inReplyTo":"04ab7301-ea34-476c-eae4-4044fef74b91@gmail.com","subject":"[PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-11-22T22:28:50Z","receivedAt":"2021-11-22T22:29:10Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"If the editor is invoked without a controlling terminal, then\nsaving the state and restoring it later is not very useful and\ncould generate signals that the invoking process wouldn't know\nhow to handle.\n\nif git's standard output is not connected to a terminal, then\npresume there is no need to worry if the invoking terminal could\ngarble it.\n\nReported-by: Alexander Veit \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n editor.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/editor.c b/editor.c\nindex 674309eed8..214e3834cb 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -86,7 +86,7 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tp.env = env;\n \t\tp.use_shell = 1;\n \t\tp.trace2_child_class = \"editor\";\n-\t\tterm_fail = save_term(1);\n+\t\tterm_fail = isatty(1) ? save_term(1) : 1;\n \t\tif (start_command(&p) < 0) {\n \t\t\tif (!term_fail)\n \t\t\t\trestore_term();\n-- \n2.34.0.352.g07dee3c5e1\n\n"},{"id":"442057","messageId":"xmqqee77byf9.fsf@gitster.g","threadId":"56954","inReplyTo":"20211122222850.674-1-carenas@gmail.com","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-22T22:47:22Z","receivedAt":"2021-11-22T22:47:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> If the editor is invoked without a controlling terminal, then\n> saving the state and restoring it later is not very useful and\n> could generate signals that the invoking process wouldn't know\n> how to handle.\n>\n> if git's standard output is not connected to a terminal, then\n> presume there is no need to worry if the invoking terminal could\n> garble it.\n>\n> Reported-by: Alexander Veit \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> editor.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/editor.c b/editor.c\n> index 674309eed8..214e3834cb 100644\n> --- a/editor.c\n> +++ b/editor.c\n> @@ -86,7 +86,7 @@ static int launch_specified_editor(const char *editor, const char *path,\n> \t\tp.env = env;\n> \t\tp.use_shell = 1;\n> \t\tp.trace2_child_class = \"editor\";\n> -\t\tterm_fail = save_term(1);\n> +\t\tterm_fail = isatty(1) ? save_term(1) : 1;\n> \t\tif (start_command(&p) < 0) {\n> \t\t\tif (!term_fail)\n> \t\t\t\trestore_term();\n\nI am tempted to say that for 2.34.1 we should revert the offending\ncommit, and for 2.35 and later we should re-queue the offending\ncommit with this fix squashed in.\n\nUnless it can be verified that this fix just works for the reporter\nin 24 hours, that is.\n\nThanks.\n"},{"id":"442060","messageId":"xmqqa6hvbxob.fsf@gitster.g","threadId":"56954","inReplyTo":"20211122222850.674-1-carenas@gmail.com","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-22T23:03:32Z","receivedAt":"2021-11-22T23:03:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> If the editor is invoked without a controlling terminal, then\n> saving the state and restoring it later is not very useful and\n> could generate signals that the invoking process wouldn't know\n> how to handle.\n>\n> if git's standard output is not connected to a terminal, then\n> presume there is no need to worry if the invoking terminal could\n> garble it.\n\nShouldn't the logic apply equally to all callers of save_term()?\n\nIn other words, why aren't we doing this check inside save_term()\nimplementation? i.e. before opening /dev/tty, we can do isatty(1)\nand return -1 if it is false, or something? That way, when we gain\nthe second caller to save/restore other than editor (prehaps the\npager code path wants to do this? I dunno), we do not have to\nremember that isatty() check must be made before doing save_term(),\nno?\n\nIn any case, I am quite tempted to just revert the offending topic\nfor now, but later accept a resurrection patch with this isatty\ncheck rolled in (either at this caller, or inside save_term) when\nthe dust settles.\n\nThanks.\n\n\n> Reported-by: Alexander Veit \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> editor.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/editor.c b/editor.c\n> index 674309eed8..214e3834cb 100644\n> --- a/editor.c\n> +++ b/editor.c\n> @@ -86,7 +86,7 @@ static int launch_specified_editor(const char *editor, const char *path,\n> \t\tp.env = env;\n> \t\tp.use_shell = 1;\n> \t\tp.trace2_child_class = \"editor\";\n> -\t\tterm_fail = save_term(1);\n> +\t\tterm_fail = isatty(1) ? save_term(1) : 1;\n> \t\tif (start_command(&p) < 0) {\n> \t\t\tif (!term_fail)\n> \t\t\t\trestore_term();\n"},{"id":"442061","messageId":"xmqq5ysjbxfm.fsf@gitster.g","threadId":"56954","inReplyTo":"xmqqa6hvbxob.fsf@gitster.g","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-22T23:08:45Z","receivedAt":"2021-11-22T23:08:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> In any case, I am quite tempted to just revert the offending topic\n> for now, but later accept a resurrection patch with this isatty\n> check rolled in (either at this caller, or inside save_term) when\n> the dust settles.\n\nSo, here is that first step, to be hopefully mergeable to 'master'\nand also to 'maint' for 2.34.1\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\nSubject: [PATCH] Revert \"editor: save and reset terminal after calling EDITOR\"\n\nThis reverts commit 3d411afabc9a96f41d47c07d6af6edda3d29ec92,\nblindly opening /dev/tty and calling tcsetattr() seems to be causing\nproblems.\n\ncf. https://bugs.eclipse.org/bugs/show_bug.cgi?id=577358\ncf. https://lore.kernel.org/git/04ab7301-ea34-476c-eae4-4044fef74b91@gmail.com/\n\nSigned-off-by: Junio C Hamano \n---\n editor.c | 8 --------\n 1 file changed, 8 deletions(-)\n\ndiff --git a/editor.c b/editor.c\nindex be7441e7e0..6303ae0ab0 100644\n--- a/editor.c\n+++ b/editor.c\n@@ -3,7 +3,6 @@\n #include \"strbuf.h\"\n #include \"run-command.h\"\n #include \"sigchain.h\"\n-#include \"compat/terminal.h\"\n \n #ifndef DEFAULT_EDITOR\n #define DEFAULT_EDITOR \"vi\"\n@@ -51,8 +50,6 @@ const char *git_sequence_editor(void)\n static int launch_specified_editor(const char *editor, const char *path,\n \t\t\t\t struct strbuf *buffer, const char *const *env)\n {\n-\tint term_fail;\n-\n \tif (!editor)\n \t\treturn error(\"Terminal is dumb, but EDITOR unset\");\n \n@@ -86,10 +83,7 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tp.env = env;\n \t\tp.use_shell = 1;\n \t\tp.trace2_child_class = \"editor\";\n-\t\tterm_fail = save_term(1);\n \t\tif (start_command(&p) < 0) {\n-\t\t\tif (!term_fail)\n-\t\t\t\trestore_term();\n \t\t\tstrbuf_release(&realpath);\n \t\t\treturn error(\"unable to start editor '%s'\", editor);\n \t\t}\n@@ -97,8 +91,6 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tsigchain_push(SIGINT, SIG_IGN);\n \t\tsigchain_push(SIGQUIT, SIG_IGN);\n \t\tret = finish_command(&p);\n-\t\tif (!term_fail)\n-\t\t\trestore_term();\n \t\tstrbuf_release(&realpath);\n \t\tsig = ret - 128;\n \t\tsigchain_pop(SIGINT);\n-- \n2.34.0-222-gceadac38b1\n\n"},{"id":"442065","messageId":"CAPUEspgDafXHHPvzNijTsPsna76yE8W=JH-78LX3jyaieSmp0A@mail.gmail.com","threadId":"56954","inReplyTo":"xmqqa6hvbxob.fsf@gitster.g","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-22T23:39:12Z","receivedAt":"2021-11-22T23:39:28Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Nov 22, 2021 at 3:03 PM Junio C Hamano wrote:\n>\n> Carlo Marcelo Arenas Belón writes:\n>\n> > If the editor is invoked without a controlling terminal, then\n> > saving the state and restoring it later is not very useful and\n> > could generate signals that the invoking process wouldn't know\n> > how to handle.\n> >\n> > if git's standard output is not connected to a terminal, then\n> > presume there is no need to worry if the invoking terminal could\n> > garble it.\n>\n> Shouldn't the logic apply equally to all callers of save_term()?\n>\n> In other words, why aren't we doing this check inside save_term()\n> implementation? i.e. before opening /dev/tty, we can do isatty(1)\n> and return -1 if it is false, or something? That way, when we gain\n> the second caller to save/restore other than editor (prehaps the\n> pager code path wants to do this? I dunno), we do not have to\n> remember that isatty() check must be made before doing save_term(),\n> no?\n\nyes, my plan was to minimize the impact of this bugfix by doing this\nas narrow as possible, but you are correct that if we consider that\nthe only caller for save_term() is in editor.c then it would had make\nmore sense to put it there to begin with and with supportability in\nmind for the future; but save_term() is also called internally, and so\nthe logic would also propagate to other places that use\ncompat/terminal.c (like our fallback version of getpass() or `git add\n-p`).\n\nI should have mentioned though that a better fix was forthcoming, just\nnot with so little time before 2.34.1 gets released.\n\n> In any case, I am quite tempted to just revert the offending topic\n> for now, but later accept a resurrection patch with this isatty\n> check rolled in (either at this caller, or inside save_term) when\n> the dust settles.\n\nI indeed suggested[1] a revert but I wouldn't have proposed this\nalternative if it wouldn't be done safely enough, agree though that\nwithout a successful report that it fixes the problem by the reporter\n(who has since moved[2] on to a different hack which was proposed by\nPeff), a revert is always safer.\n\nCarlo\n\n[1] https://lore.kernel.org/git/xmqqilwjbyj4.fsf@gitster.g/T/#mad3fd4d0015ec939c1e0001444d5affd720d56b2\n[2] https://git.eclipse.org/r/c/jgit/jgit/+/187938\n"},{"id":"442100","messageId":"37d7c35e-54a3-04f5-b03c-b8197a8070a3@nezwerg.de","threadId":"56954","inReplyTo":"xmqq5ysjbxfm.fsf@gitster.g","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Alexander Veit","fromEmail":"list@nezwerg.de","sentAt":"2021-11-23T08:52:08Z","receivedAt":"2021-11-23T09:04:16Z","isPatch":true,"sender":{"key":"list@nezwerg.de","avatar":null},"body":"Am 23.11.21 um 00:08 schrieb Junio C Hamano:\n> So, here is that first step, to be hopefully mergeable to 'master'\n> and also to 'maint' for 2.34.1\n\nI've applied the patch to 'maint' and it worked. The application does not freeze anymore.\n\n-Alex\n"},{"id":"442101","messageId":"CAPUEspiD+-FF5oqA_LrZeVf+tSzUttHdpLGB8QwrGBZ8uqUSKg@mail.gmail.com","threadId":"56954","inReplyTo":"37d7c35e-54a3-04f5-b03c-b8197a8070a3@nezwerg.de","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-23T09:08:53Z","receivedAt":"2021-11-23T09:09:06Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Nov 23, 2021 at 12:52 AM Alexander Veit wrote:\n>\n> Am 23.11.21 um 00:08 schrieb Junio C Hamano:\n> > So, here is that first step, to be hopefully mergeable to 'master'\n> > and also to 'maint' for 2.34.1\n>\n> I've applied the patch to 'maint' and it worked. The application does not freeze anymore.\n\ngreat news; apologies for the disruption.\n\nCarlo\n"},{"id":"442106","messageId":"b1f2257a-044c-17bb-2737-42b8026421eb@gmail.com","threadId":"56954","inReplyTo":"20211122222850.674-1-carenas@gmail.com","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-11-23T11:05:16Z","receivedAt":"2021-11-23T11:05:23Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Carlo\n\nOn 22/11/2021 22:28, Carlo Marcelo Arenas Belón wrote:\n> If the editor is invoked without a controlling terminal, then\n> saving the state and restoring it later is not very useful and\n> could generate signals that the invoking process wouldn't know\n> how to handle.\n> \n> if git's standard output is not connected to a terminal, then\n> presume there is no need to worry if the invoking terminal could\n> garble it.\n\nChecking if stdout is a terminal fixes the Eclipse case where stdout is \na pipe or /dev/null but if git is started in the background from a \nterminal then calling isatty() will not prevent git from receiving \nSIGTTOU. For example if the user is using a gui editor then the \nfollowing used to work\n\nGIT_EDITOR=gedit git commit&\n\nNow git receives SIGTTOU when the editor exits because we call \ntcsetattr() from a background process group. One can argue it does not \nmake much sense to be starting git in the background but it did work \nbefore these changes. I think a combination of isatty() and tcgetpgrp() \nis probably the best solution.\n\nBest Wishes\n\nPhillip\n\n\n> Reported-by: Alexander Veit \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> editor.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/editor.c b/editor.c\n> index 674309eed8..214e3834cb 100644\n> --- a/editor.c\n> +++ b/editor.c\n> @@ -86,7 +86,7 @@ static int launch_specified_editor(const char *editor, const char *path,\n> \t\tp.env = env;\n> \t\tp.use_shell = 1;\n> \t\tp.trace2_child_class = \"editor\";\n> -\t\tterm_fail = save_term(1);\n> +\t\tterm_fail = isatty(1) ? save_term(1) : 1;\n> \t\tif (start_command(&p) < 0) {\n> \t\t\tif (!term_fail)\n> \t\t\t\trestore_term();\n> \n"},{"id":"442177","messageId":"xmqqy25e93zw.fsf@gitster.g","threadId":"56954","inReplyTo":"b1f2257a-044c-17bb-2737-42b8026421eb@gmail.com","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-23T17:27:31Z","receivedAt":"2021-11-23T17:27:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Checking if stdout is a terminal fixes the Eclipse case where stdout\n> is a pipe or /dev/null but if git is started in the background from a \n> terminal then calling isatty() will not prevent git from receiving\n> SIGTTOU. For example if the user is using a gui editor then the \n> following used to work\n>\n> GIT_EDITOR=gedit git commit&\n\nIt is a good one ;-)\n\n> Now git receives SIGTTOU when the editor exits because we call\n> tcsetattr() from a background process group. One can argue it does not \n> make much sense to be starting git in the background but it did work\n> before these changes. I think a combination of isatty() and\n> tcgetpgrp() is probably the best solution.\n\nIf we are not foreground, we can and we should just skip doing this\nwhole save/restore thing, no? The \"editor might screw up the stty\nsetting so we restore to help the buggy editor\" is releavant only\nwhen the said editor actually uses the terminal, and it would get\nTTOU if it tries to do so from the background.\n\nSo, I agree it would be a good thing to do, if we still want to do\nit. In the meantime, I've reverted the offending change from 'master'\nand am planning to merge it down for 2.34.1 but I do not mind taking\na corrected change (not incremental on top of the broken 2.34.0) for\nfuture releases.\n\njgit stopped doing 'GIT_EDITOR=echo git config --system --edit' thing,\nbut not everybody will upgrade immediately.\n"},{"id":"442179","messageId":"CAPUEsphP2GRaHJa0Qnvf22WUnNB+GnnfG8TgHqwJvSGdYfUQGA@mail.gmail.com","threadId":"56954","inReplyTo":"b1f2257a-044c-17bb-2737-42b8026421eb@gmail.com","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-23T17:31:20Z","receivedAt":"2021-11-23T17:31:34Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Nov 23, 2021 at 3:05 AM Phillip Wood wrote:\n> I think a combination of isatty() and tcgetpgrp() is probably the best solution.\n\nDefinitely agree the long term fix needs to include tcgetpgrp() as\nshown by this initial draft[1] (which I apologize, just noticed is\nmissing your \"Helped-by\")\n\nThat of course introduces a regression on the other direction though;\nbefore this change, git compiled to use our getpass() replacement\n(HAVE_DEV_TTY=1) function, the following will be normally stopped by a\nSIGTTOU just like getpass() if running in the background (need to also\nnot have GIT_ASKPASS or SSH_ASKPASS defined in the environment) :\n\n $ echo \"https://user@example.com/\" | git credential fill\n\nI suspect that is probably fine though, as when that happens our\ngetpass() function still misbehaves if put back in the foreground\n(unlike getpass()) and this \"feature\" might be undesired anyway as the\nequivalent C code also runs sometimes in daemon-like processes, and\ncould even explain some of the workarounds put in place to disable\npassword prompts (ex: GIT_TERMINAL_PROMPT=0), but luckily we have all\nthe 2.35 dev cycle to figure out.\n\nRestricting this feature further, maybe through a configuration\nproperty or even special casing the EDITOR is also IMHO a good idea.\n\nCarlo\n\n[1] https://github.com/carenas/git/commit/64d15b2a74206f31e04cf0200f7be83a54a00517\n"},{"id":"442180","messageId":"xmqqtug293ml.fsf@gitster.g","threadId":"56954","inReplyTo":"CAPUEspgDafXHHPvzNijTsPsna76yE8W=JH-78LX3jyaieSmp0A@mail.gmail.com","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-23T17:35:30Z","receivedAt":"2021-11-23T17:35:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas writes:\n\n> yes, my plan was to minimize the impact of this bugfix by doing this\n> as narrow as possible, but you are correct that if we consider that\n> ...\n> I should have mentioned though that a better fix was forthcoming, just\n> not with so little time before 2.34.1 gets released.\n> ...\n>> In any case, I am quite tempted to just revert the offending topic\n>> for now, but later accept a resurrection patch with this isatty\n>> check rolled in (either at this caller, or inside save_term) when\n>> the dust settles.\n>\n> I indeed suggested[1] a revert but I wouldn't have proposed this\n> alternative if it wouldn't be done safely enough,\n\nI think the minimum impact fix is to revert the whole thing (people\nsurvived without it for long time), so that is what 2.34.1 will\nhopefully have. As I said elsewhere, I am open to a rebooted effort\nfor the future cycles, but the conclusion for the topic in 2.34 series\nis that we pretend we never heard about it ;-)\n\nThanks.\n"},{"id":"442239","messageId":"nycvar.QRO.7.76.6.2111241428190.63@tvgsbejvaqbjf.bet","threadId":"56954","inReplyTo":"xmqqtug293ml.fsf@gitster.g","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-24T13:29:29Z","receivedAt":"2021-11-24T13:31:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 23 Nov 2021, Junio C Hamano wrote:\n\n> Carlo Arenas writes:\n>\n> > yes, my plan was to minimize the impact of this bugfix by doing this\n> > as narrow as possible, but you are correct that if we consider that\n> > ...\n> > I should have mentioned though that a better fix was forthcoming, just\n> > not with so little time before 2.34.1 gets released.\n> > ...\n> >> In any case, I am quite tempted to just revert the offending topic\n> >> for now, but later accept a resurrection patch with this isatty\n> >> check rolled in (either at this caller, or inside save_term) when\n> >> the dust settles.\n> >\n> > I indeed suggested[1] a revert but I wouldn't have proposed this\n> > alternative if it wouldn't be done safely enough,\n>\n> I think the minimum impact fix is to revert the whole thing (people\n> survived without it for long time), so that is what 2.34.1 will\n> hopefully have. As I said elsewhere, I am open to a rebooted effort\n> for the future cycles, but the conclusion for the topic in 2.34 series\n> is that we pretend we never heard about it ;-)\n\nMaybe a better approach would be to hide the `save_term()` dance behind a\nnew config option, and then have it turned on automatically if the\n`editor` _happens_ to be `vi` or `vim`.\n\nThat would help the problem reported in the Windows Terminal project.\n\nCiao,\nDscho\n"},{"id":"442256","messageId":"CAPUEspi2PRs+yMUiAmvJK+z=F=kcguHpOL2inAV208yaq2n2Ww@mail.gmail.com","threadId":"56954","inReplyTo":"nycvar.QRO.7.76.6.2111241428190.63@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-24T18:25:41Z","receivedAt":"2021-11-24T18:25:55Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Nov 24, 2021 at 5:29 AM Johannes Schindelin\n wrote:\n>\n> Maybe a better approach would be to hide the `save_term()` dance behind a\n> new config option, and then have it turned on automatically if the\n> `editor` _happens_ to be `vi` or `vim`.\n\nthat but without turning it on automatically was what I was\npreparing[1] (2 commits apply on top of master after the revert) as an\nalternative to present for the 2.35 dev cycle.\n\nI agree though that \"turning it on automatically\" might be worth\nadding for the git for windows fork to minimize impact, but even\nwithout it, I would hope the people affected wouldn't mind setting the\nconfig themselves or could even be done for them at install time by\nthe installer.\n\nCarlo\n\n[1] https://github.com/carenas/git/commits/seen\n"},{"id":"442261","messageId":"xmqqwnkx1h5y.fsf@gitster.g","threadId":"56954","inReplyTo":"nycvar.QRO.7.76.6.2111241428190.63@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-24T19:34:49Z","receivedAt":"2021-11-24T19:35:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Maybe a better approach would be to hide the `save_term()` dance behind a\n> new config option, and then have it turned on automatically if the\n> `editor` _happens_ to be `vi` or `vim`.\n\nWhy 'vi' and 'vim' are so special? Is this an attempt to paper over\na bug in 'vim' on the caller side?\n\n>\n> That would help the problem reported in the Windows Terminal project.\n>\n> Ciao,\n> Dscho\n"},{"id":"442265","messageId":"CAPUEspjBf1MrN_hhNYndJ3MExikZ=rODbHiLNHNfpz1SUKd3RA@mail.gmail.com","threadId":"56954","inReplyTo":"xmqqwnkx1h5y.fsf@gitster.g","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-24T20:04:21Z","receivedAt":"2021-11-24T20:04:47Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Nov 24, 2021 at 11:34 AM Junio C Hamano wrote:\n>\n> Why 'vi' and 'vim' are so special? Is this an attempt to paper over\n> a bug in 'vim' on the caller side?\n\nnot sure if the bug is on vi/vim, cygwin's pty or the new conPTY that\nWindows Terminal uses; but in Git for Windows, using an MSYS vim (that\nuses cygwin's pty) within Windows Terminal (and other terminals that\nuse conPTY most likely) leads to this terminal output corruption, that\nbash \"fixes\" when it gets the control back, but that git does not\n(unless something like what I am proposing is done), and that is\nspecially disruptive when doing several commits in a series (ex: an\ninteractive rebase).\n\nyou could say it is an attempt to paper over that bug, but IMHO it is\nalso a way for git to protect itself from a \"rogue\" editor, as it is\nnow just trusting that the editor wouldn't mess with its terminal\nsettings after being invoked and it returns control to git.\n\nCarlo\n"},{"id":"442270","messageId":"xmqqy25dz39n.fsf@gitster.g","threadId":"56954","inReplyTo":"CAPUEspjBf1MrN_hhNYndJ3MExikZ=rODbHiLNHNfpz1SUKd3RA@mail.gmail.com","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-24T20:51:00Z","receivedAt":"2021-11-24T20:51:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas writes:\n\n> On Wed, Nov 24, 2021 at 11:34 AM Junio C Hamano wrote:\n>>\n>> Why 'vi' and 'vim' are so special? Is this an attempt to paper over\n>> a bug in 'vim' on the caller side?\n>\n> not sure if the bug is on vi/vim, cygwin's pty or the new conPTY that\n> Windows Terminal uses; but in Git for Windows, using an MSYS vim (that\n> uses cygwin's pty) within Windows Terminal (and other terminals that\n> use conPTY most likely) leads to this terminal output corruption, that\n> bash \"fixes\" when it gets the control back, but that git does not\n> (unless something like what I am proposing is done), and that is\n> specially disruptive when doing several commits in a series (ex: an\n> interactive rebase).\n>\n> you could say it is an attempt to paper over that bug, but IMHO it is\n> also a way for git to protect itself from a \"rogue\" editor, as it is\n> now just trusting that the editor wouldn't mess with its terminal\n> settings after being invoked and it returns control to git.\n\nSurely, bash keeps using the same TTY and it may be in its interest\nto \"protect\" it. We on the other hand would exit when we are done\nand no need for the TTY. In other words, bash may \"own\" TTY and it\nmay want to take a good care of it. We don't.\n\nI would have to say that your argument is a bit of stretch ;-).\n"},{"id":"442493","messageId":"nycvar.QRO.7.76.6.2111292208180.63@tvgsbejvaqbjf.bet","threadId":"56954","inReplyTo":"xmqqwnkx1h5y.fsf@gitster.g","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-29T21:12:44Z","receivedAt":"2021-11-29T21:15:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 24 Nov 2021, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > Maybe a better approach would be to hide the `save_term()` dance behind a\n> > new config option, and then have it turned on automatically if the\n> > `editor` _happens_ to be `vi` or `vim`.\n>\n> Why 'vi' and 'vim' are so special?\n\nGit's default editor is `vi`. That's what makes it special.\n\n> Is this an attempt to paper over a bug in 'vim' on the caller side?\n\nWhile this works around a concrete bug reported at\nhttps://github.com/microsoft/terminal/issues/9359, the mere fact that this\nbug was possible indicates that Git needs to be able to deal with such\nbugs, whether the bug is in the editor, in a POSIX emulation layer, in the\nused shell or in the terminal. It is all the more important to have such a\nknob because there are so many potential sources for buggy behavior that\nusers are unlikely to have the expertise to even identify which component\nis at fault.\n\nCiao,\nDscho\n"},{"id":"442625","messageId":"7138fa14-02b2-b123-4d8d-df73763f8431@gmail.com","threadId":"56954","inReplyTo":"CAPUEsphP2GRaHJa0Qnvf22WUnNB+GnnfG8TgHqwJvSGdYfUQGA@mail.gmail.com","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-11-30T11:07:40Z","receivedAt":"2021-11-30T11:07:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 23/11/2021 17:31, Carlo Arenas wrote:\n> On Tue, Nov 23, 2021 at 3:05 AM Phillip Wood wrote:\n>> I think a combination of isatty() and tcgetpgrp() is probably the best solution.\n> \n> Definitely agree the long term fix needs to include tcgetpgrp() as\n> shown by this initial draft[1] (which I apologize, just noticed is\n> missing your \"Helped-by\")\n> \n> That of course introduces a regression on the other direction though;\n> before this change, git compiled to use our getpass() replacement\n> (HAVE_DEV_TTY=1) function, the following will be normally stopped by a\n> SIGTTOU just like getpass() if running in the background (need to also\n> not have GIT_ASKPASS or SSH_ASKPASS defined in the environment) :\n> \n> $ echo \"https://user@example.com/\" | git credential fill\n> \n> I suspect that is probably fine though, as when that happens our\n> getpass() function still misbehaves if put back in the foreground\n> (unlike getpass())\n\nYes, I tried it out and I couldn't get it to work or figure out why. So \nlong as we don't start echoing the password to the screen we should be \nfine. It would be nice to know what the problem is that stops it working \nproperly but that is not really related to this patch.\n\n> and this \"feature\" might be undesired anyway as the\n> equivalent C code also runs sometimes in daemon-like processes, and\n> could even explain some of the workarounds put in place to disable\n> password prompts (ex: GIT_TERMINAL_PROMPT=0), but luckily we have all\n> the 2.35 dev cycle to figure out.\n> \n> Restricting this feature further, maybe through a configuration\n> property or even special casing the EDITOR is also IMHO a good idea.\n\nI think just doing this when we run the editor may be the way to go as I \nthink it is only that case that can mess up the terminal.\n\nBest Wishes\n\nPhillip\n\n> Carlo\n> \n> [1] https://github.com/carenas/git/commit/64d15b2a74206f31e04cf0200f7be83a54a00517\n> \n\n"},{"id":"442741","messageId":"CAPx1GvcML9TvmP1BSLN0vKWD++8LBj-68Xwmz-KrZM32Q=0_Ug@mail.gmail.com","threadId":"56954","inReplyTo":"7138fa14-02b2-b123-4d8d-df73763f8431@gmail.com","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2021-12-01T05:12:12Z","receivedAt":"2021-12-01T05:12:33Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Tue, Nov 30, 2021 at 2:41 PM Phillip Wood wrote:\n> On 23/11/2021 17:31, Carlo Arenas wrote:\n> > Restricting this feature further, maybe through a configuration\n> > property or even special casing the EDITOR is also IMHO a good idea.\n>\n> I think just doing this when we run the editor may be the way to go as I\n> think it is only that case that can mess up the terminal.\n\nIf it only happens with certain versions of vi / vim, perhaps Git could come\nwith a front end program that saves the tty state, runs vim, and restores the\ntty state. (Or set this up so that the program can run any editor.) Then add\na FAQ entry if needed: \"if your editor goofs up the terminal, insert this front\nend program\".\n\nChris\n"},{"id":"442791","messageId":"xmqq35nc15nr.fsf@gitster.g","threadId":"56954","inReplyTo":"CAPx1GvcML9TvmP1BSLN0vKWD++8LBj-68Xwmz-KrZM32Q=0_Ug@mail.gmail.com","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-01T19:33:44Z","receivedAt":"2021-12-01T19:33:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Torek writes:\n\n> On Tue, Nov 30, 2021 at 2:41 PM Phillip Wood wrote:\n>> On 23/11/2021 17:31, Carlo Arenas wrote:\n>> > Restricting this feature further, maybe through a configuration\n>> > property or even special casing the EDITOR is also IMHO a good idea.\n>>\n>> I think just doing this when we run the editor may be the way to go as I\n>> think it is only that case that can mess up the terminal.\n>\n> If it only happens with certain versions of vi / vim, perhaps Git could come\n> with a front end program that saves the tty state, runs vim, and restores the\n> tty state. (Or set this up so that the program can run any editor.) Then add\n> a FAQ entry if needed: \"if your editor goofs up the terminal, insert this front\n> end program\".\n\nThat might work, and because the user is in control, we have less\nrisk of unintended breakage. Doing so unconditionally when the\neditor's name is \"vi\" like Dscho suggested would make it more\nconvenient for the users.\n\nSome editors like Emacs can open a new window and go graphical, even\nwhen they are launched from a terminal. Doing the save/restore on\nthem would be unnecessary. Although I do not offhand think of a way\nsuch an unnecessary save/restore would break the terminal, we have\nalready seen that things can break in an unintended way by doing\nsomething unnecessary in this area, so perhaps the best way forward\nis\n\n - Add a multi-valued configuration variable whose value is the name\n of an editor program that needs this save/restore; optionally, we\n may want a way to say \"don't do save/restore on this editor\",\n e.g. \"!emacs\" may countermand an earlier value that would include\n the editor in the list.\n\n - Around the program invocation in launch_specified_editor(), check\n the name of the editor against this list and do the save/restore\n as necessary;\n\n - When the variable is not defined in the configuration, pretend\n that \"vi\" is on that list (coming up with the list of editors is\n left as an exercise to readers).\n\nThat would give us your flexibility to apply the save/restore on an\narbitrary editor that is not \"vi\", Dscho's convenience to special\ncase \"vi\" out of the box when unconfigured, and an escape hatch for\n\"vi\" users for whom it hurts to do the save/restore on their \"vi\".\n\nHmm?\n"},{"id":"442837","messageId":"xmqq7dcnyh5o.fsf@gitster.g","threadId":"56954","inReplyTo":"xmqq35nc15nr.fsf@gitster.g","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-02T00:38:59Z","receivedAt":"2021-12-02T00:39:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> - Add a multi-valued configuration variable whose value is the name\n> of an editor program that needs this save/restore; optionally, we\n> may want a way to say \"don't do save/restore on this editor\",\n> e.g. \"!emacs\" may countermand an earlier value that would include\n> the editor in the list.\n>\n> - Around the program invocation in launch_specified_editor(), check\n> the name of the editor against this list and do the save/restore\n> as necessary;\n>\n> - When the variable is not defined in the configuration, pretend\n> that \"vi\" is on that list (coming up with the list of editors is\n> left as an exercise to readers).\n>\n> That would give us your flexibility to apply the save/restore on an\n> arbitrary editor that is not \"vi\", Dscho's convenience to special\n> case \"vi\" out of the box when unconfigured, and an escape hatch for\n> \"vi\" users for whom it hurts to do the save/restore on their \"vi\".\n>\n> Hmm?\n\nThat's an overkill.\n\nA single configuration variable as an escape hatch, that enables the\nsave/restore around editor invocation, whose default value is\ndetermined by the name of the editor, is probably the right degree\nof flexibility.\n\nSomething along this line, perhaps?\n\n editor.c | 18 +++++++++++++++++-\n 1 file changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git c/editor.c w/editor.c\nindex fdd3eeafa9..70d3f80966 100644\n--- c/editor.c\n+++ w/editor.c\n@@ -3,6 +3,7 @@\n #include \"strbuf.h\"\n #include \"run-command.h\"\n #include \"sigchain.h\"\n+#include \"compat/terminal.h\"\n \n #ifndef DEFAULT_EDITOR\n #define DEFAULT_EDITOR \"vi\"\n@@ -47,6 +48,16 @@ const char *git_sequence_editor(void)\n \treturn editor;\n }\n \n+static int prepare_term(const char *editor)\n+{\n+\tint need_saverestore = !strcmp(editor, \"vi\");\n+\n+\tgit_config_get_bool(\"editor.stty\", &need_saverestore);\n+\tif (need_saverestore)\n+\t\treturn save_term(1);\n+\treturn 0;\n+}\n+\n static int launch_specified_editor(const char *editor, const char *path,\n \t\t\t\t struct strbuf *buffer, const char *const *env)\n {\n@@ -57,7 +68,7 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tstruct strbuf realpath = STRBUF_INIT;\n \t\tconst char *args[] = { editor, NULL, NULL };\n \t\tstruct child_process p = CHILD_PROCESS_INIT;\n-\t\tint ret, sig;\n+\t\tint ret, sig, need_restore = 0;\n \t\tint print_waiting_for_editor = advice_enabled(ADVICE_WAITING_FOR_EDITOR) && isatty(2);\n \n \t\tif (print_waiting_for_editor) {\n@@ -83,7 +94,10 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tp.env = env;\n \t\tp.use_shell = 1;\n \t\tp.trace2_child_class = \"editor\";\n+\t\tneed_restore = prepare_term(editor);\n \t\tif (start_command(&p) < 0) {\n+\t\t\tif (need_restore)\n+\t\t\t\trestore_term();\n \t\t\tstrbuf_release(&realpath);\n \t\t\treturn error(\"unable to start editor '%s'\", editor);\n \t\t}\n@@ -91,6 +105,8 @@ static int launch_specified_editor(const char *editor, const char *path,\n \t\tsigchain_push(SIGINT, SIG_IGN);\n \t\tsigchain_push(SIGQUIT, SIG_IGN);\n \t\tret = finish_command(&p);\n+\t\tif (need_restore)\n+\t\t\trestore_term();\n \t\tstrbuf_release(&realpath);\n \t\tsig = ret - 128;\n \t\tsigchain_pop(SIGINT);\n"},{"id":"442842","messageId":"CAPUEsphktbdxeV7hvF52Or3CVHS8oOk5-WV=xfEZa8kfCVVnVg@mail.gmail.com","threadId":"56954","inReplyTo":"xmqq7dcnyh5o.fsf@gitster.g","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-12-02T01:51:28Z","receivedAt":"2021-12-02T01:53:07Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Dec 1, 2021 at 4:39 PM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > - Add a multi-valued configuration variable whose value is the name\n> > of an editor program that needs this save/restore; optionally, we\n> > may want a way to say \"don't do save/restore on this editor\",\n> > e.g. \"!emacs\" may countermand an earlier value that would include\n> > the editor in the list.\n> >\n> > - Around the program invocation in launch_specified_editor(), check\n> > the name of the editor against this list and do the save/restore\n> > as necessary;\n> >\n> > - When the variable is not defined in the configuration, pretend\n> > that \"vi\" is on that list (coming up with the list of editors is\n> > left as an exercise to readers).\n> >\n> > That would give us your flexibility to apply the save/restore on an\n> > arbitrary editor that is not \"vi\", Dscho's convenience to special\n> > case \"vi\" out of the box when unconfigured, and an escape hatch for\n> > \"vi\" users for whom it hurts to do the save/restore on their \"vi\".\n> >\n> > Hmm?\n>\n> That's an overkill.\n\nHa!, I was going to say that before, but instead went and implemented\n(most of) it, discarding my simpler version[1] that was instead using\na boolean config and is otherwise similar to yours (but instead with a\ndefault of false, and obviously not as good looking).\n\nI was assuming the user would know better when the feature was needed\nor not (obviously not if their editor is not even terminal based,\nwhich we can't figure out programmatically anyway), and it was on Git\nfor Windows users that want to also use vi, to enable it[2]; instead\nof \"affecting\" all vi users from all platforms by default, even if we\nhave reports[3] of some of them being affected before.\n\nA couple of things that are still missing here IMHO are :\n\n* need to also disable if not in the foreground (which was really the\nroot cause of the reported regression, and needed to address Phillip's\nrelated report[4]; I cover this in my 1/2, so no need to change this\none)\n* probably still should do the isatty(2) to disable (at the editor\nlevel), as another way to protect against people scripting around\nthis, but also because it just doesn't make sense to protect a\nterminal from corruption that is not being seen by a human, and as you\npointed out, git doesn't need to be that protective of the tty as bash\ndoes (which will likely fix it itself, or let the user run `reset` to\ndo so).\n\nWill prepare a new series including your patch for review and a third\none with proposed changes to yours from the second bullet point, that\nmight be worth squashing instead, or that at least could be used as a\ndiscussion starter, or discarded if you already thought about those\nand decided against.\n\nCarlo\n\n[1] https://github.com/carenas/git/commit/910c158b42994132de2480c018b44616962a2a20\n[2] https://github.com/git-for-windows/build-extra/pull/399\n[3] https://lore.kernel.org/git/xmqqwnmswxs7.fsf@gitster.g/\n[4] https://lore.kernel.org/git/b1f2257a-044c-17bb-2737-42b8026421eb@gmail.com/\n"},{"id":"442869","messageId":"nycvar.QRO.7.76.6.2112021542390.63@tvgsbejvaqbjf.bet","threadId":"56954","inReplyTo":"CAPx1GvcML9TvmP1BSLN0vKWD++8LBj-68Xwmz-KrZM32Q=0_Ug@mail.gmail.com","subject":"Re: [PATCH] editor: only save (and restore) the terminal if using a tty","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-12-02T14:48:42Z","receivedAt":"2021-12-02T14:48:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Chris,\n\nOn Tue, 30 Nov 2021, Chris Torek wrote:\n\n> On Tue, Nov 30, 2021 at 2:41 PM Phillip Wood wrote:\n> > On 23/11/2021 17:31, Carlo Arenas wrote:\n> > > Restricting this feature further, maybe through a configuration\n> > > property or even special casing the EDITOR is also IMHO a good idea.\n> >\n> > I think just doing this when we run the editor may be the way to go as I\n> > think it is only that case that can mess up the terminal.\n>\n> If it only happens with certain versions of vi / vim, perhaps Git could come\n> with a front end program that saves the tty state, runs vim, and restores the\n> tty state. (Or set this up so that the program can run any editor.) Then add\n> a FAQ entry if needed: \"if your editor goofs up the terminal, insert this front\n> end program\".\n\nPutting myself into an end-user's shoes, I would much prefer not to be\nasked to do extra work and I imagine that the millions of other users who\ngo with Git's default editor have the very same preference.\n\nJust like we default to `LESS=FRX` for Git's default pager, and just like\nwe special-case `less` and `vi` in `git grep -O`, it makes sense\nto special-case `vi` when saving/restoring the terminal because we know it\nhelps users.\n\nAnd by \"special-case\", I want to say that Git should provide a paved path\nto success, not a workaround that users still have to configure manually.\n\nCiao,\nDscho\n"}]}