[PATCH] epoll: use wrapper functions

[PATCH] epoll: use wrapper functions

[Changli Gao]

use wrapper functions.

epoll should not touch flags in wait_queue_t. This patch introduces a new
function add_wait_queue_head_exclusive_locked(), for the users, who use
wait queue as a LIFO queue.

Signed-off-by: Changli Gao <xiaosuo@gmail.com>
----
 fs/eventpoll.c       |    5 ++---
 include/linux/wait.h |   15 +++++++++++++--
 2 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index bd056a5..8137f6e 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1140,8 +1140,7 @@ retry:
 		 * ep_poll_callback() when events will become available.
 		 */
 		init_waitqueue_entry(&wait, current);
-		wait.flags |= WQ_FLAG_EXCLUSIVE;
-		__add_wait_queue(&ep->wq, &wait);
+		add_wait_queue_head_exclusive_locked(&ep->wq, &wait);
 
 		for (;;) {
 			/*
@@ -1161,7 +1160,7 @@ retry:
 			jtimeout = schedule_timeout(jtimeout);
 			spin_lock_irqsave(&ep->lock, flags);
 		}
-		__remove_wait_queue(&ep->wq, &wait);
+		remove_wait_queue_locked(&ep->wq, &wait);
 
 		set_current_state(TASK_RUNNING);
 	}
diff --git a/include/linux/wait.h b/include/linux/wait.h
index a48e16b..de2566d 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -407,17 +407,28 @@ do {									\
  * Must be called with the spinlock in the wait_queue_head_t held.
  */
 static inline void add_wait_queue_exclusive_locked(wait_queue_head_t *q,
-						   wait_queue_t * wait)
+						   wait_queue_t *wait)
 {
 	wait->flags |= WQ_FLAG_EXCLUSIVE;
 	__add_wait_queue_tail(q,  wait);
 }
 
 /*
+ * Must be called with the spinlock in the wait_queue_head_t held, and
+ * q must be for exclusive wait only.
+ */
+static inline void add_wait_queue_head_exclusive_locked(wait_queue_head_t *q,
+							wait_queue_t *wait)
+{
+	wait->flags |= WQ_FLAG_EXCLUSIVE;
+	__add_wait_queue(q,  wait);
+}
+
+/*
  * Must be called with the spinlock in the wait_queue_head_t held.
  */
 static inline void remove_wait_queue_locked(wait_queue_head_t *q,
-					    wait_queue_t * wait)
+					    wait_queue_t *wait)
 {
 	__remove_wait_queue(q,  wait);
 }

indiscriminate get_maintainer.pl usage (was [PATCH] epoll: use wrapper functions)

[Stefan Richter]

(Cc list shortened)

Changli Gao wrote:
>  fs/eventpoll.c       |    5 ++---
>  include/linux/wait.h |   15 +++++++++++++--
>  2 files changed, 15 insertions(+), 5 deletions(-)

$ git log fs/eventpoll.c include/linux/wait.h |grep stefanr
    Reported-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
    Tested-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

Please do not Cc me on subsystems that I do not develop nor maintain.
My personal inbox is full enough and my todo-list growing.
When I have time to look at patches to subsystems that I don't work with
normally, I look into my inbox.lkml.

If you rely on scripts/get_maintainer.pl, then please use it more
carefully or improve the script to produce more qualified Cc lists.

Thanks.
-- 
Stefan Richter
-=====-==-=- -=-= --==-
http://arcgraph.de/sr/

Re: indiscriminate get_maintainer.pl usage

[Stefan Richter]

> I should add --nogit option.

The script does get some useful information out of git (but obviously
also some questionable).

--roles and --rolestats give away some more data...

$ scripts/get_maintainer.pl --rolestats -f fs/eventpoll.c
Alexander Viro <viro@zeniv.linux.org.uk> (maintainer:FILESYSTEMS (VFS...)
Andrew Morton <akpm@linux-foundation.org> (commit_signer:2/6=33%)
"Eric W. Biederman" <ebiederm@xmission.com> (commit_signer:2/6=33%)
Davide Libenzi <davidel@xmailserver.org> (commit_signer:2/6=33%)
Roland Dreier <rolandd@cisco.com> (commit_signer:1/6=17%)
Stefan Richter <stefanr@s5r6.in-berlin.de> (commit_signer:1/6=17%)
linux-fsdevel@vger.kernel.org (open list:FILESYSTEMS (VFS...)
linux-kernel@vger.kernel.org (open list)

...but still don't distinguish between signed-off-by or reviewed-by on
one hand, and reported-by and tested-by on the other hand.  Cc'ing a
reporter or tester would only make sense if the patch changes something
that directly affects the reporter's/ tester's setup, e.g. reworks a
provisional fix.
-- 
Stefan Richter
-=====-==-=- -=-= --==-
http://arcgraph.de/sr/

Re: indiscriminate get_maintainer.pl usage

[Davide Libenzi]

On Thu, 6 May 2010, Stefan Richter wrote:

> > I should add --nogit option.
> 
> The script does get some useful information out of git (but obviously
> also some questionable).
> 
> --roles and --rolestats give away some more data...
> 
> $ scripts/get_maintainer.pl --rolestats -f fs/eventpoll.c
> Alexander Viro <viro@zeniv.linux.org.uk> (maintainer:FILESYSTEMS (VFS...)
> Andrew Morton <akpm@linux-foundation.org> (commit_signer:2/6=33%)
> "Eric W. Biederman" <ebiederm@xmission.com> (commit_signer:2/6=33%)
> Davide Libenzi <davidel@xmailserver.org> (commit_signer:2/6=33%)
> Roland Dreier <rolandd@cisco.com> (commit_signer:1/6=17%)
> Stefan Richter <stefanr@s5r6.in-berlin.de> (commit_signer:1/6=17%)
> linux-fsdevel@vger.kernel.org (open list:FILESYSTEMS (VFS...)
> linux-kernel@vger.kernel.org (open list)
> 
> ...but still don't distinguish between signed-off-by or reviewed-by on
> one hand, and reported-by and tested-by on the other hand.  Cc'ing a
> reporter or tester would only make sense if the patch changes something
> that directly affects the reporter's/ tester's setup, e.g. reworks a
> provisional fix.

I have always maintained that code w/out the need of explicit entry in the 
MAINTAINERS file.  For isolated files people usually include the copyright 
owners and the ones listed in the comment header in general.
No problem in being explicitly listed if this causes noise during bug 
reporting.


- Davide

Re: indiscriminate get_maintainer.pl usage

[Joe Perches]

On Thu, 2010-05-06 at 08:42 -0700, Davide Libenzi wrote:
> On Thu, 6 May 2010, Stefan Richter wrote:
> > > I should add --nogit option.
> > The script does get some useful information out of git (but obviously
> > also some questionable).

Stefan, I think that's the price you pay for fame.
Try to do good, get more unsolicited email.

> > --roles and --rolestats give away some more data...
> > $ scripts/get_maintainer.pl --rolestats -f fs/eventpoll.c
> > Alexander Viro <viro@zeniv.linux.org.uk> (maintainer:FILESYSTEMS (VFS...)
> > Andrew Morton <akpm@linux-foundation.org> (commit_signer:2/6=33%)
> > "Eric W. Biederman" <ebiederm@xmission.com> (commit_signer:2/6=33%)
> > Davide Libenzi <davidel@xmailserver.org> (commit_signer:2/6=33%)
> > Roland Dreier <rolandd@cisco.com> (commit_signer:1/6=17%)
> > Stefan Richter <stefanr@s5r6.in-berlin.de> (commit_signer:1/6=17%)
> > linux-fsdevel@vger.kernel.org (open list:FILESYSTEMS (VFS...)
> > linux-kernel@vger.kernel.org (open list)
> > 
> > ...but still don't distinguish between signed-off-by or reviewed-by on
> > one hand, and reported-by and tested-by on the other hand.  Cc'ing a
> > reporter or tester would only make sense if the patch changes something
> > that directly affects the reporter's/ tester's setup, e.g. reworks a
> > provisional fix.
> 
> I have always maintained that code w/out the need of explicit entry in the 
> MAINTAINERS file.  For isolated files people usually include the copyright 
> owners and the ones listed in the comment header in general.
> No problem in being explicitly listed if this causes noise during bug 
> reporting.

There is also the --git-blame option for patches, but blame
is also only a semi-useful tool, as it could show cleanup patches
on individual as authors.

Another option to get_maintainers is --file-emails which looks
inside a file for email addresses.

Right now, the default for # of git commits signed to be
added to the CC list is 1.  It's possible to change the default.

    --git-min-signatures => number of signatures required (default: 1)
    --git-max-maintainers => maximum maintainers to add (default: 5)
    --git-min-percent => minimum percentage of commits required (default: 5)

Maybe some fine-tuning of the defaults could be useful.

Suggestions?

Re: indiscriminate get_maintainer.pl usage

[Davide Libenzi]

On Thu, 6 May 2010, Joe Perches wrote:

> There is also the --git-blame option for patches, but blame
> is also only a semi-useful tool, as it could show cleanup patches
> on individual as authors.
> 
> Another option to get_maintainers is --file-emails which looks
> inside a file for email addresses.
> 
> Right now, the default for # of git commits signed to be
> added to the CC list is 1.  It's possible to change the default.
> 
>     --git-min-signatures => number of signatures required (default: 1)
>     --git-max-maintainers => maximum maintainers to add (default: 5)
>     --git-min-percent => minimum percentage of commits required (default: 5)
> 
> Maybe some fine-tuning of the defaults could be useful.
> 
> Suggestions?

(not knowing the get_maintainers internals ...)
Maybe if you weigh each contributor by the amount (in terms of diffstat 
modulo) of changes, you can get more accurate results.
Part of the weight might be related to the time distance (relative to 
current time) since the changeset.


- Davide

Re: indiscriminate get_maintainer.pl usage

[Joe Perches]

On Thu, 2010-05-06 at 10:59 -0700, Davide Libenzi wrote:
> > Right now, the default for # of git commits signed to be
> > added to the CC list is 1.  It's possible to change the default.
> >     --git-min-signatures => number of signatures required (default: 1)
> >     --git-max-maintainers => maximum maintainers to add (default: 5)
> >     --git-min-percent => minimum percentage of commits required (default: 5)
> > Maybe some fine-tuning of the defaults could be useful.
> > Suggestions?
> (not knowing the get_maintainers internals ...)
> Maybe if you weigh each contributor by the amount (in terms of diffstat 
> modulo) of changes, you can get more accurate results.

That might tend to overweight cleanup against actual logic patches,
especially as the period for commits tracked is defaulted to 1 year. 

> Part of the weight might be related to the time distance
> (relative to current time) since the changeset.

Which is kind of what activity alone does.

I think get_maintainer with --git (the default):

Without a named MAINTAINER:

For relatively active files: works moderately well.

For relatively inactive files: might work less well.

It tends to hide the original developer and show people
that submit cleanup or fix patches.

Maybe the --git portion could be reduce the number of
commit signers added when files have named MAINTAINERS.

Without named MAINTAINERS, maybe the file should be
inspected for author info and those persons added.

If the original commit is after Linus' first wholesale
commit of 2.6.12, maybe the first git author should be
added.

Dunno.  I think any of those might be slight improvements,
but get_maintainers isn't really that much of an issue
as-is.  I think it more important to get the correct
MAINTAINERS entries and file patterns.

[PATCH] get_maintainer.pl: ignore non-maintainer tags

[florian]

Using --git to determine who to send a patch to, it is not
reasonable to include people that only reported an issue or tested a
patch. Thus we restrict the candidates to be the one's listed with
Reviewed-By, Signed-Off-By and Acked-By tags.

The Acked-By: is questionable also, but as people listed with this tag
tend to be active linux gatekeepers, false positives are tolerable.

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 scripts/get_maintainer.pl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 6f97a13..ad38ed3 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -496,7 +496,7 @@ version: $V
 
 MAINTAINER field selection options:
   --email => print email address(es) if any
-    --git => include recent git \*-by: signers
+    --git => include recent git {Reviewed|Signed-Off|Acked}-by: signers
     --git-chief-penguins => include ${penguin_chiefs}
     --git-min-signatures => number of signatures required (default: 1)
     --git-max-maintainers => maximum maintainers to add (default: 5)
@@ -964,7 +964,7 @@ sub vcs_find_signers {
 
     $commits = grep(/$pattern/, @lines);	# of commits
 
-    @lines = grep(/^[-_ 	a-z]+by:.*\@.*$/i, @lines);
+    @lines = grep(/(signed-off|reviewed|acked)-by:.*\@.*$/i, @lines);
     if (!$email_git_penguin_chiefs) {
 	@lines = grep(!/${penguin_chiefs}/i, @lines);
     }
-- 
1.7.0.4

Re: [PATCH] get_maintainer.pl: ignore non-maintainer tags

[Joe Perches]

On Fri, 2010-05-07 at 08:34 +0200, florian@mickler.org wrote:
> Using --git to determine who to send a patch to, it is not
> reasonable to include people that only reported an issue or tested a
> patch.

I think this is a questionable assumption.

People that test or otherwise sign a patch are also good
candidates to review new patches.

cheers, Joe

Re: [PATCH] get_maintainer.pl: ignore non-maintainer tags

[Florian Mickler]

On Thu, May 06, 2010 at 11:39:26PM -0700, Joe Perches wrote:
> On Fri, 2010-05-07 at 08:34 +0200, florian@mickler.org wrote:
> > Using --git to determine who to send a patch to, it is not
> > reasonable to include people that only reported an issue or tested a
> > patch.
> 
> I think this is a questionable assumption.
> 
> People that test or otherwise sign a patch are also good
> candidates to review new patches.
> 
> cheers, Joe

Our views differ then.

It is unreasonable to assume, that someone who is listed via
Tested-By: in a random patch is able to review another patch to the same
file. More likely will he be annoyed, because he see's a patch, dig's
in and then thinks: "wtf? why were i cc'ed on this. I don't know this
code. I don't have time for this!"

Thats just broken.

cheers,
Flo

Re: [PATCH] get_maintainer.pl: ignore non-maintainer tags

[Stefan Richter]

Florian Mickler wrote:
> On Thu, May 06, 2010 at 11:39:26PM -0700, Joe Perches wrote:
>> On Fri, 2010-05-07 at 08:34 +0200, florian@mickler.org wrote:
>>> Using --git to determine who to send a patch to, it is not
>>> reasonable to include people that only reported an issue or tested a
>>> patch.
>> I think this is a questionable assumption.
>>
>> People that test or otherwise sign a patch are also good
>> candidates to review new patches.
>>
>> cheers, Joe
> 
> Our views differ then.
> 
> It is unreasonable to assume, that someone who is listed via
> Tested-By: in a random patch is able to review another patch to the same
> file. More likely will he be annoyed, because he see's a patch, dig's
> in and then thinks: "wtf? why were i cc'ed on this. I don't know this
> code. I don't have time for this!"
> 
> Thats just broken.

Indeed.  Three points:

  - Reported-by: and Tested-by: signed commits were most likely about
    something radically different from what the new patch submission is
    about.

  - get_maintainer.pl is supposed to list maintainer addresses.
    Reporters and testers are not maintainers.

  - If a get_maintainer.pl user expects to get not only maintainer
    addresses but also addresses of potential reviewers, then he shall
    use the mailinglist addresses for that.
    People who /want/ to review patches subscribe to that mailinglist
    or register themselves in MAINTAINERS.

Pushing patches to people of whom it is unknown whether they want or can
review the patches may actually be rude.  If it can be avoided that
get_maintainer.pl lists addresses of people who are likely of that
category, then please teach it to.
-- 
Stefan Richter
-=====-==-=- -=-= --===
http://arcgraph.de/sr/

[PATCH] get_maintainer.pl: optionally ignore non-maintainer tags

[florian]

Using --git to determine who to send a patch to, it is not
reasonable to include people that only reported an issue or tested a
patch. Thus we restrict the candidates to be the one's listed with
Reviewed-By, Signed-Off-By and Acked-By tags.

Signed-Off-By:'s because that are people who are responsible for the code
in question.

Reviewed-By:'s because people responsible for the code in question
obviously thought that the review-feedback for this changeset by that
person was valuable.

The Acked-By: is questionable, but as people listed with this tag
tend to be active linux gatekeepers, false positives are tolerable.

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 scripts/get_maintainer.pl |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 6f97a13..8e974bb 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -25,6 +25,7 @@ my $email_list = 1;
 my $email_subscriber_list = 0;
 my $email_git_penguin_chiefs = 0;
 my $email_git = 1;
+my $email_git_maintainers = 0;
 my $email_git_blame = 0;
 my $email_git_min_signatures = 1;
 my $email_git_max_maintainers = 5;
@@ -50,6 +51,15 @@ my $help = 0;
 
 my $exit = 0;
 
+#
+# tags for people who are either
+# 	a) responsible for the code in question, or
+# 	b) familiar enough with it to give relevant feedback
+my @maintainer_tags = ();
+push(@maintainer_tags,"Signed-Off");
+push(@maintainer_tags,"Reviewed");
+push(@maintainer_tags,"Acked");
+
 my @penguin_chief = ();
 push(@penguin_chief,"Linus Torvalds:torvalds\@linux-foundation.org");
 #Andrew wants in on most everything - 2009/01/14
@@ -100,6 +110,7 @@ my %VCS_cmds_hg = (
 if (!GetOptions(
 		'email!' => \$email,
 		'git!' => \$email_git,
+		'git-maintainers!' => \$email_git_maintainers,
 		'git-blame!' => \$email_git_blame,
 		'git-chief-penguins!' => \$email_git_penguin_chiefs,
 		'git-min-signatures=i' => \$email_git_min_signatures,
@@ -497,6 +508,7 @@ version: $V
 MAINTAINER field selection options:
   --email => print email address(es) if any
     --git => include recent git \*-by: signers
+    --git-maintainers => only use 'Signed-Off-By:', 'Reviewed-By:' and 'Acked-By:' signers
     --git-chief-penguins => include ${penguin_chiefs}
     --git-min-signatures => number of signatures required (default: 1)
     --git-max-maintainers => maximum maintainers to add (default: 5)
@@ -957,6 +969,13 @@ sub vcs_find_signers {
     my ($cmd) = @_;
     my @lines = ();
     my $commits;
+    my $tagPattern;
+    if ($email_git_maintainers) {
+	    $tagPattern = join("|",@maintainer_tags);
+    } else {
+	    $tagPattern = "[^ \t]+";
+    }
+
 
     @lines = &{$VCS_cmds{"execute_cmd"}}($cmd);
 
@@ -964,7 +983,7 @@ sub vcs_find_signers {
 
     $commits = grep(/$pattern/, @lines);	# of commits
 
-    @lines = grep(/^[-_ 	a-z]+by:.*\@.*$/i, @lines);
+    @lines = grep(/^[ \t]*($tagPattern)-by:.*\@.*$/i, @lines);
     if (!$email_git_penguin_chiefs) {
 	@lines = grep(!/${penguin_chiefs}/i, @lines);
     }
-- 
1.7.0.4

[PATCH] [RFC] get_maintainer.pl: only list maintainers by default

[florian]

If scm meta-information is used, only list people that signed-off or
acked or reviewed the code in question.

after all it's called get_maintainer.pl and not
get_random_people_to_look_at_the_code.pl

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 scripts/get_maintainer.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 8e974bb..2c53476 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -25,7 +25,7 @@ my $email_list = 1;
 my $email_subscriber_list = 0;
 my $email_git_penguin_chiefs = 0;
 my $email_git = 1;
-my $email_git_maintainers = 0;
+my $email_git_maintainers = 1;
 my $email_git_blame = 0;
 my $email_git_min_signatures = 1;
 my $email_git_max_maintainers = 5;
-- 
1.7.0.4

Re: [PATCH] [RFC] get_maintainer.pl: only list maintainers by default

[Joe Perches]

On Sat, 2010-05-08 at 23:39 +0200, florian@mickler.org wrote:
> If scm meta-information is used, only list people that signed-off or
> acked or reviewed the code in question.
> 
> after all it's called get_maintainer.pl and not
> get_random_people_to_look_at_the_code.pl

Anyone that actually signs a patch is not "random".

Re: [PATCH] [RFC] get_maintainer.pl: only list maintainers by default

[Stefan Richter]

Joe Perches wrote:
> On Sat, 2010-05-08 at 23:39 +0200, florian@mickler.org wrote:
>> If scm meta-information is used, only list people that signed-off or
>> acked or reviewed the code in question.
>>
>> after all it's called get_maintainer.pl and not
>> get_random_people_to_look_at_the_code.pl
> 
> Anyone that actually signs a patch is not "random".

Reported-by and tested-by signatures in the git history commit /are/ in
fact random WRT getting a list of handlers/ reviewers of a new patch. It
is more likely that reporters and testers cannot or/and do not want to
do anything about such a patch.  Such a mailing is called UBE.
-- 
Stefan Richter
-=====-==-=- -=-= -=--=
http://arcgraph.de/sr/

Re: [PATCH] [RFC] get_maintainer.pl: only list maintainers by default

[Joe Perches]

On Sun, 2010-05-09 at 01:23 +0200, Stefan Richter wrote:
> Joe Perches wrote:
> Reported-by and tested-by signatures in the git history commit /are/ in
> fact random WRT getting a list of handlers/ reviewers of a new patch.

Supporting stats please.

$ git log --since=1-year-ago | grep -Pi "(reported|tested)-by:.*@" \
  | cut -f2- -d":" | sort | uniq -c | sort -rn | head -10
     77  Ingo Molnar <mingo@elte.hu>
     47  Wolfram Sang <w.sang@pengutronix.de>
     42  Stephen Rothwell <sfr@canb.auug.org.au>
     35  Dan Carpenter <error27@gmail.com>
     24  Randy Dunlap <randy.dunlap@oracle.com>
     23  Larry Finger <Larry.Finger@lwfinger.net>
     20  Brian Cavagnolo <brian@cozybit.com>
     17  Frans Pop <elendil@planet.nl>
     17  Christian Kujau <lists@nerdbynature.de>
     16  Alexander Beregalov <a.beregalov@gmail.com>

Look like mostly patch authors and maintainers to me.

Brian Cavagnolo isn't a patch author, but he's an active tester.

> It is more likely that reporters and testers cannot or/and do not want to
> do anything about such a patch.  Such a mailing is called UBE.

I doubt that's true of even half of the reporters and testers.

I also doubt such patch emails fit any reasonable definition
of UBE.

cheers, Joe

Re: [PATCH] [RFC] get_maintainer.pl: only list maintainers by default

[Stefan Richter]

Joe Perches wrote:
> On Sun, 2010-05-09 at 01:23 +0200, Stefan Richter wrote:
>> Joe Perches wrote:
>> Reported-by and tested-by signatures in the git history commit /are/ in
>> fact random WRT getting a list of handlers/ reviewers of a new patch.
> 
> Supporting stats please.
...
> Look like mostly patch authors and maintainers to me.
> 
> Brian Cavagnolo isn't a patch author, but he's an active tester.

So you looked up signatures across the entire tree, found some people
who appear more often than an average reporter/tester, and one of these
is not a classic developer/maintainer.

But how does this indicate a need to include these addresses into
get_maintainer.pl output?  In no way at all.

To get your patch reviewed, Cc the relevant mailinglists.  To get your
patch into the submission pipeline, Cc the maintainer.

>> It is more likely that reporters and testers cannot or/and do not want to
>> do anything about such a patch.  Such a mailing is called UBE.
> 
> I doubt that's true of even half of the reporters and testers.
> 
> I also doubt such patch emails fit any reasonable definition
> of UBE.

The U, B, and E all describe such mail exactly.
-- 
Stefan Richter
-=====-==-=- -=-= -=--=
http://arcgraph.de/sr/

Re: [PATCH] [RFC] get_maintainer.pl: only list maintainers by default

[Joe Perches]

On Sun, 2010-05-09 at 02:22 +0200, Stefan Richter wrote:
> But how does this indicate a need to include these addresses into
> get_maintainer.pl output?  In no way at all.

Supporting stats please.

I gave you some that seem to differ from your
position.

I'd send you Linus' original email from 2007,
but you'd already commented on that, so for now,
unless you come up with some stats that support
your view, I'll stand by mine.

cheers, Joe

Re: [PATCH] [RFC] get_maintainer.pl: only list maintainers by default

[Stefan Richter]

Joe Perches wrote:
> On Sun, 2010-05-09 at 02:22 +0200, Stefan Richter wrote:
>> But how does this indicate a need to include these addresses into
>> get_maintainer.pl output?  In no way at all.
> 
> Supporting stats please.
> 
> I gave you some that seem to differ from your
> position.

What do want with statistics here?  Look at semantics, not at
quantities.  Look at what this thread started.

You apparently overlooked what I wrote earlier, so I repeat it to be sure:

>>>
$ git log fs/eventpoll.c include/linux/wait.h |grep stefanr
    Reported-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
    Tested-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

Please do not Cc me on subsystems that I do not develop nor maintain.
My personal inbox is full enough and my todo-list growing.
When I have time to look at patches to subsystems that I don't work with
normally, I look into my inbox.lkml.

If you rely on scripts/get_maintainer.pl, then please use it more
carefully or improve the script to produce more qualified Cc lists.

Thanks.
<<<

>>>
Cc'ing a
reporter or tester would only make sense if the patch changes something
that directly affects the reporter's/ tester's setup, e.g. reworks a
provisional fix.
<<<
[which is highly unlikely, and in those special cases get_maintainer.pl
is not where we take that info from]

>>>
  - Reported-by: and Tested-by: signed commits were most likely about
    something radically different from what the new patch submission is
    about.
<<<

But wait, you want stats, you get stats.
  $ git log drivers/firewire/ drivers/ieee1394/ |
    grep -e "Tested-by: " -e "Reported-by: "
lists 32 people of whom
  - 0 need to be/ should be included in the address list of a patch
    submission,
  - 2 or 3 were active in firewire subsystem development (but not
    anymore) but were perfectly fine to reach via MAINTAINERS' L: entry
  - 3 or 2 are still somewhat involved in firewire subsystem
    development, but only in special application domains, and they are
    reached via MAINTAINERS' L: entry

You on the other had found e.g. Ingo among testers signatures.  Could
you show a single source file of which a patch should be Cc'd to Ingo
and get_maintainer.pl finds him only among testers or reporters but no
among the MAINTAINERS M: entries nor among the signed-off-bys?  (Or any
of the other maintainers that you found.)
-- 
Stefan Richter
-=====-==-=- -=-= -=--=
http://arcgraph.de/sr/

Re: [PATCH] [RFC] get_maintainer.pl: only list maintainers by default

[Stefan Richter]

Stefan Richter wrote:
> Joe Perches wrote:
>> Supporting stats please.
...
>   $ git log drivers/firewire/ drivers/ieee1394/ |
>     grep -e "Tested-by: " -e "Reported-by: "
> lists 32 people of whom
>   - 0 need to be/ should be included in the address list of a patch
>     submission,
>   - 2 or 3 were active in firewire subsystem development (but not
>     anymore) but were perfectly fine to reach via MAINTAINERS' L: entry

PS:  And when they were still active, they also clearly showed up as
patch authors.

>   - 3 or 2 are still somewhat involved in firewire subsystem
>     development, but only in special application domains, and they are
>     reached via MAINTAINERS' L: entry

Think of mailinglists as a pull model to reach potential reviewers,
whereas personal mail constitute a push model which must be used with a
lot of care --- i.e. when you as a submitter _know_ that these people
are going to want to be directly informed about the patch submission.
-- 
Stefan Richter
-=====-==-=- -=-= -=--=
http://arcgraph.de/sr/

Re: [PATCH] [RFC] get_maintainer.pl: only list maintainers by default

[Florian Mickler]

On Sat, 08 May 2010 17:57:13 -0700
Joe Perches <joe@perches.com> wrote:

> On Sun, 2010-05-09 at 02:22 +0200, Stefan Richter wrote:
> > But how does this indicate a need to include these addresses into
> > get_maintainer.pl output?  In no way at all.
> 
> Supporting stats please.

#number of one-off tested-by/reported-by appearences
git log --since=1-year-ago | grep -Pi "(reported|tested)-by:.*@"   | cut -f2- -d":" | sort | uniq -c  | awk '{print $1}' | grep '^1$' | wc -l
822

cheers,
Flo

p.s.: please double check the cmdline, but i think this is correct.

Re: [PATCH] [RFC] get_maintainer.pl: only list maintainers by default

[Stefan Richter]

Joe Perches wrote:
> I'd send you Linus' original email from 2007,
> but you'd already commented on that, so for now,
> unless you come up with some stats that support
> your view, I'll stand by mine.

I don't remember the particular mail of his that you refer to.  But I
suspect he recommended to /look/ at the history, not exactly to /blindly
grep/ through the history and Cc all addresses that got caught.

If the thread starter had looked at the history of fs/eventpoll.c, he
wouldn't have Cc'd me.  A 5 seconds glance at gitk fs/eventpoll.c would
have sufficed.
-- 
Stefan Richter
-=====-==-=- -=-= -=--=
http://arcgraph.de/sr/

Re: [PATCH] [RFC] get_maintainer.pl: only list maintainers by default

[Florian Mickler]

On Sat, 08 May 2010 15:44:34 -0700
Joe Perches <joe@perches.com> wrote:

> On Sat, 2010-05-08 at 23:39 +0200, florian@mickler.org wrote:
> > If scm meta-information is used, only list people that signed-off or
> > acked or reviewed the code in question.
> > 
> > after all it's called get_maintainer.pl and not
> > get_random_people_to_look_at_the_code.pl
> 
> Anyone that actually signs a patch is not "random".
> 

I actually think that tested-by and reviewed-by is not "signing",
because it is not the person who does the "tag insertion". 

it was one of the signed-off-by's who put the reviewed- and testet-tags
in there. presumably this is done in agreement with the mentioned
person. 

And for facts, please check this out:

$ git log --since=1-year-ago | grep -Pi "(reported|tested)-by:.*@"  \
 | cut -f2- -d":" | sort | uniq | wc -l
1280

so i would say, these numbers (combied with your top ten which
accounts for only 318 signatures ) suggest that those mentioned with
these tag's are one-off bug reporters and not maintainers.

cheers,
Flo

Re: [PATCH] get_maintainer.pl: optionally ignore non-maintainer tags

[Joe Perches]

On Sat, 2010-05-08 at 23:32 +0200, florian@mickler.org wrote:
> Using --git to determine who to send a patch to, it is not
> reasonable to include people that only reported an issue or tested a
> patch. Thus we restrict the candidates to be the one's listed with
> Reviewed-By, Signed-Off-By and Acked-By tags.
[]
> +    my $tagPattern;
> +    if ($email_git_maintainers) {
> +	    $tagPattern = join("|",@maintainer_tags);
> +    } else {
> +	    $tagPattern = "[^ \t]+";

I think the else case should be "[a-z _-]+" as some
non-standard signatures have embedded spaces.

[PATCH v2] get_maintainer.pl: optionally ignore non-maintainer tags

[florian]

Using --git to determine who to send a patch to, it is not
reasonable to include people that only reported an issue or tested a
patch. Thus we restrict the candidates to be the one's listed with
Reviewed-By, Signed-Off-By and Acked-By tags.

Signed-Off-By:'s because that are people who are responsible for the code
in question.

Reviewed-By:'s because people responsible for the code in question
obviously thought that the review-feedback for this changeset by that
person was valuable.

The Acked-By: is questionable, but as people listed with this tag
tend to be active linux gatekeepers, false positives are tolerable.

Signed-off-by: Florian Mickler <florian@mickler.org>
---

change from v1:fixed the tagPattern as per suggestion by Joe.


 scripts/get_maintainer.pl |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 6f97a13..8125b51 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -25,6 +25,7 @@ my $email_list = 1;
 my $email_subscriber_list = 0;
 my $email_git_penguin_chiefs = 0;
 my $email_git = 1;
+my $email_git_maintainers = 0;
 my $email_git_blame = 0;
 my $email_git_min_signatures = 1;
 my $email_git_max_maintainers = 5;
@@ -50,6 +51,15 @@ my $help = 0;
 
 my $exit = 0;
 
+#
+# tags for people who are either
+# 	a) responsible for the code in question, or
+# 	b) familiar enough with it to give relevant feedback
+my @maintainer_tags = ();
+push(@maintainer_tags,"Signed-Off");
+push(@maintainer_tags,"Reviewed");
+push(@maintainer_tags,"Acked");
+
 my @penguin_chief = ();
 push(@penguin_chief,"Linus Torvalds:torvalds\@linux-foundation.org");
 #Andrew wants in on most everything - 2009/01/14
@@ -100,6 +110,7 @@ my %VCS_cmds_hg = (
 if (!GetOptions(
 		'email!' => \$email,
 		'git!' => \$email_git,
+		'git-maintainers!' => \$email_git_maintainers,
 		'git-blame!' => \$email_git_blame,
 		'git-chief-penguins!' => \$email_git_penguin_chiefs,
 		'git-min-signatures=i' => \$email_git_min_signatures,
@@ -497,6 +508,7 @@ version: $V
 MAINTAINER field selection options:
   --email => print email address(es) if any
     --git => include recent git \*-by: signers
+    --git-maintainers => only use 'Signed-Off-By:', 'Reviewed-By:' and 'Acked-By:' signers
     --git-chief-penguins => include ${penguin_chiefs}
     --git-min-signatures => number of signatures required (default: 1)
     --git-max-maintainers => maximum maintainers to add (default: 5)
@@ -957,6 +969,13 @@ sub vcs_find_signers {
     my ($cmd) = @_;
     my @lines = ();
     my $commits;
+    my $tagPattern;
+    if ($email_git_maintainers) {
+	    $tagPattern = join("|",@maintainer_tags);
+    } else {
+	    $tagPattern = "[a-z _-]+";
+    }
+
 
     @lines = &{$VCS_cmds{"execute_cmd"}}($cmd);
 
@@ -964,7 +983,7 @@ sub vcs_find_signers {
 
     $commits = grep(/$pattern/, @lines);	# of commits
 
-    @lines = grep(/^[-_ 	a-z]+by:.*\@.*$/i, @lines);
+    @lines = grep(/^[ \t]*($tagPattern)-by:.*\@.*$/i, @lines);
     if (!$email_git_penguin_chiefs) {
 	@lines = grep(!/${penguin_chiefs}/i, @lines);
     }
-- 
1.7.0.4

Re: [PATCH v2] get_maintainer.pl: optionally ignore non-maintainer tags

[Stefan Richter]

florian@mickler.org wrote:
> Using --git to determine who to send a patch to, it is not
> reasonable to include people that only reported an issue or tested a
> patch. Thus we restrict the candidates to be the one's listed with
> Reviewed-By, Signed-Off-By and Acked-By tags.
> 
> Signed-Off-By:'s because that are people who are responsible for the code
> in question.
> 
> Reviewed-By:'s because people responsible for the code in question
> obviously thought that the review-feedback for this changeset by that
> person was valuable.
> 
> The Acked-By: is questionable, but as people listed with this tag
> tend to be active linux gatekeepers, false positives are tolerable.

This and "get_maintainer.pl: only list maintainers by default" in
conjunction:  Reviewed-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
But *don't* record this in the git history because then somebody might
think I want to be Cc'd on get_maintainer.pl patches.
-- 
Stefan Richter
-=====-==-=- -=-= -=--=
http://arcgraph.de/sr/

[PATCH 0/2] scripts/get_maintainer.pl: trivial improvements

[Joe Perches]

A desire exists to reduce the number of people that are listed by
get_maintainers that may not actually be interested in receiving
patches.  Non-standard signature types may not indicate a desire to
receive patches.  Add the ability to only use selected signature types.

Joe Perches (2):
  scripts/get_maintainer.pl: optionally ignore non-maintainer signatures
  scripts/get_maintainer.pl: add .get_maintainer.conf default options file

 scripts/get_maintainer.pl |   66 ++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 56 insertions(+), 10 deletions(-)

[PATCH 1/2] scripts/get_maintainer.pl: optionally ignore non-maintainer signatures

[Joe Perches]

When using --git to determine who to send a patch to, get_maintainers
will currently include all signatures.  This can include signers that
simply report an issue or test a patch.  Signers that use this tag
are not necessarily good candidates to review new patches.

This patch allows get_maintainers to optionally restrict output to
only signatures that use Signed-off-by:, Acked-by:, or Reviewed-by:.

Signed-off-by: is included because those are people who are
responsible for the code.

Acked-by: is questionable, but as signers that use this tag tend to
be active linux gatekeepers, false positives are tolerable.

Reviewed-by: is included because signers responsible for the code
thought that the review feedback for a changeset by that signer was
valuable.

This patch has been modified from Florian's original submission to
change the supported signature types to the canonical forms and use
slightly different spacing.  A couple of spacing issues were also
corrected in the original source.  The command line argument was
also renamed.

Original-patch-by: Florian Mickler <florian@mickler.org>
Signed-off-by: Florian Mickler <florian@mickler.org>
Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/get_maintainer.pl |   35 ++++++++++++++++++++++++++---------
 1 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 6f97a13..f66018d 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -25,6 +25,7 @@ my $email_list = 1;
 my $email_subscriber_list = 0;
 my $email_git_penguin_chiefs = 0;
 my $email_git = 1;
+my $email_git_all_signature_types = 1;
 my $email_git_blame = 0;
 my $email_git_min_signatures = 1;
 my $email_git_max_maintainers = 5;
@@ -51,9 +52,9 @@ my $help = 0;
 my $exit = 0;
 
 my @penguin_chief = ();
-push(@penguin_chief,"Linus Torvalds:torvalds\@linux-foundation.org");
+push(@penguin_chief, "Linus Torvalds:torvalds\@linux-foundation.org");
 #Andrew wants in on most everything - 2009/01/14
-#push(@penguin_chief,"Andrew Morton:akpm\@linux-foundation.org");
+#push(@penguin_chief, "Andrew Morton:akpm\@linux-foundation.org");
 
 my @penguin_chief_names = ();
 foreach my $chief (@penguin_chief) {
@@ -63,7 +64,16 @@ foreach my $chief (@penguin_chief) {
 	push(@penguin_chief_names, $chief_name);
     }
 }
-my $penguin_chiefs = "\(" . join("|",@penguin_chief_names) . "\)";
+my $penguin_chiefs = "\(" . join("|", @penguin_chief_names) . "\)";
+
+# Signature types of people who are either
+# 	a) responsible for the code in question, or
+# 	b) familiar enough with it to give relevant feedback
+my @signature_tags = ();
+push(@signature_tags, "Signed-off-by:");
+push(@signature_tags, "Reviewed-by:");
+push(@signature_tags, "Acked-by:");
+my $signaturePattern = "\(" . join("|", @signature_tags) . "\)";
 
 # rfc822 email address - preloaded methods go here.
 my $rfc822_lwsp = "(?:(?:\\r\\n)?[ \\t])";
@@ -100,6 +110,7 @@ my %VCS_cmds_hg = (
 if (!GetOptions(
 		'email!' => \$email,
 		'git!' => \$email_git,
+		'git-all-signature-types!' => \$email_git_all_signature_types,
 		'git-blame!' => \$email_git_blame,
 		'git-chief-penguins!' => \$email_git_penguin_chiefs,
 		'git-min-signatures=i' => \$email_git_min_signatures,
@@ -180,6 +191,10 @@ if (!top_of_kernel_tree($lk_path)) {
 	. "a linux kernel source tree.\n";
 }
 
+if ($email_git_all_signature_types) {
+    $signaturePattern = "(.+?)[Bb][Yy]:";
+}
+
 ## Read MAINTAINERS for type/value pairs
 
 my @typevalue = ();
@@ -497,13 +512,15 @@ version: $V
 MAINTAINER field selection options:
   --email => print email address(es) if any
     --git => include recent git \*-by: signers
+    --git-all-signature-types => include signers regardless of signature type
+        or use only ${signaturePattern} signers (default: $email_git_all_signature_types)
     --git-chief-penguins => include ${penguin_chiefs}
-    --git-min-signatures => number of signatures required (default: 1)
-    --git-max-maintainers => maximum maintainers to add (default: 5)
-    --git-min-percent => minimum percentage of commits required (default: 5)
+    --git-min-signatures => number of signatures required (default: $email_git_min_signatures)
+    --git-max-maintainers => maximum maintainers to add (default: $email_git_max_maintainers)
+    --git-min-percent => minimum percentage of commits required (default: $email_git_min_percent)
     --git-blame => use git blame to find modified commits for patch or file
-    --git-since => git history to use (default: 1-year-ago)
-    --hg-since => hg history to use (default: -365)
+    --git-since => git history to use (default: $email_git_since)
+    --hg-since => hg history to use (default: $email_hg_since)
     --m => include maintainer(s) if any
     --n => include name 'Full Name <addr\@domain.tld>'
     --l => include list(s) if any
@@ -964,7 +981,7 @@ sub vcs_find_signers {
 
     $commits = grep(/$pattern/, @lines);	# of commits
 
-    @lines = grep(/^[-_ 	a-z]+by:.*\@.*$/i, @lines);
+    @lines = grep(/^[ \t]*${signaturePattern}.*\@.*$/, @lines);
     if (!$email_git_penguin_chiefs) {
 	@lines = grep(!/${penguin_chiefs}/i, @lines);
     }
-- 
1.7.0.3.311.g6a6955

Re: [PATCH 1/2] scripts/get_maintainer.pl: optionally ignore non-maintainer signatures

[Florian Mickler]

On Sun,  9 May 2010 21:56:02 -0700
Joe Perches <joe@perches.com> wrote:

> This patch has been modified from Florian's original submission to
> change the supported signature types to the canonical forms and use
> slightly different spacing.  A couple of spacing issues were also
> corrected in the original source.  The command line argument was
> also renamed.
> 
> Original-patch-by: Florian Mickler <florian@mickler.org>
> Signed-off-by: Florian Mickler <florian@mickler.org>

I'm ok with your changes. Looks good.

> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  scripts/get_maintainer.pl |   35 ++++++++++++++++++++++++++---------
>  1 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index 6f97a13..f66018d 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -25,6 +25,7 @@ my $email_list = 1;
>  my $email_subscriber_list = 0;
>  my $email_git_penguin_chiefs = 0;
>  my $email_git = 1;
> +my $email_git_all_signature_types = 1;
>  my $email_git_blame = 0;
>  my $email_git_min_signatures = 1;
>  my $email_git_max_maintainers = 5;
> @@ -51,9 +52,9 @@ my $help = 0;
>  my $exit = 0;
>  
>  my @penguin_chief = ();
> -push(@penguin_chief,"Linus Torvalds:torvalds\@linux-foundation.org");
> +push(@penguin_chief, "Linus Torvalds:torvalds\@linux-foundation.org");
>  #Andrew wants in on most everything - 2009/01/14
> -#push(@penguin_chief,"Andrew Morton:akpm\@linux-foundation.org");
> +#push(@penguin_chief, "Andrew Morton:akpm\@linux-foundation.org");
>  
>  my @penguin_chief_names = ();
>  foreach my $chief (@penguin_chief) {
> @@ -63,7 +64,16 @@ foreach my $chief (@penguin_chief) {
>  	push(@penguin_chief_names, $chief_name);
>      }
>  }
> -my $penguin_chiefs = "\(" . join("|",@penguin_chief_names) . "\)";
> +my $penguin_chiefs = "\(" . join("|", @penguin_chief_names) . "\)";
> +
> +# Signature types of people who are either
> +# 	a) responsible for the code in question, or
> +# 	b) familiar enough with it to give relevant feedback
> +my @signature_tags = ();
> +push(@signature_tags, "Signed-off-by:");
> +push(@signature_tags, "Reviewed-by:");
> +push(@signature_tags, "Acked-by:");
> +my $signaturePattern = "\(" . join("|", @signature_tags) . "\)";
>  
>  # rfc822 email address - preloaded methods go here.
>  my $rfc822_lwsp = "(?:(?:\\r\\n)?[ \\t])";
> @@ -100,6 +110,7 @@ my %VCS_cmds_hg = (
>  if (!GetOptions(
>  		'email!' => \$email,
>  		'git!' => \$email_git,
> +		'git-all-signature-types!' => \$email_git_all_signature_types,
>  		'git-blame!' => \$email_git_blame,
>  		'git-chief-penguins!' => \$email_git_penguin_chiefs,
>  		'git-min-signatures=i' => \$email_git_min_signatures,
> @@ -180,6 +191,10 @@ if (!top_of_kernel_tree($lk_path)) {
>  	. "a linux kernel source tree.\n";
>  }
>  
> +if ($email_git_all_signature_types) {
> +    $signaturePattern = "(.+?)[Bb][Yy]:";
> +}
> +
>  ## Read MAINTAINERS for type/value pairs
>  
>  my @typevalue = ();
> @@ -497,13 +512,15 @@ version: $V
>  MAINTAINER field selection options:
>    --email => print email address(es) if any
>      --git => include recent git \*-by: signers
> +    --git-all-signature-types => include signers regardless of signature type
> +        or use only ${signaturePattern} signers (default: $email_git_all_signature_types)
>      --git-chief-penguins => include ${penguin_chiefs}
> -    --git-min-signatures => number of signatures required (default: 1)
> -    --git-max-maintainers => maximum maintainers to add (default: 5)
> -    --git-min-percent => minimum percentage of commits required (default: 5)
> +    --git-min-signatures => number of signatures required (default: $email_git_min_signatures)
> +    --git-max-maintainers => maximum maintainers to add (default: $email_git_max_maintainers)
> +    --git-min-percent => minimum percentage of commits required (default: $email_git_min_percent)
>      --git-blame => use git blame to find modified commits for patch or file
> -    --git-since => git history to use (default: 1-year-ago)
> -    --hg-since => hg history to use (default: -365)
> +    --git-since => git history to use (default: $email_git_since)
> +    --hg-since => hg history to use (default: $email_hg_since)
>      --m => include maintainer(s) if any
>      --n => include name 'Full Name <addr\@domain.tld>'
>      --l => include list(s) if any
> @@ -964,7 +981,7 @@ sub vcs_find_signers {
>  
>      $commits = grep(/$pattern/, @lines);	# of commits
>  
> -    @lines = grep(/^[-_ 	a-z]+by:.*\@.*$/i, @lines);
> +    @lines = grep(/^[ \t]*${signaturePattern}.*\@.*$/, @lines);
>      if (!$email_git_penguin_chiefs) {
>  	@lines = grep(!/${penguin_chiefs}/i, @lines);
>      }

[PATCH] scripts/get_maintainer.pl: default to not include unspecified tags

[florian]

This changes the default of the option --git-all-signature-types to be
disabled by default.

The effect being, that only certain (currently  Signed-Off-By:,
Acked-By: and Reviewed-By:) tags are used to get adresses of potential
maintainers.

Motivated is this change by the desire to not 'spam' people unnecessary:
A Tested-By or a Reported-By is not ment as a hint that those people
want to/are able to review patches to the code in question.

In a quest to find resilient statistics for this i came up with this:

I produced a list of all the tag-signers not already covered
with a signed-off/acked/reviewed tag somewhere in the last year of git history.

Those were 650 addresses of "assumed non-developers".

And to check if those "assumed non-developers" are professional
testers/reporters worth cc'ing, i then counted their total appearences
in the git log:

469 were mentioned only once.
123 were mentioned twice.
38 three times
8 four times
5 six times
5 five times
1 eight times
1 fourteen times

I believe this supports my thesis, that the ''non-maintainer-tags'' are
not actively useful for patch-review. (except probably the guy
mentioned fourteen times...)

But of course one could also find arguments to poke holes in this
statistics, for example does this statistic not include code-locality:
A tested-by on a patch that touches some specific piece of
code can be more worth than a signed-off in another part of the tree.

But... let's play it safe and let's err on the "safe" side
meaning to not spam those people when in doubt. We already have the
signed-off's and Maintainers file. So this should be ok. And if need be, 
the maintainers can always forward the patch.

[i probably could make a diploma thesis out of this changelog :)]

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 scripts/get_maintainer.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index b82ac95..b228198 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -25,7 +25,7 @@ my $email_list = 1;
 my $email_subscriber_list = 0;
 my $email_git_penguin_chiefs = 0;
 my $email_git = 1;
-my $email_git_all_signature_types = 1;
+my $email_git_all_signature_types = 0;
 my $email_git_blame = 0;
 my $email_git_min_signatures = 1;
 my $email_git_max_maintainers = 5;
-- 
1.7.0.4

Re: [PATCH] scripts/get_maintainer.pl: default to not include unspecified tags

[Wolfram Sang]

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

On Tue, May 11, 2010 at 08:36:36AM +0200, florian@mickler.org wrote:
> This changes the default of the option --git-all-signature-types to be
> disabled by default.
> 
> The effect being, that only certain (currently  Signed-Off-By:,
> Acked-By: and Reviewed-By:) tags are used to get adresses of potential
> maintainers.
> 
> Motivated is this change by the desire to not 'spam' people unnecessary:
> A Tested-By or a Reported-By is not ment as a hint that those people
> want to/are able to review patches to the code in question.
> 
> In a quest to find resilient statistics for this i came up with this:
> 
> I produced a list of all the tag-signers not already covered
> with a signed-off/acked/reviewed tag somewhere in the last year of git history.
> 
> Those were 650 addresses of "assumed non-developers".
> 
> And to check if those "assumed non-developers" are professional
> testers/reporters worth cc'ing, i then counted their total appearences
> in the git log:
> 
> 469 were mentioned only once.
> 123 were mentioned twice.
> 38 three times
> 8 four times
> 5 six times
> 5 five times
> 1 eight times
> 1 fourteen times
> 
> I believe this supports my thesis, that the ''non-maintainer-tags'' are
> not actively useful for patch-review. (except probably the guy
> mentioned fourteen times...)
> 
> But of course one could also find arguments to poke holes in this
> statistics, for example does this statistic not include code-locality:
> A tested-by on a patch that touches some specific piece of
> code can be more worth than a signed-off in another part of the tree.
> 
> But... let's play it safe and let's err on the "safe" side
> meaning to not spam those people when in doubt. We already have the
> signed-off's and Maintainers file. So this should be ok. And if need be, 
> the maintainers can always forward the patch.
> 
> [i probably could make a diploma thesis out of this changelog :)]

:D

> 
> Signed-off-by: Florian Mickler <florian@mickler.org>

I second this reasoning:

Acked-by: Wolfram Sang <w.sang@pengutronix.de>

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] scripts/get_maintainer.pl: default to not include unspecified tags

[Joe Perches]

On Tue, 2010-05-11 at 08:36 +0200, florian@mickler.org wrote:
> This changes the default of the option --git-all-signature-types to be
> disabled by default.
> [i probably could make a diploma thesis out of this changelog :)]

Then of course you would spell check it before submission. ;)

Luckily for you, most here don't seem to care much
about that.

> Signed-off-by: Florian Mickler <florian@mickler.org>

I-can-be-persuaded-by-logic-and-statistics-to-Sign-off-by: Joe Perches <joe@perches.com>

Re: [PATCH] scripts/get_maintainer.pl: default to not include unspecified tags

[Florian Mickler]

On Tue, 11 May 2010 08:59:55 -0700
Joe Perches <joe@perches.com> wrote:

> On Tue, 2010-05-11 at 08:36 +0200, florian@mickler.org wrote:
> > This changes the default of the option --git-all-signature-types to be
> > disabled by default.
> > [i probably could make a diploma thesis out of this changelog :)]
> 
> Then of course you would spell check it before submission. ;)
>

Come on! There were some difficult words in there! Give me some
credit... :)

Cheers,
Flo

p.s.: 
And of course i don't wanna miss out on the obligatory "lalala sorry,
i'm not a native speaker lalala". 

Re: [PATCH] scripts/get_maintainer.pl: default to not include unspecified tags

[Joe Perches]

On Tue, 2010-05-11 at 18:33 +0200, Florian Mickler wrote:
> Come on! There were some difficult words in there! Give me some
> credit... :)

If I had to rely on my prehistoric 2 semesters of German to
communicate in restaurants, I'd go hungry but not thirsty.

cheers, Joe

[PATCH 2/2] scripts/get_maintainer.pl: add .get_maintainer.conf default options file

[Joe Perches]

Allow the use of a .get_maintainer.conf file to control the
default options applied when scripts/get_maintainer.pl is run.

.get_maintainer.conf can contain any valid command-line argument.

File contents are prepended to any additional command line arguments.

Multiple lines may be used, blank lines ignored, # is a comment.

Updated scripts/get_maintainer.pl version to 0.24

Signed-off-by: Joe Perches <joe@perches.com>
Reviewed-by: Florian Mickler <florian@mickler.org>
---
 scripts/get_maintainer.pl |   31 ++++++++++++++++++++++++++++++-
 1 files changed, 30 insertions(+), 1 deletions(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index f66018d..b82ac95 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -13,7 +13,7 @@
 use strict;
 
 my $P = $0;
-my $V = '0.23';
+my $V = '0.24';
 
 use Getopt::Long qw(:config no_auto_abbrev);
 
@@ -107,6 +107,30 @@ my %VCS_cmds_hg = (
     "blame_commit_pattern" => "^([0-9a-f]+):"
 );
 
+if (-f "${lk_path}.get_maintainer.conf") {
+    my @conf_args;
+    open(my $conffile, '<', "${lk_path}.get_maintainer.conf")
+	or warn "$P: Can't open .get_maintainer.conf: $!\n";
+    while (<$conffile>) {
+	my $line = $_;
+
+	$line =~ s/\s*\n?$//g;
+	$line =~ s/^\s*//g;
+	$line =~ s/\s+/ /g;
+
+	next if ($line =~ m/^\s*#/);
+	next if ($line =~ m/^\s*$/);
+
+	my @words = split(" ", $line);
+	foreach my $word (@words) {
+	    last if ($word =~ m/^#/);
+	    push (@conf_args, $word);
+	}
+    }
+    close($conffile);
+    unshift(@ARGV, @conf_args) if @conf_args;
+}
+
 if (!GetOptions(
 		'email!' => \$email,
 		'git!' => \$email_git,
@@ -573,6 +597,11 @@ Notes:
           --git-min-signatures, --git-max-maintainers, --git-min-percent, and
           --git-blame
       Use --hg-since not --git-since to control date selection
+  File ".get_maintainer.conf", if it exists in the linux kernel source root
+      directory, can change whatever get_maintainer defaults are desired.
+      Entries in this file can be any command line argument.
+      This file is prepended to any additional command line arguments.
+      Multiple lines and # comments are allowed.
 EOT
 }
 
-- 
1.7.0.3.311.g6a6955

Re: [PATCH 2/2] scripts/get_maintainer.pl: add .get_maintainer.conf default options file

[Américo Wang]

On Sun, May 09, 2010 at 09:56:03PM -0700, Joe Perches wrote:
>Allow the use of a .get_maintainer.conf file to control the
>default options applied when scripts/get_maintainer.pl is run.
>
>.get_maintainer.conf can contain any valid command-line argument.
>
>File contents are prepended to any additional command line arguments.
>
>Multiple lines may be used, blank lines ignored, # is a comment.

Do we really have to do this? This looks odd to me.

If the user of get_maintainer.pl uses some long command line
arguments, to save his input, he should use bash aliases, not
configure files. I never see configure files used like this.

Also, it doesn't worthy a configure file for such a script like
get_maintainer.pl.

Re: [PATCH 2/2] scripts/get_maintainer.pl: add .get_maintainer.conf default options file

[Florian Mickler]

On Wed, 12 May 2010 14:30:42 +0800
Américo Wang <xiyou.wangcong@gmail.com> wrote:

> On Sun, May 09, 2010 at 09:56:03PM -0700, Joe Perches wrote:
> >Allow the use of a .get_maintainer.conf file to control the
> >default options applied when scripts/get_maintainer.pl is run.
> >
> >.get_maintainer.conf can contain any valid command-line argument.
> >
> >File contents are prepended to any additional command line arguments.
> >
> >Multiple lines may be used, blank lines ignored, # is a comment.
> 
> Do we really have to do this? This looks odd to me.

it's a little hackish, but the code impact is small. if you prefer
standard config-file syntax (i.e. "bla=" ) check the patch below. 

joe, what do you think?

> If the user of get_maintainer.pl uses some long command line
> arguments, to save his input, he should use bash aliases, not
> configure files. I never see configure files used like this.

bash aliases don't work (at least here) with "git send-email --cc-cmd="

> 
> Also, it doesn't worthy a configure file for such a script like
> get_maintainer.pl.

hm.. the only other way to use it via git send-email would be a wrapper
script? 

cheers,
Flo

>From 4f81b09c346075a062c868c59b724442c382b690 Mon Sep 17 00:00:00 2001
From: Florian Mickler <florian@mickler.org>
Date: Wed, 12 May 2010 10:57:24 +0200
Subject: [PATCH] get_maintainer.pl: change config file to use key=value pairs.

the solution to prepend the contents of the config file to the arguments
is unexpected for most people.

so we change it to be key = value syntax.

this approach uses a hash to store references to the config variables as
this makes it easier to implement default/override semantics for the
config file and cmdline.

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 scripts/get_maintainer.pl |  169 ++++++++++++++++++++++++++++-----------------
 1 files changed, 106 insertions(+), 63 deletions(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index b228198..e8aad5c 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -48,6 +48,54 @@ my $from_filename = 0;
 my $pattern_depth = 0;
 my $version = 0;
 my $help = 0;
+my %prefs = (
+		'email' => \$email,
+		'git' => \$email_git,
+		'git-all-signature-types' => \$email_git_all_signature_types,
+		'git-blame' => \$email_git_blame,
+		'git-chief-penguins' => \$email_git_penguin_chiefs,
+		'git-min-signatures' => \$email_git_min_signatures,
+		'git-max-maintainers' => \$email_git_max_maintainers,
+		'git-min-percent' => \$email_git_min_percent,
+		'git-since' => \$email_git_since,
+		'hg-since' => \$email_hg_since,
+		'remove-duplicates' => \$email_remove_duplicates,
+		'maintainer' => \$email_maintainer,
+		'names' => \$email_usename,
+		'list' => \$email_list,
+		'subscribers' => \$email_subscriber_list,
+		'multiline' => \$output_multiline,
+		'roles' => \$output_roles,
+		'rolestats' => \$output_rolestats,
+		'separator' => \$output_separator,
+		'subsystem' => \$subsystem,
+		'status' => \$status,
+		'scm' => \$scm,
+		'web' => \$web,
+		'pattern-depth' => \$pattern_depth,
+		'keywords' => \$keywords,
+		'sections' => \$sections,
+		'file-emails' => \$file_emails,
+		'file' => \$from_filename,
+		'version' => \$version,
+		'help' => \$help,
+);
+
+my $conffile =  "${lk_path}.get_maintainer.conf";
+if (-f $conffile) {
+    open(my $conffile, '<', "${lk_path}.get_maintainer.conf")
+	or warn "$P: Can't open .get_maintainer.conf: $!\n";
+    while (<$conffile>) {
+	chomp;                  # no newline
+	s/#.*//;                # no comments
+	s/^\s+//;               # no leading white
+	s/\s+$//;               # no trailing white
+	next unless length;     # anything left?
+	my ($key, $val) = split(/\s*=\s*/, $_, 2);
+	${$prefs{$key}} = $val;
+    }
+    close($conffile);
+}
 
 my $exit = 0;
 
@@ -107,61 +155,37 @@ my %VCS_cmds_hg = (
     "blame_commit_pattern" => "^([0-9a-f]+):"
 );
 
-if (-f "${lk_path}.get_maintainer.conf") {
-    my @conf_args;
-    open(my $conffile, '<', "${lk_path}.get_maintainer.conf")
-	or warn "$P: Can't open .get_maintainer.conf: $!\n";
-    while (<$conffile>) {
-	my $line = $_;
-
-	$line =~ s/\s*\n?$//g;
-	$line =~ s/^\s*//g;
-	$line =~ s/\s+/ /g;
-
-	next if ($line =~ m/^\s*#/);
-	next if ($line =~ m/^\s*$/);
-
-	my @words = split(" ", $line);
-	foreach my $word (@words) {
-	    last if ($word =~ m/^#/);
-	    push (@conf_args, $word);
-	}
-    }
-    close($conffile);
-    unshift(@ARGV, @conf_args) if @conf_args;
-}
-
-if (!GetOptions(
-		'email!' => \$email,
-		'git!' => \$email_git,
-		'git-all-signature-types!' => \$email_git_all_signature_types,
-		'git-blame!' => \$email_git_blame,
-		'git-chief-penguins!' => \$email_git_penguin_chiefs,
-		'git-min-signatures=i' => \$email_git_min_signatures,
-		'git-max-maintainers=i' => \$email_git_max_maintainers,
-		'git-min-percent=i' => \$email_git_min_percent,
-		'git-since=s' => \$email_git_since,
-		'hg-since=s' => \$email_hg_since,
-		'remove-duplicates!' => \$email_remove_duplicates,
-		'm!' => \$email_maintainer,
-		'n!' => \$email_usename,
-		'l!' => \$email_list,
-		's!' => \$email_subscriber_list,
-		'multiline!' => \$output_multiline,
-		'roles!' => \$output_roles,
-		'rolestats!' => \$output_rolestats,
-		'separator=s' => \$output_separator,
-		'subsystem!' => \$subsystem,
-		'status!' => \$status,
-		'scm!' => \$scm,
-		'web!' => \$web,
-		'pattern-depth=i' => \$pattern_depth,
-		'k|keywords!' => \$keywords,
-		'sections!' => \$sections,
-		'fe|file-emails!' => \$file_emails,
-		'f|file' => \$from_filename,
-		'v|version' => \$version,
-		'h|help|usage' => \$help,
+if (!GetOptions( \%prefs,
+		'email!',
+		'git!',
+		'git-all-signature-types!',
+		'git-blame!',
+		'git-chief-penguins!',
+		'git-min-signatures=i',
+		'git-max-maintainers=i',
+		'git-min-percent=i',
+		'git-since=s',
+		'hg-since=s',
+		'remove-duplicates!',
+		'maintainer|m!',
+		'names|n!',
+		'list|l!',
+		'subscribers|s!',
+		'multiline!',
+		'roles!',
+		'rolestats!',
+		'separator=s',
+		'subsystem!',
+		'status!',
+		'scm!',
+		'web!',
+		'pattern-depth=i',
+		'keywords|k!',
+		'sections!',
+		'file-emails|fe!',
+		'file|f',
+		'version|v',
+		'help|h|usage',
 		)) {
     die "$P: invalid argument - use --help if necessary\n";
 }
@@ -545,10 +569,10 @@ MAINTAINER field selection options:
     --git-blame => use git blame to find modified commits for patch or file
     --git-since => git history to use (default: $email_git_since)
     --hg-since => hg history to use (default: $email_hg_since)
-    --m => include maintainer(s) if any
-    --n => include name 'Full Name <addr\@domain.tld>'
-    --l => include list(s) if any
-    --s => include subscriber only list(s) if any
+    --maintainer | --m  => include maintainer(s) if any
+    --names | --n => include name 'Full Name <addr\@domain.tld>'
+    --list | --l  => include list(s) if any
+    --subscribers | --s => include subscriber only list(s) if any
     --remove-duplicates => minimize duplicate email names/addresses
     --roles => show roles (status:subsystem, git-signer, list, etc...)
     --rolestats => show roles and statistics (commits/total_commits, %)
@@ -582,26 +606,45 @@ Notes:
           no individual file within the directory or subdirectory
           is matched.
       Used with "--git-blame", does not iterate all files in directory
+  
   Using "--git-blame" is slow and may add old committers and authors
       that are no longer active maintainers to the output.
+  
   Using "--roles" or "--rolestats" with git send-email --cc-cmd or any
       other automated tools that expect only ["name"] <email address>
       may not work because of additional output after <email address>.
+  
   Using "--rolestats" and "--git-blame" shows the #/total=% commits,
       not the percentage of the entire file authored.  # of commits is
       not a good measure of amount of code authored.  1 major commit may
       contain a thousand lines, 5 trivial commits may modify a single line.
+  
   If git is not installed, but mercurial (hg) is installed and an .hg
       repository exists, the following options apply to mercurial:
           --git,
           --git-min-signatures, --git-max-maintainers, --git-min-percent, and
           --git-blame
       Use --hg-since not --git-since to control date selection
-  File ".get_maintainer.conf", if it exists in the linux kernel source root
+
+  The file ".get_maintainer.conf", if it exists in the linux kernel source root
       directory, can change whatever get_maintainer defaults are desired.
-      Entries in this file can be any command line argument.
-      This file is prepended to any additional command line arguments.
-      Multiple lines and # comments are allowed.
+      Entries in this file can be any command line argument without the
+      preceding "--" followed by an equal sign ('=') and a 0 for disabled
+      or a 1 for enabled. 
+      This makes it easy to use it with non-default options via 
+      'git send-email --cc-cmd'.
+
+      An example file would look like this:
+      -------------------------------------------------------------------------
+      
+      ### comments and blank lines are allowed. ###
+      git-all-signature-types=1
+      lists=0 # don't include mailinglists
+      names=0 # no names
+
+      -------------------------------------------------------------------------
+
+  
 EOT
 }
 
-- 
1.7.0.4

Re: [PATCH 2/2] scripts/get_maintainer.pl: add .get_maintainer.conf default options file

[Américo Wang]

On Wed, May 12, 2010 at 11:25:49AM +0200, Florian Mickler wrote:
>On Wed, 12 May 2010 14:30:42 +0800
>Américo Wang <xiyou.wangcong@gmail.com> wrote:
>
>> On Sun, May 09, 2010 at 09:56:03PM -0700, Joe Perches wrote:
>> >Allow the use of a .get_maintainer.conf file to control the
>> >default options applied when scripts/get_maintainer.pl is run.
>> >
>> >.get_maintainer.conf can contain any valid command-line argument.
>> >
>> >File contents are prepended to any additional command line arguments.
>> >
>> >Multiple lines may be used, blank lines ignored, # is a comment.
>> 
>> Do we really have to do this? This looks odd to me.
>
>it's a little hackish, but the code impact is small. if you prefer
>standard config-file syntax (i.e. "bla=" ) check the patch below. 
>

Better somwhat, but...

>joe, what do you think?
>
>> If the user of get_maintainer.pl uses some long command line
>> arguments, to save his input, he should use bash aliases, not
>> configure files. I never see configure files used like this.
>
>bash aliases don't work (at least here) with "git send-email --cc-cmd="
>
...
>> 
>> Also, it doesn't worthy a configure file for such a script like
>> get_maintainer.pl.
>
>hm.. the only other way to use it via git send-email would be a wrapper
>script? 
>

I still think we should avoid config files as possible as we can.

Thanks.

Re: [PATCH 2/2] scripts/get_maintainer.pl: add .get_maintainer.conf default options file

[Joe Perches]

On Thu, 2010-05-13 at 23:58 +0800, Américo Wang wrote:
> >hm.. the only other way to use it via git send-email would be a wrapper
> >script? 
> I still think we should avoid config files as possible as we can.

I had a request for a config capability awhile ago.

It wasn't then or is now a big deal to me.

If it's picked up by Andrew at all, I prefer the
simpler style I posted as the code is smaller and
the arguments are just the same as the command line
so all of aliases work in the .conf file as well.

I do understand the desire for a Windows style .ini
though.  If it's an .ini I think it should use:

[sections]
key=value

Maybe that only if checkpatch or some other script
uses a config as well.

[PATCH] scripts/get_maintainer.pl: add .get_maintainer.conf default options file

[Joe Perches]

Allow the use of a .get_maintainer.conf file to control the
default options applied when scripts/get_maintainer.pl is run.

.get_maintainer.conf entries can be any valid argument.

Multiple lines may be used, blank lines ignored, # is a comment

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/get_maintainer.pl |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 6f97a13..0c4c7e0 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -97,6 +97,30 @@ my %VCS_cmds_hg = (
     "blame_commit_pattern" => "^([0-9a-f]+):"
 );
 
+if (-f ${lk_path}.get_maintainer.conf) {
+    my @conf_args;
+    open(my $conffile, '<', "${lk_path}.get_maintainer.conf")
+	or warn "$P: Can't open .get_maintainer.conf: $!\n";
+    while (<$conffile>) {
+	my $line = $_;
+
+	$line =~ s/\s*\n?$//g;
+	$line =~ s/^\s*//g;
+	$line =~ s/\s+/ /g;
+
+	next if ($line =~ m/^\s*#/);
+	next if ($line =~ m/^\s*$/);
+
+	my @words = split(" ", $line);
+	foreach my $word (@words) {
+	    last if ($word =~ m/^#/);
+	    push (@conf_args, $word);
+	}
+    }
+    close($conffile);
+    unshift(@ARGV, @conf_args) if @conf_args;
+}
+
 if (!GetOptions(
 		'email!' => \$email,
 		'git!' => \$email_git,

Re: [PATCH] scripts/get_maintainer.pl: add .get_maintainer.conf default options file

[Florian Mickler]

On Sat, 08 May 2010 15:26:49 -0700
Joe Perches <joe@perches.com> wrote:

> Allow the use of a .get_maintainer.conf file to control the
> default options applied when scripts/get_maintainer.pl is run.
> 
> .get_maintainer.conf entries can be any valid argument.
> 
> Multiple lines may be used, blank lines ignored, # is a comment
> 
> Signed-off-by: Joe Perches <joe@perches.com>
 
probably worth noting, that an entry is of the form 

"--cmdlinearg" 

as that's unexpected for a config file.

> ---
>  scripts/get_maintainer.pl |   24 ++++++++++++++++++++++++
>  1 files changed, 24 insertions(+), 0 deletions(-)
> 
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index 6f97a13..0c4c7e0 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -97,6 +97,30 @@ my %VCS_cmds_hg = (
>      "blame_commit_pattern" => "^([0-9a-f]+):"
>  );
>  
> +if (-f ${lk_path}.get_maintainer.conf) {

missing "" around the argument to -f ...

i wouldn't put the trailing slash in the variables but add it when composing path's.
but it seems this is the convention for get_maintainer.pl... 


 

> +    my @conf_args;
> +    open(my $conffile, '<', "${lk_path}.get_maintainer.conf")
> +	or warn "$P: Can't open .get_maintainer.conf: $!\n";
> +    while (<$conffile>) {
> +	my $line = $_;
> +
> +	$line =~ s/\s*\n?$//g;
> +	$line =~ s/^\s*//g;
> +	$line =~ s/\s+/ /g;
> +
> +	next if ($line =~ m/^\s*#/);
> +	next if ($line =~ m/^\s*$/);
> +
> +	my @words = split(" ", $line);
> +	foreach my $word (@words) {
> +	    last if ($word =~ m/^#/);
> +	    push (@conf_args, $word);
> +	}
> +    }
> +    close($conffile);
> +    unshift(@ARGV, @conf_args) if @conf_args;
> +}

this works.

> +
>  if (!GetOptions(
>  		'email!' => \$email,
>  		'git!' => \$email_git,
> 

Reviewed-by:Florian Mickler <florian@mickler.org>

Re: indiscriminate get_maintainer.pl usage

[Roland Dreier]

 > $ scripts/get_maintainer.pl --rolestats -f fs/eventpoll.c
 > Alexander Viro <viro@zeniv.linux.org.uk> (maintainer:FILESYSTEMS (VFS...)
 > Andrew Morton <akpm@linux-foundation.org> (commit_signer:2/6=33%)
 > "Eric W. Biederman" <ebiederm@xmission.com> (commit_signer:2/6=33%)
 > Davide Libenzi <davidel@xmailserver.org> (commit_signer:2/6=33%)
 > Roland Dreier <rolandd@cisco.com> (commit_signer:1/6=17%)
 > Stefan Richter <stefanr@s5r6.in-berlin.de> (commit_signer:1/6=17%)
 > linux-fsdevel@vger.kernel.org (open list:FILESYSTEMS (VFS...)
 > linux-kernel@vger.kernel.org (open list)

And my appearing here is a bit silly as well, since I have made one
commit to fs/eventpoll.c that changed one line.  A bit different in
scope from Davide who designed and wrote the whole thing...

OTOH it's not clear how smart we want get_maintainer.pl to try to be.

 - R.
-- 
Roland Dreier <rolandd@cisco.com> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

Re: [PATCH] epoll: use wrapper functions

[Davide Libenzi]

On Thu, 6 May 2010, Changli Gao wrote:

> use wrapper functions.
> 
> epoll should not touch flags in wait_queue_t. This patch introduces a new
> function add_wait_queue_head_exclusive_locked(), for the users, who use
> wait queue as a LIFO queue.

Since we already have __add_wait_queue(), __add_wait_queue_tail() and 
__remove_wait_queue() (which all means "locked"), and while I agree in 
having the exclusive-add wrapped into a function, I much better prefer a:

static inline void __add_wait_queue_excl(wait_queue_head_t *head, 
					 wait_queue_t *new)
{
	new->flags |= WQ_FLAG_EXCLUSIVE;
	__add_wait_queue(head, new);
}

The patch you posted introduces a different naming, which leaves all the 
other __*() untouched, and wraps the already one-liner __remove_wait_queue() 
with yet another one-liner.





> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ----
>  fs/eventpoll.c       |    5 ++---
>  include/linux/wait.h |   15 +++++++++++++--
>  2 files changed, 15 insertions(+), 5 deletions(-)
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index bd056a5..8137f6e 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -1140,8 +1140,7 @@ retry:
>  		 * ep_poll_callback() when events will become available.
>  		 */
>  		init_waitqueue_entry(&wait, current);
> -		wait.flags |= WQ_FLAG_EXCLUSIVE;
> -		__add_wait_queue(&ep->wq, &wait);
> +		add_wait_queue_head_exclusive_locked(&ep->wq, &wait);
>  
>  		for (;;) {
>  			/*
> @@ -1161,7 +1160,7 @@ retry:
>  			jtimeout = schedule_timeout(jtimeout);
>  			spin_lock_irqsave(&ep->lock, flags);
>  		}
> -		__remove_wait_queue(&ep->wq, &wait);
> +		remove_wait_queue_locked(&ep->wq, &wait);
>  
>  		set_current_state(TASK_RUNNING);
>  	}
> diff --git a/include/linux/wait.h b/include/linux/wait.h
> index a48e16b..de2566d 100644
> --- a/include/linux/wait.h
> +++ b/include/linux/wait.h
> @@ -407,17 +407,28 @@ do {									\
>   * Must be called with the spinlock in the wait_queue_head_t held.
>   */
>  static inline void add_wait_queue_exclusive_locked(wait_queue_head_t *q,
> -						   wait_queue_t * wait)
> +						   wait_queue_t *wait)
>  {
>  	wait->flags |= WQ_FLAG_EXCLUSIVE;
>  	__add_wait_queue_tail(q,  wait);
>  }
>  
>  /*
> + * Must be called with the spinlock in the wait_queue_head_t held, and
> + * q must be for exclusive wait only.
> + */
> +static inline void add_wait_queue_head_exclusive_locked(wait_queue_head_t *q,
> +							wait_queue_t *wait)
> +{
> +	wait->flags |= WQ_FLAG_EXCLUSIVE;
> +	__add_wait_queue(q,  wait);
> +}
> +
> +/*
>   * Must be called with the spinlock in the wait_queue_head_t held.
>   */
>  static inline void remove_wait_queue_locked(wait_queue_head_t *q,
> -					    wait_queue_t * wait)
> +					    wait_queue_t *wait)
>  {
>  	__remove_wait_queue(q,  wait);
>  }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


- Davide

Re: [PATCH] epoll: use wrapper functions

[Peter Zijlstra]

On Thu, 2010-05-06 at 11:47 -0700, Davide Libenzi wrote:
> Since we already have __add_wait_queue(), __add_wait_queue_tail() and 
> __remove_wait_queue() (which all means "locked"), and while I agree in 
> having the exclusive-add wrapped into a function, I much better prefer a:
> 
> static inline void __add_wait_queue_excl(wait_queue_head_t *head, 
>                                          wait_queue_t *new)
> {
>         new->flags |= WQ_FLAG_EXCLUSIVE;
>         __add_wait_queue(head, new);
> }
> 
> The patch you posted introduces a different naming, which leaves all the 
> other __*() untouched, and wraps the already one-liner __remove_wait_queue() 
> with yet another one-liner. 

I concur, I always get confused by the _locked postfix (and its more
typing). Also, it goes against the lock data not code paradigm.

Re: [PATCH] epoll: use wrapper functions

[Changli Gao]

On Fri, May 7, 2010 at 2:51 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, 2010-05-06 at 11:47 -0700, Davide Libenzi wrote:
>> Since we already have __add_wait_queue(), __add_wait_queue_tail() and
>> __remove_wait_queue() (which all means "locked"), and while I agree in
>> having the exclusive-add wrapped into a function, I much better prefer a:
>>
>> static inline void __add_wait_queue_excl(wait_queue_head_t *head,
>>                                          wait_queue_t *new)
>> {
>>         new->flags |= WQ_FLAG_EXCLUSIVE;
>>         __add_wait_queue(head, new);
>> }
>>
>> The patch you posted introduces a different naming, which leaves all the
>> other __*() untouched, and wraps the already one-liner __remove_wait_queue()
>> with yet another one-liner.
>
> I concur, I always get confused by the _locked postfix (and its more
> typing). Also, it goes against the lock data not code paradigm.
>
>

I greped all the code, and found that
add_wait_queue_head_exclusive_locked() and remove_wait_queue_locked()
aren't used. It seems that no users like these APIs. So I will remove
these two APIs, and add __add_wait_queue_excl() instead. Thanks.


-- 
Regards，
Changli Gao(xiaosuo@gmail.com)