From c5b1712ae867c2a152515ab3ecde997eb1ce3663 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 12 Jan 2016 15:28:50 -0500 Subject: [PATCH] MergeOp: Pre-open all OpenRepos The openRepo method may throw NoSuchProjectException, which should fail the merge process and abandon all open changes. Better to do this in advance rather than lazily after some side effects may have already happened. Change-Id: Iba219f83de65f19a329088ff60366e34eebe569e --- .../google/gerrit/server/change/Submit.java | 2 - .../com/google/gerrit/server/git/MergeOp.java | 79 +++++++++++-------- .../gerrit/server/git/ReceiveCommits.java | 2 - .../server/git/strategy/SubmitStrategy.java | 3 +- .../git/strategy/SubmitStrategyFactory.java | 6 +- 5 files changed, 49 insertions(+), 43 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java index c1f99beb6b..43c10bf07c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java @@ -186,8 +186,6 @@ public class Submit implements RestModifyView, ReviewDb db = dbProvider.get(); op.merge(db, change, caller, true); change = db.changes().get(change.getId()); - } catch (NoSuchChangeException e) { - throw new OrmException("Submission failed", e); } if (change == null) { 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 785cc88292..3545c9b8f5 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 @@ -326,22 +326,27 @@ public class MergeOp implements AutoCloseable { commits = new CommitStatus(); } - private OpenRepo openRepo(Project.NameKey project) + private OpenRepo getRepo(Project.NameKey project) { + OpenRepo or = openRepos.get(project); + checkState(or != null, "repo not yet opened: %s", project); + return or; + } + + private void openRepo(Project.NameKey project) throws NoSuchProjectException, IOException { - OpenRepo repo = openRepos.get(project); - if (repo == null) { - ProjectState projectState = projectCache.get(project); - if (projectState == null) { - throw new NoSuchProjectException(project); - } - try { - repo = new OpenRepo(repoManager.openRepository(project), projectState); - } catch (RepositoryNotFoundException e) { - throw new NoSuchProjectException(project); - } - openRepos.put(project, repo); + checkState(!openRepos.containsKey(project), + "repo already opened: %s", project); + ProjectState projectState = projectCache.get(project); + if (projectState == null) { + throw new NoSuchProjectException(project); + } + try { + OpenRepo or = + new OpenRepo(repoManager.openRepository(project), projectState); + openRepos.put(project, or); + } catch (RepositoryNotFoundException e) { + throw new NoSuchProjectException(project); } - return repo; } @Override @@ -474,8 +479,7 @@ public class MergeOp implements AutoCloseable { } public void merge(ReviewDb db, Change change, IdentifiedUser caller, - boolean checkSubmitRules) throws NoSuchChangeException, - OrmException, ResourceConflictException { + boolean checkSubmitRules) throws OrmException, ResourceConflictException { this.caller = caller; updateSubmissionId(change); this.db = db; @@ -523,22 +527,22 @@ public class MergeOp implements AutoCloseable { } private void integrateIntoHistory(ChangeSet cs) - throws IntegrationException, NoSuchChangeException, - ResourceConflictException { + throws IntegrationException, ResourceConflictException { logDebug("Beginning merge attempt on {}", cs); Map toSubmit = new HashMap<>(); logDebug("Perform the merges"); try { Multimap br = cs.branchesByProject(); Multimap cbb = cs.changesByBranch(); + openRepos(br.keySet()); for (Branch.NameKey branch : cbb.keySet()) { - OpenRepo or = openRepo(branch.getParentKey()); + OpenRepo or = getRepo(branch.getParentKey()); toSubmit.put(branch, validateChangeList(or, cbb.get(branch))); } failFast(cs); // Done checks that don't involve running submit strategies. for (Branch.NameKey branch : cbb.keySet()) { - OpenRepo or = openRepo(branch.getParentKey()); + OpenRepo or = getRepo(branch.getParentKey()); OpenBranch ob = or.getBranch(branch); BranchBatch submitting = toSubmit.get(branch); SubmitStrategy strategy = createStrategy(or, branch, @@ -547,7 +551,7 @@ public class MergeOp implements AutoCloseable { } checkMergeStrategyResults(cs, toSubmit.values()); for (Project.NameKey project : br.keySet()) { - openRepo(project).ins.flush(); + getRepo(project).ins.flush(); } Set done = @@ -555,7 +559,7 @@ public class MergeOp implements AutoCloseable { logDebug("Write out the new branch tips"); SubmoduleOp subOp = subOpProvider.get(); for (Project.NameKey project : br.keySet()) { - OpenRepo or = openRepo(project); + OpenRepo or = getRepo(project); for (Branch.NameKey branch : br.get(project)) { OpenBranch ob = or.getBranch(branch); boolean updated = updateBranch(or, branch); @@ -574,10 +578,6 @@ public class MergeOp implements AutoCloseable { checkState(done.equals(cbb.keySet()), "programmer error: did not process" + " all branches in input set.\nExpected: %s\nActual: %s", done, cbb.keySet()); - } catch (NoSuchProjectException noProject) { - logWarn("Project " + noProject.project() + " no longer exists, " - + "abandoning open changes"); - abandonAllOpenChanges(noProject.project()); } catch (OrmException e) { throw new IntegrationException("Cannot query the database", e); } catch (IOException e) { @@ -602,8 +602,7 @@ public class MergeOp implements AutoCloseable { private SubmitStrategy createStrategy(OpenRepo or, Branch.NameKey destBranch, SubmitType submitType, - CodeReviewCommit branchTip) - throws IntegrationException, NoSuchProjectException { + CodeReviewCommit branchTip) throws IntegrationException { return submitStrategyFactory.create(submitType, db, or.repo, or.rw, or.ins, or.canMergeFlag, getAlreadyAccepted(or, branchTip), destBranch, caller, commits); @@ -1209,19 +1208,33 @@ public class MergeOp implements AutoCloseable { }); } - private void abandonAllOpenChanges(Project.NameKey destProject) - throws NoSuchChangeException { + private void openRepos(Collection projects) + throws IntegrationException { + for (Project.NameKey project : projects) { + try { + openRepo(project); + } catch (NoSuchProjectException noProject) { + logWarn("Project " + noProject.project() + " no longer exists, " + + "abandoning open changes"); + abandonAllOpenChanges(noProject.project()); + } catch (IOException e) { + throw new IntegrationException("Error opening project " + project, e); + } + } + } + + private void abandonAllOpenChanges(Project.NameKey destProject) { try { for (ChangeData cd : internalChangeQuery.byProjectOpen(destProject)) { abandonOneChange(cd.change()); } - } catch (IOException | OrmException e) { - logWarn("Cannot abandon changes for deleted project ", e); + } catch (NoSuchChangeException | IOException | OrmException e) { + logWarn("Cannot abandon changes for deleted project " + destProject, e); } } private void abandonOneChange(Change change) throws OrmException, - NoSuchChangeException, IOException { + NoSuchChangeException, IOException { db.changes().beginTransaction(change.getId()); //TODO(dborowitz): support InternalUser in ChangeUpdate 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 8defdb01dc..ce952d5139 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 @@ -1830,8 +1830,6 @@ public class ReceiveCommits { try (MergeOp op = mergeOpProvider.get()) { op.merge(db, rsrc.getChange(), changeCtl.getUser().asIdentifiedUser(), false); - } catch (NoSuchChangeException e) { - throw new OrmException(e); } addMessage(""); Change c = db.changes().get(rsrc.getChange().getId()); 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 a8c912789e..3ec78064c4 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 @@ -139,7 +139,8 @@ public abstract class SubmitStrategy { this.db = db; this.alreadyAccepted = alreadyAccepted; - this.project = projectCache.get(destBranch.getParentKey()); + this.project = checkNotNull(projectCache.get(destBranch.getParentKey()), + "project not found: %s", destBranch.getParentKey()); this.mergeSorter = new MergeSorter(rw, alreadyAccepted, canMergeFlag); this.mergeUtil = mergeUtilFactory.create(project); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java index 65fee208d2..91396d84e3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java @@ -21,7 +21,6 @@ import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk; import com.google.gerrit.server.git.IntegrationException; import com.google.gerrit.server.git.MergeOp.CommitStatus; -import com.google.gerrit.server.project.NoSuchProjectException; import com.google.inject.Inject; import com.google.inject.Singleton; @@ -51,12 +50,9 @@ public class SubmitStrategyFactory { Repository repo, CodeReviewRevWalk rw, ObjectInserter inserter, RevFlag canMergeFlag, Set alreadyAccepted, Branch.NameKey destBranch, IdentifiedUser caller, CommitStatus commits) - throws IntegrationException, NoSuchProjectException { + throws IntegrationException { SubmitStrategy.Arguments args = argsFactory.create(destBranch, commits, rw, caller, inserter, repo, canMergeFlag, db, alreadyAccepted); - if (args.project == null) { - throw new NoSuchProjectException(destBranch.getParentKey()); - } switch (submitType) { case CHERRY_PICK: return new CherryPick(args);