From ef8e320808580a150aae231776b05cfec5484a7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Mon, 12 Jan 2015 15:09:06 -0500 Subject: [PATCH] Do not delete branches concurrently. Deleting multiple branches from the UI was resulting in a 500 error when branches are in the packed-refs. This was happening because for each branch to be deleted, a separate DELETE request was fired to the server. The concurrent deletion was failing because JGit was not able to lock the pack-ref file to remove the branch from it since the first deletion that was getting executed already locked the file. Add REST API call to delete multiple branches and modify ProjectApi.deleteBranch to use that new call. When multiple branches are deleted from the UI, one request is sent to the server to delete all the selected branches. Bug: issue 2706 Change-Id: I786533eb57048b1c9c67a386f0be37bf1a39853f --- Documentation/rest-api-projects.txt | 44 +++++ .../gerrit/client/projects/ProjectApi.java | 36 ++-- .../gerrit/server/project/DeleteBranch.java | 2 +- .../gerrit/server/project/DeleteBranches.java | 181 ++++++++++++++++++ .../google/gerrit/server/project/Module.java | 1 + 5 files changed, 251 insertions(+), 13 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranches.java diff --git a/Documentation/rest-api-projects.txt b/Documentation/rest-api-projects.txt index c6215c9277..44c029629d 100644 --- a/Documentation/rest-api-projects.txt +++ b/Documentation/rest-api-projects.txt @@ -1121,6 +1121,38 @@ Deletes a branch. HTTP/1.1 204 No Content ---- +[[delete-branches]] +=== Delete Branches +-- +'POST /projects/link:#project-name[\{project-name\}]/branches:delete' +-- + +Delete one or more branches. + +The branches to be deleted must be provided in the request body as a +link:#delete-branches-input[DeleteBranchesInput] entity. + +.Request +---- + POST /projects/MyProject/branches:delete HTTP/1.0 + Content-Type: application/json;charset=UTF-8 + + { + "branches": [ + "stable-1.0", + "stable-2.0" + ] + } +---- + +.Response +---- + HTTP/1.1 204 No Content +---- + +If some branches could not be deleted, the response is "`409 Conflict`" and the +error message is contained in the response body. + [[get-content]] === Get Content -- @@ -2060,6 +2092,18 @@ in a dashboard. Tokens such as `${project}` are not resolved. |=========================== +[[delete-branches-input]] +=== DeleteBranchesInput +The `DeleteBranchesInput` entity contains information about branches that should +be deleted. + +[options="header",width="50%",cols="1,6"] +|========================== +|Field Name |Description +|`branches` |A list of branch names that identify the branches that should be +deleted. +|========================== + [[gc-input]] === GCInput The `GCInput` entity contains information to run the Git garbage diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/projects/ProjectApi.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/projects/ProjectApi.java index ee8ea6903e..f4f7087828 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/projects/ProjectApi.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/projects/ProjectApi.java @@ -15,7 +15,6 @@ package com.google.gerrit.client.projects; import com.google.gerrit.client.VoidResult; import com.google.gerrit.client.projects.ConfigInfo.ConfigParameterValue; -import com.google.gerrit.client.rpc.CallbackGroup; import com.google.gerrit.client.rpc.NativeMap; import com.google.gerrit.client.rpc.NativeString; import com.google.gerrit.client.rpc.RestApi; @@ -60,21 +59,20 @@ public class ProjectApi { } /** - * Delete branches. For each branch to be deleted a separate DELETE request is - * fired to the server. The {@code onSuccess} method of the provided callback - * is invoked once after all requests succeeded. If any request fails the - * callbacks' {@code onFailure} method is invoked. In a failure case it can be - * that still some of the branches were successfully deleted. + * Delete branches. One call is fired to the server to delete all the + * branches. */ public static void deleteBranches(Project.NameKey name, Set refs, AsyncCallback cb) { - CallbackGroup group = new CallbackGroup(); - for (String ref : refs) { - project(name).view("branches").id(ref) - .delete(group.add(cb)); - cb = CallbackGroup.emptyCallback(); + if (refs.size() == 1) { + project(name).view("branches").id(refs.iterator().next()).delete(cb); + } else { + DeleteBranchesInput d = DeleteBranchesInput.create(); + for (String ref : refs) { + d.add_branch(ref); + } + project(name).view("branches:delete").post(d, cb); } - group.done(); } public static void getConfig(Project.NameKey name, @@ -292,4 +290,18 @@ public class ProjectApi { final native void setRef(String r) /*-{ if(r)this.ref=r; }-*/; } + + private static class DeleteBranchesInput extends JavaScriptObject { + static DeleteBranchesInput create() { + DeleteBranchesInput d = createObject().cast(); + d.init(); + return d; + } + + protected DeleteBranchesInput() { + } + + final native void init() /*-{ this.branches = []; }-*/; + final native void add_branch(String b) /*-{ this.branches.push(b); }-*/; + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranch.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranch.java index 9a714f86e8..a2aa6daa06 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranch.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranch.java @@ -104,7 +104,7 @@ public class DeleteBranch implements RestModifyView{ break; case REJECTED_CURRENT_BRANCH: - log.warn("Cannot delete " + rsrc.getBranchKey() + ": " + result.name()); + log.error("Cannot delete " + rsrc.getBranchKey() + ": " + result.name()); throw new ResourceConflictException("cannot delete current branch"); default: diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranches.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranches.java new file mode 100644 index 0000000000..bdc67ac774 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteBranches.java @@ -0,0 +1,181 @@ +// Copyright (C) 2015 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.project; + +import static java.lang.String.format; + +import com.google.common.collect.Lists; +import com.google.gerrit.common.ChangeHooks; +import com.google.gerrit.extensions.restapi.ResourceConflictException; +import com.google.gerrit.extensions.restapi.Response; +import com.google.gerrit.extensions.restapi.RestModifyView; +import com.google.gerrit.reviewdb.client.Branch; +import com.google.gerrit.reviewdb.client.SubmoduleSubscription; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.extensions.events.GitReferenceUpdated; +import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.project.DeleteBranches.Input; +import com.google.gerrit.server.query.change.InternalChangeQuery; +import com.google.gwtorm.server.OrmException; +import com.google.gwtorm.server.ResultSet; +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; + +import org.eclipse.jgit.lib.BatchRefUpdate; +import org.eclipse.jgit.lib.NullProgressMonitor; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.RevWalk; +import org.eclipse.jgit.transport.ReceiveCommand; +import org.eclipse.jgit.transport.ReceiveCommand.Result; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.util.List; + +@Singleton +class DeleteBranches implements RestModifyView { + private static final Logger log = LoggerFactory.getLogger(DeleteBranches.class); + + static class Input { + List branches; + + static Input init(Input in) { + if (in == null) { + in = new Input(); + } + if (in.branches == null) { + in.branches = Lists.newArrayListWithCapacity(1); + } + return in; + } + } + + private final Provider identifiedUser; + private final GitRepositoryManager repoManager; + private final Provider dbProvider; + private final Provider queryProvider; + private final GitReferenceUpdated referenceUpdated; + private final ChangeHooks hooks; + + @Inject + DeleteBranches(Provider identifiedUser, + GitRepositoryManager repoManager, + Provider dbProvider, + Provider queryProvider, + GitReferenceUpdated referenceUpdated, + ChangeHooks hooks) { + this.identifiedUser = identifiedUser; + this.repoManager = repoManager; + this.dbProvider = dbProvider; + this.queryProvider = queryProvider; + this.referenceUpdated = referenceUpdated; + this.hooks = hooks; + } + + @Override + public Response apply(ProjectResource project, Input input) + throws OrmException, IOException, ResourceConflictException { + input = Input.init(input); + Repository r = repoManager.openRepository(project.getNameKey()); + try { + BatchRefUpdate batchUpdate = r.getRefDatabase().newBatchUpdate(); + for (String branch : input.branches) { + batchUpdate.addCommand(createDeleteCommand(project, r, branch)); + } + RevWalk rw = new RevWalk(r); + try { + batchUpdate.execute(rw, NullProgressMonitor.INSTANCE); + } finally { + rw.release(); + } + StringBuilder errorMessages = new StringBuilder(); + for (ReceiveCommand command : batchUpdate.getCommands()) { + if (command.getResult() == Result.OK) { + postDeletion(project, command); + } else { + appendAndLogErrorMessage(errorMessages, command); + } + } + if (errorMessages.length() > 0) { + throw new ResourceConflictException(errorMessages.toString()); + } + } finally { + r.close(); + } + return Response.none(); + } + + private ReceiveCommand createDeleteCommand(ProjectResource project, + Repository r, String branch) throws OrmException, IOException { + Ref ref = r.getRefDatabase().getRef(branch); + ReceiveCommand command; + if (ref == null) { + command = new ReceiveCommand(ObjectId.zeroId(), ObjectId.zeroId(), branch); + command.setResult(Result.REJECTED_OTHER_REASON, + "it doesn't exist or you do not have permission to delete it"); + return command; + } + command = + new ReceiveCommand(ref.getObjectId(), ObjectId.zeroId(), ref.getName()); + Branch.NameKey branchKey = + new Branch.NameKey(project.getNameKey(), ref.getName()); + if (!project.getControl().controlForRef(branchKey).canDelete()) { + command.setResult(Result.REJECTED_OTHER_REASON, + "it doesn't exist or you do not have permission to delete it"); + } + if (!queryProvider.get().setLimit(1).byBranchOpen(branchKey).isEmpty()) { + command.setResult(Result.REJECTED_OTHER_REASON, "it has open changes"); + } + return command; + } + + private void appendAndLogErrorMessage(StringBuilder errorMessages, + ReceiveCommand cmd) { + String msg = null; + switch (cmd.getResult()) { + case REJECTED_CURRENT_BRANCH: + msg = format("Cannot delete %s: it is the current branch", + cmd.getRefName()); + break; + case REJECTED_OTHER_REASON: + msg = format("Cannot delete %s: %s", cmd.getRefName(), cmd.getMessage()); + break; + default: + msg = format("Cannot delete %s: %s", cmd.getRefName(), cmd.getResult()); + break; + } + log.error(msg); + errorMessages.append(msg); + errorMessages.append("\n"); + } + + private void postDeletion(ProjectResource project, ReceiveCommand cmd) + throws OrmException { + referenceUpdated.fire(project.getNameKey(), cmd.getRefName(), + cmd.getOldId(), cmd.getNewId()); + Branch.NameKey branchKey = + new Branch.NameKey(project.getNameKey(), cmd.getRefName()); + hooks.doRefUpdatedHook(branchKey, cmd.getOldId(), cmd.getNewId(), + identifiedUser.get().getAccount()); + ResultSet submoduleSubscriptions = + dbProvider.get().submoduleSubscriptions().bySuperProject(branchKey); + dbProvider.get().submoduleSubscriptions().delete(submoduleSubscriptions); + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/Module.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/Module.java index ace221d7da..430d8f5a4d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/Module.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/Module.java @@ -64,6 +64,7 @@ public class Module extends RestApiModule { put(BRANCH_KIND).to(PutBranch.class); get(BRANCH_KIND).to(GetBranch.class); delete(BRANCH_KIND).to(DeleteBranch.class); + post(PROJECT_KIND, "branches:delete").to(DeleteBranches.class); install(new FactoryModuleBuilder().build(CreateBranch.Factory.class)); get(BRANCH_KIND, "reflog").to(GetReflog.class); child(BRANCH_KIND, "files").to(FilesCollection.class);