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

[Junio C Hamano <gitster@pobox.com>]

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?