Pull Requests & Git Commits
July 20th, 2022
In my time as an engineer I've seen interesting stuff submitted within Pull Requests. For the last several years I've considered myself pretty laidback on the whole process. I'd say this stems from the strength of the team I've been working with for a number of years now - they have very strong Git and PR hygiene. And this means I haven't really needed to think about it much. Recently, however, I have found myself taking stock of, "what makes a good PR?" While not terribly rigid, what follows is where I currently land on it all.
Pull Requests
First, when opening a PR, I think it's important to stop for a moment and consider what it's conveying to others: "I'm happy with what I have, I think it's complete, and there should be no problems merging this work into main." I've arrived at this thought because I've seen ridiculous stuff in PRs ranging from git commit titles of "WIP," obviously jokey method names, and comments that are clearly not meant for the main branch. Your peers reviewing your work shouldn't have to spend their time calling this stuff out. The caveat here is for Draft PRs which are reasonable places to have incomplete work.
Next, when creating the PR itself, at the very least I like to see a link to the story with which the work is associated. It's nice to have greater context on the work under review and not forcing your fellow engineer to hunt around in an issue tracker is a simple courtesy.
Beyond an issue tracker link, I generally just look for a short paragraph (or two?) on what the overall changes cover. An example of where I might include more information is when I add an index to a table. In this case, I have often found it helpful to include images of the performance before and after or some kind of profiling to indicate a positive change. At the end of the day, though, the bulk of what's included in the PR's description should come down to what the team thinks is best.
Git Commits
An integral part of any PR are the commits which compose it. Over the years I've picked up preferences and opinions on how to shape the information Git presents. What follows are how I like to structure and present my work to my peers.
For starters, I like to tell a "story" with my commits. I prefer to line them up in a way such that the progression of the work is very obvious to the reviewer. Git is a history of what you've done, so capturing that well, in my experience, can be really handy.
The 50/72 rule, which I believe has been
kicking around for a long while now, is one I hang my hat on.
For me, violations of this are most evident when reviewing
commits in GitHub and the title of the commit is truncated to
... because the title was longer than 50 characters. Not
terribly helpful. I also try to abide by the "imperative mood." This
means writing my title in a similar fashion to "Update X to resolve
Y" or "Add table X to support Y." Nothing terribly shocking here and,
over time, this approach appears to have become convention amongst
the wider dev community. At the same time, I can't bring myself to be
a stickler for this and, short of extraordinary circumstances, if
someone didn't follow this approach I'd seriously weigh whether the
correction was worth the effort.
As for the body, the "72" in the rule, adhering to that length
improves their readability when using git log. That's
it. Make the messages easy for people to read and keep it consistent.
My final thoughts, now, are what's in the body of the commit message. For one, I personally don't consider the body of the commit to be optional. Unless the change is the tiniest of tiny changes, then take a moment and share what prompted it. Adding additional context to a code change is never a bad thing. And, similar to the title of the commit, convention has begun to dictate present tense imperative. Meh. I get it. I try to use it myself, but I feel like there are bigger problems to solve. I've known people to nitpick over this and I can't seem to be bothered. Give me context in the body, let me know why, and the rest is icing on the cake.
Final Word
Finally, and more importantly, these are things worth discussing with your team. I wouldn't dream of imposing strict, unwavering structure around PRs or Git commits, but I do find some of these ideas really helpful. The real sweat spot, in my opinion, is to take these suggestions into a collaborative environment to find what works best for your team.
Much of the above I picked up from peers and finding "what works" over time. But there's also plenty of discussion happening across the web on these things. Here are some which I've found helpful previously: