The Gentle Art of Patch Review

As the next round of the FOSS Outreach Program for Women (OPW) approaches, my mind turns to mentorship, and lessons learned when dealing with newcomers to open source projects.  Many open source contributors have been in the FOSS community for long enough to forget how painful their first experience contributing to a project was.  As the coordinator for the Linux kernel OPW internships, I get to help newcomers go through that experience every six months.  I’ve learned a lot about how we, as open source reviewers, maintainers, and mentors, can help newcomers during their first contributions, and I’d like to share some of the perspective I’ve gained from OPW.

The Newcomer’s Perspective

As a newcomer, you’ll come at the project with enthusiasm and determination to do your best to make a really good first contribution.  You’ll try to find all the documentation for the project you’re working on, and read through it, only to realize it’s completely outdated and incomplete.  You’ll ping mentors and ask questions, but you may not be able to reach the right person to answer your question.  So you do the best you can with the resources you find, cross your fingers, and submit your first contribution.

It’s common for newcomers to blame themselves when they make mistakes in their first contributions.  You’ll cringe, wring your hands, smack your forehead, or maybe even put your head in your hands.  Then you’ll sigh and try again.  No matter how good the documentation for contributing to the project is, how meticulous you are, you will slip up at some point.  And that’s fine, because you are going through a process of learning something new, and expanding your skills.  The most productive contributors see each mistake they make as a growth opportunity, instead of a personal failure.

The Maintainer’s Perspective

As a long-standing open source contributor, you may get contributions from newcomers all the time.  You’ll see several of them make the same mistakes over and over again, and if you have enough time, you’ll update your project documentation to help people avoid those mistakes.  Often you don’t have time, and the documentation doesn’t get updated.  Or you’ll think that something is so blindingly obvious that everyone should understand it, without realizing how much specialized experience you need to have that knowledge.

At some point in as a maintainer, you will be completely overloaded with contributions from both newcomers and familiar, trusted contributors.  It’s easy to review those contributions from long-standing contributors, because they know your expectations and the rules around contributing.  You trust them to write solid code containing very few bugs.  So you review the contributions from trusted contributors, and put off reviewing contributions from newcomers until you have a large block of time to thoroughly review the newcomer’s contribution.

It’s tempting to just go through the newcomer’s contribution from start to finish, commenting on every single thing they missed.  The maintainer’s mindset is, “Ok, I have time, I should share my knowledge with this person who is obviously missing some tribal knowledge they need to contribute to my project.”  From the newcomer’s perspective, what they experience is their contribution being ignored for days or even weeks, followed by a very long email full of nit-picky comments on coding style, criticism of their code structure, and even comments about their spelling and grammar.  Even if the review is fair and neutrally worded with a focus on their technical mistakes, it still feels very harsh.

We Can Do Better

How can we make this process better on both sides?  How can we make the first patch review less harsh, and still respect the maintainer’s valuable time?  Can we make turn around time on first patch review even shorter?  When I was the xHCI driver maintainer, I started experimenting with a different way of reviewing contributions from newcomers that I think might help address all three of these issues.

The Three-Phase Contribution Review

Instead of putting off reviewing first-time contributions and thoroughly reviewing everything in the contribution at once, I propose a three-phase review process for maintainers:

  1. Is the idea behind the contribution sound?
  2. Is the contribution architected correctly?
  3. Is the contribution polished?

You can compare these contribution review phases to the phases of building a new house or taking on a remodeling project.  The first phase is a simple yes or no on the architectural diagram, the big idea of the contribution.  The second phase is getting all the structural issues correct and making sure the plumbing and electrical all connect properly.  The third phase is making everything polished, sanding off the rough corners, and slapping on a coat of paint to match whatever color the bike shed is currently painted.

Phase One: Good or Bad Idea?

The first phase of the contribution review should only require a simple yes or no answer from the maintainer: “Is this contribution a good idea?”  If the contribution isn’t useful or it’s a bad idea, it isn’t worth reviewing further.  The best action in this case is to refocus the newcomer on a better idea or a completely different area they could work on.  Or open a discussion with the newcomer and other contributors as to what should be done to address the issue in a different way.

If the contribution is worthwhile, but you don’t have time to go onto the second phase of patch review, do NOT say nothing.  Instead, drop the contributor an email that says, “Thanks for this contribution!  I like the concept of this patch, but I don’t have time to thoroughly review it right now.  Ping me if I haven’t reviewed it in a week.”  This builds the newcomer up by expressing appreciation for the time and effort they put into creating this contribution, and lets them know they’re on the right path.  It also gives you incentive to actually move onto phase two, because the contributor will bug you again if you haven’t reviewed the contribution.

Phase Two: Is this Architecturally Sound?

In phase two, you review the contribution to see whether the code (and only the code) is architecturally correct.  Focus on whether the code is sound at an architectural level. Is the code behavior correct?  Are they modifying the right functions, or does the code need to be moved around?  Have they structured their build files correctly?  Do they need to refactor any code?  Do they need to get buy-in on the code structure from other maintainers?  Are there potential hazards or tricky parts of the code that the everyone needs to review carefully?

You will need to squash the nit-picky, perfectionist part of yourself that wants to comment on every single grammar mistake or code style issue.  Instead, only include a sentence or two with a pointer to coding style documentation, or any tools they will need to run their contribution through.  If their patch needs to be updated against a newer version of your project, or a different maintainer’s upstream repository, point that out.  Avoid nit-picking every instance where they violate your project’s contribution style rules. Your eyeballs may be bleeding from the number of camel case variable names or variables names that use variable type encoding, but take a deep breath and ignore that.  Let them explore the tools, documentation, and fix (most) of their mistakes on their own.

Double check and make sure the documentation and tools actually document the mistakes you see in the code, and if they don’t, update them.  Your documentation and tools should clearly spell out the format of a valid contribution, and if they don’t, you need to address that technical documentation debt.  If you don’t have time to address that technical documentation debt, tell the contributor what needs to be fixed, and see if they have the time to address it.  Don’t be silent just because you don’t have time to fix it.

Phase Three: Is the Contribution Polished?

From a newcomer’s perspective, after phase two is complete, they’re hooked on getting their contribution in.  You’ve worked with them on an architectural level, and they know you want to accept their contribution.  They’re emotionally invested in getting their contribution into your project, and they’ve learned a lot by going through a couple contribution revisions.  Thank the contributor for being patient this far and remind them that you’re willing to accept the contribution, but they need to clean up a few small things first.

Now is the time for phase three: the polishing phase.  In this phase, you finally get to comment on the meta (non-code) parts of the contribution.  Correct any spelling or grammar mistakes, suggest clearer wording for comments, and ask for any updated documentation for the code.  It doesn’t make sense to create documentation for the code until the code is structurally sound, which is why the documentation phase comes last.  You may also need to encourage them to write a better commit message, mark the patch to be back ported to stable versions of your software, or Cc the right maintainers.

As a newcomer, this third and final phase can be more painful than the architectural critiques in the second phase.  Many young programmers lean towards science, math, and technology because they feel like they don’t excel in writing or people skills.  Contributors may also be writing in a language that is not their native tongue.  That’s why this nit-picky phase comes last, so that the contributors get over their embarrassment after they’re emotionally invested in getting their patches into your project. Be gentle, patient, and compassionate.  As a maintainer, you may suggest comments or patch descriptions that you hope the contributor simply copy-pastes into their patch.  You may have to just edit the patch description yourself.

How Does This Benefit Maintainers?

I’ve found that this three-phase contribution review process saves me (as a maintainer) a lot of mental stress.  The first phase is a simple yes or no question (“Is this a good or bad idea?”), which means I don’t procrastinate on reviewing first time contributions.  Being up front with contributors about not having time to review their contribution can initially feel like shirking duties, but I feel a mental load lifting when I get over that and simply say something like, “Hey, this patch looks like a good idea, but I don’t have time to review it right now. I’m heading to a conference next week, and need to work on my slides.  Can you ping me in two weeks if I haven’t reviewed your code?”

If you’re honest with contributors about your time commitments, they know their contribution is wanted, and they can pass your time commitments onto their boss or program manager.  Also, if you find yourself delaying contribution review often, it may be a sign you need a co-maintainer or you need to ask other contributors to do more code review.

The absolute worst thing you can do during phase one is be completely silent.  The newcomer doesn’t know whether their contribution is a good or bad idea, and any discussion that needs to happen with other maintainers to modify the fundamental concept never happens.  That’s why phase one is a simple yes or no answer, in order to get the code review ball rolling.

I’ve also heard some maintainers state that they want to dump all their review into phase two.  They have precious little time, and they fear they will forget specific feedback if they break code review into several phases.  I will often notice nit-picky coding style issues during my architectural review, and I will make a note to myself to nip that pattern in the bud in phase three.  Keeping a dated text file per patchset or even replying to the patch but only adding your own email address in the To field will help you keep track of the issues that need to get addressed in phase three.

Often by the time you get past the architectural discussion in phase two, you’ll find many of your initial nit-picky criticisms were addressed.  A conscientious contributor will look at the documentation and tools you point out in phase two, and will address most of them in their next revisions.  What will be left for the third (polishing) phase is mistakes made because of undocumented tribal knowledge, or rules that are undocumented because they differ from maintainer to maintainer within the project.

Try It Out!

The following three-phase contribution review process should help both maintainers and newcomers:

  1. Is the idea behind the contribution sound?
  2. Is the contribution architected correctly?
  3. Is the contribution polished?

Maintainers will be able to respond more quickly to contribution review if they focus on just answering one question during the first phase of review: “Is this a good or bad idea?”  Newcomers will be encouraged by a timely email that states whether the basic concept of their patch is sound.  Both the maintainer and the contributor benefit from splitting the actual code review into an architectural discussion, followed by a polishing phase.  Maintainers will save themselves time if they simply point out documentation and tools contributors should use to ensure their contribution is up to community standards, and the nit-picky polishing phase is saved for after the newcomer is emotionally invested in getting their contribution into your project.

I think this process should both save maintainers time, and decrease the bounce rate for newcomers, so I encourage you to try it out!

2014 Kernel Internship Report (OPW)

For the past year and a half, the Linux kernel has participated as a project under the FOSS Outreach Program for Women (OPW). OPW provides a three month paid internship for women (cis and trans) and genderqueer or genderfluid people. After a month-long application process, the selected OPW interns are paired with an open source mentor to work on a project. As of August 2014, there are eleven Linux kernel OPW alumni, and five interns that are just finishing up their internships.

The results from the past three OPW rounds are stunning:

  • 1,092 patches accepted into the Linux kernel from OPW alumni and interns
  • Lines of code added and deleted: +32,327, -193,938
  • OPW was a top contributor for the 3.11, 3.12, 3.13, and 3.14 kernels

The sheer number of patches the OPW kernel interns and alumni have created is impressive. They’ve been in the LWN top kernel contributor statistics since the program started in the 3.11 kernel, and they continue to be a top contributor despite the lack of published data for the 3.15 and 3.16 kernels. Making it over the thousand patch mark is a cause to celebrate.  More importantly, the OPW kernel interns and alumni have deleted six times more code than they added. They’re deleting dead code and unused drivers, and thus removing bugs from the Linux kernel.

The statistics from the code development efforts from the OPW kernel interns and alumni are impressive. However, contributing to open source isn’t just about writing code. It’s about interacting on mailing lists, reviewing code, writing documentation, answering questions, working on graphical design, maintaining project websites, and so much more.

The main goal of the OPW internship program is to create a long-term relationship between the mentee, the mentor, and their open source community, in order encourage minorities to continue to contribute to open source. How are we progressing towards the goal of creating more women kernel developers? Are the women who complete OPW kernel internships continuing to work on open source projects after their internship ends? Do they find jobs where they can be paid to work on open source?

In order to measure this, I created a longitudinal study to measure open source contributions of OPW alumni. I’ll send out the survey every 6 to 12 months, and compare the results of the program over time. The most recent survey results from our eleven Linux Kernel OPW alumni shows the program is successful at encouraging women to continue to participate in open source.

Graph of the monthly FOSS contributions from OPW kernel alumni

At least monthly, OPW alumni are engaging and contributing to open source communities. Most of them participate through code submission, testing, and discussion on mailing lists, IRC, or forums. However, it’s interesting to note that a few of the OPW alumni have stepped into open source leadership positions, either by reviewing contributions, maintaining a project, or by managing a team of open source contributors.

Another exciting result of OPW is that some of the kernel OPW alumni are getting paid to work as Linux Kernel developers. Teodora Băluţă is working on Android kernel drivers for Intel’s Open Source Technology Center. Lisa Nguyen is working for Linaro on ARM power management. Lidza Louina is a kernel developer at Oracle.  Xenia Ragiadakou also works on Linux kernel power tuning at OnApp. Elena Ufimtseva isn’t being paid to work on the Linux kernel, but she is working on a proprietary project at Citrix.

I’m overjoyed that these women have found jobs in the technology sector, and so many of them are paid to work as Linux kernel developers. This fact is heartening to me because some of the women that participated in OPW were working in retail before their internship. To be able to move into the technology sector or be hired as a Linux kernel developer is a giant step in the right direction, and I’m happy that the OPW program could be a part of that.

It’s exciting to see five of the eleven OPW kernel alumni get jobs in the technology sector. Four of the kernel OPW kernel alumni are still working their way through Bachelor’s or Master’s degrees. Two OPW kernel alumni are actively looking for jobs. If you need to hire a junior kernel developer, please email the opening to sarah dot a dot sharp at intel dot com, and I will pass the job description onto our OPW alumni.

I will continue to coordinate the Linux kernel mentors and interns under the FOSS Outreach Program for Women (OPW).  The next internship period will run from December 9, 2014 to March 9, 2015.  Applications for the next round open September 8th, and the Linux kernel contributions will be due October 31, 2014.  (Most OPW projects have a deadline of October 22, but the kernel project application process will be on hiatus from October 10 to 20 because many mentors will be attending LinuxCon Europe, Embedded Linux Conference Europe, and Linux Plumbers Conference.  Apply and get your kernel patches in early!)

If you’re interested in applying to be an OPW intern, you can find more information on how to apply on the OPW homepage, and on the OPW kernel project page.  Please note that you do not have to be a student to apply to OPW.  The only requirement is that you’re able to work full-time during the internship period, and that you are a woman (cis or trans), or a genderqueer or genderfluid individual.  This round, we’re also running a pilot to explore opening up the project to other under-represented minorities in tech, by allowing alumni from the Ascend Project to apply.  If the pilot is successful, we’ll be able to expand OPW to encourage chronically underemployed, LGBTQ, Latin@, and African American populations to participate in open source.

[Update 08/26: Fixed this post to note that both Lidza and Xenia are being paid to work as Linux kernel developers.]

Progress on Graphics

I’m back from my 8-week sabbatical!  My roadtrip was a lot of fun, but now it’s time to get back to work.

In the month I’ve been back, I’ve been making good progress on learning about ChromeOS and graphics.   I have some really awesome, patient mentors (Josh Triplett and Chad Versace) who have been helping me learn about the ChromeOS build system and graphics.  At this point, I can build and install my own ChromeOS images and individual packages.  The build system is both complex and kinda crufty, like some sort of horribly rusty swiss army knife.  (Really, you have two separate build systems for subversion and git projects?)  However, I’m really impressed with ChromeOS test suite and the ability to kick off remote tests.  If only the Google developers would stop breaking the build…

On the graphics side, four of my patches got merged into mesa and piglit to add support and tests for a ChromeOS-specific EGL extension.  Chad has been creating graphics tutorials to introduce both Josh and I to userspace graphics concepts.  There’s a lot of new concepts to learn, everything from vertices and model transforms to relearning my Linear Alegebra.

Getting to work in userspace again has been so much fun.  Being able to use tools like strace and gdb to step through the code simply isn’t possible when you’re in a kernel interrupt service routine.  Some days I do miss hardware, poking at bits to make lights blink, but it’s really gratifying to see the tangible graphics work on my screen.  I’ve completed the GL version of “hello world” and completed Chad’s tutorial to make a triangle appear on the screen.  It was really satisfying to figure out how to make it spin around the Z axis.

Onwards to my next adventure!

I’m joining the Intel OTC ChromeOS differentiation team, and transitioning maintainership of the xHCI driver over to Mathias Nyman.

I’ve only been officially on the team for a couple of weeks, but I’m already playing with webGL tutorials and learning about vertices, shaders, EGL extensions, piglet tests, and loads more about graphics.  It’s been really great working with Josh Triplett and Chad Versace. I managed to join the team in time to attend their group quarterly event at Ground Kontrol.  We played pinball, 80’s games, and DDR for hours. :)

Donate to the Ada Initiative!

Want to support women in tech? Donate to the Ada Initiative!

For the last two years, I’ve been going to the conferences the Ada Initiative has put on for women in open technology and culture. It’s a really awesome experience to be in a room full of hundreds of techie, geeky women. There’s everyone from open source developers to security analysts, hardware hackers to fan fiction writers. Heck, I even met a documentary producer at the last AdaCamp in San Francisco.

One of the most important things I learned at Ada Camp was how to combat impostor syndrome. It’s basically the feeling that you’re not really that smart, that your accomplishments are just luck, and some day, someone is going to find out, and you’ll get humiliated/fired/shunned. It’s surprising the number of highly successful tech women who experience this feeling.  I used to have the worst case of impostor syndrome, until the women at AdaCamp taught me how to fight it.

Continue reading

Summary on Civility

Peace, love, and Linux by flickr user amayita
The LKML thread where I stood up against verbal abuse has been winding down. I’ve posted a summary of my position. As I noted, I have been listening and learning from the arguments on the thread. In the course of the thread, my personal viewpoints have changed subtly, and I’ve chosen to push for change in areas where I think I might actually make headway. It wouldn’t be a discussion if no one changed their mind.

Nothing is going to change overnight in the Linux kernel community. As Casey Schaufler pointed out, I cannot force or demand change. I’m merely asking to discuss the possibly of change at the Linux Kernel Summit.

Thank you for listening and debating on this subject. Open discussion can only improve our community.

No more verbal abuse

I’m standing up against verbal abuse on LKML.  I will happily stand alone, however you can also support this cause.  Please speak up, either by resharing this post, or commenting on this post with words of support.  If you dare, you can also reply to my LKML email.

“Where do I put this fire? This bright red feeling? This Tiger Lily down my mouth? He wants to grow to 20 feet tall… I’m so tired of being shy; I’m not that girl any more. I’m not that straight-A anymore.”

Update

Examples of verbally abusive behavior on the Linux kernel mailing list:

Don’t be a Jerk: Responding to Ally Criticism

You are racist.  You are sexist. You are homophobic.

Now stop.  Analyze your response to my words.  Is your heart racing?  Do you feel tense, ready to fight?  Are you already in my comment section, blasting off a response about how you have plenty of black/gay/disabled/women friends and of course you don’t stereotype?  Are you ready to find holes in my argument and punch right through them?

If you want to be a true ally, you need to realize that this type of response is happening.  When someone questions you, or calls you biased, you immediately have physical and mental urges to defend yourself, to fight and stick up for yourself. This immediate defensive response is not conducive to having a well-reasoned discussion about whether you actually have a bias.  You are likely to shout at your ally, find excuses, and otherwise alienate them.  If you truly care about your allies, you need to learn how to suppress that response.

Continue reading

Preventing Violence Against Women

Trigger Warning: Violence Against Women, Rape & Victim BlamingThis week, Facebook came under fire for not pulling several pages that promote violence against women.  Pages like “Violently Raping Your Friend Just for Laughs” remained up, even after they were reported to Facebook.  After a dedicated campaign to get ad sponsors to pull their ads, Facebook said they would retrain staff to take down pages that promote gender-based violence.

That’s not enough, in my opinion.  Sending the message that violence against women isn’t socially acceptable on Facebook is a step in the right direction.  However, silencing the conversation on social media does not change how our culture views violence against women and rape.  Thoughts on how to prevent rape and violence are below the cut.

Continue reading

Linux Kernel Internships (OPW) Update

A month ago, Amanda McPherson and Greg Kroah-Hartman from the Linux Foundation asked me to coordinate an internship program aimed at getting more women to participate in the Linux kernel. In order to be considered for an internship, the applicants need to submit patches to the Linux kernel, and get them accepted.

The results have been amazing:

  • 41 women applied for 6 Linux kernel internships.
  • In 13 days, 374 patches were submitted, and 137 patches were accepted.
  • Diff stat for accepted patches:
    105 files changed, 3889 insertions(+), 4872 deletions(-)

Continue reading