Don't add name/account ID to change messages
Change messages are Git commits and have an author set. We use this author to serve the 'author' field on the REST API. The UI shows the author on the very left of the change screen. There is no need to mention this information in the message string in addition. In fact, doing so makes showing a nice message harder as the UI would have to parse out the account ID and resolve it. Change-Id: I71fe4101666b496064691f07e92c2b912f6fe2e7
This commit is contained in:
		| @@ -23,7 +23,6 @@ import static java.util.Objects.requireNonNull; | |||||||
|  |  | ||||||
| import com.google.common.flogger.FluentLogger; | import com.google.common.flogger.FluentLogger; | ||||||
| import com.google.gerrit.common.data.SubmitRecord; | import com.google.gerrit.common.data.SubmitRecord; | ||||||
| import com.google.gerrit.entities.Account; |  | ||||||
| import com.google.gerrit.entities.BranchNameKey; | import com.google.gerrit.entities.BranchNameKey; | ||||||
| import com.google.gerrit.entities.Change; | import com.google.gerrit.entities.Change; | ||||||
| import com.google.gerrit.entities.ChangeMessage; | import com.google.gerrit.entities.ChangeMessage; | ||||||
| @@ -36,13 +35,11 @@ import com.google.gerrit.exceptions.StorageException; | |||||||
| import com.google.gerrit.server.ApprovalsUtil; | import com.google.gerrit.server.ApprovalsUtil; | ||||||
| import com.google.gerrit.server.ChangeMessagesUtil; | import com.google.gerrit.server.ChangeMessagesUtil; | ||||||
| import com.google.gerrit.server.IdentifiedUser; | import com.google.gerrit.server.IdentifiedUser; | ||||||
| import com.google.gerrit.server.account.AccountState; |  | ||||||
| import com.google.gerrit.server.change.LabelNormalizer; | import com.google.gerrit.server.change.LabelNormalizer; | ||||||
| import com.google.gerrit.server.git.CodeReviewCommit; | import com.google.gerrit.server.git.CodeReviewCommit; | ||||||
| import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk; | import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk; | ||||||
| import com.google.gerrit.server.git.GroupCollector; | import com.google.gerrit.server.git.GroupCollector; | ||||||
| import com.google.gerrit.server.git.MergeUtil; | import com.google.gerrit.server.git.MergeUtil; | ||||||
| import com.google.gerrit.server.notedb.ChangeNoteUtil; |  | ||||||
| import com.google.gerrit.server.notedb.ChangeUpdate; | import com.google.gerrit.server.notedb.ChangeUpdate; | ||||||
| import com.google.gerrit.server.project.ProjectConfig; | import com.google.gerrit.server.project.ProjectConfig; | ||||||
| import com.google.gerrit.server.project.ProjectState; | import com.google.gerrit.server.project.ProjectState; | ||||||
| @@ -394,24 +391,13 @@ abstract class SubmitStrategyOp implements BatchUpdateOp { | |||||||
|     } |     } | ||||||
|   } |   } | ||||||
|  |  | ||||||
|   private String getByAccountName() { |  | ||||||
|     requireNonNull(submitter, "getByAccountName called before submitter populated"); |  | ||||||
|     Optional<Account> account = |  | ||||||
|         args.accountCache.get(submitter.accountId()).map(AccountState::account); |  | ||||||
|     if (account.isPresent() && account.get().fullName() != null) { |  | ||||||
|       return " by " + ChangeNoteUtil.getAccountIdAsUsername(account.get().id()); |  | ||||||
|     } |  | ||||||
|     return ""; |  | ||||||
|   } |  | ||||||
|  |  | ||||||
|   private ChangeMessage message(ChangeContext ctx, CodeReviewCommit commit, CommitMergeStatus s) { |   private ChangeMessage message(ChangeContext ctx, CodeReviewCommit commit, CommitMergeStatus s) { | ||||||
|     requireNonNull(s, "CommitMergeStatus may not be null"); |     requireNonNull(s, "CommitMergeStatus may not be null"); | ||||||
|     String txt = s.getDescription(); |     String txt = s.getDescription(); | ||||||
|     if (s == CommitMergeStatus.CLEAN_MERGE) { |     if (s == CommitMergeStatus.CLEAN_MERGE) { | ||||||
|       return message(ctx, commit.getPatchsetId(), txt + getByAccountName()); |       return message(ctx, commit.getPatchsetId(), txt); | ||||||
|     } else if (s == CommitMergeStatus.CLEAN_REBASE || s == CommitMergeStatus.CLEAN_PICK) { |     } else if (s == CommitMergeStatus.CLEAN_REBASE || s == CommitMergeStatus.CLEAN_PICK) { | ||||||
|       return message( |       return message(ctx, commit.getPatchsetId(), txt + " as " + commit.name()); | ||||||
|           ctx, commit.getPatchsetId(), txt + " as " + commit.name() + getByAccountName()); |  | ||||||
|     } else if (s == CommitMergeStatus.SKIPPED_IDENTICAL_TREE) { |     } else if (s == CommitMergeStatus.SKIPPED_IDENTICAL_TREE) { | ||||||
|       return message(ctx, commit.getPatchsetId(), txt); |       return message(ctx, commit.getPatchsetId(), txt); | ||||||
|     } else if (s == CommitMergeStatus.ALREADY_MERGED) { |     } else if (s == CommitMergeStatus.ALREADY_MERGED) { | ||||||
|   | |||||||
| @@ -1344,11 +1344,11 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { | |||||||
|     assertThat(messages).hasSize(3); |     assertThat(messages).hasSize(3); | ||||||
|     String last = Iterables.getLast(messages); |     String last = Iterables.getLast(messages); | ||||||
|     if (getSubmitType() == SubmitType.CHERRY_PICK) { |     if (getSubmitType() == SubmitType.CHERRY_PICK) { | ||||||
|       assertThat(last).startsWith("Change has been successfully cherry-picked as "); |       assertThat(last).startsWith("Change has been successfully cherry-picked as"); | ||||||
|     } else if (getSubmitType() == SubmitType.REBASE_ALWAYS) { |     } else if (getSubmitType() == SubmitType.REBASE_ALWAYS) { | ||||||
|       assertThat(last).startsWith("Change has been successfully rebased and submitted as"); |       assertThat(last).startsWith("Change has been successfully rebased and submitted as"); | ||||||
|     } else { |     } else { | ||||||
|       assertThat(last).isEqualTo("Change has been successfully merged by Gerrit User 1000000"); |       assertThat(last).isEqualTo("Change has been successfully merged"); | ||||||
|     } |     } | ||||||
|   } |   } | ||||||
|  |  | ||||||
|   | |||||||
		Reference in New Issue
	
	Block a user
	 Patrick Hiesel
					Patrick Hiesel