Add replay-patches command - #1290
Open
spoorcc wants to merge 23 commits into
Open
Conversation
Contributor
WalkthroughAdds ChangesReplay-patches command feature
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/commands/review_patch.py`:
- Around line 113-200: The `_review_project` method has too many branches and
conditional paths, exceeding the cyclomatic complexity limit of 8. Extract the
guard validations (checking for patch existence, on_disk_version, and local
changes) into a separate helper method that returns early if validation fails,
move the interactive review logic and non-interactive logic into their own
helper methods, and simplify the main method to orchestrate validation,
application, and restoration in a clearer sequence. This will distribute the
branching logic across focused helper methods while keeping the main method as a
clear orchestrator.
- Around line 70-78: The --count argument lacks validation, allowing negative
integers that produce unexpected Python slice behavior rather than a meaningful
CLI contract. Add a custom type validator to the add_argument call for --count
to ensure only positive integers are accepted. Additionally, update the logic
around line 173 where the raw count is forwarded to patch_count to validate
against negative values and clamp to valid ranges. Finally, modify the reporting
logic around line 181 to track and report the actual number of patches that were
successfully applied, not the requested count, since the effective count may
differ from what was requested.
- Around line 183-187: The logger.print_info_line call in the review_patch
function currently hardcodes the instruction to use git diff, but this is
inconsistent with non-Git superprojects like SVN that should use their own diff
commands. Make the diff command suggestion in the message VCS-aware by checking
the project's VCS type and conditionally including the appropriate diff command
(git diff for Git projects, svn diff for SVN projects, etc.) in the status
message printed to the user.
In `@dfetch/vcs/git.py`:
- Around line 785-793: Add the `--` separator before the path argument in both
the add_path and restore_staged methods to prevent Git from interpreting
option-style paths as flags. In add_path, insert `"--"` between `"add"` and
`path` in the command list passed to run_on_cmdline. In restore_staged, insert
`"--"` between `"--staged"` and `path` in the command list passed to
run_on_cmdline. This ensures Git treats the path as a positional argument rather
than a potential option flag.
In `@tests/test_review_patch.py`:
- Around line 79-97: Add a new test function to validate that negative count
values are rejected by the ReviewPatch command. Create a test that instantiates
ReviewPatch, mocks the required dependencies (create_super_project,
create_sub_project, in_directory, is_tty) similar to
test_review_count_1_uses_patch_count_1, and then calls cmd(_make_args(count=-1))
while asserting that this raises an appropriate validation error or exception.
This ensures the CLI contract maintains count-based validation and prevents
regression to slice-driven behavior for negative values.
🪄 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: 1290abec-833f-46ef-9d9d-5ff586b6111b
📒 Files selected for processing (11)
CHANGELOG.rstdfetch/__main__.pydfetch/commands/command.pydfetch/commands/review_patch.pydfetch/commands/update_patch.pydfetch/project/gitsuperproject.pydfetch/vcs/git.pydoc/howto/patching.rstfeatures/review-patch-in-git.featurefeatures/review-patch-in-svn.featuretests/test_review_patch.py
spoorcc
force-pushed
the
claude/review-patch-command-xyosoo
branch
from
June 23, 2026 21:14
cfc9798 to
6312b9a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/commands/review_patch.py`:
- Line 260: The Ctrl-C handler is currently catching KeyboardInterrupt, clearing
the screen, and returning normally, but the UI message advertises "Ctrl-C abort"
which implies the command should abort. Locate the exception handlers that catch
KeyboardInterrupt (near lines 260 and 319-321 in the review_patch.py file) and
modify them to re-raise the KeyboardInterrupt exception after clearing the
screen, rather than returning normally. This will allow the outer finally block
to restore state before the command properly aborts.
- Line 47: The import statement in review_patch.py violates the command-layer
dependency boundary by directly importing from dfetch.terminal, which is not an
allowed dependency. The imports of Screen, is_tty, read_key, BOLD, DIM, and
RESET must be sourced from one of the allowed layers (dfetch.reporting,
dfetch.project, dfetch.manifest, dfetch.vcs, dfetch.util, or dfetch.log). Either
move these terminal primitives to one of the allowed modules or create a
wrapper/facade in an allowed module that exposes these utilities, then update
the import in review_patch.py to import from the allowed layer instead of
directly from dfetch.terminal.
- Around line 137-147: The mutations to the subproject via subproject.update()
and to git_super via git_super.add_path() are occurring before the try/finally
restore guard begins, which means if either call fails, the restore mechanism in
the finally block will not execute and the worktree/index could be left in an
inconsistent state. Move the subproject.update() call (with patch_count=0) and
the conditional git_super.add_path() call to occur after the try block starts,
so they are protected by the restore guard in the finally block. The same issue
also applies to the code in the range around lines 159-174, so ensure all
mutations that need protection occur within the try block.
- Around line 183-202: The ReviewPatch functionality needs to validate patch
files exist and are accessible before treating them as applicable or performing
worktree operations. Add comprehensive patch file validation in the ReviewPatch
method that performs checks similar to those shown in the diff (verifying patch
existence via subproject.patch, confirming the subproject version exists via
on_disk_version(), and checking for local changes via
has_local_changes_in_dir()) to ensure that chosen_count == -1 or any decision to
apply patches is only made when the patch file is actually valid and accessible.
Ensure this validation logic is applied consistently across all locations where
patches are processed (including the locations at lines 217-221, 233-246, and
290-297) before any worktree replacement operations occur, and validate the
actual patch file object when calling Patch.from_file to catch missing or
out-of-root patch files early.
In `@dfetch/project/subproject.py`:
- Around line 90-97: The apply_patches method accepts a count parameter but does
not validate the input, allowing invalid values like -2 to be passed and cause
unexpected behavior in the slicing logic. Add a validation guard at the
beginning of the apply_patches method that raises a ValueError if count is less
than -1, ensuring only -1 (meaning all patches) or positive values are accepted
as documented in the docstring.
In `@features/steps/git_steps.py`:
- Around line 264-266: In the subprocess.check_output call for the git status
command, add the `--` separator argument between the `--porcelain` option and
the path argument to prevent paths starting with a hyphen from being interpreted
as git options. Modify the command list from `["git", "status", "--porcelain",
path]` to `["git", "status", "--porcelain", "--", path]`.
- Line 10: Move the no-name-in-module pylint suppression from the inline comment
on the import statement (from behave import given, then, when) to the
module-level header. Locate the existing pylint disable comment on line 3 in the
module header and add no-name-in-module to that existing disable list, then
remove the inline # pylint: disable=no-name-in-module comment from the import
statement on line 10.
🪄 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: 8c8ecd44-472d-43c4-89d7-b69bfa272e16
⛔ Files ignored due to path filters (1)
doc/asciicasts/review-patch.gifis excluded by!**/*.gif
📒 Files selected for processing (18)
.github/workflows/run.ymlCHANGELOG.rstdfetch/__main__.pydfetch/commands/command.pydfetch/commands/review_patch.pydfetch/commands/update_patch.pydfetch/project/gitsuperproject.pydfetch/project/subproject.pydfetch/vcs/git.pydoc/asciicasts/review-patch.castdoc/generate-casts/generate-casts.shdoc/generate-casts/review-patch-demo.shdoc/howto/patching.rstfeatures/review-patch-in-git.featurefeatures/review-patch-in-svn.featurefeatures/steps/git_steps.pysecurity/tm_usage.pytests/test_review_patch.py
| from dfetch.project.gitsuperproject import GitSuperProject | ||
| from dfetch.project.subproject import SubProject | ||
| from dfetch.project.superproject import NoVcsSuperProject, SuperProject | ||
| from dfetch.terminal import BOLD, DIM, RESET, Screen, is_tty, read_key |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Route terminal primitives through an allowed layer.
dfetch.commands.review_patch imports dfetch.terminal directly, which violates the command-layer dependency boundary. Move or expose Screen, is_tty, read_key, and the style constants through an allowed lower layer before merging.
As per coding guidelines, dfetch/commands/*.py commands may only import from dfetch.reporting, dfetch.project, dfetch.manifest, dfetch.vcs, dfetch.util, and dfetch.log.
🤖 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/commands/review_patch.py` at line 47, The import statement in
review_patch.py violates the command-layer dependency boundary by directly
importing from dfetch.terminal, which is not an allowed dependency. The imports
of Screen, is_tty, read_key, BOLD, DIM, and RESET must be sourced from one of
the allowed layers (dfetch.reporting, dfetch.project, dfetch.manifest,
dfetch.vcs, dfetch.util, or dfetch.log). Either move these terminal primitives
to one of the allowed modules or create a wrapper/facade in an allowed module
that exposes these utilities, then update the import in review_patch.py to
import from the allowed layer instead of directly from dfetch.terminal.
Source: Coding guidelines
Comment on lines
+137
to
+147
| saved_metadata = Path(subproject.metadata_path).read_bytes() | ||
| total_patches = len(list(subproject.patch)) | ||
| subproject.update( | ||
| force=True, | ||
| ignored_files_callback=_ignored, | ||
| patch_count=0, | ||
| eol_preferences_callback=superproject.eol_preferences, | ||
| ) | ||
|
|
||
| if git_super is not None: | ||
| git_super.add_path(subproject.local_path) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Start the restore guard before mutating the project.
subproject.update(... patch_count=0) and git_super.add_path(...) both mutate the worktree/index before the try/finally begins. If either raises, the command can leave the project at clean upstream or with a staged index, bypassing the restore path.
🛡️ Proposed restore-scope fix
saved_metadata = Path(subproject.metadata_path).read_bytes()
total_patches = len(list(subproject.patch))
- subproject.update(
- force=True,
- ignored_files_callback=_ignored,
- patch_count=0,
- eol_preferences_callback=superproject.eol_preferences,
- )
-
- if git_super is not None:
- git_super.add_path(subproject.local_path)
-
chosen_count = count if count is not None else -1
effective = (
total_patches if chosen_count == -1 else min(chosen_count, total_patches)
)
@@
)
worktree_fully_patched = False
try:
+ subproject.update(
+ force=True,
+ ignored_files_callback=_ignored,
+ patch_count=0,
+ eol_preferences_callback=superproject.eol_preferences,
+ )
+
+ if git_super is not None:
+ git_super.add_path(subproject.local_path)
+
worktree_fully_patched = _apply_review(
subproject, project.name, chosen_count, interactive, info_msg
)Also applies to: 159-174
🤖 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/commands/review_patch.py` around lines 137 - 147, The mutations to the
subproject via subproject.update() and to git_super via git_super.add_path() are
occurring before the try/finally restore guard begins, which means if either
call fails, the restore mechanism in the finally block will not execute and the
worktree/index could be left in an inconsistent state. Move the
subproject.update() call (with patch_count=0) and the conditional
git_super.add_path() call to occur after the try block starts, so they are
protected by the restore guard in the finally block. The same issue also applies
to the code in the range around lines 159-174, so ensure all mutations that need
protection occur within the try block.
Comment on lines
+183
to
+202
| if not subproject.patch: | ||
| logger.print_warning_line( | ||
| project_name, | ||
| 'skipped - there is no patch file, use "dfetch diff"' | ||
| f" {project_name} to create one", | ||
| ) | ||
| return False | ||
| if not subproject.on_disk_version(): | ||
| logger.print_warning_line( | ||
| project_name, | ||
| f'skipped - the project was never fetched, use "dfetch update {project_name}"', | ||
| ) | ||
| return False | ||
| if superproject.has_local_changes_in_dir(subproject.local_path): | ||
| logger.print_warning_line( | ||
| project_name, | ||
| f"skipped - uncommitted changes in {subproject.local_path}", | ||
| ) | ||
| return False | ||
| return True |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate patch files before replacing the worktree.
SubProject._apply_patches skips missing or out-of-root patch files, but ReviewPatch still treats chosen_count == -1 as fully restored/applicable and may only unstage Git afterwards. A missing patch can leave the working tree at clean upstream instead of the original patched state; interactive mode also feeds raw manifest paths directly to Patch.from_file.
🛡️ Proposed pre-validation shape
-from collections.abc import Callable, Generator
+from collections.abc import Callable, Generator, Sequence
@@
def _can_review_project(
@@
return True
+
+
+def _patch_paths_for_review(
+ patches: Sequence[str],
+ project_name: str,
+) -> list[str] | None:
+ """Return normalized patch paths, or None when review should be skipped."""
+ cwd = Path(".").resolve()
+ normalized: list[str] = []
+ for patch in patches:
+ patch_path = (cwd / patch).resolve()
+ try:
+ relative_patch_path = patch_path.relative_to(cwd)
+ except ValueError:
+ logger.print_warning_line(
+ project_name,
+ f'skipped - patch "{patch}" is outside {cwd}',
+ )
+ return None
+ if not patch_path.exists():
+ logger.print_warning_line(
+ project_name,
+ f'skipped - patch "{patch}" does not exist',
+ )
+ return None
+ normalized.append(relative_patch_path.as_posix())
+ return normalized if not _can_review_project(superproject, subproject, project.name):
return
+ patches = _patch_paths_for_review(subproject.patch, project.name)
+ if patches is None:
+ return
+
saved_metadata = Path(subproject.metadata_path).read_bytes()
- total_patches = len(list(subproject.patch))
+ total_patches = len(patches)
@@
worktree_fully_patched = _apply_review(
- subproject, project.name, chosen_count, interactive, info_msg
+ subproject, project.name, chosen_count, interactive, info_msg, patches
) def _apply_review(
subproject: SubProject,
project_name: str,
chosen_count: int,
interactive: bool,
info_msg: str,
+ patches: list[str],
) -> bool:
@@
if interactive:
- _step_tui(list(subproject.patch), subproject.local_path, project_name)
+ _step_tui(patches, subproject.local_path, project_name)
return FalseAlso applies to: 217-221, 233-246, 290-297
🤖 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/commands/review_patch.py` around lines 183 - 202, The ReviewPatch
functionality needs to validate patch files exist and are accessible before
treating them as applicable or performing worktree operations. Add comprehensive
patch file validation in the ReviewPatch method that performs checks similar to
those shown in the diff (verifying patch existence via subproject.patch,
confirming the subproject version exists via on_disk_version(), and checking for
local changes via has_local_changes_in_dir()) to ensure that chosen_count == -1
or any decision to apply patches is only made when the patch file is actually
valid and accessible. Ensure this validation logic is applied consistently
across all locations where patches are processed (including the locations at
lines 217-221, 233-246, and 290-297) before any worktree replacement operations
occur, and validate the actual patch file object when calling Patch.from_file to
catch missing or out-of-root patch files early.
| """Render the current patch-stack state to the screen.""" | ||
| count_label = str(current) if current < total else "all" | ||
| lines: list[str] = [ | ||
| f" {DIM}← → step Enter restore and exit Ctrl-C abort{RESET}", |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not swallow Ctrl-C when the UI says abort.
The frame advertises Ctrl-C abort, but the handler clears the screen and returns normally. Re-raise after clearing so the command aborts after the outer finally restores state.
🛠️ Proposed signal-handling fix
try:
key = read_key()
except KeyboardInterrupt:
screen.clear()
- return
+ raiseAlso applies to: 319-321
🤖 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/commands/review_patch.py` at line 260, The Ctrl-C handler is currently
catching KeyboardInterrupt, clearing the screen, and returning normally, but the
UI message advertises "Ctrl-C abort" which implies the command should abort.
Locate the exception handlers that catch KeyboardInterrupt (near lines 260 and
319-321 in the review_patch.py file) and modify them to re-raise the
KeyboardInterrupt exception after clearing the screen, rather than returning
normally. This will allow the outer finally block to restore state before the
command properly aborts.
| import argparse | ||
| import tempfile | ||
| from pathlib import Path | ||
| from unittest.mock import ANY, Mock, call, patch |
ben-edna
changed the title
Contributor
|
@coderabbitai resume |
Contributor
|
✅ Action performedReviews resumed. Review finished.
|
spoorcc
force-pushed
the
claude/review-patch-command-xyosoo
branch
from
June 26, 2026 05:27
67fbbc9 to
ea49e29
Compare
Contributor
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 @.github/workflows/run.yml:
- Line 59: The smoke workflow currently only checks that dfetch replay-patches
exits successfully, but it does not verify that the repository is restored to a
clean state afterward. Update the run step in the workflow job to add a
follow-up cleanliness check such as git status --porcelain or git diff
--exit-code immediately after dfetch replay-patches so CI fails if any staged or
working-tree changes remain.
In `@doc/howto/patching.rst`:
- Around line 291-301: Clarify the Dfetch replay-patches introduction by
distinguishing Git and SVN behavior: the current statement about putting clean
upstream source in the git index only applies to Git superprojects, while SVN
superprojects use warning-based `svn diff` guidance without a staging step.
Update the prose in the patching how-to section to either qualify the index
wording for Git or explicitly mention the SVN difference so readers do not
assume index-based diff behavior for all backends.
In `@features/replay-patches-in-svn.feature`:
- Around line 33-50: The replay-patches SVN scenarios currently verify file
contents but not that the working copy is clean after restore. Update the
affected scenarios around the replay-patches flow to assert the SVN status is
clean after the command finishes, using an `svn status`-based step or an
equivalent existing assertion step, so the restore guarantee is covered in the
scenarios for `replay-patches` and `MySvnProject`.
In `@security/__init__.py`:
- Around line 6-9: The threat-model modules are relying on security/__init__.py
to mutate sys.path, which only works when the package is imported and breaks if
security/tm_usage.py or security/tm_supply_chain.py are run directly. Update the
entrypoints in those modules to use package-style execution/imports only, and
make the documentation build import the package through Sphinx configuration
rather than depending on runtime path injection. If needed, move the bootstrap
responsibility into doc/conf.py or package installation setup so internal
imports like security.tm_render always resolve without executing modules as
standalone scripts.
In `@tests/test_replay_patches.py`:
- Around line 56-112: Add a failure-path cleanup test for ReplayPatches: the
current tests cover only successful restoration, but not the case where
apply_patches() or the interactive flow raises after fake_super.add_path() has
staged changes. Add a test in
test_review_all_patches_calls_update_add_path_update that makes the subproject
or tty-driven step raise, then assert the appropriate restore helper on
fake_super still runs via the ReplayPatches cleanup path (finally-style), using
the existing ReplayPatches, _make_superproject, _make_subproject, and restore_*
symbols.
🪄 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: aa839f61-fc5d-4216-bc5e-b664bc9c41a0
⛔ Files ignored due to path filters (1)
doc/asciicasts/replay-patches.gifis excluded by!**/*.gif
📒 Files selected for processing (20)
.github/workflows/run.ymlCHANGELOG.rstdfetch/__main__.pydfetch/commands/command.pydfetch/commands/replay_patches.pydfetch/commands/update_patch.pydfetch/project/gitsuperproject.pydfetch/project/subproject.pydfetch/vcs/git.pydoc/asciicasts/replay-patches.castdoc/generate-casts/generate-casts.shdoc/generate-casts/replay-patches-demo.shdoc/howto/patching.rstfeatures/replay-patches-in-git.featurefeatures/replay-patches-in-svn.featurefeatures/steps/git_steps.pysecurity/__init__.pysecurity/tm_supply_chain.pysecurity/tm_usage.pytests/test_replay_patches.py
| - run: dfetch update | ||
| - run: dfetch update | ||
| - run: dfetch update-patch | ||
| - run: dfetch replay-patches |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Verify the repo is clean after the smoke run.
dfetch replay-patches exiting 0 is not enough here; the feature's main guarantee is that it restores the original state. Add a follow-up git status --porcelain or git diff --exit-code check after this step so CI fails if the command leaves staged or working-tree changes behind.
Also applies to: 202-202
🤖 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 @.github/workflows/run.yml at line 59, The smoke workflow currently only
checks that dfetch replay-patches exits successfully, but it does not verify
that the repository is restored to a clean state afterward. Update the run step
in the workflow job to add a follow-up cleanliness check such as git status
--porcelain or git diff --exit-code immediately after dfetch replay-patches so
CI fails if any staged or working-tree changes remain.
Comment on lines
+291
to
+301
| *Dfetch* puts the clean upstream source in the git index and applies the | ||
| patches to the working tree. You can now see exactly what the patches change | ||
| using any diff tool you prefer — for example: | ||
|
|
||
| .. code-block:: console | ||
|
|
||
| $ git diff some-project/ | ||
|
|
||
| Or open the project in VS Code and browse the **Changes** view in the Source | ||
| Control panel. (The **Staged Changes** view shows something different and | ||
| unrelated to your patches — use **Changes**.) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify SVN behavior in the replay-patches introduction.
The description states that Dfetch "puts the clean upstream source in the git index," which is only true for Git superprojects. SVN superprojects are supported with a warning and use svn diff hints without a staging step. This will mislead SVN users who expect index-based diff behavior.
Add a qualifier for Git or note the SVN difference explicitly in this section.
🤖 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/patching.rst` around lines 291 - 301, Clarify the Dfetch
replay-patches introduction by distinguishing Git and SVN behavior: the current
statement about putting clean upstream source in the git index only applies to
Git superprojects, while SVN superprojects use warning-based `svn diff` guidance
without a staging step. Update the prose in the patching how-to section to
either qualify the index wording for Git or explicitly mention the SVN
difference so readers do not assume index-based diff behavior for all backends.
Comment on lines
+33
to
+50
| Scenario: All patches are set up for review and state is restored afterwards | ||
| When I run "dfetch replay-patches SomeProject" in MySvnProject | ||
| Then the output shows | ||
| """ | ||
| Dfetch (0.14.0) | ||
| replay-patches has limited support in SVN superprojects (no staging area — use `svn diff` to inspect changes) | ||
| SomeProject: | ||
| > Fetched trunk - 1 | ||
| > Applying patch "patches/SomeProject.patch" | ||
| successfully patched 1/1: b'README.md' | ||
| > stage = upstream, working tree = 1 patch(es) applied — open your editor and run `svn diff` to inspect | ||
| > restored | ||
| """ | ||
| And the patched 'MySvnProject/SomeProject/README.md' is | ||
| """ | ||
| Patched file for SomeProject | ||
| """ | ||
|
|
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Assert the SVN working copy is clean after restore.
These scenarios only re-check file contents. That can still miss the bug this command is supposed to prevent: leaving local SVN modifications behind after exit. Please add an svn status assertion (or equivalent step) after each scenario so the restore guarantee is actually covered.
Also applies to: 51-69
🤖 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 `@features/replay-patches-in-svn.feature` around lines 33 - 50, The
replay-patches SVN scenarios currently verify file contents but not that the
working copy is clean after restore. Update the affected scenarios around the
replay-patches flow to assert the SVN status is clean after the command
finishes, using an `svn status`-based step or an equivalent existing assertion
step, so the restore guarantee is covered in the scenarios for `replay-patches`
and `MySvnProject`.
Comment on lines
+56
to
+112
| def test_review_all_patches_calls_update_add_path_update(): | ||
| """All patches are applied and the staging area is restored for a git superproject.""" | ||
| cmd = ReplayPatches() | ||
| fake_super = _make_superproject(is_git=True) | ||
| fake_sub = _make_subproject() | ||
|
|
||
| with patch( | ||
| "dfetch.commands.replay_patches.create_super_project", return_value=fake_super | ||
| ): | ||
| with patch("dfetch.commands.command.in_directory"): | ||
| with patch( | ||
| "dfetch.commands.replay_patches.create_sub_project", | ||
| return_value=fake_sub, | ||
| ): | ||
| with patch("dfetch.commands.replay_patches.is_tty", return_value=False): | ||
| cmd(_make_args()) | ||
|
|
||
| fake_sub.update.assert_called_once_with( | ||
| force=True, | ||
| ignored_files_callback=ANY, | ||
| patch_count=0, | ||
| eol_preferences_callback=ANY, | ||
| ) | ||
| fake_super.add_path.assert_called_once_with("my_project") | ||
| fake_sub.apply_patches.assert_called_once_with(-1) | ||
| fake_super.restore_worktree.assert_not_called() | ||
| fake_super.restore_staged.assert_called_once_with("my_project") | ||
|
|
||
|
|
||
| def test_review_count_1_uses_patch_count_1(): | ||
| """--count 1 limits apply_patches to exactly one patch.""" | ||
| cmd = ReplayPatches() | ||
| fake_super = _make_superproject(is_git=True) | ||
| fake_sub = _make_subproject() | ||
|
|
||
| with patch( | ||
| "dfetch.commands.replay_patches.create_super_project", return_value=fake_super | ||
| ): | ||
| with patch("dfetch.commands.command.in_directory"): | ||
| with patch( | ||
| "dfetch.commands.replay_patches.create_sub_project", | ||
| return_value=fake_sub, | ||
| ): | ||
| with patch("dfetch.commands.replay_patches.is_tty", return_value=False): | ||
| cmd(_make_args(count=1)) | ||
|
|
||
| fake_sub.update.assert_called_once_with( | ||
| force=True, | ||
| ignored_files_callback=ANY, | ||
| patch_count=0, | ||
| eol_preferences_callback=ANY, | ||
| ) | ||
| fake_sub.apply_patches.assert_called_once_with(1) | ||
| fake_super.restore_from_head.assert_called_once_with("my_project") | ||
| fake_super.restore_worktree.assert_not_called() | ||
| fake_super.restore_staged.assert_not_called() | ||
|
|
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add one failure-path cleanup test.
These tests only prove restoration on successful runs. The risky part of replay-patches is leaving the index/worktree dirty when apply_patches() or the interactive step raises after staging starts, and there is no regression coverage for that contract here. Add a case that forces an exception after add_path and asserts the matching restore helper still runs from a finally path.
Also applies to: 331-378
🤖 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_replay_patches.py` around lines 56 - 112, Add a failure-path
cleanup test for ReplayPatches: the current tests cover only successful
restoration, but not the case where apply_patches() or the interactive flow
raises after fake_super.add_path() has staged changes. Add a test in
test_review_all_patches_calls_update_add_path_update that makes the subproject
or tty-driven step raise, then assert the appropriate restore helper on
fake_super still runs via the ReplayPatches cleanup path (finally-style), using
the existing ReplayPatches, _make_superproject, _make_subproject, and restore_*
symbols.
Introduces `dfetch review-patch` which stages the clean upstream source in the git index and applies the selected patches to the working tree, so any diff-aware editor sees `git diff` (working tree vs index) showing exactly what the patches contribute. The command always restores original state on exit — no permanent changes to working tree or index. - New command: dfetch/commands/review_patch.py with --count/-n, --interactive/-i - GitSuperProject.add_path() and restore_staged() methods for index control - GitLocalRepo.add_path() and restore_staged() as git-level primitives - SVN superprojects supported (with a warning; no staging step) - Interactive TUI uses read_key()/Screen for ← → step-through - 8 unit tests, 4 git BDD scenarios - Documentation and changelog updated Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
- Extract shared project-iteration loop into Command._iter_projects to eliminate duplicate code between review_patch and update_patch (R0801) - Fix import order: review_patch before update in __main__.py (isort) - Remove redundant `import dfetch.project` in review_patch; import create_sub_project directly instead (code review finding) - Remove unused MagicMock import from test_review_patch (code review finding) - Update test patch targets to match new import locations Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
- Add `dfetch review-patch` step after `dfetch update-patch` in both the `run` (all platforms) and `test-cygwin` jobs in run.yml - Add `review-patch-demo.sh` cast generation script (mirrors update-patch-demo.sh; pipes stdin to avoid blocking on input()) - Register the demo in generate-casts.sh after the update-patch line - Add synthetic review-patch.cast (asciicast v2) showing the full flow: manifest inspection, patch preview, staging, and restore - Wire `.. asciinema:: ../asciicasts/review-patch.cast` into the Reviewing a patch section of doc/howto/patching.rst Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
In the default (all-patches) case review-patch was fetching three times: once for clean upstream, once to apply all patches, and once more in the finally block to "restore" — even though the working tree was already fully patched after the second fetch. Track worktree_fully_patched after the non-interactive update call and skip the finally fetch when the working tree is already at patch_count=-1. The --count N and --interactive paths still re-fetch because the final working-tree state differs from fully patched. Update tests and the synthetic demo cast accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
After the initial update(patch_count=0) puts clean upstream in the working tree there is no reason to re-fetch for any subsequent step. - Add SubProject.apply_patches(count) — applies patch files to the already-fetched working tree without going back to the remote. - Add GitSuperProject.restore_worktree(path) / GitLocalRepo.restore_worktree — runs `git restore <path>` to reset the working tree from the staged index (which holds clean upstream throughout the review session). - Non-interactive: replace the second update() call with apply_patches(). Git all-patches case: 1 fetch total (was 2). Git --count N case: 1 fetch + restore_worktree + apply_patches (was 3). - Interactive TUI: replace per-step update() calls with direct patch apply/reverse (Patch.from_file().apply() / .reverse().apply()). No fetch at all during stepping, for both Git and SVN. - Finally block (Git): restore_worktree + apply_patches instead of a re-fetch. SVN still re-fetches when the worktree is not fully patched (no staged index to restore from). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
Extract _can_review_project, _apply_review, _restore_project, _draw_tui_frame, and _apply_step as module-level helpers so every function stays below the CC=8 limit enforced by CI. Black-format tests to match. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
After the refactor, review-patch does only one fetch (clean upstream) and applies/reverses patches directly without re-fetching. The expected output in the BDD scenarios previously included 3 Fetched lines; update them to reflect the new single-fetch flow. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
…ff cmd - Reject --count < 0 with a RuntimeError early in __call__ - Add test_negative_count_raises to prevent regression - Add -- separator before path in git add, git restore --staged, and git restore to prevent option-style paths being parsed as flags - Show `svn diff` in the status message for SVN superprojects instead of always saying `git diff` - Clamp reported patch count to min(requested, total) so the message reflects patches actually applied rather than the raw CLI argument - Add review-patch to the dfetch_cli threat-model description, noting the transient git index mutation and interruption risk - Reduce _apply_review to 5 args by pre-computing chosen_count and info_msg in _review_project (fixes pylint too-many-arguments) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
Remove the second (now-eliminated) fetch from the cast, regenerate with agg to produce an animated GIF showing the single-fetch flow. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
- Replace sp.__class__ = GitSuperProject/NoVcsSuperProject with Mock(spec=...) so isinstance() checks work without unsafe __class__ mutation that pyright rejects - Add str | None type annotation to on_disk_version parameter so passing None in test_never_fetched_logs_warning_and_skips is valid Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
Replace is_git: bool + assert isinstance(superproject, GitSuperProject) pattern with git_super: GitSuperProject | None so the type checker tracks the narrowed type directly — no assert needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
update(patch_count=0) writes patch: '' to the metadata file. The restore path fixes the working tree but apply_patches() has no metadata-writing logic, leaving the metadata file missing the patch list. Fix: snapshot the metadata file bytes before the clean-upstream fetch and write them back in the finally block after restore completes. Add a BDD step "the metadata of ... lists patch ..." and assert it in both Git and SVN review-patch scenarios to pin the invariant. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
Instead of asserting the patch field in dfetch_data.yaml directly, verify from the user's VCS perspective: git status --porcelain SomeProject must report nothing after review-patch exits, covering both the working tree files and the metadata file in one shot. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
patch_ng logs "successfully patched N/M: file" via Python logging when Patch.apply() is called. In the TUI loop this output appears between frames, so Screen.draw()'s cursor-up count is wrong and the help line gets duplicated on each arrow keypress. Suppress the patch_ng logger to CRITICAL for the duration of each LEFT/RIGHT apply/reverse operation inside the TUI loop. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
If Patch.apply() or Patch.reverse().apply() raises RuntimeError during the interactive stepper, the TUI frame was left on screen while the error and 'restored' messages printed below it. Catch RuntimeError in _step_tui, clear the screen, then re-raise so the existing try/finally in _review_project still runs _restore_project (working tree and metadata are always restored), and _iter_projects logs the error under the project name as usual. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
The old restore path did restore_worktree (WT = staged = clean upstream) followed by apply_patches(), which would hit the same failing patch and raise again, leaving the index staged, the metadata corrupted, and the working tree in the wrong state. Replace with restore_from_head (git restore --source=HEAD --staged --worktree -- <path>) which atomically resets both the working tree and the index to the committed HEAD state without re-applying patches. This makes restore unconditionally safe regardless of patch health. Also nest the metadata write_bytes in its own try/finally so the dfetch_data.yaml snapshot is always restored even if _restore_project encounters an error (e.g. SVN path where apply_patches can still fail). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
spoorcc
force-pushed
the
claude/review-patch-command-xyosoo
branch
from
July 6, 2026 19:27
ea49e29 to
1a0a8c3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
dfetch/project/subproject.py (1)
90-98: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRaise
RuntimeError(notValueError) for invalidcount, to match the codebase's error-handling contract.A previous review on this exact validation recommended
RuntimeError. As implemented, an invalidcountraisesValueError, which bypassesCommand._iter_projects'sexcept RuntimeErrorper-project isolation anddfetch/__main__.py'sexcept (RuntimeError, TypeError)top-level handler — so a caller that misuses this public API would get a raw, unhandled traceback instead of dfetch's normal graceful CLI error. All current call sites inreplay_patches.pypre-validate to-1/>=0so this isn't reachable today, butapply_patchesis a new public API meant for reuse by future commands.🐛 Proposed fix
def apply_patches(self, count: int = -1) -> None: """Apply patches to the already-fetched working tree without fetching from remote. Args: count: Number of patches to apply (-1 means all). """ if count < -1: - raise ValueError(f"count must be -1 (all) or >= 0, got {count}") + raise RuntimeError(f"count must be -1 (all) or >= 0, got {count}") self._apply_patches(count)🤖 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 90 - 98, Update apply_patches validation to raise RuntimeError instead of ValueError when count is less than -1, preserving the existing message and validation boundary so invalid public API usage follows the codebase’s standard error-handling paths.
🤖 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/commands/replay_patches.py`:
- Around line 87-104: Add concise Google-style docstrings to
_validate_superproject and _check_count_conflicts, documenting each function’s
purpose and parameters as appropriate. Preserve their existing validation logic
and error messages.
- Around line 350-379: Update _stage_one so any failure after
subproject.update(), including git_super.add_path(), restores the original
working-tree and index state via the existing _restore_project flow, not just
saved metadata bytes. Ensure restoration occurs before re-raising and preserves
the fully patched content/metadata consistency; add coverage for an add_path
failure if appropriate.
In `@dfetch/vcs/git.py`:
- Around line 825-828: Add a concise Google-style docstring to
GitLocalRepo._git_restore describing that it restores the specified path using
the provided Git flags, following the existing private-helper documentation
conventions.
In `@doc/asciicasts/replay-patches.cast`:
- Around line 4-8: Regenerate the asciicast for the replay-patches walkthrough
so it includes the replay-patches-demo.sh step that displays
patches/cpputest.patch between cat dfetch.yaml and dfetch replay-patches
cpputest. Ensure the checked-in recording matches the demo script’s command
sequence and output.
In `@doc/generate-casts/replay-patches-demo.sh`:
- Around line 8-9: Replace the fixed review-patch directory setup around mkdir
and pushd with an isolated temporary directory created via the platform’s
temporary-directory mechanism, and register a cleanup trap immediately after
creation. Update all later references, including the cleanup section, to use the
generated directory variable so concurrent runs do not collide and only the
script’s own temporary data is removed.
- Line 47: Propagate failures in the cast-generation scripts: update the piped
dfetch replay-patches command in replay-patches-demo.sh to honor pipe failures,
and update every asciinema rec invocation in generate-casts.sh, including the
command near line 31, to return or explicitly check the recorded command’s exit
status before continuing or stripping casts.
In `@security/tm_usage.py`:
- Around line 157-164: Update the replay-patches staging and cleanup flow,
including restore_staged(), to preserve each path’s original Git index state
rather than resetting it unconditionally to HEAD. Snapshot and restore
pre-existing staged entries, or explicitly reject/report paths that are already
staged, and revise the surrounding documentation to state that termination
before finally completes can leave the index persistently modified.
---
Duplicate comments:
In `@dfetch/project/subproject.py`:
- Around line 90-98: Update apply_patches validation to raise RuntimeError
instead of ValueError when count is less than -1, preserving the existing
message and validation boundary so invalid public API usage follows the
codebase’s standard error-handling paths.
🪄 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 Plus
Run ID: d9ed80d6-3a31-42d3-a645-5b142ca887e6
⛔ Files ignored due to path filters (1)
doc/asciicasts/replay-patches.gifis excluded by!**/*.gif
📒 Files selected for processing (20)
.github/workflows/run.ymlCHANGELOG.rstdfetch/__main__.pydfetch/commands/command.pydfetch/commands/replay_patches.pydfetch/commands/update_patch.pydfetch/project/gitsuperproject.pydfetch/project/subproject.pydfetch/vcs/git.pydoc/asciicasts/replay-patches.castdoc/generate-casts/generate-casts.shdoc/generate-casts/replay-patches-demo.shdoc/howto/patching.rstfeatures/replay-patches-in-git.featurefeatures/replay-patches-in-svn.featurefeatures/steps/git_steps.pysecurity/__init__.pysecurity/tm_supply_chain.pysecurity/tm_usage.pytests/test_replay_patches.py
Comment on lines
+87
to
+104
| def _validate_superproject(superproject: SuperProject) -> None: | ||
| if isinstance(superproject, NoVcsSuperProject): | ||
| raise RuntimeError( | ||
| "The project containing the manifest is not under version control," | ||
| " reviewing patches is not supported" | ||
| ) | ||
| if not isinstance(superproject, GitSuperProject): | ||
| logger.warning( | ||
| "replay-patches has limited support in SVN superprojects" | ||
| " (no staging area — use `svn diff` to inspect changes)" | ||
| ) | ||
|
|
||
|
|
||
| def _check_count_conflicts( | ||
| count: int | None, per_project_counts: dict[str, int] | ||
| ) -> None: | ||
| if count is not None and per_project_counts: | ||
| raise RuntimeError("use either --count or project:N, not both") |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add docstrings to _validate_superproject and _check_count_conflicts.
Every other function in this file has a docstring; these two are the exceptions, which is inconsistent and would likely fail pydocstyle.
📝 Proposed fix
def _validate_superproject(superproject: SuperProject) -> None:
+ """Validate that the superproject supports patch review, warning for SVN."""
if isinstance(superproject, NoVcsSuperProject): def _check_count_conflicts(
count: int | None, per_project_counts: dict[str, int]
) -> None:
+ """Raise if both --count and a project:N suffix were supplied together."""
if count is not None and per_project_counts:As per coding guidelines, "Follow Google-style docstring conventions, enforced by pydocstyle" for **/*.py.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _validate_superproject(superproject: SuperProject) -> None: | |
| if isinstance(superproject, NoVcsSuperProject): | |
| raise RuntimeError( | |
| "The project containing the manifest is not under version control," | |
| " reviewing patches is not supported" | |
| ) | |
| if not isinstance(superproject, GitSuperProject): | |
| logger.warning( | |
| "replay-patches has limited support in SVN superprojects" | |
| " (no staging area — use `svn diff` to inspect changes)" | |
| ) | |
| def _check_count_conflicts( | |
| count: int | None, per_project_counts: dict[str, int] | |
| ) -> None: | |
| if count is not None and per_project_counts: | |
| raise RuntimeError("use either --count or project:N, not both") | |
| def _validate_superproject(superproject: SuperProject) -> None: | |
| """Validate that the superproject supports patch review, warning for SVN.""" | |
| if isinstance(superproject, NoVcsSuperProject): | |
| raise RuntimeError( | |
| "The project containing the manifest is not under version control," | |
| " reviewing patches is not supported" | |
| ) | |
| if not isinstance(superproject, GitSuperProject): | |
| logger.warning( | |
| "replay-patches has limited support in SVN superprojects" | |
| " (no staging area — use `svn diff` to inspect changes)" | |
| ) | |
| def _check_count_conflicts( | |
| count: int | None, per_project_counts: dict[str, int] | |
| ) -> None: | |
| """Raise if both --count and a project:N suffix were supplied together.""" | |
| if count is not None and per_project_counts: | |
| raise RuntimeError("use either --count or project:N, not both") |
🤖 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/commands/replay_patches.py` around lines 87 - 104, Add concise
Google-style docstrings to _validate_superproject and _check_count_conflicts,
documenting each function’s purpose and parameters as appropriate. Preserve
their existing validation logic and error messages.
Source: Coding guidelines
Comment on lines
+350
to
+379
| def _stage_one( | ||
| superproject: SuperProject, | ||
| git_super: GitSuperProject | None, | ||
| project: dfetch.manifest.project.ProjectEntry, | ||
| subproject: SubProject, | ||
| ) -> _StagedEntry: | ||
| """Fetch upstream into the worktree and stage it; return a restore tuple.""" | ||
| saved_metadata = Path(subproject.metadata_path).read_bytes() | ||
|
|
||
| def _ignored() -> list[str]: | ||
| return list(superproject.ignored_files(project.destination)) | ||
|
|
||
| try: | ||
| subproject.update( | ||
| force=True, | ||
| ignored_files_callback=_ignored, | ||
| patch_count=0, | ||
| eol_preferences_callback=superproject.eol_preferences, | ||
| ) | ||
| if git_super is not None: | ||
| git_super.add_path(subproject.local_path) | ||
| except Exception: | ||
| Path(subproject.metadata_path).write_bytes(saved_metadata) | ||
| raise | ||
| state = _ProjectState( | ||
| name=project.name, | ||
| local_path=subproject.local_path, | ||
| patches=list(subproject.patch), | ||
| ) | ||
| return subproject, state, saved_metadata, _ignored |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Failure during staging leaves the working tree unrestored — potential silent data loss/corruption.
If subproject.update() succeeds (replacing local_path with the clean, unpatched upstream content) but the subsequent git_super.add_path() call fails (e.g., git index lock, permission error), the except Exception: block only restores the saved metadata bytes — it never restores the actual working-tree content back to the original fully-patched state. Compare this to _review_project's finally block, which always calls _restore_project(..., worktree_fully_patched=False, ...) first, which correctly resets both index and worktree via git_super.restore_from_head. Here, the metadata will claim the original fully-patched state while the on-disk files are actually the clean, unpatched upstream — a real content/metadata mismatch that a user would have to manually fix.
🐛 Proposed fix
try:
subproject.update(
force=True,
ignored_files_callback=_ignored,
patch_count=0,
eol_preferences_callback=superproject.eol_preferences,
)
if git_super is not None:
git_super.add_path(subproject.local_path)
except Exception:
- Path(subproject.metadata_path).write_bytes(saved_metadata)
- raise
+ try:
+ _restore_project(
+ superproject, git_super, subproject, project.name, False, _ignored
+ )
+ finally:
+ Path(subproject.metadata_path).write_bytes(saved_metadata)
+ raiseNo test currently exercises this failure path — consider adding one alongside the fix.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _stage_one( | |
| superproject: SuperProject, | |
| git_super: GitSuperProject | None, | |
| project: dfetch.manifest.project.ProjectEntry, | |
| subproject: SubProject, | |
| ) -> _StagedEntry: | |
| """Fetch upstream into the worktree and stage it; return a restore tuple.""" | |
| saved_metadata = Path(subproject.metadata_path).read_bytes() | |
| def _ignored() -> list[str]: | |
| return list(superproject.ignored_files(project.destination)) | |
| try: | |
| subproject.update( | |
| force=True, | |
| ignored_files_callback=_ignored, | |
| patch_count=0, | |
| eol_preferences_callback=superproject.eol_preferences, | |
| ) | |
| if git_super is not None: | |
| git_super.add_path(subproject.local_path) | |
| except Exception: | |
| Path(subproject.metadata_path).write_bytes(saved_metadata) | |
| raise | |
| state = _ProjectState( | |
| name=project.name, | |
| local_path=subproject.local_path, | |
| patches=list(subproject.patch), | |
| ) | |
| return subproject, state, saved_metadata, _ignored | |
| def _stage_one( | |
| superproject: SuperProject, | |
| git_super: GitSuperProject | None, | |
| project: dfetch.manifest.project.ProjectEntry, | |
| subproject: SubProject, | |
| ) -> _StagedEntry: | |
| """Fetch upstream into the worktree and stage it; return a restore tuple.""" | |
| saved_metadata = Path(subproject.metadata_path).read_bytes() | |
| def _ignored() -> list[str]: | |
| return list(superproject.ignored_files(project.destination)) | |
| try: | |
| subproject.update( | |
| force=True, | |
| ignored_files_callback=_ignored, | |
| patch_count=0, | |
| eol_preferences_callback=superproject.eol_preferences, | |
| ) | |
| if git_super is not None: | |
| git_super.add_path(subproject.local_path) | |
| except Exception: | |
| try: | |
| _restore_project( | |
| superproject, git_super, subproject, project.name, False, _ignored | |
| ) | |
| finally: | |
| Path(subproject.metadata_path).write_bytes(saved_metadata) | |
| raise | |
| state = _ProjectState( | |
| name=project.name, | |
| local_path=subproject.local_path, | |
| patches=list(subproject.patch), | |
| ) | |
| return subproject, state, saved_metadata, _ignored |
🤖 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/commands/replay_patches.py` around lines 350 - 379, Update _stage_one
so any failure after subproject.update(), including git_super.add_path(),
restores the original working-tree and index state via the existing
_restore_project flow, not just saved metadata bytes. Ensure restoration occurs
before re-raising and preserves the fully patched content/metadata consistency;
add coverage for an add_path failure if appropriate.
Comment on lines
+825
to
+828
| def _git_restore(self, *flags: str, path: str) -> None: | ||
| with in_directory(self._path): | ||
| run_on_cmdline(logger, ["git", "restore", *flags, "--", path]) | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a docstring to _git_restore.
Every other private helper in GitLocalRepo has a docstring; this one is missing, which is inconsistent and would likely trip pydocstyle.
📝 Proposed fix
def _git_restore(self, *flags: str, path: str) -> None:
+ """Run `git restore` with the given flags against a single pathspec."""
with in_directory(self._path):
run_on_cmdline(logger, ["git", "restore", *flags, "--", path])As per coding guidelines, "Follow Google-style docstring conventions, enforced by pydocstyle" for **/*.py.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _git_restore(self, *flags: str, path: str) -> None: | |
| with in_directory(self._path): | |
| run_on_cmdline(logger, ["git", "restore", *flags, "--", path]) | |
| def _git_restore(self, *flags: str, path: str) -> None: | |
| """Run `git restore` with the given flags against a single pathspec.""" | |
| with in_directory(self._path): | |
| run_on_cmdline(logger, ["git", "restore", *flags, "--", path]) |
🤖 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/git.py` around lines 825 - 828, Add a concise Google-style
docstring to GitLocalRepo._git_restore describing that it restores the specified
path using the provided Git flags, following the existing private-helper
documentation conventions.
Source: Coding guidelines
Comment on lines
+4
to
+8
| [1.6, "o", "\u001b[1mcat dfetch.yaml\u001b[0m"] | ||
| [2.6, "o", "\r\n"] | ||
| [2.7, "o", "manifest:\r\n version: 0.0\r\n\r\n remotes:\r\n - name: github\r\n url-base: https://github.com/\r\n\r\n projects:\r\n - name: cpputest\r\n dst: cpputest/src/\r\n repo-path: cpputest/cpputest.git\r\n tag: v3.4\r\n patch: patches/cpputest.patch\r\n\r\n"] | ||
| [2.75, "o", "$ "] | ||
| [3.75, "o", "\u001b[1mdfetch replay-patches cpputest\u001b[0m"] |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Regenerate the cast so it matches the demo script.
replay-patches-demo.sh displays patches/cpputest.patch before running the command, but this cast jumps directly from cat dfetch.yaml to replay-patches. The checked-in recording therefore does not reproduce the generator’s walkthrough.
🤖 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/asciicasts/replay-patches.cast` around lines 4 - 8, Regenerate the
asciicast for the replay-patches walkthrough so it includes the
replay-patches-demo.sh step that displays patches/cpputest.patch between cat
dfetch.yaml and dfetch replay-patches cpputest. Ensure the checked-in recording
matches the demo script’s command sequence and output.
Comment on lines
+8
to
+9
| mkdir review-patch | ||
| pushd review-patch || exit 1 |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Use an isolated temporary directory and cleanup trap.
If review-patch already exists, the failed mkdir is ignored, the script enters that directory, and the final rm -rf review-patch can delete unrelated user data. Concurrent runs also collide.
Proposed fix
- mkdir review-patch
- pushd review-patch || exit 1
+ review_patch_dir="$(mktemp -d "${TMPDIR:-/tmp}/dfetch-replay-patches.XXXXXX")"
+ trap 'rm -rf -- "$review_patch_dir"' EXIT
+ pushd "$review_patch_dir" || exit 1
...
popd || exit 1
- rm -rf review-patchAlso applies to: 54-55
🤖 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/generate-casts/replay-patches-demo.sh` around lines 8 - 9, Replace the
fixed review-patch directory setup around mkdir and pushd with an isolated
temporary directory created via the platform’s temporary-directory mechanism,
and register a cleanup trap immediately after creation. Update all later
references, including the cleanup section, to use the generated directory
variable so concurrent runs do not collide and only the script’s own temporary
data is removed.
| pe "cat dfetch.yaml" | ||
| pe "cat patches/cpputest.patch" | ||
| # Pipe stdin to avoid blocking on "Press Enter to restore..." | ||
| pe "echo '' | dfetch replay-patches cpputest" |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -u
echo "== files =="
git ls-files | grep -E '(^|/)generate-casts|(^|/)replay-patches-demo\.sh$' || true
echo "== repo file contents =="
for f in doc/generate-casts/replay-patches-demo.sh doc/generate-casts/generate-casts.sh; do
if [ -f "$f" ]; then
echo "--- $f ($(wc -l < "$f") lines) ---"
cat -n "$f"
else
echo "missing $f"
fi
done
echo "== search pe/pipefail/asciinema/rec =="
rg -n "^\s*set\s|-.*pipefail|pipefail|pe\(|asciinema rec|dfetch replay-patches|replay-patches\.cast" doc/generate-casts || true
echo "== shell behavioral probe for input pipeline masking status =="
cat >/tmp/probe-status-masking.sh <<'BASH'
set -o nounset
() { return 3; }
echo exit_status_of_pipe_status_in_input_pipeline=$( ( echo "" | false; echo $? ) )
BASH
bash /tmp/probe-status-masking.shRepository: dfetch-org/dfetch
Length of output: 7612
🏁 Script executed:
#!/bin/bash
set -u
echo "== demo-magic pe implementation =="
sed -n '110,150p' doc/generate-casts/demo-magic/demo-magic.sh
echo "== bash pipeline status probe =="
bash -c '
set -u
exit_status_of_pipe_status_in_input_pipeline=$( ( echo "" | false; echo $? ) )
printf "exit_status_of_pipe_status_in_input_pipeline=%s\n" "$exit_status_of_pipe_status_in_input_pipeline"
'
echo "== asciinema rec status behavior =="
python3 - <<'PY'
try:
import subprocess
result = subprocess.run(["asciinema", "rec", "--overwrite", "-c", "false"], capture_output=True, text=True)
print("asciinema_exists=true")
print("exit_code=", result.returncode)
print("stdout=", repr(result.stdout[:200]))
print("stderr=", repr(result.stderr[:200]))
except Exception as e:
print("asciinema_exists=false", type(e).__name__, str(e))
PY
echo "== status-masking summary via Python =="
python3 - <<'PY'
import subprocess, tempfile, os
def run(cmd):
return subprocess.run(["bash", "-c", cmd], capture_output=True, text=True).returncode
checks = []
for cmd in [
"set -o pipefail; ( echo '' | bash -c 'exit 3' ); echo '$?' | sed 's/exit_status_of_pipe_status_in_input_pipeline=//'",
"set +o pipefail; ( echo '' | bash -c 'exit 3' ); echo '$?' | sed 's/exit_status_of_pipe_status_in_input_pipeline=//'",
]:
tmp = tempfile.NamedTemporaryFile("w")
tmp.write(cmd + "\n")
tmp.flush()
print("run:", cmd)
print(run(tmp.name))
tmp.close()
# Shell pipeline rule: left-to-right commands, status is status of last component.
print("$? for echo '' | false:")
print(run("exit_status_of_pipe_status_in_input_pipeline=$( ( echo '' | false; echo $? ) ) ; echo \"$exit_status_of_pipe_status_in_input_pipeline\""))
PYRepository: dfetch-org/dfetch
Length of output: 1348
🌐 Web query:
bash manual pipeline exit status last command pipefail set -o pipefail
💡 Result:
In Bash, the exit status of a pipeline is determined by the behavior of its components and the shell's pipefail configuration [1][2]. Default Behavior (pipefail disabled) By default, the exit status of a pipeline is the exit status of the last (rightmost) command in the pipeline [1][3]. The status of any preceding commands is ignored by the pipeline's overall exit code [3]. With pipefail Enabled When the pipefail option is enabled using the command set -o pipefail, the pipeline's exit status changes to the value of the last (rightmost) command to exit with a non-zero status [1][4]. If all commands in the pipeline exit successfully (with a status of zero), the pipeline's exit status is zero [1][2]. Enabling and Disabling pipefail You can control this behavior using the set builtin: - Enable: set -o pipefail [3] - Disable: set +o pipefail [3] Accessing Individual Exit Statuses If you need to check the exit status of individual commands within a pipeline regardless of the pipefail setting, you can use the PIPESTATUS array variable [3]. This array stores the exit statuses of each command in the most recently executed foreground pipeline [3]. Note that if the reserved word! precedes a pipeline, the final exit status of the entire pipeline is logically negated [1][5]. Regardless of the pipefail setting, the shell will wait for all commands in the pipeline to terminate before returning the final exit status [1][2].
Citations:
- 1: https://www.gnu.org/software/bash/manual/html_node/Pipelines.html
- 2: https://bash.chrootid.com/Pipelines.html
- 3: https://www.baeldung.com/linux/exit-status-piped-processes
- 4: https://durak.org/sean/pubs/software/bash-4.3.30/bashref_7.html
- 5: https://cl72.org/130gnuOS/bash.pdf
🌐 Web query:
asciinema rec return code failure -c command fails
💡 Result:
By default, the asciinema rec command does not propagate the exit status code of the recorded session; it typically exits with status 0 regardless of the outcome of the command being recorded [1][2]. To change this behavior and ensure that asciinema exits with the same status code as the recorded session (particularly when the command fails), you should use the --return option [1][2]. When --return is enabled: If the recorded command terminates with a non-zero exit status, asciinema will propagate that status and exit with the same code [1][2]. This is specifically designed for scripted or CI/CD environments where you need to detect failures in recorded terminal sessions [3][1]. For versions of asciinema that support the asciicast v3 format, the exit status is also saved as a final event (marked as an "x" event) within the resulting cast file [1]. If you are encountering issues where the command appears to fail but you are not seeing the expected exit code, ensure you are using the --return flag and are on a compatible version of the CLI [1][2].
Citations:
- 1: return value asciinema/asciinema#599
- 2: https://www.mankier.com/1/asciinema-record
- 3: https://github.com/asciinema/asciinema?tab=readme-ov-file
Propagate demo and recording failures so generation does not produce successful-looking casts.
doc/generate-casts/replay-patches-demo.sh:47: pipefail or check the pipeddfetch replay-patchesstatus before allowing the script to succeed.doc/generate-casts/generate-casts.sh:31and otherasciinema reclines: useasciinema rec --return -c ...or an equivalent exit-status check; otherwise a failed recorded command can still let generation continue and strip a broken cast.
📍 Affects 2 files
doc/generate-casts/replay-patches-demo.sh#L47-L47(this comment)doc/generate-casts/generate-casts.sh#L31-L31
🤖 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/generate-casts/replay-patches-demo.sh` at line 47, Propagate failures in
the cast-generation scripts: update the piped dfetch replay-patches command in
replay-patches-demo.sh to honor pipe failures, and update every asciinema rec
invocation in generate-casts.sh, including the command near line 31, to return
or explicitly check the recorded command’s exit status before continuing or
stripping casts.
Comment on lines
+157
to
+164
| "update-patch, format-patch, replay-patches, freeze, import, init, report, " | ||
| "validate, environment. " | ||
| "Invokes Git and SVN as subprocesses (``shell=False``, list args). " | ||
| "Extracts archives with decompression-bomb limits and path-traversal checks." | ||
| "Extracts archives with decompression-bomb limits and path-traversal checks. " | ||
| "``replay-patches`` transiently stages the clean upstream in the git index " | ||
| "(``git add -- <path>``) and restores it on exit (``git restore --staged -- <path>``); " | ||
| "interruption before the finally block completes may leave the index temporarily " | ||
| "modified, which is visible to concurrent git operations in the same working tree." |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve the user’s original index state before documenting restoration.
The supplied implementation stages the path with git add, then restore_staged() resets that path’s index entry to HEAD; it does not restore any pre-existing staged changes. This can discard the user’s staged work, contradicting the PR objective that the original index state is restored. Snapshot and restore the original index state, or explicitly reject/report paths with staged changes. Also, termination before finally completes can leave the index persistently modified, not merely “temporarily” modified.
Based on the supplied replay_patches.py and dfetch/vcs/git.py implementations.
🤖 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 `@security/tm_usage.py` around lines 157 - 164, Update the replay-patches
staging and cleanup flow, including restore_staged(), to preserve each path’s
original Git index state rather than resetting it unconditionally to HEAD.
Snapshot and restore pre-existing staged entries, or explicitly reject/report
paths that are already staged, and revise the surrounding documentation to state
that termination before finally completes can leave the index persistently
modified.
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
3 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.
Introduces
dfetch replay-patchwhich stages the clean upstream source inthe git index and applies the selected patches to the working tree, so any
diff-aware editor sees
git diff(working tree vs index) showing exactlywhat the patches contribute. The command always restores original state on
exit — no permanent changes to working tree or index.
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_017zY8BoH65KBX6cz7Pm8aeF
Summary by CodeRabbit
New Features
dfetch replay-patchescommand for inspecting patch contributions, including interactive single-project and multi-project review.--countand project-specificname:Nselection.Bug Fixes
Documentation
Tests