Re: b4 review available in master

Mark Brown
2026-03-03T12:42:30Z

[-- Attachment #1: Type: text/plain, Size: 7578 bytes --]

On Tue, Mar 03, 2026 at 12:14:08AM -0500, Konstantin Ryabitsev wrote:
> On Sat, Feb 28, 2026 at 09:11:30PM +0000, Mark Brown wrote:

> > > - It'd be good to have a workflow for syncing the reviews between
> > > machines (with privacy!), for example I move between my laptop and
> > > desktop semi regularly.

> This was already kinda working, but it should work better now. "With privacy"
> is the complicated part, because we really-really don't want to deal with
> encryption and keys and all the stuff that comes with that. I recommend just
> having a private remote where you can push your review branches. It's not very
> well-documented, but you have a private space in
> git@gitolite.kernel.org:scratch/broonie/ -- don't let the name "scratch" spook
> you, it's just not replicated to git.kernel.org, that's all.

Yeah, pushing branches isn't a concern - it was more the database bit.

> > > - An integration with non-patchwork CI would be useful, perhaps this
> > > looks a bit like the agent stuff above? It's "hand off to an
> > > external tool and some time later see what the tool/a separate
> > > results collection tool had to say about things"?

> > Expanding on this a bit what I have ATM is effectively two operations
> > both of which take commit ranges, one to start testing and another which
> > will either say everything is OK or provide a list of outstanding issues
> > (one of which might be that it's still waiting for results, that'd
> > probably be useful to distinguish in UI).

> I'll ponder how to best do it. It's possible this may be best accomplished
> with a wrapper, but I've not given this much thought yet.

I'd expect to have to write a script - I was thinking that the agent
stuff you've got already is basically "shell out to a tool and have it
output results in a given format which we pull in", that seemed like an
OK shape for something like this.

> > - The initial import of the series and actions->rebase appear to not
> > allow the target branch to be specified, but taking does. It'd be
> > good if this were selectable consistently, and I suspect if a base
> > has been explicitly specified for a branch we'd want to default to
> > rebasing against that.

> We can now define review-target-branch, but I don't think it's wired into the
> "rebase" flow yet, just in the "take" flow. I'll make a note to check both.

I'd expect many people have at least fixes and -next branches in the
same git repo (I have that, times four for each subsystem I have) so
it'd need to be runtime selectable which branch to use.

> > - Some ability to either configure a list of branches to apply to or
> > just a recently used list of branches would be handy.
>
> The b4.review-target-branch is now settable. I think if you want to apply to
> different branches, the sanest approach is to use worktrees and set different
> git configs for them -- we'll pick whatever git says is the most relevant
> b4.review-target-branch setting for that worktree.

> > - It'd be good to have the ability to sort/filter by activity in the
> > series view ("What serieses have active discussion and should have
> > attention paid?", "Where's discussion died down and I should think
> > about applying?").

> The latest review UI does sort by activity. The series most recently updated
> will be at the top. You can run a full update with "u" to pull in the latest
> from lore.

Ah, OK - it wasn't clear if that was the date things were sent or the
date there was most reently activity.

> > - Adding signoffs and Link: tags to applied serieses doesn't seem to
> > work for me with the merge strategy. I think this is because it's
> > doing what I suggested above and reusing the commits that were being
> > used for review? Linear strategy does the right thing.

> This is "as designed" right now -- the Signed-off-by and Link trailers are
> applied to the merge commit and leave the individual commits untouched. It's a
> controversial take, I know. :)

> Would you prefer that we always applied Signed-off-by: and Link: to each
> individual commit even if we're doing a merge?

*Especially* if we're doing a merge, I think the general expectation is
that there will be a signoff from the person who committed the patch -
in -next we check every commit for both author and committer signoffs.
If we cherry pick and they get applied then that'd work out OK, but if
we merge and the unsigned commits end up in -next that'd be an issue.
Missing signoffs will also mess up with flows like cherry picking
patches back into stable, the merge would be long gone by the time the
commit shows up in stable.

> > - It feels like the reply and comment functionality isn't as joined up
> > as it could be. Adding a comment doesn't block replies, but if
> > you've replied it's not possible to comment further. I think I was
> > expecting that replies would be stored as a series of comments
> > injected at the point where the quoted text was interrupted and I'd
> > be free to switch between the two so long as I didn't edit the quoted
> > text. As it is you can comment but once you've been in reply mode it
> > is no longer possible to use the comment functionality. I do realise
> > that there is some fun with detecting that this is the case.

> There is more logic in the review UI now regarding comments and replies, but
> more work is needed to make it a more natural flow. In general, the idea is
> that you'll do inline comments first (c) and then tailor your reply with "r".
> However, both parts need polish.

FWIW I find I moderately often come up with a top level comment part way
through the reply and go back to the top of the reply to add it.

> > - At the minute cherry picking of individual patches can happen only at
> > merge time, it'd be useful to be able to do this during review. For
> > example, with multi-subsystem serieses (especially large cleanup
> > serieses that really shouldn't have been a single series in the first
> > place) it can be easier to discard irrelevant bits so you can
> > actually find the bit you want to look at.

> I'll push back on this a little simply because there's a lot that is keyed off
> the number of patches in the series. If we cherry-pick before we even review,
> this makes a lot of other functionality fragile. However, there is code now to
> mark patches as "skip" (with x) -- this should let you completely ignore them.
> When we "take" a series with ignored patches we will automatically default to
> the cherry-pick strategy and pre-unselect the ignored patches. Would that come
> kinda close to what you want?

Possibly? I'd need to try it. Some of these serieses are quite large
so even finding the relevant mails can be annoying.

> > - I edited a dummy reply to be To: another of my email addresses but
> > when I tried sending I got a mail To: broonie@kernel.org, Cc the new
> > address. Copying my main address is good for my mail archives, but
> > I'd have expected the To: to be what was configured. The metadata
> > looks good in the UI if I hit 'T' to edit To/Cc but it's wrong in
> > both the TUI and what actually gets sent. Quite possibly user error,
> > I didn't look too hard.

> I don't think I fixed this yet, but there's been churn in that code today. See
> if it's still doing that.

Yeah, still seems to be present.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]