Skip to content

Commit e29ae3d

Browse files
committed
Address CodeRabbit's latest review round
- _stage_one: KeyboardInterrupt derives from BaseException, so the prior `except Exception` skipped restoration on Ctrl-C during staging. Switch to try/finally with a staged_ok flag so restoration always runs on any failure, including Ctrl-C. - Fix a misplaced quote in the "no patch file" warning so the project name lands inside the quoted `dfetch diff` command, matching update_patch.py. - Catch OSError alongside RuntimeError when stepping the interactive TUI (single- and multi-project) so a patch file that goes missing mid-review doesn't leave the terminal stuck in raw mode. - interactive_helper.py: reject trailing tokens after REPEAT instead of silently ignoring them; terminate the driven dfetch process if _drive raises instead of leaking it. - Demo scripts: resolve demo-magic.sh and workspace paths from the script's own directory (BASH_SOURCE) instead of the caller's cwd; use a unique mktemp workspace instead of a fixed directory name so a pre-existing directory of that name can't collide with the cleanup trap's rm -rf; preserve the replay command's exit status through cleanup. - Remove an unnecessary inline pylint suppression in svn_steps.py (other step files import `then` from behave without it). - Clarify that replay-patches stages "eligible selected projects" (skips ones with no patches or local changes), not unconditionally all of them. Verified: all 724 unit tests, the git and SVN replay-patches BDD scenarios, and both demo scripts end-to-end (exit 0, no leftover directories) still pass; ruff/pylint/mypy/pydocstyle/bandit clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A7miizBGMoLEXXZxuq6XUJ
1 parent 3d4057d commit e29ae3d

7 files changed

Lines changed: 44 additions & 34 deletions

File tree

dfetch/commands/replay_patches.py

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -265,8 +265,8 @@ def _can_review_project(
265265
if not subproject.patch:
266266
logger.print_warning_line(
267267
project_name,
268-
'skipped - there is no patch file, use "dfetch diff"'
269-
f" {project_name} to create one",
268+
f'skipped - there is no patch file, use "dfetch diff {project_name}"'
269+
" to create one",
270270
)
271271
return False
272272
if not subproject.on_disk_version():
@@ -361,6 +361,7 @@ def _stage_one(
361361
def _ignored() -> list[str]:
362362
return list(superproject.ignored_files(project.destination))
363363

364+
staged_ok = False
364365
try:
365366
subproject.update(
366367
force=True,
@@ -370,14 +371,15 @@ def _ignored() -> list[str]:
370371
)
371372
if git_super is not None:
372373
git_super.add_path(subproject.local_path)
373-
except Exception:
374-
try:
375-
_restore_project(
376-
superproject, git_super, subproject, project.name, False, _ignored
377-
)
378-
finally:
379-
Path(subproject.metadata_path).write_bytes(saved_metadata)
380-
raise
374+
staged_ok = True
375+
finally:
376+
if not staged_ok:
377+
try:
378+
_restore_project(
379+
superproject, git_super, subproject, project.name, False, _ignored
380+
)
381+
finally:
382+
Path(subproject.metadata_path).write_bytes(saved_metadata)
381383
state = _ProjectState(
382384
name=project.name,
383385
local_path=subproject.local_path,
@@ -540,7 +542,7 @@ def _step_tui(patches: list[str], local_path: str, project_name: str) -> None:
540542
raise
541543
try:
542544
current, done = _apply_step(key, current, total, patches, local_path)
543-
except RuntimeError:
545+
except (RuntimeError, OSError):
544546
screen.clear()
545547
raise
546548
if done:
@@ -606,7 +608,7 @@ def _step_tui_multi(states: list[_ProjectState]) -> None:
606608
raise
607609
try:
608610
focused, done = _handle_tui_multi_key(key, focused, states)
609-
except RuntimeError:
611+
except (RuntimeError, OSError):
610612
screen.clear()
611613
raise
612614
if done:

doc/generate-casts/interactive_helper.py

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,9 @@ def _parse_line(tokens: list[str], lineno: int) -> list[Keystroke]:
116116
if pos < len(tokens):
117117
if tokens[pos] != "REPEAT":
118118
raise ValueError("expected REPEAT")
119-
repeat = int(tokens[pos + 1])
119+
repeat, pos = int(tokens[pos + 1]), pos + 2
120+
if pos != len(tokens):
121+
raise ValueError("unexpected trailing tokens")
120122
except (IndexError, ValueError) as exc:
121123
raise ValueError(f"line {lineno}: malformed keystroke: {tokens!r}") from exc
122124

@@ -216,7 +218,11 @@ def _drive(child: pexpect.spawn, keystrokes: list[Keystroke]) -> None:
216218
)
217219
dfetch_child.logfile_read = sys.stdout
218220

219-
_drive(dfetch_child, keystrokes)
220-
dfetch_child.expect(pexpect.EOF)
221+
try:
222+
_drive(dfetch_child, keystrokes)
223+
dfetch_child.expect(pexpect.EOF)
224+
finally:
225+
if dfetch_child.isalive():
226+
dfetch_child.terminate(force=True)
221227
dfetch_child.close()
222228
sys.exit(dfetch_child.exitstatus or 0)

doc/generate-casts/replay-patches-demo.sh

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,16 @@
11
#!/usr/bin/env bash
22

3-
source ./demo-magic/demo-magic.sh
3+
DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
4+
source "$DIR/demo-magic/demo-magic.sh"
45

56
PROMPT_TIMEOUT=1
67

7-
# Copy example manifest
8-
mkdir review-patch
9-
pushd review-patch || exit 1
8+
WORKDIR="$(mktemp -d "$DIR/review-patch.XXXXXX")"
9+
trap 'popd 2>/dev/null; rm -rf "$WORKDIR"' EXIT
10+
pushd "$WORKDIR" || { echo 'pushd failed' >&2; exit 1; }
1011

1112
git init
12-
cp -r ../update/* .
13+
cp -r "$DIR/update"/* .
1314
git add .
1415
git commit -m "Initial commit"
1516

@@ -47,11 +48,11 @@ pe "cat patches/cpputest.patch"
4748
# the scenes so recording doesn't block on "Press Enter to restore..."
4849
p "dfetch replay-patches cpputest"
4950
echo '' | dfetch replay-patches cpputest
51+
status=$?
5052

5153
PROMPT_TIMEOUT=3
5254
wait
5355

5456
pei ""
5557

56-
popd || exit 1
57-
rm -rf review-patch
58+
exit "$status"

doc/generate-casts/replay-patches-multi-demo.sh

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,12 @@ source "$DIR/demo-magic/demo-magic.sh"
1010

1111
PROMPT_TIMEOUT=1
1212

13-
mkdir replay-patches-multi
14-
trap 'popd 2>/dev/null; rm -rf replay-patches-multi' EXIT
15-
pushd replay-patches-multi || { echo 'pushd failed' >&2; exit 1; }
13+
WORKDIR="$(mktemp -d "$DIR/replay-patches-multi.XXXXXX")"
14+
trap 'popd 2>/dev/null; rm -rf "$WORKDIR"' EXIT
15+
pushd "$WORKDIR" || { echo 'pushd failed' >&2; exit 1; }
1616

1717
git init
18-
cp -r ../update/* .
18+
cp -r "$DIR/update"/* .
1919
git add .
2020
git commit -m "Initial commit"
2121

@@ -69,12 +69,12 @@ KEYSTROKES
6969
)
7070

7171
p "dfetch replay-patches --interactive cpputest jsmn"
72-
echo "$KEYSTROKES" | python3 ../interactive_helper.py replay-patches --interactive cpputest jsmn
72+
echo "$KEYSTROKES" | python3 "$DIR/interactive_helper.py" replay-patches --interactive cpputest jsmn
73+
status=$?
7374

7475
PROMPT_TIMEOUT=3
7576
wait
7677

7778
pei ""
7879

79-
popd || { echo 'popd failed' >&2; exit 1; }
80-
rm -rf replay-patches-multi
80+
exit "$status"

doc/howto/patching.rst

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -319,9 +319,10 @@ un-applied:
319319
**Replaying multiple projects at once**
320320

321321
When you call ``replay-patches`` with two or more project names (or with no
322-
names to select all), *dfetch* stages all of them together and presents a single
323-
pause. You can limit the patches applied to a specific project with the
324-
``name:N`` shorthand:
322+
names to select all), *dfetch* stages all eligible selected projects together
323+
and presents a single pause. Projects that have no patches or can't be safely
324+
replayed are skipped with a warning. You can limit the patches applied to a
325+
specific project with the ``name:N`` shorthand:
325326

326327
.. code-block:: console
327328

features/replay-patches-in-git.feature

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ Feature: Replay patches in git
8282
"""
8383
Dfetch (0.14.0)
8484
SomeProject:
85-
> skipped - there is no patch file, use "dfetch diff" SomeProject to create one
85+
> skipped - there is no patch file, use "dfetch diff SomeProject" to create one
8686
"""
8787

8888
Scenario: A warning is shown when the project has uncommitted local changes

features/steps/svn_steps.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
import pathlib
88
import subprocess
99

10-
from behave import given, then # pylint: disable=no-name-in-module
10+
from behave import given, then
1111

1212
from dfetch.util.util import in_directory
1313
from features.steps.generic_steps import call_command, extend_file, generate_file

0 commit comments

Comments
 (0)