diff --git a/Documentation/prolog-cookbook.txt b/Documentation/prolog-cookbook.txt index 7178f8d4eb..d68dc8a706 100644 --- a/Documentation/prolog-cookbook.txt +++ b/Documentation/prolog-cookbook.txt @@ -48,6 +48,13 @@ type. Prolog based submit type computes a submit type for each change. The computed submit type is shown on the change screen for each change. +When submitting changes in a batch using "Submit including ancestors" or "Submit +whole topic", submit type rules may not be used to mix submit types on a single +branch, and trying to submit such a batch will fail. This avoids potentially +confusing behavior and spurious submit failures. It is recommended to only use +submit type rules to change submit types for an entire branch, which avoids this +situation. + == Prolog Language This document is not a complete Prolog tutorial. link:http://en.wikipedia.org/wiki/Prolog[This Wikipedia page on Prolog] is a @@ -1006,30 +1013,6 @@ The first `submit_type` predicate defines the `Fast Forward Only` submit type for `+refs/heads/stable.*+` branches. The second `submit_type` predicate returns the project's default submit type. -=== Example 3: Don't require `Fast Forward Only` if only documentation was changed -Like in the previous example we want the `Fast Forward Only` submit type for the -`+refs/heads/stable*+` branches. However, if only documentation was changed -(only `+*.txt+` files), then we allow project's default submit type for such -changes. - -`rules.pl` -[source,prolog] ----- -submit_type(fast_forward_only) :- - gerrit:commit_delta('(? branchLog = log("branch", 1); + assertThat(branchLog.get(0).getParents()).hasLength(2); + assertThat(branchLog.get(0).getParent(1).name()) + .isEqualTo(r2.getCommit().name()); + } + + @Test + public void mixingSubmitTypesOnOneBranchFails() throws Exception { + setRulesPl(SUBMIT_TYPE_FROM_SUBJECT); + + PushOneCommit.Result r1 = createChange("master", "CHERRY_PICK 1"); + PushOneCommit.Result r2 = createChange("master", "MERGE_IF_NECESSARY 2"); + + gApi.changes().id(r1.getChangeId()).current().review(ReviewInput.approve()); + gApi.changes().id(r2.getChangeId()).current().review(ReviewInput.approve()); + + try { + gApi.changes().id(r2.getChangeId()).current().submit(); + fail("Expected ResourceConflictException"); + } catch (ResourceConflictException e) { + assertThat(e).hasMessage("Merge Conflict"); + Throwable t = e.getCause(); + assertThat(t).isInstanceOf(IntegrationException.class); + assertThat(t.getMessage()).isEqualTo( + "Change " + r1.getChange().getId() + " has submit type CHERRY_PICK, " + + "but previously chose submit type MERGE_IF_NECESSARY from change " + + r2.getChange().getId() + " in the same batch"); + } + } + private List log(String commitish, int n) throws Exception { try (Repository repo = repoManager.openRepository(project); Git git = new Git(repo)) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/CommitMergeStatus.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/CommitMergeStatus.java index 66417fbbf9..da64332b6e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/CommitMergeStatus.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/CommitMergeStatus.java @@ -47,9 +47,6 @@ public enum CommitMergeStatus { /** */ REVISION_GONE(""), - /** */ - NO_SUBMIT_TYPE(""), - /** */ MANUAL_RECURSIVE_MERGE("The change requires a local merge to resolve.\n" + "\n" diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index 61689a12a2..1f52a1c9eb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java @@ -18,13 +18,12 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; import static org.eclipse.jgit.lib.RefDatabase.ALL; +import com.google.auto.value.AutoValue; import com.google.common.base.Optional; import com.google.common.base.Predicate; -import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.HashBasedTable; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; -import com.google.common.collect.ListMultimap; import com.google.common.collect.Maps; import com.google.common.collect.Multimap; import com.google.common.collect.Table; @@ -392,8 +391,7 @@ public class MergeOp { throws IntegrationException, NoSuchChangeException, ResourceConflictException { logDebug("Beginning merge attempt on {}", cs); - Map> toSubmit = - new HashMap<>(); + Map toSubmit = new HashMap<>(); logDebug("Perform the merges"); try { Multimap br = cs.branchesByProject(); @@ -402,22 +400,15 @@ public class MergeOp { openRepository(project); for (Branch.NameKey branch : br.get(project)) { setDestProject(branch); - - ListMultimap submitting = - validateChangeList(cbb.get(branch)); + BranchBatch submitting = validateChangeList(cbb.get(branch)); toSubmit.put(branch, submitting); - Set submitTypes = new HashSet<>(submitting.keySet()); - for (SubmitType submitType : submitTypes) { - SubmitStrategy strategy = createStrategy(branch, submitType, - getBranchTip(branch), caller); - - MergeTip mergeTip = preMerge(strategy, submitting.get(submitType), - getBranchTip(branch)); - mergeTips.put(branch, mergeTip); - updateChangeStatus(submitting.get(submitType), branch, - true, caller); - } + SubmitStrategy strategy = createStrategy( + branch, submitting.submitType(), getBranchTip(branch), caller); + MergeTip mergeTip = preMerge(strategy, submitting.changes(), + getBranchTip(branch)); + mergeTips.put(branch, mergeTip); + updateChangeStatus(submitting.changes(), branch, true, caller); inserter.flush(); } closeRepository(); @@ -431,12 +422,9 @@ public class MergeOp { pendingRefUpdates.remove(branch); setDestProject(branch); - ListMultimap submitting = toSubmit.get(branch); - for (SubmitType submitType : submitting.keySet()) { - updateChangeStatus(submitting.get(submitType), branch, - false, caller); - updateSubmoduleSubscriptions(subOp, branch, getBranchTip(branch)); - } + BranchBatch submitting = toSubmit.get(branch); + updateChangeStatus(submitting.changes(), branch, false, caller); + updateSubmoduleSubscriptions(subOp, branch, getBranchTip(branch)); if (update != null) { fireRefUpdated(branch, update); } @@ -583,10 +571,16 @@ public class MergeOp { return alreadyAccepted; } - private ListMultimap validateChangeList( + @AutoValue + static abstract class BranchBatch { + abstract SubmitType submitType(); + abstract List changes(); + } + + private BranchBatch validateChangeList( Collection submitted) throws IntegrationException { logDebug("Validating {} changes", submitted.size()); - ListMultimap toSubmit = ArrayListMultimap.create(); + List toSubmit = new ArrayList<>(submitted.size()); Map allRefs; try { @@ -600,6 +594,8 @@ public class MergeOp { tips.add(r.getObjectId()); } + SubmitType submitType = null; + ChangeData choseSubmitTypeFrom = null; for (ChangeData cd : submitted) { ChangeControl ctl; Change chg; @@ -686,20 +682,27 @@ public class MergeOp { continue; } - SubmitType submitType; - submitType = getSubmitType(commit.getControl(), ps); - if (submitType == null) { + SubmitType st = getSubmitType(commit.getControl(), ps); + if (st == null) { logError("No submit type for revision " + idstr + " of patch set " + ps.getId()); - commit.setStatusCode(CommitMergeStatus.NO_SUBMIT_TYPE); - continue; + throw new IntegrationException( + "No submit type for change " + cd.getId()); + } + if (submitType == null) { + submitType = st; + choseSubmitTypeFrom = cd; + } else if (st != submitType) { + throw new IntegrationException(String.format( + "Change %s has submit type %s, but previously chose submit type %s " + + "from change %s in the same batch", + cd.getId(), st, submitType, choseSubmitTypeFrom.getId())); } - commit.add(canMergeFlag); - toSubmit.put(submitType, cd); + toSubmit.add(cd); } logDebug("Submitting on this run: {}", toSubmit); - return toSubmit; + return new AutoValue_MergeOp_BranchBatch(submitType, toSubmit); } private SubmitType getSubmitType(ChangeControl ctl, PatchSet ps) {