RenameEmail: Preserve files which are not updated
The RenameEmail REST endpoint allows to update emails in code owner
config files. The commit that was created when this REST endpoint was
invoked only contained the modified code owner config files and all
other files, non code owner config files and code owner config files
that didn't contain the email, were deleted. This was because we created
a new tree that we populated with the modifications instead of using the
existing tree as the base.
This being broken means that this REST endpoint hasn't been used so far.
Bug: Issue 16781
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Id396ff9c7dacc210d761baee855d3b2c51f64bac
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdateScanner.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdateScanner.java
index 6222b7d..dd5da50 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdateScanner.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigFileUpdateScanner.java
@@ -110,7 +110,7 @@
/** pathGlob */
null)) {
RevCommit revision = treeWalk.getRevision();
- DirCache newTree = DirCache.newInCore();
+ DirCache newTree = DirCache.read(rw.getObjectReader(), revision.getTree());
DirCacheEditor editor = newTree.editor();
boolean dirty = false;
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/RenameEmailIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/RenameEmailIT.java
index b9e7502..5878ce4 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/RenameEmailIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/RenameEmailIT.java
@@ -15,12 +15,14 @@
package com.google.gerrit.plugins.codeowners.acceptance.api;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
import static com.google.common.truth.TruthJUnit.assume;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability;
import static com.google.gerrit.plugins.codeowners.testing.CodeOwnerConfigSubject.assertThat;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.base.Splitter;
import com.google.common.collect.Iterables;
@@ -46,6 +48,14 @@
import com.google.gerrit.plugins.codeowners.restapi.RenameEmail;
import com.google.inject.Inject;
import java.util.Optional;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.ObjectLoader;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevTree;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.treewalk.TreeWalk;
import org.junit.Before;
import org.junit.Test;
@@ -676,7 +686,7 @@
}
@Test
- public void renameEmail_emailThatContainsEmailToBeReplacesAsSubstringStaysIntact()
+ public void renameEmail_emailThatContainsEmailToBeReplacedAsSubstringStaysIntact()
throws Exception {
// renaming email is not supported for the proto backend
assume().that(backendConfig.getDefaultBackend()).isNotInstanceOf(ProtoBackend.class);
@@ -728,6 +738,124 @@
secondaryEmail, otherUser1.email(), otherUser2.email(), otherUser3.email());
}
+ @Test
+ public void renameEmailDoesNotTouchCodeOwnerConfigsThatDoNotContainTheEmail() throws Exception {
+ // renaming email is not supported for the proto backend
+ assume().that(backendConfig.getDefaultBackend()).isNotInstanceOf(ProtoBackend.class);
+
+ TestAccount user2 = accountCreator.user2();
+
+ CodeOwnerConfig.Key codeOwnerConfigKey1 =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ // Create a code owner config that doesn't contain the email to be replaced.
+ CodeOwnerConfig.Key codeOwnerConfigKey2 =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(user2.email())
+ .create();
+
+ // grant all users direct push permissions
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.PUSH).ref("refs/*").group(REGISTERED_USERS))
+ .update();
+
+ String secondaryEmail = "user-foo@example.com";
+ accountOperations.account(user.id()).forUpdate().addSecondaryEmail(secondaryEmail).update();
+
+ requestScopeOperations.setApiUser(user.id());
+ RenameEmailInput input = new RenameEmailInput();
+ input.oldEmail = user.email();
+ input.newEmail = secondaryEmail;
+ RenameEmailResultInfo result = renameEmail(project, "master", input);
+ assertThat(result.commit).isNotNull();
+
+ assertThat(codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey1).get())
+ .hasCodeOwnerSetsThat()
+ .onlyElement()
+ .hasCodeOwnersEmailsThat()
+ .containsExactly(secondaryEmail, admin.email());
+
+ // Check that the second code owner config is still intact.
+ assertThat(codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey2).get())
+ .hasCodeOwnerSetsThat()
+ .onlyElement()
+ .hasCodeOwnersEmailsThat()
+ .containsExactly(user2.email());
+ }
+
+ @Test
+ public void renameEmailDoesNotTouchNonCodeOwnerConfigFiles() throws Exception {
+ // renaming email is not supported for the proto backend
+ assume().that(backendConfig.getDefaultBackend()).isNotInstanceOf(ProtoBackend.class);
+
+ CodeOwnerConfig.Key codeOwnerConfigKey =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .addCodeOwnerEmail(user.email())
+ .create();
+
+ // Create non code owner config files.
+ String contentFileA = "some content";
+ String contentFileB =
+ String.format(
+ "some content that contains the email %s that is being renamed", user.email());
+ try (TestRepository<Repository> testRepo =
+ new TestRepository<>(repoManager.openRepository(project))) {
+ testRepo.update(
+ "master",
+ testRepo
+ .commit()
+ .message("Update project.config from test")
+ .parent(projectOperations.project(project).getHead("master"))
+ .add("A", contentFileA)
+ .add("B", contentFileB));
+ }
+
+ // grant all users direct push permissions
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.PUSH).ref("refs/*").group(REGISTERED_USERS))
+ .update();
+
+ String secondaryEmail = "user-foo@example.com";
+ accountOperations.account(user.id()).forUpdate().addSecondaryEmail(secondaryEmail).update();
+
+ requestScopeOperations.setApiUser(user.id());
+ RenameEmailInput input = new RenameEmailInput();
+ input.oldEmail = user.email();
+ input.newEmail = secondaryEmail;
+ RenameEmailResultInfo result = renameEmail(project, "master", input);
+ assertThat(result.commit).isNotNull();
+
+ assertThat(codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).get())
+ .hasCodeOwnerSetsThat()
+ .onlyElement()
+ .hasCodeOwnersEmailsThat()
+ .containsExactly(secondaryEmail, admin.email());
+
+ // Check that the non code owner config files are still intact.
+ assertThat(getFileContent(project, "master", "A")).hasValue(contentFileA);
+ assertThat(getFileContent(project, "master", "B")).hasValue(contentFileB);
+ }
+
private RenameEmailResultInfo renameEmail(
Project.NameKey projectName, String branchName, RenameEmailInput input)
throws RestApiException {
@@ -736,4 +864,27 @@
.branch(branchName)
.renameEmailInCodeOwnerConfigFiles(input);
}
+
+ private Optional<String> getFileContent(Project.NameKey project, String branch, String fileName) {
+ try (Repository repo = repoManager.openRepository(project);
+ RevWalk rw = new RevWalk(repo)) {
+ if (!branch.startsWith(Constants.R_REFS)) {
+ branch = Constants.R_HEADS + branch;
+ }
+ Ref ref = repo.exactRef(branch);
+ if (ref == null) {
+ return Optional.empty();
+ }
+ RevTree tree = rw.parseTree(ref.getObjectId());
+ TreeWalk tw = TreeWalk.forPath(rw.getObjectReader(), fileName, tree);
+ if (tw == null) {
+ return Optional.empty();
+ }
+ ObjectLoader loader = rw.getObjectReader().open(tw.getObjectId(0));
+ String fileContent = new String(loader.getCachedBytes(), UTF_8);
+ return Optional.of(fileContent);
+ } catch (Exception e) {
+ throw new IllegalStateException(e);
+ }
+ }
}
