Tuesday, 20 January 2009

Tips For Getting Patches Landed

The checkin-needed queue is quite large these days. Here are a few tips for getting your checkin-needed patches landed quickly:


  • Produce your patch using "hg export" so it includes a commit message and your user identification. This not only makes the committer's job easier, it also means you're more likely to get the right attribution.
  • Make sure it's clear in the bug which patch(es) need to be committed.
  • Periodically check that it applies cleanly to the trunk and update the patch if it doesn't. If the patch is rotting because it's been stuck at checkin-needed too long, yell at someone.
  • If you've run tests locally (reftests, crashtests, mochitests, etc), and you say so in the bug, people are more likely to check your patch in quickly. You don't have to have run them all (few people can run them on all platforms anyway) but more tests -> better. This is especially important if previous versions of the patch have been backed out due to test failures.
  • Work on giant patches that fix huge problems --- I try to check those in quickly because they rot fast. And I'll love you for it.

Now, having said all that, it is our (Mozilla's) responsibility to be diligent about helping people get their patches landed. After all, it's their hard work just to get to the point of having a reviewed patch and we should respect that and be willing to clean up their patches and work around little mistakes or inefficiencies.

I'm enjoying hg's ability to bundle up a large set of patches to be committed as a unit. I often commit at night NZ time, which is very late US time, so I often get a set of build slaves of my very own and reasonably quick turnaround on builds and test runs for the whole bundle of patches. By being careful not to commit similar patches in the same bundle, it's usually quite easy to tell what's guilty if something goes wrong.



8 comments:

  1. Siddharth Agarwal20 January 2009 15:59

    Doesn't hg export produce a diff with CRLF line endings on Windows? That's the main reason I don't use it.

    ReplyDelete
  2. Robert O'Callahan20 January 2009 21:44

    Hmm, maybe it does, but "hg import" should still consume that correctly, I hope.

    ReplyDelete
  3. Good write up, I will try hg export next time I attach a patch.
    But is it really a good idea to include the commit message right away? Updating r+ flags comes to my mind there.

    ReplyDelete
  4. There are lots of free text editors that can convert Windows line endings to Unix ones too.

    ReplyDelete
  5. Robert O'Callahan21 January 2009 00:42

    Arpad: often people attach a "final patch for checkin" that addresses final review comments after review has been granted, so then it's fine to add the review flags.

    ReplyDelete
  6. roc, I wonder wether patches that need checkin but are in closed bugs (e. g. an additional reftest) get less attention than patches that are in bug's that are still open?

    ReplyDelete
  7. Robert O'Callahan24 January 2009 22:14

    Yeah, probably, since a quicksearch for "checkin-needed" wouldn't find them.

    ReplyDelete
  8. Hm, I though so. I wrote some reftests (and there are more to come) for bugs that I triaged. And those bugs are mostly closed now.

    ReplyDelete