[PATCH b4] review: Send replies for skipped patches

[Mark Brown <broonie@kernel.org>]

Currently when a patch is skipped this includes for email mode, even if
a reply has been written.  This seems unhelpful, if the user has written
something it seems reasonable they would want to send it and if they
want to not send a reply they can always delete it.  Skipping also
affects patch application and one reason a user might reply to a patch
is to explain why they are not taking it.

Change the behaviour so that replies are also sent for skipped patches.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 docs/maintainer/review.rst       |  5 +--
 src/b4/review/_review.py         | 12 +++----
 src/b4/review_tui/_review_app.py | 22 +++---------
 src/tests/test_review.py         | 20 +++++------
 src/tests/test_tui_review.py     | 77 +++++++++++++++++++++++++++++++++++++++-
 5 files changed, 96 insertions(+), 40 deletions(-)

diff --git a/docs/maintainer/review.rst b/docs/maintainer/review.rst
index 5a3e9d9..78a1108 100644
--- a/docs/maintainer/review.rst
+++ b/docs/maintainer/review.rst
@@ -760,7 +760,7 @@ Unchanged   ``≡``   —       Patch identical to prior revision (auto, on upgr
 External    ``±``   —       External reviewers have inline comments (auto)
 Draft       ``✎``   —       You have comments, a reply, or a nack trailer (auto)
 Done        ``✓``   ``d``   Review complete, include in outgoing emails (also auto)
-Skip        ``✕``   ``x``   Intentionally skipped, exclude from outgoing emails
+Skip        ``✕``   ``x``   Intentionally skipped; any replies are still sent
 ==========  ======  ======  =======================================================
 
 .. |sp| unicode:: U+0020
@@ -788,7 +788,8 @@ bold, skipped and unchanged patches appear dimmed. Press ``H`` to hide
 skipped patches from the list entirely — useful for large series where
 many patches are irrelevant to your review.
 
-Skipped patches are automatically excluded when sending review emails.
+Any unsent replies, comments, or trailers on skipped patches are still
+included when previewing and sending review emails.
 When taking patches via cherry-pick, skipped patches are pre-deselected
 in the patch selection dialog.
 
diff --git a/src/b4/review/_review.py b/src/b4/review/_review.py
index b69a7f0..3e1027e 100644
--- a/src/b4/review/_review.py
+++ b/src/b4/review/_review.py
@@ -3914,8 +3914,8 @@ def collect_review_emails(
 ) -> List[email.message.EmailMessage]:
     """Collect review emails to send for the given series.
 
-    Only sends the maintainer's own reviews, not agent or other
-    reviewers' data.
+    Only sends the maintainer's own unsent reviews, not agent or other
+    reviewers' data. Skipped patches can still have replies to send.
     """
     usercfg = b4.get_user_config()
     my_email = str(usercfg.get('email', 'unknown@example.com'))
@@ -3923,11 +3923,7 @@ def collect_review_emails(
 
     # Cover letter review (maintainer only)
     cover_review = series.get('reviews', {}).get(my_email, {})
-    if (
-        cover_review
-        and cover_review.get('patch-state') != 'skip'
-        and cover_review.get('sent-revision') is None
-    ):
+    if cover_review and cover_review.get('sent-revision') is None:
         msg = _build_review_email(series, None, cover_review, cover_text, topdir, None)
         if msg is not None:
             msgs.append(msg)
@@ -3935,7 +3931,7 @@ def collect_review_emails(
     # Per-patch reviews (maintainer only)
     for idx, patch_meta in enumerate(patches):
         patch_review = patch_meta.get('reviews', {}).get(my_email, {})
-        if not patch_review or patch_review.get('patch-state') == 'skip':
+        if not patch_review:
             continue
         if patch_review.get('sent-revision') is not None:
             continue
diff --git a/src/b4/review_tui/_review_app.py b/src/b4/review_tui/_review_app.py
index 1e70bc6..2eec9b5 100644
--- a/src/b4/review_tui/_review_app.py
+++ b/src/b4/review_tui/_review_app.py
@@ -857,22 +857,6 @@ class ReviewApp(LoreNodeShutdownMixin, CheckRunnerMixin, App[None]):
                 else None
             )
 
-        target = self._series if display_idx == 0 else patch_meta
-        if target and b4.review._get_patch_state(target, self._usercfg) == 'skip':
-            total = len(self._commit_shas)
-            label = 'cover' if display_idx == 0 else f'{display_idx}/{total}'
-            my_review = b4.review._get_my_review(target, self._usercfg)
-            skip_reason = str(my_review.get('skip-reason', ''))
-            if skip_reason:
-                viewer.write(
-                    f'[dim]Patch {label} is marked as skipped: {skip_reason}[/dim]'
-                )
-            else:
-                viewer.write(
-                    f'[dim]Patch {label} is marked as skipped — no email will be sent.[/dim]'
-                )
-            return
-
         if not review or not (
             review.get('trailers') or review.get('reply', '') or review.get('comments')
         ):
@@ -1805,15 +1789,17 @@ class ReviewApp(LoreNodeShutdownMixin, CheckRunnerMixin, App[None]):
             try:
                 self._reply_sent = True
                 self._tracking['series']['status'] = 'replied'
-                # Stamp sent-revision on every non-skip review so that
+                # Stamp sent-revision on each review whose email was sent so that
                 # if this series is later upgraded to a newer revision,
                 # the upgrade step can detect which reviews were already
                 # sent and auto-skip the unchanged patches.
                 my_email = str(self._usercfg.get('email', 'unknown@example.com'))
                 current_rev = int(self._series.get('revision', 1))
+                replied_to = {msg['In-Reply-To'] for msg in msgs}
                 for target in [self._series] + list(self._patches):
                     review = target.get('reviews', {}).get(my_email, {})
-                    if review and review.get('patch-state') != 'skip':
+                    msgid = target.get('header-info', {}).get('msgid', '')
+                    if review and f'<{msgid}>' in replied_to:
                         review['sent-revision'] = current_rev
                 if not self._save_tracking():
                     # Silence here means the next session offers these reviews
diff --git a/src/tests/test_review.py b/src/tests/test_review.py
index e40dd35..0f22c00 100644
--- a/src/tests/test_review.py
+++ b/src/tests/test_review.py
@@ -4212,10 +4212,11 @@ class TestCollectReviewEmails:
             pytest.param(False, id='normal-patch-review'),
         ],
     )
+    @pytest.mark.parametrize('patch_state', ['', 'done', 'skip'])
     def test_sends_unsent_review(
-        self, _cfg: mock.Mock, _build: mock.Mock, on_cover: bool
+        self, _cfg: mock.Mock, _build: mock.Mock, on_cover: bool, patch_state: str
     ) -> None:
-        msgs = self._collect(on_cover, {})
+        msgs = self._collect(on_cover, {'patch-state': patch_state})
         assert len(msgs) == 1
 
     @pytest.mark.parametrize(
@@ -4225,10 +4226,8 @@ class TestCollectReviewEmails:
             pytest.param(True, {'sent-revision': 1}, id='cover-with-sent-revision'),
             pytest.param(False, {'sent-revision': 1}, id='patch-with-sent-revision'),
             # The upgrade step sets patch-state=skip AND skip-reason on
-            # unchanged patches whose review was already sent (combo A+B
-            # fix).  Both the skip filter and the sent-revision filter
-            # independently prevent re-sending; this row exercises the
-            # skip-state path.
+            # unchanged patches whose review was already sent. The
+            # sent-revision filter must still prevent re-sending them.
             pytest.param(
                 False,
                 {
@@ -4238,15 +4237,14 @@ class TestCollectReviewEmails:
                 },
                 id='patch-auto-skipped-after-upgrade',
             ),
-            # Explicit skip state (manually set, no sent-revision) is honoured.
             pytest.param(
-                False,
-                {'patch-state': 'skip'},
-                id='skip-state-without-sent-revision',
+                True,
+                {'sent-revision': 1, 'patch-state': 'skip'},
+                id='skipped-cover-with-sent-revision',
             ),
         ],
     )
-    def test_skips_already_sent_or_skipped(
+    def test_skips_already_sent(
         self,
         _cfg: mock.Mock,
         _build: mock.Mock,
diff --git a/src/tests/test_tui_review.py b/src/tests/test_tui_review.py
index e164bf6..6ccbfd9 100644
--- a/src/tests/test_tui_review.py
+++ b/src/tests/test_tui_review.py
@@ -21,7 +21,7 @@ import pytest
 
 pytest.importorskip('textual')
 
-from textual.widgets import ListView
+from textual.widgets import ListView, RichLog
 
 import b4
 import b4.review
@@ -690,6 +690,81 @@ class TestBranchRestore:
         assert app.branch_checked_out is False
 
 
+class TestSkippedReplies:
+    """Skipped patches can still have replies to preview and send."""
+
+    @pytest.mark.asyncio
+    @pytest.mark.parametrize('on_cover', [False, True], ids=['patch', 'cover'])
+    @pytest.mark.parametrize('dryrun', [False, True], ids=['send', 'dryrun'])
+    async def test_skipped_reply_is_previewed_and_sent(
+        self, gitdir: str, on_cover: bool, dryrun: bool
+    ) -> None:
+        import contextlib
+
+        branch, _shas = _create_review_branch_with_patches(
+            gitdir,
+            'skipped-reply',
+            ['patch 1', 'patch 2'],
+            series_extra={'header-info': {'msgid': 'cover@example.com'}},
+        )
+        session = _build_session(gitdir, branch)
+        session['email_dryrun'] = dryrun
+        my_email = str(session['usercfg']['email'])
+        target = session['series'] if on_cover else session['patches'][0]
+        reply = 'Please fix this before I can take it.'
+        target['reviews'] = {my_email: {'patch-state': 'skip', 'reply': reply}}
+        empty_target = session['patches'][1]
+        empty_target['reviews'] = {my_email: {'patch-state': 'skip'}}
+
+        app = ReviewApp(session)
+        async with app.run_test(size=(120, 30)) as pilot:
+            await pilot.pause()
+            app._show_content(0 if on_cover else 1)
+            await pilot.press('e')
+            await pilot.pause()
+            viewer = app.query_one('#diff-viewer', RichLog)
+            assert reply in '\n'.join(line.text for line in viewer.lines)
+
+            def _confirm(screen: Any, callback: Any = None) -> None:
+                assert callback is not None
+                callback(True)
+
+            with (
+                mock.patch.object(app, 'suspend', contextlib.nullcontext),
+                mock.patch.object(app, 'push_screen', _confirm),
+                mock.patch('b4.get_smtp', return_value=(None, 'sender@example.com')),
+                mock.patch('b4.send_mail', return_value=0 if dryrun else 1) as send,
+                mock.patch('b4.review_tui._review_app.mark_outgoing_seen'),
+                mock.patch.object(app, '_mark_patches_answered'),
+            ):
+                app.action_send()
+                send.assert_called_once()
+                msgs = send.call_args.args[1]
+                assert len(msgs) == 1
+                assert msgs[0]['In-Reply-To'] == f'<{target["header-info"]["msgid"]}>'
+                assert reply in b4.LoreMessage.get_payload(msgs[0])[0]
+
+                _cover, saved = b4.review.load_tracking(gitdir, branch)
+                saved_target = saved['series'] if on_cover else saved['patches'][0]
+                saved_review = saved_target['reviews'][my_email]
+                assert saved_review['patch-state'] == 'skip'
+                assert 'sent-revision' not in saved['patches'][1]['reviews'][my_email]
+                if dryrun:
+                    assert 'sent-revision' not in saved_review
+                    unsent = b4.review.collect_review_emails(
+                        app._series,
+                        app._patches,
+                        app._cover_text,
+                        app._topdir,
+                        app._commit_shas,
+                    )
+                    assert len(unsent) == 1
+                else:
+                    assert saved_review['sent-revision'] == saved['series']['revision']
+                    app.action_send()
+                    send.assert_called_once()
+
+
 class TestSendBookkeeping:
     """Everything after the SMTP handoff is bookkeeping, and bookkeeping must
     never be able to claim the send failed."""

---
base-commit: ce4c1158edb292caca0ce4a6087e5edecc1b0371
change-id: 20261111-reply-skipped-325b3debc6f4

Best regards,
--  
Mark Brown <broonie@kernel.org>