ReceiveCommits: uniformize commit validation error messages.
Before
[abcdabc] missing subject; Change-Id must be in commit message
footer
After
commit abcdabc: missing subject; Change-Id must be in message
footer
Change-Id: I15ca67fb591b0d237cb9217936853ec8dc706aaa
diff --git a/Documentation/error-missing-changeid.txt b/Documentation/error-missing-changeid.txt
index 9cddd85..08f2c09 100644
--- a/Documentation/error-missing-changeid.txt
+++ b/Documentation/error-missing-changeid.txt
@@ -1,4 +1,4 @@
-= missing Change-Id in commit message footer
+= commit xxxxxxx: missing Change-Id in message footer
With this error message Gerrit rejects to push a commit to a project
which is configured to always require a Change-Id in the commit
diff --git a/Documentation/error-missing-subject.txt b/Documentation/error-missing-subject.txt
index 3703ade..6ef37a4 100644
--- a/Documentation/error-missing-subject.txt
+++ b/Documentation/error-missing-subject.txt
@@ -1,4 +1,4 @@
-= missing subject; Change-Id must be in commit message footer
+= commit xxxxxxx: missing subject; Change-Id must be in message footer
With this error message Gerrit rejects to push a commit to a project
which is configured to always require a Change-Id in the commit
diff --git a/Documentation/error-multiple-changeid-lines.txt b/Documentation/error-multiple-changeid-lines.txt
index 0729547..31567f4 100644
--- a/Documentation/error-multiple-changeid-lines.txt
+++ b/Documentation/error-multiple-changeid-lines.txt
@@ -1,4 +1,4 @@
-= multiple Change-Id lines in commit message footer
+= commit xxxxxxx: multiple Change-Id lines in message footer
With this error message Gerrit rejects to push a commit if the commit
message footer of the pushed commit contains several Change-Id lines.
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index bc483bb..d04ef32 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -3057,6 +3057,10 @@
}
}
+ private String messageForCommit(RevCommit c, String msg) {
+ return String.format("commit %s: %s", c.abbreviate(RevId.ABBREV_LEN).name(), msg);
+ }
+
/**
* Validates a single commit. If the commit does not validate, the command is rejected.
*
@@ -3096,11 +3100,19 @@
repo,
receiveEvent.revWalk,
change);
- messages.addAll(validators.validate(receiveEvent));
+
+ for (CommitValidationMessage m : validators.validate(receiveEvent)) {
+ messages.add(
+ new CommitValidationMessage(messageForCommit(commit, m.getMessage()), m.isError()));
+ }
} catch (CommitValidationException e) {
logger.atFine().log("Commit validation failed on %s", commit.name());
- messages.addAll(e.getMessages());
- reject(cmd, e.getMessage());
+ for (CommitValidationMessage m : e.getMessages()) {
+ // TODO(hanwen): drop the non-error messages?
+ messages.add(
+ new CommitValidationMessage(messageForCommit(commit, m.getMessage()), m.isError()));
+ }
+ reject(cmd, messageForCommit(commit, e.getMessage()));
return false;
}
validCommits.add(key);
diff --git a/java/com/google/gerrit/server/git/validators/CommitValidators.java b/java/com/google/gerrit/server/git/validators/CommitValidators.java
index 1cf71c0..84ea8d3 100644
--- a/java/com/google/gerrit/server/git/validators/CommitValidators.java
+++ b/java/com/google/gerrit/server/git/validators/CommitValidators.java
@@ -37,7 +37,6 @@
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
-import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.externalids.ExternalIdsConsistencyChecker;
@@ -237,18 +236,17 @@
public static class ChangeIdValidator implements CommitValidationListener {
private static final String CHANGE_ID_PREFIX = FooterConstants.CHANGE_ID.getName() + ":";
- private static final String MISSING_CHANGE_ID_MSG =
- "[%s] missing Change-Id in commit message footer";
+ private static final String MISSING_CHANGE_ID_MSG = "missing Change-Id in message footer";
private static final String MISSING_SUBJECT_MSG =
- "[%s] missing subject; Change-Id must be in commit message footer";
+ "missing subject; Change-Id must be in message footer";
private static final String MULTIPLE_CHANGE_ID_MSG =
- "[%s] multiple Change-Id lines in commit message footer";
+ "multiple Change-Id lines in message footer";
private static final String INVALID_CHANGE_ID_MSG =
- "[%s] invalid Change-Id line format in commit message footer";
+ "invalid Change-Id line format in message footer";
@VisibleForTesting
public static final String CHANGE_ID_MISMATCH_MSG =
- "[%s] Change-Id in commit message footer does not match Change-Id of target change";
+ "Change-Id in message footer does not match Change-Id of target change";
private static final Pattern CHANGE_ID = Pattern.compile(CHANGE_ID_PATTERN);
@@ -283,35 +281,29 @@
RevCommit commit = receiveEvent.commit;
List<CommitValidationMessage> messages = new ArrayList<>();
List<String> idList = commit.getFooterLines(FooterConstants.CHANGE_ID);
- String sha1 = commit.abbreviate(RevId.ABBREV_LEN).name();
if (idList.isEmpty()) {
String shortMsg = commit.getShortMessage();
if (shortMsg.startsWith(CHANGE_ID_PREFIX)
&& CHANGE_ID.matcher(shortMsg.substring(CHANGE_ID_PREFIX.length()).trim()).matches()) {
- String errMsg = String.format(MISSING_SUBJECT_MSG, sha1);
- throw new CommitValidationException(errMsg);
+ throw new CommitValidationException(MISSING_SUBJECT_MSG);
}
if (projectState.is(BooleanProjectConfig.REQUIRE_CHANGE_ID)) {
- String errMsg = String.format(MISSING_CHANGE_ID_MSG, sha1);
- messages.add(getMissingChangeIdErrorMsg(errMsg, commit));
- throw new CommitValidationException(errMsg, messages);
+ messages.add(getMissingChangeIdErrorMsg(MISSING_CHANGE_ID_MSG, commit));
+ throw new CommitValidationException(MISSING_CHANGE_ID_MSG, messages);
}
} else if (idList.size() > 1) {
- String errMsg = String.format(MULTIPLE_CHANGE_ID_MSG, sha1);
- throw new CommitValidationException(errMsg, messages);
+ throw new CommitValidationException(MULTIPLE_CHANGE_ID_MSG, messages);
} else {
String v = idList.get(idList.size() - 1).trim();
// Reject Change-Ids with wrong format and invalid placeholder ID from
// Egit (I0000000000000000000000000000000000000000).
if (!CHANGE_ID.matcher(v).matches() || v.matches("^I00*$")) {
- String errMsg = String.format(INVALID_CHANGE_ID_MSG, sha1);
- messages.add(getMissingChangeIdErrorMsg(errMsg, receiveEvent.commit));
- throw new CommitValidationException(errMsg, messages);
+ messages.add(getMissingChangeIdErrorMsg(INVALID_CHANGE_ID_MSG, receiveEvent.commit));
+ throw new CommitValidationException(INVALID_CHANGE_ID_MSG, messages);
}
if (change != null && !v.equals(change.getKey().get())) {
- String errMsg = String.format(CHANGE_ID_MISMATCH_MSG, sha1);
- throw new CommitValidationException(errMsg);
+ throw new CommitValidationException(CHANGE_ID_MISMATCH_MSG);
}
}
@@ -539,7 +531,7 @@
perm.check(RefPermission.FORGE_COMMITTER);
} catch (AuthException denied) {
throw new CommitValidationException(
- "not Signed-off-by author/committer/uploader in commit message footer");
+ "not Signed-off-by author/committer/uploader in message footer");
} catch (PermissionBackendException e) {
logger.atSevere().withCause(e).log("cannot check FORGE_COMMITTER");
throw new CommitValidationException("internal auth error");
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index c23c2a2..eab76c3 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -1156,7 +1156,7 @@
pushFactory.create(
db, admin.getIdent(), testRepo, PushOneCommit.SUBJECT, "b.txt", "anotherContent");
r = push.to("refs/for/master");
- r.assertErrorStatus("not Signed-off-by author/committer/uploader in commit message footer");
+ r.assertErrorStatus("not Signed-off-by author/committer/uploader in message footer");
}
@Test
@@ -1360,7 +1360,7 @@
private void testPushWithoutChangeId() throws Exception {
RevCommit c = createCommit(testRepo, "Message without Change-Id");
assertThat(GitUtil.getChangeId(testRepo, c)).isEmpty();
- pushForReviewRejected(testRepo, "missing Change-Id in commit message footer");
+ pushForReviewRejected(testRepo, "missing Change-Id in message footer");
setRequireChangeId(InheritableBoolean.FALSE);
pushForReviewOk(testRepo);
@@ -1385,8 +1385,9 @@
r = push.to("refs/changes/" + r.getChange().change().getId().get());
r.assertErrorStatus(
String.format(
- ChangeIdValidator.CHANGE_ID_MISMATCH_MSG,
- r.getCommit().abbreviate(RevId.ABBREV_LEN).name()));
+ "commit %s: %s",
+ r.getCommit().abbreviate(RevId.ABBREV_LEN).name(),
+ ChangeIdValidator.CHANGE_ID_MISMATCH_MSG));
}
@Test
@@ -1407,10 +1408,10 @@
+ "\n"
+ "Change-Id: I10f98c2ef76e52e23aa23be5afeb71e40b350e86\n"
+ "Change-Id: Ie9a132e107def33bdd513b7854b50de911edba0a\n");
- pushForReviewRejected(testRepo, "multiple Change-Id lines in commit message footer");
+ pushForReviewRejected(testRepo, "multiple Change-Id lines in message footer");
setRequireChangeId(InheritableBoolean.FALSE);
- pushForReviewRejected(testRepo, "multiple Change-Id lines in commit message footer");
+ pushForReviewRejected(testRepo, "multiple Change-Id lines in message footer");
}
@Test
@@ -1426,10 +1427,10 @@
private void testpushWithInvalidChangeId() throws Exception {
createCommit(testRepo, "Message with invalid Change-Id\n\nChange-Id: X\n");
- pushForReviewRejected(testRepo, "invalid Change-Id line format in commit message footer");
+ pushForReviewRejected(testRepo, "invalid Change-Id line format in message footer");
setRequireChangeId(InheritableBoolean.FALSE);
- pushForReviewRejected(testRepo, "invalid Change-Id line format in commit message footer");
+ pushForReviewRejected(testRepo, "invalid Change-Id line format in message footer");
}
@Test
@@ -1450,19 +1451,19 @@
"Message with invalid Change-Id\n"
+ "\n"
+ "Change-Id: I0000000000000000000000000000000000000000\n");
- pushForReviewRejected(testRepo, "invalid Change-Id line format in commit message footer");
+ pushForReviewRejected(testRepo, "invalid Change-Id line format in message footer");
setRequireChangeId(InheritableBoolean.FALSE);
- pushForReviewRejected(testRepo, "invalid Change-Id line format in commit message footer");
+ pushForReviewRejected(testRepo, "invalid Change-Id line format in message footer");
}
@Test
public void pushWithChangeIdInSubjectLine() throws Exception {
createCommit(testRepo, "Change-Id: I1234000000000000000000000000000000000000");
- pushForReviewRejected(testRepo, "missing subject; Change-Id must be in commit message footer");
+ pushForReviewRejected(testRepo, "missing subject; Change-Id must be in message footer");
setRequireChangeId(InheritableBoolean.FALSE);
- pushForReviewRejected(testRepo, "missing subject; Change-Id must be in commit message footer");
+ pushForReviewRejected(testRepo, "missing subject; Change-Id must be in message footer");
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java b/javatests/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java
index 4cd1cf2..a9182ef 100644
--- a/javatests/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java
@@ -980,7 +980,7 @@
private void assertRefUpdateFailure(RemoteRefUpdate update, String msg) {
assertThat(update.getStatus()).isEqualTo(Status.REJECTED_OTHER_REASON);
- assertThat(update.getMessage()).isEqualTo(msg);
+ assertThat(update.getMessage()).contains(msg);
}
private AutoCloseable createFailOnLoadContext() {
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
index 09c7e0b..9218336 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
@@ -101,9 +101,7 @@
ChangeInput ci = newChangeInput(ChangeStatus.NEW);
ci.subject = "Subject\n\nChange-Id: I0000000000000000000000000000000000000000";
assertCreateFails(
- ci,
- ResourceConflictException.class,
- "invalid Change-Id line format in commit message footer");
+ ci, ResourceConflictException.class, "invalid Change-Id line format in message footer");
}
@Test
@@ -113,7 +111,7 @@
assertCreateFails(
ci,
ResourceConflictException.class,
- "missing subject; Change-Id must be in commit message footer");
+ "missing subject; Change-Id must be in message footer");
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/BanCommitIT.java b/javatests/com/google/gerrit/acceptance/rest/project/BanCommitIT.java
index 19f6295..1eea84b 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/BanCommitIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/BanCommitIT.java
@@ -46,7 +46,7 @@
pushHead(testRepo, "refs/heads/master", false).getRemoteUpdate("refs/heads/master");
assertThat(u).isNotNull();
assertThat(u.getStatus()).isEqualTo(REJECTED_OTHER_REASON);
- assertThat(u.getMessage()).startsWith("contains banned commit");
+ assertThat(u.getMessage()).contains("contains banned commit");
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/ssh/BanCommitIT.java b/javatests/com/google/gerrit/acceptance/ssh/BanCommitIT.java
index 7a80f2e..2b00718 100644
--- a/javatests/com/google/gerrit/acceptance/ssh/BanCommitIT.java
+++ b/javatests/com/google/gerrit/acceptance/ssh/BanCommitIT.java
@@ -42,6 +42,6 @@
pushHead(testRepo, "refs/heads/master", false).getRemoteUpdate("refs/heads/master");
assertThat(u).isNotNull();
assertThat(u.getStatus()).isEqualTo(REJECTED_OTHER_REASON);
- assertThat(u.getMessage()).startsWith("contains banned commit");
+ assertThat(u.getMessage()).contains("contains banned commit");
}
}
