From 696457218503adf430c2be93bccbc052ae553f80 Mon Sep 17 00:00:00 2001 From: Wyatt Allen Date: Wed, 28 Sep 2016 11:41:47 -0700 Subject: [PATCH] Sort email comment groups In changes involving C++, *.h files should appear before their corresponding *.cc file, although their lexicographic order is the reverse. With this change, the relative order of *.h and *.cpp file pairs in file groups of change comment emails is corrected. Bug: Issue 4586 Change-Id: Ic9618a07167bde570bdfeb0ea54721c763034863 --- gerrit-common/BUCK | 1 + .../common/data/FilenameComparator.java | 64 ++++++++++++++++++ .../common/data/FilenameComparatorTest.java | 65 +++++++++++++++++++ .../google/gerrit/client/info/FileInfo.java | 35 +--------- .../server/mail/send/CommentSender.java | 6 +- 5 files changed, 138 insertions(+), 33 deletions(-) create mode 100644 gerrit-common/src/main/java/com/google/gerrit/common/data/FilenameComparator.java create mode 100644 gerrit-common/src/test/java/com/google/gerrit/common/data/FilenameComparatorTest.java diff --git a/gerrit-common/BUCK b/gerrit-common/BUCK index 452b2fe862..b2ae8b03cf 100644 --- a/gerrit-common/BUCK +++ b/gerrit-common/BUCK @@ -61,6 +61,7 @@ java_test( ':client', '//lib:guava', '//lib:junit', + '//lib:truth', ], ) diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/FilenameComparator.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/FilenameComparator.java new file mode 100644 index 0000000000..08a09b7b4e --- /dev/null +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/FilenameComparator.java @@ -0,0 +1,64 @@ +// Copyright (C) 2016 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.common.data; + +import com.google.gerrit.reviewdb.client.Patch; + +import java.util.Arrays; +import java.util.Comparator; +import java.util.HashSet; +import java.util.Set; + +public class FilenameComparator implements Comparator { + public static final FilenameComparator INSTANCE = new FilenameComparator(); + + private static final Set cppHeaderSuffixes = new HashSet( + Arrays.asList(".h", ".hxx", ".hpp")); + + private FilenameComparator() {} + + @Override + public int compare(final String path1, final String path2) { + if (Patch.COMMIT_MSG.equals(path1) && Patch.COMMIT_MSG.equals(path2)) { + return 0; + } else if (Patch.COMMIT_MSG.equals(path1)) { + return -1; + } else if (Patch.COMMIT_MSG.equals(path2)) { + return 1; + } + if (Patch.MERGE_LIST.equals(path1) && Patch.MERGE_LIST.equals(path2)) { + return 0; + } else if (Patch.MERGE_LIST.equals(path1)) { + return -1; + } else if (Patch.MERGE_LIST.equals(path2)) { + return 1; + } + + int s1 = path1.lastIndexOf('.'); + int s2 = path2.lastIndexOf('.'); + if (s1 > 0 && s2 > 0 && + path1.substring(0, s1).equals(path2.substring(0, s2))) { + String suffixA = path1.substring(s1); + String suffixB = path2.substring(s2); + // C++ and C: give priority to header files (.h/.hpp/...) + if (cppHeaderSuffixes.contains(suffixA)) { + return -1; + } else if (cppHeaderSuffixes.contains(suffixB)) { + return 1; + } + } + return path1.compareTo(path2); + } +} diff --git a/gerrit-common/src/test/java/com/google/gerrit/common/data/FilenameComparatorTest.java b/gerrit-common/src/test/java/com/google/gerrit/common/data/FilenameComparatorTest.java new file mode 100644 index 0000000000..ef8f0a9ebe --- /dev/null +++ b/gerrit-common/src/test/java/com/google/gerrit/common/data/FilenameComparatorTest.java @@ -0,0 +1,65 @@ +// Copyright (C) 2016 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.common.data; + +import static com.google.common.truth.Truth.assertThat; + +import org.junit.Test; + +public class FilenameComparatorTest { + private FilenameComparator comparator = FilenameComparator.INSTANCE; + + @Test + public void basicPaths() { + assertThat(comparator.compare( + "abc/xyz/FileOne.java", "xyz/abc/FileTwo.java")).isLessThan(0); + assertThat(comparator.compare( + "abc/xyz/FileOne.java", "abc/xyz/FileOne.java")).isEqualTo(0); + assertThat(comparator.compare( + "zzz/yyy/FileOne.java", "abc/xyz/FileOne.java")).isGreaterThan(0); + } + + @Test + public void specialPaths() { + assertThat(comparator.compare( + "ABC/xyz/FileOne.java", "/COMMIT_MSG")).isGreaterThan(0); + assertThat(comparator.compare( + "/COMMIT_MSG", "ABC/xyz/FileOne.java")).isLessThan(0); + + assertThat(comparator.compare( + "ABC/xyz/FileOne.java", "/MERGE_LIST")).isGreaterThan(0); + assertThat(comparator.compare( + "/MERGE_LIST", "ABC/xyz/FileOne.java")).isLessThan(0); + + assertThat(comparator.compare( + "/COMMIT_MSG", "/MERGE_LIST")).isLessThan(0); + assertThat(comparator.compare( + "/MERGE_LIST", "/COMMIT_MSG")).isGreaterThan(0); + + assertThat(comparator.compare( + "/COMMIT_MSG", "/COMMIT_MSG")).isEqualTo(0); + assertThat(comparator.compare( + "/MERGE_LIST", "/MERGE_LIST")).isEqualTo(0); + } + + @Test + public void cppExtensions() { + assertThat(comparator.compare("abc/file.h", "abc/file.cc")).isLessThan(0); + assertThat(comparator.compare("abc/file.c", "abc/file.hpp")) + .isGreaterThan(0); + assertThat(comparator.compare("abc..xyz.file.h", "abc.xyz.file.cc")) + .isLessThan(0); + } +} \ No newline at end of file diff --git a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/FileInfo.java b/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/FileInfo.java index b21126d5d8..e557470584 100644 --- a/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/FileInfo.java +++ b/gerrit-gwtui-common/src/main/java/com/google/gerrit/client/info/FileInfo.java @@ -15,6 +15,7 @@ package com.google.gerrit.client.info; import com.google.gerrit.client.rpc.Natives; +import com.google.gerrit.common.data.FilenameComparator; import com.google.gerrit.reviewdb.client.Patch; import com.google.gwt.core.client.JavaScriptObject; import com.google.gwt.core.client.JsArray; @@ -30,7 +31,6 @@ public class FileInfo extends JavaScriptObject { public final native boolean binary() /*-{ return this.binary || false; }-*/; public final native String status() /*-{ return this.status; }-*/; - // JSNI methods cannot have 'long' as a parameter type or a return type and // it's suggested to use double in this case: // http://www.gwtproject.org/doc/latest/DevGuideCodingBasicsJSNI.html#important @@ -48,37 +48,8 @@ public class FileInfo extends JavaScriptObject { public final native void _row(int r) /*-{ this._row = r }-*/; public static void sortFileInfoByPath(JsArray list) { - Collections.sort(Natives.asList(list), new Comparator() { - @Override - public int compare(FileInfo a, FileInfo b) { - if (Patch.COMMIT_MSG.equals(a.path())) { - return -1; - } else if (Patch.COMMIT_MSG.equals(b.path())) { - return 1; - } - if (Patch.MERGE_LIST.equals(a.path())) { - return -1; - } else if (Patch.MERGE_LIST.equals(b.path())) { - return 1; - } - - // Look at file suffixes to check if it makes sense to use a different order - int s1 = a.path().lastIndexOf('.'); - int s2 = b.path().lastIndexOf('.'); - if (s1 > 0 && s2 > 0 && - a.path().substring(0, s1).equals(b.path().substring(0, s2))) { - String suffixA = a.path().substring(s1); - String suffixB = b.path().substring(s2); - // C++ and C: give priority to header files (.h/.hpp/...) - if (suffixA.indexOf(".h") == 0) { - return -1; - } else if (suffixB.indexOf(".h") == 0) { - return 1; - } - } - return a.path().compareTo(b.path()); - } - }); + Collections.sort(Natives.asList(list), + Comparator.comparing(FileInfo::path, FilenameComparator.INSTANCE)); } public static String getFileName(String path) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CommentSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CommentSender.java index 35eea03a6d..77086f71dc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CommentSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CommentSender.java @@ -1,4 +1,4 @@ -// Copyright (C) 2009 The Android Open Source Project +// Copyright (C) 2016 The Android Open Source Project // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -16,6 +16,7 @@ package com.google.gerrit.server.mail.send; import com.google.common.base.Strings; import com.google.common.collect.Ordering; +import com.google.gerrit.common.data.FilenameComparator; import com.google.gerrit.common.errors.EmailException; import com.google.gerrit.common.errors.NoSuchEntityException; import com.google.gerrit.extensions.api.changes.NotifyHandling; @@ -41,6 +42,7 @@ import org.slf4j.LoggerFactory; import java.io.IOException; import java.util.ArrayList; import java.util.Collections; +import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -238,6 +240,8 @@ public class CommentSender extends ReplyToChangeSender { } } + Collections.sort(groups, + Comparator.comparing(g -> g.filename, FilenameComparator.INSTANCE)); return groups; }