On cherry-pick don't post message on source change

If a change is cherry-picked to a secret branch posting a change message
on the source change that contains the target branch name leaks the
secret branch name.

Posting a change message on the source change that says to where it was
cherry-picked is convenient but not totally required. On the change
screen there is an own section that shows cherry-picks and it's easy to
look there to find out to which branches a change was cherry-picked.
This section only shows changes that are visible to the user and hence
doesn't leak secret branch names.

We expect that nobody relies on this message, especially since
cherry-picks that are done locally and then pushed to Gerrit don't
trigger such a message and hence one can never be sure that such a
message exists.

Alternatively it was considered to just drop the branch name from the
change message and leave the message as:

  This patchset was cherry picked as commit <SHA1>.

However in this case users might see SHA1s that are not visible to them
and it might confuse them that they get a Not Found when trying to look
them up.

Change-Id: Ic0087798d948a651338f071ffcba7b4e821cc56c
Signed-off-by: Edwin Kempin <ekempin@google.com>

3 files changed, 1 insertions, 90 deletions

diff --git a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
index b78c527015..6399cdec6d 100644
--- a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
+++ b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
@@ -29,12 +29,10 @@ import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Branch;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Change.Status;
-import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
-import com.google.gerrit.server.ChangeMessagesUtil;
 import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.IdentifiedUser;
@@ -58,8 +56,6 @@ import com.google.gerrit.server.query.change.InternalChangeQuery;
 import com.google.gerrit.server.submit.IntegrationException;
 import com.google.gerrit.server.submit.MergeIdenticalTreeException;
 import com.google.gerrit.server.update.BatchUpdate;
-import com.google.gerrit.server.update.BatchUpdateOp;
-import com.google.gerrit.server.update.ChangeContext;
 import com.google.gerrit.server.update.UpdateException;
 import com.google.gerrit.server.util.time.TimeUtil;
 import com.google.gwtorm.server.OrmException;
@@ -109,7 +105,6 @@ public class CherryPickChange {
   private final ChangeNotes.Factory changeNotesFactory;
   private final ProjectCache projectCache;
   private final ApprovalsUtil approvalsUtil;
-  private final ChangeMessagesUtil changeMessagesUtil;
   private final NotifyUtil notifyUtil;
 
   @Inject
@@ -126,7 +121,6 @@ public class CherryPickChange {
       ChangeNotes.Factory changeNotesFactory,
       ProjectCache projectCache,
       ApprovalsUtil approvalsUtil,
-      ChangeMessagesUtil changeMessagesUtil,
       NotifyUtil notifyUtil) {
     this.dbProvider = dbProvider;
     this.seq = seq;
@@ -140,7 +134,6 @@ public class CherryPickChange {
     this.changeNotesFactory = changeNotesFactory;
     this.projectCache = projectCache;
     this.approvalsUtil = approvalsUtil;
-    this.changeMessagesUtil = changeMessagesUtil;
     this.notifyUtil = notifyUtil;
   }
 
@@ -155,7 +148,6 @@ public class CherryPickChange {
     return cherryPick(
         batchUpdateFactory,
         change,
-        patch.getId(),
         change.getProject(),
         ObjectId.fromString(patch.getRevision().get()),
         input,
@@ -165,7 +157,6 @@ public class CherryPickChange {
   public Result cherryPick(
       BatchUpdate.Factory batchUpdateFactory,
       @Nullable Change sourceChange,
-      @Nullable PatchSet.Id sourcePatchId,
       Project.NameKey project,
       ObjectId sourceCommit,
       CherryPickInput input,
@@ -275,13 +266,6 @@ public class CherryPickChange {
             changeId =
                 createNewChange(
                     bu, cherryPickCommit, dest.get(), newTopic, sourceChange, sourceCommit, input);
-
-            if (sourceChange != null && sourcePatchId != null) {
-              bu.addOp(
-                  sourceChange.getId(),
-                  new AddMessageToSourceChangeOp(
-                      changeMessagesUtil, sourcePatchId, dest.getShortName(), cherryPickCommit));
-            }
           }
           bu.execute();
           return Result.create(changeId, cherryPickCommit.getFilesWithGitConflicts());
@@ -390,43 +374,6 @@ public class CherryPickChange {
     return changeId;
   }
 
-  private static class AddMessageToSourceChangeOp implements BatchUpdateOp {
-    private final ChangeMessagesUtil cmUtil;
-    private final PatchSet.Id psId;
-    private final String destBranch;
-    private final ObjectId cherryPickCommit;
-
-    private AddMessageToSourceChangeOp(
-        ChangeMessagesUtil cmUtil, PatchSet.Id psId, String destBranch, ObjectId cherryPickCommit) {
-      this.cmUtil = cmUtil;
-      this.psId = psId;
-      this.destBranch = destBranch;
-      this.cherryPickCommit = cherryPickCommit;
-    }
-
-    @Override
-    public boolean updateChange(ChangeContext ctx) throws OrmException {
-      StringBuilder sb =
-          new StringBuilder("Patch Set ")
-              .append(psId.get())
-              .append(": Cherry Picked")
-              .append("\n\n")
-              .append("This patchset was cherry picked to branch ")
-              .append(destBranch)
-              .append(" as commit ")
-              .append(cherryPickCommit.name());
-      ChangeMessage changeMessage =
-          ChangeMessagesUtil.newMessage(
-              psId,
-              ctx.getUser(),
-              ctx.getWhen(),
-              sb.toString(),
-              ChangeMessagesUtil.TAG_CHERRY_PICK_CHANGE);
-      cmUtil.addChangeMessage(ctx.getDb(), ctx.getUpdate(psId), changeMessage);
-      return true;
-    }
-  }
-
   private String messageForDestinationChange(
       PatchSet.Id patchSetId,
       Branch.NameKey sourceBranch,
diff --git a/java/com/google/gerrit/server/restapi/change/CherryPickCommit.java b/java/com/google/gerrit/server/restapi/change/CherryPickCommit.java
index d18b1728bd..f76689c4e2 100644
--- a/java/com/google/gerrit/server/restapi/change/CherryPickCommit.java
+++ b/java/com/google/gerrit/server/restapi/change/CherryPickCommit.java
@@ -100,7 +100,6 @@ public class CherryPickCommit
           cherryPickChange.cherryPick(
               updateFactory,
               null,
-              null,
               projectName,
               commit,
               input,
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index 0cd91ca9f7..bde042fe93 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -323,28 +323,11 @@ public class RevisionIT extends AbstractDaemonTest {
     assertThat(changeInfo.workInProgress).isNull();
     ChangeApi cherry = gApi.changes().id(changeInfo._number);
 
-    ChangeInfo changeInfoWithDetails =
-        gApi.changes().id(project.get() + "~master~" + r.getChangeId()).get();
-    Collection<ChangeMessageInfo> messages = changeInfoWithDetails.messages;
-    assertThat(messages).hasSize(2);
-
-    String cherryPickedRevision = cherry.get().currentRevision;
-    String expectedMessage =
-        String.format(
-            "Patch Set 1: Cherry Picked\n\n"
-                + "This patchset was cherry picked to branch %s as commit %s",
-            in.destination, cherryPickedRevision);
-
-    Iterator<ChangeMessageInfo> origIt = messages.iterator();
-    origIt.next();
-    assertThat(origIt.next().message).isEqualTo(expectedMessage);
-
     ChangeInfo cherryPickChangeInfoWithDetails = cherry.get();
     assertThat(cherryPickChangeInfoWithDetails.workInProgress).isNull();
     assertThat(cherryPickChangeInfoWithDetails.messages).hasSize(1);
     Iterator<ChangeMessageInfo> cherryIt = cherryPickChangeInfoWithDetails.messages.iterator();
-    expectedMessage = "Patch Set 1: Cherry Picked from branch master.";
-    assertThat(cherryIt.next().message).isEqualTo(expectedMessage);
+    assertThat(cherryIt.next().message).isEqualTo("Patch Set 1: Cherry Picked from branch master.");
 
     assertThat(cherry.get().subject).contains(in.message);
     assertThat(cherry.get().topic).isEqualTo("someTopic-foo");
@@ -474,10 +457,6 @@ public class RevisionIT extends AbstractDaemonTest {
     assertThat(orig.get().messages).hasSize(1);
     ChangeApi cherry = orig.revision(r.getCommit().name()).cherryPick(in);
 
-    Collection<ChangeMessageInfo> messages =
-        gApi.changes().id(project.get() + "~master~" + r.getChangeId()).get().messages;
-    assertThat(messages).hasSize(2);
-
     assertThat(cherry.get().subject).contains(in.message);
     cherry.current().review(ReviewInput.approve());
     cherry.current().submit();
@@ -600,20 +579,6 @@ public class RevisionIT extends AbstractDaemonTest {
     ChangeInfo cherryPickChangeWithDetails = gApi.changes().id(cherryPickChange._number).get();
     assertThat(cherryPickChangeWithDetails.workInProgress).isTrue();
 
-    // Verify that a message has been posted on the original change.
-    String cherryPickedRevision = cherryPickChangeWithDetails.currentRevision;
-    changeApi = gApi.changes().id(r.getChange().getId().get());
-    Collection<ChangeMessageInfo> messages = changeApi.get().messages;
-    assertThat(messages).hasSize(2);
-    Iterator<ChangeMessageInfo> origIt = messages.iterator();
-    origIt.next();
-    assertThat(origIt.next().message)
-        .isEqualTo(
-            String.format(
-                "Patch Set 1: Cherry Picked\n\n"
-                    + "This patchset was cherry picked to branch %s as commit %s",
-                in.destination, cherryPickedRevision));
-
     // Verify that a message has been posted on the cherry-pick change.
     assertThat(cherryPickChangeWithDetails.messages).hasSize(1);
     Iterator<ChangeMessageInfo> cherryIt = cherryPickChangeWithDetails.messages.iterator();