opendev/gerrit
Code Issues Proposed changes

Cherry-pick: Differentiate between conflicts and already merged errors

Browse Source 
When cherry-pick operation fails with "Cherry pick failed" error, there
is no way to know the reason for the failure: merge conflict or the
commit is already on the target branch.  Differentiate between these
failures and report the proper result to the client.

Change-Id: If1139e3c13623fb98a9584e4e399cfdf45fd3613
This commit is contained in:
David Ostrovsky
2014-07-19 09:43:07 +02:00
parent 9d9e54df09
commit 6d83eba0bf
7 changed files with 128 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files

66
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.acceptance.api.revision;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
@@ -145,10 +146,67 @@ public class RevisionIT extends AbstractDaemonTest {
assertEquals(2, orig.get().messages.size());
assertTrue(cherry.get().subject.contains(in.message));
cherry.current()
.review(ReviewInput.approve());
cherry.current()
.submit();
cherry.current().review(ReviewInput.approve());
cherry.current().submit();
}
@Test
public void cherryPickIdenticalTree() throws GitAPIException,
IOException, RestApiException {
PushOneCommit.Result r = createChange();
CherryPickInput in = new CherryPickInput();
in.destination = "foo";
in.message = "it goes to stable branch";
gApi.projects()
.name(project.get())
.branch(in.destination)
.create(new BranchInput());
ChangeApi orig = gApi.changes()
.id("p~master~" + r.getChangeId());
assertEquals(1, orig.get().messages.size());
ChangeApi cherry = orig.revision(r.getCommit().name())
.cherryPick(in);
assertEquals(2, orig.get().messages.size());
assertTrue(cherry.get().subject.contains(in.message));
cherry.current().review(ReviewInput.approve());
cherry.current().submit();
try {
orig.revision(r.getCommit().name()).cherryPick(in);
fail("Cherry-pick identical tree error expected");
} catch (RestApiException e) {
assertEquals("Cherry pick failed: identical tree", e.getMessage());
}
}
@Test
public void cherryPickConflict() throws GitAPIException,
IOException, RestApiException {
PushOneCommit.Result r = createChange();
CherryPickInput in = new CherryPickInput();
in.destination = "foo";
in.message = "it goes to stable branch";
gApi.projects()
.name(project.get())
.branch(in.destination)
.create(new BranchInput());
PushOneCommit push =
pushFactory.create(db, admin.getIdent(), PushOneCommit.SUBJECT,
PushOneCommit.FILE_NAME, "another content");
push.to(git, "refs/heads/foo");
ChangeApi orig = gApi.changes().id("p~master~" + r.getChangeId());
assertEquals(1, orig.get().messages.size());
try {
orig.revision(r.getCommit().name()).cherryPick(in);
fail("Cherry-pick merge conflict error expected");
} catch (RestApiException e) {
assertEquals("Cherry pick failed: merge conflict", e.getMessage());
}
}
@Test

8
gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java
View File

@@ -27,7 +27,9 @@ import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.events.CommitReceivedEvent;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MergeConflictException;
import com.google.gerrit.server.git.MergeException;
import com.google.gerrit.server.git.MergeIdenticalTreeException;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.git.validators.CommitValidationException;
import com.google.gerrit.server.git.validators.CommitValidators;
@@ -153,14 +155,12 @@ public class CherryPickChange {
cherryPickCommit =
mergeUtilFactory.create(projectState).createCherryPickFromCommit(git, oi, mergeTip,
commitToCherryPick, committerIdent, commitMessage, revWalk);
} catch (MergeIdenticalTreeException | MergeConflictException e) {
throw new MergeException("Cherry pick failed: " + e.getMessage());
} finally {
oi.release();
}
if (cherryPickCommit == null) {
throw new MergeException("Cherry pick failed");
}
Change.Key changeKey;
final List<String> idList = cherryPickCommit.getFooterLines(CHANGE_ID);
if (!idList.isEmpty()) {

23
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeConflictException.java Normal file
View File

@@ -0,0 +1,23 @@
// Copyright (C) 2014 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.git;
/** Indicates that the commit cannot be merged without conflicts. */
public class MergeConflictException extends Exception {
private static final long serialVersionUID = 1L;
public MergeConflictException(String msg) {
super(msg, null);
}
}

23
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeIdenticalTreeException.java Normal file
View File

@@ -0,0 +1,23 @@
// Copyright (C) 2014 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.git;
/** Indicates that the commit is already contained in destination banch. */
public class MergeIdenticalTreeException extends Exception {
private static final long serialVersionUID = 1L;
public MergeIdenticalTreeException(String msg) {
super(msg, null);
}
}

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

@@ -173,7 +173,8 @@ public class MergeUtil {
public RevCommit createCherryPickFromCommit(Repository repo,
ObjectInserter inserter, RevCommit mergeTip, RevCommit originalCommit,
PersonIdent cherryPickCommitterIdent, String commitMsg, RevWalk rw)
throws MissingObjectException, IncorrectObjectTypeException, IOException {
throws MissingObjectException, IncorrectObjectTypeException, IOException,
MergeIdenticalTreeException, MergeConflictException {
final ThreeWayMerger m = newThreeWayMerger(repo, inserter);
@@ -181,7 +182,7 @@ public class MergeUtil {
if (m.merge(mergeTip, originalCommit)) {
ObjectId tree = m.getResultTreeId();
if (tree.equals(mergeTip.getTree())) {
return null;
throw new MergeIdenticalTreeException("identical tree");
}
CommitBuilder mergeCommit = new CommitBuilder();
@@ -192,7 +193,7 @@ public class MergeUtil {
mergeCommit.setMessage(commitMsg);
return rw.parseCommit(commit(inserter, mergeCommit));
} else {
return null;
throw new MergeConflictException("merge conflict");
}
}

20
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
View File

@@ -26,7 +26,9 @@ import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.CommitMergeStatus;
import com.google.gerrit.server.git.MergeConflictException;
import com.google.gerrit.server.git.MergeException;
import com.google.gerrit.server.git.MergeIdenticalTreeException;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.util.TimeUtil;
@@ -84,15 +86,16 @@ public class CherryPick extends SubmitStrategy {
// taking the delta relative to that one parent and redoing
// that on the current merge tip.
//
try {
mergeTip = writeCherryPickCommit(mergeTip, n);
if (mergeTip != null) {
newCommits.put(mergeTip.getPatchsetId().getParentKey(), mergeTip);
} else {
} catch (MergeConflictException mce) {
n.setStatusCode(CommitMergeStatus.PATH_CONFLICT);
mergeTip = null;
} catch (MergeIdenticalTreeException mie) {
n.setStatusCode(CommitMergeStatus.ALREADY_MERGED);
mergeTip = null;
}
} else {
// There are multiple parents, so this is a merge commit. We
// don't want to cherry-pick the merge as clients can't easily
@@ -131,7 +134,8 @@ public class CherryPick extends SubmitStrategy {
private CodeReviewCommit writeCherryPickCommit(CodeReviewCommit mergeTip,
CodeReviewCommit n) throws IOException, OrmException,
NoSuchChangeException {
NoSuchChangeException, MergeConflictException,
MergeIdenticalTreeException {
args.rw.parseBody(n);
@@ -156,10 +160,6 @@ public class CherryPick extends SubmitStrategy {
args.inserter, mergeTip, n, cherryPickCommitterIdent,
cherryPickCmtMsg, args.rw);
if (newCommit == null) {
return null;
}
PatchSet.Id id =
ChangeUtil.nextPatchSetId(args.repo, n.change().currentPatchSetId());
final PatchSet ps = new PatchSet(id);