opendev/gerrit
Code Issues Proposed changes

Merge branch 'stable-2.9' into stable-2.10

Browse Source 
Conflicts:
	gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java
	gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
	gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java

Change-Id: Id716ae24401756b9cfbcb46489259c0d9660412a
This commit is contained in:
Shawn Pearce 2014-08-27 17:15:33 -07:00
commit 9954c4e249
16 changed files with 71 additions and 38 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
Documentation/cmd-review.txt
View File

@ -75,7 +75,8 @@ branch.
--abandon::
Abandon the specified change(s).
(option is mutually exclusive with --submit and --restore)
(option is mutually exclusive with --submit, --restore, --publish and
--delete)
--restore::
Restore the specified abandoned change(s).
@ -84,7 +85,7 @@ branch.
--submit::
-s::
Submit the specified patch set(s) for merging.
(option is mutually exclusive with --abandon)
(option is mutually exclusive with --abandon, --publish and --delete)
--publish::
Publish the specified draft patch set(s).

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

@ -3016,7 +3016,7 @@ The number of the line for which the comment was done. +
If range is set, this equals the end line of the range. +
If neither line nor range is set, it's a file comment.
|`range` |optional|
The range of the comment as a link:rest-api.html#comment-range[CommentRange]
The range of the comment as a link:#comment-range[CommentRange]
entity.
|`in_reply_to` |optional|
The URL encoded UUID of the comment to which this comment is a reply.
@ -3055,7 +3055,7 @@ The number of the line for which the comment should be added. +
If neither line nor range is set, a file comment is added. +
If range is set, this should equal the end line of the range.
|`range` |optional|
The range of the comment as a link:rest-api.html#comment-range[CommentRange]
The range of the comment as a link:#comment-range[CommentRange]
entity.
|`in_reply_to` |optional|
The URL encoded UUID of the comment to which this comment is a reply.

13
gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityChecker.java
View File

@ -333,6 +333,11 @@ public class MergeabilityChecker implements GitReferenceUpdatedListener {
ReviewDb db = context.getReviewDbProvider().get();
try {
PatchSet ps = db.patchSets().get(change.currentPatchSetId());
if (ps == null) {
// Cannot compute mergeability if current patch set is missing.
return false;
}
Mergeable m = mergeable.get();
m.setForce(force);
@ -345,10 +350,10 @@ public class MergeabilityChecker implements GitReferenceUpdatedListener {
// change is closed
return false;
} catch (Exception e) {
String msg = "Failed to update mergeability flags for project "
+ change.getDest().getParentKey() + " on update of "
+ change.getDest().get();
log.error(msg, e);
log.error(String.format(
"cannot update mergeability flag of change %d in project %s after update of %s",
change.getId().get(),
change.getDest().getParentKey(), change.getDest().get()), e);
throw e;
} finally {
tl.setContext(old);

10
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
View File

@ -655,14 +655,17 @@ public class MergeOp {
private void fireRefUpdated(RefUpdate branchUpdate) {
gitRefUpdated.fire(destBranch.getParentKey(), branchUpdate);
hooks.doRefUpdatedHook(destBranch, branchUpdate, getAccount(mergeTip));
}
private Account getAccount(CodeReviewCommit codeReviewCommit) {
Account account = null;
PatchSetApproval submitter = approvalsUtil.getSubmitter(
db, mergeTip.notes(), mergeTip.getPatchsetId());
db, codeReviewCommit.notes(), codeReviewCommit.getPatchsetId());
if (submitter != null) {
account = accountCache.get(submitter.getAccountId()).getAccount();
}
hooks.doRefUpdatedHook(destBranch, branchUpdate, account);
return account;
}
private void updateChangeStatus(final List<Change> submitted) throws NoSuchChangeException {
@ -726,7 +729,8 @@ public class MergeOp {
if (mergeTip != null && (branchTip == null || branchTip != mergeTip)) {
SubmoduleOp subOp =
subOpFactory.create(destBranch, mergeTip, rw, repo,
destProject.getProject(), submitted, commits);
destProject.getProject(), submitted, commits,
getAccount(mergeTip));
try {
subOp.update();
} catch (SubmoduleException e) {

3
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@ -2336,7 +2336,8 @@ public class ReceiveCommits {
subOpFactory.create(
new Branch.NameKey(project.getNameKey(), cmd.getRefName()),
codeReviewCommit, rw, repo, project, new ArrayList<Change>(),
new HashMap<Change.Id, CodeReviewCommit>());
new HashMap<Change.Id, CodeReviewCommit>(),
currentUser.getAccount());
subOp.update();
} catch (InsertException e) {
log.error("Can't insert patchset", e);

12
gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java
View File

@ -15,7 +15,9 @@
package com.google.gerrit.server.git;
import com.google.common.base.Strings;
import com.google.gerrit.common.ChangeHooks;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
@ -68,7 +70,7 @@ public class SubmoduleOp {
public interface Factory {
SubmoduleOp create(Branch.NameKey destBranch, RevCommit mergeTip,
RevWalk rw, Repository db, Project destProject, List<Change> submitted,
Map<Change.Id, CodeReviewCommit> commits);
Map<Change.Id, CodeReviewCommit> commits, Account account);
}
private static final Logger log = LoggerFactory.getLogger(SubmoduleOp.class);
@ -88,6 +90,8 @@ public class SubmoduleOp {
private final GitReferenceUpdated gitRefUpdated;
private final SchemaFactory<ReviewDb> schemaFactory;
private final Set<Branch.NameKey> updatedSubscribers;
private final Account account;
private final ChangeHooks changeHooks;
@Inject
public SubmoduleOp(@Assisted final Branch.NameKey destBranch,
@ -97,7 +101,8 @@ public class SubmoduleOp {
@Assisted Project destProject, @Assisted List<Change> submitted,
@Assisted final Map<Change.Id, CodeReviewCommit> commits,
@GerritPersonIdent final PersonIdent myIdent,
GitRepositoryManager repoManager, GitReferenceUpdated gitRefUpdated) {
GitRepositoryManager repoManager, GitReferenceUpdated gitRefUpdated,
@Assisted Account account, ChangeHooks changeHooks) {
this.destBranch = destBranch;
this.mergeTip = mergeTip;
this.rw = rw;
@ -110,6 +115,8 @@ public class SubmoduleOp {
this.myIdent = myIdent;
this.repoManager = repoManager;
this.gitRefUpdated = gitRefUpdated;
this.account = account;
this.changeHooks = changeHooks;
updatedSubscribers = new HashSet<>();
}
@ -344,6 +351,7 @@ public class SubmoduleOp {
case NEW:
case FAST_FORWARD:
gitRefUpdated.fire(subscriber.getParentKey(), rfu);
changeHooks.doRefUpdatedHook(subscriber, rfu, account);
// TODO since this is performed "in the background" no mail will be
// sent to inform users about the updated branch
break;

6
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java
View File

@ -105,7 +105,7 @@ public class CherryPick extends SubmitStrategy {
mergeTip = n;
} else {
mergeTip =
args.mergeUtil.mergeOneCommit(args.myIdent, args.repo,
args.mergeUtil.mergeOneCommit(args.serverIdent.get(), args.repo,
args.rw, args.inserter, args.canMergeFlag,
args.destBranch, mergeTip, n);
}
@ -143,10 +143,10 @@ public class CherryPick extends SubmitStrategy {
cherryPickUser =
args.identifiedUserFactory.create(submitAudit.getAccountId());
cherryPickCommitterIdent = cherryPickUser.newCommitterIdent(
submitAudit.getGranted(), args.myIdent.getTimeZone());
submitAudit.getGranted(), args.serverIdent.get().getTimeZone());
} else {
cherryPickUser = args.identifiedUserFactory.create(n.change().getOwner());
cherryPickCommitterIdent = args.myIdent;
cherryPickCommitterIdent = args.serverIdent.get();
}
final String cherryPickCmtMsg = args.mergeUtil.createCherryPickCommitMessage(n);

2
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeAlways.java
View File

@ -38,7 +38,7 @@ public class MergeAlways extends SubmitStrategy {
}
while (!toMerge.isEmpty()) {
mergeTip =
args.mergeUtil.mergeOneCommit(args.myIdent, args.repo, args.rw,
args.mergeUtil.mergeOneCommit(args.serverIdent.get(), args.repo, args.rw,
args.inserter, args.canMergeFlag, args.destBranch, mergeTip,
toMerge.remove(0));
}

2
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/MergeIfNecessary.java
View File

@ -43,7 +43,7 @@ public class MergeIfNecessary extends SubmitStrategy {
// For every other commit do a pair-wise merge.
while (!toMerge.isEmpty()) {
mergeTip =
args.mergeUtil.mergeOneCommit(args.myIdent, args.repo, args.rw,
args.mergeUtil.mergeOneCommit(args.serverIdent.get(), args.repo, args.rw,
args.inserter, args.canMergeFlag, args.destBranch, mergeTip,
toMerge.remove(0));
}

10
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
View File

@ -32,7 +32,6 @@ import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gwtorm.server.OrmException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import java.io.IOException;
import java.util.HashMap;
@ -43,17 +42,14 @@ public class RebaseIfNecessary extends SubmitStrategy {
private final PatchSetInfoFactory patchSetInfoFactory;
private final RebaseChange rebaseChange;
private final Map<Change.Id, CodeReviewCommit> newCommits;
private final PersonIdent committerIdent;
RebaseIfNecessary(SubmitStrategy.Arguments args,
PatchSetInfoFactory patchSetInfoFactory,
RebaseChange rebaseChange,
PersonIdent committerIdent) {
RebaseChange rebaseChange) {
super(args);
this.patchSetInfoFactory = patchSetInfoFactory;
this.rebaseChange = rebaseChange;
this.newCommits = new HashMap<>();
this.committerIdent = committerIdent;
}
@Override
@ -91,7 +87,7 @@ public class RebaseIfNecessary extends SubmitStrategy {
final PatchSet newPatchSet =
rebaseChange.rebase(args.repo, args.rw, args.inserter,
n.getPatchsetId(), n.change(), uploader,
newMergeTip, args.mergeUtil, committerIdent,
newMergeTip, args.mergeUtil, args.serverIdent.get(),
false, false, ValidatePolicy.NONE);
List<PatchSetApproval> approvals = Lists.newArrayList();
@ -133,7 +129,7 @@ public class RebaseIfNecessary extends SubmitStrategy {
newMergeTip = n;
} else {
newMergeTip = args.mergeUtil.mergeOneCommit(
args.myIdent, args.repo, args.rw, args.inserter,
args.serverIdent.get(), args.repo, args.rw, args.inserter,
args.canMergeFlag, args.destBranch, newMergeTip, n);
}
final PatchSetApproval submitApproval = args.mergeUtil.markCleanMerges(

7
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java
View File

@ -27,6 +27,7 @@ import com.google.gerrit.server.git.MergeSorter;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.index.ChangeIndexer;
import com.google.gerrit.server.project.ChangeControl;
import com.google.inject.Provider;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.PersonIdent;
@ -52,7 +53,7 @@ public abstract class SubmitStrategy {
static class Arguments {
protected final IdentifiedUser.GenericFactory identifiedUserFactory;
protected final PersonIdent myIdent;
protected final Provider<PersonIdent> serverIdent;
protected final ReviewDb db;
protected final ChangeControl.GenericFactory changeControlFactory;
@ -68,14 +69,14 @@ public abstract class SubmitStrategy {
protected final MergeSorter mergeSorter;
Arguments(final IdentifiedUser.GenericFactory identifiedUserFactory,
final PersonIdent myIdent, final ReviewDb db,
final Provider<PersonIdent> serverIdent, final ReviewDb db,
final ChangeControl.GenericFactory changeControlFactory,
final Repository repo, final RevWalk rw, final ObjectInserter inserter,
final RevFlag canMergeFlag, final Set<RevCommit> alreadyAccepted,
final Branch.NameKey destBranch, final ApprovalsUtil approvalsUtil,
final MergeUtil mergeUtil, final ChangeIndexer indexer) {
this.identifiedUserFactory = identifiedUserFactory;
this.myIdent = myIdent;
this.serverIdent = serverIdent;
this.db = db;
this.changeControlFactory = changeControlFactory;

8
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyFactory.java
View File

@ -31,6 +31,7 @@ import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import org.eclipse.jgit.lib.ObjectInserter;
@ -51,7 +52,7 @@ public class SubmitStrategyFactory {
.getLogger(SubmitStrategyFactory.class);
private final IdentifiedUser.GenericFactory identifiedUserFactory;
private final PersonIdent myIdent;
private final Provider<PersonIdent> myIdent;
private final ChangeControl.GenericFactory changeControlFactory;
private final PatchSetInfoFactory patchSetInfoFactory;
private final GitReferenceUpdated gitRefUpdated;
@ -64,7 +65,7 @@ public class SubmitStrategyFactory {
@Inject
SubmitStrategyFactory(
final IdentifiedUser.GenericFactory identifiedUserFactory,
@GerritPersonIdent final PersonIdent myIdent,
@GerritPersonIdent Provider<PersonIdent> myIdent,
final ChangeControl.GenericFactory changeControlFactory,
final PatchSetInfoFactory patchSetInfoFactory,
final GitReferenceUpdated gitRefUpdated, final RebaseChange rebaseChange,
@ -105,8 +106,7 @@ public class SubmitStrategyFactory {
case MERGE_IF_NECESSARY:
return new MergeIfNecessary(args);
case REBASE_IF_NECESSARY:
return new RebaseIfNecessary(
args, patchSetInfoFactory, rebaseChange, myIdent);
return new RebaseIfNecessary(args, patchSetInfoFactory, rebaseChange);
default:
final String errorMsg = "No submit strategy for: " + submitType;
log.error(errorMsg);

5
gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java
View File

@ -165,7 +165,10 @@ public class AddMembers implements RestModifyView<GroupResource, Input> {
case LDAP:
if (accountResolver.find(nameOrEmail) == null) {
// account does not exist, try to create it
return createAccountByLdap(nameOrEmail);
Account a = createAccountByLdap(nameOrEmail);
if (a != null) {
return a;
}
}
break;
default:

1
gerrit-server/src/main/resources/com/google/gerrit/server/mime-types.properties
View File

@ -4,6 +4,7 @@ BUCK = text/x-python
clj = text/x-clojure
cl = text/x-common-lisp
coffee = text/x-coffeescript
cs = text/x-csharp
cxx = text/x-c++src
d = text/x-d
defs = text/x-python

13
gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java
View File

@ -15,6 +15,7 @@
package com.google.gerrit.server.git;
import static org.easymock.EasyMock.capture;
import static org.easymock.EasyMock.createNiceMock;
import static org.easymock.EasyMock.createStrictMock;
import static org.easymock.EasyMock.eq;
import static org.easymock.EasyMock.expect;
@ -22,6 +23,7 @@ import static org.easymock.EasyMock.replay;
import static org.easymock.EasyMock.verify;
import static org.junit.Assert.assertEquals;
import com.google.gerrit.common.ChangeHooks;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
@ -79,6 +81,7 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase {
private Provider<String> urlProvider;
private GitRepositoryManager repoManager;
private GitReferenceUpdated gitRefUpdated;
private ChangeHooks changeHooks;
@SuppressWarnings("unchecked")
@Override
@ -92,6 +95,7 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase {
urlProvider = createStrictMock(Provider.class);
repoManager = createStrictMock(GitRepositoryManager.class);
gitRefUpdated = createStrictMock(GitReferenceUpdated.class);
changeHooks = createNiceMock(ChangeHooks.class);
}
private void doReplay() {
@ -138,7 +142,7 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase {
final SubmoduleOp submoduleOp =
new SubmoduleOp(branchNameKey, mergeTip, new RevWalk(realDb), urlProvider,
schemaFactory, realDb, null, new ArrayList<Change>(), null, null,
null, null);
null, null, null, null);
submoduleOp.update();
@ -654,7 +658,8 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase {
new SubmoduleOp(sourceBranchNameKey, sourceMergeTip, new RevWalk(
sourceRepository), urlProvider, schemaFactory, sourceRepository,
new Project(sourceBranchNameKey.getParentKey()), submitted,
mergedCommits, myIdent, repoManager, gitRefUpdated);
mergedCommits, myIdent, repoManager, gitRefUpdated, null,
changeHooks);
submoduleOp.update();
@ -758,7 +763,7 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase {
new SubmoduleOp(sourceBranchNameKey, sourceMergeTip, new RevWalk(
sourceRepository), urlProvider, schemaFactory, sourceRepository,
new Project(sourceBranchNameKey.getParentKey()), submitted,
mergedCommits, myIdent, repoManager, gitRefUpdated);
mergedCommits, myIdent, repoManager, gitRefUpdated, null, changeHooks);
submoduleOp.update();
@ -912,7 +917,7 @@ public class SubmoduleOpTest extends LocalDiskRepositoryTestCase {
new SubmoduleOp(mergedBranch, mergeTip, new RevWalk(realDb),
urlProvider, schemaFactory, realDb, new Project(mergedBranch
.getParentKey()), new ArrayList<Change>(), null, null,
repoManager, null);
repoManager, null, null, null);
submoduleOp.update();
}

8
gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
View File

@ -159,6 +159,11 @@ public class ReviewCommand extends SshCommand {
throw error("publish and delete actions are mutually exclusive");
}
}
if (deleteDraftPatchSet) {
if (submitChange) {
throw error("delete and submit actions are mutually exclusive");
}
}
boolean ok = true;
for (final PatchSet patchSet : patchSets) {
@ -197,6 +202,9 @@ public class ReviewCommand extends SshCommand {
if (changeComment == null) {
changeComment = "";
}
if (notify == null) {
notify = NotifyHandling.ALL;
}
ReviewInput review = new ReviewInput();
review.message = Strings.emptyToNull(changeComment);