Code Reviewing Guideline

Table of Contents Rules that each pull request must obey How to rebase Code review at GitHub

Rules that each pull request must obey

1. The codebase must compile after any patch is applied. Not only is this a basic quality requirement for the code, it is also essential to avoid breaking the bisection method of git. The bisection method is very important for any codebase of production-level software because it lowers the cost of finding hard bugs.
2. A patch should never break the bisection method. Besides compiling after the application of a patch, the codebase should not include uncalled code because if the uncalled code is buggy and called later after another patch, the bisection method will break. The bisection method will point out the later patch, but the bug was introduced in the earlier patch.
3. Each patch must be coherent, and a sequence of patches must tell a story. While organizing patches this way is time-consuming, it lowers the effort of code reviewing. And code reviewing is way harder than making patches coherent.

How to rebase

Assume you have a pull request to the application codebase, and the pull request includes three commits:

 add functionality X
 add functionality Y
 add functionality Z

And the hash values of the three commits are c69bc7abdd344015a78aa52f7f346b95aad08c44, 565a74f789b4ffcd436f7834b6560f050ee1e126, and 02bc06a8e9ad5d4e5235b4a36c64b97cf28dd9bc, separately.

The git rebase command can be used to interactively change commits by "rewinding" to previous commits. For example, say you want to go back to the commit "add functionality Y". You can rewind the repository tree to the code at that point in time, edit the code, try compiling and testing, and when you're happy with it you can re-add it to the commit.

Here are the steps:

1. Find the hash of the commit that you want to change. In theory, you can do multiple commits at once, but let's just do one commit here. The hash of the "add functionality Y" commit is 565a74f789b4ffcd436f7834b6560f050ee1e126.
2. Rebase interactively (using the -i switch) by passing the hash of the commit that happened previous to the one you want to change. In git, you can just add the "^" character to mean "go to the previous commit." So you can do:

   $ git rebase -i 565a74f789b4ffcd436f7834b6560f050ee1e126^

3. When you enter this command, the rebase menu will come up. It will show you every commit from the HEAD of the branch back to the commit that you specified. By default, they all have "pick" next to them, which essentially means leave them as they are. You can replace "pick" with any one of the options shown -- for example, to edit the files in the commit you would replace "pick" with "edit" or "e"; to just change the commit message you would replace it with "reword" or "r". This is the point at which you could specify you want to change multiple commits, if you want. Save and close the rebase menu.
4. When the rebase menu closes, you are at the commit you chose to edit. You can verify this by doing command:

   $ git log

   Edit any files you want. Try compiling them, testing them, etc.
5. When you are happy with the changes, do command

   $ git status

   to see which files have been modified, added, or deleted. Add the files using command:

   $ git add

6. To change the commit, you can do:

   $ git commit --amend

   You be done with editing this commit, you can do:

   $ git rebase --continue

7. The rebase tool will start undo the rewinding process by applying the commits one-by-one. If you specified multiple commits to edit, it will stop at those commits.
8. If there is a conflict when applying the commits, you will see an error that says error: could not apply and it will tell you which commit is causing a problem. To see which file has caused an error, you can do

   $ git status

   Edit those files, removing the meta tags that show where the conflicts are (<<<<<<< , =======, and >>>>>>>) and choosing which version of the code you want to keep. Then, for any files that you edited, add them to the tree using

   $ git add 

   but do NOT do

   $ git commit --amend

   This will merge all of these changes into the previous commit. When you're done editing, compiling, testing, and adding, you can do

   $ git rebase --continue

9. When the rebase is done, you can push the changes (using the -f switch to force the update) to your copy of the repository on the branch that was used for the pull request. It will automatically update the pull request.

   $ git push <remote name> master

For more information, please refer to merging and rebasing.

Code review at GitHub

GitHub integrates functionalities to allow the developers and administrators to mange the codebase in an efficient way. A popular functionality is for code reviewing. In the following, I will use our Linux XIA project for example. To review a pull request for an repository:

1. The reviewers first need to go the XIA-for-Linux repository.
2. Then, click the Pull Requests option. The reviewer can see a list of the open pull requests.
3. Choose the pull request to review. Assume we choose pull request #19 Add Ethernet principal. By default, it shows a page recording the review history.
4. Click Commits option. The reviewer can see a list of commits in the pull requests. Also, one can see all the changed files by clicking Files changed option, which will show the detailed changes for each file. We recommend reviewing the code commit by commit.
5. Under the Commits option, choose one commit to review. You will see the changed files in the specified commit. GitHub allows you to review the code line by line. When you would like to add a comment to a line, click that line, and it will show a + sign at the left of the code line. Click the + sign, which allows you to add comments in a dialog window. Next, click the Start a review button to begin the code review.
6. After you review all the code and decide to finish the code review on that commit, click the Review changes option on the top right of the page. We recommend adding a commit message to specify which commit you are requiring the changes. Then, choose the way for submitting the review, i.e., Comment, Approve, or Request changes. Finally, click the Submit review button to finish the code review on that commit.
7. Review the remaining commits in the same way.

After the reviewer finishes reviewing all the commits in the pull requests, the developer needs to fix all the issues according to the comments by rebasing the pull request. The developer can check the comments under the Conversation option of that pull request. If the developer doesn't agree with or have questions on the comments given by the reviewer, then the developer can leave a further comment by replying that comment.