Introduce sequential success commit messages
As of now, gerrit will display successfully pushed commits based on
change number.
When user rebased and reordered the commits, this sequence is no longer
correct.
Features in depth:
- Commits list is ordered based on git history.
- New commit is indicated using [NEW].
- Updated and newly created commit no longer grouped.
--------------------------------------------------------
Example of current message:
--------------------------------------------------------
remote: New Changes:
remote: http://192.168.50.1:8080/c/test/+/41 C 5
remote: Updated Changes:
remote: http://192.168.50.1:8080/c/test/+/1 C 2
remote: http://192.168.50.1:8080/c/test/+/2 C 3
remote: http://192.168.50.1:8080/c/test/+/3 C 1
remote: http://192.168.50.1:8080/c/test/+/21 C 4
--------------------------------------------------------
Example of the new sequential message:
--------------------------------------------------------
remote: http://192.168.50.1:8080/c/test/+/3 C 1
remote: http://192.168.50.1:8080/c/test/+/1 C 2
remote: http://192.168.50.1:8080/c/test/+/2 C 3
remote: http://192.168.50.1:8080/c/test/+/21 C 4
remote: http://192.168.50.1:8080/c/test/+/41 C 5 [NEW]
--------------------------------------------------------
Change-Id: I7add03f7990b7d187e89c93b0c76a43a767a7dab
diff --git a/java/com/google/gerrit/server/git/DefaultChangeReportFormatter.java b/java/com/google/gerrit/server/git/DefaultChangeReportFormatter.java
index 1241585..914f402 100644
--- a/java/com/google/gerrit/server/git/DefaultChangeReportFormatter.java
+++ b/java/com/google/gerrit/server/git/DefaultChangeReportFormatter.java
@@ -26,6 +26,7 @@
private static final int SUBJECT_MAX_LENGTH = 80;
private static final String SUBJECT_CROP_APPENDIX = "...";
private static final int SUBJECT_CROP_RANGE = 10;
+ private static final String NEW_CHANGE_INDICATOR = " [NEW]";
private final UrlFormatter urlFormatter;
@@ -36,7 +37,7 @@
@Override
public String newChange(ChangeReportFormatter.Input input) {
- return formatChangeUrl(input);
+ return formatChangeUrl(input) + NEW_CHANGE_INDICATOR;
}
@Override
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 302c73b..8e0615c 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -31,7 +31,6 @@
import static com.google.gerrit.server.git.validators.CommitValidators.NEW_PATCHSET_PATTERN;
import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromFooters;
import static java.nio.charset.StandardCharsets.UTF_8;
-import static java.util.Comparator.comparingInt;
import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.joining;
import static java.util.stream.Collectors.toList;
@@ -173,14 +172,17 @@
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
+import java.util.TreeSet;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;
import java.util.regex.Matcher;
+import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
@@ -691,15 +693,51 @@
/** Appends messages for successful change creation/updates. */
private void queueSuccessMessages(List<CreateRequest> newChanges) {
- List<CreateRequest> created =
- newChanges.stream().filter(r -> r.change != null).collect(toList());
- List<ReplaceRequest> updated =
+ // adjacency list for commit => parent
+ Map<String, String> adjList = new HashMap<>();
+ for (CreateRequest cr : newChanges) {
+ String parent = cr.commit.getParentCount() == 0 ? null : cr.commit.getParent(0).name();
+ adjList.put(cr.commit.name(), parent);
+ }
+ for (ReplaceRequest rr : replaceByChange.values()) {
+ String parent = null;
+ if (rr.revCommit != null) {
+ parent = rr.revCommit.getParentCount() == 0 ? null : rr.revCommit.getParent(0).name();
+ }
+ adjList.put(rr.newCommitId.name(), parent);
+ }
+
+ // get commits that are not parents
+ Set<String> leafs = new TreeSet<>(adjList.keySet());
+ leafs.removeAll(adjList.values());
+ // go backwards from the last commit to its parent(s)
+ Set<String> ordered = new LinkedHashSet<>();
+ for (String leaf : leafs) {
+ if (ordered.contains(leaf)) {
+ continue;
+ }
+ while (leaf != null) {
+ if (!ordered.contains(leaf)) {
+ ordered.add(leaf);
+ }
+ leaf = adjList.get(leaf);
+ }
+ }
+ // reverse the order to start with earliest commit
+ List<String> orderedCommits = new ArrayList<>(ordered);
+ Collections.reverse(orderedCommits);
+
+ Map<String, CreateRequest> created =
+ newChanges
+ .stream()
+ .filter(r -> r.change != null)
+ .collect(Collectors.toMap(r -> r.commit.name(), r -> r));
+ Map<String, ReplaceRequest> updated =
replaceByChange
.values()
.stream()
.filter(r -> r.inputCommand.getResult() == OK)
- .sorted(comparingInt(r -> r.notes.getChangeId().get()))
- .collect(toList());
+ .collect(Collectors.toMap(r -> r.newCommitId.name(), r -> r));
if (created.isEmpty() && updated.isEmpty()) {
return;
@@ -707,63 +745,68 @@
addMessage("");
addMessage("SUCCESS");
+ addMessage("");
- if (!created.isEmpty()) {
- addMessage("");
- addMessage("New Changes:");
- for (CreateRequest c : created) {
- addMessage(
- changeFormatter.newChange(
- ChangeReportFormatter.Input.builder().setChange(c.change).build()));
- }
- }
-
+ boolean edit = false;
+ Boolean isPrivate = null;
+ Boolean wip = null;
if (!updated.isEmpty()) {
- addMessage("");
- addMessage("Updated Changes:");
- boolean edit = magicBranch != null && (magicBranch.edit || magicBranch.draft);
- for (ReplaceRequest u : updated) {
- String subject;
- Change change = u.notes.getChange();
- if (edit) {
- try {
- subject = receivePack.getRevWalk().parseCommit(u.newCommitId).getShortMessage();
- } catch (IOException e) {
- // Log and fall back to original change subject
- logger.atWarning().withCause(e).log("failed to get subject for edit patch set");
- subject = change.getSubject();
- }
- } else {
- subject = u.info.getSubject();
+ edit = magicBranch != null && (magicBranch.edit || magicBranch.draft);
+ if (magicBranch != null) {
+ if (magicBranch.isPrivate) {
+ isPrivate = true;
+ } else if (magicBranch.removePrivate) {
+ isPrivate = false;
}
-
- boolean isPrivate = change.isPrivate();
- boolean wip = change.isWorkInProgress();
- if (magicBranch != null) {
- if (magicBranch.isPrivate) {
- isPrivate = true;
- } else if (magicBranch.removePrivate) {
- isPrivate = false;
- }
- if (magicBranch.workInProgress) {
- wip = true;
- } else if (magicBranch.ready) {
- wip = false;
- }
+ if (magicBranch.workInProgress) {
+ wip = true;
+ } else if (magicBranch.ready) {
+ wip = false;
}
-
- ChangeReportFormatter.Input input =
- ChangeReportFormatter.Input.builder()
- .setChange(change)
- .setSubject(subject)
- .setIsEdit(edit)
- .setIsPrivate(isPrivate)
- .setIsWorkInProgress(wip)
- .build();
- addMessage(changeFormatter.changeUpdated(input));
}
- addMessage("");
}
+
+ for (String commit : orderedCommits) {
+ if (created.get(commit) != null) {
+ addCreatedMessage(created.get(commit));
+ } else if (updated.get(commit) != null) {
+ addReplacedMessage(updated.get(commit), edit, isPrivate, wip);
+ }
+ }
+ addMessage("");
+ }
+
+ private void addCreatedMessage(CreateRequest c) {
+ addMessage(
+ changeFormatter.newChange(
+ ChangeReportFormatter.Input.builder().setChange(c.change).build()));
+ }
+
+ private void addReplacedMessage(ReplaceRequest u, boolean edit, Boolean isPrivate, Boolean wip) {
+ String subject;
+ if (edit) {
+ subject =
+ u.revCommit == null ? u.notes.getChange().getSubject() : u.revCommit.getShortMessage();
+ } else {
+ subject = u.info.getSubject();
+ }
+
+ if (isPrivate == null) {
+ isPrivate = u.notes.getChange().isPrivate();
+ }
+ if (wip == null) {
+ wip = u.notes.getChange().isWorkInProgress();
+ }
+
+ ChangeReportFormatter.Input input =
+ ChangeReportFormatter.Input.builder()
+ .setChange(u.notes.getChange())
+ .setSubject(subject)
+ .setIsEdit(edit)
+ .setIsPrivate(isPrivate)
+ .setIsWorkInProgress(wip)
+ .build();
+ addMessage(changeFormatter.changeUpdated(input));
}
private void insertChangesAndPatchSets(List<CreateRequest> newChanges, Task replaceProgress) {
@@ -2557,6 +2600,7 @@
final ObjectId newCommitId;
final ReceiveCommand inputCommand;
final boolean checkMergedInto;
+ RevCommit revCommit;
ChangeNotes notes;
BiMap<RevCommit, PatchSet.Id> revisions;
PatchSet.Id psId;
@@ -2574,6 +2618,11 @@
this.inputCommand = requireNonNull(cmd);
this.checkMergedInto = checkMergedInto;
+ try {
+ revCommit = receivePack.getRevWalk().parseCommit(newCommitId);
+ } catch (IOException e) {
+ revCommit = null;
+ }
revisions = HashBiMap.create();
for (Ref ref : refs(toChange)) {
try {
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index 143a4a4..3e26cab 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -136,6 +136,7 @@
@Inject private RequestScopeOperations requestScopeOperations;
+ private static String NEW_CHANGE_INDICATOR = " [NEW]";
private LabelType patchSetLock;
@BeforeClass
@@ -376,7 +377,7 @@
r1.assertOkStatus();
r1.assertChange(Change.Status.NEW, null);
r1.assertMessage(
- "New changes:\n " + url + id1 + " " + r1.getCommit().getShortMessage() + "\n");
+ url + id1 + " " + r1.getCommit().getShortMessage() + NEW_CHANGE_INDICATOR + "\n");
testRepo.reset(initialHead);
String newMsg = r1.getCommit().getShortMessage() + " v2";
@@ -396,18 +397,17 @@
r2.assertMessage(
"success\n"
+ "\n"
- + "New changes:\n"
- + " "
- + url
- + id2
- + " another commit\n"
- + "\n"
- + "Updated changes:\n"
+ " "
+ url
+ id1
+ " "
+ newMsg
+ + "\n"
+ + " "
+ + url
+ + id2
+ + " another commit"
+ + NEW_CHANGE_INDICATOR
+ "\n");
}
@@ -898,8 +898,7 @@
assertThat(edit).isPresent();
EditInfo editInfo = edit.get();
r.assertMessage(
- "Updated Changes:\n "
- + canonicalWebUrl.get()
+ canonicalWebUrl.get()
+ "c/"
+ project.get()
+ "/+/"
@@ -2412,6 +2411,74 @@
.contains("warning: " + amended.abbreviate(7).name() + ": no files changed, was rebased");
}
+ @Test
+ public void sequentialCommitMessages() throws Exception {
+ String url = canonicalWebUrl.get() + "c/" + project.get() + "/+/";
+ ObjectId initialHead = testRepo.getRepository().resolve("HEAD");
+
+ PushOneCommit.Result r1 = pushTo("refs/for/master");
+ Change.Id id1 = r1.getChange().getId();
+ r1.assertOkStatus();
+ r1.assertChange(Change.Status.NEW, null);
+ r1.assertMessage(
+ url + id1 + " " + r1.getCommit().getShortMessage() + NEW_CHANGE_INDICATOR + "\n");
+
+ PushOneCommit.Result r2 = pushTo("refs/for/master");
+ Change.Id id2 = r2.getChange().getId();
+ r2.assertOkStatus();
+ r2.assertChange(Change.Status.NEW, null);
+ r2.assertMessage(
+ url + id2 + " " + r2.getCommit().getShortMessage() + NEW_CHANGE_INDICATOR + "\n");
+
+ testRepo.reset(initialHead);
+
+ // rearrange the commit so that change no. 2 is the parent of change no. 1
+ String r1Message = "Position 2";
+ String r2Message = "Position 1";
+ testRepo
+ .branch("HEAD")
+ .commit()
+ .message(r2Message)
+ .insertChangeId(r2.getChangeId().substring(1))
+ .create();
+ testRepo
+ .branch("HEAD")
+ .commit()
+ .message(r1Message)
+ .insertChangeId(r1.getChangeId().substring(1))
+ .create();
+
+ PushOneCommit.Result r3 =
+ pushFactory
+ .create(admin.getIdent(), testRepo, "another commit", "b.txt", "bbb")
+ .to("refs/for/master");
+ Change.Id id3 = r3.getChange().getId();
+ r3.assertOkStatus();
+ r3.assertChange(Change.Status.NEW, null);
+ // should display commit r2, r1, r3 in that order.
+ r3.assertMessage(
+ "success\n"
+ + "\n"
+ + " "
+ + url
+ + id2
+ + " "
+ + r2Message
+ + "\n"
+ + " "
+ + url
+ + id1
+ + " "
+ + r1Message
+ + "\n"
+ + " "
+ + url
+ + id3
+ + " another commit"
+ + NEW_CHANGE_INDICATOR
+ + "\n");
+ }
+
private DraftInput newDraft(String path, int line, String message) {
DraftInput d = new DraftInput();
d.path = path;
