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>
diff --git a/java/com/google/gerrit/server/restapi/change/CherryPickChange.java b/java/com/google/gerrit/server/restapi/change/CherryPickChange.java
index b78c527..6399cde 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.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.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 @@
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 @@
ChangeNotes.Factory changeNotesFactory,
ProjectCache projectCache,
ApprovalsUtil approvalsUtil,
- ChangeMessagesUtil changeMessagesUtil,
NotifyUtil notifyUtil) {
this.dbProvider = dbProvider;
this.seq = seq;
@@ -140,7 +134,6 @@
this.changeNotesFactory = changeNotesFactory;
this.projectCache = projectCache;
this.approvalsUtil = approvalsUtil;
- this.changeMessagesUtil = changeMessagesUtil;
this.notifyUtil = notifyUtil;
}
@@ -155,7 +148,6 @@
return cherryPick(
batchUpdateFactory,
change,
- patch.getId(),
change.getProject(),
ObjectId.fromString(patch.getRevision().get()),
input,
@@ -165,7 +157,6 @@
public Result cherryPick(
BatchUpdate.Factory batchUpdateFactory,
@Nullable Change sourceChange,
- @Nullable PatchSet.Id sourcePatchId,
Project.NameKey project,
ObjectId sourceCommit,
CherryPickInput input,
@@ -275,13 +266,6 @@
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 @@
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 d18b172..f76689c 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 @@
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 0cd91ca..bde042f 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 @@
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 @@
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 @@
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();
