If submit fails due to dependency on non-visible change include calling user into message
Submitting a change can fail if the commit that should be merged depends
on a commit that belongs to change that is not visible to the calling
user. In this case the following message is returned as part of the 409
response for the submit request:
Change <change-number>: Depends on change that was not submitted.
Commit <sha1-commit-1> depends on commit <sha1-commit-2> which cannot
be merged. Is the change of this commit not visible or was it
deleted?
With this message alone it’s difficult to investigate the problem
because it doesn’t include the calling user, the user for which the
change is not visible. Change it to
Change <change-number>: Depends on change that was not submitted.
Commit <sha1-commit-1> depends on commit <sha1-commit-2> which cannot
be merged. Is the change of this commit not visible to <calling-user>
or was it deleted?
Dry-run submits are executed in the background and hence there is no
current user available. This doesn’t matter since for dry-run submits
the message is anyway ignored.
Change-Id: I8ff1ad7041c9632b3fdbfdba19d83c984a776a01
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java b/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java
index 1ac558b..d408519 100644
--- a/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java
+++ b/java/com/google/gerrit/server/change/MergeabilityCacheImpl.java
@@ -207,7 +207,7 @@
accepted.add(rw.parseCommit(key.into));
accepted.addAll(Arrays.asList(rw.parseCommit(key.commit).getParents()));
return submitDryRun.run(
- key.submitType, repo, rw, dest, key.into, key.commit, accepted);
+ null, key.submitType, repo, rw, dest, key.into, key.commit, accepted);
}
});
} catch (ExecutionException | UncheckedExecutionException e) {
diff --git a/java/com/google/gerrit/server/query/change/ConflictsPredicate.java b/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
index 7a399f2..dfbc221 100644
--- a/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
+++ b/java/com/google/gerrit/server/query/change/ConflictsPredicate.java
@@ -129,6 +129,7 @@
CodeReviewRevWalk rw = CodeReviewCommit.newRevWalk(repo)) {
boolean conflicts =
!args.submitDryRun.run(
+ null,
str.type,
repo,
rw,
diff --git a/java/com/google/gerrit/server/submit/CommitMergeStatus.java b/java/com/google/gerrit/server/submit/CommitMergeStatus.java
index 3c7b986..b101898 100644
--- a/java/com/google/gerrit/server/submit/CommitMergeStatus.java
+++ b/java/com/google/gerrit/server/submit/CommitMergeStatus.java
@@ -16,7 +16,9 @@
import static java.util.stream.Collectors.toSet;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.PatchSet;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gwtorm.server.OrmException;
@@ -88,15 +90,18 @@
}
public static String createMissingDependencyMessage(
- Provider<InternalChangeQuery> queryProvider, String commit, String otherCommit)
+ @Nullable CurrentUser caller,
+ Provider<InternalChangeQuery> queryProvider,
+ String commit,
+ String otherCommit)
throws OrmException {
List<ChangeData> changes = queryProvider.get().enforceVisibility(true).byCommit(otherCommit);
if (changes.isEmpty()) {
return String.format(
"Commit %s depends on commit %s which cannot be merged."
- + " Is the change of this commit not visible or was it deleted?",
- commit, otherCommit);
+ + " Is the change of this commit not visible to '%s' or was it deleted?",
+ commit, otherCommit, caller != null ? caller.getLoggableName() : "<user-not-available>");
} else if (changes.size() == 1) {
ChangeData cd = changes.get(0);
if (cd.currentPatchSet().getRevision().get().equals(otherCommit)) {
diff --git a/java/com/google/gerrit/server/submit/MergeSorter.java b/java/com/google/gerrit/server/submit/MergeSorter.java
index 6dbec32..dbdb0b4 100644
--- a/java/com/google/gerrit/server/submit/MergeSorter.java
+++ b/java/com/google/gerrit/server/submit/MergeSorter.java
@@ -14,6 +14,8 @@
package com.google.gerrit.server.submit;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
import com.google.gerrit.server.query.change.InternalChangeQuery;
@@ -29,6 +31,7 @@
import org.eclipse.jgit.revwalk.RevFlag;
public class MergeSorter {
+ @Nullable private final CurrentUser caller;
private final CodeReviewRevWalk rw;
private final RevFlag canMergeFlag;
private final Set<RevCommit> accepted;
@@ -36,11 +39,13 @@
private final Set<CodeReviewCommit> incoming;
public MergeSorter(
+ @Nullable CurrentUser caller,
CodeReviewRevWalk rw,
Set<RevCommit> alreadyAccepted,
RevFlag canMergeFlag,
Provider<InternalChangeQuery> queryProvider,
Set<CodeReviewCommit> incoming) {
+ this.caller = caller;
this.rw = rw;
this.canMergeFlag = canMergeFlag;
this.accepted = alreadyAccepted;
@@ -70,7 +75,8 @@
//
n.setStatusCode(CommitMergeStatus.MISSING_DEPENDENCY);
n.setStatusMessage(
- CommitMergeStatus.createMissingDependencyMessage(queryProvider, n.name(), c.name()));
+ CommitMergeStatus.createMissingDependencyMessage(
+ caller, queryProvider, 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 32f3558..1fca330 100644
--- a/java/com/google/gerrit/server/submit/RebaseSorter.java
+++ b/java/com/google/gerrit/server/submit/RebaseSorter.java
@@ -17,6 +17,7 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change.Status;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
import com.google.gerrit.server.query.change.ChangeData;
@@ -37,6 +38,7 @@
public class RebaseSorter {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private final CurrentUser caller;
private final CodeReviewRevWalk rw;
private final RevFlag canMergeFlag;
private final RevCommit initialTip;
@@ -45,12 +47,14 @@
private final Set<CodeReviewCommit> incoming;
public RebaseSorter(
+ CurrentUser caller,
CodeReviewRevWalk rw,
RevCommit initialTip,
Set<RevCommit> alreadyAccepted,
RevFlag canMergeFlag,
Provider<InternalChangeQuery> queryProvider,
Set<CodeReviewCommit> incoming) {
+ this.caller = caller;
this.rw = rw;
this.canMergeFlag = canMergeFlag;
this.initialTip = initialTip;
@@ -85,7 +89,7 @@
n.setStatusCode(CommitMergeStatus.MISSING_DEPENDENCY);
n.setStatusMessage(
CommitMergeStatus.createMissingDependencyMessage(
- queryProvider, n.name(), c.name()));
+ caller, queryProvider, 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/SubmitDryRun.java b/java/com/google/gerrit/server/submit/SubmitDryRun.java
index e273652..391d956 100644
--- a/java/com/google/gerrit/server/submit/SubmitDryRun.java
+++ b/java/com/google/gerrit/server/submit/SubmitDryRun.java
@@ -19,8 +19,10 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Streams;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.reviewdb.client.Branch;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
import com.google.gerrit.server.git.MergeUtil;
@@ -106,6 +108,7 @@
}
public boolean run(
+ @Nullable CurrentUser caller,
SubmitType submitType,
Repository repo,
CodeReviewRevWalk rw,
@@ -124,7 +127,12 @@
rw,
mergeUtilFactory.create(getProject(destBranch)),
new MergeSorter(
- rw, alreadyAccepted, canMerge, queryProvider, ImmutableSet.of(toMergeCommit)));
+ caller,
+ rw,
+ alreadyAccepted,
+ canMerge,
+ queryProvider,
+ ImmutableSet.of(toMergeCommit)));
switch (submitType) {
case CHERRY_PICK:
diff --git a/java/com/google/gerrit/server/submit/SubmitStrategy.java b/java/com/google/gerrit/server/submit/SubmitStrategy.java
index 16c2b27..c9baf99 100644
--- a/java/com/google/gerrit/server/submit/SubmitStrategy.java
+++ b/java/com/google/gerrit/server/submit/SubmitStrategy.java
@@ -203,10 +203,16 @@
projectCache.get(destBranch.getParentKey()),
() -> String.format("project not found: %s", destBranch.getParentKey()));
this.mergeSorter =
- new MergeSorter(rw, alreadyAccepted, canMergeFlag, queryProvider, incoming);
+ new MergeSorter(caller, rw, alreadyAccepted, canMergeFlag, queryProvider, incoming);
this.rebaseSorter =
new RebaseSorter(
- rw, mergeTip.getInitialTip(), alreadyAccepted, canMergeFlag, queryProvider, incoming);
+ caller,
+ rw,
+ mergeTip.getInitialTip(),
+ alreadyAccepted,
+ canMergeFlag,
+ queryProvider,
+ incoming);
this.mergeUtil = mergeUtilFactory.create(project);
this.onSubmitValidatorsFactory = onSubmitValidatorsFactory;
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
index 94090a6..147abe1 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
@@ -599,7 +599,9 @@
+ " depends on commit "
+ changeResult.getCommit().name()
+ " which cannot be merged."
- + " Is the change of this commit not visible or was it deleted?");
+ + " Is the change of this commit not visible to '"
+ + admin.username
+ + "' or was it deleted?");
assertRefUpdatedEvents();
assertChangeMergedEvents();
@@ -652,7 +654,9 @@
+ " depends on commit "
+ changeResult.getCommit().name()
+ " which cannot be merged."
- + " Is the change of this commit not visible or was it deleted?");
+ + " Is the change of this commit not visible to '"
+ + user.username
+ + "' or was it deleted?");
assertRefUpdatedEvents();
assertChangeMergedEvents();
