Merge "Adds diff traversal helper and make diff object a builder property"
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index abe0606..ff6e94a 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -2846,9 +2846,7 @@
Each element of the `reviewers` list is an instance of
link:#reviewer-input[ReviewerInput]. The corresponding result of
adding each reviewer will be returned in a list of
-link:#add-reviewer-result[AddReviewerResult]. If there are any
-errors returned for reviewers, the entire review request will
-be rejected.
+link:#add-reviewer-result[AddReviewerResult].
.Response
----
@@ -2886,6 +2884,27 @@
}
----
+If there are any errors returned for reviewers, the entire review request will
+be rejected with `400 Bad Request`.
+
+.Error Response
+----
+ HTTP/1.1 400 Bad Request
+ Content-Disposition: attachment
+ Content-Type: application/json; charset=UTF-8
+
+ )]}'
+ {
+ "reviewers": {
+ "MyProjectVerifiers": {
+ "input": "MyProjectVerifiers",
+ "error": "The group My Group has 15 members. Do you want to add them all as reviewers?",
+ "confirm": true
+ }
+ }
+ }
+----
+
[[rebase-revision]]
=== Rebase Revision
--
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index b32fccc..324aad2 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -42,6 +42,7 @@
import com.google.gerrit.extensions.api.projects.ProjectInput;
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.client.ListChangesOption;
+import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ActionInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.EditInfo;
@@ -415,15 +416,28 @@
protected Project.NameKey createProject(String nameSuffix,
Project.NameKey parent) throws RestApiException {
// Default for createEmptyCommit should match TestProjectConfig.
- return createProject(nameSuffix, parent, true);
+ return createProject(nameSuffix, parent, true, null);
}
protected Project.NameKey createProject(String nameSuffix,
- Project.NameKey parent, boolean createEmptyCommit)
+ Project.NameKey parent, boolean createEmptyCommit) throws RestApiException {
+ // Default for createEmptyCommit should match TestProjectConfig.
+ return createProject(nameSuffix, parent, createEmptyCommit, null);
+ }
+
+ protected Project.NameKey createProject(String nameSuffix,
+ Project.NameKey parent, SubmitType submitType) throws RestApiException {
+ // Default for createEmptyCommit should match TestProjectConfig.
+ return createProject(nameSuffix, parent, true, submitType);
+ }
+
+ protected Project.NameKey createProject(String nameSuffix,
+ Project.NameKey parent, boolean createEmptyCommit, SubmitType submitType)
throws RestApiException {
ProjectInput in = new ProjectInput();
in.name = name(nameSuffix);
in.parent = parent != null ? parent.get() : null;
+ in.submitType = submitType;
in.createEmptyCommit = createEmptyCommit;
return createProject(in);
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractSubmoduleSubscription.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractSubmoduleSubscription.java
index af13c5d..2ca4691 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractSubmoduleSubscription.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractSubmoduleSubscription.java
@@ -21,6 +21,7 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.SubscribeSection;
+import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.git.ProjectConfig;
@@ -40,14 +41,52 @@
import java.util.concurrent.atomic.AtomicInteger;
public abstract class AbstractSubmoduleSubscription extends AbstractDaemonTest {
+
+ protected SubmitType getSubmitType() {
+ return cfg.getEnum("project", null, "submitType", SubmitType.MERGE_IF_NECESSARY);
+ }
+
+ protected static Config submitByMergeAlways() {
+ Config cfg = new Config();
+ cfg.setBoolean("change", null, "submitWholeTopic", true);
+ cfg.setEnum("project", null, "submitType", SubmitType.MERGE_ALWAYS);
+ return cfg;
+ }
+
+ protected static Config submitByMergeIfNecessary() {
+ Config cfg = new Config();
+ cfg.setBoolean("change", null, "submitWholeTopic", true);
+ cfg.setEnum("project", null, "submitType", SubmitType.MERGE_IF_NECESSARY);
+ return cfg;
+ }
+
+ protected static Config submitByCherryPickConifg() {
+ Config cfg = new Config();
+ cfg.setBoolean("change", null, "submitWholeTopic", true);
+ cfg.setEnum("project", null, "submitType", SubmitType.CHERRY_PICK);
+ return cfg;
+ }
+
+ protected static Config submitByRebaseConifg() {
+ Config cfg = new Config();
+ cfg.setBoolean("change", null, "submitWholeTopic", true);
+ cfg.setEnum("project", null, "submitType", SubmitType.REBASE_IF_NECESSARY);
+ return cfg;
+ }
+
protected TestRepository<?> createProjectWithPush(String name,
- @Nullable Project.NameKey parent) throws Exception {
- Project.NameKey project = createProject(name, parent);
+ @Nullable Project.NameKey parent, SubmitType submitType) throws Exception {
+ Project.NameKey project = createProject(name, parent, submitType);
grant(Permission.PUSH, project, "refs/heads/*");
grant(Permission.SUBMIT, project, "refs/for/refs/heads/*");
return cloneProject(project);
}
+ protected TestRepository<?> createProjectWithPush(String name,
+ @Nullable Project.NameKey parent) throws Exception {
+ return createProjectWithPush(name, parent, getSubmitType());
+ }
+
protected TestRepository<?> createProjectWithPush(String name)
throws Exception {
return createProjectWithPush(name, null);
@@ -56,10 +95,11 @@
private static AtomicInteger contentCounter = new AtomicInteger(0);
protected ObjectId pushChangeTo(TestRepository<?> repo, String ref,
- String message, String topic) throws Exception {
+ String file, String content, String message, String topic)
+ throws Exception {
ObjectId ret = repo.branch("HEAD").commit().insertChangeId()
.message(message)
- .add("a.txt", "a contents: " + contentCounter.incrementAndGet())
+ .add(file, content)
.create();
String pushedRef = ref;
@@ -79,6 +119,12 @@
return ret;
}
+ protected ObjectId pushChangeTo(TestRepository<?> repo, String ref,
+ String message, String topic) throws Exception {
+ return pushChangeTo(repo, ref, "a.txt",
+ "a contents: " + contentCounter.incrementAndGet(), message, topic);
+ }
+
protected ObjectId pushChangeTo(TestRepository<?> repo, String branch)
throws Exception {
return pushChangeTo(repo, "refs/heads/" + branch, "some change", "");
@@ -135,16 +181,25 @@
protected void prepareSubmoduleConfigEntry(Config config,
String subscribeToRepo, String subscribeToBranch) {
+ // The submodule subscription module checks for gerrit.canonicalWebUrl to
+ // detect if it's configured for automatic updates. It doesn't matter if
+ // it serves from that URL.
+ prepareSubmoduleConfigEntry(config, subscribeToRepo, subscribeToRepo, subscribeToBranch);
+ }
+
+ protected void prepareSubmoduleConfigEntry(Config config,
+ String subscribeToRepo, String subscribeToRepoPath, String subscribeToBranch) {
subscribeToRepo = name(subscribeToRepo);
+ subscribeToRepoPath = name(subscribeToRepoPath);
// The submodule subscription module checks for gerrit.canonicalWebUrl to
// detect if it's configured for automatic updates. It doesn't matter if
// it serves from that URL.
String url = cfg.getString("gerrit", null, "canonicalWebUrl") + "/"
+ subscribeToRepo;
- config.setString("submodule", subscribeToRepo, "path", subscribeToRepo);
- config.setString("submodule", subscribeToRepo, "url", url);
+ config.setString("submodule", subscribeToRepoPath, "path", subscribeToRepoPath);
+ config.setString("submodule", subscribeToRepoPath, "url", url);
if (subscribeToBranch != null) {
- config.setString("submodule", subscribeToRepo, "branch", subscribeToBranch);
+ config.setString("submodule", subscribeToRepoPath, "branch", subscribeToBranch);
}
}
@@ -161,6 +216,27 @@
}
protected void expectToHaveSubmoduleState(TestRepository<?> repo,
+ String branch, String submodule, TestRepository<?> subRepo,
+ String subBranch) throws Exception {
+
+ submodule = name(submodule);
+ ObjectId commitId = repo.git().fetch().setRemote("origin").call()
+ .getAdvertisedRef("refs/heads/" + branch).getObjectId();
+
+ ObjectId subHead = subRepo.git().fetch().setRemote("origin").call()
+ .getAdvertisedRef("refs/heads/" + subBranch).getObjectId();
+
+ RevWalk rw = repo.getRevWalk();
+ RevCommit c = rw.parseCommit(commitId);
+ rw.parseBody(c.getTree());
+
+ RevTree tree = c.getTree();
+ RevObject actualId = repo.get(tree, submodule);
+
+ assertThat(actualId).isEqualTo(subHead);
+ }
+
+ protected void expectToHaveSubmoduleState(TestRepository<?> repo,
String branch, String submodule, ObjectId expectedId) throws Exception {
submodule = name(submodule);
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsIT.java
index 8156f67..8423bbf 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsIT.java
@@ -215,21 +215,18 @@
// The first update doesn't include the rev log
RevWalk rw = subRepo.getRevWalk();
- RevCommit subCommitMsg = rw.parseCommit(subHEAD);
expectToHaveCommitMessage(superRepo, "master",
"Update git submodules\n\n" +
- "Project: " + name("subscribed-to-project")
- + " master " + subHEAD.name() + "\n\n");
+ "* Update " + name("subscribed-to-project") + " from branch 'master'");
// The next commit should generate only its commit message,
// omitting previous commit logs
subHEAD = pushChangeTo(subRepo, "master");
- subCommitMsg = rw.parseCommit(subHEAD);
+ RevCommit subCommitMsg = rw.parseCommit(subHEAD);
expectToHaveCommitMessage(superRepo, "master",
"Update git submodules\n\n" +
- "Project: " + name("subscribed-to-project")
- + " master " + subHEAD.name() + "\n\n" +
- subCommitMsg.getFullMessage() + "\n\n");
+ "* Update " + name("subscribed-to-project") + " from branch 'master'"
+ + "\n - " + subCommitMsg.getFullMessage().replace("\n", "\n "));
}
@Test
@@ -302,7 +299,7 @@
}
@Test
- public void testCircularSubscriptionIsDetected() throws Exception {
+ public void testBranchCircularSubscription() throws Exception {
TestRepository<?> superRepo = createProjectWithPush("super-project");
TestRepository<?> subRepo = createProjectWithPush("subscribed-to-project");
allowSubmoduleSubscription("subscribed-to-project", "refs/heads/master",
@@ -325,6 +322,37 @@
"subscribed-to-project")).isFalse();
}
+ @Test
+ public void testProjectCircularSubscription() throws Exception {
+ TestRepository<?> superRepo = createProjectWithPush("super-project");
+ TestRepository<?> subRepo = createProjectWithPush("subscribed-to-project");
+
+ allowSubmoduleSubscription("subscribed-to-project", "refs/heads/master",
+ "super-project", "refs/heads/master");
+ allowSubmoduleSubscription("super-project", "refs/heads/dev",
+ "subscribed-to-project", "refs/heads/dev");
+
+ pushChangeTo(subRepo, "master");
+ pushChangeTo(superRepo, "master");
+ pushChangeTo(subRepo, "dev");
+ pushChangeTo(superRepo, "dev");
+
+ createSubmoduleSubscription(superRepo, "master",
+ "subscribed-to-project", "master");
+ createSubmoduleSubscription(subRepo, "dev", "super-project", "dev");
+
+ ObjectId subMasterHead = pushChangeTo(subRepo, "master");
+ ObjectId superDevHead = pushChangeTo(superRepo, "dev");
+
+ assertThat(hasSubmodule(superRepo, "master",
+ "subscribed-to-project")).isTrue();
+ assertThat(hasSubmodule(subRepo, "dev",
+ "super-project")).isTrue();
+ expectToHaveSubmoduleState(superRepo, "master",
+ "subscribed-to-project", subMasterHead);
+ expectToHaveSubmoduleState(subRepo, "dev",
+ "super-project", superDevHead);
+ }
@Test
public void testSubscriptionFailOnMissingACL() throws Exception {
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java
index 0a067e9..dfce62e 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmoduleSubscriptionsWholeTopicMergeIT.java
@@ -14,11 +14,13 @@
package com.google.gerrit.acceptance.git;
+import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.gerrit.acceptance.GitUtil.getChangeId;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.testutil.ConfigSuite;
import org.eclipse.jgit.junit.TestRepository;
@@ -33,8 +35,23 @@
extends AbstractSubmoduleSubscription {
@ConfigSuite.Default
- public static Config submitWholeTopicEnabled() {
- return submitWholeTopicEnabledConfig();
+ public static Config mergeIfNecessary() {
+ return submitByMergeIfNecessary();
+ }
+
+ @ConfigSuite.Config
+ public static Config mergeAlways() {
+ return submitByMergeAlways();
+ }
+
+ @ConfigSuite.Config
+ public static Config cherryPick() {
+ return submitByCherryPickConifg();
+ }
+
+ @ConfigSuite.Config
+ public static Config rebase() {
+ return submitByRebaseConifg();
}
@Test
@@ -192,9 +209,9 @@
gApi.changes().id(getChangeId(sub1, sub1Id).get()).current().submit();
- expectToHaveSubmoduleState(superRepo, "master", "sub1", sub1Id);
- expectToHaveSubmoduleState(superRepo, "master", "sub2", sub2Id);
- expectToHaveSubmoduleState(superRepo, "master", "sub3", sub3Id);
+ expectToHaveSubmoduleState(superRepo, "master", "sub1", sub1, "master");
+ expectToHaveSubmoduleState(superRepo, "master", "sub2", sub2, "master");
+ expectToHaveSubmoduleState(superRepo, "master", "sub3", sub3, "master");
superRepo.git().fetch().setRemote("origin").call()
.getAdvertisedRef("refs/heads/master").getObjectId();
@@ -204,4 +221,216 @@
.that(superRepo.getRepository().resolve("origin/master^"))
.isEqualTo(superPreviousId);
}
+
+ @Test
+ public void testDifferentPaths() throws Exception {
+ TestRepository<?> superRepo = createProjectWithPush("super-project");
+ TestRepository<?> sub = createProjectWithPush("sub");
+
+ allowSubmoduleSubscription("sub", "refs/heads/master",
+ "super-project", "refs/heads/master");
+
+ Config config = new Config();
+ prepareSubmoduleConfigEntry(config, "sub", "master");
+ prepareSubmoduleConfigEntry(config, "sub", "sub-copy", "master");
+ pushSubmoduleConfig(superRepo, "master", config);
+
+ ObjectId superPreviousId = pushChangeTo(superRepo, "master");
+
+ ObjectId subId = pushChangeTo(sub, "refs/for/master", "some message", "");
+
+ approve(getChangeId(sub, subId).get());
+
+ gApi.changes().id(getChangeId(sub, subId).get()).current().submit();
+
+ expectToHaveSubmoduleState(superRepo, "master", "sub", sub, "master");
+ expectToHaveSubmoduleState(superRepo, "master", "sub-copy", sub, "master");
+
+ superRepo.git().fetch().setRemote("origin").call()
+ .getAdvertisedRef("refs/heads/master").getObjectId();
+
+ assertWithMessage("submodule subscription update "
+ + "should have made one commit")
+ .that(superRepo.getRepository().resolve("origin/master^"))
+ .isEqualTo(superPreviousId);
+ }
+
+ @Test
+ public void testNonSubmoduleInSameTopic() throws Exception {
+ TestRepository<?> superRepo = createProjectWithPush("super-project");
+ TestRepository<?> sub = createProjectWithPush("sub");
+ TestRepository<?> standAlone = createProjectWithPush("standalone");
+
+ allowSubmoduleSubscription("sub", "refs/heads/master",
+ "super-project", "refs/heads/master");
+
+ createSubmoduleSubscription(superRepo, "master", "sub", "master");
+
+ ObjectId superPreviousId = pushChangeTo(superRepo, "master");
+
+ ObjectId subId =
+ pushChangeTo(sub, "refs/for/master", "some message", "same-topic");
+ ObjectId standAloneId =
+ pushChangeTo(standAlone, "refs/for/master", "some message",
+ "same-topic");
+
+ String subChangeId = getChangeId(sub, subId).get();
+ String standAloneChangeId = getChangeId(standAlone, standAloneId).get();
+ approve(subChangeId);
+ approve(standAloneChangeId);
+
+ gApi.changes().id(subChangeId).current().submit();
+
+ expectToHaveSubmoduleState(superRepo, "master", "sub", sub, "master");
+
+ ChangeStatus status = gApi.changes().id(standAloneChangeId).info().status;
+ assertThat(status).isEqualTo(ChangeStatus.MERGED);
+
+ superRepo.git().fetch().setRemote("origin").call()
+ .getAdvertisedRef("refs/heads/master").getObjectId();
+
+ assertWithMessage("submodule subscription update "
+ + "should have made one commit")
+ .that(superRepo.getRepository().resolve("origin/master^"))
+ .isEqualTo(superPreviousId);
+ }
+
+ @Test
+ public void testRecursiveSubmodules() throws Exception {
+ TestRepository<?> topRepo = createProjectWithPush("top-project");
+ TestRepository<?> midRepo = createProjectWithPush("mid-project");
+ TestRepository<?> bottomRepo = createProjectWithPush("bottom-project");
+
+ allowSubmoduleSubscription("mid-project", "refs/heads/master",
+ "top-project", "refs/heads/master");
+ allowSubmoduleSubscription("bottom-project", "refs/heads/master",
+ "mid-project", "refs/heads/master");
+
+ createSubmoduleSubscription(topRepo, "master", "mid-project", "master");
+ createSubmoduleSubscription(midRepo, "master", "bottom-project", "master");
+
+ ObjectId bottomHead =
+ pushChangeTo(bottomRepo, "refs/for/master", "some message", "same-topic");
+ ObjectId topHead =
+ pushChangeTo(topRepo, "refs/for/master", "some message", "same-topic");
+
+ String id1 = getChangeId(bottomRepo, bottomHead).get();
+ String id2 = getChangeId(topRepo, topHead).get();
+
+ gApi.changes().id(id1).current().review(ReviewInput.approve());
+ gApi.changes().id(id2).current().review(ReviewInput.approve());
+
+ gApi.changes().id(id1).current().submit();
+
+ expectToHaveSubmoduleState(midRepo, "master", "bottom-project", bottomRepo, "master");
+ expectToHaveSubmoduleState(topRepo, "master", "mid-project", midRepo, "master");
+ }
+
+ @Test
+ public void testTriangleSubmodules() throws Exception {
+ TestRepository<?> topRepo = createProjectWithPush("top-project");
+ TestRepository<?> midRepo = createProjectWithPush("mid-project");
+ TestRepository<?> bottomRepo = createProjectWithPush("bottom-project");
+
+ allowSubmoduleSubscription("mid-project", "refs/heads/master",
+ "top-project", "refs/heads/master");
+ allowSubmoduleSubscription("bottom-project", "refs/heads/master",
+ "mid-project", "refs/heads/master");
+ allowSubmoduleSubscription("bottom-project", "refs/heads/master",
+ "top-project", "refs/heads/master");
+
+ createSubmoduleSubscription(midRepo, "master", "bottom-project", "master");
+ Config config = new Config();
+ prepareSubmoduleConfigEntry(config, "bottom-project", "master");
+ prepareSubmoduleConfigEntry(config, "mid-project", "master");
+ pushSubmoduleConfig(topRepo, "master", config);
+
+ ObjectId bottomHead =
+ pushChangeTo(bottomRepo, "refs/for/master", "some message", "same-topic");
+ ObjectId topHead =
+ pushChangeTo(topRepo, "refs/for/master", "some message", "same-topic");
+
+ String id1 = getChangeId(bottomRepo, bottomHead).get();
+ String id2 = getChangeId(topRepo, topHead).get();
+
+ gApi.changes().id(id1).current().review(ReviewInput.approve());
+ gApi.changes().id(id2).current().review(ReviewInput.approve());
+
+ gApi.changes().id(id1).current().submit();
+
+ expectToHaveSubmoduleState(midRepo, "master", "bottom-project", bottomRepo, "master");
+ expectToHaveSubmoduleState(topRepo, "master", "mid-project", midRepo, "master");
+ expectToHaveSubmoduleState(topRepo, "master", "bottom-project", bottomRepo, "master");
+ }
+
+ @Test
+ public void testBranchCircularSubscription() throws Exception {
+ TestRepository<?> topRepo = createProjectWithPush("top-project");
+ TestRepository<?> midRepo = createProjectWithPush("mid-project");
+ TestRepository<?> bottomRepo = createProjectWithPush("bottom-project");
+
+ createSubmoduleSubscription(midRepo, "master", "bottom-project", "master");
+ createSubmoduleSubscription(topRepo, "master", "mid-project", "master");
+ createSubmoduleSubscription(bottomRepo, "master", "top-project", "master");
+
+ allowSubmoduleSubscription("bottom-project", "refs/heads/master",
+ "mid-project", "refs/heads/master");
+ allowSubmoduleSubscription("mid-project", "refs/heads/master",
+ "top-project", "refs/heads/master");
+ allowSubmoduleSubscription("top-project", "refs/heads/master",
+ "bottom-project", "refs/heads/master");
+
+ ObjectId bottomMasterHead =
+ pushChangeTo(bottomRepo, "refs/for/master", "some message", "");
+ String changeId = getChangeId(bottomRepo, bottomMasterHead).get();
+
+ approve(changeId);
+
+ exception.expectMessage("Branch level circular subscriptions detected");
+ exception.expectMessage("top-project,refs/heads/master");
+ exception.expectMessage("mid-project,refs/heads/master");
+ exception.expectMessage("bottom-project,refs/heads/master");
+ gApi.changes().id(changeId).current().submit();
+
+ assertThat(hasSubmodule(midRepo, "master", "bottom-project")).isFalse();
+ assertThat(hasSubmodule(topRepo, "master", "mid-project")).isFalse();
+ }
+
+ @Test
+ public void testProjectCircularSubscriptionWholeTopic() throws Exception {
+ TestRepository<?> superRepo = createProjectWithPush("super-project");
+ TestRepository<?> subRepo = createProjectWithPush("subscribed-to-project");
+
+ allowSubmoduleSubscription("subscribed-to-project", "refs/heads/master",
+ "super-project", "refs/heads/master");
+ allowSubmoduleSubscription("super-project", "refs/heads/dev",
+ "subscribed-to-project", "refs/heads/dev");
+
+ pushChangeTo(subRepo, "dev");
+ pushChangeTo(superRepo, "dev");
+
+ createSubmoduleSubscription(superRepo, "master",
+ "subscribed-to-project", "master");
+ createSubmoduleSubscription(subRepo, "dev", "super-project", "dev");
+
+ ObjectId subMasterHead =
+ pushChangeTo(subRepo, "refs/for/master", "b.txt", "content b",
+ "some message", "same-topic");
+ ObjectId superDevHead =
+ pushChangeTo(superRepo, "refs/for/dev",
+ "some message", "same-topic");
+
+ approve(getChangeId(subRepo, subMasterHead).get());
+ approve(getChangeId(superRepo, superDevHead).get());
+
+ exception.expectMessage("Project level circular subscriptions detected");
+ exception.expectMessage("subscribed-to-project");
+ exception.expectMessage("super-project");
+ gApi.changes().id(getChangeId(subRepo, subMasterHead).get()).current()
+ .submit();
+
+ assertThat(hasSubmodule(superRepo, "master", "subscribed-to-project"))
+ .isFalse();
+ assertThat(hasSubmodule(subRepo, "dev", "super-project")).isFalse();
+ }
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
index 49fb102..2b32c9f 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
@@ -17,6 +17,8 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.extensions.client.ReviewerState.CC;
import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
+import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
+import static javax.servlet.http.HttpServletResponse.SC_OK;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
@@ -272,7 +274,7 @@
// In legacy mode, change owner should be the only reviewer.
assertReviewers(c, REVIEWER, admin);
assertReviewers(c, CC, user, observer);
- }
+ }
// Verify emails were sent to added reviewers.
List<Message> messages = sender.getMessages();
@@ -329,7 +331,8 @@
.reviewer(user.email)
.reviewer(observer.email, CC, false)
.reviewer(largeGroup);
- ReviewResult result = review(r.getChangeId(), r.getCommit().name(), input);
+ ReviewResult result = review(
+ r.getChangeId(), r.getCommit().name(), input, SC_BAD_REQUEST);
assertThat(result.labels).isNull();
assertThat(result.reviewers).isNotNull();
assertThat(result.reviewers).hasSize(3);
@@ -353,7 +356,8 @@
.reviewer(user.email)
.reviewer(observer.email, CC, false)
.reviewer(mediumGroup);
- result = review(r.getChangeId(), r.getCommit().name(), input);
+ result = review(r.getChangeId(), r.getCommit().name(), input,
+ SC_BAD_REQUEST);
assertThat(result.labels).isNull();
assertThat(result.reviewers).isNotNull();
assertThat(result.reviewers).hasSize(3);
@@ -400,27 +404,46 @@
private AddReviewerResult addReviewer(String changeId, String reviewer)
throws Exception {
+ return addReviewer(changeId, reviewer, SC_OK);
+ }
+
+ private AddReviewerResult addReviewer(
+ String changeId, String reviewer, int expectedStatus) throws Exception {
AddReviewerInput in = new AddReviewerInput();
in.reviewer = reviewer;
- return addReviewer(changeId, in);
+ return addReviewer(changeId, in, expectedStatus);
}
private AddReviewerResult addReviewer(String changeId, AddReviewerInput in)
throws Exception {
+ return addReviewer(changeId, in, SC_OK);
+ }
+
+ private AddReviewerResult addReviewer(String changeId, AddReviewerInput in,
+ int expectedStatus) throws Exception {
RestResponse resp =
adminRestSession.post("/changes/" + changeId + "/reviewers", in);
- return readContentFromJson(resp, AddReviewerResult.class);
+ return readContentFromJson(
+ resp, expectedStatus, AddReviewerResult.class);
}
- private ReviewResult review(String changeId, String revisionId, ReviewInput in) throws Exception {
+ private ReviewResult review(
+ String changeId, String revisionId, ReviewInput in) throws Exception {
+ return review(changeId, revisionId, in, SC_OK);
+ }
+
+ private ReviewResult review(
+ String changeId, String revisionId, ReviewInput in, int expectedStatus)
+ throws Exception {
RestResponse resp = adminRestSession.post(
"/changes/" + changeId + "/revisions/" + revisionId + "/review", in);
- return readContentFromJson(resp, ReviewResult.class);
+ return readContentFromJson(resp, expectedStatus, ReviewResult.class);
}
- private static <T> T readContentFromJson(RestResponse r, Class<T> clazz)
+ private static <T> T readContentFromJson(
+ RestResponse r, int expectedStatus, Class<T> clazz)
throws Exception {
- r.assertOK();
+ r.assertStatus(expectedStatus);
JsonReader jsonReader = new JsonReader(r.getReader());
jsonReader.setLenient(true);
return newGson().fromJson(jsonReader, clazz);
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/Response.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/Response.java
index 803a94a..633efea 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/Response.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/Response.java
@@ -52,6 +52,11 @@
return new Redirect(location);
}
+ /** Arbitrary status code with wrapped result. */
+ public static <T> Response<T> withStatusCode(int statusCode, T value) {
+ return new Impl<>(statusCode, value);
+ }
+
@SuppressWarnings({"unchecked", "rawtypes"})
public static <T> T unwrap(T obj) {
while (obj instanceof Response) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountState.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountState.java
index 5942f40..05a7179 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountState.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountState.java
@@ -150,7 +150,7 @@
*/
public <T> void put(PropertyKey<T> key, @Nullable T value) {
Cache<PropertyKey<Object>, Object> p = properties(value != null);
- if (p != null || value != null) {
+ if (p != null) {
@SuppressWarnings("unchecked")
PropertyKey<Object> k = (PropertyKey<Object>) key;
if (value != null) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
index 4e8527a..aa35da8 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
@@ -20,6 +20,7 @@
import static com.google.gerrit.server.change.PutDraftComment.side;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static java.nio.charset.StandardCharsets.UTF_8;
+import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
import com.google.auto.value.AutoValue;
import com.google.common.base.MoreObjects;
@@ -48,6 +49,7 @@
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
+import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
@@ -145,12 +147,12 @@
}
@Override
- public ReviewResult apply(RevisionResource revision, ReviewInput input)
+ public Response<ReviewResult> apply(RevisionResource revision, ReviewInput input)
throws RestApiException, UpdateException, OrmException, IOException {
return apply(revision, input, TimeUtil.nowTs());
}
- public ReviewResult apply(RevisionResource revision, ReviewInput input,
+ public Response<ReviewResult> apply(RevisionResource revision, ReviewInput input,
Timestamp ts)
throws RestApiException, UpdateException, OrmException, IOException {
// Respect timestamp, but truncate at change created-on time.
@@ -197,7 +199,7 @@
ReviewResult output = new ReviewResult();
output.reviewers = reviewerJsonResults;
if (hasError || confirm) {
- return output;
+ return Response.withStatusCode(SC_BAD_REQUEST, output);
}
output.labels = input.labels;
@@ -218,7 +220,7 @@
}
}
- return output;
+ return Response.ok(output);
}
private RevisionResource onBehalfOf(RevisionResource rev, ReviewInput in)
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/GitModules.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/GitModules.java
index e0c72c6..a5cb18d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/GitModules.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/GitModules.java
@@ -64,12 +64,12 @@
this.submissionId = orm.getSubmissionId();
Project.NameKey project = branch.getParentKey();
logDebug("Loading .gitmodules of {} for project {}", branch, project);
+ OpenRepo or;
try {
- orm.openRepo(project, false);
+ or = orm.openRepo(project, false);
} catch (NoSuchProjectException e) {
throw new IOException(e);
}
- OpenRepo or = orm.getRepo(project);
ObjectId id = or.repo.resolve(branch.get());
if (id == null) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
index c69abf5..9a38e2d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
@@ -479,7 +479,7 @@
throw new IntegrationException("Error reading changes to submit", e);
}
Set<Project.NameKey> projects = br.keySet();
- Collection<Branch.NameKey> branches = cbb.keySet();
+ Set<Branch.NameKey> branches = cbb.keySet();
openRepos(projects);
for (Branch.NameKey branch : branches) {
@@ -488,28 +488,18 @@
}
// Done checks that don't involve running submit strategies.
commits.maybeFailVerbose();
-
- List<SubmitStrategy> strategies = new ArrayList<>(branches.size());
- for (Branch.NameKey branch : branches) {
- OpenRepo or = orm.getRepo(branch.getParentKey());
- OpenBranch ob = or.getBranch(branch);
- BranchBatch submitting = toSubmit.get(branch);
- checkNotNull(submitting.submitType(),
- "null submit type for %s; expected to previously fail fast",
- submitting);
- Set<CodeReviewCommit> commitsToSubmit = commits(submitting.changes());
- ob.mergeTip = new MergeTip(ob.oldTip, commitsToSubmit);
- SubmitStrategy strategy = createStrategy(or, ob.mergeTip, branch,
- submitting.submitType(), ob.oldTip);
- strategies.add(strategy);
- strategy.addOps(or.getUpdate(), commitsToSubmit);
- }
-
+ SubmoduleOp submoduleOp = subOpFactory.create(branches, orm);
try {
+ List<SubmitStrategy> strategies = getSubmitStrategies(toSubmit, submoduleOp);
+ Set<Project.NameKey> allProjects = submoduleOp.getProjectsInOrder();
+ // in case superproject subscription is disabled, allProjects would be null
+ if (allProjects == null) {
+ allProjects = projects;
+ }
BatchUpdate.execute(
- batchUpdates(projects),
+ orm.batchUpdates(allProjects),
new SubmitStrategyListener(submitInput, strategies, commits));
- } catch (UpdateException e) {
+ } catch (UpdateException | SubmoduleException e) {
// BatchUpdate may have inadvertently wrapped an IntegrationException
// thrown by some legacy SubmitStrategyOp code that intended the error
// message to be user-visible. Copy the message from the wrapped
@@ -521,20 +511,44 @@
if (e.getCause() instanceof IntegrationException) {
msg = e.getCause().getMessage();
} else {
- msg = "Error submitting change" + (cs.size() != 1 ? "s" : "");
+ msg = "Error submitting change" + (cs.size() != 1 ? "s" : "") + ": \n"
+ + e.getMessage();
}
throw new IntegrationException(msg, e);
}
-
- updateSuperProjects(br.values());
}
- private List<BatchUpdate> batchUpdates(Collection<Project.NameKey> projects) {
- List<BatchUpdate> updates = new ArrayList<>(projects.size());
- for (Project.NameKey project : projects) {
- updates.add(orm.getRepo(project).getUpdate());
+ private List<SubmitStrategy> getSubmitStrategies(
+ Map<Branch.NameKey, BranchBatch> toSubmit, SubmoduleOp submoduleOp)
+ throws IntegrationException {
+ List<SubmitStrategy> strategies = new ArrayList<>();
+ Set<Branch.NameKey> allBranches = submoduleOp.getBranchesInOrder();
+ // in case superproject subscription is disabled, allBranches would be null
+ if (allBranches == null) {
+ allBranches = toSubmit.keySet();
}
- return updates;
+
+ for (Branch.NameKey branch : allBranches) {
+ OpenRepo or = orm.getRepo(branch.getParentKey());
+ if (toSubmit.containsKey(branch)) {
+ BranchBatch submitting = toSubmit.get(branch);
+ OpenBranch ob = or.getBranch(branch);
+ checkNotNull(submitting.submitType(),
+ "null submit type for %s; expected to previously fail fast",
+ submitting);
+ Set<CodeReviewCommit> commitsToSubmit = commits(submitting.changes());
+ ob.mergeTip = new MergeTip(ob.oldTip, commitsToSubmit);
+ SubmitStrategy strategy = createStrategy(or, ob.mergeTip, branch,
+ submitting.submitType(), ob.oldTip, submoduleOp);
+ strategies.add(strategy);
+ strategy.addOps(or.getUpdate(), commitsToSubmit);
+ } else {
+ // no open change for this branch
+ // add submodule triggered op into BatchUpdate
+ submoduleOp.addOp(or.getUpdate(), branch);
+ }
+ }
+ return strategies;
}
private Set<CodeReviewCommit> commits(List<ChangeData> cds) {
@@ -551,10 +565,10 @@
private SubmitStrategy createStrategy(OpenRepo or,
MergeTip mergeTip, Branch.NameKey destBranch, SubmitType submitType,
- CodeReviewCommit branchTip) throws IntegrationException {
+ CodeReviewCommit branchTip, SubmoduleOp submoduleOp) throws IntegrationException {
return submitStrategyFactory.create(submitType, db, or.repo, or.rw, or.ins,
or.canMergeFlag, getAlreadyAccepted(or, branchTip), destBranch, caller,
- mergeTip, commits, submissionId, submitInput.notify);
+ mergeTip, commits, submissionId, submitInput.notify, submoduleOp);
}
private Set<RevCommit> getAlreadyAccepted(OpenRepo or,
@@ -729,18 +743,6 @@
}
}
- private void updateSuperProjects(Collection<Branch.NameKey> branches) {
- logDebug("Updating superprojects");
- SubmoduleOp subOp = subOpFactory.create(branches, orm);
- try {
- subOp.updateSuperProjects();
- logDebug("Updating superprojects done");
- } catch (SubmoduleException e) {
- logError("The gitlinks were not updated according to the "
- + "subscriptions", e);
- }
- }
-
private void openRepos(Collection<Project.NameKey> projects)
throws IntegrationException {
for (Project.NameKey project : projects) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOpRepoManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOpRepoManager.java
index 2bfe5b7..33e4c66 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOpRepoManager.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOpRepoManager.java
@@ -185,16 +185,17 @@
return or;
}
- public void openRepo(Project.NameKey project, boolean abortIfOpen)
+ public OpenRepo openRepo(Project.NameKey project, boolean abortIfOpen)
throws NoSuchProjectException, IOException {
if (abortIfOpen) {
checkState(!openRepos.containsKey(project),
"repo already opened: %s", project);
- } else {
- if (openRepos.containsKey(project)) {
- return;
- }
}
+
+ if (openRepos.containsKey(project)) {
+ return openRepos.get(project);
+ }
+
ProjectState projectState = projectCache.get(project);
if (projectState == null) {
throw new NoSuchProjectException(project);
@@ -203,6 +204,7 @@
OpenRepo or =
new OpenRepo(repoManager.openRepository(project), projectState);
openRepos.put(project, or);
+ return or;
} catch (RepositoryNotFoundException e) {
throw new NoSuchProjectException(project);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleException.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleException.java
index d7e8446..5a3b4ff 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleException.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleException.java
@@ -15,7 +15,7 @@
package com.google.gerrit.server.git;
/** Indicates the gitlink's update cannot be processed at this time. */
-class SubmoduleException extends Exception {
+public class SubmoduleException extends Exception {
private static final long serialVersionUID = 1L;
SubmoduleException(final String msg) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java
index 1a8c4a7..7f79a68 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.git;
import com.google.common.collect.HashMultimap;
+import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Multimap;
import com.google.common.collect.SetMultimap;
import com.google.gerrit.common.data.SubscribeSection;
@@ -27,7 +28,6 @@
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.BatchUpdate.Listener;
import com.google.gerrit.server.git.BatchUpdate.RepoContext;
-import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
import com.google.gerrit.server.git.MergeOpRepoManager.OpenRepo;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache;
@@ -45,10 +45,8 @@
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.FileMode;
import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
-import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.ReceiveCommand;
@@ -57,11 +55,15 @@
import org.slf4j.LoggerFactory;
import java.io.IOException;
+import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.Collections;
+import java.util.Deque;
import java.util.HashMap;
-import java.util.HashSet;
+import java.util.LinkedHashSet;
import java.util.Map;
+import java.util.Objects;
import java.util.Set;
public class SubmoduleOp {
@@ -78,15 +80,17 @@
@Override
public void updateRepo(RepoContext ctx) throws Exception {
- CodeReviewCommit c = composeGitlinksCommit(branch, null);
- ctx.addRefUpdate(new ReceiveCommand(c.getParent(0), c, branch.get()));
- addBranchTip(branch, c);
+ CodeReviewCommit c = composeGitlinksCommit(branch);
+ if (c != null) {
+ ctx.addRefUpdate(new ReceiveCommand(c.getParent(0), c, branch.get()));
+ addBranchTip(branch, c);
+ }
}
}
public interface Factory {
SubmoduleOp create(
- Collection<Branch.NameKey> updatedBranches, MergeOpRepoManager orm);
+ Set<Branch.NameKey> updatedBranches, MergeOpRepoManager orm);
}
private static final Logger log = LoggerFactory.getLogger(SubmoduleOp.class);
@@ -98,10 +102,11 @@
private final boolean verboseSuperProject;
private final boolean enableSuperProjectSubscriptions;
private final Multimap<Branch.NameKey, SubmoduleSubscription> targets;
- private final Collection<Branch.NameKey> updatedBranches;
+ private final Set<Branch.NameKey> updatedBranches;
private final MergeOpRepoManager orm;
private final Map<Branch.NameKey, CodeReviewCommit> branchTips;
-
+ private final Map<Branch.NameKey, GitModules> branchGitModules;
+ private final ImmutableSet<Branch.NameKey> sortedBranches;
@AssistedInject
public SubmoduleOp(
@@ -110,7 +115,7 @@
@GerritServerConfig Config cfg,
ProjectCache projectCache,
ProjectState.Factory projectStateFactory,
- @Assisted Collection<Branch.NameKey> updatedBranches,
+ @Assisted Set<Branch.NameKey> updatedBranches,
@Assisted MergeOpRepoManager orm) throws SubmoduleException {
this.gitmodulesFactory = gitmodulesFactory;
this.myIdent = myIdent;
@@ -124,51 +129,93 @@
this.updatedBranches = updatedBranches;
this.targets = HashMultimap.create();
this.branchTips = new HashMap<>();
- calculateSubscriptionMap();
+ this.branchGitModules = new HashMap<>();
+ this.sortedBranches = calculateSubscriptionMap();
}
- private void calculateSubscriptionMap() throws SubmoduleException {
+ private ImmutableSet<Branch.NameKey> calculateSubscriptionMap()
+ throws SubmoduleException {
if (!enableSuperProjectSubscriptions) {
logDebug("Updating superprojects disabled");
- return;
+ return null;
}
logDebug("Calculating superprojects - submodules map");
+ LinkedHashSet<Branch.NameKey> allVisited = new LinkedHashSet<>();
for (Branch.NameKey updatedBranch : updatedBranches) {
- logDebug("Now processing " + updatedBranch);
- Set<Branch.NameKey> checkedTargets = new HashSet<>();
- Set<Branch.NameKey> targetsToProcess = new HashSet<>();
- targetsToProcess.add(updatedBranch);
+ if (allVisited.contains(updatedBranch)) {
+ continue;
+ }
- while (!targetsToProcess.isEmpty()) {
- Set<Branch.NameKey> newTargets = new HashSet<>();
- for (Branch.NameKey b : targetsToProcess) {
- try {
- Collection<SubmoduleSubscription> subs =
- superProjectSubscriptionsForSubmoduleBranch(b);
- for (SubmoduleSubscription sub : subs) {
- Branch.NameKey dst = sub.getSuperProject();
- targets.put(dst, sub);
- newTargets.add(dst);
- }
- } catch (IOException e) {
- throw new SubmoduleException("Cannot find superprojects for " + b, e);
- }
- }
- logDebug("adding to done " + targetsToProcess);
- checkedTargets.addAll(targetsToProcess);
- logDebug("completely done with " + checkedTargets);
+ searchForSuperprojects(updatedBranch, new LinkedHashSet<Branch.NameKey>(),
+ allVisited);
+ }
- Set<Branch.NameKey> intersection = new HashSet<>(checkedTargets);
- intersection.retainAll(newTargets);
- if (!intersection.isEmpty()) {
- throw new SubmoduleException(
- "Possible circular subscription involving " + updatedBranch);
- }
+ // Since the searchForSuperprojects will add the superprojects before one
+ // submodule in sortedBranches, need reverse the order of it
+ reverse(allVisited);
+ return ImmutableSet.copyOf(allVisited);
+ }
- targetsToProcess = newTargets;
+ private void searchForSuperprojects(Branch.NameKey current,
+ LinkedHashSet<Branch.NameKey> currentVisited,
+ LinkedHashSet<Branch.NameKey> allVisited)
+ throws SubmoduleException {
+ logDebug("Now processing " + current);
+
+ if (currentVisited.contains(current)) {
+ throw new SubmoduleException(
+ "Branch level circular subscriptions detected: " +
+ printCircularPath(currentVisited, current));
+ }
+
+ if (allVisited.contains(current)) {
+ return;
+ }
+
+ currentVisited.add(current);
+ try {
+ Collection<SubmoduleSubscription> subscriptions =
+ superProjectSubscriptionsForSubmoduleBranch(current);
+ for (SubmoduleSubscription sub : subscriptions) {
+ Branch.NameKey superProject = sub.getSuperProject();
+ searchForSuperprojects(superProject, currentVisited, allVisited);
+ targets.put(superProject, sub);
+ }
+ } catch (IOException e) {
+ throw new SubmoduleException("Cannot find superprojects for " + current,
+ e);
+ }
+ currentVisited.remove(current);
+ allVisited.add(current);
+ }
+
+ private static <T> void reverse(LinkedHashSet<T> set) {
+ if (set == null) {
+ return;
+ }
+
+ Deque<T> q = new ArrayDeque<>(set);
+ set.clear();
+
+ while (!q.isEmpty()) {
+ set.add(q.removeLast());
+ }
+ }
+
+ private <T> String printCircularPath(LinkedHashSet<T> p, T target) {
+ StringBuilder sb = new StringBuilder();
+ sb.append(target);
+ ArrayList<T> reverseP = new ArrayList<>(p);
+ Collections.reverse(reverseP);
+ for (T t : reverseP) {
+ sb.append("->");
+ sb.append(t);
+ if (t.equals(target)) {
+ break;
}
}
+ return sb.toString();
}
private Collection<Branch.NameKey> getDestinationBranches(Branch.NameKey src,
@@ -180,14 +227,15 @@
if (r.matchSource(src.get())) {
if (r.getDestination() == null) {
// no need to care for wildcard, as we matched already
+ OpenRepo or;
try {
- orm.openRepo(s.getProject(), false);
+ or = orm.openRepo(s.getProject(), false);
} catch (NoSuchProjectException e) {
// A project listed a non existent project to be allowed
// to subscribe to it. Allow this for now.
continue;
}
- OpenRepo or = orm.getRepo(s.getProject());
+
for (Ref ref : or.repo.getRefDatabase().getRefs(
RefNames.REFS_HEADS).values()) {
ret.add(new Branch.NameKey(s.getProject(), ref.getName()));
@@ -222,8 +270,7 @@
for (Branch.NameKey targetBranch : branches) {
Project.NameKey targetProject = targetBranch.getParentKey();
try {
- orm.openRepo(targetProject, false);
- OpenRepo or = orm.getRepo(targetProject);
+ OpenRepo or = orm.openRepo(targetProject, false);
ObjectId id = or.repo.resolve(targetBranch.get());
if (id == null) {
logDebug("The branch " + targetBranch + " doesn't exist.");
@@ -233,14 +280,13 @@
logDebug("The project " + targetProject + " doesn't exist");
continue;
}
- GitModules m = gitmodulesFactory.create(targetBranch, orm);
- for (SubmoduleSubscription ss : m.subscribedTo(srcBranch)) {
- logDebug("Checking SubmoduleSubscription " + ss);
- if (projectCache.get(ss.getSubmodule().getParentKey()) != null) {
- logDebug("Adding SubmoduleSubscription " + ss);
- ret.add(ss);
- }
+
+ GitModules m = branchGitModules.get(targetBranch);
+ if (m == null) {
+ m = gitmodulesFactory.create(targetBranch, orm);
+ branchGitModules.put(targetBranch, m);
}
+ ret.addAll(m.subscribedTo(srcBranch));
}
}
logDebug("Calculated superprojects for " + srcBranch + " are " + ret);
@@ -248,20 +294,26 @@
}
public void updateSuperProjects() throws SubmoduleException {
+ ImmutableSet<Project.NameKey> projects = getProjectsInOrder();
+ if (projects == null) {
+ return;
+ }
+
SetMultimap<Project.NameKey, Branch.NameKey> dst = branchesByProject();
- Set<Project.NameKey> projects = dst.keySet();
+ LinkedHashSet<Project.NameKey> superProjects = new LinkedHashSet<>();
try {
for (Project.NameKey project : projects) {
- // get a new BatchUpdate for the project
- orm.openRepo(project, false);
- //TODO:czhen remove this when MergeOp combine this into BatchUpdate
- orm.getRepo(project).resetUpdate();
- for (Branch.NameKey branch : dst.get(project)) {
- SubmoduleOp.GitlinkOp op = new SubmoduleOp.GitlinkOp(branch);
- orm.getRepo(project).getUpdate().addRepoOnlyOp(op);
+ // only need superprojects
+ if (dst.containsKey(project)) {
+ superProjects.add(project);
+ // get a new BatchUpdate for the super project
+ OpenRepo or = orm.openRepo(project, false);
+ for (Branch.NameKey branch : dst.get(project)) {
+ addOp(or.getUpdate(), branch);
+ }
}
}
- BatchUpdate.execute(orm.batchUpdates(projects), Listener.NONE);
+ BatchUpdate.execute(orm.batchUpdates(superProjects), Listener.NONE);
} catch (RestApiException | UpdateException | IOException |
NoSuchProjectException e) {
throw new SubmoduleException("Cannot update gitlinks", e);
@@ -269,136 +321,178 @@
}
/**
- * Create a gitlink update commit on the tip of subscriber or modify the
- * baseCommit with gitlink update patch
+ * Create a separate gitlink commit
*/
- public CodeReviewCommit composeGitlinksCommit(
- final Branch.NameKey subscriber, RevCommit baseCommit)
+ public CodeReviewCommit composeGitlinksCommit(final Branch.NameKey subscriber)
throws IOException, SubmoduleException {
- PersonIdent author = null;
- StringBuilder msgbuf = new StringBuilder("Update git submodules\n\n");
- boolean sameAuthorForAll = true;
-
+ OpenRepo or;
try {
- orm.openRepo(subscriber.getParentKey(), false);
+ or = orm.openRepo(subscriber.getParentKey(), false);
} catch (NoSuchProjectException | IOException e) {
throw new SubmoduleException("Cannot access superproject", e);
}
- OpenRepo or = orm.getRepo(subscriber.getParentKey());
+ CodeReviewCommit currentCommit;
Ref r = or.repo.exactRef(subscriber.get());
if (r == null) {
throw new SubmoduleException(
"The branch was probably deleted from the subscriber repository");
}
+ currentCommit = or.rw.parseCommit(r.getObjectId());
- RevCommit currentCommit = (baseCommit != null) ? baseCommit :
- or.rw.parseCommit(or.repo.exactRef(subscriber.get()).getObjectId());
- or.rw.parseBody(currentCommit);
-
+ StringBuilder msgbuf = new StringBuilder("");
+ PersonIdent author = null;
DirCache dc = readTree(or.rw, currentCommit);
DirCacheEditor ed = dc.editor();
-
for (SubmoduleSubscription s : targets.get(subscriber)) {
- try {
- orm.openRepo(s.getSubmodule().getParentKey(), false);
- } catch (NoSuchProjectException | IOException e) {
- throw new SubmoduleException("Cannot access submodule", e);
- }
- OpenRepo subOr = orm.getRepo(s.getSubmodule().getParentKey());
- Repository subRepo = subOr.repo;
-
- Ref ref = subRepo.getRefDatabase().exactRef(s.getSubmodule().get());
- if (ref == null) {
- ed.add(new DeletePath(s.getPath()));
- continue;
- }
-
- ObjectId updateTo = ref.getObjectId();
- if (branchTips.containsKey(s.getSubmodule())) {
- updateTo = branchTips.get(s.getSubmodule());
- }
- RevWalk subOrRw = subOr.rw;
- final RevCommit newCommit = subOrRw.parseCommit(updateTo);
-
- subOrRw.parseBody(newCommit);
- if (author == null) {
- author = newCommit.getAuthorIdent();
- } else if (!author.equals(newCommit.getAuthorIdent())) {
- sameAuthorForAll = false;
- }
-
- DirCacheEntry dce = dc.getEntry(s.getPath());
- ObjectId oldId;
- if (dce != null) {
- if (!dce.getFileMode().equals(FileMode.GITLINK)) {
- String errMsg = "Requested to update gitlink " + s.getPath() + " in "
- + s.getSubmodule().getParentKey().get() + " but entry "
- + "doesn't have gitlink file mode.";
- throw new SubmoduleException(errMsg);
- }
- oldId = dce.getObjectId();
- } else {
- // This submodule did not exist before. We do not want to add
- // the full submodule history to the commit message, so omit it.
- oldId = updateTo;
- }
-
- ed.add(new PathEdit(s.getPath()) {
- @Override
- public void apply(DirCacheEntry ent) {
- ent.setFileMode(FileMode.GITLINK);
- ent.setObjectId(newCommit.getId());
- }
- });
- if (verboseSuperProject) {
- msgbuf.append("Project: " + s.getSubmodule().getParentKey().get());
- msgbuf.append(" " + s.getSubmodule().getShortName());
- msgbuf.append(" " + newCommit.getName());
- msgbuf.append("\n\n");
-
- try {
- subOrRw.resetRetain(subOr.canMergeFlag);
- subOrRw.markStart(newCommit);
- subOrRw.markUninteresting(subOrRw.parseCommit(oldId));
- for (RevCommit c : subOrRw) {
- subOrRw.parseBody(c);
- msgbuf.append(c.getFullMessage() + "\n\n");
- }
- } catch (IOException e) {
- throw new SubmoduleException("Could not perform a revwalk to "
- + "create superproject commit message", e);
+ RevCommit newCommit = updateSubmodule(dc, ed, msgbuf, s);
+ if (newCommit != null) {
+ if (author == null) {
+ author = newCommit.getAuthorIdent();
+ } else if (!author.equals(newCommit.getAuthorIdent())) {
+ author = myIdent;
}
}
}
ed.finish();
+ ObjectId newTreeId = dc.writeTree(or.ins);
-
- ObjectInserter oi = or.ins;
- CodeReviewRevWalk rw = or.rw;
- ObjectId tree = dc.writeTree(oi);
-
- if (!sameAuthorForAll || author == null) {
- author = myIdent;
+ // Gitlinks are already in the branch, return null
+ if (newTreeId.equals(currentCommit.getTree())) {
+ return null;
}
-
CommitBuilder commit = new CommitBuilder();
- commit.setTreeId(tree);
- if (baseCommit != null) {
- // modify the baseCommit
- commit.setParentIds(baseCommit.getParents());
- commit.setMessage(baseCommit.getFullMessage() + "\n\n" + msgbuf.toString());
- commit.setAuthor(baseCommit.getAuthorIdent());
- } else {
- // create a new commit
- commit.setParentId(currentCommit);
- commit.setMessage(msgbuf.toString());
- commit.setAuthor(author);
+ commit.setTreeId(newTreeId);
+ commit.setParentId(currentCommit);
+ StringBuilder commitMsg = new StringBuilder("Update git submodules\n\n");
+ if (verboseSuperProject) {
+ commitMsg.append(msgbuf);
}
+ commit.setMessage(commitMsg.toString());
+ commit.setAuthor(author);
commit.setCommitter(myIdent);
+ ObjectId id = or.ins.insert(commit);
+ return or.rw.parseCommit(id);
+ }
- ObjectId id = oi.insert(commit);
- return rw.parseCommit(id);
+ /**
+ * Amend an existing commit with gitlink updates
+ */
+ public CodeReviewCommit composeGitlinksCommit(
+ final Branch.NameKey subscriber, CodeReviewCommit currentCommit)
+ throws IOException, SubmoduleException {
+ OpenRepo or;
+ try {
+ or = orm.openRepo(subscriber.getParentKey(), false);
+ } catch (NoSuchProjectException | IOException e) {
+ throw new SubmoduleException("Cannot access superproject", e);
+ }
+
+ StringBuilder msgbuf = new StringBuilder("");
+ DirCache dc = readTree(or.rw, currentCommit);
+ DirCacheEditor ed = dc.editor();
+ for (SubmoduleSubscription s : targets.get(subscriber)) {
+ updateSubmodule(dc, ed, msgbuf, s);
+ }
+ ed.finish();
+ ObjectId newTreeId = dc.writeTree(or.ins);
+
+ // Gitlinks are already updated, just return the commit
+ if (newTreeId.equals(currentCommit.getTree())) {
+ return currentCommit;
+ }
+ or.rw.parseBody(currentCommit);
+ CommitBuilder commit = new CommitBuilder();
+ commit.setTreeId(newTreeId);
+ commit.setParentIds(currentCommit.getParents());
+ if (verboseSuperProject) {
+ commit.setMessage(
+ currentCommit.getFullMessage() + "\n\n*submodules:\n" + msgbuf.toString());
+ } else {
+ commit.setMessage(currentCommit.getFullMessage());
+ }
+ commit.setAuthor(currentCommit.getAuthorIdent());
+ commit.setCommitter(myIdent);
+ ObjectId id = or.ins.insert(commit);
+ return or.rw.parseCommit(id);
+ }
+
+ private RevCommit updateSubmodule(DirCache dc, DirCacheEditor ed,
+ StringBuilder msgbuf, final SubmoduleSubscription s)
+ throws SubmoduleException, IOException {
+ OpenRepo subOr;
+ try {
+ subOr = orm.openRepo(s.getSubmodule().getParentKey(), false);
+ } catch (NoSuchProjectException | IOException e) {
+ throw new SubmoduleException("Cannot access submodule", e);
+ }
+
+ DirCacheEntry dce = dc.getEntry(s.getPath());
+ RevCommit oldCommit = null;
+ if (dce != null) {
+ if (!dce.getFileMode().equals(FileMode.GITLINK)) {
+ String errMsg = "Requested to update gitlink " + s.getPath() + " in "
+ + s.getSubmodule().getParentKey().get() + " but entry "
+ + "doesn't have gitlink file mode.";
+ throw new SubmoduleException(errMsg);
+ }
+ oldCommit = subOr.rw.parseCommit(dce.getObjectId());
+ }
+
+ final RevCommit newCommit;
+ if (branchTips.containsKey(s.getSubmodule())) {
+ newCommit = branchTips.get(s.getSubmodule());
+ } else {
+ Ref ref = subOr.repo.getRefDatabase().exactRef(s.getSubmodule().get());
+ if (ref == null) {
+ ed.add(new DeletePath(s.getPath()));
+ return null;
+ }
+ newCommit = subOr.rw.parseCommit(ref.getObjectId());
+ }
+
+ if (Objects.equals(newCommit, oldCommit)) {
+ // gitlink have already been updated for this submodule
+ return null;
+ }
+ ed.add(new PathEdit(s.getPath()) {
+ @Override
+ public void apply(DirCacheEntry ent) {
+ ent.setFileMode(FileMode.GITLINK);
+ ent.setObjectId(newCommit.getId());
+ }
+ });
+
+ if (verboseSuperProject) {
+ createSubmoduleCommitMsg(msgbuf, s, subOr, newCommit, oldCommit);
+ }
+ subOr.rw.parseBody(newCommit);
+ return newCommit;
+ }
+
+ private void createSubmoduleCommitMsg(StringBuilder msgbuf,
+ SubmoduleSubscription s, OpenRepo subOr, RevCommit newCommit, RevCommit oldCommit)
+ throws SubmoduleException {
+ msgbuf.append("* Update " + s.getPath());
+ msgbuf.append(" from branch '" + s.getSubmodule().getShortName() + "'");
+
+ // newly created submodule gitlink, do not append whole history
+ if (oldCommit == null) {
+ return;
+ }
+
+ try {
+ subOr.rw.resetRetain(subOr.canMergeFlag);
+ subOr.rw.markStart(newCommit);
+ subOr.rw.markUninteresting(oldCommit);
+ for (RevCommit c : subOr.rw) {
+ subOr.rw.parseBody(c);
+ msgbuf.append("\n - " + c.getFullMessage().replace("\n", "\n "));
+ }
+ } catch (IOException e) {
+ throw new SubmoduleException("Could not perform a revwalk to "
+ + "create superproject commit message", e);
+ }
}
private static DirCache readTree(RevWalk rw, ObjectId base)
@@ -421,10 +515,46 @@
return ret;
}
+ public ImmutableSet<Project.NameKey> getProjectsInOrder()
+ throws SubmoduleException {
+ if (sortedBranches == null) {
+ return null;
+ }
+
+ LinkedHashSet<Project.NameKey> projects = new LinkedHashSet<>();
+ Project.NameKey prev = null;
+ for (Branch.NameKey branch : sortedBranches) {
+ Project.NameKey project = branch.getParentKey();
+ if (!project.equals(prev)) {
+ if (projects.contains(project)) {
+ throw new SubmoduleException(
+ "Project level circular subscriptions detected: " +
+ printCircularPath(projects, project));
+ }
+ projects.add(project);
+ }
+ prev = project;
+ }
+
+ return ImmutableSet.copyOf(projects);
+ }
+
+ public ImmutableSet<Branch.NameKey> getBranchesInOrder() {
+ return sortedBranches;
+ }
+
+ public boolean hasSubscription(Branch.NameKey branch) {
+ return targets.containsKey(branch);
+ }
+
public void addBranchTip(Branch.NameKey branch, CodeReviewCommit tip) {
branchTips.put(branch, tip);
}
+ public void addOp(BatchUpdate bu, Branch.NameKey branch) {
+ bu.addRepoOnlyOp(new GitlinkOp(branch));
+ }
+
private void logDebug(String msg, Object... args) {
if (log.isDebugEnabled()) {
log.debug("[" + orm.getSubmissionId() + "]" + msg, args);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
index 91d2cc7..4a5e94d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
@@ -74,11 +74,12 @@
}
@Override
- protected void updateRepoImpl(RepoContext ctx) {
+ protected void updateRepoImpl(RepoContext ctx) throws IntegrationException {
// The branch is unborn. Take fast-forward resolution to create the
// branch.
- args.mergeTip.moveTipTo(toMerge, toMerge);
- toMerge.setStatusCode(CommitMergeStatus.CLEAN_MERGE);
+ CodeReviewCommit newCommit = amendGitlink(toMerge);
+ args.mergeTip.moveTipTo(newCommit, toMerge);
+ newCommit.setStatusCode(CommitMergeStatus.CLEAN_MERGE);
}
}
@@ -105,7 +106,8 @@
}
@Override
- protected void updateRepoImpl(RepoContext ctx) throws IOException {
+ protected void updateRepoImpl(RepoContext ctx)
+ throws IntegrationException, IOException {
// If there is only one parent, a cherry-pick can be done by taking the
// delta relative to that one parent and redoing that on the current merge
// tip.
@@ -132,6 +134,7 @@
}
// Initial copy doesn't have new patch set ID since change hasn't been
// updated yet.
+ newCommit = amendGitlink(newCommit);
newCommit.copyFrom(toMerge);
newCommit.setPatchsetId(psId);
newCommit.setStatusCode(CommitMergeStatus.CLEAN_PICK);
@@ -189,12 +192,13 @@
// was configured.
MergeTip mergeTip = args.mergeTip;
if (args.rw.isMergedInto(mergeTip.getCurrentTip(), toMerge)) {
- mergeTip.moveTipTo(toMerge, toMerge);
+ mergeTip.moveTipTo(amendGitlink(toMerge), toMerge);
} else {
PersonIdent myIdent = new PersonIdent(args.serverIdent, ctx.getWhen());
CodeReviewCommit result = args.mergeUtil.mergeOneCommit(myIdent,
myIdent, args.repo, args.rw, args.inserter, args.destBranch,
mergeTip.getCurrentTip(), toMerge);
+ result = amendGitlink(result);
mergeTip.moveTipTo(result, toMerge);
}
args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag,
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/FastForwardOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/FastForwardOp.java
index cd99daf..0e69128 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/FastForwardOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/FastForwardOp.java
@@ -18,16 +18,13 @@
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.IntegrationException;
-import java.io.IOException;
-
class FastForwardOp extends SubmitStrategyOp {
FastForwardOp(SubmitStrategy.Arguments args, CodeReviewCommit toMerge) {
super(args, toMerge);
}
@Override
- public void updateRepoImpl(RepoContext ctx)
- throws IntegrationException, IOException {
- args.mergeTip.moveTipTo(toMerge, toMerge);
+ protected void updateRepoImpl(RepoContext ctx) throws IntegrationException {
+ args.mergeTip.moveTipTo(amendGitlink(toMerge), toMerge);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeOneOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeOneOp.java
index ab16647..b1590bf 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeOneOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeOneOp.java
@@ -44,6 +44,6 @@
args.mergeUtil.mergeOneCommit(caller, args.serverIdent,
ctx.getRepository(), args.rw, ctx.getInserter(), args.destBranch,
args.mergeTip.getCurrentTip(), toMerge);
- args.mergeTip.moveTipTo(merged, toMerge);
+ args.mergeTip.moveTipTo(amendGitlink(merged), toMerge);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
index af1e577..cca7452 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
@@ -71,11 +71,12 @@
}
@Override
- public void updateRepoImpl(RepoContext ctx) {
+ public void updateRepoImpl(RepoContext ctx) throws IntegrationException {
// The branch is unborn. Take fast-forward resolution to create the
// branch.
toMerge.setStatusCode(CommitMergeStatus.CLEAN_MERGE);
- args.mergeTip.moveTipTo(toMerge, toMerge);
+ CodeReviewCommit newCommit = amendGitlink(toMerge);
+ args.mergeTip.moveTipTo(newCommit, toMerge);
acceptMergeTip(args.mergeTip);
}
}
@@ -110,8 +111,7 @@
// BatchUpdate how to produce CodeReviewRevWalks.
if (args.mergeUtil.canFastForward(args.mergeSorter,
args.mergeTip.getCurrentTip(), args.rw, toMerge)) {
- toMerge.setStatusCode(CommitMergeStatus.CLEAN_MERGE);
- args.mergeTip.moveTipTo(toMerge, toMerge);
+ args.mergeTip.moveTipTo(amendGitlink(toMerge), toMerge);
acceptMergeTip(args.mergeTip);
return;
}
@@ -134,6 +134,7 @@
"Cannot rebase " + toMerge.name() + ": " + e.getMessage(), e);
}
newCommit = args.rw.parseCommit(rebaseOp.getRebasedCommit());
+ newCommit = amendGitlink(newCommit);
newCommit.copyFrom(toMerge);
newCommit.setStatusCode(CommitMergeStatus.CLEAN_REBASE);
newCommit.setPatchsetId(rebaseOp.getPatchSetId());
@@ -182,13 +183,13 @@
// configured.
MergeTip mergeTip = args.mergeTip;
if (args.rw.isMergedInto(mergeTip.getCurrentTip(), toMerge)) {
- mergeTip.moveTipTo(toMerge, toMerge);
+ mergeTip.moveTipTo(amendGitlink(toMerge), toMerge);
acceptMergeTip(mergeTip);
} else {
CodeReviewCommit newTip = args.mergeUtil.mergeOneCommit(
args.serverIdent, args.serverIdent, args.repo, args.rw,
args.inserter, args.destBranch, mergeTip.getCurrentTip(), toMerge);
- mergeTip.moveTipTo(newTip, toMerge);
+ mergeTip.moveTipTo(amendGitlink(newTip), toMerge);
}
args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag,
mergeTip.getCurrentTip(), args.alreadyAccepted);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java
index c0d9d2d..3761660 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java
@@ -41,6 +41,7 @@
import com.google.gerrit.server.git.MergeSorter;
import com.google.gerrit.server.git.MergeTip;
import com.google.gerrit.server.git.MergeUtil;
+import com.google.gerrit.server.git.SubmoduleOp;
import com.google.gerrit.server.git.TagCache;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.project.ChangeControl;
@@ -93,7 +94,8 @@
ReviewDb db,
Set<RevCommit> alreadyAccepted,
String submissionId,
- NotifyHandling notifyHandling);
+ NotifyHandling notifyHandling,
+ SubmoduleOp submoduleOp);
}
final AccountCache accountCache;
@@ -125,6 +127,7 @@
final String submissionId;
final SubmitType submitType;
final NotifyHandling notifyHandling;
+ final SubmoduleOp submoduleOp;
final ProjectState project;
final MergeSorter mergeSorter;
@@ -160,7 +163,8 @@
@Assisted Set<RevCommit> alreadyAccepted,
@Assisted String submissionId,
@Assisted SubmitType submitType,
- @Assisted NotifyHandling notifyHandling) {
+ @Assisted NotifyHandling notifyHandling,
+ @Assisted SubmoduleOp submoduleOp) {
this.accountCache = accountCache;
this.approvalsUtil = approvalsUtil;
this.batchUpdateFactory = batchUpdateFactory;
@@ -190,6 +194,7 @@
this.submissionId = submissionId;
this.submitType = submitType;
this.notifyHandling = notifyHandling;
+ this.submoduleOp = submoduleOp;
this.project = checkNotNull(projectCache.get(destBranch.getParentKey()),
"project not found: %s", destBranch.getParentKey());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java
index 725a1b4..ee49eae 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java
@@ -23,6 +23,7 @@
import com.google.gerrit.server.git.IntegrationException;
import com.google.gerrit.server.git.MergeOp.CommitStatus;
import com.google.gerrit.server.git.MergeTip;
+import com.google.gerrit.server.git.SubmoduleOp;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -52,11 +53,12 @@
Repository repo, CodeReviewRevWalk rw, ObjectInserter inserter,
RevFlag canMergeFlag, Set<RevCommit> alreadyAccepted,
Branch.NameKey destBranch, IdentifiedUser caller, MergeTip mergeTip,
- CommitStatus commits, String submissionId, NotifyHandling notifyHandling)
+ CommitStatus commits, String submissionId, NotifyHandling notifyHandling,
+ SubmoduleOp submoduleOp)
throws IntegrationException {
SubmitStrategy.Arguments args = argsFactory.create(submitType, destBranch,
commits, rw, caller, mergeTip, inserter, repo, canMergeFlag, db,
- alreadyAccepted, submissionId, notifyHandling);
+ alreadyAccepted, submissionId, notifyHandling, submoduleOp);
switch (submitType) {
case CHERRY_PICK:
return new CherryPick(args);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
index 788bcb3..df29372 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java
@@ -46,6 +46,7 @@
import com.google.gerrit.server.git.LabelNormalizer;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.git.SubmoduleException;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.project.ProjectState;
import com.google.gwtorm.server.OrmException;
@@ -136,6 +137,7 @@
tipAfter,
getDest().get());
ctx.addRefUpdate(command);
+ args.submoduleOp.addBranchTip(getDest(), tipAfter);
}
private void checkProjectConfig(RepoContext ctx, CodeReviewCommit commit)
@@ -556,6 +558,34 @@
protected void postUpdateImpl(Context ctx) throws Exception {
}
+ /**
+ * Amend the commit with gitlink update
+ * @param commit
+ */
+ protected CodeReviewCommit amendGitlink(CodeReviewCommit commit)
+ throws IntegrationException {
+ CodeReviewCommit newCommit = commit;
+ // Modify the commit with gitlink update
+ if (args.submoduleOp.hasSubscription(args.destBranch)) {
+ try {
+ newCommit =
+ args.submoduleOp.composeGitlinksCommit(args.destBranch, commit);
+ newCommit.copyFrom(commit);
+ if (commit.equals(toMerge)) {
+ newCommit.setPatchsetId(ChangeUtil.nextPatchSetId(
+ args.repo, toMerge.change().currentPatchSetId()));
+ args.commits.put(newCommit);
+ }
+ } catch (SubmoduleException | IOException e) {
+ throw new IntegrationException(
+ "cannot update gitlink for the commit at branch: "
+ + args.destBranch);
+ }
+ }
+
+ return newCommit;
+ }
+
protected final void logDebug(String msg, Object... args) {
if (log.isDebugEnabled()) {
log.debug("[" + this.args.submissionId + "]" + msg, args);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
index cdaf08c..627fe66 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -584,7 +584,8 @@
//
// Parse notes from the staged result so we can return something useful
// to the caller instead of throwing.
- log.debug("Rebuilding change {} failed", getChangeId());
+ log.debug("Rebuilding change {} failed: {}",
+ getChangeId(), e.getMessage());
args.metrics.autoRebuildFailureCount.increment(CHANGES);
rebuildResult = checkNotNull(r);
checkNotNull(r.newState());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java
index 8130913..30c6d10 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java
@@ -176,6 +176,16 @@
}
}
+ private static class ConflictingUpdateException extends OrmRuntimeException {
+ private static final long serialVersionUID = 1L;
+
+ ConflictingUpdateException(Change change, String expectedNoteDbState) {
+ super(String.format(
+ "Expected change %s to have noteDbState %s but was %s",
+ change.getId(), expectedNoteDbState, change.getNoteDbState()));
+ }
+ }
+
@Override
public Result rebuild(NoteDbUpdateManager manager,
ChangeBundle bundle) throws NoSuchChangeException, IOException,
@@ -217,8 +227,13 @@
db.changes().atomicUpdate(changeId, new AtomicUpdate<Change>() {
@Override
public Change update(Change change) {
- if (!Objects.equals(oldNoteDbState, change.getNoteDbState())) {
+ String currNoteDbState = change.getNoteDbState();
+ if (Objects.equals(currNoteDbState, newNoteDbState)) {
+ // Another thread completed the same rebuild we were about to.
throw new AbortUpdateException();
+ } else if (!Objects.equals(oldNoteDbState, currNoteDbState)) {
+ // Another thread updated the state to something else.
+ throw new ConflictingUpdateException(change, oldNoteDbState);
}
change.setNoteDbState(newNoteDbState);
return change;
@@ -233,7 +248,15 @@
throw new OrmException(NoteDbUpdateManager.CHANGES_READ_ONLY);
}
} catch (AbortUpdateException e) {
- // Drop this rebuild; another thread completed it.
+ // Drop this rebuild; another thread completed it. It's ok to not execute
+ // the update in this case, since the object referenced in the Result was
+ // flushed to the repo by whatever thread won the race.
+ } catch (ConflictingUpdateException e) {
+ // Rethrow as an OrmException so the caller knows to use staged results.
+ // Strictly speaking they are not completely up to date, but result we
+ // send to the caller is the same as if this rebuild had executed before
+ // the other thread.
+ throw new OrmException(e.getMessage());
}
return r;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
index 684e4f6..08195e4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
@@ -206,7 +206,8 @@
repo.scanForRepoChanges();
} catch (OrmException | IOException e) {
// See ChangeNotes#rebuildAndOpen.
- log.debug("Rebuilding change {} via drafts failed", getChangeId());
+ log.debug("Rebuilding change {} via drafts failed: {}",
+ getChangeId(), e.getMessage());
args.metrics.autoRebuildFailureCount.increment(CHANGES);
checkNotNull(r.staged());
return LoadHandle.create(
