Gituiqette and Peer Review

The #1 Git Rule

If you are going to read only one paragraph of this text, read this one. Always, always, commit and push all your work at the end of the day. Add "WIP" to your commit message if necessary.

RULE: The Good Developer always commits and pushes all his work at the end of the day, adding "WIP" to his commit message if necessary..

About WIP Commits

Use WIP commits locally to have a intermediate commit to "save" your work and amend later, removing the WIP. Push WIP commits when work should be shared with another developer (either for early review, testing or when said developer has to continue development in your place) or, most importantly, at the end of the day.

Indicate "Work In Progress" commits by prefixing with "WIP", "(WIP)", or "WIP:", etc. It doesn't really matter as long as it's clear from the get-go.

Refrain from keeping these commits when creating a merge request as this breaks the atomicity of commits. Use fixup commits and rebase to merge the WIP commits and commits containing the final implementation. This may be a bit overwhelming for the new developer however, feel free to ask Sam for help on how to use fixup commits.

The #2 Git Rule

The second most important rule of Git usage is:

RULE: The Good Developer always keeps his Git repository in the state he would want to find it in on his first day as a developer.

This means: no open unmerged branches, no unremoved merged branches, ...

No Fast-Forward

Read the following article: Fast-Forward Git Merge.

When we merge Git branches, we typically don't fast-forward (pass flag -no-ff to git merge). In SourceTree, this can be done automatically for all merges by enabling "Do not fast-forward when merging, always create a commit" on the Git-tab of the Preferences screen. If you followed our setup guide, this checkbox should already be enabled. Otherwise, enable it immediately.

RULE: The Good Developer doesn't fast-forward when merging, unless there is a good reason.

No Legacy Code

Code is stored in version control. As a result, we consider the approach of commenting old code as a terrible practice; it is almost never necessary. Systematically remove old code instead.

Also, when removing code, don't forget to remove all dependencies (functions, configuration parameters and dependencies) of the removed code, when those are not in use by other code.

RULE: The Good Developer systematically removes all old code and its dependencies.

Commit Messages

Making good commit messages will not only help other developers quickly see what a commit is about, it is also indispensable when using tools such as git log, show, rebase, blame, etc. to quickly gain insight in a commit's changes. Furthermore, proper commits allow for making the description in a Merge/Pull Request more succinct because most of the explanation is in the commits.

  Enter a short summary (max 50 chars)

  Any other details should go into the body. Notice that the body is
  separated from the message title by a blank line. Wrap this body at 72
  characters so it doesn't appear on 1 line in the terminal.

  * feel free to make lists here
  * a body has fewer restrictions than the title

RULE: Use our Commit Messages convention for all Git Commit Messages:
Capitalize the commit message, do not add a full stop, use imperative form.
Keep the commit message short and simple (< 50 characters)
Add a body by leaving a blank line after the subject.
No process/resolve feedback commits
Indicate "Work In Progress" commits by prefixing with "WIP"

Other tips:

Capitalize the commit message. Purely visual, although this way the commits are also consistent with commits made by GitLab such as suggestion or merge commits.
Keep It Short and Simple. The commit message title should quickly sketch the context of a change, much like the subject of an email. Further details should be contained within the body. Aim for a maximum of 50 characters, git tools and editor syntax highlighting will warn you when you have gone past this limit. Never go past 72 chars as the message will be truncated and the remainder will end up in the commit's body when using GitHub, GitLab. Bitbucket, etc. For this reason try to refrain from using the -m option when committing from the CLI, always edit a commit message in a text editor or GUI text field.
No full stop. Full stops add no context to the commit message and every character counts (see previous point).
Use imperative form. Commit messages should start with a verb in the imperative form (e.g. Change) instead of the simple past (e.g. Changed). This better shows the context of the commit and what it does vs. what you, the developer, did.
Use body and wrap. Add a body by leaving a blank line after the subject. Syntax highlighting will warn you when forgetting this blank line. Make use of the body to go into further detail about the commit. Here you can drop all the restrictions that count for the git commit message. Form paragraphs, make lists, use past/present tense and/or first person singular, etc. These can easily replace code comments when used in conjunction with git blame. Set your editor to (hard) wrap the body at 72 characters as otherwise the body would just appear on one long line.
No process/resolve feedback commit messages. Naming a commit this way after implementing feedback from a PR/MR adds little context to the commit. Make a new (atomic) commit for every change (even typos) or rewrite history by using amend, fixup and rebase. When rewriting history be sure to notify your reviewer as this does not add "new" commits.

Optional resources:

Peer reviews (via GitLab merge requests)

Peer reviewing is essential for quality improvement and knowledge sharing. At Codifly, we recommend reviewing all source code changes via GitLab.

The policy

At Codifly, all source code is preferably reviewed by at least one colleague.

Reviews should never stay open for more than 1 day. There is a "before end of day" policy in place.

How should I create a merge request?

In GitLab, go to merge requests and click "Create Merge Request". Select the source branch and destination branch and select one team member.

After sufficient approvals, the creator of the merge request is responsible for performing the actual merge via the GitLab Web UI.

What if the project is on an external server (Github, Atlassian, ...)?

It is possible to add the project to GitLab via push/pull mirroring. All changes on GitLab are then propagated to the external server (push) and all changes on the external server are automatically fetched into GitLab (pull).

This is by far the preferred approach. However, this might be impractical when the customer has its own team directly working on the external server: the pull-operation is performed once every thirty minutes, and this delay can introduce conflicts.

Anyways, feel free to ask Evelyn to set this up for you if applicable.

MR "dependencies"

In most cases, it is possible to create a feature branch feature/b directly from the development branch. However, sometimes, you want to branch from an existing unmerged feature branch feature/a. This is the case when you are creating a new feature that is dependent on a recent new feature which is not merged to development yet.

In GitLab, when creating a merge request for such a branch feature/b, please assign feature/a as a dependency of the merge request. This makes life of the reviewers much easier, by doing two things:

MR diffs for feature/b will not repeat changes already present in the MR of feature/a.
Gitlab will prevent merging feature/b before its dependency feature/a is merged.

The MR Checklist

In this list I mention a few common mistakes. The list is useful for reviewers to focus their attention, but more importantly, it helps you as a developer not too push code that will not be accepted. Indeed, not making those mistakes in the first place will make the reviewer happier as he won't be required to say the same thing over and over again.

BAD: Plain-text secrets and keys

All files containing security-critical secrets, passwords or keys should always be encrypted with git-crypt. This is especially true for configuration files.

Also, we often use two different keys: one for development and acceptance environments and one for production environments. The configuration of git-crypt is stored in .gitattributes, so that's where you should look.

RULE: The Good Developer never commits secrets, passwords or keys unencrypted to Git.

BAD: Decentralized configuration

Project configuration should be centralized in configuration files. It should not be scattered around the source code.

Red flags:

Use of process.env.NODE_ENV inside source files.
Use of global constants for e-mail addresses, keys, ... inside source files.

BAD: Logging sensitive information

console.log() instructions should obviously never reach any production version. However, using console.info()/console.error() might be acceptable in many projects. If possible, using a custom logger is highly recommended (this is implemented in our seed api project). In any case, developers should never ever log sensitive or personal data in the console. Be especially cautious in authentication/authorization related code.

BAD: Inconsistent file names

We have a convention to always use camel case for filenames. This principle is followed most of the time, but somehow there is one type of files that is often ignored: graphical resources and assets. Often, these files are taken straight from the design and are not renamed to our naming convention.

Also, we try not to abbreviate things when it is not necessary. As such, an directory named img is better called images.

BAD: Insufficient error handling

Although it is not easy to really check for correct error handling in merge requests, it is doable to do some quick checks.

use of the await keyword should very often be wrapped in a try-catch block.
Empty catch statements have the smell of ignorance and laziness.
In front end code, pending, success and error states should all be handled correctly.

BAD: Incorrect documentation

Missing documentation is a sign of shortcuts, but bad documentation is an absolute no-no.

For the reviewer, it is a good idea to always check comments and their consistence with the code surrounding them.

BAD: Inconsistent commit messages

Commit messages should follow the convention discussed earlier.

Exercises

PLEASE SUBMIT ANSWERS AND PROVIDE FEEDBACK USING THE FOLLOWING FORM: https://forms.gle/QNKFkDwpe4PftavN9.

Exercise 1: suppose you see a Git history like the one below, although you are using Git flow.

Exercise 1

1) Evaluate the commit messages. Which guidelines were not followed? 2) Apart from Git messages, the developer also misconfigured his SourceTree. What is wrong? What will you tell the developer who performed those commits when you see this appearing in a Git history?