Reject restoring change if its destination branch does not exist

If a branch got deleted and there was an abandoned change for this
branch, it was possible to restore this change.

Change-Id: I0b17b1b2e4d5cd97bcb1a8a06c3938626250666c
Signed-off-by: Edwin Kempin <edwin.kempin@sap.com>
This commit is contained in:
Edwin Kempin
2012-01-26 08:53:58 +01:00
parent edfffc48b6
commit a71bd46ad9
10 changed files with 161 additions and 51 deletions

View File

@@ -76,7 +76,10 @@ public class ReviewResult {
NOT_A_DRAFT, NOT_A_DRAFT,
/** Error writing change to git repository */ /** Error writing change to git repository */
GIT_ERROR GIT_ERROR,
/** The destination branch does not exist */
DEST_BRANCH_NOT_FOUND
} }
protected Type type; protected Type type;

View File

@@ -28,6 +28,10 @@ import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject; import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.Assisted;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import java.io.IOException;
import javax.annotation.Nullable; import javax.annotation.Nullable;
class AbandonChangeHandler extends Handler<ChangeDetail> { class AbandonChangeHandler extends Handler<ChangeDetail> {
@@ -58,7 +62,8 @@ class AbandonChangeHandler extends Handler<ChangeDetail> {
@Override @Override
public ChangeDetail call() throws NoSuchChangeException, OrmException, public ChangeDetail call() throws NoSuchChangeException, OrmException,
EmailException, NoSuchEntityException, InvalidChangeOperationException, EmailException, NoSuchEntityException, InvalidChangeOperationException,
PatchSetInfoNotAvailableException { PatchSetInfoNotAvailableException, RepositoryNotFoundException,
IOException {
final ReviewResult result = final ReviewResult result =
abandonChangeFactory.create(patchSetId, message).call(); abandonChangeFactory.create(patchSetId, message).call();
if (result.getErrors().size() > 0) { if (result.getErrors().size() > 0) {

View File

@@ -33,8 +33,10 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.AnonymousUser; import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.ProjectUtil;
import com.google.gerrit.server.account.AccountInfoCacheFactory; import com.google.gerrit.server.account.AccountInfoCacheFactory;
import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MergeOp; import com.google.gerrit.server.git.MergeOp;
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ChangeControl;
@@ -46,8 +48,10 @@ import com.google.gwtorm.server.ResultSet;
import com.google.inject.Inject; import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.Assisted;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Config;
import java.io.IOException;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.Collections; import java.util.Collections;
import java.util.Comparator; import java.util.Comparator;
@@ -70,6 +74,7 @@ public class ChangeDetailFactory extends Handler<ChangeDetail> {
private final AccountInfoCacheFactory aic; private final AccountInfoCacheFactory aic;
private final AnonymousUser anonymousUser; private final AnonymousUser anonymousUser;
private final ReviewDb db; private final ReviewDb db;
private final GitRepositoryManager repoManager;
private final Change.Id changeId; private final Change.Id changeId;
@@ -84,6 +89,7 @@ public class ChangeDetailFactory extends Handler<ChangeDetail> {
ChangeDetailFactory(final ApprovalTypes approvalTypes, ChangeDetailFactory(final ApprovalTypes approvalTypes,
final FunctionState.Factory functionState, final FunctionState.Factory functionState,
final PatchSetDetailFactory.Factory patchSetDetail, final ReviewDb db, final PatchSetDetailFactory.Factory patchSetDetail, final ReviewDb db,
final GitRepositoryManager repoManager,
final ChangeControl.Factory changeControlFactory, final ChangeControl.Factory changeControlFactory,
final AccountInfoCacheFactory.Factory accountInfoCacheFactory, final AccountInfoCacheFactory.Factory accountInfoCacheFactory,
final AnonymousUser anonymousUser, final AnonymousUser anonymousUser,
@@ -94,6 +100,7 @@ public class ChangeDetailFactory extends Handler<ChangeDetail> {
this.functionState = functionState; this.functionState = functionState;
this.patchSetDetail = patchSetDetail; this.patchSetDetail = patchSetDetail;
this.db = db; this.db = db;
this.repoManager = repoManager;
this.changeControlFactory = changeControlFactory; this.changeControlFactory = changeControlFactory;
this.anonymousUser = anonymousUser; this.anonymousUser = anonymousUser;
this.aic = accountInfoCacheFactory.create(); this.aic = accountInfoCacheFactory.create();
@@ -106,7 +113,8 @@ public class ChangeDetailFactory extends Handler<ChangeDetail> {
@Override @Override
public ChangeDetail call() throws OrmException, NoSuchEntityException, public ChangeDetail call() throws OrmException, NoSuchEntityException,
PatchSetInfoNotAvailableException, NoSuchChangeException { PatchSetInfoNotAvailableException, NoSuchChangeException,
RepositoryNotFoundException, IOException {
control = changeControlFactory.validateFor(changeId); control = changeControlFactory.validateFor(changeId);
final Change change = control.getChange(); final Change change = control.getChange();
final PatchSet patch = db.patchSets().get(change.currentPatchSetId()); final PatchSet patch = db.patchSets().get(change.currentPatchSetId());
@@ -122,7 +130,9 @@ public class ChangeDetailFactory extends Handler<ChangeDetail> {
detail.setCanAbandon(change.getStatus() != Change.Status.DRAFT && change.getStatus().isOpen() && control.canAbandon()); detail.setCanAbandon(change.getStatus() != Change.Status.DRAFT && change.getStatus().isOpen() && control.canAbandon());
detail.setCanPublish(control.canPublish(db)); detail.setCanPublish(control.canPublish(db));
detail.setCanRestore(change.getStatus() == Change.Status.ABANDONED && control.canRestore()); detail.setCanRestore(change.getStatus() == Change.Status.ABANDONED
&& control.canRestore()
&& ProjectUtil.branchExists(repoManager, change.getDest()));
detail.setCanDeleteDraft(control.canDeleteDraft(db)); detail.setCanDeleteDraft(control.canDeleteDraft(db));
detail.setStarred(control.getCurrentUser().getStarredChanges().contains( detail.setStarred(control.getCurrentUser().getStarredChanges().contains(
changeId)); changeId));

View File

@@ -26,6 +26,10 @@ import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject; import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.Assisted;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import java.io.IOException;
class PublishAction extends Handler<ChangeDetail> { class PublishAction extends Handler<ChangeDetail> {
interface Factory { interface Factory {
PublishAction create(PatchSet.Id patchSetId); PublishAction create(PatchSet.Id patchSetId);
@@ -49,7 +53,7 @@ class PublishAction extends Handler<ChangeDetail> {
@Override @Override
public ChangeDetail call() throws OrmException, NoSuchEntityException, public ChangeDetail call() throws OrmException, NoSuchEntityException,
IllegalStateException, PatchSetInfoNotAvailableException, IllegalStateException, PatchSetInfoNotAvailableException,
NoSuchChangeException { NoSuchChangeException, RepositoryNotFoundException, IOException {
final ReviewResult result = publishFactory.create(patchSetId).call(); final ReviewResult result = publishFactory.create(patchSetId).call();
if (result.getErrors().size() > 0) { if (result.getErrors().size() > 0) {
throw new IllegalStateException("Cannot publish patchset"); throw new IllegalStateException("Cannot publish patchset");

View File

@@ -28,6 +28,10 @@ import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject; import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.Assisted;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import java.io.IOException;
import javax.annotation.Nullable; import javax.annotation.Nullable;
class RestoreChangeHandler extends Handler<ChangeDetail> { class RestoreChangeHandler extends Handler<ChangeDetail> {
@@ -57,7 +61,8 @@ class RestoreChangeHandler extends Handler<ChangeDetail> {
@Override @Override
public ChangeDetail call() throws NoSuchChangeException, OrmException, public ChangeDetail call() throws NoSuchChangeException, OrmException,
EmailException, NoSuchEntityException, InvalidChangeOperationException, EmailException, NoSuchEntityException, InvalidChangeOperationException,
PatchSetInfoNotAvailableException { PatchSetInfoNotAvailableException, RepositoryNotFoundException,
IOException {
final ReviewResult result = final ReviewResult result =
restoreChangeFactory.create(patchSetId, message).call(); restoreChangeFactory.create(patchSetId, message).call();
if (result.getErrors().size() > 0) { if (result.getErrors().size() > 0) {

View File

@@ -27,6 +27,10 @@ import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject; import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.Assisted;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import java.io.IOException;
class SubmitAction extends Handler<ChangeDetail> { class SubmitAction extends Handler<ChangeDetail> {
interface Factory { interface Factory {
SubmitAction create(PatchSet.Id patchSetId); SubmitAction create(PatchSet.Id patchSetId);
@@ -50,7 +54,8 @@ class SubmitAction extends Handler<ChangeDetail> {
@Override @Override
public ChangeDetail call() throws OrmException, NoSuchEntityException, public ChangeDetail call() throws OrmException, NoSuchEntityException,
IllegalStateException, InvalidChangeOperationException, IllegalStateException, InvalidChangeOperationException,
PatchSetInfoNotAvailableException, NoSuchChangeException { PatchSetInfoNotAvailableException, NoSuchChangeException,
RepositoryNotFoundException, IOException {
final ReviewResult result = final ReviewResult result =
submitFactory.create(patchSetId).call(); submitFactory.create(patchSetId).call();
if (result.getErrors().size() > 0) { if (result.getErrors().size() > 0) {

View File

@@ -52,6 +52,9 @@ import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject; import com.google.inject.Inject;
import com.google.inject.Provider; import com.google.inject.Provider;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import java.io.IOException;
import java.util.Collections; import java.util.Collections;
import java.util.HashMap; import java.util.HashMap;
import java.util.List; import java.util.List;
@@ -166,6 +169,10 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements
throw new Failure(e); throw new Failure(e);
} catch (PatchSetInfoNotAvailableException e) { } catch (PatchSetInfoNotAvailableException e) {
throw new Failure(e); throw new Failure(e);
} catch (RepositoryNotFoundException e) {
throw new Failure(e);
} catch (IOException e) {
throw new Failure(e);
} }
} }
}); });

View File

@@ -0,0 +1,48 @@
// Copyright (C) 2012 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;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.server.git.GitRepositoryManager;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.Repository;
import java.io.IOException;
public class ProjectUtil {
/**
* Checks whether the specified branch exists.
*
* @param repoManager Git repository manager to open the git repository
* @param branch the branch for which it should be checked if it exists
* @return <code>true</code> if the specified branch exists, otherwise
* <code>false</code>
* @throws RepositoryNotFoundException the repository of the branch's project
* does not exist.
* @throws IOException error while retrieving the branch from the repository.
*/
public static boolean branchExists(final GitRepositoryManager repoManager,
final Branch.NameKey branch) throws RepositoryNotFoundException,
IOException {
final Repository repo = repoManager.openRepository(branch.getParentKey());
try {
return repo.getRef(branch.get()) != null;
} finally {
repo.close();
}
}
}

View File

@@ -17,12 +17,15 @@ package com.google.gerrit.server.changedetail;
import com.google.gerrit.common.ChangeHooks; import com.google.gerrit.common.ChangeHooks;
import com.google.gerrit.common.data.ReviewResult; import com.google.gerrit.common.data.ReviewResult;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.ProjectUtil;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.mail.EmailException; import com.google.gerrit.server.mail.EmailException;
import com.google.gerrit.server.mail.RestoredSender; import com.google.gerrit.server.mail.RestoredSender;
import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ChangeControl;
@@ -33,6 +36,9 @@ import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject; import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.Assisted;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import java.io.IOException;
import java.util.concurrent.Callable; import java.util.concurrent.Callable;
public class RestoreChange implements Callable<ReviewResult> { public class RestoreChange implements Callable<ReviewResult> {
@@ -44,6 +50,7 @@ public class RestoreChange implements Callable<ReviewResult> {
private final RestoredSender.Factory restoredSenderFactory; private final RestoredSender.Factory restoredSenderFactory;
private final ChangeControl.Factory changeControlFactory; private final ChangeControl.Factory changeControlFactory;
private final ReviewDb db; private final ReviewDb db;
private final GitRepositoryManager repoManager;
private final IdentifiedUser currentUser; private final IdentifiedUser currentUser;
private final ChangeHooks hooks; private final ChangeHooks hooks;
@@ -53,12 +60,13 @@ public class RestoreChange implements Callable<ReviewResult> {
@Inject @Inject
RestoreChange(final RestoredSender.Factory restoredSenderFactory, RestoreChange(final RestoredSender.Factory restoredSenderFactory,
final ChangeControl.Factory changeControlFactory, final ReviewDb db, final ChangeControl.Factory changeControlFactory, final ReviewDb db,
final IdentifiedUser currentUser, final ChangeHooks hooks, final GitRepositoryManager repoManager, final IdentifiedUser currentUser,
@Assisted final PatchSet.Id patchSetId, final ChangeHooks hooks, @Assisted final PatchSet.Id patchSetId,
@Assisted final String changeComment) { @Assisted final String changeComment) {
this.restoredSenderFactory = restoredSenderFactory; this.restoredSenderFactory = restoredSenderFactory;
this.changeControlFactory = changeControlFactory; this.changeControlFactory = changeControlFactory;
this.db = db; this.db = db;
this.repoManager = repoManager;
this.currentUser = currentUser; this.currentUser = currentUser;
this.hooks = hooks; this.hooks = hooks;
@@ -68,56 +76,66 @@ public class RestoreChange implements Callable<ReviewResult> {
@Override @Override
public ReviewResult call() throws EmailException, public ReviewResult call() throws EmailException,
InvalidChangeOperationException, NoSuchChangeException, OrmException { InvalidChangeOperationException, NoSuchChangeException, OrmException,
RepositoryNotFoundException, IOException {
final ReviewResult result = new ReviewResult(); final ReviewResult result = new ReviewResult();
final Change.Id changeId = patchSetId.getParentKey(); final Change.Id changeId = patchSetId.getParentKey();
result.setChangeId(changeId); result.setChangeId(changeId);
final ChangeControl control = changeControlFactory.validateFor(changeId); final ChangeControl control = changeControlFactory.validateFor(changeId);
final PatchSet patch = db.patchSets().get(patchSetId);
if (!control.canRestore()) { if (!control.canRestore()) {
result.addError(new ReviewResult.Error( result.addError(new ReviewResult.Error(
ReviewResult.Error.Type.RESTORE_NOT_PERMITTED)); ReviewResult.Error.Type.RESTORE_NOT_PERMITTED));
} else if (patch == null) { return result;
throw new NoSuchChangeException(changeId);
} else {
// Create a message to accompany the restored change
final ChangeMessage cmsg =
new ChangeMessage(new ChangeMessage.Key(changeId, ChangeUtil
.messageUUID(db)), currentUser.getAccountId(), patchSetId);
final StringBuilder msgBuf =
new StringBuilder("Patch Set " + patchSetId.get() + ": Restored");
if (changeComment != null && changeComment.length() > 0) {
msgBuf.append("\n\n");
msgBuf.append(changeComment);
}
cmsg.setMessage(msgBuf.toString());
// Restore the change
final Change updatedChange = db.changes().atomicUpdate(changeId,
new AtomicUpdate<Change>() {
@Override
public Change update(Change change) {
if (change.getStatus() == Change.Status.ABANDONED
&& change.currentPatchSetId().equals(patchSetId)) {
change.setStatus(Change.Status.NEW);
ChangeUtil.updated(change);
return change;
} else {
return null;
}
}
});
ChangeUtil.updatedChange(
db, currentUser, updatedChange, cmsg, restoredSenderFactory,
"Change is not abandoned or patchset is not latest");
hooks.doChangeRestoreHook(updatedChange, currentUser.getAccount(),
changeComment, db);
} }
final PatchSet patch = db.patchSets().get(patchSetId);
if (patch == null) {
throw new NoSuchChangeException(changeId);
}
final Branch.NameKey destBranch = control.getChange().getDest();
if (!ProjectUtil.branchExists(repoManager, destBranch)) {
result.addError(new ReviewResult.Error(
ReviewResult.Error.Type.DEST_BRANCH_NOT_FOUND, destBranch.get()));
return result;
}
// Create a message to accompany the restored change
final ChangeMessage cmsg =
new ChangeMessage(new ChangeMessage.Key(changeId, ChangeUtil
.messageUUID(db)), currentUser.getAccountId(), patchSetId);
final StringBuilder msgBuf =
new StringBuilder("Patch Set " + patchSetId.get() + ": Restored");
if (changeComment != null && changeComment.length() > 0) {
msgBuf.append("\n\n");
msgBuf.append(changeComment);
}
cmsg.setMessage(msgBuf.toString());
// Restore the change
final Change updatedChange = db.changes().atomicUpdate(changeId,
new AtomicUpdate<Change>() {
@Override
public Change update(Change change) {
if (change.getStatus() == Change.Status.ABANDONED
&& change.currentPatchSetId().equals(patchSetId)) {
change.setStatus(Change.Status.NEW);
ChangeUtil.updated(change);
return change;
} else {
return null;
}
}
});
ChangeUtil.updatedChange(
db, currentUser, updatedChange, cmsg, restoredSenderFactory,
"Change is not abandoned or patchset is not latest");
hooks.doChangeRestoreHook(updatedChange, currentUser.getAccount(),
changeComment, db);
return result; return result;
} }
} }

View File

@@ -39,6 +39,7 @@ import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.ResultSet; import com.google.gwtorm.server.ResultSet;
import com.google.inject.Inject; import com.google.inject.Inject;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.kohsuke.args4j.Argument; import org.kohsuke.args4j.Argument;
import org.kohsuke.args4j.Option; import org.kohsuke.args4j.Option;
import org.slf4j.Logger; import org.slf4j.Logger;
@@ -180,8 +181,9 @@ public class ReviewCommand extends SshCommand {
} }
} }
private void approveOne(final PatchSet.Id patchSetId) throws private void approveOne(final PatchSet.Id patchSetId)
NoSuchChangeException, OrmException, EmailException, Failure { throws NoSuchChangeException, OrmException, EmailException, Failure,
RepositoryNotFoundException, IOException {
if (changeComment == null) { if (changeComment == null) {
changeComment = ""; changeComment = "";
@@ -261,6 +263,9 @@ public class ReviewCommand extends SshCommand {
case GIT_ERROR: case GIT_ERROR:
errMsg += "error writing change to git repository"; errMsg += "error writing change to git repository";
break; break;
case DEST_BRANCH_NOT_FOUND:
errMsg += "destination branch not found";
break;
default: default:
errMsg += "failure in review"; errMsg += "failure in review";
} }