opendev/gerrit
Code Issues Proposed changes

Let users know which commit prevents submit in case of MISSING_DEPENDENCY

Browse Source 
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>
This commit is contained in:
Edwin Kempin
2018-10-02 15:55:02 +02:00
parent 6c39a86bc4
commit 01838a813e
8 changed files with 53 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
java/com/google/gerrit/server/git/CodeReviewCommit.java
View File

@@ -17,11 +17,13 @@ package com.google.gerrit.server.git;
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 @@ public class CodeReviewCommit extends RevCommit {
*/
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 @@ public class CodeReviewCommit extends RevCommit {
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;
}

10
java/com/google/gerrit/server/submit/CommitMergeStatus.java
View File

@@ -67,13 +67,13 @@ public enum CommitMergeStatus {
+ "\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;
}
}

8
java/com/google/gerrit/server/submit/MergeSorter.java
View File

@@ -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.RevCommitList;
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 @@ public class MergeSorter {
// 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);

5
java/com/google/gerrit/server/submit/RebaseSorter.java
View File

@@ -81,9 +81,10 @@ public class RebaseSorter {
// 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.

14
java/com/google/gerrit/server/submit/SubmitStrategyListener.java
View File

@@ -113,7 +113,13 @@ public class SubmitStrategyListener implements BatchUpdateListener {
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 @@ public class SubmitStrategyListener implements BatchUpdateListener {
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:

2
java/com/google/gerrit/server/submit/SubmitStrategyOp.java
View File

@@ -438,7 +438,7 @@ abstract class SubmitStrategyOp implements BatchUpdateOp {
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) {

2
javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
View File

@@ -380,7 +380,7 @@ public class SubmitByCherryPickIT extends AbstractSubmit {
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(

14
javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
View File

@@ -403,7 +403,12 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge {
+ "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 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge {
"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();