Super-cool — collaborative code review stored in source control. I’ve been wishing for this for a long time. Here’s how it works:
$ git clone firstname.lastname@example.org:google/git-appraise.git Cloning into 'git-appraise'... remote: Counting objects: 543, done. remote: Compressing objects: 100% (4/4), done. remote: Total 543 (delta 0), reused 0 (delta 0), pack-reused 539 Receiving objects: 100% (543/543), 169.78 KiB | 0 bytes/s, done. Resolving deltas: 100% (265/265), done. Checking connectivity... done. $ cd git-appraise/ $ git appraise list Loaded 0 open reviews: $ git fetch origin refs/notes/*:refs/notes/* remote: Counting objects: 833, done. remote: Compressing objects: 100% (42/42), done. remote: Total 833 (delta 21), reused 0 (delta 0), pack-reused 791 Receiving objects: 100% (833/833), 117.92 KiB | 0 bytes/s, done. Resolving deltas: 100% (398/398), done. From github.com:google/git-appraise * [new ref] refs/notes/devtools/analyses -> refs/notes/devtools/analyses * [new ref] refs/notes/devtools/ci -> refs/notes/devtools/ci * [new ref] refs/notes/devtools/discuss -> refs/notes/devtools/discuss * [new ref] refs/notes/devtools/reviews -> refs/notes/devtools/reviews $ git appraise list Loaded 1 open reviews: [pending] 2c9bff89f0f8 Improve the error messages returned when a git command fails. Previously, we were simply cascading the error returned by the instance of exec.Command. However, that winds up just being something of the form "exit status 128", with all of the real error message going to the Stderr field. As such, this commit changes the behavior to save the data written to stderr, and use it to construct a new error to return.
Git-appraise’s implementation though isn’t using a normal branch with text resources for the review which would be my preference. It is using refs/notes/devtools/reviews as a store. The branch list only shows master and a feature branch:
Indeed, the GitHub interface should allow you to navigate those text resources in the branch. I’ve written about this before. Ideally I would also be able to mount a third-party plugin to GitHub’s web UI and have a meaningful navigation of the review(s).
Branches or Repos?
The classic best practice is that branches should be for change of policy on the same source. Policy can be any/all of:
- groups can/can’t view branch versus other branch
- groups can/can’t check in to branch versus other branch
- branch contains stuff for a specific release (and no further)
- branch contains a work in progress on some functional or non-functional change, with variable level of granularity
Broadly speaking, branches should always be mergeable between each other, with the policy being the guide as to whether you should/shouldn’t (can/can’t if your SCM has permissions to some degree).
Github Pages (Branches)
The suggestion for GitHub Pages use, is to have it on a gh-pages branch, adjacent to your master branch (that contains your code). Sure, for a pure blog or site you might only have the Github Pages content and no programming source at all. Indeed, in that configuration you don’t need the gh-pages branch at all. I find the gh-pages branch a little bit odd really, however wonderful the technologies are. Merging between the gh-paged branch and the master being illogical is my persistent thought.
As the title says, branches should be for change of policy on the same source.
GitHub’s Wiki Implemention (Repos)
Github implemented Wiki differently: as a first class repo, that follows a naming convention association with the repo it is associated with. D3’s repo can be cloned from
email@example.com:mbostock/d3.git and its wiki from
https://github.com/mbostock/d3.wiki.git. This is great. Less than great is the the fact that the Github web interface doesn’t allow you to navigate the revisions of pages, or see diffs etc. That would be so easy for them to implement. This is how Github Pages should have been implemented, in my opinion.
In summary, the wiki implementation is done right, given it is not a branch.
Git Appraise (Neither)
It’s being stored in refs/notes/devtools/reviews as mentioned. My thoughts:
- Should be a first class SCMed set of text files, in my opinion, allowing merge amongst other things
- Would be best not represented as a branch with a different policy to the thing it is reviewing
- Should have be navigable in the same web-UI as the regular code