[PATCH v2 0/8] New git-cc-cmd helper

Hi,

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

  % git cc-cmd v1.8.1.6^^1..v1.8.1.6^^2
  "Henrik Grubbström" [off-list ref] (author: 7%)
  junio (signer: 84%, author: 15%)
  "Nguyễn Thái Ngọc Duy" [off-list ref] (author: 30%, signer: 7%)
  "Jean-Noël AVILA" [off-list ref] (author: 7%)
  Jean-Noel Avila [off-list ref] (signer: 7%)
  Duy Nguyen [off-list ref] (author: 7%)
  Michael Haggerty [off-list ref] (author: 15%)
  Clemens Buchacher [off-list ref] (author: 7%)
  Joshua Jensen [off-list ref] (author: 7%)
  Johannes Sixt [off-list ref] (signer: 7%)

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 cc-cmd --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.
  407a963 Merge branch 'rr/remote-helper-doc'
  ec775c4 attr: Expand macros immediately when encountered.

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.

Felipe Contreras (8):
  Add new git-cc-cmd helper to contrib
  contrib: cc-cmd: add option parsing
  contrib: cc-cmd: add support for multiple patches
  contrib: cc-cmd: add option to show commits
  contrib: cc-cmd: add option to parse from committish
  contrib: cc-cmd: parse committish like format-patch
  contrib: cc-cmd: fix parsing of rev-list args
  contrib: cc-cmd: add option  to fetch aliases

 contrib/cc-cmd/git-cc-cmd | 245 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 245 insertions(+)
 create mode 100755 contrib/cc-cmd/git-cc-cmd

-- 
1.8.2.1.790.g4588561

The code finds the changes of a commit, runs 'git blame' for each chunk
to see which other commits are relevant, and then reports the author and
signers.

Finally, it calculates what percentage of the total relevant commits
each person was involved in, and show only the ones that pass the
threshold.

For example:

  % git cc-cmd 0001-remote-hg-trivial-cleanups.patch
  Felipe Contreras [off-list ref] (author: 100%)
  Jeff King [off-list ref] (signer: 83%)
  Max Horn [off-list ref] (signer: 16%)
  Junio C Hamano [off-list ref] (signer: 16%)

Thus it can be used for 'git send-email' as a cc-cmd.

Signed-off-by: Felipe Contreras <redacted>
---
 contrib/cc-cmd/git-cc-cmd | 140 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 140 insertions(+)
 create mode 100755 contrib/cc-cmd/git-cc-cmd

diff --git a/contrib/cc-cmd/git-cc-cmd b/contrib/cc-cmd/git-cc-cmd
new file mode 100755
index 0000000..c7ecf79
--- /dev/null
+++ b/contrib/cc-cmd/git-cc-cmd

@@ -0,0 +1,140 @@
+#!/usr/bin/env ruby
+
+$since = '3-years-ago'
+$min_percent = 5
+
+class Commit
+
+  attr_reader :id
+  attr_accessor :roles
+
+  def initialize(id)
+    @id = id
+    @roles = []
+  end
+
+  def self.parse(data)
+    id = author = msg = nil
+    roles = {}
+    data.each_line do |line|
+      if not msg
+        case line
+        when /^commit (.+)$/
+          id = $1
+        when /^author ([^<>]+) <(\S+)>$/
+          author = $1, $2
+          roles[author] = 'author'
+        when /^$/
+          msg = true
+        end
+      else
+        if line =~ /^(Signed-off|Reviewed|Acked)-by: ([^<>]+) <(\S+?)>$/
+          person = $2, $3
+          roles[person] = 'signer' if person != author
+        end
+      end
+    end
+    roles = roles.map do |person, role|
+      address = "%s <%s>" % person
+      [person, role]
+    end
+    [id, roles]
+  end
+
+end
+
+class Commits
+
+  attr_reader :items
+
+  def initialize()
+    @items = {}
+  end
+
+  def size
+    @items.size
+  end
+
+  def import
+    return if @items.empty?
+    format = [ 'commit %H', 'author %an <%ae>', '', '%B' ].join('%n')
+    File.popen(['git', 'show', '-z', '-s', '--format=format:' + format] + @items.keys) do |p|
+      p.each("\0") do |data|
+        next if data == "\0" # bug in git show?
+        id, roles = Commit.parse(data)
+        commit = @items[id]
+        commit.roles = roles
+      end
+    end
+  end
+
+  def each_person_role
+    commit_roles = @items.values.map { |commit| commit.roles }.flatten(1)
+    commit_roles.group_by { |person, role| person }.each do |person, commit_roles|
+      commit_roles.group_by { |person, role| role }.each do |role, commit_roles|
+        yield person, role, commit_roles.size
+      end
+    end
+  end
+
+  def get_blame(source, start, offset, from)
+    return unless source
+    File.popen(['git', 'blame', '--incremental', '-C',
+               '-L', '%u,+%u' % [start, offset],
+               '--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)
+    source = nil
+    from = 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 /^@@\s-(\d+),(\d+)/
+          get_blame(source, $1, $2, from)
+        end
+      end
+    end
+    import
+  end
+
+end
+
+exit 1 if ARGV.size != 1
+
+commits = Commits.new()
+commits.from_patch(ARGV[0])
+
+# hash of hashes
+persons = Hash.new { |hash, key| hash[key] = {} }
+
+commits.each_person_role do |person, role, count|
+  persons[person][role] = count
+end
+
+persons.each do |person, roles|
+  roles = roles.map do |role, count|
+    percent = count.to_f * 100 / commits.size
+    next if percent < $min_percent
+    "%s: %u%%" % [role, percent]
+  end.compact
+  next if roles.empty?
+
+  name, email = person
+  # must quote chars?
+  name = '"%s"' % name if name =~ /[^\w \-]/i
+  person = name ? "%s <%s>" % [name, email] : email
+  puts "%s (%s)" % [person, roles.join(', ')]
+end

-- 
1.8.2.1.790.g4588561

Signed-off-by: Felipe Contreras <redacted>
---
 contrib/cc-cmd/git-cc-cmd | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/contrib/cc-cmd/git-cc-cmd b/contrib/cc-cmd/git-cc-cmd
index c7ecf79..0a5ec01 100755
--- a/contrib/cc-cmd/git-cc-cmd
+++ b/contrib/cc-cmd/git-cc-cmd

@@ -1,8 +1,25 @@
 #!/usr/bin/env ruby
 
+require 'optparse'
+
 $since = '3-years-ago'
 $min_percent = 5
 
+begin
+  OptionParser.new do |opts|
+    opts.program_name = 'git cc-cmd'
+    opts.banner = 'usage: git cc-cmd [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 :id

@@ -107,15 +124,15 @@ class Commits
         end
       end
     end
-    import
   end
 
 end
 
 exit 1 if ARGV.size != 1
 
-commits = Commits.new()
+commits = Commits.new
 commits.from_patch(ARGV[0])
+commits.import
 
 # hash of hashes
 persons = Hash.new { |hash, key| hash[key] = {} }

-- 
1.8.2.1.790.g4588561

Signed-off-by: Felipe Contreras <redacted>
---
 contrib/cc-cmd/git-cc-cmd | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/contrib/cc-cmd/git-cc-cmd b/contrib/cc-cmd/git-cc-cmd
index 0a5ec01..e36b1bf 100755
--- a/contrib/cc-cmd/git-cc-cmd
+++ b/contrib/cc-cmd/git-cc-cmd

@@ -8,7 +8,7 @@ $min_percent = 5
 begin
   OptionParser.new do |opts|
     opts.program_name = 'git cc-cmd'
-    opts.banner = 'usage: git cc-cmd [options] <file>'
+    opts.banner = 'usage: git cc-cmd [options] <files>'
 
     opts.on('-p', '--min-percent N', Integer, 'Minium percentage of role participation') do |v|
       $min_percent = v

@@ -66,6 +66,7 @@ class Commits
 
   def initialize()
     @items = {}
+    @main_commits = {}
   end
 
   def size

@@ -103,24 +104,27 @@ 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
   end
 
-  def from_patch(file)
+  def from_patches(files)
     source = nil
-    from = 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 /^@@\s-(\d+),(\d+)/
-          get_blame(source, $1, $2, from)
+    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 /^@@\s-(\d+),(\d+)/
+            get_blame(source, $1, $2, from)
+          end
         end
       end
     end

@@ -128,10 +132,8 @@ class Commits
 
 end
 
-exit 1 if ARGV.size != 1
-
 commits = Commits.new
-commits.from_patch(ARGV[0])
+commits.from_patches(ARGV)
 commits.import
 
 # hash of hashes

-- 
1.8.2.1.790.g4588561

Signed-off-by: Felipe Contreras <redacted>
---
 contrib/cc-cmd/git-cc-cmd | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/contrib/cc-cmd/git-cc-cmd b/contrib/cc-cmd/git-cc-cmd
index 462f22c..02e1a99 100755
--- a/contrib/cc-cmd/git-cc-cmd
+++ b/contrib/cc-cmd/git-cc-cmd

@@ -138,6 +138,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.2.1.790.g4588561

Only the mutt format is supported for now.

Signed-off-by: Felipe Contreras <redacted>
---
 contrib/cc-cmd/git-cc-cmd | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/contrib/cc-cmd/git-cc-cmd b/contrib/cc-cmd/git-cc-cmd
index 6911259..02548c6 100755
--- a/contrib/cc-cmd/git-cc-cmd
+++ b/contrib/cc-cmd/git-cc-cmd

@@ -7,6 +7,8 @@ $min_percent = 5
 $show_commits = false
 $files = []
 $rev_args = []
+$get_aliases = false
+$aliases = {}
 
 begin
   OptionParser.new do |opts|

@@ -22,11 +24,32 @@ begin
     opts.on('-c', '--commits[=FORMAT]', [:raw], 'List commits instead of persons') do |v|
       $show_commits = v || true
     end
+    opts.on('-a', '--aliases', 'Use aliases') do |v|
+      $get_aliases = v
+    end
   end.parse!
 rescue OptionParser::InvalidOption => e
   $rev_args += e.args
 end
 
+def get_aliases
+  type = %x[git config sendemail.aliasfiletype].chomp
+  return if type != 'mutt'
+  file = %x[git config sendemail.aliasesfile].chomp
+  File.open(File.expand_path(file)) do |f|
+    f.each do |line|
+      if line =~ /^\s*alias\s+(?:-group\s+\S+\s+)*(\S+)\s+(.*)$/
+        key, addresses = $1, $2.split(', ')
+        addresses.each do |address|
+          $aliases[address] = key
+        end
+      end
+    end
+  end
+end
+
+get_aliases if $get_aliases
+
 class Commit
 
   attr_reader :id

@@ -60,6 +83,7 @@ class Commit
     end
     roles = roles.map do |person, role|
       address = "%s <%s>" % person
+      person = nil, $aliases[address] if $aliases.include?(address)
       [person, role]
     end
     [id, roles]

-- 
1.8.2.1.790.g4588561

For example '-1'.

Signed-off-by: Felipe Contreras <redacted>
---
 contrib/cc-cmd/git-cc-cmd | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/contrib/cc-cmd/git-cc-cmd b/contrib/cc-cmd/git-cc-cmd
index 02e1a99..6911259 100755
--- a/contrib/cc-cmd/git-cc-cmd
+++ b/contrib/cc-cmd/git-cc-cmd

@@ -23,7 +23,8 @@ begin
       $show_commits = v || true
     end
   end.parse!
-rescue OptionParser::InvalidOption
+rescue OptionParser::InvalidOption => e
+  $rev_args += e.args
 end
 
 class Commit

@@ -147,9 +148,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.2.1.790.g4588561

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

Signed-off-by: Felipe Contreras <redacted>
---
 contrib/cc-cmd/git-cc-cmd | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/contrib/cc-cmd/git-cc-cmd b/contrib/cc-cmd/git-cc-cmd
index e36b1bf..f13ed8f 100755
--- a/contrib/cc-cmd/git-cc-cmd
+++ b/contrib/cc-cmd/git-cc-cmd

@@ -4,6 +4,7 @@ require 'optparse'
 
 $since = '3-years-ago'
 $min_percent = 5
+$show_commits = false
 
 begin
   OptionParser.new do |opts|

@@ -16,6 +17,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], 'List commits instead of persons') do |v|
+      $show_commits = v || true
+    end
   end.parse!
 rescue OptionParser::InvalidOption
 end

@@ -136,6 +140,15 @@ commits = Commits.new
 commits.from_patches(ARGV)
 commits.import
 
+if $show_commits
+  if $show_commits == :raw
+    puts commits.items.keys
+  else
+    system(*['git', 'log', '--oneline', '--no-walk'] + commits.items.keys)
+  end
+  exit 0
+end
+
 # hash of hashes
 persons = Hash.new { |hash, key| hash[key] = {} }
 

-- 
1.8.2.1.790.g4588561

For example master..feature-a.

Signed-off-by: Felipe Contreras <redacted>
---
 contrib/cc-cmd/git-cc-cmd | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/contrib/cc-cmd/git-cc-cmd b/contrib/cc-cmd/git-cc-cmd
index f13ed8f..462f22c 100755
--- a/contrib/cc-cmd/git-cc-cmd
+++ b/contrib/cc-cmd/git-cc-cmd

@@ -5,11 +5,13 @@ require 'optparse'
 $since = '3-years-ago'
 $min_percent = 5
 $show_commits = false
+$files = []
+$rev_args = []
 
 begin
   OptionParser.new do |opts|
     opts.program_name = 'git cc-cmd'
-    opts.banner = 'usage: git cc-cmd [options] <files>'
+    opts.banner = 'usage: git cc-cmd [options] <files | rev-list options>'
 
     opts.on('-p', '--min-percent N', Integer, 'Minium percentage of role participation') do |v|
       $min_percent = v

@@ -134,10 +136,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 --no-pager show -C --oneline] + [id]) do |p|
+          p.each do |e|
+            case e
+            when /^---\s+(\S+)/
+              source = $1 != '/dev/null' ? $1[2..-1] : nil
+            when /^@@\s-(\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.2.1.790.g4588561

Felipe Contreras wrote:

The code finds the changes of a commit, runs 'git blame' for each chunk
to see which other commits are relevant, and then reports the author and
signers.

Finally, it calculates what percentage of the total relevant commits
each person was involved in, and show only the ones that pass the
threshold.

Um, this didn't really explain it to me.  How about:

This command takes a patch prepared by 'git format-patch' as an
argument, and runs 'git blame' on every hunk of the diff to determine
the commits that added/removed those lines.  It then aggregates the
authors and signers of all the commits, and prints out these people in
descending order of the percentage of commits they were responsible
for.  It omits people are below a certain threshold.

  % git cc-cmd 0001-remote-hg-trivial-cleanups.patch
  Felipe Contreras [off-list ref] (author: 100%)
  Jeff King [off-list ref] (signer: 83%)
  Max Horn [off-list ref] (signer: 16%)
  Junio C Hamano [off-list ref] (signer: 16%)

Won't my name appear as the first one on each of my patches?  Will I
be CC'ed on every patch that I send out?

 contrib/cc-cmd/git-cc-cmd | 140 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 140 insertions(+)
 create mode 100755 contrib/cc-cmd/git-cc-cmd

No README?

What is the minimum required version of Ruby, by the way?  I see you
haven't used any fancy lazy enumerators or optional arguments
introduced in 2.0, so we should be okay practically speaking.

+$since = '3-years-ago'
+$min_percent = 5

Do I get to configure these two?  Also, the '$' prefix from your Perl
habit?  It's not idiomatic in Ruby afaik.

+class Commit
+
+  attr_reader :id
+  attr_accessor :roles
+
+  def initialize(id)
+    @id = id
+    @roles = []

What are id, roles exactly?  This isn't C where we have types, so
you're going to have to use some (rdoc-parseable) comments, if you
want me to be able to read the code without jumping around too much.

+  end
+
+  def self.parse(data)
+    id = author = msg = nil

This is not C.  You don't need to initialize stuff, unless you're
going to start accessing it using a .append() or similar.  In that
case, you need to initialize it as an empty list, so the compiler
knows what you're appending to.

+    roles = {}

Shouldn't this be @roles?  Didn't you initialize it as an empty list
in initialize()?  Why did you suddenly change your mind and make it a
hashtable now?

+    data.each_line do |line|
+      if not msg
+        case line
+        when /^commit (.+)$/
+          id = $1

Okay, I have no idea what you're parsing yet.  There's some commit
line, so I know this is not a raw commit object, as the name of the
class Commit would've indicated.

+        when /^author ([^<>]+) <(\S+)>$/
+          author = $1, $2
+          roles[author] = 'author'

Huh?  the key is a two-item list, and the value is a hard-coded
'author' string?  If it's meant to be a sematic value, use a symbol.

Also, why don't you just collect the authors and signers in two
separate lists, instead of doing this and burdening yourself with
accumulation later?

+        when /^$/
+          msg = true

I can't see msg being used later in the function, so I don't know what
you're doing.

+    roles = roles.map do |person, role|
+      address = "%s <%s>" % person
+      [person, role]

What?!  If you wanted address in the first place, why did you stuff a
two-member list into roles as the key?  Where is address being used?

+  def import
+    return if @items.empty?
+    format = [ 'commit %H', 'author %an <%ae>', '', '%B' ].join('%n')
+    File.popen(['git', 'show', '-z', '-s', '--format=format:' + format] + @items.keys) do |p|

Ah, you're using 'git show'.  I thought cat-file --batch was
especially well-suited for this task.  What do you need the
pretty-printing for?

+      p.each("\0") do |data|
+        next if data == "\0" # bug in git show?

What is the bug exactly?  It displays two consecutive \0 characters
sometimes, when given a bunch of items to display?

+        id, roles = Commit.parse(data)
+        commit = @items[id]
+        commit.roles = roles

@items[id].roles = roles

+  def each_person_role
+    commit_roles = @items.values.map { |commit| commit.roles }.flatten(1)
+    commit_roles.group_by { |person, role| person }.each do |person, commit_roles|
+      commit_roles.group_by { |person, role| role }.each do |role, commit_roles|
+        yield person, role, commit_roles.size

Unnecessary work if you'd chosen a better way to store the person =>
role mapping in the first place.

+  def get_blame(source, start, offset, from)
+    return unless source
+    File.popen(['git', 'blame', '--incremental', '-C',
+               '-L', '%u,+%u' % [start, offset],
+               '--since', $since, from + '^',
+               '--', source]) do |p|

While at it, why not blame -M -CCC?

+  def from_patch(file)

You're not going to 'git mailsplit', like am does, first?

+        when /^@@\s-(\d+),(\d+)/
+          get_blame(source, $1, $2, from)

Okay, so this is where you get the arguments for the -L in blame.

+exit 1 if ARGV.size != 1

No usage?

+commits.each_person_role do |person, role, count|
+  persons[person][role] = count

Oh dear.  Don't you think you're over-engineering here?  Just because
you can stuff anything into hashes/lists in Ruby, doesn't mean that
you should and kill efficiency.

+persons.each do |person, roles|
+  roles = roles.map do |role, count|
+    percent = count.to_f * 100 / commits.size

Interesting.  You also take into account the size of the commits when
calculating the final score.

+    next if percent < $min_percent
+    "%s: %u%%" % [role, percent]
+  end.compact

What are these nil elements you're filtering out with .compact?

Overall, it looks like the whole thing is one giant first draft
written in a big rush.  For a relatively simple task, you've used a
lot of complex data structures and complicated things beyond belief.
I'm sorry, but this giant mess wasn't at all a pleasure to read.  No
doubt that the idea is good though.  I'm stopping here.

On Fri, Apr 19, 2013 at 8:26 AM, Ramkumar Ramachandra
[off-list ref] wrote:

Felipe Contreras wrote:

quoted

The code finds the changes of a commit, runs 'git blame' for each chunk
to see which other commits are relevant, and then reports the author and
signers.

Finally, it calculates what percentage of the total relevant commits
each person was involved in, and show only the ones that pass the
threshold.

Um, this didn't really explain it to me.  How about:

This command takes a patch prepared by 'git format-patch' as an
argument, and runs 'git blame' on every hunk of the diff to determine
the commits that added/removed those lines.  It then aggregates the
authors and signers of all the commits, and prints out these people in
descending order of the percentage of commits they were responsible
for.  It omits people are below a certain threshold.

Fine by me.

quoted

  % git cc-cmd 0001-remote-hg-trivial-cleanups.patch
  Felipe Contreras [off-list ref] (author: 100%)
  Jeff King [off-list ref] (signer: 83%)
  Max Horn [off-list ref] (signer: 16%)
  Junio C Hamano [off-list ref] (signer: 16%)

Won't my name appear as the first one on each of my patches?  Will I
be CC'ed on every patch that I send out?

No. The patches being analyzed are ignored. Either way, you are the
s-o-b, so send-email Cc's you by default.

quoted

 contrib/cc-cmd/git-cc-cmd | 140 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 140 insertions(+)
 create mode 100755 contrib/cc-cmd/git-cc-cmd

No README?

No README. Half the stuff in contrib doesn't have one. And I don't see
what stuff of value we could have there.

quoted

diff --git a/contrib/cc-cmd/git-cc-cmd b/contrib/cc-cmd/git-cc-cmd
new file mode 100755
index 0000000..c7ecf79
--- /dev/null
+++ b/contrib/cc-cmd/git-cc-cmd

@@ -0,0 +1,140 @@
+#!/usr/bin/env ruby

What is the minimum required version of Ruby, by the way?

I don't know.

I see you
haven't used any fancy lazy enumerators

I did, but they are implicit.

quoted

+$since = '3-years-ago'
+$min_percent = 5

Do I get to configure these two?

Patience. That's for another patch.

Also, the '$' prefix from your Perl
habit?  It's not idiomatic in Ruby afaik.

It is for global variables.

quoted

+class Commit
+
+  attr_reader :id
+  attr_accessor :roles
+
+  def initialize(id)
+    @id = id
+    @roles = []

What are id, roles exactly?  This isn't C where we have types, so
you're going to have to use some (rdoc-parseable) comments, if you
want me to be able to read the code without jumping around too much.

The id of the commit, of course (aka. SHA-1), and the roles people had
in this commit. We are after all in the Commit class.

quoted

+  end
+
+  def self.parse(data)
+    id = author = msg = nil

This is not C.  You don't need to initialize stuff, unless you're
going to start accessing it using a .append() or similar.  In that
case, you need to initialize it as an empty list, so the compiler
knows what you're appending to.

You need to define them if you are going to modify them in a block,
and access them after the block.

quoted

+    roles = {}

Shouldn't this be @roles?

No, we are in a class method, we don't have instance variables; we
don't have an instance.

Didn't you initialize it as an empty list
in initialize()?

No, that's for the instance; we don't have an instance yet.

Why did you suddenly change your mind and make it a
hashtable now?

It's a completely different variable; it's local to the method.

quoted

+    data.each_line do |line|
+      if not msg
+        case line
+        when /^commit (.+)$/
+          id = $1

Okay, I have no idea what you're parsing yet.  There's some commit
line, so I know this is not a raw commit object, as the name of the
class Commit would've indicated.

Yeah, it's parsing a raw commit object.

quoted

+        when /^author ([^<>]+) <(\S+)>$/
+          author = $1, $2
+          roles[author] = 'author'

Huh?  the key is a two-item list, and the value is a hard-coded
'author' string?  If it's meant to be a sematic value, use a symbol.

Yeah, that might make sense.

Also, why don't you just collect the authors and signers in two
separate lists, instead of doing this and burdening yourself with
accumulation later?

Go ahead and try this change. How are you going to group persons by
the role they took in the relevant commits? It's much simpler to have
generic code that is agnostic of the types of roles there are. Also,
this would make it simpler to add other roles.

quoted

+        when /^$/
+          msg = true

I can't see msg being used later in the function, so I don't know what
you're doing.

It's used before.

quoted

+    roles = roles.map do |person, role|
+      address = "%s <%s>" % person
+      [person, role]

What?!  If you wanted address in the first place, why did you stuff a
two-member list into roles as the key?

What difference does it make? 'person' could be an object, it could be anything.

Where is address being used?

In a later patch. This line creeped in.

quoted

+  def import
+    return if @items.empty?
+    format = [ 'commit %H', 'author %an <%ae>', '', '%B' ].join('%n')
+    File.popen(['git', 'show', '-z', '-s', '--format=format:' + format] + @items.keys) do |p|

Ah, you're using 'git show'.  I thought cat-file --batch was
especially well-suited for this task.

Perhaps, but the code might end more complicated though.

What do you need the
pretty-printing for?

To generate a raw commit; the raw format is not really so.

quoted

+      p.each("\0") do |data|
+        next if data == "\0" # bug in git show?

What is the bug exactly?  It displays two consecutive \0 characters
sometimes, when given a bunch of items to display?

Apparently.

quoted

+        id, roles = Commit.parse(data)
+        commit = @items[id]
+        commit.roles = roles

@items[id].roles = roles

NoMethodError: undefined method `roles=' for nil:NilClass

quoted

+  def each_person_role
+    commit_roles = @items.values.map { |commit| commit.roles }.flatten(1)
+    commit_roles.group_by { |person, role| person }.each do |person, commit_roles|
+      commit_roles.group_by { |person, role| role }.each do |role, commit_roles|
+        yield person, role, commit_roles.size

Unnecessary work if you'd chosen a better way to store the person =>
role mapping in the first place.

No. The contents of the person key are irrelevant here, they are
merely used as that: a key.

It's trivial to get the role a person took in a given commit; we are
not interested in that. We want the sum of all the author roles a
person had, and the signer roles, and any other rules that might be
specified in the future. *And* we want that count to be grouped first
by person, and then by role, because that's precisely what we will
show the user.

There's no better way to represent the data of each commit than by a
Commit object, and there's no better way to represent the roles each
person took in a certain commit, than a Hash. Which is exactly what we
have.

quoted

+  def get_blame(source, start, offset, from)
+    return unless source
+    File.popen(['git', 'blame', '--incremental', '-C',
+               '-L', '%u,+%u' % [start, offset],
+               '--since', $since, from + '^',
+               '--', source]) do |p|

While at it, why not blame -M -CCC?

-C implies -M, but the three times might make sense.

quoted

+  def from_patch(file)

You're not going to 'git mailsplit', like am does, first?

Could be added later.

quoted

+        when /^@@\s-(\d+),(\d+)/
+          get_blame(source, $1, $2, from)

Okay, so this is where you get the arguments for the -L in blame.

quoted

+exit 1 if ARGV.size != 1

No usage?

Later patch.

quoted

+commits.each_person_role do |person, role, count|
+  persons[person][role] = count

Oh dear.  Don't you think you're over-engineering here?  Just because
you can stuff anything into hashes/lists in Ruby, doesn't mean that
you should and kill efficiency.

Write the changes and show me the numbers. Ruby hashes are extremely
efficient, it hardly matters for less than ten people you are going to
Cc, and any other code would be more complicated, if at all possible,
I tried.

quoted

+persons.each do |person, roles|
+  roles = roles.map do |role, count|
+    percent = count.to_f * 100 / commits.size

Interesting.  You also take into account the size of the commits when
calculating the final score.

No, it's the total number of commits, like any array.size.

quoted

+    next if percent < $min_percent
+    "%s: %u%%" % [role, percent]
+  end.compact

What are these nil elements you're filtering out with .compact?

The ones that are below the threshold.

Overall, it looks like the whole thing is one giant first draft
written in a big rush.

It's not. The first draft was sent more than three years ago[1]. I
have literally been developing this for years, fixing related bugs in
git along the way [2], and shaping some features [3]. I sent the
previous version last year, which was basically the same design, and
you thought it was great and even asked why it wasn't merged[4].

I have written and rewritten this code, simplified and refactored it.
And I'm fairly certain this version is clean and tidy, and could
hardly be made any simpler.

For a relatively simple task,

That is the problem right there; you don't understand what the code is
doing, so you think because the code is relatively small, you do
understand it, and the task must be simple. I assure you, it's not.

But you have hands, go ahead and try to simplify it. If the task is so
simple, you can remove all the code you don't like, and see how it
fares. Then you'll see.

I'm sorry, but this giant mess wasn't at all a pleasure to read.

It wasn't meant to be. It was meant to solve a problem in the simplest
way possible.

Surely the code can have some cosmetic changes to explain better what
it's doing, but I doubt you find a way to substantially improve _how_
it's doing it. Certainly none of the suggestions above did help in
that regard.

Cheers.

[1] http://article.gmane.org/gmane.comp.version-control.git/130391
[2] http://article.gmane.org/gmane.comp.version-control.git/189897
[3] http://article.gmane.org/gmane.comp.version-control.git/210059
[4] http://article.gmane.org/gmane.comp.version-control.git/209419

-- 
Felipe Contreras