Fix #570: Don't wait user input and run git, ssh & svn commands non-interactive #908
Conversation
WalkthroughSwitches subprocess invocations to list-based calls, enforces non-interactive modes for Git and SVN (BatchMode / GIT_TERMINAL_PROMPT / GCM_INTERACTIVE / --non-interactive), fixes stdout/stderr handling in the cmdline utility, and adds tests and feature scenarios for credential/SSH failure cases. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant GitMod as dfetch.vcs.git
participant EnvGen as Env builder
participant Cmd as dfetch.util.cmdline.run_on_cmdline
participant Sub as subprocess
rect rgb(245,250,255)
Note over GitMod,EnvGen: Git non-interactive flow
Caller->>GitMod: request git operation (ls-remote / fetch / checkout)
GitMod->>EnvGen: _extend_env_for_non_interactive_mode()
EnvGen-->>GitMod: env {GIT_TERMINAL_PROMPT=0, GCM_INTERACTIVE=never, GIT_SSH_COMMAND=...BatchMode...}
GitMod->>Cmd: run_on_cmdline(logger, ["git", ...], env)
Cmd->>Sub: subprocess.run(cmd, env=env)
alt success
Sub-->>Cmd: CompletedProcess(stdout, stderr)
Cmd-->>GitMod: return decoded stdout/stderr
else failure
Sub-->>Cmd: CalledProcessError / non-zero exit
Cmd-->>GitMod: raise SubprocessCommandError(stdout, stderr)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
spoorcc
changed the title
spoorcc
force-pushed
the
spoorcc/issue570
branch
from
f865567 to
7c4070e
Compare
spoorcc
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dfetch/util/cmdline.py (1)
28-29: Critical bug:stdoutandstderrare swapped in attribute assignment.The constructor parameters are assigned to the wrong attributes:
self.stderr = stdoutshould beself.stderr = stderrself.stdout = stderrshould beself.stdout = stdoutThis causes callers accessing
exc.stdoutto receive stderr content and vice versa. While error detection code currently checks the swapped attributes, this creates a maintainability hazard and violates the principle of least surprise.🔎 Proposed fix
- self.stderr = stdout - self.stdout = stderr + self.stdout = stdout + self.stderr = stderr
🧹 Nitpick comments (2)
dfetch/vcs/git.py (2)
72-84: Consider the caching implications on environment changes.The
lru_cachedecorator cachesos.environ.copy()from the first invocation. If environment variables change during runtime (e.g., in tests or long-running processes), the cached value won't reflect those changes.For a CLI tool like dfetch, this is unlikely to cause issues in practice, but worth noting for test scenarios.
247-251: Consider removing non-interactive env forgit status.
git statusis a local-only command that doesn't communicate with remotes. The non-interactive environment settings (GIT_TERMINAL_PROMPT, GIT_SSH_COMMAND, GCM_INTERACTIVE) have no effect here, though they also don't cause harm.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.rstdfetch/project/svn.pydfetch/util/cmdline.pydfetch/vcs/git.pyfeatures/check-git-repo.featurefeatures/steps/generic_steps.py
🧰 Additional context used
🧬 Code graph analysis (3)
dfetch/project/svn.py (1)
dfetch/util/cmdline.py (1)
run_on_cmdline(39-68)
dfetch/vcs/git.py (1)
dfetch/util/cmdline.py (2)
run_on_cmdline(39-68)SubprocessCommandError(10-36)
dfetch/util/cmdline.py (2)
dfetch/__main__.py (1)
run(58-74)dfetch/project/svn.py (1)
check(131-144)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build / build (windows-latest)
- GitHub Check: build / build (ubuntu-latest)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (23)
CHANGELOG.rst (1)
27-28: LGTM!Changelog entries are properly formatted and accurately describe the non-interactive mode changes implemented in this PR.
features/steps/generic_steps.py (2)
250-258: LGTM!The new step "the output starts with:" and refactored "the output shows" cleanly delegate to the shared
check_outputhelper, reducing code duplication.
260-294: LGTM!The
check_outputhelper correctly implements line-count-limited comparison for prefix matching. The slicing on line 286 ensures only the first N lines of actual output are compared when testing that output "starts with" expected text.dfetch/util/cmdline.py (1)
39-48: LGTM on the API change.The signature update to
list[str]and optionalenvparameter is well-designed. The callers ingit.pycorrectly useos.environ.copy()before extending, ensuring the full environment is preserved when custom env is passed.dfetch/project/svn.py (10)
51-60: LGTM!The
--non-interactiveflag is correctly added to thesvn propgetcommand, and the command is properly converted to list form.
134-134: LGTM!Non-interactive flag correctly added to
svn infoin thecheck()method.
151-151: LGTM!Non-interactive flag correctly added to
svn infoincheck_path().
175-177: LGTM!Non-interactive flag correctly added to
svn lscommand.
186-186: LGTM!Non-interactive flag added to
svn --versioncommand.
310-312: LGTM!Non-interactive flag correctly added to
svn listin_files_in_path().
329-331: LGTM!Non-interactive flag correctly added to
svn infoin_get_info_from_target().
354-377: LGTM!The
_get_last_changed_revision()method correctly uses list-based commands. Note thatsvnversiondoesn't require--non-interactiveas it's a local-only command that doesn't communicate with servers.
428-431: LGTM!Non-interactive flag correctly added to
svn statusin_untracked_files().
454-457: LGTM!Non-interactive flag correctly added to
svn statusinignored_files().dfetch/vcs/git.py (7)
39-69: LGTM!The
_build_git_ssh_command()function correctly:
- Respects existing
GIT_SSH_COMMANDenvironment variable- Falls back to
git config --get core.sshCommand- Defaults to
sshif neither is set- Appends
-o BatchMode=yesonly if not already configured
100-104: LGTM!The
is_git()method correctly uses the non-interactive environment for thels-remotecheck.
137-141: LGTM!The
get_default_branch()method correctly uses non-interactive environment.
158-162: LGTM!The static
_ls_remote()method correctly applies non-interactive environment.
216-224: LGTM!The non-interactive environment is correctly applied only to the
fetchcommand which communicates with the remote, while local commands (init,remote add,checkout) don't require it.
275-305: LGTM!The
checkout_version()method correctly applies non-interactive environment only tofetch(line 294-298), while local commands (init,remote add,checkout,config,reset,rev-parse) don't require it.
377-377: LGTM!Correctly converted to list-based command invocation.
features/check-git-repo.feature (2)
209-224: LGTM!Good test coverage for the HTTPS credential failure scenario. This verifies that dfetch doesn't hang waiting for user input when authentication is required.
Ensure the
test-repo-privaterepository exists and remains private in the dfetch-org organization for these tests to work correctly.
226-241: LGTM!Good test coverage for the SSH authentication failure scenario. This complements the HTTPS test by verifying non-interactive SSH behavior with
BatchMode=yes.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dfetch/util/cmdline.pydfetch/vcs/git.py
🧰 Additional context used
🧬 Code graph analysis (2)
dfetch/vcs/git.py (1)
dfetch/util/cmdline.py (2)
run_on_cmdline(39-68)SubprocessCommandError(10-36)
dfetch/util/cmdline.py (3)
dfetch/__main__.py (1)
run(58-74)dfetch/project/svn.py (1)
check(131-144)dfetch/project/vcs.py (1)
check(257-258)
🪛 GitHub Actions: Test
dfetch/vcs/git.py
[error] 54-54: AttributeError: 'str' object has no attribute 'stdout' in _build_git_ssh_command(); code expects run_on_cmdline to return an object with 'stdout', but test mocks return a string.
[error] 101-101: StopIteration raised from mocked run_on_cmdline during is_git() call in test_remote_check; test supplies insufficient side_effects for the mocked command sequence.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Publish Python distribution 📦 to TestPyPI
- GitHub Check: build / build (macos-latest)
- GitHub Check: build / build (windows-latest)
- GitHub Check: build / build (ubuntu-latest)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run / run (windows-latest, 3.10)
- GitHub Check: run / run (macos-latest, 3.9)
- GitHub Check: run / run (windows-latest, 3.14)
- GitHub Check: run / run (windows-latest, 3.12)
- GitHub Check: run / run (windows-latest, 3.9)
- GitHub Check: run / run (macos-latest, 3.13)
- GitHub Check: run / run (windows-latest, 3.11)
- GitHub Check: run / run (windows-latest, 3.13)
- GitHub Check: run / run (ubuntu-latest, 3.14)
- GitHub Check: run / test-cygwin
- GitHub Check: DevContainer Build & Test
🔇 Additional comments (8)
dfetch/vcs/git.py (6)
3-3: LGTM!The
functoolsimport is necessary for thelru_cachedecorator used in_extend_env_for_non_interactive_mode().
34-34: LGTM!Conversion to list-based command format improves safety and aligns with the updated
run_on_cmdlinesignature.
138-141: LGTM!List-based command format and non-interactive environment correctly applied to remote operations (
ls-remote).Also applies to: 159-162
217-224: Correct selective application of non-interactive environment.The code correctly applies the non-interactive environment only to the
fetchoperation (line 223), which accesses the remote. Local operations likeinit,remote add, andcheckoutdon't need the environment override.
248-251: LGTM - Correct selective application of environment.Local Git operations (
status,init,remote add,checkout,config,reset,rev-parse,remote get-url) correctly exclude the non-interactive environment since they don't access remote repositories. Only thefetchoperation at line 296 applies the environment, which is the correct design.Also applies to: 275-298, 302-304, 377-377
72-85: The caching approach in_extend_env_for_non_interactive_mode()is sound for a CLI tool and does not present issues in the current test suite. The function is used consistently throughout the Git class to set environment variables for non-interactive git operations, and tests do not modify the relevant environment variables (GIT_TERMINAL_PROMPT,GIT_SSH_COMMAND,GCM_INTERACTIVE).dfetch/util/cmdline.py (2)
6-7: LGTM!Import changes correctly support the updated function signature:
Mappingfor the newenvparameter, andUnionremoved sincecmdis now alwayslist[str].
40-42: LGTM - Improved safety and support for non-interactive mode.The function signature changes improve safety and clarity:
cmd: list[str](no longerUnion[str, list[str]]) prevents shell injection risks from string-based commands- New
envparameter enables non-interactive mode by passing environment variables tosubprocess.run- Direct indexing of
cmd[0]at line 57 is safe sincecmdis guaranteed to be a listNote: This is a backwards-incompatible change—all callers must now pass commands as lists.
Also applies to: 48-48, 57-57
spoorcc
force-pushed
the
spoorcc/issue570
branch
from
4626df2 to
862d350
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
dfetch/util/cmdline.py (1)
28-29: Critical fix confirmed: stdout/stderr correctly assigned.The previous swap bug has been resolved. The assignments now properly store
stdouttoself.stdoutandstderrtoself.stderr.dfetch/vcs/git.py (1)
39-66: Implementation is correct; test coverage remains needed.The function properly:
- Respects existing
GIT_SSH_COMMANDenvironment variable- Falls back to
git config core.sshCommand- Defaults to
"ssh"- Appends
BatchMode=yeswhen not already presentThe past review comment about missing test coverage still applies — comprehensive tests should verify fallback behavior and BatchMode handling.
🧹 Nitpick comments (1)
features/steps/generic_steps.py (1)
260-293: LGTM! Correct implementation of prefix comparison.The function correctly handles both full and partial output comparison:
- When
line_count=None(via[:None]slicing): performs full output comparison- When
line_count=N: compares first N lines of actual with N lines of expectedThe pattern substitutions are consistently applied to both actual and expected outputs before comparison, and edge cases (empty output, single/multiple lines) are handled correctly.
Optional: Add docstring for clarity
While the file's style doesn't emphasize docstrings (per line 3), a brief docstring explaining the
line_countparameter could improve maintainability:def check_output(context, line_count=None): + """Check command output against expected text. + + Args: + context: Behave context with cmd_output and expected text + line_count: If set, compare only the first N lines of actual output + """ expected_text = multisub(
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
dfetch/project/svn.pydfetch/util/cmdline.pydfetch/vcs/git.pyfeatures/steps/generic_steps.pytests/test_git_vcs.py
🚧 Files skipped from review as they are similar to previous changes (1)
- dfetch/project/svn.py
🧰 Additional context used
🧬 Code graph analysis (3)
dfetch/vcs/git.py (1)
dfetch/util/cmdline.py (2)
run_on_cmdline(39-68)SubprocessCommandError(10-36)
dfetch/util/cmdline.py (2)
dfetch/__main__.py (1)
run(58-74)dfetch/project/svn.py (1)
check(131-144)
tests/test_git_vcs.py (1)
dfetch/util/cmdline.py (1)
SubprocessCommandError(10-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build / build (macos-latest)
- GitHub Check: build / build (windows-latest)
- GitHub Check: build / build (ubuntu-latest)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run / run (windows-latest, 3.11)
- GitHub Check: run / run (macos-latest, 3.12)
- GitHub Check: run / run (windows-latest, 3.9)
- GitHub Check: run / run (ubuntu-latest, 3.13)
- GitHub Check: run / run (windows-latest, 3.12)
- GitHub Check: run / run (macos-latest, 3.9)
- GitHub Check: run / run (windows-latest, 3.14)
- GitHub Check: run / run (windows-latest, 3.13)
- GitHub Check: run / run (windows-latest, 3.10)
- GitHub Check: DevContainer Build & Test
- GitHub Check: run / test-cygwin
- GitHub Check: test
🔇 Additional comments (16)
features/steps/generic_steps.py (2)
250-252: LGTM! Useful addition for partial output validation.This new step definition correctly enables validation of output prefixes, which is particularly useful for testing error messages or early warnings without requiring the entire output to match. The logic correctly passes the expected line count to limit the comparison.
255-257: LGTM! Clean refactoring following DRY.The extraction of common output-checking logic into
check_outputis a good refactoring that maintains the existing full-output comparison behavior while enabling code reuse.tests/test_git_vcs.py (3)
6-7: LGTM!The new imports support the updated test expectations (CompletedProcess objects) and environment setup (os.environ).
19-22: Test parameters correctly reflect the updated command execution pattern.The changes properly model:
- Line 19: CompletedProcess return value from
run_on_cmdline- Line 22: Early-return case when URL ends with
.gitThe empty list for "somewhere.git" is correct since
is_git()returnsTruewithout invokingrun_on_cmdlinewhen the URL has a.gitsuffix.
26-27: Good defensive test setup.Setting
GIT_SSH_COMMANDin the environment prevents_build_git_ssh_command()from invokingrun_on_cmdline()for git config, simplifying the mock setup.dfetch/util/cmdline.py (4)
6-7: LGTM!The new imports support the
envparameter's type annotation (Optional[Mapping[str, str]]).
40-42: LGTM: Improved type safety and environment control.The updated signature removes runtime string splitting and adds explicit environment variable support, both of which improve clarity and align with the non-interactive execution objective.
48-48: LGTM: Environment propagation correctly implemented.The
envparameter is properly forwarded tosubprocess.run(), enabling callers to control the subprocess environment for non-interactive execution.
57-57: LGTM: Error message properly uses command name.Using
cmd[0]provides a cleaner error message (e.g., "git not available" instead of "['git', '--version'] not available").dfetch/vcs/git.py (7)
3-3: LGTM!The
functoolsimport supports the@lru_cachedecorator used for optimizing_extend_env_for_non_interactive_mode().
34-34: LGTM: Correctly migrated to list-based command.The command is properly constructed as a list, aligning with the updated
run_on_cmdlinesignature.
69-82: LGTM: Comprehensive non-interactive environment setup with appropriate caching.The function correctly sets:
GIT_TERMINAL_PROMPT=0(disables credential prompts)GIT_SSH_COMMANDwith BatchMode (prevents SSH password prompts)GCM_INTERACTIVE=never(disables Git Credential Manager prompts)The
@lru_cachedecorator is appropriate for a CLI tool where the environment is stable during execution, as noted in the comment.
98-102: LGTM: Non-interactive mode properly applied to remote check.The
ls-remotecommand is correctly constructed as a list and uses the non-interactive environment.
135-139: LGTM: Non-interactive mode properly applied.The command is correctly constructed and uses the non-interactive environment.
156-160: LGTM: Non-interactive mode properly applied.The command is correctly constructed with multiple flags and uses the non-interactive environment.
214-221: LGTM: Selective and appropriate use of non-interactive environment.The code correctly applies non-interactive mode only to remote operations (
fetch,ls-remote) while local operations (init,config,status,reset,rev-parse,remote get-url) execute without modified environment. This is the correct distinction.Also applies to: 245-248, 272-296, 374-374
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
features/steps/generic_steps.py (1)
157-197: Excellent refactoring!The new
check_outputhelper function effectively centralizes output comparison logic, reducing duplication and improving maintainability. The implementation correctly handles both full and partial output comparison through the optionalline_countparameter. The use of[:line_count]on line 188 works correctly even whenline_count=None, as Python slicing treatsNoneas "no limit."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
features/steps/generic_steps.pytests/test_git_vcs.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Publish Python distribution 📦 to TestPyPI
- GitHub Check: build / build (windows-latest)
- GitHub Check: build / build (ubuntu-latest)
- GitHub Check: build / build (macos-latest)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run / run (windows-latest, 3.13)
- GitHub Check: run / run (macos-latest, 3.13)
- GitHub Check: run / run (windows-latest, 3.12)
- GitHub Check: run / run (windows-latest, 3.11)
- GitHub Check: run / run (windows-latest, 3.14)
- GitHub Check: run / run (windows-latest, 3.10)
- GitHub Check: run / run (macos-latest, 3.10)
- GitHub Check: run / run (windows-latest, 3.9)
- GitHub Check: run / test-cygwin
- GitHub Check: DevContainer Build & Test
- GitHub Check: test
🔇 Additional comments (5)
tests/test_git_vcs.py (3)
6-8: LGTM!The new imports appropriately support the updated test expectations and the new SSH command builder tests.
19-19: LGTM!The update to expect
CompletedProcessaligns with the changes to subprocess handling in the git module.
22-22: Short-circuit behavior for.gitURLs is correctly implemented.The implementation of
GitRemote.is_git()correctly returnsTrueimmediately when the remote URL ends with.git, before attempting any subprocess calls. The empty list forcmd_resultin the test case is appropriate and the test will pass as expected, sincerun_on_cmdline()is never invoked for URLs ending in.git.features/steps/generic_steps.py (2)
292-294: LGTM!The new step "the output starts with:" is a valuable addition for scenarios where only the beginning of the output is deterministic or relevant. It correctly delegates to
check_outputwith the appropriate line count.
297-299: LGTM!The refactored step correctly delegates to the new
check_outputhelper, maintaining the original behavior of comparing the entire output while eliminating code duplication.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_git_vcs.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_git_vcs.py (2)
dfetch/util/cmdline.py (1)
SubprocessCommandError(10-36)dfetch/vcs/git.py (1)
_build_git_ssh_command(39-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: Publish Python distribution 📦 to TestPyPI
- GitHub Check: build / build (windows-latest)
- GitHub Check: build / build (ubuntu-latest)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: run / run (macos-latest, 3.14)
- GitHub Check: run / run (windows-latest, 3.10)
- GitHub Check: run / run (windows-latest, 3.14)
- GitHub Check: run / run (windows-latest, 3.13)
- GitHub Check: run / run (windows-latest, 3.11)
- GitHub Check: run / run (ubuntu-latest, 3.14)
- GitHub Check: run / run (windows-latest, 3.12)
- GitHub Check: run / run (ubuntu-latest, 3.10)
- GitHub Check: run / run (macos-latest, 3.9)
- GitHub Check: run / run (windows-latest, 3.9)
- GitHub Check: run / test-cygwin
- GitHub Check: test
- GitHub Check: DevContainer Build & Test
🔇 Additional comments (3)
tests/test_git_vcs.py (3)
6-8: LGTM!The new imports appropriately support the enhanced test coverage for non-interactive Git operations.
19-23: LGTM!The updated test parameters correctly reflect the new
CompletedProcess-based return values. The "somewhere.git" test case with an emptycmd_resultappropriately validates path-based.gitdetection without requiring subprocess invocation.
118-161: Excellent test coverage!The test comprehensively validates the
_build_git_ssh_commandfunction across multiple scenarios:
- Environment variable precedence
- Git config fallback
- Default SSH behavior
- BatchMode deduplication with appropriate debug logging
The mock isolation and assertion logic are well-structured.
#570
Summary by CodeRabbit
New Features
Tests
Bug Fixes
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.