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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 23, 2021, 17:27 UTC
Message-ID: <xmqqy25e93zw.fsf@gitster.g>
In-Reply-To: <b1f2257a-044c-17bb-2737-42b8026421eb@gmail.com>

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

Show 7 quoted lines

> 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 ;-)

Show 5 quoted lines

> 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.