From 93703efaf4ef0183a506b76741e4b09de209413e Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 4 Oct 2018 10:34:05 +0200 Subject: [PATCH] Let users know which change 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. Commit depends on commit which cannot be merged. To find the change to which 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 depends on commit of change which cannot be merged. Returning the commit SHA1 of 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 --- .../google/gerrit/server/git/MergeUtil.java | 4 +-- .../server/submit/CommitMergeStatus.java | 27 +++++++++++++++++++ .../gerrit/server/submit/MergeSorter.java | 11 +++++--- .../gerrit/server/submit/RebaseSorter.java | 7 ++--- .../server/submit/RebaseSubmitStrategy.java | 2 +- .../gerrit/server/submit/SubmitDryRun.java | 12 +++++++-- .../gerrit/server/submit/SubmitStrategy.java | 3 ++- .../change/SubmitByMergeIfNecessaryIT.java | 4 +++ 8 files changed, 58 insertions(+), 12 deletions(-) diff --git a/java/com/google/gerrit/server/git/MergeUtil.java b/java/com/google/gerrit/server/git/MergeUtil.java index c2e147b317..016415cf9c 100644 --- a/java/com/google/gerrit/server/git/MergeUtil.java +++ b/java/com/google/gerrit/server/git/MergeUtil.java @@ -216,7 +216,7 @@ public class MergeUtil { List 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); } } diff --git a/java/com/google/gerrit/server/submit/CommitMergeStatus.java b/java/com/google/gerrit/server/submit/CommitMergeStatus.java index 7f4f4af0e3..202fab7a06 100644 --- a/java/com/google/gerrit/server/submit/CommitMergeStatus.java +++ b/java/com/google/gerrit/server/submit/CommitMergeStatus.java @@ -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 queryProvider, String commit, String otherCommit) + throws OrmException { + List 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())); + } + } } diff --git a/java/com/google/gerrit/server/submit/MergeSorter.java b/java/com/google/gerrit/server/submit/MergeSorter.java index fa5435bc44..6dbec325bd 100644 --- a/java/com/google/gerrit/server/submit/MergeSorter.java +++ b/java/com/google/gerrit/server/submit/MergeSorter.java @@ -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 accepted; + private final Provider queryProvider; private final Set incoming; public MergeSorter( CodeReviewRevWalk rw, Set alreadyAccepted, RevFlag canMergeFlag, + Provider queryProvider, Set incoming) { this.rw = rw; this.canMergeFlag = canMergeFlag; this.accepted = alreadyAccepted; + this.queryProvider = queryProvider; this.incoming = incoming; } public Collection sort(Collection toMerge) - throws IOException { + throws IOException, OrmException { final Set heads = new HashSet<>(); final Set 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); diff --git a/java/com/google/gerrit/server/submit/RebaseSorter.java b/java/com/google/gerrit/server/submit/RebaseSorter.java index bd4f7eafad..32f35580ba 100644 --- a/java/com/google/gerrit/server/submit/RebaseSorter.java +++ b/java/com/google/gerrit/server/submit/RebaseSorter.java @@ -59,7 +59,8 @@ public class RebaseSorter { this.incoming = incoming; } - public List sort(Collection toSort) throws IOException { + public List sort(Collection toSort) + throws IOException, OrmException { final List sorted = new ArrayList<>(); final Set 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. diff --git a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java index b2b54aeb18..fa0fddead4 100644 --- a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java +++ b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java @@ -59,7 +59,7 @@ public class RebaseSubmitStrategy extends SubmitStrategy { List sorted; try { sorted = args.rebaseSorter.sort(toMerge); - } catch (IOException e) { + } catch (IOException | OrmException e) { throw new IntegrationException("Commit sorting failed", e); } List ops = new ArrayList<>(sorted.size()); diff --git a/java/com/google/gerrit/server/submit/SubmitDryRun.java b/java/com/google/gerrit/server/submit/SubmitDryRun.java index ed9bfeb2fd..e273652d9d 100644 --- a/java/com/google/gerrit/server/submit/SubmitDryRun.java +++ b/java/com/google/gerrit/server/submit/SubmitDryRun.java @@ -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 queryProvider; @Inject - SubmitDryRun(ProjectCache projectCache, MergeUtil.Factory mergeUtilFactory) { + SubmitDryRun( + ProjectCache projectCache, + MergeUtil.Factory mergeUtilFactory, + Provider 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: diff --git a/java/com/google/gerrit/server/submit/SubmitStrategy.java b/java/com/google/gerrit/server/submit/SubmitStrategy.java index 6e3e0b8fc0..2b45cd9dd6 100644 --- a/java/com/google/gerrit/server/submit/SubmitStrategy.java +++ b/java/com/google/gerrit/server/submit/SubmitStrategy.java @@ -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); diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java index c606bb026c..0da5e5919b 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java @@ -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();