On Fri, Jul 26, 2013 at 11:51:00AM +0200, Daniel Vetter wrote: > HI all, > > So Ben&I had a bit a private discussion and one thing I've explained a bit > more in detail is what kind of review I'm doing as maintainer. I've > figured this is generally useful. We've also discussed a bit that for > developers without their own lab it would be nice if QA could test random > branches on their set of machines. But imo that'll take quite a while, > there's lots of other stuff to improve in QA land first. Anyway, here's > it: > > Now an explanation for why this freaked me out, which is essentially an > explanation of what I do when I do maintainer reviews: > > Probably the most important question I ask myself when reading a patch is > "if a regression would bisect to this, and the bisect is the only useful > piece of evidence, would I stand a chance to understand it?". Your patch > is big, has the appearance of doing a few unrelated things and could very > well hide a bug which would take me an awful lot of time to spot. So imo > the answer for your patch is a clear "no". > > I've merged a few such patches in the past where I've had a similar hunch > and regretted it almost always. I've also sometimes split-up the patch > while applying, but that approach doesn't scale any more with our rather > big team. You should never do this, IMO. If you require the patches to be split in your tree, the developer should do it. See below for reasons I think this sucks. > > The second thing I try to figure out is whether the patch author is indeed > the local expert on the topic at hand now. With our team size and patch > flow I don't stand a chance if I try to understand everything to the last > detail. Instead I try to assess this through the proxy of convincing > myself the the patch submitter understands stuff much better than I do. I > tend to check that by asking random questions, proposing alternative > approaches and also by rating code/patch clarity. The obj_set_color > double-loop very much gave me the impression that you didn't have a clear > idea about how exactly this should work, so that hunk trigger this > maintainer hunch. > > I admit that this is all rather fluffy and very much an inexact science, > but it's the only tools I have as a maintainer. The alternative of doing > shit myself or checking everything myself in-depth just doesnt scale. > > Cheers, Daniel > > > On Mon, Jul 22, 2013 at 4:08 AM, Ben Widawsky <ben@xxxxxxxxxxxx> wrote: I think the subthread Jesse started had a bunch of good points, but concisely I see 3 problems with our current process (and these were addressed in my original mail, but I guess you didn't want to air my dirty laundry :p): 1. Delay hurts QA. Balking on patches because they're hard to review limits QA on that patch, and reduces QA time on the fixed up patches. I agree this is something which is fixable within QA, but it doesn't exist at present. 2. We don't have a way to bound review/merge. I tried to do this on this series. After your initial review, I gave a list of things I was going to fix, and asked you for an ack that if I fixed those, you would merge. IMO, you didn't stick to this agreement, and came back with rework requests on a patch I had already submitted. I don't know how to fix this one because I think you should be entitled to change your mind. A caveat to this: I did make some mistakes on rebase that needed addressing. ie. the ends justified the means. 3a. Reworking code introduces bugs. I feel I am more guilty here than most, but, consider even in the best case of those new bugs being caught in review. In such a case, you've now introduced at least 2 extra revs, and 2 extra lag cycles waiting for review. That assumes further work doesn't spiral into more requested fixups, or more bugs. In the less ideal case, you've simply introduced a new bug in addition to the delay. 3b. Patch splitting is art not science. There is a really delicate balance between splitting patches because it's logically a functional split vs. splitting things up to make things easier to chew on. Now in my case specifically, I think overall the series has improved, and I found some crud that got squashed in which shouldn't have been there. I also believe a lot of the splitting really doesn't make much sense other than for review purposes and sometimes that is okay. In my case, I had a huge patch, but a lot of that patch was actually a sed job of "s/obj/obj,vm/." You came back with, "you're doing a bunch of extra lookups." That was exactly the point of the patch; the extra lookups should have made the review simpler, and could be cleaned up later. My point is: A larger quantity of small patches is not always easier to review than a small quantity of large patches. Large patch series review often requires the reviewer to keep a lot of context as they review. *4. The result of all this is I think a lot of the time we (the developers) end up writing your patch for you. While I respect your opinion very highly, and I think more often than not that your way is better, it's just inefficient. I'll wrap this all up with, I don't envy you. On a bunch of emails, I've seen you be apologetic for putting developers in between a rock, and a hard place (you, and program management). I recognize you have the same dilemma with Dave/Linus, and the rest of us developers. I think the overall strategy should be to improve QA, but then you have to take the leap of limiting your requests for reworks, and accepting QAs stamp of approval. -- Ben Widawsky, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
