| Message ID | 20211122222850.674-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | editor: only save (and restore) the terminal if using a tty | expand |
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.
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();
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);
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
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
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
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(); >
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.
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
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.
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
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
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
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
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 ;-).
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
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 >
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
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?
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);
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/
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
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();
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(-)