Prevent new patchset notification emails for trivial rebase

It is usually not interesting to know that the change was rebased.

Sending mail for this kind of changes distracts the users from
valuable notifications, and is considered spam by many users[1].

Some users completely ignore all mails from gerrit because of this.

[1] https://groups.google.com/d/msg/repo-discuss/fFX_MZerngM/BCitSBFDvSAJ

Change-Id: I7b61ae8445d6fd5dcce2b9a4710609eef43546a5
This commit is contained in:
Orgad Shaneh 2015-02-12 14:24:26 +02:00 committed by David Pursehouse
parent 4512bb4582
commit a5ffcf940d
4 changed files with 39 additions and 34 deletions

View File

@ -228,6 +228,7 @@ public class CherryPickChange {
.setMessage("Uploaded patch set " + newPatchSetId.get() + ".")
.setDraft(current.isDraft())
.setUploader(identifiedUser.getAccountId())
.setSendMail(false)
.insert();
return change.getId();
}

View File

@ -138,7 +138,7 @@ public class RebaseChange {
rebase(git, rw, inserter, patchSetId, change,
uploader, baseCommit, mergeUtilFactory.create(
changeControl.getProjectControl().getProjectState(), true),
committerIdent, true, true, ValidatePolicy.GERRIT);
committerIdent, true, ValidatePolicy.GERRIT);
} catch (MergeConflictException e) {
throw new IOException(e.getMessage());
} finally {
@ -265,7 +265,6 @@ public class RebaseChange {
* @param baseCommit the commit that should be the new base
* @param mergeUtil merge utilities for the destination project
* @param committerIdent the committer's identity
* @param sendMail if a mail notification should be sent for the new patch set
* @param runHooks if hooks should be run for the new patch set
* @param validate if commit validation should be run for the new patch set
* @return the new patch set which is based on the given base commit
@ -279,7 +278,7 @@ public class RebaseChange {
final ObjectInserter inserter, final PatchSet.Id patchSetId,
final Change change, final IdentifiedUser uploader, final RevCommit baseCommit,
final MergeUtil mergeUtil, PersonIdent committerIdent,
boolean sendMail, boolean runHooks, ValidatePolicy validate)
boolean runHooks, ValidatePolicy validate)
throws NoSuchChangeException,
OrmException, IOException, InvalidChangeOperationException,
MergeConflictException {
@ -303,7 +302,7 @@ public class RebaseChange {
.setValidatePolicy(validate)
.setDraft(originalPatchSet.isDraft())
.setUploader(uploader.getAccountId())
.setSendMail(sendMail)
.setSendMail(false)
.setRunHooks(runHooks);
final PatchSet.Id newPatchSetId = patchSetInserter.getPatchSetId();

View File

@ -1985,14 +1985,12 @@ public class ReceiveCommits {
return Futures.makeChecked(future, INSERT_EXCEPTION);
}
private ChangeMessage newChangeMessage(ReviewDb db) throws OrmException {
private ChangeMessage newChangeMessage(ReviewDb db, ChangeKind changeKind)
throws OrmException {
msg =
new ChangeMessage(new ChangeMessage.Key(change.getId(), ChangeUtil
.messageUUID(db)), currentUser.getAccountId(), newPatchSet.getCreatedOn(),
newPatchSet.getId());
RevCommit priorCommit = revisions.inverse().get(priorPatchSet);
ChangeKind changeKind = changeKindCache.getChangeKind(
projectControl.getProjectState(), repo, priorCommit, newCommit);
String message = "Uploaded patch set " + newPatchSet.getPatchSetId();
switch (changeKind) {
case TRIVIAL_REBASE:
@ -2032,6 +2030,7 @@ public class ReceiveCommits {
recipients.remove(me);
db.changes().beginTransaction(change.getId());
ChangeKind changeKind = ChangeKind.REWORK;
try {
change = db.changes().get(change.getId());
if (change == null || change.getStatus().isClosed()) {
@ -2057,7 +2056,11 @@ public class ReceiveCommits {
changeCtl, approvals);
recipients.add(oldRecipients);
cmUtil.addChangeMessage(db, update, newChangeMessage(db));
RevCommit priorCommit = revisions.inverse().get(priorPatchSet);
changeKind = changeKindCache.getChangeKind(
projectControl.getProjectState(), repo, priorCommit, newCommit);
cmUtil.addChangeMessage(db, update, newChangeMessage(db, changeKind));
if (mergedIntoRef == null) {
// Change should be new, so it can go through review again.
@ -2119,32 +2122,34 @@ public class ReceiveCommits {
cmd.execute(rp);
}
CheckedFuture<?, IOException> f = indexer.indexAsync(change.getId());
workQueue.getDefaultQueue()
.submit(requestScopePropagator.wrap(new Runnable() {
@Override
public void run() {
try {
ReplacePatchSetSender cm =
replacePatchSetFactory.create(change);
cm.setFrom(me);
cm.setPatchSet(newPatchSet, info);
cm.setChangeMessage(msg);
cm.addReviewers(recipients.getReviewers());
cm.addExtraCC(recipients.getCcOnly());
cm.send();
} catch (Exception e) {
log.error("Cannot send email for new patch set " + newPatchSet.getId(), e);
if (changeKind != ChangeKind.TRIVIAL_REBASE) {
workQueue.getDefaultQueue()
.submit(requestScopePropagator.wrap(new Runnable() {
@Override
public void run() {
try {
ReplacePatchSetSender cm =
replacePatchSetFactory.create(change);
cm.setFrom(me);
cm.setPatchSet(newPatchSet, info);
cm.setChangeMessage(msg);
cm.addReviewers(recipients.getReviewers());
cm.addExtraCC(recipients.getCcOnly());
cm.send();
} catch (Exception e) {
log.error("Cannot send email for new patch set " + newPatchSet.getId(), e);
}
if (mergedIntoRef != null) {
sendMergedEmail(ReplaceRequest.this);
}
}
if (mergedIntoRef != null) {
sendMergedEmail(ReplaceRequest.this);
}
}
@Override
public String toString() {
return "send-email newpatchset";
}
}));
@Override
public String toString() {
return "send-email newpatchset";
}
}));
}
f.checkedGet();
gitRefUpdated.fire(project.getNameKey(), newPatchSet.getRefName(),

View File

@ -91,7 +91,7 @@ public class RebaseIfNecessary extends SubmitStrategy {
rebaseChange.rebase(args.repo, args.rw, args.inserter,
n.getPatchsetId(), n.change(), uploader,
mergeTip.getCurrentTip(), args.mergeUtil,
args.serverIdent.get(), false, false, ValidatePolicy.NONE);
args.serverIdent.get(), false, ValidatePolicy.NONE);
List<PatchSetApproval> approvals = Lists.newArrayList();
for (PatchSetApproval a : args.approvalsUtil.byPatchSet(args.db,