Review Workflows

- enumeration of key concepts related to development process and tooling on top of VCS.

WIP, notes based on discussion at the 3.8 sprint.

Purpose

- what do we mean when we talk about review workflows and what do we hope to get out of it:

• Get (early) feedback to changes - leverage other developer's experience
• Knowledge sharing - inform others about changes
• Catch bugs and issues in changes before landing them in mainline (rare but valuable)
• Let area owners maintain ownership while the area is changed by others
• Enforce and support a formal process to make sure best practice actually is followed - collect and check human and automatic approvals
• Coordinate and automate the actual landing of changes to the main line
• Ensure consistent and possibly clean version control history while landing - prepare for actual use of history with bisect and annotate

Nomenclature

- we identify some general areas that can be observed in most review workflows and we investigate what's in it:

Submit

- how the changes are submitted:

• Push - deep integration with repo hosting
  • Repository - where to push
    • Push to the main repo (on branches?)
    • Push to some shared "try"/"review"/"draft" fork
    • Push to personal (temporary) fork
  • Branches - how to tell incoming changes apart
    • Follows from using separate fork
    • Using temporary branch (named or bookmark/git) (rebase and/or git/bookmark merge will discard it)
    • Using permanent branch (named branch, merged not rebased)
  • Create a "Pull request" on data in the repo (usually misleading name - better named as "Merge request", "Integration request", "Review request", "Change request" ... or just as a "Series" or "Changeset set" that can be RFC, reviewed, approved, grafted, merged, rebased, etc)
  • Diff and annotate info can come from live repo data
  • Iterations can be submitted by pushing additional changes on top or with history modification (possibly using obsolescence to chain iterations)
• Patch submission - review pretty much works on a plain diff
  • Transport:
    • Upload diff to server using something like a web interface
    • Mail - probably patchbomb to mailing list
    • Push gate can make anything look like the repo push model above
    • Deep VCS integration can make anything look like anything
  • Will inherently lean towards rebasing and not using branches
• Series - submitting more than one change at once
  • Dirty history
    • Probably append only history, no history revisionism
    • Only the end result matters - don't look so much at individual changes but take all or nothing
    • Intent of either merging dirty history or collapsing (squashing) history
  • Clean history
    • History has been cleaned and is a product too
    • Each change is self contained and leaves a perfectly fine and releasable state but might depend on other changes before it
    • Changes are ordered by maturity/risk so they can be taken one by one or as far as everybody agrees and the later ones
• Local future of submitted changesets
  • Submitted changes might be considered "published" and permanent and will be merged
  • Submitted changes might be considered "draft" and will not be applied upstream as is - the submitted changeset was temporary and should disappear

Show

- how the changes are shown to reviewers:

• Visualization of changes:
  • Unified diffs
  • Side-by-side - changed chunks or whole file
  • Full file or chunks with context
  • Diff / word diff highlighting
  • Language syntax highlighting
  • Semantic information/markup/hyperlinking
  • Whitespace (tab/space indentation and trailing) and other code style violations
  • Sensible diffs for non-plaintext file types (images, GPG keyrings, archives, documents, etc.)
• Granularity:
  • Show individual changesets - assuming clean history
    • Aggregate diff
    • Diff from common ancestor
    • Diff between heads (possibly constrained to files ... but still not optimal)
  • Diff of merge preview (with conflict warnings)
  • Iterations ... somehow whatever that means
• Media - how/where the change is shown:
  • Web - interactive
  • E-mail - static and using email client threading
  • Native client
    • Editor
      • So many - define a plugin standard for review system client plugins?

    • VCS GUI tool (TortoiseHg)

Comment / communicate

- how reviewers can communicate with author:

• Forum style
  • Async communication
  • Topic/location of discussion
    • a line of change
      • in changeset
      • in aggregated view
      • in aggregated view making/aggregating comments on changesets
    • a changeset or
    • the whole "pull request"
  • Input method
    • web inline click'n'comment
    • mail inline replies
    • local native client comments
  • Comments can be posted 1 by 1 or batched (like mail might have several inline replies)
• Chat style real time discussion
  • inline in system
  • link to external chat and archive
• Constructive contribution, collaboration, show don't tell, make changes instead of asking for them (using existing process of modifying history or adding changesets, thus effectively iterating)

Iterate

- how changes might change during the process:

• Author or collaborators might want to change the changes, possibly addressing feedback from the review process, looping back to earlier state in the process
• How to iterate
  • Change history - modify changesets - something evolve-ish might be able to integrate deeply with the review process
  • Append only - make extra commits with changes ... and thus dirty history
• Changing the subject of the review
  • the review might be a moving target and move forward as the source branch moves
  • the review might be a moving target and move "backwards" as the target branch moves (when using merge preview)
  • the review might be static - changing it might essentially require starting over and creating a new review
• Visualizing the iteration
  • Somehow show meta diff between iterations
    • easy if just adding changesets
    • harder when merging target branch
    • even harder if rebasing or modifying history
  • Comments relevance
    • Might still apply if not addressed by the iteration
    • Might still apply but location might have changed and be hard/impossible fo find
    • Issue might have been addressed and "issue has been resolved" ... but should still be around for review and approval and history
  • Comment location
    • Simple line numbers will generally change
    • Use diff context and use fuzzy mathc like "patch" does to find the location of the comment
    • Use some sort of annotate tracing from old location to track new location
    • Put comments in the source itself (assuming something like evolve to propagate it)
    • Make comments a first class component in the VCS and handle them as code changes
• Consequence of hanging the content/subject of the "review"
  • Might move the process back to square one
  • Existing reviews and approvals might be invalidated - possibly depending on what the iteration changed

Approve

- how reviewers can control the propagation of the changes:

• Organizations might have different approval workflows
  • can be formal or informal
  • can be fully automated and enforced
  • can just be "encouraging" and trust based
• Areas (files and patterns) can have owners
  • Owner approval might be required
  • Owner approval might be suggested
  • Owners might be added or informed automatically - perhaps even early on before asking for review, perhaps on push
• There can be different kind of reviews and approvals
  • Owner of changed areas approves of the change
    • Review for bugs and other issues
    • Full review or only some/owned areas / aspects
    • FYI - ACK seen
  • Stylistic review
  • Technology usage review
  • Architectural approval
  • Product management approval
  • Release management approval
  • Documentaion ready to ship
  • CI and automatic verification passed
  • QA approval ready to ship
• Approvals may or may not be invalidated by iteration

Land

- actual landing of the changes:

• Trigger
  • Automatic when all gates passes
  • "merge button" is pressed manually - possibly when allowed by gates
  • Done manually with local VCS commands
• How
  • Merge changes
  • Rebase/graft changes - create linear history
  • Collapse change - create linear history
• Scope - depending on the intent, standard procedure and cleanliness
  • land everything that has been reviewed
  • partially landing a series, probably from the beginning, while continuing iterating on the remaining parts

Tools and implementations

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

Mercurial project

• 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)

Kallithea

• 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)

Unity Technologies

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

CategoryDeveloper