Merge changes from topic "fix-reject-implicit-merge-on-submit"
* changes:
Reject implicit merges on submit (except for cherry pick submit type).
Fix tests to avoid implicit merges.
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/WORKSPACE b/WORKSPACE
index be957f2..22f128a 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -65,8 +65,8 @@
http_archive(
name = "build_bazel_rules_nodejs",
- sha256 = "94070eff79305be05b7699207fbac5d2608054dd53e6109f7d00d923919ff45a",
- urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/5.8.2/rules_nodejs-5.8.2.tar.gz"],
+ sha256 = "709cc0dcb51cf9028dd57c268066e5bc8f03a119ded410a13b5c3925d6e43c48",
+ urls = ["https://github.com/bazelbuild/rules_nodejs/releases/download/5.8.4/rules_nodejs-5.8.4.tar.gz"],
)
load("@build_bazel_rules_nodejs//:repositories.bzl", "build_bazel_rules_nodejs_dependencies")
@@ -142,7 +142,7 @@
load("@build_bazel_rules_nodejs//:index.bzl", "node_repositories", "yarn_install")
node_repositories(
- node_version = "17.9.1",
+ node_version = "18.17.0",
yarn_version = "1.22.19",
)
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index b2924ef..b69f110 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -1849,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/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/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/server/util/TaskListenerIT.java b/javatests/com/google/gerrit/acceptance/server/util/TaskListenerIT.java
index fdfef87..e62cb2b 100644
--- a/javatests/com/google/gerrit/acceptance/server/util/TaskListenerIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/util/TaskListenerIT.java
@@ -278,8 +278,8 @@
private void assertAwaitQueueSize(int size) throws InterruptedException {
long i = 0;
do {
- TimeUnit.NANOSECONDS.sleep(10);
- assertThat(i++).isLessThan(100);
+ TimeUnit.NANOSECONDS.sleep(100);
+ assertThat(i++).isLessThan(1000);
} while (size != workQueue.getTasks().size());
}
}
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>
