opendev/gerrit
Code Issues Proposed changes

Support allow_conflicts option in cherry-pick input

Browse Source 
If the allow_conflicts option in the cherry-pick input is set the
cherry-pick also succeeds if there are conflicts. If there are conflicts
the file contents of the created change contain git conflict markers to
indicate the conflicts. Callers can find out if there were conflicts by
checking the contains_git_conflicts field in the CherryPickChangeInfo
that is returned by the cherry-pick REST endpoints.

The cherry-pick REST endpoints now return CherryPickChangeInfo instead
of ChangeInfo to carry the contains_git_conflicts field back to the
caller. This is backwards compatible since CherryPickChangeInfo extends
ChangeInfo and only adds contains_git_conflicts as an additional field.

A draw-back of this solution is that we must adapt ChangeJson to be able
to populate a CherryPickChangeInfo instance which makes the ChangeJson
API a little more complicated.

Alternatively I considered the following solutions to let the caller
know if there were conflicts during the cherry-pick:

1. Extend ChangeInfo:
   Add the contains_git_conflicts field directly in ChangeInfo. This
   would work, but it's bad to have a field in ChangeInfo which is
   specific to one REST endpoint and otherwise unused. This can be very
   confusing to users, e.g. the field is set when the ChangeInfo is
   returned from the cherry-pick REST endpoint, but when the same change
   is retrieved again (e.g. via the change details REST endpoint) it's
   not set, although the change still contains the same conflicts.
2. Return a CherryPickResultInfo from the cherry-pick REST endpoint
   which contains ChangeInfo and the contains_git_conflicts field. This
   would break clients that expect a ChangeInfo to be returned. We could
   return CherryPickResultInfo only when allow_conflicts is specified
   but it's confusing that the response type depends on the options that
   are set in the input.
3. Return a CherryPickChangeInfo (as in the chosen solution) but use
   ChangeJson to populate a ChangeInfo and then copy all its fields into
   a CherryPickChangeInfo instance. For this we would need to have some
   code that copies over the many ChangeInfo fields which we have to
   adapt each time a ChangeInfo field is added, removed or changed.
   There is a risk that we forget to dapt this copy code. It would be
   possible to use reflection to do the copying but some fields require
   deep defensive copies which makes an reflection-based approach more
   difficult.

For the extension API I had to add a new cherryPick method that returns
CherryPickChangeInfo since the existing cherryPick method returns
ChangeApi and hence cannot be extended to return additional fields.

Change-Id: Iae9eef38ad2a7810a736823c7bd80b8a7a2a214f
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2018-10-05 13:39:34 +02:00
parent 1a0af67341
commit 5ac370dbac
16 changed files with 450 additions and 157 deletions
Download Patch File Download Diff File Expand all files Collapse all files

33
Documentation/rest-api-changes.txt
View File

@@ -5318,8 +5318,8 @@ does not specify a Change-Id, a new one is picked for the destination change.
}
----
As response a link:#change-info[ChangeInfo] entity is returned that
describes the resulting cherry picked change.
As response a link:#cherry-pick-change-info[CherryPickChangeInfo]
entity is returned that describes the resulting cherry-pick change.
.Response
----
@@ -5921,6 +5921,23 @@ invocations of the REST call are required.
Which patchset (if any) generated this message.
|==================================
[[cherry-pick-change-info]]
=== CherryPickChangeInfo
The `CherryPickChangeInfo` entity contains information about a
cherry-pick change.
`CherryPickChangeInfo` has the same fields as link:#change-info[
ChangeInfo]. In addition `CherryPickChangeInfo` has the following
fields:
[options="header",cols="1,^1,5"]
|======================================
|Field Name ||Description
|`contains_git_conflicts` |optional, not set if `false`|
Whether any file in the change contains Git conflict markers.
|======================================
[[cherrypick-input]]
=== CherryPickInput
The `CherryPickInput` entity contains information for cherry-picking a change to a new branch.
@@ -5928,7 +5945,7 @@ The `CherryPickInput` entity contains information for cherry-picking a change to
[options="header",cols="1,^1,5"]
|===========================
|Field Name ||Description
|`message` ||Commit message for the cherry-picked change
|`message` ||Commit message for the cherry-pick change
|`destination` ||Destination branch
|`base` |optional|
40-hex digit SHA-1 of the commit which will be the parent commit of the newly created change.
@@ -5944,7 +5961,15 @@ If not set, the default is `NONE`.
Additional information about whom to notify about the update as a map
of recipient type to link:#notify-info[NotifyInfo] entity.
|`keep_reviewers` |optional, defaults to false|
If true, carries reviewers and ccs over from original change to newly created one.
If `true`, carries reviewers and ccs over from original change to newly created one.
|`allow_conflicts` |optional, defaults to false|
If `true`, the cherry-pick uses content merge and succeeds also if
there are conflicts. If there are conflicts the file contents of the
created change contain git conflict markers to indicate the conflicts.
Callers can find out if there were conflicts by checking the
`contains_git_conflicts` field in the link:#cherry-pick-change-info[
CherryPickChangeInfo] that is returned by the cherry-pick REST
endpoints.
|===========================
[[comment-info]]

5
Documentation/rest-api-projects.txt
View File

@@ -2596,8 +2596,9 @@ commit will be used.
}
----
As response a link:rest-api-changes.html#change-info[ChangeInfo] entity is returned that
describes the resulting cherry-picked change.
As response a link:rest-api-changes.html#cherry-pick-change-info[
CherryPickChangeInfo] entity is returned that describes the resulting
cherry-picked change.
.Response
----

1
java/com/google/gerrit/extensions/api/changes/CherryPickInput.java
View File

@@ -28,4 +28,5 @@ public class CherryPickInput {
public Map<RecipientType, NotifyInfo> notifyDetails;
public boolean keepReviewers;
public boolean allowConflicts;
}

8
java/com/google/gerrit/extensions/api/changes/RevisionApi.java
View File

@@ -16,6 +16,7 @@ package com.google.gerrit.extensions.api.changes;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ActionInfo;
import com.google.gerrit.extensions.common.CherryPickChangeInfo;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.extensions.common.CommitInfo;
import com.google.gerrit.extensions.common.EditInfo;
@@ -53,6 +54,8 @@ public interface RevisionApi {
ChangeApi cherryPick(CherryPickInput in) throws RestApiException;
CherryPickChangeInfo cherryPickAsInfo(CherryPickInput in) throws RestApiException;
ChangeApi rebase() throws RestApiException;
ChangeApi rebase(RebaseInput in) throws RestApiException;
@@ -189,6 +192,11 @@ public interface RevisionApi {
throw new NotImplementedException();
}
@Override
public CherryPickChangeInfo cherryPickAsInfo(CherryPickInput in) throws RestApiException {
throw new NotImplementedException();
}
@Override
public ChangeApi rebase() throws RestApiException {
throw new NotImplementedException();

19
java/com/google/gerrit/extensions/common/CherryPickChangeInfo.java Normal file
View File

@@ -0,0 +1,19 @@
// Copyright (C) 2018 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.extensions.common;
public class CherryPickChangeInfo extends ChangeInfo {
public Boolean containsGitConflicts;
}

10
java/com/google/gerrit/server/api/changes/RevisionApiImpl.java
View File

@@ -34,6 +34,7 @@ import com.google.gerrit.extensions.api.changes.RobotCommentApi;
import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ActionInfo;
import com.google.gerrit.extensions.common.CherryPickChangeInfo;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.extensions.common.CommitInfo;
import com.google.gerrit.extensions.common.DescriptionInput;
@@ -289,6 +290,15 @@ class RevisionApiImpl implements RevisionApi {
}
}
@Override
public CherryPickChangeInfo cherryPickAsInfo(CherryPickInput in) throws RestApiException {
try {
return cherryPick.apply(revision, in);
} catch (Exception e) {
throw asRestApiException("Cannot cherry pick", e);
}
}
@Override
public RevisionReviewerApi reviewer(String id) throws RestApiException {
try {

50
java/com/google/gerrit/server/change/ChangeJson.java
View File

@@ -161,6 +161,7 @@ import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Future;
import java.util.function.Supplier;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
@@ -371,6 +372,11 @@ public class ChangeJson {
}
public ChangeInfo format(Project.NameKey project, Change.Id id) throws OrmException {
return format(project, id, ChangeInfo::new);
}
public <I extends ChangeInfo> I format(
Project.NameKey project, Change.Id id, Supplier<I> changeInfoSupplier) throws OrmException {
ChangeNotes notes;
try {
notes = notesFactory.createChecked(db.get(), project, id);
@@ -378,26 +384,40 @@ public class ChangeJson {
if (!has(CHECK)) {
throw e;
}
return checkOnly(changeDataFactory.create(db.get(), project, id));
return checkOnly(changeDataFactory.create(db.get(), project, id), changeInfoSupplier);
}
return format(changeDataFactory.create(db.get(), notes));
return format(changeDataFactory.create(db.get(), notes), changeInfoSupplier);
}
public ChangeInfo format(ChangeData cd) throws OrmException {
return format(cd, Optional.empty(), true);
return format(cd, ChangeInfo::new);
}
public <I extends ChangeInfo> I format(ChangeData cd, Supplier<I> changeInfoSupplier)
throws OrmException {
return format(cd, Optional.empty(), true, changeInfoSupplier);
}
private ChangeInfo format(
ChangeData cd, Optional<PatchSet.Id> limitToPsId, boolean fillAccountLoader)
throws OrmException {
return format(cd, limitToPsId, fillAccountLoader, ChangeInfo::new);
}
private <I extends ChangeInfo> I format(
ChangeData cd,
Optional<PatchSet.Id> limitToPsId,
boolean fillAccountLoader,
Supplier<I> changeInfoSupplier)
throws OrmException {
try {
if (fillAccountLoader) {
accountLoader = accountLoaderFactory.create(has(DETAILED_ACCOUNTS));
ChangeInfo res = toChangeInfo(cd, limitToPsId);
I res = toChangeInfo(cd, limitToPsId, changeInfoSupplier);
accountLoader.fill();
return res;
}
return toChangeInfo(cd, limitToPsId);
return toChangeInfo(cd, limitToPsId, changeInfoSupplier);
} catch (PatchListNotAvailableException
| GpgException
| OrmException
@@ -408,7 +428,7 @@ public class ChangeJson {
Throwables.throwIfInstanceOf(e, OrmException.class);
throw new OrmException(e);
}
return checkOnly(cd);
return checkOnly(cd, changeInfoSupplier);
}
}
@@ -539,14 +559,14 @@ public class ChangeJson {
}
}
private ChangeInfo checkOnly(ChangeData cd) {
private <I extends ChangeInfo> I checkOnly(ChangeData cd, Supplier<I> changeInfoSupplier) {
ChangeNotes notes;
try {
notes = cd.notes();
} catch (OrmException e) {
String msg = "Error loading change";
logger.atWarning().withCause(e).log(msg + " %s", cd.getId());
ChangeInfo info = new ChangeInfo();
I info = changeInfoSupplier.get();
info._number = cd.getId().get();
ProblemInfo p = new ProblemInfo();
p.message = msg;
@@ -555,10 +575,9 @@ public class ChangeJson {
}
ConsistencyChecker.Result result = checkerProvider.get().check(notes, fix);
ChangeInfo info;
I info = changeInfoSupplier.get();
Change c = result.change();
if (c != null) {
info = new ChangeInfo();
info.project = c.getProject().get();
info.branch = c.getDest().getShortName();
info.topic = c.getTopic();
@@ -575,25 +594,26 @@ public class ChangeJson {
info.hasReviewStarted = c.hasReviewStarted();
finish(info);
} else {
info = new ChangeInfo();
info._number = result.id().get();
info.problems = result.problems();
}
return info;
}
private ChangeInfo toChangeInfo(ChangeData cd, Optional<PatchSet.Id> limitToPsId)
private <I extends ChangeInfo> I toChangeInfo(
ChangeData cd, Optional<PatchSet.Id> limitToPsId, Supplier<I> changeInfoSupplier)
throws PatchListNotAvailableException, GpgException, OrmException, PermissionBackendException,
IOException {
try (Timer0.Context ignored = metrics.toChangeInfoLatency.start()) {
return toChangeInfoImpl(cd, limitToPsId);
return toChangeInfoImpl(cd, limitToPsId, changeInfoSupplier);
}
}
private ChangeInfo toChangeInfoImpl(ChangeData cd, Optional<PatchSet.Id> limitToPsId)
private <I extends ChangeInfo> I toChangeInfoImpl(
ChangeData cd, Optional<PatchSet.Id> limitToPsId, Supplier<I> changeInfoSupplier)
throws PatchListNotAvailableException, GpgException, OrmException, PermissionBackendException,
IOException {
ChangeInfo out = new ChangeInfo();
I out = changeInfoSupplier.get();
CurrentUser user = userProvider.get();
if (has(CHECK)) {

11
java/com/google/gerrit/server/git/CodeReviewCommit.java
View File

@@ -126,6 +126,9 @@ public class CodeReviewCommit extends RevCommit {
*/
private Optional<String> statusMessage = Optional.empty();
/** Whether any of the files in this commit contains Git conflict markers. */
private boolean containsGitConflicts;
public CodeReviewCommit(AnyObjectId id) {
super(id);
}
@@ -150,6 +153,14 @@ public class CodeReviewCommit extends RevCommit {
this.statusMessage = Optional.ofNullable(statusMessage);
}
public boolean containsGitConflicts() {
return containsGitConflicts;
}
public void setContainsGitConflicts(boolean hasConflicts) {
this.containsGitConflicts = hasConflicts;
}
public PatchSet.Id getPatchsetId() {
return patchsetId;
}

167
java/com/google/gerrit/server/git/MergeUtil.java
View File

@@ -16,6 +16,8 @@ package com.google.gerrit.server.git;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.base.Joiner;
import com.google.common.base.Strings;
@@ -29,6 +31,7 @@ import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.MergeConflictException;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
@@ -57,15 +60,22 @@ import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.io.IOException;
import java.io.InputStream;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import org.eclipse.jgit.diff.Sequence;
import org.eclipse.jgit.dircache.DirCache;
import org.eclipse.jgit.dircache.DirCacheBuilder;
import org.eclipse.jgit.dircache.DirCacheEntry;
import org.eclipse.jgit.errors.AmbiguousObjectException;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
import org.eclipse.jgit.errors.LargeObjectException;
@@ -81,6 +91,8 @@ import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.merge.MergeFormatter;
import org.eclipse.jgit.merge.MergeResult;
import org.eclipse.jgit.merge.MergeStrategy;
import org.eclipse.jgit.merge.Merger;
import org.eclipse.jgit.merge.ResolveMerger;
@@ -92,6 +104,7 @@ import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevFlag;
import org.eclipse.jgit.revwalk.RevSort;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.util.TemporaryBuffer;
/**
* Utility methods used during the merge process.
@@ -232,29 +245,155 @@ public class MergeUtil {
String commitMsg,
CodeReviewRevWalk rw,
int parentIndex,
boolean ignoreIdenticalTree)
boolean ignoreIdenticalTree,
boolean allowConflicts)
throws MissingObjectException, IncorrectObjectTypeException, IOException,
MergeIdenticalTreeException, MergeConflictException {
final ThreeWayMerger m = newThreeWayMerger(inserter, repoConfig);
MergeIdenticalTreeException, MergeConflictException, MethodNotAllowedException {
ThreeWayMerger m = newThreeWayMerger(inserter, repoConfig);
m.setBase(originalCommit.getParent(parentIndex));
DirCache dc = DirCache.newInCore();
if (allowConflicts && m instanceof ResolveMerger) {
// The DirCache must be set on ResolveMerger before calling
// ResolveMerger#merge(AnyObjectId...) otherwise the entries in DirCache don't get populated.
((ResolveMerger) m).setDirCache(dc);
}
ObjectId tree;
boolean containsGitConflicts;
if (m.merge(mergeTip, originalCommit)) {
ObjectId tree = m.getResultTreeId();
containsGitConflicts = false;
tree = m.getResultTreeId();
if (tree.equals(mergeTip.getTree()) && !ignoreIdenticalTree) {
throw new MergeIdenticalTreeException("identical tree");
}
} else {
if (!allowConflicts) {
throw new MergeConflictException("merge conflict");
}
CommitBuilder mergeCommit = new CommitBuilder();
mergeCommit.setTreeId(tree);
mergeCommit.setParentId(mergeTip);
mergeCommit.setAuthor(originalCommit.getAuthorIdent());
mergeCommit.setCommitter(cherryPickCommitterIdent);
mergeCommit.setMessage(commitMsg);
matchAuthorToCommitterDate(project, mergeCommit);
return rw.parseCommit(inserter.insert(mergeCommit));
if (!useContentMerge) {
// If content merge is disabled we don't have a ResolveMerger and hence cannot merge with
// conflict markers.
throw new MethodNotAllowedException(
"Cherry-pick with allow conflicts requires that content merge is enabled.");
}
// For merging with conflict markers we need a ResolveMerger, double-check that we have one.
checkState(m instanceof ResolveMerger, "allow conflicts is not supported");
containsGitConflicts = true;
tree =
mergeWithConflicts(
rw,
inserter,
dc,
"HEAD",
mergeTip,
"CHANGE",
originalCommit,
((ResolveMerger) m).getMergeResults());
}
throw new MergeConflictException("merge conflict");
CommitBuilder cherryPickCommit = new CommitBuilder();
cherryPickCommit.setTreeId(tree);
cherryPickCommit.setParentId(mergeTip);
cherryPickCommit.setAuthor(originalCommit.getAuthorIdent());
cherryPickCommit.setCommitter(cherryPickCommitterIdent);
cherryPickCommit.setMessage(commitMsg);
matchAuthorToCommitterDate(project, cherryPickCommit);
CodeReviewCommit commit = rw.parseCommit(inserter.insert(cherryPickCommit));
commit.setContainsGitConflicts(containsGitConflicts);
return commit;
}
public static ObjectId mergeWithConflicts(
RevWalk rw,
ObjectInserter ins,
DirCache dc,
String oursName,
RevCommit ours,
String theirsName,
RevCommit theirs,
Map<String, MergeResult<? extends Sequence>> mergeResults)
throws IOException {
rw.parseBody(ours);
rw.parseBody(theirs);
String oursMsg = ours.getShortMessage();
String theirsMsg = theirs.getShortMessage();
int nameLength = Math.max(oursName.length(), theirsName.length());
String oursNameFormatted =
String.format(
"%0$-" + nameLength + "s (%s %s)",
oursName,
ours.abbreviate(6).name(),
oursMsg.substring(0, Math.min(oursMsg.length(), 60)));
String theirsNameFormatted =
String.format(
"%0$-" + nameLength + "s (%s %s)",
theirsName,
theirs.abbreviate(6).name(),
theirsMsg.substring(0, Math.min(theirsMsg.length(), 60)));
MergeFormatter fmt = new MergeFormatter();
Map<String, ObjectId> resolved = new HashMap<>();
for (Map.Entry<String, MergeResult<? extends Sequence>> entry : mergeResults.entrySet()) {
MergeResult<? extends Sequence> p = entry.getValue();
try (TemporaryBuffer buf = new TemporaryBuffer.LocalFile(null, 10 * 1024 * 1024)) {
fmt.formatMerge(buf, p, "BASE", oursNameFormatted, theirsNameFormatted, UTF_8.name());
buf.close();
try (InputStream in = buf.openInputStream()) {
resolved.put(entry.getKey(), ins.insert(Constants.OBJ_BLOB, buf.length(), in));
}
}
}
DirCacheBuilder builder = dc.builder();
int cnt = dc.getEntryCount();
for (int i = 0; i < cnt; ) {
DirCacheEntry entry = dc.getEntry(i);
if (entry.getStage() == 0) {
builder.add(entry);
i++;
continue;
}
int next = dc.nextEntry(i);
String path = entry.getPathString();
DirCacheEntry res = new DirCacheEntry(path);
if (resolved.containsKey(path)) {
// For a file with content merge conflict that we produced a result
// above on, collapse the file down to a single stage 0 with just
// the blob content, and a randomly selected mode (the lowest stage,
// which should be the merge base, or ours).
res.setFileMode(entry.getFileMode());
res.setObjectId(resolved.get(path));
} else if (next == i + 1) {
// If there is exactly one stage present, shouldn't be a conflict...
res.setFileMode(entry.getFileMode());
res.setObjectId(entry.getObjectId());
} else if (next == i + 2) {
// Two stages suggests a delete/modify conflict. Pick the higher
// stage as the automatic result.
entry = dc.getEntry(i + 1);
res.setFileMode(entry.getFileMode());
res.setObjectId(entry.getObjectId());
} else {
// 3 stage conflict, no resolve above
// Punt on the 3-stage conflict and show the base, for now.
res.setFileMode(entry.getFileMode());
res.setObjectId(entry.getObjectId());
}
builder.add(res);
i = next;
}
builder.finish();
return dc.writeTree(ins);
}
public static RevCommit createMergeCommit(

99
java/com/google/gerrit/server/patch/AutoMerger.java
View File

@@ -15,7 +15,6 @@
package com.google.gerrit.server.patch;
import static com.google.common.base.Preconditions.checkArgument;
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
@@ -24,18 +23,12 @@ import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.UsedAt;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.InMemoryInserter;
import com.google.gerrit.server.git.MergeUtil;
import com.google.inject.Inject;
import java.io.IOException;
import java.io.InputStream;
import java.util.HashMap;
import java.util.Map;
import org.eclipse.jgit.diff.Sequence;
import org.eclipse.jgit.dircache.DirCache;
import org.eclipse.jgit.dircache.DirCacheBuilder;
import org.eclipse.jgit.dircache.DirCacheEntry;
import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.ObjectReader;
@@ -43,14 +36,11 @@ import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.merge.MergeFormatter;
import org.eclipse.jgit.merge.MergeResult;
import org.eclipse.jgit.merge.ResolveMerger;
import org.eclipse.jgit.merge.ThreeWayMergeStrategy;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevObject;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.util.TemporaryBuffer;
public class AutoMerger {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -127,84 +117,17 @@ public class AutoMerger {
ObjectId treeId;
if (couldMerge) {
treeId = m.getResultTreeId();
} else {
RevCommit ours = merge.getParent(0);
RevCommit theirs = merge.getParent(1);
rw.parseBody(ours);
rw.parseBody(theirs);
String oursMsg = ours.getShortMessage();
String theirsMsg = theirs.getShortMessage();
String oursName =
String.format(
"HEAD (%s %s)",
ours.abbreviate(6).name(), oursMsg.substring(0, Math.min(oursMsg.length(), 60)));
String theirsName =
String.format(
"BRANCH (%s %s)",
theirs.abbreviate(6).name(),
theirsMsg.substring(0, Math.min(theirsMsg.length(), 60)));
MergeFormatter fmt = new MergeFormatter();
Map<String, MergeResult<? extends Sequence>> r = m.getMergeResults();
Map<String, ObjectId> resolved = new HashMap<>();
for (Map.Entry<String, MergeResult<? extends Sequence>> entry : r.entrySet()) {
MergeResult<? extends Sequence> p = entry.getValue();
try (TemporaryBuffer buf = new TemporaryBuffer.LocalFile(null, 10 * 1024 * 1024)) {
fmt.formatMerge(buf, p, "BASE", oursName, theirsName, UTF_8.name());
buf.close();
try (InputStream in = buf.openInputStream()) {
resolved.put(entry.getKey(), ins.insert(Constants.OBJ_BLOB, buf.length(), in));
}
}
}
DirCacheBuilder builder = dc.builder();
int cnt = dc.getEntryCount();
for (int i = 0; i < cnt; ) {
DirCacheEntry entry = dc.getEntry(i);
if (entry.getStage() == 0) {
builder.add(entry);
i++;
continue;
}
int next = dc.nextEntry(i);
String path = entry.getPathString();
DirCacheEntry res = new DirCacheEntry(path);
if (resolved.containsKey(path)) {
// For a file with content merge conflict that we produced a result
// above on, collapse the file down to a single stage 0 with just
// the blob content, and a randomly selected mode (the lowest stage,
// which should be the merge base, or ours).
res.setFileMode(entry.getFileMode());
res.setObjectId(resolved.get(path));
} else if (next == i + 1) {
// If there is exactly one stage present, shouldn't be a conflict...
res.setFileMode(entry.getFileMode());
res.setObjectId(entry.getObjectId());
} else if (next == i + 2) {
// Two stages suggests a delete/modify conflict. Pick the higher
// stage as the automatic result.
entry = dc.getEntry(i + 1);
res.setFileMode(entry.getFileMode());
res.setObjectId(entry.getObjectId());
} else {
// 3 stage conflict, no resolve above
// Punt on the 3-stage conflict and show the base, for now.
res.setFileMode(entry.getFileMode());
res.setObjectId(entry.getObjectId());
}
builder.add(res);
i = next;
}
builder.finish();
treeId = dc.writeTree(ins);
treeId =
MergeUtil.mergeWithConflicts(
rw,
ins,
dc,
"HEAD",
merge.getParent(0),
"BRANCH",
merge.getParent(1),
m.getMergeResults());
}
return commit(repo, rw, tmpIns, ins, refName, treeId, merge);

15
java/com/google/gerrit/server/restapi/change/CherryPick.java
View File

@@ -18,13 +18,12 @@ import static com.google.gerrit.extensions.conditions.BooleanCondition.and;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.extensions.api.changes.CherryPickInput;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.CherryPickChangeInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.change.RevisionResource;
@@ -50,7 +49,7 @@ import org.eclipse.jgit.errors.ConfigInvalidException;
@Singleton
public class CherryPick
extends RetryingRestModifyView<RevisionResource, CherryPickInput, ChangeInfo>
extends RetryingRestModifyView<RevisionResource, CherryPickInput, CherryPickChangeInfo>
implements UiAction<RevisionResource> {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -77,7 +76,7 @@ public class CherryPick
}
@Override
public ChangeInfo applyImpl(
public CherryPickChangeInfo applyImpl(
BatchUpdate.Factory updateFactory, RevisionResource rsrc, CherryPickInput input)
throws OrmException, IOException, UpdateException, RestApiException,
PermissionBackendException, ConfigInvalidException, NoSuchProjectException {
@@ -99,14 +98,18 @@ public class CherryPick
projectCache.checkedGet(rsrc.getProject()).checkStatePermitsWrite();
try {
Change.Id cherryPickedChangeId =
CherryPickChange.Result cherryPickResult =
cherryPickChange.cherryPick(
updateFactory,
rsrc.getChange(),
rsrc.getPatchSet(),
input,
new Branch.NameKey(rsrc.getProject(), refName));
return json.noOptions().format(rsrc.getProject(), cherryPickedChangeId);
CherryPickChangeInfo changeInfo =
json.noOptions()
.format(rsrc.getProject(), cherryPickResult.changeId(), CherryPickChangeInfo::new);
changeInfo.containsGitConflicts = cherryPickResult.containsGitConflicts() ? true : null;
return changeInfo;
} catch (InvalidChangeOperationException e) {
throw new BadRequestException(e.getMessage());
} catch (IntegrationException | NoSuchChangeException e) {

54
java/com/google/gerrit/server/restapi/change/CherryPickChange.java
View File

@@ -14,6 +14,7 @@
package com.google.gerrit.server.restapi.change;
import com.google.auto.value.AutoValue;
import com.google.common.base.Strings;
import com.google.gerrit.common.FooterConstants;
import com.google.gerrit.common.Nullable;
@@ -84,6 +85,16 @@ import org.eclipse.jgit.util.ChangeIdUtil;
@Singleton
public class CherryPickChange {
@AutoValue
abstract static class Result {
static Result create(Change.Id changeId, boolean containsGitConflicts) {
return new AutoValue_CherryPickChange_Result(changeId, containsGitConflicts);
}
abstract Change.Id changeId();
abstract boolean containsGitConflicts();
}
private final Provider<ReviewDb> dbProvider;
private final Sequences seq;
@@ -132,7 +143,7 @@ public class CherryPickChange {
this.notifyUtil = notifyUtil;
}
public Change.Id cherryPick(
public Result cherryPick(
BatchUpdate.Factory batchUpdateFactory,
Change change,
PatchSet patch,
@@ -150,7 +161,7 @@ public class CherryPickChange {
dest);
}
public Change.Id cherryPick(
public Result cherryPick(
BatchUpdate.Factory batchUpdateFactory,
@Nullable Change sourceChange,
@Nullable PatchSet.Id sourcePatchId,
@@ -205,19 +216,26 @@ public class CherryPickChange {
throw new NoSuchProjectException(dest.getParentKey());
}
try {
MergeUtil mergeUtil;
if (input.allowConflicts) {
// allowConflicts requires to use content merge
mergeUtil = mergeUtilFactory.create(projectState, true);
} else {
// use content merge only if it's configured on the project
mergeUtil = mergeUtilFactory.create(projectState);
}
cherryPickCommit =
mergeUtilFactory
.create(projectState)
.createCherryPickFromCommit(
oi,
git.getConfig(),
baseCommit,
commitToCherryPick,
committerIdent,
commitMessage,
revWalk,
input.parent - 1,
false);
mergeUtil.createCherryPickFromCommit(
oi,
git.getConfig(),
baseCommit,
commitToCherryPick,
committerIdent,
commitMessage,
revWalk,
input.parent - 1,
false,
input.allowConflicts);
Change.Key changeKey;
final List<String> idList = cherryPickCommit.getFooterLines(FooterConstants.CHANGE_ID);
@@ -241,11 +259,11 @@ public class CherryPickChange {
try (BatchUpdate bu =
batchUpdateFactory.create(dbProvider.get(), project, identifiedUser, now)) {
bu.setRepository(git, revWalk, oi);
Change.Id result;
Change.Id changeId;
if (destChanges.size() == 1) {
// The change key exists on the destination branch. The cherry pick
// will be added as a new patch set.
result = insertPatchSet(bu, git, destChanges.get(0).notes(), cherryPickCommit, input);
changeId = insertPatchSet(bu, git, destChanges.get(0).notes(), cherryPickCommit, input);
} else {
// Change key not found on destination branch. We can create a new
// change.
@@ -253,7 +271,7 @@ public class CherryPickChange {
if (sourceChange != null && !Strings.isNullOrEmpty(sourceChange.getTopic())) {
newTopic = sourceChange.getTopic() + "-" + newDest.getShortName();
}
result =
changeId =
createNewChange(
bu, cherryPickCommit, dest.get(), newTopic, sourceChange, sourceCommit, input);
@@ -265,7 +283,7 @@ public class CherryPickChange {
}
}
bu.execute();
return result;
return Result.create(changeId, cherryPickCommit.containsGitConflicts());
}
} catch (MergeIdenticalTreeException | MergeConflictException e) {
throw new IntegrationException("Cherry pick failed: " + e.getMessage());

15
java/com/google/gerrit/server/restapi/change/CherryPickCommit.java
View File

@@ -16,12 +16,11 @@ package com.google.gerrit.server.restapi.change;
import com.google.common.base.Strings;
import com.google.gerrit.extensions.api.changes.CherryPickInput;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.CherryPickChangeInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.CurrentUser;
@@ -48,7 +47,7 @@ import org.eclipse.jgit.revwalk.RevCommit;
@Singleton
public class CherryPickCommit
extends RetryingRestModifyView<CommitResource, CherryPickInput, ChangeInfo> {
extends RetryingRestModifyView<CommitResource, CherryPickInput, CherryPickChangeInfo> {
private final PermissionBackend permissionBackend;
private final Provider<CurrentUser> user;
private final CherryPickChange cherryPickChange;
@@ -72,7 +71,7 @@ public class CherryPickCommit
}
@Override
public ChangeInfo applyImpl(
public CherryPickChangeInfo applyImpl(
BatchUpdate.Factory updateFactory, CommitResource rsrc, CherryPickInput input)
throws OrmException, IOException, UpdateException, RestApiException,
PermissionBackendException, ConfigInvalidException, NoSuchProjectException {
@@ -97,7 +96,7 @@ public class CherryPickCommit
rsrc.getProjectState().checkStatePermitsWrite();
try {
Change.Id cherryPickedChangeId =
CherryPickChange.Result cherryPickResult =
cherryPickChange.cherryPick(
updateFactory,
null,
@@ -106,7 +105,11 @@ public class CherryPickCommit
commit,
input,
new Branch.NameKey(rsrc.getProjectState().getNameKey(), refName));
return json.noOptions().format(projectName, cherryPickedChangeId);
CherryPickChangeInfo changeInfo =
json.noOptions()
.format(projectName, cherryPickResult.changeId(), CherryPickChangeInfo::new);
changeInfo.containsGitConflicts = cherryPickResult.containsGitConflicts() ? true : null;
return changeInfo;
} catch (InvalidChangeOperationException e) {
throw new BadRequestException(e.getMessage());
} catch (IntegrationException e) {

4
java/com/google/gerrit/server/submit/CherryPick.java
View File

@@ -20,6 +20,7 @@ import static com.google.gerrit.server.submit.CommitMergeStatus.SKIPPED_IDENTICA
import com.google.common.collect.ImmutableList;
import com.google.gerrit.extensions.restapi.MergeConflictException;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
@@ -90,7 +91,7 @@ public class CherryPick extends SubmitStrategy {
@Override
protected void updateRepoImpl(RepoContext ctx)
throws IntegrationException, IOException, OrmException {
throws IntegrationException, IOException, OrmException, MethodNotAllowedException {
// If there is only one parent, a cherry-pick can be done by taking the
// delta relative to that one parent and redoing that on the current merge
// tip.
@@ -116,6 +117,7 @@ public class CherryPick extends SubmitStrategy {
cherryPickCmtMsg,
args.rw,
0,
false,
false);
} catch (MergeConflictException mce) {
// Keep going in the case of a single merge failure; the goal is to

3
java/com/google/gerrit/server/submit/RebaseSubmitStrategy.java
View File

@@ -156,7 +156,8 @@ public class RebaseSubmitStrategy extends SubmitStrategy {
cherryPickCmtMsg,
args.rw,
0,
true);
true,
false);
} catch (MergeConflictException mce) {
// Unlike in Cherry-pick case, this should never happen.
toMerge.setStatusCode(CommitMergeStatus.REBASE_MERGE_CONFLICT);

113
javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
View File

@@ -59,6 +59,7 @@ import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.common.ApprovalInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.extensions.common.CherryPickChangeInfo;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.extensions.common.CommitInfo;
import com.google.gerrit.extensions.common.FileInfo;
@@ -317,7 +318,9 @@ public class RevisionIT extends AbstractDaemonTest {
ChangeApi orig = gApi.changes().id(project.get() + "~master~" + r.getChangeId());
assertThat(orig.get().messages).hasSize(1);
ChangeApi cherry = orig.revision(r.getCommit().name()).cherryPick(in);
CherryPickChangeInfo changeInfo = orig.revision(r.getCommit().name()).cherryPickAsInfo(in);
assertThat(changeInfo.containsGitConflicts).isNull();
ChangeApi cherry = gApi.changes().id(changeInfo._number);
Collection<ChangeMessageInfo> messages =
gApi.changes().id(project.get() + "~master~" + r.getChangeId()).get().messages;
@@ -368,7 +371,7 @@ public class RevisionIT extends AbstractDaemonTest {
}
@Test
public void cherryPickwithNoTopic() throws Exception {
public void cherryPickWithNoTopic() throws Exception {
PushOneCommit.Result r = pushTo("refs/for/master");
CherryPickInput in = new CherryPickInput();
in.destination = "foo";
@@ -494,6 +497,112 @@ public class RevisionIT extends AbstractDaemonTest {
orig.revision(r.getCommit().name()).cherryPick(in);
}
@Test
public void cherryPickConflictWithAllowConflicts() throws Exception {
ObjectId initial = repo().exactRef(HEAD).getLeaf().getObjectId();
// Create a branch and push a commit to it (by-passing review)
String destBranch = "foo";
gApi.projects().name(project.get()).branch(destBranch).create(new BranchInput());
String destContent = "some content";
PushOneCommit push =
pushFactory.create(
db,
admin.getIdent(),
testRepo,
PushOneCommit.SUBJECT,
PushOneCommit.FILE_NAME,
destContent);
push.to("refs/heads/" + destBranch);
// Create a change on master with a commit that conflicts with the commit on the other branch.
testRepo.reset(initial);
String changeContent = "another content";
push =
pushFactory.create(
db,
admin.getIdent(),
testRepo,
PushOneCommit.SUBJECT,
PushOneCommit.FILE_NAME,
changeContent);
PushOneCommit.Result r = push.to("refs/for/master%topic=someTopic");
// Verify before the cherry-pick that the change has exactly 1 message.
ChangeApi changeApi = gApi.changes().id(r.getChange().getId().get());
assertThat(changeApi.get().messages).hasSize(1);
// Cherry-pick the change to the other branch, that should fail with a conflict.
CherryPickInput in = new CherryPickInput();
in.destination = destBranch;
in.message = "Cherry-Pick";
try {
changeApi.revision(r.getCommit().name()).cherryPickAsInfo(in);
fail("expected ResourceConflictException");
} catch (ResourceConflictException e) {
assertThat(e.getMessage()).isEqualTo("Cherry pick failed: merge conflict");
}
// Cherry-pick with auto merge should succeed.
in.allowConflicts = true;
CherryPickChangeInfo cherryPickChange =
changeApi.revision(r.getCommit().name()).cherryPickAsInfo(in);
assertThat(cherryPickChange.containsGitConflicts).isTrue();
// Verify that subject and topic on the cherry-pick change have been correctly populated.
assertThat(cherryPickChange.subject).contains(in.message);
assertThat(cherryPickChange.topic).isEqualTo("someTopic-" + destBranch);
// Verify that the file content in the cherry-pick change is correct.
// We expect that it has conflict markers to indicate the conflict.
BinaryResult bin =
gApi.changes()
.id(cherryPickChange._number)
.current()
.file(PushOneCommit.FILE_NAME)
.content();
ByteArrayOutputStream os = new ByteArrayOutputStream();
bin.writeTo(os);
String fileContent = new String(os.toByteArray(), UTF_8);
String destSha1 = getRemoteHead(project, destBranch).abbreviate(6).name();
String changeSha1 = r.getCommit().abbreviate(6).name();
assertThat(fileContent)
.isEqualTo(
"<<<<<<< HEAD ("
+ destSha1
+ " test commit)\n"
+ destContent
+ "\n"
+ "=======\n"
+ changeContent
+ "\n"
+ ">>>>>>> CHANGE ("
+ changeSha1
+ " test commit)\n");
// Get details of cherry-pick change.
ChangeInfo cherryPickChangeWithDetails = gApi.changes().id(cherryPickChange._number).get();
// Verify that a message has been posted on the original change.
String cherryPickedRevision = cherryPickChangeWithDetails.currentRevision;
changeApi = gApi.changes().id(r.getChange().getId().get());
Collection<ChangeMessageInfo> messages = changeApi.get().messages;
assertThat(messages).hasSize(2);
Iterator<ChangeMessageInfo> origIt = messages.iterator();
origIt.next();
assertThat(origIt.next().message)
.isEqualTo(
String.format(
"Patch Set 1: Cherry Picked\n\n"
+ "This patchset was cherry picked to branch %s as commit %s",
in.destination, cherryPickedRevision));
// Verify that a message has been posted on the cherry-pick change.
assertThat(cherryPickChangeWithDetails.messages).hasSize(1);
Iterator<ChangeMessageInfo> cherryIt = cherryPickChangeWithDetails.messages.iterator();
assertThat(cherryIt.next().message).isEqualTo("Patch Set 1: Cherry Picked from branch master.");
}
@Test
public void cherryPickToExistingChange() throws Exception {
PushOneCommit.Result r1 =