webchick.net

very helpful lioness

Diaries of a Core Maintainer #1: The Danger of Patch Context Switching

Sat, 09/13/2008 - 06:22 -- webchick

We've all been there. You sit down to write a "simple" patch. Maybe it upgrades a module to Drupal 6. Maybe it adds a small feature, or fixes a bug that's been annoying you for awhile.

You grab your caffeinated beverage of choice, pop open your text editor/IDE, and throw on your coding tunes. You search through the file for the lines you need to change and... sayyy... You know, the indentation is off a bit here. I'll just go ahead and fix that. Oops. And this function's PHPDoc is missing. No problem, a couple quick lines here. OH! That's clearly a bug. Better fix that while I'm at it. Hey, and that reminds me! I always kind of wished that this particular feature worked a different way. Might as well pop that in while I'm at it...

Suddenly your "simple" patch has grown into 30-40K of changes. It fixes the original thing you set out to fix, and it also makes several improvements to the code that you found as you were going through. Great! So what's the problem?

The problem is context switching.

It's hard work doing a good, solid patch review. You need to browse through the code and look for any weirdness. You need to download and apply the patch to your test site (and make a test site, if you don't have one yet). You need to actually test the changes and make sure that they do what they're supposed to do. And, if you're talking about Drupal 7 core, which is my life now ;), you also need to run the test suite, or at least the tests that you know for a fact are affected by the patch.

When you upload that horrendous, multi-headed, kitten-eating monster of a patch, suddenly that 15 minutes between phone calls isn't sufficient to get the patch reviewed. Now reviewers need to find more like 45 minutes or an hour between things, which is much harder to come by during the run of a day. Parts of the patch that make API changes that need careful attention and discussion are lost in the noise of minor whitespace fixes and other things, so the chances of not catching problems ahead of time is much higher. And the individual parts of the patch don't get ample attention, since they're mooshed together in one big glomp of diffs.

So please, when you upload patches, try to keep them to one "context" at a time (feature X, bug fix Y, minor cleanup, etc.). More patches will get committed sooner, more reviewers will help out, and kittens will live happily uneaten.

Thanks! ;)

Tags: 
drupal
drupal core diaries

Comments

Good to know how to best help the maintainers make Drupal 7 the best yet! I'd probably add that if your patch changes functionality, make sure it has a test to cover it. If not, write one! webhick will be your bestest friend ever if you do.

Submitted by sdboyer (not verified) on

So in case there weren't enough uses for the term "context..." :)

With all the tooling around I'm doing in Eclipse these days, I've been playing around a fair bit with Mylyn, which is one of those rare pieces of software that truly can blow you into a whole new paradigm faster than you can blink. Unfortunately it's not really usable for Drupal dev directly yet, for a few reasons, but one of the major things that it facilitates is setting up these sort of patch contexts you're talking about in a very real way. Simply put, Mylyn will let you explicitly define a context for a given issue # (and therefore, patch), then associate a specific set of files with that context. It then locks down Eclipse so that only stuff really pertinent to that patch context is visible in the GUI.

That alone is nice; it gets even cooler because you can then turn that context on and off with a simple toggle button. Toggle it off, and Mylyn closes all the open files associated with the patch context and you get your whole, un-locked-down GUI back. Toggle it back on, and it restores the entire patch context. Tasktop takes this an even nutsier step further...you can save browser windows, emails, etc., into patch contexts, too. Drupal project* integration with Mylyn is still a ways away, but I can also picture it being possible to embed Mylyn contexts into an issue itself (transparently to the web interface), so that when you start working on an issue via Mylyn, you can inherit the context that other people working on the issue have already created. Man, wouldn't _that_ be sexy...

Now, none of the Mylyn tools are capable of preventing patch drift/context switching WITHIN a file (how COULD they?) but I tend to find that when I'm actually explicitly using Mylyn, it's easier to keep myself from engaging in lazy patch drift.

I could also talk about git/bzr/hg(?) stash, or even crazier, git-cherry, but that's a whole different can of worms :)

Submitted by catch (not verified) on

Would be this patch which Benjamin Melancon and I worked on at the taxonomy sprint, and which grew pretty rapidly - http://drupal.org/node/305740. The actual functionality requires a 2k patch, cleaning up all the mess around that functionality was closer to 30k. We can probably manage to split it into a 2k patch for the UI, and 20k-ish patch (or a couple of 10k patches even) for the cleanup.

I'm going to flatter myself by thinking that the taxonomy patch that I turned into a monster and Catch shepherded in helped inspire this.

For the record: I don't drink any caffeinated beverages regularly, and as a vegan, I generally disapprove of eating kittens.

My expansion of that patch was written on a 20 hour train trip with no internet access.

I will restrict future contributions to one context at a time unless doing so would demonstrably slice a kitten in half.

Only other thing I want to say– contributing to core is awesome!

Just wanted to let you know that it's quite easy using a VCS like bazaar or Git to put part of your patch on a "shelve" or for bigger context switches, just create a new branch to keep the changes separated.
This also works for changes in the same file. If you are using only CVS, "patchutils" might help a lot, e.g. the "editdiff" tool: just copy the patch and edit it so that any irrelevant things are removed. Then submit it separately.