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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename uppercase shell parameters #4414

Closed
zanchey opened this Issue Sep 18, 2017 · 22 comments

Comments

Projects
None yet
8 participants
@zanchey
Copy link
Member

zanchey commented Sep 18, 2017

馃敟 鈿狅笍 Today's fire danger is: HIGH 鈿狅笍 馃敟
This is an issue which has generated high emotion and personal attacks in the past; please conduct debate respectfully and douse all flames with water.


Before releasing 2.7.0, I'd like to consider renaming the FISH_HISTORY variable to fish_history. There is no released version using FISH_HISTORY, so I think we can do this safely.

I'd also suggest renaming FISH_READ_BYTE_LIMIT to fish_read_byte_limit, either providing both versions in 2.7.0 or renaming it in 3.0.0.

The rationale is the convention that lowercase variables are for shell parameters, whereas uppercase variables are for the environment. Consequently, a variable which changes the behaviour of programs other than fish should be uppercase (think SHELL, LANG), whereas variables that change the behaviour of fish only should be lowercase.

The difference between these two concepts is well-established in bash and zsh (parameters vs environment), csh and tcsh (variables vs environment variables), but the upper/lowercase differentiation is only explicitly specified in fish's documentation.

The argument has been made that FISH_READ_BYTE_LIMIT should remain uppercase, as the intended use case is to set it as an exported variable before launching a new fish instance. I don't support that - it could just as easily be set as a universal variable to affect all instances, and there is some precedent here in the fish_use_fishd and fish_use_posix_spawn feature flags (though neither were ever officially documented).

The documentation could use some clarification on this point.

I'd like to get 2.7.0 out the door soon, and would appreciate your thoughts.

@zanchey zanchey added the RFC label Sep 18, 2017

@zanchey zanchey added this to the fish 2.7.0 milestone Sep 18, 2017

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Sep 19, 2017

This definitely seems like bike shedding and a gratuitous set of changes. I'll also point out that the variables fish_use_fishd and fish_use_posix_spawn were clearly meant for testing potentially buggy new code and not meant to be used by the typical user. Which is why they were never documented. Too, the former no longer exists in the code and the latter should be removed. So it isn't valid to compare them to the other vars being discussed which are explicitly documented and meant to be used as needed for unusual situations.

The all uppercase convention typically has two meanings: exported and read-only. It's also just that: a convention and not a hard and fast rule. I think there has been much misguided concern that all uppercase var names aren't always exported. If that was a hard and fast rule we wouldn't need set --export. Fish would simply export every var that was all uppercase and not export any var that had a lowercase letter. Of course that begs the question of what to do with var names of ambiguous case like set 123 abc.

Having said that I don't really care one way or the other. At the end of the day all that really matters is that the public fish vars are clearly documented. Including whether they can be put in the environment to affect a new fish instance and/or it is reasonable to set them as a universal variable.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Sep 19, 2017

I'd also suggest renaming FISH_READ_BYTE_LIMIT to fish_read_byte_limit, either providing both versions in 2.7.0 or renaming it in 3.0.0.

If you change it in 2.7.0 you definitely have to support both variants. There have been too many 2.x changes, many of which I regret advocating for, that were not backward compatible. Let's not add another (and only) backward incompatible change to the final 2.x release.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Sep 19, 2017

Also, consider that FISH_HISTORY is meant to be exported before starting a new fish instance as part of doing a demo or something similar. Yes, it can be changed on the fly in a running fish instance. But it was expected that the usual pattern would be env FISH_HISTORY=mydemo fish. Hence the all uppercase name to communicate to the user that they would normally export it before starting a new fish instance to ensure that their normal history would not be visible.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Sep 19, 2017

Final comment: It is reasonable to rename FISH_READ_BYTE_LIMIT to lowercase. Primarily because anyone who needs to change it will probably want to do so as a universal variable rather than as an environment variable so that it affects all fish instances on a given machine. Rather than setting it as a env var that affects only downstream fish instances. So a tepid +1 to renaming FISH_READ_BYTE_LIMIT to lowercase but -1 to renaming FISH_HISTORY. Nonetheless, changing FISH_READ_BYTE_LIMIT to lowercase is still a gratuitous change that serves no useful purpose so I don't see the point of renaming it. However, with fish 3.0 being worked on now is the time to change it if the uppercase spelling bothers people.

P.S., That uvars are machine specific is a whole different discussion.

@floam

This comment has been minimized.

Copy link
Member

floam commented Sep 20, 2017

I'd like to see all the proposed renames happen.

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Sep 23, 2017

I agree with this, let's do it.

@floam

This comment has been minimized.

Copy link
Member

floam commented Sep 23, 2017

There's also $CMD_DURATION, not exported.

@floam

This comment has been minimized.

Copy link
Member

floam commented Sep 23, 2017

I'd also prefer $version over $fish_version because the former has always been supported, rather than add a third variant.

zanchey added a commit to zanchey/fish-shell that referenced this issue Sep 24, 2017

zanchey added a commit that referenced this issue Sep 24, 2017

Rename FISH_HISTORY to fish_history
Work on #4414.

(cherry picked and edited from commit
472e186)
@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Sep 25, 2017

I've got to say I'm perplexed by the recently merged commit to change FISH_HISTORY to fish_history. Of all the uppercase var names that is the one which is meant to be explicitly exported by the user (albeit they don't have to). There is a reasonable argument that every other uppercase var I can think of (CMD_DURATION, FISH_VERSION, FISH_READ_BYTE_LIMIT, etc.) should be lowercase. But that is not true of FISH_HISTORY. The primary purpose of that var is to ensure that when doing a demo (or equivalent task) your normal history is not visible. The only safe way to do that, which guards against interactive sub-shells using your normal history, is to export FISH_HISTORY before doing exec fish. Yes, we don't require the user to use the mechanism in that manner and it will work if they change it on the fly without exporting the var. But they do so at their own risk. The all uppercase name is intended to reinforce that it should be exported.

P.S., If you change FISH_READ_BYTE_LIMIT to lowercase I would recommend removing the BYTE text from the name. It is only a byte limit for the read command. For command substitutions it is a (wide) char limit. That the read command treats it as a byte limit is because doing so was much simpler and at the time there didn't seem to be much point in complicating the code to apply the limit to code points (i.e., wide chars). The read code should really be rewritten to treat it as a character limit for consistency with command substitutions and because doing so is a better match for how people think of the limit.

@floam

This comment has been minimized.

Copy link
Member

floam commented Sep 25, 2017

The primary purpose of that var is to ensure that when doing a demo (or equivalent task) your normal history is not visible.

That may be how you use it for your one-offs, but It could just as well be used to implement session-based history like Terminal.app does with bash where you would not export it. I don't see how there's anything about it that explicitly means it should be exported.

If you change FISH_READ_BYTE_LIMIT to lowercase I would recommend removing the BYTE text from the name. It is only a byte limit for the read command.

Makes sense.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Sep 25, 2017

That may be how you use it for your one-offs, but It could just as well be used to implement session-based history like Terminal.app does with bash where you would not export it. I don't see how there's anything about it that explicitly means it should be exported.

So that caused me to search for os x bash_sessions (after first searching for terminal.app bash session history). Which resulted in search results like the links below. OMFG! I'd probably be pulling my hair out had I been using bash when I upgraded to that OS X version. And your argument still doesn't change the fact that in the case of fish the intent was to provide a safe way to do demos without leaking sensitive info. There is nothing stopping a user from using it in the manner you alluded to (i.e., not exporting the var). Notice too that all of the relevant vars in your example have all uppercase names. See, for example, /etc/bashrc_Apple_Terminal which has statements like SHELL_SESSION_HISTFILE="$SHELL_SESSION_DIR/$TERM_SESSION_ID.history". Your argument that only exported vars should be all uppercase is specious.

https://www.reddit.com/r/osx/comments/397uep/changes_to_bash_sessions_and_terminal_in_el/
https://stackoverflow.com/questions/32418438/how-can-i-disable-bash-sessions-in-os-x-el-capitan

@floam

This comment has been minimized.

Copy link
Member

floam commented Sep 25, 2017

Notice too that all of the relevant vars in your example have all uppercase names

I wasn't arguing we should look at Bash for guidance here.

@floam floam closed this in 18b06f3 Oct 14, 2017

floam added a commit that referenced this issue Oct 14, 2017

floam added a commit that referenced this issue Oct 14, 2017

Rename $FISH_VERSION back to the original $version.
Order is restored in the universe. Fixes #4414
@floam

This comment has been minimized.

Copy link
Member

floam commented Oct 14, 2017

It's done. I have become light, bird, wind. My leaves sing. The uppercase vars have been renamed.

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Oct 14, 2017

I didn't expect the $fish_version -> $version change, what's the idea behind that? It seems like this is more likely to collide with ambient environment variables. When was it $version?

@floam

This comment has been minimized.

Copy link
Member

floam commented Oct 14, 2017

When was it $version?

Fish always has supported $version, an electric variable since the 1.x days. This was originally modeled after tcsh. @krader1961 changed the electric variable from $version to $FISH_VERSION last year for 2.4.0. I was pretty vocal at the time about it being a bad change. For the same release, we kept a set -g version $FISH_VERSION in config.fish to ensure compatibility.

Rather than name it $fish_version to merely fix the capitalization, it made more sense to me to (as I mentioned a few comments above) to simply go back to $version instead of adding a third variant into the fray. The reason you might check the version string would be to offer different behavior for different versions of fish, to extend compatibility, so it's not really very useful if it keeps changing.

@floam

This comment has been minimized.

Copy link
Member

floam commented Oct 14, 2017

As it was, before this commit, if $version was set in the environment it was getting overwritten on launch with whatever was in $FISH_VERSION - I think if that happening bothered anybody they would have complained by now.

@mqudsi

This comment has been minimized.

Copy link
Contributor

mqudsi commented Oct 14, 2017

I think the variable scoping rules make it a non-issue, it's almost like it's defined only as a fallback.

floam added a commit that referenced this issue Oct 15, 2017

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Oct 18, 2017

Thanks for the explanation. It's unfortunate that $version was ever a thing, but I understand why we want to avoid yet another variable.

@oschrenk

This comment has been minimized.

Copy link

oschrenk commented Oct 20, 2017

Can you announce that change in the changelog? I updated to master today and it created some minor issues with a fzf key binding relying on $FISH_VERSION, see https://github.com/junegunn/fzf/blob/7b5ccc45bc76f289fe2c48cf91e895b9ca99b1e2/shell/key-bindings.fish#L49 . There might be other scripts out there relying on this.

oschrenk added a commit to oschrenk/fzf that referenced this issue Oct 20, 2017

fish might use $version instead $FISH_VERSION
Starting with fish 2.7 uppercase var are lowercases, see

  fish-shell/fish-shell#4414

or in case of $FISH_VERSION renamed, see also

  fish-shell/fish-shell@fb8ae04

oschrenk added a commit to oschrenk/fzf that referenced this issue Oct 20, 2017

Use $version instead of $FISH_VERSION
$FISH_VERSION is dropped in 2.7, but every version has $version

See also

  fish-shell/fish-shell#4414

and

  fish-shell/fish-shell@fb8ae04
@franciscolourenco

This comment has been minimized.

Copy link

franciscolourenco commented Mar 24, 2018

Which version is the $CMD_DURATION to $cmd_duration rename scheduled for? Are there backward compatibility measures in place?

@faho

This comment has been minimized.

Copy link
Member

faho commented Mar 25, 2018

Which version is the $CMD_DURATION to $cmd_duration rename scheduled for?

3.0 - the next one.

Are there backward compatibility measures in place?

Currently not. We didn't expect it to be used much in scripts and using "$CMD_DURATION$cmd_duration" isn't much to ask.

We might however rethink a bunch of these renames and keep the old names for a while.

@franciscolourenco

This comment has been minimized.

Copy link

franciscolourenco commented Mar 26, 2018

"$CMD_DURATION$cmd_duration" neat trick. It won't work if you keep the old one around..

junegunn added a commit to junegunn/fzf that referenced this issue Jun 27, 2018

[fish] Use $version instead of $FISH_VERSION (#1100)
$FISH_VERSION is dropped in 2.7, but every version has $version

- fish-shell/fish-shell#4414
- fish-shell/fish-shell@fb8ae04

Comment from @faho in #1316:

Unfortunately, $FISH_VERSION was only ever a thing from fish 2.0 to fish 2.7.1.

All fish versions from the very beginning though used a variable called simply "$version" to store their version, so that is the one that should be used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment