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
This commit is contained in:
Dave Borowitz 2015-12-13 21:59:04 -05:00
parent 6ba7976580
commit 0761fd5ce4
5 changed files with 113 additions and 61 deletions

View File

@ -48,6 +48,13 @@ type.
Prolog based submit type computes a submit type for each change. The computed Prolog based submit type computes a submit type for each change. The computed
submit type is shown on the change screen for each change. 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 == Prolog Language
This document is not a complete Prolog tutorial. This document is not a complete Prolog tutorial.
link:http://en.wikipedia.org/wiki/Prolog[This Wikipedia page on Prolog] is a 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 for `+refs/heads/stable.*+` branches. The second `submit_type` predicate returns
the project's default submit type. 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('(?<!\.txt)$'),
gerrit:change_branch(B), regex_matches('refs/heads/stable.*', B),
!.
submit_type(T) :- gerrit:project_default_submit_type(T).
----
The `gerrit:commit_delta('(?<!\.txt)$')` succeeds if the change contains a file
whose name doesn't end with `.txt` The rest of this rule is same like in the
previous example.
If all file names in the change end with `.txt`, then the
`gerrit:commit_delta('(?<!\.txt)$')` will fail as no file name will match this
regular expression.
GERRIT GERRIT
------ ------
Part of link:index.html[Gerrit Code Review] Part of link:index.html[Gerrit Code Review]

View File

@ -66,6 +66,13 @@ by core plugins:
* TODO add more * TODO add more
Changes
~~~~~~~
In order to avoid potentially confusing behavior, when submitting changes in a
batch, submit type rules may not be used to mix submit types on a single branch,
and trying to submit such a batch will fail.
Bug Fixes Bug Fixes
--------- ---------

View File

@ -20,6 +20,7 @@ import static com.google.gerrit.extensions.client.SubmitType.FAST_FORWARD_ONLY;
import static com.google.gerrit.extensions.client.SubmitType.MERGE_ALWAYS; import static com.google.gerrit.extensions.client.SubmitType.MERGE_ALWAYS;
import static com.google.gerrit.extensions.client.SubmitType.MERGE_IF_NECESSARY; import static com.google.gerrit.extensions.client.SubmitType.MERGE_IF_NECESSARY;
import static com.google.gerrit.extensions.client.SubmitType.REBASE_IF_NECESSARY; import static com.google.gerrit.extensions.client.SubmitType.REBASE_IF_NECESSARY;
import static org.junit.Assert.fail;
import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.AbstractDaemonTest;
@ -29,15 +30,19 @@ import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.projects.BranchInput; import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.TestSubmitRuleInput; import com.google.gerrit.extensions.common.TestSubmitRuleInput;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.git.IntegrationException;
import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.git.VersionedMetaData; import com.google.gerrit.server.git.VersionedMetaData;
import com.google.gerrit.testutil.ConfigSuite;
import org.eclipse.jgit.api.Git; import org.eclipse.jgit.api.Git;
import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevCommit;
@ -50,6 +55,11 @@ import java.util.concurrent.atomic.AtomicInteger;
@NoHttpd @NoHttpd
public class SubmitTypeRuleIT extends AbstractDaemonTest { public class SubmitTypeRuleIT extends AbstractDaemonTest {
@ConfigSuite.Default
public static Config submitWholeTopicEnabled() {
return submitWholeTopicEnabledConfig();
}
private class RulesPl extends VersionedMetaData { private class RulesPl extends VersionedMetaData {
private static final String FILENAME = "rules.pl"; private static final String FILENAME = "rules.pl";
@ -185,6 +195,58 @@ public class SubmitTypeRuleIT extends AbstractDaemonTest {
assertThat(log.get(0).getFullMessage()).contains("Reviewed-on: "); assertThat(log.get(0).getFullMessage()).contains("Reviewed-on: ");
} }
@Test
public void mixingSubmitTypesAcrossBranchesSucceeds() throws Exception {
setRulesPl(SUBMIT_TYPE_FROM_SUBJECT);
PushOneCommit.Result r1 = createChange("master", "MERGE_IF_NECESSARY 1");
RevCommit initialCommit = r1.getCommit().getParent(0);
BranchInput bin = new BranchInput();
bin.revision = initialCommit.name();
gApi.projects().name(project.get()).branch("branch").create(bin);
testRepo.reset(initialCommit);
PushOneCommit.Result r2 = createChange("branch", "MERGE_ALWAYS 1");
gApi.changes().id(r1.getChangeId()).topic(name("topic"));
gApi.changes().id(r1.getChangeId()).current().review(ReviewInput.approve());
gApi.changes().id(r2.getChangeId()).topic(name("topic"));
gApi.changes().id(r2.getChangeId()).current().review(ReviewInput.approve());
gApi.changes().id(r2.getChangeId()).current().submit();
assertThat(log("master", 1).get(0).name()).isEqualTo(r1.getCommit().name());
List<RevCommit> 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<RevCommit> log(String commitish, int n) throws Exception { private List<RevCommit> log(String commitish, int n) throws Exception {
try (Repository repo = repoManager.openRepository(project); try (Repository repo = repoManager.openRepository(project);
Git git = new Git(repo)) { Git git = new Git(repo)) {

View File

@ -47,9 +47,6 @@ public enum CommitMergeStatus {
/** */ /** */
REVISION_GONE(""), REVISION_GONE(""),
/** */
NO_SUBMIT_TYPE(""),
/** */ /** */
MANUAL_RECURSIVE_MERGE("The change requires a local merge to resolve.\n" MANUAL_RECURSIVE_MERGE("The change requires a local merge to resolve.\n"
+ "\n" + "\n"

View File

@ -18,13 +18,12 @@ import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER; import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static org.eclipse.jgit.lib.RefDatabase.ALL; 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.Optional;
import com.google.common.base.Predicate; import com.google.common.base.Predicate;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.HashBasedTable; import com.google.common.collect.HashBasedTable;
import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables; import com.google.common.collect.Iterables;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Maps; import com.google.common.collect.Maps;
import com.google.common.collect.Multimap; import com.google.common.collect.Multimap;
import com.google.common.collect.Table; import com.google.common.collect.Table;
@ -392,8 +391,7 @@ public class MergeOp {
throws IntegrationException, NoSuchChangeException, throws IntegrationException, NoSuchChangeException,
ResourceConflictException { ResourceConflictException {
logDebug("Beginning merge attempt on {}", cs); logDebug("Beginning merge attempt on {}", cs);
Map<Branch.NameKey, ListMultimap<SubmitType, ChangeData>> toSubmit = Map<Branch.NameKey, BranchBatch> toSubmit = new HashMap<>();
new HashMap<>();
logDebug("Perform the merges"); logDebug("Perform the merges");
try { try {
Multimap<Project.NameKey, Branch.NameKey> br = cs.branchesByProject(); Multimap<Project.NameKey, Branch.NameKey> br = cs.branchesByProject();
@ -402,22 +400,15 @@ public class MergeOp {
openRepository(project); openRepository(project);
for (Branch.NameKey branch : br.get(project)) { for (Branch.NameKey branch : br.get(project)) {
setDestProject(branch); setDestProject(branch);
BranchBatch submitting = validateChangeList(cbb.get(branch));
ListMultimap<SubmitType, ChangeData> submitting =
validateChangeList(cbb.get(branch));
toSubmit.put(branch, submitting); toSubmit.put(branch, submitting);
Set<SubmitType> submitTypes = new HashSet<>(submitting.keySet()); SubmitStrategy strategy = createStrategy(
for (SubmitType submitType : submitTypes) { branch, submitting.submitType(), getBranchTip(branch), caller);
SubmitStrategy strategy = createStrategy(branch, submitType, MergeTip mergeTip = preMerge(strategy, submitting.changes(),
getBranchTip(branch), caller);
MergeTip mergeTip = preMerge(strategy, submitting.get(submitType),
getBranchTip(branch)); getBranchTip(branch));
mergeTips.put(branch, mergeTip); mergeTips.put(branch, mergeTip);
updateChangeStatus(submitting.get(submitType), branch, updateChangeStatus(submitting.changes(), branch, true, caller);
true, caller);
}
inserter.flush(); inserter.flush();
} }
closeRepository(); closeRepository();
@ -431,12 +422,9 @@ public class MergeOp {
pendingRefUpdates.remove(branch); pendingRefUpdates.remove(branch);
setDestProject(branch); setDestProject(branch);
ListMultimap<SubmitType, ChangeData> submitting = toSubmit.get(branch); BranchBatch submitting = toSubmit.get(branch);
for (SubmitType submitType : submitting.keySet()) { updateChangeStatus(submitting.changes(), branch, false, caller);
updateChangeStatus(submitting.get(submitType), branch,
false, caller);
updateSubmoduleSubscriptions(subOp, branch, getBranchTip(branch)); updateSubmoduleSubscriptions(subOp, branch, getBranchTip(branch));
}
if (update != null) { if (update != null) {
fireRefUpdated(branch, update); fireRefUpdated(branch, update);
} }
@ -583,10 +571,16 @@ public class MergeOp {
return alreadyAccepted; return alreadyAccepted;
} }
private ListMultimap<SubmitType, ChangeData> validateChangeList( @AutoValue
static abstract class BranchBatch {
abstract SubmitType submitType();
abstract List<ChangeData> changes();
}
private BranchBatch validateChangeList(
Collection<ChangeData> submitted) throws IntegrationException { Collection<ChangeData> submitted) throws IntegrationException {
logDebug("Validating {} changes", submitted.size()); logDebug("Validating {} changes", submitted.size());
ListMultimap<SubmitType, ChangeData> toSubmit = ArrayListMultimap.create(); List<ChangeData> toSubmit = new ArrayList<>(submitted.size());
Map<String, Ref> allRefs; Map<String, Ref> allRefs;
try { try {
@ -600,6 +594,8 @@ public class MergeOp {
tips.add(r.getObjectId()); tips.add(r.getObjectId());
} }
SubmitType submitType = null;
ChangeData choseSubmitTypeFrom = null;
for (ChangeData cd : submitted) { for (ChangeData cd : submitted) {
ChangeControl ctl; ChangeControl ctl;
Change chg; Change chg;
@ -686,20 +682,27 @@ public class MergeOp {
continue; continue;
} }
SubmitType submitType; SubmitType st = getSubmitType(commit.getControl(), ps);
submitType = getSubmitType(commit.getControl(), ps); if (st == null) {
if (submitType == null) {
logError("No submit type for revision " + idstr + " of patch set " logError("No submit type for revision " + idstr + " of patch set "
+ ps.getId()); + ps.getId());
commit.setStatusCode(CommitMergeStatus.NO_SUBMIT_TYPE); throw new IntegrationException(
continue; "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); commit.add(canMergeFlag);
toSubmit.put(submitType, cd); toSubmit.add(cd);
} }
logDebug("Submitting on this run: {}", toSubmit); logDebug("Submitting on this run: {}", toSubmit);
return toSubmit; return new AutoValue_MergeOp_BranchBatch(submitType, toSubmit);
} }
private SubmitType getSubmitType(ChangeControl ctl, PatchSet ps) { private SubmitType getSubmitType(ChangeControl ctl, PatchSet ps) {