> There clearly are people who think reviewing something large in small chunks > is categorically easier, but, I don't completely understand the reasoning. > > 1) It seems to me that whether the small chunks are easier to actually > understand, depends on whether the chunks can really be understood in > isolation. Sometimes, that's not really the case. It's well known from Linux > kernel development that not infrequently, splitting a logical change actually > makes review and intake more difficult, producing extra workload on > submitters who are asked to make chunks build separately, bisectable, etc. > > 2) When a set of chunks could have been understood in isolation, then a > competent reviewer can probably look at sets of related, changed files > (navigating to them in the review tool), with the added benefit that the > reviewer can jump to any other part of the logical change to compare > context.
I agree in part with you, and I do think the kernel community sometimes takes granularity too far. Of course in a land of e-mail patch review, clearly too large a patch is impossible to review in e-mail. I think there's a balance. I know I'm still working to find that balance for myself. There are times when a single huge patch is certainly appropriate (new FSAL is a great one). I also like the "introduce a new function in one patch, and then modify places to call it in another patch, or even multiple patches if there's a lot of changes". One nice thing about the multiple patches for multiple changes across a wide space is for example, say I'm making a major FSAL API change. If each FSAL can be changed in a separate patch, then when FSAL_GPFS is Acked by Marc, I know that piece is done. If all the FSALs are in one patch, when Marc Acks, that doesn't tell me anything about the acceptability of the change as a whole (and we don't easily know whose Ack is missing - though that would be solved by having a required reviewers list). What I would ask people to do is be considerate of the reviewers. Break up large features into sensible patches. Avoid combining small changes unless they are trivial (I don't need to see 20 patches because you found 20 one or two line bugs while running some test suite - go ahead and give me one patch for those). Note also that with git-review or other mechanism of pulling the patches into a local repo, it is then possible to review the patch set as a single diff (and I actually do that sometimes, though actually what I look at is the final result). Frank > ----- "Frank Filz" <ffilz...@mindspring.com> wrote: > > > > From: Malahal Naineni [mailto:mala...@us.ibm.com] DENIEL Philippe > > > [philippe.den...@cea.fr] wrote: > > > > issue. If I do a big patch or 10 small ones, all of my changed > > files > > > > will have to be reviewed, which does have no impact on the > > workload. > > > > In fact, one big patch is a cool situation : it is easy to rebase, > > and > > > > it depends only on stuff already pushed and landed. If I publish > > 5 > > > > patches, what if patch 1, 2 and 3 merge fine, but 4 produce a > > conflict > > > > ? How could I rebase 4 without touch 1,2 and 3 ? This leads to a > > > > dependencies maze, and this is precisely the situation we fell > > into. > > > > > > There is no doubt that a "well" split patchset is easier to review. > > I did > > rebase > > > mega patches from others (that happens when you pick up someone > > else's > > > work) in the past and it is a PITA. Even if it is my code, I find it > > lot > > easier to > > > rebase small patches than one big patch. > > > > > From: Dominique Martinet [mailto:dominique.marti...@cea.fr] > > > > - we should provide big and squashed patches, one per > > feature. > > > > For example, CEA will soon push a rework of FSAL_HPSS, this will > > be a > > > > single commit. > > > > > > We do that for HPSS because the FSAL is old, has been pruned out a > > while > > > ago, and currently doesn't checkpatch/individual patches won't > > checkpatch. > > > That's purely selfish and we don't actually expect anyone to review > > that > > code > > > anyway, since no-one in the active community will use it - it's > > pretty far > > from > > > normal workflow submitting... > > > > > > > > > I believe patches should be kept reasonably small for review, > > really, but > > > "reasonably small" is subjective and flexible. > > > what we really need is for more people to review and that's not > > going to > > > improve if we push big hunks :) > > > > I absolutely agree. Maybe we don't need to have the patch granularity > > as fine as Linux kernel, but I like to see smaller patches. They also > > make for more bisectable code since we can see "ok, this part of the > > feature caused the problem" rather than "hmm, there's a problem > > somewhere in this new huge feature." > > > > Frank > > > > > > > > ---------------------------------------------------------------------- > > -------- One dashboard for servers and applications across > > Physical-Virtual-Cloud Widest out-of-the-box monitoring support with > > 50+ applications Performance metrics, stats and reports that give you > > Actionable Insights Deep dive visibility with transaction tracing > > using APM Insight. > > http://ad.doubleclick.net/ddm/clk/290420510;117567292;y > > _______________________________________________ > > Nfs-ganesha-devel mailing list > > Nfs-ganesha-devel@lists.sourceforge.net > > https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel > > -- > Matt Benjamin > CohortFS, LLC. > 315 West Huron Street, Suite 140A > Ann Arbor, Michigan 48103 > > http://cohortfs.com > > tel. 734-761-4689 > fax. 734-769-8938 > cel. 734-216-5309 ------------------------------------------------------------------------------ One dashboard for servers and applications across Physical-Virtual-Cloud Widest out-of-the-box monitoring support with 50+ applications Performance metrics, stats and reports that give you Actionable Insights Deep dive visibility with transaction tracing using APM Insight. http://ad.doubleclick.net/ddm/clk/290420510;117567292;y _______________________________________________ Nfs-ganesha-devel mailing list Nfs-ganesha-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/nfs-ganesha-devel
