>On 04/14/2014 12:42 AM, Robert O'Callahan wrote: >> On Sat, Apr 12, 2014 at 8:29 AM, Gregory Szorc <g...@mozilla.com> wrote: >> As a reviewer one of the first things I do when reviewing a big patch is to >> see if I can suggest a reasonable way to split it into smaller patches. > >As a reviewer I usually want to see _also_ a patch which contains all the >changes. >Otherwise it can be very difficult to see the big picture. >But sure, having large patches split to smaller pieces may help.
yes, especially if you have big, complex patchesets like roc often does. :-) Seriously, it helps a lot, and helps isolate out the "known-reviewed-parts" from the ones with questions as well. This helps when re-reviewing. Another thing that helps with re-reviewing is to provide not just the updated patch, but also interdiffs (since the automated interdiff tools usually fail). I develop fixes to an uploaded patch by adding a new patch to my queue and making all my mods there, uploading as an interdiff, and then qfolding it into the patch it modified and uploading the full patch. This makes it *so* much easier to re-review. >> Honestly, I think we're already pretty close to most of those >> recommendations, most of the time. "More descriptive commit messages" is >> the only recommendation there that is not commonly followed, as far as I >> can see. > >I always just use the link to the bug, so I haven't seen multiline comment >useful at all. >Mostly they are just annoying, making logs harder to read. I've always preferred them in the past - but those were also places without solid bug tool usage patterns. What I *do* recommend is that when uploading your patches to a bug (either in the form or via 'hg bzexport -c "comment"') is to write a descriptive comment about the patch and what it's doing and why, and how it changed from the earlier version. This helps a lot when reviewing the bug. And in general: be verbose in bugs. Throw an update in there with your design, plan, analysis, what you ate for lunch (ok, kidding). Don't just have a bug with <report>, 4 WIP patches obsoleted, final-patch, r?, r+ land, at least for anything of any complexity. -- Randell Jesup, Mozilla Corp remove "news" for personal email _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
