Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dev-cmd/contributions: usability/performance improvements. #15923

Merged
merged 1 commit into from
Aug 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 63 additions & 67 deletions Library/Homebrew/dev-cmd/contributions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,28 +13,32 @@
OFFICIAL_CMD_TAPS.keys.map { |t| t.delete_prefix("homebrew/") },
OFFICIAL_CASK_TAPS.reject { |t| t == "cask" },
].flatten.freeze
MAX_REPO_COMMITS = 1000

sig { returns(CLI::Parser) }
def contributions_args
Homebrew::CLI::Parser.new do
usage_banner "`contributions` [--user=<email|username>] [<--repositories>`=`] [<--csv>]"
description <<~EOS
Contributions to Homebrew repos.
Contributions to Homebrew repositories.
EOS

comma_array "--repositories",
description: "Specify a comma-separated (no spaces) list of repositories to search. " \
description: "Specify a comma-separated list of repositories to search. " \
"Supported repositories: #{SUPPORTED_REPOS.map { |t| "`#{t}`" }.to_sentence}. " \
"Omitting this flag, or specifying `--repositories=all`, searches all repositories. " \
"Use `--repositories=primary` to search only the main repositories: brew,core,cask."
"Omitting this flag, or specifying `--repositories=primary`, searches only the " \
"main repositories: brew,core,cask. " \
"Specifying `--repositories=all`, searches all repositories. "
flag "--from=",
description: "Date (ISO-8601 format) to start searching contributions."
description: "Date (ISO-8601 format) to start searching contributions. " \
"Omitting this flag searches the last year."

flag "--to=",
description: "Date (ISO-8601 format) to stop searching contributions."

flag "--user=",
description: "A GitHub username or email address of a specific person to find contribution data for."
comma_array "--user=",
description: "Specify a comma-separated list of GitHub usernames or email addresses to find " \
"contributions from. Omitting this flag searches maintainers."

switch "--csv",
description: "Print a CSV of contributions across repositories over the time period."
Expand All @@ -48,41 +52,47 @@
results = {}
grand_totals = {}

all_repos = args.repositories.nil? || args.repositories.include?("all")
repos = if all_repos
SUPPORTED_REPOS
elsif args.repositories.include?("primary")
repos = if args.repositories.blank? || args.repositories.include?("primary")

Check warning on line 55 in Library/Homebrew/dev-cmd/contributions.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/contributions.rb#L55

Added line #L55 was not covered by tests
PRIMARY_REPOS
elsif args.repositories.include?("all")
SUPPORTED_REPOS
else
args.repositories
end

if args.user
user = args.user
results[user] = scan_repositories(repos, user, args)
grand_totals[user] = total(results[user])
from = args.from.presence || Date.today.prev_year.iso8601

Check warning on line 63 in Library/Homebrew/dev-cmd/contributions.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/contributions.rb#L63

Added line #L63 was not covered by tests

puts "#{user} contributed #{Utils.pluralize("time", grand_totals[user].values.sum,
include_count: true)} #{time_period(args)}."
puts generate_csv(T.must(user), results[user], grand_totals[user]) if args.csv?
return
end
contribution_types = [:author, :committer, :coauthorship, :review]

Check warning on line 65 in Library/Homebrew/dev-cmd/contributions.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/contributions.rb#L65

Added line #L65 was not covered by tests

maintainers = GitHub.members_by_team("Homebrew", "maintainers")
maintainers.each do |username, _|
users = args.user.presence || GitHub.members_by_team("Homebrew", "maintainers")
users.each do |username, _|

Check warning on line 68 in Library/Homebrew/dev-cmd/contributions.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/contributions.rb#L67-L68

Added lines #L67 - L68 were not covered by tests
# TODO: Using the GitHub username to scan the `git log` undercounts some
# contributions as people might not always have configured their Git
# committer details to match the ones on GitHub.
# TODO: Switch to using the GitHub APIs instead of `git log` if
# they ever support trailers.
results[username] = scan_repositories(repos, username, args)
results[username] = scan_repositories(repos, username, args, from: from)

Check warning on line 74 in Library/Homebrew/dev-cmd/contributions.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/contributions.rb#L74

Added line #L74 was not covered by tests
grand_totals[username] = total(results[username])

puts "#{username} contributed #{Utils.pluralize("time", grand_totals[username].values.sum,
include_count: true)} #{time_period(args)}."
contributions = contribution_types.map do |type|
type_count = grand_totals[username][type]

Check warning on line 78 in Library/Homebrew/dev-cmd/contributions.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/contributions.rb#L77-L78

Added lines #L77 - L78 were not covered by tests
next if type_count.to_i.zero?

"#{Utils.pluralize("time", type_count, include_count: true)} (#{type})"

Check warning on line 81 in Library/Homebrew/dev-cmd/contributions.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/contributions.rb#L81

Added line #L81 was not covered by tests
end.compact
contributions << "#{Utils.pluralize("time", grand_totals[username].values.sum, include_count: true)} (total)"

Check warning on line 83 in Library/Homebrew/dev-cmd/contributions.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/contributions.rb#L83

Added line #L83 was not covered by tests

puts [

Check warning on line 85 in Library/Homebrew/dev-cmd/contributions.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/contributions.rb#L85

Added line #L85 was not covered by tests
"#{username} contributed",
*contributions.to_sentence,
"#{time_period(from: from, to: args.to)}.",
].join(" ")
end

puts generate_maintainers_csv(grand_totals) if args.csv?
return unless args.csv?

puts
puts generate_csv(grand_totals)

Check warning on line 95 in Library/Homebrew/dev-cmd/contributions.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/contributions.rb#L94-L95

Added lines #L94 - L95 were not covered by tests
end

sig { params(repo: String).returns(Pathname) }
Expand All @@ -92,63 +102,44 @@
Tap.fetch("homebrew", repo).path
end

sig { params(args: Homebrew::CLI::Args).returns(String) }
def time_period(args)
if args.from && args.to
"between #{args.from} and #{args.to}"
elsif args.from
"after #{args.from}"
elsif args.to
"before #{args.to}"
sig { params(from: T.nilable(String), to: T.nilable(String)).returns(String) }
def time_period(from:, to:)
if from && to

Check warning on line 107 in Library/Homebrew/dev-cmd/contributions.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/contributions.rb#L107

Added line #L107 was not covered by tests
"between #{from} and #{to}"
elsif from
"after #{from}"
elsif to
"before #{to}"
else
"in all time"
end
end

sig { params(totals: Hash).returns(String) }
def generate_maintainers_csv(totals)
def generate_csv(totals)
CSV.generate do |csv|
csv << %w[user repo author committer coauthorships reviews total]
csv << %w[user repo author committer coauthorship review total]

Check warning on line 121 in Library/Homebrew/dev-cmd/contributions.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/contributions.rb#L121

Added line #L121 was not covered by tests

totals.sort_by { |_, v| -v.values.sum }.each do |user, total|
csv << grand_total_row(user, total)
end
end
end

sig { params(user: String, results: Hash, grand_total: Hash).returns(String) }
def generate_csv(user, results, grand_total)
CSV.generate do |csv|
csv << %w[user repo author committer coauthorships reviews total]
results.each do |repo, counts|
csv << [
user,
repo,
counts[:author],
counts[:committer],
counts[:coauthorships],
counts[:reviews],
counts.values.sum,
]
end
csv << grand_total_row(user, grand_total)
end
end

sig { params(user: String, grand_total: Hash).returns(Array) }
def grand_total_row(user, grand_total)
[
user,
"all",
grand_total[:author],
grand_total[:committer],
grand_total[:coauthorships],
grand_total[:reviews],
grand_total[:coauthorship],
grand_total[:review],
grand_total.values.sum,
]
end

def scan_repositories(repos, person, args)
def scan_repositories(repos, person, args, from:)
data = {}

repos.each do |repo|
Expand All @@ -171,11 +162,13 @@

puts "Determining contributions for #{person} on #{repo_full_name}..." if args.verbose?

author_commits, committer_commits = GitHub.count_repo_commits(repo_full_name, person, args,

Check warning on line 165 in Library/Homebrew/dev-cmd/contributions.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/contributions.rb#L165

Added line #L165 was not covered by tests
max: MAX_REPO_COMMITS)
data[repo] = {
author: GitHub.count_repo_commits(repo_full_name, person, "author", args),
committer: GitHub.count_repo_commits(repo_full_name, person, "committer", args),
coauthorships: git_log_trailers_cmd(T.must(repo_path), person, "Co-authored-by", args),
reviews: count_reviews(repo_full_name, person, args),
author: author_commits,
committer: committer_commits,
coauthorship: git_log_trailers_cmd(T.must(repo_path), person, "Co-authored-by", from: from, to: args.to),
review: count_reviews(repo_full_name, person, args),
}
end

Expand All @@ -184,7 +177,7 @@

sig { params(results: Hash).returns(Hash) }
def total(results)
totals = { author: 0, committer: 0, coauthorships: 0, reviews: 0 }
totals = { author: 0, committer: 0, coauthorship: 0, review: 0 }

Check warning on line 180 in Library/Homebrew/dev-cmd/contributions.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/contributions.rb#L180

Added line #L180 was not covered by tests

results.each_value do |counts|
counts.each do |kind, count|
Expand All @@ -195,12 +188,15 @@
totals
end

sig { params(repo_path: Pathname, person: String, trailer: String, args: Homebrew::CLI::Args).returns(Integer) }
def git_log_trailers_cmd(repo_path, person, trailer, args)
sig {
params(repo_path: Pathname, person: String, trailer: String, from: T.nilable(String),

Check warning on line 192 in Library/Homebrew/dev-cmd/contributions.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/dev-cmd/contributions.rb#L192

Added line #L192 was not covered by tests
to: T.nilable(String)).returns(Integer)
}
def git_log_trailers_cmd(repo_path, person, trailer, from:, to:)
cmd = ["git", "-C", repo_path, "log", "--oneline"]
cmd << "--format='%(trailers:key=#{trailer}:)'"
cmd << "--before=#{args.to}" if args.to
cmd << "--after=#{args.from}" if args.from
cmd << "--before=#{to}" if to
cmd << "--after=#{from}" if from

Utils.safe_popen_read(*cmd).lines.count { |l| l.include?(person) }
end
Expand Down
33 changes: 16 additions & 17 deletions Library/Homebrew/test/utils/github_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -90,49 +90,48 @@

it "counts commits authored by a user" do
allow(described_class).to receive(:repo_commits_for_user)
.with("homebrew/cask", "user1", "author", {}).and_return(five_shas)
.with("homebrew/cask", "user1", "author", {}, nil).and_return(five_shas)
allow(described_class).to receive(:repo_commits_for_user)
.with("homebrew/cask", "user1", "committer", {}, nil).and_return([])

expect(described_class.count_repo_commits("homebrew/cask", "user1", "author", {})).to eq(5)
expect(described_class.count_repo_commits("homebrew/cask", "user1", {})).to eq([5, 0])
end

it "counts commits committed by a user" do
allow(described_class).to receive(:repo_commits_for_user)
.with("homebrew/core", "user1", "author", {}).and_return([])
.with("homebrew/core", "user1", "author", {}, nil).and_return([])
allow(described_class).to receive(:repo_commits_for_user)
.with("homebrew/core", "user1", "committer", {}).and_return(five_shas)
.with("homebrew/core", "user1", "committer", {}, nil).and_return(five_shas)

expect(described_class.count_repo_commits("homebrew/core", "user1", "committer", {})).to eq(5)
expect(described_class.count_repo_commits("homebrew/core", "user1", {})).to eq([0, 5])
end

it "calculates correctly when authored > committed with different shas" do
allow(described_class).to receive(:repo_commits_for_user)
.with("homebrew/cask", "user1", "author", {}).and_return(ten_shas)
.with("homebrew/cask", "user1", "author", {}, nil).and_return(ten_shas)
allow(described_class).to receive(:repo_commits_for_user)
.with("homebrew/cask", "user1", "committer", {}).and_return(%w[1 2 3 4 5])
.with("homebrew/cask", "user1", "committer", {}, nil).and_return(%w[1 2 3 4 5])

expect(described_class.count_repo_commits("homebrew/cask", "user1", "author", {})).to eq(10)
expect(described_class.count_repo_commits("homebrew/cask", "user1", "committer", {})).to eq(5)
expect(described_class.count_repo_commits("homebrew/cask", "user1", {})).to eq([10, 5])
end

it "calculates correctly when committed > authored" do
allow(described_class).to receive(:repo_commits_for_user)
.with("homebrew/cask", "user1", "author", {}).and_return(five_shas)
.with("homebrew/cask", "user1", "author", {}, nil).and_return(five_shas)
allow(described_class).to receive(:repo_commits_for_user)
.with("homebrew/cask", "user1", "committer", {}).and_return(ten_shas)
.with("homebrew/cask", "user1", "committer", {}, nil).and_return(ten_shas)

expect(described_class.count_repo_commits("homebrew/cask", "user1", "author", {})).to eq(5)
expect(described_class.count_repo_commits("homebrew/cask", "user1", "committer", {})).to eq(5)
expect(described_class.count_repo_commits("homebrew/cask", "user1", {})).to eq([5, 5])
end

it "deduplicates commits authored and committed by the same user" do
allow(described_class).to receive(:repo_commits_for_user)
.with("homebrew/core", "user1", "author", {}).and_return(five_shas)
.with("homebrew/core", "user1", "author", {}, nil).and_return(five_shas)
allow(described_class).to receive(:repo_commits_for_user)
.with("homebrew/core", "user1", "committer", {}).and_return(five_shas)
.with("homebrew/core", "user1", "committer", {}, nil).and_return(five_shas)

# Because user1 authored and committed the same 5 commits.
expect(described_class.count_repo_commits("homebrew/core", "user1", "author", {})).to eq(5)
expect(described_class.count_repo_commits("homebrew/core", "user1", "committer", {})).to eq(0)
expect(described_class.count_repo_commits("homebrew/core", "user1", {})).to eq([5, 0])
end
end
end
23 changes: 14 additions & 9 deletions Library/Homebrew/utils/github.rb
Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,7 @@
output[/^Status: (200)/, 1] != "200"
end

def self.repo_commits_for_user(nwo, user, filter, args)
def self.repo_commits_for_user(nwo, user, filter, args, max)
return if Homebrew::EnvConfig.no_github_api?

params = ["#{filter}=#{user}"]
Expand All @@ -768,20 +768,25 @@
commits = []
API.paginate_rest("#{API_URL}/repos/#{nwo}/commits", additional_query_params: params.join("&")) do |result|
commits.concat(result.map { |c| c["sha"] })
if max.present? && commits.length >= max
opoo "#{user} exceeded #{max} #{nwo} commits as #{filter}, stopped counting!"
break

Check warning on line 773 in Library/Homebrew/utils/github.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github.rb#L773

Added line #L773 was not covered by tests
end
end
commits
end

def self.count_repo_commits(nwo, user, filter, args)
return if Homebrew::EnvConfig.no_github_api?

author_shas = repo_commits_for_user(nwo, user, "author", args)
return author_shas.count if filter == "author"
def self.count_repo_commits(nwo, user, args, max: nil)
odie "Cannot count commits, HOMEBREW_NO_GITHUB_API set!" if Homebrew::EnvConfig.no_github_api?

committer_shas = repo_commits_for_user(nwo, user, "committer", args)
return 0 if committer_shas.empty?
author_shas = repo_commits_for_user(nwo, user, "author", args, max)
committer_shas = repo_commits_for_user(nwo, user, "committer", args, max)
return [0, 0] if author_shas.blank? && committer_shas.blank?

author_count = author_shas.count
# Only count commits where the author and committer are different.
committer_shas.difference(author_shas).count
committer_count = committer_shas.difference(author_shas).count

[author_count, committer_count]
end
end
13 changes: 12 additions & 1 deletion Library/Homebrew/utils/github/api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
API_URL = "https://api.github.com"
API_MAX_PAGES = 50
API_MAX_ITEMS = 5000
PAGINATE_RETRY_COUNT = 3

CREATE_GIST_SCOPES = ["gist"].freeze
CREATE_ISSUE_FORK_OR_PR_SCOPES = ["repo"].freeze
Expand Down Expand Up @@ -281,7 +282,17 @@

def self.paginate_rest(url, additional_query_params: nil, per_page: 100)
(1..API_MAX_PAGES).each do |page|
result = API.open_rest("#{url}?per_page=#{per_page}&page=#{page}&#{additional_query_params}")
retry_count = 1

Check warning on line 285 in Library/Homebrew/utils/github/api.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github/api.rb#L285

Added line #L285 was not covered by tests
result = begin
API.open_rest("#{url}?per_page=#{per_page}&page=#{page}&#{additional_query_params}")

Check warning on line 287 in Library/Homebrew/utils/github/api.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github/api.rb#L287

Added line #L287 was not covered by tests
rescue Error
if retry_count < PAGINATE_RETRY_COUNT
retry_count += 1
retry

Check warning on line 291 in Library/Homebrew/utils/github/api.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github/api.rb#L291

Added line #L291 was not covered by tests
end

raise

Check warning on line 294 in Library/Homebrew/utils/github/api.rb

View check run for this annotation

Codecov / codecov/patch

Library/Homebrew/utils/github/api.rb#L294

Added line #L294 was not covered by tests
end
break if result.blank?

yield(result, page)
Expand Down
8 changes: 4 additions & 4 deletions completions/fish/brew.fish
Original file line number Diff line number Diff line change
Expand Up @@ -528,15 +528,15 @@ __fish_brew_complete_arg 'config' -l quiet -d 'Make some output more quiet'
__fish_brew_complete_arg 'config' -l verbose -d 'Make some output more verbose'


__fish_brew_complete_cmd 'contributions' 'Contributions to Homebrew repos'
__fish_brew_complete_cmd 'contributions' 'Contributions to Homebrew repositories'
__fish_brew_complete_arg 'contributions' -l csv -d 'Print a CSV of contributions across repositories over the time period'
__fish_brew_complete_arg 'contributions' -l debug -d 'Display any debugging information'
__fish_brew_complete_arg 'contributions' -l from -d 'Date (ISO-8601 format) to start searching contributions'
__fish_brew_complete_arg 'contributions' -l from -d 'Date (ISO-8601 format) to start searching contributions. Omitting this flag searches the last year'
__fish_brew_complete_arg 'contributions' -l help -d 'Show this message'
__fish_brew_complete_arg 'contributions' -l quiet -d 'Make some output more quiet'
__fish_brew_complete_arg 'contributions' -l repositories -d 'Specify a comma-separated (no spaces) list of repositories to search. Supported repositories: `brew`, `core`, `cask`, `aliases`, `autoupdate`, `bundle`, `command-not-found`, `test-bot`, `services`, `cask-fonts` and `cask-versions`. Omitting this flag, or specifying `--repositories=all`, searches all repositories. Use `--repositories=primary` to search only the main repositories: brew,core,cask'
__fish_brew_complete_arg 'contributions' -l repositories -d 'Specify a comma-separated list of repositories to search. Supported repositories: `brew`, `core`, `cask`, `aliases`, `autoupdate`, `bundle`, `command-not-found`, `test-bot`, `services`, `cask-fonts` and `cask-versions`. Omitting this flag, or specifying `--repositories=primary`, searches only the main repositories: brew,core,cask. Specifying `--repositories=all`, searches all repositories. '
__fish_brew_complete_arg 'contributions' -l to -d 'Date (ISO-8601 format) to stop searching contributions'
__fish_brew_complete_arg 'contributions' -l user -d 'A GitHub username or email address of a specific person to find contribution data for'
__fish_brew_complete_arg 'contributions' -l user -d 'Specify a comma-separated list of GitHub usernames or email addresses to find contributions from. Omitting this flag searches maintainers'
__fish_brew_complete_arg 'contributions' -l verbose -d 'Make some output more verbose'


Expand Down
Loading