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

[Junio C Hamano <gitster@pobox.com>]

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

> diff --git a/contrib/related/git-related b/contrib/related/git-related
> new file mode 100755
> index 0000000..b96dcdd
> --- /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)
> +  '%s <%s>' % [name, email]
> +end

Micronit.  I suspect you do not need this helper, unless later
patches start using it.

> +  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', '-CCC',

I am torn on the hardcoded use of "-CCC" here.

Depending on the nature of the change in question, it may match well
or worse to what you are trying to find out.  When you are trying to
say "What were you smoking when you implemented this broken logic?",
using -C may be good, but when your question is "Even though all the
callers of this function live in that other file, somebody moved
this function that used to be file static in that file to here and
made it public. Why?", you do not want to use -C.

I am reasonably sure that in the finished code later in the series
it will become configurable, but a fallback default is better to be
not so expensive one.

> +               '-L', '%u,+%u' % [start, len],
> +               '--since', $since, from + '^',

Is "from" unconditionally set?

Perhaps that nil + '^' magically disappear and this code is relying
on that, but it smells like a too much magic to me.

> +               '--', 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)
> +        end

Makes sense to start from the preimage so that you can find out who
wrote the original block of lines your patch is removing.

But then if source is /dev/null, wouldn't you be able to stop
without running blame at all?  You know the patch is creating a new
file at that point and there is nobody to point a finger at.

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