Re: rdma kernel tree

[Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>]

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 <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html