Repository: mockito/mockito
Issue: Generated release notes contain unsorted and duplicate committers
Labels: continuous integration

Generated release notes contain unsorted and duplicate committers


Issue comments:

Pull request review comments:
@szczepiq you could use [`stripIndent`](http://mrhaki.blogspot.com/2010/06/groovy-goodness-strip-leading-spaces.html) for better readability here

check [out](https://git-scm.com/docs/git-check-mailmap) out git's `.mailmap`, might help in this case

Thanks for hint and review!!!

I didn't know about this command. I've tried it but it does not give me github user id. However, it does seem interesting and it solves the problem of misspelled names / emails. Wanna help, PR! :)

I'll give it a shot next time. Thanks for the tip!!!

I'm so used to used to just multi-line string that my brain does not even register that it is hard to read ;)


Patch that resolved the issue:
diff --git a/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/Commit.java b/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/Commit.java
index ebe7913890..c6d9218bee 100644
--- a/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/Commit.java
+++ b/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/Commit.java
@@ -7,12 +7,12 @@ public interface Commit {
     /**
      * Author identifier. For git it would be 'email'
      */
-    String getAuthorId();
+    String getAuthorEmail();
 
     /**
      * Author display name. For git it would be 'author'
      */
-    String getAuthor();
+    String getAuthorName();
 
     /**
      * Commit message
diff --git a/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/Contribution.java b/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/Contribution.java
index b26290659b..5f83408445 100644
--- a/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/Contribution.java
+++ b/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/Contribution.java
@@ -7,28 +7,31 @@
 class Contribution implements Comparable<Contribution> {
 
     //email identifies the contributor, author alias not necessarily
-    final String authorId;
-    final String author;
+    final String authorEmail;
+    final String authorName;
     final List<Commit> commits = new LinkedList<Commit>();
 
     Contribution(Commit commit) {
-        authorId = commit.getAuthorId();
-        author = commit.getAuthor();
+        authorEmail = commit.getAuthorEmail();
+        authorName = commit.getAuthorName();
         commits.add(commit);
     }
 
     Contribution add(Commit commit) {
-        assert authorId.equals(commit.getAuthorId());
         commits.add(commit);
         return this;
     }
 
     public String toText() {
-        return commits.size() + ": " + author;
+        return commits.size() + ": " + authorName;
     }
 
     public int compareTo(Contribution other) {
-        return Integer.valueOf(other.getCommits().size()).compareTo(commits.size());
+        int byCommitCount = Integer.valueOf(other.getCommits().size()).compareTo(commits.size());
+        if (byCommitCount != 0) {
+            return byCommitCount;
+        }
+        return this.authorName.toUpperCase().compareTo(other.authorName.toUpperCase());
     }
 
     public Collection<Commit> getCommits() {
diff --git a/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/DefaultContributionSet.java b/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/DefaultContributionSet.java
index 65d27c33c0..d0c31fd452 100644
--- a/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/DefaultContributionSet.java
+++ b/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/DefaultContributionSet.java
@@ -5,7 +5,9 @@
 import java.util.*;
 
 class DefaultContributionSet implements ContributionSet {
-    private final Map<String, Contribution> contributions = new HashMap<String, Contribution>();
+
+    private final List<Contribution> contributions = new LinkedList<Contribution>();
+
     private final Collection<Commit> commits = new LinkedList<Commit>();
     private final Predicate<Commit> ignoreCommit;
     private final Set<String> tickets = new LinkedHashSet<String>();
@@ -20,12 +22,29 @@ void add(Commit commit) {
         }
         commits.add(commit);
         tickets.addAll(commit.getTickets());
-        Contribution c = contributions.get(commit.getAuthorId());
-        if (c == null) {
-            contributions.put(commit.getAuthorId(), new Contribution(commit));
+
+        Contribution existing = findContribution(commit, contributions);
+        if (existing != null) {
+            existing.add(commit);
         } else {
-            c.add(commit);
+            contributions.add(new Contribution(commit));
+        }
+    }
+
+    private static Contribution findContribution(Commit commit, Iterable<Contribution> contributions) {
+        for (Contribution c : contributions) {
+            //From Git Log we don't know the GitHub user ID, only the email and name.
+            //Sometimes contributors have different email addresses while the same name
+            //This leads to awkward looking release notes, where same author is shown multiple times
+            //We consider the contribution to be the same if any of: email or name is the same
+            //
+            //This approach comes with a caveat. What if the user have same author name, different email and indeed it is a different user?
+            // This scenario is not handled well but it is unlikely and we consider it a trade-off
+            if (c.authorEmail.equals(commit.getAuthorEmail()) || c.authorName.equals(commit.getAuthorName())) {
+                return c;
+            }
         }
+        return null;
     }
 
     public Collection<Commit> getAllCommits() {
@@ -40,23 +59,11 @@ public String toText() {
         StringBuilder sb = new StringBuilder("* Authors: ").append(contributions.size())
                 .append("\n* Commits: ").append(commits.size());
 
-        for (Map.Entry<String, Contribution> entry : sortByValue(contributions)) {
-            Contribution c = entry.getValue();
+        Collections.sort(contributions);
+        for (Contribution c : contributions) {
             sb.append("\n  * ").append(c.toText());
         }
 
         return sb.toString();
     }
-
-    public static <K, V extends Comparable<V>> List<Map.Entry<K, V>> sortByValue(Map<K, V> map) {
-        List<Map.Entry<K, V>> entries = new ArrayList<Map.Entry<K, V>>(map.entrySet());
-        Collections.sort(entries, new ByValue<K, V>());
-        return entries;
-    }
-
-    private static class ByValue<K, V extends Comparable<V>> implements Comparator<Map.Entry<K, V>> {
-        public int compare(Map.Entry<K, V> o1, Map.Entry<K, V> o2) {
-            return o1.getValue().compareTo(o2.getValue());
-        }
-    }
 }
\ No newline at end of file
diff --git a/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/GitCommit.java b/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/GitCommit.java
index 5c4f4973c2..f1dfc91669 100644
--- a/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/GitCommit.java
+++ b/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/GitCommit.java
@@ -17,11 +17,11 @@ public GitCommit(String email, String author, String message) {
         this.tickets = TicketParser.parseTickets(message);
     }
 
-    public String getAuthorId() {
+    public String getAuthorEmail() {
         return email;
     }
 
-    public String getAuthor() {
+    public String getAuthorName() {
         return author;
     }
 
diff --git a/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/GitContributionsProvider.java b/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/GitContributionsProvider.java
index 93d5d78901..8e5ba8f050 100644
--- a/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/GitContributionsProvider.java
+++ b/buildSrc/src/main/groovy/org/mockito/release/notes/vcs/GitContributionsProvider.java
@@ -32,6 +32,7 @@ public ContributionSet getContributionsBetween(String fromRev, String toRev) {
                 String email = entryParts[0].trim();
                 String author = entryParts[1].trim();
                 String message = entryParts[2].trim();
+                LOG.info("Loaded commit - email: {}, author: {}, message (trimmed): {}", email, author, message.replaceAll("\n.*", ""));
                 contributions.add(new GitCommit(email, author, message));
             }
         }


Test patch:
diff --git a/buildSrc/src/test/groovy/org/mockito/release/notes/vcs/ContributionTest.groovy b/buildSrc/src/test/groovy/org/mockito/release/notes/vcs/ContributionTest.groovy
index 37d0bd0931..152f1ede5f 100644
--- a/buildSrc/src/test/groovy/org/mockito/release/notes/vcs/ContributionTest.groovy
+++ b/buildSrc/src/test/groovy/org/mockito/release/notes/vcs/ContributionTest.groovy
@@ -8,8 +8,8 @@ class ContributionTest extends Specification {
         def c = new Contribution(new GitCommit("a@b", "lad", "m1"))
 
         expect:
-        c.author == "lad"
-        c.authorId == "a@b"
+        c.authorName == "lad"
+        c.authorEmail == "a@b"
         c.toText() == "1: lad"
 
         when: c.add(new GitCommit("a@b", "lad", "m2"))
@@ -32,6 +32,16 @@ class ContributionTest extends Specification {
         set as List == [c3, c2, c1]
     }
 
+    def "sorted by commits and uppercase name"() {
+        def d = new Contribution(new GitCommit("d", "d", "")).add(new GitCommit("d", "d", ""))
+        def c = new Contribution(new GitCommit("c", "c", ""))
+        def b = new Contribution(new GitCommit("B", "B", ""))
+        def a = new Contribution(new GitCommit("a", "a", ""))
+
+        expect:
+        new TreeSet([d, c, b, a]) as List == [d, a, b, c]
+    }
+
     def "has String representation"() {
         def c = new GitCommit("john.doe@gmail.com", "John Doe", "some message")
         expect:
diff --git a/buildSrc/src/test/groovy/org/mockito/release/notes/vcs/DefaultContributionSetTest.groovy b/buildSrc/src/test/groovy/org/mockito/release/notes/vcs/DefaultContributionSetTest.groovy
index bac80a590a..26d73c5220 100644
--- a/buildSrc/src/test/groovy/org/mockito/release/notes/vcs/DefaultContributionSetTest.groovy
+++ b/buildSrc/src/test/groovy/org/mockito/release/notes/vcs/DefaultContributionSetTest.groovy
@@ -49,6 +49,35 @@ class DefaultContributionSetTest extends Specification {
         then:
         c.size() == 1
         c[0].message == "good one"
-        c[0].author == "A"
+        c[0].authorName == "A"
+    }
+
+    def "contributions by same author with different email"() {
+        contributions.add(new GitCommit("john@x", "john", ""))
+        contributions.add(new GitCommit("john@x", "john", ""))
+        contributions.add(new GitCommit("john@y", "john", "")) //same person, different email
+        contributions.add(new GitCommit("x@y", "x", "")) //different person
+
+        expect:
+        contributions.toText() == """* Authors: 2
+* Commits: 4
+  * 3: john
+  * 1: x"""
+    }
+
+    def "contributions sorted by name if number of commits the same"() {
+        contributions.add(new GitCommit("d@d", "d", ""))
+        contributions.add(new GitCommit("d@d", "d", ""))
+        contributions.add(new GitCommit("c@c", "c", ""))
+        contributions.add(new GitCommit("B@B", "B", ""))
+        contributions.add(new GitCommit("a@a", "a", ""))
+
+        expect:
+        contributions.toText() == """* Authors: 4
+* Commits: 5
+  * 2: d
+  * 1: a
+  * 1: B
+  * 1: c"""
     }
 }
diff --git a/buildSrc/src/test/groovy/org/mockito/release/notes/vcs/GitContributionsProviderTest.groovy b/buildSrc/src/test/groovy/org/mockito/release/notes/vcs/GitContributionsProviderTest.groovy
index b074506aea..8027e9344b 100644
--- a/buildSrc/src/test/groovy/org/mockito/release/notes/vcs/GitContributionsProviderTest.groovy
+++ b/buildSrc/src/test/groovy/org/mockito/release/notes/vcs/GitContributionsProviderTest.groovy
@@ -30,8 +30,8 @@ john@doe@@info@@John R. Doe@@info@@dummy commit
 
         and:
         def commits = c.allCommits as List
-        commits[0].author == "Szczepan Faber"
-        commits[0].authorId == "szczepiq@gmail.com"
+        commits[0].authorName == "Szczepan Faber"
+        commits[0].authorEmail == "szczepiq@gmail.com"
         commits[0].message == "Tidy-up in buildSrc\nnext line"
     }