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
This commit is contained in:
parent
25afcccd2e
commit
6964572185
@ -61,6 +61,7 @@ java_test(
|
||||
':client',
|
||||
'//lib:guava',
|
||||
'//lib:junit',
|
||||
'//lib:truth',
|
||||
],
|
||||
)
|
||||
|
||||
|
@ -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<String> {
|
||||
public static final FilenameComparator INSTANCE = new FilenameComparator();
|
||||
|
||||
private static final Set<String> cppHeaderSuffixes = new HashSet<String>(
|
||||
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);
|
||||
}
|
||||
}
|
@ -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);
|
||||
}
|
||||
}
|
@ -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<FileInfo> list) {
|
||||
Collections.sort(Natives.asList(list), new Comparator<FileInfo>() {
|
||||
@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) {
|
||||
|
@ -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;
|
||||
}
|
||||
|
||||
|
Loading…
Reference in New Issue
Block a user