From 01838a813e787cad438e6bb9c216ec7dd01c19d9 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 2 Oct 2018 15:55:02 +0200 Subject: [PATCH] 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 depends on commit which cannot be merged. Change-Id: I92db5a74dea6e88bd01b0df21a394ddf21f977b5 Signed-off-by: Edwin Kempin --- .../gerrit/server/git/CodeReviewCommit.java | 16 ++++++++++++++++ .../gerrit/server/submit/CommitMergeStatus.java | 10 +++++----- .../google/gerrit/server/submit/MergeSorter.java | 8 +++----- .../gerrit/server/submit/RebaseSorter.java | 5 +++-- .../server/submit/SubmitStrategyListener.java | 14 ++++++++++++-- .../gerrit/server/submit/SubmitStrategyOp.java | 2 +- .../rest/change/SubmitByCherryPickIT.java | 2 +- .../rest/change/SubmitByMergeIfNecessaryIT.java | 14 ++++++++++++-- 8 files changed, 53 insertions(+), 18 deletions(-) diff --git a/java/com/google/gerrit/server/git/CodeReviewCommit.java b/java/com/google/gerrit/server/git/CodeReviewCommit.java index 6ee5a2a158..e7ccd33111 100644 --- a/java/com/google/gerrit/server/git/CodeReviewCommit.java +++ b/java/com/google/gerrit/server/git/CodeReviewCommit.java @@ -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 statusMessage = Optional.empty(); + public CodeReviewCommit(AnyObjectId id) { super(id); } @@ -134,6 +142,14 @@ public class CodeReviewCommit extends RevCommit { this.statusCode = statusCode; } + public Optional 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 35ce175056..7f4f4af0e3 100644 --- a/java/com/google/gerrit/server/submit/CommitMergeStatus.java +++ b/java/com/google/gerrit/server/submit/CommitMergeStatus.java @@ -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; } } diff --git a/java/com/google/gerrit/server/submit/MergeSorter.java b/java/com/google/gerrit/server/submit/MergeSorter.java index 14a2913865..fa5435bc44 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.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 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); diff --git a/java/com/google/gerrit/server/submit/RebaseSorter.java b/java/com/google/gerrit/server/submit/RebaseSorter.java index a2d07fa274..bd4f7eafad 100644 --- a/java/com/google/gerrit/server/submit/RebaseSorter.java +++ b/java/com/google/gerrit/server/submit/RebaseSorter.java @@ -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. diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyListener.java b/java/com/google/gerrit/server/submit/SubmitStrategyListener.java index 9570207f25..d1f847bbd7 100644 --- a/java/com/google/gerrit/server/submit/SubmitStrategyListener.java +++ b/java/com/google/gerrit/server/submit/SubmitStrategyListener.java @@ -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: diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java index 82e3619ded..98c669f147 100644 --- a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java +++ b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java @@ -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) { diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java index 2182b2f328..8160d9a2a6 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 @@ 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( diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java index ca557d549a..c606bb026c 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 @@ 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();