Support copying of approvals on trivial rebase

It can now be configured that all scores for a label are copied
forward when a new patch set is uploaded that is a trivial rebase.
A new patch set is considered as trivial rebase if the commit message
is the same as in the previous patch set and if it has the same code
delta as the previous patch set. This is the case if the change was
rebased onto a different parent. This can be used to enable sticky
approvals, reducing turn-around for trivial rebases prior to
submitting a change.

This makes it also easier for authors to make trivial edits in the
middle of a patch series, as approvals on later changes in the same
series will be carried forward automatically.

This won't work where there are conflicts in the rebase.
This may allow a bad rebase to be submitted, such as when the author
inverts the order of two commits and messes up setup order, such as
by using a method before it is declared. We can't catch everything
in every change using automated tools.

This change is based on a change from Shawn Pearce that was abandoned
some time ago:
  https://gerrit-review.googlesource.com/34801

All the credits for detecting trivial rebases go to him.

Change-Id: I4768e35b3fcb432e0489bc6e10a7f18a51aafd8d
Signed-off-by: Edwin Kempin <edwin.kempin@sap.com>

Documentation/config-labels.txt

@@ -236,6 +236,18 @@ forward when a new patch set is uploaded. This can be used to enable
 sticky approvals, reducing turn-around for trivial cleanups prior to
 submitting a change.
 
+[[label_copyAllScoresOnTrivialRebase]]
+`label.Label-Name.copyAllScoresOnTrivialRebase`
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+If true, all scores for the label are copied forward when a new patch
+set is uploaded that is a trivial rebase. A new patch set is considered
+as trivial rebase if the commit message is the same as in the previous
+patch set and if it has the same code delta as the previous patch set.
+This is the case if the change was rebased onto a different parent.
+This can be used to enable sticky approvals, reducing turn-around for
+trivial rebases prior to submitting a change. Defaults to false.
+
 [[label_canOverride]]
 `label.Label-Name.canOverride`
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

gerrit-common/src/main/java/com/google/gerrit/common/data/LabelType.java

@@ -97,6 +97,7 @@ public class LabelType {
   protected String functionName;
   protected boolean copyMinScore;
   protected boolean copyMaxScore;
+  protected boolean copyAllScoresOnTrivialRebase;
 
   protected List<LabelValue> values;
   protected short maxNegative;
@@ -205,6 +206,14 @@ public class LabelType {
     this.copyMaxScore = copyMaxScore;
   }
 
+  public boolean isCopyAllScoresOnTrivialRebase() {
+    return copyAllScoresOnTrivialRebase;
+  }
+
+  public void setCopyAllScoresOnTrivialRebase(boolean copyAllScoresOnTrivialRebase) {
+    this.copyAllScoresOnTrivialRebase = copyAllScoresOnTrivialRebase;
+  }
+
   public boolean isMaxNegative(PatchSetApproval ca) {
     return maxNegative == ca.getValue();
   }

gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java

@@ -69,10 +69,10 @@ public class ApprovalsUtil {
    * @throws OrmException
    */
   public static void copyLabels(ReviewDb db, LabelTypes labelTypes,
-      PatchSet.Id source, PatchSet.Id dest) throws OrmException {
+      PatchSet.Id source, PatchSet dest, boolean trivialRebase) throws OrmException {
     Iterable<PatchSetApproval> sourceApprovals =
         db.patchSetApprovals().byPatchSet(source);
-    copyLabels(db, labelTypes, sourceApprovals, source, dest);
+    copyLabels(db, labelTypes, sourceApprovals, source, dest, trivialRebase);
   }
 
   /**
@@ -82,7 +82,7 @@ public class ApprovalsUtil {
    */
   public static void copyLabels(ReviewDb db, LabelTypes labelTypes,
       Iterable<PatchSetApproval> sourceApprovals, PatchSet.Id source,
-      PatchSet.Id dest) throws OrmException {
+      PatchSet dest, boolean trivialRebase) throws OrmException {
     List<PatchSetApproval> copied = Lists.newArrayList();
     for (PatchSetApproval a : sourceApprovals) {
       if (source.equals(a.getPatchSetId())) {
@@ -90,9 +90,11 @@ public class ApprovalsUtil {
         if (type == null) {
           continue;
         } else if (type.isCopyMinScore() && type.isMaxNegative(a)) {
-          copied.add(new PatchSetApproval(dest, a));
+          copied.add(new PatchSetApproval(dest.getId(), a));
         } else if (type.isCopyMaxScore() && type.isMaxPositive(a)) {
-          copied.add(new PatchSetApproval(dest, a));
+          copied.add(new PatchSetApproval(dest.getId(), a));
+        } else if (type.isCopyAllScoresOnTrivialRebase() && trivialRebase) {
+          copied.add(new PatchSetApproval(dest.getId(), a));
         }
       }
     }

gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java

@@ -25,6 +25,7 @@ import com.google.gerrit.reviewdb.client.ChangeMessage;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.PatchSetInfo;
+import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
@@ -33,12 +34,14 @@ import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.config.TrackingFooters;
 import com.google.gerrit.server.events.CommitReceivedEvent;
 import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
+import com.google.gerrit.server.git.MergeUtil;
 import com.google.gerrit.server.git.validators.CommitValidationException;
 import com.google.gerrit.server.git.validators.CommitValidators;
 import com.google.gerrit.server.index.ChangeIndexer;
 import com.google.gerrit.server.mail.ReplacePatchSetSender;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.project.InvalidChangeOperationException;
+import com.google.gerrit.server.project.ProjectState;
 import com.google.gerrit.server.project.RefControl;
 import com.google.gerrit.server.ssh.NoSshInfo;
 import com.google.gerrit.server.ssh.SshInfo;
@@ -50,6 +53,8 @@ import com.google.inject.assistedinject.Assisted;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.RefUpdate;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.merge.MergeStrategy;
+import org.eclipse.jgit.merge.ThreeWayMerger;
 import org.eclipse.jgit.revwalk.FooterLine;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
@@ -90,6 +95,7 @@ public class PatchSetInserter {
   private final CommitValidators.Factory commitValidatorsFactory;
   private final ChangeIndexer indexer;
   private final ReplacePatchSetSender.Factory replacePatchSetFactory;
+  private final MergeUtil.Factory mergeUtilFactory;
 
   private final Repository git;
   private final RevWalk revWalk;
@@ -115,6 +121,7 @@ public class PatchSetInserter {
       CommitValidators.Factory commitValidatorsFactory,
       ChangeIndexer indexer,
       ReplacePatchSetSender.Factory replacePatchSetFactory,
+      MergeUtil.Factory mergeUtilFactory,
       @Assisted Repository git,
       @Assisted RevWalk revWalk,
       @Assisted RefControl refControl,
@@ -130,6 +137,7 @@ public class PatchSetInserter {
     this.commitValidatorsFactory = commitValidatorsFactory;
     this.indexer = indexer;
     this.replacePatchSetFactory = replacePatchSetFactory;
+    this.mergeUtilFactory = mergeUtilFactory;
 
     this.git = git;
     this.revWalk = revWalk;
@@ -265,8 +273,16 @@ public class PatchSetInserter {
       }
 
       if (copyLabels) {
+        PatchSet priorPatchSet = db.patchSets().get(currentPatchSetId);
+        ObjectId priorCommitId = ObjectId.fromString(priorPatchSet.getRevision().get());
+        RevCommit priorCommit = revWalk.parseCommit(priorCommitId);
+        ProjectState projectState =
+            refControl.getProjectControl().getProjectState();
+        boolean trivialRebase =
+            isTrivialRebase(mergeUtilFactory, projectState, git, priorCommit, commit);
+
         ApprovalsUtil.copyLabels(db, refControl.getProjectControl()
-            .getLabelTypes(), currentPatchSetId, updatedChange.currentPatchSetId());
+            .getLabelTypes(), currentPatchSetId, patchSet, trivialRebase);
       }
 
       final List<FooterLine> footerLines = commit.getFooterLines();
@@ -346,6 +362,39 @@ public class PatchSetInserter {
     }
   }
 
+  public static boolean isTrivialRebase(MergeUtil.Factory mergeUtilFactory, ProjectState project,
+      Repository git, RevCommit prior, RevCommit next) {
+    if (!next.getFullMessage().equals(prior.getFullMessage())) {
+      return false;
+    }
+
+    if (prior.getParentCount() != 1 || next.getParentCount() != 1) {
+      // Trivial rebases done by machine only work well on 1 parent.
+      return false;
+    }
+
+    if (next.getTree() == prior.getTree() &&
+       prior.getParent(0).equals(next.getParent(0))) {
+      return true;
+    }
+
+    // A trivial rebase can be detected by looking for the next commit
+    // having the same tree as would exist when the prior commit is
+    // cherry-picked onto the next commit's new first parent.
+    try {
+      MergeUtil mergeUtil = mergeUtilFactory.create(project);
+      ThreeWayMerger merger =
+          mergeUtil.newThreeWayMerger(git, mergeUtil.createDryRunInserter());
+      merger.setBase(prior.getParent(0));
+      return merger.merge(next.getParent(0), prior)
+          && merger.getResultTreeId().equals(next.getTree());
+    } catch (IOException err) {
+      log.warn("Cannot check trivial rebase of new patch set " + next.name()
+          + " in " + project.getProject().getName(), err);
+      return false;
+    }
+  }
+
   public class ChangeModifiedException extends InvalidChangeOperationException {
     private static final long serialVersionUID = 1L;
 

gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java

@@ -487,15 +487,10 @@ public class MergeUtil {
 
   public ObjectInserter createDryRunInserter() {
     return new ObjectInserter() {
-      private final MutableObjectId buf = new MutableObjectId();
-      private static final int LAST_BYTE = Constants.OBJECT_ID_LENGTH - 1;
-
       @Override
       public ObjectId insert(int objectType, long length, InputStream in)
           throws IOException {
-        // create non-existing dummy ID
-        buf.setByte(LAST_BYTE, buf.getByte(LAST_BYTE) + 1);
-        return buf.copy();
+        return idFor(objectType, length, in);
       }
 
       @Override

gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java

@@ -126,6 +126,7 @@ public class ProjectConfig extends VersionedMetaData {
   private static final String KEY_FUNCTION = "function";
   private static final String KEY_COPY_MIN_SCORE = "copyMinScore";
   private static final String KEY_COPY_MAX_SCORE = "copyMaxScore";
+  private static final String KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE = "copyAllScoresOnTrivialRebase";
   private static final String KEY_VALUE = "value";
   private static final String KEY_CAN_OVERRIDE = "canOverride";
   private static final String KEY_Branch = "branch";
@@ -651,6 +652,8 @@ public class ProjectConfig extends VersionedMetaData {
           rc.getBoolean(LABEL, name, KEY_COPY_MIN_SCORE, false));
       label.setCopyMaxScore(
           rc.getBoolean(LABEL, name, KEY_COPY_MAX_SCORE, false));
+      label.setCopyAllScoresOnTrivialRebase(
+          rc.getBoolean(LABEL, name, KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE, false));
       label.setCanOverride(
           rc.getBoolean(LABEL, name, KEY_CAN_OVERRIDE, true));
       label.setRefPatterns(getStringListOrNull(rc, LABEL, name, KEY_Branch));
@@ -978,6 +981,11 @@ public class ProjectConfig extends VersionedMetaData {
       } else {
         rc.unset(LABEL, name, KEY_COPY_MAX_SCORE);
       }
+      if (label.isCopyAllScoresOnTrivialRebase()) {
+        rc.setBoolean(LABEL, name, KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE, true);
+      } else {
+        rc.unset(LABEL, name, KEY_COPY_ALL_SCORES_ON_TRIVIAL_REBASE);
+      }
       if (!label.canOverride()) {
         rc.setBoolean(LABEL, name, KEY_CAN_OVERRIDE, false);
       } else {

gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java

@@ -65,6 +65,7 @@ import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.account.AccountResolver;
 import com.google.gerrit.server.change.ChangeInserter;
 import com.google.gerrit.server.change.ChangeResource;
+import com.google.gerrit.server.change.PatchSetInserter;
 import com.google.gerrit.server.change.RevisionResource;
 import com.google.gerrit.server.change.Submit;
 import com.google.gerrit.server.config.AllProjectsName;
@@ -294,6 +295,7 @@ public class ReceiveCommits {
   private final SubmoduleOp.Factory subOpFactory;
   private final Provider<Submit> submitProvider;
   private final MergeQueue mergeQueue;
+  private final MergeUtil.Factory mergeUtilFactory;
 
   private final List<CommitValidationMessage> messages = new ArrayList<CommitValidationMessage>();
   private ListMultimap<Error, String> errors = LinkedListMultimap.create();
@@ -337,7 +339,8 @@ public class ReceiveCommits {
       @Assisted final Repository repo,
       final SubmoduleOp.Factory subOpFactory,
       final Provider<Submit> submitProvider,
-      final MergeQueue mergeQueue) throws IOException {
+      final MergeQueue mergeQueue,
+      final MergeUtil.Factory mergeUtilFactory) throws IOException {
     this.currentUser = (IdentifiedUser) projectControl.getCurrentUser();
     this.db = db;
     this.schemaFactory = schemaFactory;
@@ -376,6 +379,7 @@ public class ReceiveCommits {
     this.subOpFactory = subOpFactory;
     this.submitProvider = submitProvider;
     this.mergeQueue = mergeQueue;
+    this.mergeUtilFactory = mergeUtilFactory;
 
     this.messageSender = new ReceivePackMessageSender();
 
@@ -1597,6 +1601,7 @@ public class ReceiveCommits {
     ChangeMessage msg;
     String mergedIntoRef;
     boolean skip;
+    boolean trivialRebase;
     private PatchSet.Id priorPatchSet;
 
     ReplaceRequest(final Change.Id toChange, final RevCommit newCommit,
@@ -1605,6 +1610,7 @@ public class ReceiveCommits {
       this.newCommit = newCommit;
       this.inputCommand = cmd;
       this.checkMergedInto = checkMergedInto;
+      this.trivialRebase = false;
 
       revisions = HashBiMap.create();
       for (Ref ref : refs(toChange)) {
@@ -1705,6 +1711,10 @@ public class ReceiveCommits {
         }
       }
 
+      trivialRebase =
+          PatchSetInserter.isTrivialRebase(mergeUtilFactory,
+              projectControl.getProjectState(), repo, priorCommit, newCommit);
+
       PatchSet.Id id =
           ChangeUtil.nextPatchSetId(allRefs, change.currentPatchSetId());
       newPatchSet = new PatchSet(id);
@@ -1783,7 +1793,7 @@ public class ReceiveCommits {
         final MailRecipients oldRecipients = getRecipientsFromApprovals(
             oldChangeApprovals);
         ApprovalsUtil.copyLabels(db, labelTypes, oldChangeApprovals,
-            priorPatchSet, newPatchSet.getId());
+            priorPatchSet, newPatchSet, trivialRebase);
         approvalsUtil.addReviewers(db, labelTypes, change, newPatchSet, info,
             recipients.getReviewers(), oldRecipients.getAll());
         recipients.add(oldRecipients);