Finding Clean-up Tasks

Finding Clean-up Tasks

[Alison Schofield]

In Linux Kernel Community we know that it takes a bit of time
and effort to get traction on the cleanup patches.

Please ask for support on this list or irc.

Here are some notes from me. Please chime in with your current
experiences. It helps other applicants and helps us improve
the program.

Snowballing:
------------
I can see on this list that a few folks already have patches
accepted that are 'snowballing'. This is when a reviewer says
'Thanks - and how about fixing this <insert other thing> too?!'
This is great. One patch, spawns more improvements.

Finding the first patch:
------------------------
It's fine to submit patches for checkpatch "CHECK" reports.
Get these by using the --strict option to checkpatch.

There seem to be plenty of checkpatch cleanups in staging/drivers,
and TODO files w beginner level suggestions. I personaly haven't
run sparse, smatch, or coccicheck. 

Another method for finding cleanups is *this* list. Scroll
the list of PATCH Subject lines, some will catch your eye as
not looking like a checkpatch patch. Look at it. See what they
did, how they found it. Sometimes a cocci script is included
in the patch body, so you can just grab that and run.

Confidence to send the patch:
-----------------------------
Go ahead and search on this mailing list for the checkpatch
string. You should find many examples that match what you
are about to do. See what worked well, what needed rework.

If you have a patch AND a question, you can send the patch
and put your question below the scissors line. For example,
you might see multiple instances of something but are not sure
the patch will be well-received. Fix one instance - and below
the scissor line ask you question: "There are 10 more of these
in this file, just want to sanity check that my approach here
is wanted."  (If I were doing cleanup today, I'd use this tactic
for drivers/staging/iio checkpatch ERROR about octals.)

Just for fun, try this on this list:
------------------------------------
- Search for v2, then search for v14. (You can search all the
way in-between them also). Point is, we revision patches a lot!
And, it's not unique to the Outreachy list. Go to any kernel.org
list if you are feeling glum about getting to v5,6,or 7.

Here's the first patch I ever posted to this list:
--------------------------------------------------
https://lore.kernel.org/outreachy/20151005183935.GA3350@Ubuntu-D830/#t 

Reach out if you get stuck or need some camaraderie,
Alison

Re: Finding Clean-up Tasks

[Alison Schofield]

In case you didn't know...the complete archive is here:

https://lore.kernel.org/outreachy/

Re: Finding Clean-up Tasks

[Jaehee Park]

On Thu, Mar 31, 2022 at 11:38:05AM -0700, Alison Schofield wrote:
> In Linux Kernel Community we know that it takes a bit of time
> and effort to get traction on the cleanup patches.
> 
> Please ask for support on this list or irc.
> 
> Here are some notes from me. Please chime in with your current
> experiences. It helps other applicants and helps us improve
> the program.
> 
> Snowballing:
> ------------
> I can see on this list that a few folks already have patches
> accepted that are 'snowballing'. This is when a reviewer says
> 'Thanks - and how about fixing this <insert other thing> too?!'
> This is great. One patch, spawns more improvements.
> 
> Finding the first patch:
> ------------------------
> It's fine to submit patches for checkpatch "CHECK" reports.
> Get these by using the --strict option to checkpatch.
> 
> There seem to be plenty of checkpatch cleanups in staging/drivers,
> and TODO files w beginner level suggestions. I personaly haven't
> run sparse, smatch, or coccicheck. 
> 
> Another method for finding cleanups is *this* list. Scroll
> the list of PATCH Subject lines, some will catch your eye as
> not looking like a checkpatch patch. Look at it. See what they
> did, how they found it. Sometimes a cocci script is included
> in the patch body, so you can just grab that and run.
> 
> Confidence to send the patch:
> -----------------------------
> Go ahead and search on this mailing list for the checkpatch
> string. You should find many examples that match what you
> are about to do. See what worked well, what needed rework.
> 
> If you have a patch AND a question, you can send the patch
> and put your question below the scissors line. For example,
> you might see multiple instances of something but are not sure
> the patch will be well-received. Fix one instance - and below
> the scissor line ask you question: "There are 10 more of these
> in this file, just want to sanity check that my approach here
> is wanted."  (If I were doing cleanup today, I'd use this tactic
> for drivers/staging/iio checkpatch ERROR about octals.)
> 

Thank you for advice! I had a question about where to put the questions
in the patch. When you say scissor line, are we putting dashed lined
somewhere in the patch and writing our questions? Or did you mean we
should reply to our patch?   

> Just for fun, try this on this list:
> ------------------------------------
> - Search for v2, then search for v14. (You can search all the
> way in-between them also). Point is, we revision patches a lot!
> And, it's not unique to the Outreachy list. Go to any kernel.org
> list if you are feeling glum about getting to v5,6,or 7.
> 
> Here's the first patch I ever posted to this list:
> --------------------------------------------------
> https://lore.kernel.org/outreachy/20151005183935.GA3350@Ubuntu-D830/#t 
> 
> Reach out if you get stuck or need some camaraderie,
> Alison
> 

Re: Finding Clean-up Tasks

[Stefano Brivio]

On Fri, 1 Apr 2022 10:28:03 -0400
Jaehee Park <jhpark1013@gmail.com> wrote:

> On Thu, Mar 31, 2022 at 11:38:05AM -0700, Alison Schofield wrote:
>
> [...]
>
> > If you have a patch AND a question, you can send the patch
> > and put your question below the scissors line. For example,
> > you might see multiple instances of something but are not sure
> > the patch will be well-received. Fix one instance - and below
> > the scissor line ask you question: "There are 10 more of these
> > in this file, just want to sanity check that my approach here
> > is wanted."  (If I were doing cleanup today, I'd use this tactic
> > for drivers/staging/iio checkpatch ERROR about octals.)
> >   
> 
> Thank you for advice! I had a question about where to put the questions
> in the patch. When you say scissor line, are we putting dashed lined
> somewhere in the patch and writing our questions?

Let's say this is the original patch as prepared by git format-patch:

--
This is a commit message.

Signed-off-by: you
---
 file.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/file.c
index 148b06a..ceb10a6 100644
--- a/file.c
+++ b/file.c
@@ -927,7 +927,6 @@ int function(void)
 
-
 
--

you can put your question (or any kind of comment, really) there:

--
This is a commit message.

Signed-off-by: you
---
I'm not sure this patch does anything. Should it do something?

 file.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/file.c
index 148b06a..ceb10a6 100644
--- a/file.c
+++ b/file.c
@@ -927,7 +927,6 @@ int function(void)
 
-
 
--

Those three dashes are also commonly called Signed-off-by-line, or
SoB-line (the term might refer to the line above, to the dashes, to the
line below, depending on whom you're speaking to :)).

> Or did you mean we should reply to our patch?   

You can also do that. If you're really unsure, you can also add [RFC]
to the subject, before [PATCH], to make it clear you're asking for
comments rather than actually submitting a change, and then write your
questions, concerns or considerations directly in the commit message.

-- 
Stefano

Re: Finding Clean-up Tasks

[Fabio M. De Francesco]

On venerd? 1 aprile 2022 16:28:03 CEST Jaehee Park wrote:
> On Thu, Mar 31, 2022 at 11:38:05AM -0700, Alison Schofield wrote: 
> > Confidence to send the patch:
> > -----------------------------
> > Go ahead and search on this mailing list for the checkpatch
> > string. You should find many examples that match what you
> > are about to do. See what worked well, what needed rework.
> > 
> > If you have a patch AND a question, you can send the patch
> > and put your question below the scissors line. For example,
> > you might see multiple instances of something but are not sure
> > the patch will be well-received. Fix one instance - and below
> > the scissor line ask you question: "There are 10 more of these
> > in this file, just want to sanity check that my approach here
> > is wanted."  (If I were doing cleanup today, I'd use this tactic
> > for drivers/staging/iio checkpatch ERROR about octals.)
> > 
> 
> Thank you for advice! I had a question about where to put the questions
> in the patch. When you say scissor line, are we putting dashed lined
> somewhere in the patch and writing our questions? Or did you mean we
> should reply to our patch?
> 
Hi Jaehee,

Alison is talking about the three dashes that patches have soon after 
the line with the "Signed-off-by:" tag.

Look at your own patch as an example:

"Change variable name to be consistent with the naming conventions.
ssidlen was changed to ssid_len and ssidie was changed to ssid_ie to be
consistent. This makes the variables more readable. The other ssid
names in the code are separated by an underscore. For example,
bssid_filter and num_of_ssids have the ssid separated from the rest of
the words with an underscore.

Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
--- 

-> Place your questions and revision history here <-

drivers/staging/wfx/hif_tx.c | 10 +++++-----
 drivers/staging/wfx/sta.c    | 20 ++++++++++----------
 2 files changed, 15 insertions(+), 15 deletions(-)"

Regards,

Fabio M. De Francesco

Re: Finding Clean-up Tasks

[Jaehee Park]

On Fri, Apr 01, 2022 at 05:14:48PM +0200, Fabio M. De Francesco wrote:
> On venerd? 1 aprile 2022 16:28:03 CEST Jaehee Park wrote:
> > On Thu, Mar 31, 2022 at 11:38:05AM -0700, Alison Schofield wrote: 
> > > Confidence to send the patch:
> > > -----------------------------
> > > Go ahead and search on this mailing list for the checkpatch
> > > string. You should find many examples that match what you
> > > are about to do. See what worked well, what needed rework.
> > > 
> > > If you have a patch AND a question, you can send the patch
> > > and put your question below the scissors line. For example,
> > > you might see multiple instances of something but are not sure
> > > the patch will be well-received. Fix one instance - and below
> > > the scissor line ask you question: "There are 10 more of these
> > > in this file, just want to sanity check that my approach here
> > > is wanted."  (If I were doing cleanup today, I'd use this tactic
> > > for drivers/staging/iio checkpatch ERROR about octals.)
> > > 
> > 
> > Thank you for advice! I had a question about where to put the questions
> > in the patch. When you say scissor line, are we putting dashed lined
> > somewhere in the patch and writing our questions? Or did you mean we
> > should reply to our patch?
> > 
> Hi Jaehee,
> 
> Alison is talking about the three dashes that patches have soon after 
> the line with the "Signed-off-by:" tag.
> 
> Look at your own patch as an example:
> 
> "Change variable name to be consistent with the naming conventions.
> ssidlen was changed to ssid_len and ssidie was changed to ssid_ie to be
> consistent. This makes the variables more readable. The other ssid
> names in the code are separated by an underscore. For example,
> bssid_filter and num_of_ssids have the ssid separated from the rest of
> the words with an underscore.
> 
> Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
> --- 
> 
> -> Place your questions and revision history here <-
> 
> drivers/staging/wfx/hif_tx.c | 10 +++++-----
>  drivers/staging/wfx/sta.c    | 20 ++++++++++----------
>  2 files changed, 15 insertions(+), 15 deletions(-)"
> 
> Regards,
> 
> Fabio M. De Francesco
> 
> 
>
Got it, Thanks Fabio!

Re: Finding Clean-up Tasks

[Jaehee Park]

On Fri, Apr 01, 2022 at 05:08:39PM +0200, Stefano Brivio wrote:
> On Fri, 1 Apr 2022 10:28:03 -0400
> Jaehee Park <jhpark1013@gmail.com> wrote:
> 
> > On Thu, Mar 31, 2022 at 11:38:05AM -0700, Alison Schofield wrote:
> >
> > [...]
> >
> > > If you have a patch AND a question, you can send the patch
> > > and put your question below the scissors line. For example,
> > > you might see multiple instances of something but are not sure
> > > the patch will be well-received. Fix one instance - and below
> > > the scissor line ask you question: "There are 10 more of these
> > > in this file, just want to sanity check that my approach here
> > > is wanted."  (If I were doing cleanup today, I'd use this tactic
> > > for drivers/staging/iio checkpatch ERROR about octals.)
> > >   
> > 
> > Thank you for advice! I had a question about where to put the questions
> > in the patch. When you say scissor line, are we putting dashed lined
> > somewhere in the patch and writing our questions?
> 
> Let's say this is the original patch as prepared by git format-patch:
> 
> --
> This is a commit message.
> 
> Signed-off-by: you
> ---
>  file.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/file.c
> index 148b06a..ceb10a6 100644
> --- a/file.c
> +++ b/file.c
> @@ -927,7 +927,6 @@ int function(void)
>  
> -
>  
> --
> 
> you can put your question (or any kind of comment, really) there:
> 
> --
> This is a commit message.
> 
> Signed-off-by: you
> ---
> I'm not sure this patch does anything. Should it do something?
> 
>  file.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/file.c
> index 148b06a..ceb10a6 100644
> --- a/file.c
> +++ b/file.c
> @@ -927,7 +927,6 @@ int function(void)
>  
> -
>  
> --
> 
> Those three dashes are also commonly called Signed-off-by-line, or
> SoB-line (the term might refer to the line above, to the dashes, to the
> line below, depending on whom you're speaking to :)).
> 
> > Or did you mean we should reply to our patch?   
> 
> You can also do that. If you're really unsure, you can also add [RFC]
> to the subject, before [PATCH], to make it clear you're asking for
> comments rather than actually submitting a change, and then write your
> questions, concerns or considerations directly in the commit message.
> 
> -- 
> Stefano
>
Makes sense, Thank you Stefano!

Re: Finding Clean-up Tasks

[Alison Schofield]

On Fri, Apr 01, 2022 at 10:28:03AM -0400, Jaehee Park wrote:
> On Thu, Mar 31, 2022 at 11:38:05AM -0700, Alison Schofield wrote:

snip

> > Confidence to send the patch:
> > -----------------------------
> > Go ahead and search on this mailing list for the checkpatch
> > string. You should find many examples that match what you
> > are about to do. See what worked well, what needed rework.
> > 
> > If you have a patch AND a question, you can send the patch
> > and put your question below the scissors line. For example,
> > you might see multiple instances of something but are not sure
> > the patch will be well-received. Fix one instance - and below
> > the scissor line ask you question: "There are 10 more of these
> > in this file, just want to sanity check that my approach here
> > is wanted."  (If I were doing cleanup today, I'd use this tactic
> > for drivers/staging/iio checkpatch ERROR about octals.)
> > 
> 
> Thank you for advice! I had a question about where to put the questions
> in the patch. When you say scissor line, are we putting dashed lined
> somewhere in the patch and writing our questions? Or did you mean we
> should reply to our patch?   
> 
Below the 'scissor line' is the same place you would place a Changelog
when versioning the patch. Anything below the "---" and before the
diffstat gets automagically tossed when the patch is applied.
So, it looks something like this:

>> bssid_filter and num_of_ssids have the ssid separated from the rest of
>> the words with an underscore.
>> 
>> Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
>> ---

Insert question, additional comment here. 
In the future, you'll learn about RFC patches, where we are sometimes
asking for a greater conceptual review before posting. But, for small
questions about a specific patch - this space works well.

>>  drivers/staging/wfx/hif_tx.c | 10 +++++-----
>>  drivers/staging/wfx/sta.c    | 20 ++++++++++----------
>>  2 files changed, 15 insertions(+), 15 deletions(-)

Alison