refactor: consolidate ControlMaster side-channel invocation - #128
Merged
Conversation
Seven SSHSession methods built the same one-shot ssh argv by hand (tmux_capture, push_tmux_options, upload_file, finalize_upload, remove_remote_tmp, list_dir, download_file) and five of them repeated the same subprocess.run timeout/exception dance. Fold the argv into _mux_argv(), the socket guard into _mux_ready(), and the run+error handling into _mux_run() — so a future ssh option change cannot silently miss one of the call sites. Per-site error strings, timeouts, and I/O settings are preserved byte-for-byte (list_dir keeps its historical bare str(e) message via err_prefix=""). terminate_remote_tmux is deliberately not converted: it re-dials with -p/-l/ConnectTimeout because its master may still be authenticating. No wire-visible change; 486 tests OK.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What
Seven
SSHSessionmethods hand-built the same one-shot ssh argv (tmux_capture,push_tmux_options,upload_file,finalize_upload,remove_remote_tmp,list_dir,download_file), and five of them repeated the samesubprocess.runtimeout/exception handling. This folds:_mux_argv(remote_cmd)_mux_ready()_mux_run(remote_cmd, timeout, timeout_msg, err_prefix)so a future ssh-option change can't silently miss one of the seven sites. Net −19 lines, and the dangerous duplication (argv drift between sites) is gone.
Behavior preserved byte-for-byte
tmux capture timeout/tmux set timeout/finalize timeout/rm timeout/timeout;list_dirkeeps its historical barestr(e)viaerr_prefix=""returncode != 0interpretation stays at the call sites (genuinely differs per operation)upload_file,download_file) keep their exact stdin/stdout/stderr settings and only take the argv helperterminate_remote_tmuxdeliberately NOT converted — it re-dials with-p/-l/ConnectTimeoutbecause its master may still be authenticating (documented in the new section comment)One hardening folded in:
_mux_runreturnsstr(e) or repr(e)so an exception with an empty message can't produce a falsy error string that callers'if err:would miss.Tests
Full suite: 486 tests OK (no test changes needed — argv assertions and error-string expectations all still hold).