Hi, 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. Matt ----- "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
