[PATCH v5 00/15] New git-related helper

[PATCH v5 00/15] New git-related helper

[Felipe Contreras]

Hi,

Here goes version 5. I decided to start with a very very minimal working
version that is only 124 lines of code, then slowly but steadily introduce all
the fancy features. I also tweaked the defaults so they give more meaninful
results (IMO).

I also fixed the parsing of diffs so that it tackles corner case Junio
mentioned.

This script allows you to get a list of relevant persons to Cc when sending a
patch series.

  % git related v1.8.1.6^^1..v1.8.1.6^^2
  Junio C Hamano <gitster@pobox.com> (signer: 92%, author: 7%)
  Nguyễn Thái Ngọc Duy <pclouds@gmail.com> (author: 38%)
  Michael Haggerty <mhagger@alum.mit.edu> (author: 15%)

It finds people that might be interesting in a patch, by going back through the
history for each single hunk modified, and finding people that reviewed,
acknowledged, 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.

The code finds the changes in each commit in the list, runs 'git blame'
to see which other commits are relevant to those lines, and then adds
the author and signer to the list.

Finally, it calculates what percentage of the total relevant commits
each person was involved in, and if it passes the threshold, it goes in.

You can also choose to show the commits themselves:

  % git related --commits v1.8.1.6^^1..v1.8.1.6^^2
  9db9eec attr: avoid calling find_basename() twice per path
  94bc671 Add directory pattern matching to attributes
  82dce99 attr: more matching optimizations from .gitignore
  593cb88 exclude: split basename matching code into a separate function
  b559263 exclude: split pathname matching code into a separate function
  4742d13 attr: avoid searching for basename on every match
  f950eb9 rename pathspec_prefix() to common_prefix() and move to dir.[ch]
  4a085b1 consolidate pathspec_prefix and common_prefix
  d932f4e Rename git_checkattr() to git_check_attr()
  2d72174 Extract a function collect_all_attrs()
  8cf2a84 Add string comparison functions that respect the ignore_case variable.
  2c5b011 dir.c: Fix two minor grammatical errors in comments
  377d9c4 Makefile: update the default build options for AIX

But wait, there's more: you can also specify a list of patch files, which means
this can be used for 'git send-emails' --cc-cmd option.

I don't know about you, but I've found this script incredibly useful.

Felipe Contreras (15):
  Add new git-related helper to contrib
  contrib: related: add option parsing
  contrib: related: sort by amount of involvement
  contrib: related: print the amount of involvement
  contrib: related: add helper Person classes
  contrib: related: show role count
  contrib: related: add support for more roles
  contrib: related: group persons with same email
  contrib: related: add mailmap support
  contrib: related: allow usage on other directories
  contrib: related: add support for multiple patches
  contrib: related: add option to show commits
  contrib: related: add option to parse from committish
  contrib: related: parse committish like format-patch
  contrib: related: fix parsing of rev-list args

 contrib/related/git-related | 312 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 312 insertions(+)
 create mode 100755 contrib/related/git-related

-- 
1.8.3.rc2.542.g24820ba

[PATCH v5 01/15] 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>
---
 contrib/related/git-related | 124 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 124 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..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
+
+  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
+      else
+        if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^<>]+) <(\S+?)>$/
+          @persons << fmt_person($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 |l|
+        if l =~ /^(\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',
+               '-L', '%u,+%u' % [start, len],
+               '--since', $since, from + '^',
+               '--', source]) do |p|
+      p.each do |line|
+        if line =~ /^(\h{40})/
+          id = $1
+          @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)
+        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.rc2.542.g24820ba

[PATCH v5 02/15] contrib: related: add option parsing

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/related/git-related | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index 4f31482..6f18cc8 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -3,6 +3,8 @@
 # This script finds people that might be interested in a patch
 # usage: git related <file>
 
+require 'optparse'
+
 $since = '5-years-ago'
 $min_percent = 10
 
@@ -10,6 +12,21 @@ def fmt_person(name, email)
   name ? '%s <%s>' % [name, email] : email
 end
 
+begin
+  OptionParser.new do |opts|
+    opts.program_name = 'git related'
+    opts.banner = 'usage: git related [options] <file>'
+
+    opts.on('-p', '--min-percent N', Integer, 'Minium percentage of role participation') do |v|
+      $min_percent = v
+    end
+    opts.on('-d', '--since DATE', 'How far back to search for relevant commits') do |v|
+      $since = v
+    end
+  end.parse!
+rescue OptionParser::InvalidOption
+end
+
 class Commit
 
   attr_reader :persons
-- 
1.8.3.rc2.542.g24820ba

[PATCH v5 03/15] contrib: related: sort by amount of involvement

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/related/git-related | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index 6f18cc8..8c26cf8 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -134,7 +134,12 @@ commits.each do |id, commit|
   end
 end
 
-count_per_person.each do |person, count|
+# sort by number of participations
+count_sort = lambda do |a, b|
+  b[1] <=> a[1]
+end
+
+count_per_person.sort(&count_sort).each do |person, count|
   percent = count.to_f * 100 / commits.size
   next if percent < $min_percent
   puts person
-- 
1.8.3.rc2.542.g24820ba

[PATCH v5 04/15] contrib: related: print the amount of involvement

[Felipe Contreras]

100% means the person was involved in all the commits, in one way or the
other.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/related/git-related | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index 8c26cf8..7be2829 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -142,5 +142,5 @@ end
 count_per_person.sort(&count_sort).each do |person, count|
   percent = count.to_f * 100 / commits.size
   next if percent < $min_percent
-  puts person
+  puts '%s (involved: %u%%)' % [person, percent]
 end
-- 
1.8.3.rc2.542.g24820ba

[PATCH v5 05/15] contrib: related: add helper Person classes

[Felipe Contreras]

No functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/related/git-related | 80 +++++++++++++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 24 deletions(-)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index 7be2829..df13148 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -8,10 +8,6 @@ require 'optparse'
 $since = '5-years-ago'
 $min_percent = 10
 
-def fmt_person(name, email)
-  name ? '%s <%s>' % [name, email] : email
-end
-
 begin
   OptionParser.new do |opts|
     opts.program_name = 'git related'
@@ -27,13 +23,59 @@ begin
 rescue OptionParser::InvalidOption
 end
 
-class Commit
+class Person
+
+  attr_reader :roles
+
+  def initialize(name, email)
+    @name = name
+    @email = email
+    @commits = {}
+  end
+
+  def add_role(commit)
+    @commits[commit] = true
+  end
+
+  def <=>(b)
+    self.size <=> b.size
+  end
+
+  def size
+    @commits.size
+  end
+
+  def to_s
+    @name ? '%s <%s>' % [@name, @email] : @email
+  end
+
+end
 
-  attr_reader :persons
+class Persons
+
+  @@index = {}
+
+  include Enumerable
+
+  def each(&block)
+    @@index.values.each(&block)
+  end
+
+  def self.get(name, email)
+    id = [name, email]
+    person = @@index[id]
+    if not person
+      person = @@index[id] = Person.new(name, email)
+    end
+    person
+  end
+
+end
+
+class Commit
 
   def initialize(id)
     @id = id
-    @persons = []
   end
 
   def parse(data)
@@ -42,17 +84,18 @@ class Commit
       if not msg
         case line
         when /^author ([^<>]+) <(\S+)> (.+)$/
-          @persons << fmt_person($1, $2)
+          author = Persons.get($1, $2)
+          author.add_role(@id)
         when /^$/
           msg = true
         end
       else
         if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^<>]+) <(\S+?)>$/
-          @persons << fmt_person($2, $3)
+          person = Persons.get($2, $3)
+          person.add_role(@id)
         end
       end
     end
-    @persons.uniq!
   end
 
 end
@@ -126,21 +169,10 @@ 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
-
-# sort by number of participations
-count_sort = lambda do |a, b|
-  b[1] <=> a[1]
-end
+persons = Persons.new
 
-count_per_person.sort(&count_sort).each do |person, count|
-  percent = count.to_f * 100 / commits.size
+persons.sort.reverse.each do |person|
+  percent = person.size.to_f * 100 / commits.size
   next if percent < $min_percent
   puts '%s (involved: %u%%)' % [person, percent]
 end
-- 
1.8.3.rc2.542.g24820ba

[PATCH v5 06/15] contrib: related: show role count

[Felipe Contreras]

Instead of showing the total involvement, show it per role: author, or
signer.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/related/git-related | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index df13148..cd1ef59 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -31,10 +31,12 @@ class Person
     @name = name
     @email = email
     @commits = {}
+    @roles = Hash.new(0)
   end
 
-  def add_role(commit)
+  def add_role(commit, role)
     @commits[commit] = true
+    @roles[role] += 1
   end
 
   def <=>(b)
@@ -79,20 +81,20 @@ class Commit
   end
 
   def parse(data)
-    msg = nil
+    msg = author = nil
     data.each_line do |line|
       if not msg
         case line
         when /^author ([^<>]+) <(\S+)> (.+)$/
           author = Persons.get($1, $2)
-          author.add_role(@id)
+          author.add_role(@id, :author)
         when /^$/
           msg = true
         end
       else
         if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^<>]+) <(\S+?)>$/
           person = Persons.get($2, $3)
-          person.add_role(@id)
+          person.add_role(@id, :signer) if person != author
         end
       end
     end
@@ -174,5 +176,11 @@ persons = Persons.new
 persons.sort.reverse.each do |person|
   percent = person.size.to_f * 100 / commits.size
   next if percent < $min_percent
-  puts '%s (involved: %u%%)' % [person, percent]
+
+  roles = person.roles.map do |role, role_count|
+    role_percent = role_count.to_f * 100 / commits.size
+    '%s: %u%%' % [role, role_percent]
+  end
+
+  puts '%s (%s)' % [person, roles.join(', ')]
 end
-- 
1.8.3.rc2.542.g24820ba

[PATCH v5 07/15] contrib: related: add support for more roles

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/related/git-related | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index cd1ef59..eef776a 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -23,6 +23,12 @@ begin
 rescue OptionParser::InvalidOption
 end
 
+KNOWN_ROLES = {
+  'Signed-off' => :signer,
+  'Reviewed' => :reviewer,
+  'Acked' => :acker,
+}
+
 class Person
 
   attr_reader :roles
@@ -92,9 +98,12 @@ class Commit
           msg = true
         end
       else
-        if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^<>]+) <(\S+?)>$/
+        role_regex = KNOWN_ROLES.keys.join('|')
+        if line =~ /^(#{role_regex})-by: ([^<>]+) <(\S+?)>$/
           person = Persons.get($2, $3)
-          person.add_role(@id, :signer) if person != author
+          role = KNOWN_ROLES[$1]
+          next if role == :signer and person == author
+          person.add_role(@id, role)
         end
       end
     end
-- 
1.8.3.rc2.542.g24820ba

[PATCH v5 08/15] contrib: related: group persons with same email

[Felipe Contreras]

Suggested-by: Duy Nguyen <pclouds@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/related/git-related | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index eef776a..9194777 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -70,7 +70,7 @@ class Persons
   end
 
   def self.get(name, email)
-    id = [name, email]
+    id = email.downcase
     person = @@index[id]
     if not person
       person = @@index[id] = Person.new(name, email)
-- 
1.8.3.rc2.542.g24820ba

[PATCH v5 09/15] contrib: related: add mailmap support

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/related/git-related | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index 9194777..3b11930 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -8,6 +8,9 @@ require 'optparse'
 $since = '5-years-ago'
 $min_percent = 10
 
+$mailmaps = {}
+$mailmaps_complex = {}
+
 begin
   OptionParser.new do |opts|
     opts.program_name = 'git related'
@@ -23,6 +26,29 @@ begin
 rescue OptionParser::InvalidOption
 end
 
+def get_mailmap(filename)
+  return unless File.exists?(filename)
+  File.open(filename) do |f|
+    f.each do |line|
+      case line.gsub(/\s*#.*$/, '')
+      when /^([^<>]+)\s+<(\S+)>$/
+        $mailmaps[$2] = [ $1, nil ]
+      when /^<(\S+)>\s+<(\S+)>$/
+        $mailmaps[$2] = [ nil, $1 ]
+      when /^([^<>]+)\s+<(\S+)>\s+<(\S+)>$/
+        $mailmaps[$3] = [ $1, $2 ]
+      when /^([^<>]+)\s+<(\S+)>\s+([^<>]+)\s+<(\S+)>$/
+        $mailmaps_complex[[$3, $4]] = [ $1, $2 ]
+      end
+    end
+  end
+end
+
+get_aliases if $get_aliases
+get_mailmap('.mailmap')
+mailmap_file = %x[git config mailmap.file].chomp
+get_mailmap(mailmap_file)
+
 KNOWN_ROLES = {
   'Signed-off' => :signer,
   'Reviewed' => :reviewer,
@@ -70,6 +96,17 @@ class Persons
   end
 
   def self.get(name, email)
+
+    # fix with mailmap
+    person = [name, email]
+    new = nil
+    new = $mailmaps_complex[person] if not new and $mailmaps_complex.include?(person)
+    new = $mailmaps[email] if not new and $mailmaps.include?(email)
+    if new
+      name = new[0] if new[0]
+      email = new[1] if new[1]
+    end
+
     id = email.downcase
     person = @@index[id]
     if not person
-- 
1.8.3.rc2.542.g24820ba

[PATCH v5 10/15] contrib: related: allow usage on other directories

[Felipe Contreras]

Not just the root one (of the project).

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/related/git-related | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index 3b11930..def2af5 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -44,8 +44,12 @@ def get_mailmap(filename)
   end
 end
 
+git_dir = %x[git rev-parse --git-dir].chomp
+$base_dir = File.dirname(git_dir)
+$cur_dir = Dir.pwd
+
 get_aliases if $get_aliases
-get_mailmap('.mailmap')
+get_mailmap(File.join($base_dir, '.mailmap'))
 mailmap_file = %x[git config mailmap.file].chomp
 get_mailmap(mailmap_file)
 
@@ -180,6 +184,7 @@ class Commits
   def get_blame(source, start, len, from)
     return if len == 0
     len ||= 1
+    Dir.chdir($base_dir)
     File.popen(['git', 'blame', '--incremental', '-C',
                '-L', '%u,+%u' % [start, len],
                '--since', $since, from + '^',
@@ -191,6 +196,7 @@ class Commits
         end
       end
     end
+    Dir.chdir($cur_dir)
   end
 
   def from_patch(file)
-- 
1.8.3.rc2.542.g24820ba

[PATCH v5 11/15] contrib: related: add support for multiple patches

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/related/git-related | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index def2af5..a2f98d9 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -14,7 +14,7 @@ $mailmaps_complex = {}
 begin
   OptionParser.new do |opts|
     opts.program_name = 'git related'
-    opts.banner = 'usage: git related [options] <file>'
+    opts.banner = 'usage: git related [options] <files>'
 
     opts.on('-p', '--min-percent N', Integer, 'Minium percentage of role participation') do |v|
       $min_percent = v
@@ -156,6 +156,7 @@ class Commits
 
   def initialize
     @items = {}
+    @main_commits = {}
   end
 
   def size
@@ -192,24 +193,28 @@ class Commits
       p.each do |line|
         if line =~ /^(\h{40})/
           id = $1
-          @items[id] = Commit.new(id)
+          @items[id] = Commit.new(id) if not @main_commits.include?(id)
         end
       end
     end
     Dir.chdir($cur_dir)
   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)
+  def from_patches(files)
+    source = nil
+    files.each do |file|
+      from = nil
+      File.open(file) do |f|
+        f.each do |line|
+          case line
+          when /^From (\h+) (.+)$/
+            from = $1
+            @main_commits[from] = true
+          when /^---\s+(\S+)/
+            source = $1 != '/dev/null' ? $1[2..-1] : nil
+          when /^@@ -(\d+)(?:,(\d+))?/
+            get_blame(source, $1, $2, from)
+          end
         end
       end
     end
@@ -217,10 +222,8 @@ class Commits
 
 end
 
-exit 1 if ARGV.size != 1
-
 commits = Commits.new
-commits.from_patch(ARGV[0])
+commits.from_patches(ARGV)
 commits.import
 
 persons = Persons.new
-- 
1.8.3.rc2.542.g24820ba

[PATCH v5 12/15] contrib: related: add option to show commits

[Felipe Contreras]

Instead of showing the authors and signers, show the commits themselves.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/related/git-related | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index a2f98d9..aec156e 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -7,6 +7,7 @@ require 'optparse'
 
 $since = '5-years-ago'
 $min_percent = 10
+$show_commits = false
 
 $mailmaps = {}
 $mailmaps_complex = {}
@@ -22,6 +23,9 @@ begin
     opts.on('-d', '--since DATE', 'How far back to search for relevant commits') do |v|
       $since = v
     end
+    opts.on('-c', '--commits[=FORMAT]', [:raw, :full], 'List commits instead of persons') do |v|
+      $show_commits = v || true
+    end
   end.parse!
 rescue OptionParser::InvalidOption
 end
@@ -167,6 +171,10 @@ class Commits
     @items.each(&block)
   end
 
+  def list
+    @items.keys
+  end
+
   def import
     return if @items.empty?
     File.popen(%w[git cat-file --batch], 'r+') do |p|
@@ -226,6 +234,20 @@ commits = Commits.new
 commits.from_patches(ARGV)
 commits.import
 
+if $show_commits
+  cmd = nil
+  case $show_commits
+  when :raw
+    puts commits.list
+  when :full
+    cmd = %w[git log --patch --no-walk]
+  else
+    cmd = %w[git log --oneline --no-walk]
+  end
+  system(*cmd + commits.list) if cmd
+  exit 0
+end
+
 persons = Persons.new
 
 persons.sort.reverse.each do |person|
-- 
1.8.3.rc2.542.g24820ba

[PATCH v5 13/15] contrib: related: add option to parse from committish

[Felipe Contreras]

For example master..feature-a.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/related/git-related | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index aec156e..8394cdc 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -8,6 +8,8 @@ require 'optparse'
 $since = '5-years-ago'
 $min_percent = 10
 $show_commits = false
+$files = []
+$rev_args = []
 
 $mailmaps = {}
 $mailmaps_complex = {}
@@ -15,7 +17,7 @@ $mailmaps_complex = {}
 begin
   OptionParser.new do |opts|
     opts.program_name = 'git related'
-    opts.banner = 'usage: git related [options] <files>'
+    opts.banner = 'usage: git related [options] <files | rev-list options>'
 
     opts.on('-p', '--min-percent N', Integer, 'Minium percentage of role participation') do |v|
       $min_percent = v
@@ -228,10 +230,40 @@ class Commits
     end
   end
 
+  def from_rev_args(args)
+    return if args.empty?
+    source = nil
+    File.popen(%w[git rev-list --reverse] + args) do |p|
+      p.each do |e|
+        id = e.chomp
+        @main_commits[id] = true
+        File.popen(%w[git show -C --oneline] + [id]) do |p|
+          p.each do |e|
+            case e
+            when /^---\s+(\S+)/
+              source = $1 != '/dev/null' ? $1[2..-1] : nil
+            when /^@@ -(\d+)(?:,(\d+))?/
+              get_blame(source, $1, $2, id)
+            end
+          end
+        end
+      end
+    end
+  end
+
+end
+
+ARGV.each do |e|
+  if File.exists?(e)
+    $files << e
+  else
+    $rev_args << e
+  end
 end
 
 commits = Commits.new
-commits.from_patches(ARGV)
+commits.from_patches($files)
+commits.from_rev_args($rev_args)
 commits.import
 
 if $show_commits
-- 
1.8.3.rc2.542.g24820ba

[PATCH v5 14/15] contrib: related: parse committish like format-patch

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/related/git-related | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index 8394cdc..62c9b56 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -232,6 +232,20 @@ class Commits
 
   def from_rev_args(args)
     return if args.empty?
+
+    revs = []
+
+    File.popen(%w[git rev-parse --revs-only --default=HEAD --symbolic] + args).each do |rev|
+      revs << rev.chomp
+    end
+
+    case revs.size
+    when 1
+      committish = [ '%s..HEAD' % revs[0] ]
+    else
+      committish = revs
+    end
+
     source = nil
     File.popen(%w[git rev-list --reverse] + args) do |p|
       p.each do |e|
-- 
1.8.3.rc2.542.g24820ba

[PATCH v5 15/15] contrib: related: fix parsing of rev-list args

[Felipe Contreras]

For example '-1'.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/related/git-related | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/contrib/related/git-related b/contrib/related/git-related
index 62c9b56..69737ac 100755
--- a/contrib/related/git-related
+++ b/contrib/related/git-related
@@ -29,7 +29,8 @@ begin
       $show_commits = v || true
     end
   end.parse!
-rescue OptionParser::InvalidOption
+rescue OptionParser::InvalidOption => e
+  $rev_args += e.args
 end
 
 def get_mailmap(filename)
@@ -241,9 +242,11 @@ class Commits
 
     case revs.size
     when 1
-      committish = [ '%s..HEAD' % revs[0] ]
+      r = revs[0]
+      r = '^' + r if r[0] != '-'
+      args = [ r, 'HEAD' ]
     else
-      committish = revs
+      args = revs
     end
 
     source = nil
-- 
1.8.3.rc2.542.g24820ba

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

[Felipe Contreras]

On Sat, May 18, 2013 at 6:46 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:

>  contrib/related/git-related | 124 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 124 insertions(+)
>  create mode 100755 contrib/related/git-related

I tried everything and I don't think it's physically possible to make
this script any simpler without severely crippling it's main goal.

-- 
Felipe Contreras

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Sat, May 18, 2013 at 6:46 AM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>
>>  contrib/related/git-related | 124 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 124 insertions(+)
>>  create mode 100755 contrib/related/git-related
>
> I tried everything and I don't think it's physically possible to make
> this script any simpler without severely crippling it's main goal.

Hmm, I haven't read these patches yet (I just came back a few hours
ago to a state in which I am well enough to read and write e-mails),
but did anybody complain that it is too complex?

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

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > On Sat, May 18, 2013 at 6:46 AM, Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
> >
> >>  contrib/related/git-related | 124 ++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 124 insertions(+)
> >>  create mode 100755 contrib/related/git-related
> >
> > I tried everything and I don't think it's physically possible to make
> > this script any simpler without severely crippling it's main goal.
> 
> Hmm, I haven't read these patches yet (I just came back a few hours
> ago to a state in which I am well enough to read and write e-mails),
> but did anybody complain that it is too complex?

This comment was again targetted to Ramkumar, who yes, indeed he complained
about it being too complex, at least v3.

-- 
Felipe Contreras

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

[Ramkumar Ramachandra]

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.

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

[Felipe Contreras]

On Sun, May 19, 2013 at 9:40 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> 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.

The commit's unique identifier. How is that not clear?

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

That's not efficient, it's more efficient to parse line by line
lazily, and it's not that complicated.

>> +      else
>> +        if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^<>]+) <(\S+?)>$/
>
> elif?

It will get bigger soon enough.

> Can this be more generic to include Whatevered-by?

That's done in later patches. This is a minimal version.

>> +          @persons << fmt_person($2, $3)
>
> Will $2 ever be nil (from fmt_person)?  ie. Why are you checking for
> the special case " <\S+?>$"?

Yes, '<email>' was valid in earlier versions of git.

>> +        end
>> +      end
>> +    end
>> +    @persons.uniq!
>
> So you don't care if the person has appeared twice or thrice in the message.

Yes I do, otherwise they be counted multiple times, and their
participation calculation would go beyond 100%.

>> +  def size
>> +    @items.size
>> +  end
>
> #size is reminiscent of Array#size and Hash#size.

Precisely. I would even include Array if it made sense.

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

We can include Enumerable, it won't make a difference.

> 1. Use block_given? to make sure that #each works even without a block
> (just like Enumerator#each).

It already works just like Enumerator#each; commits.each returns an
Enumerator because @items.each returns an Enumerator.

> 2. Mixin Enumerable by putting in an 'include Enumerable' after the
> class line.  Aside from clarity, you get stuff like #find for free.

Stuff we don't need or want.

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

Fine.

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

How exactly is it not equivalent to len = len || 1?

This is what I want:

len = 1 if not len

And I think the above does exactly that.

>> +    File.popen(['git', 'blame', '--incremental', '-C',
>
> Still no -CCC?

I forgot.

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

No, I'm not matching the whole line, but you are right; there's no
need for groups.

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

Later. This does the minimum.

> Overall, significant improvement over v3 which used all kinds of
> unnecessarily complex data structures and convoluted logic.

It's coming.

-- 
Felipe Contreras

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

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> How exactly is it not equivalent to len = len || 1?

Here, I dug up an article for you on the issue:

http://www.rubyinside.com/what-rubys-double-pipe-or-equals-really-does-5488.html

Although it's fine in this case, I wouldn't recommend using ||=
because of the potential confusion.

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

[Ramkumar Ramachandra]

Felipe Contreras wrote:
>> Will $2 ever be nil (from fmt_person)?  ie. Why are you checking for
>> the special case " <\S+?>$"?
>
> Yes, '<email>' was valid in earlier versions of git.

There's a non-optional space before the "<email>" in your regex, which
is what I was pointing out.

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

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> There's a non-optional space before the "<email>" in your regex, which
> is what I was pointing out.

Er, scratch that.  It's the space after the "Whatevered-by:"

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

[Felipe Contreras]

On Sun, May 19, 2013 at 10:13 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> How exactly is it not equivalent to len = len || 1?
>
> Here, I dug up an article for you on the issue:
>
> http://www.rubyinside.com/what-rubys-double-pipe-or-equals-really-does-5488.html
>
> Although it's fine in this case, I wouldn't recommend using ||=
> because of the potential confusion.

I don't see the confusion, 'len ||= 1' is *exactly* the same as 'len =
1 if not len', which is what I expected.

-- 
Felipe Contreras

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

[Felipe Contreras]

On Sun, May 19, 2013 at 10:17 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Ramkumar Ramachandra wrote:
>> There's a non-optional space before the "<email>" in your regex, which
>> is what I was pointing out.
>
> Er, scratch that.  It's the space after the "Whatevered-by:"

It doesn't really matter. We can operate under the assumption that
there will always be a name and remove the extra code in fmt_person(),
IIRC it was because of the muttrc simplification which I don't think
makes sense any more. It's still possible that the name will be
missing, but I don't personally care, it would probably work for
99.99% of the cases.

-- 
Felipe Contreras

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

[Felipe Contreras]

On Sun, May 19, 2013 at 10:05 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Sun, May 19, 2013 at 9:40 AM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:

>>> +               '-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.
>
> No, I'm not matching the whole line, but you are right; there's no
> need for groups.

Actually $&.

-- 
Felipe Contreras

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

[David Aguilar]

On Sun, May 19, 2013 at 8:41 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Sun, May 19, 2013 at 10:13 AM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>> Felipe Contreras wrote:
>>> How exactly is it not equivalent to len = len || 1?
>>
>> Here, I dug up an article for you on the issue:
>>
>> http://www.rubyinside.com/what-rubys-double-pipe-or-equals-really-does-5488.html
>>
>> Although it's fine in this case, I wouldn't recommend using ||=
>> because of the potential confusion.
>
> I don't see the confusion, 'len ||= 1' is *exactly* the same as 'len =
> 1 if not len', which is what I expected.

I spend more time in Python, C*, and Perl then in Ruby, and I was not
confused by this at all.

It behaves just like $foo ||= 1 does in Perl.

In git.git:

$ git grep '||=' | wc -l
     121
--
David