diff --git a/Documentation/cmd-review.txt b/Documentation/cmd-review.txt index 787c70ccc8..4c9962dae4 100644 --- a/Documentation/cmd-review.txt +++ b/Documentation/cmd-review.txt @@ -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). diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 7a01b9985f..655309dcd9 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -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. diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityChecker.java index 0734d12496..481e535473 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityChecker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityChecker.java @@ -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); 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 74b5871451..c49ed30c8c 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 @@ -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 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) { 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 711c3f8561..9005bdf588 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 @@ -2336,7 +2336,8 @@ public class ReceiveCommits { subOpFactory.create( new Branch.NameKey(project.getNameKey(), cmd.getRefName()), codeReviewCommit, rw, repo, project, new ArrayList(), - new HashMap()); + new HashMap(), + currentUser.getAccount()); subOp.update(); } catch (InsertException e) { log.error("Can't insert patchset", e); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java index 927d20cf4a..441e31c97f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmoduleOp.java @@ -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 submitted, - Map commits); + Map 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 schemaFactory; private final Set 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 submitted, @Assisted final Map 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; 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 d091d1ce01..3ff36ab079 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 @@ -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); 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 dc0d72192e..9023623ef0 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 @@ -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)); } 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 601c6f8ccc..b84586f682 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 @@ -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)); } 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 c38f5f21f1..130d170dc5 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 @@ -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 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 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( 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 aa080857dc..a864b6c380 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 @@ -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 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 serverIdent, final ReviewDb db, final ChangeControl.GenericFactory changeControlFactory, final Repository repo, final RevWalk rw, final ObjectInserter inserter, final RevFlag canMergeFlag, final Set 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; 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 d427b8d955..091523bdc5 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 @@ -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 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 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); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java index e720af0c7f..df58c8f262 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddMembers.java @@ -165,7 +165,10 @@ public class AddMembers implements RestModifyView { 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: diff --git a/gerrit-server/src/main/resources/com/google/gerrit/server/mime-types.properties b/gerrit-server/src/main/resources/com/google/gerrit/server/mime-types.properties index d6062c3f21..8960ff982e 100644 --- a/gerrit-server/src/main/resources/com/google/gerrit/server/mime-types.properties +++ b/gerrit-server/src/main/resources/com/google/gerrit/server/mime-types.properties @@ -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 diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java index f931dbfcb0..0fe246ee46 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java @@ -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 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(), 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(), null, null, - repoManager, null); + repoManager, null, null, null); submoduleOp.update(); } diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java index da393577a0..f39cd36557 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java @@ -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);