Review Workflows

- about process and tooling on top of VCS.

WIP, notes based on discussion at the 3.8 sprint.

What is it / possible goals

get (early) feedback to changes
knowledge sharing - inform others about changes
catch bugs and issues before landing changes in mainline
let area owners maintain ownership while changed by others
collect approvals (human and automatic) and enforce/support change approval workflow
coordinate and automate landing of changes
clean history while landing - make/ensure it is bisectable

We identify some main areas where there can be different solutions:

- how the changes are submitted:

(temporary) fork & push & Pull Request working on live diff
shared repo push to (temporary?) branches & Pull Request working on live diff
upload patch(es) & Integration Request (thus some history cleaning)
can be submitted as all-or-nothing "dirty" batch or as a sequence of clean changes ordered by stability/risk and dependencies

- how the changes are shown to reviewers:

Unified diffs or side-by-side
Plain text or with syntax highlighting
- diff/word-diff highlighting, language highlighting
Semantic information/markup/hyperlinking
Individual changesets or aggregate diff
Conflict warnings/merge diffs
Aggregate diff since ancestor or diff between heads
Trailing whitespace/code style rules
Sensible diffs for non-plaintext file types (images, GPG keyrings, etc.)
Web/e-mail/native client
- define a standard for review system client plugins?
- Be able to review within your favorite editor/GUI SCM client?

- how reviewers can communicate with author:

forum style async communication centered around a line of change, a changeset or the whole pull request
- web click'n'comment
- mail inline replies
- local native client comments
post comments 1 by 1 or batch them (like mail might have several inline replies)
chat style real time discussion - inline in system or link to external chat and archive
constructive contribution, show don't tell, make changes instead of asking for them (using existing process of modifying history or adding changesets, thus effectively iterating)

- how changes might change during the process:

show meta diff between iterations
static or moving target
extra commits with changes
history modification
but how to show comments from old iteration in new iteration where the original lines might have moved, be hard to find, or have gone completely
- heuristics are tough to make accurate
  - fuzzy match diff context around comment?
- use some sort of annotation mechanism to track new location?
- tracking built in to vcs and moved around as source changes?
new iteration might invalidate reviews / approvals

- how reviewers can control the propagation of the changes:

some approval workflow can be specified and be more or less mandatory/enforced
possibly formalized ownership of touched files, suggested or added/informed automatically
different kind of reviews and approvals - architectural, ownership, catch bugs, full or partial, FYI/ACK, stylistic review, product management, release management, CI
may or may not be invalidated by iteration

- actual landing of the changes:

How different tools and implementations relate to this nomenclature / frame of reference:

Submit: mainly patchbombs, automated with push gateway
Show: patch emails to mailing list, also snarfed by patchwork
Comment: inline replies on mailing list
Approve: trusted people collect patches from mailing list, read informal responses, reject bad parts, might tweak, approve good parts by pushing to pre-landing repo
Land: pre-landed changesets are (currently) re-reviewed, rebased, tweaked and pulled upstream by mpm
Iterate: rejected parts must be "history modified" and re-sent to the mailing list tagged v(n+1)

Submit: repo hosting, fork or shared, PR on live diff
Show: web with unified diff (partly side-by-side), mail listing commit descriptions, notifications on iterations
Comment: web inline comments, notifications
Approve: reviewers are assigned manually - no automatic landing and thus no enforcement
Land: manual offline, shows hint about what to pull and merge
Iterate: static view until iterating (closes old PR and creates new one with additional changes - no good support for modifications)

Tweaked implementation of Kallithea

Submit: shared repo with named branches
Approve: automatic recommentation/review of assigned revieweres based on touched files plus release management plus "auto land request", CI status shows up too
Land: "trunk guardians" are gate keepers and make final meta review, prepare batch merging all possible PRs to verification branch, runs all tests, bisect failures, merge good parts to mainline, bounce PRs causing conflicts or failures
Iterate: append-only history - not clean, only bisectable on mainline, new iteration pretty much invalidates previous approvals ... but negotiable with guardians