opendev/gerrit
Code Issues Proposed changes

Let users know which change 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. Commit <commit> depends on
  commit <other-commit> which cannot be merged.

To find the change to which <other-commit> belongs the user must do a
change query. Do this for the user and return the change ID too.

Now we return:

  Depends on change that was not submitted. Commit <commit> depends on
  commit <other-commit> of change <other-change> which cannot be merged.

Returning the commit SHA1 of <other-commit> is still useful since the
commit that prevents submit may be an outdated patch set and this can be
found easier if the commit SHA1 is known.

Change-Id: I1368af204702a06d01ee6f812a5da5ac419b89d7
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2018-10-04 10:34:05 +02:00
parent 01838a813e
commit 93703efaf4
8 changed files with 58 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
java/com/google/gerrit/server/git/MergeUtil.java
View File

@@ -216,7 +216,7 @@ public class MergeUtil {
List<CodeReviewCommit> result = new ArrayList<>();
try {
result.addAll(mergeSorter.sort(toSort));
} catch (IOException e) {
} catch (IOException | OrmException e) {
throw new IntegrationException("Branch head sorting failed", e);
}
result.sort(CodeReviewCommit.ORDER);
@@ -566,7 +566,7 @@ public class MergeUtil {
throws IntegrationException {
try {
return !mergeSorter.sort(Collections.singleton(toMerge)).contains(toMerge);
} catch (IOException e) {
} catch (IOException | OrmException e) {
throw new IntegrationException("Branch head sorting failed", e);
}
}

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

@@ -14,6 +14,14 @@
package com.google.gerrit.server.submit;
import static java.util.stream.Collectors.toSet;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Provider;
import java.util.List;
/**
* Status codes set on {@link com.google.gerrit.server.git.CodeReviewCommit}s by {@link
* SubmitStrategy} implementations.
@@ -76,4 +84,23 @@ public enum CommitMergeStatus {
public String getDescription() {
return description;
}
public static String createMissingDependencyMessage(
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.", commit, otherCommit);
} else if (changes.size() == 1) {
return String.format(
"Commit %s depends on commit %s of change %d which cannot be merged.",
commit, otherCommit, changes.get(0).getId().get());
} else {
return String.format(
"Commit %s depends on commit %s of changes %s which cannot be merged.",
commit, otherCommit, changes.stream().map(cd -> cd.getId().get()).collect(toSet()));
}
}
}

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

@@ -16,6 +16,9 @@ package com.google.gerrit.server.submit;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Provider;
import java.io.IOException;
import java.util.Collection;
import java.util.HashSet;
@@ -29,21 +32,24 @@ public class MergeSorter {
private final CodeReviewRevWalk rw;
private final RevFlag canMergeFlag;
private final Set<RevCommit> accepted;
private final Provider<InternalChangeQuery> queryProvider;
private final Set<CodeReviewCommit> incoming;
public MergeSorter(
CodeReviewRevWalk rw,
Set<RevCommit> alreadyAccepted,
RevFlag canMergeFlag,
Provider<InternalChangeQuery> queryProvider,
Set<CodeReviewCommit> incoming) {
this.rw = rw;
this.canMergeFlag = canMergeFlag;
this.accepted = alreadyAccepted;
this.queryProvider = queryProvider;
this.incoming = incoming;
}
public Collection<CodeReviewCommit> sort(Collection<CodeReviewCommit> toMerge)
throws IOException {
throws IOException, OrmException {
final Set<CodeReviewCommit> heads = new HashSet<>();
final Set<CodeReviewCommit> sort = new HashSet<>(toMerge);
while (!sort.isEmpty()) {
@@ -64,8 +70,7 @@ public class MergeSorter {
//
n.setStatusCode(CommitMergeStatus.MISSING_DEPENDENCY);
n.setStatusMessage(
String.format(
"Commit %s depends on commit %s which cannot be merged.", n.name(), c.name()));
CommitMergeStatus.createMissingDependencyMessage(queryProvider, n.name(), c.name()));
break;
}
contents.add(c);

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

@@ -59,7 +59,8 @@ public class RebaseSorter {
this.incoming = incoming;
}
public List<CodeReviewCommit> sort(Collection<CodeReviewCommit> toSort) throws IOException {
public List<CodeReviewCommit> sort(Collection<CodeReviewCommit> toSort)
throws IOException, OrmException {
final List<CodeReviewCommit> sorted = new ArrayList<>();
final Set<CodeReviewCommit> sort = new HashSet<>(toSort);
while (!sort.isEmpty()) {
@@ -83,8 +84,8 @@ public class RebaseSorter {
//
n.setStatusCode(CommitMergeStatus.MISSING_DEPENDENCY);
n.setStatusMessage(
String.format(
"Commit %s depends on commit %s which cannot be merged.", n.name(), c.name()));
CommitMergeStatus.createMissingDependencyMessage(
queryProvider, n.name(), c.name()));
}
// Stop RevWalk because c is either a merged commit or a missing
// dependency. Not need to walk further.

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

@@ -59,7 +59,7 @@ public class RebaseSubmitStrategy extends SubmitStrategy {
List<CodeReviewCommit> sorted;
try {
sorted = args.rebaseSorter.sort(toMerge);
} catch (IOException e) {
} catch (IOException | OrmException e) {
throw new IntegrationException("Commit sorting failed", e);
}
List<SubmitStrategyOp> ops = new ArrayList<>(sorted.size());

12
java/com/google/gerrit/server/submit/SubmitDryRun.java
View File

@@ -27,7 +27,9 @@ import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.io.IOException;
import java.util.Collection;
import java.util.HashSet;
@@ -91,11 +93,16 @@ public class SubmitDryRun {
private final ProjectCache projectCache;
private final MergeUtil.Factory mergeUtilFactory;
private final Provider<InternalChangeQuery> queryProvider;
@Inject
SubmitDryRun(ProjectCache projectCache, MergeUtil.Factory mergeUtilFactory) {
SubmitDryRun(
ProjectCache projectCache,
MergeUtil.Factory mergeUtilFactory,
Provider<InternalChangeQuery> queryProvider) {
this.projectCache = projectCache;
this.mergeUtilFactory = mergeUtilFactory;
this.queryProvider = queryProvider;
}
public boolean run(
@@ -116,7 +123,8 @@ public class SubmitDryRun {
repo,
rw,
mergeUtilFactory.create(getProject(destBranch)),
new MergeSorter(rw, alreadyAccepted, canMerge, ImmutableSet.of(toMergeCommit)));
new MergeSorter(
rw, alreadyAccepted, canMerge, queryProvider, ImmutableSet.of(toMergeCommit)));
switch (submitType) {
case CHERRY_PICK:

3
java/com/google/gerrit/server/submit/SubmitStrategy.java
View File

@@ -204,7 +204,8 @@ public abstract class SubmitStrategy {
projectCache.get(destBranch.getParentKey()),
"project not found: %s",
destBranch.getParentKey());
this.mergeSorter = new MergeSorter(rw, alreadyAccepted, canMergeFlag, incoming);
this.mergeSorter =
new MergeSorter(rw, alreadyAccepted, canMergeFlag, queryProvider, incoming);
this.rebaseSorter =
new RebaseSorter(
rw, mergeTip.getInitialTip(), alreadyAccepted, canMergeFlag, queryProvider, incoming);

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

@@ -408,6 +408,8 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge {
+ change3a.getCommit().name()
+ " depends on commit "
+ change2.getCommit().name()
+ " of change "
+ change2.getChange().getId()
+ " which cannot be merged.");
RevCommit tipbranch = getRemoteLog(project, "branch").get(0);
@@ -511,6 +513,8 @@ public class SubmitByMergeIfNecessaryIT extends AbstractSubmitByMerge {
+ change3.getCommit().name()
+ " depends on commit "
+ change2result.getCommit().name()
+ " of change "
+ change2result.getChange().getId()
+ " which cannot be merged.");
assertRefUpdatedEvents();