By Agustin Aliaga – Developer at Santex
Git is a powerful version control system designed to make software development collaboration easy. It can be used for personal (single contributor) repositories, but it really stands out in projects where multiple developers modify the same codebase every day. Multiple branching models can be adopted. However, in most cases, a Pull Request (PR) will be created when a bug fix or a new feature is ready to be merged into the main branch. The PR itself is an attempt to merge specific changes from one branch to another. Let’s explore the Pull Request workflow in detail.
PR Creation
Whenever a PR is created, the following best practices should be taken into consideration:
-
- Add a helpful description to the PR that answers the following questions: What is the requirement or bug? Is there a link to the issue (e.g. on JIRA)? How does your code fix it? Is there anything else the reviewer should take into consideration?
-
- Make small, consistent, and logical PRs: We want our PR to be merged as soon as possible. Imagine how hard it would be to review hundreds of file changes at the same time. If this happens, it is likely the reviewer won’t have, or take, the time to do it properly. Do your best to keep it simple and concise.
-
- Make sure the PR’s metadata has been set. For example, this can be done by assigning the reviewer (to trigger a notification), setting the assignees, or adding a label (if needed), etc.
-
- Configuring the repo’s branching security settings to keep developers away from self-merging their PRs or pushing to a shared base branch. To avoid this, I recommend enforcing Github’s branching security settings. When a large team of developers are collaborating on the same repo, there is a good chance of accidents happening. A single mis-click in a “merge” button can cause terrible headaches. Protecting important branches is something I’d advise most of the time.
- Avoid submissions that include unresolved conflicts. Fixing those conflicts shouldn’t be a reviewer’s responsibility. PR creators should dedicate time to solving them, either by using Github’s conflict resolution tool (when it’s simple enough) or by using your favorite diff tool locally. You can achieve this by pulling the base branch and merging it into your feature branch. After you’ve pushed it to the remote repo, the PR will automatically update.
Automated tests
If a Continuous Integration system (such as Jenkins, Travis, or CircleCI) is set up, a bunch of hooks can be configured to run unit tests on PR creation. This will allow the team to detect and catch bugs rapidly, as well as prevent conflictive code from being merged. This is a long topic that requires its own blog post, so I’ll just move on to the following stages.
Code Review
After everything related to the creation of the PR is dealt with and the CI tests have been passed, it is time for a code review. Projects tend to have tight deadlines, which can sometimes make code reviews seem like a waste of time to other team members or external agents. Ironically, code revisions can actually help increase productivity and reduce reworks because we avoid bad practices in our code and share knowledge between contributors.
Some benefits of implementing code reviews are:
-
- Less experienced developers get to learn from their mistakes.
-
- Experienced developers can consolidate their knowledge as they teach others.
- A high-quality codebase is ensured.
Humility: a soft skill that matters
Sometimes, as work starts to accumulate and things get tense, engineers have a tendency to become less aware of their attitude toward their peers. It is always important to avoid egocentric behavior, listen to our co-workers, and moderate our communication when reviewing other peoples’ work. If we write a PR review in a disrespectful/arrogant manner, we could be damaging the teams’ confidence and the work environment.
The “reviewer” role
Ideally, teams should implement peer reviews. This means that anyone who has the required experience and skills should review others’ code. In practice, collaborators regularly have different levels of experience in both the technology used for the project and the codebase itself, including its set-up, architecture, code styling, deployment, etc. In this case, experienced developers should be conducting the reviews and newer team members should be included as they progressively get more comfortable with the project.
Merging the PR
After the PR is approved, it’s time to merge it. We have a couple of options for doing so. Let’s explore them:
-
- Create a Merge Commit (default): This option will merge all the commits from the feature branch, plus a new merge commit. This is the safest way to perform the merge, since it is a “non-destructive” operation. The downside of using this option is that, since it creates a merge commit by tying together the history of both branches, it pollutes your history tree with multiple “irrelevant commits.”
-
- Squash and Merge: This option will “squash” all the commits into a single commit. If the PR includes a lot of commits, this would be the most practical way to approach a squash and merge. It will make your history tree much cleaner and easier to read in your base branch. However, you will lose granularity due to the bigger size of the resulting commit.
- Rebase and Merge: The “rebase” operation is another way to combine commits from two different branches. This will put your feature branch commits on top of your base branch’s latest commit, effectively rewriting the commit history. After this, it will perform a fast-forward merge, making it look like all the work was done on the base branch. This is extremely dangerous when the rewritten branch is public and shared with other team members. Generally, the rule of thumb is to keep rebasing operations for private-only branches.
