Let users know which commit prevents submit in case of MISSING_DEPENDENCY
If a change cannot be submitted because it depends on a commit that
cannot be merged the user got the following message:
Depends on change that was not submitted.
This message is not very helpful since it doesn't contain which commit
prevents the submit.
Now we return:
Depends on change that was not submitted. Commit <commit> depends on
commit <other-commit> which cannot be merged.
Change-Id: I92db5a74dea6e88bd01b0df21a394ddf21f977b5
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/server/git/CodeReviewCommit.java b/java/com/google/gerrit/server/git/CodeReviewCommit.java
index 6ee5a2a..e7ccd331 100644
--- a/java/com/google/gerrit/server/git/CodeReviewCommit.java
+++ b/java/com/google/gerrit/server/git/CodeReviewCommit.java
@@ -17,11 +17,13 @@
import static com.google.common.base.Preconditions.checkArgument;
import com.google.common.collect.Ordering;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.submit.CommitMergeStatus;
import java.io.IOException;
+import java.util.Optional;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.AnyObjectId;
@@ -118,6 +120,12 @@
*/
private CommitMergeStatus statusCode;
+ /**
+ * Message for the status that is returned to the calling user if the status indicates a problem
+ * that prevents submit.
+ */
+ private Optional<String> statusMessage = Optional.empty();
+
public CodeReviewCommit(AnyObjectId id) {
super(id);
}
@@ -134,6 +142,14 @@
this.statusCode = statusCode;
}
+ public Optional<String> getStatusMessage() {
+ return statusMessage;
+ }
+
+ public void setStatusMessage(@Nullable String statusMessage) {
+ this.statusMessage = Optional.ofNullable(statusMessage);
+ }
+
public PatchSet.Id getPatchsetId() {
return patchsetId;
}
diff --git a/java/com/google/gerrit/server/submit/CommitMergeStatus.java b/java/com/google/gerrit/server/submit/CommitMergeStatus.java
index 35ce175..7f4f4af 100644
--- a/java/com/google/gerrit/server/submit/CommitMergeStatus.java
+++ b/java/com/google/gerrit/server/submit/CommitMergeStatus.java
@@ -67,13 +67,13 @@
+ "\n"
+ "Project policy requires all commits to contain modifications to at least one file.");
- private final String message;
+ private final String description;
- CommitMergeStatus(String message) {
- this.message = message;
+ CommitMergeStatus(String description) {
+ this.description = description;
}
- public String getMessage() {
- return message;
+ public String getDescription() {
+ return description;
}
}
diff --git a/java/com/google/gerrit/server/submit/MergeSorter.java b/java/com/google/gerrit/server/submit/MergeSorter.java
index 14a2913..fa5435b 100644
--- a/java/com/google/gerrit/server/submit/MergeSorter.java
+++ b/java/com/google/gerrit/server/submit/MergeSorter.java
@@ -14,7 +14,6 @@
package com.google.gerrit.server.submit;
-import com.google.common.flogger.FluentLogger;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
import java.io.IOException;
@@ -27,8 +26,6 @@
import org.eclipse.jgit.revwalk.RevFlag;
public class MergeSorter {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
private final CodeReviewRevWalk rw;
private final RevFlag canMergeFlag;
private final Set<RevCommit> accepted;
@@ -65,9 +62,10 @@
// We cannot merge n as it would bring something we
// aren't permitted to merge at this time. Drop n.
//
- logger.atFine().log(
- "commit %s depends on commit %s which cannot be merged", n.name(), c.name());
n.setStatusCode(CommitMergeStatus.MISSING_DEPENDENCY);
+ n.setStatusMessage(
+ String.format(
+ "Commit %s depends on commit %s which cannot be merged.", n.name(), c.name()));
break;
}
contents.add(c);
diff --git a/java/com/google/gerrit/server/submit/RebaseSorter.java b/java/com/google/gerrit/server/submit/RebaseSorter.java
index a2d07fa..bd4f7ea 100644
--- a/java/com/google/gerrit/server/submit/RebaseSorter.java
+++ b/java/com/google/gerrit/server/submit/RebaseSorter.java
@@ -81,9 +81,10 @@
// We cannot merge n as it would bring something we
// aren't permitted to merge at this time. Drop n.
//
- logger.atFine().log(
- "commit %s depends on commit %s which cannot be merged", n.name(), c.name());
n.setStatusCode(CommitMergeStatus.MISSING_DEPENDENCY);
+ n.setStatusMessage(
+ String.format(
+ "Commit %s depends on commit %s which cannot be merged.", n.name(), c.name()));
}
// Stop RevWalk because c is either a merged commit or a missing
// dependency. Not need to walk further.
diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyListener.java b/java/com/google/gerrit/server/submit/SubmitStrategyListener.java
index 9570207..d1f847b 100644
--- a/java/com/google/gerrit/server/submit/SubmitStrategyListener.java
+++ b/java/com/google/gerrit/server/submit/SubmitStrategyListener.java
@@ -113,7 +113,13 @@
commitStatus.problem(id, "internal error: change not processed by merge strategy");
continue;
}
- logger.atFine().log("change %d: status for commit %s is %s", id.get(), commit.name(), s);
+ if (commit.getStatusMessage().isPresent()) {
+ logger.atFine().log(
+ "change %d: Status for commit %s is %s. %s",
+ id.get(), commit.name(), s, commit.getStatusMessage().get());
+ } else {
+ logger.atFine().log("change %d: Status for commit %s is %s.", id.get(), commit.name(), s);
+ }
switch (s) {
case CLEAN_MERGE:
case CLEAN_REBASE:
@@ -136,7 +142,11 @@
case MISSING_DEPENDENCY:
// TODO(dborowitz): Reformat these messages to be more appropriate for
// short problem descriptions.
- commitStatus.problem(id, CharMatcher.is('\n').collapseFrom(s.getMessage(), ' '));
+ String message = s.getDescription();
+ if (commit.getStatusMessage().isPresent()) {
+ message += " " + commit.getStatusMessage().get();
+ }
+ commitStatus.problem(id, CharMatcher.is('\n').collapseFrom(message, ' '));
break;
default:
diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
index 82e3619..98c669f 100644
--- a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
+++ b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java
@@ -438,7 +438,7 @@
private ChangeMessage message(ChangeContext ctx, CodeReviewCommit commit, CommitMergeStatus s)
throws OrmException {
checkNotNull(s, "CommitMergeStatus may not be null");
- String txt = s.getMessage();
+ String txt = s.getDescription();
if (s == CommitMergeStatus.CLEAN_MERGE) {
return message(ctx, commit.getPatchsetId(), txt + getByAccountName());
} else if (s == CommitMergeStatus.CLEAN_REBASE || s == CommitMergeStatus.CLEAN_PICK) {
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
index 2182b2f..8160d9a 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
@@ -380,7 +380,7 @@
ChangeInfo info2 = get(change2.getChangeId(), MESSAGES);
assertThat(info2.status).isEqualTo(ChangeStatus.MERGED);
assertThat(Iterables.getLast(info2.messages).message)
- .isEqualTo(CommitMergeStatus.SKIPPED_IDENTICAL_TREE.getMessage());
+ .isEqualTo(CommitMergeStatus.SKIPPED_IDENTICAL_TREE.getDescription());
assertRefUpdatedEvents(initialHead, headAfterFirstSubmit);
assertChangeMergedEvents(
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
index ca557d5..c606bb0 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
@@ -403,7 +403,12 @@
+ "Change "
+ change3a.getChange().getId()
+ ": Depends on change that"
- + " was not submitted.");
+ + " was not submitted."
+ + " Commit "
+ + change3a.getCommit().name()
+ + " depends on commit "
+ + change2.getCommit().name()
+ + " which cannot be merged.");
RevCommit tipbranch = getRemoteLog(project, "branch").get(0);
assertThat(tipbranch.getShortMessage()).isEqualTo(change1.getCommit().getShortMessage());
@@ -501,7 +506,12 @@
"Failed to submit 1 change due to the following problems:\n"
+ "Change "
+ change3.getPatchSetId().getParentKey().get()
- + ": Depends on change that was not submitted.");
+ + ": Depends on change that was not submitted."
+ + " Commit "
+ + change3.getCommit().name()
+ + " depends on commit "
+ + change2result.getCommit().name()
+ + " which cannot be merged.");
assertRefUpdatedEvents();
assertChangeMergedEvents();
