This script find people that might be interested in a patch, by going
back through the history for each single hunk modified, and finding
people that reviewed, acknowledge, signed, or authored the code the
patch is modifying.
It does this by running 'git blame' incrementally on each hunk, and then
parsing the commit message. After gathering all the relevant people, it
groups them to show what exactly was their role when the participated in
the development of the relevant commit, and on how many relevant commits
they participated. They are only displayed if they pass a minimum
threshold of participation.
For example:
% git related 0001-remote-hg-trivial-cleanups.patch
Felipe Contreras [off-list ref]
Jeff King [off-list ref]
Max Horn [off-list ref]
Junio C Hamano [off-list ref]
Thus it can be used for 'git send-email' as a cc-cmd.
There might be some other related functions to this script, not just to
be used as a cc-cmd.
Comments-by: Ramkumar Ramachandra [off-list ref]
Signed-off-by: Felipe Contreras <redacted>
---
Small changes since v6:
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -6,10 +6,6 @@
$since = '5-years-ago'
$min_percent = 10
-def fmt_person(name, email)
- '%s <%s>' % [name, email]
-end
-
class Commit
attr_reader :persons
@@ -25,13 +21,13 @@ class Commit
if not msg
case line
when /^author ([^<>]+) <(\S+)> (.+)$/
- @persons << fmt_person($1, $2)
+ @persons << '%s <%s>' % [$1, $2]
when /^$/
msg = true
end
else
if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^<>]+) <(\S+?)>$/
- @persons << fmt_person($2, $3)
+ @persons << '%s <%s>' % [$2, $3]
end
end
end@@ -72,12 +68,12 @@ class Commits
def get_blame(source, start, len, from)
return if len == 0
len ||= 1
- File.popen(['git', 'blame', '--incremental', '-CCC',
+ File.popen(['git', 'blame', '--incremental', '-C', '-C',
'-L', '%u,+%u' % [start, len],
'--since', $since, from + '^',
'--', source]) do |p|
p.each do |line|
- if line =~ /^(\h{40})/
+ if line =~ /^\h{40}/
id = $&
@items[id] = Commit.new(id)
end@@ -95,7 +91,7 @@ class Commits
when /^---\s+(\S+)/
source = $1 != '/dev/null' ? $1[2..-1] : nil
when /^@@ -(\d+)(?:,(\d+))?/
- get_blame(source, $1, $2, from)
+ get_blame(source, $1, $2, from) if source and from
end
end
end
contrib/related/git-related | 120 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 120 insertions(+)
create mode 100755 contrib/related/git-related
diff --git a/contrib/related/git-related b/contrib/related/git-related
new file mode 100755
index 0000000..1b9b1e7
--- /dev/null
+++ b/contrib/related/git-related
@@ -0,0 +1,120 @@
+#!/usr/bin/env ruby
+
+# This script finds people that might be interested in a patch
+# usage: git related <file>
+
+$since = '5-years-ago'
+$min_percent = 10
+
+class Commit
+
+ attr_reader :persons
+
+ def initialize(id)
+ @id = id
+ @persons = []
+ end
+
+ def parse(data)
+ msg = nil
+ data.each_line do |line|
+ if not msg
+ case line
+ when /^author ([^<>]+) <(\S+)> (.+)$/
+ @persons << '%s <%s>' % [$1, $2]
+ when /^$/
+ msg = true
+ end
+ else
+ if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^<>]+) <(\S+?)>$/
+ @persons << '%s <%s>' % [$2, $3]
+ end
+ end
+ end
+ @persons.uniq!
+ end
+
+end
+
+class Commits
+
+ def initialize
+ @items = {}
+ end
+
+ def size
+ @items.size
+ end
+
+ def each(&block)
+ @items.each(&block)
+ end
+
+ def import
+ return if @items.empty?
+ File.popen(%w[git cat-file --batch], 'r+') do |p|
+ p.write(@items.keys.join("\n"))
+ p.close_write
+ p.each do |line|
+ if line =~ /^(\h{40}) commit (\d+)/
+ id, len = $1, $2
+ data = p.read($2.to_i)
+ @items[id].parse(data)
+ end
+ end
+ end
+ end
+
+ def get_blame(source, start, len, from)
+ return if len == 0
+ len ||= 1
+ File.popen(['git', 'blame', '--incremental', '-C', '-C',
+ '-L', '%u,+%u' % [start, len],
+ '--since', $since, from + '^',
+ '--', source]) do |p|
+ p.each do |line|
+ if line =~ /^\h{40}/
+ id = $&
+ @items[id] = Commit.new(id)
+ end
+ end
+ end
+ end
+
+ def from_patch(file)
+ from = source = nil
+ File.open(file) do |f|
+ f.each do |line|
+ case line
+ when /^From (\h+) (.+)$/
+ from = $1
+ when /^---\s+(\S+)/
+ source = $1 != '/dev/null' ? $1[2..-1] : nil
+ when /^@@ -(\d+)(?:,(\d+))?/
+ get_blame(source, $1, $2, from) if source and from
+ end
+ end
+ end
+ end
+
+end
+
+exit 1 if ARGV.size != 1
+
+commits = Commits.new
+commits.from_patch(ARGV[0])
+commits.import
+
+count_per_person = Hash.new(0)
+
+commits.each do |id, commit|
+ commit.persons.each do |person|
+ count_per_person[person] += 1
+ end
+end
+
+count_per_person.each do |person, count|
+ percent = count.to_f * 100 / commits.size
+ next if percent < $min_percent
+ puts person
+end--
1.8.3.rc3.312.g47657de
Let's do one more review.
Felipe Contreras wrote:
quoted hunk
diff --git a/contrib/related/git-related b/contrib/related/git-related
new file mode 100755
index 0000000..1b9b1e7
--- /dev/null
+++ b/contrib/related/git-related
@@ -0,0 +1,120 @@
+#!/usr/bin/env ruby
+
+# This script finds people that might be interested in a patch
+# usage: git related <file>
+
+$since = '5-years-ago'
+$min_percent = 10
+
+class Commit
+
+ attr_reader :persons
Unless you plan to introduce many more fields (I haven't looked at the
later patches), you might as well implement an #each, like in Commits.
+ def initialize(id)
+ @id = id
+ @persons = []
+ end
+
+ def parse(data)
+ msg = nil
msg = false, to indicate that it is a boolean.
+ data.each_line do |line|
+ if not msg
+ case line
+ when /^author ([^<>]+) <(\S+)> (.+)$/
+ @persons << '%s <%s>' % [$1, $2]
Why capture the third group when $3 is unused?
+ when /^$/
+ msg = true
+ end
+ else
+ if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^<>]+) <(\S+?)>$/
+ @persons << '%s <%s>' % [$2, $3]
Why capture the first group when $1 is unused?
+ end
+ end
+ end
+ @persons.uniq!
+ end
+
+end
+
+class Commits
+
+ def initialize
+ @items = {}
+ end
+
+ def size
+ @items.size
+ end
+
+ def each(&block)
+ @items.each(&block)
+ end
+
+ def import
+ return if @items.empty?
+ File.popen(%w[git cat-file --batch], 'r+') do |p|
Don't you need rb+ to suppress the CRLF nonsense on Windows?
+ p.write(@items.keys.join("\n"))
As you might have realized, the parentheses are optional everywhere
(except when it is required for disambiguation). I'm merely pointing
it out here, because this line looks especially ugly.
+ p.close_write
+ p.each do |line|
+ if line =~ /^(\h{40}) commit (\d+)/
+ id, len = $1, $2
id, len = $1, Integer $2. And drop the .to_i on the next line.
+ data = p.read($2.to_i)
+ @items[id].parse(data)
+ end
+ end
+ end
+ end
+
+ def get_blame(source, start, len, from)
+ return if len == 0
+ len ||= 1
I asked you to use 'len =1 if not len' for clarity, but you didn't like it.
+ File.popen(['git', 'blame', '--incremental', '-C', '-C',
+ '-L', '%u,+%u' % [start, len],
+ '--since', $since, from + '^',
+ '--', source]) do |p|
+ p.each do |line|
+ if line =~ /^\h{40}/
+ id = $&
+ @items[id] = Commit.new(id)
+ end
+ end
+ end
+ end
+
+ def from_patch(file)
+ from = source = nil
+ File.open(file) do |f|
+ f.each do |line|
File.readlines(file).each do |line|.
+ case line
+ when /^From (\h+) (.+)$/
+ from = $1
Useless capture.
+ when /^---\s+(\S+)/
+ source = $1 != '/dev/null' ? $1[2..-1] : nil
+ when /^@@ -(\d+)(?:,(\d+))?/
+ get_blame(source, $1, $2, from) if source and from
Useless capture. When is len ($2) going to be nil?
+ end
+ end
+ end
+ end
+
+end
+
+exit 1 if ARGV.size != 1
+
+commits = Commits.new
+commits.from_patch(ARGV[0])
+commits.import
+
+count_per_person = Hash.new(0)
+
+commits.each do |id, commit|
commits.each do |_, commit|, since you're not using id.
+ commit.persons.each do |person|
+ count_per_person[person] += 1
+ end
+end
+
+count_per_person.each do |person, count|
+ percent = count.to_f * 100 / commits.size
I prefer 'Float count' over count.to_f, but that's just a matter of taste.
+ next if percent < $min_percent
+ puts person
+end
--
1.8.3.rc3.312.g47657de
On Thu, May 30, 2013 at 4:01 AM, Ramkumar Ramachandra
[off-list ref] wrote:
Let's do one more review.
Felipe Contreras wrote:
quoted
diff --git a/contrib/related/git-related b/contrib/related/git-related
new file mode 100755
index 0000000..1b9b1e7
--- /dev/null
+++ b/contrib/related/git-related
@@ -0,0 +1,120 @@
+#!/usr/bin/env ruby
+
+# This script finds people that might be interested in a patch
+# usage: git related <file>
+
+$since = '5-years-ago'
+$min_percent = 10
+
+class Commit
+
+ attr_reader :persons
Unless you plan to introduce many more fields (I haven't looked at the
later patches), you might as well implement an #each, like in Commits.
commit.each doesn't make sense; each what?
We could do 'commit.each_person', but why is that so better than
commit.persons.each? It's not.
quoted
+ data.each_line do |line|
+ if not msg
+ case line
+ when /^author ([^<>]+) <(\S+)> (.+)$/
+ @persons << '%s <%s>' % [$1, $2]
Why capture the third group when $3 is unused?
Completeness.
quoted
+ when /^$/
+ msg = true
+ end
+ else
+ if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^<>]+) <(\S+?)>$/
+ @persons << '%s <%s>' % [$2, $3]
Why capture the first group when $1 is unused?
You want to complicate the regex even more with:
/^(?:Signed-off|Reviewed|Acked)-by: ([^<>]+) <(\S+?)>$/
For what purpose?
quoted
+ end
+ end
+ end
+ @persons.uniq!
+ end
+
+end
+
+class Commits
+
+ def initialize
+ @items = {}
+ end
+
+ def size
+ @items.size
+ end
+
+ def each(&block)
+ @items.each(&block)
+ end
+
+ def import
+ return if @items.empty?
+ File.popen(%w[git cat-file --batch], 'r+') do |p|
Don't you need rb+ to suppress the CRLF nonsense on Windows?
Who knows.
quoted
+ p.write(@items.keys.join("\n"))
As you might have realized, the parentheses are optional everywhere
(except when it is required for disambiguation). I'm merely pointing
it out here, because this line looks especially ugly.
I suspect most Git developers would prefer the traditional function call style.
quoted
+ p.close_write
+ p.each do |line|
+ if line =~ /^(\h{40}) commit (\d+)/
+ id, len = $1, $2
id, len = $1, Integer $2. And drop the .to_i on the next line.
This is way is better.
quoted
+ data = p.read($2.to_i)
+ @items[id].parse(data)
+ end
+ end
+ end
+ end
+
+ def get_blame(source, start, len, from)
+ return if len == 0
+ len ||= 1
I asked you to use 'len =1 if not len' for clarity, but you didn't like it.
git grep "||=" disagrees.
quoted
+ File.popen(['git', 'blame', '--incremental', '-C', '-C',
+ '-L', '%u,+%u' % [start, len],
+ '--since', $since, from + '^',
+ '--', source]) do |p|
+ p.each do |line|
+ if line =~ /^\h{40}/
+ id = $&
+ @items[id] = Commit.new(id)
+ end
+ end
+ end
+ end
+
+ def from_patch(file)
+ from = source = nil
+ File.open(file) do |f|
+ f.each do |line|
File.readlines(file).each do |line|.
That's less efficient.
quoted
+ when /^---\s+(\S+)/
+ source = $1 != '/dev/null' ? $1[2..-1] : nil
+ when /^@@ -(\d+)(?:,(\d+))?/
+ get_blame(source, $1, $2, from) if source and from
Useless capture. When is len ($2) going to be nil?
Junio already went over it, see the diff format.
What's your objective? Block this patch from ever going in?
Not a single one of these comments makes a difference at all, all of
them can wait until after the patch is merged, many of them are a
matter of preferences, and some of them have already been addressed as
precisely that: disagreements in style.
--
Felipe Contreras
Felipe Contreras wrote:
What's your objective? Block this patch from ever going in?
Not a single one of these comments makes a difference at all, all of
them can wait until after the patch is merged, many of them are a
matter of preferences, and some of them have already been addressed as
precisely that: disagreements in style.
You posted a patch, and I reviewed it. End of story. I never
explicitly or implicitly indicated that I want to block the patch, so
stop pulling stuff out of your arse.
If you don't want a review, write "DO NOT REVIEW" (or better yet,
don't hit my inbox). I'm not interested in wasting my time either.
On Thu, May 30, 2013 at 6:31 AM, Felipe Contreras
[off-list ref] wrote:
On Thu, May 30, 2013 at 4:01 AM, Ramkumar Ramachandra
Not a single one of these comments makes a difference at all, all of
them can wait until after the patch is merged, many of them are a
matter of preferences, and some of them have already been addressed as
precisely that: disagreements in style.
I was going to make these stylistic changes to make you happy, but
then I realized the only that does really make sense is to change msg
= nil to msg = false, and it's not even worth to waste a thought on
changes like that.
So I'll move on to the next patches, hopefully Duy or Junio would have
some comments of actual significance, or maybe you would too, but for
the moment it seems pretty clear you are only stating opinions about
what Ruby code-style you like best.
Cheers.
--
Felipe Contreras
Felipe Contreras wrote:
I was going to make these stylistic changes to make you happy, but
then I realized the only that does really make sense is to change msg
= nil to msg = false, and it's not even worth to waste a thought on
changes like that.
We don't have existing Ruby code in git.git to follow, so what I say
can obviously not have more weightage than "personal opinion". Don't
do things to "make me happy"; I am nobody. Have a good sense of style
and defend it instead of flaming me because you thought I was stalling
work.
"Some of these style changes seem to make sense, and I'll fix them
after this patch gets merged."
or
"I have a different opinion on these issues, and here's why: "
would have both been appropriate responses.
So I'll move on to the next patches, hopefully Duy or Junio would have
some comments of actual significance, or maybe you would too, but for
the moment it seems pretty clear you are only stating opinions about
what Ruby code-style you like best.
I've done reviews of several iterations of this patch, and stylistic
comments were all that I had left: I might not be adequately
intelligent to come up with "comments of actual significance", but
that's a limitation you're going to have to deal with.
On Fri, May 31, 2013 at 3:03 AM, Ramkumar Ramachandra
[off-list ref] wrote:
Felipe Contreras wrote:
quoted
I was going to make these stylistic changes to make you happy, but
then I realized the only that does really make sense is to change msg
= nil to msg = false, and it's not even worth to waste a thought on
changes like that.
We don't have existing Ruby code in git.git to follow, so what I say
can obviously not have more weightage than "personal opinion". Don't
do things to "make me happy"; I am nobody. Have a good sense of style
and defend it instead of flaming me because you thought I was stalling
work.
I already defended the style, only to hear the same comment again in
the next iteration.
quoted
So I'll move on to the next patches, hopefully Duy or Junio would have
some comments of actual significance, or maybe you would too, but for
the moment it seems pretty clear you are only stating opinions about
what Ruby code-style you like best.
I've done reviews of several iterations of this patch, and stylistic
comments were all that I had left:
When a reviewer reaches that point, (s)he usually says: other than
cosmetic preferences: Reviewed-by: me.
--
Felipe Contreras