Hi, Le mardi 19 mai 2015 à 15:49 -0400, Doug Ledford a écrit : > On Tue, 2015-05-19 at 21:47 +0300, Or Gerlitz wrote: > > On Tue, May 19, 2015 at 3:22 PM, Doug Ledford <[email protected]> wrote: > > > On Tue, 2015-05-19 at 07:35 +0300, Or Gerlitz wrote: > > >> >> I pulled that via a bundle from patchworks. I'll double check it. > > >> > Did you check it out? fixed it out? > > >> > > >> I took a look now, you've rebased to-be-rebased/for-4.2 to 4.1-rc4 and > > > > > > Correct. > > > > > >> it seems this is what you are going to push into the kernel.org treem > > > > > > Correct. > > > > > >> but this series is still there with the zillion tested/reviewed/etc > > >> signature per one 2-3 patch, I think we've agreed this needs to be > > >> addressed prior to the upstream push, right? > > > > > > Incorrect. What you objected to before was the large Cc: list in the > > > patches. That is gone. What is there now is just the reviewed-by: list > > > of three people, and the tested-by list of two people. As the entire > > > patch set as a whole was reviewed and tested by those people, it seems > > > accurate to me. > > > > Doug, I have never ever seen a patch set (specifically the 15~23 part > > of it) with that level of simplicity > > That portion of the patchset didn't start out with that level of > simplicity per patch, it evolved to that because it made review > *significantly* easier. It's very simple to review a patch that does: > > Add 1 helper > Replace tests in code with just that 1 helper > > because you can scroll through that patch and know that every line being > replaced is related to that one helper. If you want to know every line > that was replaced with rdma_cap_iw_cm, you go to that one patch and it's > all listed very easy to read. > > On the other hand, when you squash all those patches together, review > becomes much harder because if you want to see what a single helper > does, you have to sift through all of the other helper changes and hope > you find the right ones, and that you found all of the right ones. > > > and > > signature/reviewers/tested-by/etc inflation. > > I added those myself as part of an automated addition. It applied to > the entire series, so it was put on each patch. The people that > tested/reviewed the series did not do so to individual patches, they hit > all of them. And as was pointed out a couple of weeks ago on an earlier > patchset I picked up, it is generally good behavior to give attribution > where it is due to encourage people to participate. >
I agree with all the above replies from Doug: - small patches are easier to read and review: I prefer reviewing 10 littles patches than one big patch. - Signed-off-by:, Reviewed-by:, Tested-by: and Cc: are required too so that when it come to modifying existing code, we can contact people previously involved for reviews, tests, advice, etc. (I usually add Link: with a reference to the e-mail so that one commit can be traced back to the related patch submission and discussions (cover-letter !), and, to be able to trace back all patch iterations, I add a link to the previous patchset submission, and so on). Regards. -- Yann Droneaud OPTEYA -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
