[PATCH v7] Add new git-related helper to contrib

[PATCH v7] Add new git-related helper to contrib

[Felipe Contreras]

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 <felipe.contreras@gmail.com>
  Jeff King <peff@peff.net>
  Max Horn <max@quendi.de>
  Junio C Hamano <gitster@pobox.com>

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 <artagnon@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---

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

Re: [PATCH v7] Add new git-related helper to contrib

[Ramkumar Ramachandra]

Let's do one more review.

Felipe Contreras wrote:
> 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

Re: [PATCH v7] Add new git-related helper to contrib

[Felipe Contreras]

On Thu, May 30, 2013 at 4:01 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Let's do one more review.
>
> Felipe Contreras wrote:
>> 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.

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

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

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

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

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

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

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

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

Re: [PATCH v7] Add new git-related helper to contrib

[Ramkumar Ramachandra]

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.

Re: [PATCH v7] Add new git-related helper to contrib

[Felipe Contreras]

On Thu, May 30, 2013 at 7:08 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> 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.

That's exactly what you are doing.

Do you see any other reason for not merging this if not your comments?

-- 
Felipe Contreras

Re: [PATCH v7] Add new git-related helper to contrib

[Felipe Contreras]

On Thu, May 30, 2013 at 6:31 AM, Felipe Contreras
<felipe.contreras@gmail.com> 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

Re: [PATCH v7] Add new git-related helper to contrib

[Ramkumar Ramachandra]

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.

Re: [PATCH v7] Add new git-related helper to contrib

[Felipe Contreras]

On Fri, May 31, 2013 at 3:03 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> 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.

I already defended the style, only to hear the same comment again in
the next iteration.

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

Re: [PATCH v7] Add new git-related helper to contrib

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> When a reviewer reaches that point, (s)he usually says: other than
> cosmetic preferences: Reviewed-by: me.

Yes, you can have a Reviewed-by from me if you want.

And yes, it is appropriate not to CC me on v8 and v9, since I have no
further comments.