Jair Trejo

Git commit sins

I work at Vinco Orbis, a software development studio in Mexico City. We recently overhauled our website and set up a Github repo for further development and maintenance. We are pretty proud, you should check it out.

However, amidst the creative enthusiasm we fell into some bad committing practices, much of them coming from members of the staff that were contributing from Github’s web interface and are less used to version control in their day to day job. I thought it would be cool to share them here so we all keep them in mind in the future.

This is a commit — Commit messages that don’t convey intent

Github’s default commit message for when you edit a file in the web interface is Updated <filename>, which is not helpful at all. The title of a commit message should convey the intent of the change, which is going to be useful when exploring the repository’s history. Other information can be left for the body of the message, and of course the diff already contains the filenames and locations of changes, so just restating them in the title is not very useful.

For deeper advice on writing good commit messages you should read Tim Pope’s classic blog post about it.

Pay no attention to those lines of code behind the curtain — Commenting stuff out just in case you need it later

Commenting stuff out is confusing for future readers of your code, ugly because you keep code around that is not doing anything and dangerous because you are keeping that code outside of your normal testing and QA processes, so you don’t know what evils you will unleash upon the world if you uncomment it later.

On the other hand, the whole point of version control is to keep old versions of your source code in case you need them later. You can just delete those lines, because you can always find them later, together with the change set in which they occurred and, if you write good commit messages, a meaningful description of how they came about.

You should take me with a grain of salt — A commit doing something, followed by a commit doing the opposite

This usually comes from people that are used to SVN, where the history of the repository was shared and immutable. However, with Git other people don’t really need to know that you screwed up and had to recant later: you can use git rebase to get your local history clean and understandable before sharing it with the world.

If you want to make that easier, check out this post by Elliot Cable about a Git workflow that enables a clean history without abandoning the virtual Ctrl+S of frequent committing.

Shooting from the hip — Pushing directly to master

At Vinco Orbis we grant push permissions to a project’s repo to the people in charge of reviewing and accepting contributions from the team, which come in the form of pull requests. This is not meant to be authorization for committing directly to master, bypassing our normal process; there is a lot of value in having changes reviewed by somebody else, and we have found that it’s easier to fix bugs or improve the code when potential improvements are identified earlier.

Even with small projects with only one contributor, it’s useful to run things by our Jenkins job pull request builder, and taking a look at the diff one last time has saved me much pain in more than one occasion.

Tweak, break the site for everybody, repeat — Using the production site as a testing ground

Lastly, and related to having commit privileges, it’s really irritating when someone triggers a deployment just to see the effect of their changes. We try to eliminate that need by clearly describing the set-up process of the development environment in our README files, and making said process very simple by using tools like Vagrant. Our goal is that anyone that wants to make a contribution can experiment at leisure on their machines before sending their changes upstream.

Even people without commit privileges sometimes make changes without checking they work at all. Although this can be spotted by the reviewer of their pull requests, we also have Jenkins run unit tests and publishing the result as a comment, which helps catch regressions. Still, it is better that people run their changes at least once to see if they are working, and absolutely wonderful if they write tests even for “small” fixes.

In any case, the reviewer can checkout the pull request locally and check them manually. I do that frequently with changes that impact user experience.

¿What are good and bad committing practices in your organization?