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

From: Junio C Hamano <gitster@pobox.com>
Date: May 22, 2013, 19:23 UTC
Message-ID: <7vr4gyizti.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1368978823-18247-1-git-send-email-felipe.contreras@gmail.com>

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

Show 17 quoted lines

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

Show 19 quoted lines

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

Show 22 quoted lines

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

Show 25 quoted lines

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