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

git login shell #1752

Merged
merged 5 commits into from
Jan 9, 2025
Merged

git login shell #1752

merged 5 commits into from
Jan 9, 2025

Conversation

Byron
Copy link
Member

@Byron Byron commented Jan 9, 2025

Support for another standard path which helps executing things.

@Byron Byron force-pushed the git-shell branch 2 times, most recently from b634162 to 6d5f8e8 Compare January 9, 2025 17:27
@Byron Byron marked this pull request as ready for review January 9, 2025 17:27
@Byron Byron enabled auto-merge January 9, 2025 17:27
@Byron Byron force-pushed the git-shell branch 9 times, most recently from dbbfb3e to f951f26 Compare January 9, 2025 19:58
@Byron Byron merged commit 1ca480a into main Jan 9, 2025
20 checks passed
Copy link
Member

@EliahKagan EliahKagan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will work as intended. The login shell is a shell that the user prefers for interactive use. There are no restrictions on its syntax. It need not be POSIX-compatible or Bourne-style. For example, csh/tcsh, fish, xonsh, and occasionally even pwsh or tclsh are used as login shells by some users on Unix-like systems.

Different popular shells have different syntax. Even where the syntax is superficially similar--which is not always the case--it tends to differ in quoting rules, affecting what requires quoting and how quoting must be done. (The quoting rules in csh and tcsh are particularly tricky.)

Furthermore, $SHELL need not conceptually be a shell. While, when set, it is usually some kind of shell--or, for users who do not log on with shell access, /bin/false--even that is not guaranteed.

The only good automated use I am aware of that runs or arranges to run code in the login shell is when an application has startup script logic for login or otherwise interactive shells, or shell integrations such as prompt customizations. Then $SHELL may be used as one source of information about what shells the user may wish to have configured. Most often, this should still be accompanied by interactively checking if the user wants that to be done. More importantly, it has to be done differently for different shells, typically by inferring the shell language from the shell's name.

Using whatever the user has picked as a login shell to run Git hooks would be particularly undesirable. Hopefully hooks have working shebangs, but if they don't and are interpreted as being in a different language, then unexpected effects that are hard to reason about will occur. I think this would also make it infeasible, in many if not most situations, to write hooks securely.

That is not necessarily to say that there is no value in gitoxide having a notion of login shell and making this information available to applications. I've noted a hypothetical use in one of my review comments below, for interactive applications. In addition, part of the goal here seems to be to more often manage to run scripts in the Git for Windows bash.exe. That seems like a good idea, though it looks like the approach taken here to do so is not reliable, as detailed in another review comment.

If the goal is to find a shell that the user may prefer or consider acceptable for running scripts or running configured commands, and if there are particular shells that are known to be reasonable to try for this purpose, then they could be named in a list and looked up via a PATH search. But as far as I know, sh (including other shells that behave as sh when run with the name sh) is the only generally safe and reasonable choice.


One possible motive for finding out the login shell is to actually run it as a login shell to set up a login-like environment. The code here does not seem strongly oriented toward doing that, since in most shells (I can only say most, since $SHELL need not even be a shell at all) it would be necessary to invoke it in a special way to cause it to behave as a login shell. Although some shells recognize -l or --login for this purpose, the way a login shell is actually started in a Unix-like system is by prepending a - to the name that is passed to it as its own name, i.e., to the value of argv[0].

Outside of actual logins or actions similar to logins such as sudo -i, running a login shell to set up a login-like environment is not usually the right thing to do. That this motive exists may even be a reason not to have this functionality in gitoxide: having it may mislead application developers into using it, and in the uncommon case that it should be done, both that it should be done and how it should be done can only be determined by the developer of an application doing it.

The reasons this should not usually be done are:

  • If a login shell has already run, running it again in the same login may do something that should only be done once per login. This applies to most logins, including graphical logins, on many GNU/Linux distributions and on various other Unix-like systems. This oversimplifies, as I believe sometimes graphical logins run or source startup scripts that some login shells use, such as .profile, without actually running a login shell. Also, I believe graphical logins on macOS do not automatically read shell startup scripts.
  • Popular shells, including bash, treat interactive and non-interactive login shells differently, and do not run the same startup scripts in both. Sometimes this is due to inherent behavior of the shell itself, varying across shells. But some startup scripts that often take effect are actually sourced by other startup scripts, based on interactivity, while others are often written to immediately return, when running non-interactively. So some of the variation it is due to default or common code that checks for interactivity in startup scripts--which may ship with, and be customized by, particular distributions--and some may be bespoke user customizations. (This sometimes has unexpected effects.)
  • Running (or, really, sourcing) startup scripts for a login shell can take a significant amount of time, constituting a drain on performance. It is possible to avoid running the startup scripts in some shells--the options for doing so vary across shells, and sometimes environment variables can also be involved--but without the startup scripts, most of the environment customizations that one would wish the shell to do will not be done.
  • Even for startup scripts that run (or, rather, are sourced) when a login shell runs noninteractively, users may have placed commands in them that rely on interactivity, since non-interactive login shells are unintuitive and moderately uncommon. (That post, in addition to describing how login shells are started, summarizes the four combination of login/non-login and interactive/non-interactive shells, including this least common combination.)
  • Users may have configured login shells with an environment that allows access to secrets that are used for authentication. Outside of environments where this is automatically in place with no effort from an application, using this environment this is a double-edged sword, unless the user has explicitly chosen to cause it to be used. It allows useful authentication to occur, but may also allow authentication to take place when the user does not expect or want it, or even might object to it, and in situations where the user would not object to it but that give rise to security problems because the user is unaware that an operation has more access to a resource than they expect.

If it must be done, then I think it is a higher level of abstraction than the other gix-path code. If it is to be done in gix-path (or elsewhere in gitoxide) then I would suggest it be opt-in, not automatically selected as a strategy for gix-command and related functionality, and documented explicitly as only suitable for narrow uses, and rarely outside of interactive applications.

Various applications and libraries make use of $SHELL for purposes where they should not, and it often works with common choices like bash and zsh. But I don't recommend it, it does not typically result in gaining a special environment when one is expected, and I suspect there are many cases where it does the wrong thing and is not reported because the connection between the cause and effect of the problem is unintuitive and poorly discoverable.

VS Code makes some use of login shells and of heuristic detection of what shells are reasonable for various purposes, and does some things that one might be tempted to use a login shell for in other, better ways. So that might be something to look at. But I think that would be more useful as a reference for developing higher level application code, rather than figuring out or refining any login-shell-related functionality of gitoxide itself. (Also, VS Code is mostly doing this to know what shell should run in its own terminal, and to support integration between its terminal and shells.)

@@ -28,6 +28,23 @@ pub fn installation_config_prefix() -> Option<&'static Path> {
installation_config().map(git::config_to_base_path)
}

/// Return the shell that Git would prefer as login shell, the shell to execute Git commands from.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, only Git for Windows has such a preference, in that its Git Bash environment is intended to be a good environment for running git commands from. Even there, the preference is mild, in that the Git for Windows installer defaults to making the git command available in the user's PATH even outside Git Bash. To the best of my knowledge, no such preference exists on the part of the upstream Git project.

This doesn't necessarily mean that a login_shell function is not useful. But it is not clear from this description what it should be used for. (Relatedly, some of the existing and planned uses seem to be things it should not be used for.)

@@ -28,6 +28,23 @@ pub fn installation_config_prefix() -> Option<&'static Path> {
installation_config().map(git::config_to_base_path)
}

/// Return the shell that Git would prefer as login shell, the shell to execute Git commands from.
///
/// On Windows, this is the `bash.exe` bundled with it, and on Unix it's the shell specified by `SHELL`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The antecedent of "it" may be ambiguous for readers who are less familiar with Git for Windows. If the function is kept, I suggest adjusting this to:

Suggested change
/// On Windows, this is the `bash.exe` bundled with it, and on Unix it's the shell specified by `SHELL`,
/// On Windows, this is the `bash.exe` bundled with Git, and on Unix it's the shell specified by `SHELL`,

gix-path/src/env/mod.rs Show resolved Hide resolved
gix-path/src/env/mod.rs Show resolved Hide resolved
tests/tools/src/lib.rs Show resolved Hide resolved
gix-path/tests/path/env.rs Show resolved Hide resolved
@Byron
Copy link
Member Author

Byron commented Jan 11, 2025

Thanks a lot for sharing your incredible insights! I wholeheartedly agree and believe that I have unconditionally ported the shell-selection behaviour of git2-hooks here, while trying to make improvements in how to acquire bash.exe on Windows.

Originally the intent was to just run hooks, but another goal would be to be able to run programs in the same environment as one would have when running in a terminal. Particularly, we need programs like ssh and gpg to be able to pickup the user's configuration.

Interestingly, when checking which environment GUI applications start up, at least on MacOS, then it seemed that they do pick up the environment of the login shell, yet there are reported failures where external programs still don't work as they should. It's a bit puzzling, but also I am currently lacking good reproductions of the issue so I am poking around in the dark. That should probably stop in favor of a more distinctive approach.

If the goal is to find a shell that the user may prefer or consider acceptable for running scripts or running configured commands, and if there are particular shells that are known to be reasonable to try for this purpose, then they could be named in a list and looked up via a PATH search. But as far as I know, sh (including other shells that behave as sh when run with the name sh) is the only generally safe and reasonable choice.

This is the behaviour currently implemented in gix-command, even though on Windows it will bypass the shell if possible by splitting arguments itself to run the program directly.
The reason git2-hooks arrived at this is probably that it also wants to assure that hooks run in the correct environment, while assuming that executable bits and shebangs are set.

One possible motive for finding out the login shell is to actually run it as a login shell to set up a login-like environment. The code here does not seem strongly oriented toward doing that, since in most shells (I can only say most, since $SHELL need not even be a shell at all) it would be necessary to invoke it in a special way to cause it to behave as a login shell. Although some shells recognize -l or --login for this purpose, the way a login shell is actually started in a Unix-like system is by prepending a - to the name that is passed to it as its own name, i.e., to the value of argv[0].

I clearly didn't do my research and rushed to action, but now learned there are differences between interactive shells, interactive login shells, non-interactive login shells, and those that are none of that. I also see that login shell is nothing applications should ever trigger by themselves.

I will keep working on answers to the line comments as well.

#1758 has the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants