Update to Git 2.34.0 breaks application

Update to Git 2.34.0 breaks application

[Alexander Veit]

After an update from Git 2.25.1 to 2.34.0 the Java application that uses the Eclipse JGit library freezes.

Strace suggests that the JVM receives a SIGTTOU from the child process "git config --system --edit" created with java.lang.ProcessBuilder.

Upstream[1] has identified 3d411afa[2] as the possible commit that introduced the problem.

The problem does not occur on all Linux systems.


-Alex

Environment:
OS: Linux Mint 20
Kernel: 5.4.0-90-generic
Git 2.34.0 from http://ppa.launchpad.net/git-core/ppa/ubuntu
Java: OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.10+9, mixed mode)


Ref.:
[1] https://bugs.eclipse.org/bugs/show_bug.cgi?id=577358
[2] https://github.com/git/git/commit/3d411afabc9a96f41d47c07d6af6edda3d29ec92#diff-01b59b6a71e42b9c1251ffbf76a1119b965be087a78538e80e01f9239c8e5880

Re: Update to Git 2.34.0 breaks application

[Phillip Wood]

Hi Alex

Thanks for the report

On 22/11/2021 08:42, Alexander Veit wrote:
> After an update from Git 2.25.1 to 2.34.0 the Java application that uses 
> the Eclipse JGit library freezes.
> 
> Strace suggests that the JVM receives a SIGTTOU from the child process 
> "git config --system --edit" created with java.lang.ProcessBuilder.
> 
> Upstream[1] has identified 3d411afa[2] as the possible commit that 
> introduced the problem.
> 
> The problem does not occur on all Linux systems.

I think the problem is possibly that git is calling tcsetattr() from a 
background process group[1]. A possible fix would be to call tcgetpgrp() 
after opening /dev/tty to see if git is in the foreground process group.

Best Wishes

Phillip

[1] https://www.man7.org/linux/man-pages/man3/tcsetattr.3p.html

> -Alex
> 
> Environment:
> OS: Linux Mint 20
> Kernel: 5.4.0-90-generic
> Git 2.34.0 from http://ppa.launchpad.net/git-core/ppa/ubuntu
> Java: OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.10+9, mixed mode)
> 
> 
> Ref.:
> [1] https://bugs.eclipse.org/bugs/show_bug.cgi?id=577358
> [2] 
> https://github.com/git/git/commit/3d411afabc9a96f41d47c07d6af6edda3d29ec92#diff-01b59b6a71e42b9c1251ffbf76a1119b965be087a78538e80e01f9239c8e5880 
> 

[PATCH] editor: only save (and restore) the terminal if using a tty

[Carlo Marcelo Arenas Belón]

If the editor is invoked without a controlling terminal, then
saving the state and restoring it later is not very useful and
could generate signals that the invoking process wouldn't know
how to handle.

if git's standard output is not connected to a terminal, then
presume there is no need to worry if the invoking terminal could
garble it.

Reported-by: Alexander Veit <alexander.veit@gmx.net>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 editor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/editor.c b/editor.c
index 674309eed8..214e3834cb 100644
--- a/editor.c
+++ b/editor.c
@@ -86,7 +86,7 @@ static int launch_specified_editor(const char *editor, const char *path,
 		p.env = env;
 		p.use_shell = 1;
 		p.trace2_child_class = "editor";
-		term_fail = save_term(1);
+		term_fail = isatty(1) ? save_term(1) : 1;
 		if (start_command(&p) < 0) {
 			if (!term_fail)
 				restore_term();
-- 
2.34.0.352.g07dee3c5e1

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> If the editor is invoked without a controlling terminal, then
> saving the state and restoring it later is not very useful and
> could generate signals that the invoking process wouldn't know
> how to handle.
>
> if git's standard output is not connected to a terminal, then
> presume there is no need to worry if the invoking terminal could
> garble it.
>
> Reported-by: Alexander Veit <alexander.veit@gmx.net>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  editor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/editor.c b/editor.c
> index 674309eed8..214e3834cb 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -86,7 +86,7 @@ static int launch_specified_editor(const char *editor, const char *path,
>  		p.env = env;
>  		p.use_shell = 1;
>  		p.trace2_child_class = "editor";
> -		term_fail = save_term(1);
> +		term_fail = isatty(1) ? save_term(1) : 1;
>  		if (start_command(&p) < 0) {
>  			if (!term_fail)
>  				restore_term();

I am tempted to say that for 2.34.1 we should revert the offending
commit, and for 2.35 and later we should re-queue the offending
commit with this fix squashed in.

Unless it can be verified that this fix just works for the reporter
in 24 hours, that is.

Thanks.

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> If the editor is invoked without a controlling terminal, then
> saving the state and restoring it later is not very useful and
> could generate signals that the invoking process wouldn't know
> how to handle.
>
> if git's standard output is not connected to a terminal, then
> presume there is no need to worry if the invoking terminal could
> garble it.

Shouldn't the logic apply equally to all callers of save_term()?

In other words, why aren't we doing this check inside save_term()
implementation?  i.e. before opening /dev/tty, we can do isatty(1)
and return -1 if it is false, or something?  That way, when we gain
the second caller to save/restore other than editor (prehaps the
pager code path wants to do this?  I dunno), we do not have to
remember that isatty() check must be made before doing save_term(),
no?

In any case, I am quite tempted to just revert the offending topic
for now, but later accept a resurrection patch with this isatty
check rolled in (either at this caller, or inside save_term) when
the dust settles.

Thanks.


> Reported-by: Alexander Veit <alexander.veit@gmx.net>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  editor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/editor.c b/editor.c
> index 674309eed8..214e3834cb 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -86,7 +86,7 @@ static int launch_specified_editor(const char *editor, const char *path,
>  		p.env = env;
>  		p.use_shell = 1;
>  		p.trace2_child_class = "editor";
> -		term_fail = save_term(1);
> +		term_fail = isatty(1) ? save_term(1) : 1;
>  		if (start_command(&p) < 0) {
>  			if (!term_fail)
>  				restore_term();

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> In any case, I am quite tempted to just revert the offending topic
> for now, but later accept a resurrection patch with this isatty
> check rolled in (either at this caller, or inside save_term) when
> the dust settles.

So, here is that first step, to be hopefully mergeable to 'master'
and also to 'maint' for 2.34.1

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH] Revert "editor: save and reset terminal after calling EDITOR"

This reverts commit 3d411afabc9a96f41d47c07d6af6edda3d29ec92,
blindly opening /dev/tty and calling tcsetattr() seems to be causing
problems.

cf. https://bugs.eclipse.org/bugs/show_bug.cgi?id=577358
cf. https://lore.kernel.org/git/04ab7301-ea34-476c-eae4-4044fef74b91@gmail.com/

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 editor.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/editor.c b/editor.c
index be7441e7e0..6303ae0ab0 100644
--- a/editor.c
+++ b/editor.c
@@ -3,7 +3,6 @@
 #include "strbuf.h"
 #include "run-command.h"
 #include "sigchain.h"
-#include "compat/terminal.h"
 
 #ifndef DEFAULT_EDITOR
 #define DEFAULT_EDITOR "vi"
@@ -51,8 +50,6 @@ const char *git_sequence_editor(void)
 static int launch_specified_editor(const char *editor, const char *path,
 				   struct strbuf *buffer, const char *const *env)
 {
-	int term_fail;
-
 	if (!editor)
 		return error("Terminal is dumb, but EDITOR unset");
 
@@ -86,10 +83,7 @@ static int launch_specified_editor(const char *editor, const char *path,
 		p.env = env;
 		p.use_shell = 1;
 		p.trace2_child_class = "editor";
-		term_fail = save_term(1);
 		if (start_command(&p) < 0) {
-			if (!term_fail)
-				restore_term();
 			strbuf_release(&realpath);
 			return error("unable to start editor '%s'", editor);
 		}
@@ -97,8 +91,6 @@ static int launch_specified_editor(const char *editor, const char *path,
 		sigchain_push(SIGINT, SIG_IGN);
 		sigchain_push(SIGQUIT, SIG_IGN);
 		ret = finish_command(&p);
-		if (!term_fail)
-			restore_term();
 		strbuf_release(&realpath);
 		sig = ret - 128;
 		sigchain_pop(SIGINT);
-- 
2.34.0-222-gceadac38b1

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Alexander Veit]

Am 23.11.21 um 00:08 schrieb Junio C Hamano:
> So, here is that first step, to be hopefully mergeable to 'master'
> and also to 'maint' for 2.34.1

I've applied the patch to 'maint' and it worked. The application does not freeze anymore.

-Alex

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Carlo Arenas]

On Tue, Nov 23, 2021 at 12:52 AM Alexander Veit <list@nezwerg.de> wrote:
>
> Am 23.11.21 um 00:08 schrieb Junio C Hamano:
> > So, here is that first step, to be hopefully mergeable to 'master'
> > and also to 'maint' for 2.34.1
>
> I've applied the patch to 'maint' and it worked. The application does not freeze anymore.

great news; apologies for the disruption.

Carlo

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Carlo Arenas]

On Mon, Nov 22, 2021 at 3:03 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
>
> > If the editor is invoked without a controlling terminal, then
> > saving the state and restoring it later is not very useful and
> > could generate signals that the invoking process wouldn't know
> > how to handle.
> >
> > if git's standard output is not connected to a terminal, then
> > presume there is no need to worry if the invoking terminal could
> > garble it.
>
> Shouldn't the logic apply equally to all callers of save_term()?
>
> In other words, why aren't we doing this check inside save_term()
> implementation?  i.e. before opening /dev/tty, we can do isatty(1)
> and return -1 if it is false, or something?  That way, when we gain
> the second caller to save/restore other than editor (prehaps the
> pager code path wants to do this?  I dunno), we do not have to
> remember that isatty() check must be made before doing save_term(),
> no?

yes, my plan was to minimize the impact of this bugfix by doing this
as narrow as possible, but you are correct that if we consider that
the only caller for  save_term() is in editor.c then it would had make
more sense to put it there to begin with and with supportability in
mind for the future; but save_term() is also called internally, and so
the logic would also propagate to other places that use
compat/terminal.c (like our fallback version of getpass() or `git add
-p`).

I should have mentioned though that a better fix was forthcoming, just
not with so little time before 2.34.1 gets released.

> In any case, I am quite tempted to just revert the offending topic
> for now, but later accept a resurrection patch with this isatty
> check rolled in (either at this caller, or inside save_term) when
> the dust settles.

I indeed suggested[1] a revert but I wouldn't have proposed this
alternative if it wouldn't be done safely enough, agree though that
without a successful report that it fixes the problem by the reporter
(who has since moved[2] on to a different hack which was proposed by
Peff), a revert is always safer.

Carlo

[1] https://lore.kernel.org/git/xmqqilwjbyj4.fsf@gitster.g/T/#mad3fd4d0015ec939c1e0001444d5affd720d56b2
[2] https://git.eclipse.org/r/c/jgit/jgit/+/187938

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Junio C Hamano]

Carlo Arenas <carenas@gmail.com> writes:

> yes, my plan was to minimize the impact of this bugfix by doing this
> as narrow as possible, but you are correct that if we consider that
> ...
> I should have mentioned though that a better fix was forthcoming, just
> not with so little time before 2.34.1 gets released.
> ...
>> In any case, I am quite tempted to just revert the offending topic
>> for now, but later accept a resurrection patch with this isatty
>> check rolled in (either at this caller, or inside save_term) when
>> the dust settles.
>
> I indeed suggested[1] a revert but I wouldn't have proposed this
> alternative if it wouldn't be done safely enough,

I think the minimum impact fix is to revert the whole thing (people
survived without it for long time), so that is what 2.34.1 will
hopefully have.  As I said elsewhere, I am open to a rebooted effort
for the future cycles, but the conclusion for the topic in 2.34 series
is that we pretend we never heard about it ;-)

Thanks.

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Johannes Schindelin]

Hi,

On Tue, 23 Nov 2021, Junio C Hamano wrote:

> Carlo Arenas <carenas@gmail.com> writes:
>
> > yes, my plan was to minimize the impact of this bugfix by doing this
> > as narrow as possible, but you are correct that if we consider that
> > ...
> > I should have mentioned though that a better fix was forthcoming, just
> > not with so little time before 2.34.1 gets released.
> > ...
> >> In any case, I am quite tempted to just revert the offending topic
> >> for now, but later accept a resurrection patch with this isatty
> >> check rolled in (either at this caller, or inside save_term) when
> >> the dust settles.
> >
> > I indeed suggested[1] a revert but I wouldn't have proposed this
> > alternative if it wouldn't be done safely enough,
>
> I think the minimum impact fix is to revert the whole thing (people
> survived without it for long time), so that is what 2.34.1 will
> hopefully have.  As I said elsewhere, I am open to a rebooted effort
> for the future cycles, but the conclusion for the topic in 2.34 series
> is that we pretend we never heard about it ;-)

Maybe a better approach would be to hide the `save_term()` dance behind a
new config option, and then have it turned on automatically if the
`editor` _happens_ to be `vi` or `vim`.

That would help the problem reported in the Windows Terminal project.

Ciao,
Dscho

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Carlo Arenas]

On Wed, Nov 24, 2021 at 5:29 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Maybe a better approach would be to hide the `save_term()` dance behind a
> new config option, and then have it turned on automatically if the
> `editor` _happens_ to be `vi` or `vim`.

that but without turning it on automatically was what I was
preparing[1] (2 commits apply on top of master after the revert) as an
alternative to present for the 2.35 dev cycle.

I agree though that "turning it on automatically" might be worth
adding for the git for windows fork to minimize impact, but even
without it, I would hope the people affected wouldn't mind setting the
config themselves or could even be done for them at install time by
the installer.

Carlo

[1] https://github.com/carenas/git/commits/seen

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Maybe a better approach would be to hide the `save_term()` dance behind a
> new config option, and then have it turned on automatically if the
> `editor` _happens_ to be `vi` or `vim`.

Why 'vi' and 'vim' are so special?  Is this an attempt to paper over
a bug in 'vim' on the caller side?

>
> That would help the problem reported in the Windows Terminal project.
>
> Ciao,
> Dscho

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Carlo Arenas]

On Wed, Nov 24, 2021 at 11:34 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Why 'vi' and 'vim' are so special?  Is this an attempt to paper over
> a bug in 'vim' on the caller side?

not sure if the bug is on vi/vim, cygwin's pty or the new conPTY that
Windows Terminal uses; but in Git for Windows, using an MSYS vim (that
uses cygwin's pty) within Windows Terminal (and other terminals that
use conPTY most likely) leads to this terminal output corruption, that
bash "fixes" when it gets the control back, but that git does not
(unless something like what I am proposing is done), and that is
specially disruptive when doing several commits in a series (ex: an
interactive rebase).

you could say it is an attempt to paper over that bug, but IMHO it is
also a way for git to protect itself from a "rogue" editor, as it is
now just trusting that the editor wouldn't mess with its terminal
settings after being invoked and it returns control to git.

Carlo

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Junio C Hamano]

Carlo Arenas <carenas@gmail.com> writes:

> On Wed, Nov 24, 2021 at 11:34 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Why 'vi' and 'vim' are so special?  Is this an attempt to paper over
>> a bug in 'vim' on the caller side?
>
> not sure if the bug is on vi/vim, cygwin's pty or the new conPTY that
> Windows Terminal uses; but in Git for Windows, using an MSYS vim (that
> uses cygwin's pty) within Windows Terminal (and other terminals that
> use conPTY most likely) leads to this terminal output corruption, that
> bash "fixes" when it gets the control back, but that git does not
> (unless something like what I am proposing is done), and that is
> specially disruptive when doing several commits in a series (ex: an
> interactive rebase).
>
> you could say it is an attempt to paper over that bug, but IMHO it is
> also a way for git to protect itself from a "rogue" editor, as it is
> now just trusting that the editor wouldn't mess with its terminal
> settings after being invoked and it returns control to git.

Surely, bash keeps using the same TTY and it may be in its interest
to "protect" it.  We on the other hand would exit when we are done
and no need for the TTY.  In other words, bash may "own" TTY and it
may want to take a good care of it.  We don't.

I would have to say that your argument is a bit of stretch ;-).

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Johannes Schindelin]

Hi Junio,

On Wed, 24 Nov 2021, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Maybe a better approach would be to hide the `save_term()` dance behind a
> > new config option, and then have it turned on automatically if the
> > `editor` _happens_ to be `vi` or `vim`.
>
> Why 'vi' and 'vim' are so special?

Git's default editor is `vi`. That's what makes it special.

> Is this an attempt to paper over a bug in 'vim' on the caller side?

While this works around a concrete bug reported at
https://github.com/microsoft/terminal/issues/9359, the mere fact that this
bug was possible indicates that Git needs to be able to deal with such
bugs, whether the bug is in the editor, in a POSIX emulation layer, in the
used shell or in the terminal. It is all the more important to have such a
knob because there are so many potential sources for buggy behavior that
users are unlikely to have the expertise to even identify which component
is at fault.

Ciao,
Dscho

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Phillip Wood]

Hi Carlo

On 22/11/2021 22:28, Carlo Marcelo Arenas Belón wrote:
> If the editor is invoked without a controlling terminal, then
> saving the state and restoring it later is not very useful and
> could generate signals that the invoking process wouldn't know
> how to handle.
> 
> if git's standard output is not connected to a terminal, then
> presume there is no need to worry if the invoking terminal could
> garble it.

Checking if stdout is a terminal fixes the Eclipse case where stdout is 
a pipe or /dev/null but if git is started in the background from a 
terminal then calling isatty() will not prevent git from receiving 
SIGTTOU. For example if the user is using a gui editor then the 
following used to work

GIT_EDITOR=gedit git commit&

Now git receives SIGTTOU when the editor exits because we call 
tcsetattr() from a background process group. One can argue it does not 
make much sense to be starting git in the background but it did work 
before these changes. I think a combination of isatty() and tcgetpgrp() 
is probably the best solution.

Best Wishes

Phillip


> Reported-by: Alexander Veit <alexander.veit@gmx.net>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>   editor.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/editor.c b/editor.c
> index 674309eed8..214e3834cb 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -86,7 +86,7 @@ static int launch_specified_editor(const char *editor, const char *path,
>   		p.env = env;
>   		p.use_shell = 1;
>   		p.trace2_child_class = "editor";
> -		term_fail = save_term(1);
> +		term_fail = isatty(1) ? save_term(1) : 1;
>   		if (start_command(&p) < 0) {
>   			if (!term_fail)
>   				restore_term();
> 

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Checking if stdout is a terminal fixes the Eclipse case where stdout
> is a pipe or /dev/null but if git is started in the background from a 
> terminal then calling isatty() will not prevent git from receiving
> SIGTTOU. For example if the user is using a gui editor then the 
> following used to work
>
> GIT_EDITOR=gedit git commit&

It is a good one ;-)

> Now git receives SIGTTOU when the editor exits because we call
> tcsetattr() from a background process group. One can argue it does not 
> make much sense to be starting git in the background but it did work
> before these changes. I think a combination of isatty() and
> tcgetpgrp() is probably the best solution.

If we are not foreground, we can and we should just skip doing this
whole save/restore thing, no?  The "editor might screw up the stty
setting so we restore to help the buggy editor" is releavant only
when the said editor actually uses the terminal, and it would get
TTOU if it tries to do so from the background.

So, I agree it would be a good thing to do, if we still want to do
it.  In the meantime, I've reverted the offending change from 'master'
and am planning to merge it down for 2.34.1 but I do not mind taking
a corrected change (not incremental on top of the broken 2.34.0) for
future releases.

jgit stopped doing 'GIT_EDITOR=echo git config --system --edit' thing,
but not everybody will upgrade immediately.

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Carlo Arenas]

On Tue, Nov 23, 2021 at 3:05 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> I think a combination of isatty() and tcgetpgrp() is probably the best solution.

Definitely agree the long term fix needs to include tcgetpgrp() as
shown by this initial draft[1] (which I apologize, just noticed is
missing your "Helped-by")

That of course introduces a regression on the other direction though;
before this change, git compiled to use our getpass() replacement
(HAVE_DEV_TTY=1) function, the following will be normally stopped by a
SIGTTOU just like getpass() if running in the background (need to also
not have GIT_ASKPASS or SSH_ASKPASS defined in the environment)  :

  $ echo "https://user@example.com/" | git credential fill

I suspect that is probably fine though, as when that happens our
getpass() function still misbehaves if put back in the foreground
(unlike getpass()) and this "feature" might be undesired anyway as the
equivalent C code also runs sometimes in daemon-like processes, and
could even explain some of the workarounds put in place to disable
password prompts (ex: GIT_TERMINAL_PROMPT=0), but luckily we have all
the 2.35 dev cycle to figure out.

Restricting this feature further, maybe through a configuration
property or even special casing the EDITOR is also IMHO a good idea.

Carlo

[1] https://github.com/carenas/git/commit/64d15b2a74206f31e04cf0200f7be83a54a00517

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Phillip Wood]

On 23/11/2021 17:31, Carlo Arenas wrote:
> On Tue, Nov 23, 2021 at 3:05 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> I think a combination of isatty() and tcgetpgrp() is probably the best solution.
> 
> Definitely agree the long term fix needs to include tcgetpgrp() as
> shown by this initial draft[1] (which I apologize, just noticed is
> missing your "Helped-by")
> 
> That of course introduces a regression on the other direction though;
> before this change, git compiled to use our getpass() replacement
> (HAVE_DEV_TTY=1) function, the following will be normally stopped by a
> SIGTTOU just like getpass() if running in the background (need to also
> not have GIT_ASKPASS or SSH_ASKPASS defined in the environment)  :
> 
>    $ echo "https://user@example.com/" | git credential fill
> 
> I suspect that is probably fine though, as when that happens our
> getpass() function still misbehaves if put back in the foreground
> (unlike getpass())

Yes, I tried it out and I couldn't get it to work or figure out why. So 
long as we don't start echoing the password to the screen we should be 
fine. It would be nice to know what the problem is that stops it working 
properly but that is not really related to this patch.

> and this "feature" might be undesired anyway as the
> equivalent C code also runs sometimes in daemon-like processes, and
> could even explain some of the workarounds put in place to disable
> password prompts (ex: GIT_TERMINAL_PROMPT=0), but luckily we have all
> the 2.35 dev cycle to figure out.
> 
> Restricting this feature further, maybe through a configuration
> property or even special casing the EDITOR is also IMHO a good idea.

I think just doing this when we run the editor may be the way to go as I 
think it is only that case that can mess up the terminal.

Best Wishes

Phillip

> Carlo
> 
> [1] https://github.com/carenas/git/commit/64d15b2a74206f31e04cf0200f7be83a54a00517
> 

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Chris Torek]

On Tue, Nov 30, 2021 at 2:41 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 23/11/2021 17:31, Carlo Arenas wrote:
> > Restricting this feature further, maybe through a configuration
> > property or even special casing the EDITOR is also IMHO a good idea.
>
> I think just doing this when we run the editor may be the way to go as I
> think it is only that case that can mess up the terminal.

If it only happens with certain versions of vi / vim, perhaps Git could come
with a front end program that saves the tty state, runs vim, and restores the
tty state. (Or set this up so that the program can run any editor.)  Then add
a FAQ entry if needed: "if your editor goofs up the terminal, insert this front
end program".

Chris

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Junio C Hamano]

Chris Torek <chris.torek@gmail.com> writes:

> On Tue, Nov 30, 2021 at 2:41 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 23/11/2021 17:31, Carlo Arenas wrote:
>> > Restricting this feature further, maybe through a configuration
>> > property or even special casing the EDITOR is also IMHO a good idea.
>>
>> I think just doing this when we run the editor may be the way to go as I
>> think it is only that case that can mess up the terminal.
>
> If it only happens with certain versions of vi / vim, perhaps Git could come
> with a front end program that saves the tty state, runs vim, and restores the
> tty state. (Or set this up so that the program can run any editor.)  Then add
> a FAQ entry if needed: "if your editor goofs up the terminal, insert this front
> end program".

That might work, and because the user is in control, we have less
risk of unintended breakage.  Doing so unconditionally when the
editor's name is "vi" like Dscho suggested would make it more
convenient for the users.

Some editors like Emacs can open a new window and go graphical, even
when they are launched from a terminal.  Doing the save/restore on
them would be unnecessary.  Although I do not offhand think of a way
such an unnecessary save/restore would break the terminal, we have
already seen that things can break in an unintended way by doing
something unnecessary in this area, so perhaps the best way forward
is

 - Add a multi-valued configuration variable whose value is the name
   of an editor program that needs this save/restore; optionally, we
   may want a way to say "don't do save/restore on this editor",
   e.g. "!emacs" may countermand an earlier value that would include
   the editor in the list.

 - Around the program invocation in launch_specified_editor(), check
   the name of the editor against this list and do the save/restore
   as necessary;

 - When the variable is not defined in the configuration, pretend
   that "vi" is on that list (coming up with the list of editors is
   left as an exercise to readers).

That would give us your flexibility to apply the save/restore on an
arbitrary editor that is not "vi", Dscho's convenience to special
case "vi" out of the box when unconfigured, and an escape hatch for
"vi" users for whom it hurts to do the save/restore on their "vi".

Hmm?

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>  - Add a multi-valued configuration variable whose value is the name
>    of an editor program that needs this save/restore; optionally, we
>    may want a way to say "don't do save/restore on this editor",
>    e.g. "!emacs" may countermand an earlier value that would include
>    the editor in the list.
>
>  - Around the program invocation in launch_specified_editor(), check
>    the name of the editor against this list and do the save/restore
>    as necessary;
>
>  - When the variable is not defined in the configuration, pretend
>    that "vi" is on that list (coming up with the list of editors is
>    left as an exercise to readers).
>
> That would give us your flexibility to apply the save/restore on an
> arbitrary editor that is not "vi", Dscho's convenience to special
> case "vi" out of the box when unconfigured, and an escape hatch for
> "vi" users for whom it hurts to do the save/restore on their "vi".
>
> Hmm?

That's an overkill.

A single configuration variable as an escape hatch, that enables the
save/restore around editor invocation, whose default value is
determined by the name of the editor, is probably the right degree
of flexibility.

Something along this line, perhaps?

 editor.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git c/editor.c w/editor.c
index fdd3eeafa9..70d3f80966 100644
--- c/editor.c
+++ w/editor.c
@@ -3,6 +3,7 @@
 #include "strbuf.h"
 #include "run-command.h"
 #include "sigchain.h"
+#include "compat/terminal.h"
 
 #ifndef DEFAULT_EDITOR
 #define DEFAULT_EDITOR "vi"
@@ -47,6 +48,16 @@ const char *git_sequence_editor(void)
 	return editor;
 }
 
+static int prepare_term(const char *editor)
+{
+	int need_saverestore = !strcmp(editor, "vi");
+
+	git_config_get_bool("editor.stty", &need_saverestore);
+	if (need_saverestore)
+		return save_term(1);
+	return 0;
+}
+
 static int launch_specified_editor(const char *editor, const char *path,
 				   struct strbuf *buffer, const char *const *env)
 {
@@ -57,7 +68,7 @@ static int launch_specified_editor(const char *editor, const char *path,
 		struct strbuf realpath = STRBUF_INIT;
 		const char *args[] = { editor, NULL, NULL };
 		struct child_process p = CHILD_PROCESS_INIT;
-		int ret, sig;
+		int ret, sig, need_restore = 0;
 		int print_waiting_for_editor = advice_enabled(ADVICE_WAITING_FOR_EDITOR) && isatty(2);
 
 		if (print_waiting_for_editor) {
@@ -83,7 +94,10 @@ static int launch_specified_editor(const char *editor, const char *path,
 		p.env = env;
 		p.use_shell = 1;
 		p.trace2_child_class = "editor";
+		need_restore = prepare_term(editor);
 		if (start_command(&p) < 0) {
+			if (need_restore)
+				restore_term();
 			strbuf_release(&realpath);
 			return error("unable to start editor '%s'", editor);
 		}
@@ -91,6 +105,8 @@ static int launch_specified_editor(const char *editor, const char *path,
 		sigchain_push(SIGINT, SIG_IGN);
 		sigchain_push(SIGQUIT, SIG_IGN);
 		ret = finish_command(&p);
+		if (need_restore)
+			restore_term();
 		strbuf_release(&realpath);
 		sig = ret - 128;
 		sigchain_pop(SIGINT);

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Carlo Arenas]

On Wed, Dec 1, 2021 at 4:39 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> >  - Add a multi-valued configuration variable whose value is the name
> >    of an editor program that needs this save/restore; optionally, we
> >    may want a way to say "don't do save/restore on this editor",
> >    e.g. "!emacs" may countermand an earlier value that would include
> >    the editor in the list.
> >
> >  - Around the program invocation in launch_specified_editor(), check
> >    the name of the editor against this list and do the save/restore
> >    as necessary;
> >
> >  - When the variable is not defined in the configuration, pretend
> >    that "vi" is on that list (coming up with the list of editors is
> >    left as an exercise to readers).
> >
> > That would give us your flexibility to apply the save/restore on an
> > arbitrary editor that is not "vi", Dscho's convenience to special
> > case "vi" out of the box when unconfigured, and an escape hatch for
> > "vi" users for whom it hurts to do the save/restore on their "vi".
> >
> > Hmm?
>
> That's an overkill.

Ha!, I was going to say that before, but instead went and implemented
(most of) it, discarding my simpler version[1] that was instead using
a boolean config and is otherwise similar to yours (but instead with a
default of false, and obviously not as good looking).

I was assuming the user would know better when the feature was needed
or not (obviously not if their editor is not even terminal based,
which we can't figure out programmatically anyway), and it was on Git
for Windows users that want to also use vi, to enable it[2]; instead
of "affecting" all vi users from all platforms by default, even if we
have reports[3] of some of them being affected before.

A couple of things that are still missing here IMHO are :

* need to also disable if not in the foreground (which was really the
root cause of the reported regression, and needed to address Phillip's
related report[4]; I cover this in my 1/2, so no need to change this
one)
* probably still should do the isatty(2) to disable (at the editor
level), as another way to protect against people scripting around
this, but also because it just doesn't make sense to protect a
terminal from corruption that is not being seen by a human, and as you
pointed out, git doesn't need to be that protective of the tty as bash
does (which will likely fix it itself, or let the user run `reset` to
do so).

Will prepare a new series including your patch for review and a third
one with proposed changes to yours from the second bullet point, that
might be worth squashing instead, or that at least could be used as a
discussion starter, or discarded if you already thought about those
and decided against.

Carlo

[1] https://github.com/carenas/git/commit/910c158b42994132de2480c018b44616962a2a20
[2] https://github.com/git-for-windows/build-extra/pull/399
[3] https://lore.kernel.org/git/xmqqwnmswxs7.fsf@gitster.g/
[4] https://lore.kernel.org/git/b1f2257a-044c-17bb-2737-42b8026421eb@gmail.com/

Re: [PATCH] editor: only save (and restore) the terminal if using a tty

[Johannes Schindelin]

Hi Chris,

On Tue, 30 Nov 2021, Chris Torek wrote:

> On Tue, Nov 30, 2021 at 2:41 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> > On 23/11/2021 17:31, Carlo Arenas wrote:
> > > Restricting this feature further, maybe through a configuration
> > > property or even special casing the EDITOR is also IMHO a good idea.
> >
> > I think just doing this when we run the editor may be the way to go as I
> > think it is only that case that can mess up the terminal.
>
> If it only happens with certain versions of vi / vim, perhaps Git could come
> with a front end program that saves the tty state, runs vim, and restores the
> tty state. (Or set this up so that the program can run any editor.)  Then add
> a FAQ entry if needed: "if your editor goofs up the terminal, insert this front
> end program".

Putting myself into an end-user's shoes, I would much prefer not to be
asked to do extra work and I imagine that the millions of other users who
go with Git's default editor have the very same preference.

Just like we default to `LESS=FRX` for Git's default pager, and just like
we special-case `less` and `vi` in `git grep -O<program>`, it makes sense
to special-case `vi` when saving/restoring the terminal because we know it
helps users.

And by "special-case", I want to say that Git should provide a paved path
to success, not a workaround that users still have to configure manually.

Ciao,
Dscho