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

From: Carlo Arenas <carenas@gmail.com>
Date: Dec 2, 2021, 01:51 UTC
Message-ID: <CAPUEsphktbdxeV7hvF52Or3CVHS8oOk5-WV=xfEZa8kfCVVnVg@mail.gmail.com>
In-Reply-To: <xmqq7dcnyh5o.fsf@gitster.g>

On Wed, Dec 1, 2021 at 4:39 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 25 quoted lines

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