Re: Consensus on high-level objective of cleanup, refactor or rework patches.

[Joseph Myers <joseph at codesourcery dot com>]

On Fri, 14 Feb 2020, Carlos O'Donell wrote:

> In the cases where implementation causes a deviation from what was
> expressed in the initial concept then I expect the trusted developer
> to come back to the community to seek consensus around the new
> problem.

I think the implementation is generally needed to understand better 
exactly what the concept is.

Maybe you're trying to get meta-consensus for a not-fully-defined 
meta-concept.  The sort of things I think of on reading your description 
are e.g. all the changes rearranging the code in preparation for 
supporting 64-bit times on systems currently using 32-bit times, or the 
preparation for binary128 long double on powerpc64le.  We have consensus 
on the overall design, but there are lots of issues of detail in 
individual patches that come up in review and show that we do need to keep 
watching and reviewing the individual patches, and monitoring what those 
patches imply for the overall design - it would not have been sensible to 
treat consensus on the design as consensus for particular patches that 
often have issues of their own.

> For example I like Zach's struct-splitting-into-header concept. Should
> we allow any patches that do that to just get committed with assumed
> consensus? Do we assume this is too disruptive?

That's one of the rare more mechanical examples.

If you take, say, Zack's no-nested-includes patch series (reducing the 
extent to which one public header includes another), when someone approves 
of the concept they are bringing with that approval their own 
understanding of what "should" be defined by particular headers, and may 
well find issues with a particular change where there seems to be a 
significant compatibility issue removing something from a header's 
interface; most patches in that series have their own special-case issues 
that are described in the commit messages.  So while in reviewing those 
patches it's probably more useful to concentrate on the special cases and 
choices described in the commit message, rather than the more mechanical 
parts of the patches, there are clearly things needing review in each 
patch.  (And distribution build testing would be helpful once those 
changes are in, to help detect compatibility issues that didn't show up in 
review.)

> Do you have any suggestions for reducing the cost of cleanups or refactoring?

Reviewers should consider carefully exactly what needs reviewing when 
reviewing a particular patch or patch series.  In some cases, especially 
given a good commit message, this may well concentrate on the higher-level 
choices and special cases involved in that particular patch, and on 
architecture- and OS-dependencies, and not the parts of the patch that are 
clearly implementing the mechanical parts of a previously agreed cleanup, 
unless those parts suggest a design issue that needs to be addressed.

-- 
Joseph S. Myers
joseph@codesourcery.com