Here, I'm going to focus mostly on the process of posting the patch, rather than the process of writing it. Writing good patches is complicated and difficult in all kinds of ways, but here I'm going to assume that you've already done that and that what you're trying to do now is convince somebody else - ideally somebody with commit rights - that it makes sense to put your patch into the official repository. The details depend considerably on whether you're talking about posting a brand new patch or whether you're talking about posting a new version of a patch that has previously been posted, as well as whether this is just a single patch or a whole patch set, and how complicated it is. So let's go through a few scenarios.
For new patches, I recommend that you start by giving a clear and compelling summary of what the patch does and why it's valuable. By the time I reach the end of the first (or maybe second) paragraph, I should have a clear, high-level understanding both of the basic purpose of the patch and of how the patch accomplishes that purpose. For example, the purpose might be "add incremental backup" and the method of accomplishing that goal might be "summarize the WAL to determine which blocks have been modified, and then use that information to decide what should be included in the incremental backup." Or, the purpose might be "simplify the code" and the means of accomplishing that purpose might be "remove some code for dead architectures that we no longer support." The mistakes I most often see people make in this area are either (a) assuming that the reader already understands why the patch is a great idea and thus not explaining it or (b) taking too long to get to the point.
It is useful to include links to previous discussions of the topic, if there are any, but try to make following those links optional. If you write "this has previously been discussed, see [link]," anyone who wants to participate in that discussion probably has to follow that link and read whatever thread they find over there. If you instead write "Robert used to hate this idea [link] but decided it wasn't so bad after all [link]," the reader has the option to follow those links and may choose to do so at some point but it isn't mandatory. If and when they do follow it, they will do so knowing why you think that other discussion is relevant and can read it with that context in mind. Maybe they won't end up agreeing with your summary of the previous discussion, but even if that happens, at least the point of disagreement will be clear. Nothing wastes more time and causes more frustration than two people writing a long chain of emails, talking past each other.
For a simple patch, you might not need to do much more than what is described above, but for more complicated patches, you'll want to write more. Imagine that your reader is tentatively convinced that the patch might have merit, but has concerns. Try to imagine what those concerns might be and address them. For example, if your patch is a performance patch, your reader's concerns are likely to be whether it really improves performance in the cases it's intended to address, and whether it regresses performance in any other cases. So, you could show benchmark results of the best case (which hopefully got way better) and the worst case (which hopefully did not get much worse). If you don't include those results, you're hoping that the reader is going to like the patch enough to do those experiments themselves. And that might well happen, but the reader needs to be more interested to run benchmarks themselves than they do to look at your results. Make it easy for them. Now say on the other hand that you're fixing a bug. Now the reader is quite likely to be concerned about whether you've really understood the nature of the bug and whether the proposed fix really fixes it. Convince them. No matter what type of patch you've written, the reader may have concerns about whether the patch breaks anything else. Perhaps you want to argue that it doesn't. There's no way to address every possible objection in a single email, and you shouldn't try -- but it is a good idea to take a guess at what objections are most likely, and address those.
If you're posting a series of patches rather than a single patch and if your email is not already too long, the last thing I think you might want to consider including is a brief summary of what each patch in the series does. This is often not needed, and it is often better to put this information in the commit messages for each patch in the series rather than in the body of the email. However, it's sometimes helpful, especially when it helps the reader to better understand the design you've chosen, thus possibly allaying a concern that you may have chosen a poor design.
When you're posting a new version of an existing patch, the two main things I suggest you include are a summary of changes since the previous version of the patch, and a summary of the current state of the discussion that can be understood even by someone who has not read the rest of the thread. Many experienced hackers who are not yet committers make the mistake of assuming that nobody who hasn't read the thread from the beginning will ever take an interested in it in the future, and this becomes a self-fulfilling prophecy. Reading through a thread with 300 emails that is cross-linked with several other threads about similar topics can take hours or even days; reading a good summary and then selectively checking the older discussion for messages about any points of particular interest can take an order of magnitude less time. It isn't necessary to post a new summary for something like a trivial rebase, but if there have been more than 10 or so emails since the last time you summarized, it's probably a good idea.
I don't see a lot of other people doing it, but I also deeply appreciate a summary of what has changed since the previous version of the patch set. What I like to do is diff each patch against the previous version of the corresponding patch and briefly summarize the differences in English, e.g. "changed to a simple power-of-two memory allocation loop to address Tom's concern about non-standard coding" or "reworded comments to more accurately reflect the current design". These kinds of notes allow a casual reader to understand how the patch is evolving and what kinds of changes are being made over time.
Finally, regardless of what kind of patch you're posting or how many revisions it's been through, always remember that the fundamental problem you're trying to solve is that you understand the value of your patch and the reader, perhaps, does not. Use all the tools you have available to solve that problem. Don't stop at writing a good email. Write a commit message for the patch, or for each patch in the series. Write good comments. Consider including a README file. Even the way that you decompose the patch into a series of patches can be instructive: separating changes by directory doesn't help me understanding anything any better, but breaking up one big change into a bunch of little changes that can be separately understood as a logical progression toward an end goal sure does.
It isn't very important that you do all of these things exactly perfectly, but it is important that you do attempt to do them. If you try to write comments and for some reason they aren't very good, that's a lot easier to fix than if you don't write any comments at all. If English is not your first language, write comments in your native language and run them through Google Translate. Even if the result looks awful, it's a beautiful sight compared to no comments at all. Likewise, it's unlikely that any commit messages you write yourself will be adopted by the committer (and you should take it as a compliment if they are). But that doesn't mean that they have no value: a commit message that you spend ten minutes writing can easily save me an hour of time reviewing. No joke. And again, the README that you write may very well fail to survive in the committed version of the patch: that content may get moved elsewhere, or rewritten completely, or discarded, but if they save somebody hours of time trying to puzzle out the intention of your code, that's amazing.
One of the things that astonishes me in this area is how often people submit patches that are completely lacking in comments - or the comments are clearly wrong and don't describe how the patch actually works - and when I point that out, they say something like "well, before I spent the effort to write good comments, I wanted to find out whether you thought the patch was good idea." This imagines that I'm able to understand complicated C code with no explanation at all (or a misleading one). I wouldn't be surprised if some PostgreSQL committers can do that, but I'm not usually one of them. I'm reading your comments to understand the design and your code to see whether it matches your comments. It would be far easier for me to review a file with great comments but no code than to review a file with great code but no comments.
I realize that by talking about comments I'm getting a bit into the topic of how to write the patch rather than how to present the patch on pgsql-hackers, but I'm going to make two more points about them anyway, because like the email, they're absolutely critical parts of communicating the value and craftsmanship of your patches. First, try to always write comments that explain why the code does something, rather than just what the code does. If the code says demolish(house), the comment shouldn't say /* Demolish the house */; it should say /* Caller must already have verified that the house is unoccupied */. The former is blindingly obvious; the latter is what the reader needs to know to understand that the code is safe (and they can check for themselves whether all callers really do the appropriate verification, if they have concerns).
Second, focus on explaining whatever won't be obvious even to an expert. Just because you were confused about something doesn't mean that everyone will be, and if you try to write comments that explain everything that anyone could be confused about, your comments will be unwieldy and overlong. Instead, imagine that your patch gets committed and everything is working fine, and then later, another patch gets committed which modifies your code and breaks something. Your comments need to be clear enough about what the hidden dangers are that a committer who is thinking about committing that second patch - the one that breaks something - will realize that they shouldn't do so. Or, alternatively, imagine that someone else - perhaps a committer, perhaps another senior contributor - is thinking about adding a new feature on top of your work. Will they be able to understand your work well enough to do so, or are they going to end up complaining on the list that this code is incomprehensible? If the former, you've done your job well.
That's all I've got for now. I hope this is helpful. If you have other suggestions for what to do (or not do) when posting patches to pgsql-hackers, feel free to leave those in the comments.
Lack of tests and documentation is not uncommon unfortunately. Another piece of advice I would give: use `git format-patch`, this simplifies live for the reviewers a lot.
ReplyDeleteBest regards,
Aleksander Alekseev