Allow rebase with conflicts

So far if a change could not be rebased due to conflicts the Rebase
Change REST endpoint failed with '409 Conflict'. This change adds a new
input option that allows the rebase to succeed even if there are
conflicts. Adding this option e.g. enables third-party integrators to
implement an online editor that allows users to rebase changes and then
resolve conflicts online.

If the new option is set and there are conflicts, the rebase succeeds
and the new patch set is created, the change is set to work-in-progress,
the conflicting files contain git conflict markers and a change message
with the conflicting files is posted. In addition the returned
ChangeInfo has the 'contains_git_conflicts' field set to true so that
callers can detect that there were conflicts. This is consistent with
the same option that change Ib6bc8eedf added for the Create Change and
Create Merge Patch Set REST endpoints. The implementation follows the
example of these existing options, but some things are worth to be
pointed out:

* rebasing with conflicts only works if content merges are enabled
  (see RebaseChangeOp#setForceContentMerge(boolean)):
  - this is always true for the Rebase Change REST endpoint
  - this is not true for Rebases that are automatically done on submit
  - the forceContentMerge decides which Merger is used to create the
    merge commit, if forceContentMerge is true, the created Merger is a
    ResolveMerger, which is the Merger that allows to merge with
    conflicts
* to convey the information about which files have conflicts we need to
  create a CodeReviewCommit rather than a RevCommit:
  - to create a CodeReviewCommit we need a CodeReviewRevWalk, this rev
    walk must be set on the BatchUpdate that executes the rebase
    operation (the rebase operation can then cast the RevWalk from the
    context to CodeReviewRevWalk, this is a bit error prone, but a
    pattern that we already use, e.g. in SubmitStrategyOp)
  - when the rebase operation is used from a submit strategy the RevWalk
    in the context already was a CodeReviewRevWalk, for the Rebase
    Change REST endpoint we are setting it now
* the 'contains_git_conflicts' field in ChangeInfo is only populated in
  the ChangeInfo that is returned by the Rebase Change REST endpoint,
  but not when then ChangeInfo is newly retrieved from the server (this
  is because the information whether the change has conflicts is not
  persisted, which is the same as for the other REST endpoints that
  support this field)
* in the Java API we had to add a new method that exposes the ChangeInfo
  that is returned by the Rebase Change REST endpoint
  (RevisionApi#rebaseAsInfo(RebaseInput)), this follows the example of
  the Changes#createAsInfo(ChangeInput) method

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

Documentation/rest-api-changes.txt

@@ -7488,6 +7488,13 @@ Alternatively, a change number can be specified, in which case the current
 patch set is inferred. +
 Empty string is used for rebasing directly on top of the target branch,
 which effectively breaks dependency towards a parent change.
+|`allow_conflicts`|optional, defaults to false|
+If `true`, the rebase also succeeds if there are conflicts. +
+If there are conflicts the file contents of the rebased patch set contain
+git conflict markers to indicate the conflicts. +
+Callers can find out whether there were conflicts by checking the
+`contains_git_conflicts` field in the returned link:#change-info[ChangeInfo]. +
+If there are conflicts the change is marked as work-in-progress.
 |===========================
 
 [[related-change-and-commit-info]]

java/com/google/gerrit/extensions/api/changes/RebaseInput.java

@@ -16,4 +16,12 @@ package com.google.gerrit.extensions.api.changes;
 
 public class RebaseInput {
   public String base;
+
+  /**
+   * Whether the rebase should succeed if there are conflicts.
+   *
+   * <p>If there are conflicts the file contents of the rebased change contain git conflict markers
+   * to indicate the conflicts.
+   */
+  public boolean allowConflicts;
 }

java/com/google/gerrit/extensions/api/changes/RevisionApi.java

@@ -68,6 +68,8 @@ public interface RevisionApi {
 
   ChangeApi rebase(RebaseInput in) throws RestApiException;
 
+  ChangeInfo rebaseAsInfo(RebaseInput in) throws RestApiException;
+
   boolean canRebase() throws RestApiException;
 
   RevisionReviewerApi reviewer(String id) throws RestApiException;
@@ -217,6 +219,11 @@ public interface RevisionApi {
       throw new NotImplementedException();
     }
 
+    @Override
+    public ChangeInfo rebaseAsInfo(RebaseInput in) throws RestApiException {
+      throw new NotImplementedException();
+    }
+
     @Override
     public boolean canRebase() throws RestApiException {
       throw new NotImplementedException();

java/com/google/gerrit/extensions/common/ChangeInfo.java

@@ -83,7 +83,8 @@ public class ChangeInfo {
    * com.google.gerrit.server.restapi.change.CreateChange}, {@link
    * com.google.gerrit.server.restapi.change.CreateMergePatchSet}, {@link
    * com.google.gerrit.server.restapi.change.CherryPick}, {@link
-   * com.google.gerrit.server.restapi.change.CherryPickCommit}
+   * com.google.gerrit.server.restapi.change.CherryPickCommit}, {@link
+   * com.google.gerrit.server.restapi.change.Rebase}
    */
   public Boolean containsGitConflicts;
 

java/com/google/gerrit/server/api/changes/RevisionApiImpl.java

@@ -282,7 +282,16 @@ class RevisionApiImpl implements RevisionApi {
   @Override
   public ChangeApi rebase(RebaseInput in) throws RestApiException {
     try {
-      return changes.id(rebase.apply(revision, in).value()._number);
+      return changes.id(rebaseAsInfo(in)._number);
+    } catch (Exception e) {
+      throw asRestApiException("Cannot rebase ps", e);
+    }
+  }
+
+  @Override
+  public ChangeInfo rebaseAsInfo(RebaseInput in) throws RestApiException {
+    try {
+      return rebase.apply(revision, in).value();
     } catch (Exception e) {
       throw asRestApiException("Cannot rebase ps", e);
     }

java/com/google/gerrit/server/change/RebaseChangeOp.java

@@ -15,8 +15,10 @@
 package com.google.gerrit.server.change;
 
 import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.gerrit.server.project.ProjectCache.illegalState;
 
+import com.google.common.collect.ImmutableSet;
 import com.google.gerrit.entities.PatchSet;
 import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.MergeConflictException;
@@ -26,6 +28,8 @@ import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.change.RebaseUtil.Base;
+import com.google.gerrit.server.git.CodeReviewCommit;
+import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
 import com.google.gerrit.server.git.GroupCollector;
 import com.google.gerrit.server.git.MergeUtil;
 import com.google.gerrit.server.notedb.ChangeNotes;
@@ -41,13 +45,26 @@ import com.google.gerrit.server.update.RepoContext;
 import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
 import java.io.IOException;
+import java.util.Map;
+import org.eclipse.jgit.diff.Sequence;
+import org.eclipse.jgit.dircache.DirCache;
 import org.eclipse.jgit.lib.CommitBuilder;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.merge.MergeResult;
+import org.eclipse.jgit.merge.ResolveMerger;
 import org.eclipse.jgit.merge.ThreeWayMerger;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
 
+/**
+ * BatchUpdate operation that rebases a change.
+ *
+ * <p>Can only be executed in a {@link com.google.gerrit.server.update.BatchUpdate} set has a {@link
+ * CodeReviewRevWalk} set as {@link RevWalk} (set via {@link
+ * com.google.gerrit.server.update.BatchUpdate#setRepository(org.eclipse.jgit.lib.Repository,
+ * RevWalk, org.eclipse.jgit.lib.ObjectInserter)}).
+ */
 public class RebaseChangeOp implements BatchUpdateOp {
   public interface Factory {
     RebaseChangeOp create(ChangeNotes notes, PatchSet originalPatchSet, ObjectId baseCommitId);
@@ -69,12 +86,13 @@ public class RebaseChangeOp implements BatchUpdateOp {
   private boolean validate = true;
   private boolean checkAddPatchSetPermission = true;
   private boolean forceContentMerge;
+  private boolean allowConflicts;
   private boolean detailedCommitMessage;
   private boolean postMessage = true;
   private boolean sendEmail = true;
   private boolean matchAuthorToCommitterDate = false;
 
-  private RevCommit rebasedCommit;
+  private CodeReviewCommit rebasedCommit;
   private PatchSet.Id rebasedPatchSetId;
   private PatchSetInserter patchSetInserter;
   private PatchSet rebasedPatchSet;
@@ -126,6 +144,19 @@ public class RebaseChangeOp implements BatchUpdateOp {
     return this;
   }
 
+  /**
+   * Allows the rebase to succeed if there are conflicts.
+   *
+   * <p>This setting requires that {@link #forceContentMerge} is set {@code true}. If {@link
+   * #forceContentMerge} is {@code false} this setting has no effect.
+   *
+   * @see #setForceContentMerge(boolean)
+   */
+  public RebaseChangeOp setAllowConflicts(boolean allowConflicts) {
+    this.allowConflicts = allowConflicts;
+    return this;
+  }
+
   public RebaseChangeOp setDetailedCommitMessage(boolean detailedCommitMessage) {
     this.detailedCommitMessage = detailedCommitMessage;
     return this;
@@ -186,14 +217,11 @@ public class RebaseChangeOp implements BatchUpdateOp {
             .setFireRevisionCreated(fireRevisionCreated)
             .setCheckAddPatchSetPermission(checkAddPatchSetPermission)
             .setValidate(validate)
-            .setSendEmail(sendEmail);
+            .setSendEmail(sendEmail)
+            .setWorkInProgress(!rebasedCommit.getFilesWithGitConflicts().isEmpty());
     if (postMessage) {
       patchSetInserter.setMessage(
-          "Patch Set "
-              + rebasedPatchSetId.get()
-              + ": Patch Set "
-              + originalPatchSet.id().get()
-              + " was rebased");
+          messageForRebasedChange(rebasedPatchSetId, originalPatchSet.id(), rebasedCommit));
     }
 
     if (base != null && !base.notes().getChange().isMerged()) {
@@ -208,6 +236,24 @@ public class RebaseChangeOp implements BatchUpdateOp {
     patchSetInserter.updateRepo(ctx);
   }
 
+  private static String messageForRebasedChange(
+      PatchSet.Id rebasePatchSetId, PatchSet.Id originalPatchSetId, CodeReviewCommit commit) {
+    StringBuilder stringBuilder =
+        new StringBuilder(
+            String.format(
+                "Patch Set %d: Patch Set %d was rebased",
+                rebasePatchSetId.get(), originalPatchSetId.get()));
+
+    if (!commit.getFilesWithGitConflicts().isEmpty()) {
+      stringBuilder.append("\n\nThe following files contain Git conflicts:\n");
+      commit.getFilesWithGitConflicts().stream()
+          .sorted()
+          .forEach(filePath -> stringBuilder.append("* ").append(filePath).append("\n"));
+    }
+
+    return stringBuilder.toString();
+  }
+
   @Override
   public boolean updateChange(ChangeContext ctx)
       throws ResourceConflictException, IOException, BadRequestException {
@@ -221,7 +267,7 @@ public class RebaseChangeOp implements BatchUpdateOp {
     patchSetInserter.postUpdate(ctx);
   }
 
-  public RevCommit getRebasedCommit() {
+  public CodeReviewCommit getRebasedCommit() {
     checkState(rebasedCommit != null, "getRebasedCommit() only valid after updateRepo");
     return rebasedCommit;
   }
@@ -254,7 +300,7 @@ public class RebaseChangeOp implements BatchUpdateOp {
    * @throws MergeConflictException the rebase failed due to a merge conflict.
    * @throws IOException the merge failed for another reason.
    */
-  private RevCommit rebaseCommit(
+  private CodeReviewCommit rebaseCommit(
       RepoContext ctx, RevCommit original, ObjectId base, String commitMessage)
       throws ResourceConflictException, IOException {
     RevCommit parentCommit = original.getParent(0);
@@ -266,15 +312,50 @@ public class RebaseChangeOp implements BatchUpdateOp {
     ThreeWayMerger merger =
         newMergeUtil().newThreeWayMerger(ctx.getInserter(), ctx.getRepoView().getConfig());
     merger.setBase(parentCommit);
+
+    DirCache dc = DirCache.newInCore();
+    if (allowConflicts && merger instanceof ResolveMerger) {
+      // The DirCache must be set on ResolveMerger before calling
+      // ResolveMerger#merge(AnyObjectId...) otherwise the entries in DirCache don't get populated.
+      ((ResolveMerger) merger).setDirCache(dc);
+    }
+
     boolean success = merger.merge(original, base);
 
-    if (!success || merger.getResultTreeId() == null) {
+    ObjectId tree;
+    ImmutableSet<String> filesWithGitConflicts;
+    if (success) {
+      filesWithGitConflicts = null;
+      tree = merger.getResultTreeId();
+    } else {
+      if (!allowConflicts || !(merger instanceof ResolveMerger)) {
         throw new MergeConflictException(
             "The change could not be rebased due to a conflict during merge.");
       }
 
+      Map<String, MergeResult<? extends Sequence>> mergeResults =
+          ((ResolveMerger) merger).getMergeResults();
+
+      filesWithGitConflicts =
+          mergeResults.entrySet().stream()
+              .filter(e -> e.getValue().containsConflicts())
+              .map(Map.Entry::getKey)
+              .collect(toImmutableSet());
+
+      tree =
+          MergeUtil.mergeWithConflicts(
+              ctx.getRevWalk(),
+              ctx.getInserter(),
+              dc,
+              "PATCH SET",
+              original,
+              "BASE",
+              ctx.getRevWalk().parseCommit(base),
+              mergeResults);
+    }
+
     CommitBuilder cb = new CommitBuilder();
-    cb.setTreeId(merger.getResultTreeId());
+    cb.setTreeId(tree);
     cb.setParentId(base);
     cb.setAuthor(original.getAuthorIdent());
     cb.setMessage(commitMessage);
@@ -290,6 +371,8 @@ public class RebaseChangeOp implements BatchUpdateOp {
     }
     ObjectId objectId = ctx.getInserter().insert(cb);
     ctx.getInserter().flush();
-    return ctx.getRevWalk().parseCommit(objectId);
+    CodeReviewCommit commit = ((CodeReviewRevWalk) ctx.getRevWalk()).parseCommit(objectId);
+    commit.setFilesWithGitConflicts(filesWithGitConflicts);
+    return commit;
   }
 }

java/com/google/gerrit/server/restapi/change/Rebase.java

@@ -42,6 +42,7 @@ import com.google.gerrit.server.change.RebaseChangeOp;
 import com.google.gerrit.server.change.RebaseUtil;
 import com.google.gerrit.server.change.RebaseUtil.Base;
 import com.google.gerrit.server.change.RevisionResource;
+import com.google.gerrit.server.git.CodeReviewCommit;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.permissions.ChangePermission;
 import com.google.gerrit.server.permissions.PermissionBackend;
@@ -115,7 +116,7 @@ public class Rebase
     try (Repository repo = repoManager.openRepository(change.getProject());
         ObjectInserter oi = repo.newObjectInserter();
         ObjectReader reader = oi.newReader();
-        RevWalk rw = new RevWalk(reader);
+        RevWalk rw = CodeReviewCommit.newRevWalk(reader);
         BatchUpdate bu =
             updateFactory.create(change.getProject(), rsrc.getUser(), TimeUtil.nowTs())) {
       if (!change.isNew()) {
@@ -124,18 +125,23 @@ public class Rebase
         throw new ResourceConflictException(
             "cannot rebase merge commits or commit with no ancestor");
       }
-      // TODO(dborowitz): Why no notification? This seems wrong; dig up blame.
-      bu.setNotify(NotifyResolver.Result.none());
-      bu.setRepository(repo, rw, oi);
-      bu.addOp(
-          change.getId(),
+      RebaseChangeOp rebaseOp =
           rebaseFactory
               .create(rsrc.getNotes(), rsrc.getPatchSet(), findBaseRev(repo, rw, rsrc, input))
               .setForceContentMerge(true)
-              .setFireRevisionCreated(true));
+              .setAllowConflicts(input.allowConflicts)
+              .setFireRevisionCreated(true);
+      // TODO(dborowitz): Why no notification? This seems wrong; dig up blame.
+      bu.setNotify(NotifyResolver.Result.none());
+      bu.setRepository(repo, rw, oi);
+      bu.addOp(change.getId(), rebaseOp);
       bu.execute();
+
+      ChangeInfo changeInfo = json.create(OPTIONS).format(change.getProject(), change.getId());
+      changeInfo.containsGitConflicts =
+          !rebaseOp.getRebasedCommit().getFilesWithGitConflicts().isEmpty() ? true : null;
+      return Response.ok(changeInfo);
     }
-    return Response.ok(json.create(OPTIONS).format(change.getProject(), change.getId()));
   }
 
   private ObjectId findBaseRev(

javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -47,6 +47,7 @@ import static com.google.gerrit.extensions.client.ListChangesOption.TRACKING_IDS
 import static com.google.gerrit.extensions.client.ReviewerState.CC;
 import static com.google.gerrit.extensions.client.ReviewerState.REMOVED;
 import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
+import static com.google.gerrit.git.ObjectIds.abbreviateName;
 import static com.google.gerrit.server.StarredChangesUtil.DEFAULT_LABEL;
 import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
 import static com.google.gerrit.server.group.SystemGroupBackend.CHANGE_OWNER;
@@ -142,8 +143,10 @@ import com.google.gerrit.extensions.common.GitPerson;
 import com.google.gerrit.extensions.common.LabelInfo;
 import com.google.gerrit.extensions.common.RevisionInfo;
 import com.google.gerrit.extensions.common.TrackingIdInfo;
+import com.google.gerrit.extensions.events.WorkInProgressStateChangedListener;
 import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.BinaryResult;
 import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
@@ -178,6 +181,7 @@ import com.google.gerrit.testing.FakeEmailSender.Message;
 import com.google.inject.AbstractModule;
 import com.google.inject.Inject;
 import com.google.inject.name.Named;
+import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.sql.Timestamp;
 import java.util.ArrayList;
@@ -1345,6 +1349,91 @@ public class ChangeIT extends AbstractDaemonTest {
         () -> gApi.changes().id(r2.getChangeId()).revision(r2.getCommit().name()).rebase());
   }
 
+  @Test
+  public void rebaseConflict_conflictsAllowed() throws Exception {
+    String patchSetSubject = "patch set change";
+    String patchSetContent = "patch set content";
+    String baseSubject = "base change";
+    String baseContent = "base content";
+
+    PushOneCommit.Result r1 = createChange(baseSubject, PushOneCommit.FILE_NAME, baseContent);
+    gApi.changes()
+        .id(r1.getChangeId())
+        .revision(r1.getCommit().name())
+        .review(ReviewInput.approve());
+    gApi.changes().id(r1.getChangeId()).revision(r1.getCommit().name()).submit();
+
+    testRepo.reset("HEAD~1");
+    PushOneCommit push =
+        pushFactory.create(
+            admin.newIdent(), testRepo, patchSetSubject, PushOneCommit.FILE_NAME, patchSetContent);
+    PushOneCommit.Result r2 = push.to("refs/for/master");
+    r2.assertOkStatus();
+
+    String changeId = r2.getChangeId();
+    RevCommit patchSet = r2.getCommit();
+    RevCommit base = r1.getCommit();
+
+    TestWorkInProgressStateChangedListener wipStateChangedListener =
+        new TestWorkInProgressStateChangedListener();
+    try (Registration registration =
+        extensionRegistry.newRegistration().add(wipStateChangedListener)) {
+      RebaseInput rebaseInput = new RebaseInput();
+      rebaseInput.allowConflicts = true;
+      ChangeInfo changeInfo =
+          gApi.changes().id(changeId).revision(patchSet.name()).rebaseAsInfo(rebaseInput);
+      assertThat(changeInfo.containsGitConflicts).isTrue();
+      assertThat(changeInfo.workInProgress).isTrue();
+    }
+    assertThat(wipStateChangedListener.invoked).isTrue();
+    assertThat(wipStateChangedListener.wip).isTrue();
+
+    // To get the revisions, we must retrieve the change with more change options.
+    ChangeInfo changeInfo =
+        gApi.changes().id(changeId).get(ALL_REVISIONS, CURRENT_COMMIT, CURRENT_REVISION);
+    assertThat(changeInfo.revisions).hasSize(2);
+    assertThat(changeInfo.revisions.get(changeInfo.currentRevision).commit.parents.get(0).commit)
+        .isEqualTo(base.name());
+
+    // Verify that the file content in the created patch set is correct.
+    // We expect that it has conflict markers to indicate the conflict.
+    BinaryResult bin =
+        gApi.changes().id(changeId).current().file(PushOneCommit.FILE_NAME).content();
+    ByteArrayOutputStream os = new ByteArrayOutputStream();
+    bin.writeTo(os);
+    String fileContent = new String(os.toByteArray(), UTF_8);
+    String patchSetSha1 = abbreviateName(patchSet, 6);
+    String baseSha1 = abbreviateName(base, 6);
+    assertThat(fileContent)
+        .isEqualTo(
+            "<<<<<<< PATCH SET ("
+                + patchSetSha1
+                + " "
+                + patchSetSubject
+                + ")\n"
+                + patchSetContent
+                + "\n"
+                + "=======\n"
+                + baseContent
+                + "\n"
+                + ">>>>>>> BASE      ("
+                + baseSha1
+                + " "
+                + baseSubject
+                + ")\n");
+
+    // Verify the message that has been posted on the change.
+    List<ChangeMessageInfo> messages = gApi.changes().id(changeId).messages();
+    assertThat(messages).hasSize(2);
+    assertThat(Iterables.getLast(messages).message)
+        .isEqualTo(
+            "Patch Set 2: Patch Set 1 was rebased\n\n"
+                + "The following files contain Git conflicts:\n"
+                + "* "
+                + PushOneCommit.FILE_NAME
+                + "\n");
+  }
+
   @Test
   public void rebaseChangeBase() throws Exception {
     PushOneCommit.Result r1 = createChange();
@@ -4354,4 +4443,17 @@ public class ChangeIT extends AbstractDaemonTest {
   private interface AddReviewerCaller {
     void call(String changeId, String reviewer) throws RestApiException;
   }
+
+  private static class TestWorkInProgressStateChangedListener
+      implements WorkInProgressStateChangedListener {
+    boolean invoked;
+    Boolean wip;
+
+    @Override
+    public void onWorkInProgressStateChanged(Event event) {
+      this.invoked = true;
+      this.wip =
+          event.getChange().workInProgress != null ? event.getChange().workInProgress : false;
+    }
+  }
 }