Merge changes If23c598d,I5fd1a074 * changes: MergeUtil: Add debug logging to PluggableCommitMessageGenerator Expand acceptance tests for ChangeMessageModifiers
diff --git a/java/com/google/gerrit/server/change/ConsistencyChecker.java b/java/com/google/gerrit/server/change/ConsistencyChecker.java index ded7912..b6df374 100644 --- a/java/com/google/gerrit/server/change/ConsistencyChecker.java +++ b/java/com/google/gerrit/server/change/ConsistencyChecker.java
@@ -64,6 +64,7 @@ import java.util.Collections; import java.util.HashSet; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Set; import java.util.TreeSet; @@ -360,10 +361,10 @@ private ProblemInfo wrongChangeStatus(PatchSet.Id psId, RevCommit commit) { String refName = change().getDest().get(); return problem( - String.format( + formatProblemMessage( "Patch set %d (%s) is merged into destination ref %s (%s), but change" + " status is %s", - psId.get(), commit.name(), refName, tip.name(), change().getStatus())); + psId.get(), commit.name(), refName, tip.name())); } private void checkMergedBitMatchesStatus(PatchSet.Id psId, RevCommit commit, boolean merged) { @@ -375,13 +376,24 @@ } } else if (!merged && change().isMerged()) { problem( - String.format( + formatProblemMessage( "Patch set %d (%s) is not merged into" + " destination ref %s (%s), but change status is %s", - currPs.getId().get(), commit.name(), refName, tip.name(), change().getStatus())); + currPs.getId().get(), commit.name(), refName, tip.name())); } } + private String formatProblemMessage( + String message, int psId, String commitName, String refName, String tipName) { + return String.format( + message, + psId, + commitName, + refName, + tipName, + ChangeUtil.status(change()).toUpperCase(Locale.US)); + } + private void checkExpectMergedAs() { ObjectId objId = parseObjectId(fix.expectMergedAs, "expected merged commit"); RevCommit commit = parseCommit(objId, "expected merged commit");
diff --git a/java/com/google/gerrit/server/change/LabelsJson.java b/java/com/google/gerrit/server/change/LabelsJson.java index 16d3ad1..9d8f3a8 100644 --- a/java/com/google/gerrit/server/change/LabelsJson.java +++ b/java/com/google/gerrit/server/change/LabelsJson.java
@@ -43,6 +43,7 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.server.ApprovalsUtil; +import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.account.AccountLoader; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ReviewerStateInternal; @@ -436,7 +437,7 @@ checkState( !cd.change().isMerged(), "should not call setAllApprovals on %s change", - cd.change().getStatus()); + ChangeUtil.status(cd.change())); // Include a user in the output for this label if either: // - They are an explicit reviewer.
diff --git a/java/com/google/gerrit/server/edit/ChangeEditModifier.java b/java/com/google/gerrit/server/edit/ChangeEditModifier.java index 15ebdd1..9698867 100644 --- a/java/com/google/gerrit/server/edit/ChangeEditModifier.java +++ b/java/com/google/gerrit/server/edit/ChangeEditModifier.java
@@ -23,6 +23,7 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; @@ -399,8 +400,7 @@ Change c = notes.getChange(); if (!c.isNew()) { throw new ResourceConflictException( - String.format( - "change %s is %s", c.getChangeId(), c.getStatus().toString().toLowerCase())); + String.format("change %s is %s", c.getChangeId(), ChangeUtil.status(c))); } // Not allowed to edit if the current patch set is locked.
diff --git a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java index 30aa5ae..4cd93e3 100644 --- a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java +++ b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
@@ -314,7 +314,7 @@ throw new ResourceConflictException( String.format( "Change %s with commit %s is %s", - change.getChangeId(), base, change.getStatus().asChangeStatus())); + change.getChangeId(), base, ChangeUtil.status(change))); } private Change.Id insertPatchSet(
diff --git a/java/com/google/gerrit/server/restapi/change/GetRelated.java b/java/com/google/gerrit/server/restapi/change/GetRelated.java index 9a65165..d3961c1 100644 --- a/java/com/google/gerrit/server/restapi/change/GetRelated.java +++ b/java/com/google/gerrit/server/restapi/change/GetRelated.java
@@ -27,6 +27,7 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.CommonConverters; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.change.RevisionResource; @@ -147,7 +148,7 @@ info._revisionNumber = ps != null ? ps.getPatchSetId() : null; PatchSet.Id curr = change.currentPatchSetId(); info._currentRevisionNumber = curr != null ? curr.get() : null; - info.status = change.getStatus().asChangeStatus().toString(); + info.status = ChangeUtil.status(change); } info.commit = new CommitInfo();
diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java index d9b882d..f41e776 100644 --- a/java/com/google/gerrit/server/submit/MergeOp.java +++ b/java/com/google/gerrit/server/submit/MergeOp.java
@@ -53,6 +53,7 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.ChangeMessagesUtil; +import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.InternalUser; import com.google.gerrit.server.change.NotifyResolver; @@ -386,8 +387,7 @@ if (!cd.change().isNew()) { if (!(cd.change().isMerged() && allowMerged)) { commitStatus.problem( - cd.getId(), - "Change " + cd.getId() + " is " + cd.change().getStatus().toString().toLowerCase()); + cd.getId(), "Change " + cd.getId() + " is " + ChangeUtil.status(cd.change())); } } else if (cd.change().isWorkInProgress()) { commitStatus.problem(cd.getId(), "Change " + cd.getId() + " is work in progress");
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java index 4e48165..d90c4bd 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -3962,12 +3962,28 @@ gApi.changes().id(r.getChangeId()).ignore(true); assertThat(gApi.changes().id(r.getChangeId()).ignored()).isTrue(); + // New patch set notification is not sent to users ignoring the change sender.clear(); requestScopeOperations.setApiUser(admin.getId()); - gApi.changes().id(r.getChangeId()).abandon(); + amendChange(r.getChangeId()); List<Message> messages = sender.getMessages(); assertThat(messages).hasSize(1); - assertThat(messages.get(0).rcpt()).containsExactly(new Address(fullname, email)); + Address address = new Address(fullname, email); + assertThat(messages.get(0).rcpt()).containsExactly(address); + + // Review notification is not sent to users ignoring the change + sender.clear(); + gApi.changes().id(r.getChangeId()).current().review(ReviewInput.approve()); + messages = sender.getMessages(); + assertThat(messages).hasSize(1); + assertThat(messages.get(0).rcpt()).containsExactly(address); + + // Abandoned notification is not sent to users ignoring the change + sender.clear(); + gApi.changes().id(r.getChangeId()).abandon(); + messages = sender.getMessages(); + assertThat(messages).hasSize(1); + assertThat(messages.get(0).rcpt()).containsExactly(address); requestScopeOperations.setApiUser(user.getId()); gApi.changes().id(r.getChangeId()).ignore(false);
diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java index b00597c..f5ebfea 100644 --- a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java +++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
@@ -84,6 +84,8 @@ import com.google.gerrit.server.index.group.GroupIndexer; import com.google.gerrit.server.index.group.StalenessChecker; import com.google.gerrit.server.notedb.Sequences; +import com.google.gerrit.server.project.ProjectConfig; +import com.google.gerrit.server.project.testing.Util; import com.google.gerrit.server.util.MagicBranch; import com.google.gerrit.server.util.time.TimeUtil; import com.google.gerrit.testing.TestTimeUtil; @@ -1074,11 +1076,15 @@ private void assertPushToGroupBranch( Project.NameKey project, String groupRefName, String expectedErrorOnUpdate) throws Exception { - grant(project, RefNames.REFS_GROUPS + "*", Permission.CREATE, false, REGISTERED_USERS); - grant(project, RefNames.REFS_GROUPS + "*", Permission.PUSH, false, REGISTERED_USERS); - grant(project, RefNames.REFS_DELETED_GROUPS + "*", Permission.CREATE, false, REGISTERED_USERS); - grant(project, RefNames.REFS_DELETED_GROUPS + "*", Permission.PUSH, false, REGISTERED_USERS); - grant(project, RefNames.REFS_GROUPNAMES, Permission.PUSH, false, REGISTERED_USERS); + try (ProjectConfigUpdate u = updateProject(project)) { + ProjectConfig cfg = u.getConfig(); + Util.allow(cfg, Permission.CREATE, REGISTERED_USERS, RefNames.REFS_GROUPS + "*"); + Util.allow(cfg, Permission.PUSH, REGISTERED_USERS, RefNames.REFS_GROUPS + "*"); + Util.allow(cfg, Permission.CREATE, REGISTERED_USERS, RefNames.REFS_DELETED_GROUPS + "*"); + Util.allow(cfg, Permission.PUSH, REGISTERED_USERS, RefNames.REFS_DELETED_GROUPS + "*"); + Util.allow(cfg, Permission.PUSH, REGISTERED_USERS, RefNames.REFS_GROUPNAMES); + u.save(); + } TestRepository<InMemoryRepository> repo = cloneProject(project); @@ -1097,8 +1103,12 @@ } private void assertCreateGroupBranch(Project.NameKey project) throws Exception { - grant(project, RefNames.REFS_GROUPS + "*", Permission.CREATE, false, REGISTERED_USERS); - grant(project, RefNames.REFS_GROUPS + "*", Permission.PUSH, false, REGISTERED_USERS); + try (ProjectConfigUpdate u = updateProject(project)) { + ProjectConfig cfg = u.getConfig(); + Util.allow(cfg, Permission.CREATE, REGISTERED_USERS, RefNames.REFS_GROUPS + "*"); + Util.allow(cfg, Permission.PUSH, REGISTERED_USERS, RefNames.REFS_GROUPS + "*"); + u.save(); + } TestRepository<InMemoryRepository> repo = cloneProject(project); PushOneCommit.Result r = pushFactory
diff --git a/javatests/com/google/gerrit/acceptance/api/project/CheckAccessIT.java b/javatests/com/google/gerrit/acceptance/api/project/CheckAccessIT.java index 30a8b6b..4088ebf 100644 --- a/javatests/com/google/gerrit/acceptance/api/project/CheckAccessIT.java +++ b/javatests/com/google/gerrit/acceptance/api/project/CheckAccessIT.java
@@ -32,6 +32,8 @@ import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.group.SystemGroupBackend; +import com.google.gerrit.server.project.ProjectConfig; +import com.google.gerrit.server.project.testing.Util; import com.google.inject.Inject; import java.util.List; import org.eclipse.jgit.lib.RefUpdate; @@ -60,26 +62,29 @@ privilegedUser = accountCreator.create("privilegedUser", "snowden@nsa.gov", "Ed Snowden"); groupOperations.group(privilegedGroupUuid).forUpdate().addMember(privilegedUser.id).update(); - grant(secretProject, "refs/*", Permission.READ, false, privilegedGroupUuid); - block(secretProject, "refs/*", Permission.READ, SystemGroupBackend.REGISTERED_USERS); + try (ProjectConfigUpdate u = updateProject(secretProject)) { + ProjectConfig cfg = u.getConfig(); + Util.allow(cfg, Permission.READ, privilegedGroupUuid, "refs/*"); + Util.block(cfg, Permission.READ, SystemGroupBackend.REGISTERED_USERS, "refs/*"); + u.save(); + } - deny(secretRefProject, "refs/*", Permission.READ, SystemGroupBackend.ANONYMOUS_USERS); - grant(secretRefProject, "refs/heads/secret/*", Permission.READ, false, privilegedGroupUuid); - block( - secretRefProject, - "refs/heads/secret/*", - Permission.READ, - SystemGroupBackend.REGISTERED_USERS); - grant( - secretRefProject, - "refs/heads/*", - Permission.READ, - false, - SystemGroupBackend.REGISTERED_USERS); + try (ProjectConfigUpdate u = updateProject(secretRefProject)) { + ProjectConfig cfg = u.getConfig(); + Util.deny(cfg, Permission.READ, SystemGroupBackend.ANONYMOUS_USERS, "refs/*"); + Util.allow(cfg, Permission.READ, privilegedGroupUuid, "refs/heads/secret/*"); + Util.block(cfg, Permission.READ, SystemGroupBackend.REGISTERED_USERS, "refs/heads/secret/*"); + Util.allow(cfg, Permission.READ, SystemGroupBackend.REGISTERED_USERS, "refs/heads/*"); + u.save(); + } // Ref permission - grant(normalProject, "refs/*", Permission.VIEW_PRIVATE_CHANGES, false, privilegedGroupUuid); - grant(normalProject, "refs/*", Permission.FORGE_SERVER, false, privilegedGroupUuid); + try (ProjectConfigUpdate u = updateProject(normalProject)) { + ProjectConfig cfg = u.getConfig(); + Util.allow(cfg, Permission.VIEW_PRIVATE_CHANGES, privilegedGroupUuid, "refs/*"); + Util.allow(cfg, Permission.FORGE_SERVER, privilegedGroupUuid, "refs/*"); + u.save(); + } } @Test
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java index e8c828a..9538e59 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -881,8 +881,8 @@ exception.expect(ResourceConflictException.class); exception.expectMessage( String.format( - "Change %s with commit %s is %s", - change2.getChange().getId().get(), input.base, ChangeStatus.ABANDONED)); + "Change %s with commit %s is abandoned", + change2.getChange().getId().get(), input.base)); gApi.changes().id(change1.getChangeId()).current().cherryPick(input); }
diff --git a/resources/com/google/gerrit/server/commit-msg_test.sh b/resources/com/google/gerrit/server/commit-msg_test.sh index 44e65e4..d797be3 100755 --- a/resources/com/google/gerrit/server/commit-msg_test.sh +++ b/resources/com/google/gerrit/server/commit-msg_test.sh
@@ -95,6 +95,21 @@ fi } +# Change-Id should not be inserted if gerrit.createChangeId=false +function test_suppress_changeid { + cat << EOF > input +bla bla +EOF + + git config gerrit.createChangeId false + ${hook} input || fail "failed hook execution" + git config --unset gerrit.createChangeId + found=$(grep -c '^Change-Id' input || true) + if [[ "${found}" != "0" ]]; then + fail "got ${found} Change-Ids, want 0" + fi +} + # Change-Id goes after existing trailers. function test_at_end { cat << EOF > input
diff --git a/resources/com/google/gerrit/server/tools/root/hooks/commit-msg b/resources/com/google/gerrit/server/tools/root/hooks/commit-msg index f39d4ef..2901232 100755 --- a/resources/com/google/gerrit/server/tools/root/hooks/commit-msg +++ b/resources/com/google/gerrit/server/tools/root/hooks/commit-msg
@@ -27,6 +27,11 @@ exit 1 fi +# Do not create a change id if requested +if test "false" = "`git config --bool --get gerrit.createChangeId`" ; then + exit 0 +fi + # $RANDOM will be undefined if not using bash, so don't use set -u random=$( (whoami ; hostname ; date; cat $1 ; echo $RANDOM) | git hash-object --stdin) dest="$1.tmp.${random}"