wireless-drivers: random cleanup patches piling up

wireless-drivers: random cleanup patches piling up

[Kalle Valo]

Hi,

I have quite a lot of random cleanup patches from new developers waiting
in my queue:

https://patchwork.kernel.org/project/linux-wireless/list/?state=10&delegate=25621&order=date

(Not all of them are cleanup patches, there are also few patches
deferred due to other reasons, but you get the idea.)

These cleanup patches usually take quite a lot of my time and I'm
starting to doubt the benefit, compared to the time needed to dig
through them and figuring out what to apply. And this is of course time
away from other patches, so it's slowing down "real" development.

I really don't know what to do. Part of me is saying that I just should
drop them unless it's reviewed by a more experienced developer but on
the other hand this is a good way get new developers onboard.

What others think? Are these kind of patches useful?

-- 
Kalle Valo

Re: wireless-drivers: random cleanup patches piling up

[Larry Finger]

On 01/21/2016 08:58 AM, Kalle Valo wrote:
> Hi,
>
> I have quite a lot of random cleanup patches from new developers waiting
> in my queue:
>
> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&delegate=25621&order=date
>
> (Not all of them are cleanup patches, there are also few patches
> deferred due to other reasons, but you get the idea.)
>
> These cleanup patches usually take quite a lot of my time and I'm
> starting to doubt the benefit, compared to the time needed to dig
> through them and figuring out what to apply. And this is of course time
> away from other patches, so it's slowing down "real" development.
>
> I really don't know what to do. Part of me is saying that I just should
> drop them unless it's reviewed by a more experienced developer but on
> the other hand this is a good way get new developers onboard.
>
> What others think? Are these kind of patches useful?

Kalle,

We might get new developers, but the cost may be high. In the staging tree, 
things are worse. The tools can be applied in a blind fashion, but the results 
can be really stupid. GregKH has told a few would-be contributors to "go away" 
after a few patches that would not build.

As most of these patches are based on "problems" found by application of various 
standard tools, they will likely be resubmitted over and over until the code is 
"fixed". Whether the patches are useful may not be the main question.

My real complaint with these patches is that very few are more than compile 
tested. For example, there are 3 patches for memory leaks in b43. One 
(https://patchwork.kernel.org/patch/7998941) was rejected because it missed some 
such leaks, but there was not a formal NACK. The patch was fixed and resubmitted 
(https://patchwork.kernel.org/patch/8014311/), but not yet tested. The author 
then resent it (https://patchwork.kernel.org/patch/8049041/) and was chastised 
for resending as it still had not been tested. Of course, the first two of these 
can be dropped. Unfortunately, there are very few devs who have the necessary 
hardware to test.

Most of the current set do not account for the directory restructuring and will 
not apply.Those can be rejected with the appropriate message asking that they be 
rebased. That should not require too much of your time. That will at last clean 
out the current backlog.

Is it possible for us to require the patch author to supply the level of testing 
when that is not obvious? This information should be in the comments location 
after the first ---. I suspect I know the answer for non-maintainers, but the 
formal requirement might be helpful. I will start NACKing those patches without 
such information.

I also promise to be more diligent in reviewing the patches that are directed at 
the drivers that I maintain.

Larry

Re: wireless-drivers: random cleanup patches piling up

[Kalle Valo]

Larry Finger <Larry.Finger@lwfinger.net> writes:

> We might get new developers, but the cost may be high. In the staging
> tree, things are worse. The tools can be applied in a blind fashion,
> but the results can be really stupid. GregKH has told a few would-be
> contributors to "go away" after a few patches that would not build.
>
> As most of these patches are based on "problems" found by application
> of various standard tools, they will likely be resubmitted over and
> over until the code is "fixed". Whether the patches are useful may not
> be the main question.
>
> My real complaint with these patches is that very few are more than
> compile tested. For example, there are 3 patches for memory leaks in
> b43. One (https://patchwork.kernel.org/patch/7998941) was rejected
> because it missed some such leaks, but there was not a formal NACK.
> The patch was fixed and resubmitted
> (https://patchwork.kernel.org/patch/8014311/), but not yet tested. The
> author then resent it (https://patchwork.kernel.org/patch/8049041/)
> and was chastised for resending as it still had not been tested. Of
> course, the first two of these can be dropped. Unfortunately, there
> are very few devs who have the necessary hardware to test.
>
> Most of the current set do not account for the directory restructuring
> and will not apply.Those can be rejected with the appropriate message
> asking that they be rebased. That should not require too much of your
> time. That will at last clean out the current backlog.

Actually 'git am -3' handles the directory restructuring just fine, so
that's not a problem. And I can also manage the current backlog, it just
takes some time to clear it up. I'm just worried that these cleanup
patches become even a bigger problem in the future.

> Is it possible for us to require the patch author to supply the level
> of testing when that is not obvious? This information should be in the
> comments location after the first ---. I suspect I know the answer for
> non-maintainers, but the formal requirement might be helpful. 

I think that's a good idea. If it's only compile tested that should be
properly documented in the commit log so that people are aware of it.

> I also promise to be more diligent in reviewing the patches that are
> directed at the drivers that I maintain.

Thanks, I appreciate that.

-- 
Kalle Valo

Re: wireless-drivers: random cleanup patches piling up

[Julian Calaby]

Hi Kalle,

On Fri, Jan 22, 2016 at 1:58 AM, Kalle Valo <kvalo@codeaurora.org> wrote:
> Hi,
>
> I have quite a lot of random cleanup patches from new developers waiting
> in my queue:
>
> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&delegate=25621&order=date
>
> (Not all of them are cleanup patches, there are also few patches
> deferred due to other reasons, but you get the idea.)
>
> These cleanup patches usually take quite a lot of my time and I'm
> starting to doubt the benefit, compared to the time needed to dig
> through them and figuring out what to apply. And this is of course time
> away from other patches, so it's slowing down "real" development.
>
> I really don't know what to do. Part of me is saying that I just should
> drop them unless it's reviewed by a more experienced developer but on
> the other hand this is a good way get new developers onboard.
>
> What others think? Are these kind of patches useful?

I'm not experienced or knowledgeable enough to give an ack or formal
review of a patch, however I generally read all of them. Would it be
helpful if I were to give an informal "this patch looks sane" for
cleanups and other small patches?

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: wireless-drivers: random cleanup patches piling up

[Kalle Valo]

Julian Calaby <julian.calaby@gmail.com> writes:

> Hi Kalle,
>
> On Fri, Jan 22, 2016 at 1:58 AM, Kalle Valo <kvalo@codeaurora.org> wrote:
>> Hi,
>>
>> I have quite a lot of random cleanup patches from new developers waiting
>> in my queue:
>>
>> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&delegate=25621&order=date
>>
>> (Not all of them are cleanup patches, there are also few patches
>> deferred due to other reasons, but you get the idea.)
>>
>> These cleanup patches usually take quite a lot of my time and I'm
>> starting to doubt the benefit, compared to the time needed to dig
>> through them and figuring out what to apply. And this is of course time
>> away from other patches, so it's slowing down "real" development.
>>
>> I really don't know what to do. Part of me is saying that I just should
>> drop them unless it's reviewed by a more experienced developer but on
>> the other hand this is a good way get new developers onboard.
>>
>> What others think? Are these kind of patches useful?
>
> I'm not experienced or knowledgeable enough

I have seen your comments, you are most certainly knowledgeable enough :)

> to give an ack or formal review of a patch, however I generally read
> all of them. Would it be helpful if I were to give an informal "this
> patch looks sane" for cleanups and other small patches?

That would be _very_ helpful, especially with these cleanup patches from
newbies.

And even better if you can add the official Reviewed-by tag because when
we get the promised patchwork update (crossing my fingers) it will
actually show the review count on the top page. That makes it really
easy to spot what cleanup patches are worth looking at (and which one to
drop). So I very much welcome this practice.

-- 
Kalle Valo

Re: wireless-drivers: random cleanup patches piling up

[Julian Calaby]

Hi Kalle,

On Fri, Jan 22, 2016 at 11:17 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
> Julian Calaby <julian.calaby@gmail.com> writes:
>
>> Hi Kalle,
>>
>> On Fri, Jan 22, 2016 at 1:58 AM, Kalle Valo <kvalo@codeaurora.org> wrote:
>>> Hi,
>>>
>>> I have quite a lot of random cleanup patches from new developers waiting
>>> in my queue:
>>>
>>> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&delegate=25621&order=date
>>>
>>> (Not all of them are cleanup patches, there are also few patches
>>> deferred due to other reasons, but you get the idea.)
>>>
>>> These cleanup patches usually take quite a lot of my time and I'm
>>> starting to doubt the benefit, compared to the time needed to dig
>>> through them and figuring out what to apply. And this is of course time
>>> away from other patches, so it's slowing down "real" development.
>>>
>>> I really don't know what to do. Part of me is saying that I just should
>>> drop them unless it's reviewed by a more experienced developer but on
>>> the other hand this is a good way get new developers onboard.
>>>
>>> What others think? Are these kind of patches useful?
>>
>> I'm not experienced or knowledgeable enough
>
> I have seen your comments, you are most certainly knowledgeable enough :)

I'm glad one of us has confidence in me. I know general stuff, but I
lack driver / subsystem specific knowledge, i.e. I can tell if
someone's patch looks stupid, but I can't generally tell if the end
result is doing the "right" thing. Thankfully most of the small
cleanup patches aren't generally changing anything that significant.

>> to give an ack or formal review of a patch, however I generally read
>> all of them. Would it be helpful if I were to give an informal "this
>> patch looks sane" for cleanups and other small patches?
>
> That would be _very_ helpful, especially with these cleanup patches from
> newbies.
>
> And even better if you can add the official Reviewed-by tag because when
> we get the promised patchwork update (crossing my fingers) it will
> actually show the review count on the top page. That makes it really
> easy to spot what cleanup patches are worth looking at (and which one to
> drop). So I very much welcome this practice.

Ok, done. I'll give any sane patches my reviewed-by with a brief "this
looks sane" note.

I'll also try to ensure the maintainers of the driver are CC'd on my
reply and ask them to review the more technical stuff if it's
required.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: wireless-drivers: random cleanup patches piling up

[Joe Perches]

On Thu, 2016-01-21 at 16:58 +0200, Kalle Valo wrote:
> Hi,
> 
> I have quite a lot of random cleanup patches from new developers waiting
> in my queue:
> 
> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&delegate=25621&order=date
> 
> (Not all of them are cleanup patches, there are also few patches
> deferred due to other reasons, but you get the idea.)
> 
> These cleanup patches usually take quite a lot of my time and I'm
> starting to doubt the benefit, compared to the time needed to dig
> through them and figuring out what to apply. And this is of course time
> away from other patches, so it's slowing down "real" development.
> 
> I really don't know what to do. Part of me is saying that I just should
> drop them unless it's reviewed by a more experienced developer but on
> the other hand this is a good way get new developers onboard.
> 
> What others think? Are these kind of patches useful?

Some yes, mostly not really.

While whitespace style patches have some small value,
very few of the new contributors that use tools like
"scripts/checkpatch.pl -f" on various kernel files 
actually continue on to submit actual defect fixing
or optimization or code clarity patches.

Whitespace patches, where git diff -w does not show
any difference and objdiff on the objects for a few
randconfigs are identical, maybe could be sifted
into a separate category from other patches.

Maybe the kbuild test robot could help identify the
whitespace style
patches that can be easily applied.

Maybe the kernel-janitors list should be used and
also maybe some maintainers that want these style
patches could opt-in to getting these applied after
some milestone like an -rc1.

Fengguang?

Re: wireless-drivers: random cleanup patches piling up

[Joe Perches]

On Thu, 2016-01-21 at 16:58 +0200, Kalle Valo wrote:
> Hi,
> 
> I have quite a lot of random cleanup patches from new developers waiting
> in my queue:
> 
> https://patchwork.kernel.org/project/linux-wireless/list/?state\x10&delegate%621&orderÚte
> 
> (Not all of them are cleanup patches, there are also few patches
> deferred due to other reasons, but you get the idea.)
> 
> These cleanup patches usually take quite a lot of my time and I'm
> starting to doubt the benefit, compared to the time needed to dig
> through them and figuring out what to apply. And this is of course time
> away from other patches, so it's slowing down "real" development.
> 
> I really don't know what to do. Part of me is saying that I just should
> drop them unless it's reviewed by a more experienced developer but on
> the other hand this is a good way get new developers onboard.
> 
> What others think? Are these kind of patches useful?

Some yes, mostly not really.

While whitespace style patches have some small value,
very few of the new contributors that use tools like
"scripts/checkpatch.pl -f" on various kernel files 
actually continue on to submit actual defect fixing
or optimization or code clarity patches.

Whitespace patches, where git diff -w does not show
any difference and objdiff on the objects for a few
randconfigs are identical, maybe could be sifted
into a separate category from other patches.

Maybe the kbuild test robot could help identify the
whitespace style
patches that can be easily applied.

Maybe the kernel-janitors list should be used and
also maybe some maintainers that want these style
patches could opt-in to getting these applied after
some milestone like an -rc1.

Fengguang?

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: wireless-drivers: random cleanup patches piling up

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 777 bytes --]

On Thu, Jan 21, 2016 at 04:52:45PM -0800, Joe Perches wrote:
> Whitespace patches, where git diff -w does not show
> any difference and objdiff on the objects for a few
> randconfigs are identical, maybe could be sifted
> into a separate category from other patches.
> Maybe the kbuild test robot could help identify the
> whitespace style patches that can be easily applied.

It sort of takes a while to test the randconfig thing...  diff -w
doesn't catch every single issue.  For example:

http://www.spinics.net/lists/linux-driver-devel/msg78707.html

That's the only time I noticed a mistake like that which diff -w missed.

Also here is my rename_rev.pl script.  It's pretty useful.  Pipe
patches to it and it filters out the white space changes.

regards,
dan carpenter


[-- Attachment #2: rename_rev.pl --]
[-- Type: text/x-perl, Size: 10630 bytes --]

#!/usr/bin/perl

# This is a tool to help review variable rename patches. The goal is
# to strip out the automatic sed renames and the white space changes
# and leaves the interesting code changes.
#
# Example 1: A patch renames openInfo to open_info:
#     cat diff | rename_review.pl openInfo open_info
#
# Example 2: A patch swaps the first two arguments to some_func():
#     cat diff | rename_review.pl \
#                    -e 's/some_func\((.*?),(.*?),/some_func\($2, $1,/'
#
# Example 3: A patch removes the xkcd_ prefix from some but not all the
# variables.  Instead of trying to figure out which variables were renamed
# just remove the prefix from them all:
#     cat diff | rename_review.pl -ea 's/xkcd_//g'
#
# Example 4: A patch renames 20 CamelCase variables.  To review this let's
# just ignore all case changes and all '_' chars.
#     cat diff | rename_review -ea 'tr/[A-Z]/[a-z]/' -ea 's/_//g'
#
# The other arguments are:
# -nc removes comments
# -ns removes '\' chars if they are at the end of the line.

use strict;
use File::Temp qw/ :mktemp  /;

sub usage() {
    print "usage: cat diff | $0 old new old new old new...\n";
    print "   or: cat diff | $0 -e 's/old/new/g'\n";
    print " -a : auto";
    print " -e : execute on old lines\n";
    print " -ea: execute on all lines\n";
    print " -nc: no comments\n";
    print " -nb: no unneeded braces\n";
    print " -ns: no slashes at the end of a line\n";
    print " -pull: for function pull.  deletes context.\n";
    print " -r <recipe>: NULL, bool";
    exit(1);
}
my @subs;
my @strict_subs;
my @cmds;
my $strip_comments;
my $strip_braces;
my $strip_slashes;
my $pull_context;
my $auto;

sub filter($) {
    my $_ = shift();
    my $old = 0;
    if ($_ =~ /^-/) {
        $old = 1;
    }
    # remove the first char
    s/^[ +-]//;
    if ($strip_comments) {
        s/\/\*.*?\*\///g;
        s/\/\/.*//;
    }
    foreach my $cmd (@cmds) {
        if ($old || $cmd->[0] =~ /^-ea$/) {
            eval $cmd->[1];
        }
    }
    foreach my $sub (@subs) {
        if ($old) {
            s/$sub->[0]/$sub->[1]/g;
        }
    }
    foreach my $sub (@strict_subs) {
        if ($old) {
            s/\b$sub->[0]\b/$sub->[1]/g;
        }
    }

    # remove the newline so we can move curly braces here if we want.
    s/\n//;
    return $_;
}

while (my $param1 = shift()) {
    if ($param1 =~ /^-a$/) {
        $auto = 1;
        next;
    }
    if ($param1 =~ /^-nc$/) {
        $strip_comments = 1;
        next;
    }
    if ($param1 =~ /^-nb$/) {
        $strip_braces = 1;
        next;
    }
    if ($param1 =~ /^-ns$/) {
        $strip_slashes = 1;
        next;
    }
    if ($param1 =~ /^-pull$/) {
        $pull_context = 1;
        next;
    }
    my $param2 = shift();
    if ($param2 =~ /^$/) {
        usage();
    }
    if ($param1 =~ /^-e(a|)$/) {
        push @cmds, [$param1, $param2];
        next;
    }
    if ($param1 =~ /^-r$/) {
        if ($param2 =~ /bool/) {
            push @cmds, ["-e", "s/== true//"];
            push @cmds, ["-e", "s/true ==//"];
            push @cmds, ["-e", "s/([a-zA-Z\-\>\._]+) == false/!\$1/"];
            next;
        } elsif ($param2 =~ /NULL/) {
            push @cmds, ["-e", "s/ != NULL//"];
            push @cmds, ["-e", "s/([a-zA-Z\-\>\._0-9]+) == NULL/!\$1/"];
            next;
        } elsif ($param2 =~ /BIT/) {
            push @cmds, ["-e", 's/1[uUlL]* *<< *(\d+)/BIT($1)/'];
            push @cmds, ["-e", 's/\(1 *<< *(\w+)\)/BIT($1)/'];
            push @cmds, ["-e", 's/\(BIT\((.*?)\)\)/BIT($1)/'];
            next;
        }
        usage();
    }

    push @subs, [$param1, $param2];
}

my ($oldfh, $oldfile) = mkstemp("/tmp/oldXXXXX");
my ($newfh, $newfile) = mkstemp("/tmp/newXXXXX");

my @input = <STDIN>;

# auto works on the observation that the - line comes before the + line when we
# rename variables.  Take the first - line.  Find the first + line.  Find the
# one word difference.  Test that the old word never occurs in the new text.
if ($auto) {
    my %c_keywords = (  auto => 1,
                        break => 1,
                        case => 1,
                        char => 1,
                        const => 1,
                        continue => 1,
                        default => 1,
                        do => 1,
                        double => 1,
                        else => 1,
                        enum => 1,
                        extern => 1,
                        float => 1,
                        for => 1,
                        goto => 1,
                        if => 1,
                        int => 1,
                        long => 1,
                        register => 1,
                        return => 1,
                        short => 1,
                        signed => 1,
                        sizeof => 1,
                        static => 1,
                        struct => 1,
                        switch => 1,
                        typedef => 1,
                        union => 1,
                        unsigned => 1,
                        void => 1,
                        volatile => 1,
                        while => 1);
    my %old_words;
    my %new_words;
    my %added_cmds;
    my @new_subs;

    my $inside = 0;
    foreach my $line (@input) {
        if ($line =~ /^(---|\+\+\+)/) {
            next;
        }

        if ($line =~ /^@/) {
            $inside = 1;
        }
        if ($inside && !(($_ =~ /^[- @+]/) || ($_ =~ /^$/))) {
            $inside = 0;
        }
        if (!$inside) {
            next;
        }

        if ($line =~ /^-/) {
            s/-//;
            my @words = split(/\W+/, $line);
            foreach my $word (@words) {
                $old_words{$word} = 1;
            }
        } elsif ($line =~ /^\+/) {
            s/\+//;
            my @words = split(/\W+/, $line);
            foreach my $word (@words) {
                $new_words{$word} = 1;
            }
        }
    }

    my $old_line;
    my $new_line;
    $inside = 0;
    foreach my $line (@input) {
        if ($line =~ /^(---|\+\+\+)/) {
            next;
        }

        if ($line =~ /^@/) {
            $inside = 1;
        }
        if ($inside && !(($_ =~ /^[- @+]/) || ($_ =~ /^$/))) {
            $inside = 0;
        }
        if (!$inside) {
            next;
        }


        if ($line =~ /^-/ && !$old_line) {
            s/^-//;
            $old_line = $line;
            next;
        } elsif ($old_line && $line =~ /^\+/) {
            s/^\+//;
            $new_line = $line;
        } else {
            next;
        }

        my @old_words = split(/\W+/, $old_line);
        my @new_words = split(/\W+/, $new_line);
        my @new_cmds;

        my $i;
        my $diff_count = 0;
        for ($i = 0; ; $i++) {
            if (!defined($old_words[$i]) && !defined($new_words[$i])) {
                last;
            }
            if (!defined($old_words[$i]) || !defined($new_words[$i])) {
                $diff_count = 1000;
                last;
            }
            if ($old_words[$i] eq $new_words[$i]) {
                next;
            }
            if ($c_keywords{$old_words[$i]}) {
                $diff_count = 1000;
                last;
            }
            if ($new_words{$old_words[$i]}) {
                $diff_count++;
            }
            push @new_cmds, [$old_words[$i], $new_words[$i]];
        }
        if ($diff_count <= 2) {
            foreach my $sub (@new_cmds) {
                if ($added_cmds{$sub->[0] . $sub->[1]}) {
                    next;
                }
                $added_cmds{$sub->[0] . $sub->[1]} = 1;
                push @new_subs, [$sub->[0] , $sub->[1]];
            }
        }

        $old_line = 0;
    }

    if (@new_subs) {
        print "RENAMES:\n";
        foreach my $sub (@new_subs) {
            print "$sub->[0] => $sub->[1]\n";
            push @strict_subs, [$sub->[0] , $sub->[1]];
        }
        print "---\n";
    }
}

my $output;

#recreate an old file and a new file
my $inside = 0;
foreach (@input) {
    if ($pull_context && !($_ =~ /^[+-@]/)) {
        next;
    }

    if ($_ =~ /^(---|\+\+\+)/) {
        next;
    }

    if ($_ =~ /^@/) {
        $inside = 1;
    }
    if ($inside && !(($_ =~ /^[- @+]/) || ($_ =~ /^$/))) {
        $inside = 0;
    }
    if (!$inside) {
        next;
    }

    $output = filter($_);

    if ($strip_braces && $_ =~ /^(\+|-)\W+{/) {
        $output =~ s/^[\t ]+(.*)/ $1/;
    } else {
        $output = "\n" . $output;
    }

    if ($_ =~ /^-/) {
        print $oldfh $output;
        next;
    }
    if ($_ =~ /^\+/) {
        print $newfh $output;
        next;
    }
    print $oldfh $output;
    print $newfh $output;

}
print $oldfh "\n";
print $newfh "\n";
# git diff puts a -- and version at the end of the diff.  put the -- into the
# new file as well so it's ignored
if ($output =~ /\n-/) {
    print $newfh "-\n";
}

my $hunk;
my $old_txt;
my $new_txt;

open diff, "diff -uw $oldfile $newfile |";
while (<diff>) {
    if ($_ =~ /^(---|\+\+\+)/) {
        next;
    }

    if ($_ =~ /^@/) {

        if ($strip_comments) {
            $old_txt =~ s/\/\*.*?\*\///g;
            $new_txt =~ s/\/\*.*?\*\///g;
        }
        if ($strip_braces) {
            $old_txt =~ s/{([^;{]*?);}/$1;/g;
            $new_txt =~ s/{([^;{]*?);}/$1;/g;
            # this is a hack because i don't know how to replace nested
            # unneeded curly braces.
            $old_txt =~ s/{([^;{]*?);}/$1;/g;
            $new_txt =~ s/{([^;{]*?);}/$1;/g;
        }

        if ($old_txt ne $new_txt) {
            print $hunk;
            print $_;
        }
        $hunk = "";
        $old_txt = "";
        $new_txt = "";
        next;
    }

    $hunk = $hunk . $_;

    if ($strip_slashes) {
        s/\\$//;
    }

    if ($_ =~ /^-/) {
        s/-//;
        s/[ \t\n]//g;
        $old_txt = $old_txt . $_;
        next;
    }
    if ($_ =~ /^\+/) {
        s/\+//;
        s/[ \t\n]//g;
        $new_txt = $new_txt . $_;
        next;
    }
    if ($_ =~ /^ /) {
        s/^ //;
        s/[ \t\n]//g;
        $old_txt = $old_txt . $_;
        $new_txt = $new_txt . $_;
    }
}

if ($old_txt ne $new_txt) {
    if ($strip_comments) {
        $old_txt =~ s/\/\*.*?\*\///g;
        $new_txt =~ s/\/\*.*?\*\///g;
    }
    if ($strip_braces) {
        $old_txt =~ s/{([^;{]*?);}/$1;/g;
        $new_txt =~ s/{([^;{]*?);}/$1;/g;
        $old_txt =~ s/{([^;{]*?);}/$1;/g;
        $new_txt =~ s/{([^;{]*?);}/$1;/g;
    }

    print $hunk;
}

unlink($oldfile);
unlink($newfile);

print "\ndone.\n";

Re: wireless-drivers: random cleanup patches piling up

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 777 bytes --]

On Thu, Jan 21, 2016 at 04:52:45PM -0800, Joe Perches wrote:
> Whitespace patches, where git diff -w does not show
> any difference and objdiff on the objects for a few
> randconfigs are identical, maybe could be sifted
> into a separate category from other patches.
> Maybe the kbuild test robot could help identify the
> whitespace style patches that can be easily applied.

It sort of takes a while to test the randconfig thing...  diff -w
doesn't catch every single issue.  For example:

http://www.spinics.net/lists/linux-driver-devel/msg78707.html

That's the only time I noticed a mistake like that which diff -w missed.

Also here is my rename_rev.pl script.  It's pretty useful.  Pipe
patches to it and it filters out the white space changes.

regards,
dan carpenter


[-- Attachment #2: rename_rev.pl --]
[-- Type: text/x-perl, Size: 10630 bytes --]

#!/usr/bin/perl

# This is a tool to help review variable rename patches. The goal is
# to strip out the automatic sed renames and the white space changes
# and leaves the interesting code changes.
#
# Example 1: A patch renames openInfo to open_info:
#     cat diff | rename_review.pl openInfo open_info
#
# Example 2: A patch swaps the first two arguments to some_func():
#     cat diff | rename_review.pl \
#                    -e 's/some_func\((.*?),(.*?),/some_func\($2, $1,/'
#
# Example 3: A patch removes the xkcd_ prefix from some but not all the
# variables.  Instead of trying to figure out which variables were renamed
# just remove the prefix from them all:
#     cat diff | rename_review.pl -ea 's/xkcd_//g'
#
# Example 4: A patch renames 20 CamelCase variables.  To review this let's
# just ignore all case changes and all '_' chars.
#     cat diff | rename_review -ea 'tr/[A-Z]/[a-z]/' -ea 's/_//g'
#
# The other arguments are:
# -nc removes comments
# -ns removes '\' chars if they are at the end of the line.

use strict;
use File::Temp qw/ :mktemp  /;

sub usage() {
    print "usage: cat diff | $0 old new old new old new...\n";
    print "   or: cat diff | $0 -e 's/old/new/g'\n";
    print " -a : auto";
    print " -e : execute on old lines\n";
    print " -ea: execute on all lines\n";
    print " -nc: no comments\n";
    print " -nb: no unneeded braces\n";
    print " -ns: no slashes at the end of a line\n";
    print " -pull: for function pull.  deletes context.\n";
    print " -r <recipe>: NULL, bool";
    exit(1);
}
my @subs;
my @strict_subs;
my @cmds;
my $strip_comments;
my $strip_braces;
my $strip_slashes;
my $pull_context;
my $auto;

sub filter($) {
    my $_ = shift();
    my $old = 0;
    if ($_ =~ /^-/) {
        $old = 1;
    }
    # remove the first char
    s/^[ +-]//;
    if ($strip_comments) {
        s/\/\*.*?\*\///g;
        s/\/\/.*//;
    }
    foreach my $cmd (@cmds) {
        if ($old || $cmd->[0] =~ /^-ea$/) {
            eval $cmd->[1];
        }
    }
    foreach my $sub (@subs) {
        if ($old) {
            s/$sub->[0]/$sub->[1]/g;
        }
    }
    foreach my $sub (@strict_subs) {
        if ($old) {
            s/\b$sub->[0]\b/$sub->[1]/g;
        }
    }

    # remove the newline so we can move curly braces here if we want.
    s/\n//;
    return $_;
}

while (my $param1 = shift()) {
    if ($param1 =~ /^-a$/) {
        $auto = 1;
        next;
    }
    if ($param1 =~ /^-nc$/) {
        $strip_comments = 1;
        next;
    }
    if ($param1 =~ /^-nb$/) {
        $strip_braces = 1;
        next;
    }
    if ($param1 =~ /^-ns$/) {
        $strip_slashes = 1;
        next;
    }
    if ($param1 =~ /^-pull$/) {
        $pull_context = 1;
        next;
    }
    my $param2 = shift();
    if ($param2 =~ /^$/) {
        usage();
    }
    if ($param1 =~ /^-e(a|)$/) {
        push @cmds, [$param1, $param2];
        next;
    }
    if ($param1 =~ /^-r$/) {
        if ($param2 =~ /bool/) {
            push @cmds, ["-e", "s/== true//"];
            push @cmds, ["-e", "s/true ==//"];
            push @cmds, ["-e", "s/([a-zA-Z\-\>\._]+) == false/!\$1/"];
            next;
        } elsif ($param2 =~ /NULL/) {
            push @cmds, ["-e", "s/ != NULL//"];
            push @cmds, ["-e", "s/([a-zA-Z\-\>\._0-9]+) == NULL/!\$1/"];
            next;
        } elsif ($param2 =~ /BIT/) {
            push @cmds, ["-e", 's/1[uUlL]* *<< *(\d+)/BIT($1)/'];
            push @cmds, ["-e", 's/\(1 *<< *(\w+)\)/BIT($1)/'];
            push @cmds, ["-e", 's/\(BIT\((.*?)\)\)/BIT($1)/'];
            next;
        }
        usage();
    }

    push @subs, [$param1, $param2];
}

my ($oldfh, $oldfile) = mkstemp("/tmp/oldXXXXX");
my ($newfh, $newfile) = mkstemp("/tmp/newXXXXX");

my @input = <STDIN>;

# auto works on the observation that the - line comes before the + line when we
# rename variables.  Take the first - line.  Find the first + line.  Find the
# one word difference.  Test that the old word never occurs in the new text.
if ($auto) {
    my %c_keywords = (  auto => 1,
                        break => 1,
                        case => 1,
                        char => 1,
                        const => 1,
                        continue => 1,
                        default => 1,
                        do => 1,
                        double => 1,
                        else => 1,
                        enum => 1,
                        extern => 1,
                        float => 1,
                        for => 1,
                        goto => 1,
                        if => 1,
                        int => 1,
                        long => 1,
                        register => 1,
                        return => 1,
                        short => 1,
                        signed => 1,
                        sizeof => 1,
                        static => 1,
                        struct => 1,
                        switch => 1,
                        typedef => 1,
                        union => 1,
                        unsigned => 1,
                        void => 1,
                        volatile => 1,
                        while => 1);
    my %old_words;
    my %new_words;
    my %added_cmds;
    my @new_subs;

    my $inside = 0;
    foreach my $line (@input) {
        if ($line =~ /^(---|\+\+\+)/) {
            next;
        }

        if ($line =~ /^@/) {
            $inside = 1;
        }
        if ($inside && !(($_ =~ /^[- @+]/) || ($_ =~ /^$/))) {
            $inside = 0;
        }
        if (!$inside) {
            next;
        }

        if ($line =~ /^-/) {
            s/-//;
            my @words = split(/\W+/, $line);
            foreach my $word (@words) {
                $old_words{$word} = 1;
            }
        } elsif ($line =~ /^\+/) {
            s/\+//;
            my @words = split(/\W+/, $line);
            foreach my $word (@words) {
                $new_words{$word} = 1;
            }
        }
    }

    my $old_line;
    my $new_line;
    $inside = 0;
    foreach my $line (@input) {
        if ($line =~ /^(---|\+\+\+)/) {
            next;
        }

        if ($line =~ /^@/) {
            $inside = 1;
        }
        if ($inside && !(($_ =~ /^[- @+]/) || ($_ =~ /^$/))) {
            $inside = 0;
        }
        if (!$inside) {
            next;
        }


        if ($line =~ /^-/ && !$old_line) {
            s/^-//;
            $old_line = $line;
            next;
        } elsif ($old_line && $line =~ /^\+/) {
            s/^\+//;
            $new_line = $line;
        } else {
            next;
        }

        my @old_words = split(/\W+/, $old_line);
        my @new_words = split(/\W+/, $new_line);
        my @new_cmds;

        my $i;
        my $diff_count = 0;
        for ($i = 0; ; $i++) {
            if (!defined($old_words[$i]) && !defined($new_words[$i])) {
                last;
            }
            if (!defined($old_words[$i]) || !defined($new_words[$i])) {
                $diff_count = 1000;
                last;
            }
            if ($old_words[$i] eq $new_words[$i]) {
                next;
            }
            if ($c_keywords{$old_words[$i]}) {
                $diff_count = 1000;
                last;
            }
            if ($new_words{$old_words[$i]}) {
                $diff_count++;
            }
            push @new_cmds, [$old_words[$i], $new_words[$i]];
        }
        if ($diff_count <= 2) {
            foreach my $sub (@new_cmds) {
                if ($added_cmds{$sub->[0] . $sub->[1]}) {
                    next;
                }
                $added_cmds{$sub->[0] . $sub->[1]} = 1;
                push @new_subs, [$sub->[0] , $sub->[1]];
            }
        }

        $old_line = 0;
    }

    if (@new_subs) {
        print "RENAMES:\n";
        foreach my $sub (@new_subs) {
            print "$sub->[0] => $sub->[1]\n";
            push @strict_subs, [$sub->[0] , $sub->[1]];
        }
        print "---\n";
    }
}

my $output;

#recreate an old file and a new file
my $inside = 0;
foreach (@input) {
    if ($pull_context && !($_ =~ /^[+-@]/)) {
        next;
    }

    if ($_ =~ /^(---|\+\+\+)/) {
        next;
    }

    if ($_ =~ /^@/) {
        $inside = 1;
    }
    if ($inside && !(($_ =~ /^[- @+]/) || ($_ =~ /^$/))) {
        $inside = 0;
    }
    if (!$inside) {
        next;
    }

    $output = filter($_);

    if ($strip_braces && $_ =~ /^(\+|-)\W+{/) {
        $output =~ s/^[\t ]+(.*)/ $1/;
    } else {
        $output = "\n" . $output;
    }

    if ($_ =~ /^-/) {
        print $oldfh $output;
        next;
    }
    if ($_ =~ /^\+/) {
        print $newfh $output;
        next;
    }
    print $oldfh $output;
    print $newfh $output;

}
print $oldfh "\n";
print $newfh "\n";
# git diff puts a -- and version at the end of the diff.  put the -- into the
# new file as well so it's ignored
if ($output =~ /\n-/) {
    print $newfh "-\n";
}

my $hunk;
my $old_txt;
my $new_txt;

open diff, "diff -uw $oldfile $newfile |";
while (<diff>) {
    if ($_ =~ /^(---|\+\+\+)/) {
        next;
    }

    if ($_ =~ /^@/) {

        if ($strip_comments) {
            $old_txt =~ s/\/\*.*?\*\///g;
            $new_txt =~ s/\/\*.*?\*\///g;
        }
        if ($strip_braces) {
            $old_txt =~ s/{([^;{]*?);}/$1;/g;
            $new_txt =~ s/{([^;{]*?);}/$1;/g;
            # this is a hack because i don't know how to replace nested
            # unneeded curly braces.
            $old_txt =~ s/{([^;{]*?);}/$1;/g;
            $new_txt =~ s/{([^;{]*?);}/$1;/g;
        }

        if ($old_txt ne $new_txt) {
            print $hunk;
            print $_;
        }
        $hunk = "";
        $old_txt = "";
        $new_txt = "";
        next;
    }

    $hunk = $hunk . $_;

    if ($strip_slashes) {
        s/\\$//;
    }

    if ($_ =~ /^-/) {
        s/-//;
        s/[ \t\n]//g;
        $old_txt = $old_txt . $_;
        next;
    }
    if ($_ =~ /^\+/) {
        s/\+//;
        s/[ \t\n]//g;
        $new_txt = $new_txt . $_;
        next;
    }
    if ($_ =~ /^ /) {
        s/^ //;
        s/[ \t\n]//g;
        $old_txt = $old_txt . $_;
        $new_txt = $new_txt . $_;
    }
}

if ($old_txt ne $new_txt) {
    if ($strip_comments) {
        $old_txt =~ s/\/\*.*?\*\///g;
        $new_txt =~ s/\/\*.*?\*\///g;
    }
    if ($strip_braces) {
        $old_txt =~ s/{([^;{]*?);}/$1;/g;
        $new_txt =~ s/{([^;{]*?);}/$1;/g;
        $old_txt =~ s/{([^;{]*?);}/$1;/g;
        $new_txt =~ s/{([^;{]*?);}/$1;/g;
    }

    print $hunk;
}

unlink($oldfile);
unlink($newfile);

print "\ndone.\n";

Re: wireless-drivers: random cleanup patches piling up

[Kalle Valo]

Joe Perches <joe@perches.com> writes:

> On Thu, 2016-01-21 at 16:58 +0200, Kalle Valo wrote:
>> Hi,
>> 
>> I have quite a lot of random cleanup patches from new developers waiting
>> in my queue:
>> 
>> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&delegate=25621&order=date
>> 
>> (Not all of them are cleanup patches, there are also few patches
>> deferred due to other reasons, but you get the idea.)
>> 
>> These cleanup patches usually take quite a lot of my time and I'm
>> starting to doubt the benefit, compared to the time needed to dig
>> through them and figuring out what to apply. And this is of course time
>> away from other patches, so it's slowing down "real" development.
>> 
>> I really don't know what to do. Part of me is saying that I just should
>> drop them unless it's reviewed by a more experienced developer but on
>> the other hand this is a good way get new developers onboard.
>> 
>> What others think? Are these kind of patches useful?
>
> Some yes, mostly not really.
>
> While whitespace style patches have some small value,
> very few of the new contributors that use tools like
> "scripts/checkpatch.pl -f" on various kernel files 
> actually continue on to submit actual defect fixing
> or optimization or code clarity patches.

That's also my experience from maintaining wireless-drivers for a year,
this seems to be a "hit and run" type of phenomenon.

-- 
Kalle Valo

Re: wireless-drivers: random cleanup patches piling up

[Kalle Valo]

Joe Perches <joe@perches.com> writes:

> On Thu, 2016-01-21 at 16:58 +0200, Kalle Valo wrote:
>> Hi,
>> 
>> I have quite a lot of random cleanup patches from new developers waiting
>> in my queue:
>> 
>> https://patchwork.kernel.org/project/linux-wireless/list/?state\x10&delegate%621&orderÚte
>> 
>> (Not all of them are cleanup patches, there are also few patches
>> deferred due to other reasons, but you get the idea.)
>> 
>> These cleanup patches usually take quite a lot of my time and I'm
>> starting to doubt the benefit, compared to the time needed to dig
>> through them and figuring out what to apply. And this is of course time
>> away from other patches, so it's slowing down "real" development.
>> 
>> I really don't know what to do. Part of me is saying that I just should
>> drop them unless it's reviewed by a more experienced developer but on
>> the other hand this is a good way get new developers onboard.
>> 
>> What others think? Are these kind of patches useful?
>
> Some yes, mostly not really.
>
> While whitespace style patches have some small value,
> very few of the new contributors that use tools like
> "scripts/checkpatch.pl -f" on various kernel files 
> actually continue on to submit actual defect fixing
> or optimization or code clarity patches.

That's also my experience from maintaining wireless-drivers for a year,
this seems to be a "hit and run" type of phenomenon.

-- 
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: wireless-drivers: random cleanup patches piling up

[John W. Linville]

On Fri, Jan 22, 2016 at 02:21:20PM +0200, Kalle Valo wrote:
> Joe Perches <joe@perches.com> writes:
> 
> > On Thu, 2016-01-21 at 16:58 +0200, Kalle Valo wrote:
> >> Hi,
> >> 
> >> I have quite a lot of random cleanup patches from new developers waiting
> >> in my queue:
> >> 
> >> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&delegate=25621&order=date
> >> 
> >> (Not all of them are cleanup patches, there are also few patches
> >> deferred due to other reasons, but you get the idea.)
> >> 
> >> These cleanup patches usually take quite a lot of my time and I'm
> >> starting to doubt the benefit, compared to the time needed to dig
> >> through them and figuring out what to apply. And this is of course time
> >> away from other patches, so it's slowing down "real" development.
> >> 
> >> I really don't know what to do. Part of me is saying that I just should
> >> drop them unless it's reviewed by a more experienced developer but on
> >> the other hand this is a good way get new developers onboard.
> >> 
> >> What others think? Are these kind of patches useful?
> >
> > Some yes, mostly not really.
> >
> > While whitespace style patches have some small value,
> > very few of the new contributors that use tools like
> > "scripts/checkpatch.pl -f" on various kernel files 
> > actually continue on to submit actual defect fixing
> > or optimization or code clarity patches.
> 
> That's also my experience from maintaining wireless-drivers for a year,
> this seems to be a "hit and run" type of phenomenon.

Should we be looking for someone to run a "wireless-driver-cleanups"
tree?  They could handle the cleanups and trivial stuff, and send
you a pull request a couple of times per release...?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: wireless-drivers: random cleanup patches piling up

[John W. Linville]

On Fri, Jan 22, 2016 at 02:21:20PM +0200, Kalle Valo wrote:
> Joe Perches <joe@perches.com> writes:
> 
> > On Thu, 2016-01-21 at 16:58 +0200, Kalle Valo wrote:
> >> Hi,
> >> 
> >> I have quite a lot of random cleanup patches from new developers waiting
> >> in my queue:
> >> 
> >> https://patchwork.kernel.org/project/linux-wireless/list/?state\x10&delegate%621&orderÚte
> >> 
> >> (Not all of them are cleanup patches, there are also few patches
> >> deferred due to other reasons, but you get the idea.)
> >> 
> >> These cleanup patches usually take quite a lot of my time and I'm
> >> starting to doubt the benefit, compared to the time needed to dig
> >> through them and figuring out what to apply. And this is of course time
> >> away from other patches, so it's slowing down "real" development.
> >> 
> >> I really don't know what to do. Part of me is saying that I just should
> >> drop them unless it's reviewed by a more experienced developer but on
> >> the other hand this is a good way get new developers onboard.
> >> 
> >> What others think? Are these kind of patches useful?
> >
> > Some yes, mostly not really.
> >
> > While whitespace style patches have some small value,
> > very few of the new contributors that use tools like
> > "scripts/checkpatch.pl -f" on various kernel files 
> > actually continue on to submit actual defect fixing
> > or optimization or code clarity patches.
> 
> That's also my experience from maintaining wireless-drivers for a year,
> this seems to be a "hit and run" type of phenomenon.

Should we be looking for someone to run a "wireless-driver-cleanups"
tree?  They could handle the cleanups and trivial stuff, and send
you a pull request a couple of times per release...?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: wireless-drivers: random cleanup patches piling up

[Kalle Valo]

"John W. Linville" <linville@tuxdriver.com> writes:

> On Fri, Jan 22, 2016 at 02:21:20PM +0200, Kalle Valo wrote:
>> Joe Perches <joe@perches.com> writes:
>> 
>> > On Thu, 2016-01-21 at 16:58 +0200, Kalle Valo wrote:
>> >> Hi,
>> >> 
>> >> I have quite a lot of random cleanup patches from new developers waiting
>> >> in my queue:
>> >> 
>> >> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&delegate=25621&order=date
>> >> 
>> >> (Not all of them are cleanup patches, there are also few patches
>> >> deferred due to other reasons, but you get the idea.)
>> >> 
>> >> These cleanup patches usually take quite a lot of my time and I'm
>> >> starting to doubt the benefit, compared to the time needed to dig
>> >> through them and figuring out what to apply. And this is of course time
>> >> away from other patches, so it's slowing down "real" development.
>> >> 
>> >> I really don't know what to do. Part of me is saying that I just should
>> >> drop them unless it's reviewed by a more experienced developer but on
>> >> the other hand this is a good way get new developers onboard.
>> >> 
>> >> What others think? Are these kind of patches useful?
>> >
>> > Some yes, mostly not really.
>> >
>> > While whitespace style patches have some small value,
>> > very few of the new contributors that use tools like
>> > "scripts/checkpatch.pl -f" on various kernel files 
>> > actually continue on to submit actual defect fixing
>> > or optimization or code clarity patches.
>> 
>> That's also my experience from maintaining wireless-drivers for a year,
>> this seems to be a "hit and run" type of phenomenon.
>
> Should we be looking for someone to run a "wireless-driver-cleanups"
> tree?  They could handle the cleanups and trivial stuff, and send
> you a pull request a couple of times per release...?

Not a bad idea! But I don't think we need a separate tree as applying
patches from patchwork is easy. It should be doable that we add an
account to patchwork and whenever I see a this type of trivial cleanup
patch I'll assign it to the cleanup maintainer and whenever he/she
thinks it's ready he assigns the patch back to me and I'll apply it.

The only difficult part is finding a victim/volunteer to
do that ;)

-- 
Kalle Valo

Re: wireless-drivers: random cleanup patches piling up

[Kalle Valo]

"John W. Linville" <linville@tuxdriver.com> writes:

> On Fri, Jan 22, 2016 at 02:21:20PM +0200, Kalle Valo wrote:
>> Joe Perches <joe@perches.com> writes:
>> 
>> > On Thu, 2016-01-21 at 16:58 +0200, Kalle Valo wrote:
>> >> Hi,
>> >> 
>> >> I have quite a lot of random cleanup patches from new developers waiting
>> >> in my queue:
>> >> 
>> >> https://patchwork.kernel.org/project/linux-wireless/list/?state\x10&delegate%621&orderÚte
>> >> 
>> >> (Not all of them are cleanup patches, there are also few patches
>> >> deferred due to other reasons, but you get the idea.)
>> >> 
>> >> These cleanup patches usually take quite a lot of my time and I'm
>> >> starting to doubt the benefit, compared to the time needed to dig
>> >> through them and figuring out what to apply. And this is of course time
>> >> away from other patches, so it's slowing down "real" development.
>> >> 
>> >> I really don't know what to do. Part of me is saying that I just should
>> >> drop them unless it's reviewed by a more experienced developer but on
>> >> the other hand this is a good way get new developers onboard.
>> >> 
>> >> What others think? Are these kind of patches useful?
>> >
>> > Some yes, mostly not really.
>> >
>> > While whitespace style patches have some small value,
>> > very few of the new contributors that use tools like
>> > "scripts/checkpatch.pl -f" on various kernel files 
>> > actually continue on to submit actual defect fixing
>> > or optimization or code clarity patches.
>> 
>> That's also my experience from maintaining wireless-drivers for a year,
>> this seems to be a "hit and run" type of phenomenon.
>
> Should we be looking for someone to run a "wireless-driver-cleanups"
> tree?  They could handle the cleanups and trivial stuff, and send
> you a pull request a couple of times per release...?

Not a bad idea! But I don't think we need a separate tree as applying
patches from patchwork is easy. It should be doable that we add an
account to patchwork and whenever I see a this type of trivial cleanup
patch I'll assign it to the cleanup maintainer and whenever he/she
thinks it's ready he assigns the patch back to me and I'll apply it.

The only difficult part is finding a victim/volunteer to
do that ;)

-- 
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: wireless-drivers: random cleanup patches piling up

[Sudip Mukherjee]

On Fri, Jan 22, 2016 at 05:54:12PM +0200, Kalle Valo wrote:
> "John W. Linville" <linville@tuxdriver.com> writes:
> 
> > On Fri, Jan 22, 2016 at 02:21:20PM +0200, Kalle Valo wrote:
> >> Joe Perches <joe@perches.com> writes:
> >> 
> >> > On Thu, 2016-01-21 at 16:58 +0200, Kalle Valo wrote:
> >> >> Hi,
> >> >> 
> >> >> I have quite a lot of random cleanup patches from new developers waiting
> >> >> in my queue:
> >> >> 
> >> >> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&delegate=25621&order=date
> >> >> 
> >> >> (Not all of them are cleanup patches, there are also few patches
> >> >> deferred due to other reasons, but you get the idea.)
> >> >> 
> >> >> These cleanup patches usually take quite a lot of my time and I'm
> >> >> starting to doubt the benefit, compared to the time needed to dig
> >> >> through them and figuring out what to apply. And this is of course time
> >> >> away from other patches, so it's slowing down "real" development.
> >> >> 
> >> >> I really don't know what to do. Part of me is saying that I just should
> >> >> drop them unless it's reviewed by a more experienced developer but on
> >> >> the other hand this is a good way get new developers onboard.
> >> >> 
> >> >> What others think? Are these kind of patches useful?
> >> >
> >> > Some yes, mostly not really.
> >> >
> >> > While whitespace style patches have some small value,
> >> > very few of the new contributors that use tools like
> >> > "scripts/checkpatch.pl -f" on various kernel files 
> >> > actually continue on to submit actual defect fixing
> >> > or optimization or code clarity patches.
> >> 
> >> That's also my experience from maintaining wireless-drivers for a year,
> >> this seems to be a "hit and run" type of phenomenon.
> >
> > Should we be looking for someone to run a "wireless-driver-cleanups"
> > tree?  They could handle the cleanups and trivial stuff, and send
> > you a pull request a couple of times per release...?
> 
> Not a bad idea! But I don't think we need a separate tree as applying
> patches from patchwork is easy. It should be doable that we add an
> account to patchwork and whenever I see a this type of trivial cleanup
> patch I'll assign it to the cleanup maintainer and whenever he/she
> thinks it's ready he assigns the patch back to me and I'll apply it.
> 
> The only difficult part is finding a victim/volunteer to
> do that ;)

I can be a volunteer (victim?). Though i donot know much about
wireless-drivers, but I do know a little about cleanup patches.
And maybe, in the process I will start knowing wireless-drivers.

regards
sudip

Re: wireless-drivers: random cleanup patches piling up

[Sudip Mukherjee]

On Fri, Jan 22, 2016 at 05:54:12PM +0200, Kalle Valo wrote:
> "John W. Linville" <linville@tuxdriver.com> writes:
> 
> > On Fri, Jan 22, 2016 at 02:21:20PM +0200, Kalle Valo wrote:
> >> Joe Perches <joe@perches.com> writes:
> >> 
> >> > On Thu, 2016-01-21 at 16:58 +0200, Kalle Valo wrote:
> >> >> Hi,
> >> >> 
> >> >> I have quite a lot of random cleanup patches from new developers waiting
> >> >> in my queue:
> >> >> 
> >> >> https://patchwork.kernel.org/project/linux-wireless/list/?state\x10&delegate%621&orderÚte
> >> >> 
> >> >> (Not all of them are cleanup patches, there are also few patches
> >> >> deferred due to other reasons, but you get the idea.)
> >> >> 
> >> >> These cleanup patches usually take quite a lot of my time and I'm
> >> >> starting to doubt the benefit, compared to the time needed to dig
> >> >> through them and figuring out what to apply. And this is of course time
> >> >> away from other patches, so it's slowing down "real" development.
> >> >> 
> >> >> I really don't know what to do. Part of me is saying that I just should
> >> >> drop them unless it's reviewed by a more experienced developer but on
> >> >> the other hand this is a good way get new developers onboard.
> >> >> 
> >> >> What others think? Are these kind of patches useful?
> >> >
> >> > Some yes, mostly not really.
> >> >
> >> > While whitespace style patches have some small value,
> >> > very few of the new contributors that use tools like
> >> > "scripts/checkpatch.pl -f" on various kernel files 
> >> > actually continue on to submit actual defect fixing
> >> > or optimization or code clarity patches.
> >> 
> >> That's also my experience from maintaining wireless-drivers for a year,
> >> this seems to be a "hit and run" type of phenomenon.
> >
> > Should we be looking for someone to run a "wireless-driver-cleanups"
> > tree?  They could handle the cleanups and trivial stuff, and send
> > you a pull request a couple of times per release...?
> 
> Not a bad idea! But I don't think we need a separate tree as applying
> patches from patchwork is easy. It should be doable that we add an
> account to patchwork and whenever I see a this type of trivial cleanup
> patch I'll assign it to the cleanup maintainer and whenever he/she
> thinks it's ready he assigns the patch back to me and I'll apply it.
> 
> The only difficult part is finding a victim/volunteer to
> do that ;)

I can be a volunteer (victim?). Though i donot know much about
wireless-drivers, but I do know a little about cleanup patches.
And maybe, in the process I will start knowing wireless-drivers.

regards
sudip
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: wireless-drivers: random cleanup patches piling up

[Kalle Valo]

Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:

>> >> That's also my experience from maintaining wireless-drivers for a year,
>> >> this seems to be a "hit and run" type of phenomenon.
>> >
>> > Should we be looking for someone to run a "wireless-driver-cleanups"
>> > tree?  They could handle the cleanups and trivial stuff, and send
>> > you a pull request a couple of times per release...?
>> 
>> Not a bad idea! But I don't think we need a separate tree as applying
>> patches from patchwork is easy. It should be doable that we add an
>> account to patchwork and whenever I see a this type of trivial cleanup
>> patch I'll assign it to the cleanup maintainer and whenever he/she
>> thinks it's ready he assigns the patch back to me and I'll apply it.
>> 
>> The only difficult part is finding a victim/volunteer to
>> do that ;)
>
> I can be a volunteer (victim?). Though i donot know much about
> wireless-drivers, but I do know a little about cleanup patches.
> And maybe, in the process I will start knowing wireless-drivers.

I think it's better that you have prior experience with linux-wireless
before doing something like this. You can start by reviewing patches and
providing Reviewed-by tags.

-- 
Kalle Valo

Re: wireless-drivers: random cleanup patches piling up

[Kalle Valo]

Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:

>> >> That's also my experience from maintaining wireless-drivers for a year,
>> >> this seems to be a "hit and run" type of phenomenon.
>> >
>> > Should we be looking for someone to run a "wireless-driver-cleanups"
>> > tree?  They could handle the cleanups and trivial stuff, and send
>> > you a pull request a couple of times per release...?
>> 
>> Not a bad idea! But I don't think we need a separate tree as applying
>> patches from patchwork is easy. It should be doable that we add an
>> account to patchwork and whenever I see a this type of trivial cleanup
>> patch I'll assign it to the cleanup maintainer and whenever he/she
>> thinks it's ready he assigns the patch back to me and I'll apply it.
>> 
>> The only difficult part is finding a victim/volunteer to
>> do that ;)
>
> I can be a volunteer (victim?). Though i donot know much about
> wireless-drivers, but I do know a little about cleanup patches.
> And maybe, in the process I will start knowing wireless-drivers.

I think it's better that you have prior experience with linux-wireless
before doing something like this. You can start by reviewing patches and
providing Reviewed-by tags.

-- 
Kalle Valo

Re: wireless-drivers: random cleanup patches piling up

[Sudip Mukherjee]

On Fri, Jan 29, 2016 at 10:08:23AM +0200, Kalle Valo wrote:
> Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
> 
> >> >> That's also my experience from maintaining wireless-drivers for a year,
> >> >> this seems to be a "hit and run" type of phenomenon.
> >> >
> >> > Should we be looking for someone to run a "wireless-driver-cleanups"
> >> > tree?  They could handle the cleanups and trivial stuff, and send
> >> > you a pull request a couple of times per release...?
> >> 
> >> Not a bad idea! But I don't think we need a separate tree as applying
> >> patches from patchwork is easy. It should be doable that we add an
> >> account to patchwork and whenever I see a this type of trivial cleanup
> >> patch I'll assign it to the cleanup maintainer and whenever he/she
> >> thinks it's ready he assigns the patch back to me and I'll apply it.
> >> 
> >> The only difficult part is finding a victim/volunteer to
> >> do that ;)
> >
> > I can be a volunteer (victim?). Though i donot know much about
> > wireless-drivers, but I do know a little about cleanup patches.
> > And maybe, in the process I will start knowing wireless-drivers.
> 
> I think it's better that you have prior experience with linux-wireless
> before doing something like this. You can start by reviewing patches and
> providing Reviewed-by tags.

Sure, I am starting that way. I checked in patchwork and I do not see
any checkpatch related patch pending (except staging, which Greg will
handle). I think you must have cleared all of them.

regards
sudip

Re: wireless-drivers: random cleanup patches piling up

[Sudip Mukherjee]

On Fri, Jan 29, 2016 at 10:08:23AM +0200, Kalle Valo wrote:
> Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
> 
> >> >> That's also my experience from maintaining wireless-drivers for a year,
> >> >> this seems to be a "hit and run" type of phenomenon.
> >> >
> >> > Should we be looking for someone to run a "wireless-driver-cleanups"
> >> > tree?  They could handle the cleanups and trivial stuff, and send
> >> > you a pull request a couple of times per release...?
> >> 
> >> Not a bad idea! But I don't think we need a separate tree as applying
> >> patches from patchwork is easy. It should be doable that we add an
> >> account to patchwork and whenever I see a this type of trivial cleanup
> >> patch I'll assign it to the cleanup maintainer and whenever he/she
> >> thinks it's ready he assigns the patch back to me and I'll apply it.
> >> 
> >> The only difficult part is finding a victim/volunteer to
> >> do that ;)
> >
> > I can be a volunteer (victim?). Though i donot know much about
> > wireless-drivers, but I do know a little about cleanup patches.
> > And maybe, in the process I will start knowing wireless-drivers.
> 
> I think it's better that you have prior experience with linux-wireless
> before doing something like this. You can start by reviewing patches and
> providing Reviewed-by tags.

Sure, I am starting that way. I checked in patchwork and I do not see
any checkpatch related patch pending (except staging, which Greg will
handle). I think you must have cleared all of them.

regards
sudip

Re: wireless-drivers: random cleanup patches piling up

[Kalle Valo]

Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:

> Sure, I am starting that way. I checked in patchwork and I do not see
> any checkpatch related patch pending (except staging, which Greg will
> handle). I think you must have cleared all of them.

They are in deferred state. The search functionality in patchwork is not
that intuitive and they are not easy to find so here's a direct link:

https://patchwork.kernel.org/project/linux-wireless/list/?state=10&order=date

-- 
Kalle Valo

Re: wireless-drivers: random cleanup patches piling up

[Kalle Valo]

Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:

> Sure, I am starting that way. I checked in patchwork and I do not see
> any checkpatch related patch pending (except staging, which Greg will
> handle). I think you must have cleared all of them.

They are in deferred state. The search functionality in patchwork is not
that intuitive and they are not easy to find so here's a direct link:

https://patchwork.kernel.org/project/linux-wireless/list/?state\x10&orderÚte

-- 
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: wireless-drivers: random cleanup patches piling up

[Julian Calaby]

Hi Kalle,

On Mon, Feb 1, 2016 at 7:21 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
> Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
>
>> Sure, I am starting that way. I checked in patchwork and I do not see
>> any checkpatch related patch pending (except staging, which Greg will
>> handle). I think you must have cleared all of them.
>
> They are in deferred state. The search functionality in patchwork is not
> that intuitive and they are not easy to find so here's a direct link:
>
> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&order=date

I'm currently going through that list and producing a bundle of
"applyable" patches.

My criteria is:
1. The change is sane.
2. It's either obviously correct, I can review it, or someone else has
reviewed or acked it.
3. No changes other than rebasing and fixing commit messages are
required to apply it.

Some of these patches need work on their commit messages, some are
complicated enough that I feel I should be providing review notes so
someone else can double check my review, and all of them should be
rebased and compile tested. Also, some are controversial, so I'll be
segregating them from the main set.

How would you like me to communicate this list to you? I'm happy to
provide branches you can pull from or I could just post updated
versions to the list and give reviewed-by tags to those that don't
need more work.

Every patch will get an email on linux-wireless regardless.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: wireless-drivers: random cleanup patches piling up

[Julian Calaby]

Hi Kalle,

On Mon, Feb 1, 2016 at 7:21 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
> Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
>
>> Sure, I am starting that way. I checked in patchwork and I do not see
>> any checkpatch related patch pending (except staging, which Greg will
>> handle). I think you must have cleared all of them.
>
> They are in deferred state. The search functionality in patchwork is not
> that intuitive and they are not easy to find so here's a direct link:
>
> https://patchwork.kernel.org/project/linux-wireless/list/?state\x10&orderÚte

I'm currently going through that list and producing a bundle of
"applyable" patches.

My criteria is:
1. The change is sane.
2. It's either obviously correct, I can review it, or someone else has
reviewed or acked it.
3. No changes other than rebasing and fixing commit messages are
required to apply it.

Some of these patches need work on their commit messages, some are
complicated enough that I feel I should be providing review notes so
someone else can double check my review, and all of them should be
rebased and compile tested. Also, some are controversial, so I'll be
segregating them from the main set.

How would you like me to communicate this list to you? I'm happy to
provide branches you can pull from or I could just post updated
versions to the list and give reviewed-by tags to those that don't
need more work.

Every patch will get an email on linux-wireless regardless.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: wireless-drivers: random cleanup patches piling up

[Kalle Valo]

Julian Calaby <julian.calaby@gmail.com> writes:

> Hi Kalle,
>
> On Mon, Feb 1, 2016 at 7:21 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>> Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
>>
>>> Sure, I am starting that way. I checked in patchwork and I do not see
>>> any checkpatch related patch pending (except staging, which Greg will
>>> handle). I think you must have cleared all of them.
>>
>> They are in deferred state. The search functionality in patchwork is not
>> that intuitive and they are not easy to find so here's a direct link:
>>
>> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&order=date
>
> I'm currently going through that list and producing a bundle of
> "applyable" patches.

Nice.

> My criteria is:
> 1. The change is sane.
> 2. It's either obviously correct, I can review it, or someone else has
> reviewed or acked it.
> 3. No changes other than rebasing and fixing commit messages are
> required to apply it.

BTW, 'git am -s -3' is the best way to apply a patch. The three way
merge is awesome (if the submitter has sent the patch correctly).

> Some of these patches need work on their commit messages, some are
> complicated enough that I feel I should be providing review notes so
> someone else can double check my review, and all of them should be
> rebased and compile tested. Also, some are controversial, so I'll be
> segregating them from the main set.
>
> How would you like me to communicate this list to you? I'm happy to
> provide branches you can pull from or I could just post updated
> versions to the list and give reviewed-by tags to those that don't
> need more work.
>
> Every patch will get an email on linux-wireless regardless.

I guess posting the patches to linux-wireless is the easiest for
everyone? I have a script which automatically takes patches from
patchwork so that's very easy for me. But remember to use Signed-off-by
instead of Reviewed-by as you are resending the patches.

Thanks you, your help here is very much appreciated.

-- 
Kalle Valo

Re: wireless-drivers: random cleanup patches piling up

[Kalle Valo]

Julian Calaby <julian.calaby@gmail.com> writes:

> Hi Kalle,
>
> On Mon, Feb 1, 2016 at 7:21 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>> Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
>>
>>> Sure, I am starting that way. I checked in patchwork and I do not see
>>> any checkpatch related patch pending (except staging, which Greg will
>>> handle). I think you must have cleared all of them.
>>
>> They are in deferred state. The search functionality in patchwork is not
>> that intuitive and they are not easy to find so here's a direct link:
>>
>> https://patchwork.kernel.org/project/linux-wireless/list/?state\x10&orderÚte
>
> I'm currently going through that list and producing a bundle of
> "applyable" patches.

Nice.

> My criteria is:
> 1. The change is sane.
> 2. It's either obviously correct, I can review it, or someone else has
> reviewed or acked it.
> 3. No changes other than rebasing and fixing commit messages are
> required to apply it.

BTW, 'git am -s -3' is the best way to apply a patch. The three way
merge is awesome (if the submitter has sent the patch correctly).

> Some of these patches need work on their commit messages, some are
> complicated enough that I feel I should be providing review notes so
> someone else can double check my review, and all of them should be
> rebased and compile tested. Also, some are controversial, so I'll be
> segregating them from the main set.
>
> How would you like me to communicate this list to you? I'm happy to
> provide branches you can pull from or I could just post updated
> versions to the list and give reviewed-by tags to those that don't
> need more work.
>
> Every patch will get an email on linux-wireless regardless.

I guess posting the patches to linux-wireless is the easiest for
everyone? I have a script which automatically takes patches from
patchwork so that's very easy for me. But remember to use Signed-off-by
instead of Reviewed-by as you are resending the patches.

Thanks you, your help here is very much appreciated.

-- 
Kalle Valo
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: wireless-drivers: random cleanup patches piling up

[Julian Calaby]

Hi Kalle,

On Wed, Mar 16, 2016 at 8:22 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
> Julian Calaby <julian.calaby@gmail.com> writes:
>
>> Hi Kalle,
>>
>> On Mon, Feb 1, 2016 at 7:21 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>>> Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
>>>
>>>> Sure, I am starting that way. I checked in patchwork and I do not see
>>>> any checkpatch related patch pending (except staging, which Greg will
>>>> handle). I think you must have cleared all of them.
>>>
>>> They are in deferred state. The search functionality in patchwork is not
>>> that intuitive and they are not easy to find so here's a direct link:
>>>
>>> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&order=date
>>
>> I'm currently going through that list and producing a bundle of
>> "applyable" patches.
>
> Nice.

Thanks, I figured that checking the deferred list on patchwork at some
point would be a good plan. After a release seemed like a good time to
do it.

>> My criteria is:
>> 1. The change is sane.
>> 2. It's either obviously correct, I can review it, or someone else has
>> reviewed or acked it.
>> 3. No changes other than rebasing and fixing commit messages are
>> required to apply it.
>
> BTW, 'git am -s -3' is the best way to apply a patch. The three way
> merge is awesome (if the submitter has sent the patch correctly).
>
>> Some of these patches need work on their commit messages, some are
>> complicated enough that I feel I should be providing review notes so
>> someone else can double check my review, and all of them should be
>> rebased and compile tested. Also, some are controversial, so I'll be
>> segregating them from the main set.
>>
>> How would you like me to communicate this list to you? I'm happy to
>> provide branches you can pull from or I could just post updated
>> versions to the list and give reviewed-by tags to those that don't
>> need more work.
>>
>> Every patch will get an email on linux-wireless regardless.
>
> I guess posting the patches to linux-wireless is the easiest for
> everyone? I have a script which automatically takes patches from
> patchwork so that's very easy for me. But remember to use Signed-off-by
> instead of Reviewed-by as you are resending the patches.

If they end up being exactly identical to the original, I'll just add
reviewed-bys to the original patches, otherwise I'll do exactly that.

> Thanks you, your help here is very much appreciated.

No problem!

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: wireless-drivers: random cleanup patches piling up

[Julian Calaby]

Hi Kalle,

On Wed, Mar 16, 2016 at 8:22 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
> Julian Calaby <julian.calaby@gmail.com> writes:
>
>> Hi Kalle,
>>
>> On Mon, Feb 1, 2016 at 7:21 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>>> Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
>>>
>>>> Sure, I am starting that way. I checked in patchwork and I do not see
>>>> any checkpatch related patch pending (except staging, which Greg will
>>>> handle). I think you must have cleared all of them.
>>>
>>> They are in deferred state. The search functionality in patchwork is not
>>> that intuitive and they are not easy to find so here's a direct link:
>>>
>>> https://patchwork.kernel.org/project/linux-wireless/list/?state\x10&orderÚte
>>
>> I'm currently going through that list and producing a bundle of
>> "applyable" patches.
>
> Nice.

Thanks, I figured that checking the deferred list on patchwork at some
point would be a good plan. After a release seemed like a good time to
do it.

>> My criteria is:
>> 1. The change is sane.
>> 2. It's either obviously correct, I can review it, or someone else has
>> reviewed or acked it.
>> 3. No changes other than rebasing and fixing commit messages are
>> required to apply it.
>
> BTW, 'git am -s -3' is the best way to apply a patch. The three way
> merge is awesome (if the submitter has sent the patch correctly).
>
>> Some of these patches need work on their commit messages, some are
>> complicated enough that I feel I should be providing review notes so
>> someone else can double check my review, and all of them should be
>> rebased and compile tested. Also, some are controversial, so I'll be
>> segregating them from the main set.
>>
>> How would you like me to communicate this list to you? I'm happy to
>> provide branches you can pull from or I could just post updated
>> versions to the list and give reviewed-by tags to those that don't
>> need more work.
>>
>> Every patch will get an email on linux-wireless regardless.
>
> I guess posting the patches to linux-wireless is the easiest for
> everyone? I have a script which automatically takes patches from
> patchwork so that's very easy for me. But remember to use Signed-off-by
> instead of Reviewed-by as you are resending the patches.

If they end up being exactly identical to the original, I'll just add
reviewed-bys to the original patches, otherwise I'll do exactly that.

> Thanks you, your help here is very much appreciated.

No problem!

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: wireless-drivers: random cleanup patches piling up

[Julian Calaby]

Hi Kalle,

On Wed, Mar 16, 2016 at 8:42 PM, Julian Calaby <julian.calaby@gmail.com> wrote:
> Hi Kalle,
>
> On Wed, Mar 16, 2016 at 8:22 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>> Julian Calaby <julian.calaby@gmail.com> writes:
>>
>>> Hi Kalle,
>>>
>>> On Mon, Feb 1, 2016 at 7:21 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>>>> Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
>>>>
>>>>> Sure, I am starting that way. I checked in patchwork and I do not see
>>>>> any checkpatch related patch pending (except staging, which Greg will
>>>>> handle). I think you must have cleared all of them.
>>>>
>>>> They are in deferred state. The search functionality in patchwork is not
>>>> that intuitive and they are not easy to find so here's a direct link:
>>>>
>>>> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&order=date
>>>
>>> I'm currently going through that list and producing a bundle of
>>> "applyable" patches.
>>
>> Nice.
>
> Thanks, I figured that checking the deferred list on patchwork at some
> point would be a good plan. After a release seemed like a good time to
> do it.
>
>>> My criteria is:
>>> 1. The change is sane.
>>> 2. It's either obviously correct, I can review it, or someone else has
>>> reviewed or acked it.
>>> 3. No changes other than rebasing and fixing commit messages are
>>> required to apply it.
>>
>> BTW, 'git am -s -3' is the best way to apply a patch. The three way
>> merge is awesome (if the submitter has sent the patch correctly).
>>
>>> Some of these patches need work on their commit messages, some are
>>> complicated enough that I feel I should be providing review notes so
>>> someone else can double check my review, and all of them should be
>>> rebased and compile tested. Also, some are controversial, so I'll be
>>> segregating them from the main set.
>>>
>>> How would you like me to communicate this list to you? I'm happy to
>>> provide branches you can pull from or I could just post updated
>>> versions to the list and give reviewed-by tags to those that don't
>>> need more work.
>>>
>>> Every patch will get an email on linux-wireless regardless.
>>
>> I guess posting the patches to linux-wireless is the easiest for
>> everyone? I have a script which automatically takes patches from
>> patchwork so that's very easy for me. But remember to use Signed-off-by
>> instead of Reviewed-by as you are resending the patches.
>
> If they end up being exactly identical to the original, I'll just add
> reviewed-bys to the original patches, otherwise I'll do exactly that.

I'm going to just repost everything as it'll just be easier at my end.

Git tree: https://github.com/SkUrRiEr/wireless-drivers-pending

I've split the pending patches into 4 sets:

1. Cleanup: patches that weren't reviewed or were just forgotten.
2. Detail: patches that needed a detailed review
3. More Work: patches that only partially fix a problem
4. Controversial: patches people hated but fit my criteria

I'll go into a lot more detail in my cover letter.

At this point, everything in patchwork that's deferred is either:
1. Unreviewable by me (I poked the authors of most of the older
patches yesterday)
2. An earlier version of a patch I picked up
3. Too "new" (less than a couple of months old)

I'll start sending stuff shortly.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: wireless-drivers: random cleanup patches piling up

[Julian Calaby]

Hi Kalle,

On Wed, Mar 16, 2016 at 8:42 PM, Julian Calaby <julian.calaby@gmail.com> wrote:
> Hi Kalle,
>
> On Wed, Mar 16, 2016 at 8:22 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>> Julian Calaby <julian.calaby@gmail.com> writes:
>>
>>> Hi Kalle,
>>>
>>> On Mon, Feb 1, 2016 at 7:21 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>>>> Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
>>>>
>>>>> Sure, I am starting that way. I checked in patchwork and I do not see
>>>>> any checkpatch related patch pending (except staging, which Greg will
>>>>> handle). I think you must have cleared all of them.
>>>>
>>>> They are in deferred state. The search functionality in patchwork is not
>>>> that intuitive and they are not easy to find so here's a direct link:
>>>>
>>>> https://patchwork.kernel.org/project/linux-wireless/list/?state\x10&orderÚte
>>>
>>> I'm currently going through that list and producing a bundle of
>>> "applyable" patches.
>>
>> Nice.
>
> Thanks, I figured that checking the deferred list on patchwork at some
> point would be a good plan. After a release seemed like a good time to
> do it.
>
>>> My criteria is:
>>> 1. The change is sane.
>>> 2. It's either obviously correct, I can review it, or someone else has
>>> reviewed or acked it.
>>> 3. No changes other than rebasing and fixing commit messages are
>>> required to apply it.
>>
>> BTW, 'git am -s -3' is the best way to apply a patch. The three way
>> merge is awesome (if the submitter has sent the patch correctly).
>>
>>> Some of these patches need work on their commit messages, some are
>>> complicated enough that I feel I should be providing review notes so
>>> someone else can double check my review, and all of them should be
>>> rebased and compile tested. Also, some are controversial, so I'll be
>>> segregating them from the main set.
>>>
>>> How would you like me to communicate this list to you? I'm happy to
>>> provide branches you can pull from or I could just post updated
>>> versions to the list and give reviewed-by tags to those that don't
>>> need more work.
>>>
>>> Every patch will get an email on linux-wireless regardless.
>>
>> I guess posting the patches to linux-wireless is the easiest for
>> everyone? I have a script which automatically takes patches from
>> patchwork so that's very easy for me. But remember to use Signed-off-by
>> instead of Reviewed-by as you are resending the patches.
>
> If they end up being exactly identical to the original, I'll just add
> reviewed-bys to the original patches, otherwise I'll do exactly that.

I'm going to just repost everything as it'll just be easier at my end.

Git tree: https://github.com/SkUrRiEr/wireless-drivers-pending

I've split the pending patches into 4 sets:

1. Cleanup: patches that weren't reviewed or were just forgotten.
2. Detail: patches that needed a detailed review
3. More Work: patches that only partially fix a problem
4. Controversial: patches people hated but fit my criteria

I'll go into a lot more detail in my cover letter.

At this point, everything in patchwork that's deferred is either:
1. Unreviewable by me (I poked the authors of most of the older
patches yesterday)
2. An earlier version of a patch I picked up
3. Too "new" (less than a couple of months old)

I'll start sending stuff shortly.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: wireless-drivers: random cleanup patches piling up

[Joe Perches]

On Fri, 2016-01-22 at 10:12 -0500, John W. Linville wrote:
> On Fri, Jan 22, 2016 at 02:21:20PM +0200, Kalle Valo wrote:
> > Joe Perches <joe@perches.com> writes:
> > 
> > > On Thu, 2016-01-21 at 16:58 +0200, Kalle Valo wrote:
> > > > Hi,
> > > > 
> > > > I have quite a lot of random cleanup patches from new developers waiting
> > > > in my queue:
> > > > 
> > > > https://patchwork.kernel.org/project/linux-wireless/list/?state=10&delegate=25621&order=date
> > > > 
> > > > (Not all of them are cleanup patches, there are also few patches
> > > > deferred due to other reasons, but you get the idea.)
> > > > 
> > > > These cleanup patches usually take quite a lot of my time and I'm
> > > > starting to doubt the benefit, compared to the time needed to dig
> > > > through them and figuring out what to apply. And this is of course time
> > > > away from other patches, so it's slowing down "real" development.
> > > > 
> > > > I really don't know what to do. Part of me is saying that I just should
> > > > drop them unless it's reviewed by a more experienced developer but on
> > > > the other hand this is a good way get new developers onboard.
> > > > 
> > > > What others think? Are these kind of patches useful?
> > > 
> > > Some yes, mostly not really.
> > > 
> > > While whitespace style patches have some small value,
> > > very few of the new contributors that use tools like
> > > "scripts/checkpatch.pl -f" on various kernel files 
> > > actually continue on to submit actual defect fixing
> > > or optimization or code clarity patches.
> > 
> > That's also my experience from maintaining wireless-drivers for a year,
> > this seems to be a "hit and run" type of phenomenon.
> 
> Should we be looking for someone to run a "wireless-driver-cleanups"
> tree?  They could handle the cleanups and trivial stuff, and send
> you a pull request a couple of times per release...?

If you are really interested in this sort of code cleanup,
and not in a new developer that might show up because of
a "my first kernel patch" process, maybe it'd be better
to do a preemptive run of something like:

$ git ls-files drivers/net/wireless | \
  while read file ; do \
    ./scripts/checkpatch.pl -f --fix-inplace --types=spacing $file ; \
  done

with git diff -w, compile every modified file, use objdiff, etc.
and a commit per subdirectory or driver.

A problem with that is checkpatch messages really aren't
dicta and there are some things that maybe look nicer
before the script mucks them up.

For instance, in the first file from that pass:

drivers/net/wireless/admtek/adm8211.c
[]
@@ -273,7 +273,7 @@ static void adm8211_write_sram_bytes(struct ieee80211_hw *dev
                }
        } else {
                for (i = 0; i < len; i += 4) {
-                       u32 val = (buf[i + 0] << 0 ) | (buf[i + 1] << 8 ) |
+                       u32 val = (buf[i + 0] << 0) | (buf[i + 1] << 8 ) |
                                  (buf[i + 2] << 16) | (buf[i + 3] << 24);
                        adm8211_write_sram(dev, addr + i / 4, val);
                }

You could reasonably argue that the "<< 0 )" was used
for alignment and doesn't need to be changed.  Perhaps
this should be "<<  0)" instead.

But a better change would be not be a whitespace change
but "get_unaligned_le32(buf + i)", maybe removing the
temporary too.

And there's an equivalent change that could use
get_unaligned_le16 in the preceding block that
checkpatch doesn't flag.

Anyway, a trivial change like the first block I looked
at could be done automatically, but it really doesn't
improve the code much.

Re: wireless-drivers: random cleanup patches piling up

[Joe Perches]

On Fri, 2016-01-22 at 10:12 -0500, John W. Linville wrote:
> On Fri, Jan 22, 2016 at 02:21:20PM +0200, Kalle Valo wrote:
> > Joe Perches <joe@perches.com> writes:
> > 
> > > On Thu, 2016-01-21 at 16:58 +0200, Kalle Valo wrote:
> > > > Hi,
> > > > 
> > > > I have quite a lot of random cleanup patches from new developers waiting
> > > > in my queue:
> > > > 
> > > > https://patchwork.kernel.org/project/linux-wireless/list/?state\x10&delegate%621&orderÚte
> > > > 
> > > > (Not all of them are cleanup patches, there are also few patches
> > > > deferred due to other reasons, but you get the idea.)
> > > > 
> > > > These cleanup patches usually take quite a lot of my time and I'm
> > > > starting to doubt the benefit, compared to the time needed to dig
> > > > through them and figuring out what to apply. And this is of course time
> > > > away from other patches, so it's slowing down "real" development.
> > > > 
> > > > I really don't know what to do. Part of me is saying that I just should
> > > > drop them unless it's reviewed by a more experienced developer but on
> > > > the other hand this is a good way get new developers onboard.
> > > > 
> > > > What others think? Are these kind of patches useful?
> > > 
> > > Some yes, mostly not really.
> > > 
> > > While whitespace style patches have some small value,
> > > very few of the new contributors that use tools like
> > > "scripts/checkpatch.pl -f" on various kernel files 
> > > actually continue on to submit actual defect fixing
> > > or optimization or code clarity patches.
> > 
> > That's also my experience from maintaining wireless-drivers for a year,
> > this seems to be a "hit and run" type of phenomenon.
> 
> Should we be looking for someone to run a "wireless-driver-cleanups"
> tree?  They could handle the cleanups and trivial stuff, and send
> you a pull request a couple of times per release...?

If you are really interested in this sort of code cleanup,
and not in a new developer that might show up because of
a "my first kernel patch" process, maybe it'd be better
to do a preemptive run of something like:

$ git ls-files drivers/net/wireless | \
  while read file ; do \
    ./scripts/checkpatch.pl -f --fix-inplace --types=spacing $file ; \
  done

with git diff -w, compile every modified file, use objdiff, etc.
and a commit per subdirectory or driver.

A problem with that is checkpatch messages really aren't
dicta and there are some things that maybe look nicer
before the script mucks them up.

For instance, in the first file from that pass:

drivers/net/wireless/admtek/adm8211.c
[]
@@ -273,7 +273,7 @@ static void adm8211_write_sram_bytes(struct ieee80211_hw *dev
                }
        } else {
                for (i = 0; i < len; i += 4) {
-                       u32 val = (buf[i + 0] << 0 ) | (buf[i + 1] << 8 ) |
+                       u32 val = (buf[i + 0] << 0) | (buf[i + 1] << 8 ) |
                                  (buf[i + 2] << 16) | (buf[i + 3] << 24);
                        adm8211_write_sram(dev, addr + i / 4, val);
                }

You could reasonably argue that the "<< 0 )" was used
for alignment and doesn't need to be changed.  Perhaps
this should be "<<  0)" instead.

But a better change would be not be a whitespace change
but "get_unaligned_le32(buf + i)", maybe removing the
temporary too.

And there's an equivalent change that could use
get_unaligned_le16 in the preceding block that
checkpatch doesn't flag.

Anyway, a trivial change like the first block I looked
at could be done automatically, but it really doesn't
improve the code much.

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html