From edeefebf619acc26d6cd7662833398ad77a7c775 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Sun, 5 Mar 2023 14:13:48 +0000 Subject: [PATCH] utils/github: Fix double counting of author/committer numbers - The usage of this in `brew contributions` wasn't correct for a user with 5 authored commits to homebrew/cask that had been committed by other people, the numbers would turn out as 5 authored, 5 committed. - I decided to do this properly by getting the SHAs for author and committer and determine the differences between the two arrays. This also accounts for when authored commits are 0, or committed commits, or both. - Add tests, because I don't want to fix this a third time! --- Library/Homebrew/dev-cmd/contributions.rb | 9 +--- Library/Homebrew/test/utils/github_spec.rb | 52 ++++++++++++++++++++++ Library/Homebrew/utils/github.rb | 19 ++++++-- 3 files changed, 70 insertions(+), 10 deletions(-) diff --git a/Library/Homebrew/dev-cmd/contributions.rb b/Library/Homebrew/dev-cmd/contributions.rb index 9fe19f8b59..ffbb7fafd6 100755 --- a/Library/Homebrew/dev-cmd/contributions.rb +++ b/Library/Homebrew/dev-cmd/contributions.rb @@ -171,14 +171,9 @@ module Homebrew puts "Determining contributions for #{person} on #{repo_full_name}..." if args.verbose? - commits_authored = GitHub.repo_commit_count_for_user(repo_full_name, person, "author", args) - commits_committed = GitHub.repo_commit_count_for_user(repo_full_name, person, "committer", args) - # Only count committers where the author is not the same person. Avoid negative numbers or subtracting zero. - commits_committed = commits_authored - commits_committed if commits_authored > commits_committed - data[repo] = { - author: commits_authored, - committer: commits_committed, + 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: GitHub.count_issues( "", diff --git a/Library/Homebrew/test/utils/github_spec.rb b/Library/Homebrew/test/utils/github_spec.rb index da72e7e90c..cc8765e1f8 100644 --- a/Library/Homebrew/test/utils/github_spec.rb +++ b/Library/Homebrew/test/utils/github_spec.rb @@ -93,4 +93,56 @@ describe GitHub do expect(described_class.pull_request_commits("Homebrew", "legacy-homebrew", 50678, per_page: 1)).to eq(hashes) end end + + describe "::count_repo_commits" do + let(:five_shas) { %w[abcdef ghjkl mnop qrst uvwxyz] } + let(:ten_shas) { %w[abcdef ghjkl mnop qrst uvwxyz fedcba lkjhg ponm tsrq zyxwvu] } + + 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) + + expect(described_class.count_repo_commits("homebrew/cask", "user1", "author", {})).to eq(5) + 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([]) + allow(described_class).to receive(:repo_commits_for_user) + .with("homebrew/core", "user1", "committer", {}).and_return(five_shas) + + expect(described_class.count_repo_commits("homebrew/core", "user1", "committer", {})).to eq(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) + allow(described_class).to receive(:repo_commits_for_user) + .with("homebrew/cask", "user1", "committer", {}).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) + 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) + allow(described_class).to receive(:repo_commits_for_user) + .with("homebrew/cask", "user1", "committer", {}).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) + 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) + allow(described_class).to receive(:repo_commits_for_user) + .with("homebrew/core", "user1", "committer", {}).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) + end + end end diff --git a/Library/Homebrew/utils/github.rb b/Library/Homebrew/utils/github.rb index d4bf5b7027..01ed527f82 100644 --- a/Library/Homebrew/utils/github.rb +++ b/Library/Homebrew/utils/github.rb @@ -717,17 +717,30 @@ module GitHub output[/^Status: (200)/, 1] != "200" end - def repo_commit_count_for_user(nwo, user, filter, args) + def repo_commits_for_user(nwo, user, filter, args) return if Homebrew::EnvConfig.no_github_api? params = ["#{filter}=#{user}"] params << "since=#{DateTime.parse(args.from).iso8601}" if args.from params << "until=#{DateTime.parse(args.to).iso8601}" if args.to - commits = 0 + commits = [] API.paginate_rest("#{API_URL}/repos/#{nwo}/commits", additional_query_params: params.join("&")) do |result| - commits += result.length + commits.concat(result.map { |c| c["sha"] }) end commits end + + def 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" + + committer_shas = repo_commits_for_user(nwo, user, "committer", args) + return 0 if committer_shas.empty? + + # Only count commits where the author and committer are different. + committer_shas.difference(author_shas).count + end end