Infer or set whether to create our own socket #22
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
The semantics of `--reuse-socket` were confusing; `--create-socket` tries to be more straightforward, a true value indicates that the program is going to do something (which seems more intuitive.) This also should open the door for inferring the value as in stairwell-inc#9 if the flag is not specified (probably by making it an enum type rather than an `Option<bool>`.)
has_user_socket is now called infer_create_socket, and has the same polarity as create_socket (it returns true if the program should do something.) It also returns a simple bool, treating any errors as false returns (i.e. "the program shouldn't do anything special.") Also removed the `controlpersist` check since it is not straightforward to decide whether this value is set appropriately; infer_create_socket is a heuristic check, and it's okay for it to be a loose heuristic.
This wound up being a shocking amount of boilerplate and I'm not sure it was worth it since all that it really gets is the ability to explicitly specify the default.
I don't like this. It's too much boilerplate for not enough gain. This reverts commit bbcb254.
|
The commit-by-commit history is best viewed from the CLI; one of the merges has three parents and the GitHub UI presently butchers those. |
Relative to bbcb254, this drops the ability to explicitly specify the default but keeps everything else and has a lot less code.
mrdomino
added a commit
to mrdomino/aspect-reauth
that referenced
this pull request
Mar 15, 2025
…ersion' into infer-socket These commits are not directly related to stairwell-inc#22, but they're small so I'm including them anyway to reduce the number of PRs to review.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
I had thought the former was deprecated; it turns out that it was only its usage without argument (`default_value` rather than `default_value = ...`) that was deprecated.
Matches what aspect-credential-helper will be using on Linux, but continues using Secret Service for persistence.
This was referenced Mar 15, 2025
Closed
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.
This integrates #9 and #20 into one PR.
We add a
--create-socketflag, deprecating--reuse-socketsince the former seems more intuitive. If--create-socketis true, then the program will create and manage its own temporary SSH control socket; if it is false, the program will not. If it is unspecified, then the program will try to parse the output ofssh -Gto decide whether or not to create a socket, specifically looking for acontrolmaster autoline that would imply that the user is already using their own SSH connection multiplexing.This PR also includes a couple other changes:
linux-native-sync-persistentkeyring on Linux, in the hopes that this will work with whataspect-credential-helperwill be using.