Remove rework check for dependencies in RebaseSorter
The REWORK check that was added by me in Change 91703, is just to be cautious. However, after some thoughts about this, I don't think it is really necessary based on the following facts: * The diff of the change (which based on some old ps of its parent change) has been reviewed. * If there was a major rework on the parent change, there will be a merge conflict when the change is merged. Change-Id: Iabf544e8a119e262dd54e78d470af2e2800cfe4b
This commit is contained in:
		@@ -648,14 +648,27 @@ public abstract class AbstractDaemonTest {
 | 
			
		||||
      String changeId, String ref, TestAccount testAccount, TestRepository<?> repo)
 | 
			
		||||
      throws Exception {
 | 
			
		||||
    Collections.shuffle(RANDOM);
 | 
			
		||||
    return amendChange(changeId, ref, testAccount, repo, PushOneCommit.SUBJECT,
 | 
			
		||||
        PushOneCommit.FILE_NAME, new String(Chars.toArray(RANDOM)));
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  protected PushOneCommit.Result amendChange(String changeId, String subject, String fileName,
 | 
			
		||||
      String content) throws Exception {
 | 
			
		||||
    return amendChange(changeId, "refs/for/master", admin, testRepo, subject, fileName, content);
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  protected PushOneCommit.Result amendChange(
 | 
			
		||||
      String changeId, String ref, TestAccount testAccount, TestRepository<?> repo, String subject,
 | 
			
		||||
      String fileName, String content)
 | 
			
		||||
      throws Exception {
 | 
			
		||||
    PushOneCommit push =
 | 
			
		||||
        pushFactory.create(
 | 
			
		||||
            db,
 | 
			
		||||
            testAccount.getIdent(),
 | 
			
		||||
            repo,
 | 
			
		||||
            PushOneCommit.SUBJECT,
 | 
			
		||||
            PushOneCommit.FILE_NAME,
 | 
			
		||||
            new String(Chars.toArray(RANDOM)),
 | 
			
		||||
            subject,
 | 
			
		||||
            fileName,
 | 
			
		||||
            content,
 | 
			
		||||
            changeId);
 | 
			
		||||
    return push.to(ref);
 | 
			
		||||
  }
 | 
			
		||||
 
 | 
			
		||||
@@ -412,6 +412,27 @@ public abstract class AbstractSubmitByRebase extends AbstractSubmit {
 | 
			
		||||
    submit(change2.getChangeId());
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  @Test
 | 
			
		||||
  @TestProjectInput(useContentMerge = InheritableBoolean.TRUE)
 | 
			
		||||
  public void submitChainFailsOnRework() throws Exception {
 | 
			
		||||
    PushOneCommit.Result change1 = createChange("subject 1", "fileName 1", "content 1");
 | 
			
		||||
    RevCommit headAfterChange1 = change1.getCommit();
 | 
			
		||||
    PushOneCommit.Result change2 = createChange("subject 2", "fileName 2", "content 2");
 | 
			
		||||
    testRepo.reset(headAfterChange1);
 | 
			
		||||
    change1 = amendChange(change1.getChangeId(), "subject 1 amend", "fileName 2",
 | 
			
		||||
        "rework content 2");
 | 
			
		||||
    submit(change1.getChangeId());
 | 
			
		||||
    headAfterChange1 = getRemoteHead();
 | 
			
		||||
 | 
			
		||||
    submitWithConflict(
 | 
			
		||||
        change2.getChangeId(),
 | 
			
		||||
        "Cannot rebase "
 | 
			
		||||
            + change2.getCommit().getName()
 | 
			
		||||
            + ": "
 | 
			
		||||
            + "The change could not be rebased due to a conflict during merge.");
 | 
			
		||||
    assertThat(getRemoteHead()).isEqualTo(headAfterChange1);
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  @Test
 | 
			
		||||
  @TestProjectInput(useContentMerge = InheritableBoolean.TRUE)
 | 
			
		||||
  public void submitChainOneByOneManualRebase() throws Exception {
 | 
			
		||||
 
 | 
			
		||||
@@ -14,11 +14,8 @@
 | 
			
		||||
 | 
			
		||||
package com.google.gerrit.server.git;
 | 
			
		||||
 | 
			
		||||
import com.google.gerrit.extensions.client.ChangeKind;
 | 
			
		||||
import com.google.gerrit.reviewdb.client.Branch;
 | 
			
		||||
import com.google.gerrit.reviewdb.client.Change.Status;
 | 
			
		||||
import com.google.gerrit.reviewdb.client.Project;
 | 
			
		||||
import com.google.gerrit.server.change.ChangeKindCache;
 | 
			
		||||
import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
 | 
			
		||||
import com.google.gerrit.server.git.strategy.CommitMergeStatus;
 | 
			
		||||
import com.google.gerrit.server.query.change.ChangeData;
 | 
			
		||||
@@ -32,8 +29,6 @@ import java.util.HashSet;
 | 
			
		||||
import java.util.Iterator;
 | 
			
		||||
import java.util.List;
 | 
			
		||||
import java.util.Set;
 | 
			
		||||
import org.eclipse.jgit.lib.ObjectId;
 | 
			
		||||
import org.eclipse.jgit.lib.Repository;
 | 
			
		||||
import org.eclipse.jgit.revwalk.RevCommit;
 | 
			
		||||
import org.eclipse.jgit.revwalk.RevFlag;
 | 
			
		||||
import org.slf4j.Logger;
 | 
			
		||||
@@ -47,24 +42,18 @@ public class RebaseSorter {
 | 
			
		||||
  private final RevCommit initialTip;
 | 
			
		||||
  private final Set<RevCommit> alreadyAccepted;
 | 
			
		||||
  private final InternalChangeQuery internalChangeQuery;
 | 
			
		||||
  private final ChangeKindCache changeKindCache;
 | 
			
		||||
  private final Repository repo;
 | 
			
		||||
 | 
			
		||||
  public RebaseSorter(
 | 
			
		||||
      CodeReviewRevWalk rw,
 | 
			
		||||
      RevCommit initialTip,
 | 
			
		||||
      Set<RevCommit> alreadyAccepted,
 | 
			
		||||
      RevFlag canMergeFlag,
 | 
			
		||||
      InternalChangeQuery internalChangeQuery,
 | 
			
		||||
      ChangeKindCache changeKindCache,
 | 
			
		||||
      Repository repo) {
 | 
			
		||||
      InternalChangeQuery internalChangeQuery) {
 | 
			
		||||
    this.rw = rw;
 | 
			
		||||
    this.canMergeFlag = canMergeFlag;
 | 
			
		||||
    this.initialTip = initialTip;
 | 
			
		||||
    this.alreadyAccepted = alreadyAccepted;
 | 
			
		||||
    this.internalChangeQuery = internalChangeQuery;
 | 
			
		||||
    this.changeKindCache = changeKindCache;
 | 
			
		||||
    this.repo = repo;
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  public List<CodeReviewCommit> sort(Collection<CodeReviewCommit> incoming) throws IOException {
 | 
			
		||||
@@ -127,8 +116,7 @@ public class RebaseSorter {
 | 
			
		||||
      List<ChangeData> changes = internalChangeQuery.byCommit(commit);
 | 
			
		||||
      for (ChangeData change : changes) {
 | 
			
		||||
        if (change.change().getStatus() == Status.MERGED
 | 
			
		||||
            && change.change().getDest().equals(dest)
 | 
			
		||||
            && !isRework(dest.getParentKey(), commit, change)) {
 | 
			
		||||
            && change.change().getDest().equals(dest)) {
 | 
			
		||||
          log.debug(
 | 
			
		||||
              "Dependency {} associated with merged change {}.", commit.getName(), change.getId());
 | 
			
		||||
          return true;
 | 
			
		||||
@@ -140,14 +128,6 @@ public class RebaseSorter {
 | 
			
		||||
    }
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  private boolean isRework(Project.NameKey project, RevCommit oldCommit, ChangeData change)
 | 
			
		||||
      throws OrmException, IOException {
 | 
			
		||||
    RevCommit currentCommit =
 | 
			
		||||
        rw.parseCommit(ObjectId.fromString(change.currentPatchSet().getRevision().get()));
 | 
			
		||||
    return ChangeKind.REWORK
 | 
			
		||||
        == changeKindCache.getChangeKind(project, repo, oldCommit, currentCommit);
 | 
			
		||||
  }
 | 
			
		||||
 | 
			
		||||
  private static <T> T removeOne(final Collection<T> c) {
 | 
			
		||||
    final Iterator<T> i = c.iterator();
 | 
			
		||||
    final T r = i.next();
 | 
			
		||||
 
 | 
			
		||||
@@ -295,9 +295,7 @@ public class RebaseSubmitStrategy extends SubmitStrategy {
 | 
			
		||||
              args.mergeTip.getInitialTip(),
 | 
			
		||||
              args.alreadyAccepted,
 | 
			
		||||
              args.canMergeFlag,
 | 
			
		||||
              args.internalChangeQuery,
 | 
			
		||||
              args.changeKindCache,
 | 
			
		||||
              args.repo)
 | 
			
		||||
              args.internalChangeQuery)
 | 
			
		||||
          .sort(toSort);
 | 
			
		||||
    } catch (IOException e) {
 | 
			
		||||
      throw new IntegrationException("Commit sorting failed", e);
 | 
			
		||||
 
 | 
			
		||||
@@ -31,7 +31,6 @@ import com.google.gerrit.server.GerritPersonIdent;
 | 
			
		||||
import com.google.gerrit.server.IdentifiedUser;
 | 
			
		||||
import com.google.gerrit.server.PatchSetUtil;
 | 
			
		||||
import com.google.gerrit.server.account.AccountCache;
 | 
			
		||||
import com.google.gerrit.server.change.ChangeKindCache;
 | 
			
		||||
import com.google.gerrit.server.change.RebaseChangeOp;
 | 
			
		||||
import com.google.gerrit.server.extensions.events.ChangeMerged;
 | 
			
		||||
import com.google.gerrit.server.git.CodeReviewCommit;
 | 
			
		||||
@@ -123,7 +122,6 @@ public abstract class SubmitStrategy {
 | 
			
		||||
    final OnSubmitValidators.Factory onSubmitValidatorsFactory;
 | 
			
		||||
    final TagCache tagCache;
 | 
			
		||||
    final InternalChangeQuery internalChangeQuery;
 | 
			
		||||
    final ChangeKindCache changeKindCache;
 | 
			
		||||
 | 
			
		||||
    final Branch.NameKey destBranch;
 | 
			
		||||
    final CodeReviewRevWalk rw;
 | 
			
		||||
@@ -167,7 +165,6 @@ public abstract class SubmitStrategy {
 | 
			
		||||
        OnSubmitValidators.Factory onSubmitValidatorsFactory,
 | 
			
		||||
        TagCache tagCache,
 | 
			
		||||
        InternalChangeQuery internalChangeQuery,
 | 
			
		||||
        ChangeKindCache changeKindCache,
 | 
			
		||||
        @Assisted Branch.NameKey destBranch,
 | 
			
		||||
        @Assisted CommitStatus commitStatus,
 | 
			
		||||
        @Assisted CodeReviewRevWalk rw,
 | 
			
		||||
@@ -200,7 +197,6 @@ public abstract class SubmitStrategy {
 | 
			
		||||
      this.rebaseFactory = rebaseFactory;
 | 
			
		||||
      this.tagCache = tagCache;
 | 
			
		||||
      this.internalChangeQuery = internalChangeQuery;
 | 
			
		||||
      this.changeKindCache = changeKindCache;
 | 
			
		||||
 | 
			
		||||
      this.serverIdent = serverIdent;
 | 
			
		||||
      this.destBranch = destBranch;
 | 
			
		||||
 
 | 
			
		||||
		Reference in New Issue
	
	Block a user