Preserve owner approvals for rebase-only file changes
The owners plugin currently removes approvals guarded by
approverin:already-approved-by_owners whenever the newest patch set
modifies files owned by the approver.
This also happens when the apparent modifications are caused solely by a
rebase, where file content shifts due to base movement rather than
manual edits by the uploader. In these cases, the owner has already
reviewed the effective change, yet is forced to re-approve
unnecessarily.
Extend the already-approved-by_owners predicate so that owned files
modified only by rebase operations are treated as unchanged. Rebase-only
diff hunks, no longer cause owner approvals to be cleared.
This preserves owner approvals across rebase-only patch sets while still
clearing them when genuine content changes are introduced.
Bug: Issue 475579558
Change-Id: Icf649f1bcbc833a92853f0bf5c2c59ae5f245590
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/AlreadyApprovedByPredicate.java b/owners/src/main/java/com/googlesource/gerrit/owners/AlreadyApprovedByPredicate.java
index f7ee5b0..bf4aede 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/AlreadyApprovedByPredicate.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/AlreadyApprovedByPredicate.java
@@ -15,6 +15,7 @@
package com.googlesource.gerrit.owners;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.flogger.LazyArgs.lazy;
import static com.googlesource.gerrit.owners.AlreadyApprovedByOperand.FULL_OPERAND_WITH_PLUGIN_NAME;
import static com.googlesource.gerrit.owners.AlreadyApprovedByOperand.OPERAND;
@@ -28,6 +29,8 @@
import com.google.gerrit.server.git.InMemoryInserter;
import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.DiffOperations;
+import com.google.gerrit.server.patch.DiffOptions;
+import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import com.google.gerrit.server.patch.gitdiff.ModifiedFile;
import com.google.gerrit.server.query.approval.ApprovalContext;
import com.google.gerrit.server.query.approval.UserInPredicate;
@@ -35,6 +38,10 @@
import com.googlesource.gerrit.owners.restapi.GetFilesOwners;
import java.io.IOException;
import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -48,6 +55,8 @@
private final UserInPredicate.Field predicateField;
private static final boolean DISABLE_RENAME_DETECTION = false;
+ private static final DiffOptions DO_NOT_IGNORE_REBASE =
+ DiffOptions.builder().skipFilesWithAllEditsDueToRebase(false).build();
public AlreadyApprovedByPredicate(
GetFilesOwners getFilesOwners,
@@ -81,27 +90,32 @@
ctx.sourcePatchSetId(),
project);
- Map<String, ModifiedFile> priorVsCurrent =
- diffOperations.loadModifiedFilesIfNecessary(
- project,
- sourcePatchSet.commitId(),
- targetPatchSet.commitId(),
- ctx.repoView().getRevWalk(),
- ctx.repoView().getConfig(),
- DISABLE_RENAME_DETECTION);
+ Map<String, FileDiffOutput> priorVsCurrent =
+ diffOperations.listModifiedFiles(
+ project, sourcePatchSet.commitId(), targetPatchSet.commitId(), DO_NOT_IGNORE_REBASE);
- boolean newPatchSetHasFilesOwnedByMe =
- getFilesOwners.isAnyFileOwnedBy(
+ // We can't simply look at keys because it won't contain the old name of renamed-files.
+ Set<String> allFilePathsInDiff =
+ priorVsCurrent.values().stream()
+ .flatMap(v -> Stream.of(v.newPath(), v.oldPath()))
+ .filter(Optional::isPresent)
+ .map(Optional::get)
+ .collect(Collectors.toSet());
+
+ Set<String> filesOwnedByApprover =
+ getFilesOwners.filterFilesOwnedBy(
currentApprover,
- priorVsCurrent.keySet(),
+ allFilePathsInDiff,
project,
ctx.changeData().branchOrThrow().branch());
- if (newPatchSetHasFilesOwnedByMe) {
+ if (!filesOwnedByApprover.isEmpty()) {
logger.atFinest().log(
- "Approver '%s' owns files that were changed in this new patch set, must re-approve",
- currentApprover);
- return false;
+ "Approver '%s' owns files that were changed in this new patch set: %s",
+ currentApprover, lazy(() -> String.join(",", filesOwnedByApprover)));
+
+ return shouldCopyLabelForOwnedFiles(
+ priorVsCurrent.values(), filesOwnedByApprover, currentApprover);
}
// The new patchSet has not modified anything I own.
@@ -141,6 +155,49 @@
}
}
+ private static boolean shouldCopyLabelForOwnedFiles(
+ Iterable<FileDiffOutput> diffs, Set<String> ownedPaths, Account.Id currentApprover) {
+
+ for (FileDiffOutput diff : diffs) {
+ if (!touchesOwnedPath(diff, ownedPaths)) {
+ continue;
+ }
+
+ if (isPathChange(diff)) {
+ logger.atFinest().log(
+ "File owned by approver %s has a path change (rename/add/delete): oldPath=%s,"
+ + " newPath=%s",
+ currentApprover, diff.oldPath().orElse("<none>"), diff.newPath().orElse("<none>"));
+ return false;
+ }
+
+ // Content changes are acceptable only if they are entirely due to rebase.
+ if (!diff.allEditsDueToRebase()) {
+ logger.atFinest().log(
+ "file owned by approver %s has content edits (that are not only due to rebase):"
+ + " path=%s",
+ currentApprover, diff.newPath().orElseGet(() -> diff.oldPath().orElse("<unknown>")));
+ return false;
+ }
+ }
+
+ logger.atFinest().log(
+ "All edits for file owned by approver %s are either rebase-only or no path changes;"
+ + " approval can be copied.",
+ currentApprover);
+ return true;
+ }
+
+ private static boolean touchesOwnedPath(FileDiffOutput d, Set<String> ownedPaths) {
+ return d.newPath().filter(ownedPaths::contains).isPresent()
+ || d.oldPath().filter(ownedPaths::contains).isPresent();
+ }
+
+ private static boolean isPathChange(FileDiffOutput d) {
+ // A path change means rename/add/delete: oldPath != newPath, including empty vs present.
+ return !d.oldPath().equals(d.newPath());
+ }
+
private int getParentNum(ObjectId objectId, RevWalk revWalk) {
try {
RevCommit commit = revWalk.parseCommit(objectId);
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java b/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
index d13e709..c395179 100644
--- a/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
+++ b/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
@@ -94,11 +94,18 @@
public boolean isAnyFileOwnedBy(
Account.Id owner, Set<String> changePaths, Project.NameKey project, String branch)
throws IOException, InvalidOwnersFileException {
+ return !filterFilesOwnedBy(owner, changePaths, project, branch).isEmpty();
+ }
+
+ public Set<String> filterFilesOwnedBy(
+ Account.Id owner, Set<String> changePaths, Project.NameKey project, String branch)
+ throws IOException, InvalidOwnersFileException {
PathOwners owners = getPathOwners(project, branch, changePaths);
Map<String, Set<Account.Id>> filesWithOwner = owners.getFileOwners();
return changePaths.stream()
- .anyMatch(filePath -> filesWithOwner.getOrDefault(filePath, Set.of()).contains(owner));
+ .filter(filePath -> filesWithOwner.getOrDefault(filePath, Set.of()).contains(owner))
+ .collect(Collectors.toSet());
}
@Override
diff --git a/owners/src/main/resources/Documentation/copy-conditions.md b/owners/src/main/resources/Documentation/copy-conditions.md
index de970f1..b7f617b 100644
--- a/owners/src/main/resources/Documentation/copy-conditions.md
+++ b/owners/src/main/resources/Documentation/copy-conditions.md
@@ -22,10 +22,12 @@
returns true only when:
1. The approval was previously given by a user who owns at least one file in the change; and
-2. The newly uploaded patch set does not modify any files owned by that user.
+2. The newly uploaded patch set does not modify any files owned by that user (edits due to
+ rebase-only are ignored).
If a patch set updates files that the approver owns, the approval is cleared and the owner is
-expected to review the new changes.
+expected to review the new changes. However, if the modifications are identified as rebase edits,
+they are treated as unchanged, and the approval is preserved.
**NOTE**: this operand is only supported for `approverin:` predicates. Using it with an `uploaderin`
predicate will result in the label **not** being copied (and an error emitted in the logs).
@@ -54,6 +56,44 @@
**Result**: The `Code-Review +2` is copied forward.
+### Approval copied (edits due to rebase-only)
+
+* A `user-frontend` owner gives `Code-Review +2` on `Patch Set 1`.
+* The uploader rebases the change on a different parent (e.g. a new `master` tip) that also brings
+ changes to `.js` files.
+* `Patch Set 2` is created. `.js` files in `Patch Set 2` are different from `Patch Set 1` (they now
+ include the changes from the new parent).
+
+**Result**: The `Code-Review +2` is copied forward.
+
+This is because the `approverin:already-approved-by_owners` predicate ignores changes
+that are marked as `due_to_rebase`. Even though the owned files in `Patch Set 2` are
+different from `Patch Set 1` (due to the new parent), the content modifications are
+determined to be purely from the rebase, not from new edits by the uploader.
+
+### Approval copied (edits due to rebase-only + unrelated file modification)
+
+* A `user-backend` owner gives `Code-Review +2` on `Patch Set 1`.
+* The uploader rebases the change as in the previous example (modifying `backend.java` due to
+ rebase).
+* The uploader *also* includes a change to a file NOT owned by `user-backend`, in the same new Patch
+ Set.
+
+**Result**: The `Code-Review +2` is copied forward.
+
+The modification to `backend.java` is ignored because it is exclusively "due to rebase".
+The modification to `unrelated.txt` is ignored because `user-backend` does not own it.
+
+### Note on conflict resolutions during rebase
+
+When a rebase encounters conflicts, the resulting patch set necessarily
+introduces new content edits by the uploader while resolving those
+conflicts. Such changes are treated as genuine modifications, even if
+they conceptually originate from upstream changes.
+
+As a result, approvals guarded by `approverin:already-approved-by_owners`
+are **not copied** across rebases that require conflict resolution.
+
### Approval not copied
* A `user-backend` owner gives `Code-Review +2` on `Patch Set 3`.
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/AlreadyApprovedByCopyConditionIT.java b/owners/src/test/java/com/googlesource/gerrit/owners/AlreadyApprovedByCopyConditionIT.java
index 73d2d3c..d9aa7c1 100644
--- a/owners/src/test/java/com/googlesource/gerrit/owners/AlreadyApprovedByCopyConditionIT.java
+++ b/owners/src/test/java/com/googlesource/gerrit/owners/AlreadyApprovedByCopyConditionIT.java
@@ -16,6 +16,7 @@
package com.googlesource.gerrit.owners;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_COMMIT;
import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVISION;
@@ -25,6 +26,7 @@
import static java.util.stream.Collectors.joining;
import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestPlugin;
import com.google.gerrit.acceptance.UseLocalDisk;
@@ -34,15 +36,20 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.LabelId;
import com.google.gerrit.entities.LabelType;
+import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.extensions.api.changes.RebaseInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.common.ApprovalInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.RevisionInfo;
import com.google.gerrit.server.project.testing.TestLabels;
import com.google.inject.Inject;
import java.util.List;
import java.util.Map;
import java.util.function.Consumer;
+import java.util.stream.IntStream;
+import org.eclipse.jgit.lib.ObjectId;
import org.junit.Before;
import org.junit.Test;
@@ -60,6 +67,11 @@
private static final String BACKEND_OWNED_FILE = "foo.java";
private static final String FILE_WITH_NO_OWNERS = "foo.txt";
+ private static final String FILE_CONTENT =
+ IntStream.rangeClosed(1, 10)
+ .mapToObj(number -> String.format("Line %d\n", number))
+ .collect(joining());
+
@Before
public void setup() throws Exception {
projectOperations
@@ -70,6 +82,7 @@
.ref(RefNames.REFS_HEADS + "*")
.group(REGISTERED_USERS)
.range(-2, 2))
+ .add(allow(Permission.REBASE).ref("refs/*").group(REGISTERED_USERS))
.update();
FRONTEND_FILES_OWNER = accountCreator.create("user-frontend");
@@ -223,6 +236,196 @@
assertVotes(c, BACKEND_FILES_OWNER, 0);
}
+ @Test
+ public void shouldCopyApprovalWhenAllEditsToOwnedFileAreDueToRebase() throws Exception {
+ ObjectId initialCommitId = createInitialContentFor(BACKEND_OWNED_FILE);
+ PushOneCommit.Result amendL3 =
+ createChangeWithReplacedContent(BACKEND_OWNED_FILE, "Line 3\n", "Line three\n");
+ vote(BACKEND_FILES_OWNER, amendL3.getChangeId(), 2);
+
+ testRepo.reset(initialCommitId);
+ PushOneCommit.Result amendL7 =
+ createChangeWithReplacedContent(BACKEND_OWNED_FILE, "Line 7\n", "Line seven\n");
+
+ rebaseChangeOn(amendL3.getChangeId(), amendL7.getCommit().getId());
+
+ ChangeInfo c = detailedChange(amendL3.getChangeId());
+ assertVotes(c, BACKEND_FILES_OWNER, 2);
+ }
+
+ @Test
+ public void
+ shouldCopyApprovalWhenAllEditsToOwnedFileAreDueToRebaseEvenIfAnUnrelatedFileWasAddedByTheNewBase()
+ throws Exception {
+ ObjectId initialCommitId = createInitialContentFor(FRONTEND_OWNED_FILE);
+ PushOneCommit.Result amendL3 =
+ createChangeWithReplacedContent(FRONTEND_OWNED_FILE, "Line 3\n", "Line three\n");
+ vote(FRONTEND_FILES_OWNER, amendL3.getChangeId(), 2);
+
+ testRepo.reset(initialCommitId);
+ Change.Id baseChangeWithUnrelatedFiles =
+ changeOperations
+ .newChange()
+ .project(project)
+ .file(FRONTEND_OWNED_FILE)
+ .content(FILE_CONTENT.replace("Line 7\n", "Line seven\n"))
+ .file("unrelated.txt")
+ .content("unrelated change")
+ .create();
+
+ rebaseChangeOn(amendL3.getChangeId(), commitOf(baseChangeWithUnrelatedFiles));
+
+ vote(FRONTEND_FILES_OWNER, amendL3.getChangeId(), 2);
+ }
+
+ @Test
+ public void
+ shouldCopyApprovalWhenAllEditsToOwnedFileAreDueToRebaseEvenIfAnUnrelatedFileWasAddedByTheRebasedChange()
+ throws Exception {
+ ObjectId initialCommitId = createInitialContentFor(BACKEND_OWNED_FILE);
+ PushOneCommit.Result amendL3 =
+ createChangeWithReplacedContent(BACKEND_OWNED_FILE, "Line 3\n", "Line three\n");
+ vote(BACKEND_FILES_OWNER, amendL3.getChangeId(), 2);
+
+ testRepo.reset(initialCommitId);
+ PushOneCommit.Result amendL7 =
+ createChangeWithReplacedContent(BACKEND_OWNED_FILE, "Line 7\n", "Line seven\n");
+
+ // Rebase and include an unrelated file
+ testRepo.reset(amendL7.getCommit().getId());
+ testRepo.cherryPick(amendL3.getCommit());
+ PushOneCommit.Result rebasedL30WithUnrelatedChanges =
+ amendChange(
+ amendL3.getChangeId(), "Rebased with changes", "unrelated.txt", "Unrelated change");
+ rebasedL30WithUnrelatedChanges.assertOkStatus();
+
+ ChangeInfo c = detailedChange(amendL3.getChangeId());
+ assertVotes(c, BACKEND_FILES_OWNER, 2);
+ }
+
+ @Test
+ public void
+ shouldNotCopyApprovalWhenAllEditsToOwnedFileAreDueToRebaseButAnotherOwnedFileWasAdded()
+ throws Exception {
+ ObjectId initialCommitId = createInitialContentFor(FRONTEND_OWNED_FILE);
+ PushOneCommit.Result amendL3 =
+ createChangeWithReplacedContent(FRONTEND_OWNED_FILE, "Line 3\n", "Line three\n");
+ vote(FRONTEND_FILES_OWNER, amendL3.getChangeId(), 2);
+
+ testRepo.reset(initialCommitId);
+ PushOneCommit.Result amendL7 =
+ createChangeWithReplacedContent(FRONTEND_OWNED_FILE, "Line 7\n", "Line seven\n");
+
+ testRepo.reset(amendL7.getCommit().getId());
+ testRepo.cherryPick(amendL3.getCommit());
+ PushOneCommit.Result rebasedL30PlusOtherChangesToOwnedFile =
+ amendChange(
+ amendL3.getChangeId(),
+ "Rebased + Add another owned file",
+ "another-owned.js",
+ "Line 1\n");
+ rebasedL30PlusOtherChangesToOwnedFile.assertOkStatus();
+
+ ChangeInfo c = detailedChange(amendL3.getChangeId());
+ assertVotes(c, FRONTEND_FILES_OWNER, 0);
+ }
+
+ @Test
+ public void shouldNotCopyApprovalWhenRebasingButFurtherAmending() throws Exception {
+ ObjectId initialCommitId = createInitialContentFor(BACKEND_OWNED_FILE);
+ PushOneCommit.Result amendL3 =
+ createChangeWithReplacedContent(BACKEND_OWNED_FILE, "Line 3\n", "Line three\n");
+ vote(BACKEND_FILES_OWNER, amendL3.getChangeId(), 2);
+
+ testRepo.reset(initialCommitId);
+ PushOneCommit.Result amendL7 =
+ createChangeWithReplacedContent(BACKEND_OWNED_FILE, "Line 7\n", "Line seven\n");
+
+ // Rebase and include an unrelated file
+ testRepo.reset(amendL7.getCommit().getId());
+ testRepo.cherryPick(amendL3.getCommit());
+ PushOneCommit.Result rebasedL3WithUnrelatedChanges =
+ amendChange(
+ amendL3.getChangeId(),
+ "Rebased with additional change to Line 5",
+ BACKEND_OWNED_FILE,
+ FILE_CONTENT.replace("Line 5\n", "Line five\n"));
+ rebasedL3WithUnrelatedChanges.assertOkStatus();
+
+ ChangeInfo c = detailedChange(amendL3.getChangeId());
+ assertVotes(c, BACKEND_FILES_OWNER, 0);
+ }
+
+ @Test
+ public void shouldCopyApprovalWhenInheritingARemovalOfAnOwnedFileFromRebase() throws Exception {
+ createInitialContentFor(BACKEND_OWNED_FILE);
+ ObjectId latestMasterCommitId = createInitialContentFor("another-owned.java");
+
+ Change.Id removedFileChangeId =
+ changeOperations.newChange().project(project).file("another-owned.java").delete().create();
+
+ vote(BACKEND_FILES_OWNER, removedFileChangeId.toString(), 2);
+
+ testRepo.reset(latestMasterCommitId);
+ Change.Id baseRemovedFileChangeId =
+ changeOperations.newChange().project(project).file(BACKEND_OWNED_FILE).delete().create();
+
+ rebaseChangeOn(removedFileChangeId.toString(), commitOf(baseRemovedFileChangeId));
+
+ ChangeInfo c = detailedChange(removedFileChangeId.toString());
+ assertVotes(c, BACKEND_FILES_OWNER, 2);
+ }
+
+ @Test
+ public void shouldCopyApprovalWhenRemovingOwnedFileAndRebaseOnChangeThatRemovesTheSameFile()
+ throws Exception {
+ ObjectId initialCommitId = createInitialContentFor(BACKEND_OWNED_FILE);
+ Change.Id removedFileChangeId =
+ changeOperations.newChange().project(project).file(BACKEND_OWNED_FILE).delete().create();
+
+ vote(BACKEND_FILES_OWNER, removedFileChangeId.toString(), 2);
+
+ testRepo.reset(initialCommitId);
+ Change.Id baseRemovedFileChangeId =
+ changeOperations.newChange().project(project).file(BACKEND_OWNED_FILE).delete().create();
+
+ rebaseChangeOn(removedFileChangeId.toString(), commitOf(baseRemovedFileChangeId));
+
+ ChangeInfo c = detailedChange(removedFileChangeId.toString());
+ assertVotes(c, BACKEND_FILES_OWNER, 2);
+ }
+
+ private PushOneCommit.Result createChangeWithReplacedContent(
+ String file, String oldLine, String replacement) throws Exception {
+ PushOneCommit.Result r =
+ createChange(
+ String.format("Replaced '%s' with '%s'", oldLine, replacement),
+ file,
+ FILE_CONTENT.replace(oldLine, replacement));
+ r.assertOkStatus();
+ return r;
+ }
+
+ private ObjectId createInitialContentFor(String fileName) throws Exception {
+ PushOneCommit.Result initial =
+ createCommitAndPush(
+ testRepo, "refs/heads/master", "Create base file", fileName, FILE_CONTENT);
+ initial.assertOkStatus();
+ return initial.getCommit().getId();
+ }
+
+ private ObjectId commitOf(Change.Id changeId) throws Exception {
+ RevisionInfo currentRevision =
+ gApi.changes().id(project.get(), changeId.get()).get().getCurrentRevision();
+ return ObjectId.fromString(currentRevision.commit.commit);
+ }
+
+ private void rebaseChangeOn(String changeId, ObjectId newParent) throws Exception {
+ RebaseInput rebaseInput = new RebaseInput();
+ rebaseInput.base = newParent.getName();
+ gApi.changes().id(changeId).current().rebase(rebaseInput);
+ }
+
private ChangeInfo detailedChange(String changeId) throws Exception {
return gApi.changes().id(changeId).get(DETAILED_LABELS, CURRENT_REVISION, CURRENT_COMMIT);
}
