Best practices for patchsets
Lately we've been having more groups of patches than single patches, which I believe to be a good thing because it also means we've been getting less 500 line patches entitled "Fix everything." :) However, it's also increasing our opportunity for confusion, as we have more opportunity for patchset fragmentation. I have a couple of suggestions for how we should handle patchsets, along with a few things that have been floating around my head for a while about patches in general, so I'm laying them out here. I'd like to know what everybody thinks, and eventually get the final version somewhere on the website as well. Patches in general: 1. When you add a patch to the queue you have an idea of where you're going with it, and the commit message should reflect that. Be specific. We have a tendency to say something like "Various fixes to AllocationCapabilities" (I'm a huge offender here), when we really should be saying what was actually fixed, like "add EnumInstances support" or "eliminate duplicate instances." 2. Stay on task with a patch. If you notice other problems while you are working on patch, and they are not most definitely specific to your patch, they should be another patch. 3. This will sound a lot like "Stay on task," but resist the temptation to nitpick with a patch. I have a peculiar desire to have all declares at the beginning of the function, sorted according to length in ascending order. We all have our own idea on how best to name a variable or format a function call. Often, the change is in fact worthwhile, and is welcome, but lots of little changes that don't directly relate to a fix can really make that patch hard to read. They should be another patch. 4. Before you type, "hg email," you should always type, "hg export," first. Review your patch. Does it have any typos in the comments? Did you accidentally include an irrelevant change? Is your commit message still accurate and useful? Patchsets: 1. When you send a group of patches, Mercurial's email extension will create a "header" email. Make the subject and body of that email meaningful, so we know how the patches relate. It's easy to say, "Each patch has a commit message, it's obvious how they work together," but the rest of the list usually won't agree with that. If the commit messages for each patch are good, you shouldn't need more than a sentence or two to tie them all together, but you do need it. 2. If any of your patches are rejected and you rework them, resend the entire set. This prevents things like, "So use patch 1 of 4 from the set I sent yesterday, 2 and 3 of 4 from the patch I sent an hour later, and patch 4 of 4 from today." 3. If you resend a patchset, and the "Patch [0 of 3]" subject line was, "AllocationCapabilities fix," then the new subject line should be, "AllocationCapabilities fix, version 2." This will make it easier for email clients to thread things correctly, and for human readers to know they are looking at the most current revision of your set. I think that's everything for now. Additions, comments, suggestions are welcome. -- -Jay
JG> 1. When you add a patch to the queue you have an idea of where JG> you're going with it, and the commit message should reflect that. JG> Be specific. We have a tendency to say something like "Various JG> fixes to AllocationCapabilities" (I'm a huge offender here), when JG> we really should be saying what was actually fixed, like "add JG> EnumInstances support" or "eliminate duplicate instances." 100% agree. MQ allows you to edit a patch description at any time with "hg qrefresh -e", so it should be easy to update the message with information as you proceed. For patches that do more than one significant thing, a list of changes in the message would be good. JG> 2. Stay on task with a patch. If you notice other problems while JG> you are working on patch, and they are not most definitely JG> specific to your patch, they should be another patch. 110% agree. Patches are much easier to read if they're small. Even if you have two patches, one named "cleanups" and another named "fix specific issue X", that's better than just lumping the cleanups in with the fix. JG> 4. Before you type, "hg email," you should always type, "hg JG> export," first. Review your patch. Does it have any typos in the JG> comments? Did you accidentally include an irrelevant change? Is JG> your commit message still accurate and useful? Agree. JG> 2. If any of your patches are rejected and you rework them, resend JG> the entire set. This prevents things like, "So use patch 1 of 4 JG> from the set I sent yesterday, 2 and 3 of 4 from the patch I sent JG> an hour later, and patch 4 of 4 from today." Yes, re-sending the whole set makes it much easier for me to apply the relevant patches instead of cherry-picking the last-known-good version of each patch of a set. It also helps reviewers set context on a specific change that is being iterated. JG> 3. If you resend a patchset, and the "Patch [0 of 3]" subject line JG> was, "AllocationCapabilities fix," then the new subject line JG> should be, "AllocationCapabilities fix, version 2." This will JG> make it easier for email clients to thread things correctly, and JG> for human readers to know they are looking at the most current JG> revision of your set. Agreed, especially with respect to the "human readers" part. I don't know that ", version 2" needs to be the exact suffix, but something consistent and informative would be good. JG> I think that's everything for now. Additions, comments, JG> suggestions are welcome. Thanks a lot for doing this. After we settle on some of these, can you write this up as a patch to the SubmittingPatches file in the tree? That way we can claim it's policy :) -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
Agreed, especially with respect to the "human readers" part. I don't know that ", version 2" needs to be the exact suffix, but something consistent and informative would be good.
I figured this one would be the most likely to generate discussion. I took a couple of stabs at the right suffix while I was writing this, then just decided to go with something and let everyone throw ideas out. Maybe we could go with something more succinct like just "#x". So "AllocationCapabilities EnumInstance" followed by "AllocationCapabilities EnumInstance #2" sort of thing.
JG> I think that's everything for now. Additions, comments, JG> suggestions are welcome.
Thanks a lot for doing this. After we settle on some of these, can you write this up as a patch to the SubmittingPatches file in the tree? That way we can claim it's policy :)
Sure, no problem. -- -Jay
JG> I figured this one would be the most likely to generate JG> discussion. I took a couple of stabs at the right suffix while I JG> was writing this, then just decided to go with something and let JG> everyone throw ideas out. Maybe we could go with something more JG> succinct like just "#x". So "AllocationCapabilities EnumInstance" JG> followed by "AllocationCapabilities EnumInstance #2" sort of JG> thing. Sure. I'd prefer a shorter prefix, so just " #x" is fine with me. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
JG> I figured this one would be the most likely to generate JG> discussion. I took a couple of stabs at the right suffix while I JG> was writing this, then just decided to go with something and let JG> everyone throw ideas out. Maybe we could go with something more JG> succinct like just "#x". So "AllocationCapabilities EnumInstance" JG> followed by "AllocationCapabilities EnumInstance #2" sort of JG> thing.
Sure. I'd prefer a shorter prefix, so just " #x" is fine with me.
I don't really have anything else to add - good ideas. =) -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
JG> I figured this one would be the most likely to generate JG> discussion. I took a couple of stabs at the right suffix while I JG> was writing this, then just decided to go with something and let JG> everyone throw ideas out. Maybe we could go with something more JG> succinct like just "#x". So "AllocationCapabilities EnumInstance" JG> followed by "AllocationCapabilities EnumInstance #2" sort of JG> thing.
Sure. I'd prefer a shorter prefix, so just " #x" is fine with me. Agreed, a short prefix is a very good eye catcher. I'd prefer to have
Dan Smith wrote: the prefix at the beginning of the message. The subjects are sometimes that long, that they are not completely shown in the email client's preview pane. -- Regards Heidi Eckhart Software Engineer Linux Technology Center - Open Hypervisor heidieck@linux.vnet.ibm.com ************************************************** IBM Deutschland Entwicklung GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschaeftsfuehrung: Herbert Kircher Sitz der Gesellschaft: Boeblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
Heidi Eckhart wrote:
JG> I figured this one would be the most likely to generate JG> discussion. I took a couple of stabs at the right suffix while I JG> was writing this, then just decided to go with something and let JG> everyone throw ideas out. Maybe we could go with something more JG> succinct like just "#x". So "AllocationCapabilities EnumInstance" JG> followed by "AllocationCapabilities EnumInstance #2" sort of JG> thing.
Sure. I'd prefer a shorter prefix, so just " #x" is fine with me. Agreed, a short prefix is a very good eye catcher. I'd prefer to have
Dan Smith wrote: the prefix at the beginning of the message. The subjects are sometimes that long, that they are not completely shown in the email client's preview pane.
I think having the revision number at the beginning is a good suggestion. I'm an offender of the sometimes long e-mail subject. ;) -- Kaitlin Rupert IBM Linux Technology Center karupert@us.ibm.com
I think I'm going to add one more thing, since I just noticed Dan do it and I like it. For the first section: 5. If you have to rework and resend a patch, the commit message should reflect what changed since you last sent the patch. Nobody likes having to diff two diffs. -- -Jay
JG> 5. If you have to rework and resend a patch, the commit message JG> should reflect what changed since you last sent the patch. Nobody JG> likes having to diff two diffs. Obviously, I agree with this point :) -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Jay Gagnon wrote:
4. Before you type, "hg email," you should always type, "hg export," first. Review your patch. Does it have any typos in the comments? Did you accidentally include an irrelevant change? Is your commit message still accurate and useful?
What am I doing wrong, if "hg export" returns with: abort: export requires at least one changeset I'd like to suggest to add another thing to section 1 6. If a discussion for a certain patch (set) identified an issue, that will be fixed in a separate patch (set) and by another developer, but the second patch (set) will touch same code parts as the first patch (set), its recommended to wait with the second one until the first patch (set) was finished and accepted. This avoids reworking patches because of problems during apply. -- Regards Heidi Eckhart Software Engineer Linux Technology Center - Open Hypervisor heidieck@linux.vnet.ibm.com ************************************************** IBM Deutschland Entwicklung GmbH Vorsitzender des Aufsichtsrats: Martin Jetter Geschaeftsfuehrung: Herbert Kircher Sitz der Gesellschaft: Boeblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
HE> What am I doing wrong, if "hg export" returns with: HE> abort: export requires at least one changeset The patch must be applied before you can export it. You can also just do "less .hg/patches/foo" to see the patch if it's not applied. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
HE> What am I doing wrong, if "hg export" returns with: HE> abort: export requires at least one changeset
The patch must be applied before you can export it. You can also just do "less .hg/patches/foo" to see the patch if it's not applied.
I think I was unclear. Typing "hg export" or "hg email" alone will not do anything; both require a patch name for their argument. In other words, if your patch is named alloc_cap_fix, type "hg export alloc_cap_fix" to check it over, then "hg email alloc_cap_fix" to email it. -- -Jay
participants (4)
-
Dan Smith -
Heidi Eckhart -
Jay Gagnon -
Kaitlin Rupert