Allow a Link trailer to be used as an alternative change ID trailer

In some projects it may be desirable for the trailer to contain a link to
the Gerrit review page so that it is convenient to access the review page
starting from the commit message. The Link trailer is a standard trailer
used for inserting links in the commit message, which has been adopted by
the Linux kernel among other projects. For example, the Linux kernel has a
policy about linking to the mailing list archive with Link trailers:
https://www.kernel.org/doc/html/latest/maintainer/configure-git.html#creating-commit-links-to-lore-kernel-org

This change makes Gerrit interoperate well with the Link trailer. Specifically,
it teaches the Gerrit server to recognize trailers of the form:

    Link: https://gerrit-review.googlesource.com/id/I78e884a944cedb5144f661a057e4829c8f84e933

as well as the existing Change-Id trailer, teaches the server to recognize
/id/ as a search prefix for change IDs and modifies the commit-msg hook
to optionally add a Link trailer (using a server URL provided in the
property gerrit.reviewUrl) instead of the Change-Id trailer.

Change-Id: I78e884a944cedb5144f661a057e4829c8f84e933
This commit is contained in:
Peter Collingbourne
2020-08-18 14:42:44 -07:00
parent 8f32d8238a
commit 8cab93302d
21 changed files with 228 additions and 34 deletions

View File

@@ -56,6 +56,26 @@ change viewed on the web.
The `Change-Id` will not be added if `gerrit.createChangeId` is set
to `false` in the git config.
If `gerrit.reviewUrl` is set to the base URL of the Gerrit server that
changes are uploaded to (e.g. `https://gerrit-review.googlesource.com/`)
in the git config, then instead of adding a `Change-Id` trailer, a `Link`
trailer will be inserted that will look like this:
----
Improve foo widget by attaching a bar.
We want a bar, because it improves the foo by providing more
wizbangery to the dowhatimeanery.
Link: https://gerrit-review.googlesource.com/id/Ic8aaa0728a43936cd4c6e1ed590e01ba8f0fbf5b
Signed-off-by: A. U. Thor <author@example.com>
----
This link will become a valid link to the review page once the change is
uploaded to the Gerrit server. Newer versions of the Gerrit server will read
the change identifier out of the appropriate `Link` trailer and treat it in
the same way as the change identifier in a `Change-Id` trailer.
== OBTAINING
To obtain the `commit-msg` script use `scp`, `wget` or `curl` to download

View File

@@ -203,6 +203,34 @@ While there are several ways you can add a Change-Id, the standard
method uses git's link:cmd-hook-commit-msg.html[commit-msg hook]
to automatically add the Change-Id to each new commit.
== The Link footer
Gerrit also supports the Link footer as an alternative to the Change-Id
footer. A Link footer looks like this:
....
Link: https://gerrit-review.googlesource.com/id/Ic8aaa0728a43936cd4c6e1ed590e01ba8f0fbf5b
....
The advantage of this style of footer is that it usually acts
as a link directly to the change's review page, provided that
the change has been uploaded to Gerrit. Projects such as the
link:https://www.kernel.org/doc/html/latest/maintainer/configure-git.html#creating-commit-links-to-lore-kernel-org[Linux kernel]
have a convention of adding links to commit messages using the
Link footer.
If multiple changes have been uploaded to Gerrit with the same
change ID, for example if a change has been cherry-picked to multiple
branches, the link will take the user to a list of changes.
The base URL in the footer is required to match the server's base URL.
If the URL does not match, the server will not recognize the footer
as a change ID footer.
The link:cmd-hook-commit-msg.html[commit-msg hook] can be configured
to insert Link footers instead of Change-Id footers by setting the
property `gerrit.reviewUrl` to the base URL of the Gerrit server.
GERRIT
------
Part of link:index.html[Gerrit Code Review]

View File

@@ -20,6 +20,9 @@ public class FooterConstants {
/** The change ID as used to track patch sets. */
public static final FooterKey CHANGE_ID = new FooterKey("Change-Id");
/** Link is an alternative footer that may be used to track patch sets. */
public static final FooterKey LINK = new FooterKey("Link");
/** The footer telling us who reviewed the change. */
public static final FooterKey REVIEWED_BY = new FooterKey("Reviewed-by");

View File

@@ -69,6 +69,7 @@ public class StaticModule extends ServletModule {
ImmutableList.of(
"/",
"/c/*",
"/id/*",
"/p/*",
"/q/*",
"/x/*",

View File

@@ -19,17 +19,24 @@ import static java.util.stream.Collectors.toSet;
import com.google.common.collect.Ordering;
import com.google.common.io.BaseEncoding;
import com.google.gerrit.common.FooterConstants;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.server.config.UrlFormatter;
import com.google.inject.Singleton;
import java.io.IOException;
import java.security.SecureRandom;
import java.util.Collection;
import java.util.List;
import java.util.Optional;
import java.util.Random;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Stream;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
@Singleton
public class ChangeUtil {
@@ -106,5 +113,29 @@ public class ChangeUtil {
return c != null ? c.getStatus().name().toLowerCase() : "deleted";
}
private static final Pattern LINK_CHANGE_ID_PATTERN = Pattern.compile("I[0-9a-f]{40}");
public static List<String> getChangeIdsFromFooter(RevCommit c, UrlFormatter urlFormatter) {
List<String> changeIds = c.getFooterLines(FooterConstants.CHANGE_ID);
Optional<String> webUrl = urlFormatter.getWebUrl();
if (!webUrl.isPresent()) {
return changeIds;
}
String prefix = webUrl.get() + "id/";
for (String link : c.getFooterLines(FooterConstants.LINK)) {
if (!link.startsWith(prefix)) {
continue;
}
String changeId = link.substring(prefix.length());
Matcher m = LINK_CHANGE_ID_PATTERN.matcher(changeId);
if (m.matches()) {
changeIds.add(changeId);
}
}
return changeIds;
}
private ChangeUtil() {}
}

View File

@@ -28,7 +28,6 @@ import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.Streams;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.FooterConstants;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
@@ -41,17 +40,20 @@ import com.google.gerrit.entities.PatchSetInfo;
import com.google.gerrit.entities.SubmissionId;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.ReviewerAdder.InternalAddReviewerInput;
import com.google.gerrit.server.change.ReviewerAdder.ReviewerAddition;
import com.google.gerrit.server.change.ReviewerAdder.ReviewerAdditionList;
import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.config.UrlFormatter;
import com.google.gerrit.server.events.CommitReceivedEvent;
import com.google.gerrit.server.extensions.events.CommentAdded;
import com.google.gerrit.server.extensions.events.RevisionCreated;
@@ -110,6 +112,7 @@ public class ChangeInserter implements InsertChangeOp {
private final CommentAdded commentAdded;
private final ReviewerAdder reviewerAdder;
private final MessageIdGenerator messageIdGenerator;
private final DynamicItem<UrlFormatter> urlFormatter;
private final Change.Id changeId;
private final PatchSet.Id psId;
@@ -159,6 +162,7 @@ public class ChangeInserter implements InsertChangeOp {
RevisionCreated revisionCreated,
ReviewerAdder reviewerAdder,
MessageIdGenerator messageIdGenerator,
DynamicItem<UrlFormatter> urlFormatter,
@Assisted Change.Id changeId,
@Assisted ObjectId commitId,
@Assisted String refName) {
@@ -175,6 +179,7 @@ public class ChangeInserter implements InsertChangeOp {
this.commentAdded = commentAdded;
this.reviewerAdder = reviewerAdder;
this.messageIdGenerator = messageIdGenerator;
this.urlFormatter = urlFormatter;
this.changeId = changeId;
this.psId = PatchSet.id(changeId, INITIAL_PATCH_SET_ID);
@@ -191,7 +196,7 @@ public class ChangeInserter implements InsertChangeOp {
public Change createChange(Context ctx) throws IOException {
change =
new Change(
getChangeKey(ctx.getRevWalk(), commitId),
getChangeKey(ctx.getRevWalk()),
changeId,
ctx.getAccountId(),
BranchNameKey.create(ctx.getProject(), refName),
@@ -206,10 +211,10 @@ public class ChangeInserter implements InsertChangeOp {
return change;
}
private static Change.Key getChangeKey(RevWalk rw, ObjectId id) throws IOException {
RevCommit commit = rw.parseCommit(id);
private Change.Key getChangeKey(RevWalk rw) throws IOException {
RevCommit commit = rw.parseCommit(commitId);
rw.parseBody(commit);
List<String> idList = commit.getFooterLines(FooterConstants.CHANGE_ID);
List<String> idList = ChangeUtil.getChangeIdsFromFooter(commit, urlFormatter.get());
if (!idList.isEmpty()) {
return Change.key(idList.get(idList.size() - 1).trim());
}

View File

@@ -28,7 +28,6 @@ import com.google.common.collect.Iterables;
import com.google.common.collect.MultimapBuilder;
import com.google.common.collect.SetMultimap;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.FooterConstants;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
@@ -38,12 +37,14 @@ import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.api.changes.FixInput;
import com.google.gerrit.extensions.common.ProblemInfo;
import com.google.gerrit.extensions.common.ProblemInfo.Status;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.config.UrlFormatter;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.PatchSetState;
@@ -115,6 +116,7 @@ public class ConsistencyChecker {
private final Provider<CurrentUser> user;
private final Provider<PersonIdent> serverIdent;
private final RetryHelper retryHelper;
private final DynamicItem<UrlFormatter> urlFormatter;
private BatchUpdate.Factory updateFactory;
private FixInput fix;
@@ -141,7 +143,8 @@ public class ConsistencyChecker {
PatchSetInserter.Factory patchSetInserterFactory,
PatchSetUtil psUtil,
Provider<CurrentUser> user,
RetryHelper retryHelper) {
RetryHelper retryHelper,
DynamicItem<UrlFormatter> urlFormatter) {
this.accounts = accounts;
this.accountPatchReviewStore = accountPatchReviewStore;
this.notesFactory = notesFactory;
@@ -152,6 +155,7 @@ public class ConsistencyChecker {
this.retryHelper = retryHelper;
this.serverIdent = serverIdent;
this.user = user;
this.urlFormatter = urlFormatter;
reset();
}
@@ -456,7 +460,8 @@ public class ConsistencyChecker {
// No patch set for this commit; insert one.
rw.parseBody(commit);
String changeId =
Iterables.getFirst(commit.getFooterLines(FooterConstants.CHANGE_ID), null);
Iterables.getFirst(
ChangeUtil.getChangeIdsFromFooter(commit, urlFormatter.get()), null);
// Missing Change-Id footer is ok, but mismatched is not.
if (changeId != null && !changeId.equals(change().getKey().get())) {
problem(

View File

@@ -48,6 +48,7 @@ import com.google.gerrit.extensions.restapi.MergeConflictException;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.UrlFormatter;
@@ -531,7 +532,7 @@ public class MergeUtil {
msgbuf.append('\n');
}
if (!contains(footers, FooterConstants.CHANGE_ID, c.getKey().get())) {
if (ChangeUtil.getChangeIdsFromFooter(n, urlFormatter.get()).isEmpty()) {
msgbuf.append(FooterConstants.CHANGE_ID.getName());
msgbuf.append(": ");
msgbuf.append(c.getKey().get());

View File

@@ -60,7 +60,6 @@ import com.google.common.collect.Sets;
import com.google.common.collect.SortedSetMultimap;
import com.google.common.collect.Streams;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.FooterConstants;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.UsedAt;
import com.google.gerrit.entities.Account;
@@ -117,6 +116,7 @@ import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.PluginConfig;
import com.google.gerrit.server.config.ProjectConfigEntry;
import com.google.gerrit.server.config.UrlFormatter;
import com.google.gerrit.server.edit.ChangeEdit;
import com.google.gerrit.server.edit.ChangeEditUtil;
import com.google.gerrit.server.git.BanCommit;
@@ -347,6 +347,7 @@ class ReceiveCommits {
private final ProjectConfig.Factory projectConfigFactory;
private final SetPrivateOp.Factory setPrivateOpFactory;
private final ReplyAttentionSetUpdates replyAttentionSetUpdates;
private final DynamicItem<UrlFormatter> urlFormatter;
// Assisted injected fields.
private final ProjectState projectState;
@@ -427,6 +428,7 @@ class ReceiveCommits {
TagCache tagCache,
SetPrivateOp.Factory setPrivateOpFactory,
ReplyAttentionSetUpdates replyAttentionSetUpdates,
DynamicItem<UrlFormatter> urlFormatter,
@Assisted ProjectState projectState,
@Assisted IdentifiedUser user,
@Assisted ReceivePack rp,
@@ -475,6 +477,7 @@ class ReceiveCommits {
this.projectConfigFactory = projectConfigFactory;
this.setPrivateOpFactory = setPrivateOpFactory;
this.replyAttentionSetUpdates = replyAttentionSetUpdates;
this.urlFormatter = urlFormatter;
// Assisted injected fields.
this.projectState = projectState;
@@ -2092,7 +2095,7 @@ class ReceiveCommits {
} catch (IOException e) {
throw new StorageException("Can't parse commit", e);
}
List<String> idList = create.commit.getFooterLines(FooterConstants.CHANGE_ID);
List<String> idList = ChangeUtil.getChangeIdsFromFooter(create.commit, urlFormatter.get());
if (idList.isEmpty()) {
messages.add(
@@ -2184,7 +2187,7 @@ class ReceiveCommits {
}
}
List<String> idList = c.getFooterLines(FooterConstants.CHANGE_ID);
List<String> idList = ChangeUtil.getChangeIdsFromFooter(c, urlFormatter.get());
if (!idList.isEmpty()) {
pending.put(c, lookupByChangeKey(c, Change.key(idList.get(idList.size() - 1).trim())));
} else {
@@ -3323,7 +3326,8 @@ class ReceiveCommits {
}
}
for (String changeId : c.getFooterLines(FooterConstants.CHANGE_ID)) {
for (String changeId :
ChangeUtil.getChangeIdsFromFooter(c, urlFormatter.get())) {
if (byKey == null) {
byKey =
retryHelper

View File

@@ -16,7 +16,6 @@ package com.google.gerrit.server.git.receive;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.gerrit.common.FooterConstants.CHANGE_ID;
import static com.google.gerrit.server.change.ReviewerAdder.newAddReviewerInputFromCommitIdentity;
import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromFooters;
import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromReviewers;
@@ -41,11 +40,13 @@ import com.google.gerrit.extensions.api.changes.AddReviewerInput;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.client.ChangeKind;
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.change.AddReviewersOp;
@@ -56,6 +57,7 @@ import com.google.gerrit.server.change.ReviewerAdder.InternalAddReviewerInput;
import com.google.gerrit.server.change.ReviewerAdder.ReviewerAddition;
import com.google.gerrit.server.change.ReviewerAdder.ReviewerAdditionList;
import com.google.gerrit.server.config.SendEmailExecutor;
import com.google.gerrit.server.config.UrlFormatter;
import com.google.gerrit.server.extensions.events.CommentAdded;
import com.google.gerrit.server.extensions.events.RevisionCreated;
import com.google.gerrit.server.git.MergedByPushOp;
@@ -131,6 +133,7 @@ public class ReplaceOp implements BatchUpdateOp {
private final ReviewerAdder reviewerAdder;
private final Change change;
private final MessageIdGenerator messageIdGenerator;
private final DynamicItem<UrlFormatter> urlFormatter;
private final ProjectState projectState;
private final BranchNameKey dest;
@@ -175,6 +178,7 @@ public class ReplaceOp implements BatchUpdateOp {
ReviewerAdder reviewerAdder,
Change change,
MessageIdGenerator messageIdGenerator,
DynamicItem<UrlFormatter> urlFormatter,
@Assisted ProjectState projectState,
@Assisted BranchNameKey dest,
@Assisted boolean checkMergedInto,
@@ -202,6 +206,7 @@ public class ReplaceOp implements BatchUpdateOp {
this.reviewerAdder = reviewerAdder;
this.change = change;
this.messageIdGenerator = messageIdGenerator;
this.urlFormatter = urlFormatter;
this.projectState = projectState;
this.dest = dest;
@@ -483,7 +488,7 @@ public class ReplaceOp implements BatchUpdateOp {
change.setStatus(Change.Status.NEW);
change.setCurrentPatchSet(info);
List<String> idList = commit.getFooterLines(CHANGE_ID);
List<String> idList = ChangeUtil.getChangeIdsFromFooter(commit, urlFormatter.get());
change.setKey(Change.key(idList.get(idList.size() - 1).trim()));
}

View File

@@ -34,6 +34,7 @@ import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.externalids.ExternalIdsConsistencyChecker;
@@ -302,7 +303,7 @@ public class CommitValidators {
}
RevCommit commit = receiveEvent.commit;
List<CommitValidationMessage> messages = new ArrayList<>();
List<String> idList = commit.getFooterLines(FooterConstants.CHANGE_ID);
List<String> idList = ChangeUtil.getChangeIdsFromFooter(commit, urlFormatter);
if (idList.isEmpty()) {
String shortMsg = commit.getShortMessage();

View File

@@ -14,7 +14,6 @@
package com.google.gerrit.server.project;
import static com.google.gerrit.common.FooterConstants.CHANGE_ID;
import static com.google.gerrit.index.query.Predicate.and;
import static com.google.gerrit.index.query.Predicate.or;
import static com.google.gerrit.server.query.change.ChangeStatusPredicate.open;
@@ -36,13 +35,16 @@ import com.google.gerrit.extensions.api.projects.CheckProjectResultInfo;
import com.google.gerrit.extensions.api.projects.CheckProjectResultInfo.AutoCloseableChangesCheckResult;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.index.IndexConfig;
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.config.UrlFormatter;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.query.change.ChangeData;
@@ -75,17 +77,20 @@ public class ProjectsConsistencyChecker {
private final RetryHelper retryHelper;
private final ChangeJson.Factory changeJsonFactory;
private final IndexConfig indexConfig;
private final DynamicItem<UrlFormatter> urlFormatter;
@Inject
ProjectsConsistencyChecker(
GitRepositoryManager repoManager,
RetryHelper retryHelper,
ChangeJson.Factory changeJsonFactory,
IndexConfig indexConfig) {
IndexConfig indexConfig,
DynamicItem<UrlFormatter> urlFormatter) {
this.repoManager = repoManager;
this.retryHelper = retryHelper;
this.changeJsonFactory = changeJsonFactory;
this.indexConfig = indexConfig;
this.urlFormatter = urlFormatter;
}
public CheckProjectResultInfo check(Project.NameKey projectName, CheckProjectInput input)
@@ -172,7 +177,7 @@ public class ProjectsConsistencyChecker {
mergedSha1s.add(commitId);
// Consider all Change-Id lines since this is what ReceiveCommits#autoCloseChanges does.
List<String> changeIds = commit.getFooterLines(CHANGE_ID);
List<String> changeIds = ChangeUtil.getChangeIdsFromFooter(commit, urlFormatter.get());
// Number of predicates that we need to add for this commit, 1 per Change-Id plus one for
// the commit.

View File

@@ -20,7 +20,6 @@ import static com.google.gerrit.server.project.ProjectCache.noSuchProject;
import com.google.auto.value.AutoValue;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.common.FooterConstants;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.BranchNameKey;
@@ -29,6 +28,7 @@ import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.api.changes.CherryPickInput;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.MergeConflictException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -43,6 +43,7 @@ import com.google.gerrit.server.change.ChangeInserter;
import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.change.PatchSetInserter;
import com.google.gerrit.server.change.SetCherryPickOp;
import com.google.gerrit.server.config.UrlFormatter;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -112,6 +113,7 @@ public class CherryPickChange {
private final ApprovalsUtil approvalsUtil;
private final NotifyResolver notifyResolver;
private final BatchUpdate.Factory batchUpdateFactory;
private final DynamicItem<UrlFormatter> urlFormatter;
@Inject
CherryPickChange(
@@ -128,7 +130,8 @@ public class CherryPickChange {
ProjectCache projectCache,
ApprovalsUtil approvalsUtil,
NotifyResolver notifyResolver,
BatchUpdate.Factory batchUpdateFactory) {
BatchUpdate.Factory batchUpdateFactory,
DynamicItem<UrlFormatter> urlFormatter) {
this.seq = seq;
this.queryProvider = queryProvider;
this.gitManager = gitManager;
@@ -143,6 +146,7 @@ public class CherryPickChange {
this.approvalsUtil = approvalsUtil;
this.notifyResolver = notifyResolver;
this.batchUpdateFactory = batchUpdateFactory;
this.urlFormatter = urlFormatter;
}
/**
@@ -312,7 +316,8 @@ public class CherryPickChange {
input.allowConflicts);
Change.Key changeKey;
final List<String> idList = cherryPickCommit.getFooterLines(FooterConstants.CHANGE_ID);
final List<String> idList =
ChangeUtil.getChangeIdsFromFooter(cherryPickCommit, urlFormatter.get());
if (!idList.isEmpty()) {
final String idStr = idList.get(idList.size() - 1).trim();
changeKey = Change.key(idStr);

View File

@@ -21,6 +21,7 @@ import com.google.gerrit.entities.BooleanProjectConfig;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.common.CommitMessageInput;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -34,6 +35,7 @@ import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.NotifyResolver;
import com.google.gerrit.server.change.PatchSetInserter;
import com.google.gerrit.server.config.UrlFormatter;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
@@ -73,6 +75,7 @@ public class PutMessage implements RestModifyView<ChangeResource, CommitMessageI
private final PatchSetUtil psUtil;
private final NotifyResolver notifyResolver;
private final ProjectCache projectCache;
private final DynamicItem<UrlFormatter> urlFormatter;
@Inject
PutMessage(
@@ -84,7 +87,8 @@ public class PutMessage implements RestModifyView<ChangeResource, CommitMessageI
@GerritPersonIdent PersonIdent gerritIdent,
PatchSetUtil psUtil,
NotifyResolver notifyResolver,
ProjectCache projectCache) {
ProjectCache projectCache,
DynamicItem<UrlFormatter> urlFormatter) {
this.updateFactory = updateFactory;
this.repositoryManager = repositoryManager;
this.userProvider = userProvider;
@@ -94,6 +98,7 @@ public class PutMessage implements RestModifyView<ChangeResource, CommitMessageI
this.psUtil = psUtil;
this.notifyResolver = notifyResolver;
this.projectCache = projectCache;
this.urlFormatter = urlFormatter;
}
@Override
@@ -200,7 +205,7 @@ public class PutMessage implements RestModifyView<ChangeResource, CommitMessageI
}
}
private static String ensureChangeIdIsCorrect(
private String ensureChangeIdIsCorrect(
boolean requireChangeId, String currentChangeId, String newCommitMessage)
throws ResourceConflictException, BadRequestException {
RevCommit revCommit =
@@ -210,7 +215,7 @@ public class PutMessage implements RestModifyView<ChangeResource, CommitMessageI
// Check that the commit message without footers is not empty
CommitMessageUtil.checkAndSanitizeCommitMessage(revCommit.getShortMessage());
List<String> changeIdFooters = revCommit.getFooterLines(FooterConstants.CHANGE_ID);
List<String> changeIdFooters = ChangeUtil.getChangeIdsFromFooter(revCommit, urlFormatter.get());
if (!changeIdFooters.isEmpty() && !changeIdFooters.get(0).equals(currentChangeId)) {
throw new ResourceConflictException("wrong Change-Id footer");
}

View File

@@ -1556,6 +1556,27 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
testPushWithChangeIdAboveFooter();
}
@Test
public void pushWithLinkFooter() throws Exception {
String changeId = "I0123456789abcdef0123456789abcdef01234567";
String url = cfg.getString("gerrit", null, "canonicalWebUrl");
if (!url.endsWith("/")) {
url += "/";
}
RevCommit c = createCommit(testRepo, "test commit\n\nLink: " + url + "id/" + changeId);
pushForReviewOk(testRepo);
List<ChangeMessageInfo> messages = getMessages(changeId);
assertThat(messages.get(0).message).isEqualTo("Uploaded patch set 1.");
}
@Test
public void pushWithWrongHostLinkFooter() throws Exception {
String changeId = "I0123456789abcdef0123456789abcdef01234567";
RevCommit c = createCommit(testRepo, "test commit\n\nLink: https://wronghost/id/" + changeId);
pushForReviewRejected(testRepo, "missing Change-Id in message footer");
}
@Test
public void pushWithChangeIdAboveFooterWithCreateNewChangeForAllNotInTarget() throws Exception {
enableCreateNewChangeForAllNotInTarget();

View File

@@ -78,7 +78,8 @@ const CHANGE_ID_ERROR = {
MISMATCH: 'mismatch',
MISSING: 'missing',
};
const CHANGE_ID_REGEX_PATTERN = /^Change-Id\:\s(I[0-9a-f]{8,40})/gm;
const CHANGE_ID_REGEX_PATTERN =
/^(Change-Id\:\s|Link:.*\/id\/)(I[0-9a-f]{8,40})/gm;
const MIN_LINES_FOR_COMMIT_COLLAPSE = 30;
const DEFAULT_NUM_FILES_SHOWN = 200;
@@ -1316,7 +1317,7 @@ class GrChangeView extends KeyboardShortcutMixin(
let changeIdArr;
while (changeIdArr = CHANGE_ID_REGEX_PATTERN.exec(commitMessage)) {
changeId = changeIdArr[1];
changeId = changeIdArr[2];
}
if (changeId) {

View File

@@ -160,6 +160,8 @@ const RoutePattern = {
*/
QUERY_LEGACY_SUFFIX: /^\/q\/.+,n,z$/,
CHANGE_ID_QUERY: /^\/id\/(I[0-9a-f]{40})$/,
// Matches /c/<changeNum>/[<basePatchNum>..][<patchNum>][/].
CHANGE_LEGACY: /^\/c\/(\d+)\/?(((-?\d+|edit)(\.\.(\d+|edit))?))?\/?$/,
CHANGE_NUMBER_LEGACY: /^\/(\d+)\/?/,
@@ -1018,6 +1020,8 @@ export class GrRouter extends GestureEventListeners(
this._mapRoute(RoutePattern.QUERY, '_handleQueryRoute');
this._mapRoute(RoutePattern.CHANGE_ID_QUERY, '_handleChangeIdQueryRoute');
this._mapRoute(RoutePattern.DIFF_LEGACY_LINENUM, '_handleLegacyLinenum');
this._mapRoute(
@@ -1510,6 +1514,16 @@ export class GrRouter extends GestureEventListeners(
});
}
_handleChangeIdQueryRoute(data: PageContextWithQueryMap) {
// TODO(pcc): This will need to indicate that this was a change ID query if
// standard queries gain the ability to search places like commit messages
// for change IDs.
this._setParams({
view: GerritNav.View.SEARCH,
query: data.params[0],
});
}
_handleQueryLegacySuffixRoute(ctx: PageContextWithQueryMap) {
this._redirect(ctx.path.replace(LEGACY_QUERY_SUFFIX_PATTERN, ''));
}

View File

@@ -182,6 +182,7 @@ suite('gr-router tests', () => {
'_handleBranchListFilterOffsetRoute',
'_handleBranchListFilterRoute',
'_handleBranchListOffsetRoute',
'_handleChangeIdQueryRoute',
'_handleChangeNumberLegacyRoute',
'_handleChangeRoute',
'_handleCommentRoute',
@@ -739,6 +740,14 @@ suite('gr-router tests', () => {
});
});
test('_handleChangeIdQueryRoute', () => {
const data = {params: ['I0123456789abcdef0123456789abcdef01234567']};
assertDataToParams(data, '_handleChangeIdQueryRoute', {
view: GerritNav.View.SEARCH,
query: 'I0123456789abcdef0123456789abcdef01234567',
});
});
suite('_handleRegisterRoute', () => {
test('happy path', () => {
const ctx = {params: ['/foo/bar']};

View File

@@ -482,7 +482,7 @@ type server struct{}
// Any path prefixes that should resolve to index.html.
var (
fePaths = []string{"/q/", "/c/", "/p/", "/x/", "/dashboard/", "/admin/", "/settings/"}
fePaths = []string{"/q/", "/c/", "/id/", "/p/", "/x/", "/dashboard/", "/admin/", "/settings/"}
issueNumRE = regexp.MustCompile(`^\/\d+\/?$`)
)

View File

@@ -110,6 +110,25 @@ EOF
fi
}
# gerrit.reviewUrl causes us to create Link instead of Change-Id.
function test_link {
cat << EOF > input
bla bla
EOF
git config gerrit.reviewUrl https://myhost/
${hook} input || fail "failed hook execution"
git config --unset gerrit.reviewUrl
found=$(grep -c '^Change-Id' input || true)
if [[ "${found}" != "0" ]]; then
fail "got ${found} Change-Ids, want 0"
fi
found=$(grep -c '^Link: https://myhost/id/I' input || true)
if [[ "${found}" != "1" ]]; then
fail "got ${found} Link footers, want 1"
fi
}
# Change-Id goes after existing trailers.
function test_at_end {
cat << EOF > input

View File

@@ -48,12 +48,23 @@ if test ! -s "${dest}" ; then
exit 1
fi
# Avoid the --in-place option which only appeared in Git 2.8
# Avoid the --if-exists option which only appeared in Git 2.15
if ! git -c trailer.ifexists=doNothing interpret-trailers \
--trailer "Change-Id: I${random}" < "$1" > "${dest}" ; then
echo "cannot insert change-id line in $1"
exit 1
reviewurl="$(git config --get gerrit.reviewUrl)"
if test -n "${reviewurl}" ; then
if ! git interpret-trailers --parse < "$1" | grep -q '^Link:.*/id/I[0-9a-f]\{40\}$' ; then
if ! git interpret-trailers \
--trailer "Link: ${reviewurl%/}/id/I${random}" < "$1" > "${dest}" ; then
echo "cannot insert link footer in $1"
exit 1
fi
fi
else
# Avoid the --in-place option which only appeared in Git 2.8
# Avoid the --if-exists option which only appeared in Git 2.15
if ! git -c trailer.ifexists=doNothing interpret-trailers \
--trailer "Change-Id: I${random}" < "$1" > "${dest}" ; then
echo "cannot insert change-id line in $1"
exit 1
fi
fi
if ! mv "${dest}" "$1" ; then