diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 3f0c751437..415465e5d8 100644 --- a/Documentation/rest-api-changes.txt +++ b/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]] diff --git a/java/com/google/gerrit/extensions/api/changes/RebaseInput.java b/java/com/google/gerrit/extensions/api/changes/RebaseInput.java index 5f4a014e59..10559a3667 100644 --- a/java/com/google/gerrit/extensions/api/changes/RebaseInput.java +++ b/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. + * + *

If there are conflicts the file contents of the rebased change contain git conflict markers + * to indicate the conflicts. + */ + public boolean allowConflicts; } diff --git a/java/com/google/gerrit/extensions/api/changes/RevisionApi.java b/java/com/google/gerrit/extensions/api/changes/RevisionApi.java index b419c2fc3f..73e6a4efc1 100644 --- a/java/com/google/gerrit/extensions/api/changes/RevisionApi.java +++ b/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(); diff --git a/java/com/google/gerrit/extensions/common/ChangeInfo.java b/java/com/google/gerrit/extensions/common/ChangeInfo.java index 190a97eb64..7ed2f954e1 100644 --- a/java/com/google/gerrit/extensions/common/ChangeInfo.java +++ b/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; diff --git a/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java b/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java index 04d2e8aee2..36d48033b8 100644 --- a/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java +++ b/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); } diff --git a/java/com/google/gerrit/server/change/RebaseChangeOp.java b/java/com/google/gerrit/server/change/RebaseChangeOp.java index 231359b2e9..acb4e3fdc7 100644 --- a/java/com/google/gerrit/server/change/RebaseChangeOp.java +++ b/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. + * + *

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. + * + *

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) { - throw new MergeConflictException( - "The change could not be rebased due to a conflict during merge."); + ObjectId tree; + ImmutableSet 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> 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; } } diff --git a/java/com/google/gerrit/server/restapi/change/Rebase.java b/java/com/google/gerrit/server/restapi/change/Rebase.java index 75ba4c12b1..cfdf04dae5 100644 --- a/java/com/google/gerrit/server/restapi/change/Rebase.java +++ b/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( diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java index 4015ccb645..815d5ef51b 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/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 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; + } + } }