From 2109f3c35a247d41fea5af8cee5382476837082b Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 4 Feb 2019 08:50:39 +0900 Subject: [PATCH 1/8] CommitValidators: Prefer using Splitter to String.split Partial cherry-pick of commit 5174d2d done in order to make subsequent cherry-picks cleaner. Change-Id: I685ef2a3ac9ecd66aa32c6556aa1a70ba37704e1 --- .../gerrit/server/git/validators/CommitValidators.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java index 541cbfa3fb..0eb781939e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -20,7 +20,9 @@ import static com.google.gerrit.reviewdb.client.RefNames.REFS_CONFIG; import static java.util.stream.Collectors.toList; import com.google.common.base.CharMatcher; +import com.google.common.base.Splitter; import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; import com.google.gerrit.common.FooterConstants; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.PageLinks; @@ -289,9 +291,7 @@ public class CommitValidators { sb.append("ERROR: ").append(errMsg); if (c.getFullMessage().indexOf(CHANGE_ID_PREFIX) >= 0) { - String[] lines = c.getFullMessage().trim().split("\n"); - String lastLine = lines.length > 0 ? lines[lines.length - 1] : ""; - + String lastLine = Iterables.getLast(Splitter.on('\n').split(c.getFullMessage()), ""); if (lastLine.indexOf(CHANGE_ID_PREFIX) == -1) { sb.append('\n'); sb.append('\n'); From b73cb380c4ac4a5e6e151e69a05c45b32269940f Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 4 Feb 2019 08:52:57 +0900 Subject: [PATCH 2/8] CommitValidators: Replace indexOf calls with String#contains Partial cherry-pick of commit 249d47b done in order to make subsequent cherry-picks cleaner. Change-Id: I7288a18d38351ebe87aa8fc25b2c232310b1273f --- .../google/gerrit/server/git/validators/CommitValidators.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java index 0eb781939e..6724ddeab6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -290,9 +290,9 @@ public class CommitValidators { StringBuilder sb = new StringBuilder(); sb.append("ERROR: ").append(errMsg); - if (c.getFullMessage().indexOf(CHANGE_ID_PREFIX) >= 0) { + if (c.getFullMessage().contains(CHANGE_ID_PREFIX)) { String lastLine = Iterables.getLast(Splitter.on('\n').split(c.getFullMessage()), ""); - if (lastLine.indexOf(CHANGE_ID_PREFIX) == -1) { + if (!lastLine.contains(CHANGE_ID_PREFIX)) { sb.append('\n'); sb.append('\n'); sb.append("Hint: A potential "); From 9c5e2cc7626c16b3e9c52d449521e1e414c97d70 Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Thu, 5 Jul 2018 16:33:02 +0200 Subject: [PATCH 3/8] Clean up Change-Id hint text Before: remote: ERROR: [c3dcb3a] ... remote: remote: Hint: A potential Change-Id was found, but it was not in the footer (last paragraph) of the commit message. remote: After: remote: ERROR: [a1a077b] ... remote: remote: Hint: run remote: git commit --amend remote: and move 'Change-Id: Ixxx..' to the bottom on a separate line Change-Id: I57387d88c7f105a33ede17ef76a4ac1f064fefdd --- .../gerrit/server/git/validators/CommitValidators.java | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java index 6724ddeab6..12f4eb86b8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -293,12 +293,10 @@ public class CommitValidators { if (c.getFullMessage().contains(CHANGE_ID_PREFIX)) { String lastLine = Iterables.getLast(Splitter.on('\n').split(c.getFullMessage()), ""); if (!lastLine.contains(CHANGE_ID_PREFIX)) { - sb.append('\n'); - sb.append('\n'); - sb.append("Hint: A potential "); - sb.append(FooterConstants.CHANGE_ID.getName()); - sb.append(" was found, but it was not in the "); - sb.append("footer (last paragraph) of the commit message."); + sb.append("\n\n") + .append("Hint: run\n") + .append(" git commit --amend\n") + .append("and move 'Change-Id: Ixxx..' to the bottom on a separate line\n"); } } sb.append('\n'); From f61bc3f4f7e732742083868e93cd2ffae970be4e Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Thu, 5 Jul 2018 17:00:04 +0200 Subject: [PATCH 4/8] Print only one hint about Change-Ids at a time Change-Id: Idd6b5f63c71000c660a0167510d47fdd55904e92 --- .../git/validators/CommitValidators.java | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java index 12f4eb86b8..a71317f54b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -290,25 +290,26 @@ public class CommitValidators { StringBuilder sb = new StringBuilder(); sb.append("ERROR: ").append(errMsg); + boolean hinted = false; if (c.getFullMessage().contains(CHANGE_ID_PREFIX)) { String lastLine = Iterables.getLast(Splitter.on('\n').split(c.getFullMessage()), ""); if (!lastLine.contains(CHANGE_ID_PREFIX)) { + hinted = true; sb.append("\n\n") .append("Hint: run\n") .append(" git commit --amend\n") .append("and move 'Change-Id: Ixxx..' to the bottom on a separate line\n"); } } - sb.append('\n'); - sb.append('\n'); - sb.append("Hint: To automatically insert "); - sb.append(FooterConstants.CHANGE_ID.getName()); - sb.append(", install the hook:\n"); - sb.append(getCommitMessageHookInstallationHint()); - sb.append('\n'); - sb.append("And then amend the commit:\n"); - sb.append(" git commit --amend\n"); + // Print only one hint to avoid overwhelming the user. + if (!hinted) { + sb.append("Hint: to automatically insert a Change-Id, install the hook:\n") + .append(getCommitMessageHookInstallationHint()) + .append("\n") + .append("and then amend the commit:\n") + .append(" git commit --amend\n"); + } return new CommitValidationMessage(sb.toString(), false); } From 25aee8f0bf0bb03ab0be82263a3542907d49c7b0 Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Wed, 25 Jul 2018 10:28:16 +0200 Subject: [PATCH 5/8] Always end the "Change-Id missing" error message with \n. This should fix the current output which looks like remote: ERROR: [5bfebfb] missing .. footerHint: to automatically .. Change-Id: Id555b98cccdc25faf2faf641f564692ced10e35d --- .../gerrit/server/git/validators/CommitValidators.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java index a71317f54b..ccc5461f77 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -288,14 +288,14 @@ public class CommitValidators { private CommitValidationMessage getMissingChangeIdErrorMsg(String errMsg, RevCommit c) { StringBuilder sb = new StringBuilder(); - sb.append("ERROR: ").append(errMsg); + sb.append("ERROR: ").append(errMsg).append("\n"); boolean hinted = false; if (c.getFullMessage().contains(CHANGE_ID_PREFIX)) { String lastLine = Iterables.getLast(Splitter.on('\n').split(c.getFullMessage()), ""); if (!lastLine.contains(CHANGE_ID_PREFIX)) { hinted = true; - sb.append("\n\n") + sb.append("\n") .append("Hint: run\n") .append(" git commit --amend\n") .append("and move 'Change-Id: Ixxx..' to the bottom on a separate line\n"); @@ -304,7 +304,7 @@ public class CommitValidators { // Print only one hint to avoid overwhelming the user. if (!hinted) { - sb.append("Hint: to automatically insert a Change-Id, install the hook:\n") + sb.append("\nHint: to automatically insert a Change-Id, install the hook:\n") .append(getCommitMessageHookInstallationHint()) .append("\n") .append("and then amend the commit:\n") From 1c682698d36ff93a02133c2911141c682b46e186 Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Wed, 25 Jul 2018 10:37:43 +0200 Subject: [PATCH 6/8] Inline "Change-Id" string into error messages Change-Id: Id74f483a07dca0baf2cd50f2d26f007bdcbc8fd4 --- .../server/git/validators/CommitValidators.java | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java index ccc5461f77..ef98550825 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -210,17 +210,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 " + FooterConstants.CHANGE_ID.getName() + " in commit message footer"; + "[%s] missing Change-Id in commit message footer"; private static final String MISSING_SUBJECT_MSG = - "[%s] missing subject; " - + FooterConstants.CHANGE_ID.getName() - + " must be in commit message footer"; + "[%s] missing subject; Change-Id must be in commit message footer"; private static final String MULTIPLE_CHANGE_ID_MSG = - "[%s] multiple " + FooterConstants.CHANGE_ID.getName() + " lines in commit message footer"; + "[%s] multiple Change-Id lines in commit message footer"; private static final String INVALID_CHANGE_ID_MSG = - "[%s] invalid " - + FooterConstants.CHANGE_ID.getName() - + " line format in commit message footer"; + "[%s] invalid Change-Id line format in commit message footer"; private static final Pattern CHANGE_ID = Pattern.compile(CHANGE_ID_PATTERN); private final ProjectState projectState; From de4dc899a54f49d5c132bf4a7caffb4c654dd688 Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Mon, 27 Aug 2018 20:04:08 +0200 Subject: [PATCH 7/8] 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 --- Documentation/error-missing-changeid.txt | 2 +- Documentation/error-missing-subject.txt | 2 +- .../error-multiple-changeid-lines.txt | 2 +- .../acceptance/git/AbstractPushForReview.java | 20 ++++++------- .../acceptance/rest/account/ExternalIdIT.java | 2 +- .../rest/change/CreateChangeIT.java | 6 ++-- .../acceptance/rest/project/BanCommitIT.java | 2 +- .../gerrit/acceptance/ssh/BanCommitIT.java | 2 +- .../server/git/receive/ReceiveCommits.java | 15 ++++++++-- .../git/validators/CommitValidators.java | 29 +++++++------------ 10 files changed, 41 insertions(+), 41 deletions(-) diff --git a/Documentation/error-missing-changeid.txt b/Documentation/error-missing-changeid.txt index 9cddd85b03..08f2c09475 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 3703ade177..6ef37a4577 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 072954720b..31567f4f21 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/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java index e053d1325b..02d3802d84 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -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 diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java index 0f8308cfaa..33313d13e4 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java @@ -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); } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java index ba37248d78..216eb3a824 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java @@ -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 diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/BanCommitIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/BanCommitIT.java index 90d51e041e..00a11de19d 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/BanCommitIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/BanCommitIT.java @@ -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 diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/BanCommitIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/BanCommitIT.java index 7a80f2e49f..2b00718870 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/BanCommitIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/BanCommitIT.java @@ -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"); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index 35259bb8e2..80e0da8a43 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -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()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java index ef98550825..5562ff5e55 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -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 messages = new ArrayList<>(); List 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"); From 2c5b2c3bb506391c44c1c3a23fecd68ed0a5115e Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Tue, 28 Aug 2018 19:40:36 +0200 Subject: [PATCH 8/8] CommitValidators: trim "ERROR" shouting from "forge committer" check Change-Id: I848e210da540524142b0e44f7e826cc0d0a7646b --- .../git/validators/CommitValidators.java | 42 +++++++------------ 1 file changed, 14 insertions(+), 28 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java index 5562ff5e55..9395ffb959 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -544,8 +544,7 @@ public class CommitValidators { return Collections.emptyList(); } catch (AuthException e) { throw new CommitValidationException( - "invalid author", - invalidEmail(receiveEvent.commit, "author", author, user, canonicalWebUrl)); + "invalid author", invalidEmail("author", author, user, canonicalWebUrl)); } catch (PermissionBackendException e) { log.error("cannot check FORGE_AUTHOR", e); throw new CommitValidationException("internal auth error"); @@ -578,8 +577,7 @@ public class CommitValidators { return Collections.emptyList(); } catch (AuthException e) { throw new CommitValidationException( - "invalid committer", - invalidEmail(receiveEvent.commit, "committer", committer, user, canonicalWebUrl)); + "invalid committer", invalidEmail("committer", committer, user, canonicalWebUrl)); } catch (PermissionBackendException e) { log.error("cannot check FORGE_COMMITTER", e); throw new CommitValidationException("internal auth error"); @@ -747,42 +745,30 @@ public class CommitValidators { } private static CommitValidationMessage invalidEmail( - RevCommit c, - String type, - PersonIdent who, - IdentifiedUser currentUser, - String canonicalWebUrl) { + String type, PersonIdent who, IdentifiedUser currentUser, String canonicalWebUrl) { StringBuilder sb = new StringBuilder(); - sb.append("\n"); - sb.append("ERROR: In commit ").append(c.name()).append("\n"); - sb.append("ERROR: ") - .append(type) - .append(" email address ") + + sb.append("email address ") .append(who.getEmailAddress()) - .append("\n"); - sb.append("ERROR: does not match your user account and you have no 'forge ") + .append(" is not registered in your account, and you lack 'forge ") .append(type) .append("' permission.\n"); - sb.append("ERROR:\n"); + if (currentUser.getEmailAddresses().isEmpty()) { - sb.append("ERROR: You have not registered any email addresses.\n"); + sb.append("You have not registered any email addresses.\n"); } else { - sb.append("ERROR: The following addresses are currently registered:\n"); + sb.append("The following addresses are currently registered:\n"); for (String address : currentUser.getEmailAddresses()) { - sb.append("ERROR: ").append(address).append("\n"); + sb.append(" ").append(address).append("\n"); } } - sb.append("ERROR:\n"); + if (canonicalWebUrl != null) { - sb.append("ERROR: To register an email address, please visit:\n"); - sb.append("ERROR: ") - .append(canonicalWebUrl) - .append("#") - .append(PageLinks.SETTINGS_CONTACT) - .append("\n"); + sb.append("To register an email address, visit:\n"); + sb.append(canonicalWebUrl).append("#").append(PageLinks.SETTINGS_CONTACT).append("\n"); } sb.append("\n"); - return new CommitValidationMessage(sb.toString(), false); + return new CommitValidationMessage(sb.toString(), true); } /**