From 4745a16a8b7f0748b7de80e4b5137cee46937147 Mon Sep 17 00:00:00 2001 From: Simon Lei Date: Mon, 11 Aug 2014 15:25:02 -0400 Subject: [PATCH 01/10] Set default for review SSH command to notify=ALL When the --notify option wasn't given, it defaulted to notify=NONE. Make it default to notify=ALL. Bug: issue 2801 Change-Id: I799d3036f93286328311f3339a0e384cc56542c3 --- .../java/com/google/gerrit/sshd/commands/ReviewCommand.java | 3 +++ 1 file changed, 3 insertions(+) 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 e30d41bece..7bc1aaa597 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 @@ -200,6 +200,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); From 4f1f3313a311437828ca3caecb5ba8994c2b8286 Mon Sep 17 00:00:00 2001 From: Jay Soffian Date: Wed, 13 Aug 2014 12:00:06 -0400 Subject: [PATCH 02/10] AddMembers.apply: Prevent NPE when account doesn't exist When trying to add an account to a group, if the account doesn't exist yet and cannot be created, AddMembers.apply() would throw an NPE. Fix it to throw UnprocessableEntityException instead, which is handled by RestApiServlet. Signed-off-by: Jay Soffian Change-Id: I724a7b8a95a8df70506ae591575c74153278adc4 --- .../main/java/com/google/gerrit/server/group/AddMembers.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 8ec6a3fd1d..3da783997a 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 @@ -163,7 +163,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: From 72de517dffc1312e7ebbd3f6a2714bf982d5a4da Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Wed, 20 Aug 2014 14:40:59 -0700 Subject: [PATCH 03/10] Fix stale dates in committer field The server's PersonIdent cannot be held by a singleton such as SubmitStrategyFactory. Holding it would retain the timestamp of the first merge in this process, with the time forever stuck in the past. Instead use Provider so Guice can defer creation of the server's identity until it is required to write a new commit. This allows the current date to be injected into a newly created PersonIdent, keeping the committer field current. Bug: issue 2773 Change-Id: I9e5edd41e42a702ed8541cfef57660204a8c5b57 (cherry picked from commit efcd3168a2f2053597ac467d53f254500de6c1ed) --- .../google/gerrit/server/git/strategy/CherryPick.java | 6 +++--- .../google/gerrit/server/git/strategy/MergeAlways.java | 2 +- .../gerrit/server/git/strategy/MergeIfNecessary.java | 2 +- .../gerrit/server/git/strategy/RebaseIfNecessary.java | 10 +++------- .../gerrit/server/git/strategy/SubmitStrategy.java | 7 ++++--- .../server/git/strategy/SubmitStrategyFactory.java | 8 ++++---- 6 files changed, 16 insertions(+), 19 deletions(-) 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 14a68bb454..3b24baca36 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 a97b3fde2e..f8177f888b 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 a0cd5fd044..a3c9e6ee38 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 d23cc821d8..f260249b55 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 org.eclipse.jgit.lib.ObjectInserter; import org.eclipse.jgit.lib.PersonIdent; @@ -49,7 +50,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; @@ -62,7 +63,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, @@ -103,8 +104,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); From 038b642cbee95c8e3213161c8c84f51c4525be35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Thu, 14 Aug 2014 14:57:05 -0400 Subject: [PATCH 04/10] Add missing call to ref-updated hook for submodule updates Add call to ref-updated hook when a project ref is updated because of submodule subscription. Bug: issue 2831 Change-Id: I176ea044fe4bbb2d5abc596b3da6dcd0a8d3e12f --- .../java/com/google/gerrit/server/git/MergeOp.java | 10 +++++++--- .../google/gerrit/server/git/ReceiveCommits.java | 3 ++- .../com/google/gerrit/server/git/SubmoduleOp.java | 12 ++++++++++-- .../google/gerrit/server/git/SubmoduleOpTest.java | 13 +++++++++---- 4 files changed, 28 insertions(+), 10 deletions(-) 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 d92400e02f..ac72c2e181 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 @@ -648,14 +648,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) { @@ -719,7 +722,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 625c757d20..0a8b198bfb 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 @@ -2258,7 +2258,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 5424887ee3..6cf6e25687 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 @@ -14,7 +14,9 @@ package com.google.gerrit.server.git; +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; @@ -67,7 +69,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); @@ -87,6 +89,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, @@ -96,7 +100,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; @@ -109,6 +114,8 @@ public class SubmoduleOp { this.myIdent = myIdent; this.repoManager = repoManager; this.gitRefUpdated = gitRefUpdated; + this.account = account; + this.changeHooks = changeHooks; updatedSubscribers = new HashSet(); } @@ -341,6 +348,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/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/git/SubmoduleOpTest.java index 33ba36e4c3..7dc6bf9fbf 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(); @@ -665,7 +669,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(); @@ -770,7 +775,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(); @@ -927,7 +932,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(); } From 7f039ce62570ae5ab1a27f31149149b476d39170 Mon Sep 17 00:00:00 2001 From: Gustaf Lundh Date: Thu, 21 Aug 2014 17:36:09 +0200 Subject: [PATCH 05/10] Support for CSharp syntax highlighting Bug: Issue 2848 Change-Id: Ibdc6a5c678c4340570d016d59545a40f6fabdf91 (cherry picked from commit 90459db79d9e284baf047ce7ad99241535470eaf) --- .../resources/com/google/gerrit/server/mime-types.properties | 1 + 1 file changed, 1 insertion(+) 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 aac1e5ade1..817790f1da 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 From ac3d692e6e17b885d441fcd0ed1f2f1889c7cb92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Fri, 22 Aug 2014 09:27:10 -0400 Subject: [PATCH 06/10] Fix mutual exclusivity of --delete and --submit review command options As per documentation, --delete --submit were supposed to be mutually exclusive but they were not. Add check to make sure those two options are not used at the same time. Change-Id: Ic63573b1e0a455fa879c88bddbdd4fcd05d7d595 --- .../java/com/google/gerrit/sshd/commands/ReviewCommand.java | 5 +++++ 1 file changed, 5 insertions(+) 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 7bc1aaa597..da29139484 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 @@ -162,6 +162,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) { From 8d869ea79360a5af1ae048ad1ad36726bddb851e Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Tue, 26 Aug 2014 14:09:53 +0900 Subject: [PATCH 07/10] Fix broken links to CommentRange entity in changes REST documentation Change-Id: I35f6ad50d8dbe73df93297b76f4511e484dc9c7e --- Documentation/rest-api-changes.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 1ce7d6cdf1..3385378801 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -2851,7 +2851,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. @@ -2892,7 +2892,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. From ddb825131b84734b4b1ba421570d3ebb2336df2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Mon, 25 Aug 2014 14:30:32 -0400 Subject: [PATCH 08/10] Update review command documentation for mutually exclusive options Review command documentation was not accurate regarding the mutually exclusive options. Change-Id: Ic5e7fa36a844258a448f37d42f041d350a0d6441 --- Documentation/cmd-review.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Documentation/cmd-review.txt b/Documentation/cmd-review.txt index 510edce7fa..40393be6f7 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). From a7ef14229dedc9105dc364e9d771cee093e8de6c Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Tue, 26 Aug 2014 14:58:48 -0700 Subject: [PATCH 09/10] Gracefully skip mergeability checking on broken changes If a change is missing its current patch set the mergeablity check will NPE. Skip the check and just return false, indicating there was no update made to the change. Change-Id: Ic82b2db1d2f7d27105a06c4082b51e7308f02b04 --- .../com/google/gerrit/server/change/MergeabilityChecker.java | 5 +++++ 1 file changed, 5 insertions(+) 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..1e8ed67818 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); From 7a4cdcd5f85c15996d065ed4860f01b1e6705756 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Tue, 26 Aug 2014 15:00:50 -0700 Subject: [PATCH 10/10] Clarify the failing change in MergeabilityChecker If an update check fails, it may be change specific. Record the change as part of the logged message. Change-Id: I33aba050572f919e150d8e0f1bd678f575301b72 --- .../google/gerrit/server/change/MergeabilityChecker.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 1e8ed67818..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 @@ -350,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);