Okay, let's look at this part.
Felipe Contreras wrote:
> diff --git a/contrib/related/git-related b/contrib/related/git-related
> new file mode 100755
> index 0000000..4f31482
> --- /dev/null
> +++ b/contrib/related/git-related
> @@ -0,0 +1,124 @@
> +#!/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
> +
> +def fmt_person(name, email)
> + name ? '%s <%s>' % [name, email] : email
> +end
> +
> +class Commit
> +
> + attr_reader :persons
> +
> + def initialize(id)
> + @id = id
> + @persons = []
> + end
Okay, although I'm wondering what id is.
> + def parse(data)
> + msg = nil
> + data.each_line do |line|
> + if not msg
> + case line
> + when /^author ([^<>]+) <(\S+)> (.+)$/
> + @persons << fmt_person($1, $2)
> + when /^$/
> + msg = true
> + end
When there's a blank line, flip the switch and start looking in the
commit message. Why though? You're worried that the commit message
will contain a line matching /^author ([^<>]+) <(\S+)> (.+)$/? If so,
why don't you split('\n', 2), look for the author in the $1 and
Whatevered-by in $2?
> + else
> + if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^<>]+) <(\S+?)>$/
elif? Can this be more generic to include Whatevered-by?
> + @persons << fmt_person($2, $3)
Will $2 ever be nil (from fmt_person)? ie. Why are you checking for
the special case " <\S+?>$"?
> + end
> + end
> + end
> + @persons.uniq!
So you don't care if the person has appeared twice or thrice in the message.
> + end
> +
> +end
> +
> +class Commits
> +
> + def initialize
> + @items = {}
> + end
Okay.
> + def size
> + @items.size
> + end
#size is reminiscent of Array#size and Hash#size.
> + def each(&block)
> + @items.each(&block)
> + end
I can see how you iterate over commits from the code below. However,
using #each like this is confusing, because the reader expects #each
to be invoked on an Enumerable. So, I have two suggestions:
1. Use block_given? to make sure that #each works even without a block
(just like Enumerator#each).
2. Mixin Enumerable by putting in an 'include Enumerable' after the
class line. Aside from clarity, you get stuff like #find for free.
> + def import
>From reading the code below, your calling semantics are: first set
each @items[id] to a new Commit object. import is meant to invoke
Commit#parse and set @persons there. Okay.
> + return if @items.empty?
> + File.popen(%w[git cat-file --batch], 'r+') do |p|
> + p.write(@items.keys.join("\n"))
> + p.close_write
Okay.
> + p.each do |l|
> + if l =~ /^(\h{40}) commit (\d+)/
s/l/line/?
> + 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
Please don't use ||=. It is notorious for causing confusion in
Ruby-land. Hint: it's not exactly equivalent to either len = len || 1
or len || len = 1.
> + File.popen(['git', 'blame', '--incremental', '-C',
Still no -CCC?
> + '-L', '%u,+%u' % [start, len],
> + '--since', $since, from + '^',
> + '--', source]) do |p|
> + p.each do |line|
> + if line =~ /^(\h{40})/
> + id = $1
Use $0 and remove the parens: you're matching the whole line.
> + @items[id] = Commit.new(id)
Okay.
> + 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
Okay.
> + when /^---\s+(\S+)/
> + source = $1 != '/dev/null' ? $1[2..-1] : nil
Okay.
> + when /^@@ -(\d+)(?:,(\d+))?/
> + get_blame(source, $1, $2, from)
Okay.
> + end
> + end
> + end
> + end
> +
> +end
> +
> +exit 1 if ARGV.size != 1
Okay.
> +commits = Commits.new
> +commits.from_patch(ARGV[0])
> +commits.import
The calling semantics could be better, but it's not a big complaint.
> +count_per_person = Hash.new(0)
Initializing all keys to 0. Okay.
> +commits.each do |id, commit|
Cute.
> + commit.persons.each do |person|
> + count_per_person[person] += 1
Okay.
> + end
> +end
> +
> +count_per_person.each do |person, count|
> + percent = count.to_f * 100 / commits.size
> + next if percent < $min_percent
> + puts person
Not going to print percentage as well?
Overall, significant improvement over v3 which used all kinds of
unnecessarily complex data structures and convoluted logic. Looks
like something I'd want to use: not blindly as a cc-cmd, but just to
get a quick idea. I also wish for depth most times: a working
shortlog -L --follow would be really nice.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html