Merge branch 'stable-3.4'
* stable-3.4: (48 commits)
Update git submodules
Update git submodules
Disable setting cancelLeftPadding if avatars are not set in the config
Disable setting cancelLeftPadding if avatars are not set in the config
ProjectOperations: Add method to wipe all access sections
Migrate to python 3
Pop up accounts when typing within assignee or attention on the searchbar
Pop up accounts when typing within assignee or attention on the searchbar
Add attention operator for auto-complete
Update CodeMirror to 5.62.2
Update CodeMirror to 5.62.2
Remove contenteditable="false" from within dragDropArea
Fix drag and drop
Remove float from rightControls on mobiles
Remove contenteditable="false" from within dragDropArea
Replace non-standard event.path with event.composedPath
Suppress error when conflicts predicate is disabled
Add missing "Show trailing whitespace" to edit preference
Add missing "Show trailing whitespace" to edit preference
Prevent removing e-mail associated with OpenId accounts
...
Change-Id: I313022111b61622dfbac2ce2800faa21bac45992
diff --git a/Documentation/config-labels.txt b/Documentation/config-labels.txt
index b6184d7..9d3446e 100644
--- a/Documentation/config-labels.txt
+++ b/Documentation/config-labels.txt
@@ -293,9 +293,9 @@
patch-set is uploaded that has the same list of files as the previous
patch-set.
-Renames are considered the same file when computing whether new files
-were added or old files were deleted. Hence, if there are only renames,
-scores will still be copied over.
+Renames are considered different files when computing whether new files
+were added or old files were deleted. Hence, if there are renames, scores will
+*NOT* be copied over.
Defaults to false.
diff --git a/java/com/google/gerrit/server/ApprovalInference.java b/java/com/google/gerrit/server/ApprovalInference.java
index d77427a..04d874c 100644
--- a/java/com/google/gerrit/server/ApprovalInference.java
+++ b/java/com/google/gerrit/server/ApprovalInference.java
@@ -29,11 +29,12 @@
import com.google.gerrit.entities.Patch.ChangeType;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.client.ChangeKind;
-import com.google.gerrit.extensions.client.DiffPreferencesInfo;
import com.google.gerrit.server.change.ChangeKindCache;
import com.google.gerrit.server.change.LabelNormalizer;
+import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.TraceContext;
import com.google.gerrit.server.logging.TraceContext.TraceTimer;
@@ -46,10 +47,14 @@
import com.google.gerrit.server.project.ProjectState;
import com.google.inject.Inject;
import com.google.inject.Singleton;
+import java.io.IOException;
import java.util.Collection;
import java.util.Collections;
import java.util.Map;
+import java.util.stream.Collectors;
import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevWalk;
/**
@@ -68,17 +73,20 @@
private final ChangeKindCache changeKindCache;
private final LabelNormalizer labelNormalizer;
private final PatchListCache patchListCache;
+ private final GitRepositoryManager repositoryManager;
@Inject
ApprovalInference(
ProjectCache projectCache,
ChangeKindCache changeKindCache,
LabelNormalizer labelNormalizer,
- PatchListCache patchListCache) {
+ PatchListCache patchListCache,
+ GitRepositoryManager repositoryManager) {
this.projectCache = projectCache;
this.changeKindCache = changeKindCache;
this.labelNormalizer = labelNormalizer;
this.patchListCache = patchListCache;
+ this.repositoryManager = repositoryManager;
}
/**
@@ -111,7 +119,8 @@
PatchSet.Id psId,
ChangeKind kind,
LabelType type,
- @Nullable PatchList patchList) {
+ @Nullable PatchList patchListCurrentPatchset,
+ @Nullable PatchList patchListPriorPatchset) {
int n = psa.key().patchSetId().get();
checkArgument(n != psId.get());
@@ -172,11 +181,7 @@
project.getName());
return true;
} else if (type.isCopyAllScoresIfListOfFilesDidNotChange()
- && patchList.getPatches().stream()
- .noneMatch(
- p ->
- p.getChangeType() == ChangeType.ADDED
- || p.getChangeType() == ChangeType.DELETED)) {
+ && didListOfFilesNotChange(patchListCurrentPatchset, patchListPriorPatchset)) {
logger.atFine().log(
"approval %d on label %s of patch set %d of change %d can be copied"
+ " to patch set %d because the label has set "
@@ -308,6 +313,20 @@
}
}
+ private static boolean didListOfFilesNotChange(PatchList oldPatchList, PatchList newPatchList) {
+ Map<String, ChangeType> fileToChangeTypePs1 = getFileToChangeType(oldPatchList);
+ Map<String, ChangeType> fileToChangeTypePs2 = getFileToChangeType(newPatchList);
+ return fileToChangeTypePs1.equals(fileToChangeTypePs2);
+ }
+
+ private static Map<String, ChangeType> getFileToChangeType(PatchList ps) {
+ return ps.getPatches().stream()
+ .collect(
+ Collectors.toMap(
+ f -> f.getNewName() != null ? f.getNewName() : f.getOldName(),
+ f -> f.getChangeType()));
+ }
+
private Collection<PatchSetApproval> getForPatchSetWithoutNormalization(
ChangeNotes notes,
ProjectState project,
@@ -368,7 +387,8 @@
logger.atFine().log(
"change kind for patch set %d of change %d against prior patch set %s is %s",
ps.id().get(), ps.id().changeId().get(), priorPatchSet.getValue().id().changeId(), kind);
- PatchList patchList = null;
+ PatchList patchListCurrentPatchset = null;
+ PatchList patchListPriorPatchset = null;
LabelTypes labelTypes = project.getLabelTypes();
for (PatchSetApproval psa : priorApprovals) {
if (resultByUser.contains(psa.label(), psa.accountId())) {
@@ -376,10 +396,14 @@
}
LabelType type = labelTypes.byLabel(psa.labelId());
// Only compute patchList if there is a relevant label, since this is expensive.
- if (patchList == null && type != null && type.isCopyAllScoresIfListOfFilesDidNotChange()) {
- patchList = getPatchList(project, ps, priorPatchSet);
+ if (patchListCurrentPatchset == null
+ && type != null
+ && type.isCopyAllScoresIfListOfFilesDidNotChange()) {
+ patchListCurrentPatchset = getPatchList(project, ps);
+ patchListPriorPatchset = getPatchList(project, priorPatchSet.getValue());
}
- if (!canCopy(project, psa, ps.id(), kind, type, patchList)) {
+ if (!canCopy(
+ project, psa, ps.id(), kind, type, patchListCurrentPatchset, patchListPriorPatchset)) {
continue;
}
resultByUser.put(psa.label(), psa.accountId(), psa.copyWithPatchSet(ps.id()));
@@ -388,16 +412,18 @@
}
/**
- * Gets the {@link PatchList} between the two latest patch-sets. Can be used to compute difference
- * in files between those two patch-sets .
+ * Gets the {@link PatchList} between a patch-set and the base. Can be used to compute difference
+ * in files between two patch-sets by using both {@link PatchList}s of those 2 patch-sets.
*/
- private PatchList getPatchList(
- ProjectState project, PatchSet ps, Map.Entry<PatchSet.Id, PatchSet> priorPatchSet) {
+ private PatchList getPatchList(ProjectState project, PatchSet ps) {
+ // Compare against the base:
+ // * For merge commits the comparison is done against the 1st parent, which is the destination
+ // branch.
+ // * For non-merge commits the comparison is done against the only parent, or an empty base if
+ // no parent exists.
PatchListKey key =
- PatchListKey.againstCommit(
- priorPatchSet.getValue().commitId(),
- ps.commitId(),
- DiffPreferencesInfo.Whitespace.IGNORE_NONE);
+ PatchListKey.againstBase(
+ ps.commitId(), getParentCount(project.getNameKey(), ps.commitId()));
try {
return patchListCache.get(key, project.getNameKey());
} catch (PatchListNotAvailableException ex) {
@@ -408,4 +434,13 @@
ex);
}
}
+
+ private int getParentCount(Project.NameKey project, ObjectId objectId) {
+ try (Repository repo = repositoryManager.openRepository(project);
+ RevWalk revWalk = new RevWalk(repo)) {
+ return revWalk.parseCommit(objectId).getParentCount();
+ } catch (IOException ex) {
+ throw new StorageException(ex);
+ }
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java b/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
index 5c59129..4e47bb1 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
@@ -37,6 +37,7 @@
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
@@ -360,6 +361,46 @@
}
@Test
+ public void notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileAlreadyExists()
+ throws Exception {
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig()
+ .updateLabelType(
+ LabelId.CODE_REVIEW, b -> b.setCopyAllScoresIfListOfFilesDidNotChange(true));
+ u.save();
+ }
+
+ // create "existing file" and submit it.
+ String existingFile = "existing file";
+ Change.Id prep =
+ changeOperations
+ .newChange()
+ .project(project)
+ .file(existingFile)
+ .content("content")
+ .create();
+ vote(admin, prep.toString(), 2, 1);
+ gApi.changes().id(prep.get()).current().submit();
+
+ Change.Id changeId = changeOperations.newChange().project(project).create();
+ vote(admin, changeId.toString(), 2, 1);
+ vote(user, changeId.toString(), -2, -1);
+
+ changeOperations
+ .change(changeId)
+ .newPatchset()
+ .file(existingFile)
+ .content("new content")
+ .create();
+ ChangeInfo c = detailedChange(changeId.toString());
+
+ // no votes are copied since the list of files changed ("existing file" was added to the
+ // change).
+ assertVotes(c, admin, 0, 0);
+ assertVotes(c, user, 0, 0);
+ }
+
+ @Test
public void notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsDeleted()
throws Exception {
try (ProjectConfigUpdate u = updateProject(project)) {
@@ -405,7 +446,32 @@
}
@Test
- public void stickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsRenamed() throws Exception {
+ @TestProjectInput(createEmptyCommit = false)
+ public void stickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsModifiedAsInitialCommit()
+ throws Exception {
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig()
+ .updateLabelType(
+ LabelId.CODE_REVIEW, b -> b.setCopyAllScoresIfListOfFilesDidNotChange(true));
+ u.save();
+ }
+ Change.Id changeId =
+ changeOperations.newChange().project(project).file("file").content("content").create();
+ vote(admin, changeId.toString(), 2, 1);
+ vote(user, changeId.toString(), -2, -1);
+
+ changeOperations.change(changeId).newPatchset().file("file").content("new content").create();
+ ChangeInfo c = detailedChange(changeId.toString());
+
+ // only code review votes are copied since copyAllScoresIfListOfFilesDidNotChange is
+ // configured for that label, and list of files didn't change.
+ assertVotes(c, admin, 2, 0);
+ assertVotes(c, user, -2, 0);
+ }
+
+ @Test
+ public void notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsRenamed()
+ throws Exception {
try (ProjectConfigUpdate u = updateProject(project)) {
u.getConfig()
.updateLabelType(
@@ -420,10 +486,9 @@
changeOperations.change(changeId).newPatchset().file("file").renameTo("new_file").create();
ChangeInfo c = detailedChange(changeId.toString());
- // only code review votes are copied since copyAllScoresIfListOfFilesDidNotChange is
- // configured for that label, and list of files didn't change (rename is still the same file).
- assertVotes(c, admin, 2, 0);
- assertVotes(c, user, -2, 0);
+ // no votes are copied since the list of files changed (rename).
+ assertVotes(c, admin, 0, 0);
+ assertVotes(c, user, 0, 0);
}
@Test
