This is a rough draft of a git based code review system. Requirements:
- Usable without a web interface (just git and some mails)
- Integration with CI tools
- minimal server-side requirements
- proposed changes are modeled as git branches. Review and improvements are made by committing to these branches.
- review status, and all other state, is tracked in the commit message, using lines starting with CR-<name>. State is updated during regular commits and using annotated tags.
- special code comments are used for inline discussions
- the git-cr command provides sub-commands for handling changes
- changes apply to target branches. The target branch would typically be "master", but could be any remote branch.
- changes are based on other changes, or directly on a target branch.
- changes have logical names (used as part of the branch name), somtimes called "topics". Two changes with the same name but different target branches should be considered logically equivalent, that is, doing the same thing to different versions of the code base.
- branch names are build from logical names as follows: a changes "full name" or "unique name" is composed of <owner>/<name>/for/<target>. The branch name is created be prepending "cr/" to the full name: cr/<owner>/<name>/for/<target>.
- changes can be refered to by just their logical name, if it is unique, or by <owner>/<name> - this then indicates a change for the master target branch (or the target branch of the current operation or discussion).
--- * -- * / ^ --- * -- * | / ^ [bar] -- * | ^ [foo] | [master]
Submitting a Change
To create a new change, use:
git cr new <name> [<target>]
This will create a new branch called cr/<owner>/<name>/for/<target>. Per default, <target> would be "master" (or whatever you configure it to be). If you are not currently on the head of the target branch, you will be prompted to confirm that you want to create the new change based on the current commit instead of the target branch.
An annotated tag (the change's root tag) is committed to the new branch, introducing basic meta-information into the commit message, namely:
CR-NAME: <name> CR-TARGET: <target> CR-BASE: <base> CR-BASE_COMMIT: <base commit> CR-OWNER: <email@example.com>
A change can be set to draft mode using the CR-DRAFT marker. This can be done manually, or automatically when creating the change, as per configuration.
Now, the desired modifications are made on that branch, using any number of commits (keep it small though, makes review quicker). A commit hook is used to pass all the CR-XXXX lines on from one commit to the next one, starting with the commit message of the initial tag on the branch.
Once the code is ready, the change can be submitted for review:
git cr submit [<reviewer>, ...]
This will add another annotated tag (the submit tag) to the branch, with some additional meta-information:
Some tag line about the change. Note that this should describe the whole change. [TBD: perhaps this should be defined when creating the change, and stored in the commit message of the initial tag. Or this meta-info should go into a .dotfile?] CR-NAME: <name> CR-TARGET: <target> CR-OWNER: <firstname.lastname@example.org> CR-REVIEWERS: <email@example.com> <firstname.lastname@example.org>
Of course, the list of reviewers can be manually edited in the commit message.
The branch is then immediately pushed to the server, which will send mail to the reviewers (if so configured). The server may require the submit tag to be signed by the submitter.
[foo/for/X] | v --- * -- * -- * / ^ ^ -- * -- | | ^ (root) (submit) | "base"
TBD: git cr squash; the latest veto, verify and approve-tags are kept, all other changes get squashed.
Reviewing a Change
The review a change, use:
git cr checkout <name> [<target>]
This checks out the branch called cr/<owner>/<name>/for/<target>. To see what the change does, we can look at the diff:
git cr show
This will show the change by displaying the submit tag's commit (which should be the branche's HEAD) message, and a diff between the branche's base and the HEAD. The branche's base will be on the target branch, or on another (parent) change's branch that is based on the target (more about that laster). Similarly, to see only the diff:
git cr show
To see the change's history, we can look at the log:
git cr log
This is much like the regular git log, but automatically limited to commits on the change's branch.
When an issue is found during review, it should be described in a special comment, like this:
/*! ! This is off by one, Kenobi! !*/
The exact syntax would vary with the type of file in question, to match the native comment syntax.
The reviewer would then submit the change again, for additional reviews or for improvement by the owner:
git cr submit
This works exactly like the submit command used earlier by the owner, creating a new submit tag. Any CR comments (the ones starting with a "!") present in the code are extracted and included in the commit message. The reviewer can give additional comments in the commit message. A simple verdict like "needs improvement" or "some open questions" or "looks fine to me" is encouraged. [TBD: how to repliably track who said what in the commit message? How to best manage/show diffs between commit messages?]
The change's owner and the other reviewers get notified by the server. The owner is expected to reply to comments and/or improve the code according to the comments. Any comments that habe been addressed should be removed (some could be converted to regular comments, for posterity).
[foo] | v --- * -- * -- * ------ * / ^ ^ ^ -- * -- | | | ^ (root) (submit) (submit) | "base"
If the reviewer is satisfied that the change is redy to be merged into the target branch, they can approve it:
git cr approve
This works much like git cr submit, creating a new submit tag, adding the follwing meta-information:
The server should require approval tags to be issued by a member of a list of trusted maintainers, and should require such tags to be signed. The approval is only valid for that exact tag, if subsequent commits are made, these are not considered automatically approved (and should not automatically get the CR-APPROVED line). That is, any subsequent commit will cause the change to lose the "approved" status.
The approve command validates that all necessary conditions are met:
- prompt for confirmation if the code still contains any unresolved "special" comments
- prompt for confirmation if the change is not currently verified (see #Verification).
- prompt for confirmation if the change is currently vetored (see Vetos)
- prompt for confirmation if the change is currently marked as a draft
[foo] | v --- * -- * -- * ------ * -------- * / ^ ^ ^ ^ -- * -- | | | | ^ (root) (submit) (submit) (approve) | "base"
If the reviewer determins that the change absolutely must not be merged in its present state, or objects to the change in general, they can veto it:
git cr veto
This works just like git cr approve, creating a new submit tag, but adding the follwing meta-information:
Simmilarly to approval tags, veto tags should be checked against a whitelist of trusted maintainers, and expected to be signed. A veto is "sticky": it is considered to apply to the whole change, including any later commits, unless a newer commit/tag without that CR-VETOED line and signed by a trusted maintainer exists. That is, a submitter cannot undo a veto by just submitting another commit.
[foo] | v --- * -- * -- * ------ * ------ * / ^ ^ ^ ^ -- * -- | | | | ^ (root) (submit) (submit) (veto) | "base"
A special kind of approval is verification: the assertion that the code passed some pre-defined suite of tests. An automated testing system can be added as a reviewer (manually or per default, as per configuration). When notified of a new submit, the testing agent would run the test, and create a new annotated tag with a report of the test run included in the commit message. In case the tests failed, this would be done with git cr submit, as before. If the tests passed, the change is marked as verified using:
git cr verify
This will create the submit tag with a special marker in the commit message:
Like "approved", the "verified" marker is volatile: it is only valid until new commits are made. And like "approved", it should be checked against a whitelist of trusted maintainers, and expected to be signed.
[foo] | v --- * -- * -- * ------ * ------ * / ^ ^ ^ ^ -- * -- | | | | ^ (root) (submit) (submit) (verified) | "base"
Applying Approved Changes
Changes can be merged into the target branch either manually by an authorized submitter, or automatically once approved:
git cr apply <name> [<target>]
This will attempt to merge <name> into <target> (given the branch cr/<owner>/<name>/for/<target> exists), if the change is mergeable. For a change to be mergeable, the following must be true:
- the changes base must be either the target branch, or the base branch must have been merged into the target branch.
- the branch must apply cleanly to the target branche's current head.
- all external dependencies must be satisfied (merged).
- the change must not be in DRAFT state.
- the change must be in "approved" state (no commits after the last approval tag)
- the change must not be in "vetoed" state (there must be a signed approval tag without the veto marker) [git cr approve already enforces this, but it has to be double-checked here]
- (if so configured) the change must be in "verified" state (no commits after the last verification tag)
If these conditions are met, the change is merged by:
- optionally, running another round of ("gateway") tests [TBD: perhaps the apply command itself shouldn't run the gateway testing, this should be done by the automation framework before calling git cr apply].
- squashing the change's branch into a single commit (preserving the description of the change and the newest CR-APPROVED and CR-VERIFIED markers in the commit message).
- merging it onto the target branche's head.
- reset the change's branch to point to the new merge commit just created o nthe target branch.
The the conditions are not met, or the above steps fail (e.g. because of a failed test case, or a merge conflict), the change should be re-submitted using a regular git cr submit call, including a description of the failure in the commit message. This way, all reviewers and owners are notified.
-- * -- * ^ ^ | | | [master] | [foo] | "foo's base"
A change is considered to be in "merged" state if its branche's head is contained in its target branch:
-- * -- * -- * -- * ^ ^ ^ | | | | | [master] | [foo] "foo's base"
Changes may be "ported" to another base/target. This is e.g. useful to "backport" a bug fix to a release or deployment branch. Porting is done by cherry-picking each commit on the change's branch onto the new base/target and adjusting the CR-TARGET and CR-BASE (and possibly CR-OWNER) markers in the commit messages.
Porting can be done before or after a change has been merged. If the change was already merged, the squashed version of the change will be "resurefcted" as a change for review, with the review history being lost due to squashing sappied when applying/merging.
Porting preserves the change's logical name, but the branch name is adopted to the new target branch and owner: cr/john/foo/for/X becomes cr/christine/foo/for/Y.
Changes may declare external dependencies, that is, changes in other repositories. This is useful when one change depends on a change to a different modules, e.g. a library, or the server side code matching some change in the client code, etc.
External dependencies are tracked using markers in the commit message:
This information may be used as follows:
- changes can only be applied once all their dependencies have been merged.
- client side tools may automatically check out the relevant version of the relevant changes to the respective local repositories.
- CI systems should test the change together with the changes it depends on.
In an environment where the base of a change frequently changes, changes will often need to be rebased on the latest version of the base branch:
git cr rebase
This will automatically rebase the change onto the correct branch:
--- * ---- * / ^ --- * -- * -- * | / ^ [bar] -- * -- * | ^ [foo] | [master] --- * ---- * / ^ --- * -- * -- * | / ^ [bar] -- * -- * | ^ [foo] | [master]
Note that rebasing invalidates any signed revisions/tags. In particular, all approval or validation tags become invalid. Veto tags also become invalid (unsigned), but should still be taken in consideration [TBD: how, exactly?]