Remove experiment flag for rebasing merge commits
This was fully rolled out at Google without any issues. Hence this no
longer needs to be protected by an experiment flag.
Bug: Google b/262486352
Release-Notes: skip
Change-Id: I5e3891ba2594b636d6647514ed7e5435f35933cc
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
index 61e3819..cd91745 100644
--- a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
+++ b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
@@ -57,9 +57,6 @@
public static String GERRIT_BACKEND_FEATURE_ALWAYS_REJECT_IMPLICIT_MERGES_ON_MERGE =
"GerritBackendFeature__always_reject_implicit_merges_on_merge";
- /** Whether the rebase submit strategies should rebase merge commits. */
- public static final String REBASE_MERGE_COMMITS = "GerritBackendFeature__rebase_merge_commits";
-
/** Whether we allow fix suggestions in HumanComments. */
public static final String ALLOW_FIX_SUGGESTIONS_IN_COMMENTS =
"GerritBackendFeature__allow_fix_suggestions_in_comments";
diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java
index 3fa4157..eb41690 100644
--- a/java/com/google/gerrit/server/submit/MergeOp.java
+++ b/java/com/google/gerrit/server/submit/MergeOp.java
@@ -19,7 +19,6 @@
import static com.google.gerrit.server.experiments.ExperimentFeaturesConstants.GERRIT_BACKEND_FEATURE_ALWAYS_REJECT_IMPLICIT_MERGES_ON_MERGE;
import static com.google.gerrit.server.experiments.ExperimentFeaturesConstants.GERRIT_BACKEND_FEATURE_CHECK_IMPLICIT_MERGES_ON_MERGE;
import static com.google.gerrit.server.experiments.ExperimentFeaturesConstants.GERRIT_BACKEND_FEATURE_REJECT_IMPLICIT_MERGES_ON_MERGE;
-import static com.google.gerrit.server.experiments.ExperimentFeaturesConstants.REBASE_MERGE_COMMITS;
import static com.google.gerrit.server.project.ProjectCache.illegalState;
import static com.google.gerrit.server.update.RetryableAction.ActionType.INDEX_QUERY;
import static com.google.gerrit.server.update.context.RefUpdateContext.RefUpdateType.MERGE_CHANGE;
@@ -877,9 +876,7 @@
GERRIT_BACKEND_FEATURE_CHECK_IMPLICIT_MERGES_ON_MERGE, project)) {
return;
}
- boolean rebaseMergeCommits = experimentFeatures.isFeatureEnabled(REBASE_MERGE_COMMITS, project);
- if (submitType == SubmitType.CHERRY_PICK
- || (rebaseMergeCommits && submitType == SubmitType.REBASE_ALWAYS)) {
+ if (submitType == SubmitType.CHERRY_PICK || submitType == SubmitType.REBASE_ALWAYS) {
return;
}
diff --git a/java/com/google/gerrit/server/submit/RebaseSorter.java b/java/com/google/gerrit/server/submit/RebaseSorter.java
index 3645d3f..d769ddd 100644
--- a/java/com/google/gerrit/server/submit/RebaseSorter.java
+++ b/java/com/google/gerrit/server/submit/RebaseSorter.java
@@ -31,6 +31,7 @@
import java.util.Iterator;
import java.util.List;
import java.util.Set;
+import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevFlag;
@@ -65,8 +66,10 @@
public List<CodeReviewCommit> sort(Collection<CodeReviewCommit> toSort) throws IOException {
final List<CodeReviewCommit> sorted = new ArrayList<>();
final Set<CodeReviewCommit> sort = new HashSet<>(toSort);
+ final Set<ObjectId> uninterestingParents = new HashSet<>();
while (!sort.isEmpty()) {
final CodeReviewCommit n = removeOne(sort);
+ collectUninterestingParents(n, uninterestingParents);
rw.resetRetain(canMergeFlag);
rw.markStart(n);
@@ -77,6 +80,8 @@
CodeReviewCommit c;
final List<CodeReviewCommit> contents = new ArrayList<>();
while ((c = rw.next()) != null) {
+ collectUninterestingParents(c, uninterestingParents);
+
if (!c.has(canMergeFlag) || !incoming.contains(c)) {
if (isMergedInBranchAsSubmittedChange(c, n.change().getDest())
|| isAlreadyMergedInAnyBranch(c)) {
@@ -106,9 +111,27 @@
sorted.removeAll(contents);
sorted.addAll(contents);
}
+ sorted.removeAll(uninterestingParents);
return sorted;
}
+ /**
+ * Rebasing merge commits is done by rebasing their first parent commit, i.e. the first parent is
+ * updated to the new base while the second parent stays intact. This means for chains of changes
+ * we only need to rebase changes that are reachable via the first parents. Changes that are
+ * reachable via second parents do not need to be rebased (since the second parent of merge
+ * commits stays intact) which is why we filter them out here by marking them as uninteresting.
+ */
+ private void collectUninterestingParents(CodeReviewCommit c, Set<ObjectId> uninterestingParents)
+ throws IOException {
+ if (c.getParentCount() > 0) {
+ for (int parent = 1; parent < c.getParentCount(); parent++) {
+ uninterestingParents.add(c.getParent(parent));
+ rw.markUninteresting(c.getParent(parent));
+ }
+ }
+ }
+
private boolean isAlreadyMergedInAnyBranch(CodeReviewCommit commit) throws IOException {
try (CodeReviewRevWalk mirw = CodeReviewCommit.newRevWalk(rw.getObjectReader())) {
mirw.reset();
diff --git a/java/com/google/gerrit/server/submit/RebaseSorterNew.java b/java/com/google/gerrit/server/submit/RebaseSorterNew.java
deleted file mode 100644
index 7fbabb4..0000000
--- a/java/com/google/gerrit/server/submit/RebaseSorterNew.java
+++ /dev/null
@@ -1,178 +0,0 @@
-// Copyright (C) 2012 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.submit;
-
-import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.entities.BranchNameKey;
-import com.google.gerrit.exceptions.StorageException;
-import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.git.CodeReviewCommit;
-import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
-import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.query.change.InternalChangeQuery;
-import com.google.inject.Provider;
-import java.io.IOException;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashSet;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Set;
-import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.revwalk.RevCommit;
-import org.eclipse.jgit.revwalk.RevFlag;
-
-/**
- * This is a temporary copy of {@code RebaseSorter} with an extension to filter out secondary
- * parents, which is needed for rebasing merge commits. Rebasing merge commits is protected by an
- * experiment flag, which means for now we have to keep the old logic working (hence we cannot
- * replace the original {@code RebaseSorter} yet. Once the experiment was successful, we will
- * replace {@code RebaseSorter} with this class.
- */
-public class RebaseSorterNew {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
- private final CurrentUser caller;
- private final CodeReviewRevWalk rw;
- private final RevFlag canMergeFlag;
- private final RevCommit initialTip;
- private final Set<RevCommit> alreadyAccepted;
- private final Provider<InternalChangeQuery> queryProvider;
- private final Set<CodeReviewCommit> incoming;
-
- public RebaseSorterNew(
- CurrentUser caller,
- CodeReviewRevWalk rw,
- RevCommit initialTip,
- Set<RevCommit> alreadyAccepted,
- RevFlag canMergeFlag,
- Provider<InternalChangeQuery> queryProvider,
- Set<CodeReviewCommit> incoming) {
- this.caller = caller;
- this.rw = rw;
- this.canMergeFlag = canMergeFlag;
- this.initialTip = initialTip;
- this.alreadyAccepted = alreadyAccepted;
- this.queryProvider = queryProvider;
- this.incoming = incoming;
- }
-
- public List<CodeReviewCommit> sort(Collection<CodeReviewCommit> toSort) throws IOException {
- final List<CodeReviewCommit> sorted = new ArrayList<>();
- final Set<CodeReviewCommit> sort = new HashSet<>(toSort);
- final Set<ObjectId> uninterestingParents = new HashSet<>();
- while (!sort.isEmpty()) {
- final CodeReviewCommit n = removeOne(sort);
- collectUninterestingParents(n, uninterestingParents);
-
- rw.resetRetain(canMergeFlag);
- rw.markStart(n);
- if (initialTip != null) {
- rw.markUninteresting(initialTip);
- }
-
- CodeReviewCommit c;
- final List<CodeReviewCommit> contents = new ArrayList<>();
- while ((c = rw.next()) != null) {
- collectUninterestingParents(c, uninterestingParents);
-
- if (!c.has(canMergeFlag) || !incoming.contains(c)) {
- if (isMergedInBranchAsSubmittedChange(c, n.change().getDest())
- || isAlreadyMergedInAnyBranch(c)) {
- rw.markUninteresting(c);
- } else {
- // We cannot merge n as it would bring something we
- // aren't permitted to merge at this time. Drop n.
- //
- n.setStatusCode(CommitMergeStatus.MISSING_DEPENDENCY);
- n.setStatusMessage(
- CommitMergeStatus.createMissingDependencyMessage(
- caller, queryProvider, n.name(), c.name()));
- }
- // Stop RevWalk because c is either a merged commit or a missing
- // dependency. Not need to walk further.
- break;
- }
- contents.add(c);
- }
-
- if (n.getStatusCode() == CommitMergeStatus.MISSING_DEPENDENCY) {
- continue;
- }
-
- sort.removeAll(contents);
- Collections.reverse(contents);
- sorted.removeAll(contents);
- sorted.addAll(contents);
- }
- sorted.removeAll(uninterestingParents);
- return sorted;
- }
-
- /**
- * Rebasing merge commits is done by rebasing their first parent commit, i.e. the first parent is
- * updated to the new base while the second parent stays intact. This means for chains of changes
- * we only need to rebase changes that are reachable via the first parents. Changes that are
- * reachable via second parents do not need to be rebased (since the second parent of merge
- * commits stays intact) which is why we filter them out here by marking them as uninteresting.
- */
- private void collectUninterestingParents(CodeReviewCommit c, Set<ObjectId> uninterestingParents)
- throws IOException {
- if (c.getParentCount() > 0) {
- for (int parent = 1; parent < c.getParentCount(); parent++) {
- uninterestingParents.add(c.getParent(parent));
- rw.markUninteresting(c.getParent(parent));
- }
- }
- }
-
- private boolean isAlreadyMergedInAnyBranch(CodeReviewCommit commit) throws IOException {
- try (CodeReviewRevWalk mirw = CodeReviewCommit.newRevWalk(rw.getObjectReader())) {
- mirw.reset();
- mirw.markStart(commit);
- // check if the commit is merged in other branches
- for (RevCommit accepted : alreadyAccepted) {
- if (mirw.isMergedInto(mirw.parseCommit(commit), mirw.parseCommit(accepted))) {
- logger.atFine().log(
- "Dependency %s merged into branch head %s.", commit.getName(), accepted.getName());
- return true;
- }
- }
- return false;
- } catch (StorageException e) {
- throw new IOException(e);
- }
- }
-
- private boolean isMergedInBranchAsSubmittedChange(CodeReviewCommit commit, BranchNameKey dest) {
- List<ChangeData> changes = queryProvider.get().byBranchCommit(dest, commit.getId().getName());
- for (ChangeData change : changes) {
- if (change.change().isMerged()) {
- logger.atFine().log(
- "Dependency %s associated with merged change %s.", commit.getName(), change.getId());
- return true;
- }
- }
- return false;
- }
-
- private static <T> T removeOne(Collection<T> c) {
- final Iterator<T> i = c.iterator();
- final T r = i.next();
- i.remove();
- return r;
- }
-}
diff --git a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
index 9ba1f48..e3d7fc4 100644
--- a/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
+++ b/java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
@@ -15,12 +15,9 @@
package com.google.gerrit.server.submit;
import static com.google.common.base.Preconditions.checkState;
-import static com.google.common.collect.ImmutableList.toImmutableList;
-import static com.google.gerrit.server.experiments.ExperimentFeaturesConstants.REBASE_MERGE_COMMITS;
import static com.google.gerrit.server.submit.CommitMergeStatus.EMPTY_COMMIT;
import com.google.common.collect.ImmutableList;
-import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.BooleanProjectConfig;
import com.google.gerrit.entities.PatchSet;
@@ -43,13 +40,10 @@
import java.io.IOException;
import java.util.Collection;
import java.util.List;
-import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
/** This strategy covers RebaseAlways and RebaseIfNecessary ones. */
public class RebaseSubmitStrategy extends SubmitStrategy {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
private final boolean rebaseAlways;
RebaseSubmitStrategy(SubmitStrategy.Arguments args, boolean rebaseAlways) {
@@ -59,55 +53,13 @@
@Override
public ImmutableList<SubmitStrategyOp> buildOps(Collection<CodeReviewCommit> toMerge) {
- boolean rebaseMergeCommits =
- args.experimentFeatures.isFeatureEnabled(REBASE_MERGE_COMMITS, args.destBranch.project());
- logger.atFine().log("rebaseMergeCommits=%s", rebaseMergeCommits);
-
List<CodeReviewCommit> sorted;
try {
- sorted =
- rebaseMergeCommits ? args.rebaseSorterNew.sort(toMerge) : args.rebaseSorter.sort(toMerge);
+ sorted = args.rebaseSorter.sort(toMerge);
} catch (IOException | StorageException e) {
throw new StorageException("Commit sorting failed", e);
}
- if (!rebaseMergeCommits) {
- // We cannot rebase merge commits. This is why we integrate merge changes into the target
- // branch the same way as if MERGE_IF_NECESSARY was the submit strategy. This means if needed
- // we create a merge commit that integrates the merge change into the target branch.
- // If we integrate a change series that consists out of a normal change and a merge change,
- // where the merge change depends on the normal change, we must skip rebasing the normal
- // change, because it already gets integrated by merging the merge change. If the rebasing of
- // the normal change is not skipped, it would appear twice in the history after the submit is
- // done (once through its rebased commit, and once through its original commit which is a
- // parent of the merge change that was merged into the target branch. To skip the rebasing of
- // the normal change, we call MergeUtil#reduceToMinimalMerge, as it excludes commits which
- // will be implicitly integrated by merging the series. Then we use the MergeIfNecessaryOp to
- // integrate the whole series.
- // If on the other hand, we integrate a change series that consists out of a merge change and
- // a normal change, where the normal change depends on the merge change, we can first
- // integrate the merge change by a merge and then integrate the normal change by a rebase. In
- // this case we do not want to call MergeUtil#reduceToMinimalMerge as we are not intending to
- // integrate the whole series by a merge, but rather do the integration of the commits one by
- // one.
- boolean foundNonMerge = false;
- for (CodeReviewCommit c : sorted) {
- if (c.getParentCount() > 1) {
- if (!foundNonMerge) {
- // found a merge change, but it doesn't depend on a normal change, this means we are not
- // required to merge the whole series at once
- continue;
- }
- // found a merge commit that depends on a normal change, this means we are required to
- // merge
- // the whole series at once
- sorted = args.mergeUtil.reduceToMinimalMerge(args.mergeSorter, sorted);
- return sorted.stream().map(n -> new MergeIfNecessaryOp(n)).collect(toImmutableList());
- }
- foundNonMerge = true;
- }
- }
-
ImmutableList.Builder<SubmitStrategyOp> ops =
ImmutableList.builderWithExpectedSize(sorted.size());
boolean first = true;
@@ -119,12 +71,8 @@
ops.add(new FastForwardOp(args, n));
} else if (n.getParentCount() == 0) {
ops.add(new RebaseRootOp(n));
- } else if (rebaseMergeCommits) {
- ops.add(new RebaseOneOp(n));
- } else if (n.getParentCount() == 1) {
- ops.add(new RebaseOneOp(n));
} else {
- ops.add(new MergeIfNecessaryOp(n));
+ ops.add(new RebaseOneOp(n));
}
first = false;
}
@@ -270,49 +218,6 @@
}
}
- private class MergeIfNecessaryOp extends SubmitStrategyOp {
- private MergeIfNecessaryOp(CodeReviewCommit toMerge) {
- super(RebaseSubmitStrategy.this.args, toMerge);
- }
-
- @Override
- public void updateRepoImpl(RepoContext ctx) throws IntegrationConflictException, IOException {
- // There are multiple parents, so this is a merge commit. We don't want
- // to rebase the merge as clients can't easily rebase their history with
- // that merge present and replaced by an equivalent merge with a different
- // first parent. So instead behave as though MERGE_IF_NECESSARY was
- // configured.
- // TODO(tandrii): this is not in spirit of RebaseAlways strategy because
- // the commit messages can not be modified in the process. It's also
- // possible to implement rebasing of merge commits. E.g., the Cherry Pick
- // REST endpoint already supports cherry-picking of merge commits.
- // For now, users of RebaseAlways strategy for whom changed commit footers
- // are important would be well advised to prohibit uploading patches with
- // merge commits.
- MergeTip mergeTip = args.mergeTip;
- if (args.rw.isMergedInto(mergeTip.getCurrentTip(), toMerge)
- && !args.subscriptionGraph.hasSubscription(args.destBranch)) {
- mergeTip.moveTipTo(toMerge, toMerge);
- } else {
- PersonIdent caller = ctx.newCommitterIdent();
- CodeReviewCommit newTip =
- args.mergeUtil.mergeOneCommit(
- caller,
- caller,
- args.rw,
- ctx.getInserter(),
- ctx.getRepoView().getConfig(),
- args.destBranch,
- mergeTip.getCurrentTip(),
- toMerge);
- mergeTip.moveTipTo(amendGitlink(newTip), toMerge);
- }
- args.mergeUtil.markCleanMerges(
- args.rw, args.canMergeFlag, mergeTip.getCurrentTip(), args.alreadyAccepted);
- acceptMergeTip(mergeTip);
- }
- }
-
private void acceptMergeTip(MergeTip mergeTip) {
args.alreadyAccepted.add(mergeTip.getCurrentTip());
}
diff --git a/java/com/google/gerrit/server/submit/SubmitStrategy.java b/java/com/google/gerrit/server/submit/SubmitStrategy.java
index 4b95685..d4dd67a 100644
--- a/java/com/google/gerrit/server/submit/SubmitStrategy.java
+++ b/java/com/google/gerrit/server/submit/SubmitStrategy.java
@@ -38,7 +38,6 @@
import com.google.gerrit.server.change.RebaseChangeOp;
import com.google.gerrit.server.change.SetPrivateOp;
import com.google.gerrit.server.change.TestSubmitInput;
-import com.google.gerrit.server.experiments.ExperimentFeatures;
import com.google.gerrit.server.extensions.events.ChangeMerged;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
@@ -127,7 +126,6 @@
final ProjectConfig.Factory projectConfigFactory;
final SetPrivateOp.Factory setPrivateOpFactory;
final SubmitWithStickyApprovalDiff submitWithStickyApprovalDiff;
- final ExperimentFeatures experimentFeatures;
final BranchNameKey destBranch;
final CodeReviewRevWalk rw;
@@ -145,7 +143,6 @@
final ProjectState project;
final MergeSorter mergeSorter;
final RebaseSorter rebaseSorter;
- final RebaseSorterNew rebaseSorterNew;
final MergeUtil mergeUtil;
final boolean dryrun;
@@ -170,7 +167,6 @@
ProjectConfig.Factory projectConfigFactory,
SetPrivateOp.Factory setPrivateOpFactory,
SubmitWithStickyApprovalDiff submitWithStickyApprovalDiff,
- ExperimentFeatures experimentFeatures,
@Assisted BranchNameKey destBranch,
@Assisted CommitStatus commitStatus,
@Assisted CodeReviewRevWalk rw,
@@ -201,7 +197,6 @@
this.queryProvider = queryProvider;
this.setPrivateOpFactory = setPrivateOpFactory;
this.submitWithStickyApprovalDiff = submitWithStickyApprovalDiff;
- this.experimentFeatures = experimentFeatures;
this.serverIdent = serverIdent;
this.destBranch = destBranch;
@@ -231,15 +226,6 @@
canMergeFlag,
queryProvider,
incoming);
- this.rebaseSorterNew =
- new RebaseSorterNew(
- caller,
- rw,
- mergeTip.getInitialTip(),
- alreadyAccepted,
- canMergeFlag,
- queryProvider,
- incoming);
this.mergeUtil = mergeUtilFactory.create(project);
this.onSubmitValidatorsFactory = onSubmitValidatorsFactory;
}
diff --git a/javatests/com/google/gerrit/acceptance/api/config/ListExperimentsIT.java b/javatests/com/google/gerrit/acceptance/api/config/ListExperimentsIT.java
index a8b3e60..fe3cb00 100644
--- a/javatests/com/google/gerrit/acceptance/api/config/ListExperimentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/config/ListExperimentsIT.java
@@ -52,7 +52,6 @@
.GERRIT_BACKEND_FEATURE_ALWAYS_REJECT_IMPLICIT_MERGES_ON_MERGE,
ExperimentFeaturesConstants.GERRIT_BACKEND_FEATURE_ATTACH_NONCE_TO_DOCUMENTATION,
ExperimentFeaturesConstants.GERRIT_BACKEND_FEATURE_CHECK_IMPLICIT_MERGES_ON_MERGE,
- ExperimentFeaturesConstants.REBASE_MERGE_COMMITS,
ExperimentFeaturesConstants.GERRIT_BACKEND_FEATURE_REJECT_IMPLICIT_MERGES_ON_MERGE)
.inOrder();
@@ -88,7 +87,6 @@
.GERRIT_BACKEND_FEATURE_ATTACH_NONCE_TO_DOCUMENTATION)
.enabled)
.isFalse();
- assertThat(experiments.get(ExperimentFeaturesConstants.REBASE_MERGE_COMMITS).enabled).isFalse();
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/git/ImplicitMergeOnSubmitByCherryPickOrRebaseAlwaysIT.java b/javatests/com/google/gerrit/acceptance/git/ImplicitMergeOnSubmitByCherryPickOrRebaseAlwaysIT.java
index 78c2786..592220e 100644
--- a/javatests/com/google/gerrit/acceptance/git/ImplicitMergeOnSubmitByCherryPickOrRebaseAlwaysIT.java
+++ b/javatests/com/google/gerrit/acceptance/git/ImplicitMergeOnSubmitByCherryPickOrRebaseAlwaysIT.java
@@ -18,11 +18,9 @@
import static com.google.common.truth.TruthJUnit.assume;
import com.google.common.collect.ImmutableMap;
-import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ChangeInfo;
-import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
import com.google.gerrit.testing.ConfigSuite;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -78,9 +76,6 @@
}
@Test
- @GerritConfig(
- name = "experiments.enabled",
- value = ExperimentFeaturesConstants.REBASE_MERGE_COMMITS)
public void doesntAddContentFromParentForImplicitMergeChange() throws Exception {
gApi.changes().id(implicitMergeChangeId).current().submit();
diff --git a/javatests/com/google/gerrit/acceptance/git/ImplicitMergeOnSubmitExperimentsIT.java b/javatests/com/google/gerrit/acceptance/git/ImplicitMergeOnSubmitExperimentsIT.java
index 6739071..d85e613 100644
--- a/javatests/com/google/gerrit/acceptance/git/ImplicitMergeOnSubmitExperimentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/git/ImplicitMergeOnSubmitExperimentsIT.java
@@ -60,7 +60,9 @@
// uses @RunWith(ConfigSuite.class). Emulate parameters using configs.
ImmutableMap.Builder<String, Config> builder = ImmutableMap.builder();
for (SubmitType submitType : SubmitType.values()) {
- if (submitType == SubmitType.INHERIT || submitType == SubmitType.CHERRY_PICK) {
+ if (submitType == SubmitType.INHERIT
+ || submitType == SubmitType.CHERRY_PICK
+ || submitType == SubmitType.REBASE_ALWAYS) {
continue;
}
Config cfg = new Config();
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
index b9c07a2..25af040 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
@@ -93,7 +93,6 @@
import com.google.gerrit.server.change.MergeabilityComputationBehavior;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.change.TestSubmitInput;
-import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
import com.google.gerrit.server.git.validators.OnSubmitValidationListener;
import com.google.gerrit.server.index.change.ChangeIndexer;
import com.google.gerrit.server.notedb.ChangeNotes;
@@ -689,36 +688,6 @@
List<RevCommit> log = getRemoteLog();
assertThat(log).contains(stable.getCommit());
- assertThat(log).contains(mergeReview.getCommit());
- }
-
- @Test
- @GerritConfig(
- name = "experiments.enabled",
- value = ExperimentFeaturesConstants.REBASE_MERGE_COMMITS)
- public void submitMergeOfNonChangeBranchTip_withRebasingMergeCommits() throws Throwable {
- // Merge a branch with commits that have not been submitted as
- // changes.
- //
- // M -- mergeCommit (pushed for review and submitted)
- // | \
- // | S -- stable (pushed directly to refs/heads/stable)
- // | /
- // I -- master
- //
- RevCommit master = projectOperations.project(project).getHead("master");
- PushOneCommit stableTip =
- pushFactory.create(admin.newIdent(), testRepo, "Tip of branch stable", "stable.txt", "");
- PushOneCommit.Result stable = stableTip.to("refs/heads/stable");
- PushOneCommit mergeCommit =
- pushFactory.create(admin.newIdent(), testRepo, "The merge commit", "merge.txt", "");
- mergeCommit.setParents(ImmutableList.of(master, stable.getCommit()));
- PushOneCommit.Result mergeReview = mergeCommit.to("refs/for/master");
- approve(mergeReview.getChangeId());
- submit(mergeReview.getChangeId());
-
- List<RevCommit> log = getRemoteLog();
- assertThat(log).contains(stable.getCommit());
if (getSubmitType() == SubmitType.REBASE_ALWAYS) {
// the merge commit has been rebased
@@ -773,53 +742,6 @@
List<RevCommit> log = getRemoteLog();
assertThat(log).contains(s1.getCommit());
- assertThat(log).contains(mergeReview.getCommit());
- }
-
- @Test
- @GerritConfig(
- name = "experiments.enabled",
- value = ExperimentFeaturesConstants.REBASE_MERGE_COMMITS)
- public void submitMergeOfNonChangeBranchNonTip_withRebasingMergeCommits() throws Throwable {
- // Merge a branch with commits that have not been submitted as
- // changes.
- //
- // MC -- merge commit (pushed for review and submitted)
- // |\ S2 -- new stable tip (pushed directly to refs/heads/stable)
- // M \ /
- // | S1 -- stable (pushed directly to refs/heads/stable)
- // | /
- // I -- master
- //
- RevCommit initial = projectOperations.project(project).getHead("master");
- // push directly to stable to S1
- PushOneCommit.Result s1 =
- pushFactory
- .create(admin.newIdent(), testRepo, "new commit into stable", "stable1.txt", "")
- .to("refs/heads/stable");
- // move the stable tip ahead to S2
- pushFactory
- .create(admin.newIdent(), testRepo, "Tip of branch stable", "stable2.txt", "")
- .to("refs/heads/stable");
-
- testRepo.reset(initial);
-
- // move the master ahead
- PushOneCommit.Result m =
- pushFactory
- .create(admin.newIdent(), testRepo, "Move master ahead", "master.txt", "")
- .to("refs/heads/master");
-
- // create merge change
- PushOneCommit mc =
- pushFactory.create(admin.newIdent(), testRepo, "The merge commit", "merge.txt", "");
- mc.setParents(ImmutableList.of(m.getCommit(), s1.getCommit()));
- PushOneCommit.Result mergeReview = mc.to("refs/for/master");
- approve(mergeReview.getChangeId());
- submit(mergeReview.getChangeId());
-
- List<RevCommit> log = getRemoteLog();
- assertThat(log).contains(s1.getCommit());
if (getSubmitType() == SubmitType.REBASE_ALWAYS) {
// the merge commit has been rebased
@@ -1059,81 +981,6 @@
assertMerged(mergeId);
testRepo.git().fetch().call();
RevWalk rw = testRepo.getRevWalk();
- master = rw.parseCommit(projectOperations.project(project).getHead("master"));
- assertThat(rw.isMergedInto(merge, master)).isTrue();
- assertThat(rw.isMergedInto(fix, master)).isTrue();
- }
-
- @Test
- @GerritConfig(
- name = "experiments.enabled",
- value = ExperimentFeaturesConstants.REBASE_MERGE_COMMITS)
- public void submitWithCommitAndItsMergeCommitTogether_withRebasingMergeCommits()
- throws Throwable {
- assume().that(isSubmitWholeTopicEnabled()).isTrue();
-
- RevCommit initialHead = projectOperations.project(project).getHead("master");
-
- // Create a stable branch and bootstrap it.
- gApi.projects().name(project.get()).branch("stable").create(new BranchInput());
- PushOneCommit push =
- pushFactory.create(user.newIdent(), testRepo, "initial commit", "a.txt", "a");
- PushOneCommit.Result change = push.to("refs/heads/stable");
-
- RevCommit stable = projectOperations.project(project).getHead("stable");
- RevCommit master = projectOperations.project(project).getHead("master");
-
- assertThat(master).isEqualTo(initialHead);
- assertThat(stable).isEqualTo(change.getCommit());
-
- testRepo.git().fetch().call();
- testRepo.git().branchCreate().setName("stable").setStartPoint(stable).call();
- testRepo.git().branchCreate().setName("master").setStartPoint(master).call();
-
- // Create a fix in stable branch.
- testRepo.reset(stable);
- RevCommit fix =
- testRepo
- .commit()
- .parent(stable)
- .message("small fix")
- .add("b.txt", "b")
- .insertChangeId()
- .create();
- testRepo.branch("refs/heads/stable").update(fix);
- testRepo
- .git()
- .push()
- .setRefSpecs(new RefSpec("refs/heads/stable:refs/for/stable%topic=" + name("topic")))
- .call();
-
- // Merge the fix into master.
- testRepo.reset(master);
- RevCommit merge =
- testRepo
- .commit()
- .parent(master)
- .parent(fix)
- .message("Merge stable into master")
- .insertChangeId()
- .create();
- testRepo.branch("refs/heads/master").update(merge);
- testRepo
- .git()
- .push()
- .setRefSpecs(new RefSpec("refs/heads/master:refs/for/master%topic=" + name("topic")))
- .call();
-
- // Submit together.
- String fixId = GitUtil.getChangeId(testRepo, fix).get();
- String mergeId = GitUtil.getChangeId(testRepo, merge).get();
- approve(fixId);
- approve(mergeId);
- submit(mergeId);
- assertMerged(fixId);
- assertMerged(mergeId);
- testRepo.git().fetch().call();
- RevWalk rw = testRepo.getRevWalk();
RevCommit newMaster = rw.parseCommit(projectOperations.project(project).getHead("master"));
assertThat(rw.isMergedInto(fix, newMaster)).isTrue();
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java
index 4fbdab4..c34625e 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java
@@ -26,7 +26,6 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestProjectInput;
-import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.BranchNameKey;
@@ -38,7 +37,6 @@
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.server.change.MergeabilityComputationBehavior;
-import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
import com.google.gerrit.server.project.testing.TestLabels;
import com.google.inject.Inject;
import org.eclipse.jgit.lib.ObjectId;
@@ -173,68 +171,6 @@
@Test
public void submitMergeCommitThatDependsOnNormalChangeViaTheFirstParent() throws Throwable {
/*
- * merge created by Gerrit to integrate change 1 and change 2
- /|
- / |
- | |
- * | change3 (new tip)
- | |
- | * change2 (merge)
- | |\
- | | |
- | * | change1
- \|/
- * initialHead
- */
- RevCommit initialHead = projectOperations.project(project).getHead("master");
- PushOneCommit.Result change1 = createChange("Added a", "a.txt", "");
-
- PushOneCommit change2Push =
- pushFactory.create(admin.newIdent(), testRepo, "Merge to master", "m.txt", "");
- change2Push.setParents(ImmutableList.of(change1.getCommit(), initialHead));
- PushOneCommit.Result change2 = change2Push.to("refs/for/master");
-
- testRepo.reset(initialHead);
- PushOneCommit.Result change3 = createChange("New tip", "b.txt", "");
-
- approve(change3.getChangeId());
- submit(change3.getChangeId());
-
- approve(change1.getChangeId());
- approve(change2.getChangeId());
- submit(change2.getChangeId());
-
- RevCommit newHead = projectOperations.project(project).getHead("master");
- assertThat(newHead.getParentCount()).isEqualTo(2);
-
- RevCommit headParent1 = parse(newHead.getParent(0).getId());
- RevCommit headParent2 = parse(newHead.getParent(1).getId());
-
- if (getSubmitType() == SubmitType.REBASE_ALWAYS) {
- assertCurrentRevision(change3.getChangeId(), 2, headParent1.getId());
- } else {
- assertThat(change3.getCommit().getId()).isEqualTo(headParent1.getId());
- }
- assertThat(headParent1.getParentCount()).isEqualTo(1);
- assertThat(headParent1.getParent(0)).isEqualTo(initialHead);
-
- assertThat(headParent2.getId()).isEqualTo(change2.getCommit().getId());
- assertThat(headParent2.getParentCount()).isEqualTo(2);
-
- RevCommit headGrandparent1 = parse(headParent2.getParent(0).getId());
- RevCommit headGrandparent2 = parse(headParent2.getParent(1).getId());
-
- assertThat(headGrandparent1.getId()).isEqualTo(change1.getCommit().getId());
- assertThat(headGrandparent2.getId()).isEqualTo(initialHead.getId());
- }
-
- @Test
- @GerritConfig(
- name = "experiments.enabled",
- value = ExperimentFeaturesConstants.REBASE_MERGE_COMMITS)
- public void submitMergeCommitThatDependsOnNormalChangeViaTheFirstParent_withRebasingMergeCommits()
- throws Throwable {
- /*
* change2 (merge, rebased)
| \
* \ change1 (rebased)
@@ -290,66 +226,6 @@
@Test
public void submitMergeCommitThatDependsOnNormalChangeViaTheSecondParent() throws Throwable {
/*
- * merge created by Gerrit to integrate change 1 and change 2
- |\
- | * change2 (merge)
- | |\
- | | * change1
- | |/
- * | change3 (new tip)
- |/
- * initialHead
- */
- RevCommit initialHead = projectOperations.project(project).getHead("master");
- PushOneCommit.Result change1 = createChange("Added a", "a.txt", "");
-
- PushOneCommit change2Push =
- pushFactory.create(admin.newIdent(), testRepo, "Merge to master", "m.txt", "");
- change2Push.setParents(ImmutableList.of(initialHead, change1.getCommit()));
- PushOneCommit.Result change2 = change2Push.to("refs/for/master");
-
- testRepo.reset(initialHead);
- PushOneCommit.Result change3 = createChange("New tip", "b.txt", "");
-
- approve(change3.getChangeId());
- submit(change3.getChangeId());
-
- approve(change1.getChangeId());
- approve(change2.getChangeId());
- submit(change2.getChangeId());
-
- RevCommit newHead = projectOperations.project(project).getHead("master");
- assertThat(newHead.getParentCount()).isEqualTo(2);
-
- RevCommit headParent1 = parse(newHead.getParent(0).getId());
- RevCommit headParent2 = parse(newHead.getParent(1).getId());
-
- if (getSubmitType() == SubmitType.REBASE_ALWAYS) {
- assertCurrentRevision(change3.getChangeId(), 2, headParent1.getId());
- } else {
- assertThat(change3.getCommit().getId()).isEqualTo(headParent1.getId());
- }
- assertThat(headParent1.getParentCount()).isEqualTo(1);
- assertThat(headParent1.getParent(0)).isEqualTo(initialHead);
-
- assertThat(headParent2.getId()).isEqualTo(change2.getCommit().getId());
- assertThat(headParent2.getParentCount()).isEqualTo(2);
-
- RevCommit headGrandparent1 = parse(headParent2.getParent(0).getId());
- RevCommit headGrandparent2 = parse(headParent2.getParent(1).getId());
-
- assertThat(headGrandparent1.getId()).isEqualTo(initialHead.getId());
- assertThat(headGrandparent2.getId()).isEqualTo(change1.getCommit().getId());
- }
-
- @Test
- @GerritConfig(
- name = "experiments.enabled",
- value = ExperimentFeaturesConstants.REBASE_MERGE_COMMITS)
- public void
- submitMergeCommitThatDependsOnNormalChangeViaTheSecondParent_withRebasingMergeCommits()
- throws Throwable {
- /*
* change2 (merge, rebased)
| \
* \ change3 (new tip, rebased if 'Rebase Always')
