[PATCH net v2] docs: netdev: document guidance on cleanup patches

[PATCH net v2] docs: netdev: document guidance on cleanup patches

[Simon Horman]

The purpose of this section is to document what is the current practice
regarding clean-up patches which address checkpatch warnings and similar
problems. I feel there is a value in having this documented so others
can easily refer to it.

Clearly this topic is subjective. And to some extent the current
practice discourages a wider range of patches than is described here.
But I feel it is best to start somewhere, with the most well established
part of the current practice.

--
I did think this was already documented. And perhaps it is.
But I was unable to find it after a quick search.

Signed-off-by: Simon Horman <horms@kernel.org>
---
Changes in v2:
- Drop RFC designation
- Correct capitalisation of heading
- Add that:
  + devm_ conversions are also discouraged, outside the context of other work
  + Spelling and grammar fixes are not discouraged
- Reformat text accordingly
- Link to v1: https://lore.kernel.org/r/20241004-doc-mc-clean-v1-1-20c28dcb0d52@kernel.org
---
 Documentation/process/maintainer-netdev.rst | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/process/maintainer-netdev.rst b/Documentation/process/maintainer-netdev.rst
index c9edf9e7362d..1ae71e31591c 100644
--- a/Documentation/process/maintainer-netdev.rst
+++ b/Documentation/process/maintainer-netdev.rst
@@ -355,6 +355,8 @@ just do it. As a result, a sequence of smaller series gets merged quicker and
 with better review coverage. Re-posting large series also increases the mailing
 list traffic.
 
+.. _rcs:
+
 Local variable ordering ("reverse xmas tree", "RCS")
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
@@ -391,6 +393,21 @@ APIs and helpers, especially scoped iterators. However, direct use of
 ``__free()`` within networking core and drivers is discouraged.
 Similar guidance applies to declaring variables mid-function.
 
+Clean-up patches
+~~~~~~~~~~~~~~~~
+
+Netdev discourages patches which perform simple clean-ups, which are not in
+the context of other work. For example:
+
+* Addressing ``checkpatch.pl`` warnings
+* Addressing :ref:`Local variable ordering<rcs>` issues
+* Conversions to device-managed APIs (``devm_`` helpers)
+
+This is because it is felt that the churn that such changes produce comes
+at a greater cost than the value of such clean-ups.
+
+Conversely, spelling and grammar fixes are not discouraged.
+
 Resending after review
 ~~~~~~~~~~~~~~~~~~~~~~
 

Re: [PATCH net v2] docs: netdev: document guidance on cleanup patches

[Przemek Kitszel]

On 10/9/24 11:12, Simon Horman wrote:
> The purpose of this section is to document what is the current practice
> regarding clean-up patches which address checkpatch warnings and similar
> problems. I feel there is a value in having this documented so others
> can easily refer to it.
> 
> Clearly this topic is subjective. And to some extent the current
> practice discourages a wider range of patches than is described here.
> But I feel it is best to start somewhere, with the most well established
> part of the current practice.
> 
> --
> I did think this was already documented. And perhaps it is.
> But I was unable to find it after a quick search.
> 
> Signed-off-by: Simon Horman <horms@kernel.org>

Looks like you wanted to say "please don't submit autogenerated clenups"

> ---
> Changes in v2:
> - Drop RFC designation
> - Correct capitalisation of heading
> - Add that:
>    + devm_ conversions are also discouraged, outside the context of other work

devm_ is generally discouraged in netdev, so much that I will welcome
the opposite cleanup :)

Your write-up on this is correct, no objections.

Perhaps we could say more about the status of the code that is fixed -
Maintained/Odd fixes/Orphaned - I would don't touch anything below
"Maintained" for good reason

>    + Spelling and grammar fixes are not discouraged
> - Reformat text accordingly
> - Link to v1: https://lore.kernel.org/r/20241004-doc-mc-clean-v1-1-20c28dcb0d52@kernel.org
> ---
>   Documentation/process/maintainer-netdev.rst | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/Documentation/process/maintainer-netdev.rst b/Documentation/process/maintainer-netdev.rst
> index c9edf9e7362d..1ae71e31591c 100644
> --- a/Documentation/process/maintainer-netdev.rst
> +++ b/Documentation/process/maintainer-netdev.rst
> @@ -355,6 +355,8 @@ just do it. As a result, a sequence of smaller series gets merged quicker and
>   with better review coverage. Re-posting large series also increases the mailing
>   list traffic.
>   
> +.. _rcs:
> +
>   Local variable ordering ("reverse xmas tree", "RCS")
>   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   
> @@ -391,6 +393,21 @@ APIs and helpers, especially scoped iterators. However, direct use of
>   ``__free()`` within networking core and drivers is discouraged.
>   Similar guidance applies to declaring variables mid-function.
>   
> +Clean-up patches
> +~~~~~~~~~~~~~~~~
> +
> +Netdev discourages patches which perform simple clean-ups, which are not in
> +the context of other work. For example:
> +
> +* Addressing ``checkpatch.pl`` warnings
> +* Addressing :ref:`Local variable ordering<rcs>` issues
> +* Conversions to device-managed APIs (``devm_`` helpers)
> +
> +This is because it is felt that the churn that such changes produce comes
> +at a greater cost than the value of such clean-ups.
> +
> +Conversely, spelling and grammar fixes are not discouraged.
> +
>   Resending after review
>   ~~~~~~~~~~~~~~~~~~~~~~

Re: [PATCH net v2] docs: netdev: document guidance on cleanup patches

[Simon Horman]

On Wed, Oct 09, 2024 at 11:42:14AM +0200, Przemek Kitszel wrote:
> On 10/9/24 11:12, Simon Horman wrote:
> > The purpose of this section is to document what is the current practice
> > regarding clean-up patches which address checkpatch warnings and similar
> > problems. I feel there is a value in having this documented so others
> > can easily refer to it.
> > 
> > Clearly this topic is subjective. And to some extent the current
> > practice discourages a wider range of patches than is described here.
> > But I feel it is best to start somewhere, with the most well established
> > part of the current practice.
> > 
> > --
> > I did think this was already documented. And perhaps it is.
> > But I was unable to find it after a quick search.
> > 
> > Signed-off-by: Simon Horman <horms@kernel.org>
> 
> Looks like you wanted to say "please don't submit autogenerated clenups"

:)

> > ---
> > Changes in v2:
> > - Drop RFC designation
> > - Correct capitalisation of heading
> > - Add that:
> >    + devm_ conversions are also discouraged, outside the context of other work
> 
> devm_ is generally discouraged in netdev, so much that I will welcome
> the opposite cleanup :)
> 
> Your write-up on this is correct, no objections.
> 
> Perhaps we could say more about the status of the code that is fixed -
> Maintained/Odd fixes/Orphaned - I would don't touch anything below
> "Maintained" for good reason

I agree that there is room to expand on that.  But I would rather defer on
that, because, as mentioned by Jakub in his review of v1, that quickly
becomes quite subjective.

IOW, let's start with something and improve on it later.

...

Re: [PATCH net v2] docs: netdev: document guidance on cleanup patches

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 09 Oct 2024 10:12:19 +0100 you wrote:
> The purpose of this section is to document what is the current practice
> regarding clean-up patches which address checkpatch warnings and similar
> problems. I feel there is a value in having this documented so others
> can easily refer to it.
> 
> Clearly this topic is subjective. And to some extent the current
> practice discourages a wider range of patches than is described here.
> But I feel it is best to start somewhere, with the most well established
> part of the current practice.
> 
> [...]

Here is the summary with links:
  - [net,v2] docs: netdev: document guidance on cleanup patches
    https://git.kernel.org/netdev/net/c/aeb218d900e3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html