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

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

[Carlos O'Donell]

Community,

Several senior glibc developers are doing  an amazing to cleanup glibc
sources. There is a lot of much-needed refactoring, cleanup, and
rework that needs to be done to remove alloca, duplicated headers,
redundancy between ports etc.

I would like to see this work proceed *more* quickly during early
development of a new branch to allow machine maintainers to check if
it breaks things.

To that end I propose we approve via consensus the concept of a set of
changes and allow the maintainer to commit across the entire code base
to implement the concept without having to go through a second round
of review. The initial review of the concept is approval enough.

(1) Post a patch detailing what you want to change and refactor and why.
- MUST include [CONCEPT] or something so anyone can filter it and
respond immediately for review.
- MUST include a breakdown of the stages the work would happen.
- MUST include information about performance implications.
- MUST describe standards compliance implications.
- MUST include details for changes to machine implementations.

(2) Give some time for consensus to appear around the concept.

(3) Start implementing the changes following the stages of the work
you outlined.
- Do not wait for review.
- Commit the cleanup patches in logical sequence that yields a tree
that always builds and is always clean for testing unless you outlined
exceptions in (1).

I would love to see more great cleanups being done early and
efficiently followed by more time for machines to cleanup if we have
fallout.

Thoughts?

Cheers,
Carlos.

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

[Joseph Myers]

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

> To that end I propose we approve via consensus the concept of a set of
> changes and allow the maintainer to commit across the entire code base
> to implement the concept without having to go through a second round
> of review. The initial review of the concept is approval enough.

I think it's very rare that this would be appropriate.  Consensus on a 
high-level concept is very different from consensus on the particular 
details of changes - furthermore, various aspects of the details of the 
changes will only emerge as they are implemented, and it's quite possible 
that something seems attractive at the high-level design level but the 
details of the changes result in the whole idea seeming less attractive to 
the community or the design needing to be revisited.

Only in some of the most purely mechanical cases (e.g. removal of 
conditional code after we next increase the minimum supported Linux kernel 
version) might the concept make the underlying changes clear enough.  In 
most cases, such a change runs into several special cases that need 
individual attention and review, even if the bulk of the patches are 
mechanical.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Carlos O'Donell]

On Fri, Feb 14, 2020 at 12:23 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Fri, 14 Feb 2020, Carlos O'Donell wrote:
>
> > To that end I propose we approve via consensus the concept of a set of
> > changes and allow the maintainer to commit across the entire code base
> > to implement the concept without having to go through a second round
> > of review. The initial review of the concept is approval enough.
>
> I think it's very rare that this would be appropriate.  Consensus on a
> high-level concept is very different from consensus on the particular
> details of changes - furthermore, various aspects of the details of the
> changes will only emerge as they are implemented, and it's quite possible
> that something seems attractive at the high-level design level but the
> details of the changes result in the whole idea seeming less attractive to
> the community or the design needing to be revisited.

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.

In some ways a high-level review of a concept is an extension of trust
to the developer. We will follow a consensus driven model but the
consensus is at the architectural level, and trust is extended that
they will carry out an implementation within the shared understanding
we have regarding the code base.

My opinion is that it is costing us too much review time to cleanup
and refactor. These operations should have less implementation review
and more conceptual review.

> Only in some of the most purely mechanical cases (e.g. removal of
> conditional code after we next increase the minimum supported Linux kernel
> version) might the concept make the underlying changes clear enough.  In
> most cases, such a change runs into several special cases that need
> individual attention and review, even if the bulk of the patches are
> mechanical.

For senior developers in the community, like Florian and Adhemerval
what does our review of their implementation details actually gain us?

Their implementations are usually very high quality, but might not
exactly match what I want to see in glibc, but the reality is that
likely either implementation achieves a high degree of success. Only
with the additional review cost. What did that additional delay and
review cost and what do we gain from it?

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?

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

Cheers,
Carlos.

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

[Joseph Myers]

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

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

[Carlos O'Donell]

On Fri, Feb 14, 2020 at 4:31 PM Joseph Myers <joseph@codesourcery.com> wrote:
> > 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.

Let me reiterate your suggestion then, which is somewhat of an
inversion of my suggestion.

Reviewers, like myself, should consider reviewing *less* when it is
clearly evident that additional review is not required?

I can accept that such a  solution yields a faster turnaround for
patch review, but the latency may still exist, that is that developers
are waiting around for patches to be reviewed.

Cheers,
Carlos.

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

[Joseph Myers]

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

> Reviewers, like myself, should consider reviewing *less* when it is
> clearly evident that additional review is not required?

Yes.  Consider what it is about the patch that is relevant to review, 
taking into account factors such as existing consensus for an overall 
design, any relevant expertise of the patch submitter and the quality and 
contents of the commit message and the testing described in that commit 
message.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Carlos O'Donell]

On Fri, Feb 14, 2020 at 5:25 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Fri, 14 Feb 2020, Carlos O'Donell wrote:
>
> > Reviewers, like myself, should consider reviewing *less* when it is
> > clearly evident that additional review is not required?
>
> Yes.  Consider what it is about the patch that is relevant to review,
> taking into account factors such as existing consensus for an overall
> design, any relevant expertise of the patch submitter and the quality and
> contents of the commit message and the testing described in that commit
> message.

I appreciate your points about the complexity of implementations.

I will try to review less and see how it goes :-)

Cheers,
Carlos.