Merge "Update related changes on topic edit"
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index a274206..16e61f6 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -3130,6 +3130,28 @@
HTTP/1.1 204 No Content
----
+[[set-message]]
+=== Set Commit Message
+--
+'PUT /changes/link:#change-id[\{change-id\}]/message'
+--
+
+Creates a new patch set with a new commit message.
+
+The new commit message must be provided in the request body inside a
+link:#commit-message-input[CommitMessageInput] entity and contain the change ID footer if
+link:project-configuration.html#require-change-id[Require Change-Id] was specified.
+
+.Request
+----
+ PUT /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/message HTTP/1.0
+ Content-Type: application/json; charset=UTF-8
+
+ {
+ "message": "New Commit message \n\nChange-Id: I10394472cbd17dd12454f229e4f6de00b143a444\n"
+ }
+----
+
[[revision-endpoints]]
== Revision Endpoints
@@ -5921,6 +5943,25 @@
link:#web-link-info[WebLinkInfo] entities.
|===========================
+[[commit-message-input]]
+=== CommitMessageInput
+The `CommitMessageInput` entity contains information for changing
+the commit message of a change.
+
+[options="header",cols="1,^1,5"]
+|=============================
+|Field Name ||Description
+|`message` ||New commit message.
+|`notify` |optional|
+Notify handling that defines to whom email notifications should be sent
+after the commit message was updated. +
+Allowed values are `NONE`, `OWNER`, `OWNER_REVIEWERS` and `ALL`. +
+If not set, the default is `ALL`.
+|`notify_details`|optional|
+Additional information about whom to notify about the update as a map
+of recipient type to link:#notify-info[NotifyInfo] entity.
+|=============================
+
[[delete-comment-input]]
=== DeleteCommentInput
The `DeleteCommentInput` entity contains the option for deleting a comment.
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 cf09f0c..d2b1f32 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
@@ -237,7 +237,7 @@
protected TestRepository<InMemoryRepository> testRepo;
protected String resourcePrefix;
protected Description description;
- protected boolean useSsh;
+ protected boolean testRequiresSsh;
@Inject private ChangeIndexCollection changeIndexes;
@Inject private EventRecorder.Factory eventRecorderFactory;
@@ -259,7 +259,7 @@
@Before
public void assumeSshIfRequired() {
- if (useSsh) {
+ if (testRequiresSsh) {
// If the test uses ssh, we use assume() to make sure ssh is enabled on
// the test suite. JUnit will skip tests annotated with @UseSsh if we
// disable them using the command line flag.
@@ -341,8 +341,10 @@
db = reviewDbProvider.open();
- useSsh = SshMode.useSsh() && (classDesc.useSsh() || methodDesc.useSsh());
- if (useSsh && (adminSshSession == null || userSshSession == null)) {
+ testRequiresSsh = classDesc.useSshAnnotation() || methodDesc.useSshAnnotation();
+ if (testRequiresSsh
+ && SshMode.useSsh()
+ && (adminSshSession == null || userSshSession == null)) {
// Create Ssh sessions
initSsh(admin);
Context ctx = newRequestContext(user);
@@ -1293,4 +1295,13 @@
return revCommit;
}
}
+
+ protected RevCommit parseCurrentRevision(RevWalk rw, PushOneCommit.Result r) throws Exception {
+ return parseCurrentRevision(rw, r.getChangeId());
+ }
+
+ protected RevCommit parseCurrentRevision(RevWalk rw, String changeId) throws Exception {
+ return rw.parseCommit(
+ ObjectId.fromString(get(changeId, ListChangesOption.CURRENT_REVISION).currentRevision));
+ }
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index 48b66f6..1c5f178 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -211,28 +211,13 @@
@Test
public void create() throws Exception {
- TestAccount foo = accountCreator.create("foo");
- AccountInfo info = gApi.accounts().id(foo.id.get()).get();
- assertThat(info.username).isEqualTo("foo");
- if (useSsh) {
- accountIndexedCounter.assertReindexOf(
- foo, 3); // account creation + external ID creation + adding SSH keys
- } else {
- accountIndexedCounter.assertReindexOf(foo, 2); // account creation + external ID creation
- }
+ create(2); // account creation + external ID creation
+ }
- // check user branch
- try (Repository repo = repoManager.openRepository(allUsers);
- RevWalk rw = new RevWalk(repo)) {
- Ref ref = repo.exactRef(RefNames.refsUsers(foo.getId()));
- assertThat(ref).isNotNull();
- RevCommit c = rw.parseCommit(ref.getObjectId());
- long timestampDiffMs =
- Math.abs(
- c.getCommitTime() * 1000L
- - accountCache.get(foo.getId()).getAccount().getRegisteredOn().getTime());
- assertThat(timestampDiffMs).isAtMost(ChangeRebuilderImpl.MAX_WINDOW_MS);
- }
+ @Test
+ @UseSsh
+ public void createWithSshKeys() throws Exception {
+ create(3); // account creation + external ID creation + adding SSH keys
}
@Test
@@ -1086,6 +1071,26 @@
assertThat(checkInfo.checkAccountsResult.problems).containsExactlyElementsIn(expectedProblems);
}
+ public void create(int expectedAccountReindexCalls) throws Exception {
+ TestAccount foo = accountCreator.create("foo");
+ AccountInfo info = gApi.accounts().id(foo.id.get()).get();
+ assertThat(info.username).isEqualTo("foo");
+ accountIndexedCounter.assertReindexOf(foo, expectedAccountReindexCalls);
+
+ // check user branch
+ try (Repository repo = repoManager.openRepository(allUsers);
+ RevWalk rw = new RevWalk(repo)) {
+ Ref ref = repo.exactRef(RefNames.refsUsers(foo.getId()));
+ assertThat(ref).isNotNull();
+ RevCommit c = rw.parseCommit(ref.getObjectId());
+ long timestampDiffMs =
+ Math.abs(
+ c.getCommitTime() * 1000L
+ - accountCache.get(foo.getId()).getAccount().getRegisteredOn().getTime());
+ assertThat(timestampDiffMs).isAtMost(ChangeRebuilderImpl.MAX_WINDOW_MS);
+ }
+ }
+
private void assertSequenceNumbers(List<SshKeyInfo> sshKeys) {
int seq = 1;
for (SshKeyInfo key : sshKeys) {
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index e92d255..429ee0f 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -52,7 +52,9 @@
import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
import com.google.gerrit.extensions.api.changes.RevisionApi;
import com.google.gerrit.extensions.api.projects.BranchInput;
+import com.google.gerrit.extensions.api.projects.ConfigInput;
import com.google.gerrit.extensions.client.ChangeStatus;
+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.AccountInfo;
@@ -75,6 +77,7 @@
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.extensions.webui.PatchSetWebLink;
import com.google.gerrit.reviewdb.client.Account;
@@ -82,11 +85,13 @@
import com.google.gerrit.reviewdb.client.Branch.NameKey;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.change.GetRevisionActions;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import java.io.ByteArrayOutputStream;
+import java.io.IOException;
import java.sql.Timestamp;
import java.text.DateFormat;
import java.text.SimpleDateFormat;
@@ -99,6 +104,8 @@
import java.util.Locale;
import java.util.Map;
import java.util.Optional;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.RefUpdate;
@@ -1203,6 +1210,84 @@
.containsExactlyElementsIn(ImmutableSet.of(admin.getId(), user.getId()));
}
+ @Test
+ public void changeCommitMessage() throws Exception {
+ // Tests mutating the commit message as both the owner of the change and a regular user with
+ // addPatchSet permission. Asserts that both cases succeed.
+ PushOneCommit.Result r = createChange();
+ r.assertOkStatus();
+ assertThat(getCommitMessage(r.getChangeId()))
+ .isEqualTo("test commit\n\nChange-Id: " + r.getChangeId() + "\n");
+
+ for (TestAccount acc : ImmutableList.of(admin, user)) {
+ setApiUser(acc);
+ String newMessage =
+ "modified commit by " + acc.username + "\n\nChange-Id: " + r.getChangeId() + "\n";
+ gApi.changes().id(r.getChangeId()).setMessage(newMessage);
+ RevisionApi rApi = gApi.changes().id(r.getChangeId()).current();
+ assertThat(rApi.files().keySet()).containsExactly("/COMMIT_MSG", "a.txt");
+ assertThat(getCommitMessage(r.getChangeId())).isEqualTo(newMessage);
+ }
+ }
+
+ @Test
+ public void changeCommitMessageWithNoChangeIdSucceedsIfChangeIdNotRequired() throws Exception {
+ ConfigInput configInput = new ConfigInput();
+ configInput.requireChangeId = InheritableBoolean.FALSE;
+ gApi.projects().name(project.get()).config(configInput);
+
+ PushOneCommit.Result r = createChange();
+ r.assertOkStatus();
+ assertThat(getCommitMessage(r.getChangeId()))
+ .isEqualTo("test commit\n\nChange-Id: " + r.getChangeId() + "\n");
+
+ String newMessage = "modified commit\n";
+ gApi.changes().id(r.getChangeId()).setMessage(newMessage);
+ RevisionApi rApi = gApi.changes().id(r.getChangeId()).current();
+ assertThat(rApi.files().keySet()).containsExactly("/COMMIT_MSG", "a.txt");
+ assertThat(getCommitMessage(r.getChangeId())).isEqualTo(newMessage);
+ }
+
+ @Test
+ public void changeCommitMessageWithNoChangeIdFails() throws Exception {
+ PushOneCommit.Result r = createChange();
+ assertThat(getCommitMessage(r.getChangeId()))
+ .isEqualTo("test commit\n\nChange-Id: " + r.getChangeId() + "\n");
+ exception.expect(ResourceConflictException.class);
+ exception.expectMessage("missing Change-Id footer");
+ gApi.changes().id(r.getChangeId()).setMessage("modified commit\n");
+ }
+
+ @Test
+ public void changeCommitMessageWithWrongChangeIdFails() throws Exception {
+ PushOneCommit.Result otherChange = createChange();
+ PushOneCommit.Result r = createChange();
+ assertThat(getCommitMessage(r.getChangeId()))
+ .isEqualTo("test commit\n\nChange-Id: " + r.getChangeId() + "\n");
+ exception.expect(ResourceConflictException.class);
+ exception.expectMessage("wrong Change-Id footer");
+ gApi.changes()
+ .id(r.getChangeId())
+ .setMessage("modified commit\n\nChange-Id: " + otherChange.getChangeId() + "\n");
+ }
+
+ @Test
+ public void changeCommitMessageWithoutPermissionFails() throws Exception {
+ // Create new project with clean permissions
+ Project.NameKey p = createProject("addPatchSetEdit");
+ TestRepository<InMemoryRepository> userTestRepo = cloneProject(p, user);
+ // Block default permission
+ block(p, "refs/for/*", Permission.ADD_PATCH_SET, REGISTERED_USERS);
+ // Create change as user
+ PushOneCommit push = pushFactory.create(db, user.getIdent(), userTestRepo);
+ PushOneCommit.Result r = push.to("refs/for/master");
+ r.assertOkStatus();
+ // Try to change the commit message
+ exception.expect(AuthException.class);
+ exception.expectMessage("modifying commit message not permitted");
+ gApi.changes().id(r.getChangeId()).setMessage("foo");
+ }
+
private static void assertCherryPickResult(
ChangeInfo changeInfo, CherryPickInput input, String srcChangeId) throws Exception {
assertThat(changeInfo.changeId).isEqualTo(srcChangeId);
@@ -1279,6 +1364,10 @@
return li.all.stream().filter(a -> a._accountId == accountId).findFirst().get();
}
+ private String getCommitMessage(String changeId) throws RestApiException, IOException {
+ return gApi.changes().id(changeId).current().file("/COMMIT_MSG").content().asString();
+ }
+
private static Iterable<Account.Id> getReviewers(Collection<AccountInfo> r) {
return Iterables.transform(r, a -> new Account.Id(a._accountId));
}
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 6941765..d64d67f 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
@@ -16,20 +16,25 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
+import static com.google.common.truth.TruthJUnit.assume;
import static com.google.gerrit.acceptance.GitUtil.getChangeId;
+import com.google.common.collect.ImmutableList;
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.extensions.client.SubmitType;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.change.Submit.TestSubmitInput;
import com.google.gerrit.testutil.ConfigSuite;
+import java.util.ArrayDeque;
import java.util.Map;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.RefSpec;
import org.junit.Test;
@@ -740,4 +745,69 @@
expectToHaveSubmoduleState(repoA, "master", "project-b", repoB, "master");
expectToHaveSubmoduleState(repoA, "dev", "project-b", repoB, "dev");
}
+
+ @Test
+ public void retrySubmitAfterTornTopicOnLockFailure() throws Exception {
+ assume().that(notesMigration.fuseUpdates()).isTrue();
+
+ TestRepository<?> superRepo = createProjectWithPush("super-project");
+ TestRepository<?> sub1 = createProjectWithPush("sub1");
+ TestRepository<?> sub2 = createProjectWithPush("sub2");
+
+ allowMatchingSubmoduleSubscription(
+ "sub1", "refs/heads/master", "super-project", "refs/heads/master");
+ allowMatchingSubmoduleSubscription(
+ "sub2", "refs/heads/master", "super-project", "refs/heads/master");
+
+ Config config = new Config();
+ prepareSubmoduleConfigEntry(config, "sub1", "master");
+ prepareSubmoduleConfigEntry(config, "sub2", "master");
+ pushSubmoduleConfig(superRepo, "master", config);
+
+ ObjectId superPreviousId = pushChangeTo(superRepo, "master");
+
+ String topic = "same-topic";
+ ObjectId sub1Id = pushChangeTo(sub1, "refs/for/master", "some message", topic);
+ ObjectId sub2Id = pushChangeTo(sub2, "refs/for/master", "some message", topic);
+
+ String changeId1 = getChangeId(sub1, sub1Id).get();
+ String changeId2 = getChangeId(sub2, sub2Id).get();
+ approve(changeId1);
+ approve(changeId2);
+
+ TestSubmitInput input = new TestSubmitInput();
+ input.generateLockFailures =
+ new ArrayDeque<>(
+ ImmutableList.of(
+ false, // Change 1, attempt 1: success
+ true, // Change 2, attempt 1: lock failure
+ false, // Change 1, attempt 2: success
+ false, // Change 2, attempt 2: success
+ false)); // Leftover value to check total number of calls.
+ gApi.changes().id(changeId1).current().submit(input);
+
+ assertThat(info(changeId1).status).isEqualTo(ChangeStatus.MERGED);
+ assertThat(info(changeId2).status).isEqualTo(ChangeStatus.MERGED);
+
+ sub1.git().fetch().call();
+ RevWalk rw1 = sub1.getRevWalk();
+ RevCommit master1 = rw1.parseCommit(getRemoteHead(name("sub1"), "master"));
+ RevCommit change1Ps = parseCurrentRevision(rw1, changeId1);
+ assertThat(rw1.isMergedInto(change1Ps, master1)).isTrue();
+
+ sub2.git().fetch().call();
+ RevWalk rw2 = sub2.getRevWalk();
+ RevCommit master2 = rw2.parseCommit(getRemoteHead(name("sub2"), "master"));
+ RevCommit change2Ps = parseCurrentRevision(rw2, changeId2);
+ assertThat(rw2.isMergedInto(change2Ps, master2)).isTrue();
+
+ assertThat(input.generateLockFailures).containsExactly(false);
+
+ expectToHaveSubmoduleState(superRepo, "master", "sub1", sub1, "master");
+ expectToHaveSubmoduleState(superRepo, "master", "sub2", sub2, "master");
+
+ assertWithMessage("submodule subscription update should have made one commit")
+ .that(superRepo.getRepository().resolve("origin/master^"))
+ .isEqualTo(superPreviousId);
+ }
}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
index e4b5ff5..4334555 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
@@ -67,6 +67,7 @@
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.change.Submit;
+import com.google.gerrit.server.change.Submit.TestSubmitInput;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.git.validators.OnSubmitValidationListener;
import com.google.gerrit.server.notedb.ChangeNotes;
@@ -81,6 +82,7 @@
import com.google.inject.Inject;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
+import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
@@ -482,6 +484,43 @@
}
@Test
+ public void submitReusingOldTopic() throws Exception {
+ assume().that(isSubmitWholeTopicEnabled()).isTrue();
+
+ String topic = "test-topic";
+ PushOneCommit.Result change1 = createChange("Change 1", "a.txt", "content", topic);
+ PushOneCommit.Result change2 = createChange("Change 2", "a.txt", "content", topic);
+ String id1 = change1.getChangeId();
+ String id2 = change2.getChangeId();
+ approve(id1);
+ approve(id2);
+ assertSubmittedTogether(id1, ImmutableList.of(id1, id2));
+ assertSubmittedTogether(id2, ImmutableList.of(id1, id2));
+ submit(id2);
+
+ String expectedTopic = name(topic);
+ change1.assertChange(Change.Status.MERGED, expectedTopic, admin);
+ change2.assertChange(Change.Status.MERGED, expectedTopic, admin);
+ assertSubmittedTogether(id1, ImmutableList.of(id1, id2));
+ assertSubmittedTogether(id2, ImmutableList.of(id1, id2));
+
+ PushOneCommit.Result change3 = createChange("Change 3", "c.txt", "content", topic);
+ String id3 = change3.getChangeId();
+ approve(id3);
+ assertSubmittedTogether(id3, ImmutableList.of());
+ submit(id3);
+
+ change3.assertChange(Change.Status.MERGED, expectedTopic, admin);
+ assertSubmittedTogether(id3, ImmutableList.of());
+ }
+
+ private void assertSubmittedTogether(String changeId, Iterable<String> expected)
+ throws Exception {
+ assertThat(gApi.changes().id(changeId).submittedTogether().stream().map(i -> i.changeId))
+ .containsExactlyElementsIn(expected);
+ }
+
+ @Test
public void submitDraftChange() throws Exception {
PushOneCommit.Result draft = createDraftChange();
Change.Id num = draft.getChange().getId();
@@ -875,6 +914,81 @@
assertThat(rw.isMergedInto(fix, master)).isTrue();
}
+ @Test
+ public void retrySubmitSingleChangeOnLockFailure() throws Exception {
+ assume().that(notesMigration.fuseUpdates()).isTrue();
+
+ PushOneCommit.Result change = createChange();
+ String id = change.getChangeId();
+ approve(id);
+
+ TestSubmitInput input = new TestSubmitInput();
+ input.generateLockFailures =
+ new ArrayDeque<>(
+ ImmutableList.of(
+ true, // Attempt 1: lock failure
+ false, // Attempt 2: success
+ false)); // Leftover value to check total number of calls.
+ submit(id, input);
+ assertMerged(id);
+
+ testRepo.git().fetch().call();
+ RevWalk rw = testRepo.getRevWalk();
+ RevCommit master = rw.parseCommit(getRemoteHead(project, "master"));
+ RevCommit patchSet = parseCurrentRevision(rw, change);
+ assertThat(rw.isMergedInto(patchSet, master)).isTrue();
+
+ assertThat(input.generateLockFailures).containsExactly(false);
+ }
+
+ @Test
+ public void retrySubmitAfterTornTopicOnLockFailure() throws Exception {
+ assume().that(notesMigration.fuseUpdates()).isTrue();
+ assume().that(isSubmitWholeTopicEnabled()).isTrue();
+
+ String topic = "test-topic";
+
+ TestRepository<?> repoA = createProjectWithPush("project-a", null, getSubmitType());
+ TestRepository<?> repoB = createProjectWithPush("project-b", null, getSubmitType());
+
+ PushOneCommit.Result change1 =
+ createChange(repoA, "master", "Change 1", "a.txt", "content", topic);
+ PushOneCommit.Result change2 =
+ createChange(repoB, "master", "Change 2", "b.txt", "content", topic);
+
+ approve(change1.getChangeId());
+ approve(change2.getChangeId());
+
+ TestSubmitInput input = new TestSubmitInput();
+ input.generateLockFailures =
+ new ArrayDeque<>(
+ ImmutableList.of(
+ false, // Change 1, attempt 1: success
+ true, // Change 2, attempt 1: lock failure
+ false, // Change 1, attempt 2: success
+ false, // Change 2, attempt 2: success
+ false)); // Leftover value to check total number of calls.
+ submit(change2.getChangeId(), input);
+
+ String expectedTopic = name(topic);
+ change1.assertChange(Change.Status.MERGED, expectedTopic, admin);
+ change2.assertChange(Change.Status.MERGED, expectedTopic, admin);
+
+ repoA.git().fetch().call();
+ RevWalk rwA = repoA.getRevWalk();
+ RevCommit masterA = rwA.parseCommit(getRemoteHead(name("project-a"), "master"));
+ RevCommit change1Ps = parseCurrentRevision(rwA, change1);
+ assertThat(rwA.isMergedInto(change1Ps, masterA)).isTrue();
+
+ repoB.git().fetch().call();
+ RevWalk rwB = repoB.getRevWalk();
+ RevCommit masterB = rwB.parseCommit(getRemoteHead(name("project-b"), "master"));
+ RevCommit change2Ps = parseCurrentRevision(rwB, change2);
+ assertThat(rwB.isMergedInto(change2Ps, masterB)).isTrue();
+
+ assertThat(input.generateLockFailures).containsExactly(false);
+ }
+
private void setChangeStatusToNew(PushOneCommit.Result... changes) throws Exception {
for (PushOneCommit.Result change : changes) {
try (BatchUpdate bu =
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java
index 48499f8..b4d8557 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByMerge.java
@@ -138,10 +138,11 @@
testRepo.reset(initialHead);
PushOneCommit.Result change2 = createChange("Change 2", "b.txt", "other content");
Change.Id id2 = change2.getChange().getId();
- SubmitInput failAfterRefUpdates = new TestSubmitInput(new SubmitInput(), true);
+ TestSubmitInput failInput = new TestSubmitInput();
+ failInput.failAfterRefUpdates = true;
submit(
change2.getChangeId(),
- failAfterRefUpdates,
+ failInput,
ResourceConflictException.class,
"Failing after ref updates");
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java
index b94b062..5dfc76d 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmitByRebase.java
@@ -26,7 +26,6 @@
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.common.data.Permission;
-import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.client.SubmitType;
@@ -256,10 +255,11 @@
testRepo.reset(initialHead);
PushOneCommit.Result change2 = createChange("Change 2", "b.txt", "other content");
Change.Id id2 = change2.getChange().getId();
- SubmitInput failAfterRefUpdates = new TestSubmitInput(new SubmitInput(), true);
+ TestSubmitInput failInput = new TestSubmitInput();
+ failInput.failAfterRefUpdates = true;
submit(
change2.getChangeId(),
- failAfterRefUpdates,
+ failInput,
ResourceConflictException.class,
"Failing after ref updates");
RevCommit headAfterFailedSubmit = getRemoteHead();
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
index 5235b14..a385932 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByCherryPickIT.java
@@ -400,10 +400,11 @@
testRepo.reset(initialHead);
PushOneCommit.Result change2 = createChange("Change 2", "b.txt", "other content");
Change.Id id2 = change2.getChange().getId();
- SubmitInput failAfterRefUpdates = new TestSubmitInput(new SubmitInput(), true);
+ TestSubmitInput failInput = new TestSubmitInput();
+ failInput.failAfterRefUpdates = true;
submit(
change2.getChangeId(),
- failAfterRefUpdates,
+ failInput,
ResourceConflictException.class,
"Failing after ref updates");
RevCommit headAfterFailedSubmit = getRemoteHead();
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java
index a65ba82..d4397d64 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java
@@ -22,7 +22,6 @@
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.common.data.Permission;
-import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ActionInfo;
@@ -151,10 +150,11 @@
PushOneCommit.Result change = createChange("Change 1", "a.txt", "content");
Change.Id id = change.getChange().getId();
- SubmitInput failAfterRefUpdates = new TestSubmitInput(new SubmitInput(), true);
+ TestSubmitInput failInput = new TestSubmitInput();
+ failInput.failAfterRefUpdates = true;
submit(
change.getChangeId(),
- failAfterRefUpdates,
+ failInput,
ResourceConflictException.class,
"Failing after ref updates");
diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
index 3dabced..d52cd98 100644
--- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
+++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
@@ -190,6 +190,9 @@
*/
ChangeEditApi edit() throws RestApiException;
+ /** Create a new patch set with a new commit message. */
+ void setMessage(String message) throws RestApiException;
+
/** Set hashtags on a change */
void setHashtags(HashtagsInput input) throws RestApiException;
@@ -431,6 +434,11 @@
}
@Override
+ public void setMessage(String message) {
+ throw new NotImplementedException();
+ }
+
+ @Override
public EditInfo getEdit() {
throw new NotImplementedException();
}
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/HostPageServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/HostPageServlet.java
index 93673bc..51340ae 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/HostPageServlet.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/HostPageServlet.java
@@ -133,7 +133,7 @@
String src = "gerrit_ui/gerrit_ui.nocache.js";
try (InputStream in = servletContext.getResourceAsStream("/" + src)) {
if (in != null) {
- Hasher md = Hashing.md5().newHasher();
+ Hasher md = Hashing.murmur3_128().newHasher();
byte[] buf = new byte[1024];
int n;
while ((n = in.read(buf)) > 0) {
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/ResourceServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/ResourceServlet.java
index 914c830..150acc6 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/ResourceServlet.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/ResourceServlet.java
@@ -312,7 +312,7 @@
this.lastModified = checkNotNull(lastModified, "lastModified");
this.contentType = checkNotNull(contentType, "contentType");
this.raw = checkNotNull(raw, "raw");
- this.etag = Hashing.md5().hashBytes(raw).toString();
+ this.etag = Hashing.murmur3_128().hashBytes(raw).toString();
}
boolean isStale(Path p, ResourceServlet rs) throws IOException {
diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/LibraryDownloader.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/LibraryDownloader.java
index fb4e0eb..e0589ed 100644
--- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/LibraryDownloader.java
+++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/LibraryDownloader.java
@@ -280,7 +280,7 @@
System.err.flush();
return;
}
- Hasher h = Hashing.sha1().newHasher();
+ Hasher h = Hashing.murmur3_128().newHasher();
try (InputStream in = Files.newInputStream(dst);
OutputStream out = Funnels.asOutputStream(h)) {
ByteStreams.copy(in, out);
diff --git a/gerrit-plugin-api/BUILD b/gerrit-plugin-api/BUILD
index d17bda6..cc01802 100644
--- a/gerrit-plugin-api/BUILD
+++ b/gerrit-plugin-api/BUILD
@@ -43,6 +43,7 @@
"//lib:args4j",
"//lib:blame-cache",
"//lib:guava",
+ "//lib:guava-retrying",
"//lib:gson",
"//lib:gwtorm",
"//lib:icu4j",
diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/DisallowReadFromChangesReviewDbWrapper.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/DisallowReadFromChangesReviewDbWrapper.java
index 9791572..d4c6354 100644
--- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/DisallowReadFromChangesReviewDbWrapper.java
+++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/DisallowReadFromChangesReviewDbWrapper.java
@@ -14,7 +14,6 @@
package com.google.gerrit.reviewdb.server;
-import com.google.common.util.concurrent.CheckedFuture;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
@@ -82,8 +81,10 @@
throw new UnsupportedOperationException(MSG);
}
+ @SuppressWarnings("deprecation")
@Override
- public CheckedFuture<Change, OrmException> getAsync(Change.Id key) {
+ public com.google.common.util.concurrent.CheckedFuture<Change, OrmException> getAsync(
+ Change.Id key) {
throw new UnsupportedOperationException(MSG);
}
@@ -113,8 +114,10 @@
throw new UnsupportedOperationException(MSG);
}
+ @SuppressWarnings("deprecation")
@Override
- public CheckedFuture<PatchSetApproval, OrmException> getAsync(PatchSetApproval.Key key) {
+ public com.google.common.util.concurrent.CheckedFuture<PatchSetApproval, OrmException> getAsync(
+ PatchSetApproval.Key key) {
throw new UnsupportedOperationException(MSG);
}
@@ -149,8 +152,10 @@
throw new UnsupportedOperationException(MSG);
}
+ @SuppressWarnings("deprecation")
@Override
- public CheckedFuture<ChangeMessage, OrmException> getAsync(ChangeMessage.Key key) {
+ public com.google.common.util.concurrent.CheckedFuture<ChangeMessage, OrmException> getAsync(
+ ChangeMessage.Key key) {
throw new UnsupportedOperationException(MSG);
}
@@ -190,8 +195,10 @@
throw new UnsupportedOperationException(MSG);
}
+ @SuppressWarnings("deprecation")
@Override
- public CheckedFuture<PatchSet, OrmException> getAsync(PatchSet.Id key) {
+ public com.google.common.util.concurrent.CheckedFuture<PatchSet, OrmException> getAsync(
+ PatchSet.Id key) {
throw new UnsupportedOperationException(MSG);
}
@@ -221,8 +228,10 @@
throw new UnsupportedOperationException(MSG);
}
+ @SuppressWarnings("deprecation")
@Override
- public CheckedFuture<PatchLineComment, OrmException> getAsync(PatchLineComment.Key key) {
+ public com.google.common.util.concurrent.CheckedFuture<PatchLineComment, OrmException> getAsync(
+ PatchLineComment.Key key) {
throw new UnsupportedOperationException(MSG);
}
diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/ReviewDbWrapper.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/ReviewDbWrapper.java
index 3b22889..c8c4fb2 100644
--- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/ReviewDbWrapper.java
+++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/server/ReviewDbWrapper.java
@@ -16,7 +16,6 @@
import static com.google.common.base.Preconditions.checkNotNull;
-import com.google.common.util.concurrent.CheckedFuture;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
@@ -190,8 +189,10 @@
return delegate.toMap(c);
}
+ @SuppressWarnings("deprecation")
@Override
- public CheckedFuture<Change, OrmException> getAsync(Change.Id key) {
+ public com.google.common.util.concurrent.CheckedFuture<Change, OrmException> getAsync(
+ Change.Id key) {
return delegate.getAsync(key);
}
@@ -278,8 +279,10 @@
return delegate.toMap(c);
}
+ @SuppressWarnings("deprecation")
@Override
- public CheckedFuture<PatchSetApproval, OrmException> getAsync(PatchSetApproval.Key key) {
+ public com.google.common.util.concurrent.CheckedFuture<PatchSetApproval, OrmException> getAsync(
+ PatchSetApproval.Key key) {
return delegate.getAsync(key);
}
@@ -384,8 +387,10 @@
return delegate.toMap(c);
}
+ @SuppressWarnings("deprecation")
@Override
- public CheckedFuture<ChangeMessage, OrmException> getAsync(ChangeMessage.Key key) {
+ public com.google.common.util.concurrent.CheckedFuture<ChangeMessage, OrmException> getAsync(
+ ChangeMessage.Key key) {
return delegate.getAsync(key);
}
@@ -483,8 +488,10 @@
return delegate.toMap(c);
}
+ @SuppressWarnings("deprecation")
@Override
- public CheckedFuture<PatchSet, OrmException> getAsync(PatchSet.Id key) {
+ public com.google.common.util.concurrent.CheckedFuture<PatchSet, OrmException> getAsync(
+ PatchSet.Id key) {
return delegate.getAsync(key);
}
@@ -577,8 +584,10 @@
return delegate.toMap(c);
}
+ @SuppressWarnings("deprecation")
@Override
- public CheckedFuture<PatchLineComment, OrmException> getAsync(PatchLineComment.Key key) {
+ public com.google.common.util.concurrent.CheckedFuture<PatchLineComment, OrmException> getAsync(
+ PatchLineComment.Key key) {
return delegate.getAsync(key);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalId.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalId.java
index 4aabf59..9c456f5 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalId.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/externalids/ExternalId.java
@@ -96,12 +96,13 @@
* Returns the SHA1 of the external ID that is used as note ID in the refs/meta/external-ids
* notes branch.
*/
+ @SuppressWarnings("deprecation") // Use Hashing.sha1 for compatibility.
public ObjectId sha1() {
return ObjectId.fromRaw(Hashing.sha1().hashString(get(), UTF_8).asBytes());
}
/**
- * Exports this external ID key as string with the format "scheme:id", or "id" id scheme is
+ * Exports this external ID key as string with the format "scheme:id", or "id" if scheme is
* null.
*
* <p>This string representation is used as subsection name in the Git config file that stores
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
index f4ea3b0..4f4af54 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
@@ -71,6 +71,7 @@
import com.google.gerrit.server.change.PostReviewers;
import com.google.gerrit.server.change.PublishDraftPatchSet;
import com.google.gerrit.server.change.PutAssignee;
+import com.google.gerrit.server.change.PutMessage;
import com.google.gerrit.server.change.PutTopic;
import com.google.gerrit.server.change.Rebase;
import com.google.gerrit.server.change.Restore;
@@ -139,6 +140,7 @@
private final Unmute unmute;
private final SetWorkInProgress setWip;
private final SetReadyForReview setReady;
+ private final PutMessage putMessage;
@Inject
ChangeApiImpl(
@@ -182,6 +184,7 @@
Unmute unmute,
SetWorkInProgress setWip,
SetReadyForReview setReady,
+ PutMessage putMessage,
@Assisted ChangeResource change) {
this.changeApi = changeApi;
this.revert = revert;
@@ -223,6 +226,7 @@
this.unmute = unmute;
this.setWip = setWip;
this.setReady = setReady;
+ this.putMessage = putMessage;
this.change = change;
}
@@ -511,6 +515,17 @@
}
@Override
+ public void setMessage(String in) throws RestApiException {
+ try {
+ PutMessage.Input input = new PutMessage.Input();
+ input.message = in;
+ putMessage.apply(change, input);
+ } catch (Exception e) {
+ throw asRestApiException("Cannot edit commit message", e);
+ }
+ }
+
+ @Override
public ChangeInfo info() throws RestApiException {
return get(EnumSet.noneOf(ListChangesOption.class));
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeResource.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeResource.java
index 1bec3d1..f27c53b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeResource.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeResource.java
@@ -137,7 +137,7 @@
@Override
public String getETag() {
CurrentUser user = control.getUser();
- Hasher h = Hashing.md5().newHasher();
+ Hasher h = Hashing.murmur3_128().newHasher();
if (user.isIdentifiedUser()) {
h.putString(starredChangesUtil.getObjectId(user.getAccountId(), getId()).name(), UTF_8);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/FileContentUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/FileContentUtil.java
index 0c2ec68..181505d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/FileContentUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/FileContentUtil.java
@@ -273,7 +273,7 @@
// an attacker could upload a *.class file and have us send a ZIP
// that can be invoked through an applet tag in the victim's browser.
//
- Hasher h = Hashing.md5().newHasher();
+ Hasher h = Hashing.murmur3_128().newHasher();
byte[] buf = new byte[8];
NB.encodeInt64(buf, 0, TimeUtil.nowMs());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRevisionActions.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRevisionActions.java
index e476e73..8a6a1ab 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRevisionActions.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetRevisionActions.java
@@ -63,7 +63,7 @@
@Override
public String getETag(RevisionResource rsrc) {
- Hasher h = Hashing.md5().newHasher();
+ Hasher h = Hashing.murmur3_128().newHasher();
CurrentUser user = rsrc.getControl().getUser();
try {
rsrc.getChangeResource().prepareETag(h, user);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java
index db31c17..c66469c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java
@@ -94,6 +94,7 @@
put(CHANGE_KIND, "unmute").to(Unmute.class);
post(CHANGE_KIND, "wip").to(SetWorkInProgress.class);
post(CHANGE_KIND, "ready").to(SetReadyForReview.class);
+ put(CHANGE_KIND, "message").to(PutMessage.class);
post(CHANGE_KIND, "reviewers").to(PostReviewers.class);
get(CHANGE_KIND, "suggest_reviewers").to(SuggestChangeReviewers.class);
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 40c6e06..bebccc4 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
@@ -745,7 +745,7 @@
comment.key.patchSetId,
comment.lineNbr,
Side.fromShort(comment.side),
- Hashing.sha1().hashString(comment.message, UTF_8),
+ Hashing.murmur3_128().hashString(comment.message, UTF_8),
comment.range);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PreviewSubmit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PreviewSubmit.java
index ab3478b..53f127e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PreviewSubmit.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PreviewSubmit.java
@@ -36,8 +36,6 @@
import com.google.gerrit.server.git.MergeOpRepoManager.OpenRepo;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchProjectException;
-import com.google.gerrit.server.update.BatchUpdate;
-import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.UpdateException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -60,8 +58,7 @@
private static final int MAX_DEFAULT_BUNDLE_SIZE = 100 * 1024 * 1024;
private final Provider<ReviewDb> dbProvider;
- private final RetryHelper retryHelper;
- private final MergeOp.Factory mergeOpFactory;
+ private final Provider<MergeOp> mergeOpProvider;
private final AllowedFormats allowedFormats;
private int maxBundleSize;
private String format;
@@ -74,60 +71,53 @@
@Inject
PreviewSubmit(
Provider<ReviewDb> dbProvider,
- RetryHelper retryHelper,
- MergeOp.Factory mergeOpFactory,
+ Provider<MergeOp> mergeOpProvider,
AllowedFormats allowedFormats,
@GerritServerConfig Config cfg) {
this.dbProvider = dbProvider;
- this.retryHelper = retryHelper;
- this.mergeOpFactory = mergeOpFactory;
+ this.mergeOpProvider = mergeOpProvider;
this.allowedFormats = allowedFormats;
this.maxBundleSize = cfg.getInt("download", "maxBundleSize", MAX_DEFAULT_BUNDLE_SIZE);
}
@Override
- public BinaryResult apply(RevisionResource rsrc) throws UpdateException, RestApiException {
- // Shouldn't ever need to retry, since we are doing a dry-run BatchUpdate, but this is required
- // to get access to a BatchUpdate.Factory.
- return retryHelper.execute(
- (updateFactory) -> {
- if (Strings.isNullOrEmpty(format)) {
- throw new BadRequestException("format is not specified");
- }
- ArchiveFormat f = allowedFormats.extensions.get("." + format);
- if (f == null && format.equals("tgz")) {
- // Always allow tgz, even when the allowedFormats doesn't contain it.
- // Then we allow at least one format even if the list of allowed
- // formats is empty.
- f = ArchiveFormat.TGZ;
- }
- if (f == null) {
- throw new BadRequestException("unknown archive format");
- }
+ public BinaryResult apply(RevisionResource rsrc)
+ throws OrmException, RestApiException, UpdateException {
+ if (Strings.isNullOrEmpty(format)) {
+ throw new BadRequestException("format is not specified");
+ }
+ ArchiveFormat f = allowedFormats.extensions.get("." + format);
+ if (f == null && format.equals("tgz")) {
+ // Always allow tgz, even when the allowedFormats doesn't contain it.
+ // Then we allow at least one format even if the list of allowed
+ // formats is empty.
+ f = ArchiveFormat.TGZ;
+ }
+ if (f == null) {
+ throw new BadRequestException("unknown archive format");
+ }
- Change change = rsrc.getChange();
- if (!change.getStatus().isOpen()) {
- throw new PreconditionFailedException("change is " + ChangeUtil.status(change));
- }
- ChangeControl control = rsrc.getControl();
- if (!control.getUser().isIdentifiedUser()) {
- throw new MethodNotAllowedException("Anonymous users cannot submit");
- }
+ Change change = rsrc.getChange();
+ if (!change.getStatus().isOpen()) {
+ throw new PreconditionFailedException("change is " + ChangeUtil.status(change));
+ }
+ ChangeControl control = rsrc.getControl();
+ if (!control.getUser().isIdentifiedUser()) {
+ throw new MethodNotAllowedException("Anonymous users cannot submit");
+ }
- return getBundles(updateFactory, rsrc, f);
- });
+ return getBundles(rsrc, f);
}
- private BinaryResult getBundles(
- BatchUpdate.Factory updateFactory, RevisionResource rsrc, ArchiveFormat f)
- throws OrmException, RestApiException {
+ private BinaryResult getBundles(RevisionResource rsrc, ArchiveFormat f)
+ throws OrmException, RestApiException, UpdateException {
ReviewDb db = dbProvider.get();
ChangeControl control = rsrc.getControl();
IdentifiedUser caller = control.getUser().asIdentifiedUser();
Change change = rsrc.getChange();
@SuppressWarnings("resource") // Returned BinaryResult takes ownership and handles closing.
- MergeOp op = mergeOpFactory.create(updateFactory);
+ MergeOp op = mergeOpProvider.get();
try {
op.merge(db, change, caller, false, new SubmitInput(), true);
BinaryResult bin = new SubmitPreviewResult(op, f, maxBundleSize);
@@ -135,7 +125,7 @@
.setContentType(f.getMimeType())
.setAttachmentName("submit-preview-" + change.getChangeId() + "." + format);
return bin;
- } catch (OrmException | RestApiException | RuntimeException e) {
+ } catch (OrmException | RestApiException | UpdateException | RuntimeException e) {
op.close();
throw e;
}
@@ -162,8 +152,7 @@
BundleWriter bw = new BundleWriter(or.getCodeReviewRevWalk().getObjectReader());
bw.setObjectCountCallback(null);
bw.setPackConfig(new PackConfig(or.getRepo()));
- Collection<ReceiveCommand> refs =
- or.getUpdate(mergeOp.getBatchUpdateFactory()).getRefUpdates().values();
+ Collection<ReceiveCommand> refs = or.getUpdate().getRefUpdates().values();
for (ReceiveCommand r : refs) {
bw.include(r.getRefName(), r.getNewId());
ObjectId oldId = r.getOldId();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PutMessage.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutMessage.java
new file mode 100644
index 0000000..0935c71
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PutMessage.java
@@ -0,0 +1,217 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.change;
+
+import com.google.gerrit.common.FooterConstants;
+import com.google.gerrit.common.TimeUtil;
+import com.google.gerrit.extensions.api.changes.NotifyHandling;
+import com.google.gerrit.extensions.api.changes.NotifyInfo;
+import com.google.gerrit.extensions.api.changes.RecipientType;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.DefaultInput;
+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.reviewdb.client.PatchSet;
+import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.ChangeUtil;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.GerritPersonIdent;
+import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.edit.UnchangedCommitMessageException;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.permissions.ChangePermission;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.update.BatchUpdate;
+import com.google.gerrit.server.update.RetryHelper;
+import com.google.gerrit.server.update.RetryingRestModifyView;
+import com.google.gerrit.server.update.UpdateException;
+import com.google.gerrit.server.util.CommitMessageUtil;
+import com.google.gwtorm.server.OrmException;
+import java.io.IOException;
+import java.sql.Timestamp;
+import java.util.List;
+import java.util.Map;
+import java.util.TimeZone;
+import javax.inject.Inject;
+import javax.inject.Provider;
+import javax.inject.Singleton;
+import org.eclipse.jgit.lib.CommitBuilder;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
+
+@Singleton
+public class PutMessage
+ extends RetryingRestModifyView<ChangeResource, PutMessage.Input, Response<?>> {
+
+ public static class Input {
+ @DefaultInput public String message;
+
+ public NotifyHandling notify = NotifyHandling.ALL;
+
+ public Map<RecipientType, NotifyInfo> notifyDetails;
+ }
+
+ private final GitRepositoryManager repositoryManager;
+ private final Provider<CurrentUser> currentUserProvider;
+ private final Provider<ReviewDb> db;
+ private final TimeZone tz;
+ private final PatchSetInserter.Factory psInserterFactory;
+ private final PermissionBackend permissionBackend;
+ private final PatchSetUtil psUtil;
+ private final NotifyUtil notifyUtil;
+
+ @Inject
+ PutMessage(
+ RetryHelper retryHelper,
+ GitRepositoryManager repositoryManager,
+ Provider<CurrentUser> currentUserProvider,
+ Provider<ReviewDb> db,
+ PatchSetInserter.Factory psInserterFactory,
+ PermissionBackend permissionBackend,
+ @GerritPersonIdent PersonIdent gerritIdent,
+ PatchSetUtil psUtil,
+ NotifyUtil notifyUtil) {
+ super(retryHelper);
+ this.repositoryManager = repositoryManager;
+ this.currentUserProvider = currentUserProvider;
+ this.db = db;
+ this.psInserterFactory = psInserterFactory;
+ this.tz = gerritIdent.getTimeZone();
+ this.permissionBackend = permissionBackend;
+ this.psUtil = psUtil;
+ this.notifyUtil = notifyUtil;
+ }
+
+ @Override
+ protected Response<String> applyImpl(
+ BatchUpdate.Factory updateFactory, ChangeResource resource, Input input)
+ throws IOException, UnchangedCommitMessageException, RestApiException, UpdateException,
+ PermissionBackendException, OrmException {
+ PatchSet ps = psUtil.current(db.get(), resource.getNotes());
+ if (ps == null) {
+ throw new ResourceConflictException("current revision is missing");
+ } else if (!resource.getControl().isPatchVisible(ps, db.get())) {
+ throw new AuthException("current revision not accessible");
+ }
+
+ if (input == null) {
+ throw new BadRequestException("input cannot be null");
+ }
+ String sanitizedCommitMessage = CommitMessageUtil.checkAndSanitizeCommitMessage(input.message);
+
+ ensureCanEditCommitMessage(resource.getControl().getNotes());
+ ensureChangeIdIsCorrect(
+ resource.getControl().getProjectControl().getProjectState().isRequireChangeID(),
+ resource.getChange().getKey().get(),
+ sanitizedCommitMessage);
+
+ try (Repository repository = repositoryManager.openRepository(resource.getProject());
+ RevWalk revWalk = new RevWalk(repository);
+ ObjectInserter objectInserter = repository.newObjectInserter()) {
+ RevCommit patchSetCommit = revWalk.parseCommit(ObjectId.fromString(ps.getRevision().get()));
+
+ String currentCommitMessage = patchSetCommit.getFullMessage();
+ if (input.equals(currentCommitMessage)) {
+ throw new ResourceConflictException("new and existing commit message are the same");
+ }
+
+ Timestamp ts = TimeUtil.nowTs();
+ try (BatchUpdate bu =
+ updateFactory.create(
+ db.get(), resource.getChange().getProject(), currentUserProvider.get(), ts)) {
+ // Ensure that BatchUpdate will update the same repo
+ bu.setRepository(repository, new RevWalk(objectInserter.newReader()), objectInserter);
+
+ PatchSet.Id psId = ChangeUtil.nextPatchSetId(repository, ps.getId());
+ ObjectId newCommit =
+ createCommit(objectInserter, patchSetCommit, sanitizedCommitMessage, ts);
+ PatchSetInserter inserter =
+ psInserterFactory.create(resource.getControl(), psId, newCommit);
+ inserter.setMessage(
+ String.format("Patch Set %s: Commit message was updated.", psId.getId()));
+ inserter.setNotify(input.notify);
+ inserter.setAccountsToNotify(notifyUtil.resolveAccounts(input.notifyDetails));
+ bu.addOp(resource.getChange().getId(), inserter);
+ bu.execute();
+ }
+ }
+ return Response.ok("ok");
+ }
+
+ private ObjectId createCommit(
+ ObjectInserter objectInserter,
+ RevCommit basePatchSetCommit,
+ String commitMessage,
+ Timestamp timestamp)
+ throws IOException {
+ CommitBuilder builder = new CommitBuilder();
+ builder.setTreeId(basePatchSetCommit.getTree());
+ builder.setParentIds(basePatchSetCommit.getParents());
+ builder.setAuthor(basePatchSetCommit.getAuthorIdent());
+ builder.setCommitter(
+ currentUserProvider.get().asIdentifiedUser().newCommitterIdent(timestamp, tz));
+ builder.setMessage(commitMessage);
+ ObjectId newCommitId = objectInserter.insert(builder);
+ objectInserter.flush();
+ return newCommitId;
+ }
+
+ private void ensureCanEditCommitMessage(ChangeNotes changeNotes)
+ throws AuthException, PermissionBackendException {
+ if (!currentUserProvider.get().isIdentifiedUser()) {
+ throw new AuthException("Authentication required");
+ }
+ try {
+ permissionBackend
+ .user(currentUserProvider.get())
+ .database(db.get())
+ .change(changeNotes)
+ .check(ChangePermission.ADD_PATCH_SET);
+ } catch (AuthException denied) {
+ throw new AuthException("modifying commit message not permitted", denied);
+ }
+ }
+
+ private static void ensureChangeIdIsCorrect(
+ boolean requireChangeId, String currentChangeId, String newCommitMessage)
+ throws ResourceConflictException, BadRequestException {
+ RevCommit revCommit =
+ RevCommit.parse(
+ Constants.encode("tree " + ObjectId.zeroId().name() + "\n\n" + newCommitMessage));
+
+ // Check that the commit message without footers is not empty
+ CommitMessageUtil.checkAndSanitizeCommitMessage(revCommit.getShortMessage());
+
+ List<String> changeIdFooters = revCommit.getFooterLines(FooterConstants.CHANGE_ID);
+ if (requireChangeId && changeIdFooters.isEmpty()) {
+ throw new ResourceConflictException("missing Change-Id footer");
+ }
+ if (!changeIdFooters.isEmpty() && !changeIdFooters.get(0).equals(currentChangeId)) {
+ throw new ResourceConflictException("wrong Change-Id footer");
+ }
+ if (changeIdFooters.size() > 1) {
+ throw new ResourceConflictException("multiple Change-Id footers");
+ }
+ }
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
index a86767a..3fb8a79 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
@@ -29,6 +29,7 @@
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Branch;
@@ -45,7 +46,6 @@
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.ProjectUtil;
import com.google.gerrit.server.account.AccountsCollection;
-import com.google.gerrit.server.change.Submit.Output;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.ChangeSet;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -58,9 +58,7 @@
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
-import com.google.gerrit.server.update.BatchUpdate;
-import com.google.gerrit.server.update.RetryHelper;
-import com.google.gerrit.server.update.RetryingRestModifyView;
+import com.google.gerrit.server.update.UpdateException;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.OrmRuntimeException;
import com.google.inject.Inject;
@@ -73,6 +71,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
+import java.util.Queue;
import java.util.Set;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.Config;
@@ -84,8 +83,8 @@
import org.slf4j.LoggerFactory;
@Singleton
-public class Submit extends RetryingRestModifyView<RevisionResource, SubmitInput, Output>
- implements UiAction<RevisionResource> {
+public class Submit
+ implements RestModifyView<RevisionResource, SubmitInput>, UiAction<RevisionResource> {
private static final Logger log = LoggerFactory.getLogger(Submit.class);
private static final String DEFAULT_TOOLTIP = "Submit patch set ${patchSet} into ${branch}";
@@ -119,13 +118,14 @@
*/
@VisibleForTesting
public static class TestSubmitInput extends SubmitInput {
- public final boolean failAfterRefUpdates;
+ public boolean failAfterRefUpdates;
- public TestSubmitInput(SubmitInput base, boolean failAfterRefUpdates) {
- this.onBehalfOf = base.onBehalfOf;
- this.notify = base.notify;
- this.failAfterRefUpdates = failAfterRefUpdates;
- }
+ /**
+ * For each change being submitted, an element is removed from this queue and, if the value is
+ * true, a bogus ref update is added to the batch, in order to generate a lock failure during
+ * execution.
+ */
+ public Queue<Boolean> generateLockFailures;
}
private final Provider<ReviewDb> dbProvider;
@@ -134,7 +134,7 @@
private final ChangeData.Factory changeDataFactory;
private final ChangeMessagesUtil cmUtil;
private final ChangeNotes.Factory changeNotesFactory;
- private final MergeOp.Factory mergeOpFactory;
+ private final Provider<MergeOp> mergeOpProvider;
private final Provider<MergeSuperSet> mergeSuperSet;
private final AccountsCollection accounts;
private final String label;
@@ -152,24 +152,22 @@
Provider<ReviewDb> dbProvider,
GitRepositoryManager repoManager,
PermissionBackend permissionBackend,
- RetryHelper retryHelper,
ChangeData.Factory changeDataFactory,
ChangeMessagesUtil cmUtil,
ChangeNotes.Factory changeNotesFactory,
- MergeOp.Factory mergeOpFactory,
+ Provider<MergeOp> mergeOpProvider,
Provider<MergeSuperSet> mergeSuperSet,
AccountsCollection accounts,
@GerritServerConfig Config cfg,
Provider<InternalChangeQuery> queryProvider,
PatchSetUtil psUtil) {
- super(retryHelper);
this.dbProvider = dbProvider;
this.repoManager = repoManager;
this.permissionBackend = permissionBackend;
this.changeDataFactory = changeDataFactory;
this.cmUtil = cmUtil;
this.changeNotesFactory = changeNotesFactory;
- this.mergeOpFactory = mergeOpFactory;
+ this.mergeOpProvider = mergeOpProvider;
this.mergeSuperSet = mergeSuperSet;
this.accounts = accounts;
this.label =
@@ -202,10 +200,9 @@
}
@Override
- protected Output applyImpl(
- BatchUpdate.Factory updateFactory, RevisionResource rsrc, SubmitInput input)
+ public Output apply(RevisionResource rsrc, SubmitInput input)
throws RestApiException, RepositoryNotFoundException, IOException, OrmException,
- PermissionBackendException {
+ PermissionBackendException, UpdateException {
input.onBehalfOf = Strings.emptyToNull(input.onBehalfOf);
IdentifiedUser submitter;
if (input.onBehalfOf != null) {
@@ -215,15 +212,11 @@
submitter = rsrc.getUser().asIdentifiedUser();
}
- return new Output(mergeChange(updateFactory, rsrc, submitter, input));
+ return new Output(mergeChange(rsrc, submitter, input));
}
- public Change mergeChange(
- BatchUpdate.Factory updateFactory,
- RevisionResource rsrc,
- IdentifiedUser submitter,
- SubmitInput input)
- throws OrmException, RestApiException, IOException {
+ public Change mergeChange(RevisionResource rsrc, IdentifiedUser submitter, SubmitInput input)
+ throws OrmException, RestApiException, IOException, UpdateException {
Change change = rsrc.getChange();
if (!change.getStatus().isOpen()) {
throw new ResourceConflictException("change is " + ChangeUtil.status(change));
@@ -237,7 +230,7 @@
"revision %s is not current revision", rsrc.getPatchSet().getRevision().get()));
}
- try (MergeOp op = mergeOpFactory.create(updateFactory)) {
+ try (MergeOp op = mergeOpProvider.get()) {
ReviewDb db = dbProvider.get();
op.merge(db, change, submitter, true, input, false);
try {
@@ -291,7 +284,7 @@
if (c.change().isWorkInProgress()) {
return BLOCKED_WORK_IN_PROGRESS;
}
- MergeOp.checkSubmitRule(c);
+ MergeOp.checkSubmitRule(c, false);
}
Collection<ChangeData> unmergeable = unmergeableChanges(cs);
@@ -328,7 +321,7 @@
&& resource.isCurrent()
&& !resource.getPatchSet().isDraft()
&& resource.permissions().test(ChangePermission.SUBMIT);
- MergeOp.checkSubmitRule(cd);
+ MergeOp.checkSubmitRule(cd, false);
} catch (ResourceConflictException e) {
visible = false;
} catch (PermissionBackendException e) {
@@ -513,8 +506,7 @@
}
}
- public static class CurrentRevision
- extends RetryingRestModifyView<ChangeResource, SubmitInput, ChangeInfo> {
+ public static class CurrentRevision implements RestModifyView<ChangeResource, SubmitInput> {
private final Provider<ReviewDb> dbProvider;
private final Submit submit;
private final ChangeJson.Factory json;
@@ -523,11 +515,9 @@
@Inject
CurrentRevision(
Provider<ReviewDb> dbProvider,
- RetryHelper retryHelper,
Submit submit,
ChangeJson.Factory json,
PatchSetUtil psUtil) {
- super(retryHelper);
this.dbProvider = dbProvider;
this.submit = submit;
this.json = json;
@@ -535,10 +525,9 @@
}
@Override
- protected ChangeInfo applyImpl(
- BatchUpdate.Factory updateFactory, ChangeResource rsrc, SubmitInput input)
+ public ChangeInfo apply(ChangeResource rsrc, SubmitInput input)
throws RestApiException, RepositoryNotFoundException, IOException, OrmException,
- PermissionBackendException {
+ PermissionBackendException, UpdateException {
PatchSet ps = psUtil.current(dbProvider.get(), rsrc.getNotes());
if (ps == null) {
throw new ResourceConflictException("current revision is missing");
@@ -546,7 +535,7 @@
throw new AuthException("current revision not accessible");
}
- Output out = submit.applyImpl(updateFactory, new RevisionResource(rsrc, ps), input);
+ Output out = submit.apply(new RevisionResource(rsrc, ps), input);
return json.noOptions().format(out.change);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 366cbd1..f827bd5 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -115,7 +115,6 @@
import com.google.gerrit.server.git.EmailMerge;
import com.google.gerrit.server.git.GitModule;
import com.google.gerrit.server.git.GitModules;
-import com.google.gerrit.server.git.MergeOp;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.git.MergedByPushOp;
import com.google.gerrit.server.git.NotesBranchUtil;
@@ -401,7 +400,6 @@
factory(MergedByPushOp.Factory.class);
factory(GitModules.Factory.class);
factory(VersionedAuthorizedKeys.Factory.class);
- factory(MergeOp.Factory.class);
bind(AccountManager.class);
factory(ChangeUserName.Factory.class);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java
index 8d3cbae..79dc9c2 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditModifier.java
@@ -14,12 +14,10 @@
package com.google.gerrit.server.edit;
-import static com.google.common.base.Preconditions.checkState;
-
-import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.MergeConflictException;
import com.google.gerrit.extensions.restapi.RawInput;
import com.google.gerrit.reviewdb.client.Change;
@@ -42,6 +40,7 @@
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.InvalidChangeOperationException;
+import com.google.gerrit.server.util.CommitMessageUtil;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -205,13 +204,14 @@
* @throws AuthException if the user isn't authenticated or not allowed to use change edits
* @throws UnchangedCommitMessageException if the commit message is the same as before
* @throws PermissionBackendException
+ * @throws BadRequestException if the commit message is malformed
*/
public void modifyMessage(
Repository repository, ChangeControl changeControl, String newCommitMessage)
throws AuthException, IOException, UnchangedCommitMessageException, OrmException,
- PermissionBackendException {
+ PermissionBackendException, BadRequestException {
assertCanEdit(changeControl);
- newCommitMessage = getWellFormedCommitMessage(newCommitMessage);
+ newCommitMessage = CommitMessageUtil.checkAndSanitizeCommitMessage(newCommitMessage);
Optional<ChangeEdit> optionalChangeEdit = lookupChangeEdit(changeControl);
PatchSet basePatchSet = getBasePatchSet(optionalChangeEdit, changeControl);
@@ -435,13 +435,6 @@
}
}
- private static String getWellFormedCommitMessage(String commitMessage) {
- String wellFormedMessage = Strings.nullToEmpty(commitMessage).trim();
- checkState(!wellFormedMessage.isEmpty(), "Commit message cannot be null or empty");
- wellFormedMessage = wellFormedMessage + "\n";
- return wellFormedMessage;
- }
-
private Optional<ChangeEdit> lookupChangeEdit(ChangeControl changeControl)
throws AuthException, IOException {
return changeEditUtil.byChange(changeControl);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/BanCommit.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/BanCommit.java
index 29570a7..322d158 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/BanCommit.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/BanCommit.java
@@ -30,7 +30,6 @@
import java.util.Date;
import java.util.List;
import java.util.TimeZone;
-import org.eclipse.jgit.api.errors.ConcurrentRefUpdateException;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.Constants;
@@ -87,7 +86,7 @@
public BanCommitResult ban(
ProjectControl projectControl, List<ObjectId> commitsToBan, String reason)
- throws PermissionDeniedException, IOException, ConcurrentRefUpdateException {
+ throws PermissionDeniedException, LockFailureException, IOException {
if (!projectControl.isOwner()) {
throw new PermissionDeniedException("Not project owner: not permitted to ban commits");
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java
index 03d44ca..d4fa9ad 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ChangeSet.java
@@ -22,6 +22,7 @@
import com.google.common.collect.MultimapBuilder;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import java.util.Collection;
@@ -91,6 +92,14 @@
return changeData.values();
}
+ public ImmutableSet<Project.NameKey> projects() throws OrmException {
+ ImmutableSet.Builder<Project.NameKey> ret = ImmutableSet.builder();
+ for (ChangeData cd : changeData.values()) {
+ ret.add(cd.project());
+ }
+ return ret.build();
+ }
+
public ImmutableSet<Change.Id> nonVisibleIds() {
return nonVisibleChanges.keySet();
}
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 28cdebf..ce6b5c6 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
@@ -20,6 +20,8 @@
import static java.util.Comparator.comparing;
import static java.util.stream.Collectors.toSet;
+import com.github.rholder.retry.Attempt;
+import com.github.rholder.retry.RetryListener;
import com.google.auto.value.AutoValue;
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableListMultimap;
@@ -69,12 +71,13 @@
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
+import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.util.RequestId;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
+import com.google.inject.Provider;
import com.google.inject.Singleton;
-import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
import java.sql.Timestamp;
import java.util.ArrayList;
@@ -108,18 +111,17 @@
private static final Logger log = LoggerFactory.getLogger(MergeOp.class);
private static final SubmitRuleOptions SUBMIT_RULE_OPTIONS = SubmitRuleOptions.defaults().build();
-
- public interface Factory {
- MergeOp create(BatchUpdate.Factory batchUpdateFactory);
- }
+ private static final SubmitRuleOptions SUBMIT_RULE_OPTIONS_ALLOW_CLOSED =
+ SUBMIT_RULE_OPTIONS.toBuilder().allowClosed(true).build();
public static class CommitStatus {
private final ImmutableMap<Change.Id, ChangeData> changes;
private final ImmutableSetMultimap<Branch.NameKey, Change.Id> byBranch;
private final Map<Change.Id, CodeReviewCommit> commits;
private final ListMultimap<Change.Id, String> problems;
+ private final boolean allowClosed;
- private CommitStatus(ChangeSet cs) throws OrmException {
+ private CommitStatus(ChangeSet cs, boolean allowClosed) throws OrmException {
checkArgument(
!cs.furtherHiddenChanges(), "CommitStatus must not be called with hidden changes");
changes = cs.changesById();
@@ -130,6 +132,7 @@
byBranch = bb.build();
commits = new HashMap<>();
problems = MultimapBuilder.treeKeys(comparing(Change.Id::get)).arrayListValues(1).build();
+ this.allowClosed = allowClosed;
}
public ImmutableSet<Change.Id> getChangeIds() {
@@ -182,7 +185,7 @@
// date by this point.
ChangeData cd = checkNotNull(changes.get(id), "ChangeData for %s", id);
return checkNotNull(
- cd.getSubmitRecords(SUBMIT_RULE_OPTIONS),
+ cd.getSubmitRecords(submitRuleOptions(allowClosed)),
"getSubmitRecord only valid after submit rules are evalutated");
}
@@ -226,13 +229,15 @@
private final InternalChangeQuery internalChangeQuery;
private final SubmitStrategyFactory submitStrategyFactory;
private final SubmoduleOp.Factory subOpFactory;
- private final MergeOpRepoManager orm;
+ private final Provider<MergeOpRepoManager> ormProvider;
private final NotifyUtil notifyUtil;
+ private final RetryHelper retryHelper;
private Timestamp ts;
private RequestId submissionId;
private IdentifiedUser caller;
+ private MergeOpRepoManager orm;
private CommitStatus commitStatus;
private ReviewDb db;
private SubmitInput submitInput;
@@ -244,40 +249,45 @@
@Inject
MergeOp(
ChangeMessagesUtil cmUtil,
+ BatchUpdate.Factory batchUpdateFactory,
InternalUser.Factory internalUserFactory,
MergeSuperSet mergeSuperSet,
MergeValidators.Factory mergeValidatorsFactory,
InternalChangeQuery internalChangeQuery,
SubmitStrategyFactory submitStrategyFactory,
SubmoduleOp.Factory subOpFactory,
- MergeOpRepoManager orm,
+ Provider<MergeOpRepoManager> ormProvider,
NotifyUtil notifyUtil,
TopicMetrics topicMetrics,
- @Assisted BatchUpdate.Factory batchUpdateFactory) {
+ RetryHelper retryHelper) {
this.cmUtil = cmUtil;
+ this.batchUpdateFactory = batchUpdateFactory;
this.internalUserFactory = internalUserFactory;
this.mergeSuperSet = mergeSuperSet;
this.mergeValidatorsFactory = mergeValidatorsFactory;
this.internalChangeQuery = internalChangeQuery;
this.submitStrategyFactory = submitStrategyFactory;
this.subOpFactory = subOpFactory;
- this.orm = orm;
+ this.ormProvider = ormProvider;
this.notifyUtil = notifyUtil;
- this.batchUpdateFactory = batchUpdateFactory;
+ this.retryHelper = retryHelper;
this.topicMetrics = topicMetrics;
}
@Override
public void close() {
- orm.close();
+ if (orm != null) {
+ orm.close();
+ }
}
- public static void checkSubmitRule(ChangeData cd) throws ResourceConflictException, OrmException {
+ public static void checkSubmitRule(ChangeData cd, boolean allowClosed)
+ throws ResourceConflictException, OrmException {
PatchSet patchSet = cd.currentPatchSet();
if (patchSet == null) {
throw new ResourceConflictException("missing current patch set for change " + cd.getId());
}
- List<SubmitRecord> results = getSubmitRecords(cd);
+ List<SubmitRecord> results = getSubmitRecords(cd, allowClosed);
if (SubmitRecord.findOkRecord(results).isPresent()) {
// Rules supplied a valid solution.
return;
@@ -311,8 +321,13 @@
throw new IllegalStateException();
}
- private static List<SubmitRecord> getSubmitRecords(ChangeData cd) throws OrmException {
- return cd.submitRecords(SUBMIT_RULE_OPTIONS);
+ private static SubmitRuleOptions submitRuleOptions(boolean allowClosed) {
+ return allowClosed ? SUBMIT_RULE_OPTIONS_ALLOW_CLOSED : SUBMIT_RULE_OPTIONS;
+ }
+
+ private static List<SubmitRecord> getSubmitRecords(ChangeData cd, boolean allowClosed)
+ throws OrmException {
+ return cd.submitRecords(submitRuleOptions(allowClosed));
}
private static String describeLabels(ChangeData cd, List<SubmitRecord.Label> labels)
@@ -346,19 +361,23 @@
return Joiner.on("; ").join(labelResults);
}
- private void checkSubmitRulesAndState(ChangeSet cs) throws ResourceConflictException {
+ private void checkSubmitRulesAndState(ChangeSet cs, boolean allowMerged)
+ throws ResourceConflictException {
checkArgument(
!cs.furtherHiddenChanges(), "checkSubmitRulesAndState called for topic with hidden change");
for (ChangeData cd : cs.changes()) {
try {
- if (cd.change().getStatus() != Change.Status.NEW) {
- commitStatus.problem(
- cd.getId(),
- "Change " + cd.getId() + " is " + cd.change().getStatus().toString().toLowerCase());
+ Change.Status status = cd.change().getStatus();
+ if (status != Change.Status.NEW) {
+ if (!(status == Change.Status.MERGED && allowMerged)) {
+ commitStatus.problem(
+ cd.getId(),
+ "Change " + cd.getId() + " is " + cd.change().getStatus().toString().toLowerCase());
+ }
} else if (cd.change().isWorkInProgress()) {
commitStatus.problem(cd.getId(), "Change " + cd.getId() + " is work in progress");
} else {
- checkSubmitRule(cd);
+ checkSubmitRule(cd, allowMerged);
}
} catch (ResourceConflictException e) {
commitStatus.problem(cd.getId(), e.getMessage());
@@ -371,13 +390,13 @@
commitStatus.maybeFailVerbose();
}
- private void bypassSubmitRules(ChangeSet cs) {
+ private void bypassSubmitRules(ChangeSet cs, boolean allowClosed) {
checkArgument(
!cs.furtherHiddenChanges(), "cannot bypass submit rules for topic with hidden change");
for (ChangeData cd : cs.changes()) {
List<SubmitRecord> records;
try {
- records = new ArrayList<>(getSubmitRecords(cd));
+ records = new ArrayList<>(getSubmitRecords(cd, allowClosed));
} catch (OrmException e) {
log.warn("Error checking submit rules for change " + cd.getId(), e);
records = new ArrayList<>(1);
@@ -385,7 +404,7 @@
SubmitRecord forced = new SubmitRecord();
forced.status = SubmitRecord.Status.FORCED;
records.add(forced);
- cd.setSubmitRecords(SUBMIT_RULE_OPTIONS, records);
+ cd.setSubmitRecords(submitRuleOptions(allowClosed), records);
}
}
@@ -411,7 +430,7 @@
boolean checkSubmitRules,
SubmitInput submitInput,
boolean dryrun)
- throws OrmException, RestApiException {
+ throws OrmException, RestApiException, UpdateException {
this.submitInput = submitInput;
this.accountsToNotify = notifyUtil.resolveAccounts(submitInput.notifyDetails);
this.dryrun = dryrun;
@@ -419,7 +438,7 @@
this.ts = TimeUtil.nowTs();
submissionId = RequestId.forChange(change);
this.db = db;
- orm.setContext(db, ts, caller, submissionId);
+ openRepoManager();
logDebug("Beginning integration of {}", change);
try {
@@ -430,21 +449,47 @@
throw new AuthException(
"A change to be submitted with " + change.getId() + " is not visible");
}
- this.commitStatus = new CommitStatus(cs);
- MergeSuperSet.reloadChanges(cs);
logDebug("Calculated to merge {}", cs);
- if (checkSubmitRules) {
- logDebug("Checking submit rules and state");
- checkSubmitRulesAndState(cs);
- } else {
- logDebug("Bypassing submit rules");
- bypassSubmitRules(cs);
+
+ // Count cross-project submissions outside of the retry loop. The chance of a single project
+ // failing increases with the number of projects, so the failure count would be inflated if
+ // this metric were incremented inside of integrateIntoHistory.
+ int projects = cs.projects().size();
+ if (projects > 1) {
+ topicMetrics.topicSubmissions.increment();
}
- try {
- integrateIntoHistory(cs);
- } catch (IntegrationException e) {
- logError("Error from integrateIntoHistory", e);
- throw new ResourceConflictException(e.getMessage(), e);
+
+ RetryTracker retryTracker = new RetryTracker();
+ retryHelper.execute(
+ updateFactory -> {
+ long attempt = retryTracker.lastAttemptNumber + 1;
+ boolean isRetry = attempt > 1;
+ if (isRetry) {
+ logDebug("Retrying, attempt #{}; skipping merged changes", attempt);
+ this.ts = TimeUtil.nowTs();
+ openRepoManager();
+ }
+ this.commitStatus = new CommitStatus(cs, isRetry);
+ MergeSuperSet.reloadChanges(cs);
+ if (checkSubmitRules) {
+ logDebug("Checking submit rules and state");
+ checkSubmitRulesAndState(cs, isRetry);
+ } else {
+ logDebug("Bypassing submit rules");
+ bypassSubmitRules(cs, isRetry);
+ }
+ try {
+ integrateIntoHistory(cs);
+ } catch (IntegrationException e) {
+ logError("Error from integrateIntoHistory", e);
+ throw new ResourceConflictException(e.getMessage(), e);
+ }
+ return null;
+ },
+ retryTracker);
+
+ if (projects > 1) {
+ topicMetrics.topicSubmissionsCompleted.increment();
}
} catch (IOException e) {
// Anything before the merge attempt is an error
@@ -452,6 +497,23 @@
}
}
+ private void openRepoManager() {
+ if (orm != null) {
+ orm.close();
+ }
+ orm = ormProvider.get();
+ orm.setContext(db, ts, caller, submissionId);
+ }
+
+ private class RetryTracker implements RetryListener {
+ long lastAttemptNumber;
+
+ @Override
+ public <V> void onRetry(Attempt<V> attempt) {
+ lastAttemptNumber = attempt.getAttemptNumber();
+ }
+ }
+
@Singleton
private static class TopicMetrics {
final Counter0 topicSubmissions;
@@ -471,7 +533,8 @@
}
}
- private void integrateIntoHistory(ChangeSet cs) throws IntegrationException, RestApiException {
+ private void integrateIntoHistory(ChangeSet cs)
+ throws IntegrationException, RestApiException, UpdateException {
checkArgument(!cs.furtherHiddenChanges(), "cannot integrate hidden changes into history");
logDebug("Beginning merge attempt on {}", cs);
Map<Branch.NameKey, BranchBatch> toSubmit = new HashMap<>();
@@ -484,30 +547,22 @@
}
Set<Branch.NameKey> branches = cbb.keySet();
- int projects = 0;
for (Branch.NameKey branch : branches) {
OpenRepo or = openRepo(branch.getParentKey());
if (or != null) {
- BranchBatch bb = validateChangeList(or, cbb.get(branch));
- toSubmit.put(branch, bb);
- if (!bb.commits().isEmpty()) {
- projects++;
- }
+ toSubmit.put(branch, validateChangeList(or, cbb.get(branch)));
}
}
// Done checks that don't involve running submit strategies.
commitStatus.maybeFailVerbose();
- if (projects > 1) {
- topicMetrics.topicSubmissions.increment();
- }
try {
SubmoduleOp submoduleOp = subOpFactory.create(branches, orm);
List<SubmitStrategy> strategies = getSubmitStrategies(toSubmit, submoduleOp, dryrun);
this.allProjects = submoduleOp.getProjectsInOrder();
batchUpdateFactory.execute(
- orm.batchUpdates(batchUpdateFactory, allProjects),
+ orm.batchUpdates(allProjects),
new SubmitStrategyListener(submitInput, strategies, commitStatus),
submissionId,
dryrun);
@@ -516,6 +571,15 @@
} catch (IOException | SubmoduleException e) {
throw new IntegrationException(e);
} catch (UpdateException e) {
+ if (e.getCause() instanceof LockFailureException) {
+ // Lock failures are a special case: RetryHelper depends on this specific causal chain in
+ // order to trigger a retry. The downside of throwing here is we will not get the nicer
+ // error message constructed below, in the case where this is the final attempt and the
+ // operation is not retried further. This is not a huge downside, and is hopefully so rare
+ // as to be unnoticeable, assuming RetryHelper is retrying sufficiently.
+ throw 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
@@ -531,10 +595,6 @@
}
throw new IntegrationException(msg, e);
}
-
- if (projects > 1) {
- topicMetrics.topicSubmissionsCompleted.increment();
- }
}
public Set<Project.NameKey> getAllProjects() {
@@ -545,10 +605,6 @@
return orm;
}
- public BatchUpdate.Factory getBatchUpdateFactory() {
- return batchUpdateFactory;
- }
-
private List<SubmitStrategy> getSubmitStrategies(
Map<Branch.NameKey, BranchBatch> toSubmit, SubmoduleOp submoduleOp, boolean dryrun)
throws IntegrationException, NoSuchProjectException, IOException {
@@ -580,20 +636,20 @@
ob.mergeTip,
commitStatus,
submissionId,
- submitInput.notify,
+ submitInput,
accountsToNotify,
submoduleOp,
dryrun);
strategies.add(strategy);
- strategy.addOps(or.getUpdate(batchUpdateFactory), commitsToSubmit);
+ strategy.addOps(or.getUpdate(), commitsToSubmit);
if (submitting.submitType().equals(SubmitType.FAST_FORWARD_ONLY)
&& submoduleOp.hasSubscription(branch)) {
- submoduleOp.addOp(or.getUpdate(batchUpdateFactory), branch);
+ submoduleOp.addOp(or.getUpdate(), branch);
}
} else {
// no open change for this branch
// add submodule triggered op into BatchUpdate
- submoduleOp.addOp(or.getUpdate(batchUpdateFactory), branch);
+ submoduleOp.addOp(or.getUpdate(), branch);
}
}
return strategies;
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 ae2c481..ad205f8 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
@@ -101,7 +101,7 @@
return rw;
}
- public BatchUpdate getUpdate(BatchUpdate.Factory batchUpdateFactory) {
+ public BatchUpdate getUpdate() {
checkState(db != null, "call setContext before getUpdate");
if (update == null) {
update =
@@ -149,6 +149,7 @@
}
private final Map<Project.NameKey, OpenRepo> openRepos;
+ private final BatchUpdate.Factory batchUpdateFactory;
private final OnSubmitValidators.Factory onSubmitValidatorsFactory;
private final GitRepositoryManager repoManager;
private final ProjectCache projectCache;
@@ -162,9 +163,11 @@
MergeOpRepoManager(
GitRepositoryManager repoManager,
ProjectCache projectCache,
+ BatchUpdate.Factory batchUpdateFactory,
OnSubmitValidators.Factory onSubmitValidatorsFactory) {
this.repoManager = repoManager;
this.projectCache = projectCache;
+ this.batchUpdateFactory = batchUpdateFactory;
this.onSubmitValidatorsFactory = onSubmitValidatorsFactory;
openRepos = new HashMap<>();
@@ -199,12 +202,11 @@
}
}
- public List<BatchUpdate> batchUpdates(
- BatchUpdate.Factory batchUpdateFactory, Collection<Project.NameKey> projects)
+ public List<BatchUpdate> batchUpdates(Collection<Project.NameKey> projects)
throws NoSuchProjectException, IOException {
List<BatchUpdate> updates = new ArrayList<>(projects.size());
for (Project.NameKey project : projects) {
- updates.add(getRepo(project).getUpdate(batchUpdateFactory).setRefLogMessage("merged"));
+ updates.add(getRepo(project).getUpdate().setRefLogMessage("merged"));
}
return updates;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/NotesBranchUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/NotesBranchUtil.java
index fe35231..24b3727 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/NotesBranchUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/NotesBranchUtil.java
@@ -14,32 +14,29 @@
package com.google.gerrit.server.git;
+import static com.google.common.base.MoreObjects.firstNonNull;
+
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
+import com.google.gerrit.server.update.RefUpdateUtil;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
-import org.eclipse.jgit.api.errors.ConcurrentRefUpdateException;
-import org.eclipse.jgit.errors.CorruptObjectException;
-import org.eclipse.jgit.errors.IncorrectObjectTypeException;
-import org.eclipse.jgit.errors.MissingObjectException;
+import org.eclipse.jgit.lib.BatchRefUpdate;
import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
-import org.eclipse.jgit.lib.RefUpdate;
-import org.eclipse.jgit.lib.RefUpdate.Result;
import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.merge.MergeStrategy;
import org.eclipse.jgit.notes.Note;
import org.eclipse.jgit.notes.NoteMap;
-import org.eclipse.jgit.notes.NoteMapMerger;
import org.eclipse.jgit.notes.NoteMerger;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.transport.ReceiveCommand;
/** A utility class for updating a notes branch with automatic merge of note trees. */
public class NotesBranchUtil {
@@ -47,9 +44,6 @@
NotesBranchUtil create(Project.NameKey project, Repository db, ObjectInserter inserter);
}
- private static final int MAX_LOCK_FAILURE_CALLS = 10;
- private static final int SLEEP_ON_LOCK_FAILURE_MS = 25;
-
private final PersonIdent gerritIdent;
private final GitReferenceUpdated gitRefUpdated;
private final Project.NameKey project;
@@ -86,16 +80,20 @@
* Create a new commit in the {@code notesBranch} by updating existing or creating new notes from
* the {@code notes} map.
*
+ * <p>Does not retry in the case of lock failure; callers may use {@link
+ * com.google.gerrit.server.update.RetryHelper}.
+ *
* @param notes map of notes
* @param notesBranch notes branch to update
* @param commitAuthor author of the commit in the notes branch
* @param commitMessage for the commit in the notes branch
- * @throws IOException
- * @throws ConcurrentRefUpdateException
+ * @throws LockFailureException if committing the notes failed due to a lock failure on the notes
+ * branch
+ * @throws IOException if committing the notes failed for any other reason
*/
public final void commitAllNotes(
NoteMap notes, String notesBranch, PersonIdent commitAuthor, String commitMessage)
- throws IOException, ConcurrentRefUpdateException {
+ throws IOException {
this.overwrite = true;
commitNotes(notes, notesBranch, commitAuthor, commitMessage);
}
@@ -105,17 +103,21 @@
* {@code notes} map. The notes from the {@code notes} map which already exist in the note-tree of
* the tip of the {@code notesBranch} will not be updated.
*
+ * <p>Does not retry in the case of lock failure; callers may use {@link
+ * com.google.gerrit.server.update.RetryHelper}.
+ *
* @param notes map of notes
* @param notesBranch notes branch to update
* @param commitAuthor author of the commit in the notes branch
* @param commitMessage for the commit in the notes branch
* @return map with those notes from the {@code notes} that were newly created
- * @throws IOException
- * @throws ConcurrentRefUpdateException
+ * @throws LockFailureException if committing the notes failed due to a lock failure on the notes
+ * branch
+ * @throws IOException if committing the notes failed for any other reason
*/
public final NoteMap commitNewNotes(
NoteMap notes, String notesBranch, PersonIdent commitAuthor, String commitMessage)
- throws IOException, ConcurrentRefUpdateException {
+ throws IOException {
this.overwrite = false;
commitNotes(notes, notesBranch, commitAuthor, commitMessage);
NoteMap newlyCreated = NoteMap.newEmptyMap();
@@ -129,7 +131,7 @@
private void commitNotes(
NoteMap notes, String notesBranch, PersonIdent commitAuthor, String commitMessage)
- throws IOException, ConcurrentRefUpdateException {
+ throws LockFailureException, IOException {
try {
revWalk = new RevWalk(db);
reader = db.newObjectReader();
@@ -209,61 +211,16 @@
return revWalk.parseCommit(commitId);
}
- private void updateRef(String notesBranch)
- throws IOException, MissingObjectException, IncorrectObjectTypeException,
- CorruptObjectException, ConcurrentRefUpdateException {
+ private void updateRef(String notesBranch) throws LockFailureException, IOException {
if (baseCommit != null && oursCommit.getTree().equals(baseCommit.getTree())) {
// If the trees are identical, there is no change in the notes.
// Avoid saving this commit as it has no new information.
return;
}
-
- int remainingLockFailureCalls = MAX_LOCK_FAILURE_CALLS;
- RefUpdate refUpdate = createRefUpdate(notesBranch, oursCommit, baseCommit);
-
- for (; ; ) {
- Result result = refUpdate.update();
-
- if (result == Result.LOCK_FAILURE) {
- if (--remainingLockFailureCalls > 0) {
- try {
- Thread.sleep(SLEEP_ON_LOCK_FAILURE_MS);
- } catch (InterruptedException e) {
- // ignore
- }
- } else {
- throw new ConcurrentRefUpdateException(
- "Failed to lock the ref: " + notesBranch, refUpdate.getRef(), result);
- }
-
- } else if (result == Result.REJECTED) {
- RevCommit theirsCommit = revWalk.parseCommit(refUpdate.getOldObjectId());
- NoteMap theirs = NoteMap.read(revWalk.getObjectReader(), theirsCommit);
- NoteMapMerger merger = new NoteMapMerger(db, getNoteMerger(), MergeStrategy.RESOLVE);
- NoteMap merged = merger.merge(base, ours, theirs);
- RevCommit mergeCommit =
- createCommit(merged, gerritIdent, "Merged note commits\n", theirsCommit, oursCommit);
- refUpdate = createRefUpdate(notesBranch, mergeCommit, theirsCommit);
- remainingLockFailureCalls = MAX_LOCK_FAILURE_CALLS;
-
- } else if (result == Result.IO_FAILURE) {
- throw new IOException("Couldn't update " + notesBranch + ". " + result.name());
- } else {
- gitRefUpdated.fire(project, refUpdate, null);
- break;
- }
- }
- }
-
- private RefUpdate createRefUpdate(
- String notesBranch, ObjectId newObjectId, ObjectId expectedOldObjectId) throws IOException {
- RefUpdate refUpdate = db.updateRef(notesBranch);
- refUpdate.setNewObjectId(newObjectId);
- if (expectedOldObjectId == null) {
- refUpdate.setExpectedOldObjectId(ObjectId.zeroId());
- } else {
- refUpdate.setExpectedOldObjectId(expectedOldObjectId);
- }
- return refUpdate;
+ BatchRefUpdate bru = db.getRefDatabase().newBatchUpdate();
+ bru.addCommand(
+ new ReceiveCommand(firstNonNull(baseCommit, ObjectId.zeroId()), oursCommit, notesBranch));
+ RefUpdateUtil.executeChecked(bru, revWalk);
+ gitRefUpdated.fire(project, bru, null);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
index 585c8e2..86daf8e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
@@ -346,7 +346,7 @@
private Map<String, Ref> allRefs;
private final SubmoduleOp.Factory subOpFactory;
- private final MergeOp.Factory mergeOpFactory;
+ private final Provider<MergeOp> mergeOpProvider;
private final Provider<MergeOpRepoManager> ormProvider;
private final DynamicMap<ProjectConfigEntry> pluginConfigEntries;
private final NotesMigration notesMigration;
@@ -400,7 +400,7 @@
@Assisted ProjectControl projectControl,
@Assisted Repository repo,
SubmoduleOp.Factory subOpFactory,
- MergeOp.Factory mergeOpFactory,
+ Provider<MergeOp> mergeOpProvider,
Provider<MergeOpRepoManager> ormProvider,
DynamicMap<ProjectConfigEntry> pluginConfigEntries,
NotesMigration notesMigration,
@@ -448,7 +448,7 @@
this.receiveId = RequestId.forProject(project.getNameKey());
this.subOpFactory = subOpFactory;
- this.mergeOpFactory = mergeOpFactory;
+ this.mergeOpProvider = mergeOpProvider;
this.ormProvider = ormProvider;
this.pluginConfigEntries = pluginConfigEntries;
this.notesMigration = notesMigration;
@@ -657,7 +657,7 @@
try (MergeOpRepoManager orm = ormProvider.get()) {
orm.setContext(db, TimeUtil.nowTs(), user, receiveId);
SubmoduleOp op = subOpFactory.create(branches, orm);
- op.updateSuperProjects(batchUpdateFactory);
+ op.updateSuperProjects();
} catch (SubmoduleException e) {
logError("Can't update the superprojects", e);
}
@@ -815,7 +815,7 @@
} catch (ResourceConflictException e) {
addMessage(e.getMessage());
reject(magicBranchCmd, "conflict");
- } catch (RestApiException | OrmException e) {
+ } catch (RestApiException | OrmException | UpdateException e) {
logError("Error submitting changes to " + project.getName(), e);
reject(magicBranchCmd, "error during submit");
}
@@ -2261,7 +2261,7 @@
}
private void submit(Collection<CreateRequest> create, Collection<ReplaceRequest> replace)
- throws OrmException, RestApiException {
+ throws OrmException, RestApiException, UpdateException {
Map<ObjectId, Change> bySha = Maps.newHashMapWithExpectedSize(create.size() + replace.size());
for (CreateRequest r : create) {
checkNotNull(r.change, "cannot submit new change %s; op may not have run", r.changeId);
@@ -2275,7 +2275,7 @@
tipChange, "tip of push does not correspond to a change; found these changes: %s", bySha);
logDebug(
"Processing submit with tip change {} ({})", tipChange.getId(), magicBranch.cmd.getNewId());
- try (MergeOp op = mergeOpFactory.create(batchUpdateFactory)) {
+ try (MergeOp op = mergeOpProvider.get()) {
op.merge(db, tipChange, user, false, new SubmitInput(), false);
}
}
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 69909eb..eb9c024 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
@@ -355,7 +355,7 @@
return ret;
}
- public void updateSuperProjects(BatchUpdate.Factory updateFactory) throws SubmoduleException {
+ public void updateSuperProjects() throws SubmoduleException {
ImmutableSet<Project.NameKey> projects = getProjectsInOrder();
if (projects == null) {
return;
@@ -370,15 +370,12 @@
// get a new BatchUpdate for the super project
OpenRepo or = orm.getRepo(project);
for (Branch.NameKey branch : branchesByProject.get(project)) {
- addOp(or.getUpdate(updateFactory), branch);
+ addOp(or.getUpdate(), branch);
}
}
}
batchUpdateFactory.execute(
- orm.batchUpdates(updateFactory, superProjects),
- BatchUpdateListener.NONE,
- orm.getSubmissionId(),
- false);
+ orm.batchUpdates(superProjects), BatchUpdateListener.NONE, orm.getSubmissionId(), false);
} catch (RestApiException | UpdateException | IOException | NoSuchProjectException e) {
throw new SubmoduleException("Cannot update gitlinks", e);
}
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 95bdf33..9892b6e 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
@@ -18,12 +18,13 @@
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Sets;
-import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.api.changes.RecipientType;
+import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.config.FactoryModule;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Branch;
+import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeMessagesUtil;
@@ -32,6 +33,7 @@
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.change.RebaseChangeOp;
+import com.google.gerrit.server.change.Submit.TestSubmitInput;
import com.google.gerrit.server.extensions.events.ChangeMerged;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
@@ -96,7 +98,7 @@
Set<RevCommit> alreadyAccepted,
Set<CodeReviewCommit> incoming,
RequestId submissionId,
- NotifyHandling notifyHandling,
+ SubmitInput submitInput,
ListMultimap<RecipientType, Account.Id> accountsToNotify,
SubmoduleOp submoduleOp,
boolean dryrun);
@@ -129,7 +131,7 @@
final Set<RevCommit> alreadyAccepted;
final RequestId submissionId;
final SubmitType submitType;
- final NotifyHandling notifyHandling;
+ final SubmitInput submitInput;
final ListMultimap<RecipientType, Account.Id> accountsToNotify;
final SubmoduleOp submoduleOp;
@@ -169,7 +171,7 @@
@Assisted Set<CodeReviewCommit> incoming,
@Assisted RequestId submissionId,
@Assisted SubmitType submitType,
- @Assisted NotifyHandling notifyHandling,
+ @Assisted SubmitInput submitInput,
@Assisted ListMultimap<RecipientType, Account.Id> accountsToNotify,
@Assisted SubmoduleOp submoduleOp,
@Assisted boolean dryrun) {
@@ -199,7 +201,7 @@
this.alreadyAccepted = alreadyAccepted;
this.submissionId = submissionId;
this.submitType = submitType;
- this.notifyHandling = notifyHandling;
+ this.submitInput = submitInput;
this.accountsToNotify = accountsToNotify;
this.submoduleOp = submoduleOp;
this.dryrun = dryrun;
@@ -255,12 +257,21 @@
List<CodeReviewCommit> difference = new ArrayList<>(Sets.difference(toMerge, added));
Collections.reverse(difference);
for (CodeReviewCommit c : difference) {
- bu.addOp(c.change().getId(), new ImplicitIntegrateOp(args, c));
+ Change.Id id = c.change().getId();
+ bu.addOp(id, new ImplicitIntegrateOp(args, c));
+ maybeAddTestHelperOp(bu, id);
}
// Then ops for explicitly merged changes
for (SubmitStrategyOp op : ops) {
bu.addOp(op.getId(), op);
+ maybeAddTestHelperOp(bu, op.getId());
+ }
+ }
+
+ private void maybeAddTestHelperOp(BatchUpdate bu, Change.Id changeId) {
+ if (args.submitInput instanceof TestSubmitInput) {
+ bu.addOp(changeId, new TestHelperOp(changeId, args));
}
}
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 8f43a49..7678623 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
@@ -15,8 +15,8 @@
package com.google.gerrit.server.git.strategy;
import com.google.common.collect.ListMultimap;
-import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.api.changes.RecipientType;
+import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Branch;
@@ -61,7 +61,7 @@
MergeTip mergeTip,
CommitStatus commitStatus,
RequestId submissionId,
- NotifyHandling notifyHandling,
+ SubmitInput submitInput,
ListMultimap<RecipientType, Account.Id> accountsToNotify,
SubmoduleOp submoduleOp,
boolean dryrun)
@@ -79,7 +79,7 @@
alreadyAccepted,
incoming,
submissionId,
- notifyHandling,
+ submitInput,
accountsToNotify,
submoduleOp,
dryrun);
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 6bf8b2e..aaad040 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
@@ -77,7 +77,8 @@
private ObjectId mergeResultRev;
private PatchSet mergedPatchSet;
private Change updatedChange;
- private CodeReviewCommit alreadyMerged;
+ private CodeReviewCommit alreadyMergedCommit;
+ private boolean changeAlreadyMerged;
protected SubmitStrategyOp(SubmitStrategy.Arguments args, CodeReviewCommit toMerge) {
this.args = args;
@@ -112,11 +113,11 @@
// Run the submit strategy implementation and record the merge tip state so
// we can create the ref update.
CodeReviewCommit tipBefore = args.mergeTip.getCurrentTip();
- alreadyMerged = getAlreadyMergedCommit(ctx);
- if (alreadyMerged == null) {
+ alreadyMergedCommit = getAlreadyMergedCommit(ctx);
+ if (alreadyMergedCommit == null) {
updateRepoImpl(ctx);
} else {
- logDebug("Already merged as {}", alreadyMerged.name());
+ logDebug("Already merged as {}", alreadyMergedCommit.name());
}
CodeReviewCommit tipAfter = args.mergeTip.getCurrentTip();
@@ -219,8 +220,23 @@
PatchSet.Id oldPsId = checkNotNull(toMerge.getPatchsetId());
PatchSet.Id newPsId;
- if (alreadyMerged != null) {
- alreadyMerged.setControl(ctx.getControl());
+ if (ctx.getChange().getStatus() == Change.Status.MERGED) {
+ // Either another thread won a race, or we are retrying a whole topic submission after one
+ // repo failed with lock failure.
+ if (alreadyMergedCommit == null) {
+ logDebug(
+ "Change is already merged according to its status, but we were unable to find it"
+ + " merged into the current tip ({})",
+ args.mergeTip.getCurrentTip().name());
+ } else {
+ logDebug("Change is already merged");
+ }
+ changeAlreadyMerged = true;
+ return false;
+ }
+
+ if (alreadyMergedCommit != null) {
+ alreadyMergedCommit.setControl(ctx.getControl());
mergedPatchSet = getOrCreateAlreadyMergedPatchSet(ctx);
newPsId = mergedPatchSet.getId();
} else {
@@ -263,12 +279,12 @@
setApproval(ctx, args.caller);
mergeResultRev =
- alreadyMerged == null
+ alreadyMergedCommit == null
? args.mergeTip.getMergeResults().get(commit)
// Our fixup code is not smart enough to find a merge commit
// corresponding to the merge result. This results in a different
// ChangeMergedEvent in the fixup case, but we'll just live with that.
- : alreadyMerged;
+ : alreadyMergedCommit;
try {
setMerged(ctx, message(ctx, commit, s));
} catch (OrmException err) {
@@ -284,13 +300,13 @@
private PatchSet getOrCreateAlreadyMergedPatchSet(ChangeContext ctx)
throws IOException, OrmException {
- PatchSet.Id psId = alreadyMerged.getPatchsetId();
+ PatchSet.Id psId = alreadyMergedCommit.getPatchsetId();
logDebug("Fixing up already-merged patch set {}", psId);
PatchSet prevPs = args.psUtil.current(ctx.getDb(), ctx.getNotes());
- ctx.getRevWalk().parseBody(alreadyMerged);
+ ctx.getRevWalk().parseBody(alreadyMergedCommit);
ctx.getChange()
.setCurrentPatchSet(
- psId, alreadyMerged.getShortMessage(), ctx.getChange().getOriginalSubject());
+ psId, alreadyMergedCommit.getShortMessage(), ctx.getChange().getOriginalSubject());
PatchSet existing = args.psUtil.get(ctx.getDb(), ctx.getNotes(), psId);
if (existing != null) {
logDebug("Patch set row exists, only updating change");
@@ -300,13 +316,13 @@
// a patch set ref. Fix up the database. Note that this uses the current
// user as the uploader, which is as good a guess as any.
List<String> groups =
- prevPs != null ? prevPs.getGroups() : GroupCollector.getDefaultGroups(alreadyMerged);
+ prevPs != null ? prevPs.getGroups() : GroupCollector.getDefaultGroups(alreadyMergedCommit);
return args.psUtil.insert(
ctx.getDb(),
ctx.getRevWalk(),
ctx.getUpdate(psId),
psId,
- alreadyMerged,
+ alreadyMergedCommit,
false,
groups,
null,
@@ -482,6 +498,18 @@
@Override
public final void postUpdate(Context ctx) throws Exception {
+ if (changeAlreadyMerged) {
+ // TODO(dborowitz): This is suboptimal behavior in the presence of retries: postUpdate steps
+ // will never get run for changes that submitted successfully on any but the final attempt.
+ // This is primarily a temporary workaround for the fact that the submitter field is not
+ // populated in the changeAlreadyMerged case.
+ //
+ // If we naively execute postUpdate even if the change is already merged when updateChange
+ // being, then we are subject to a race where postUpdate steps are run twice if two submit
+ // processes run at the same time.
+ logDebug("Skipping post-update steps for change {}", getId());
+ return;
+ }
postUpdateImpl(ctx);
if (command != null) {
@@ -508,7 +536,7 @@
ctx.getProject(),
getId(),
submitter.getAccountId(),
- args.notifyHandling,
+ args.submitInput.notify,
args.accountsToNotify)
.sendAsync();
} catch (Exception e) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/TestHelperOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/TestHelperOp.java
new file mode 100644
index 0000000..8d95045
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/TestHelperOp.java
@@ -0,0 +1,58 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.git.strategy;
+
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.server.change.Submit.TestSubmitInput;
+import com.google.gerrit.server.update.BatchUpdateOp;
+import com.google.gerrit.server.update.RepoContext;
+import com.google.gerrit.server.util.RequestId;
+import java.io.IOException;
+import java.util.Queue;
+import org.eclipse.jgit.lib.ObjectId;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+class TestHelperOp implements BatchUpdateOp {
+ private static final Logger log = LoggerFactory.getLogger(TestHelperOp.class);
+
+ private final Change.Id changeId;
+ private final TestSubmitInput input;
+ private final RequestId submissionId;
+
+ TestHelperOp(Change.Id changeId, SubmitStrategy.Arguments args) {
+ this.changeId = changeId;
+ this.input = (TestSubmitInput) args.submitInput;
+ this.submissionId = args.submissionId;
+ }
+
+ @Override
+ public void updateRepo(RepoContext ctx) throws IOException {
+ Queue<Boolean> q = input.generateLockFailures;
+ if (q != null && !q.isEmpty() && q.remove()) {
+ logDebug("Adding bogus ref update to trigger lock failure, via change {}", changeId);
+ ctx.addRefUpdate(
+ ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"),
+ ObjectId.zeroId(),
+ "refs/test/" + getClass().getSimpleName());
+ }
+ }
+
+ private void logDebug(String msg, Object... args) {
+ if (log.isDebugEnabled()) {
+ log.debug(submissionId + msg, args);
+ }
+ }
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java
index 4edfab2..431ac7f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java
@@ -19,7 +19,6 @@
import com.google.common.base.Function;
import com.google.common.util.concurrent.Atomics;
-import com.google.common.util.concurrent.CheckedFuture;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningExecutorService;
@@ -73,7 +72,8 @@
ChangeIndexer create(ListeningExecutorService executor, ChangeIndexCollection indexes);
}
- public static CheckedFuture<?, IOException> allAsList(
+ @SuppressWarnings("deprecation")
+ public static com.google.common.util.concurrent.CheckedFuture<?, IOException> allAsList(
List<? extends ListenableFuture<?>> futures) {
// allAsList propagates the first seen exception, wrapped in
// ExecutionException, so we can reuse the same mapper as for a single
@@ -173,7 +173,9 @@
* @param id change to index.
* @return future for the indexing task.
*/
- public CheckedFuture<?, IOException> indexAsync(Project.NameKey project, Change.Id id) {
+ @SuppressWarnings("deprecation")
+ public com.google.common.util.concurrent.CheckedFuture<?, IOException> indexAsync(
+ Project.NameKey project, Change.Id id) {
return submit(new IndexTask(project, id));
}
@@ -183,7 +185,8 @@
* @param ids changes to index.
* @return future for completing indexing of all changes.
*/
- public CheckedFuture<?, IOException> indexAsync(
+ @SuppressWarnings("deprecation")
+ public com.google.common.util.concurrent.CheckedFuture<?, IOException> indexAsync(
Project.NameKey project, Collection<Change.Id> ids) {
List<ListenableFuture<?>> futures = new ArrayList<>(ids.size());
for (Change.Id id : ids) {
@@ -277,7 +280,8 @@
* @param id change to delete.
* @return future for the deleting task.
*/
- public CheckedFuture<?, IOException> deleteAsync(Change.Id id) {
+ @SuppressWarnings("deprecation")
+ public com.google.common.util.concurrent.CheckedFuture<?, IOException> deleteAsync(Change.Id id) {
return submit(new DeleteTask(id));
}
@@ -300,7 +304,9 @@
* @param id ID of the change to index.
* @return future for reindexing the change; returns true if the change was stale.
*/
- public CheckedFuture<Boolean, IOException> reindexIfStale(Project.NameKey project, Change.Id id) {
+ @SuppressWarnings("deprecation")
+ public com.google.common.util.concurrent.CheckedFuture<Boolean, IOException> reindexIfStale(
+ Project.NameKey project, Change.Id id) {
return submit(new ReindexIfStaleTask(project, id), batchExecutor);
}
@@ -324,11 +330,14 @@
return indexes != null ? indexes.getWriteIndexes() : Collections.singleton(index);
}
- private <T> CheckedFuture<T, IOException> submit(Callable<T> task) {
+ @SuppressWarnings("deprecation")
+ private <T> com.google.common.util.concurrent.CheckedFuture<T, IOException> submit(
+ Callable<T> task) {
return submit(task, executor);
}
- private static <T> CheckedFuture<T, IOException> submit(
+ @SuppressWarnings("deprecation")
+ private static <T> com.google.common.util.concurrent.CheckedFuture<T, IOException> submit(
Callable<T> task, ListeningExecutorService executor) {
return Futures.makeChecked(Futures.nonCancellationPropagating(executor.submit(task)), MAPPER);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java
index a9f5306..2ab5c55 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java
@@ -90,11 +90,7 @@
Account.Id accountId = Account.Id.fromRef(event.getRefName());
if (accountId != null) {
try {
- if (event.isDelete()) {
- // TODO(ekempin): Delete account from cache and index.
- } else {
- accountCache.evict(accountId);
- }
+ accountCache.evict(accountId);
} catch (IOException e) {
log.error(String.format("Reindex account %s failed.", accountId), e);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
index 6b3492a..45cf244 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbUpdateManager.java
@@ -22,7 +22,6 @@
import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES;
import com.google.auto.value.AutoValue;
-import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.HashBasedTable;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ListMultimap;
@@ -39,9 +38,9 @@
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.InMemoryInserter;
import com.google.gerrit.server.git.InsertedObject;
-import com.google.gerrit.server.git.LockFailureException;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.update.ChainedReceiveCommands;
+import com.google.gerrit.server.update.RefUpdateUtil;
import com.google.gwtorm.server.OrmConcurrencyException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -55,9 +54,7 @@
import java.util.Optional;
import java.util.Set;
import org.eclipse.jgit.errors.ConfigInvalidException;
-import org.eclipse.jgit.internal.JGitText;
import org.eclipse.jgit.lib.BatchRefUpdate;
-import org.eclipse.jgit.lib.NullProgressMonitor;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.ObjectReader;
@@ -548,54 +545,11 @@
bru.setAllowNonFastForwards(true);
if (!dryrun) {
- bru.execute(or.rw, NullProgressMonitor.INSTANCE);
- checkResults(bru);
+ RefUpdateUtil.executeChecked(bru, or.rw);
}
return bru;
}
- /**
- * Check results of all commands in the update batch, reducing to a single exception if there was
- * a failure.
- *
- * <p>Throws {@link LockFailureException} if at least one command failed with {@code
- * LOCK_FAILURE}, and the entire transaction was aborted, i.e. any non-{@code LOCK_FAILURE}
- * results, if there were any, failed with "transaction aborted".
- *
- * <p>In particular, if the underlying ref database does not {@link
- * org.eclipse.jgit.lib.RefDatabase#performsAtomicTransactions() perform atomic transactions},
- * then a combination of {@code LOCK_FAILURE} on one ref and {@code OK} or another result on other
- * refs will <em>not</em> throw {@code LockFailureException}.
- *
- * @param bru batch update; should already have been executed.
- * @throws LockFailureException if the transaction was aborted due to lock failure.
- * @throws IOException if any result was not {@code OK}.
- */
- @VisibleForTesting
- static void checkResults(BatchRefUpdate bru) throws LockFailureException, IOException {
- int lockFailure = 0;
- int aborted = 0;
- int failure = 0;
-
- for (ReceiveCommand cmd : bru.getCommands()) {
- if (cmd.getResult() != ReceiveCommand.Result.OK) {
- failure++;
- }
- if (cmd.getResult() == ReceiveCommand.Result.LOCK_FAILURE) {
- lockFailure++;
- } else if (cmd.getResult() == ReceiveCommand.Result.REJECTED_OTHER_REASON
- && JGitText.get().transactionAborted.equals(cmd.getMessage())) {
- aborted++;
- }
- }
-
- if (lockFailure + aborted == bru.getCommands().size()) {
- throw new LockFailureException("Update aborted with one or more lock failures: " + bru);
- } else if (failure > 0) {
- throw new IOException("Update failed: " + bru);
- }
- }
-
private void addCommands() throws OrmException, IOException {
if (isEmpty()) {
return;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/BanCommit.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/BanCommit.java
index 41e8fbc..278b2af 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/BanCommit.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/BanCommit.java
@@ -17,21 +17,24 @@
import com.google.common.collect.Lists;
import com.google.gerrit.common.errors.PermissionDeniedException;
import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.extensions.restapi.ResourceConflictException;
-import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.server.git.BanCommitResult;
+import com.google.gerrit.server.project.BanCommit.BanResultInfo;
import com.google.gerrit.server.project.BanCommit.Input;
+import com.google.gerrit.server.update.BatchUpdate;
+import com.google.gerrit.server.update.RetryHelper;
+import com.google.gerrit.server.update.RetryingRestModifyView;
+import com.google.gerrit.server.update.UpdateException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
-import org.eclipse.jgit.api.errors.ConcurrentRefUpdateException;
import org.eclipse.jgit.lib.ObjectId;
@Singleton
-public class BanCommit implements RestModifyView<ProjectResource, Input> {
+public class BanCommit extends RetryingRestModifyView<ProjectResource, Input, BanResultInfo> {
public static class Input {
public List<String> commits;
public String reason;
@@ -50,13 +53,15 @@
private final com.google.gerrit.server.git.BanCommit banCommit;
@Inject
- BanCommit(com.google.gerrit.server.git.BanCommit banCommit) {
+ BanCommit(RetryHelper retryHelper, com.google.gerrit.server.git.BanCommit banCommit) {
+ super(retryHelper);
this.banCommit = banCommit;
}
@Override
- public BanResultInfo apply(ProjectResource rsrc, Input input)
- throws UnprocessableEntityException, AuthException, ResourceConflictException, IOException {
+ protected BanResultInfo applyImpl(
+ BatchUpdate.Factory updateFactory, ProjectResource rsrc, Input input)
+ throws RestApiException, UpdateException, IOException {
BanResultInfo r = new BanResultInfo();
if (input != null && input.commits != null && !input.commits.isEmpty()) {
List<ObjectId> commitsToBan = new ArrayList<>(input.commits.size());
@@ -75,8 +80,6 @@
r.ignored = transformCommits(result.getIgnoredObjectIds());
} catch (PermissionDeniedException e) {
throw new AuthException(e.getMessage());
- } catch (ConcurrentRefUpdateException e) {
- throw new ResourceConflictException(e.getMessage(), e);
}
}
return r;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/schema/NoChangesReviewDbWrapper.java b/gerrit-server/src/main/java/com/google/gerrit/server/schema/NoChangesReviewDbWrapper.java
index af55b00..fd0c7fc 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/schema/NoChangesReviewDbWrapper.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/schema/NoChangesReviewDbWrapper.java
@@ -17,7 +17,6 @@
import static com.google.common.base.Preconditions.checkState;
import com.google.common.collect.ImmutableList;
-import com.google.common.util.concurrent.CheckedFuture;
import com.google.common.util.concurrent.Futures;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
@@ -51,7 +50,9 @@
return new ListResultSet<>(ImmutableList.of());
}
- private static <T, K extends Key<?>> CheckedFuture<T, OrmException> emptyFuture() {
+ @SuppressWarnings("deprecation")
+ private static <T, K extends Key<?>>
+ com.google.common.util.concurrent.CheckedFuture<T, OrmException> emptyFuture() {
return Futures.immediateCheckedFuture(null);
}
@@ -164,8 +165,9 @@
return empty();
}
+ @SuppressWarnings("deprecation")
@Override
- public final CheckedFuture<T, OrmException> getAsync(K key) {
+ public final com.google.common.util.concurrent.CheckedFuture<T, OrmException> getAsync(K key) {
return emptyFuture();
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/FusedNoteDbBatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/FusedNoteDbBatchUpdate.java
index f8ef5f9..b979636 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/update/FusedNoteDbBatchUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/FusedNoteDbBatchUpdate.java
@@ -21,7 +21,6 @@
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
-import com.google.common.util.concurrent.CheckedFuture;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Change;
@@ -81,7 +80,9 @@
setRequestIds(updates, requestId);
try {
- List<CheckedFuture<?, IOException>> indexFutures = new ArrayList<>();
+ @SuppressWarnings("deprecation")
+ List<com.google.common.util.concurrent.CheckedFuture<?, IOException>> indexFutures =
+ new ArrayList<>();
List<ChangesHandle> handles = new ArrayList<>(updates.size());
Order order = getOrder(updates, listener);
try {
@@ -357,12 +358,14 @@
FusedNoteDbBatchUpdate.this.batchRefUpdate = manager.execute(dryrun);
}
- List<CheckedFuture<?, IOException>> startIndexFutures() {
+ @SuppressWarnings("deprecation")
+ List<com.google.common.util.concurrent.CheckedFuture<?, IOException>> startIndexFutures() {
if (dryrun) {
return ImmutableList.of();
}
logDebug("Reindexing {} changes", results.size());
- List<CheckedFuture<?, IOException>> indexFutures = new ArrayList<>(results.size());
+ List<com.google.common.util.concurrent.CheckedFuture<?, IOException>> indexFutures =
+ new ArrayList<>(results.size());
for (Map.Entry<Change.Id, ChangeResult> e : results.entrySet()) {
Change.Id id = e.getKey();
switch (e.getValue()) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/RefUpdateUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/RefUpdateUtil.java
new file mode 100644
index 0000000..ab0b78e
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/RefUpdateUtil.java
@@ -0,0 +1,84 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.update;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.gerrit.server.git.LockFailureException;
+import java.io.IOException;
+import org.eclipse.jgit.internal.JGitText;
+import org.eclipse.jgit.lib.BatchRefUpdate;
+import org.eclipse.jgit.lib.NullProgressMonitor;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.transport.ReceiveCommand;
+
+/** Static utilities for working with JGit's ref update APIs. */
+public class RefUpdateUtil {
+ /**
+ * Execute a batch ref update, throwing a checked exception if not all updates succeeded.
+ *
+ * @param bru batch update; should already have been executed.
+ * @throws LockFailureException if the transaction was aborted due to lock failure; see {@link
+ * #checkResults(BatchRefUpdate)} for details.
+ * @throws IOException if any result was not {@code OK}.
+ */
+ public static void executeChecked(BatchRefUpdate bru, RevWalk rw) throws IOException {
+ bru.execute(rw, NullProgressMonitor.INSTANCE);
+ checkResults(bru);
+ }
+
+ /**
+ * Check results of all commands in the update batch, reducing to a single exception if there was
+ * a failure.
+ *
+ * <p>Throws {@link LockFailureException} if at least one command failed with {@code
+ * LOCK_FAILURE}, and the entire transaction was aborted, i.e. any non-{@code LOCK_FAILURE}
+ * results, if there were any, failed with "transaction aborted".
+ *
+ * <p>In particular, if the underlying ref database does not {@link
+ * org.eclipse.jgit.lib.RefDatabase#performsAtomicTransactions() perform atomic transactions},
+ * then a combination of {@code LOCK_FAILURE} on one ref and {@code OK} or another result on other
+ * refs will <em>not</em> throw {@code LockFailureException}.
+ *
+ * @param bru batch update; should already have been executed.
+ * @throws LockFailureException if the transaction was aborted due to lock failure.
+ * @throws IOException if any result was not {@code OK}.
+ */
+ @VisibleForTesting
+ static void checkResults(BatchRefUpdate bru) throws IOException {
+ int lockFailure = 0;
+ int aborted = 0;
+ int failure = 0;
+
+ for (ReceiveCommand cmd : bru.getCommands()) {
+ if (cmd.getResult() != ReceiveCommand.Result.OK) {
+ failure++;
+ }
+ if (cmd.getResult() == ReceiveCommand.Result.LOCK_FAILURE) {
+ lockFailure++;
+ } else if (cmd.getResult() == ReceiveCommand.Result.REJECTED_OTHER_REASON
+ && JGitText.get().transactionAborted.equals(cmd.getMessage())) {
+ aborted++;
+ }
+ }
+
+ if (lockFailure + aborted == bru.getCommands().size()) {
+ throw new LockFailureException("Update aborted with one or more lock failures: " + bru);
+ } else if (failure > 0) {
+ throw new IOException("Update failed: " + bru);
+ }
+ }
+
+ private RefUpdateUtil() {}
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java
index 3cc2f9e..1561fc4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java
@@ -23,7 +23,6 @@
import com.google.common.base.Stopwatch;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
-import com.google.common.util.concurrent.CheckedFuture;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.ListeningExecutorService;
@@ -321,7 +320,10 @@
private final ReviewDb db;
private final SchemaFactory<ReviewDb> schemaFactory;
private final long skewMs;
- private final List<CheckedFuture<?, IOException>> indexFutures = new ArrayList<>();
+
+ @SuppressWarnings("deprecation")
+ private final List<com.google.common.util.concurrent.CheckedFuture<?, IOException>> indexFutures =
+ new ArrayList<>();
@Inject
ReviewDbBatchUpdate(
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/UnfusedNoteDbBatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/UnfusedNoteDbBatchUpdate.java
index ab4b701..b5c7256 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/update/UnfusedNoteDbBatchUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/UnfusedNoteDbBatchUpdate.java
@@ -22,7 +22,6 @@
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Maps;
-import com.google.common.util.concurrent.CheckedFuture;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Change;
@@ -250,7 +249,8 @@
private final GitReferenceUpdated gitRefUpdated;
private final ReviewDb db;
- private List<CheckedFuture<?, IOException>> indexFutures;
+ @SuppressWarnings("deprecation")
+ private List<com.google.common.util.concurrent.CheckedFuture<?, IOException>> indexFutures;
@Inject
UnfusedNoteDbBatchUpdate(
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/util/CommitMessageUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/util/CommitMessageUtil.java
new file mode 100644
index 0000000..fa55597
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/util/CommitMessageUtil.java
@@ -0,0 +1,40 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.util;
+
+import com.google.common.base.Strings;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+
+/** Utility functions to manipulate commit messages. */
+public class CommitMessageUtil {
+
+ private CommitMessageUtil() {}
+
+ /**
+ * Checks for null or empty commit messages and appends a newline character to the commit message.
+ *
+ * @throws BadRequestException if the commit message is null or empty
+ * @returns the trimmed message with a trailing newline character
+ */
+ public static String checkAndSanitizeCommitMessage(String commitMessage)
+ throws BadRequestException {
+ String wellFormedMessage = Strings.nullToEmpty(commitMessage).trim();
+ if (wellFormedMessage.isEmpty()) {
+ throw new BadRequestException("Commit message cannot be null or empty");
+ }
+ wellFormedMessage = wellFormedMessage + "\n";
+ return wellFormedMessage;
+ }
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/util/RequestId.java b/gerrit-server/src/main/java/com/google/gerrit/server/util/RequestId.java
index dc7dd3d..8e8db12 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/util/RequestId.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/util/RequestId.java
@@ -47,7 +47,7 @@
private final String str;
private RequestId(String resourceId) {
- Hasher h = Hashing.sha1().newHasher();
+ Hasher h = Hashing.murmur3_128().newHasher();
h.putLong(Thread.currentThread().getId()).putUnencodedChars(MACHINE_ID);
str =
"["
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/NoteDbUpdateManagerTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/update/RefUpdateUtilTest.java
similarity index 91%
rename from gerrit-server/src/test/java/com/google/gerrit/server/notedb/NoteDbUpdateManagerTest.java
rename to gerrit-server/src/test/java/com/google/gerrit/server/update/RefUpdateUtilTest.java
index 12eb39d..286827a 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/NoteDbUpdateManagerTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/update/RefUpdateUtilTest.java
@@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-package com.google.gerrit.server.notedb;
+package com.google.gerrit.server.update;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.truth.Truth.assertThat;
@@ -32,9 +32,8 @@
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
-/** Unit tests for {@link NoteDbUpdateManager}. */
@RunWith(JUnit4.class)
-public class NoteDbUpdateManagerTest {
+public class RefUpdateUtilTest {
private static final Consumer<ReceiveCommand> OK = c -> c.setResult(ReceiveCommand.Result.OK);
private static final Consumer<ReceiveCommand> LOCK_FAILURE =
c -> c.setResult(ReceiveCommand.Result.LOCK_FAILURE);
@@ -76,13 +75,13 @@
@SafeVarargs
private static void checkResults(Consumer<ReceiveCommand>... resultSetters) throws Exception {
- NoteDbUpdateManager.checkResults(newBatchRefUpdate(resultSetters));
+ RefUpdateUtil.checkResults(newBatchRefUpdate(resultSetters));
}
@SafeVarargs
private static void assertIoException(Consumer<ReceiveCommand>... resultSetters) {
try {
- NoteDbUpdateManager.checkResults(newBatchRefUpdate(resultSetters));
+ RefUpdateUtil.checkResults(newBatchRefUpdate(resultSetters));
assert_().fail("expected IOException");
} catch (IOException e) {
assertThat(e).isNotInstanceOf(LockFailureException.class);
@@ -93,7 +92,7 @@
private static void assertLockFailureException(Consumer<ReceiveCommand>... resultSetters)
throws Exception {
try {
- NoteDbUpdateManager.checkResults(newBatchRefUpdate(resultSetters));
+ RefUpdateUtil.checkResults(newBatchRefUpdate(resultSetters));
assert_().fail("expected LockFailureException");
} catch (LockFailureException e) {
// Expected.
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/BanCommitCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/BanCommitCommand.java
index 78726a0..22d7e9a 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/BanCommitCommand.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/BanCommitCommand.java
@@ -18,7 +18,6 @@
import com.google.common.base.Joiner;
import com.google.common.collect.Lists;
-import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.project.BanCommit;
import com.google.gerrit.server.project.BanCommit.BanResultInfo;
import com.google.gerrit.server.project.ProjectControl;
@@ -26,7 +25,6 @@
import com.google.gerrit.sshd.CommandMetaData;
import com.google.gerrit.sshd.SshCommand;
import com.google.inject.Inject;
-import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import org.eclipse.jgit.lib.ObjectId;
@@ -77,7 +75,7 @@
printCommits(r.newlyBanned, "The following commits were banned");
printCommits(r.alreadyBanned, "The following commits were already banned");
printCommits(r.ignored, "The following ids do not represent commits and were ignored");
- } catch (RestApiException | IOException e) {
+ } catch (Exception e) {
throw die(e);
}
}
diff --git a/lib/guava.bzl b/lib/guava.bzl
index c71379e..768b99e 100644
--- a/lib/guava.bzl
+++ b/lib/guava.bzl
@@ -1,5 +1,5 @@
-GUAVA_VERSION = "21.0"
+GUAVA_VERSION = "22.0"
-GUAVA_BIN_SHA1 = "3a3d111be1be1b745edfa7d91678a12d7ed38709"
+GUAVA_BIN_SHA1 = "3564ef3803de51fb0530a8377ec6100b33b0d073"
GUAVA_DOC_URL = "https://google.github.io/guava/releases/" + GUAVA_VERSION + "/api/docs/"
diff --git a/plugins/reviewnotes b/plugins/reviewnotes
index 1a64a6f..f0930e9 160000
--- a/plugins/reviewnotes
+++ b/plugins/reviewnotes
@@ -1 +1 @@
-Subproject commit 1a64a6f6407df31c06350a5c6e9266e9ba0cf72a
+Subproject commit f0930e9dfc56644105cb21e7246096097eeaa385
