Merge "Bazel: Bump NodeJS version to 20.9.0"
diff --git a/.bazelrc b/.bazelrc
index 6828f9e..d0ce34f 100644
--- a/.bazelrc
+++ b/.bazelrc
@@ -1,3 +1,7 @@
+# TODO(davido): Migrate all dependencies from WORKSPACE to MODULE.bazel
+# https://issues.gerritcodereview.com/issues/303819949
+common --noenable_bzlmod
+
build --workspace_status_command="python3 ./tools/workspace_status.py"
build --repository_cache=~/.gerritcodereview/bazel-cache/repository
build --action_env=PATH
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index df5566f..bcea72c 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -1331,6 +1331,9 @@
Rebases a change.
+For merge commits always the first parent is rebased. This means the new base becomes the first
+parent of the rebased merge commit while the second parent stays intact.
+
If one of the secondary emails associated with the user performing the operation was used as the
committer email in the current patch set, the same email will be used as the committer email in the
new patch set; otherwise, the user's preferred email will be used.
diff --git a/MODULE.bazel b/MODULE.bazel
new file mode 100644
index 0000000..0b932b8
--- /dev/null
+++ b/MODULE.bazel
@@ -0,0 +1,2 @@
+# TODO(davido): Migrate all dependencies from WORKSPACE to MODULE.bazel
+# https://issues.gerritcodereview.com/issues/303819949
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 80582a4..b69f110 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -553,6 +553,19 @@
baseConfig.setInt("index", null, "batchThreads", -1);
+ if (enableExperimentsRejectImplicitMergesOnMerge()) {
+ // When changes are merged/submitted - reject the operation if there is an implicit merge (
+ // even if rejectImplicitMerges is disabled in the project config).
+ baseConfig.setStringList(
+ "experiments",
+ null,
+ "enabled",
+ ImmutableList.of(
+ "GerritBackendFeature__check_implicit_merges_on_merge",
+ "GerritBackendFeature__reject_implicit_merges_on_merge",
+ "GerritBackendFeature__always_reject_implicit_merges_on_merge"));
+ }
+
initServer(classDesc, methodDesc);
server.getTestInjector().injectMembers(this);
@@ -568,6 +581,12 @@
methodDesc.useSystemTime(), methodDesc.useClockStep(), methodDesc.useTimezone());
}
+ protected boolean enableExperimentsRejectImplicitMergesOnMerge() {
+ // By default any attempt to make an explicit merge is rejected. This allows to check
+ // that existing workflows continue to work even if gerrit rejects implicit merges on merge.
+ return true;
+ }
+
protected void setUpDatabase(GerritServer.Description classDesc) throws Exception {
admin = accountCreator.admin();
user = accountCreator.user1();
@@ -1830,7 +1849,7 @@
return new ProjectConfigUpdate(projectName);
}
- protected class ProjectConfigUpdate implements AutoCloseable {
+ public class ProjectConfigUpdate implements AutoCloseable {
private final ProjectConfig projectConfig;
private MetaDataUpdate metaDataUpdate;
diff --git a/java/com/google/gerrit/server/ChangeDraftUpdateExecutor.java b/java/com/google/gerrit/server/ChangeDraftUpdateExecutor.java
index 3ab3a13..9b21851 100644
--- a/java/com/google/gerrit/server/ChangeDraftUpdateExecutor.java
+++ b/java/com/google/gerrit/server/ChangeDraftUpdateExecutor.java
@@ -14,7 +14,7 @@
package com.google.gerrit.server;
-import static autovalue.shaded.com.google$.common.collect.$ImmutableList.toImmutableList;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.MultimapBuilder;
@@ -45,7 +45,17 @@
* </ol>
*/
public interface ChangeDraftUpdateExecutor {
- interface AbstractFactory<T extends ChangeDraftUpdateExecutor> {
+ interface AbstractFactory {
+ // Guice cannot bind either:
+ // - A parameterized entity.
+ // - A factory creating an interface (rather than a class).
+ // To overcome this - we declare the create method in this non-parameterized interface, then
+ // extend it with a factory returning an actual class.
+ ChangeDraftUpdateExecutor create();
+ }
+
+ interface Factory<T extends ChangeDraftUpdateExecutor> extends AbstractFactory {
+ @Override
T create();
}
diff --git a/java/com/google/gerrit/server/change/RebaseChangeOp.java b/java/com/google/gerrit/server/change/RebaseChangeOp.java
index de3b7d5..054a6dc9 100644
--- a/java/com/google/gerrit/server/change/RebaseChangeOp.java
+++ b/java/com/google/gerrit/server/change/RebaseChangeOp.java
@@ -56,6 +56,7 @@
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.io.IOException;
+import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Optional;
@@ -501,9 +502,18 @@
mergeResults);
}
+ List<ObjectId> parents = new ArrayList<>();
+ parents.add(base);
+ if (original.getParentCount() > 1) {
+ // If a merge commit is rebased add all other parents (parent 2 to N).
+ for (int parent = 1; parent < original.getParentCount(); parent++) {
+ parents.add(original.getParent(parent));
+ }
+ }
+
CommitBuilder cb = new CommitBuilder();
cb.setTreeId(tree);
- cb.setParentId(base);
+ cb.setParentIds(parents);
cb.setAuthor(original.getAuthorIdent());
cb.setMessage(commitMessage);
if (committerIdent != null) {
diff --git a/java/com/google/gerrit/server/change/RebaseUtil.java b/java/com/google/gerrit/server/change/RebaseUtil.java
index 47a1e11..2a215c2 100644
--- a/java/com/google/gerrit/server/change/RebaseUtil.java
+++ b/java/com/google/gerrit/server/change/RebaseUtil.java
@@ -252,18 +252,16 @@
String.format("Change %s is %s", change.getId(), ChangeUtil.status(change)));
}
- if (!hasOneParent(rw, patchSet)) {
+ if (!hasAtLeastOneParent(rw, patchSet)) {
throw new ResourceConflictException(
String.format(
- "Error rebasing %s. Cannot rebase %s",
- change.getId(),
- countParents(rw, patchSet) > 1 ? "merge commits" : "commit with no ancestor"));
+ "Error rebasing %s. Cannot rebase commit with no ancestor", change.getId()));
}
}
- public static boolean hasOneParent(RevWalk rw, PatchSet ps) throws IOException {
- // Prevent rebase of exotic changes (merge commit, no ancestor).
- return countParents(rw, ps) == 1;
+ public static boolean hasAtLeastOneParent(RevWalk rw, PatchSet ps) throws IOException {
+ // Prevent rebase of changes with no ancestor.
+ return countParents(rw, ps) >= 1;
}
private static int countParents(RevWalk rw, PatchSet ps) throws IOException {
@@ -487,9 +485,7 @@
ObjectId baseId = null;
RevCommit commit = rw.parseCommit(patchSet.commitId());
- if (commit.getParentCount() > 1) {
- throw new UnprocessableEntityException("Cannot rebase a change with multiple parents.");
- } else if (commit.getParentCount() == 0) {
+ if (commit.getParentCount() == 0) {
throw new UnprocessableEntityException(
"Cannot rebase a change without any parents (is this the initial commit?).");
}
diff --git a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
index 32ec401..52c1f6b 100644
--- a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
+++ b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
@@ -25,4 +25,35 @@
/** Features, enabled by default in the current release. */
public static final ImmutableSet<String> DEFAULT_ENABLED_FEATURES = ImmutableSet.of();
+
+ /**
+ * If true, gerrit checks implicit merges on each merge operations.
+ *
+ * <p>If only this option is set (without {@link
+ * #GERRIT_BACKEND_FEATURE_REJECT_IMPLICIT_MERGES_ON_MERGE}) - then the outcome of the check is
+ * only logged and doesn't block merge operation. Any exceptions during the check are logged and
+ * doesn't block merge operation.
+ */
+ public static String GERRIT_BACKEND_FEATURE_CHECK_IMPLICIT_MERGES_ON_MERGE =
+ "GerritBackendFeature__check_implicit_merges_on_merge";
+
+ /**
+ * If true, gerrit rejects implicit merges on merge.
+ *
+ * <p>Should work together with {@link #GERRIT_BACKEND_FEATURE_CHECK_IMPLICIT_MERGES_ON_MERGE}.
+ *
+ * <p>If {@link #GERRIT_BACKEND_FEATURE_ALWAYS_REJECT_IMPLICIT_MERGES_ON_MERGE} is set to true
+ * then implicit merges are rejected even if rejectImplicitMerges in project config is set to
+ * false.
+ *
+ * <p>If {@link #GERRIT_BACKEND_FEATURE_ALWAYS_REJECT_IMPLICIT_MERGES_ON_MERGE} is set to false
+ * then implicit merges are rejected only if rejectImplicitMerges in project config is set to
+ * true.
+ */
+ public static String GERRIT_BACKEND_FEATURE_REJECT_IMPLICIT_MERGES_ON_MERGE =
+ "GerritBackendFeature__reject_implicit_merges_on_merge";
+
+ /** If true, gerrit ignores rejectImplicitMerges setting from the project config on merge. */
+ public static String GERRIT_BACKEND_FEATURE_ALWAYS_REJECT_IMPLICIT_MERGES_ON_MERGE =
+ "GerritBackendFeature__always_reject_implicit_merges_on_merge";
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeDraftNotesUpdate.java b/java/com/google/gerrit/server/notedb/ChangeDraftNotesUpdate.java
index b32158b..972206a 100644
--- a/java/com/google/gerrit/server/notedb/ChangeDraftNotesUpdate.java
+++ b/java/com/google/gerrit/server/notedb/ChangeDraftNotesUpdate.java
@@ -106,7 +106,7 @@
}
static class Executor implements ChangeDraftUpdateExecutor, AutoCloseable {
- interface Factory extends ChangeDraftUpdateExecutor.AbstractFactory<Executor> {}
+ interface Factory extends ChangeDraftUpdateExecutor.Factory<Executor> {}
private final GitRepositoryManager repoManager;
private final AllUsersName allUsersName;
diff --git a/java/com/google/gerrit/server/restapi/change/Rebase.java b/java/com/google/gerrit/server/restapi/change/Rebase.java
index 98a3f83..9d574a4 100644
--- a/java/com/google/gerrit/server/restapi/change/Rebase.java
+++ b/java/com/google/gerrit/server/restapi/change/Rebase.java
@@ -172,7 +172,7 @@
boolean enabled = false;
try (Repository repo = repoManager.openRepository(change.getDest().project());
RevWalk rw = new RevWalk(repo)) {
- if (RebaseUtil.hasOneParent(rw, rsrc.getPatchSet())) {
+ if (RebaseUtil.hasAtLeastOneParent(rw, rsrc.getPatchSet())) {
enabled = rebaseUtil.canRebase(rsrc.getPatchSet(), change.getDest(), repo, rw);
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/RebaseChain.java b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
index 76c5253..68d3c63 100644
--- a/java/com/google/gerrit/server/restapi/change/RebaseChain.java
+++ b/java/com/google/gerrit/server/restapi/change/RebaseChain.java
@@ -311,7 +311,7 @@
} else {
for (RevisionResource psRsrc : chainAsRevisionResources) {
if (patchSetUtil.isPatchSetLocked(psRsrc.getNotes())
- || !RebaseUtil.hasOneParent(rw, psRsrc.getPatchSet())) {
+ || !RebaseUtil.hasAtLeastOneParent(rw, psRsrc.getPatchSet())) {
enabled = false;
break;
}
diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java
index 2b8a662..7db0c3b 100644
--- a/java/com/google/gerrit/server/submit/MergeOp.java
+++ b/java/com/google/gerrit/server/submit/MergeOp.java
@@ -16,10 +16,15 @@
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.Preconditions.checkArgument;
+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.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;
import static java.util.Comparator.comparing;
import static java.util.Objects.requireNonNull;
+import static java.util.stream.Collectors.joining;
import static java.util.stream.Collectors.toSet;
import com.github.rholder.retry.Attempt;
@@ -33,8 +38,10 @@
import com.google.common.collect.ListMultimap;
import com.google.common.collect.MultimapBuilder;
import com.google.common.collect.SetMultimap;
+import com.google.common.collect.Sets;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.BooleanProjectConfig;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Change.Status;
@@ -63,6 +70,7 @@
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.InternalUser;
import com.google.gerrit.server.change.NotifyResolver;
+import com.google.gerrit.server.experiments.ExperimentFeatures;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.MergeTip;
import com.google.gerrit.server.git.validators.MergeValidationException;
@@ -73,6 +81,7 @@
import com.google.gerrit.server.notedb.StoreSubmitRequirementsOp;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.NoSuchProjectException;
+import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.SubmitRuleOptions;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
@@ -93,13 +102,17 @@
import com.google.inject.Singleton;
import java.io.IOException;
import java.time.Instant;
+import java.util.AbstractMap.SimpleImmutableEntry;
+import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.Deque;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
+import java.util.Map.Entry;
import java.util.Optional;
import java.util.Set;
import java.util.function.Function;
@@ -110,6 +123,7 @@
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
/**
* Merges changes in submission order into a single branch.
@@ -134,6 +148,8 @@
private final ImmutableSetMultimap<BranchNameKey, Change.Id> byBranch;
private final Map<Change.Id, CodeReviewCommit> commits;
private final ListMultimap<Change.Id, String> problems;
+ private final Set<SimpleImmutableEntry<Project.NameKey, BranchNameKey>> implicitMergeProblems;
+
private final boolean allowClosed;
private CommitStatus(ChangeSet cs, boolean allowClosed) {
@@ -147,6 +163,7 @@
byBranch = bb.build();
commits = new HashMap<>();
problems = MultimapBuilder.treeKeys(comparing(Change.Id::get)).arrayListValues(1).build();
+ implicitMergeProblems = new HashSet<>();
this.allowClosed = allowClosed;
}
@@ -181,8 +198,12 @@
problems.put(id, msg);
}
+ public void addImplicitMerge(Project.NameKey projectName, BranchNameKey branchName) {
+ implicitMergeProblems.add(new SimpleImmutableEntry<>(projectName, branchName));
+ }
+
public boolean isOk() {
- return problems.isEmpty();
+ return problems.isEmpty() && implicitMergeProblems.isEmpty();
}
public List<SubmitRecord> getSubmitRecords(Change.Id id) {
@@ -214,6 +235,21 @@
for (Change.Id id : problems.keySet()) {
ps.add("Change " + id + ": " + Joiner.on("; ").join(problems.get(id)));
}
+ if (ps.isEmpty()) {
+ // An implicit merge can be also detected when there are another problems with changes(e.g.
+ // the parent change is deleted). It can confuse the user if gerrit reports both the correct
+ // problem and implicit merge problem at the same time - so report implicit merge problem
+ // only if no other problems are reported.
+ for (SimpleImmutableEntry<Project.NameKey, BranchNameKey> projectBranch :
+ implicitMergeProblems) {
+ // TODO(dmfilippov): Make message more clear to the user and add the exact change id.
+ ps.add(
+ String.format(
+ "submit makes implicit merge to the branch %s of the project %s from some other "
+ + "branch",
+ projectBranch.getValue().shortName(), projectBranch.getKey().get()));
+ }
+ }
throw new ResourceConflictException(msg + Joiner.on('\n').join(ps));
}
@@ -252,6 +288,10 @@
// Changes that were updated by this MergeOp.
private final Map<Change.Id, Change> updatedChanges;
+ private final ExperimentFeatures experimentFeatures;
+
+ private final ProjectCache projectCache;
+
private Instant ts;
private SubmissionId submissionId;
private IdentifiedUser caller;
@@ -283,7 +323,9 @@
RetryHelper retryHelper,
ChangeData.Factory changeDataFactory,
StoreSubmitRequirementsOp.Factory storeSubmitRequirementsOpFactory,
- MergeMetrics mergeMetrics) {
+ MergeMetrics mergeMetrics,
+ ProjectCache projectCache,
+ ExperimentFeatures experimentFeatures) {
this.cmUtil = cmUtil;
this.batchUpdateFactory = batchUpdateFactory;
this.internalUserFactory = internalUserFactory;
@@ -302,6 +344,8 @@
this.updatedChanges = new HashMap<>();
this.storeSubmitRequirementsOpFactory = storeSubmitRequirementsOpFactory;
this.mergeMetrics = mergeMetrics;
+ this.projectCache = projectCache;
+ this.experimentFeatures = experimentFeatures;
}
@Override
@@ -768,6 +812,9 @@
submitting.submitType(),
String.format("null submit type for %s; expected to previously fail fast", submitting));
Set<CodeReviewCommit> commitsToSubmit = submitting.commits();
+ checkImplicitMerges(
+ branch, or.rw, submitting.commits(), submitting.submitType(), ob.oldTip);
+
ob.mergeTip = new MergeTip(ob.oldTip, commitsToSubmit);
SubmitStrategy strategy =
submitStrategyFactory.create(
@@ -793,6 +840,171 @@
return strategies;
}
+ private void checkImplicitMerges(
+ BranchNameKey branch,
+ RevWalk rw,
+ Set<CodeReviewCommit> commitsToSubmit,
+ SubmitType submitType,
+ @Nullable RevCommit branchTip)
+ throws IOException {
+ if (branchTip == null) {
+ // The branch doesn't exist.
+ return;
+ }
+ Project.NameKey project = branch.project();
+ if (!experimentFeatures.isFeatureEnabled(
+ GERRIT_BACKEND_FEATURE_CHECK_IMPLICIT_MERGES_ON_MERGE, project)) {
+ return;
+ }
+ if (submitType == SubmitType.CHERRY_PICK) {
+ return;
+ }
+
+ boolean projectConfigRejectImplicitMerges =
+ projectCache
+ .get(project)
+ .orElseThrow(illegalState(project))
+ .is(BooleanProjectConfig.REJECT_IMPLICIT_MERGES);
+ boolean rejectImplicitMergesOnMerges =
+ experimentFeatures.isFeatureEnabled(
+ GERRIT_BACKEND_FEATURE_REJECT_IMPLICIT_MERGES_ON_MERGE, project)
+ && (experimentFeatures.isFeatureEnabled(
+ GERRIT_BACKEND_FEATURE_ALWAYS_REJECT_IMPLICIT_MERGES_ON_MERGE, project)
+ || projectConfigRejectImplicitMerges);
+ try {
+ if (hasImplicitMerges(branch, rw, commitsToSubmit, branchTip)) {
+ if (rejectImplicitMergesOnMerges) {
+ commitStatus.addImplicitMerge(project, branch);
+ } else {
+ String allCommits =
+ commitsToSubmit.stream()
+ .map(CodeReviewCommit::getId)
+ .map(c -> ObjectId.toString(c))
+ .collect(joining(", "));
+ logger.atWarning().log(
+ "Implicit merge was detected for the branch %s of the project %s. "
+ + "Commits to be merged are: %s",
+ branch.shortName(), project, allCommits);
+ }
+ }
+ } catch (Exception e) {
+ if (rejectImplicitMergesOnMerges) {
+ throw e;
+ }
+ logger.atWarning().withCause(e).log("Error while checking for implicit merges");
+ }
+ }
+
+ private boolean isMergedInBranchAsSubmittedChange(RevCommit 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;
+ }
+
+ /**
+ * Checks if merging {@code commitsToSubmit} into the target branch leads to implicit merge.
+ *
+ * <p>All commits in the {@code commitsToSubmit} have {@code targetBranch} as a target. When
+ * multiple changes are submitted together, the {@code commitsToSubmit} contains transitive
+ * dependencies, not a single change (the method is never called for the cherry pick strategy
+ * because the strategy always submit a single change).
+ */
+ private boolean hasImplicitMerges(
+ BranchNameKey targetBranch,
+ RevWalk rw,
+ Set<CodeReviewCommit> commitsToSubmit,
+ RevCommit branchTip)
+ throws IOException {
+
+ // rootCommits - top level commits in chains. It is all commits which don't have children in
+ // the commitsToSubmit set (no commits have them as parents).
+ Set<CodeReviewCommit> rootCommits = new HashSet<>(commitsToSubmit);
+ Set<RevCommit> allParents = new HashSet<>();
+ for (CodeReviewCommit commit : commitsToSubmit) {
+ rw.parseBody(commit);
+ for (RevCommit parent : commit.getParents()) {
+ rootCommits.remove(parent);
+ allParents.add(parent);
+ }
+ }
+
+ // Calculate all "external" parents of commitsToSubmit - i.e. all parents which already
+ // present in the repository.
+ // targetBranchParents - all "external" parents which were merged into the targetBranch (
+ // they are reachable from the targetBranchTip).
+ Set<RevCommit> targetBranchParents = new HashSet<>();
+ int nonTargetBranchParentsCount = 0;
+ for (RevCommit parent : Sets.difference(allParents, commitsToSubmit)) {
+ if (rw.isMergedInto(parent, branchTip)) {
+ targetBranchParents.add(parent);
+ } else {
+ // Special case: user created chain of changes and then submit first changes from the chain.
+ // It should be allowed for the user to submit remaining changes of the chain without
+ // rebasing them (otherwise votes can be lost).
+ // When a rebase... strategy is used in this scenario, submitting the first few changes of
+ // the chain creates new patchset(s), but all others changes are not rebased on top of new
+ // patchset(s). In this situation isMergedInto check is not enough and additional
+ // isMergedInBranchAsSubmittedChange check should be used.
+ if (isMergedInBranchAsSubmittedChange(parent, targetBranch)) {
+ targetBranchParents.add(parent);
+ } else {
+ nonTargetBranchParentsCount++;
+ }
+ }
+ }
+ if (nonTargetBranchParentsCount == 0) {
+ // All parents are in target branch, no implicit merge is possible.
+ return false;
+ }
+ // There are some parents not in the target branch.
+ if (rootCommits.size() == 1) {
+ // There is only one root commit - this is the case when a single chain of changes is
+ // submitted to the branch.
+ // If the target branch is not reachable from the root commit then it means that there is no
+ // explicit merge with the target branch and the merge operation will create an implicit merge
+ // (except if rebase is used; but for consistency between different strategies we reject
+ // merge even for rebase).
+ return targetBranchParents.isEmpty();
+ }
+ // There are multiple root commits - check that a target branch is reachable from each root
+ // commit. This situation means that multiple chain of changes are submitted (e.g. as a part
+ // of a single topic).
+ // reachableCommits contains pairs of commit: the first item in pair is always one of the root
+ // commits. The second item in pair - a commit reachable from this root (following parents).
+ // Loop implements breadth-search.
+ Deque<Entry<CodeReviewCommit, RevCommit>> reachableCommits =
+ new ArrayDeque<>(rootCommits.size());
+ rootCommits.forEach(commit -> reachableCommits.add(Map.entry(commit, commit)));
+ // Tracks all chains roots which can lead to implicit merge.
+ Set<CodeReviewCommit> implicitMergesRoots = new HashSet<>(rootCommits);
+ while (!reachableCommits.isEmpty()) {
+ Entry<CodeReviewCommit, RevCommit> entry = reachableCommits.pop();
+ if (!implicitMergesRoots.contains(entry.getKey())) {
+ // We already know that from the given root (key in the entry) one of the
+ // targetBranchParents is reachable and this is not an implicit merge.
+ continue;
+ }
+ if (targetBranchParents.contains(entry.getValue())) {
+ // The target branch is reachable from the root. We don't need to process other items
+ // in the queue for this root.
+ implicitMergesRoots.remove(entry.getKey());
+ continue;
+ }
+ for (RevCommit parent : entry.getValue().getParents()) {
+ reachableCommits.push(Map.entry(entry.getKey(), parent));
+ }
+ }
+ // only commits which don't have parents in the targetBranch remains in the implicitMergesRoots.
+ // If there are at least one commit - this is an implicit merge.
+ return !implicitMergesRoots.isEmpty();
+ }
+
private Set<RevCommit> getAlreadyAccepted(OpenRepo or, CodeReviewCommit branchTip) {
Set<RevCommit> alreadyAccepted = new HashSet<>();
diff --git a/java/com/google/gerrit/server/update/BatchUpdate.java b/java/com/google/gerrit/server/update/BatchUpdate.java
index cf9b01b..74911d6 100644
--- a/java/com/google/gerrit/server/update/BatchUpdate.java
+++ b/java/com/google/gerrit/server/update/BatchUpdate.java
@@ -95,8 +95,10 @@
import java.util.Optional;
import java.util.TreeMap;
import java.util.function.Function;
+import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
@@ -310,8 +312,35 @@
@Override
public void addRefUpdate(ReceiveCommand cmd) throws IOException {
+ logger.atFine().log(
+ "Adding ref update: %s: %s %s %s (new tree ID: %s)",
+ cmd.getType().name(),
+ cmd.getOldId().name(),
+ cmd.getNewId().name(),
+ cmd.getRefName(),
+ getNewTreeId(cmd).map(ObjectId::name).orElse("n/a"));
getRepoView().getCommands().add(cmd);
}
+
+ private Optional<ObjectId> getNewTreeId(ReceiveCommand cmd) throws IOException {
+ if (ReceiveCommand.Type.DELETE.equals(cmd.getType())) {
+ // Ref deletions do not have a new tree.
+ return Optional.empty();
+ }
+
+ try {
+ return Optional.of(getRevWalk().parseCommit(cmd.getNewId()).getTree());
+ } catch (MissingObjectException e) {
+ logger.atWarning().withCause(e).log(
+ "Failed parsing new commit %s for ref update (%s: %s %s %s)",
+ cmd.getNewId().name(),
+ cmd.getType().name(),
+ cmd.getOldId().name(),
+ cmd.getNewId().name(),
+ cmd.getRefName());
+ return Optional.empty();
+ }
+ }
}
private class ChangeContextImpl extends ContextImpl implements ChangeContext {
diff --git a/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java b/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
index c637916..af57417 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/RebaseIT.java
@@ -18,12 +18,15 @@
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
+import static com.google.gerrit.extensions.client.ChangeKind.MERGE_FIRST_PARENT_UPDATE;
import static com.google.gerrit.extensions.client.ListChangesOption.ALL_REVISIONS;
import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_COMMIT;
import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVISION;
import static com.google.gerrit.extensions.client.ListChangesOption.DETAILED_LABELS;
import static com.google.gerrit.git.ObjectIds.abbreviateName;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import static com.google.gerrit.server.project.testing.TestLabels.labelBuilder;
+import static com.google.gerrit.server.project.testing.TestLabels.value;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.eclipse.jgit.lib.Constants.HEAD;
@@ -44,8 +47,10 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.LabelId;
+import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Permission;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.changes.AttentionSetInput;
import com.google.gerrit.extensions.api.changes.RebaseInput;
@@ -65,6 +70,7 @@
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.BinaryResult;
+import com.google.gerrit.extensions.restapi.MergeConflictException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -75,6 +81,7 @@
import com.google.gerrit.server.git.validators.CommitValidationMessage;
import com.google.inject.Inject;
import java.io.ByteArrayOutputStream;
+import java.io.IOException;
import java.util.Arrays;
import java.util.List;
import java.util.stream.Collectors;
@@ -145,6 +152,474 @@
}
@Test
+ public void rebaseMerge() throws Exception {
+ // Create a new project for this test so that we can configure a copy condition without
+ // affecting any other tests. Copy Code-Review approvals if change kind is
+ // MERGE_FIRST_PARENT_UPDATE. MERGE_FIRST_PARENT_UPDATE is the change kind when a merge commit
+ // is rebased without conflicts.
+ Project.NameKey project = projectOperations.newProject().create();
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ LabelType.Builder codeReview =
+ labelBuilder(
+ LabelId.CODE_REVIEW,
+ value(2, "Looks good to me, approved"),
+ value(1, "Looks good to me, but someone else must approve"),
+ value(0, "No score"),
+ value(-1, "I would prefer this is not submitted as is"),
+ value(-2, "This shall not be submitted"))
+ .setCopyCondition("changekind:" + MERGE_FIRST_PARENT_UPDATE.name());
+ u.getConfig().upsertLabelType(codeReview.build());
+ u.save();
+ }
+
+ String file1 = "foo/a.txt";
+ String file2 = "bar/b.txt";
+ String file3 = "baz/c.txt";
+
+ // Create an initial change that adds file1, so that we can modify it later.
+ Change.Id initialChange =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .file(file1)
+ .content("base content")
+ .create();
+ approveAndSubmit(initialChange);
+
+ // Create another branch
+ String branchName = "foo";
+ BranchInput branchInput = new BranchInput();
+ branchInput.ref = branchName;
+ branchInput.revision = projectOperations.project(project).getHead("master").name();
+ gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput);
+
+ // Create a change in master that touches file1.
+ Change.Id baseChangeInMaster =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .file(file1)
+ .content("master content")
+ .create();
+ approveAndSubmit(baseChangeInMaster);
+
+ // Create a change in the other branch and that touches file1 and creates file2.
+ Change.Id changeInOtherBranch =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch(branchName)
+ .file(file1)
+ .content("other content")
+ .file(file2)
+ .content("content")
+ .create();
+ approveAndSubmit(changeInOtherBranch);
+
+ // Create a merge change with a conflict resolution for file1. file2 has the same content as
+ // in the other branch (no conflict on file2).
+ Change.Id mergeChangeId =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .mergeOfButBaseOnFirst()
+ .tipOfBranch("master")
+ .and()
+ .tipOfBranch(branchName)
+ .file(file1)
+ .content("merged content")
+ .file(file2)
+ .content("content")
+ .create();
+
+ // Create a change in master onto which the merge change can be rebased. This change touches
+ // an unrelated file (file3) so that there is no conflict on rebase.
+ Change.Id newBaseChangeInMaster =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .file(file3)
+ .content("other content")
+ .create();
+ approveAndSubmit(newBaseChangeInMaster);
+
+ // Add an approval whose score should be copied on rebase.
+ gApi.changes().id(mergeChangeId.get()).current().review(ReviewInput.recommend());
+
+ // Rebase the merge change
+ rebaseCall.call(mergeChangeId.toString());
+
+ verifyRebaseForChange(
+ mergeChangeId,
+ ImmutableList.of(
+ getCurrentRevision(newBaseChangeInMaster), getCurrentRevision(changeInOtherBranch)),
+ /* shouldHaveApproval= */ true,
+ /* expectedNumRevisions= */ 2);
+
+ // Verify the file contents.
+ assertThat(getFileContent(mergeChangeId, file1)).isEqualTo("merged content");
+ assertThat(getFileContent(mergeChangeId, file2)).isEqualTo("content");
+ assertThat(getFileContent(mergeChangeId, file3)).isEqualTo("other content");
+
+ // Rebasing the merge change again should fail
+ verifyChangeIsUpToDate(mergeChangeId.toString());
+ }
+
+ @Test
+ public void rebaseMergeWithConflict_fails() throws Exception {
+ String file1 = "foo/a.txt";
+ String file2 = "bar/b.txt";
+
+ // Create an initial change that adds file1, so that we can modify it later.
+ Change.Id initialChange =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .file(file1)
+ .content("base content")
+ .create();
+ approveAndSubmit(initialChange);
+
+ // Create another branch
+ String branchName = "foo";
+ BranchInput branchInput = new BranchInput();
+ branchInput.ref = branchName;
+ branchInput.revision = projectOperations.project(project).getHead("master").name();
+ gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput);
+
+ // Create a change in master that touches file1.
+ Change.Id baseChangeInMaster =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .file(file1)
+ .content("master content")
+ .create();
+ approveAndSubmit(baseChangeInMaster);
+
+ // Create a change in the other branch and that touches file1 and creates file2.
+ Change.Id changeInOtherBranch =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch(branchName)
+ .file(file1)
+ .content("other content")
+ .file(file2)
+ .content("content")
+ .create();
+ approveAndSubmit(changeInOtherBranch);
+
+ // Create a merge change with a conflict resolution for file1. file2 has the same content as
+ // in the other branch (no conflict on file2).
+ Change.Id mergeChangeId =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .mergeOfButBaseOnFirst()
+ .tipOfBranch("master")
+ .and()
+ .tipOfBranch(branchName)
+ .file(file1)
+ .content("merged content")
+ .file(file2)
+ .content("content")
+ .create();
+
+ // Create a change in master onto which the merge change can be rebased. This change touches
+ // file1 again so that there is a conflict on rebase.
+ Change.Id newBaseChangeInMaster =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .file(file1)
+ .content("conflicting content")
+ .create();
+ approveAndSubmit(newBaseChangeInMaster);
+
+ // Try to rebase the merge change
+ MergeConflictException mergeConflictException =
+ assertThrows(
+ MergeConflictException.class, () -> rebaseCall.call(mergeChangeId.toString()));
+ assertThat(mergeConflictException)
+ .hasMessageThat()
+ .isEqualTo(
+ String.format(
+ "Change %s could not be rebased due to a conflict during merge.\n"
+ + "\n"
+ + "merge conflict(s):\n"
+ + "%s",
+ mergeChangeId, file1));
+ }
+
+ @Test
+ public void rebaseMergeWithConflict_conflictsAllowed() throws Exception {
+ // Create a new project for this test so that we can configure a copy condition without
+ // affecting any other tests. Copy Code-Review approvals if change kind is
+ // MERGE_FIRST_PARENT_UPDATE. MERGE_FIRST_PARENT_UPDATE is the change kind when a merge commit
+ // is rebased without conflicts.
+ Project.NameKey project = projectOperations.newProject().create();
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ LabelType.Builder codeReview =
+ labelBuilder(
+ LabelId.CODE_REVIEW,
+ value(2, "Looks good to me, approved"),
+ value(1, "Looks good to me, but someone else must approve"),
+ value(0, "No score"),
+ value(-1, "I would prefer this is not submitted as is"),
+ value(-2, "This shall not be submitted"))
+ .setCopyCondition("changekind:" + MERGE_FIRST_PARENT_UPDATE.name());
+ u.getConfig().upsertLabelType(codeReview.build());
+ u.save();
+ }
+
+ String file = "foo/a.txt";
+
+ // Create an initial change that adds a file, so that we can modify it later.
+ Change.Id initialChange =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .file(file)
+ .content("base content")
+ .create();
+ approveAndSubmit(initialChange);
+
+ // Create another branch
+ String branchName = "foo";
+ BranchInput branchInput = new BranchInput();
+ branchInput.ref = branchName;
+ branchInput.revision = projectOperations.project(project).getHead("master").name();
+ gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput);
+
+ // Create a change in master that touches the file.
+ Change.Id baseChangeInMaster =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .file(file)
+ .content("master content")
+ .create();
+ approveAndSubmit(baseChangeInMaster);
+
+ // Create a change in the other branch and that also touches the file.
+ Change.Id changeInOtherBranch =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch(branchName)
+ .file(file)
+ .content("other content")
+ .create();
+ approveAndSubmit(changeInOtherBranch);
+
+ // Create a merge change with a conflict resolution.
+ String mergeCommitMessage = "Merge";
+ String mergeContent = "merged content";
+ Change.Id mergeChangeId =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .commitMessage(mergeCommitMessage)
+ .mergeOfButBaseOnFirst()
+ .tipOfBranch("master")
+ .and()
+ .tipOfBranch(branchName)
+ .file(file)
+ .content(mergeContent)
+ .create();
+ String mergeSha1 = abbreviateName(ObjectId.fromString(getCurrentRevision(mergeChangeId)), 6);
+
+ // Create a change in master onto which the merge change can be rebased. This change touches
+ // the file again so that there is a conflict on rebase.
+ String newBaseCommitMessage = "Foo";
+ String newBaseContent = "conflicting content";
+ Change.Id newBaseChangeInMaster =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .commitMessage(newBaseCommitMessage)
+ .file(file)
+ .content(newBaseContent)
+ .create();
+ approveAndSubmit(newBaseChangeInMaster);
+
+ // Add an approval whose score should NOT be copied on rebase (since there is a conflict the
+ // change kind should be REWORK).
+ gApi.changes().id(mergeChangeId.get()).current().review(ReviewInput.recommend());
+
+ // Rebase the merge change with conflicts allowed.
+ TestWorkInProgressStateChangedListener wipStateChangedListener =
+ new TestWorkInProgressStateChangedListener();
+ try (ExtensionRegistry.Registration registration =
+ extensionRegistry.newRegistration().add(wipStateChangedListener)) {
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.allowConflicts = true;
+ rebaseCallWithInput.call(mergeChangeId.toString(), rebaseInput);
+ }
+ assertThat(wipStateChangedListener.invoked).isTrue();
+ assertThat(wipStateChangedListener.wip).isTrue();
+
+ String baseCommit = getCurrentRevision(newBaseChangeInMaster);
+ verifyRebaseForChange(
+ mergeChangeId,
+ ImmutableList.of(baseCommit, getCurrentRevision(changeInOtherBranch)),
+ /* shouldHaveApproval= */ false,
+ /* expectedNumRevisions= */ 2);
+
+ // Verify the file contents.
+ String baseSha1 = abbreviateName(ObjectId.fromString(baseCommit), 6);
+ assertThat(getFileContent(mergeChangeId, file))
+ .isEqualTo(
+ "<<<<<<< PATCH SET ("
+ + mergeSha1
+ + " "
+ + mergeCommitMessage
+ + ")\n"
+ + mergeContent
+ + "\n"
+ + "=======\n"
+ + newBaseContent
+ + "\n"
+ + ">>>>>>> BASE ("
+ + baseSha1
+ + " "
+ + newBaseCommitMessage
+ + ")\n");
+
+ // Verify that a change message has been posted on the change that informs about the conflict
+ // and the outdated vote.
+ List<ChangeMessageInfo> messages = gApi.changes().id(mergeChangeId.get()).messages();
+ assertThat(messages).hasSize(3);
+ assertThat(Iterables.getLast(messages).message)
+ .isEqualTo(
+ "Patch Set 2: Patch Set 1 was rebased\n\n"
+ + "The following files contain Git conflicts:\n"
+ + "* "
+ + file
+ + "\n\n"
+ + "Outdated Votes:\n"
+ + "* Code-Review+1"
+ + " (copy condition: \"changekind:MERGE_FIRST_PARENT_UPDATE\")\n");
+
+ // Rebasing the merge change again should fail
+ verifyChangeIsUpToDate(mergeChangeId.toString());
+ }
+
+ @Test
+ public void rebaseMergeWithConflict_strategyAcceptTheirs() throws Exception {
+ rebaseMergeWithConflict_strategy("theirs");
+ }
+
+ @Test
+ public void rebaseMergeWithConflict_strategyAcceptOurs() throws Exception {
+ rebaseMergeWithConflict_strategy("ours");
+ }
+
+ private void rebaseMergeWithConflict_strategy(String strategy) throws Exception {
+ String file = "foo/a.txt";
+
+ // Create an initial change that adds a file, so that we can modify it later.
+ Change.Id initialChange =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .file(file)
+ .content("base content")
+ .create();
+ approveAndSubmit(initialChange);
+
+ // Create another branch
+ String branchName = "foo";
+ BranchInput branchInput = new BranchInput();
+ branchInput.ref = branchName;
+ branchInput.revision = projectOperations.project(project).getHead("master").name();
+ gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput);
+
+ // Create a change in master that touches the file.
+ Change.Id baseChangeInMaster =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .file(file)
+ .content("master content")
+ .create();
+ approveAndSubmit(baseChangeInMaster);
+
+ // Create a change in the other branch and that also touches the file.
+ Change.Id changeInOtherBranch =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch(branchName)
+ .file(file)
+ .content("other content")
+ .create();
+ approveAndSubmit(changeInOtherBranch);
+
+ // Create a merge change with a conflict resolution for the file.
+ String mergeContent = "merged content";
+ Change.Id mergeChangeId =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .mergeOfButBaseOnFirst()
+ .tipOfBranch("master")
+ .and()
+ .tipOfBranch(branchName)
+ .file(file)
+ .content(mergeContent)
+ .create();
+
+ // Create a change in master onto which the merge change can be rebased. This change touches
+ // the file again so that there is a conflict on rebase.
+ String newBaseContent = "conflicting content";
+ Change.Id newBaseChangeInMaster =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .file(file)
+ .content(newBaseContent)
+ .create();
+ approveAndSubmit(newBaseChangeInMaster);
+
+ // Rebase the merge change with setting a merge strategy
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.strategy = strategy;
+ rebaseCallWithInput.call(mergeChangeId.toString(), rebaseInput);
+
+ verifyRebaseForChange(
+ mergeChangeId,
+ ImmutableList.of(
+ getCurrentRevision(newBaseChangeInMaster), getCurrentRevision(changeInOtherBranch)),
+ /* shouldHaveApproval= */ false,
+ /* expectedNumRevisions= */ 2);
+
+ // Verify the file contents.
+ assertThat(getFileContent(mergeChangeId, file))
+ .isEqualTo(strategy.equals("theirs") ? newBaseContent : mergeContent);
+
+ // Rebasing the merge change again should fail
+ verifyChangeIsUpToDate(mergeChangeId.toString());
+ }
+
+ @Test
public void rebaseWithCommitterEmail() throws Exception {
// Create three changes with the same parent
PushOneCommit.Result r1 = createChange();
@@ -660,6 +1135,23 @@
/* expectedNumRevisions= */ 2);
}
+ protected void approveAndSubmit(Change.Id changeId) throws Exception {
+ approve(Integer.toString(changeId.get()));
+ gApi.changes().id(changeId.get()).current().submit();
+ }
+
+ protected String getCurrentRevision(Change.Id changeId) throws RestApiException {
+ return gApi.changes().id(changeId.get()).get(CURRENT_REVISION).currentRevision;
+ }
+
+ protected String getFileContent(Change.Id changeId, String file)
+ throws RestApiException, IOException {
+ BinaryResult bin = gApi.changes().id(changeId.get()).current().file(file).content();
+ ByteArrayOutputStream os = new ByteArrayOutputStream();
+ bin.writeTo(os);
+ return new String(os.toByteArray(), UTF_8);
+ }
+
protected void verifyRebaseForChange(
Change.Id changeId, Change.Id baseChangeId, boolean shouldHaveApproval)
throws RestApiException {
@@ -672,14 +1164,26 @@
boolean shouldHaveApproval,
int expectedNumRevisions)
throws RestApiException {
- ChangeInfo baseInfo = gApi.changes().id(baseChangeId.get()).get(CURRENT_REVISION);
verifyRebaseForChange(
- changeId, baseInfo.currentRevision, shouldHaveApproval, expectedNumRevisions);
+ changeId,
+ ImmutableList.of(getCurrentRevision(baseChangeId)),
+ shouldHaveApproval,
+ expectedNumRevisions);
}
protected void verifyRebaseForChange(
Change.Id changeId, String baseCommit, boolean shouldHaveApproval, int expectedNumRevisions)
throws RestApiException {
+ verifyRebaseForChange(
+ changeId, ImmutableList.of(baseCommit), shouldHaveApproval, expectedNumRevisions);
+ }
+
+ protected void verifyRebaseForChange(
+ Change.Id changeId,
+ List<String> baseCommits,
+ boolean shouldHaveApproval,
+ int expectedNumRevisions)
+ throws RestApiException {
ChangeInfo info =
gApi.changes().id(changeId.get()).get(CURRENT_REVISION, CURRENT_COMMIT, DETAILED_LABELS);
@@ -688,10 +1192,12 @@
assertThat(r.realUploader).isNull();
// ...and the base should be correct
- assertThat(r.commit.parents).hasSize(1);
- assertWithMessage("base commit for change " + changeId)
- .that(r.commit.parents.get(0).commit)
- .isEqualTo(baseCommit);
+ assertThat(r.commit.parents).hasSize(baseCommits.size());
+ for (int baseNum = 0; baseNum < baseCommits.size(); baseNum++) {
+ assertWithMessage("base commit " + baseNum + " for change " + changeId)
+ .that(r.commit.parents.get(baseNum).commit)
+ .isEqualTo(baseCommits.get(baseNum));
+ }
// ...and the committer and description should be correct
GitPerson committer = r.commit.committer;
@@ -711,8 +1217,12 @@
}
protected void verifyChangeIsUpToDate(PushOneCommit.Result r) {
+ verifyChangeIsUpToDate(r.getChangeId());
+ }
+
+ protected void verifyChangeIsUpToDate(String changeId) {
ResourceConflictException thrown =
- assertThrows(ResourceConflictException.class, () -> rebaseCall.call(r.getChangeId()));
+ assertThrows(ResourceConflictException.class, () -> rebaseCall.call(changeId));
assertThat(thrown).hasMessageThat().contains("Change is already up to date");
}
@@ -1167,9 +1677,9 @@
}
@Override
- protected void verifyChangeIsUpToDate(PushOneCommit.Result r) {
+ protected void verifyChangeIsUpToDate(String changeId) {
ResourceConflictException thrown =
- assertThrows(ResourceConflictException.class, () -> rebaseCall.call(r.getChangeId()));
+ assertThrows(ResourceConflictException.class, () -> rebaseCall.call(changeId));
assertThat(thrown).hasMessageThat().contains("The whole chain is already up to date.");
}
@@ -1294,6 +1804,152 @@
}
@Test
+ public void rebaseChainWithMerges() throws Exception {
+ String file1 = "foo/a.txt";
+ String file2 = "bar/b.txt";
+
+ // Create an initial change that adds file1, so that we can modify it later.
+ Change.Id initialChange =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .file(file1)
+ .content("base content")
+ .create();
+ approveAndSubmit(initialChange);
+
+ // Create another branch
+ String branchName = "foo";
+ BranchInput branchInput = new BranchInput();
+ branchInput.ref = branchName;
+ branchInput.revision = projectOperations.project(project).getHead("master").name();
+ gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput);
+
+ // Create a change in master that touches file1.
+ Change.Id baseChangeInMaster =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .file(file1)
+ .content("master content")
+ .create();
+ approveAndSubmit(baseChangeInMaster);
+
+ // Create a change in the other branch and that also touches file1.
+ Change.Id changeInOtherBranch =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch(branchName)
+ .file(file1)
+ .content("other content")
+ .create();
+ approveAndSubmit(changeInOtherBranch);
+
+ // Create a merge change with a conflict resolution.
+ Change.Id mergeChangeId =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .mergeOfButBaseOnFirst()
+ .tipOfBranch("master")
+ .and()
+ .tipOfBranch(branchName)
+ .file(file1)
+ .content("merged content")
+ .create();
+
+ // Create a follow up change.
+ Change.Id followUpChangeId =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .childOf()
+ .change(mergeChangeId)
+ .file(file1)
+ .content("modified content")
+ .create();
+
+ // Create another change in the other branch so that we can create another merge
+ Change.Id anotherChangeInOtherBranch =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch(branchName)
+ .file(file1)
+ .content("yet another content")
+ .create();
+ approveAndSubmit(anotherChangeInOtherBranch);
+
+ // Create a second merge change with a conflict resolution.
+ Change.Id followUpMergeChangeId =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .childOf()
+ .change(followUpChangeId)
+ .mergeOfButBaseOnFirst()
+ .change(followUpChangeId)
+ .and()
+ .tipOfBranch(branchName)
+ .file(file1)
+ .content("another merged content")
+ .create();
+
+ // Create a change in master onto which the chain can be rebased. This change touches an
+ // unrelated file (file2) so that there is no conflict on rebase.
+ Change.Id newBaseChangeInMaster =
+ changeOperations
+ .newChange()
+ .project(project)
+ .branch("master")
+ .file(file2)
+ .content("other content")
+ .create();
+ approveAndSubmit(newBaseChangeInMaster);
+
+ // Rebase the chain
+ RebaseChainInfo rebaseChainInfo =
+ gApi.changes().id(followUpMergeChangeId.get()).rebaseChain().value();
+ assertThat(rebaseChainInfo.rebasedChanges).hasSize(3);
+ assertThat(rebaseChainInfo.containsGitConflicts).isNull();
+
+ verifyRebaseForChange(
+ mergeChangeId,
+ ImmutableList.of(
+ getCurrentRevision(newBaseChangeInMaster), getCurrentRevision(changeInOtherBranch)),
+ /* shouldHaveApproval= */ false,
+ /* expectedNumRevisions= */ 2);
+ verifyRebaseForChange(
+ followUpChangeId,
+ ImmutableList.of(getCurrentRevision(mergeChangeId)),
+ /* shouldHaveApproval= */ false,
+ /* expectedNumRevisions= */ 2);
+ verifyRebaseForChange(
+ followUpMergeChangeId,
+ ImmutableList.of(
+ getCurrentRevision(followUpChangeId), getCurrentRevision(anotherChangeInOtherBranch)),
+ /* shouldHaveApproval= */ false,
+ /* expectedNumRevisions= */ 2);
+
+ // Verify the file contents.
+ assertThat(getFileContent(mergeChangeId, file1)).isEqualTo("merged content");
+ assertThat(getFileContent(mergeChangeId, file2)).isEqualTo("other content");
+ assertThat(getFileContent(followUpChangeId, file1)).isEqualTo("modified content");
+ assertThat(getFileContent(followUpChangeId, file2)).isEqualTo("other content");
+ assertThat(getFileContent(followUpMergeChangeId, file1)).isEqualTo("another merged content");
+ assertThat(getFileContent(followUpMergeChangeId, file2)).isEqualTo("other content");
+
+ // Rebasing the chain again should fail
+ verifyChangeIsUpToDate(followUpChangeId.toString());
+ }
+
+ @Test
public void rebasePartlyOutdatedChain() throws Exception {
final String file = "modified_file.txt";
final String oldContent = "old content";
diff --git a/javatests/com/google/gerrit/acceptance/api/change/SubmitTypeRuleIT.java b/javatests/com/google/gerrit/acceptance/api/change/SubmitTypeRuleIT.java
index 308e4e0..8e11f70 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/SubmitTypeRuleIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/SubmitTypeRuleIT.java
@@ -28,6 +28,7 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.PushOneCommit.Result;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.changes.ReviewInput;
@@ -89,13 +90,19 @@
}
private AtomicInteger fileCounter;
+
+ // The change is used only to verify that the rule is valid. It is never submitted in the test.
private Change.Id testChangeId;
@Before
public void setUp() throws Exception {
fileCounter = new AtomicInteger();
gApi.projects().name(project.get()).branch("test").create(new BranchInput());
- testChangeId = createChange("test", "test change").getChange().getId();
+ Result testChange = createChange("test", "test change");
+ testChangeId = testChange.getChange().getId();
+ // Reset repo back to the original state - otherwise all changes in tests have testChange as a
+ // parent.
+ testRepo.reset(testChange.getCommit().getParent(0));
}
private void setRulesPl(String rule) throws Exception {
diff --git a/javatests/com/google/gerrit/acceptance/api/project/CommitIT.java b/javatests/com/google/gerrit/acceptance/api/project/CommitIT.java
index 84a4a40..c33175d 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/CommitIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/CommitIT.java
@@ -121,7 +121,10 @@
createBranch(BranchNameKey.create(project, "test-branch-1"));
createBranch(BranchNameKey.create(project, "test-branch-2"));
- createAndSubmitChange("refs/for/test-branch-1");
+ RevCommit changeCommit = createAndSubmitChange("refs/for/test-branch-1").getCommit();
+ // Reset repo back to the original state - otherwise all changes in tests have testChange as a
+ // parent.
+ testRepo.reset(changeCommit.getParent(0));
createAndSubmitChange("refs/for/test-branch-2");
assertThat(getIncludedIn(baseChange.getCommit().getId()).branches)
diff --git a/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java b/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java
index a93c0a5..540e443 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java
@@ -1140,6 +1140,10 @@
}
@Test
+ @GerritConfig(
+ name = "experiments.disabled",
+ // The test intentionally create an implicit merge change.
+ value = "GerritBackendFeature__reject_implicit_merges_on_merge")
public void commitsIncludedInRefsMergedChangeNonTipCommit() throws Exception {
String branchWithChange1 = R_HEADS + "branch-with-change1";
String tagWithChange1 = R_TAGS + "tag-with-change1";
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractImplicitMergeTest.java b/javatests/com/google/gerrit/acceptance/git/AbstractImplicitMergeTest.java
new file mode 100644
index 0000000..fe195f5
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractImplicitMergeTest.java
@@ -0,0 +1,142 @@
+// Copyright (C) 2023 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.acceptance.git;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.entities.BooleanProjectConfig;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.client.InheritableBoolean;
+import com.google.gerrit.extensions.client.SubmitType;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevTree;
+import org.eclipse.jgit.treewalk.TreeWalk;
+import org.eclipse.jgit.treewalk.filter.TreeFilter;
+import org.eclipse.jgit.util.RawParseUtils;
+
+/**
+ * Base class for different tests for implicit merge.
+ *
+ * <p>Provides shared methods for tests changes and branches manipulations.
+ */
+public abstract class AbstractImplicitMergeTest extends AbstractDaemonTest {
+
+ /** Creates and pushes a simple approved changes without files and with specified parents. */
+ protected PushOneCommit.Result createApprovedChange(String targetBranch, RevCommit... parents)
+ throws Exception {
+ PushOneCommit.Result result = pushTo("refs/for/" + targetBranch, ImmutableMap.of(), parents);
+ gApi.changes().id(result.getChangeId()).current().review(ReviewInput.approve());
+ return result;
+ }
+
+ /** Creates and pushes simple approved changes without files and with specified parents. */
+ protected PushOneCommit.Result createApprovedChange(
+ String targetBranch, PushOneCommit.Result... parents) throws Exception {
+ return createApprovedChange(
+ targetBranch,
+ Arrays.stream(parents).map(PushOneCommit.Result::getCommit).toArray(RevCommit[]::new));
+ }
+
+ /** Creates and pushes a commit with specified files and parents. */
+ protected PushOneCommit.Result pushTo(
+ String ref, ImmutableMap<String, String> files, RevCommit... parents) throws Exception {
+ PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo, "Some commit", files);
+ push.setParents(List.of(parents));
+ PushOneCommit.Result result = push.to(ref);
+ result.assertOkStatus();
+ return result;
+ }
+
+ /**
+ * Creates a change in the in-memory repository but doesn't push it to gerrit.
+ *
+ * <p>The method can be used to create chain of changes. The last change in the chain can be
+ * created using {@link #createApprovedChange} or {@link #pushTo} methods - these method will push
+ * the whole chain to gerrit as a single git push operations.
+ */
+ protected RevCommit createChangeWithoutPush(
+ String changeId, ImmutableMap<String, String> files, RevCommit... parents) throws Exception {
+ TestRepository.CommitBuilder commitBuilder =
+ testRepo
+ .commit()
+ .message("Change " + changeId)
+ // The passed changeId starts with 'I', but insertChangeId expects id without 'I'.
+ .insertChangeId(changeId.substring(1));
+ for (RevCommit parent : parents) {
+ commitBuilder.parent(parent);
+ }
+ for (Map.Entry<String, String> entry : files.entrySet()) {
+ commitBuilder.add(entry.getKey(), entry.getValue());
+ }
+
+ return commitBuilder.create();
+ }
+
+ protected void setRejectImplicitMerges() throws Exception {
+ setRejectImplicitMerges(/*reject=*/ true);
+ }
+
+ protected void setRejectImplicitMerges(boolean reject) throws Exception {
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig()
+ .updateProject(
+ p ->
+ p.setBooleanConfig(
+ BooleanProjectConfig.REJECT_IMPLICIT_MERGES,
+ reject ? InheritableBoolean.TRUE : InheritableBoolean.FALSE));
+ u.save();
+ }
+ }
+
+ protected void setSubmitType(SubmitType submitType) throws Exception {
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig().updateProject(p -> p.setSubmitType(submitType));
+ u.save();
+ }
+ }
+
+ protected ImmutableMap<String, String> getRemoteBranchRootPathContent(String refName)
+ throws Exception {
+ String revision = gApi.projects().name(project.get()).branch(refName).get().revision;
+ testRepo.git().fetch().setRemote("origin").call();
+ RevTree revTree =
+ testRepo.getRepository().parseCommit(testRepo.getRepository().resolve(revision)).getTree();
+ try (TreeWalk tw = new TreeWalk(testRepo.getRepository())) {
+ tw.setFilter(TreeFilter.ALL);
+ tw.setRecursive(false);
+ tw.reset(revTree);
+ ImmutableMap.Builder<String, String> builder = ImmutableMap.builder();
+ while (tw.next()) {
+ String path = tw.getPathString();
+ String content =
+ RawParseUtils.decode(
+ testRepo.getRepository().open(tw.getObjectId(0)).getCachedBytes(Integer.MAX_VALUE));
+ builder.put(path, content);
+ }
+ return builder.buildOrThrow();
+ }
+ }
+
+ protected PushOneCommit.Result push(String ref, String subject, String fileName, String content)
+ throws Exception {
+ PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo, subject, fileName, content);
+ return push.to(ref);
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/git/ImplicitMergeCheckIT.java b/javatests/com/google/gerrit/acceptance/git/ImplicitMergeCheckOnReceiveIT.java
similarity index 77%
rename from javatests/com/google/gerrit/acceptance/git/ImplicitMergeCheckIT.java
rename to javatests/com/google/gerrit/acceptance/git/ImplicitMergeCheckOnReceiveIT.java
index e352e2d..90ba7c1 100644
--- a/javatests/com/google/gerrit/acceptance/git/ImplicitMergeCheckIT.java
+++ b/javatests/com/google/gerrit/acceptance/git/ImplicitMergeCheckOnReceiveIT.java
@@ -17,16 +17,14 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.GitUtil.pushHead;
-import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
-import com.google.gerrit.entities.BooleanProjectConfig;
-import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.git.ObjectIds;
import java.util.Locale;
import org.eclipse.jgit.lib.ObjectId;
import org.junit.Test;
-public class ImplicitMergeCheckIT extends AbstractDaemonTest {
+/** Checks that gerrit rejects/accepts implicit merges when receives a git push. */
+public class ImplicitMergeCheckOnReceiveIT extends AbstractImplicitMergeTest {
@Test
public void implicitMergeViaFastForward() throws Exception {
@@ -84,21 +82,4 @@
return "implicit merge of "
+ ObjectIds.abbreviateName(commit, testRepo.getRevWalk().getObjectReader());
}
-
- private void setRejectImplicitMerges() throws Exception {
- try (ProjectConfigUpdate u = updateProject(project)) {
- u.getConfig()
- .updateProject(
- p ->
- p.setBooleanConfig(
- BooleanProjectConfig.REJECT_IMPLICIT_MERGES, InheritableBoolean.TRUE));
- u.save();
- }
- }
-
- private PushOneCommit.Result push(String ref, String subject, String fileName, String content)
- throws Exception {
- PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo, subject, fileName, content);
- return push.to(ref);
- }
}
diff --git a/javatests/com/google/gerrit/acceptance/git/ImplicitMergeOnSubmitCherryPickIT.java b/javatests/com/google/gerrit/acceptance/git/ImplicitMergeOnSubmitCherryPickIT.java
new file mode 100644
index 0000000..d019fb9
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/git/ImplicitMergeOnSubmitCherryPickIT.java
@@ -0,0 +1,80 @@
+// Copyright (C) 2023 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.acceptance.git;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.client.SubmitType;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.junit.Before;
+import org.junit.Test;
+
+/**
+ * Tests how implicit merges are submitted by the cherry pick strategy.
+ *
+ * <p>Verifies that implicit merges can be submitted and that they doesn't add content from the
+ * implicitly merged branch to the target branch.
+ */
+public class ImplicitMergeOnSubmitCherryPickIT extends AbstractImplicitMergeTest {
+
+ private String implicitMergeChangeId;
+
+ @Before
+ public void setUp() throws Exception {
+ setRejectImplicitMerges(false);
+ setSubmitType(SubmitType.CHERRY_PICK);
+ RevCommit base = repo().parseCommit(repo().exactRef("HEAD").getObjectId());
+ RevCommit masterBranchTip =
+ pushTo("refs/heads/master", ImmutableMap.of("master-content", "master-first-line\n"), base)
+ .getCommit();
+ pushTo("refs/heads/stable", ImmutableMap.of("stable-content", "stable-first-line\n"), base)
+ .getCommit();
+ implicitMergeChangeId =
+ pushTo(
+ "refs/for/stable",
+ ImmutableMap.of("master-content2", "added-by-implicit-merge\n"),
+ masterBranchTip)
+ .getChangeId();
+ gApi.changes().id(implicitMergeChangeId).current().review(ReviewInput.approve());
+ }
+
+ @Test
+ public void doesntAddContentFromParentForImplicitMergeChange() throws Exception {
+ gApi.changes().id(implicitMergeChangeId).current().submit();
+
+ ChangeInfo ci = gApi.changes().id(implicitMergeChangeId).info();
+ assertThat(ci.submitted).isNotNull();
+ assertThat(ci.submitter).isNotNull();
+ assertThat(ci.submitter._accountId).isEqualTo(atrScope.get().getUser().getAccountId().get());
+
+ assertThat(getRemoteBranchRootPathContent("refs/heads/stable"))
+ .containsExactly(
+ "master-content2", "added-by-implicit-merge\n",
+ "stable-content", "stable-first-line\n");
+ }
+
+ @Test
+ public void canSubmitImplicitMergeChange() throws Exception {
+ gApi.changes().id(implicitMergeChangeId).current().submit();
+
+ ChangeInfo ci = gApi.changes().id(implicitMergeChangeId).info();
+ assertThat(ci.submitted).isNotNull();
+ assertThat(ci.submitter).isNotNull();
+ assertThat(ci.submitter._accountId).isEqualTo(atrScope.get().getUser().getAccountId().get());
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/git/ImplicitMergeOnSubmitExperimentsIT.java b/javatests/com/google/gerrit/acceptance/git/ImplicitMergeOnSubmitExperimentsIT.java
new file mode 100644
index 0000000..d778ef2
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/git/ImplicitMergeOnSubmitExperimentsIT.java
@@ -0,0 +1,331 @@
+// Copyright (C) 2023 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.acceptance.git;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.TruthJUnit.assume;
+import static com.google.gerrit.server.util.CommitMessageUtil.generateChangeId;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+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.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.testing.ConfigSuite;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.junit.Before;
+import org.junit.Test;
+
+/**
+ * Verifies that gerrit correctly rejects or submits implicit merges depending on experiments.
+ *
+ * <p>All tests use the same commit configuration (master branch is one commit ahead of stable
+ * branch):
+ *
+ * <pre>{@code
+ * change[1] (target - stable, explicit merge of stable branch and master branches)
+ * | \
+ * | change[0] (target - stable, i.e. implicit merge of master and stable branches)
+ * | |
+ * | master
+ * | |
+ * stable <--- |
+ * }</pre>
+ */
+public class ImplicitMergeOnSubmitExperimentsIT extends AbstractImplicitMergeTest {
+ @Override
+ protected boolean enableExperimentsRejectImplicitMergesOnMerge() {
+ // Tests uses own experiment setup.
+ return false;
+ }
+
+ @ConfigSuite.Configs
+ public static ImmutableMap<String, Config> configs() {
+ // The @RunWith(Parameterized.class) can't be used, because AbstractDaemonClass already
+ // 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) {
+ continue;
+ }
+ Config cfg = new Config();
+ cfg.setString("test", null, "submitType", submitType.name());
+ builder.put(String.format("submitType=%s", submitType), cfg);
+ }
+ return builder.buildOrThrow();
+ }
+
+ private String implicitMergeChangeId;
+ private String explicitMergeChangeId;
+
+ private SubmitType submitType;
+
+ @Before
+ public void setUp() throws Exception {
+ // The ConfigSuite runner always adds a default config. Ignore it (submitType is not set for
+ // it).
+ assume().that(cfg.getString("test", null, "submitType")).isNotEmpty();
+ RevCommit base = repo().parseCommit(repo().exactRef("HEAD").getObjectId());
+ RevCommit stableBranchTip =
+ pushTo("refs/heads/stable", ImmutableMap.of("stable-content", "stable-first-line\n"), base)
+ .getCommit();
+ RevCommit masterBranchTip =
+ pushTo(
+ "refs/heads/master",
+ ImmutableMap.of("master-content", "master-first-line\n"),
+ stableBranchTip)
+ .getCommit();
+ implicitMergeChangeId = "I" + generateChangeId().name();
+ RevCommit implicitMergeChange =
+ createChangeWithoutPush(
+ implicitMergeChangeId,
+ ImmutableMap.of("master-content2", "added-by-implicit-merge\n"),
+ masterBranchTip);
+ explicitMergeChangeId =
+ pushTo(
+ "refs/for/stable",
+ ImmutableMap.of("stable-content", "stable-first-line\nadded-by-explicit-merge\n"),
+ implicitMergeChange,
+ stableBranchTip)
+ .getChangeId();
+ gApi.changes().id(implicitMergeChangeId).current().review(ReviewInput.approve());
+ gApi.changes().id(explicitMergeChangeId).current().review(ReviewInput.approve());
+ submitType = SubmitType.valueOf(cfg.getString("test", null, "submitType"));
+ setSubmitType(submitType);
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ values = {
+ "GerritBackendFeature__check_implicit_merges_on_merge",
+ "GerritBackendFeature__reject_implicit_merges_on_merge",
+ "GerritBackendFeature__always_reject_implicit_merges_on_merge"
+ })
+ public void alwaysRejectOnMerge_rejectImplicitMergeFalse_rejectImplicitMergeOnSubmit()
+ throws Exception {
+ setRejectImplicitMerges(/*reject=*/ false);
+ assertThatImplicitMergeSubmitRejected();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ values = {
+ "GerritBackendFeature__check_implicit_merges_on_merge",
+ "GerritBackendFeature__reject_implicit_merges_on_merge",
+ "GerritBackendFeature__always_reject_implicit_merges_on_merge"
+ })
+ public void alwaysRejectOnMerge_rejectImplicitMergeFalse_canSubmitExplicitMerge()
+ throws Exception {
+ setRejectImplicitMerges(/*reject=*/ false);
+ assertThatExcplicitMergeSubmitAllowed();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ values = {
+ "GerritBackendFeature__check_implicit_merges_on_merge",
+ "GerritBackendFeature__reject_implicit_merges_on_merge",
+ "GerritBackendFeature__always_reject_implicit_merges_on_merge"
+ })
+ public void alwaysRejectOnMerge_rejectImplicitMergeTrue_rejectImplicitMergeOnSubmit()
+ throws Exception {
+ setRejectImplicitMerges(/*reject=*/ true);
+ assertThatImplicitMergeSubmitRejected();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ values = {
+ "GerritBackendFeature__check_implicit_merges_on_merge",
+ "GerritBackendFeature__reject_implicit_merges_on_merge",
+ "GerritBackendFeature__always_reject_implicit_merges_on_merge"
+ })
+ public void alwaysRejectOnMerge_rejectImplicitMergeTrue_canSubmitExplicitMerge()
+ throws Exception {
+ setRejectImplicitMerges(/*reject=*/ true);
+ assertThatExcplicitMergeSubmitAllowed();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ values = {
+ "GerritBackendFeature__check_implicit_merges_on_merge",
+ "GerritBackendFeature__reject_implicit_merges_on_merge",
+ })
+ public void rejectOnMerge_rejectImplicitMergeFalse_canSubmitImplicitMerge() throws Exception {
+ setRejectImplicitMerges(/*reject=*/ false);
+ assertThatImplicitMergeSubmitAllowed();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ values = {
+ "GerritBackendFeature__check_implicit_merges_on_merge",
+ "GerritBackendFeature__reject_implicit_merges_on_merge",
+ })
+ public void rejectOnMerge_rejectImplicitMergeFalse_canSubmitExplicitMerge() throws Exception {
+ setRejectImplicitMerges(/*reject=*/ false);
+ assertThatExcplicitMergeSubmitAllowed();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ values = {
+ "GerritBackendFeature__check_implicit_merges_on_merge",
+ "GerritBackendFeature__reject_implicit_merges_on_merge",
+ })
+ public void rejectOnMerge_rejectImplicitMergeTrue_rejectImplicitMergeOnSubmit() throws Exception {
+ setRejectImplicitMerges(/*reject=*/ true);
+ assertThatImplicitMergeSubmitRejected();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ values = {
+ "GerritBackendFeature__check_implicit_merges_on_merge",
+ "GerritBackendFeature__reject_implicit_merges_on_merge",
+ })
+ public void rejectOnMerge_rejectImplicitMergeTrue_canSubmitExplicitMerge() throws Exception {
+ setRejectImplicitMerges(/*reject=*/ true);
+ assertThatExcplicitMergeSubmitAllowed();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ values = {
+ "GerritBackendFeature__check_implicit_merges_on_merge",
+ })
+ public void checkOnly_rejectImplicitMergeFalse_canSubmitImplicitMerge() throws Exception {
+ setRejectImplicitMerges(/*reject=*/ false);
+ assertThatImplicitMergeSubmitAllowed();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ values = {
+ "GerritBackendFeature__check_implicit_merges_on_merge",
+ })
+ public void checkOnly_rejectImplicitMergeFalse_canSubmitExplicitMerge() throws Exception {
+ setRejectImplicitMerges(/*reject=*/ false);
+ assertThatExcplicitMergeSubmitAllowed();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ values = {
+ "GerritBackendFeature__check_implicit_merges_on_merge",
+ })
+ public void checkOnly_rejectImplicitMergeTrue_canSubmitImplicitMerge() throws Exception {
+ setRejectImplicitMerges(/*reject=*/ true);
+ assertThatImplicitMergeSubmitAllowed();
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ values = {
+ "GerritBackendFeature__check_implicit_merges_on_merge",
+ })
+ public void checkOnly_rejectImplicitMergeTrue_canSubmitExplicitMerge() throws Exception {
+ setRejectImplicitMerges(/*reject=*/ true);
+ assertThatExcplicitMergeSubmitAllowed();
+ }
+
+ @Test
+ public void noExperiments_rejectImplicitMergeFalse_canSubmitImplicitMerge() throws Exception {
+ setRejectImplicitMerges(/*reject=*/ false);
+ assertThatImplicitMergeSubmitAllowed();
+ }
+
+ @Test
+ public void noExperiments_rejectImplicitMergeFalse_canSubmitExplicitMerge() throws Exception {
+ setRejectImplicitMerges(/*reject=*/ false);
+ assertThatExcplicitMergeSubmitAllowed();
+ }
+
+ @Test
+ public void noExperiments_rejectImplicitMergeTrue_canSubmitImplicitMerge() throws Exception {
+ setRejectImplicitMerges(/*reject=*/ true);
+ assertThatImplicitMergeSubmitAllowed();
+ }
+
+ @Test
+ public void noExperiments_rejectImplicitMergeTrue_canSubmitExplicitMerge() throws Exception {
+ setRejectImplicitMerges(/*reject=*/ true);
+ assertThatExcplicitMergeSubmitAllowed();
+ }
+
+ private void assertThatImplicitMergeSubmitRejected() throws Exception {
+ ResourceConflictException e =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(implicitMergeChangeId).current().submit());
+ assertThat(e.getMessage().toLowerCase()).contains("submit makes implicit merge to the branch");
+ ChangeInfo ci = gApi.changes().id(implicitMergeChangeId).info();
+ assertThat(ci.submitted).isNull();
+ assertThat(getRemoteBranchRootPathContent("refs/heads/stable"))
+ .containsExactly("stable-content", "stable-first-line\n");
+ }
+
+ private void assertThatImplicitMergeSubmitAllowed() throws Exception {
+ gApi.changes().id(implicitMergeChangeId).current().submit();
+
+ ChangeInfo ci = gApi.changes().id(implicitMergeChangeId).info();
+ assertThat(ci.submitted).isNotNull();
+ assertThat(ci.submitter).isNotNull();
+ assertThat(ci.submitter._accountId).isEqualTo(atrScope.get().getUser().getAccountId().get());
+
+ if (submitType != SubmitType.REBASE_ALWAYS) {
+ assertThat(getRemoteBranchRootPathContent("refs/heads/stable"))
+ .containsExactly(
+ "master-content", "master-first-line\n",
+ "master-content2", "added-by-implicit-merge\n",
+ "stable-content", "stable-first-line\n");
+ } else {
+ assertThat(getRemoteBranchRootPathContent("refs/heads/stable"))
+ .containsExactly(
+ "master-content2", "added-by-implicit-merge\n",
+ "stable-content", "stable-first-line\n");
+ }
+ }
+
+ private void assertThatExcplicitMergeSubmitAllowed() throws Exception {
+ gApi.changes().id(explicitMergeChangeId).current().submit();
+
+ ChangeInfo ci = gApi.changes().id(explicitMergeChangeId).info();
+ assertThat(ci.submitted).isNotNull();
+ assertThat(ci.submitter).isNotNull();
+ assertThat(ci.submitter._accountId).isEqualTo(atrScope.get().getUser().getAccountId().get());
+ assertThat(getRemoteBranchRootPathContent("refs/heads/stable"))
+ .containsExactly(
+ "master-content", "master-first-line\n",
+ "master-content2", "added-by-implicit-merge\n",
+ "stable-content", "stable-first-line\nadded-by-explicit-merge\n");
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/git/ImplicitMergeOnSubmitIT.java b/javatests/com/google/gerrit/acceptance/git/ImplicitMergeOnSubmitIT.java
new file mode 100644
index 0000000..e69d54b
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/git/ImplicitMergeOnSubmitIT.java
@@ -0,0 +1,321 @@
+// Copyright (C) 2023 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.acceptance.git;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.extensions.api.projects.BranchInput;
+import com.google.gerrit.extensions.client.SubmitType;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.junit.Before;
+import org.junit.Test;
+
+/**
+ * Verifies that gerrit correctly detects implicit merges on submit..
+ *
+ * <p>The setup creates a repository with 2 branches: master and target. Both branches have common
+ * parent:
+ *
+ * <pre>{@code
+ * master target
+ * | |
+ * ----->base commit <------
+ * }</pre>
+ *
+ * Tests use only MergeAlways strategy. All other submit strategies (except cherry pick) use the
+ * same checks on submit. The {@link ImplicitMergeOnSubmitExperimentsIT} validates that the implicit
+ * merge check is applied to all strategies (except cherry pick) and {@link
+ * ImplicitMergeOnSubmitCherryPickIT} contains tests for the cherry pick strategy.
+ */
+public class ImplicitMergeOnSubmitIT extends AbstractImplicitMergeTest {
+ private RevCommit masterTip;
+ private RevCommit otherTip;
+ RevCommit baseCommit;
+
+ @Before
+ public void setUp() throws Exception {
+ setSubmitType(SubmitType.MERGE_ALWAYS);
+ gApi.projects().name(project.get()).branch("other").create(new BranchInput());
+ baseCommit =
+ repo()
+ .parseCommit(
+ ObjectId.fromString(
+ gApi.projects().name(project.get()).branch("master").get().revision));
+ masterTip =
+ pushTo("refs/heads/master", ImmutableMap.of("master-file", "master-content"), baseCommit)
+ .getCommit();
+ otherTip =
+ pushTo("refs/heads/other", ImmutableMap.of("target-file", "target1-content"), baseCommit)
+ .getCommit();
+ }
+
+ @Test
+ public void singleChangeImplicitMerge() throws Exception {
+ PushOneCommit.Result implicitMerge = createApprovedChange("master", otherTip);
+ assertSubmitRejectedWithImplicitMerge(implicitMerge.getChangeId());
+ }
+
+ @Test
+ public void chainOfChangesImplicitMerge() throws Exception {
+ PushOneCommit.Result implicitMerge = createApprovedChange("master", otherTip);
+ PushOneCommit.Result c1 = createApprovedChange("master", implicitMerge);
+ PushOneCommit.Result c2 = createApprovedChange("master", c1);
+ assertSubmitRejectedWithImplicitMerge(implicitMerge.getChangeId());
+ assertSubmitRejectedWithImplicitMerge(c1.getChangeId());
+ assertSubmitRejectedWithImplicitMerge(c2.getChangeId());
+ }
+
+ @Test
+ public void chainOfChangesOnTopOfTargetBranchTipNoImplicitMerge() throws Exception {
+ PushOneCommit.Result c1 = createApprovedChange("master", masterTip);
+ PushOneCommit.Result c2 = createApprovedChange("master", c1);
+ PushOneCommit.Result c3 = createApprovedChange("master", c2);
+ assertThatChangeSubmittable(c1.getChangeId());
+ assertThatChangeSubmittable(c2.getChangeId());
+ assertThatChangeSubmittable(c3.getChangeId());
+ }
+
+ @Test
+ public void chainOfChangesNotOnTopOfTargetBranchTipNoImplicitMerge() throws Exception {
+ // Add one more commit to master branch.
+ pushTo("refs/heads/master", ImmutableMap.of(), masterTip);
+ PushOneCommit.Result c1 = createApprovedChange("master", masterTip);
+ PushOneCommit.Result c2 = createApprovedChange("master", c1);
+ PushOneCommit.Result c3 = createApprovedChange("master", c2);
+ assertThatChangeSubmittable(c1.getChangeId());
+ assertThatChangeSubmittable(c2.getChangeId());
+ assertThatChangeSubmittable(c3.getChangeId());
+ }
+
+ @Test
+ public void chainOfChangesNotOnTopOfTargetBranchTipWithImplicitMerge() throws Exception {
+ // Add one more commit to master branch.
+ pushTo("refs/heads/master", ImmutableMap.of(), masterTip);
+ PushOneCommit.Result implicitMerge = createApprovedChange("master", otherTip);
+ PushOneCommit.Result c2 = createApprovedChange("master", implicitMerge);
+ PushOneCommit.Result c3 = createApprovedChange("master", c2);
+ assertSubmitRejectedWithImplicitMerge(implicitMerge.getChangeId());
+ assertSubmitRejectedWithImplicitMerge(c2.getChangeId());
+ assertSubmitRejectedWithImplicitMerge(c3.getChangeId());
+ }
+
+ @Test
+ public void chainOfChangesEndsWithExplicitMerge_onlyExplcitMergeCanBeSubmitted()
+ throws Exception {
+ PushOneCommit.Result implicitMerge = createApprovedChange("master", otherTip);
+ PushOneCommit.Result changeInChange = createApprovedChange("master", implicitMerge);
+ PushOneCommit.Result explicitMerge =
+ createApprovedChange("master", changeInChange.getCommit(), masterTip);
+ assertSubmitRejectedWithImplicitMerge(implicitMerge.getChangeId());
+ assertSubmitRejectedWithImplicitMerge(changeInChange.getChangeId());
+ assertThatChangeSubmittable(explicitMerge.getChangeId());
+ }
+
+ @Test
+ public void twoChainOfChangesSameTopic_oneChainImplicitMerge_rejectedOnSubmit() throws Exception {
+ cfg.setBoolean("change", null, "submitWholeTopic", true);
+ PushOneCommit.Result c1 = createApprovedChange("master", masterTip);
+ PushOneCommit.Result c2 = createApprovedChange("master", c1);
+ PushOneCommit.Result c3 = createApprovedChange("master", c2.getCommit());
+ PushOneCommit.Result implicitMerge = createApprovedChange("master", otherTip);
+ PushOneCommit.Result im1 = createApprovedChange("master", implicitMerge);
+ PushOneCommit.Result im2 = createApprovedChange("master", im1.getCommit());
+ // The AbstractDaemonTest doesn't fully reset gerrit; it creates a new project for each test
+ // and doesn't remove changes created in tests. As a result, if the same topic is used in
+ // several tests gerrit tries to submit all changes, including changes from other tests.
+ // The name method returns name scoped to this test method .
+ String topic = name("topic");
+ gApi.changes().id(c1.getChangeId()).topic(topic);
+ gApi.changes().id(c2.getChangeId()).topic(topic);
+ gApi.changes().id(c3.getChangeId()).topic(topic);
+ gApi.changes().id(implicitMerge.getChangeId()).topic(topic);
+ gApi.changes().id(im1.getChangeId()).topic(topic);
+ gApi.changes().id(im2.getChangeId()).topic(topic);
+
+ assertSubmitRejectedWithImplicitMerge(c1.getChangeId());
+ }
+
+ @Test
+ public void twoChainOfChangesSameTopic_noImplicitMerge_canSubmit() throws Exception {
+ cfg.setBoolean("change", null, "submitWholeTopic", true);
+ PushOneCommit.Result chain1change1 = createApprovedChange("master", masterTip);
+ PushOneCommit.Result chain1change2 = createApprovedChange("master", chain1change1);
+ PushOneCommit.Result chain1change3 = createApprovedChange("master", chain1change2);
+ PushOneCommit.Result chain2change1 = createApprovedChange("master", masterTip);
+ PushOneCommit.Result chain2change2 = createApprovedChange("master", chain2change1);
+ PushOneCommit.Result chain2change3 = createApprovedChange("master", chain2change2);
+ // The AbstractDaemonTest doesn't fully reset gerrit; it creates a new project for each test
+ // and doesn't remove changes created in tests. As a result, if the same topic is used in
+ // several tests gerrit tries to submit all changes, including changes from other tests.
+ // The name method returns name scoped to this test method .
+ String topic = name("topic");
+ gApi.changes().id(chain1change1.getChangeId()).topic(topic);
+ gApi.changes().id(chain1change2.getChangeId()).topic(topic);
+ gApi.changes().id(chain1change3.getChangeId()).topic(topic);
+ gApi.changes().id(chain2change1.getChangeId()).topic(topic);
+ gApi.changes().id(chain2change2.getChangeId()).topic(topic);
+ gApi.changes().id(chain2change3.getChangeId()).topic(topic);
+
+ assertThatChangeSubmittable(chain1change1.getChangeId());
+ }
+
+ @Test
+ public void twoChainOfChangesSameTopicNotOnTopOfBranch_noImplicitMerge_canSubmit()
+ throws Exception {
+ // Add one more commit to master branch.
+ pushTo("refs/heads/master", ImmutableMap.of(), masterTip);
+ cfg.setBoolean("change", null, "submitWholeTopic", true);
+ PushOneCommit.Result chain1change1 = createApprovedChange("master", masterTip);
+ PushOneCommit.Result chain1change2 = createApprovedChange("master", chain1change1);
+ PushOneCommit.Result chain1change3 = createApprovedChange("master", chain1change2);
+ PushOneCommit.Result chain2change1 = createApprovedChange("master", masterTip);
+ PushOneCommit.Result chain2change2 = createApprovedChange("master", chain2change1);
+ PushOneCommit.Result chain2change3 = createApprovedChange("master", chain2change2);
+ // The AbstractDaemonTest doesn't fully reset gerrit; it creates a new project for each test
+ // and doesn't remove changes created in tests. As a result, if the same topic is used in
+ // several tests gerrit tries to submit all changes, including changes from other tests.
+ // The name method returns name scoped to this test method .
+ String topic = name("topic");
+ gApi.changes().id(chain1change1.getChangeId()).topic(topic);
+ gApi.changes().id(chain1change2.getChangeId()).topic(topic);
+ gApi.changes().id(chain1change3.getChangeId()).topic(topic);
+ gApi.changes().id(chain2change1.getChangeId()).topic(topic);
+ gApi.changes().id(chain2change2.getChangeId()).topic(topic);
+ gApi.changes().id(chain2change3.getChangeId()).topic(topic);
+
+ assertThatChangeSubmittable(chain1change1.getChangeId());
+ }
+
+ @Test
+ public void twoChainOfChangesEndsWithExplicitMergeSameTopicNotTipOfBranches_canBeSubmitted()
+ throws Exception {
+ cfg.setBoolean("change", null, "submitWholeTopic", true);
+ // Add one more commit to master branch.
+ pushTo("refs/heads/master", ImmutableMap.of(), masterTip);
+ PushOneCommit.Result implicitMerge1 = createApprovedChange("master", otherTip);
+ PushOneCommit.Result changeInChain1 = createApprovedChange("master", implicitMerge1);
+ PushOneCommit.Result explicitMerge1 =
+ createApprovedChange("master", changeInChain1.getCommit(), masterTip);
+ PushOneCommit.Result implicitMerge2 = createApprovedChange("master", otherTip);
+ PushOneCommit.Result changeInChain2 = createApprovedChange("master", implicitMerge2);
+ PushOneCommit.Result explicitMerge2 =
+ createApprovedChange("master", changeInChain2.getCommit(), masterTip);
+ // The AbstractDaemonTest doesn't fully reset gerrit; it creates a new project for each test
+ // and doesn't remove changes created in tests. As a result, if the same topic is used in
+ // several tests gerrit tries to submit all changes, including changes from other tests.
+ // The name method returns name scoped to this test method .
+ String topic = name("topic");
+ gApi.changes().id(implicitMerge1.getChangeId()).topic(topic);
+ gApi.changes().id(changeInChain1.getChangeId()).topic(topic);
+ gApi.changes().id(explicitMerge1.getChangeId()).topic(topic);
+ gApi.changes().id(implicitMerge2.getChangeId()).topic(topic);
+ gApi.changes().id(changeInChain2.getChangeId()).topic(topic);
+ gApi.changes().id(explicitMerge2.getChangeId()).topic(topic);
+
+ assertThatChangeSubmittable(explicitMerge2.getChangeId());
+ }
+
+ @Test
+ public void twoChainOfChangesDifferentBranchesSameTopic_oneChainImplicitMerge_rejectedOnSubmit()
+ throws Exception {
+ cfg.setBoolean("change", null, "submitWholeTopic", true);
+ PushOneCommit.Result implicitMerge = createApprovedChange("other", masterTip);
+ PushOneCommit.Result im1 = createApprovedChange("other", implicitMerge);
+ PushOneCommit.Result im2 = createApprovedChange("other", im1.getCommit());
+ PushOneCommit.Result c1 = createApprovedChange("master", masterTip);
+ PushOneCommit.Result c2 = createApprovedChange("master", c1);
+ PushOneCommit.Result c3 = createApprovedChange("master", c2.getCommit());
+ // The AbstractDaemonTest doesn't fully reset gerrit; it creates a new project for each test
+ // and doesn't remove changes created in tests. As a result, if the same topic is used in
+ // several tests gerrit tries to submit all changes, including changes from other tests.
+ // The name method returns name scoped to this test method .
+ String topic = name("topic");
+ gApi.changes().id(implicitMerge.getChangeId()).topic(topic);
+ gApi.changes().id(im1.getChangeId()).topic(topic);
+ gApi.changes().id(im2.getChangeId()).topic(topic);
+ gApi.changes().id(c1.getChangeId()).topic(topic);
+ gApi.changes().id(c2.getChangeId()).topic(topic);
+ gApi.changes().id(c3.getChangeId()).topic(topic);
+
+ assertSubmitRejectedWithImplicitMerge(implicitMerge.getChangeId());
+ }
+
+ @Test
+ public void explicitMergeOnTopOfChain_onlyTopSubmittable() throws Exception {
+ PushOneCommit.Result implicitMerge = createApprovedChange("master", otherTip);
+ PushOneCommit.Result im1 = createApprovedChange("master", implicitMerge);
+ PushOneCommit.Result im2 = createApprovedChange("master", im1.getCommit());
+ PushOneCommit.Result explicitMerge = createApprovedChange("master", masterTip, im2.getCommit());
+
+ assertSubmitRejectedWithImplicitMerge(implicitMerge.getChangeId());
+ assertSubmitRejectedWithImplicitMerge(im1.getChangeId());
+ assertSubmitRejectedWithImplicitMerge(im2.getChangeId());
+ assertThatChangeSubmittable(explicitMerge.getChangeId());
+ }
+
+ @Test
+ public void explicitMergeOnTopOfChainParentIsNotBranchTip_onlyTopSubmittable() throws Exception {
+ // Add one more commit to master and other branches.
+ pushTo("refs/heads/master", ImmutableMap.of(), masterTip);
+ pushTo("refs/heads/other", ImmutableMap.of(), otherTip);
+
+ PushOneCommit.Result implicitMerge = createApprovedChange("master", otherTip);
+ PushOneCommit.Result im1 = createApprovedChange("master", implicitMerge);
+ PushOneCommit.Result im2 = createApprovedChange("master", im1.getCommit());
+ PushOneCommit.Result explicitMerge = createApprovedChange("master", masterTip, im2.getCommit());
+
+ assertSubmitRejectedWithImplicitMerge(implicitMerge.getChangeId());
+ assertSubmitRejectedWithImplicitMerge(im1.getChangeId());
+ assertSubmitRejectedWithImplicitMerge(im2.getChangeId());
+ assertThatChangeSubmittable(explicitMerge.getChangeId());
+ }
+
+ @Test
+ public void threeBranches_onlyExplicitCommitSubmittable() throws Exception {
+ BranchInput bi = new BranchInput();
+ bi.revision = baseCommit.getName();
+ gApi.projects().name(project.get()).branch("third").create(bi);
+ RevCommit thirdBranchTip =
+ pushTo("refs/heads/third", ImmutableMap.of("third-file", "third-content"), baseCommit)
+ .getCommit();
+
+ PushOneCommit.Result explicitMerge = createApprovedChange("master", masterTip, otherTip);
+ PushOneCommit.Result implicitMerge = createApprovedChange("master", otherTip, thirdBranchTip);
+ PushOneCommit.Result explicitMerge2 =
+ createApprovedChange("master", explicitMerge, implicitMerge);
+
+ assertSubmitRejectedWithImplicitMerge(implicitMerge.getChangeId());
+ assertThatChangeSubmittable(explicitMerge.getChangeId());
+ assertThatChangeSubmittable(explicitMerge2.getChangeId());
+ }
+
+ private void assertSubmitRejectedWithImplicitMerge(String changeId) throws Exception {
+ ResourceConflictException e =
+ assertThrows(
+ ResourceConflictException.class, () -> gApi.changes().id(changeId).current().submit());
+ assertThat(e.getMessage()).contains("implicit merge");
+ }
+
+ private void assertThatChangeSubmittable(String changeId) throws Exception {
+ ChangeInfo ci = gApi.changes().id(changeId).current().submit();
+ assertThat(ci.submitted).isNotNull();
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
index 3bec694..0dd026f 100644
--- a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
+++ b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
@@ -163,8 +163,6 @@
// (c3_open) (c4_open)
//
private void setUpChanges() throws Exception {
- gApi.projects().name(project.get()).branch("branch").create(new BranchInput());
-
// First 2 changes are merged, which means the tags pointing to them are
// visible.
projectOperations
@@ -183,6 +181,9 @@
metaRef1 = RefNames.changeMetaRef(cd1.getId());
// rcMaster (c1 master) <-- rcBranch (c2 branch)
+ BranchInput branchInput = new BranchInput();
+ branchInput.revision = mr.getCommit().getName();
+ gApi.projects().name(project.get()).branch("branch").create(branchInput);
PushOneCommit.Result br =
pushFactory.create(admin.newIdent(), testRepo).to("refs/for/branch%submit");
br.assertOkStatus();
@@ -196,7 +197,7 @@
// \
// (c3_open)
//
- mr = pushFactory.create(admin.newIdent(), testRepo).to("refs/for/master");
+ mr = pushFactory.create(admin.newIdent(), testRepo).setParent(rcMaster).to("refs/for/master");
mr.assertOkStatus();
cd3 = mr.getChange();
psRef3 = cd3.currentPatchSet().id().toRefName();
@@ -205,7 +206,7 @@
// rcMaster (c1 master) <-- rcBranch (c2 branch)
// \ \
// (c3_open) (c4_open)
- br = pushFactory.create(admin.newIdent(), testRepo).to("refs/for/branch");
+ br = pushFactory.create(admin.newIdent(), testRepo).setParent(rcBranch).to("refs/for/branch");
br.assertOkStatus();
cd4 = br.getChange();
psRef4 = cd4.currentPatchSet().id().toRefName();
diff --git a/javatests/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java b/javatests/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java
index 0d751f1..dd079de 100644
--- a/javatests/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java
+++ b/javatests/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java
@@ -645,12 +645,19 @@
allowMatchingSubmoduleSubscription(subKey, "refs/heads/master", superKey, "refs/heads/master");
allowMatchingSubmoduleSubscription(superKey, "refs/heads/dev", subKey, "refs/heads/dev");
+ // Create 'dev' branches in both repos by pushing changes.
pushChangeTo(subRepo, "dev");
pushChangeTo(superRepo, "dev");
createSubmoduleSubscription(superRepo, "master", subKey, "master");
createSubmoduleSubscription(subRepo, "dev", superKey, "dev");
+ // Reset the state of local repositories to avoid implicit merge changes.
+ subRepo.git().fetch();
+ subRepo.reset(subRepo.git().getRepository().findRef("origin/master").getObjectId().getName());
+ superRepo.git().fetch();
+ superRepo.reset(superRepo.git().getRepository().findRef("origin/dev").getObjectId().getName());
+
ObjectId subMasterHead =
pushChangeTo(
subRepo, "refs/for/master", "b.txt", "content b", "some message", "same-topic");
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
index ac3622f..dccc057 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SubmitByMergeIfNecessaryIT.java
@@ -26,6 +26,7 @@
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.PushOneCommit;
+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;
@@ -42,6 +43,7 @@
import com.google.inject.Inject;
import java.util.List;
import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.transport.RefSpec;
import org.junit.Test;
@@ -280,6 +282,10 @@
}
@Test
+ @GerritConfig(
+ name = "experiments.disabled",
+ // The test intentionally create an implicit merge change.
+ value = "GerritBackendFeature__reject_implicit_merges_on_merge")
public void submitWithMergedAncestorsOnOtherBranch() throws Throwable {
RevCommit initialHead = projectOperations.project(project).getHead("master");
@@ -329,6 +335,10 @@
}
@Test
+ @GerritConfig(
+ name = "experiments.disabled",
+ // The test intentionally create an implicit merge change.
+ value = "GerritBackendFeature__reject_implicit_merges_on_merge")
public void submitWithOpenAncestorsOnOtherBranch() throws Throwable {
RevCommit initialHead = projectOperations.project(project).getHead("master");
PushOneCommit.Result change1 =
@@ -549,19 +559,27 @@
PushOneCommit.Result changeResult = change.to("refs/for/master");
approve(changeResult.getChangeId());
- // Create a successor change.
+ // Create a destination branch that later will be made non-visible to user.
+ BranchNameKey secretBranch = BranchNameKey.create(project, "secretBranch");
+ String secretBranchTip =
+ gApi.projects()
+ .name(secretBranch.project().get())
+ .branch(secretBranch.branch())
+ .create(new BranchInput())
+ .get()
+ .revision;
+
+ // Create a successor change which merges visible and non-visible branch. This change
+ // is created as an explicit merge - otherwise Gerrit rejects it on submit as implicit merge.
PushOneCommit change2 =
pushFactory.create(admin.newIdent(), testRepo, "feature", "b.txt", "bar");
+ change2.setParents(
+ List.of(
+ changeResult.getCommit(), repo().parseCommit(ObjectId.fromString(secretBranchTip))));
PushOneCommit.Result change2Result = change2.to("refs/for/master");
-
- // Move the first change to a destination branch that is non-visible to user so that user cannot
- // this change anymore.
- BranchNameKey secretBranch = BranchNameKey.create(project, "secretBranch");
- gApi.projects()
- .name(secretBranch.project().get())
- .branch(secretBranch.branch())
- .create(new BranchInput());
gApi.changes().id(changeResult.getChangeId()).move(secretBranch.branch());
+
+ // Hide branch from the user so that user cannot this change anymore.
projectOperations
.project(project)
.forUpdate()
diff --git a/javatests/com/google/gerrit/acceptance/server/experiments/ExperimentFeaturesIT.java b/javatests/com/google/gerrit/acceptance/server/experiments/ExperimentFeaturesIT.java
index e011ffc..e172153 100644
--- a/javatests/com/google/gerrit/acceptance/server/experiments/ExperimentFeaturesIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/experiments/ExperimentFeaturesIT.java
@@ -29,6 +29,11 @@
@Inject ExperimentFeatures experimentFeatures;
+ @Override
+ public boolean enableExperimentsRejectImplicitMergesOnMerge() {
+ return false;
+ }
+
@Test
public void emptyConfig_defaultFeatures_enabled() {
for (String defaultFeature : ExperimentFeaturesConstants.DEFAULT_ENABLED_FEATURES) {
diff --git a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.ts b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.ts
index e810637..516ee5f 100644
--- a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.ts
+++ b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.ts
@@ -194,15 +194,25 @@
// 4. Rewrite plain text ("text") to apply linking and other config-based
// rewrites. Text within code blocks is not passed here.
// 5. Open links in a new tab by rendering with target="_blank" attribute.
+ // 6. Relative links without "/" prefix are assumed to be absolute links.
function customRenderer(renderer: {[type: string]: Function}) {
- renderer['link'] = (href: string, title: string, text: string) =>
+ renderer['link'] = (href: string, title: string, text: string) => {
+ if (
+ !href.startsWith('https://') &&
+ !href.startsWith('mailto:') &&
+ !href.startsWith('http://') &&
+ !href.startsWith('/')
+ ) {
+ href = `https://${href}`;
+ }
/* HTML */
- `<a
+ return `<a
href="${href}"
${sameOrigin(href) ? '' : 'target="_blank" rel="noopener noreferrer"'}
${title ? `title="${title}"` : ''}
>${text}</a
>`;
+ };
renderer['image'] = (href: string, _title: string, text: string) =>
``;
renderer['codespan'] = (text: string) =>
diff --git a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts
index a287659..23f1594 100644
--- a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts
@@ -510,6 +510,9 @@
element.content = `[myLink1](https://www.google.com)
[myLink2](/destiny)
[myLink3](${origin}/destiny)
+ [myLink4](google.com)
+ [myLink5](http://google.com)
+ [myLink6](mailto:google@google.com)
`;
await element.updateComplete;
@@ -529,6 +532,27 @@
<a href="/destiny">myLink2</a>
<br />
<a href="${origin}/destiny">myLink3</a>
+ <br />
+ <a
+ href="https://google.com"
+ rel="noopener noreferrer"
+ target="_blank"
+ >myLink4</a
+ >
+ <br />
+ <a
+ href="http://google.com"
+ rel="noopener noreferrer"
+ target="_blank"
+ >myLink5</a
+ >
+ <br />
+ <a
+ href="mailto:google@google.com"
+ rel="noopener noreferrer"
+ target="_blank"
+ >myLink6</a
+ >
</p>
</div>
</marked-element>
