Merge "groups_bysubgroups: Batch loading values"
diff --git a/Documentation/config-accounts.txt b/Documentation/config-accounts.txt
index 716fa2f..f46c821 100644
--- a/Documentation/config-accounts.txt
+++ b/Documentation/config-accounts.txt
@@ -380,6 +380,14 @@
log in to Gerrit, then Gerrit will update the external ID record with
the new email address.
+=== Transition from LDAP to Google OAuth
+
+When authentication is changed from LDAP to Google Oauth gerrit will automatically
+adjust the external IDs in the `refs/meta/external-ids` branch. Gerrit will re-use
+the same account ID that was used by the LDAP account. Transition to other OAuth
+mechanisms will fail and require manual changes to the `refs/meta/external-ids` branch.
+The LDAP e-mail and Google OAuth e-mail must be the same.
+
[[starred-changes]]
== Starred Changes
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 0162c48..8586840 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -3012,7 +3012,7 @@
'GET /changes/link:#change-id[\{change-id\}]/edit
--
-Retrieves a change edit details.
+Retrieves the details of the change edit done by the caller to the given change.
.Request
----
@@ -6751,7 +6751,8 @@
|`patch` |required|
The details of the patch to be applied as a link:#applypatch-input[ApplyPatchInput] entity.
|`commit_message` |optional|
-The commit message for the new patch set. If not specified, a predefined message will be used.
+The commit message for the new patch set. If not specified, the latest patch-set message will be
+used.
|`base` |optional|
40-hex digit SHA-1 of the commit which will be the parent commit of the newly created patch set.
If set, it must be a merged commit or a change revision on the destination branch.
@@ -8139,9 +8140,7 @@
Rebasing on behalf of the uploader is only supported for trivial rebases.
This means this option cannot be combined with the `allow_conflicts` option. +
In addition, rebasing on behalf of the uploader is only supported for the
-current patch set of a change and not when rebasing a chain. +
-Using this option is not supported when rebasing a chain via the
-link:#rebase-chain[Rebase Chain] REST endpoint. +
+current patch set of a change. +
If the caller is the uploader this flag is ignored and a normal rebase is done.
|`validation_options` |optional|
Map with key-value pairs that are forwarded as options to the commit validation
diff --git a/Documentation/user-inline-edit.txt b/Documentation/user-inline-edit.txt
index 4a9d18f..e3e0f68 100644
--- a/Documentation/user-inline-edit.txt
+++ b/Documentation/user-inline-edit.txt
@@ -173,7 +173,7 @@
[[search-for-changes]]
== Searching for Changes with Pending Edits
-To find changes with pending edits:
+To find changes with pending edits created by you:
* From the Gerrit dashboard, select Your > Changes. All your changes are
listed, according to Work in progress, Outgoing reviews, Incoming reviews,
@@ -183,6 +183,12 @@
link:user-search.html[Searching Changes]. For example, to find only
those changes that contain edits, see link:user-search.html#has[has:edit].
+[NOTE]
+Though edits created by others are not accessible from the Gerrit UI, edits
+are not considered to be private data, and are stored in non-encrypted special
+branches under the target repository. As such, they can be accessed by users who
+have access to the repository.
+
[[change-edit-actions]]
== Modifying Changes
diff --git a/java/com/google/gerrit/server/account/AccountDelta.java b/java/com/google/gerrit/server/account/AccountDelta.java
index fac3233..8f285b5 100644
--- a/java/com/google/gerrit/server/account/AccountDelta.java
+++ b/java/com/google/gerrit/server/account/AccountDelta.java
@@ -161,6 +161,11 @@
*/
public abstract Optional<EditPreferencesInfo> getEditPreferences();
+ public boolean hasExternalIdUpdates() {
+ return !this.getCreatedExternalIds().isEmpty()
+ || !this.getDeletedExternalIds().isEmpty()
+ || !this.getUpdatedExternalIds().isEmpty();
+ }
/**
* Class to build an {@link AccountDelta}.
*
diff --git a/java/com/google/gerrit/server/account/AccountManager.java b/java/com/google/gerrit/server/account/AccountManager.java
index 891a467..edec52c 100644
--- a/java/com/google/gerrit/server/account/AccountManager.java
+++ b/java/com/google/gerrit/server/account/AccountManager.java
@@ -16,6 +16,8 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_GERRIT;
+import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_GOOGLE_OAUTH;
import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_USERNAME;
import com.google.common.annotations.VisibleForTesting;
@@ -146,10 +148,7 @@
try {
Optional<ExternalId> optionalExtId = externalIds.get(who.getExternalIdKey());
if (!optionalExtId.isPresent()) {
- logger.atFine().log(
- "External ID for account %s not found. A new account will be automatically created.",
- who.getUserName());
- return create(who);
+ return createOrLinkAccount(who);
}
ExternalId extId = optionalExtId.get();
@@ -180,6 +179,46 @@
}
}
+ /**
+ * Determines if a new account should be created or if we should link to an existing account.
+ *
+ * @param who identity of the user, with any details we received about them.
+ * @return the result of authenticating the user.
+ * @throws AccountException the account does not exist, and cannot be created, or exists, but
+ * cannot be located, is unable to be activated or deactivated, or is inactive, or cannot be
+ * added to the admin group (only for the first account).
+ */
+ private AuthResult createOrLinkAccount(AuthRequest who)
+ throws AccountException, IOException, ConfigInvalidException {
+ // TODO: in case of extension of further migration paths this code should
+ // probably be refactored out by creating an AccountMigrator extension point.
+ if (who.getExternalIdKey().isScheme(SCHEME_GOOGLE_OAUTH)) {
+ Optional<ExternalId> existingLDAPExtID = findLdapExternalId(who);
+ if (existingLDAPExtID.isPresent()) {
+ return migrateLdapAccountToOauth(who, existingLDAPExtID.get());
+ }
+ }
+ logger.atFine().log(
+ "External ID for account %s not found. A new account will be automatically created.",
+ who.getEmailAddress());
+ return create(who);
+ }
+
+ private AuthResult migrateLdapAccountToOauth(AuthRequest who, ExternalId ldapExternalId)
+ throws AccountException, IOException, ConfigInvalidException {
+ Account.Id extAccId = ldapExternalId.accountId();
+ AuthResult res = link(extAccId, who);
+ accountsUpdateProvider
+ .get()
+ .update(
+ "remove existing LDAP externalId with matching e-mail",
+ extAccId,
+ u -> {
+ u.deleteExternalId(ldapExternalId);
+ });
+ return res;
+ }
+
private void deactivateAccountIfItExists(AuthRequest authRequest) {
if (!shouldUpdateActiveStatus(authRequest)) {
return;
@@ -277,6 +316,17 @@
}
}
+ private Optional<ExternalId> findLdapExternalId(AuthRequest who) throws IOException {
+ String email = who.getEmailAddress();
+ if (email == null || email.isEmpty()) {
+ return Optional.empty();
+ }
+
+ Optional<ExternalId> ldapExternalId =
+ externalIds.byEmail(email).stream().filter(a -> a.isScheme(SCHEME_GERRIT)).findFirst();
+ return ldapExternalId;
+ }
+
private AuthResult create(AuthRequest who)
throws AccountException, IOException, ConfigInvalidException {
Account.Id newId = Account.id(sequences.nextAccountId());
diff --git a/java/com/google/gerrit/server/account/AccountsUpdate.java b/java/com/google/gerrit/server/account/AccountsUpdate.java
index 1f2bc8c..b706bca 100644
--- a/java/com/google/gerrit/server/account/AccountsUpdate.java
+++ b/java/com/google/gerrit/server/account/AccountsUpdate.java
@@ -26,6 +26,7 @@
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.exceptions.DuplicateKeyException;
import com.google.gerrit.exceptions.StorageException;
@@ -201,7 +202,7 @@
private final Runnable beforeCommit;
/** Single instance that accumulates updates from the batch. */
- private ExternalIdNotes externalIdNotes;
+ @Nullable private ExternalIdNotes externalIdNotes;
@AssistedInject
AccountsUpdate(
@@ -402,13 +403,17 @@
delta.getDeletedExternalIds()),
updateArguments.accountId);
- if (externalIdNotes == null) {
- externalIdNotes =
- extIdNotesLoader.load(
- repo, accountConfig.getExternalIdsRev().orElse(ObjectId.zeroId()));
+ if (delta.hasExternalIdUpdates()) {
+ // Only load the externalIds if they are going to be updated
+ // This makes e.g. preferences updates faster.
+ if (externalIdNotes == null) {
+ externalIdNotes =
+ extIdNotesLoader.load(
+ repo, accountConfig.getExternalIdsRev().orElse(ObjectId.zeroId()));
+ }
+ externalIdNotes.replace(delta.getDeletedExternalIds(), delta.getCreatedExternalIds());
+ externalIdNotes.upsert(delta.getUpdatedExternalIds());
}
- externalIdNotes.replace(delta.getDeletedExternalIds(), delta.getCreatedExternalIds());
- externalIdNotes.upsert(delta.getUpdatedExternalIds());
CachedPreferences cachedDefaultPreferences =
CachedPreferences.fromConfig(VersionedDefaultPreferences.get(repo, allUsersName));
@@ -509,17 +514,22 @@
beforeCommit.run();
BatchRefUpdate batchRefUpdate = allUsersRepo.getRefDatabase().newBatchUpdate();
-
- String externalIdUpdateMessage =
- updatedAccounts.size() == 1
- ? Iterables.getOnlyElement(updatedAccounts).message
- : "Batch update for " + updatedAccounts.size() + " accounts";
- ObjectId oldExternalIdsRevision = externalIdNotes.getRevision();
- // These update the same ref, so they need to be stacked on top of one another using the same
- // ExternalIdNotes instance.
- RevCommit revCommit =
- commitExternalIdUpdates(externalIdUpdateMessage, allUsersRepo, batchRefUpdate);
- boolean externalIdsUpdated = !Objects.equals(revCommit.getId(), oldExternalIdsRevision);
+ // External ids may be not updated if:
+ // * externalIdNotes is not loaded (there were no externalId updates in the delta)
+ // * new revCommit is identical to the previous externalId tip
+ boolean externalIdsUpdated = false;
+ if (externalIdNotes != null) {
+ String externalIdUpdateMessage =
+ updatedAccounts.size() == 1
+ ? Iterables.getOnlyElement(updatedAccounts).message
+ : "Batch update for " + updatedAccounts.size() + " accounts";
+ ObjectId oldExternalIdsRevision = externalIdNotes.getRevision();
+ // These update the same ref, so they need to be stacked on top of one another using the same
+ // ExternalIdNotes instance.
+ RevCommit revCommit =
+ commitExternalIdUpdates(externalIdUpdateMessage, allUsersRepo, batchRefUpdate);
+ externalIdsUpdated = !Objects.equals(revCommit.getId(), oldExternalIdsRevision);
+ }
for (UpdatedAccount updatedAccount : updatedAccounts) {
// These updates are all for different refs (because batches never update the same account
@@ -544,8 +554,10 @@
RefUpdateUtil.executeChecked(batchRefUpdate, allUsersRepo);
Set<Account.Id> accountsToSkipForReindex = getUpdatedAccountIds(batchRefUpdate);
- extIdNotesLoader.updateExternalIdCacheAndMaybeReindexAccounts(
- externalIdNotes, accountsToSkipForReindex);
+ if (externalIdsUpdated) {
+ extIdNotesLoader.updateExternalIdCacheAndMaybeReindexAccounts(
+ externalIdNotes, accountsToSkipForReindex);
+ }
gitRefUpdated.fire(
allUsersName, batchRefUpdate, currentUser.map(IdentifiedUser::state).orElse(null));
diff --git a/java/com/google/gerrit/server/account/AuthRequest.java b/java/com/google/gerrit/server/account/AuthRequest.java
index b4fbcdb..cf1e552 100644
--- a/java/com/google/gerrit/server/account/AuthRequest.java
+++ b/java/com/google/gerrit/server/account/AuthRequest.java
@@ -16,6 +16,7 @@
import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_EXTERNAL;
import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_GERRIT;
+import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_GOOGLE_OAUTH;
import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_MAILTO;
import com.google.common.base.Strings;
@@ -66,6 +67,14 @@
return r;
}
+ public AuthRequest createForOAuthUser(String userName) {
+ AuthRequest r =
+ new AuthRequest(
+ externalIdKeyFactory.create(SCHEME_GOOGLE_OAUTH, userName), externalIdKeyFactory);
+ r.setUserName(userName);
+ return r;
+ }
+
/**
* Create a request for an email address registration.
*
diff --git a/java/com/google/gerrit/server/account/externalids/ExternalId.java b/java/com/google/gerrit/server/account/externalids/ExternalId.java
index 1616198..9196db8 100644
--- a/java/com/google/gerrit/server/account/externalids/ExternalId.java
+++ b/java/com/google/gerrit/server/account/externalids/ExternalId.java
@@ -150,6 +150,9 @@
/** Scheme for xri resources. OpenID in particular makes use of these external IDs. */
public static final String SCHEME_XRI = "xri";
+ /** Scheme for Google OAuth external IDs. */
+ public static final String SCHEME_GOOGLE_OAUTH = "google-oauth";
+
@AutoValue
public abstract static class Key implements Serializable {
private static final long serialVersionUID = 1L;
diff --git a/java/com/google/gerrit/server/api/changes/ChangesImpl.java b/java/com/google/gerrit/server/api/changes/ChangesImpl.java
index 0596524..6b107f1 100644
--- a/java/com/google/gerrit/server/api/changes/ChangesImpl.java
+++ b/java/com/google/gerrit/server/api/changes/ChangesImpl.java
@@ -21,6 +21,7 @@
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.changes.ChangeApi;
import com.google.gerrit.extensions.api.changes.Changes;
import com.google.gerrit.extensions.client.ListChangesOption;
@@ -41,6 +42,7 @@
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.util.List;
+import org.eclipse.jgit.lib.ObjectId;
@Singleton
class ChangesImpl implements Changes {
@@ -101,7 +103,11 @@
public ChangeApi create(ChangeInput in) throws RestApiException {
try {
ChangeInfo out = createChange.apply(TopLevelResource.INSTANCE, in).value();
- return api.create(changes.parse(Change.id(out._number)));
+ return api.create(
+ changes.parse(
+ Project.nameKey(out.project),
+ Change.id(out._number),
+ ObjectId.fromString(out.metaRevId)));
} catch (Exception e) {
throw asRestApiException("Cannot create change", e);
}
diff --git a/java/com/google/gerrit/server/change/ReviewerModifier.java b/java/com/google/gerrit/server/change/ReviewerModifier.java
index b5e0181..49bd28a 100644
--- a/java/com/google/gerrit/server/change/ReviewerModifier.java
+++ b/java/com/google/gerrit/server/change/ReviewerModifier.java
@@ -113,6 +113,7 @@
private enum FailureType {
NOT_FOUND,
+ INACTIVE,
OTHER;
}
@@ -232,6 +233,9 @@
ReviewerModification byAccountId = byAccountId(input, notes, user);
+ if (byAccountId.isFailure() && byAccountId.failureType.equals(FailureType.INACTIVE)) {
+ return byAccountId;
+ }
ReviewerModification wholeGroup = null;
if (!byAccountId.exactMatchFound) {
wholeGroup = addWholeGroup(input, notes, user, confirmed, allowGroup, allowByEmail);
@@ -285,6 +289,13 @@
} else {
reviewerUser = accountResolver.resolveIncludeInactive(input.reviewer).asUniqueUser();
}
+ if (reviewerUser.getAccount().inactive()) {
+ return fail(
+ input,
+ FailureType.INACTIVE,
+ String.format(
+ "Cannot add inactive account %s, %s", reviewerUser.getAccountId(), input.reviewer));
+ }
if (input.reviewer.equalsIgnoreCase(reviewerUser.getName())
|| input.reviewer.equals(String.valueOf(reviewerUser.getAccountId()))) {
exactMatchFound = true;
diff --git a/java/com/google/gerrit/server/patch/DiffUtil.java b/java/com/google/gerrit/server/patch/DiffUtil.java
index 70a3208..79f5ab9 100644
--- a/java/com/google/gerrit/server/patch/DiffUtil.java
+++ b/java/com/google/gerrit/server/patch/DiffUtil.java
@@ -15,19 +15,29 @@
package com.google.gerrit.server.patch;
+import com.google.common.base.Strings;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ListMultimap;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Patch.ChangeType;
+import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.server.patch.diff.ModifiedFilesCache;
import com.google.gerrit.server.patch.gitdiff.GitModifiedFilesCache;
import com.google.gerrit.server.patch.gitdiff.ModifiedFile;
import java.io.IOException;
+import java.io.OutputStream;
import java.util.Comparator;
import java.util.List;
+import java.util.Optional;
+import org.eclipse.jgit.diff.DiffFormatter;
import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectReader;
+import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevTree;
import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.treewalk.filter.PathFilter;
/**
* A utility class used by the diff cache interfaces {@link GitModifiedFilesCache} and {@link
@@ -116,6 +126,75 @@
return 0;
}
+ /**
+ * Get formatted diff between the given commits, either for a single path if specified, or for the
+ * full trees.
+ *
+ * @param repo to get the diff from
+ * @param baseCommit to compare with
+ * @param childCommit to compare
+ * @param path to narrow the diff to
+ * @param out to append the diff to
+ * @throws IOException if the diff couldn't be written
+ */
+ public static void getFormattedDiff(
+ Repository repo,
+ RevCommit baseCommit,
+ RevCommit childCommit,
+ @Nullable String path,
+ OutputStream out)
+ throws IOException {
+ getFormattedDiff(repo, null, baseCommit.getTree(), childCommit.getTree(), path, out);
+ }
+
+ public static void getFormattedDiff(
+ Repository repo,
+ @Nullable ObjectReader reader,
+ RevTree baseTree,
+ RevTree childTree,
+ @Nullable String path,
+ OutputStream out)
+ throws IOException {
+ try (DiffFormatter fmt = new DiffFormatter(out)) {
+ fmt.setRepository(repo);
+ if (reader != null) {
+ fmt.setReader(reader, repo.getConfig());
+ }
+ if (path != null) {
+ fmt.setPathFilter(PathFilter.create(path));
+ }
+ fmt.format(baseTree, childTree);
+ fmt.flush();
+ }
+ }
+
+ public static String cleanPatch(final String patch) {
+ String res = patch;
+ if (res.contains("\ndiff --git")) {
+ // Remove the header
+ res = res.substring(patch.lastIndexOf("\ndiff --git"), patch.length() - 1);
+ }
+ return res
+ // Remove "index NN..NN lines
+ .replaceAll("(?m)^index.*", "")
+ // Remove empty lines
+ .replaceAll("\n+", "\n")
+ // Trim
+ .trim();
+ }
+
+ public static Optional<String> getPatchHeader(final String patch) {
+ if (!patch.contains("\ndiff --git")) {
+ return Optional.empty();
+ }
+ return Optional.ofNullable(
+ Strings.emptyToNull(patch.trim().substring(0, patch.lastIndexOf("\ndiff --git"))));
+ }
+
+ public static String cleanPatch(BinaryResult bin) throws IOException {
+ return cleanPatch(bin.asString());
+ }
+
private static boolean isRootOrMergeCommit(RevCommit commit) {
return commit.getParentCount() != 1;
}
diff --git a/java/com/google/gerrit/server/restapi/change/ApplyPatch.java b/java/com/google/gerrit/server/restapi/change/ApplyPatch.java
index f368eb9..da9810d 100644
--- a/java/com/google/gerrit/server/restapi/change/ApplyPatch.java
+++ b/java/com/google/gerrit/server/restapi/change/ApplyPatch.java
@@ -53,7 +53,6 @@
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.UpdateException;
import com.google.gerrit.server.update.context.RefUpdateContext;
-import com.google.gerrit.server.util.CommitMessageUtil;
import com.google.gerrit.server.util.time.TimeUtil;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -70,6 +69,7 @@
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.FooterLine;
import org.eclipse.jgit.revwalk.RevCommit;
@Singleton
@@ -141,13 +141,14 @@
destChange.change().getStatus().name()));
}
+ RevCommit latestPatchset = revWalk.parseCommit(destChange.currentPatchSet().commitId());
+
RevCommit baseCommit;
if (!Strings.isNullOrEmpty(input.base)) {
baseCommit =
CommitUtil.getBaseCommit(
project.get(), queryProvider.get(), revWalk, destRef, input.base);
} else {
- RevCommit latestPatchset = revWalk.parseCommit(destChange.currentPatchSet().commitId());
if (latestPatchset.getParentCount() != 1) {
throw new BadRequestException(
String.format(
@@ -164,12 +165,17 @@
input.author == null
? committerIdent
: new PersonIdent(input.author.name, input.author.email, now, serverZoneId);
+ List<FooterLine> footerLines = latestPatchset.getFooterLines();
+ String messageWithNoFooters =
+ !Strings.isNullOrEmpty(input.commitMessage)
+ ? input.commitMessage
+ : removeFooters(latestPatchset.getFullMessage(), footerLines);
String commitMessage =
- CommitMessageUtil.checkAndSanitizeCommitMessage(
- input.commitMessage != null
- ? input.commitMessage
- : "The following patch was applied:\n>\t"
- + input.patch.patch.replaceAll("\n", "\n>\t"));
+ ApplyPatchUtil.buildCommitMessage(
+ messageWithNoFooters,
+ footerLines,
+ input.patch.patch,
+ ApplyPatchUtil.getResultPatch(repo, reader, baseCommit, revWalk.lookupTree(treeId)));
ObjectId appliedCommit =
CommitUtil.createCommitWithTree(
@@ -215,4 +221,11 @@
private static String buildMessageForPatchSet(PatchSet.Id psId) {
return new StringBuilder(String.format("Uploaded patch set %s.", psId.get())).toString();
}
+
+ private String removeFooters(String originalMessage, List<FooterLine> footerLines) {
+ if (footerLines.isEmpty()) {
+ return originalMessage;
+ }
+ return originalMessage.substring(0, originalMessage.indexOf(footerLines.get(0).getKey()));
+ }
}
diff --git a/java/com/google/gerrit/server/restapi/change/ApplyPatchUtil.java b/java/com/google/gerrit/server/restapi/change/ApplyPatchUtil.java
index 4021f77..74033c1 100644
--- a/java/com/google/gerrit/server/restapi/change/ApplyPatchUtil.java
+++ b/java/com/google/gerrit/server/restapi/change/ApplyPatchUtil.java
@@ -19,17 +19,26 @@
import com.google.gerrit.extensions.api.changes.ApplyPatchInput;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.server.patch.DiffUtil;
+import com.google.gerrit.server.util.CommitMessageUtil;
import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
+import java.io.OutputStream;
import java.nio.charset.StandardCharsets;
+import java.util.List;
+import java.util.Optional;
import org.apache.commons.codec.binary.Base64;
+import org.apache.commons.lang3.StringUtils;
import org.eclipse.jgit.api.errors.PatchApplyException;
import org.eclipse.jgit.api.errors.PatchFormatException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.patch.PatchApplier;
+import org.eclipse.jgit.revwalk.FooterLine;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevTree;
@@ -52,12 +61,8 @@
throws IOException, RestApiException {
checkNotNull(mergeTip);
RevTree tip = mergeTip.getTree();
- InputStream patchStream;
- if (Base64.isBase64(input.patch)) {
- patchStream = new ByteArrayInputStream(org.eclipse.jgit.util.Base64.decode(input.patch));
- } else {
- patchStream = new ByteArrayInputStream(input.patch.getBytes(StandardCharsets.UTF_8));
- }
+ InputStream patchStream =
+ new ByteArrayInputStream(decodeIfNecessary(input.patch).getBytes(StandardCharsets.UTF_8));
try {
PatchApplier applier = new PatchApplier(repo, tip, oi);
PatchApplier.Result applyResult = applier.applyPatch(patchStream);
@@ -69,5 +74,88 @@
}
}
+ /**
+ * Build commit message for commits with applied patch.
+ *
+ * <p>Message structure:
+ *
+ * <ol>
+ * <li>Provided {@code message}.
+ * <li>In case the result change's patch is not the same as the original patch - a warning
+ * message which includes the diff; as well as the original patch's header if available, or
+ * the full original patch otherwise.
+ * <li>The provided {@code footerLines}, if any.
+ * </ol>
+ *
+ * @param message the first message piece, excluding footers
+ * @param footerLines footer lines to append to the message
+ * @param originalPatch to compare the result patch to
+ * @param resultPatch to validate accuracy for
+ * @return the commit message
+ * @throws BadRequestException if the commit message cannot be sanitized
+ */
+ public static String buildCommitMessage(
+ String message, List<FooterLine> footerLines, String originalPatch, String resultPatch)
+ throws BadRequestException {
+ String decodedOriginalPatch = decodeIfNecessary(originalPatch);
+ StringBuilder res = new StringBuilder(message.trim());
+ Optional<String> patchDiff = verifyAppliedPatch(decodedOriginalPatch, resultPatch);
+ if (!patchDiff.isEmpty()) {
+ res.append(
+ "\n\nNOTE FOR REVIEWERS - original patch and result patch are not identical."
+ + "\nPLEASE REVIEW CAREFULLY.\nDiffs between the patches:\n"
+ + patchDiff.get());
+ }
+ Optional<String> originalPatchHeader = DiffUtil.getPatchHeader(decodedOriginalPatch);
+ if (!patchDiff.isEmpty()) {
+ res.append(
+ "\n\nOriginal patch:\n"
+ + (originalPatchHeader.isEmpty() ? decodedOriginalPatch : originalPatchHeader.get()));
+ }
+ if (!footerLines.isEmpty()) {
+ res.append('\n');
+ }
+ for (FooterLine footer : footerLines) {
+ res.append("\n" + footer.toString());
+ }
+ return CommitMessageUtil.checkAndSanitizeCommitMessage(res.toString());
+ }
+
+ /**
+ * Fetch the patch of the result tree.
+ *
+ * @param repo in which the patch was applied
+ * @param reader for the repo objects, including {@code resultTree}
+ * @param baseCommit to generate patch against
+ * @param resultTree to generate the patch for
+ * @return the result patch
+ * @throws IOException if the result patch cannot be written
+ */
+ public static String getResultPatch(
+ Repository repo, ObjectReader reader, RevCommit baseCommit, RevTree resultTree)
+ throws IOException {
+ try (OutputStream resultPatchStream = new ByteArrayOutputStream()) {
+ DiffUtil.getFormattedDiff(
+ repo, reader, baseCommit.getTree(), resultTree, null, resultPatchStream);
+ return resultPatchStream.toString();
+ }
+ }
+
+ private static Optional<String> verifyAppliedPatch(String originalPatch, String resultPatch) {
+ String cleanOriginalPatch = DiffUtil.cleanPatch(originalPatch);
+ String cleanResultPatch = DiffUtil.cleanPatch(resultPatch);
+ if (cleanOriginalPatch.equals(cleanResultPatch)) {
+ return Optional.empty();
+ }
+ return Optional.of(StringUtils.difference(cleanOriginalPatch, cleanResultPatch));
+ }
+
+ private static String decodeIfNecessary(String patch) {
+ if (Base64.isBase64(patch)) {
+ return new String(org.eclipse.jgit.util.Base64.decode(patch), StandardCharsets.UTF_8);
+ }
+ return patch;
+ }
+
private ApplyPatchUtil() {}
}
diff --git a/java/com/google/gerrit/server/restapi/change/ChangesCollection.java b/java/com/google/gerrit/server/restapi/change/ChangesCollection.java
index a0c5b16..84cf209 100644
--- a/java/com/google/gerrit/server/restapi/change/ChangesCollection.java
+++ b/java/com/google/gerrit/server/restapi/change/ChangesCollection.java
@@ -39,6 +39,7 @@
import java.io.IOException;
import java.util.List;
import java.util.Optional;
+import org.eclipse.jgit.lib.ObjectId;
@Singleton
public class ChangesCollection implements RestCollection<TopLevelResource, ChangeResource> {
@@ -49,6 +50,7 @@
private final ChangeResource.Factory changeResourceFactory;
private final PermissionBackend permissionBackend;
private final ProjectCache projectCache;
+ private final ChangeNotes.Factory changeNotesFactory;
@Inject
public ChangesCollection(
@@ -58,7 +60,9 @@
ChangeFinder changeFinder,
ChangeResource.Factory changeResourceFactory,
PermissionBackend permissionBackend,
- ProjectCache projectCache) {
+ ProjectCache projectCache,
+ ChangeNotes.Factory changeNotesFactory) {
+ this.changeNotesFactory = changeNotesFactory;
this.user = user;
this.queryFactory = queryFactory;
this.views = views;
@@ -78,6 +82,11 @@
return views;
}
+ /**
+ * Parses {@link ChangeResource} from {@link Change.Id}
+ *
+ * <p>Reads the change from index, since project is unknown.
+ */
@Override
public ChangeResource parse(TopLevelResource root, IdString id)
throws RestApiException, PermissionBackendException, IOException {
@@ -96,6 +105,28 @@
return changeResourceFactory.create(change, user.get());
}
+ /**
+ * Parses {@link ChangeResource} from {@link Change.Id} in {@code project} at {@code metaRevId}
+ *
+ * <p>Read change from ChangeNotesCache, so the method can be used upon creation, when the change
+ * might not be yet available in the index.
+ */
+ public ChangeResource parse(Project.NameKey project, Change.Id id, ObjectId metaRevId)
+ throws ResourceConflictException, ResourceNotFoundException, PermissionBackendException {
+ checkProjectStatePermitsRead(project);
+ ChangeNotes change = changeNotesFactory.createChecked(project, id, metaRevId);
+ if (!canRead(change)) {
+ throw new ResourceNotFoundException(toIdString(id));
+ }
+
+ return changeResourceFactory.create(change, user.get());
+ }
+
+ /**
+ * Parses {@link ChangeResource} from {@link Change.Id}
+ *
+ * <p>Reads the change from index, since project is unknown.
+ */
public ChangeResource parse(Change.Id id)
throws ResourceConflictException, ResourceNotFoundException, PermissionBackendException {
List<ChangeNotes> notes = changeFinder.find(id);
diff --git a/java/com/google/gerrit/server/restapi/change/CreateChange.java b/java/com/google/gerrit/server/restapi/change/CreateChange.java
index c0263df..fd3f11e 100644
--- a/java/com/google/gerrit/server/restapi/change/CreateChange.java
+++ b/java/com/google/gerrit/server/restapi/change/CreateChange.java
@@ -406,10 +406,18 @@
throw new BadRequestException("Cannot apply patch on top of an empty tree.");
}
ObjectId treeId = ApplyPatchUtil.applyPatch(git, oi, input.patch, mergeTip);
+ String appliedPatchCommitMessage =
+ getCommitMessage(
+ ApplyPatchUtil.buildCommitMessage(
+ input.subject,
+ ImmutableList.of(),
+ input.patch.patch,
+ ApplyPatchUtil.getResultPatch(git, reader, mergeTip, rw.lookupTree(treeId))),
+ me);
c =
rw.parseCommit(
CommitUtil.createCommitWithTree(
- oi, author, committer, mergeTip, commitMessage, treeId));
+ oi, author, committer, mergeTip, appliedPatchCommitMessage, treeId));
} else {
// create an empty commit.
c = createEmptyCommit(oi, rw, author, committer, mergeTip, commitMessage);
diff --git a/java/com/google/gerrit/server/restapi/change/GetPatch.java b/java/com/google/gerrit/server/restapi/change/GetPatch.java
index dea4dc4..d8946a7 100644
--- a/java/com/google/gerrit/server/restapi/change/GetPatch.java
+++ b/java/com/google/gerrit/server/restapi/change/GetPatch.java
@@ -24,6 +24,7 @@
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.patch.DiffUtil;
import com.google.inject.Inject;
import java.io.IOException;
import java.io.OutputStream;
@@ -32,12 +33,10 @@
import java.util.Locale;
import java.util.zip.ZipEntry;
import java.util.zip.ZipOutputStream;
-import org.eclipse.jgit.diff.DiffFormatter;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
-import org.eclipse.jgit.treewalk.filter.PathFilter;
import org.kohsuke.args4j.Option;
public class GetPatch implements RestReadView<RevisionResource> {
@@ -98,14 +97,7 @@
if (path == null) {
out.write(formatEmailHeader(commit).getBytes(UTF_8));
}
- try (DiffFormatter fmt = new DiffFormatter(out)) {
- fmt.setRepository(repo);
- if (path != null) {
- fmt.setPathFilter(PathFilter.create(path));
- }
- fmt.format(base.getTree(), commit.getTree());
- fmt.flush();
- }
+ DiffUtil.getFormattedDiff(repo, base, commit, path, out);
}
@Override
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountManagerIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountManagerIT.java
index 875b520..0d246e3 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountManagerIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountManagerIT.java
@@ -18,6 +18,7 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.common.truth.Truth8.assertThat;
+import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_GOOGLE_OAUTH;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static com.google.gerrit.testing.TestActionRefUpdateContext.openTestRefUpdateContext;
import static java.util.stream.Collectors.toSet;
@@ -567,6 +568,108 @@
}
@Test
+ public void errorCreatingOAuthAccountDueToPresentDuplicateUsernameExternalID() throws Exception {
+ String username = "foo";
+ String gerritEmail = "bar@example.com";
+
+ ExternalId.Key gerritExtIdKey = externalIdKeyFactory.create(ExternalId.SCHEME_GERRIT, username);
+ AuthRequest whoGerrit = authRequestFactory.createForUser(username);
+ whoGerrit.setEmailAddress(gerritEmail);
+ AuthResult authResultGerrit = accountManager.authenticate(whoGerrit);
+ assertAuthResultForNewAccount(authResultGerrit, gerritExtIdKey);
+
+ // Check that OAuth externalID is not in use.
+ ExternalId.Key externalExtIdKey = externalIdKeyFactory.create(SCHEME_GOOGLE_OAUTH, username);
+ assertNoSuchExternalIds(externalExtIdKey);
+
+ String googleOAuthEmail = "baz@example.com";
+ AuthRequest whoOAuth = authRequestFactory.createForOAuthUser(username);
+ whoOAuth.setEmailAddress(googleOAuthEmail);
+
+ AccountException thrown =
+ assertThrows(AccountException.class, () -> accountManager.authenticate(whoOAuth));
+ assertThat(thrown).hasMessageThat().contains("Cannot assign external ID \"username:foo\" to");
+ }
+
+ @Test
+ public void errorCreatingOAuthAccountDueToDuplicateEmailExternalIDInNonLDAPExternalId()
+ throws Exception {
+ String username = "foo";
+ String gerritEmail = "foo@example.com";
+
+ ExternalId.Key gerritExtIdKey =
+ externalIdKeyFactory.create(ExternalId.SCHEME_EXTERNAL, username);
+ AuthRequest whoGerrit = authRequestFactory.createForExternalUser(username);
+ whoGerrit.setEmailAddress(gerritEmail);
+ AuthResult authResultGerrit = accountManager.authenticate(whoGerrit);
+ assertAuthResultForNewAccount(authResultGerrit, gerritExtIdKey);
+
+ // Check that OAuth externalID is not in use.
+ ExternalId.Key externalExtIdKey = externalIdKeyFactory.create(SCHEME_GOOGLE_OAUTH, username);
+ assertNoSuchExternalIds(externalExtIdKey);
+
+ String googleOAuthEmail = "foo@example.com";
+ AuthRequest whoOAuth = authRequestFactory.createForOAuthUser(username);
+ whoOAuth.setEmailAddress(googleOAuthEmail);
+
+ AccountException thrown =
+ assertThrows(AccountException.class, () -> accountManager.authenticate(whoOAuth));
+ assertThat(thrown)
+ .hasMessageThat()
+ .contains("Email 'foo@example.com' in use by another account");
+ }
+
+ @Test
+ public void errorCreatingOAuthAccountDueToDuplicateUsernameIdentityAlreadyInUse()
+ throws Exception {
+ String username = "foo";
+ String gerritEmail = "foo@example.com";
+
+ ExternalId.Key externalExtIdKey =
+ externalIdKeyFactory.create(ExternalId.SCHEME_EXTERNAL, username);
+ AuthRequest whoExternal = authRequestFactory.createForExternalUser(username);
+ whoExternal.setEmailAddress(gerritEmail);
+ AuthResult authResultGerrit = accountManager.authenticate(whoExternal);
+ assertAuthResultForNewAccount(authResultGerrit, externalExtIdKey);
+
+ // Check that OAuth externalID is not in use.
+ ExternalId.Key OAuthExtIdKey = externalIdKeyFactory.create(SCHEME_GOOGLE_OAUTH, username);
+ assertNoSuchExternalIds(OAuthExtIdKey);
+
+ String googleOAuthEmail = "baz@example.com";
+ AuthRequest whoOAuth = authRequestFactory.createForOAuthUser(username);
+ whoExternal.setEmailAddress(googleOAuthEmail);
+
+ AccountException thrown =
+ assertThrows(AccountException.class, () -> accountManager.authenticate(whoOAuth));
+ assertThat(thrown)
+ .hasMessageThat()
+ .contains("Cannot assign external ID \"username:foo\" to account");
+ }
+
+ @Test
+ public void linkOAuthAccountToLDAPAccountWithEmail() throws Exception {
+ String username = "foo";
+ String email = "foo@example.com";
+ ExternalId.Key gerritExtIdKey = externalIdKeyFactory.create(ExternalId.SCHEME_GERRIT, username);
+ AuthRequest whoGerrit = authRequestFactory.createForUser(username);
+ whoGerrit.setEmailAddress(email);
+ AuthResult authResultGerrit = accountManager.authenticate(whoGerrit);
+ Account.Id accID = authResultGerrit.getAccountId();
+ assertAuthResultForNewAccount(authResultGerrit, gerritExtIdKey);
+ // Check that OAuth externalID is not in use.
+ ExternalId.Key OAuthExtIdKey = externalIdKeyFactory.create(SCHEME_GOOGLE_OAUTH, username);
+ assertNoSuchExternalIds(OAuthExtIdKey);
+
+ AuthRequest whoOAuth = authRequestFactory.createForOAuthUser(username);
+ whoOAuth.setEmailAddress(email);
+ AuthResult authResultOAuth = accountManager.authenticate(whoOAuth);
+ assertAuthResultForExistingAccount(authResultOAuth, accID, OAuthExtIdKey);
+
+ assertThat(authResultOAuth.getAccountId()).isEqualTo(authResultGerrit.getAccountId());
+ }
+
+ @Test
public void updateExternalIdOnLink() throws Exception {
// Create an account with a SCHEME_GERRIT external ID and no email
String username = "foo";
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ApplyPatchIT.java b/javatests/com/google/gerrit/acceptance/api/change/ApplyPatchIT.java
index b451656..7b7e145a 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ApplyPatchIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ApplyPatchIT.java
@@ -21,6 +21,7 @@
import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_FILES;
import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVISION;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import static com.google.gerrit.server.patch.DiffUtil.cleanPatch;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.eclipse.jgit.lib.Constants.HEAD;
@@ -48,7 +49,6 @@
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.inject.Inject;
-import java.io.IOException;
import org.eclipse.jgit.api.errors.PatchApplyException;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.util.Base64;
@@ -56,7 +56,6 @@
public class ApplyPatchIT extends AbstractDaemonTest {
- private static final String COMMIT_MESSAGE = "Applying patch";
private static final String DESTINATION_BRANCH = "destBranch";
private static final String ADDED_FILE_NAME = "a_new_file.txt";
@@ -402,7 +401,7 @@
}
@Test
- public void applyPatchWithExplicitBase_OverrideParentId() throws Exception {
+ public void applyPatchWithExplicitBase_overrideParentId() throws Exception {
PushOneCommit.Result inputParent = createChange("Input parent", "file1", "content");
PushOneCommit.Result parent = createChange("Parent Change", "file2", "content");
parent.assertOkStatus();
@@ -440,6 +439,44 @@
ADDED_FILE_CONTENT);
}
+ @Test
+ public void commitMessage_providedMessage() throws Exception {
+ final String msg = "custom message";
+ initDestBranch();
+ ApplyPatchPatchSetInput in = buildInput(ADDED_FILE_DIFF);
+ in.commitMessage = msg;
+
+ ChangeInfo result = applyPatch(in);
+
+ ChangeInfo info = get(result.changeId, CURRENT_REVISION, CURRENT_COMMIT);
+ assertThat(info.revisions.get(info.currentRevision).commit.message)
+ .isEqualTo(msg + "\n\nChange-Id: " + result.changeId + "\n");
+ }
+
+ @Test
+ public void commitMessage_defaultMessageAndPatchHeader() throws Exception {
+ initDestBranch();
+ ApplyPatchPatchSetInput in = buildInput("Patch header\n" + ADDED_FILE_DIFF);
+
+ ChangeInfo result = applyPatch(in);
+
+ ChangeInfo info = get(result.changeId, CURRENT_REVISION, CURRENT_COMMIT);
+ assertThat(info.revisions.get(info.currentRevision).commit.message)
+ .isEqualTo("Default commit message\n\nChange-Id: " + result.changeId + "\n");
+ }
+
+ @Test
+ public void commitMessage_defaultMessageAndNoPatchHeader() throws Exception {
+ initDestBranch();
+ ApplyPatchPatchSetInput in = buildInput(ADDED_FILE_DIFF);
+
+ ChangeInfo result = applyPatch(in);
+
+ ChangeInfo info = get(result.changeId, CURRENT_REVISION, CURRENT_COMMIT);
+ assertThat(info.revisions.get(info.currentRevision).commit.message)
+ .isEqualTo("Default commit message\n\nChange-Id: " + result.changeId + "\n");
+ }
+
private void initDestBranch() throws Exception {
String head = getHead(repo(), HEAD).name();
createBranchWithRevision(BranchNameKey.create(project, ApplyPatchIT.DESTINATION_BRANCH), head);
@@ -462,27 +499,11 @@
private ChangeInfo applyPatch(ApplyPatchPatchSetInput input) throws RestApiException {
input.responseFormatOptions = ImmutableList.of(ListChangesOption.CURRENT_REVISION);
return gApi.changes()
- .create(new ChangeInput(project.get(), DESTINATION_BRANCH, COMMIT_MESSAGE))
+ .create(new ChangeInput(project.get(), DESTINATION_BRANCH, "Default commit message"))
.applyPatch(input);
}
private DiffInfo fetchDiffForFile(ChangeInfo result, String fileName) throws RestApiException {
return gApi.changes().id(result.id).current().file(fileName).diff();
}
-
- private String cleanPatch(BinaryResult bin) throws IOException {
- return cleanPatch(bin.asString());
- }
-
- private String cleanPatch(String s) {
- return s
- // Remove the header
- .substring(s.lastIndexOf("\ndiff --git"), s.length() - 1)
- // Remove "index NN..NN lines
- .replaceAll("(?m)^index.*", "")
- // Remove empty lines
- .replaceAll("\n+", "\n")
- // Trim
- .trim();
- }
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index d3ab2bb..393dacf 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -125,7 +125,6 @@
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
import com.google.gerrit.extensions.api.changes.ReviewResult;
-import com.google.gerrit.extensions.api.changes.ReviewerInfo;
import com.google.gerrit.extensions.api.changes.ReviewerInput;
import com.google.gerrit.extensions.api.changes.ReviewerResult;
import com.google.gerrit.extensions.api.changes.RevisionApi;
@@ -1356,26 +1355,23 @@
}
@Test
- public void addReviewerThatIsInactiveByUsername() throws Exception {
+ public void addReviewerThatIsInactiveByUsername_notAllowed() throws Exception {
PushOneCommit.Result result = createChange();
String username = name("new-user");
- Account.Id id = accountOperations.newAccount().username(username).inactive().create();
+ Account.Id accountId = accountOperations.newAccount().username(username).inactive().create();
ReviewerInput in = new ReviewerInput();
in.reviewer = username;
ReviewerResult r = gApi.changes().id(result.getChangeId()).addReviewer(in);
assertThat(r.input).isEqualTo(in.reviewer);
- assertThat(r.error).isNull();
- assertThat(r.reviewers).hasSize(1);
- ReviewerInfo reviewer = r.reviewers.get(0);
- assertThat(reviewer._accountId).isEqualTo(id.get());
- assertThat(reviewer.username).isEqualTo(username);
+ assertThat(r.error)
+ .isEqualTo(String.format("Cannot add inactive account %s, %s", accountId, username));
}
@Test
- public void addReviewerThatIsInactiveById() throws Exception {
+ public void addReviewerThatIsInactiveById_notAllowed() throws Exception {
PushOneCommit.Result result = createChange();
String username = name("new-user");
@@ -1386,33 +1382,28 @@
ReviewerResult r = gApi.changes().id(result.getChangeId()).addReviewer(in);
assertThat(r.input).isEqualTo(in.reviewer);
- assertThat(r.error).isNull();
- assertThat(r.reviewers).hasSize(1);
- ReviewerInfo reviewer = r.reviewers.get(0);
- assertThat(reviewer._accountId).isEqualTo(id.get());
- assertThat(reviewer.username).isEqualTo(username);
+
+ assertThat(r.error)
+ .isEqualTo(String.format("Cannot add inactive account %s, %s", id, id.get()));
}
@Test
- public void addReviewerThatIsInactiveByEmail() throws Exception {
+ public void addReviewerThatIsInactiveByEmail_notAllowed() throws Exception {
ConfigInput conf = new ConfigInput();
conf.enableReviewerByEmail = InheritableBoolean.TRUE;
gApi.projects().name(project.get()).config(conf);
PushOneCommit.Result result = createChange();
String username = "user@domain.com";
- Account.Id id = accountOperations.newAccount().username(username).inactive().create();
+ Account.Id accountId = accountOperations.newAccount().username(username).inactive().create();
ReviewerInput in = new ReviewerInput();
in.reviewer = username;
in.state = ReviewerState.CC;
ReviewerResult r = gApi.changes().id(result.getChangeId()).addReviewer(in);
- assertThat(r.input).isEqualTo(username);
- assertThat(r.error).isNull();
- assertThat(r.ccs).hasSize(1);
- AccountInfo reviewer = r.ccs.get(0);
- assertThat(reviewer._accountId).isEqualTo(id.get());
- assertThat(reviewer.username).isEqualTo(username);
+ assertThat(r.input).isEqualTo(in.reviewer);
+ assertThat(r.error)
+ .isEqualTo(String.format("Cannot add inactive account %s, %s", accountId, username));
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
index 7a04ee5..0360622 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
@@ -1022,6 +1022,8 @@
DiffInfo diff = gApi.changes().id(info.id).current().file(PATCH_FILE_NAME).diff();
assertDiffForNewFile(diff, info.currentRevision, PATCH_FILE_NAME, PATCH_NEW_FILE_CONTENT);
+ assertThat(info.revisions.get(info.currentRevision).commit.message)
+ .isEqualTo("apply patch to other\n\nChange-Id: " + info.changeId + "\n");
}
@Test
diff --git a/plugins/codemirror-editor b/plugins/codemirror-editor
index 13dcd3f..597573b 160000
--- a/plugins/codemirror-editor
+++ b/plugins/codemirror-editor
@@ -1 +1 @@
-Subproject commit 13dcd3f4673c56e2b1001ef19eb54236943a35e6
+Subproject commit 597573b5d1ed44c75844d2b8047aae08f7ef8619
diff --git a/plugins/download-commands b/plugins/download-commands
index b83ce67..b4209a5 160000
--- a/plugins/download-commands
+++ b/plugins/download-commands
@@ -1 +1 @@
-Subproject commit b83ce6746fe404da5f4709d8f8270a99f04c800e
+Subproject commit b4209a5a4c334077b255002cbadc2ef659adee3c
diff --git a/plugins/package.json b/plugins/package.json
index 134c433..612062b 100644
--- a/plugins/package.json
+++ b/plugins/package.json
@@ -29,6 +29,7 @@
"@codemirror/lint": "^6.1.1",
"@codemirror/search": "^6.2.3",
"@codemirror/state": "^6.2.0",
+ "@codemirror/theme-one-dark": "^6.1.1",
"@codemirror/view": "^6.9.1",
"lit": "^2.2.3",
"rxjs": "^6.6.7",
diff --git a/plugins/yarn.lock b/plugins/yarn.lock
index 1512abe..3156f4c 100644
--- a/plugins/yarn.lock
+++ b/plugins/yarn.lock
@@ -129,9 +129,9 @@
"@lezer/php" "^1.0.0"
"@codemirror/lang-python@^6.0.0", "@codemirror/lang-python@^6.1.1":
- version "6.1.1"
- resolved "https://registry.yarnpkg.com/@codemirror/lang-python/-/lang-python-6.1.1.tgz#378c69199da41e0b09eaadc56f6d70ad6001fd34"
- integrity sha512-AddGMIKUssUAqaDKoxKWA5GAzy/CVE0eSY7/ANgNzdS1GYBkp6N49XKEyMElkuN04UsZ+bTIQdj+tVV75NMwJw==
+ version "6.1.2"
+ resolved "https://registry.yarnpkg.com/@codemirror/lang-python/-/lang-python-6.1.2.tgz#cabb57529679981f170491833dbf798576e7ab18"
+ integrity sha512-nbQfifLBZstpt6Oo4XxA2LOzlSp4b/7Bc5cmodG1R+Cs5PLLCTUvsMNWDnziiCfTOG/SW1rVzXq/GbIr6WXlcw==
dependencies:
"@codemirror/autocomplete" "^6.3.2"
"@codemirror/language" "^6.0.0"
@@ -239,6 +239,16 @@
resolved "https://registry.yarnpkg.com/@codemirror/state/-/state-6.2.0.tgz#a0fb08403ced8c2a68d1d0acee926bd20be922f2"
integrity sha512-69QXtcrsc3RYtOtd+GsvczJ319udtBf1PTrr2KbLWM/e2CXUPnh0Nz9AUo8WfhSQ7GeL8dPVNUmhQVgpmuaNGA==
+"@codemirror/theme-one-dark@^6.1.1":
+ version "6.1.1"
+ resolved "https://registry.yarnpkg.com/@codemirror/theme-one-dark/-/theme-one-dark-6.1.1.tgz#76600555cbb314c495216f018f75b0c28daff158"
+ integrity sha512-+CfzmScfJuD6uDF5bHJkAjWTQ2QAAHxODCPxUEgcImDYcJLT+4l5vLnBHmDVv46kCC5uUJGMrBJct2Z6JbvqyQ==
+ dependencies:
+ "@codemirror/language" "^6.0.0"
+ "@codemirror/state" "^6.0.0"
+ "@codemirror/view" "^6.0.0"
+ "@lezer/highlight" "^1.0.0"
+
"@codemirror/view@^6.0.0", "@codemirror/view@^6.2.2", "@codemirror/view@^6.6.0", "@codemirror/view@^6.9.1":
version "6.9.1"
resolved "https://registry.yarnpkg.com/@codemirror/view/-/view-6.9.1.tgz#2ce4c528974b6172a5a4a738b7b0a0f04a4c1140"
diff --git a/polygerrit-ui/app/.eslintrc.js b/polygerrit-ui/app/.eslintrc.js
index 89259dc..e18a3af 100644
--- a/polygerrit-ui/app/.eslintrc.js
+++ b/polygerrit-ui/app/.eslintrc.js
@@ -327,6 +327,8 @@
'error',
{argsIgnorePattern: '^_'},
],
+ // https://github.com/mysticatea/eslint-plugin-node/blob/master/docs/rules/no-unsupported-features/es-builtins.md
+ 'node/no-unsupported-features/es-builtins': 'off',
// https://github.com/mysticatea/eslint-plugin-node/blob/master/docs/rules/no-unsupported-features/node-builtins.md
'node/no-unsupported-features/node-builtins': 'off',
// Disable no-invalid-this for ts files, because it incorrectly reports
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
index ccb750d..aba9a8b 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
@@ -1569,6 +1569,7 @@
const payload = {
base: e.detail.base,
allow_conflicts: e.detail.allowConflicts,
+ on_behalf_of_uploader: e.detail.onBehalfOfUploader,
};
const rebaseChain = !!e.detail.rebaseChain;
this.fireAction(
@@ -1576,7 +1577,10 @@
assertUIActionInfo(this.revisionActions.rebase),
rebaseChain ? false : true,
payload,
- {allow_conflicts: payload.allow_conflicts}
+ {
+ allow_conflicts: payload.allow_conflicts,
+ on_behalf_of_uploader: payload.on_behalf_of_uploader,
+ }
);
}
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
index 9c01f2f..277eddd 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
@@ -624,15 +624,20 @@
assert.isTrue(fetchChangesStub.called);
element.handleRebaseConfirm(
new CustomEvent('', {
- detail: {base: '1234', allowConflicts: false, rebaseChain: false},
+ detail: {
+ base: '1234',
+ allowConflicts: false,
+ rebaseChain: false,
+ onBehalfOfUploader: true,
+ },
})
);
assert.deepEqual(fireActionStub.lastCall.args, [
'/rebase',
assertUIActionInfo(rebaseAction),
true,
- {base: '1234', allow_conflicts: false},
- {allow_conflicts: false},
+ {base: '1234', allow_conflicts: false, on_behalf_of_uploader: true},
+ {allow_conflicts: false, on_behalf_of_uploader: true},
]);
});
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.ts b/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.ts
index 34869f2..3982666 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog.ts
@@ -23,6 +23,10 @@
import {ValueChangedEvent} from '../../../types/events';
import {throwingErrorCallback} from '../../shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper';
import {fireNoBubbleNoCompose} from '../../../utils/event-util';
+import {KnownExperimentId} from '../../../services/flags/flags';
+import {resolve} from '../../../models/dependency';
+import {changeModelToken} from '../../../models/change/change-model';
+import {subscribe} from '../../lit/subscription-controller';
export interface RebaseChange {
name: string;
@@ -33,6 +37,7 @@
base: string | null;
allowConflicts: boolean;
rebaseChain: boolean;
+ onBehalfOfUploader: boolean;
}
@customElement('gr-confirm-rebase-dialog')
@@ -79,6 +84,12 @@
@state()
recentChanges?: RebaseChange[];
+ @state()
+ allowConflicts = false;
+
+ @state()
+ isOwner = false;
+
@query('#rebaseOnParentInput')
private rebaseOnParentInput!: HTMLInputElement;
@@ -99,9 +110,18 @@
private readonly restApiService = getAppContext().restApiService;
+ private readonly flagsService = getAppContext().flagsService;
+
+ private readonly getChangeModel = resolve(this, changeModelToken);
+
constructor() {
super();
this.query = input => this.getChangeSuggestions(input);
+ subscribe(
+ this,
+ () => this.getChangeModel().isOwner$,
+ x => (this.isOwner = x)
+ );
}
override willUpdate(changedProperties: PropertyValues): void {
@@ -135,7 +155,7 @@
display: block;
width: 100%;
}
- .rebaseAllowConflicts {
+ .rebaseCheckbox:first-of-type {
margin-top: var(--spacing-m);
}
.rebaseOption {
@@ -225,16 +245,34 @@
>
</gr-autocomplete>
</div>
- <div class="rebaseAllowConflicts">
- <input id="rebaseAllowConflicts" type="checkbox" />
+ <div class="rebaseCheckbox">
+ <input
+ id="rebaseAllowConflicts"
+ type="checkbox"
+ @change=${() => {
+ this.allowConflicts = !!this.rebaseAllowConflicts?.checked;
+ }}
+ />
<label for="rebaseAllowConflicts"
>Allow rebase with conflicts</label
>
</div>
${when(
+ this.flagsService.isEnabled(
+ KnownExperimentId.REBASE_ON_BEHALF_OF_UPLOADER
+ ) &&
+ !this.isOwner &&
+ this.allowConflicts,
+ () =>
+ html`<span class="message"
+ >Rebase cannot be done on behalf of the uploader when allowing
+ conflicts.</span
+ >`
+ )}
+ ${when(
this.hasParent,
() =>
- html`<div>
+ html`<div class="rebaseCheckbox">
<input
id="rebaseChain"
type="checkbox"
@@ -351,11 +389,24 @@
base: this.getSelectedBase(),
allowConflicts: this.rebaseAllowConflicts.checked,
rebaseChain: !!this.rebaseChain?.checked,
+ onBehalfOfUploader: this.rebaseOnBehalfOfUploader(),
};
fireNoBubbleNoCompose(this, 'confirm-rebase', detail);
this.text = '';
}
+ private rebaseOnBehalfOfUploader() {
+ if (
+ !this.flagsService.isEnabled(
+ KnownExperimentId.REBASE_ON_BEHALF_OF_UPLOADER
+ )
+ ) {
+ return false;
+ }
+ if (this.allowConflicts) return false;
+ return true;
+ }
+
private handleCancelTap(e: Event) {
e.preventDefault();
e.stopPropagation();
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog_test.ts b/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog_test.ts
index 2644d81..3064014 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-rebase-dialog/gr-confirm-rebase-dialog_test.ts
@@ -9,6 +9,7 @@
import {
pressKey,
queryAndAssert,
+ stubFlags,
stubRestApi,
waitUntil,
} from '../../../test/test-utils';
@@ -22,6 +23,7 @@
let element: GrConfirmRebaseDialog;
setup(async () => {
+ stubFlags('isEnabled').returns(true);
element = await fixture(
html`<gr-confirm-rebase-dialog></gr-confirm-rebase-dialog>`
);
@@ -78,7 +80,7 @@
>
</gr-autocomplete>
</div>
- <div class="rebaseAllowConflicts">
+ <div class="rebaseCheckbox">
<input id="rebaseAllowConflicts" type="checkbox" />
<label for="rebaseAllowConflicts">
Allow rebase with conflicts
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.ts
index 3a88eaf..a20135b 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.ts
@@ -9,6 +9,7 @@
queryAndAssert,
stubRestApi,
waitEventLoop,
+ waitUntil,
} from '../../../test/test-utils';
import {GrReplyDialog} from './gr-reply-dialog';
@@ -23,6 +24,7 @@
import {GrButton} from '../../shared/gr-button/gr-button';
import {testResolver} from '../../../test/common-test-setup';
import {pluginLoaderToken} from '../../shared/gr-js-api-interface/gr-plugin-loader';
+import {GrComment} from '../../shared/gr-comment/gr-comment';
suite('gr-reply-dialog-it tests', () => {
let element: GrReplyDialog;
@@ -96,10 +98,15 @@
});
test('lgtm plugin', async () => {
+ const attachStub = sinon.stub();
+ const callbackStub = sinon.stub();
window.Gerrit.install(
plugin => {
const replyApi = plugin.changeReply();
+ const hook = plugin.hook('reply-text');
+ hook.onAttached(attachStub);
replyApi.addReplyTextChangedCallback(text => {
+ callbackStub(text);
const label = 'Code-Review';
const labelValue = replyApi.getLabelValue(label);
if (labelValue && labelValue === ' 0' && text.indexOf('LGTM') === 0) {
@@ -114,16 +121,27 @@
setupElement(element);
const pluginLoader = testResolver(pluginLoaderToken);
pluginLoader.loadPlugins([]);
- await pluginLoader.awaitPluginsLoaded();
- await waitEventLoop();
- await waitEventLoop();
+ // This may seem a bit weird, but we have to somehow make sure that the
+ // event listener is actually installed, and apparently a `gr-comment` is
+ // attached twice inside the 'reply-text' endpoint. Could not find a better
+ // way to make sure that the callback is ready to receive events.
+ await waitUntil(() => attachStub.callCount === 2);
+
+ const comment = queryAndAssert<GrComment>(
+ element,
+ 'gr-comment#patchsetLevelComment'
+ );
+ comment.messageText = 'LGTM';
+
+ await waitUntil(() => callbackStub.calledWith('LGTM'));
+
const labelScoreRows = queryAndAssert(
element.getLabelScores(),
'gr-label-score-row[name="Code-Review"]'
);
const selectedBtn = queryAndAssert(
labelScoreRows,
- 'gr-button[data-value="+1"]'
+ 'gr-button[data-value="+1"].iron-selected'
);
assert.isOk(selectedBtn);
});
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
index 893eeb9..38acf80 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -914,6 +914,8 @@
}}
@comment-text-changed=${(e: ValueChangedEvent<string>) => {
this.patchsetLevelDraftMessage = e.detail.value;
+ // See `addReplyTextChangedCallback` in `ChangeReplyPluginApi`.
+ fire(e.currentTarget as HTMLElement, 'value-changed', e.detail);
}}
.messagePlaceholder=${this.messagePlaceholder}
hide-header
diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts
index 1cc8468..a729e44 100644
--- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts
+++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts
@@ -383,7 +383,7 @@
></gr-smart-search>
<gr-endpoint-decorator
class="hideOnMobile"
- name="header-browse-source"
+ name="header-top-right"
></gr-endpoint-decorator>
<gr-endpoint-decorator class="feedbackButton" name="header-feedback">
${this.renderFeedback()}
diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.ts b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.ts
index eced28c..155ba10 100644
--- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.ts
+++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.ts
@@ -63,10 +63,7 @@
</gr-endpoint-decorator>
<gr-smart-search id="search" label="Search for changes">
</gr-smart-search>
- <gr-endpoint-decorator
- class="hideOnMobile"
- name="header-browse-source"
- >
+ <gr-endpoint-decorator class="hideOnMobile" name="header-top-right">
</gr-endpoint-decorator>
<gr-endpoint-decorator
class="feedbackButton"
diff --git a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts
index 12b19c7..cf555eb 100644
--- a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts
+++ b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts
@@ -39,6 +39,7 @@
} from '../../../models/views/change';
import {userModelToken} from '../../../models/user/user-model';
import {storageServiceToken} from '../../../services/storage/gr-storage_impl';
+import {isDarkTheme} from '../../../utils/theme-util';
const RESTORED_MESSAGE = 'Content restored from a previous edit.';
const SAVING_MESSAGE = 'Saving changes...';
@@ -88,6 +89,8 @@
// private but used in test
@state() latestPatchsetNumber?: RevisionPatchSetNum;
+ @state() private darkMode = false;
+
private readonly restApiService = getAppContext().restApiService;
private readonly reporting = getAppContext().reportingService;
@@ -132,6 +135,13 @@
() => this.getChangeModel().latestPatchNumWithEdit$,
x => (this.latestPatchsetNumber = x)
);
+ subscribe(
+ this,
+ () => this.getUserModel().preferenceTheme$,
+ theme => {
+ this.darkMode = isDarkTheme(theme);
+ }
+ );
this.shortcuts.addLocal({key: 's', modifiers: [Modifier.CTRL_KEY]}, () =>
this.handleSaveShortcut()
);
@@ -284,6 +294,10 @@
name="lineNum"
.value=${this.viewState?.editView?.lineNum}
></gr-endpoint-param>
+ <gr-endpoint-param
+ name="darkMode"
+ .value=${this.darkMode}
+ ></gr-endpoint-param>
<gr-default-editor
id="file"
.fileContent=${this.newContent}
diff --git a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view_test.ts b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view_test.ts
index 0a36a05..a2c92c5 100644
--- a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view_test.ts
+++ b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view_test.ts
@@ -117,6 +117,7 @@
<gr-endpoint-param name="prefs"> </gr-endpoint-param>
<gr-endpoint-param name="fileType"> </gr-endpoint-param>
<gr-endpoint-param name="lineNum"> </gr-endpoint-param>
+ <gr-endpoint-param name="darkMode"> </gr-endpoint-param>
<gr-default-editor id="file"> </gr-default-editor>
</gr-endpoint-decorator>
</div>
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
index b8a77db..bfa27a2 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -870,14 +870,15 @@
}
private renderShowFixButton() {
- if (!(this.comment as RobotCommentInfo)?.fix_suggestions) return;
+ const fix_suggestions = (this.comment as RobotCommentInfo)?.fix_suggestions;
+ if (!fix_suggestions || fix_suggestions.length === 0) return;
return html`
<gr-button
link
secondary
class="action show-fix"
?disabled=${this.saving}
- @click=${this.handleShowFix}
+ @click=${() => this.handleShowFix()}
>
Show Fix
</gr-button>
diff --git a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.ts b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.ts
index 023d8b5..011b80c 100644
--- a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.ts
+++ b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text.ts
@@ -104,6 +104,9 @@
word-wrap: var(--linked-text-word-wrap, break-word);
}
.markdown-html {
+ /* code overrides white-space to pre, everything else should wrap as
+ normal. */
+ white-space: normal;
/* prose will automatically wrap but inline <code> blocks won't and we
should overflow in that case rather than wrapping or leaking out */
overflow-x: auto;
@@ -116,7 +119,15 @@
subscribe(
this,
() => this.getConfigModel().repoCommentLinks$,
- repoCommentLinks => (this.repoCommentLinks = repoCommentLinks)
+ repoCommentLinks => {
+ this.repoCommentLinks = repoCommentLinks;
+ // Always linkify URLs starting with https?://
+ this.repoCommentLinks.ALWAYS_LINK_HTTP = {
+ match: '(https?://\\S+[\\w/])',
+ link: '$1',
+ enabled: true,
+ };
+ }
);
}
@@ -140,15 +151,36 @@
}
private renderAsMarkdown() {
- // need to find out here, since customRender is not arrow function
+ // Need to find out here, since customRender is not arrow function
const suggestEditsEnable = this.flagsService.isEnabled(
KnownExperimentId.SUGGEST_EDIT
);
- // <marked-element> internals will be in charge of calling our custom
- // renderer so we wrap 'this.rewriteText' so that 'this' is preserved via
- // closure.
- const boundRewriteText = (text: string) =>
- linkifyUrlsAndApplyRewrite(text, this.repoCommentLinks);
+ // Bind `this` via closure.
+ const boundRewriteText = (text: string) => {
+ const nonAsteriskRewrites = Object.fromEntries(
+ Object.entries(this.repoCommentLinks).filter(
+ ([_name, rewrite]) => !rewrite.match.includes('\\*')
+ )
+ );
+ return linkifyUrlsAndApplyRewrite(text, nonAsteriskRewrites);
+ };
+
+ // Due to a tokenizer bug in the old version of markedjs we use, text with a
+ // single asterisk is separated into 2 tokens before passing to renderer
+ // ['text'] which breaks our rewrites that would span across the 2 tokens.
+ // Since upgrading our markedjs version is infeasible, we are applying those
+ // asterisk rewrites again at the end (using renderer['paragraph'] hook)
+ // after all the nodes are combined.
+ // Bind `this` via closure.
+ const boundRewriteAsterisks = (text: string) => {
+ const asteriskRewrites = Object.fromEntries(
+ Object.entries(this.repoCommentLinks).filter(([_name, rewrite]) =>
+ rewrite.match.includes('\\*')
+ )
+ );
+ const linkedText = linkifyUrlsAndApplyRewrite(text, asteriskRewrites);
+ return `<p>${linkedText}</p>`;
+ };
// We are overriding some marked-element renderers for a few reasons:
// 1. Disable inline images as a design/policy choice.
@@ -180,15 +212,20 @@
renderer['code'] = (text: string, infostring: string) => {
if (suggestEditsEnable && infostring === USER_SUGGESTION_INFO_STRING) {
// default santizer in markedjs is very restrictive, we need to use
- // existing html element to mark element. We cannot use css class for it.
- // Therefore we pick mark - as not frequently used html element to represent
- // unconverted gr-user-suggestion-fix.
- // TODO(milutin): Find a way to override sanitizer to directly use gr-user-suggestion-fix
+ // existing html element to mark element. We cannot use css class for
+ // it. Therefore we pick mark - as not frequently used html element to
+ // represent unconverted gr-user-suggestion-fix.
+ // TODO(milutin): Find a way to override sanitizer to directly use
+ // gr-user-suggestion-fix
return `<mark>${text}</mark>`;
} else {
return `<pre><code>${text}</code></pre>`;
}
};
+ // <marked-element> internals will be in charge of calling our custom
+ // renderer so we write these functions separately so that 'this' is
+ // preserved via closure.
+ renderer['paragraph'] = boundRewriteAsterisks;
renderer['text'] = boundRewriteText;
}
diff --git a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts
index 0e5117a..81048a3 100644
--- a/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-formatted-text/gr-formatted-text_test.ts
@@ -208,6 +208,38 @@
/* HTML */ '<pre class="plaintext"># A Markdown Heading</pre>'
);
});
+
+ test('does default linking', async () => {
+ element.content = 'http://www.google.com';
+ await element.updateComplete;
+ assert.shadowDom.equal(
+ element,
+ /* HTML*/ `
+ <pre class="plaintext">
+ <a
+ href="http://www.google.com"
+ rel="noopener"
+ target="_blank"
+ >http://www.google.com</a>
+ </pre>
+ `
+ );
+
+ element.content = 'https://www.google.com';
+ await element.updateComplete;
+ assert.shadowDom.equal(
+ element,
+ /* HTML*/ `
+ <pre class="plaintext">
+ <a
+ href="https://www.google.com"
+ rel="noopener"
+ target="_blank"
+ >https://www.google.com</a>
+ </pre>
+ `
+ );
+ });
});
suite('as markdown', () => {
@@ -588,6 +620,74 @@
);
});
+ test('renders rewrites with an asterisk', async () => {
+ await setCommentLinks({
+ customLinkRewrite: {
+ match: 'asterisks (\\*) rule',
+ link: 'http://google.com',
+ },
+ });
+
+ element.content = 'I think asterisks * rule';
+ await element.updateComplete;
+
+ assert.shadowDom.equal(
+ element,
+ /* HTML */ `
+ <marked-element>
+ <div slot="markdown-html" class="markdown-html">
+ <p>
+ I think
+ <a href="http://google.com" rel="noopener" target="_blank"
+ >asterisks * rule</a
+ >
+ </p>
+ </div>
+ </marked-element>
+ `
+ );
+ });
+
+ test('does default linking', async () => {
+ element.content = 'http://www.google.com';
+ await element.updateComplete;
+ assert.shadowDom.equal(
+ element,
+ /* HTML*/ `
+ <marked-element>
+ <div slot="markdown-html" class="markdown-html">
+ <p>
+ <a
+ href="http://www.google.com"
+ rel="noopener"
+ target="_blank"
+ >http://www.google.com</a>
+ </p>
+ </div>
+ </marked-element>
+ `
+ );
+
+ element.content = 'https://www.google.com';
+ await element.updateComplete;
+ assert.shadowDom.equal(
+ element,
+ /* HTML*/ `
+ <marked-element>
+ <div slot="markdown-html" class="markdown-html">
+ <p>
+ <a
+ href="https://www.google.com"
+ rel="noopener"
+ target="_blank"
+ >https://www.google.com</a>
+ </p>
+ </div>
+ </marked-element>
+ `
+ );
+ });
+
suite('user suggest fix', () => {
setup(async () => {
const flagsService = getAppContext().flagsService;
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
index a16a1d1..386a166 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
@@ -76,6 +76,7 @@
}
async handleShowChange(detail: ShowChangeDetail) {
+ if (!detail.change) return;
await this.waitForPluginsToLoad();
// Note (issue 8221) Shallow clone the change object and add a mergeable
// getter with deprecation warning. This makes the change detail appear as
diff --git a/polygerrit-ui/app/services/flags/flags.ts b/polygerrit-ui/app/services/flags/flags.ts
index 7488e79..5c7bf2d 100644
--- a/polygerrit-ui/app/services/flags/flags.ts
+++ b/polygerrit-ui/app/services/flags/flags.ts
@@ -19,4 +19,5 @@
PUSH_NOTIFICATIONS_DEVELOPER = 'UiFeature__push_notifications_developer',
PUSH_NOTIFICATIONS = 'UiFeature__push_notifications',
SUGGEST_EDIT = 'UiFeature__suggest_edit',
+ REBASE_ON_BEHALF_OF_UPLOADER = 'UiFeature__rebase_on_behalf_of_uploader',
}
diff --git a/polygerrit-ui/app/test/test-data-generators.ts b/polygerrit-ui/app/test/test-data-generators.ts
index a927a5b..a3f24d4 100644
--- a/polygerrit-ui/app/test/test-data-generators.ts
+++ b/polygerrit-ui/app/test/test-data-generators.ts
@@ -867,7 +867,24 @@
robot_id: 'robot-id-123' as RobotId,
robot_run_id: 'robot-run-id-456' as RobotRunId,
properties: {},
- fix_suggestions: [],
+ fix_suggestions: [
+ {
+ fix_id: 'robot-run-id-456-fix' as FixId,
+ description: 'Robot suggestion',
+ replacements: [
+ {
+ path: 'abc.txt'!,
+ range: {
+ start_line: 0,
+ start_character: 0,
+ end_line: 1,
+ end_character: 10,
+ },
+ replacement: 'replacement',
+ },
+ ],
+ },
+ ],
...extra,
};
}
diff --git a/polygerrit-ui/app/utils/link-util.ts b/polygerrit-ui/app/utils/link-util.ts
index ccafa5a..48e9c07 100644
--- a/polygerrit-ui/app/utils/link-util.ts
+++ b/polygerrit-ui/app/utils/link-util.ts
@@ -33,11 +33,6 @@
commentLinkInfo =>
commentLinkInfo.enabled !== false && commentLinkInfo.link !== undefined
);
- // Always linkify URLs starting with https?://
- enabledRewrites.push({
- match: '(https?://\\S+[\\w/])',
- link: '$1',
- });
return enabledRewrites.flatMap(rewrite => {
const regexp = new RegExp(rewrite.match, 'g');
const partialResults: RewriteResult[] = [];
diff --git a/polygerrit-ui/app/utils/link-util_test.ts b/polygerrit-ui/app/utils/link-util_test.ts
index 52b8288..e4e719b 100644
--- a/polygerrit-ui/app/utils/link-util_test.ts
+++ b/polygerrit-ui/app/utils/link-util_test.ts
@@ -12,17 +12,6 @@
}
suite('link rewrites', () => {
- test('default linking', () => {
- assert.equal(
- linkifyUrlsAndApplyRewrite('http://www.google.com', {}),
- link('http://www.google.com', 'http://www.google.com')
- );
- assert.equal(
- linkifyUrlsAndApplyRewrite('https://www.google.com', {}),
- link('https://www.google.com', 'https://www.google.com')
- );
- });
-
test('without text', () => {
assert.equal(
linkifyUrlsAndApplyRewrite('foo', {
