Respect superproject .gitattributes eol rule when fetching git dependencies#1260
Conversation
WalkthroughDfetch now discovers and applies a superproject's line-ending preferences (via ChangesSuperproject Line-Ending Enforcement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dfetch/project/gitsubproject.py`:
- Line 5: The file imports pathlib twice; resolve the redundancy by keeping one
style and updating usages: either remove the top-level import pathlib and use
the existing from pathlib import Path everywhere (replace any occurrences of
pathlib.Path with Path, e.g., where gitsubproject references pathlib.Path), or
keep import pathlib and remove from pathlib import Path and change any bare Path
uses to pathlib.Path; update the symbol usage around the code that currently
calls pathlib.Path (reference: pathlib.Path and Path in this module) so imports
and usages are consistent.
In `@dfetch/vcs/git.py`:
- Around line 364-369: The _configure_eol method currently writes any string to
.git/info/attributes and sets git core.eol without validation; add input
validation at the start of _configure_eol(eol: str) to only accept "lf" or
"crlf" (case-normalize if desired) and raise a ValueError (with a clear message
referencing the invalid eol) before touching Path(".git/info/attributes") or
calling run_on_cmdline(logger, ["git", "config", "core.eol", eol]) when the
value is not recognized.
In `@features/steps/generic_steps.py`:
- Around line 430-440: The CRLF check in step_impl is too permissive (it only
checks that CRLF appears and no lone CR), so change the CRLF branch to assert
that every newline byte is preceded by a CR: read content as bytes and assert
not re.search(rb'(?<!\r)\n', content) (i.e. no LF that isn't preceded by CR) and
optionally assert b"\r\n" in content to ensure at least one newline exists; keep
the LF branch as-is (assert b"\r" not in content) and raise the same ValueError
for unknown endings.
In `@features/steps/git_steps.py`:
- Around line 162-168: The step_impl test currently strips context.text before
CRLF conversion which can remove the trailing newline and prevent EOL
normalization from being exercised; change step_impl to stop using strip(),
convert all '\n' to '\r\n' from the original context.text, and ensure the
resulting bytes end with a '\r\n' (append one if missing) before calling
pathlib.Path(path).write_bytes(...) and commit_all("Add CRLF file") so the file
always has a trailing CRLF for the EOL conversion test.
In `@features/superproject-line-ending-attrs.feature`:
- Around line 9-26: The scenario "Superproject forces CRLF line endings" is
platform-dependent because the "a git repository" setup step leaves README.md
with implicit system line endings; change the test fixture so the repository
created by the step that yields SomeProject.git includes an explicit README.md
with LF line endings (not relying on the system default) before committing, so
that when the test sets .gitattributes (in MyProject) and runs "dfetch update"
the conversion to CRLF is exercised deterministically; locate the
repository-creation helper used by the "a git repository" step or the test data
for SomeProject.git and add an explicit README.md file with LF-only line endings
to ensure consistent behavior across platforms.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fcf68fd8-5611-4904-95e2-af874b03fdcb
📒 Files selected for processing (8)
CHANGELOG.rstdfetch/project/gitsubproject.pydfetch/vcs/git.pydfetch/vcs/git_types.pydoc/howto/updating-projects.rstfeatures/steps/generic_steps.pyfeatures/steps/git_steps.pyfeatures/superproject-line-ending-attrs.feature
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dfetch/project/gitsubproject.py (1)
81-81: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider documenting the dummy filename suffix.
The
/asuffix appended toself.local_pathqueries the effective EOL for a hypothetical file inside the destination directory (since.gitattributesrules are path-based). While functionally correct, this pattern is not immediately obvious. A brief comment would improve clarity.📝 Suggested documentation
+ # Query effective eol for a file within the destination path + # (gitattributes rules are path-specific, so we append a dummy filename) eol = GitLocalRepo(Path.cwd()).effective_eol(f"{self.local_path}/a")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dfetch/project/gitsubproject.py` at line 81, Add a short inline comment explaining why a dummy file suffix is appended when calling GitLocalRepo.effective_eol — clarify that eol = GitLocalRepo(Path.cwd()).effective_eol(f"{self.local_path}/a") uses "/a" to query a hypothetical file inside the destination directory because .gitattributes rules are path-based; place the comment near the call that references self.local_path and the effective_eol method so future readers understand the intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dfetch/vcs/git.py`:
- Around line 364-381: The binary-detection heuristic in _apply_eol currently
checks only the first 8192 bytes for a NUL byte; add a brief inline comment
above that check explaining this is an intentional, common heuristic (matching
Git’s behavior) to avoid scanning whole files for performance and that it may
misclassify some binaries but is acceptable for this use case. Reference the
_apply_eol method and the "if b\"\\x00\" in raw[:8192]:" check so the maintainer
knows exactly where to add the explanatory comment.
---
Outside diff comments:
In `@dfetch/project/gitsubproject.py`:
- Line 81: Add a short inline comment explaining why a dummy file suffix is
appended when calling GitLocalRepo.effective_eol — clarify that eol =
GitLocalRepo(Path.cwd()).effective_eol(f"{self.local_path}/a") uses "/a" to
query a hypothetical file inside the destination directory because
.gitattributes rules are path-based; place the comment near the call that
references self.local_path and the effective_eol method so future readers
understand the intent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba0cc472-4400-4852-a1bc-65b71088ddb5
📒 Files selected for processing (5)
dfetch/project/gitsubproject.pydfetch/vcs/git.pyfeatures/steps/generic_steps.pyfeatures/steps/git_steps.pyfeatures/superproject-line-ending-attrs.feature
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dfetch/project/svnsubproject.py`:
- Around line 163-167: The helper _apply_superproject_eol currently assumes
self.local_path is a directory and calls
GitLocalRepo.apply_eol_conversion(self.local_path, eol), which fails for
single-file exports supported by _fetch_impl; update _apply_superproject_eol to
detect whether pathlib.Path(self.local_path).is_file() and, for a file target,
call GitLocalRepo.apply_eol_conversion on the file's parent directory (and
ensure the conversion is applied only to that file if needed), otherwise call it
with the directory as before; keep references to _apply_superproject_eol,
_fetch_impl, GitLocalRepo.apply_eol_conversion and the existing effective_eol
call when implementing the branch.
In `@dfetch/vcs/git.py`:
- Around line 375-392: Before running the git round-trip in the block using
run_on_cmdline, snapshot any empty directories (e.g., iterate
Path(".").iterdir() to collect dirs with no files) and preserve that list; after
run_on_cmdline(["git", "checkout", "HEAD", "--", "."]) recreate those empty
directories (and any nested structure) before safe_rm(Path(".git")). Also guard
the commit steps in this sequence (the run_on_cmdline calls around git
add/commit) so a missing tracked-file set does not crash: if there are no files
staged, either create and later remove a temporary placeholder file or skip the
commit and handle the export cleanup accordingly. Apply these changes around the
existing functions/run calls (run_on_cmdline, safe_rm, Path(".git") usage) to
ensure empty SVN directories are restored and the flow safely handles zero-file
exports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1cb66b4c-417c-4b7d-bcf7-893eeddd4c86
📒 Files selected for processing (4)
dfetch/project/svnsubproject.pydfetch/vcs/git.pyfeatures/steps/svn_steps.pyfeatures/superproject-line-ending-attrs.feature
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dfetch/vcs/git.py`:
- Line 379: Before writing to the attributes file at the expression Path(".git")
/ "info" / "attributes" (after the git init call), ensure the parent directory
exists by creating the .git/info directory first; do this by creating the parent
Path (the Path(".git") / "info") and invoking its mkdir with
recursive/exists-safe options before calling write_text, so write_text cannot
raise FileNotFoundError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d83acc47-d148-4825-973d-aec6ec8c8455
📒 Files selected for processing (5)
.gitignoredfetch/project/svnsubproject.pydfetch/vcs/git.pytests/test_git_vcs.pytests/test_svnsubproject.py
|
@coderabbitai resume |
|
✅ Action performedReviews resumed. Review finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
doc/howto/updating-projects.rst (1)
135-141: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a supported code-block lexer here.
gitattributesis already producing the documented Sphinx warning in CI, so this block adds build noise for no gain.textornonewill render the same example without the warning.Patch
-.. code-block:: gitattributes +.. code-block:: text🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doc/howto/updating-projects.rst` around lines 135 - 141, Replace the unsupported Sphinx lexer "gitattributes" in the code-block directive with a supported lexer such as "text" or "none" so the example under the code-block (the lines starting with "* text=auto eol=lf" and "* text=auto eol=crlf") no longer triggers the Sphinx warning; update the code-block declaration in the doc/howto/updating-projects.rst snippet (the code-block directive that currently reads ".. code-block:: gitattributes") to use ".. code-block:: text" or ".. code-block:: none".Source: Pipeline failures
tests/test_subproject.py (1)
141-174: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd a regression test for file-destination EOL hint propagation.
The new
eol_preferences_callbackcontract is not directly asserted here. Add one update-path test where destination is a file path and verify_fetch_impl(..., eol_hint=...)receives the expected hint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_subproject.py` around lines 141 - 174, The test missing assertion for eol_preferences_callback should be extended to cover file-destination EOL hint propagation: set up an eol_preferences_callback (e.g., MagicMock returning a specific hint like "lf") and configure the SubProject instance so its destination is a file path, patch or mock the instance method _fetch_impl on ConcreteSubProject, call subproject.update(..., eol_preferences_callback=your_callback) and assert _fetch_impl was called with eol_hint equal to the callback return; reference the test function test_update_uses_ignored_files_callback_for_stored_hash and the symbols eol_preferences_callback and _fetch_impl to locate where to add the extra setup and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dfetch/project/subproject.py`:
- Around line 176-177: The code only queries the probe path ("<destination>/_")
so file destinations miss exact-path rules; change the call to query both the
exact destination and the probe by computing exact =
pathlib.PurePath(self.local_path).as_posix(), probe = f"{exact}/_" and then call
callback([exact, probe]).get(probe) so the lookup considers the exact file path
as well as the probe path (use the same probe as the .get key).
---
Outside diff comments:
In `@doc/howto/updating-projects.rst`:
- Around line 135-141: Replace the unsupported Sphinx lexer "gitattributes" in
the code-block directive with a supported lexer such as "text" or "none" so the
example under the code-block (the lines starting with "* text=auto eol=lf" and
"* text=auto eol=crlf") no longer triggers the Sphinx warning; update the
code-block declaration in the doc/howto/updating-projects.rst snippet (the
code-block directive that currently reads ".. code-block:: gitattributes") to
use ".. code-block:: text" or ".. code-block:: none".
In `@tests/test_subproject.py`:
- Around line 141-174: The test missing assertion for eol_preferences_callback
should be extended to cover file-destination EOL hint propagation: set up an
eol_preferences_callback (e.g., MagicMock returning a specific hint like "lf")
and configure the SubProject instance so its destination is a file path, patch
or mock the instance method _fetch_impl on ConcreteSubProject, call
subproject.update(..., eol_preferences_callback=your_callback) and assert
_fetch_impl was called with eol_hint equal to the callback return; reference the
test function test_update_uses_ignored_files_callback_for_stored_hash and the
symbols eol_preferences_callback and _fetch_impl to locate where to add the
extra setup and assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c33cb3b1-9ddd-4444-83b2-8f17c5d7b9a6
📒 Files selected for processing (23)
.gitignoreCHANGELOG.rstdfetch/commands/update.pydfetch/commands/update_patch.pydfetch/project/archivesubproject.pydfetch/project/gitsubproject.pydfetch/project/gitsuperproject.pydfetch/project/subproject.pydfetch/project/superproject.pydfetch/project/svnsubproject.pydfetch/project/svnsuperproject.pydfetch/util/cmdline.pydfetch/vcs/git.pydfetch/vcs/svn.pydoc/howto/updating-projects.rstfeatures/steps/generic_steps.pyfeatures/steps/git_steps.pyfeatures/steps/svn_steps.pyfeatures/superproject-line-ending-attrs.featuretests/test_git_vcs.pytests/test_subproject.pytests/test_svn_vcs.pytests/test_update.py
spoorcc
force-pushed
the
claude/superproject-line-ending-attrs-wz5647
branch
from
d287861 to
9fea06f
Compare
…encies
When a superproject has a .gitattributes file with a global eol setting
(e.g. `* text=auto eol=lf` or `* text=auto eol=crlf`), dfetch now uses
`git check-attr` to read the effective eol preference for the destination
path and applies it to the temporary git checkout before `git reset --hard`,
so vendored files always have the line endings the superproject enforces.
- Add `eol` field to `CheckoutOptions` in `git_types.py`
- Add `effective_eol()` (queries superproject via git check-attr) and
`_configure_eol()` (writes .git/info/attributes + core.eol) to
`GitLocalRepo` in `git.py`
- Wire up in `GitSubProject._fetch_impl()` in `gitsubproject.py`
- Add BDD feature test with CRLF-forcing and LF-forcing scenarios
- Add `'<path>' has {ending} line endings` step for binary byte checks
- Add step for committing CRLF content into a test git repo
- Document the feature in doc/howto/updating-projects.rst
https://claude.ai/code/session_01WAGxAB7145LZ2CEvGbAKYu
- Remove duplicate `import pathlib` from gitsubproject.py; use `Path` throughout - Replace pre-checkout git-attributes approach (_configure_eol) with Python-based post-checkout conversion (_apply_eol): walks every text file (NULL-byte binary detection), normalises CRLF→LF then optionally LF→CRLF; this reliably handles CRLF-stored objects where git's own eol=lf smudge filter leaves CRLF unchanged - Add input validation (ValueError) to _apply_eol for unrecognised eol values - Tighten CRLF assertion in generic_steps.py: use (?<!\r)\n lookbehind to ensure every LF is preceded by CR, not just that CRLF appears - Fix CRLF commit step in git_steps.py: use splitlines()+join instead of strip() to preserve trailing CRLF - Add explicit `with LF content` / `with CRLF content` git-repo setup steps so line-ending test fixtures are deterministic across platforms - Restructure both feature scenarios to use the new explicit setup steps, fixing a bug where the CRLF scenario was referencing the wrong git tag https://claude.ai/code/session_01WAGxAB7145LZ2CEvGbAKYu
- _apply_eol: note that NUL-in-first-8KiB is an intentional binary detection heuristic that mirrors git's own approach - _fetch_impl: explain that the "/a" suffix on the effective_eol path query is needed so path-based .gitattributes rules are evaluated against the destination directory https://claude.ai/code/session_01WAGxAB7145LZ2CEvGbAKYu
Move _configure_eol (writes .git/info/attributes + sets core.eol/autocrlf) to run BEFORE git fetch so the smudge filter is active when git checks out files, rather than converting after the fact in Python. Scenario 2 now uses LF-content remote (git-native eol=lf doesn't convert CRLF blobs; that smudge direction is handled by eol=crlf in Scenario 1). https://claude.ai/code/session_01WAGxAB7145LZ2CEvGbAKYu
When the superproject is a git repo whose .gitattributes enforces an eol style, dfetch now also applies that preference to SVN subprojects. Since svn export is a one-shot operation with no built-in line-ending config, the conversion uses git after the export: git init in the destination, configure .git/info/attributes + core.eol, run git-add (normalises text files to LF in the index) + git-checkout-index --all --force (re-emits files with the requested eol), then remove .git/. Adds two BDD scenarios: - Git superproject eol=crlf + SVN subproject with LF content → CRLF - Git superproject eol=lf + SVN subproject with CRLF content → LF https://claude.ai/code/session_01WAGxAB7145LZ2CEvGbAKYu
…out-index git checkout-index --all --force does NOT apply smudge filters (including eol line-ending conversion). Replace with: commit the normalised content, delete all working-tree files, then git checkout HEAD -- . which runs the smudge filter on the way out, writing files with the requested eol. Also add commit.gpgsign=false, user.email, and user.name so the temp commit works in environments with global gpg signing enabled. https://claude.ai/code/session_01WAGxAB7145LZ2CEvGbAKYu
Bug A1: Empty subdirectories were silently lost during the git round-trip in GitLocalRepo.apply_eol_conversion. Add a static helper _collect_empty_dirs() that records all file-less dirs before the round-trip and recreates them afterward. Bug A2: apply_eol_conversion crashed when the export directory contained no files (git commit fails with "nothing to commit"). Add an early-return guard before git init. Bug B1: SvnSubProject._apply_superproject_eol passed a file path directly to apply_eol_conversion when local_path is a single file, causing in_directory() to raise RuntimeError. Now resolve the parent directory when local_path is a file. Includes failing tests added before each fix and verified passing. https://claude.ai/code/session_01WAGxAB7145LZ2CEvGbAKYu
Agent worktrees are ephemeral artefacts that should not be tracked. https://claude.ai/code/session_01WAGxAB7145LZ2CEvGbAKYu
git checkout -- . reads from the index and applies the eol smudge filter without needing a HEAD commit, so the user.email/user.name/commit.gpgsign config and the git commit call are no longer required. Also remove _collect_empty_dirs and the related test since empty-directory preservation is no longer attempted (SVN exports don't produce empty dirs in practice). https://claude.ai/code/session_01WAGxAB7145LZ2CEvGbAKYu
Complete the 4-combination matrix for SVN subprojects: - remote CRLF + eol=crlf → CRLF (content must not be corrupted) - remote LF + eol=lf → LF (content must not be corrupted) The conversion and no-conversion cases now all have explicit coverage. https://claude.ai/code/session_01WAGxAB7145LZ2CEvGbAKYu
Replace 6 near-identical scenarios with two Scenario Outlines: - git subproject: 2 rows (eol=crlf / eol=lf) - SVN subproject: 4 rows covering all remote_eol × eol combinations Update the docs prose to point readers at the Examples tables. https://claude.ai/code/session_01WAGxAB7145LZ2CEvGbAKYu
spoorcc
force-pushed
the
claude/superproject-line-ending-attrs-wz5647
branch
from
9fea06f to
55c6605
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dfetch/project/gitsubproject.py`:
- Around line 76-78: The issue is that
`Path(self.local_path).mkdir(parents=True, exist_ok=True)` creates the
destination path itself as a directory instead of creating just the parent
directory, which breaks file export placement. Change the code to create only
the parent directory by using `Path(self.local_path).parent.mkdir(parents=True,
exist_ok=True)` so that the actual file can be written to the correct
destination path.
In `@dfetch/project/subproject.py`:
- Around line 178-179: The return statement in the callback result handling has
reversed precedence. Currently it checks the probe result first and falls back
to exact, but it should prioritize the exact-path result over the probe
fallback. Swap the order of the two result.get() calls so that result.get(exact)
is evaluated first, with result.get(probe) as the fallback, ensuring that more
specific exact file rules take precedence over generic probe matches when both
exist.
In `@dfetch/util/cmdline.py`:
- Around line 49-50: Remove the `# nosec` inline Bandit suppression comment from
the subprocess.run call that creates the proc variable. The inline waiver
suppresses security review for all callers of this helper function, which
violates the coding guidelines. Instead of relying on an inline suppression,
ensure that the executable and arguments passed to subprocess.run are properly
validated and constrained in the code itself, so the security properties are
evident from the implementation rather than from a waiver.
In `@dfetch/vcs/svn.py`:
- Around line 293-307: The `_inherited_auto_props()` method currently uses a
generic catch-all exception handler that suppresses SSH/auth failures and
transport errors. Refactor the svn propget call to use the `_run_svn_raw()`
helper instead of `run_on_cmdline()`, which will convert SSH host-key errors to
`SshHostKeyError` and allow them to propagate immediately. Replace the broad
exception handler that catches both `SubprocessCommandError` and `RuntimeError`
with a more specific catch that only handles the "path is not a versioned
working-copy" error to continue the ancestor walk; all other errors should be
allowed to raise. Additionally, in the `_match_auto_props_eol_style()` method,
fix the `" - "` substring split logic to properly strip inherited-props block
headers without misparsing fnmatch patterns that contain that literal substring.
- Around line 127-136: The " - " prefix stripping logic in the for loop
iterating over props.splitlines() is being applied to every line, but according
to the function's docstring, the " - " prefix only appears on the first line of
each inherited property block. This causes valid auto-props entries like "foo -
bar.txt = svn:eol-style=LF" to be incorrectly truncated, breaking pattern
matching for filenames containing spaces or hyphens. Refactor the prefix
stripping to only occur when a line marks the start of a new inherited property
block (detect this by checking if the line has the block header structure), and
leave property lines without that prefix unchanged as they are processed in
subsequent iterations.
In `@tests/test_svn_vcs.py`:
- Around line 47-56: The assertion in
test_export_with_native_eol_passes_correct_args expects the SVN command
arguments in the wrong order. The --non-interactive flag should come immediately
after the svn command and before the export subcommand, not after it. Reorder
the expected arguments in the assertion so that the command follows the pattern
["svn", "--non-interactive", "export", ...] instead of the current ["svn",
"export", "--non-interactive", ...] order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7ec480c0-dbfd-4212-a2ce-bc1168fbcffe
📒 Files selected for processing (25)
.gitignore.pre-commit-config.yamlCHANGELOG.rstdfetch/commands/update.pydfetch/commands/update_patch.pydfetch/project/archivesubproject.pydfetch/project/gitsubproject.pydfetch/project/gitsuperproject.pydfetch/project/subproject.pydfetch/project/superproject.pydfetch/project/svnsubproject.pydfetch/project/svnsuperproject.pydfetch/util/cmdline.pydfetch/vcs/git.pydfetch/vcs/git_types.pydfetch/vcs/svn.pydoc/howto/updating-projects.rstfeatures/steps/generic_steps.pyfeatures/steps/git_steps.pyfeatures/steps/svn_steps.pyfeatures/superproject-line-ending-attrs.featuretests/test_git_vcs.pytests/test_subproject.pytests/test_svn_vcs.pytests/test_update.py
| # When exporting a file, the destination directory must already exist | ||
| pathlib.Path(self.local_path).mkdir(parents=True, exist_ok=True) | ||
| Path(self.local_path).mkdir(parents=True, exist_ok=True) | ||
|
|
There was a problem hiding this comment.
Create the parent directory, not the destination path, before file exports.
At Line 77, Path(self.local_path).mkdir(...) creates the destination itself as a directory. For file destinations, that breaks fetch/output placement.
Suggested fix
- Path(self.local_path).mkdir(parents=True, exist_ok=True)
+ Path(self.local_path).parent.mkdir(parents=True, exist_ok=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dfetch/project/gitsubproject.py` around lines 76 - 78, The issue is that
`Path(self.local_path).mkdir(parents=True, exist_ok=True)` creates the
destination path itself as a directory instead of creating just the parent
directory, which breaks file export placement. Change the code to create only
the parent directory by using `Path(self.local_path).parent.mkdir(parents=True,
exist_ok=True)` so that the actual file can be written to the correct
destination path.
| result = callback([exact, probe]) | ||
| return result.get(probe) or result.get(exact) |
There was a problem hiding this comment.
Prefer exact-path EOL over probe fallback.
At Line 179, precedence is reversed. If the callback returns both keys, probe can override a more specific exact file rule, producing the wrong eol_hint.
Suggested fix
- return result.get(probe) or result.get(exact)
+ return result.get(exact) or result.get(probe)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dfetch/project/subproject.py` around lines 178 - 179, The return statement in
the callback result handling has reversed precedence. Currently it checks the
probe result first and falls back to exact, but it should prioritize the
exact-path result over the probe fallback. Swap the order of the two
result.get() calls so that result.get(exact) is evaluated first, with
result.get(probe) as the fallback, ensuring that more specific exact file rules
take precedence over generic probe matches when both exist.
| proc = subprocess.run( # nosec | ||
| cmd, env=env, input=input_data, capture_output=True, check=True |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Remove the inline Bandit suppression from the shared process boundary.
This helper is the single subprocess runner for Git/SVN operations, so keeping # nosec here suppresses security review for every current and future caller. shell=False avoids shell injection, but it does not prove that the executable and arguments are always constrained; that proof should live in code, not in an inline waiver.
As per coding guidelines, dfetch/**/*.py: "Avoid inline lint suppressions (# noqa, # type: ignore, # pylint: disable, # pyright: ignore) without fixing the root cause".
🧰 Tools
🪛 ast-grep (0.43.0)
[error] 48-50: Command coming from incoming request
Context: subprocess.run( # nosec
cmd, env=env, input=input_data, capture_output=True, check=True
)
Note: [CWE-20].
(subprocess-from-request)
[error] 48-50: Use of unsanitized data to create processes
Context: subprocess.run( # nosec
cmd, env=env, input=input_data, capture_output=True, check=True
)
Note: [CWE-78].
(os-system-unsanitized-data)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dfetch/util/cmdline.py` around lines 49 - 50, Remove the `# nosec` inline
Bandit suppression comment from the subprocess.run call that creates the proc
variable. The inline waiver suppresses security review for all callers of this
helper function, which violates the coding guidelines. Instead of relying on an
inline suppression, ensure that the executable and arguments passed to
subprocess.run are properly validated and constrained in the code itself, so the
security properties are evident from the implementation rather than from a
waiver.
Sources: Coding guidelines, Linters/SAST tools
| for line in props.splitlines(): | ||
| if " - " in line: | ||
| line = line.split(" - ", 1)[1] | ||
| pattern, _, values = line.partition("=") | ||
| if not values or not fnmatch.fnmatch(filename, pattern.strip()): | ||
| continue | ||
| for prop in values.split(";"): | ||
| name, _, value = prop.partition("=") | ||
| if name.strip() == "svn:eol-style": | ||
| style = value.strip() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n dfetch/vcs/svn.py | head -160 | tail -50Repository: dfetch-org/dfetch
Length of output: 1895
🌐 Web query:
svn propget svn:auto-props --show-inherited-props format output example
💡 Result:
The --show-inherited-props option for the svn propget command allows you to view properties that a target path inherits from its parent directories in the repository [1][2]. When this option is combined with --verbose (-v), the output explicitly lists which parent directory provided each inherited property [1][3]. Example Output When you run the command against a target that inherits svn:auto-props, the output is structured to group the inherited properties by their source parent [1][4]: $ svn propget svn:auto-props --verbose --show-inherited-props site Inherited properties on 'site', from '.': svn:auto-props *.py = svn:eol-style=native *.c = svn:eol-style=native *.h = svn:eol-style=native Properties on 'site': svn:auto-props *.html = svn:eol-style=native Key Features of the Output Format 1. Inheritance Headers: The output identifies inherited properties with a header like "Inherited properties on 'TARGET'," followed by "from 'PARENT_PATH':" [1][3]. 2. Grouping: If a target inherits properties from multiple parents (e.g., the repository root and a subdirectory), they are listed separately under their respective source paths [3][4]. 3. Explicit Properties: The command also displays any properties explicitly set on the target path itself, often labeled under a header like "Properties on 'TARGET'" [1][3]. 4. Verbosity: Including --verbose (-v) is recommended, as it displays the actual values of the properties rather than just the property names [1][5]. This functionality was introduced in Subversion 1.8 to support repository-dictated configurations, allowing administrators to define properties like svn:auto-props or svn:global-ignores at the repository root or high-level subdirectories and have them automatically apply to all subtrees [2][4].
Citations:
- 1: https://svnbook.red-bean.com/nightly/en/svn.advanced.props.html
- 2: https://subversion.apache.org/blog/2013-06-24-repository-dictated-configuration-part-1-inheritable-properties.html
- 3: https://cwiki.apache.org/confluence/display/SVN/InheritedProperties
- 4: https://subversion.apache.org/blog/2013-06-25-repository-dictated-configuration-part-2-autoprops.html
- 5: https://svnbook.red-bean.com/nightly/en/svn.ref.svn.c.propget.html
🌐 Web query:
svn auto-props inherited-props header format " - " separator example
💡 Result:
In Subversion, the separator between multiple auto-prop assignments inside one auto-props rule is a semicolon (‘;’), not a “ - ”. 1) Runtime config ([auto-props]) format In ~/.subversion/config, each line is: file-name-pattern = PROPNAME=VALUE[;PROPNAME=VALUE...] So the separator between multiple PROPNAME=VALUE pairs is “;” (semicolon-delimited assignments) [1][2]. Example: [auto-props] *.sh = svn:eol-style=native;svn:executable Here “svn:eol-style=native” and “svn:executable” are separated by “;” [1][2]. 2) Inherited (repository) auto-props via svn:auto-props Subversion 1.8+ also has an inheritable reserved property svn:auto-props. Its value uses the same format as the runtime auto-props configuration: newline-separated FILE_PATTERN = PROPNAME=VALUE[;PROPNAME=VALUE...] entries [3][4]. Example (as shown when viewing svn:auto-props on a repository root): svn:auto-props contains lines like: *.sh = svn:eol-style=native;svn:executable Again, the separator between multiple property assignments is “;” [3]. 3) What about “ - ”? The authoritative documented syntax does not mention “ - ” as any separator for auto-props; instead it specifies the semicolon-delimited format for multiple property assignments within a single pattern rule [1][2][4]. 4) Inheritance overview (related to inherited-props) When svn:auto-props is set on a path, it is inheritable to children; explicit svn:auto-props set on a nearer path override inherited/parent auto-props when patterns conflict [3][5].
Citations:
- 1: https://svnbook.red-bean.com/nightly/en/svn.advanced.confarea.html
- 2: https://svn.apache.org/repos/asf/oltu/committers/subversion/config
- 3: https://subversion.apache.org/blog/2013-06-25-repository-dictated-configuration-part-2-autoprops.html
- 4: https://svnbook.red-bean.com/nightly/en/svn.advanced.props.html
- 5: https://cwiki.apache.org/confluence/display/SVN/Inheritable-Ignores-AutoProps
🌐 Web query:
svn propget inherited properties output format path dash inherited-props header
💡 Result:
When using the --show-inherited-props option with the svn propget (or svn pget, svn pg) command, Subversion displays property information for a path and its parent directories [1][2]. The output format for inherited properties follows a specific structure identifying the target path and the parent location from which the property was inherited [3][2][4]: Inherited properties on 'TARGET_PATH', from 'PARENT_PATH_OR_URL': PROPERTY_NAME PROPERTY_VALUE Key characteristics of this format include: 1. Header: Each block of inherited properties begins with a line stating Inherited properties on 'TARGET_PATH', followed by a second line starting with from 'PARENT_PATH_OR_URL': [3][2][4]. 2. Property Details: The property name is displayed indented below the header, followed by its associated value on the subsequent line, also indented [2][4]. 3. Multiple Inheritances: If a target inherits properties from multiple parent directories, Subversion displays separate blocks for each parent, formatted in the same manner as above [1][4]. 4. Verbose Interaction: The --verbose (-v) option is typically used to ensure the property value is printed clearly below the name [1][4]. Without it, or depending on the specific property type, the display may vary [3]. This mechanism allows users to see not only the properties explicitly set on the target path but also those defined in the hierarchy above it [1][2]. Explicit properties on the target are still listed in the output, typically following the inherited property sections, often under a header such as Properties on 'TARGET_PATH': [1][4].
Citations:
- 1: https://cwiki.apache.org/confluence/display/SVN/InheritedProperties
- 2: https://svnbook.red-bean.com/en/1.8/svn.advanced.props.html
- 3: https://www.visualsvn.com/support/svnbook/ref/svn/c/propget/
- 4: https://subversion.apache.org/blog/2013-06-24-repository-dictated-configuration-part-1-inheritable-properties.html
🌐 Web query:
svn propget svn:auto-props --show-inherited-props actual output example format block header
💡 Result:
Example actual output format block headers for: svn propget svn:auto-props --show-inherited-props 1) When showing inherited properties (typical “inherited properties” block) Inherited properties on '', from '': svn:auto-props = =[;...] Example (blog example): Inherited properties on 'calc-trunk-wc\src\bindings.h', from 'http://svn.example.com/repos': svn:auto-props *.c = svn:eol-style=native *.h = svn:eol-style=native *.py = svn:eol-style=native 2) When both inherited and explicit properties are printed (often with --verbose) The output uses multiple blocks, e.g. inherited first, then an explicit properties block: Inherited properties on '', from '': svn:auto-props = =[;...] Properties on '': svn:auto-props = =[;...] Example pattern from the official Apache Subversion examples: Inherited properties on 'calc-trunk-wc\src\bindings.h', from 'http://svn.example.com/repos': svn:auto-props *.c = svn:eol-style=native *.h = svn:eol-style=native *.py = svn:eol-style=native Properties on 'calc-trunk-wc\src\bindings.h': svn:eol-style native (That second block may show the computed/assigned auto-props result via other commands, but the header format for the “Properties on …” block is the same.) 3) When you use a recursive target and only want inherited properties at the repository-root ancestor The header still uses the same phrase “Inherited properties on '', from ''”, and may repeat for different ancestors if multiple parents apply. Reference: Subversion documents that --show-inherited-props makes propget (and proplist) “display inherited properties on a path”, and the Apache examples show the exact header strings “Inherited properties on …, from …” and “Properties on …”. [1][2][3]
Citations:
- 1: https://cwiki.apache.org/confluence/display/SVN/InheritedProperties
- 2: https://svnbook.red-bean.com/nightly/en/svn.ref.svn.c.propget.html
- 3: https://subversion.apache.org/blog/2013-06-25-repository-dictated-configuration-part-2-autoprops.html
🌐 Web query:
svn propget svn:auto-props "from" header "path -" format separator
💡 Result:
The svn:auto-props property is used in Subversion to define automatic property assignments for files added or imported to a directory that inherits the property [1][2]. The required format for the value of the svn:auto-props property follows a specific syntax for key-value pairs, separated by newlines [1][3]. The general structure is [1][4]: FILE_PATTERN = PROPNAME=VALUE[;PROPNAME=VALUE...] Key details regarding this format: 1. FILE_PATTERN: This is a file pattern that matches one or more filenames, which can include wildcards such as * and? [3][4]. 2. Property Assignment: The rest of the line consists of a semicolon-delimited set of property assignments (PROPNAME=VALUE) [3]. 3. Separator: Entries for different file patterns must be separated by newlines [1][3]. 4. Escaping: If a semicolon is required within a property name or value, it can be escaped by doubling it (;;) [3]. When you use svn propget to view the svn:auto-props property, the Subversion client typically displays each pattern-assignment pair on its own line [1][2]. If you need to see the raw, unformatted value (for instance, to confirm the exact structure of separators or avoid default pretty-printing), you should use the --strict option with the svn propget command [5]. Note that unlike the svn:global-ignores property, which combines rules from different levels, the svn:auto-props property behaves such that specific patterns can override previous ones, and explicit properties on a directory override inherited ones [3][2]. Additionally, svn:auto-props is only effective when applied to directories [1].
Citations:
- 1: https://subversion.apache.org/blog/2013-06-25-repository-dictated-configuration-part-2-autoprops.html
- 2: https://svnbook.red-bean.com/en/1.8/svn.advanced.props.html
- 3: https://cwiki.apache.org/confluence/display/SVN/Inheritable-Ignores-AutoProps
- 4: https://svn.apache.org/repos/asf/oltu/committers/subversion/config
- 5: https://svnbook.red-bean.com/nightly/en/svn.ref.svn.c.propget.html
🏁 Script executed:
# Search the dfetch codebase for any documentation or tests showing the props format
rg -A 10 "show-inherited-props" dfetch/Repository: dfetch-org/dfetch
Length of output: 1370
🏁 Script executed:
# Look for tests related to this function
rg -B 5 -A 15 "_match_auto_props_eol_style" dfetch/Repository: dfetch-org/dfetch
Length of output: 2492
🏁 Script executed:
# Find test files for svn module
find dfetch -name "*test*svn*" -o -name "*svn*test*" | head -10Repository: dfetch-org/dfetch
Length of output: 43
🏁 Script executed:
# Find test files more broadly
fd -t f "*test*.py" | grep -i svnRepository: dfetch-org/dfetch
Length of output: 350
🏁 Script executed:
# Find test files more broadly
find . -name "*test*.py" -type f | grep -i svnRepository: dfetch-org/dfetch
Length of output: 135
🏁 Script executed:
# Check the test files for _match_auto_props_eol_style
rg "_match_auto_props_eol_style|props.*=" tests/test_svn.py tests/test_svn_vcs.py -B 2 -A 5Repository: dfetch-org/dfetch
Length of output: 1129
🏁 Script executed:
# Get the test parameters for test_match_auto_props_eol_style
rg -B 50 "def test_match_auto_props_eol_style" tests/test_svn_vcs.py | grep -A 50 "`@pytest.mark.parametrize`"Repository: dfetch-org/dfetch
Length of output: 935
🏁 Script executed:
# Look at more context around that test to see if there are edge cases with hyphens in patterns
cat -n tests/test_svn_vcs.py | sed -n '1,100p'Repository: dfetch-org/dfetch
Length of output: 4263
🏁 Script executed:
# Check if there are any comments or documentation about the " - " format in SVN output
rg "path.*-|inherited.*block" dfetch/vcs/svn.py -B 3 -A 3Repository: dfetch-org/dfetch
Length of output: 2416
🏁 Script executed:
# Check the entire _inherited_auto_props method and how props is passed to _match_auto_props_eol_style
cat -n dfetch/vcs/svn.py | sed -n '140,180p'Repository: dfetch-org/dfetch
Length of output: 1527
Only strip inherited-props prefixes at block starts.
The current " - " split runs on every line. A valid auto-props entry such as foo - bar.txt = svn:eol-style=LF gets truncated before matching, so files with glob patterns containing spaces or hyphens will fail to match. Per the function's docstring, <path> - is a prefix only on the first line of each inherited property block; strip it separately from the actual pattern lines.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dfetch/vcs/svn.py` around lines 127 - 136, The " - " prefix stripping logic
in the for loop iterating over props.splitlines() is being applied to every
line, but according to the function's docstring, the " - " prefix only appears
on the first line of each inherited property block. This causes valid auto-props
entries like "foo - bar.txt = svn:eol-style=LF" to be incorrectly truncated,
breaking pattern matching for filenames containing spaces or hyphens. Refactor
the prefix stripping to only occur when a line marks the start of a new
inherited property block (detect this by checking if the line has the block
header structure), and leave property lines without that prefix unchanged as
they are processed in subsequent iterations.
| try: | ||
| result = run_on_cmdline( | ||
| logger, | ||
| [ | ||
| "svn", | ||
| "propget", | ||
| "svn:auto-props", | ||
| "--show-inherited-props", | ||
| "--non-interactive", | ||
| candidate, | ||
| ], | ||
| ) | ||
| except (SubprocessCommandError, RuntimeError): | ||
| continue | ||
| return str(result.stdout.decode()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /root/repo && find . -name "*.py" -path "*/svn*" | head -20Repository: dfetch-org/dfetch
Length of output: 122
🏁 Script executed:
cd /root/repo && cat -n dfetch/vcs/svn.py | sed -n '290,310p'Repository: dfetch-org/dfetch
Length of output: 122
🏁 Script executed:
cd /root/repo && rg "_inherited_auto_props|eol_preferences" --type py -A 3 -B 3Repository: dfetch-org/dfetch
Length of output: 122
🏁 Script executed:
cd /root/repo && rg "_run_svn" dfetch/vcs/svn.pyRepository: dfetch-org/dfetch
Length of output: 122
🏁 Script executed:
find . -name "svn.py" -type fRepository: dfetch-org/dfetch
Length of output: 81
🏁 Script executed:
cat -n dfetch/vcs/svn.py | sed -n '290,310p'Repository: dfetch-org/dfetch
Length of output: 987
🏁 Script executed:
rg "_inherited_auto_props|eol_preferences" --type py -A 3 -B 3Repository: dfetch-org/dfetch
Length of output: 6919
🏁 Script executed:
rg "_run_svn" dfetch/vcs/svn.pyRepository: dfetch-org/dfetch
Length of output: 1357
🏁 Script executed:
rg "def _run_svn" dfetch/vcs/svn.py -A 20Repository: dfetch-org/dfetch
Length of output: 1538
🏁 Script executed:
rg "def run_on_cmdline" --type py -A 15Repository: dfetch-org/dfetch
Length of output: 975
🏁 Script executed:
cat -n dfetch/vcs/svn.py | sed -n '260,290p'Repository: dfetch-org/dfetch
Length of output: 1546
🏁 Script executed:
cat -n dfetch/project/svnsuperproject.py | sed -n '1,50p'Repository: dfetch-org/dfetch
Length of output: 2122
🏁 Script executed:
rg "eol_style_for" dfetch/vcs/svn.py -B 3 -A 10Repository: dfetch-org/dfetch
Length of output: 608
🏁 Script executed:
rg "_raise_if_ssh_host_key_error" dfetch/vcs/svn.py -B 5 -A 15Repository: dfetch-org/dfetch
Length of output: 1840
🏁 Script executed:
rg "SshHostKeyError" dfetch/vcs/svn.py -B 2 -A 2Repository: dfetch-org/dfetch
Length of output: 2423
🏁 Script executed:
cat -n dfetch/vcs/svn.py | sed -n '1,100p' | grep -E "(import|class|def)" | head -30Repository: dfetch-org/dfetch
Length of output: 1122
🏁 Script executed:
rg "not a versioned" dfetch/vcs/svn.py -B 5 -A 5Repository: dfetch-org/dfetch
Length of output: 43
🏁 Script executed:
cat -n dfetch/vcs/svn.py | sed -n '287,310p'Repository: dfetch-org/dfetch
Length of output: 1195
🏁 Script executed:
rg "_match_auto_props_eol_style" dfetch/vcs/svn.py -B 5 -A 20Repository: dfetch-org/dfetch
Length of output: 2152
🏁 Script executed:
rg "def _match_auto_props_eol_style" dfetch/vcs/svn.py -A 50Repository: dfetch-org/dfetch
Length of output: 1650
🏁 Script executed:
rg "propget.*svn:auto-props" dfetch/vcs/svn.py -B 3 -A 5Repository: dfetch-org/dfetch
Length of output: 468
🏁 Script executed:
rg "eol_preferences" dfetch/commands/ dfetch/project/ --type py -B 2 -A 2Repository: dfetch-org/dfetch
Length of output: 3777
🏁 Script executed:
cat -n dfetch/vcs/svn.py | sed -n '110,130p'Repository: dfetch-org/dfetch
Length of output: 1026
🏁 Script executed:
rg "SubprocessCommandError" dfetch/util/cmdline.py -A 3Repository: dfetch-org/dfetch
Length of output: 654
🏁 Script executed:
rg "not a versioned|E00005|E155007" --type pyRepository: dfetch-org/dfetch
Length of output: 43
🏁 Script executed:
rg "_inherited_auto_props" dfetch/ --type py -B 2 -A 8 | head -60Repository: dfetch-org/dfetch
Length of output: 1046
🏁 Script executed:
cat -n dfetch/vcs/svn.py | sed -n '1,30p'Repository: dfetch-org/dfetch
Length of output: 1113
Route _inherited_auto_props() through _run_svn() helpers; distinguish ancestor-not-versioned errors from SSH/auth failures.
SvnSuperProject.eol_preferences() now calls eol_style_for() for every destination path during update, which in turn invokes _inherited_auto_props() for ancestor discovery. Currently, any svn propget failure (SSH host-key verification, authentication, transport errors) is caught generically and treated as "try the parent directory". This silently suppresses SSH/auth issues and disables EOL enforcement without alerting the user.
Refactor to use _run_svn_raw() so SSH host-key errors are converted to SshHostKeyError and propagate immediately (matching the error handling pattern elsewhere in SvnRepo). Catch only the specific "path is not a versioned working-copy" failure to continue the ancestor walk; all other errors should raise.
Also, the " - " substring split in _match_auto_props_eol_style() (line 129) is intended to strip inherited-props block headers but will misparsе any fnmatch pattern containing that literal substring.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dfetch/vcs/svn.py` around lines 293 - 307, The `_inherited_auto_props()`
method currently uses a generic catch-all exception handler that suppresses
SSH/auth failures and transport errors. Refactor the svn propget call to use the
`_run_svn_raw()` helper instead of `run_on_cmdline()`, which will convert SSH
host-key errors to `SshHostKeyError` and allow them to propagate immediately.
Replace the broad exception handler that catches both `SubprocessCommandError`
and `RuntimeError` with a more specific catch that only handles the "path is not
a versioned working-copy" error to continue the ancestor walk; all other errors
should be allowed to raise. Additionally, in the `_match_auto_props_eol_style()`
method, fix the `" - "` substring split logic to properly strip inherited-props
block headers without misparsing fnmatch patterns that contain that literal
substring.
55c6605 to
155675e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
dfetch/project/gitsubproject.py (1)
76-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCreate the parent directory, not the destination path itself.
At Line 77, creating
self.local_pathas a directory breaks file destinations.Suggested fix
- Path(self.local_path).mkdir(parents=True, exist_ok=True) + Path(self.local_path).parent.mkdir(parents=True, exist_ok=True)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dfetch/project/gitsubproject.py` around lines 76 - 78, The issue is that at line 77 in the mkdir call within the GitSubproject class, the code creates self.local_path itself as a directory, but when exporting a file, self.local_path should be the file destination, not a directory. Instead of calling mkdir on self.local_path, call mkdir on its parent directory using Path(self.local_path).parent.mkdir(parents=True, exist_ok=True) to ensure only the containing directory is created, allowing self.local_path to be used as a file destination.dfetch/project/subproject.py (1)
178-179:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrefer exact-path EOL over probe fallback.
At Line 179,
probeis prioritized overexact, so a directory-level fallback can override a more specific file rule when both keys are present.Suggested fix
- return result.get(probe) or result.get(exact) + return result.get(exact) or result.get(probe)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dfetch/project/subproject.py` around lines 178 - 179, The return statement at line 179 currently prioritizes the probe key over the exact key when retrieving the callback result, allowing directory-level fallback rules to override more specific file rules. Swap the order of the .get() calls so that exact is checked first with probe as the fallback, ensuring that the more specific file-level rule takes precedence over the directory-level rule when both keys are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@dfetch/project/gitsubproject.py`:
- Around line 76-78: The issue is that at line 77 in the mkdir call within the
GitSubproject class, the code creates self.local_path itself as a directory, but
when exporting a file, self.local_path should be the file destination, not a
directory. Instead of calling mkdir on self.local_path, call mkdir on its parent
directory using Path(self.local_path).parent.mkdir(parents=True, exist_ok=True)
to ensure only the containing directory is created, allowing self.local_path to
be used as a file destination.
In `@dfetch/project/subproject.py`:
- Around line 178-179: The return statement at line 179 currently prioritizes
the probe key over the exact key when retrieving the callback result, allowing
directory-level fallback rules to override more specific file rules. Swap the
order of the .get() calls so that exact is checked first with probe as the
fallback, ensuring that the more specific file-level rule takes precedence over
the directory-level rule when both keys are present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2b86797d-d5e6-46b6-9177-be983c284365
📒 Files selected for processing (27)
.pre-commit-config.yamlCHANGELOG.rstdfetch/commands/update.pydfetch/commands/update_patch.pydfetch/project/archivesubproject.pydfetch/project/gitsubproject.pydfetch/project/gitsuperproject.pydfetch/project/subproject.pydfetch/project/superproject.pydfetch/project/svnsubproject.pydfetch/project/svnsuperproject.pydfetch/util/cmdline.pydfetch/vcs/git.pydfetch/vcs/svn.pydoc/conf.pydoc/explanation/line-endings.rstdoc/howto/updating-projects.rstdoc/index.rstdoc/static/uml/update.pumlfeatures/steps/generic_steps.pyfeatures/steps/git_steps.pyfeatures/steps/svn_steps.pyfeatures/superproject-line-ending-attrs.featuretests/test_git_vcs.pytests/test_subproject.pytests/test_svn_vcs.pytests/test_update.py
2 participants
When a superproject has a .gitattributes file with a global eol setting
(e.g.
* text=auto eol=lfor* text=auto eol=crlf), dfetch now usesgit check-attrto read the effective eol preference for the destinationpath and applies it to the temporary git checkout before
git reset --hard,so vendored files always have the line endings the superproject enforces.
eolfield toCheckoutOptionsingit_types.pyeffective_eol()(queries superproject via git check-attr) and_configure_eol()(writes .git/info/attributes + core.eol) toGitLocalRepoingit.pyGitSubProject._fetch_impl()ingitsubproject.py'<path>' has {ending} line endingsstep for binary byte checkshttps://claude.ai/code/session_01WAGxAB7145LZ2CEvGbAKYu
Summary by CodeRabbit
Release Notes
New Features
.gitattributesrules; SVN exports honorsvn:auto-propsand native EOL; archives remain byte-for-byte when no preference applies.Documentation
Bug Fixes
Tests