Skip to content
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

Unexported locale variables aren't used #5442

Open
faho opened this issue Dec 29, 2018 · 2 comments
Open

Unexported locale variables aren't used #5442

faho opened this issue Dec 29, 2018 · 2 comments
Labels
bug Something that's not working as intended
Milestone

Comments

@faho
Copy link
Member

faho commented Dec 29, 2018

As discovered on gitter:

$ set -g var ""
$ echo $var
►
$ set -g LANG $LANG
$ echo $var
?
$ set -gx LANG $LANG
$ echo $var

This is because env.cpp:init_locale() has:

    for (const auto &var_name : locale_variables) {
        const auto var = env_get(var_name, ENV_EXPORT);
        const std::string &name = wcs2string(var_name);
        if (var.missing_or_empty()) {
            debug(5, L"locale var %s missing or empty", name.c_str());
            unsetenv(name.c_str());
        } else {
            const std::string value = wcs2string(var->as_string());
            debug(5, L"locale var %s='%s'", name.c_str(), value.c_str());
            setenv(name.c_str(), value.c_str(), 1);
        }
    }

The ENV_EXPORT bit there means that an unexported variable (like one set with set -g) will be unset.

I'm not sure if we should simply remove that ENV_EXPORT, or if that would affect external processes? I think we set the exports up again before executing anything.

@faho faho added the bug Something that's not working as intended label Dec 29, 2018
@faho faho added this to the fish-3.1 milestone Dec 29, 2018
@ridiculousfish
Copy link
Member

So inside init_locale(), if an exported locale variable is set, we call setenv(). This makes the variable visible to various C functions that care about the locale. As an (unwanted) side effect, it also makes it visible to child processes.

If we removed the ENV_EXPORT, what would happen is that fish would call setenv(), and then the system's notion of exported variables would get out of sync with fish's notion. That is, fish's internal data structures would not show an exported locale variable, but there would in fact be one. So this is more complicated than just removing the ENV_EXPORT.

We should work backwards from what the desired behavior is. What happens if you set an unexported locale variable? We would to make it visible to C functions only, not child processes; unfortunately setenv() doesn't allow that.

So we have to choose between either the current behavior, or exporting these variables whenever set. Probably exporting more is OK; I don't think many people want to set a locale without also exporting it.

I think the fix would have to take the form of a notion of "always-exported variables", and we would fix up the flags in env_set_internal(). Then we would assert that the variable is exported in init_locale(). Personally I think it's not worth the trouble, but if someone feels strongly we can do it.

@faho
Copy link
Member Author

faho commented Apr 7, 2019

So inside init_locale(), if an exported locale variable is set, we call setenv(). This makes the variable visible to various C functions that care about the locale.

Isn't that what setlocale does? I'm not quite sure that we actually need to setenv.

@zanchey zanchey modified the milestones: fish 3.1.0, fish-future Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that's not working as intended
Projects
None yet
Development

No branches or pull requests

3 participants