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

@@ -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,
+        author:        GitHub.count_repo_commits(repo_full_name, person, "author", args),
-        committer:     commits_committed,
+        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 @@ 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

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