Merge pull request #14889 from issyl0/contributions-improve-committer…

…s-dedup

utils/github: Fix double counting of author/committer numbers

Library/Homebrew/dev-cmd/contributions.rb

@@ -171,14 +171,9 @@ def scan_repositories(repos, person, args)
 
       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(
           "",

Library/Homebrew/test/utils/github_spec.rb

@@ -93,4 +93,56 @@
       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

Library/Homebrew/utils/github.rb

@@ -717,17 +717,30 @@ def multiple_short_commits_exist?(user, repo, commit)
     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