opendev/gerrit
Code Issues Proposed changes

ReceiveCommits: uniformize commit validation error messages.

Browse Source 
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
This commit is contained in:
Han-Wen Nienhuys 2018-08-27 20:04:08 +02:00 committed by David Pursehouse
parent 1c682698d3
commit de4dc899a5
10 changed files with 41 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
Documentation/error-missing-changeid.txt
View File

@ -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

2
Documentation/error-missing-subject.txt
View File

@ -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

2
Documentation/error-multiple-changeid-lines.txt
View File

@ -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.

20
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
View File

@ -1098,7 +1098,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
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
@ -1293,7 +1293,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
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);
@ -1317,10 +1317,10 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
+ "\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
@ -1336,10 +1336,10 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
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
@ -1360,19 +1360,19 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
"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

2
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java
View File

@ -968,6 +968,6 @@ public class ExternalIdIT extends AbstractDaemonTest {
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);
}
}

6
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
View File

@ -102,9 +102,7 @@ public class CreateChangeIT extends AbstractDaemonTest {
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
@ -114,7 +112,7 @@ public class CreateChangeIT extends AbstractDaemonTest {
assertCreateFails(
ci,
ResourceConflictException.class,
"missing subject; Change-Id must be in commit message footer");
"missing subject; Change-Id must be in message footer");
}
@Test

2
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/BanCommitIT.java
View File

@ -46,7 +46,7 @@ public class BanCommitIT extends AbstractDaemonTest {
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

2
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/BanCommitIT.java
View File

@ -42,6 +42,6 @@ public class BanCommitIT extends AbstractDaemonTest {
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");
}
}

15
gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
View File

@ -2777,6 +2777,10 @@ class ReceiveCommits {
}
}
private String messageForCommit(RevCommit c, String msg) {
return String.format("commit %s: %s", c.abbreviate(RevId.ABBREV_LEN).name(), msg);
}
private boolean validCommit(
RevWalk rw,
PermissionBackend.ForRef perm,
@ -2803,11 +2807,16 @@ class ReceiveCommits {
? commitValidatorsFactory.forMergedCommits(perm, user.asIdentifiedUser())
: commitValidatorsFactory.forReceiveCommits(
perm, branch, user.asIdentifiedUser(), sshInfo, repo, rw);
messages.addAll(validators.validate(receiveEvent));
for (CommitValidationMessage m : validators.validate(receiveEvent)) {
messages.add(new CommitValidationMessage(messageForCommit(c, m.getMessage()), m.isError()));
}
} catch (CommitValidationException e) {
logDebug("Commit validation failed on {}", c.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(c, m.getMessage()), m.isError()));
}
reject(cmd, messageForCommit(c, e.getMessage()));
return false;
}
validCommits.add(c.copy());

29
gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java
View File

@ -32,7 +32,6 @@ import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Branch;
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.WatchConfig;
@ -209,14 +208,13 @@ public class CommitValidators {
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";
private static final Pattern CHANGE_ID = Pattern.compile(CHANGE_ID_PATTERN);
private final ProjectState projectState;
@ -247,31 +245,26 @@ public class CommitValidators {
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.isRequireChangeID()) {
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);
}
}
return Collections.emptyList();
@ -516,7 +509,7 @@ public class CommitValidators {
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) {
log.error("cannot check FORGE_COMMITTER", e);
throw new CommitValidationException("internal auth error");