Merge "Introduce "header-feedback" plugin endpoint"
diff --git a/Documentation/rest-api-projects.txt b/Documentation/rest-api-projects.txt
index eb38434..92c6dbf 100644
--- a/Documentation/rest-api-projects.txt
+++ b/Documentation/rest-api-projects.txt
@@ -2561,6 +2561,9 @@
Retrieves the branches and tags in which a change is included. As result
an link:rest-api-changes.html#included-in-info[IncludedInInfo] entity is returned.
+Branches that are not visible to the calling user according to the project's
+read permissions are filtered out from the result.
+
.Request
----
GET /projects/work%2Fmy-project/commits/a8a477efffbbf3b44169bb9a1d3a334cbbd9aa96/in HTTP/1.0
diff --git a/Documentation/user-search.txt b/Documentation/user-search.txt
index a9779b1..006cd71 100644
--- a/Documentation/user-search.txt
+++ b/Documentation/user-search.txt
@@ -588,14 +588,17 @@
author:'AUTHOR'::
+
Changes where 'AUTHOR' is the author of the current patch set. 'AUTHOR' may be
-the author's exact email address, or part of the name or email address.
+the author's exact email address, or part of the name or email address. The
+special case of `author:self` will find changes authored by the caller.
[[committer]]
committer:'COMMITTER'::
+
Changes where 'COMMITTER' is the committer of the current patch set.
'COMMITTER' may be the committer's exact email address, or part of the name or
-email address.
+email address. The special case of `committer:self` will find changes committed
+by the caller.
+
[[submittable]]
submittable:'SUBMIT_STATUS'::
diff --git a/java/com/google/gerrit/server/approval/ApprovalInference.java b/java/com/google/gerrit/server/approval/ApprovalInference.java
index 8d409e5..dfc5bdb 100644
--- a/java/com/google/gerrit/server/approval/ApprovalInference.java
+++ b/java/com/google/gerrit/server/approval/ApprovalInference.java
@@ -125,7 +125,8 @@
PatchSet.Id psId,
ChangeKind kind,
LabelType type,
- @Nullable Map<String, FileDiffOutput> modifiedFiles) {
+ @Nullable Map<String, FileDiffOutput> modifiedFiles,
+ @Nullable Map<String, FileDiffOutput> modifiedFilesLastPatchset) {
int n = psa.key().patchSetId().get();
checkArgument(n != psId.get());
@@ -175,7 +176,7 @@
project.getName());
return true;
} else if (type.isCopyAllScoresIfListOfFilesDidNotChange()
- && listOfFilesUnchangedPredicate.match(modifiedFiles)) {
+ && listOfFilesUnchangedPredicate.match(modifiedFiles, modifiedFilesLastPatchset)) {
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 "
@@ -393,6 +394,7 @@
"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);
Map<String, FileDiffOutput> modifiedFiles = null;
+ Map<String, FileDiffOutput> modifiedFilesLastPatchSet = null;
LabelTypes labelTypes = project.getLabelTypes();
for (PatchSetApproval psa : priorApprovals) {
if (resultByUser.contains(psa.label(), psa.accountId())) {
@@ -403,7 +405,8 @@
if (modifiedFiles == null
&& type.isPresent()
&& type.get().isCopyAllScoresIfListOfFilesDidNotChange()) {
- modifiedFiles = listModifiedFiles(project, ps, priorPatchSet);
+ modifiedFiles = listModifiedFiles(project, ps);
+ modifiedFilesLastPatchSet = listModifiedFiles(project, priorPatchSet.getValue());
}
if (!type.isPresent()) {
logger.atFine().log(
@@ -417,7 +420,8 @@
project.getName());
continue;
}
- if (!canCopyBasedOnBooleanLabelConfigs(project, psa, ps.id(), kind, type.get(), modifiedFiles)
+ if (!canCopyBasedOnBooleanLabelConfigs(
+ project, psa, ps.id(), kind, type.get(), modifiedFiles, modifiedFilesLastPatchSet)
&& !canCopyBasedOnCopyCondition(notes, psa, ps.id(), type.get(), kind)) {
continue;
}
@@ -430,11 +434,14 @@
* Gets the modified files between the two latest patch-sets. Can be used to compute difference in
* files between those two patch-sets .
*/
- private Map<String, FileDiffOutput> listModifiedFiles(
- ProjectState project, PatchSet ps, Map.Entry<PatchSet.Id, PatchSet> priorPatchSet) {
+ private Map<String, FileDiffOutput> listModifiedFiles(ProjectState project, PatchSet ps) {
try {
- return diffOperations.listModifiedFiles(
- project.getNameKey(), priorPatchSet.getValue().commitId(), ps.commitId());
+ Integer parentNum =
+ listOfFilesUnchangedPredicate.isInitialCommit(project.getNameKey(), ps.commitId())
+ ? 0
+ : 1;
+ return diffOperations.listModifiedFilesAgainstParent(
+ project.getNameKey(), ps.commitId(), parentNum);
} catch (DiffNotAvailableException ex) {
throw new StorageException(
"failed to compute difference in files, so won't copy"
diff --git a/java/com/google/gerrit/server/change/IncludedIn.java b/java/com/google/gerrit/server/change/IncludedIn.java
index 3c66c2c..c06ce82 100644
--- a/java/com/google/gerrit/server/change/IncludedIn.java
+++ b/java/com/google/gerrit/server/change/IncludedIn.java
@@ -14,6 +14,12 @@
package com.google.gerrit.server.change;
+import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.collect.ImmutableSortedSet.toImmutableSortedSet;
+import static java.util.Comparator.naturalOrder;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSortedSet;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.MultimapBuilder;
import com.google.gerrit.entities.Project;
@@ -23,13 +29,18 @@
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackend.RefFilterOptions;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.plugincontext.PluginSetContext;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
+import java.util.Collection;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
@@ -37,17 +48,21 @@
@Singleton
public class IncludedIn {
private final GitRepositoryManager repoManager;
+ private final PermissionBackend permissionBackend;
private final PluginSetContext<ExternalIncludedIn> externalIncludedIn;
@Inject
IncludedIn(
- GitRepositoryManager repoManager, PluginSetContext<ExternalIncludedIn> externalIncludedIn) {
+ GitRepositoryManager repoManager,
+ PermissionBackend permissionBackend,
+ PluginSetContext<ExternalIncludedIn> externalIncludedIn) {
this.repoManager = repoManager;
+ this.permissionBackend = permissionBackend;
this.externalIncludedIn = externalIncludedIn;
}
public IncludedInInfo apply(Project.NameKey project, String revisionId)
- throws RestApiException, IOException {
+ throws RestApiException, IOException, PermissionBackendException {
try (Repository r = repoManager.openRepository(project);
RevWalk rw = new RevWalk(r)) {
rw.setRetainBody(false);
@@ -61,18 +76,48 @@
}
IncludedInResolver.Result d = IncludedInResolver.resolve(r, rw, rev);
+
+ // Filter branches and tags according to their visbility by the user
+ ImmutableSortedSet<String> filteredBranches =
+ sortedShortNames(filterReadableRefs(project, d.branches()));
+ ImmutableSortedSet<String> filteredTags =
+ sortedShortNames(filterReadableRefs(project, d.tags()));
+
ListMultimap<String, String> external = MultimapBuilder.hashKeys().arrayListValues().build();
externalIncludedIn.runEach(
ext -> {
ListMultimap<String, String> extIncludedIns =
- ext.getIncludedIn(project.get(), rev.name(), d.tags(), d.branches());
+ ext.getIncludedIn(project.get(), rev.name(), filteredBranches, filteredTags);
if (extIncludedIns != null) {
external.putAll(extIncludedIns);
}
});
return new IncludedInInfo(
- d.branches(), d.tags(), (!external.isEmpty() ? external.asMap() : null));
+ filteredBranches, filteredTags, (!external.isEmpty() ? external.asMap() : null));
}
}
+
+ /**
+ * Filter readable branches or tags according to the caller's refs visibility.
+ *
+ * @param project specific Gerrit project.
+ * @param inputRefs a list of branches (in short name) as strings
+ */
+ private Collection<String> filterReadableRefs(
+ Project.NameKey project, ImmutableList<Ref> inputRefs)
+ throws IOException, PermissionBackendException {
+ PermissionBackend.ForProject perm = permissionBackend.currentUser().project(project);
+ try (Repository repo = repoManager.openRepository(project)) {
+ return perm.filter(inputRefs, repo, RefFilterOptions.defaults()).stream()
+ .map(Ref::getName)
+ .collect(toImmutableList());
+ }
+ }
+
+ private ImmutableSortedSet<String> sortedShortNames(Collection<String> refs) {
+ return refs.stream()
+ .map(Repository::shortenRefName)
+ .collect(toImmutableSortedSet(naturalOrder()));
+ }
}
diff --git a/java/com/google/gerrit/server/change/IncludedInResolver.java b/java/com/google/gerrit/server/change/IncludedInResolver.java
index 3e1b69b..b2b0a64 100644
--- a/java/com/google/gerrit/server/change/IncludedInResolver.java
+++ b/java/com/google/gerrit/server/change/IncludedInResolver.java
@@ -14,13 +14,11 @@
package com.google.gerrit.server.change;
-import static com.google.common.collect.ImmutableSortedSet.toImmutableSortedSet;
import static java.util.Comparator.comparing;
-import static java.util.Comparator.naturalOrder;
import static java.util.stream.Collectors.toList;
import com.google.auto.value.AutoValue;
-import com.google.common.collect.ImmutableSortedSet;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.LinkedListMultimap;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
@@ -117,13 +115,12 @@
* Returns the short names of refs which are as well in the matchingRefs list as well as in the
* allRef list.
*/
- private static ImmutableSortedSet<String> getMatchingRefNames(
+ private static ImmutableList<Ref> getMatchingRefNames(
Set<String> matchingRefs, Collection<Ref> allRefs) {
return allRefs.stream()
- .map(Ref::getName)
- .filter(matchingRefs::contains)
- .map(Repository::shortenRefName)
- .collect(toImmutableSortedSet(naturalOrder()));
+ .filter(r -> matchingRefs.contains(r.getName()))
+ .distinct()
+ .collect(ImmutableList.toImmutableList());
}
/** Parse commit of ref and store the relation between ref and commit. */
@@ -157,8 +154,8 @@
@AutoValue
public abstract static class Result {
- public abstract ImmutableSortedSet<String> branches();
+ public abstract ImmutableList<Ref> branches();
- public abstract ImmutableSortedSet<String> tags();
+ public abstract ImmutableList<Ref> tags();
}
}
diff --git a/java/com/google/gerrit/server/query/approval/ListOfFilesUnchangedPredicate.java b/java/com/google/gerrit/server/query/approval/ListOfFilesUnchangedPredicate.java
index 459a8b0..ec658ac 100644
--- a/java/com/google/gerrit/server/query/approval/ListOfFilesUnchangedPredicate.java
+++ b/java/com/google/gerrit/server/query/approval/ListOfFilesUnchangedPredicate.java
@@ -16,38 +16,52 @@
import com.google.gerrit.entities.Patch.ChangeType;
import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.index.query.Predicate;
+import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.DiffOperations;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import com.google.inject.Inject;
import com.google.inject.Singleton;
+import java.io.IOException;
import java.util.Collection;
import java.util.Map;
import java.util.Objects;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevWalk;
/** Predicate that matches when the new patch-set includes the same files as the old patch-set. */
@Singleton
public class ListOfFilesUnchangedPredicate extends ApprovalPredicate {
private final DiffOperations diffOperations;
+ private final GitRepositoryManager repositoryManager;
@Inject
- public ListOfFilesUnchangedPredicate(DiffOperations diffOperations) {
+ public ListOfFilesUnchangedPredicate(
+ DiffOperations diffOperations, GitRepositoryManager repositoryManager) {
this.diffOperations = diffOperations;
+ this.repositoryManager = repositoryManager;
}
@Override
public boolean match(ApprovalContext ctx) {
- PatchSet currentPatchset = ctx.changeNotes().getCurrentPatchSet();
- Map.Entry<PatchSet.Id, PatchSet> priorPatchSet =
- ctx.changeNotes().getPatchSets().lowerEntry(currentPatchset.id());
+ PatchSet targetPatchSet = ctx.changeNotes().getPatchSets().get(ctx.target());
+ PatchSet sourcePatchSet =
+ ctx.changeNotes().getPatchSets().get(ctx.patchSetApproval().patchSetId());
+
+ Integer parentNum =
+ isInitialCommit(ctx.changeNotes().getProjectName(), targetPatchSet.commitId()) ? 0 : 1;
try {
- return match(
- diffOperations.listModifiedFiles(
- ctx.changeNotes().getProjectName(),
- priorPatchSet.getValue().commitId(),
- currentPatchset.commitId()));
+ Map<String, FileDiffOutput> modifiedTargetPatchSet =
+ diffOperations.listModifiedFilesAgainstParent(
+ ctx.changeNotes().getProjectName(), targetPatchSet.commitId(), parentNum);
+ Map<String, FileDiffOutput> modifiedSourcePatchSet =
+ diffOperations.listModifiedFilesAgainstParent(
+ ctx.changeNotes().getProjectName(), sourcePatchSet.commitId(), parentNum);
+ return match(modifiedTargetPatchSet, modifiedSourcePatchSet);
} catch (DiffNotAvailableException ex) {
throw new StorageException(
"failed to compute difference in files, so won't copy"
@@ -57,24 +71,46 @@
}
}
- public boolean match(Map<String, FileDiffOutput> modifiedFiles) {
- return modifiedFiles.values().stream()
- .noneMatch(
- p ->
- p.changeType() == ChangeType.ADDED
- || p.changeType() == ChangeType.DELETED
- || p.changeType() == ChangeType.RENAMED);
+ /**
+ * returns {@code true} if the files that were modified are the same in both inputs, and the
+ * {@link ChangeType} matches for each modified file.
+ */
+ public boolean match(
+ Map<String, FileDiffOutput> modifiedFiles1, Map<String, FileDiffOutput> modifiedFiles2) {
+ if (modifiedFiles1.size() != modifiedFiles2.size()) {
+ return false;
+ }
+ for (String file : modifiedFiles1.keySet()) {
+ FileDiffOutput fileDiffOutput1 = modifiedFiles1.get(file);
+ FileDiffOutput fileDiffOutput2 = modifiedFiles2.get(file);
+ if (fileDiffOutput2 == null) {
+ return false;
+ }
+ if (!fileDiffOutput2.changeType().equals(fileDiffOutput1.changeType())) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ public boolean isInitialCommit(Project.NameKey project, ObjectId objectId) {
+ try (Repository repo = repositoryManager.openRepository(project);
+ RevWalk revWalk = new RevWalk(repo)) {
+ return revWalk.parseCommit(objectId).getParentCount() == 0;
+ } catch (IOException ex) {
+ throw new StorageException(ex);
+ }
}
@Override
public Predicate<ApprovalContext> copy(
Collection<? extends Predicate<ApprovalContext>> children) {
- return new ListOfFilesUnchangedPredicate(diffOperations);
+ return new ListOfFilesUnchangedPredicate(diffOperations, repositoryManager);
}
@Override
public int hashCode() {
- return Objects.hash(diffOperations);
+ return Objects.hash(diffOperations, repositoryManager);
}
@Override
@@ -83,6 +119,7 @@
return false;
}
ListOfFilesUnchangedPredicate o = (ListOfFilesUnchangedPredicate) other;
- return Objects.equals(o.diffOperations, diffOperations);
+ return Objects.equals(o.diffOperations, diffOperations)
+ && Objects.equals(o.repositoryManager, repositoryManager);
}
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 383e385..e244d6c 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -1539,6 +1539,12 @@
private Predicate<ChangeData> getAuthorOrCommitterFullTextPredicate(
String who, Function<String, Predicate<ChangeData>> fullPredicateFunc)
throws QueryParseException {
+ if (isSelf(who)) {
+ IdentifiedUser me = args.getIdentifiedUser();
+ List<Predicate<ChangeData>> predicates =
+ me.getEmailAddresses().stream().map(fullPredicateFunc).collect(toList());
+ return Predicate.or(predicates);
+ }
Set<String> parts = SchemaUtil.getNameParts(who);
if (parts.isEmpty()) {
throw error("invalid value");
diff --git a/java/com/google/gerrit/server/restapi/change/ChangeIncludedIn.java b/java/com/google/gerrit/server/restapi/change/ChangeIncludedIn.java
index 67b5870..517fbdf 100644
--- a/java/com/google/gerrit/server/restapi/change/ChangeIncludedIn.java
+++ b/java/com/google/gerrit/server/restapi/change/ChangeIncludedIn.java
@@ -22,6 +22,7 @@
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.IncludedIn;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
@@ -38,7 +39,8 @@
}
@Override
- public Response<IncludedInInfo> apply(ChangeResource rsrc) throws RestApiException, IOException {
+ public Response<IncludedInInfo> apply(ChangeResource rsrc)
+ throws RestApiException, IOException, PermissionBackendException {
PatchSet ps = psUtil.current(rsrc.getNotes());
return Response.ok(includedIn.apply(rsrc.getProject(), ps.commitId().name()));
}
diff --git a/java/com/google/gerrit/server/restapi/project/CommitIncludedIn.java b/java/com/google/gerrit/server/restapi/project/CommitIncludedIn.java
index a4a82ce..e566858 100644
--- a/java/com/google/gerrit/server/restapi/project/CommitIncludedIn.java
+++ b/java/com/google/gerrit/server/restapi/project/CommitIncludedIn.java
@@ -20,6 +20,7 @@
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.server.change.IncludedIn;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.CommitResource;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -36,7 +37,8 @@
}
@Override
- public Response<IncludedInInfo> apply(CommitResource rsrc) throws RestApiException, IOException {
+ public Response<IncludedInInfo> apply(CommitResource rsrc)
+ throws RestApiException, IOException, PermissionBackendException {
RevCommit commit = rsrc.getCommit();
Project.NameKey project = rsrc.getProjectState().getNameKey();
return Response.ok(includedIn.apply(project, commit.getId().getName()));
diff --git a/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java b/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
index 53a9364..0b3b27a 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
@@ -35,6 +35,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.ChangeKindCreator;
import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
@@ -394,14 +395,31 @@
}
@Test
- public void notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsAdded()
- throws Exception {
+ public void
+ notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsAdded_withoutCopyCondition()
+ throws Exception {
try (ProjectConfigUpdate u = updateProject(project)) {
u.getConfig()
.updateLabelType(
LabelId.CODE_REVIEW, b -> b.setCopyAllScoresIfListOfFilesDidNotChange(true));
u.save();
}
+ notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsAdded();
+ }
+
+ @Test
+ public void notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsAdded_withCopyCondition()
+ throws Exception {
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig()
+ .updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyCondition("has:unchanged-files"));
+ u.save();
+ }
+ notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsAdded();
+ }
+
+ private void notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsAdded()
+ throws Exception {
Change.Id changeId =
changeOperations.newChange().project(project).file("file").content("content").create();
vote(admin, changeId.toString(), 2, 1);
@@ -421,14 +439,90 @@
}
@Test
- public void notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsDeleted()
- throws Exception {
+ public void
+ notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileAlreadyExists_withoutCopyCondition()
+ throws Exception {
try (ProjectConfigUpdate u = updateProject(project)) {
u.getConfig()
.updateLabelType(
LabelId.CODE_REVIEW, b -> b.setCopyAllScoresIfListOfFilesDidNotChange(true));
u.save();
}
+ notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileAlreadyExists();
+ }
+
+ @Test
+ public void
+ notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileAlreadyExists_withCopyCondition()
+ throws Exception {
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig()
+ .updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyCondition("has:unchanged-files"));
+ u.save();
+ }
+
+ notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileAlreadyExists();
+ }
+
+ private void notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileAlreadyExists()
+ throws Exception {
+ // 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_withoutCopyCondition()
+ throws Exception {
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig()
+ .updateLabelType(
+ LabelId.CODE_REVIEW, b -> b.setCopyAllScoresIfListOfFilesDidNotChange(true));
+ u.save();
+ }
+ notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsDeleted();
+ }
+
+ @Test
+ public void
+ notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsDeleted_withCopyCondition()
+ throws Exception {
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig()
+ .updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyCondition("has:unchanged-files"));
+ u.save();
+ }
+ notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsDeleted();
+ }
+
+ private void notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsDeleted()
+ throws Exception {
Change.Id changeId =
changeOperations.newChange().project(project).file("file").content("content").create();
vote(admin, changeId.toString(), 2, 1);
@@ -443,14 +537,72 @@
}
@Test
- public void stickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsModified()
- throws Exception {
+ public void
+ stickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsModified_withoutCopyCondition()
+ throws Exception {
try (ProjectConfigUpdate u = updateProject(project)) {
u.getConfig()
.updateLabelType(
LabelId.CODE_REVIEW, b -> b.setCopyAllScoresIfListOfFilesDidNotChange(true));
u.save();
}
+ stickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsModified();
+ }
+
+ @Test
+ public void stickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsModified_withCopyCondition()
+ throws Exception {
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig()
+ .updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyCondition("has:unchanged-files"));
+ u.save();
+ }
+ stickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsModified();
+ }
+
+ private void stickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsModified()
+ throws Exception {
+ 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);
+ }
+
+ @TestProjectInput(createEmptyCommit = false)
+ public void
+ stickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsModifiedAsInitialCommit_withoutCopyCondition()
+ throws Exception {
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig()
+ .updateLabelType(
+ LabelId.CODE_REVIEW, b -> b.setCopyAllScoresIfListOfFilesDidNotChange(true));
+ u.save();
+ }
+ stickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsModifiedAsInitialCommit();
+ }
+
+ @TestProjectInput(createEmptyCommit = false)
+ public void
+ stickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsModifiedAsInitialCommit_withCopyCondition()
+ throws Exception {
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig()
+ .updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyCondition("has:unchanged-files"));
+ u.save();
+ }
+ stickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsModifiedAsInitialCommit();
+ }
+
+ private void stickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsModifiedAsInitialCommit()
+ throws Exception {
Change.Id changeId =
changeOperations.newChange().project(project).file("file").content("content").create();
vote(admin, changeId.toString(), 2, 1);
@@ -466,14 +618,79 @@
}
@Test
- public void notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsRenamed()
- throws Exception {
+ public void
+ notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsModifiedOnEarlierPatchset_withoutCopyCondition()
+ throws Exception {
try (ProjectConfigUpdate u = updateProject(project)) {
u.getConfig()
.updateLabelType(
LabelId.CODE_REVIEW, b -> b.setCopyAllScoresIfListOfFilesDidNotChange(true));
u.save();
}
+ notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsModifiedOnEarlierPatchset();
+ }
+
+ @Test
+ public void
+ notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsModifiedOnEarlierPatchset_withCopyCondition()
+ throws Exception {
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig()
+ .updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyCondition("has:unchanged-files"));
+ u.save();
+ }
+ notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsModifiedOnEarlierPatchset();
+ }
+
+ private void
+ notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsModifiedOnEarlierPatchset()
+ throws Exception {
+ 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("new file").content("content").create();
+ changeOperations
+ .change(changeId)
+ .newPatchset()
+ .file("new file")
+ .content("new content")
+ .create();
+ ChangeInfo c = detailedChange(changeId.toString());
+
+ // Don't copy over votes since ps1->ps2 should copy over, but ps2->ps3 should not.
+ assertVotes(c, admin, 0, 0);
+ assertVotes(c, user, 0, 0);
+ }
+
+ @Test
+ public void
+ notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsRenamed_withoutCopyCondition()
+ throws Exception {
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig()
+ .updateLabelType(
+ LabelId.CODE_REVIEW, b -> b.setCopyAllScoresIfListOfFilesDidNotChange(true));
+ u.save();
+ }
+ notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsRenamed();
+ }
+
+ @Test
+ public void
+ notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsRenamed_withCopyCondition()
+ throws Exception {
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig()
+ .updateLabelType(LabelId.CODE_REVIEW, b -> b.setCopyCondition("has:unchanged-files"));
+ u.save();
+ }
+ notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsRenamed();
+ }
+
+ private void notStickyWithCopyAllScoresIfListOfFilesDidNotChangeWhenFileIsRenamed()
+ throws Exception {
Change.Id changeId =
changeOperations.newChange().project(project).file("file").content("content").create();
vote(admin, changeId.toString(), 2, 1);
diff --git a/javatests/com/google/gerrit/acceptance/api/change/SubmitWithStickyApprovalDiffIT.java b/javatests/com/google/gerrit/acceptance/api/change/SubmitWithStickyApprovalDiffIT.java
index ca54050..607fbc0 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/SubmitWithStickyApprovalDiffIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/SubmitWithStickyApprovalDiffIT.java
@@ -419,6 +419,44 @@
}
@Test
+ public void diffChangeMessageOnSubmitWithStickyVote_addedMultipleFiles() throws Exception {
+ Change.Id changeId = changeOperations.newChange().project(project).create();
+ gApi.changes().id(changeId.get()).current().review(ReviewInput.approve());
+
+ changeOperations
+ .change(changeId)
+ .newPatchset()
+ .file("file")
+ .content("content1\nmore content\nlast content")
+ .create();
+
+ changeOperations
+ .change(changeId)
+ .newPatchset()
+ .file("otherFile")
+ .content("content2\nmore content\nlast content")
+ .create();
+
+ // add a reviewer to ensure an email is sent.
+ gApi.changes().id(changeId.get()).addReviewer(user.email());
+
+ gApi.changes().id(changeId.get()).current().submit();
+
+ assertDiffChangeMessageAndEmailWithStickyApproval(
+ Iterables.getLast(gApi.changes().id(changeId.get()).messages()).message,
+ /* file1= */ "otherFile",
+ /* insertions1= */ 3,
+ /* deletions1= */ 0,
+ /* expectedFileDiff1= */ "@@ -0,0 +1,3 @@\n+content2\n+more content\n+last content",
+ /* oldFileName1= */ null,
+ /* file2= */ "file",
+ /* insertions2= */ 3,
+ /* deletions2= */ 0,
+ /* expectedFileDiff2= */ "@@ -0,0 +1,3 @@\n+content1\n+more content\n+last content",
+ /* oldFileName2= */ null);
+ }
+
+ @Test
public void diffChangeMessageOnSubmitWithStickyVote_removedFile() throws Exception {
Change.Id changeId =
changeOperations
@@ -537,11 +575,51 @@
int deletions,
String expectedFileDiff,
String oldFileName) {
+ assertDiffChangeMessageAndEmailWithStickyApproval(
+ message,
+ file,
+ insertions,
+ deletions,
+ expectedFileDiff,
+ oldFileName,
+ /* file2= */ null,
+ /* insertions2= */ 0,
+ /* deletions2 =
+ */ 0,
+ /* expectedFileDiff2= */ null,
+ /* oldFileName2= */ null);
+ }
+
+ private void assertDiffChangeMessageAndEmailWithStickyApproval(
+ String message,
+ String file1,
+ int insertions1,
+ int deletions1,
+ String expectedFileDiff1,
+ String oldFileName1,
+ String file2,
+ int insertions2,
+ int deletions2,
+ String expectedFileDiff2,
+ String oldFileName2) {
String expectedMessage =
"1 is the latest approved patch-set.\n"
+ "The change was submitted with unreviewed changes in the following files:\n"
- + "\n"
- + "```\n"
+ + "\n";
+ expectedMessage += fileDiff(expectedFileDiff1, oldFileName1, file1, insertions1, deletions1);
+ expectedMessage += fileDiff(expectedFileDiff2, oldFileName2, file2, insertions2, deletions2);
+ String expectedChangeMessage = "Change has been successfully merged\n\n" + expectedMessage;
+ assertThat(message.trim()).isEqualTo(expectedChangeMessage.trim());
+ assertThat(Iterables.getLast(sender.getMessages()).body()).contains(expectedMessage);
+ }
+
+ private String fileDiff(
+ String expectedFileDiff, String oldFileName, String file, int insertions, int deletions) {
+ if (file == null) {
+ return "";
+ }
+ String expectedMessage =
+ "```\n"
+ String.format("The name of the file: %s\n", file)
+ String.format("Insertions: %d, Deletions: %d.\n\n", insertions, deletions);
@@ -550,8 +628,6 @@
}
expectedMessage += expectedFileDiff;
expectedMessage += "\n```\n";
- String expectedChangeMessage = "Change has been successfully merged\n\n" + expectedMessage;
- assertThat(message.trim()).isEqualTo(expectedChangeMessage.trim());
- assertThat(Iterables.getLast(sender.getMessages()).body()).contains(expectedMessage);
+ return expectedMessage;
}
}
diff --git a/javatests/com/google/gerrit/acceptance/api/project/CommitIT.java b/javatests/com/google/gerrit/acceptance/api/project/CommitIT.java
index bdb03d2..18e192d 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/CommitIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/CommitIT.java
@@ -15,7 +15,10 @@
package com.google.gerrit.acceptance.api.project;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.GitUtil.pushHead;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static java.util.stream.Collectors.toList;
import static org.eclipse.jgit.lib.Constants.R_TAGS;
@@ -28,6 +31,7 @@
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Permission;
+import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.changes.CherryPickInput;
import com.google.gerrit.extensions.api.changes.IncludedInInfo;
import com.google.gerrit.extensions.api.changes.ReviewInput;
@@ -99,6 +103,53 @@
}
@Test
+ public void includedInMergedChange_filtersOutNonVisibleBranches() throws Exception {
+ Result baseChange = createAndSubmitChange("refs/for/master");
+
+ createBranch(BranchNameKey.create(project, "test-branch-1"));
+ createBranch(BranchNameKey.create(project, "test-branch-2"));
+ createAndSubmitChange("refs/for/test-branch-1");
+ createAndSubmitChange("refs/for/test-branch-2");
+
+ assertThat(getIncludedIn(baseChange.getCommit().getId()).branches)
+ .containsExactly("master", "test-branch-1", "test-branch-2");
+
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(block(Permission.READ).ref("refs/heads/test-branch-1").group(REGISTERED_USERS))
+ .update();
+
+ assertThat(getIncludedIn(baseChange.getCommit().getId()).branches)
+ .containsExactly("master", "test-branch-2");
+ }
+
+ @Test
+ public void includedInMergedChange_filtersOutNonVisibleTags() throws Exception {
+ String tagBase = "tag_base";
+ String tagBranch1 = "tag_1";
+
+ Result baseChange = createAndSubmitChange("refs/for/master");
+ createLightWeightTag(tagBase);
+ assertThat(getIncludedIn(baseChange.getCommit().getId()).tags).containsExactly(tagBase);
+
+ createBranch(BranchNameKey.create(project, "test-branch-1"));
+ createAndSubmitChange("refs/for/test-branch-1");
+ createLightWeightTag(tagBranch1);
+ assertThat(getIncludedIn(baseChange.getCommit().getId()).tags)
+ .containsExactly(tagBase, tagBranch1);
+
+ projectOperations
+ .project(project)
+ .forUpdate()
+ // Tag permissions are controlled by read permissions on branches. Blocking read permission
+ // on test-branch-1 so that tagBranch1 becomes non-visible
+ .add(block(Permission.READ).ref("refs/heads/test-branch-1").group(REGISTERED_USERS))
+ .update();
+ assertThat(getIncludedIn(baseChange.getCommit().getId()).tags).containsExactly(tagBase);
+ }
+
+ @Test
public void cherryPickWithoutMessageSameBranch() throws Exception {
String destBranch = "master";
@@ -390,4 +441,15 @@
assertThat(actual.email).isEqualTo(expected.email());
assertThat(actual.name).isEqualTo(expected.fullName());
}
+
+ private Result createAndSubmitChange(String branch) throws Exception {
+ Result r = createChange(branch);
+ approve(r.getChangeId());
+ gApi.changes().id(r.getChangeId()).current().submit();
+ return r;
+ }
+
+ private void createLightWeightTag(String tagName) throws Exception {
+ pushHead(testRepo, RefNames.REFS_TAGS + tagName, false, false);
+ }
}
diff --git a/javatests/com/google/gerrit/server/change/IncludedInResolverTest.java b/javatests/com/google/gerrit/server/change/IncludedInResolverTest.java
index 19c479d..b69a894 100644
--- a/javatests/com/google/gerrit/server/change/IncludedInResolverTest.java
+++ b/javatests/com/google/gerrit/server/change/IncludedInResolverTest.java
@@ -17,9 +17,13 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.entities.RefNames.REFS_TAGS;
+import com.google.common.truth.Correspondence;
+import com.google.gerrit.truth.NullAwareCorrespondence;
import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevObject;
import org.eclipse.jgit.revwalk.RevTag;
@@ -112,8 +116,12 @@
IncludedInResolver.Result detail = resolve(commit_v2_5);
// Check that only tags and branches which refer the tip are returned
- assertThat(detail.tags()).containsExactly(TAG_2_5, TAG_2_5_ANNOTATED, TAG_2_5_ANNOTATED_TWICE);
- assertThat(detail.branches()).containsExactly(BRANCH_2_5);
+ assertThat(detail.tags())
+ .comparingElementsUsing(hasShortName())
+ .containsExactly(TAG_2_5, TAG_2_5_ANNOTATED, TAG_2_5_ANNOTATED_TWICE);
+ assertThat(detail.branches())
+ .comparingElementsUsing(hasShortName())
+ .containsExactly(BRANCH_2_5);
}
@Test
@@ -123,6 +131,7 @@
// Check whether all tags and branches are returned
assertThat(detail.tags())
+ .comparingElementsUsing(hasShortName())
.containsExactly(
TAG_1_0,
TAG_1_0_1,
@@ -133,6 +142,7 @@
TAG_2_5_ANNOTATED,
TAG_2_5_ANNOTATED_TWICE);
assertThat(detail.branches())
+ .comparingElementsUsing(hasShortName())
.containsExactly(BRANCH_MASTER, BRANCH_1_0, BRANCH_1_3, BRANCH_2_0, BRANCH_2_5);
}
@@ -143,8 +153,11 @@
// Check whether all succeeding tags and branches are returned
assertThat(detail.tags())
+ .comparingElementsUsing(hasShortName())
.containsExactly(TAG_1_3, TAG_2_5, TAG_2_5_ANNOTATED, TAG_2_5_ANNOTATED_TWICE);
- assertThat(detail.branches()).containsExactly(BRANCH_1_3, BRANCH_2_5);
+ assertThat(detail.branches())
+ .comparingElementsUsing(hasShortName())
+ .containsExactly(BRANCH_1_3, BRANCH_2_5);
}
private IncludedInResolver.Result resolve(RevCommit commit) throws Exception {
@@ -154,4 +167,9 @@
private RevTag tag(String name, RevObject dest) throws Exception {
return tr.update(REFS_TAGS + name, tr.tag(name, dest));
}
+
+ private static Correspondence<Ref, String> hasShortName() {
+ return NullAwareCorrespondence.transforming(
+ ref -> Repository.shortenRefName(ref.getName()), "has short name");
+ }
}
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 1f29f45..2d6f9b6 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -621,9 +621,14 @@
PersonIdent johnDoe = new PersonIdent("John Doe", "john.doe@example.com");
PersonIdent john = new PersonIdent("John", "john@example.com");
PersonIdent doeSmith = new PersonIdent("Doe Smith", "doe_smith@example.com");
+ Account ua = user.asIdentifiedUser().getAccount();
+ PersonIdent myself = new PersonIdent("I Am", ua.preferredEmail());
+ PersonIdent selfName = new PersonIdent("My Self", "my.self@example.com");
+
Change change1 = createChange(repo, johnDoe);
Change change2 = createChange(repo, john);
Change change3 = createChange(repo, doeSmith);
+ Change change4 = createChange(repo, selfName);
// Only email address.
assertQuery(searchOperator + "john.doe@example.com", change1);
@@ -639,6 +644,18 @@
assertQuery(searchOperator + "\"John <john.doe@example.com>\"");
assertQuery(searchOperator + "\"Doe John <john@example.com>\"");
assertQuery(searchOperator + "\"Doe John <doe_smith@example.com>\"");
+
+ // Partial name
+ assertQuery(searchOperator + "ohn");
+ assertQuery(searchOperator + "smith", change3);
+
+ // The string 'self' in the name should not be matched
+ assertQuery(searchOperator + "self");
+
+ // ':self' matches a change created with the current user's email address
+ Change change5 = createChange(repo, myself);
+ assertQuery(searchOperator + "me", change5);
+ assertQuery(searchOperator + "self", change5);
}
private void byAuthorOrCommitterFullText(String searchOperator) throws Exception {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element_test.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element_test.js
index 4455bd5..196567e 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element_test.js
@@ -53,7 +53,8 @@
let element;
let builder;
- const LINE_FEED_HTML = '<span class="style-scope gr-diff br"></span>';
+ const LINE_BREAK_HTML = '<span class="style-scope gr-diff br"></span>';
+ const WBR_HTML = '<wbr class="style-scope gr-diff">';
setup(() => {
element = basicFixture.instantiate();
@@ -78,59 +79,60 @@
test('newlines 1', () => {
let text = 'abcdef';
- assert.equal(builder._formatText(text, 4, 10).innerHTML, text);
+ assert.equal(builder._formatText(text, 'NONE', 4, 10).innerHTML, text);
text = 'a'.repeat(20);
- assert.equal(builder._formatText(text, 4, 10).innerHTML,
+ assert.equal(builder._formatText(text, 'NONE', 4, 10).innerHTML,
'a'.repeat(10) +
- LINE_FEED_HTML +
+ LINE_BREAK_HTML +
'a'.repeat(10));
});
test('newlines 2', () => {
const text = '<span class="thumbsup">👍</span>';
- assert.equal(builder._formatText(text, 4, 10).innerHTML,
+ assert.equal(builder._formatText(text, 'NONE', 4, 10).innerHTML,
'<span clas' +
- LINE_FEED_HTML +
+ LINE_BREAK_HTML +
's="thumbsu' +
- LINE_FEED_HTML +
+ LINE_BREAK_HTML +
'p">👍</span' +
- LINE_FEED_HTML +
+ LINE_BREAK_HTML +
'>');
});
test('newlines 3', () => {
const text = '01234\t56789';
- assert.equal(builder._formatText(text, 4, 10).innerHTML,
+ assert.equal(builder._formatText(text, 'NONE', 4, 10).innerHTML,
'01234' + builder._getTabWrapper(3).outerHTML + '56' +
- LINE_FEED_HTML +
+ LINE_BREAK_HTML +
'789');
});
test('newlines 4', () => {
const text = '👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍';
- assert.equal(builder._formatText(text, 4, 20).innerHTML,
+ assert.equal(builder._formatText(text, 'NONE', 4, 20).innerHTML,
'👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍' +
- LINE_FEED_HTML +
+ LINE_BREAK_HTML +
'👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍' +
- LINE_FEED_HTML +
+ LINE_BREAK_HTML +
'👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍');
});
- test('line_length ignored if line_wrapping is true', () => {
+ test('line_length applied with <wbr> if line_wrapping is true', () => {
builder._prefs = {line_wrapping: true, tab_size: 4, line_length: 50};
const text = 'a'.repeat(51);
const line = {text, highlights: []};
+ const expected = 'a'.repeat(50) + WBR_HTML + 'a';
const result = builder._createTextEl(undefined, line).firstChild.innerHTML;
- assert.equal(result, text);
+ assert.equal(result, expected);
});
- test('line_length applied if line_wrapping is false', () => {
+ test('line_length applied with line break if line_wrapping is false', () => {
builder._prefs = {line_wrapping: false, tab_size: 4, line_length: 50};
const text = 'a'.repeat(51);
const line = {text, highlights: []};
- const expected = 'a'.repeat(50) + LINE_FEED_HTML + 'a';
+ const expected = 'a'.repeat(50) + LINE_BREAK_HTML + 'a';
const result = builder._createTextEl(undefined, line).firstChild.innerHTML;
assert.equal(result, expected);
});
@@ -173,22 +175,25 @@
test('text length with tabs and unicode', () => {
function expectTextLength(text, tabSize, expected) {
// Formatting to |expected| columns should not introduce line breaks.
- const result = builder._formatText(text, tabSize, expected);
+ const result = builder._formatText(text, 'NONE', tabSize, expected);
assert.isNotOk(result.querySelector('.contentText > .br'),
` Expected the result of: \n` +
- ` _formatText(${text}', ${tabSize}, ${expected})\n` +
+ ` _formatText(${text}', 'NONE', ${tabSize}, ${expected})\n` +
` to not contain a br. But the actual result HTML was:\n` +
` '${result.innerHTML}'\nwhereupon`);
// Increasing the line limit should produce the same markup.
- assert.equal(builder._formatText(text, tabSize, Infinity).innerHTML,
+ assert.equal(
+ builder._formatText(text, 'NONE', tabSize, Infinity).innerHTML,
result.innerHTML);
- assert.equal(builder._formatText(text, tabSize, expected + 1).innerHTML,
+ assert.equal(
+ builder._formatText(text, 'NONE', tabSize, expected + 1).innerHTML,
result.innerHTML);
// Decreasing the line limit should introduce line breaks.
if (expected > 0) {
- const tooSmall = builder._formatText(text, tabSize, expected - 1);
+ const tooSmall = builder._formatText(text,
+ 'NONE', tabSize, expected - 1);
assert.isOk(tooSmall.querySelector('.contentText > .br'),
` Expected the result of: \n` +
` _formatText(${text}', ${tabSize}, ${expected - 1})\n` +
@@ -216,7 +221,7 @@
assert.ok(wrapper);
assert.equal(wrapper.innerText, '\t');
assert.equal(
- builder._formatText(html, tabSize, Infinity).innerHTML,
+ builder._formatText(html, 'NONE', tabSize, Infinity).innerHTML,
'abc' + wrapper.outerHTML + 'def');
});
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts
index 987f808..869faa8 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.ts
@@ -89,6 +89,12 @@
return 'NONE';
}
+export function isResponsive(responsiveMode: DiffResponsiveMode) {
+ return (
+ responsiveMode === 'FULL_RESPONSIVE' || responsiveMode === 'SHRINK_ONLY'
+ );
+}
+
export abstract class GrDiffBuilder {
private readonly _diff: DiffInfo;
@@ -513,12 +519,11 @@
const {beforeNumber, afterNumber} = line;
if (beforeNumber !== 'FILE' && beforeNumber !== 'LOST') {
const responsiveMode = getResponsiveMode(this._prefs, this._renderPrefs);
- const lineLimit =
- responsiveMode === 'NONE' ? this._prefs.line_length : Infinity;
const contentText = this._formatText(
line.text,
+ responsiveMode,
this._prefs.tab_size,
- lineLimit
+ this._prefs.line_length
);
if (side) {
@@ -544,6 +549,12 @@
return td;
}
+ private createLineBreak(responsive: boolean) {
+ return responsive
+ ? this._createElement('wbr')
+ : this._createElement('span', 'br');
+ }
+
/**
* Returns a 'div' element containing the supplied |text| as its innerText,
* with '\t' characters expanded to a width determined by |tabSize|, and the
@@ -554,9 +565,14 @@
* @param tabSize The width of each tab stop.
* @param lineLimit The column after which to wrap lines.
*/
- _formatText(text: string, tabSize: number, lineLimit: number): HTMLElement {
+ _formatText(
+ text: string,
+ responsiveMode: DiffResponsiveMode,
+ tabSize: number,
+ lineLimit: number
+ ): HTMLElement {
const contentText = this._createElement('div', 'contentText');
-
+ const responsive = isResponsive(responsiveMode);
let columnPos = 0;
let textOffset = 0;
for (const segment of text.split(REGEX_TAB_OR_SURROGATE_PAIR)) {
@@ -570,7 +586,7 @@
contentText.appendChild(
document.createTextNode(segment.substring(rowStart, rowEnd))
);
- contentText.appendChild(this._createElement('span', 'br'));
+ contentText.appendChild(this.createLineBreak(responsive));
columnPos = 0;
rowStart = rowEnd;
rowEnd += lineLimit;
@@ -588,7 +604,7 @@
// Append a single '\t' character.
let effectiveTabSize = tabSize - (columnPos % tabSize);
if (columnPos + effectiveTabSize > lineLimit) {
- contentText.appendChild(this._createElement('span', 'br'));
+ contentText.appendChild(this.createLineBreak(responsive));
columnPos = 0;
effectiveTabSize = tabSize;
}
@@ -598,7 +614,7 @@
} else {
// Append a single surrogate pair.
if (columnPos >= lineLimit) {
- contentText.appendChild(this._createElement('span', 'br'));
+ contentText.appendChild(this.createLineBreak(responsive));
columnPos = 0;
}
contentText.appendChild(
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
index 9fb2a19..7efd2f8 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
@@ -85,6 +85,7 @@
import {
DiffContextExpandedEventDetail,
getResponsiveMode,
+ isResponsive,
} from '../gr-diff-builder/gr-diff-builder';
const NO_NEWLINE_BASE = 'No newline at end of base file.';
@@ -754,8 +755,7 @@
const stylesToUpdate: {[key: string]: string} = {};
const responsiveMode = getResponsiveMode(prefs, renderPrefs);
- const responsive =
- responsiveMode === 'FULL_RESPONSIVE' || responsiveMode === 'SHRINK_ONLY';
+ const responsive = isResponsive(responsiveMode);
this._diffTableClass = responsive ? 'responsive' : '';
const lineLimit = `${lineLength}ch`;
stylesToUpdate['--line-limit-marker'] =
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
index 0b43879..6745f58 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
@@ -547,6 +547,7 @@
reply.__editing = true;
}
+ this.commentsService.addDraft(reply);
this.push('comments', reply);
if (!isEditing) {
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
index 0bf122a..5f860f4 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
@@ -603,6 +603,7 @@
message: 'i like you, too',
in_reply_to: 'sallys_confession' as UrlEncodedCommentId,
updated: '2015-12-25 15:00:20.396000000' as Timestamp,
+ path: 'abcd',
unresolved: false,
},
{
@@ -610,17 +611,20 @@
in_reply_to: 'nonexistent_comment' as UrlEncodedCommentId,
message: 'i like you, jack',
updated: '2015-12-24 15:00:20.396000000' as Timestamp,
+ path: 'abcd',
},
{
id: 'sally_to_dr_finklestein' as UrlEncodedCommentId,
in_reply_to: 'nonexistent_comment' as UrlEncodedCommentId,
message: 'i’m running away',
updated: '2015-10-31 09:00:20.396000000' as Timestamp,
+ path: 'abcd',
},
{
id: 'sallys_defiance' as UrlEncodedCommentId,
message: 'i will poison you so i can get away',
updated: '2015-10-31 15:00:20.396000000' as Timestamp,
+ path: 'abcd',
},
];
});
