Re: [PATCH v5 01/15] Add new git-related helper to contrib

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: May 19, 2013, 14:40 UTC
Message-ID: <CALkWK0ke-Ns0JCt6LAzWqympWZ=2x3rRHOjnzB9QN7gc2=ckgw@mail.gmail.com>
In-Reply-To: <1368877615-9563-2-git-send-email-felipe.contreras@gmail.com>

Okay, let's look at this part.

Felipe Contreras wrote:

Show 26 quoted lines

> 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.

Show 10 quoted lines

> +  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.

Show 9 quoted lines

> +  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/?

Show 11 quoted lines

> +          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?

Show 6 quoted lines

> +               '-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.

Show 12 quoted lines

> +        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.

Show 8 quoted lines

> +        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.

Show 7 quoted lines

> +  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.