Skip to content

Pull Requests

Rolf Eike Beer edited this page Sep 6, 2019 · 8 revisions

A pull request is ready when it...

  • Solves an agreed problem, in an agreed way, in an agreed sequence
  • Describes the costs, risks, and benefits it addresses and/or introduces
  • Serves our goals, without broaching any of our non-goals
    • (Rare but not impossible: adjusting our goals instead of the proposed patch)
  • Includes a very brief description in CHANGES
  • Adds any new object (or otherwise generated) files to TARGETS
  • Builds in TravisCI and CirrusCI
  • Matches the conventions of surrounding code, where we're still trying to do that
  • Uses new and improved conventions, where we're trying to do that (see below about what that means)
  • Has at least two reviewers' approval, and no significant objections

We'd like to further require...

  • Includes tests
  • Passes tests
  • Passes static analysis
  • Passes fuzzing
  • Architecture changes (if any) do not break the security model

To iterate on a PR, we...

  • Respond to each piece of feedback
  • Squash commits into the sequence we'd want merged to master
  • Force-push to the feature branch

To merge a PR, we...

  • Click the popup menu for "Merge pull request"
  • Choose "Rebase and merge"

A note on "surrounding code" and conventions

  • if a single-line function declaration (i.e. name and all types on the same line) is changed then convert it to C89
  • if a function declaration in a header is changed, and the declaration in the .c file either is C89 already or is converted in the same patch by the above rule, then switch the one in the header too (do NOT do that if it would drag in additional headers to the .h file to get the types to avoid more disruptive changes)
  • use uid_t and gid_t where appropriate
You can’t perform that action at this time.