How to submit a drive-by patch and get it accepted

I think it’s weird that I have to write this post in 2015, but earlier today I had to explain to someone with the technical skills to submit a good patch that he was doing the process wrong in some basic and extremely annoying ways.

Googling revealed that most explanations of patch etiquette are rather project-specific in their advice. So I’m going to explain the basics of patch submission that apply to just about any open-source project, with a focus on how to do it right when you aren’t a regular committer (that is, it’s what’s often called a drive-by patch). Here we go…

Let’s start with the blunder that motivated this post. It was email that inquired “Have you had a chance to review my previously submitted patch?” What was infuriating was that said inquiry did not identify or include a copy of that drive-by patch, didn’t identify what revision of the project it was made against, and for that matter didn’t even name the project.

This violated the first rule of getting your patch accepted, which is to minimize the cognitive load on the person processing it. That person may see dozens of patches a week on multiple projects (and frequently does, if it’s me). Expecting him to remember lots of context and match it to your name when you’re not already a regular committer that he recognizes is a good way to make yourself disliked.

So make it easy. When you mail a drive-by patch, always specify the target project it applies to, and what base revision of the project it applies to (by commit ID or date). The hapless n00b I am now excoriating replied, when asked what revision his patch was against, replied “HEAD” and it was all I could do not to leap through the screen and throttle him. Because of course HEAD now is not necessarily the same revision it was when he composed the patch.

Good etiquette is different if you have a well-established working relationship with the person you are submitting to. If I get a git-format patch submission from a regular contributor whom I recognize to one of my projects. he doesn’t have to repeat that context; I can generally assume it was made from a recently-synced repository clone and will apply against head. But I can only be relaxed about this if I am sure he will be explicit about the target project and base revision if it’s for a different project.

Another way of minimizing cognitive load is to make the patch self-explaining. Always include an explanation of the patch, its motivation, and its risks (in particular, if the patch is untested you should say so).

Hapless n00b failed to do this. I had to Google for the name of the principal function in his patch to learn that it’s a code-hardening move derived from OpenBSD security work. Even though it’s a good patch, and I merged it, that’s more work than he should have made me do. Through not being explicit, he actually slowed down the merge of a fix for a potential security issue by a couple of weeks.

Even if you are not sending a git-format patch, the git conventions for change comments are good to follow. That is, a summary line in imperative form (“Fix bug 2317.”; “Implement feature foo.”) should be optionally followed by a blank line and explanatory paragraphs.

Really good bug-fix patches include a recipe for reproducing the problem that motivated them. Because I can test those instantly, I can merge them instantly. On projects with a regression-test suite, you scale the heights of best practice by including a patch bands that enhance the test suite to demonstrate the correctness of the patched code. When you do that, I love you and want to see more patches from you.

Here’s a mistake the n00b thankfully did not make: his patch was small and simple, easily reviewed. When in doubt, send a patch series of simple changes rather than a huge rollup patch that does multiple things – that sort of mess is very likely to be rejected because reviewing it hurts the maintainer’s brain.

At least he shipped in diff -u format; it’d been years since I saw even a n00b use the horrible default -e format, which is very brittle in the face of any change to the target code at all (never do this!). diff -u -p format, which tries to put the name of the affected function in the header before each patch band, is better (this what git format-patch does).

Maintainers vary in whether they like patches to be plain text inline in the message or a MIME attachment. I myself am relatively indifferent to this, but others have strong preferences. If you are not sure, include an offer to resend in the form the maintainer prefers in your cover note.

On the other hand, unless the patch is large enough to blow an email size limit (which suggests it’s much too large) do not ever send it as an http/ftp link to be chased to the patch text. Your maintainer might be on the road with spotty network access when he tries to process it. Even if he isn’t, every step of hand-work he has to do (including trivial steps like typing a wget command) increases his process friction, which is unkind and reduces the odds that your patch will be merged.

It is irritating to the maintainer to get a patch against an old release, it is very irritating to get a patch for a problem he/she has already fixed, and it is extremely irritating to get a patch for a problem that the maintainer has to do extra work to find out he has already fixed. Find the project repository, clone it, test your issue, and make the patch against head. This will, among other things, greatly reduce the odds of your patch failing to apply.

Many maintainers (including me) dislike unnecessary merge bubbles. If you write a patch series using git that takes a while to complete, rebase it against current head before you ship. Again, this maximizes the chances that it will apply cleanly.

I’m going to finish with a preference of mine which while not necessarily universal practice, at least won’t offend anyone. If you have to refer to a previous commit in your change comment, please do not use commit IDs like git hashes or Subversion revision numbers to do it. Instead, quote the summary line of the commit and if in doubt cite the author and date.

There are two good reasons for this practice. One is that it’s human-friendly; a patch summary line is a much better cue to memory than a numeric or hex cookie. The other is that at some time in the future the repository might undergo surgery or conversion that will turn that cookie into a turd. Be kind to future maintainers by using a reference format that won’t become invalid and require hand translation.