From 0761fd5ce4a7a34fa5b776c0bcfdc3b6e18d8697 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Sun, 13 Dec 2015 21:59:04 -0500 Subject: [PATCH] MergeOp: Disallow multiple submit types on a single branch Submit type rules allow different changes to the same branch to have different submit types. A main use case for this functionality is allowing branch-specific rather than project-specific submit types. However, in theory this also allows a single batch of commits to mix submit types. MergeOp currently handles this situation by splitting up open changes on a branch by submit type and running submit types on each subset in arbitrary order (based on HashMap iteration). My thesis is that this behavior produces nondeterministic results that, even if we can justify them as "sane", are likely to be surprising and/or confusing to the user, and that we are better off failing fast rather than trying to support this scenario. In the past, there was a distinct reason for this behavior, which was that there might be (through no fault of a user) changes in the submitted state with various submit types. Spinning through the submit type list and making progress, while perhaps confusing, was probably better than not making progress at all. But now that the submitted state is gone, the only way in which multiple changes can be submitted at once is within a single batch (including parents or by topic), so this reasoning does not really exist any more. For one example of confusing behavior, say we have two changes A<-B based on the branch tip 0, where A is Merge Always and B is Merge If Necessary. If MergeOp chooses to run Merge Always first, the resulting history will be: 0----Ma--Mab \-A-/ / \-B-/ If, however, MergeOp chooses to run Merge If Necessary first, the merge sorter will choose the fast-forward resolution for B, resulting in: 0--A--B When Merge Always runs, it will find that A is already merged and do nothing. For another example, consider three changes A<-B<-C, where A and C are Cherry-Pick and B is Merge If Necessary. If MergeOp chooses to run Cherry-Pick first, it cherry-picks A' and C': 0--A'--C' Then merging B fails since it now depends on an out-of-date patch set of A. If MergeOp chooses Merge If Necessary first, then B gets chosen as a fast-forward and C gets cherry-picked on top: 0--A--B--C' It is not at all obvious that any one of these solutions is what the user expects to get, to say nothing of more complicated cases. Note that I am only about 75% sure of what actually happens in these scenarios; I might be completely wrong. That just goes to show how weird this behavior is. Enforce during validateChangeList that only a single submit type is present on each branch. This also eliminates one level of looping in the main integrateIntoHistory logic. Another possible solution would be in the case of mixed submit types to run the entire process one change at a time in topological order. This at least might be easier to reason about, although it would still not always succeed, for example if a Merge Always change follows a Cherry-Pick change. But it would introduce considerably more complexity to rework the loops in MergeOp, all for the questionable benefit of making it easier for users to get into a confused situation. Better to just not let them do it at all. Change-Id: I0cec2a7e3e3625fedbdd621b0c6eca6c4100f232 --- Documentation/prolog-cookbook.txt | 31 ++------ ReleaseNotes/ReleaseNotes-2.13.txt | 7 ++ .../api/change/SubmitTypeRuleIT.java | 62 ++++++++++++++++ .../gerrit/server/git/CommitMergeStatus.java | 3 - .../com/google/gerrit/server/git/MergeOp.java | 71 ++++++++++--------- 5 files changed, 113 insertions(+), 61 deletions(-) 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) {