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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 1, 2021, 19:33 UTC
Message-ID: <xmqq35nc15nr.fsf@gitster.g>
In-Reply-To: <CAPx1GvcML9TvmP1BSLN0vKWD++8LBj-68Xwmz-KrZM32Q=0_Ug@mail.gmail.com>

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

Show 13 quoted lines

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