[PATCH net-next] docs: netdev: clarify the need to sending reverts as patches

[PATCH net-next] docs: netdev: clarify the need to sending reverts as patches

[Jakub Kicinski]

We don't state explicitly that reverts need to be submitted
as a patch. It occasionally comes up.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: corbet@lwn.net
CC: linux-doc@vger.kernel.org
---
 Documentation/process/maintainer-netdev.rst | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/process/maintainer-netdev.rst b/Documentation/process/maintainer-netdev.rst
index e31d7a951073..f6983563ff06 100644
--- a/Documentation/process/maintainer-netdev.rst
+++ b/Documentation/process/maintainer-netdev.rst
@@ -184,11 +184,18 @@ Handling misapplied patches
 
 Occasionally a patch series gets applied before receiving critical feedback,
 or the wrong version of a series gets applied.
-There is no revert possible, once it is pushed out, it stays like that.
+
+Making the patch disappear once it is pushed out is not possible, the commit
+history in netdev trees is stable.
 Please send incremental versions on top of what has been merged in order to fix
 the patches the way they would look like if your latest patch series was to be
 merged.
 
+In cases where full revert is needed the revert has to be submitted
+as a patch to the list with a commit message explaining the technical
+problems with the reverted commit. Reverts should be used as a last resort,
+when original change is completely wrong; incremental fixes are preferred.
+
 Stable tree
 ~~~~~~~~~~~
 
-- 
2.39.2

Re: [PATCH net-next] docs: netdev: clarify the need to sending reverts as patches

[Jacob Keller]

On 3/27/2023 10:26 AM, Jakub Kicinski wrote:
> We don't state explicitly that reverts need to be submitted
> as a patch. It occasionally comes up.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: corbet@lwn.net
> CC: linux-doc@vger.kernel.org
> ---
>  Documentation/process/maintainer-netdev.rst | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/process/maintainer-netdev.rst b/Documentation/process/maintainer-netdev.rst
> index e31d7a951073..f6983563ff06 100644
> --- a/Documentation/process/maintainer-netdev.rst
> +++ b/Documentation/process/maintainer-netdev.rst
> @@ -184,11 +184,18 @@ Handling misapplied patches
>  
>  Occasionally a patch series gets applied before receiving critical feedback,
>  or the wrong version of a series gets applied.
> -There is no revert possible, once it is pushed out, it stays like that.
> +
> +Making the patch disappear once it is pushed out is not possible, the commit
> +history in netdev trees is stable.
>  Please send incremental versions on top of what has been merged in order to fix
>  the patches the way they would look like if your latest patch series was to be
>  merged.
>  
> +In cases where full revert is needed the revert has to be submitted
> +as a patch to the list with a commit message explaining the technical
> +problems with the reverted commit. Reverts should be used as a last resort,
> +when original change is completely wrong; incremental fixes are preferred.
> +

This is much clearer. It highlights that you won't rewind/modify
history, and explains the desire for incremental fixes better.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>  Stable tree
>  ~~~~~~~~~~~
>  

Re: [PATCH net-next] docs: netdev: clarify the need to sending reverts as patches

[Florian Fainelli]

On 3/27/23 10:26, Jakub Kicinski wrote:
> We don't state explicitly that reverts need to be submitted
> as a patch. It occasionally comes up.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: corbet@lwn.net
> CC: linux-doc@vger.kernel.org
> ---
>   Documentation/process/maintainer-netdev.rst | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/process/maintainer-netdev.rst b/Documentation/process/maintainer-netdev.rst
> index e31d7a951073..f6983563ff06 100644
> --- a/Documentation/process/maintainer-netdev.rst
> +++ b/Documentation/process/maintainer-netdev.rst
> @@ -184,11 +184,18 @@ Handling misapplied patches
>   
>   Occasionally a patch series gets applied before receiving critical feedback,
>   or the wrong version of a series gets applied.
> -There is no revert possible, once it is pushed out, it stays like that.
> +
> +Making the patch disappear once it is pushed out is not possible, the commit
> +history in netdev trees is stable.

I would write immutable instead of stable here to convey the idea that 
there are no history rewrites once the tree is pushed out. With that:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next] docs: netdev: clarify the need to sending reverts as patches

[Jakub Kicinski]

On Mon, 27 Mar 2023 12:19:14 -0700 Florian Fainelli wrote:
> I would write immutable instead of stable here to convey the idea that 
> there are no history rewrites once the tree is pushed out.

I'll s/stable/immutable/ when applying, thanks!

Re: [PATCH net-next] docs: netdev: clarify the need to sending reverts as patches

[patchwork-bot+netdevbpf]

Hello:

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

On Mon, 27 Mar 2023 10:26:46 -0700 you wrote:
> We don't state explicitly that reverts need to be submitted
> as a patch. It occasionally comes up.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: corbet@lwn.net
> CC: linux-doc@vger.kernel.org
> 
> [...]

Here is the summary with links:
  - [net-next] docs: netdev: clarify the need to sending reverts as patches
    https://git.kernel.org/netdev/net-next/c/e70f94c6c75c

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

Re: [PATCH net-next] docs: netdev: clarify the need to sending reverts as patches

[Thorsten Leemhuis]

On 27.03.23 19:26, Jakub Kicinski wrote:
> We don't state explicitly that reverts need to be submitted
> as a patch. It occasionally comes up.
>  [...]
> +In cases where full revert is needed the revert has to be submitted
> +as a patch to the list with a commit message explaining the technical
> +problems with the reverted commit. Reverts should be used as a last resort,
> +when original change is completely wrong; incremental fixes are preferred.
> +

FWIW, I see how this is well meant, but I'm not really happy with the
last sentence, as one of the problems I notice when handling regression
is: it sometimes takes weeks to get regressions fixed that could have
been solved quickly by reverting the culprit (and reapplying an improved
version of the change or the change together and a fix later). That's
why Documentation/process/handling-regressions.rst strongly suggest to
revert changes that cause regressions if the problem can't be fixed
quickly -- especially if the change made it into a proper release. The
two texts thus now not slightly contradict each other.

I noticed that this change was already applied, but how would you feel
about changing the second sentence into something like this maybe?

```
Use reverts to quickly fix regressions that otherwise would take too
long to resolve. Apart from this reverts should be used as a last
resort, when the original change is completely wrong; incremental fixes
are preferred.
```

Or maybe this?

```
Incremental fixes in general are preferred over reverts, but the latter
are useful to quickly fix regressions that otherwise would take too long
to resolve. Apart from this reverts should be used as a last resort,
when the original change is completely wrong.
```

Ciao, Thorsten

Re: [PATCH net-next] docs: netdev: clarify the need to sending reverts as patches

[Jakub Kicinski]

On Wed, 29 Mar 2023 11:04:01 +0200 Thorsten Leemhuis wrote:
> FWIW, I see how this is well meant, but I'm not really happy with the
> last sentence, as one of the problems I notice when handling regression
> is: it sometimes takes weeks to get regressions fixed that could have
> been solved quickly by reverting the culprit (and reapplying an improved
> version of the change or the change together and a fix later). That's
> why Documentation/process/handling-regressions.rst strongly suggest to
> revert changes that cause regressions if the problem can't be fixed
> quickly -- especially if the change made it into a proper release. The
> two texts thus now not slightly contradict each other.
> 
> I noticed that this change was already applied, but how would you feel
> about changing the second sentence into something like this maybe?

Please escalate the cases which can be fixed by easy reverts because 
I can't think of any in networking :(

The entire doc is based on our painful experience telling people the
same thing over and over again, we don't want to include things which
don't actually happen on netdev. Longer the doc is the less likely
people will actually read it :(