Merge "Merge branch 'stable-3.0'"
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index aa1de1d..00350d1 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -1157,17 +1157,6 @@
+
Default is true.
-[[change.allowDrafts]]change.allowDrafts::
-+
-Legacy support for drafts workflow. If set to true, pushing a new change
-with draft option will create a private change. Pushing with draft option
-to an existing change will create change edit.
-+
-Enabling this option allows to push to the `refs/drafts/branch`. When
-disabled any push to `refs/drafts/branch` will be rejected.
-+
-Default is false.
-
[[change.api.allowedIdentifier]]change.api.allowedIdentifier::
+
Change identifier(s) that are allowed on the API. See
diff --git a/java/com/google/gerrit/acceptance/InProcessProtocol.java b/java/com/google/gerrit/acceptance/InProcessProtocol.java
index 2b6cb00..ae85134 100644
--- a/java/com/google/gerrit/acceptance/InProcessProtocol.java
+++ b/java/com/google/gerrit/acceptance/InProcessProtocol.java
@@ -31,7 +31,6 @@
import com.google.gerrit.server.RemotePeer;
import com.google.gerrit.server.RequestCleanup;
import com.google.gerrit.server.config.GerritRequestModule;
-import com.google.gerrit.server.git.DefaultAdvertiseRefsHook;
import com.google.gerrit.server.git.PermissionAwareRepositoryManager;
import com.google.gerrit.server.git.ReceivePackInitializer;
import com.google.gerrit.server.git.TransferConfig;
@@ -39,7 +38,6 @@
import com.google.gerrit.server.git.receive.AsyncReceiveCommits;
import com.google.gerrit.server.git.validators.UploadValidators;
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.permissions.ProjectPermission;
import com.google.gerrit.server.plugincontext.PluginSetContext;
@@ -252,7 +250,6 @@
UploadPack up = new UploadPack(permissionAwareRepository);
up.setPackConfig(transferConfig.getPackConfig());
up.setTimeout(transferConfig.getTimeout());
- up.setAdvertiseRefsHook(new DefaultAdvertiseRefsHook(perm, RefFilterOptions.defaults()));
List<PreUploadHook> hooks = Lists.newArrayList(preUploadHooks);
hooks.add(
uploadValidatorsFactory.create(
diff --git a/java/com/google/gerrit/httpd/GitOverHttpServlet.java b/java/com/google/gerrit/httpd/GitOverHttpServlet.java
index 755bf7a..ff2a83d 100644
--- a/java/com/google/gerrit/httpd/GitOverHttpServlet.java
+++ b/java/com/google/gerrit/httpd/GitOverHttpServlet.java
@@ -30,7 +30,6 @@
import com.google.gerrit.server.RequestListener;
import com.google.gerrit.server.audit.HttpAuditEvent;
import com.google.gerrit.server.cache.CacheModule;
-import com.google.gerrit.server.git.DefaultAdvertiseRefsHook;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.PermissionAwareRepositoryManager;
import com.google.gerrit.server.git.TransferConfig;
@@ -40,7 +39,6 @@
import com.google.gerrit.server.group.GroupAuditService;
import com.google.gerrit.server.logging.TraceContext;
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.permissions.ProjectPermission;
import com.google.gerrit.server.plugincontext.PluginSetContext;
@@ -55,6 +53,7 @@
import com.google.inject.name.Named;
import java.io.IOException;
import java.time.Duration;
+import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.Map;
@@ -338,6 +337,11 @@
up.setTimeout(config.getTimeout());
up.setPreUploadHook(PreUploadHookChain.newChain(Lists.newArrayList(preUploadHooks)));
up.setPostUploadHook(PostUploadHookChain.newChain(Lists.newArrayList(postUploadHooks)));
+ String header = req.getHeader("Git-Protocol");
+ if (header != null) {
+ String[] params = header.split(":");
+ up.setExtraParameters(Arrays.asList(params));
+ }
uploadPackInitializers.runEach(initializer -> initializer.init(state.getNameKey(), up));
return up;
}
@@ -411,7 +415,6 @@
up.setPreUploadHook(
PreUploadHookChain.newChain(
Lists.newArrayList(up.getPreUploadHook(), uploadValidators)));
- up.setAdvertiseRefsHook(new DefaultAdvertiseRefsHook(perm, RefFilterOptions.defaults()));
next.doFilter(httpRequest, responseWrapper);
} finally {
groupAuditService.dispatch(
diff --git a/java/com/google/gerrit/server/git/DefaultAdvertiseRefsHook.java b/java/com/google/gerrit/server/git/DefaultAdvertiseRefsHook.java
deleted file mode 100644
index d1a6df6..0000000
--- a/java/com/google/gerrit/server/git/DefaultAdvertiseRefsHook.java
+++ /dev/null
@@ -1,62 +0,0 @@
-// Copyright (C) 2018 The Android Open Source Project
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.gerrit.server.git;
-
-import com.google.common.collect.ImmutableList;
-import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.server.permissions.PermissionBackend;
-import com.google.gerrit.server.permissions.PermissionBackendException;
-import java.io.IOException;
-import java.util.List;
-import java.util.Map;
-import org.eclipse.jgit.lib.Ref;
-import org.eclipse.jgit.lib.RefDatabase;
-import org.eclipse.jgit.lib.Repository;
-import org.eclipse.jgit.revwalk.RevWalk;
-import org.eclipse.jgit.transport.AbstractAdvertiseRefsHook;
-import org.eclipse.jgit.transport.ServiceMayNotContinueException;
-
-/**
- * Wrapper around {@link com.google.gerrit.server.permissions.PermissionBackend.ForProject} that
- * implements {@link org.eclipse.jgit.transport.AdvertiseRefsHook}.
- */
-public class DefaultAdvertiseRefsHook extends AbstractAdvertiseRefsHook {
- private static final FluentLogger logger = FluentLogger.forEnclosingClass();
-
- private final PermissionBackend.ForProject perm;
- private final PermissionBackend.RefFilterOptions opts;
-
- public DefaultAdvertiseRefsHook(
- PermissionBackend.ForProject perm, PermissionBackend.RefFilterOptions opts) {
- this.perm = perm;
- this.opts = opts;
- }
-
- @Override
- protected Map<String, Ref> getAdvertisedRefs(Repository repo, RevWalk revWalk)
- throws ServiceMayNotContinueException {
- logger.atFine().log("ref filter options = %s", opts);
- try {
- List<String> prefixes =
- !opts.prefixes().isEmpty() ? opts.prefixes() : ImmutableList.of(RefDatabase.ALL);
- return perm.filter(
- repo.getRefDatabase().getRefsByPrefix(prefixes.toArray(new String[0])), repo, opts);
- } catch (IOException | PermissionBackendException e) {
- ServiceMayNotContinueException ex = new ServiceMayNotContinueException();
- ex.initCause(e);
- throw ex;
- }
- }
-}
diff --git a/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java b/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java
index a644b52..95ce8a1 100644
--- a/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java
@@ -286,6 +286,7 @@
// Check objects mentioned inside the incoming pack file are reachable from visible refs.
Project.NameKey projectName = projectState.getNameKey();
this.perm = permissionBackend.user(user).project(projectName);
+
receivePack = new ReceivePack(PermissionAwareRepositoryManager.wrap(repo, perm));
receivePack.setAllowCreates(true);
receivePack.setAllowDeletes(true);
@@ -309,8 +310,7 @@
allRefsWatcher = new AllRefsWatcher();
receivePack.setAdvertiseRefsHook(
- ReceiveCommitsAdvertiseRefsHookChain.create(
- allRefsWatcher, perm, queryProvider, projectName));
+ ReceiveCommitsAdvertiseRefsHookChain.create(allRefsWatcher, queryProvider, projectName));
resultChangeIds = new ResultChangeIds();
receiveCommits =
factory.create(
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index e991318..415811f 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -238,9 +238,6 @@
class ReceiveCommits {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
- private static final String CODE_REVIEW_ERROR =
- "You need 'Push' rights to upload code review requests.\n"
- + "Verify that you are pushing to the right branch.";
private static final String CANNOT_DELETE_CHANGES = "Cannot delete from '" + REFS_CHANGES + "'";
private static final String CANNOT_DELETE_CONFIG =
"Cannot delete project configuration from '" + RefNames.REFS_CONFIG + "'";
@@ -801,7 +798,7 @@
Boolean isPrivate = null;
Boolean wip = null;
if (!updated.isEmpty()) {
- edit = magicBranch != null && (magicBranch.edit || magicBranch.draft);
+ edit = magicBranch != null && magicBranch.edit;
if (magicBranch != null) {
if (magicBranch.isPrivate) {
isPrivate = true;
@@ -1439,13 +1436,6 @@
@Option(name = "--topic", metaVar = "NAME", usage = "attach topic to changes")
String topic;
- @Option(
- name = "--draft",
- usage =
- "Will be removed. Before that, this option will be mapped to '--private'"
- + "for new changes and '--edit' for existing changes")
- boolean draft;
-
@Option(name = "--private", usage = "mark new/updated change as private")
boolean isPrivate;
@@ -1581,7 +1571,6 @@
this.projectState = projectState;
this.deprecatedTopicSeen = false;
this.cmd = cmd;
- this.draft = cmd.getRefName().startsWith(MagicBranch.NEW_DRAFT_CHANGE);
this.labelTypes = labelTypes;
GeneralPreferencesInfo prefs = user.state().generalPreferences();
this.defaultPublishComments =
@@ -1798,14 +1787,6 @@
return;
}
- // TODO(davido): Remove legacy support for drafts magic branch option
- // after repo-tool supports private and work-in-progress changes.
- if (magicBranch.draft && !receiveConfig.allowDrafts) {
- errors.put(CODE_REVIEW_ERROR, ref);
- reject(cmd, "draft workflow is disabled");
- return;
- }
-
if (magicBranch.isPrivate && magicBranch.removePrivate) {
reject(cmd, "the options 'private' and 'remove-private' are mutually exclusive");
return;
@@ -1814,9 +1795,7 @@
boolean privateByDefault =
projectCache.get(project.getNameKey()).is(BooleanProjectConfig.PRIVATE_BY_DEFAULT);
setChangeAsPrivate =
- magicBranch.draft
- || magicBranch.isPrivate
- || (privateByDefault && !magicBranch.removePrivate);
+ magicBranch.isPrivate || (privateByDefault && !magicBranch.removePrivate);
if (receiveConfig.disablePrivateChanges && setChangeAsPrivate) {
reject(cmd, "private changes are disabled");
@@ -2662,7 +2641,7 @@
logger.atFine().log("Read %d changes to replace", replaceByChange.size());
if (magicBranch != null && magicBranch.cmd.getResult() != NOT_ATTEMPTED) {
- // Cancel creations tied to refs/for/ or refs/drafts/ command.
+ // Cancel creations tied to refs/for/ command.
for (ReplaceRequest req : replaceByChange.values()) {
if (req.inputCommand == magicBranch.cmd && req.cmd != null) {
req.cmd.setResult(Result.REJECTED_OTHER_REASON, "aborted");
@@ -2756,7 +2735,7 @@
return false;
}
- if (magicBranch.edit || magicBranch.draft) {
+ if (magicBranch.edit) {
return newEdit();
}
}
@@ -2949,7 +2928,7 @@
void addOps(BatchUpdate bu, @Nullable Task progress) throws IOException {
try (TraceTimer traceTimer = newTimer(ReplaceRequest.class, "addOps")) {
- if (magicBranch != null && (magicBranch.edit || magicBranch.draft)) {
+ if (magicBranch != null && magicBranch.edit) {
bu.addOp(notes.getChangeId(), new ReindexOnlyOp());
if (prev != null) {
bu.addRepoOnlyOp(new UpdateOneRefOp(prev));
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHook.java b/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHook.java
index 686cd81..02edc44 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHook.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHook.java
@@ -58,9 +58,6 @@
* objects that client sends to the server. Unnecessary here refers to objects that the server
* already has.
*
- * <p>For some code paths in {@link com.google.gerrit.server.git.DefaultAdvertiseRefsHook}, we
- * already removed refs/changes, so the logic to skip these in this class become a no-op.
- *
* <p>TODO(hiesel): Instrument this heuristic and proof its value.
*/
public class ReceiveCommitsAdvertiseRefsHook implements AdvertiseRefsHook {
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHookChain.java b/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHookChain.java
index d10fc9a..14654a5 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHookChain.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHookChain.java
@@ -16,9 +16,6 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.gerrit.reviewdb.client.Project;
-import com.google.gerrit.server.git.DefaultAdvertiseRefsHook;
-import com.google.gerrit.server.permissions.PermissionBackend;
-import com.google.gerrit.server.permissions.PermissionBackend.RefFilterOptions;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.inject.Provider;
import java.util.ArrayList;
@@ -37,10 +34,9 @@
*/
public static AdvertiseRefsHook create(
AllRefsWatcher allRefsWatcher,
- PermissionBackend.ForProject perm,
Provider<InternalChangeQuery> queryProvider,
Project.NameKey projectName) {
- return create(allRefsWatcher, perm, queryProvider, projectName, false);
+ return create(allRefsWatcher, queryProvider, projectName, false);
}
/**
@@ -51,22 +47,17 @@
*/
@VisibleForTesting
public static AdvertiseRefsHook createForTest(
- PermissionBackend.ForProject perm,
- Provider<InternalChangeQuery> queryProvider,
- Project.NameKey projectName) {
- return create(new AllRefsWatcher(), perm, queryProvider, projectName, true);
+ Provider<InternalChangeQuery> queryProvider, Project.NameKey projectName) {
+ return create(new AllRefsWatcher(), queryProvider, projectName, true);
}
private static AdvertiseRefsHook create(
AllRefsWatcher allRefsWatcher,
- PermissionBackend.ForProject perm,
Provider<InternalChangeQuery> queryProvider,
Project.NameKey projectName,
boolean skipHackPushNegotiateHook) {
List<AdvertiseRefsHook> advHooks = new ArrayList<>();
advHooks.add(allRefsWatcher);
- advHooks.add(
- new DefaultAdvertiseRefsHook(perm, RefFilterOptions.builder().setFilterMeta(true).build()));
advHooks.add(new ReceiveCommitsAdvertiseRefsHook(queryProvider, projectName));
if (!skipHackPushNegotiateHook) {
advHooks.add(new HackPushNegotiateHook());
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveConfig.java b/java/com/google/gerrit/server/git/receive/ReceiveConfig.java
index b1c9f13..cdbf310 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveConfig.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveConfig.java
@@ -27,7 +27,6 @@
class ReceiveConfig {
final boolean checkMagicRefs;
final boolean checkReferencedObjectsAreReachable;
- final boolean allowDrafts;
final int maxBatchCommits;
final boolean disablePrivateChanges;
private final int systemMaxBatchChanges;
@@ -38,7 +37,6 @@
checkMagicRefs = config.getBoolean("receive", null, "checkMagicRefs", true);
checkReferencedObjectsAreReachable =
config.getBoolean("receive", null, "checkReferencedObjectsAreReachable", true);
- allowDrafts = config.getBoolean("change", null, "allowDrafts", false);
maxBatchCommits = config.getInt("receive", null, "maxBatchCommits", 10000);
systemMaxBatchChanges = config.getInt("receive", "maxBatchChanges", 0);
disablePrivateChanges = config.getBoolean("change", null, "disablePrivateChanges", false);
diff --git a/java/com/google/gerrit/server/util/MagicBranch.java b/java/com/google/gerrit/server/util/MagicBranch.java
index 4e41be0..86d1ecb 100644
--- a/java/com/google/gerrit/server/util/MagicBranch.java
+++ b/java/com/google/gerrit/server/util/MagicBranch.java
@@ -26,28 +26,19 @@
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
public static final String NEW_CHANGE = "refs/for/";
- // TODO(xchangcheng): remove after 'repo' supports private/wip changes.
- public static final String NEW_DRAFT_CHANGE = "refs/drafts/";
/** Extracts the destination from a ref name */
public static String getDestBranchName(String refName) {
- String magicBranch = NEW_CHANGE;
- if (refName.startsWith(NEW_DRAFT_CHANGE)) {
- magicBranch = NEW_DRAFT_CHANGE;
- }
- return refName.substring(magicBranch.length());
+ return refName.substring(NEW_CHANGE.length());
}
/** Checks if the supplied ref name is a magic branch */
public static boolean isMagicBranch(String refName) {
- return refName.startsWith(NEW_DRAFT_CHANGE) || refName.startsWith(NEW_CHANGE);
+ return refName.startsWith(NEW_CHANGE);
}
/** Returns the ref name prefix for a magic branch, {@code null} if the branch is not magic */
public static String getMagicRefNamePrefix(String refName) {
- if (refName.startsWith(NEW_DRAFT_CHANGE)) {
- return NEW_DRAFT_CHANGE;
- }
if (refName.startsWith(NEW_CHANGE)) {
return NEW_CHANGE;
}
@@ -63,15 +54,7 @@
* branch.
*/
public static Capable checkMagicBranchRefs(Repository repo, Project project) {
- Capable result = checkMagicBranchRef(NEW_CHANGE, repo, project);
- if (result != Capable.OK) {
- return result;
- }
- result = checkMagicBranchRef(NEW_DRAFT_CHANGE, repo, project);
- if (result != Capable.OK) {
- return result;
- }
- return Capable.OK;
+ return checkMagicBranchRef(NEW_CHANGE, repo, project);
}
private static Capable checkMagicBranchRef(String branchName, Repository repo, Project project) {
diff --git a/java/com/google/gerrit/sshd/AbstractGitCommand.java b/java/com/google/gerrit/sshd/AbstractGitCommand.java
index f3ba99b..3978e4c 100644
--- a/java/com/google/gerrit/sshd/AbstractGitCommand.java
+++ b/java/com/google/gerrit/sshd/AbstractGitCommand.java
@@ -30,6 +30,8 @@
import org.kohsuke.args4j.Argument;
public abstract class AbstractGitCommand extends BaseCommand {
+ private static final String GIT_PROTOCOL = "GIT_PROTOCOL";
+
@Argument(index = 0, metaVar = "PROJECT.git", required = true, usage = "project name")
protected ProjectState projectState;
@@ -46,9 +48,15 @@
protected Repository repo;
protected Project.NameKey projectName;
protected Project project;
+ protected String[] extraParameters;
@Override
public void start(ChannelSession channel, Environment env) {
+ String gitProtocol = env.getEnv().get(GIT_PROTOCOL);
+ if (gitProtocol != null) {
+ extraParameters = gitProtocol.split(":");
+ }
+
Context ctx = context.subContext(newSession(), context.getCommandLine());
final Context old = sshScope.set(ctx);
try {
diff --git a/java/com/google/gerrit/sshd/commands/Receive.java b/java/com/google/gerrit/sshd/commands/Receive.java
index d9fd5d3..9653b65 100644
--- a/java/com/google/gerrit/sshd/commands/Receive.java
+++ b/java/com/google/gerrit/sshd/commands/Receive.java
@@ -19,10 +19,8 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.data.Capable;
import com.google.gerrit.extensions.restapi.AuthException;
-import com.google.gerrit.git.ObjectIds;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.git.DefaultAdvertiseRefsHook;
import com.google.gerrit.server.git.receive.AsyncReceiveCommits;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.permissions.PermissionBackend;
@@ -33,12 +31,8 @@
import com.google.gerrit.sshd.SshSession;
import com.google.inject.Inject;
import java.io.IOException;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Map;
import org.eclipse.jgit.errors.TooLargeObjectInPackException;
import org.eclipse.jgit.errors.UnpackException;
-import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.transport.AdvertiseRefsHook;
import org.eclipse.jgit.transport.ReceivePack;
import org.kohsuke.args4j.Option;
@@ -120,58 +114,19 @@
logger.atInfo().log(msg.toString());
throw new UnloggedFailure(128, "error: " + badStream.getCause().getMessage());
}
-
- // This may have been triggered by branch level access controls.
- // Log what the heck is going on, as detailed as we can.
- //
StringBuilder msg = new StringBuilder();
msg.append("Unpack error on project \"").append(projectState.getName()).append("\":\n");
msg.append(" AdvertiseRefsHook: ").append(rp.getAdvertiseRefsHook());
if (rp.getAdvertiseRefsHook() == AdvertiseRefsHook.DEFAULT) {
msg.append("DEFAULT");
- } else if (rp.getAdvertiseRefsHook() instanceof DefaultAdvertiseRefsHook) {
- msg.append("DefaultAdvertiseRefsHook");
} else {
msg.append(rp.getAdvertiseRefsHook().getClass());
}
msg.append("\n");
- if (rp.getAdvertiseRefsHook() instanceof DefaultAdvertiseRefsHook) {
- Map<String, Ref> adv = rp.getAdvertisedRefs();
- msg.append(" Visible references (").append(adv.size()).append("):\n");
- for (Ref ref : adv.values()) {
- msg.append(" - ")
- .append(abbreviateName(ref, rp))
- .append(" ")
- .append(ref.getName())
- .append("\n");
- }
-
- List<Ref> allRefs = rp.getRepository().getRefDatabase().getRefs();
- List<Ref> hidden = new ArrayList<>();
- for (Ref ref : allRefs) {
- if (!adv.containsKey(ref.getName())) {
- hidden.add(ref);
- }
- }
-
- msg.append(" Hidden references (").append(hidden.size()).append("):\n");
- for (Ref ref : hidden) {
- msg.append(" - ")
- .append(abbreviateName(ref, rp))
- .append(" ")
- .append(ref.getName())
- .append("\n");
- }
- }
-
IOException detail = new IOException(msg.toString(), badStream);
throw new Failure(128, "fatal: Unpack error, check server log", detail);
}
}
-
- private String abbreviateName(Ref ref, ReceivePack rp) throws IOException {
- return ObjectIds.abbreviateName(ref.getObjectId(), rp.getRevWalk().getObjectReader());
- }
}
diff --git a/java/com/google/gerrit/sshd/commands/Upload.java b/java/com/google/gerrit/sshd/commands/Upload.java
index 5a3c745..41323dd 100644
--- a/java/com/google/gerrit/sshd/commands/Upload.java
+++ b/java/com/google/gerrit/sshd/commands/Upload.java
@@ -14,12 +14,12 @@
package com.google.gerrit.sshd.commands;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.server.RequestInfo;
import com.google.gerrit.server.RequestListener;
-import com.google.gerrit.server.git.DefaultAdvertiseRefsHook;
import com.google.gerrit.server.git.PermissionAwareRepositoryManager;
import com.google.gerrit.server.git.TransferConfig;
import com.google.gerrit.server.git.UploadPackInitializer;
@@ -27,7 +27,6 @@
import com.google.gerrit.server.git.validators.UploadValidators;
import com.google.gerrit.server.logging.TraceContext;
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.permissions.ProjectPermission;
import com.google.gerrit.server.plugincontext.PluginSetContext;
@@ -59,7 +58,6 @@
PermissionBackend.ForProject perm =
permissionBackend.user(user).project(projectState.getNameKey());
try {
-
perm.check(ProjectPermission.RUN_UPLOAD_PACK);
} catch (AuthException e) {
throw new Failure(1, "fatal: upload-pack not permitted on this server");
@@ -67,17 +65,20 @@
throw new Failure(1, "fatal: unable to check permissions " + e);
}
- Repository permissionAwareRepository = PermissionAwareRepositoryManager.wrap(repo, perm);
- final UploadPack up = new UploadPack(permissionAwareRepository);
- up.setAdvertiseRefsHook(new DefaultAdvertiseRefsHook(perm, RefFilterOptions.defaults()));
+ Repository permissionAwareRepo = PermissionAwareRepositoryManager.wrap(repo, perm);
+ UploadPack up = new UploadPack(permissionAwareRepo);
+
up.setPackConfig(config.getPackConfig());
up.setTimeout(config.getTimeout());
up.setPostUploadHook(PostUploadHookChain.newChain(Lists.newArrayList(postUploadHooks)));
+ if (extraParameters != null) {
+ up.setExtraParameters(ImmutableList.copyOf(extraParameters));
+ }
List<PreUploadHook> allPreUploadHooks = Lists.newArrayList(preUploadHooks);
allPreUploadHooks.add(
uploadValidatorsFactory.create(
- project, permissionAwareRepository, session.getRemoteAddressAsString()));
+ project, permissionAwareRepo, session.getRemoteAddressAsString()));
up.setPreUploadHook(PreUploadHookChain.newChain(allPreUploadHooks));
for (UploadPackInitializer initializer : uploadPackInitializers) {
initializer.init(projectState.getNameKey(), up);
diff --git a/javatests/com/google/gerrit/acceptance/api/change/DisablePrivateChangesIT.java b/javatests/com/google/gerrit/acceptance/api/change/DisablePrivateChangesIT.java
index 57d7ece..42d62bd 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/DisablePrivateChangesIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/DisablePrivateChangesIT.java
@@ -20,16 +20,11 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GerritConfig;
import com.google.gerrit.acceptance.PushOneCommit;
-import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.extensions.common.ChangeInput;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
-import com.google.inject.Inject;
-import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Test;
public class DisablePrivateChangesIT extends AbstractDaemonTest {
- @Inject private ProjectOperations projectOperations;
-
@Test
@GerritConfig(name = "change.disablePrivateChanges", value = "true")
public void createPrivateChangeWithDisablePrivateChangesTrue() throws Exception {
@@ -63,20 +58,6 @@
}
@Test
- @GerritConfig(name = "change.allowDrafts", value = "true")
- @GerritConfig(name = "change.disablePrivateChanges", value = "true")
- public void pushDraftsWithDisablePrivateChangesTrue() throws Exception {
- RevCommit initialHead = projectOperations.project(project).getHead("master");
- PushOneCommit.Result result =
- pushFactory.create(admin.newIdent(), testRepo).to("refs/for/master%draft");
- result.assertErrorStatus();
-
- testRepo.reset(initialHead);
- result = pushFactory.create(admin.newIdent(), testRepo).to("refs/drafts/master");
- result.assertErrorStatus();
- }
-
- @Test
@GerritConfig(name = "change.disablePrivateChanges", value = "true")
public void pushWithDisablePrivateChangesTrue() throws Exception {
PushOneCommit.Result result =
@@ -86,27 +67,6 @@
}
@Test
- @GerritConfig(name = "change.allowDrafts", value = "true")
- public void pushPrivatesWithDisablePrivateChangesFalse() throws Exception {
- PushOneCommit.Result result =
- pushFactory.create(admin.newIdent(), testRepo).to("refs/for/master%private");
- assertThat(result.getChange().change().isPrivate()).isTrue();
- }
-
- @Test
- @GerritConfig(name = "change.allowDrafts", value = "true")
- public void pushDraftsWithDisablePrivateChangesFalse() throws Exception {
- RevCommit initialHead = projectOperations.project(project).getHead("master");
- PushOneCommit.Result result =
- pushFactory.create(admin.newIdent(), testRepo).to("refs/for/master%draft");
- assertThat(result.getChange().change().isPrivate()).isTrue();
-
- testRepo.reset(initialHead);
- result = pushFactory.create(admin.newIdent(), testRepo).to("refs/drafts/master");
- assertThat(result.getChange().change().isPrivate()).isTrue();
- }
-
- @Test
@GerritConfig(name = "change.disablePrivateChanges", value = "true")
public void setPrivateWithDisablePrivateChangesTrue() throws Exception {
PushOneCommit.Result result = createChange();
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index 564a29b..e8ab515 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -53,7 +53,6 @@
import com.google.gerrit.acceptance.GerritConfig;
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.PushOneCommit;
-import com.google.gerrit.acceptance.Sandboxed;
import com.google.gerrit.acceptance.SkipProjectClone;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestProjectInput;
@@ -83,7 +82,6 @@
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.extensions.common.LabelInfo;
import com.google.gerrit.extensions.common.RevisionInfo;
-import com.google.gerrit.extensions.common.testing.EditInfoSubject;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.registration.RegistrationHandle;
import com.google.gerrit.git.ObjectIds;
@@ -2199,53 +2197,6 @@
amendChanges(unChanged.toObjectId(), commits, "refs/for/master%publish-comments");
}
- @Test
- public void pushWithDraftOptionIsDisabledPerDefault() throws Exception {
- for (String ref : ImmutableSet.of("refs/drafts/master", "refs/for/master%draft")) {
- PushOneCommit.Result r = pushTo(ref);
- r.assertErrorStatus();
- r.assertMessage("draft workflow is disabled");
- }
- }
-
- @GerritConfig(name = "change.allowDrafts", value = "true")
- @Test
- public void pushDraftGetsPrivateChange() throws Exception {
- String changeId1 = createChange("refs/drafts/master").getChangeId();
- String changeId2 = createChange("refs/for/master%draft").getChangeId();
-
- ChangeInfo info1 = gApi.changes().id(changeId1).get();
- ChangeInfo info2 = gApi.changes().id(changeId2).get();
-
- assertThat(info1.status).isEqualTo(ChangeStatus.NEW);
- assertThat(info2.status).isEqualTo(ChangeStatus.NEW);
- assertThat(info1.isPrivate).isTrue();
- assertThat(info2.isPrivate).isTrue();
- assertThat(info1.revisions).hasSize(1);
- assertThat(info2.revisions).hasSize(1);
- }
-
- @GerritConfig(name = "change.allowDrafts", value = "true")
- @Sandboxed
- @Test
- public void pushWithDraftOptionToExistingNewChangeGetsChangeEdit() throws Exception {
- String changeId = createChange().getChangeId();
- EditInfoSubject.assertThat(getEdit(changeId)).isAbsent();
-
- ChangeInfo changeInfo = gApi.changes().id(changeId).get();
- ChangeStatus originalChangeStatus = changeInfo.status;
-
- PushOneCommit.Result result = amendChange(changeId, "refs/drafts/master");
- result.assertOkStatus();
-
- changeInfo = gApi.changes().id(changeId).get();
- assertThat(changeInfo.status).isEqualTo(originalChangeStatus);
- assertThat(changeInfo.isPrivate).isNull();
- assertThat(changeInfo.revisions).hasSize(1);
-
- EditInfoSubject.assertThat(getEdit(changeId)).isPresent();
- }
-
@GerritConfig(name = "receive.maxBatchCommits", value = "2")
@Test
public void maxBatchCommits() throws Exception {
diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
index 88fc557..f2d0a1c 100644
--- a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
+++ b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
@@ -1027,7 +1027,7 @@
@Test
public void receivePackListsOpenChangesAsAdditionalHaves() throws Exception {
- TestRefAdvertiser.Result r = getReceivePackRefs(admin);
+ TestRefAdvertiser.Result r = getReceivePackRefs();
assertThat(r.allRefs().keySet())
.containsExactly(
// meta refs are excluded
@@ -1050,7 +1050,7 @@
.update();
requestScopeOperations.setApiUser(user.id());
- assertThat(getReceivePackRefs(user).additionalHaves()).containsExactly(obj(cd3, 1));
+ assertThat(getReceivePackRefs().additionalHaves()).containsExactly(obj(cd3, 1));
}
@Test
@@ -1059,8 +1059,7 @@
PushOneCommit.Result r = amendChange(cd3.change().getKey().get());
r.assertOkStatus();
cd3 = r.getChange();
- assertThat(getReceivePackRefs(admin).additionalHaves())
- .containsExactly(obj(cd3, 2), obj(cd4, 1));
+ assertThat(getReceivePackRefs().additionalHaves()).containsExactly(obj(cd3, 2), obj(cd4, 1));
}
@Test
@@ -1098,7 +1097,7 @@
indexer.index(c.getProject(), c.getId());
}
- assertThat(getReceivePackRefs(admin).additionalHaves()).containsExactly(obj(cd4, 1));
+ assertThat(getReceivePackRefs().additionalHaves()).containsExactly(obj(cd4, 1));
}
@Test
@@ -1444,11 +1443,10 @@
}
}
- private TestRefAdvertiser.Result getReceivePackRefs(TestAccount u) throws Exception {
+ private TestRefAdvertiser.Result getReceivePackRefs() throws Exception {
try (Repository repo = repoManager.openRepository(project)) {
AdvertiseRefsHook adv =
- ReceiveCommitsAdvertiseRefsHookChain.createForTest(
- newFilter(project, u), queryProvider, project);
+ ReceiveCommitsAdvertiseRefsHookChain.createForTest(queryProvider, project);
ReceivePack rp = new ReceivePack(repo);
rp.setAdvertiseRefsHook(adv);
TestRefAdvertiser advertiser = new TestRefAdvertiser(repo);
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/PrivateByDefaultIT.java b/javatests/com/google/gerrit/acceptance/rest/change/PrivateByDefaultIT.java
index a6fa9fc5..92e00aa 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/PrivateByDefaultIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/PrivateByDefaultIT.java
@@ -30,7 +30,6 @@
import com.google.inject.Inject;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
-import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Before;
import org.junit.Test;
@@ -126,22 +125,6 @@
result.assertErrorStatus();
}
- @Test
- @GerritConfig(name = "change.disablePrivateChanges", value = "true")
- public void pushDraftsWithPrivateByDefaultAndDisablePrivateChangesTrue() throws Exception {
- setPrivateByDefault(project2, InheritableBoolean.TRUE);
-
- RevCommit initialHead = projectOperations.project(project2).getHead("master");
- TestRepository<InMemoryRepository> testRepo = cloneProject(project2);
- PushOneCommit.Result result =
- pushFactory.create(admin.newIdent(), testRepo).to("refs/for/master%draft");
- result.assertErrorStatus();
-
- testRepo.reset(initialHead);
- result = pushFactory.create(admin.newIdent(), testRepo).to("refs/drafts/master");
- result.assertErrorStatus();
- }
-
private void setPrivateByDefault(Project.NameKey proj, InheritableBoolean value)
throws Exception {
ConfigInput input = new ConfigInput();
diff --git a/javatests/com/google/gerrit/server/permissions/RefControlTest.java b/javatests/com/google/gerrit/server/permissions/RefControlTest.java
index 2869743..68d3821 100644
--- a/javatests/com/google/gerrit/server/permissions/RefControlTest.java
+++ b/javatests/com/google/gerrit/server/permissions/RefControlTest.java
@@ -334,50 +334,6 @@
}
@Test
- public void blockPushDrafts() throws Exception {
- projectOperations
- .project(parentKey)
- .forUpdate()
- .add(allow(PUSH).ref("refs/for/refs/*").group(REGISTERED_USERS))
- .add(block(PUSH).ref("refs/drafts/*").group(ANONYMOUS_USERS))
- .update();
- projectOperations
- .project(localKey)
- .forUpdate()
- .add(allow(PUSH).ref("refs/drafts/*").group(REGISTERED_USERS))
- .update();
-
- ProjectControl u = user(localKey);
- assertCreateChange("refs/heads/master", u);
- assertThat(u.controlForRef("refs/drafts/master").canPerform(PUSH)).isFalse();
- }
-
- @Test
- public void blockPushDraftsUnblockAdmin() throws Exception {
- projectOperations
- .project(parentKey)
- .forUpdate()
- .add(block(PUSH).ref("refs/drafts/*").group(ANONYMOUS_USERS))
- .add(allow(PUSH).ref("refs/drafts/*").group(ADMIN))
- .update();
- projectOperations
- .project(localKey)
- .forUpdate()
- .add(allow(PUSH).ref("refs/drafts/*").group(REGISTERED_USERS))
- .update();
-
- ProjectControl u = user(localKey);
- ProjectControl a = user(localKey, "a", ADMIN);
-
- assertWithMessage("push is allowed")
- .that(a.controlForRef("refs/drafts/master").canPerform(PUSH))
- .isTrue();
- assertWithMessage("push is not allowed")
- .that(u.controlForRef("refs/drafts/master").canPerform(PUSH))
- .isFalse();
- }
-
- @Test
public void inheritRead_SingleBranchDoesNotOverrideInherited() throws Exception {
projectOperations
.project(parentKey)
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
index f29a5da..780301b 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html
@@ -20,7 +20,6 @@
<link rel="import" href="../gr-coverage-layer/gr-coverage-layer.html">
<link rel="import" href="../gr-diff-processor/gr-diff-processor.html">
<link rel="import" href="../gr-ranged-comment-layer/gr-ranged-comment-layer.html">
-<link rel="import" href="../gr-syntax-layer/gr-syntax-layer.html">
<dom-module id="gr-diff-builder">
<template>
@@ -30,9 +29,6 @@
<gr-ranged-comment-layer
id="rangeLayer"
comment-ranges="[[commentRanges]]"></gr-ranged-comment-layer>
- <gr-syntax-layer
- id="syntaxLayer"
- diff="[[diff]]"></gr-syntax-layer>
<gr-coverage-layer
id="coverageLayerLeft"
coverage-ranges="[[_leftCoverageRanges]]"
@@ -64,13 +60,6 @@
UNIFIED: 'UNIFIED_DIFF',
};
- // If any line of the diff is more than the character limit, then disable
- // syntax highlighting for the entire file.
- const SYNTAX_MAX_LINE_LENGTH = 500;
-
- // Disable syntax highlighting if the overall diff is too large.
- const SYNTAX_MAX_DIFF_LENGTH = 20000;
-
const TRAILING_WHITESPACE_PATTERN = /\s+$/;
Polymer({
@@ -84,18 +73,11 @@
*/
/**
- * Fired when the diff finishes rendering text content and starts
- * syntax highlighting.
+ * Fired when the diff finishes rendering text content.
*
* @event render-content
*/
- /**
- * Fired when the diff finishes syntax highlighting.
- *
- * @event render-syntax
- */
-
properties: {
diff: Object,
diffPath: String,
@@ -138,7 +120,7 @@
* @type {?Object}
*/
_cancelableRenderPromise: Object,
- pluginLayers: {
+ layers: {
type: Array,
value: [],
},
@@ -171,11 +153,10 @@
// attached before plugins are installed.
this._setupAnnotationLayers();
- this.$.syntaxLayer.enabled = prefs.syntax_highlighting;
this._showTabs = !!prefs.show_tabs;
this._showTrailingWhitespace = !!prefs.show_whitespace_errors;
- // Stop the processor and syntax layer (if they're running).
+ // Stop the processor if it's running.
this.cancel();
this._builder = this._getDiffBuilder(this.diff, prefs);
@@ -198,16 +179,6 @@
}
this.dispatchEvent(new CustomEvent('render-content',
{bubbles: true, composed: true}));
-
- if (this._diffTooLargeForSyntax()) {
- this.$.syntaxLayer.enabled = false;
- }
-
- return this.$.syntaxLayer.process();
- })
- .then(() => {
- this.dispatchEvent(new CustomEvent(
- 'render-syntax', {bubbles: true, composed: true}));
}));
return this._cancelableRenderPromise
.finally(() => { this._cancelableRenderPromise = null; })
@@ -220,7 +191,6 @@
_setupAnnotationLayers() {
const layers = [
this._createTrailingWhitespaceLayer(),
- this.$.syntaxLayer,
this._createIntralineLayer(),
this._createTabIndicatorLayer(),
this.$.rangeLayer,
@@ -228,8 +198,8 @@
this.$.coverageLayerRight,
];
- if (this.pluginLayers) {
- layers.push(...this.pluginLayers);
+ if (this.layers) {
+ layers.push(...this.layers);
}
this._layers = layers;
},
@@ -307,7 +277,6 @@
cancel() {
this.$.processor.cancel();
- this.$.syntaxLayer.cancel();
if (this._cancelableRenderPromise) {
this._cancelableRenderPromise.cancel();
this._cancelableRenderPromise = null;
@@ -446,44 +415,10 @@
};
},
- /**
- * @return {boolean} whether any of the lines in _groups are longer
- * than SYNTAX_MAX_LINE_LENGTH.
- */
- _anyLineTooLong() {
- return this._groups.reduce((acc, group) => {
- return acc || group.lines.reduce((acc, line) => {
- return acc || line.text.length >= SYNTAX_MAX_LINE_LENGTH;
- }, false);
- }, false);
- },
-
- _diffTooLargeForSyntax() {
- return this._anyLineTooLong() ||
- this.getDiffLength() > SYNTAX_MAX_DIFF_LENGTH;
- },
-
setBlame(blame) {
if (!this._builder || !blame) { return; }
this._builder.setBlame(blame);
},
-
- /**
- * Get the approximate length of the diff as the sum of the maximum
- * length of the chunks.
- * @return {number}
- */
- getDiffLength() {
- return this.diff.content.reduce((sum, sec) => {
- if (sec.hasOwnProperty('ab')) {
- return sum + sec.ab.length;
- } else {
- return sum + Math.max(
- sec.hasOwnProperty('a') ? sec.a.length : 0,
- sec.hasOwnProperty('b') ? sec.b.length : 0);
- }
- }, 0);
- },
});
})();
</script>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
index 6831a8d..45de810 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder_test.html
@@ -590,38 +590,38 @@
});
});
- suite('layers from plugins', () => {
+ suite('layers', () => {
let element;
let initialLayersCount;
- let withPluginLayerCount;
+ let withLayerCount;
setup(() => {
- const pluginLayers = [];
+ const layers = [];
element = fixture('basic');
- element.pluginLayers = pluginLayers;
+ element.layers = layers;
element._showTrailingWhitespace = true;
element._setupAnnotationLayers();
initialLayersCount = element._layers.length;
});
- test('no plugin layers', () => {
+ test('no layers', () => {
element._setupAnnotationLayers();
assert.equal(element._layers.length, initialLayersCount);
});
- suite('with plugin layers', () => {
- const pluginLayers = [{}, {}];
+ suite('with layers', () => {
+ const layers = [{}, {}];
setup(() => {
element = fixture('basic');
- element.pluginLayers = pluginLayers;
+ element.layers = layers;
element._showTrailingWhitespace = true;
element._setupAnnotationLayers();
- withPluginLayerCount = element._layers.length;
+ withLayerCount = element._layers.length;
});
- test('with plugin layers', () => {
+ test('with layers', () => {
element._setupAnnotationLayers();
- assert.equal(element._layers.length, withPluginLayerCount);
- assert.equal(initialLayersCount + pluginLayers.length,
- withPluginLayerCount);
+ assert.equal(element._layers.length, withLayerCount);
+ assert.equal(initialLayersCount + layers.length,
+ withLayerCount);
});
});
});
@@ -733,7 +733,6 @@
element.viewMode = 'SIDE_BY_SIDE';
processStub = sandbox.stub(element.$.processor, 'process')
.returns(Promise.resolve());
- sandbox.stub(element, '_anyLineTooLong').returns(true);
keyLocations = {left: {}, right: {}};
prefs = {
line_length: 10,
@@ -862,37 +861,14 @@
.map(c => { return c.args[0].type; });
assert.include(firedEventTypes, 'render-start');
assert.include(firedEventTypes, 'render-content');
- assert.include(firedEventTypes, 'render-syntax');
- done();
- });
- });
-
- test('rendering normal-sized diff does not disable syntax', () => {
- assert.isTrue(element.$.syntaxLayer.enabled);
- });
-
- test('rendering large diff disables syntax', done => {
- // Before it renders, set the first diff line to 500 '*' characters.
- element.diff.content[0].a = [new Array(501).join('*')];
- const prefs = {
- line_length: 10,
- show_tabs: true,
- tab_size: 4,
- context: -1,
- syntax_highlighting: true,
- };
- element.render(keyLocations, prefs).then(() => {
- assert.isFalse(element.$.syntaxLayer.enabled);
done();
});
});
test('cancel', () => {
const processorCancelStub = sandbox.stub(element.$.processor, 'cancel');
- const syntaxCancelStub = sandbox.stub(element.$.syntaxLayer, 'cancel');
element.cancel();
assert.isTrue(processorCancelStub.called);
- assert.isTrue(syntaxCancelStub.called);
});
});
@@ -921,10 +897,6 @@
});
});
- test('getDiffLength', () => {
- assert.equal(element.getDiffLength(diff), 52);
- });
-
test('getContentByLine', () => {
let actual;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html
index b70764b..7d60f13 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html
@@ -23,6 +23,7 @@
<link rel="import" href="../../shared/gr-comment-thread/gr-comment-thread.html">
<link rel="import" href="../../shared/gr-js-api-interface/gr-js-api-interface.html">
<link rel="import" href="../gr-diff/gr-diff.html">
+<link rel="import" href="../gr-syntax-layer/gr-syntax-layer.html">
<dom-module id="gr-diff-host">
<template>
@@ -49,8 +50,13 @@
revision-image=[[_revisionImage]]
coverage-ranges="[[_coverageRanges]]"
blame="[[_blame]]"
- plugin-layers="[[pluginLayers]]"
- diff="[[diff]]"></gr-diff>
+ layers="[[_layers]]"
+ diff="[[diff]]">
+ </gr-diff>
+ <gr-syntax-layer
+ id="syntaxLayer"
+ enabled="[[_syntaxHighlightingEnabled]]"
+ diff="[[diff]]"></gr-syntax-layer>
<gr-js-api-interface id="jsAPI"></gr-js-api-interface>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
<gr-reporting id="reporting" category="diff"></gr-reporting>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
index e2dec69..a538dcf 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js
@@ -35,6 +35,13 @@
SYNTAX: 'Diff Syntax Render',
};
+ // Disable syntax highlighting if the overall diff is too large.
+ const SYNTAX_MAX_DIFF_LENGTH = 20000;
+
+ // If any line of the diff is more than the character limit, then disable
+ // syntax highlighting for the entire file.
+ const SYNTAX_MAX_LINE_LENGTH = 500;
+
const WHITESPACE_IGNORE_NONE = 'IGNORE_NONE';
/**
@@ -210,7 +217,13 @@
computed: '_computeParentIndex(patchRange.*)',
},
- pluginLayers: {
+ _syntaxHighlightingEnabled: {
+ type: Boolean,
+ computed:
+ '_isSyntaxHighlightingEnabled(prefs.syntax_highlighting, _diff)',
+ },
+
+ _layers: {
type: Array,
value: [],
},
@@ -235,7 +248,6 @@
'render-start': '_handleRenderStart',
'render-content': '_handleRenderContent',
- 'render-syntax': '_handleRenderSyntax',
'normalize-range': '_handleNormalizeRange',
},
@@ -263,13 +275,13 @@
this._errorMessage = null;
const whitespaceLevel = this._getIgnoreWhitespace();
- const pluginLayers = [];
+ const layers = [this.$.syntaxLayer];
// Get layers from plugins (if any).
for (const pluginLayer of this.$.jsAPI.getDiffLayers(
this.diffPath, this.changeNum, this.patchNum)) {
- pluginLayers.push(pluginLayer);
+ layers.push(pluginLayer);
}
- this.push('pluginLayers', ...pluginLayers);
+ this._layers = layers;
this._coverageRanges = [];
const {changeNum, path, patchRange: {basePatchNum, patchNum}} = this;
@@ -312,8 +324,20 @@
}
this.filesWeblinks = this._getFilesWeblinks(diff);
return new Promise(resolve => {
- const callback = () => {
- resolve();
+ const callback = event => {
+ const needsSyntaxHighlighting = event.detail
+ && event.detail.contentRendered;
+ if (needsSyntaxHighlighting) {
+ this.$.reporting.time(TimingLabel.SYNTAX);
+ this.$.syntaxLayer.process().then(() => {
+ this.$.reporting.timeEnd(TimingLabel.SYNTAX);
+ this.$.reporting.timeEnd(TimingLabel.TOTAL);
+ resolve();
+ });
+ } else {
+ this.$.reporting.timeEnd(TimingLabel.TOTAL);
+ resolve();
+ }
this.removeEventListener('render', callback);
};
this.addEventListener('render', callback);
@@ -856,6 +880,25 @@
item => item.__draftID === comment.__draftID);
},
+ _isSyntaxHighlightingEnabled(preference, diff) {
+ if (!preference) return false;
+ return !this._anyLineTooLong(diff) &&
+ this.$.diff.getDiffLength(diff) <= SYNTAX_MAX_DIFF_LENGTH;
+ },
+
+ /**
+ * @return {boolean} whether any of the lines in diff are longer
+ * than SYNTAX_MAX_LINE_LENGTH.
+ */
+ _anyLineTooLong(diff) {
+ return diff.content.some(section => {
+ const lines = section.ab ?
+ section.ab :
+ (section.a || []).concat(section.b || []);
+ return lines.some(line => line.length >= SYNTAX_MAX_LINE_LENGTH);
+ });
+ },
+
_handleRenderStart() {
this.$.reporting.time(TimingLabel.TOTAL);
this.$.reporting.time(TimingLabel.CONTENT);
@@ -863,12 +906,6 @@
_handleRenderContent() {
this.$.reporting.timeEnd(TimingLabel.CONTENT);
- this.$.reporting.time(TimingLabel.SYNTAX);
- },
-
- _handleRenderSyntax() {
- this.$.reporting.timeEnd(TimingLabel.SYNTAX);
- this.$.reporting.timeEnd(TimingLabel.TOTAL);
},
_handleNormalizeRange(event) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html
index f87ef7a..58edfdb 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.html
@@ -60,7 +60,7 @@
suite('plugin layers', () => {
- const pluginLayers = [{}, {}];
+ const pluginLayers = [{annotate: () => {}}, {annotate: () => {}}];
setup(() => {
stub('gr-js-api-interface', {
getDiffLayers() { return pluginLayers; },
@@ -302,24 +302,81 @@
done();
});
- test('ends content and starts syntax timer on render-content', done => {
+ test('ends content timer on render-content', () => {
element.dispatchEvent(
new CustomEvent('render-content', {bubbles: true, composed: true}));
- assert.isTrue(element.$.reporting.time.calledWithExactly(
- 'Diff Syntax Render'));
assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
'Diff Content Render'));
- done();
});
- test('ends total and syntax timer on render-syntax', done => {
- element.dispatchEvent(
- new CustomEvent('render-syntax', {bubbles: true, composed: true}));
- assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
- 'Diff Total Render'));
- assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
- 'Diff Syntax Render'));
- done();
+ test('ends total and syntax timer after syntax layer processing', done => {
+ let notifySyntaxProcessed;
+ sandbox.stub(element.$.syntaxLayer, 'process').returns(new Promise(
+ resolve => {
+ notifySyntaxProcessed = resolve;
+ }));
+ sandbox.stub(element.$.restAPI, 'getDiff').returns(
+ Promise.resolve({content: []}));
+ element.patchRange = {};
+ element.$.restAPI.getDiffPreferences().then(prefs => {
+ element.prefs = prefs;
+ return element.reload();
+ });
+ // Multiple cascading microtasks are scheduled.
+ setTimeout(() => {
+ notifySyntaxProcessed();
+ // Assert after the notification task is processed.
+ Promise.resolve().then(() => {
+ assert.isTrue(element.$.reporting.timeEnd.calledThrice);
+ assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
+ 'Diff Total Render'));
+ assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
+ 'Diff Syntax Render'));
+ done();
+ });
+ });
+ });
+
+ test('ends total timer w/ no syntax layer processing', done => {
+ sandbox.stub(element.$.restAPI, 'getDiff').returns(
+ Promise.resolve({content: []}));
+ element.patchRange = {};
+ element.reload();
+ // Multiple cascading microtasks are scheduled.
+ setTimeout(() => {
+ assert.isTrue(element.$.reporting.timeEnd.calledOnce);
+ assert.isTrue(element.$.reporting.timeEnd.calledWithExactly(
+ 'Diff Total Render'));
+ done();
+ });
+ });
+
+ test('completes reload promise after syntax layer processing', done => {
+ let notifySyntaxProcessed;
+ sandbox.stub(element.$.syntaxLayer, 'process').returns(new Promise(
+ resolve => {
+ notifySyntaxProcessed = resolve;
+ }));
+ sandbox.stub(element.$.restAPI, 'getDiff').returns(
+ Promise.resolve({content: []}));
+ element.patchRange = {};
+ let reloadComplete = false;
+ element.$.restAPI.getDiffPreferences().then(prefs => {
+ element.prefs = prefs;
+ return element.reload();
+ }).then(() => {
+ reloadComplete = true;
+ });
+ // Multiple cascading microtasks are scheduled.
+ setTimeout(() => {
+ assert.isFalse(reloadComplete);
+ notifySyntaxProcessed();
+ // Assert after the notification task is processed.
+ setTimeout(() => {
+ assert.isTrue(reloadComplete);
+ done();
+ });
+ });
});
});
@@ -1285,5 +1342,57 @@
assert.deepEqual(element._filterThreadElsForLocation(threadEls, line,
Gerrit.DiffSide.RIGHT), [r]);
});
+
+ suite('syntax layer', () => {
+ setup(() => {
+ const prefs = {
+ line_length: 10,
+ show_tabs: true,
+ tab_size: 4,
+ context: -1,
+ syntax_highlighting: true,
+ };
+ element.prefs = prefs;
+ });
+
+ test('gr-diff-host provides syntax highlighting layer to gr-diff', () => {
+ element.patchRange = {};
+ element.reload();
+ assert.equal(element.$.diff.layers[0], element.$.syntaxLayer);
+ });
+
+ test('rendering normal-sized diff does not disable syntax', () => {
+ element._diff = {
+ content: [{
+ a: ['foo'],
+ }],
+ };
+ assert.isTrue(element.$.syntaxLayer.enabled);
+ });
+
+ test('rendering large diff disables syntax', () => {
+ // Before it renders, set the first diff line to 500 '*' characters.
+ element._diff = {
+ content: [{
+ a: [new Array(501).join('*')],
+ }],
+ };
+ assert.isFalse(element.$.syntaxLayer.enabled);
+ });
+
+ test('starts syntax layer processing on render event', done => {
+ sandbox.stub(element.$.syntaxLayer, 'process').returns(Promise.resolve());
+ sandbox.stub(element.$.restAPI, 'getDiff').returns(
+ Promise.resolve({content: []}));
+ element.patchRange = {};
+ element.reload();
+ setTimeout(() => {
+ element.dispatchEvent(
+ new CustomEvent('render', {bubbles: true, composed: true}));
+ assert.isTrue(element.$.syntaxLayer.process.called);
+ done();
+ });
+ });
+ });
});
</script>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
index 32fa7e5..374368c 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html
@@ -378,7 +378,7 @@
line-wrapping="[[lineWrapping]]"
is-image-diff="[[isImageDiff]]"
base-image="[[baseImage]]"
- plugin-layers="[[pluginLayers]]"
+ layers="[[layers]]"
revision-image="[[revisionImage]]">
<table
id="diffTable"
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
index 16c92b1..7606928 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js
@@ -271,7 +271,7 @@
/** Set by Polymer. */
isAttached: Boolean,
- pluginLayers: Array,
+ layers: Array,
},
behaviors: [
@@ -724,7 +724,7 @@
_diffChanged(newValue) {
if (newValue) {
- this._diffLength = this.$.diffBuilder.getDiffLength();
+ this._diffLength = this.getDiffLength(newValue);
this._debounceRenderDiffTable();
}
},
@@ -766,7 +766,11 @@
this.$.diffBuilder.render(keyLocations, this._getBypassPrefs())
.then(() => {
this.dispatchEvent(
- new CustomEvent('render', {bubbles: true, composed: true}));
+ new CustomEvent('render', {
+ bubbles: true,
+ composed: true,
+ detail: {contentRendered: true},
+ }));
});
},
@@ -955,5 +959,23 @@
if (loading || !warning) { return 'newlineWarning hidden'; }
return 'newlineWarning';
},
+
+ /**
+ * Get the approximate length of the diff as the sum of the maximum
+ * length of the chunks.
+ * @param {Object} diff object
+ * @return {number}
+ */
+ getDiffLength(diff) {
+ return diff.content.reduce((sum, sec) => {
+ if (sec.hasOwnProperty('ab')) {
+ return sum + sec.ab.length;
+ } else {
+ return sum + Math.max(
+ sec.hasOwnProperty('a') ? sec.a.length : 0,
+ sec.hasOwnProperty('b') ? sec.b.length : 0);
+ }
+ }, 0);
+ },
});
})();
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
index 15deaff..5366664 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.html
@@ -767,7 +767,7 @@
new CustomEvent('render', {bubbles: true, composed: true}));
});
const mock = document.createElement('mock-diff-response');
- sandbox.stub(element.$.diffBuilder, 'getDiffLength').returns(10000);
+ sandbox.stub(element, 'getDiffLength').returns(10000);
element.diff = mock.diffResponse;
element.noRenderOnPrefsChange = true;
});
@@ -1101,6 +1101,23 @@
));
});
});
+
+ test('getDiffLength', () => {
+ const diff = document.createElement('mock-diff-response').diffResponse;
+ assert.equal(element.getDiffLength(diff), 52);
+ });
+
+ test('`render` event has contentRendered field in detail', done => {
+ element = fixture('basic');
+ element.prefs = {};
+ renderStub = sandbox.stub(element.$.diffBuilder, 'render')
+ .returns(Promise.resolve());
+ element.addEventListener('render', event => {
+ assert.isTrue(event.detail.contentRendered);
+ done();
+ });
+ element._renderDiffTable();
+ });
});
a11ySuite('basic');
diff --git a/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js b/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js
index 627a279..fcaa696 100644
--- a/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js
+++ b/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js
@@ -225,7 +225,7 @@
process() {
// Cancel any still running process() calls, because they append to the
// same _baseRanges and _revisionRanges fields.
- this.cancel();
+ this._cancel();
// Discard existing ranges.
this._baseRanges = [];
@@ -295,7 +295,7 @@
/**
* Cancel any asynchronous syntax processing jobs.
*/
- cancel() {
+ _cancel() {
if (this._processHandle != null) {
this.cancelAsync(this._processHandle);
this._processHandle = null;
@@ -306,7 +306,7 @@
},
_diffChanged() {
- this.cancel();
+ this._cancel();
this._baseRanges = [];
this._revisionRanges = [];
},
