Carlo Marcelo Arenas Belón [off-list ref] 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.
quoted hunk
Reported-by: Alexander Veit <redacted>
Signed-off-by: Carlo Marcelo Arenas Belón <redacted>
---
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();
On Mon, Nov 22, 2021 at 3:03 PM Junio C Hamano [off-list ref] wrote:
Carlo Marcelo Arenas Belón [off-list ref] writes:
quoted
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