Zack/handle login shells #68
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zackw
force-pushed
the
zack/handle-login-shells
branch
from
2eab3a8 to
6c32e5b
Compare
All silver subcommands look up the parent process’s name, and use that, verbatim, to identify the shell in use. This has several problems: - On Windows, the parent process’s name may or may not end with “.exe”; there was special case logic to handle “powershell.exe” versus “powershell” but not “bash.exe” versus “bash”. Relatedly, the parent process’s name ought to be interpreted case- insensitively on Windows; BASH.EXE is the same as bash.exe. - On Unix, there is a notion of a _login_ shell (invoked directly by login(1) for console logins, or sshd(1) for remote logins); these are identified by a leading dash in their process name, *not* by a special option. There was no code to handle this at all (bug reujab#67). - sysinfo is slow, because it reads a whole bunch of files in /proc whether it’s going to need the data within or not. Some but not all of this overhead can be eliminated by using `System::new_with_specifics(RefreshKind::new())` instead of `System::new_all()`, but even then the wall-clock time for `silver init` drops from 0.021s to 0.002s (release build) if sysinfo is not invoked at all. This patch addresses all these problems, as follows: - There is a new command line option `--shell` which may be used to explicitly set the shell in use. Also, support for the `SILVER_SHELL` environment variable (used for this purpose in 1.1 and previous) is restored. sysinfo is only consulted if neither the command line option, nor the environment variable, is available. - No matter where we get the process name from, it is lowercased, a trailing “.exe” is removed, a leading dash is removed, and any leading directory names are also removed (bug reujab#16). - The initialization scripts set `SILVER_SHELL`, so sysinfo will be consulted at most once per session, not once per command. I also reorganized the code in the Command::Init branch of main for readability’s sake, and made the “unknown shell” error message not be a panic. (Panics are for _bugs_, not for errors with a cause outside the program.)
All of the options and subcommands now have one-line descriptions, and the after_help text gives a basic explanation of how to set up silver as well as referring to the wiki.
zackw
force-pushed
the
zack/handle-login-shells
branch
from
6c32e5b to
2d8bf57
Compare
|
I like this PR, but it has a lot of unrelated changes that should be put in a separate PR. |
|
@j0hnmeow I'm happy to split up the PR, but how fine-grained do you want it? One PR for each of the three commits, or do I need to break up c2ff279, and if so, what do you think would be logical boundaries? |
|
For this PR it's enough to have a |
|
OK. It might be a couple days before I get to this, I need to work on
unrelated stuff today.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #67. See commit comments for details of each change.
Note that I disabled the rustfmt part of the pre-commit hook, because I got a flood of errors about options only available with nightly rust, and then it wanted to reformat huge blocks of code that I didn't even touch. I apologize if this means these patches introduce bad formatting.Fixed in the rebased branch.