New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ncurses when TERM is assigned. #3060

Closed
layus opened this Issue May 24, 2016 · 18 comments

Comments

Projects
None yet
4 participants
@layus

layus commented May 24, 2016

Fish does not reload/reconfigure the underlying ncurses when TERM is assigned like other shells do (bash, zsh, tcsh). It is annoying because TERMINFO may be customized by profile scripts for custom user environments. Fish needs a way to detect changes to TERM* variables and update itself.

Reproduction Steps:

Trick fish into a wrong TERM, then update TERM from within Fish.

In my use-case, terminfo is located in ~, and therefore not available at fish startup (by ssh).

Expected behavior:

Fish should reconfigure curses when the exported terminal is changed, and optionally when other TERMINFO* variables are changed.

Observed behavior:

...

Additional information:


Fish version: 2.2.0

Operating system: NixOS (linux)

Terminal or terminal emulator: rxvt-unicode-256color


  • Release notes updated
@layus

This comment has been minimized.

Show comment
Hide comment
@layus

layus May 24, 2016

/cc @Shados, for he also hit the issue.

layus commented May 24, 2016

/cc @Shados, for he also hit the issue.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho May 24, 2016

Member

We already do this on startup since c5bc221, so you'll want fish 2.3.0.

If you want us to also do it at runtime, is there any particular reason why you are changing these variables then? Usually you'll want to set your terminal to send a proper $TERM value, and if you can't do that you'll want to set it in your config.fish.

Member

faho commented May 24, 2016

We already do this on startup since c5bc221, so you'll want fish 2.3.0.

If you want us to also do it at runtime, is there any particular reason why you are changing these variables then? Usually you'll want to set your terminal to send a proper $TERM value, and if you can't do that you'll want to set it in your config.fish.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho May 24, 2016

Member

I should probably clarify: Since that commit, setting TERM in config.fish works.

Do you need anything more?

Member

faho commented May 24, 2016

I should probably clarify: Since that commit, setting TERM in config.fish works.

Do you need anything more?

@layus

This comment has been minimized.

Show comment
Hide comment
@layus

layus May 24, 2016

That should be enough, if setting TERMINFO and TERMINFO_DIRS in config.fish is also taken into account. I will try with version 2.3.0, and open a new issue if need be.

Thanks for the fast and accurate reply!

layus commented May 24, 2016

That should be enough, if setting TERMINFO and TERMINFO_DIRS in config.fish is also taken into account. I will try with version 2.3.0, and open a new issue if need be.

Thanks for the fast and accurate reply!

@layus layus closed this May 24, 2016

@faho faho added this to the 2.3.0 milestone May 24, 2016

@faho faho added the bug label May 24, 2016

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey May 24, 2016

Member

TERMINFO and TERMINFO_DIRS is entirely up to ncurses, so it should be fine if your terminfo(5) manual page references those variables.

input_init gets called when the terminal goes into interactive mode, so it won't change once the terminal is set up. I'm pretty sure changing the terminal type underneath programs is undefined behaviour :-)

Member

zanchey commented May 24, 2016

TERMINFO and TERMINFO_DIRS is entirely up to ncurses, so it should be fine if your terminfo(5) manual page references those variables.

input_init gets called when the terminal goes into interactive mode, so it won't change once the terminal is set up. I'm pretty sure changing the terminal type underneath programs is undefined behaviour :-)

@layus layus referenced this issue May 24, 2016

Merged

fish: 2.2.0 -> 2.3.0 #15596

6 of 9 tasks complete
@layus

This comment has been minimized.

Show comment
Hide comment
@layus

layus May 24, 2016

@faho, @zanchey Well, I can confirm that setting TERM in config.fish is indeed picked by fish, but setting and/or exporting TERMINFO and TERMINFO_DIRS has no effect on the underlying curses.

My terminfo resides in a custom location in my home. The global system is not aware of it, and should not be. The only way for may login shell to work with my terminal is to receive the TERMINFO{,_DIRS} variables from config.fish. If these variables are not exported to fish own environment, there is no way for curses to get them.
In my case, the best workaround is to (re)exec fish from itself, with the right environment variables.

In zsh, when TERMINFO or TERM are updated, they overwrite the current environment of the zsh process itself, then the term (ncurses) interface is reinitialized [zsh].
In bash the reset callback is called for the three variables [bash].
In tcsh, only TERM triggers a re-initialization of the underlying curses toolset, but it has the setenv that explicitly modifies the environment of the current tcsh process [tcs].

So all these shells have a way to overwrite TERM* variables for the current shell.

If fish does not want to allow live update of these variables, it would still be a good idea to propagate TERM, TERMINFO and TERMINFO_DIRS to the very environment fish is running in (i.e. call setenv under the hood) so that when curses is initialized, it gets the updated values from the user, not the old ones from the system.
This avoid the reset of curses, but still allows some configurability.

I hope I made the issue clear enough. Reopening for it seems valid with these new elements.

[zsh] https://github.com/zsh-users/zsh/blob/master/Src/params.c#L4423-4437
[bash] http://git.savannah.gnu.org/cgit/bash.git/tree/variables.c#n4894-4903
[tcsh] https://github.com/tcsh-org/tcsh/blob/master/sh.set.c#L132-148

layus commented May 24, 2016

@faho, @zanchey Well, I can confirm that setting TERM in config.fish is indeed picked by fish, but setting and/or exporting TERMINFO and TERMINFO_DIRS has no effect on the underlying curses.

My terminfo resides in a custom location in my home. The global system is not aware of it, and should not be. The only way for may login shell to work with my terminal is to receive the TERMINFO{,_DIRS} variables from config.fish. If these variables are not exported to fish own environment, there is no way for curses to get them.
In my case, the best workaround is to (re)exec fish from itself, with the right environment variables.

In zsh, when TERMINFO or TERM are updated, they overwrite the current environment of the zsh process itself, then the term (ncurses) interface is reinitialized [zsh].
In bash the reset callback is called for the three variables [bash].
In tcsh, only TERM triggers a re-initialization of the underlying curses toolset, but it has the setenv that explicitly modifies the environment of the current tcsh process [tcs].

So all these shells have a way to overwrite TERM* variables for the current shell.

If fish does not want to allow live update of these variables, it would still be a good idea to propagate TERM, TERMINFO and TERMINFO_DIRS to the very environment fish is running in (i.e. call setenv under the hood) so that when curses is initialized, it gets the updated values from the user, not the old ones from the system.
This avoid the reset of curses, but still allows some configurability.

I hope I made the issue clear enough. Reopening for it seems valid with these new elements.

[zsh] https://github.com/zsh-users/zsh/blob/master/Src/params.c#L4423-4437
[bash] http://git.savannah.gnu.org/cgit/bash.git/tree/variables.c#n4894-4903
[tcsh] https://github.com/tcsh-org/tcsh/blob/master/sh.set.c#L132-148

@layus layus reopened this May 24, 2016

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 May 24, 2016

Contributor

@layus: Are you saying you are not exporting TERMINFO and TERMINFO_DIRS? If you explicitly do something like this

set -x TERMINFO_DIRS /some/dir
set -x TERMINFO whatever
set -x TERM my_term_name

in your config.fish do you get the expected behavior?

Regardless of the answers to the above I'm inclined to agree with @layus. Fish really should be monitoring those vars and resetting ncurses (or whatever is needed to cause it to notice the environment has changed) even if done interactively. Relabeling this as an enhancement since strictly speaking the current behavior isn't in conflict with the documented behavior.

Contributor

krader1961 commented May 24, 2016

@layus: Are you saying you are not exporting TERMINFO and TERMINFO_DIRS? If you explicitly do something like this

set -x TERMINFO_DIRS /some/dir
set -x TERMINFO whatever
set -x TERM my_term_name

in your config.fish do you get the expected behavior?

Regardless of the answers to the above I'm inclined to agree with @layus. Fish really should be monitoring those vars and resetting ncurses (or whatever is needed to cause it to notice the environment has changed) even if done interactively. Relabeling this as an enhancement since strictly speaking the current behavior isn't in conflict with the documented behavior.

@krader1961 krader1961 modified the milestones: fish-future, 2.3.0 May 24, 2016

@krader1961 krader1961 added enhancement and removed bug labels May 24, 2016

@layus

This comment has been minimized.

Show comment
Hide comment
@layus

layus May 24, 2016

What I am saying is that if I put

echo $TERM - $TERMINFO - $TERMINFO_DIRS
set -x TERMINFO $HOME/.nix-profile/share/terminfo/
set -x TERMINFO_DIRS empty

in my config.fish, I get

rxvt-unicode-256color - - /home/layus/.nix-profile/share/terminfo:/nix/var/nix/profiles/default/share/terminfo:/run/current-system/sw/share/terminfo
ansi - /home/layus/.nix-profile/share/terminfo/ - /home/layus/.nix-profile/share/terminfo:/nix/var/nix/profiles/default/share/terminfo:/run/current-system/sw/share/terminfo

(yes two lines, I guess the file is evaluated twice) in my terminal.
This shows that fish changed the "rxvt-unicode-256color" received from the terminal via ssh to "ansi", which happens when no info has been found for $TERM in the terminfo database.

You can also observe that TERMINFO was properly updated, but TERMINFO_DIRS kept the value from the process environment despite being set exactly the same way (I discovered fish today, excuse my ignorance if this part is obvious).

So TERMINFO receives the right value, but fish ignores it.

layus@sto-lat ~> tree $TERMINFO
/home/layus/.nix-profile/share/terminfo/
└── r
    ├── rxvt-unicode
    └── rxvt-unicode-256color

1 directory, 2 files

I conclude that passing TERMINFO* to fish in the init sequence is too late for fish to take them into account. It' a shame because as a normal user, there is no other location where I could do that.
Fish cannot reasonably expect to run in a fully configured environment. Defining environment variables is the job of the shell.

NB: The value of TERMINFO_DIRS is correctly defined in /etc/fish/config.fish which is sourced before ~/.config/fish/config.fish. This is a system-wide value configured by the system administrator in a global config file, but it is not used by fish either.

layus commented May 24, 2016

What I am saying is that if I put

echo $TERM - $TERMINFO - $TERMINFO_DIRS
set -x TERMINFO $HOME/.nix-profile/share/terminfo/
set -x TERMINFO_DIRS empty

in my config.fish, I get

rxvt-unicode-256color - - /home/layus/.nix-profile/share/terminfo:/nix/var/nix/profiles/default/share/terminfo:/run/current-system/sw/share/terminfo
ansi - /home/layus/.nix-profile/share/terminfo/ - /home/layus/.nix-profile/share/terminfo:/nix/var/nix/profiles/default/share/terminfo:/run/current-system/sw/share/terminfo

(yes two lines, I guess the file is evaluated twice) in my terminal.
This shows that fish changed the "rxvt-unicode-256color" received from the terminal via ssh to "ansi", which happens when no info has been found for $TERM in the terminfo database.

You can also observe that TERMINFO was properly updated, but TERMINFO_DIRS kept the value from the process environment despite being set exactly the same way (I discovered fish today, excuse my ignorance if this part is obvious).

So TERMINFO receives the right value, but fish ignores it.

layus@sto-lat ~> tree $TERMINFO
/home/layus/.nix-profile/share/terminfo/
└── r
    ├── rxvt-unicode
    └── rxvt-unicode-256color

1 directory, 2 files

I conclude that passing TERMINFO* to fish in the init sequence is too late for fish to take them into account. It' a shame because as a normal user, there is no other location where I could do that.
Fish cannot reasonably expect to run in a fully configured environment. Defining environment variables is the job of the shell.

NB: The value of TERMINFO_DIRS is correctly defined in /etc/fish/config.fish which is sourced before ~/.config/fish/config.fish. This is a system-wide value configured by the system administrator in a global config file, but it is not used by fish either.

@layus

This comment has been minimized.

Show comment
Hide comment
@layus

layus May 24, 2016

Here is a patch that fixes the issue (I am quite proud of the XOR operator :-).

Of course, return values are not checked, the style is not perfect, I have no idea if entering multiple times in input_init causes memory leaks or is at all valid and there are algorithmic issues when a valid state is broken by setting some TERM var to an invalid value. This explains why it is not a PR.

But it "Just Works!" ™️. Here comes the beast:

diff --git a/src/env.cpp b/src/env.cpp
index 56c9deb..6b0f982 100644
--- a/src/env.cpp
+++ b/src/env.cpp
@@ -318,6 +318,10 @@ static void react_to_variable_change(const wcstring &key)
     {
         update_wait_on_escape_ms();
     }
+    else if (key == L"TERM" || key == L"TERMINFO" || key == L"TERMINFO_DIRS")
+    {
+        input_init(true);
+    }
 }

 /**
diff --git a/src/input.cpp b/src/input.cpp
index b427772..0ec4de0 100644
--- a/src/input.cpp
+++ b/src/input.cpp
@@ -457,15 +457,31 @@ void update_fish_color_support(void)
     output_set_color_support(support);
 }

-int input_init()
+int input_init(bool update)
 {
-    if (is_init)
+    // A valid use of XOR:
+    // We want to init if we are not yet initialized,
+    // but we want to update only when it has already been initialized.
+    // There is no sense in updating when not initialized.
+    if (is_init ^ update)
         return 1;

     is_init = true;

     input_common_init(&interrupt_handler);

+    const env_var_t terminfo = env_get_string(L"TERMINFO", ENV_GLOBAL | ENV_EXPORT);
+    if (terminfo.missing())
+        unsetenv("TERMINFO");
+    else
+        setenv("TERMINFO", const_cast<char *>(wcs2string(terminfo).c_str()), 1);
+
+    const env_var_t terminfo_dirs = env_get_string(L"TERMINFO_DIRS", ENV_GLOBAL | ENV_EXPORT);
+    if (terminfo_dirs.missing())
+        unsetenv("TERMINFO_DIRS");
+    else
+        setenv("TERMINFO_DIRS", const_cast<char *>(wcs2string(terminfo_dirs).c_str()), 1);
+
     const env_var_t term = env_get_string(L"TERM");
     int errret;
     if (setupterm(const_cast<char *>(wcs2string(term).c_str()), STDOUT_FILENO, &errret) == ERR)
diff --git a/src/input.h b/src/input.h
index 47315c1..b30b0b0 100644
--- a/src/input.h
+++ b/src/input.h
@@ -27,7 +27,7 @@ wcstring describe_char(wint_t c);

    Before calling input_init, terminfo is not initialized and MUST not be used
 */
-int input_init();
+int input_init(bool update = false);

 /**
    free up memory used by terminal functions.

layus commented May 24, 2016

Here is a patch that fixes the issue (I am quite proud of the XOR operator :-).

Of course, return values are not checked, the style is not perfect, I have no idea if entering multiple times in input_init causes memory leaks or is at all valid and there are algorithmic issues when a valid state is broken by setting some TERM var to an invalid value. This explains why it is not a PR.

But it "Just Works!" ™️. Here comes the beast:

diff --git a/src/env.cpp b/src/env.cpp
index 56c9deb..6b0f982 100644
--- a/src/env.cpp
+++ b/src/env.cpp
@@ -318,6 +318,10 @@ static void react_to_variable_change(const wcstring &key)
     {
         update_wait_on_escape_ms();
     }
+    else if (key == L"TERM" || key == L"TERMINFO" || key == L"TERMINFO_DIRS")
+    {
+        input_init(true);
+    }
 }

 /**
diff --git a/src/input.cpp b/src/input.cpp
index b427772..0ec4de0 100644
--- a/src/input.cpp
+++ b/src/input.cpp
@@ -457,15 +457,31 @@ void update_fish_color_support(void)
     output_set_color_support(support);
 }

-int input_init()
+int input_init(bool update)
 {
-    if (is_init)
+    // A valid use of XOR:
+    // We want to init if we are not yet initialized,
+    // but we want to update only when it has already been initialized.
+    // There is no sense in updating when not initialized.
+    if (is_init ^ update)
         return 1;

     is_init = true;

     input_common_init(&interrupt_handler);

+    const env_var_t terminfo = env_get_string(L"TERMINFO", ENV_GLOBAL | ENV_EXPORT);
+    if (terminfo.missing())
+        unsetenv("TERMINFO");
+    else
+        setenv("TERMINFO", const_cast<char *>(wcs2string(terminfo).c_str()), 1);
+
+    const env_var_t terminfo_dirs = env_get_string(L"TERMINFO_DIRS", ENV_GLOBAL | ENV_EXPORT);
+    if (terminfo_dirs.missing())
+        unsetenv("TERMINFO_DIRS");
+    else
+        setenv("TERMINFO_DIRS", const_cast<char *>(wcs2string(terminfo_dirs).c_str()), 1);
+
     const env_var_t term = env_get_string(L"TERM");
     int errret;
     if (setupterm(const_cast<char *>(wcs2string(term).c_str()), STDOUT_FILENO, &errret) == ERR)
diff --git a/src/input.h b/src/input.h
index 47315c1..b30b0b0 100644
--- a/src/input.h
+++ b/src/input.h
@@ -27,7 +27,7 @@ wcstring describe_char(wint_t c);

    Before calling input_init, terminfo is not initialized and MUST not be used
 */
-int input_init();
+int input_init(bool update = false);

 /**
    free up memory used by terminal functions.
@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 May 24, 2016

Contributor

You can also observe that TERMINFO was properly updated, but TERMINFO_DIRS kept the value from the process environment despite being set exactly the same way...

That's because somehow you're starting a second fish instance and the system wide fish init scripts are presumably setting TERMINFO_DIRS but not TERMINFO. So the set -x TERMINFO done by the first fish process hasn't been overridden when the second fish process echoes those vars. It's rather odd that ssh'ing to your system is starting two fish processes. It implies that somewhere in your config.fish initialization you're conditionally starting a second fish process.

Also, set -x TERMINFO_DIRS empty won't actually set it to an empty string. You can set -x TERMINFO_DIRS but it won't be exported because it doesn't have a value. You really want set -e TERMINFO_DIRS in that case.

Thanks for the proof of concept patch. As you noted, someone will need to answer some of the questions you raised before it is merged.

Contributor

krader1961 commented May 24, 2016

You can also observe that TERMINFO was properly updated, but TERMINFO_DIRS kept the value from the process environment despite being set exactly the same way...

That's because somehow you're starting a second fish instance and the system wide fish init scripts are presumably setting TERMINFO_DIRS but not TERMINFO. So the set -x TERMINFO done by the first fish process hasn't been overridden when the second fish process echoes those vars. It's rather odd that ssh'ing to your system is starting two fish processes. It implies that somewhere in your config.fish initialization you're conditionally starting a second fish process.

Also, set -x TERMINFO_DIRS empty won't actually set it to an empty string. You can set -x TERMINFO_DIRS but it won't be exported because it doesn't have a value. You really want set -e TERMINFO_DIRS in that case.

Thanks for the proof of concept patch. As you noted, someone will need to answer some of the questions you raised before it is merged.

@layus

This comment has been minimized.

Show comment
Hide comment
@layus

layus May 25, 2016

😄 I should have used foo istead of empty, that would have been less confusing.

layus commented May 25, 2016

😄 I should have used foo istead of empty, that would have been less confusing.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 May 28, 2016

Contributor

@layus: Can you confirm that your system wide fish init scripts are setting TERMINFO_DIRS? It would help if you confirmed my conclusion in my previous comment. Perhaps the easiest way is to add echo "WTF TERMINFO_DIRS is $TERMINFO_DIRS" >&2 to the top of your config.fish. You could also run find / -xdev -type f | grep TERMINFO_DIRS to find the places which reference that env var.

Contributor

krader1961 commented May 28, 2016

@layus: Can you confirm that your system wide fish init scripts are setting TERMINFO_DIRS? It would help if you confirmed my conclusion in my previous comment. Perhaps the easiest way is to add echo "WTF TERMINFO_DIRS is $TERMINFO_DIRS" >&2 to the top of your config.fish. You could also run find / -xdev -type f | grep TERMINFO_DIRS to find the places which reference that env var.

@layus

This comment has been minimized.

Show comment
Hide comment
@layus

layus May 28, 2016

Yes, your previous post got it right indeed. Printing these variables is exactly what I did.
My config.fish contains only

echo $TERM - $TERMINFO - $TERMINFO_DIRS
set -x TERMINFO $HOME/.nix-profile/share/terminfo/
set -x TERMINFO_DIRS empty

and my output is as explained in #3060 (comment)
The only part that is unclear to me is why two fishes are started instead of just one.

My init sequence is

/nix/store/ngvwj7r6s56c4yqr3wv6byxcfdzsh829-system-path/bin/fish -> /nix/store/l350g4ij5bzhim8m2lmh8ngxfyw6a8mq-fish-2.3.0/bin/fish
+-- source /nix/store/l350g4ij5bzhim8m2lmh8ngxfyw6a8mq-fish-2.3.0/etc/fish/config.fish
    +-- source /etc/fish/config.fish
        +-- load fish-foreign-env plugin (/nix/store/fslr3dc3nr3a684l0a7jr8d3wgcnklr6-fish-foreign-env-git-20151223/share/fish-foreign-env/functions)
        +-- fenv source /nix/store/hx6n42vzdgfdc7c9dxacz0dgmb9j77x0-set-environment
        +-- fenv source /etc/fish/foreign-env/shellInit
        +-- (if login shell) fenv source /etc/fish/foreign-env/loginShellInit
        +-- (if inteercative) fenv souce /etc/fish/foreign-env/interactiveShellInit
+-- source ~/.config/fish/config.fish

These files are included in fish_config.zip

In there, /nix/store/*-set-environment is the one exporting TERMINFO_DIRS, and assigning TERM to itself because this triggers a refresh of the curses data structures in most shell (see comment in the file).

The first indirection before sourcing /etc/fish/config.fish is due to fis sourcing ../etc/fish/config.fish relative to its executable.

layus commented May 28, 2016

Yes, your previous post got it right indeed. Printing these variables is exactly what I did.
My config.fish contains only

echo $TERM - $TERMINFO - $TERMINFO_DIRS
set -x TERMINFO $HOME/.nix-profile/share/terminfo/
set -x TERMINFO_DIRS empty

and my output is as explained in #3060 (comment)
The only part that is unclear to me is why two fishes are started instead of just one.

My init sequence is

/nix/store/ngvwj7r6s56c4yqr3wv6byxcfdzsh829-system-path/bin/fish -> /nix/store/l350g4ij5bzhim8m2lmh8ngxfyw6a8mq-fish-2.3.0/bin/fish
+-- source /nix/store/l350g4ij5bzhim8m2lmh8ngxfyw6a8mq-fish-2.3.0/etc/fish/config.fish
    +-- source /etc/fish/config.fish
        +-- load fish-foreign-env plugin (/nix/store/fslr3dc3nr3a684l0a7jr8d3wgcnklr6-fish-foreign-env-git-20151223/share/fish-foreign-env/functions)
        +-- fenv source /nix/store/hx6n42vzdgfdc7c9dxacz0dgmb9j77x0-set-environment
        +-- fenv source /etc/fish/foreign-env/shellInit
        +-- (if login shell) fenv source /etc/fish/foreign-env/loginShellInit
        +-- (if inteercative) fenv souce /etc/fish/foreign-env/interactiveShellInit
+-- source ~/.config/fish/config.fish

These files are included in fish_config.zip

In there, /nix/store/*-set-environment is the one exporting TERMINFO_DIRS, and assigning TERM to itself because this triggers a refresh of the curses data structures in most shell (see comment in the file).

The first indirection before sourcing /etc/fish/config.fish is due to fis sourcing ../etc/fish/config.fish relative to its executable.

@layus

This comment has been minimized.

Show comment
Hide comment
@layus

layus May 28, 2016

I have submitted a safe PR at #3071. It propagates TERM* variables exported within the shell to fish's process environment (using setenv) right before initializing input. This way, input is never reinitialized, but fish takes tese variables into account if they are defined early on. It is an extension of the logic used for TERM to TERMINFO and TERMINFO_DIRS.

layus commented May 28, 2016

I have submitted a safe PR at #3071. It propagates TERM* variables exported within the shell to fish's process environment (using setenv) right before initializing input. This way, input is never reinitialized, but fish takes tese variables into account if they are defined early on. It is an extension of the logic used for TERM to TERMINFO and TERMINFO_DIRS.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jun 1, 2016

Contributor

My most recent comment to PR #3071:

As I feared fish is exporting vars by passing them to the execve() or posix_spawn() functions. I want to do some spelunking of the history of that code to understand why it is structured that way. This change might be the best way of fixing the issue but I can't help but wonder if it wouldn't be better to dynamically update the process global environ as set commands are executed. That's because there may be other library code that is affected when env vars change. In fact, the locale env vars are already special-cased. See handle_locale() in env.cpp. The terminfo/ncurses vars should be handled in a similar manner.

Contributor

krader1961 commented Jun 1, 2016

My most recent comment to PR #3071:

As I feared fish is exporting vars by passing them to the execve() or posix_spawn() functions. I want to do some spelunking of the history of that code to understand why it is structured that way. This change might be the best way of fixing the issue but I can't help but wonder if it wouldn't be better to dynamically update the process global environ as set commands are executed. That's because there may be other library code that is affected when env vars change. In fact, the locale env vars are already special-cased. See handle_locale() in env.cpp. The terminfo/ncurses vars should be handled in a similar manner.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jun 1, 2016

Contributor

The code for handling exported vars in this manner dates back to 2005-09-23 with commit f971e02. The commit comment just says "Exportable universal variables" and, of course, there's no issue linked to that would explain the rationale for this design. I'm not willing to make a radical change to how exported vars are handled given how many other things need to be done (like switching to UTF-8 internally). The simplest solution is to special-case the curses env vars in react_to_variable_change() like we do the locale env vars.

Contributor

krader1961 commented Jun 1, 2016

The code for handling exported vars in this manner dates back to 2005-09-23 with commit f971e02. The commit comment just says "Exportable universal variables" and, of course, there's no issue linked to that would explain the rationale for this design. I'm not willing to make a radical change to how exported vars are handled given how many other things need to be done (like switching to UTF-8 internally). The simplest solution is to special-case the curses env vars in react_to_variable_change() like we do the locale env vars.

@krader1961 krader1961 self-assigned this Jun 2, 2016

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jun 2, 2016

Contributor

Implementing my own fix for this caused me to notice that input_init() is called repeatedly. Most notably from handling bind commands in the user's fish_user_key_bindings function. Which explains the first two lines in that function to ensure the body is never run more than once. So the initial fix for this issue won't allow redefining the curses/terminfo vars after config.fish has run. Doing so will affect subsequent external commands but not the current fish process. This fix is fragile because a user who runs bind in their config.fish outside of a fish_user_key_bindings() function can cause the curses to be initialized before the terminfo env vars are set.

Contributor

krader1961 commented Jun 2, 2016

Implementing my own fix for this caused me to notice that input_init() is called repeatedly. Most notably from handling bind commands in the user's fish_user_key_bindings function. Which explains the first two lines in that function to ensure the body is never run more than once. So the initial fix for this issue won't allow redefining the curses/terminfo vars after config.fish has run. Doing so will affect subsequent external commands but not the current fish process. This fix is fragile because a user who runs bind in their config.fish outside of a fish_user_key_bindings() function can cause the curses to be initialized before the terminfo env vars are set.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jun 4, 2016

Contributor

Fixed by commit 53e865b.

Contributor

krader1961 commented Jun 4, 2016

Fixed by commit 53e865b.

@krader1961 krader1961 closed this Jun 4, 2016

@faho faho modified the milestones: next-2.x, fish-future Jun 9, 2016

@krader1961 krader1961 modified the milestones: 2.3.1, next-2.x Jun 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment