From 9686cf224b2bf7abfe1eebe9b942359cb7518396 Mon Sep 17 00:00:00 2001 From: Sven Selberg Date: Mon, 17 Nov 2014 12:51:06 +0100 Subject: [PATCH] Include merge result commit in change-merged stream-event When CI gets a change merged event what the CI most likely wants to build is not the last patch set of the change but the result on the target branch. If the merge of a change was not fast forward these are two separate commits. With this change the CI can retrieve the merge result from the newRev of a change merged event. Change-Id: I1d61ab253f4028a26cfbf0427b33873c294ee29e --- Documentation/cmd-stream-events.txt | 2 + .../rest/change/AbstractSubmit.java | 49 ++++++++++++ .../gerrit/common/ChangeHookRunner.java | 7 +- .../com/google/gerrit/common/ChangeHooks.java | 3 +- .../gerrit/common/DisabledChangeHooks.java | 2 +- .../server/events/ChangeMergedEvent.java | 1 + .../com/google/gerrit/server/git/MergeOp.java | 58 ++++++++------ .../google/gerrit/server/git/MergeTip.java | 70 +++++++++++++++++ .../gerrit/server/git/ReceiveCommits.java | 4 +- .../server/git/strategy/CherryPick.java | 39 ++++++---- .../server/git/strategy/FastForwardOnly.java | 20 +++-- .../server/git/strategy/MergeAlways.java | 28 ++++--- .../server/git/strategy/MergeIfNecessary.java | 36 +++++---- .../git/strategy/RebaseIfNecessary.java | 77 +++++++++++-------- .../server/git/strategy/SubmitStrategy.java | 11 +-- 15 files changed, 289 insertions(+), 118 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/git/MergeTip.java diff --git a/Documentation/cmd-stream-events.txt b/Documentation/cmd-stream-events.txt index ba3d6092f8..c754f35019 100644 --- a/Documentation/cmd-stream-events.txt +++ b/Documentation/cmd-stream-events.txt @@ -72,6 +72,8 @@ patchSet:: link:json.html#patchSet[patchSet attribute] submitter:: link:json.html#account[account attribute] +newRev:: The resulting revision of the merge. + eventCreatedOn:: Time in seconds since the UNIX epoch when this event was created. diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java index 07e5c39713..16a25cd7a3 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java @@ -22,11 +22,14 @@ import static com.google.gerrit.extensions.common.ListChangesOption.CURRENT_REVI import static com.google.gerrit.extensions.common.ListChangesOption.DETAILED_LABELS; import com.google.common.collect.Lists; +import com.google.common.collect.Maps; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.GitUtil; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.RestResponse; import com.google.gerrit.acceptance.SshSession; +import com.google.gerrit.common.ChangeHooks; +import com.google.gerrit.common.ChangeListener; import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.api.changes.SubmitInput; import com.google.gerrit.extensions.common.ChangeInfo; @@ -41,7 +44,12 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.ApprovalsUtil; +import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.events.ChangeEvent; +import com.google.gerrit.server.events.ChangeMergedEvent; import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.project.ListBranches.BranchInfo; import com.google.gerrit.server.project.PutConfig; import com.google.gson.reflect.TypeToken; import com.google.gwtorm.server.OrmException; @@ -66,16 +74,39 @@ import java.io.IOException; import java.sql.Timestamp; import java.util.Collections; import java.util.List; +import java.util.Map; public abstract class AbstractSubmit extends AbstractDaemonTest { + + private Map mergeResults; + @Inject private ChangeNotes.Factory notesFactory; @Inject private ApprovalsUtil approvalsUtil; + @Inject + private IdentifiedUser.GenericFactory factory; + + @Inject + ChangeHooks hooks; + @Before public void setUp() throws Exception { + mergeResults = Maps.newHashMap(); + CurrentUser listenerUser = factory.create(user.id); + hooks.addChangeListener(new ChangeListener() { + + @Override + public void onChangeEvent(ChangeEvent event) { + if (event instanceof ChangeMergedEvent) { + ChangeMergedEvent cMEvent = (ChangeMergedEvent) event; + mergeResults.put(cMEvent.change.number, cMEvent.newRev); + } + } + + }, listenerUser); project = new Project.NameKey("p2"); } @@ -181,10 +212,28 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { newGson().fromJson(r.getReader(), new TypeToken() {}.getType()); assertThat(change.status).isEqualTo(ChangeStatus.MERGED); + + checkMergeResult(change); } r.consume(); } + private void checkMergeResult(ChangeInfo change) throws IOException { + // Get the revision of the branch after the submit to compare with the + // newRev of the ChangeMergedEvent. + RestResponse b = + adminSession.get("/projects/" + project.get() + "/branches/" + + change.branch); + if (b.getStatusCode() == HttpStatus.SC_OK) { + BranchInfo branch = + newGson().fromJson(b.getReader(), + new TypeToken() {}.getType()); + assertThat(branch.revision).isEqualTo( + mergeResults.get(Integer.toString(change._number))); + } + b.consume(); + } + private void approve(String changeId) throws IOException { RestResponse r = adminSession.post( "/changes/" + changeId + "/revisions/current/review", diff --git a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java index 179497e782..61bec009e6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java +++ b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java @@ -446,14 +446,16 @@ public class ChangeHookRunner implements ChangeHooks, LifecycleListener { } @Override - public void doChangeMergedHook(final Change change, final Account account, - final PatchSet patchSet, final ReviewDb db) throws OrmException { + public void doChangeMergedHook(final Change change, final Account account, + final PatchSet patchSet, final ReviewDb db, String mergeResultRev) + throws OrmException { final ChangeMergedEvent event = new ChangeMergedEvent(); final AccountState owner = accountCache.get(change.getOwner()); event.change = eventFactory.asChangeAttribute(change); event.submitter = eventFactory.asAccountAttribute(account); event.patchSet = eventFactory.asPatchSetAttribute(patchSet); + event.newRev = mergeResultRev; fireEvent(change, event, db); final List args = new ArrayList<>(); @@ -465,6 +467,7 @@ public class ChangeHookRunner implements ChangeHooks, LifecycleListener { addArg(args, "--topic", event.change.topic); addArg(args, "--submitter", getDisplayName(account)); addArg(args, "--commit", event.patchSet.revision); + addArg(args, "--newrev", mergeResultRev); runHook(change.getProject(), changeMergedHook, args); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHooks.java b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHooks.java index 550b5f0117..c4a8e00150 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHooks.java +++ b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHooks.java @@ -79,10 +79,11 @@ public interface ChangeHooks { * @param change The change itself. * @param account The gerrit user who submitted the change. * @param patchSet The patchset that was merged. + * @param mergeResultRev The SHA-1 of the merge result revision. * @throws OrmException */ public void doChangeMergedHook(Change change, Account account, - PatchSet patchSet, ReviewDb db) throws OrmException; + PatchSet patchSet, ReviewDb db, String mergeResultRev) throws OrmException; /** * Fire the Merge Failed Hook. diff --git a/gerrit-server/src/main/java/com/google/gerrit/common/DisabledChangeHooks.java b/gerrit-server/src/main/java/com/google/gerrit/common/DisabledChangeHooks.java index 27b54bc1a3..e719109e21 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/common/DisabledChangeHooks.java +++ b/gerrit-server/src/main/java/com/google/gerrit/common/DisabledChangeHooks.java @@ -44,7 +44,7 @@ public final class DisabledChangeHooks implements ChangeHooks { @Override public void doChangeMergedHook(Change change, Account account, - PatchSet patchSet, ReviewDb db) { + PatchSet patchSet, ReviewDb db, String mergeResultRev) { } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeMergedEvent.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeMergedEvent.java index f1aaa0a880..0bf7330a49 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeMergedEvent.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/ChangeMergedEvent.java @@ -27,6 +27,7 @@ public class ChangeMergedEvent extends ChangeEvent { public ChangeAttribute change; public PatchSetAttribute patchSet; public AccountAttribute submitter; + public String newRev; @Override public String getType() { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java index e4c6e0a2a7..106c1e3b8c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java @@ -171,7 +171,7 @@ public class MergeOp { private RevWalk rw; private RevFlag canMergeFlag; private CodeReviewCommit branchTip; - private CodeReviewCommit mergeTip; + private MergeTip mergeTip; private ObjectInserter inserter; private PersonIdent refLogIdent; @@ -277,11 +277,11 @@ public class MergeOp { branchUpdate = openBranch(); } SubmitStrategy strategy = createStrategy(submitType); - preMerge(strategy, toMerge.get(submitType)); + MergeTip mergeTip = preMerge(strategy, toMerge.get(submitType)); RefUpdate update = updateBranch(strategy, branchUpdate); reopen = true; - updateChangeStatus(toSubmit.get(submitType)); + updateChangeStatus(toSubmit.get(submitType), mergeTip); updateSubscriptions(toSubmit.get(submitType)); if (update != null) { fireRefUpdated(update); @@ -315,7 +315,7 @@ public class MergeOp { logDebug("Adding {} changes to merge on next run", toMerge.size()); } - updateChangeStatus(toUpdate); + updateChangeStatus(toUpdate, mergeTip); for (CodeReviewCommit commit : potentiallyStillSubmittableOnNextRun) { Capable capable = isSubmitStillPossible(commit); @@ -403,7 +403,7 @@ public class MergeOp { return true; } - private void preMerge(SubmitStrategy strategy, + private MergeTip preMerge(SubmitStrategy strategy, List toMerge) throws MergeException { logDebug("Running submit strategy {} for {} commits", strategy.getClass().getSimpleName(), toMerge.size()); @@ -411,6 +411,7 @@ public class MergeOp { refLogIdent = strategy.getRefLogIdent(); logDebug("Produced {} new commits", strategy.getNewCommits().size()); commits.putAll(strategy.getNewCommits()); + return mergeTip; } private SubmitStrategy createStrategy(SubmitType submitType) @@ -604,7 +605,7 @@ public class MergeOp { idstr, ps.getId()); commit.setStatusCode(CommitMergeStatus.ALREADY_MERGED); try { - setMerged(chg, null); + setMerged(chg, null, commit.getName()); } catch (OrmException e) { logError("Cannot mark change " + chg.getId() + " merged", e); } @@ -650,11 +651,13 @@ public class MergeOp { private RefUpdate updateBranch(SubmitStrategy strategy, RefUpdate branchUpdate) throws MergeException { - if (branchTip == mergeTip) { + CodeReviewCommit currentTip = + mergeTip != null ? mergeTip.getCurrentTip() : null; + if (branchTip == currentTip) { logDebug("Branch already at merge tip {}, no update to perform", - mergeTip.name()); + currentTip.name()); return null; - } else if (mergeTip == null) { + } else if (currentTip == null) { logDebug("No merge tip, no update to perform"); return null; } @@ -664,17 +667,17 @@ public class MergeOp { try { ProjectConfig cfg = new ProjectConfig(destProject.getProject().getNameKey()); - cfg.load(repo, mergeTip); + cfg.load(repo, currentTip); } catch (Exception e) { throw new MergeException("Submit would store invalid" - + " project configuration " + mergeTip.name() + " for " + + " project configuration " + currentTip.name() + " for " + destProject.getProject().getName(), e); } } branchUpdate.setRefLogIdent(refLogIdent); branchUpdate.setForceUpdate(false); - branchUpdate.setNewObjectId(mergeTip); + branchUpdate.setNewObjectId(currentTip); branchUpdate.setRefLogMessage("merged", true); try { RefUpdate.Result result = branchUpdate.update(rw); @@ -688,7 +691,7 @@ public class MergeOp { tagCache.updateFastForward(destBranch.getParentKey(), branchUpdate.getName(), branchUpdate.getOldObjectId(), - mergeTip); + currentTip); } if (RefNames.REFS_CONFIG.equals(branchUpdate.getName())) { @@ -722,7 +725,8 @@ public class MergeOp { private void fireRefUpdated(RefUpdate branchUpdate) { logDebug("Firing ref updated hooks for {}", branchUpdate.getName()); gitRefUpdated.fire(destBranch.getParentKey(), branchUpdate); - hooks.doRefUpdatedHook(destBranch, branchUpdate, getAccount(mergeTip)); + hooks.doRefUpdatedHook(destBranch, branchUpdate, + getAccount(mergeTip.getCurrentTip())); } private Account getAccount(CodeReviewCommit codeReviewCommit) { @@ -743,7 +747,7 @@ public class MergeOp { return ""; } - private void updateChangeStatus(List submitted) + private void updateChangeStatus(List submitted, MergeTip mergeTip) throws NoSuchChangeException { logDebug("Updating change status for {} changes", submitted.size()); for (Change c : submitted) { @@ -761,21 +765,25 @@ public class MergeOp { String txt = s.getMessage(); logDebug("Status of change {} ({}) on {}: {}", c.getId(), commit.name(), c.getDest(), s); - + String commitName = commit.getName(); + // If mergeTip is null merge failed and mergeResultRev will not be read. + String mergeResultRev = + mergeTip != null ? mergeTip.getMergeResults().get(commitName) : null; try { switch (s) { case CLEAN_MERGE: - setMerged(c, message(c, txt + getByAccountName(commit))); + setMerged(c, message(c, txt + getByAccountName(commit)), + mergeResultRev); break; case CLEAN_REBASE: case CLEAN_PICK: setMerged(c, message(c, txt + " as " + commit.name() - + getByAccountName(commit))); + + getByAccountName(commit)), mergeResultRev); break; case ALREADY_MERGED: - setMerged(c, null); + setMerged(c, null, mergeResultRev); break; case PATH_CONFLICT: @@ -810,13 +818,14 @@ public class MergeOp { } private void updateSubscriptions(List submitted) { - if (mergeTip != null && (branchTip == null || branchTip != mergeTip)) { + if (mergeTip != null + && (branchTip == null || branchTip != mergeTip.getCurrentTip())) { logDebug("Updating submodule subscriptions for {} changes", submitted.size()); SubmoduleOp subOp = - subOpFactory.create(destBranch, mergeTip, rw, repo, + subOpFactory.create(destBranch, mergeTip.getCurrentTip(), rw, repo, destProject.getProject(), submitted, commits, - getAccount(mergeTip)); + getAccount(mergeTip.getCurrentTip())); try { subOp.update(); } catch (SubmoduleException e) { @@ -928,7 +937,7 @@ public class MergeOp { return m; } - private void setMerged(Change c, ChangeMessage msg) + private void setMerged(Change c, ChangeMessage msg, String mergeResultRev) throws OrmException, IOException { logDebug("Setting change {} merged", c.getId()); ChangeUpdate update = null; @@ -954,6 +963,7 @@ public class MergeOp { cmUtil.addChangeMessage(db, update, msg); } db.commit(); + } finally { db.rollback(); } @@ -964,7 +974,7 @@ public class MergeOp { try { hooks.doChangeMergedHook(c, accountCache.get(submitter.getAccountId()).getAccount(), - merged, db); + merged, db, mergeResultRev); } catch (OrmException ex) { logError("Cannot run hook for submitted patch set " + c.getId(), ex); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeTip.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeTip.java new file mode 100644 index 0000000000..1d8a1d5cde --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeTip.java @@ -0,0 +1,70 @@ +// 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; + +import com.google.common.collect.Maps; + +import java.util.Collection; +import java.util.Map; + +/** + * Class describing a merge tip during merge operation. + */ +public class MergeTip { + private CodeReviewCommit branchTip; + private Map mergeResults; + + /** + * @param initial Tip before the merge operation. + * @param toMerge List of CodeReview commits to be merged in merge operation. + */ + public MergeTip(CodeReviewCommit initial, Collection toMerge) { + this.mergeResults = Maps.newHashMap(); + this.branchTip = initial; + // Assume fast-forward merge until opposite is proven. + for (CodeReviewCommit commit : toMerge) { + mergeResults.put(commit.getName(), commit.getName()); + } + } + + /** + * Moves this MergeTip to newTip and appends mergeResult. + * + * @param newTip The new tip + * @param mergedFrom The result of the merge of newTip + */ + public void moveTipTo(CodeReviewCommit newTip, String mergedFrom) { + branchTip = newTip; + mergeResults.put(mergedFrom, newTip.getName()); + } + + /** + * The merge results of all the merges of this merge operation. + * + * @return The merge results of the merge operation Map<, > + */ + public Map getMergeResults() { + return this.mergeResults; + } + + /** + * + * @return The current tip of the current merge operation. + */ + public CodeReviewCommit getCurrentTip() { + return branchTip; + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index 02f8643231..4a38242ae6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -2147,7 +2147,7 @@ public class ReceiveCommits { hooks.doPatchsetCreatedHook(change, newPatchSet, db); if (mergedIntoRef != null) { hooks.doChangeMergedHook( - change, currentUser.getAccount(), newPatchSet, db); + change, currentUser.getAccount(), newPatchSet, db, newCommit.getName()); } if (magicBranch != null && magicBranch.submit) { @@ -2434,7 +2434,7 @@ public class ReceiveCommits { result.mergedIntoRef = refName; markChangeMergedByPush(db, result, result.changeCtl); hooks.doChangeMergedHook( - change, currentUser.getAccount(), result.newPatchSet, db); + change, currentUser.getAccount(), result.newPatchSet, db, commit.getName()); sendMergedEmail(result); return change.getKey(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java index 35e0689428..c9fe1d5962 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java @@ -30,6 +30,7 @@ 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.git.MergeTip; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gwtorm.server.OrmException; @@ -63,18 +64,20 @@ public class CherryPick extends SubmitStrategy { } @Override - protected CodeReviewCommit _run(CodeReviewCommit mergeTip, + protected MergeTip _run(CodeReviewCommit branchTip, Collection toMerge) throws MergeException { - List sorted = CodeReviewCommit.ORDER.sortedCopy(toMerge); - while (!sorted.isEmpty()) { - CodeReviewCommit n = sorted.remove(0); - + MergeTip mergeTip = + branchTip != null ? new MergeTip(branchTip, toMerge) + : null; + List sorted = CodeReviewCommit.ORDER.sortedCopy(toMerge); + while (!sorted.isEmpty()) { + CodeReviewCommit n = sorted.remove(0); try { if (mergeTip == null) { // The branch is unborn. Take a fast-forward resolution to // create the branch. // - mergeTip = n; + mergeTip = new MergeTip(n, Lists.newArrayList(n)); n.setStatusCode(CommitMergeStatus.CLEAN_MERGE); } else if (n.getParentCount() == 0) { @@ -89,8 +92,10 @@ public class CherryPick extends SubmitStrategy { // that on the current merge tip. // try { - mergeTip = writeCherryPickCommit(mergeTip, n); - newCommits.put(mergeTip.getPatchsetId().getParentKey(), mergeTip); + CodeReviewCommit merge = writeCherryPickCommit(mergeTip.getCurrentTip(), n); + mergeTip.moveTipTo(merge, merge.getName()); + newCommits.put(mergeTip.getCurrentTip().getPatchsetId() + .getParentKey(), mergeTip.getCurrentTip()); } catch (MergeConflictException mce) { n.setStatusCode(CommitMergeStatus.PATH_CONFLICT); mergeTip = null; @@ -106,15 +111,17 @@ public class CherryPick extends SubmitStrategy { // instead behave as though MERGE_IF_NECESSARY was configured. // if (!args.mergeUtil.hasMissingDependencies(args.mergeSorter, n)) { - if (args.rw.isMergedInto(mergeTip, n)) { - mergeTip = n; + if (args.rw.isMergedInto(mergeTip.getCurrentTip(), n)) { + mergeTip.moveTipTo(n, n.getName()); } else { - mergeTip = args.mergeUtil.mergeOneCommit(args.serverIdent.get(), - args.repo, args.rw, args.inserter, args.canMergeFlag, - args.destBranch, mergeTip, n); - } - PatchSetApproval submitApproval = args.mergeUtil.markCleanMerges( - args.rw, args.canMergeFlag, mergeTip, args.alreadyAccepted); + mergeTip.moveTipTo( + args.mergeUtil.mergeOneCommit(args.serverIdent.get(), args.repo, + args.rw, args.inserter, args.canMergeFlag, + args.destBranch, mergeTip.getCurrentTip(), n), n.getName()); + } + final PatchSetApproval submitApproval = + args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag, + mergeTip.getCurrentTip(), args.alreadyAccepted); setRefLogIdent(submitApproval); } else { // One or more dependencies were not met. The status was diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/FastForwardOnly.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/FastForwardOnly.java index 9e37e1103d..cdf3a9cb53 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/FastForwardOnly.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/FastForwardOnly.java @@ -18,6 +18,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.git.CommitMergeStatus; import com.google.gerrit.server.git.MergeException; +import com.google.gerrit.server.git.MergeTip; import java.util.Collection; import java.util.List; @@ -28,23 +29,26 @@ public class FastForwardOnly extends SubmitStrategy { } @Override - protected CodeReviewCommit _run(CodeReviewCommit mergeTip, - Collection toMerge) throws MergeException { + protected MergeTip _run(final CodeReviewCommit branchTip, + final Collection toMerge) throws MergeException { + MergeTip mergeTip = new MergeTip(branchTip, toMerge); List sorted = args.mergeUtil.reduceToMinimalMerge( args.mergeSorter, toMerge); - CodeReviewCommit newMergeTip = - args.mergeUtil.getFirstFastForward(mergeTip, args.rw, sorted); + final CodeReviewCommit newMergeTipCommit = + args.mergeUtil.getFirstFastForward(branchTip, args.rw, sorted); + mergeTip.moveTipTo(newMergeTipCommit, newMergeTipCommit.getName()); while (!sorted.isEmpty()) { - CodeReviewCommit n = sorted.remove(0); + final CodeReviewCommit n = sorted.remove(0); n.setStatusCode(CommitMergeStatus.NOT_FAST_FORWARD); } - PatchSetApproval submitApproval = args.mergeUtil.markCleanMerges(args.rw, - args.canMergeFlag, newMergeTip, args.alreadyAccepted); + PatchSetApproval submitApproval = + args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag, newMergeTipCommit, + args.alreadyAccepted); setRefLogIdent(submitApproval); - return newMergeTip; + return mergeTip; } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeAlways.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeAlways.java index 5bf69e936f..cdf6799429 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeAlways.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeAlways.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.git.strategy; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.git.MergeException; +import com.google.gerrit.server.git.MergeTip; import java.util.Collection; import java.util.List; @@ -27,24 +28,29 @@ public class MergeAlways extends SubmitStrategy { } @Override - protected CodeReviewCommit _run(CodeReviewCommit mergeTip, + protected MergeTip _run(CodeReviewCommit branchTip, Collection toMerge) throws MergeException { - List sorted = args.mergeUtil.reduceToMinimalMerge( - args.mergeSorter, toMerge); - - if (mergeTip == null) { + List sorted = args.mergeUtil.reduceToMinimalMerge(args.mergeSorter, toMerge); + MergeTip mergeTip; + if (branchTip == null) { // The branch is unborn. Take a fast-forward resolution to // create the branch. - mergeTip = sorted.remove(0); + mergeTip = new MergeTip(sorted.get(0), toMerge); + sorted.remove(0); + } else { + mergeTip = new MergeTip(branchTip, toMerge); } while (!sorted.isEmpty()) { - mergeTip = args.mergeUtil.mergeOneCommit(args.serverIdent.get(), - args.repo, args.rw, args.inserter, args.canMergeFlag, args.destBranch, - mergeTip, sorted.remove(0)); + CodeReviewCommit mergedFrom = sorted.remove(0); + CodeReviewCommit newTip = + args.mergeUtil.mergeOneCommit(args.serverIdent.get(), args.repo, args.rw, + args.inserter, args.canMergeFlag, args.destBranch, mergeTip.getCurrentTip(), + mergedFrom); + mergeTip.moveTipTo(newTip, mergedFrom.getName()); } - PatchSetApproval submitApproval = - args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag, mergeTip, + final PatchSetApproval submitApproval = + args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag, mergeTip.getCurrentTip(), args.alreadyAccepted); setRefLogIdent(submitApproval); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeIfNecessary.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeIfNecessary.java index 1c172d05cc..af5d803ad0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeIfNecessary.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeIfNecessary.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.git.strategy; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.git.MergeException; +import com.google.gerrit.server.git.MergeTip; import java.util.Collection; import java.util.List; @@ -27,29 +28,36 @@ public class MergeIfNecessary extends SubmitStrategy { } @Override - protected CodeReviewCommit _run(CodeReviewCommit mergeTip, + protected MergeTip _run(CodeReviewCommit branchTip, Collection toMerge) throws MergeException { - List sorted = args.mergeUtil.reduceToMinimalMerge( - args.mergeSorter, toMerge); - - if (mergeTip == null) { + List sorted = + args.mergeUtil.reduceToMinimalMerge(args.mergeSorter, toMerge); + MergeTip mergeTip; + if (branchTip == null) { // The branch is unborn. Take a fast-forward resolution to // create the branch. - mergeTip = sorted.remove(0); + mergeTip = new MergeTip(sorted.get(0), toMerge); + branchTip = sorted.remove(0); + } else { + mergeTip = new MergeTip(branchTip, toMerge); + branchTip = + args.mergeUtil.getFirstFastForward(branchTip, args.rw, sorted); } - - mergeTip = - args.mergeUtil.getFirstFastForward(mergeTip, args.rw, sorted); + mergeTip.moveTipTo(branchTip, branchTip.getName()); // For every other commit do a pair-wise merge. while (!sorted.isEmpty()) { - mergeTip = args.mergeUtil.mergeOneCommit(args.serverIdent.get(), - args.repo, args.rw, args.inserter, args.canMergeFlag, args.destBranch, - mergeTip, sorted.remove(0)); + CodeReviewCommit mergedFrom = sorted.remove(0); + branchTip = + args.mergeUtil.mergeOneCommit(args.serverIdent.get(), args.repo, + args.rw, args.inserter, args.canMergeFlag, args.destBranch, + branchTip, mergedFrom); + mergeTip.moveTipTo(branchTip, mergedFrom.getName()); } - PatchSetApproval submitApproval = args.mergeUtil.markCleanMerges( - args.rw, args.canMergeFlag, mergeTip, args.alreadyAccepted); + final PatchSetApproval submitApproval = + args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag, branchTip, + args.alreadyAccepted); setRefLogIdent(submitApproval); return mergeTip; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java index 211cb2739d..482dae3a3e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java @@ -25,6 +25,7 @@ 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.MergeTip; import com.google.gerrit.server.git.RebaseSorter; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.project.InvalidChangeOperationException; @@ -55,20 +56,19 @@ public class RebaseIfNecessary extends SubmitStrategy { } @Override - protected CodeReviewCommit _run(CodeReviewCommit mergeTip, - Collection toMerge) throws MergeException { - CodeReviewCommit newMergeTip = mergeTip; + protected MergeTip _run(final CodeReviewCommit branchTip, + final Collection toMerge) throws MergeException { + MergeTip mergeTip = new MergeTip(branchTip, toMerge); List sorted = sort(toMerge); - while (!sorted.isEmpty()) { CodeReviewCommit n = sorted.remove(0); - if (newMergeTip == null) { + if (mergeTip.getCurrentTip() == null) { // The branch is unborn. Take a fast-forward resolution to // create the branch. // - newMergeTip = n; n.setStatusCode(CommitMergeStatus.CLEAN_MERGE); + mergeTip.moveTipTo(n, n.getName()); } else if (n.getParentCount() == 0) { // Refuse to merge a root commit into an existing branch, @@ -77,37 +77,44 @@ public class RebaseIfNecessary extends SubmitStrategy { n.setStatusCode(CommitMergeStatus.CANNOT_REBASE_ROOT); } else if (n.getParentCount() == 1) { - if (args.mergeUtil.canFastForward( - args.mergeSorter, newMergeTip, args.rw, n)) { - newMergeTip = n; + if (args.mergeUtil.canFastForward(args.mergeSorter, + mergeTip.getCurrentTip(), args.rw, n)) { n.setStatusCode(CommitMergeStatus.CLEAN_MERGE); + mergeTip.moveTipTo(n, n.getName()); } else { try { - IdentifiedUser uploader = args.identifiedUserFactory.create( - args.mergeUtil.getSubmitter(n).getAccountId()); - PatchSet newPatchSet = rebaseChange.rebase(args.repo, args.rw, - args.inserter, n.getPatchsetId(), n.change(), uploader, - newMergeTip, args.mergeUtil, args.serverIdent.get(), false, - false, ValidatePolicy.NONE); + IdentifiedUser uploader = + args.identifiedUserFactory.create(args.mergeUtil + .getSubmitter(n).getAccountId()); + PatchSet newPatchSet = + rebaseChange.rebase(args.repo, args.rw, args.inserter, + n.getPatchsetId(), n.change(), uploader, + mergeTip.getCurrentTip(), args.mergeUtil, + args.serverIdent.get(), false, false, ValidatePolicy.NONE); List approvals = Lists.newArrayList(); - for (PatchSetApproval a : args.approvalsUtil.byPatchSet( - args.db, n.getControl(), n.getPatchsetId())) { + for (PatchSetApproval a : args.approvalsUtil.byPatchSet(args.db, + n.getControl(), n.getPatchsetId())) { approvals.add(new PatchSetApproval(newPatchSet.getId(), a)); } // rebaseChange.rebase() may already have copied some approvals, // use upsert, not insert, to avoid constraint violation on database args.db.patchSetApprovals().upsert(approvals); - newMergeTip = (CodeReviewCommit) args.rw.parseCommit( - ObjectId.fromString(newPatchSet.getRevision().get())); + mergeTip.moveTipTo((CodeReviewCommit) args.rw.parseCommit(ObjectId + .fromString(newPatchSet.getRevision().get())), newPatchSet + .getRevision().get()); n.change().setCurrentPatchSet( - patchSetInfoFactory.get(newMergeTip, newPatchSet.getId())); - newMergeTip.copyFrom(n); - newMergeTip.setControl(args.changeControlFactory.controlFor(n.change(), uploader)); - newMergeTip.setPatchsetId(newPatchSet.getId()); - newMergeTip.setStatusCode(CommitMergeStatus.CLEAN_REBASE); - newCommits.put(newPatchSet.getId().getParentKey(), newMergeTip); + patchSetInfoFactory.get(mergeTip.getCurrentTip(), + newPatchSet.getId())); + mergeTip.getCurrentTip().copyFrom(n); + mergeTip.getCurrentTip().setControl( + args.changeControlFactory.controlFor(n.change(), uploader)); + mergeTip.getCurrentTip().setPatchsetId(newPatchSet.getId()); + mergeTip.getCurrentTip().setStatusCode( + CommitMergeStatus.CLEAN_REBASE); + newCommits.put(newPatchSet.getId().getParentKey(), + mergeTip.getCurrentTip()); setRefLogIdent(args.mergeUtil.getSubmitter(n)); } catch (MergeConflictException e) { n.setStatusCode(CommitMergeStatus.REBASE_MERGE_CONFLICT); @@ -125,25 +132,27 @@ public class RebaseIfNecessary extends SubmitStrategy { // instead behave as though MERGE_IF_NECESSARY was configured. // try { - if (args.rw.isMergedInto(newMergeTip, n)) { - newMergeTip = n; + if (args.rw.isMergedInto(mergeTip.getCurrentTip(), n)) { + mergeTip.moveTipTo(n, n.getName()); } else { - newMergeTip = args.mergeUtil.mergeOneCommit( - args.serverIdent.get(), args.repo, args.rw, args.inserter, - args.canMergeFlag, args.destBranch, newMergeTip, n); + mergeTip.moveTipTo( + args.mergeUtil.mergeOneCommit(args.serverIdent.get(), + args.repo, args.rw, args.inserter, args.canMergeFlag, + args.destBranch, mergeTip.getCurrentTip(), n), n.getName()); } - PatchSetApproval submitApproval = args.mergeUtil.markCleanMerges( - args.rw, args.canMergeFlag, newMergeTip, args.alreadyAccepted); + PatchSetApproval submitApproval = + args.mergeUtil.markCleanMerges(args.rw, args.canMergeFlag, + mergeTip.getCurrentTip(), args.alreadyAccepted); setRefLogIdent(submitApproval); } catch (IOException e) { throw new MergeException("Cannot merge " + n.name(), e); } } - args.alreadyAccepted.add(newMergeTip); + args.alreadyAccepted.add(mergeTip.getCurrentTip()); } - return newMergeTip; + return mergeTip; } private List sort(Collection toSort) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java index e38e749920..cba32c91e9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java @@ -24,6 +24,7 @@ import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.git.CodeReviewCommit; import com.google.gerrit.server.git.MergeException; import com.google.gerrit.server.git.MergeSorter; +import com.google.gerrit.server.git.MergeTip; import com.google.gerrit.server.git.MergeUtil; import com.google.gerrit.server.index.ChangeIndexer; import com.google.gerrit.server.project.ChangeControl; @@ -104,21 +105,21 @@ public abstract class SubmitStrategy { *

* If possible, the provided commits will be merged with this submit strategy. * - * @param mergeTip the merge tip. + * @param currentTip the mergeTip * @param toMerge the list of submitted commits that should be merged using * this submit strategy. Implementations are responsible for ordering * of commits, and should not modify the input in place. * @return the new merge tip. * @throws MergeException */ - public CodeReviewCommit run(CodeReviewCommit mergeTip, - Collection toMerge) throws MergeException { + public final MergeTip run(final CodeReviewCommit currentTip, + final Collection toMerge) throws MergeException { refLogIdent = null; - return _run(mergeTip, toMerge); + return _run(currentTip, toMerge); } /** @see #run(CodeReviewCommit, Collection) */ - protected abstract CodeReviewCommit _run(CodeReviewCommit mergeTip, + protected abstract MergeTip _run(CodeReviewCommit currentTip, Collection toMerge) throws MergeException; /**