From 3a20cefc4bbd025173f83489855bbf9348bea642 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 5 Feb 2014 12:15:41 -0800 Subject: [PATCH 1/6] PostReview: update and insert comments/approvals in a single step Conflicts: gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java Change-Id: Ieab4ac0628e397495bfd8a278c879f79817ba443 --- .../gerrit/server/change/PostReview.java | 42 ++++++++----------- 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index a6530e4803..2607de0e69 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java @@ -348,16 +348,14 @@ public class PostReview implements RestModifyView { } List del = Lists.newArrayList(); - List ins = Lists.newArrayList(); - List upd = Lists.newArrayList(); + List ups = Lists.newArrayList(); for (Map.Entry> ent : in.entrySet()) { String path = ent.getKey(); for (Comment c : ent.getValue()) { String parent = Url.decode(c.inReplyTo); PatchLineComment e = drafts.remove(Url.decode(c.id)); - boolean create = e == null; - if (create) { + if (e == null) { e = new PatchLineComment( new PatchLineComment.Key( new Patch.Key(rsrc.getPatchSet().getId(), path), @@ -373,7 +371,7 @@ public class PostReview implements RestModifyView { e.setSide(c.side == Side.PARENT ? (short) 0 : (short) 1); e.setMessage(c.message); e.setRange(c.range); - (create ? ins : upd).add(e); + ups.add(e); } } @@ -388,16 +386,14 @@ public class PostReview implements RestModifyView { for (PatchLineComment e : drafts.values()) { e.setStatus(PatchLineComment.Status.PUBLISHED); e.setWrittenOn(timestamp); - upd.add(e); + ups.add(e); } break; } db.get().patchComments().delete(del); - db.get().patchComments().insert(ins); - db.get().patchComments().update(upd); - comments.addAll(ins); - comments.addAll(upd); - return !del.isEmpty() || !ins.isEmpty() || !upd.isEmpty(); + db.get().patchComments().upsert(ups); + comments.addAll(ups); + return !del.isEmpty() || !ups.isEmpty(); } private Map scanDraftComments( @@ -418,8 +414,7 @@ public class PostReview implements RestModifyView { } List del = Lists.newArrayList(); - List ins = Lists.newArrayList(); - List upd = Lists.newArrayList(); + List ups = Lists.newArrayList(); Map current = scanLabels(rsrc, del); LabelTypes labelTypes = rsrc.getControl().getLabelTypes(); @@ -445,7 +440,7 @@ public class PostReview implements RestModifyView { c.setValue(ent.getValue()); c.setGranted(timestamp); c.cache(change); - upd.add(c); + ups.add(c); labelDelta.add(format(normName, c.getValue())); categories.put(normName, c.getValue()); } else if (c != null && c.getValue() == ent.getValue()) { @@ -458,23 +453,22 @@ public class PostReview implements RestModifyView { ent.getValue(), TimeUtil.nowTs()); c.setGranted(timestamp); c.cache(change); - ins.add(c); + ups.add(c); labelDelta.add(format(normName, c.getValue())); categories.put(normName, c.getValue()); } } - forceCallerAsReviewer(rsrc, current, ins, upd, del); + forceCallerAsReviewer(rsrc, current, ups, del); db.get().patchSetApprovals().delete(del); - db.get().patchSetApprovals().insert(ins); - db.get().patchSetApprovals().update(upd); - return !del.isEmpty() || !ins.isEmpty() || !upd.isEmpty(); + db.get().patchSetApprovals().upsert(ups); + return !del.isEmpty() || !ups.isEmpty(); } private void forceCallerAsReviewer(RevisionResource rsrc, - Map current, List ins, - List upd, List del) { - if (current.isEmpty() && ins.isEmpty() && upd.isEmpty()) { + Map current, List ups, + List del) { + if (current.isEmpty() && ups.isEmpty()) { // TODO Find another way to link reviewers to changes. if (del.isEmpty()) { // If no existing label is being set to 0, hack in the caller @@ -487,7 +481,7 @@ public class PostReview implements RestModifyView { (short) 0, TimeUtil.nowTs()); c.setGranted(timestamp); c.cache(change); - ins.add(c); + ups.add(c); } else { // Pick a random label that is about to be deleted and keep it. Iterator i = del.iterator(); @@ -496,7 +490,7 @@ public class PostReview implements RestModifyView { c.setGranted(timestamp); c.cache(change); i.remove(); - upd.add(c); + ups.add(c); } } } From fed49276d888801f81224e1b56220af44aa5096f Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Tue, 11 Mar 2014 10:42:58 +0900 Subject: [PATCH 2/6] Update 2.8.2 release notes Change-Id: Ia580893c2c1700af9b6eddc6ee9a7916e68aa551 --- ReleaseNotes/ReleaseNotes-2.8.2.txt | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ReleaseNotes/ReleaseNotes-2.8.2.txt b/ReleaseNotes/ReleaseNotes-2.8.2.txt index bf21946b09..f3f1741aa1 100644 --- a/ReleaseNotes/ReleaseNotes-2.8.2.txt +++ b/ReleaseNotes/ReleaseNotes-2.8.2.txt @@ -124,6 +124,12 @@ Handle null commits when updating submodules. In some edge cases it was possible that a null commit would exist, and this caused a crash when updating submodules. +* Update and insert comments/approvals in a single step. ++ +When a review includes both new label scores and updates to existing label +scores, use `upsert` to record them all at the same time, rather than in +separate `update` and `insert` operations. + * Remove dependency on joda time library in gerrit launcher. + The joda time library was being unnecessarily packaged in the root of From bf690c6c13643afaf44a608e6253330860cf026d Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Tue, 11 Mar 2014 10:35:15 +0900 Subject: [PATCH 3/6] Bump GERRIT_VERSION to 2.8.2 Change-Id: I24e6deb8755fda9d90181db6a35397236f6c789f --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index b5fed024a6..127a560c2f 100644 --- a/VERSION +++ b/VERSION @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = '2.8.1' +GERRIT_VERSION = '2.8.2' From cd6572d43516b8735e420332b803a319fe57a66c Mon Sep 17 00:00:00 2001 From: Bruce Zu Date: Tue, 21 Jan 2014 11:11:42 +0800 Subject: [PATCH 4/6] Prevent duplicate commits in same project when uploading to refs/changes/n Under certain circumstances, when pushing to 'refs/changes/n', the same commit can be pushed onto multiple changes even if they are on the same branch. As a result of this, if the commit sha1 is used as change identifier in the 'review' ssh command, it will fail with the error: 'fatal: matches multiple patch sets.' This commit performs the same check as when pushing to 'refs/for/branch' to prevent this, and will show "commit already exists (in the project)" error message to reject uploading a commit to an existing change via `refs/changes/n` if the commit was already successfully pushed to a change in project scope. Bug: issue 2374 Change-Id: I1c000b2cc3e155617cdc9c295a46ce2107ec47ca --- Documentation/error-commit-already-exists.txt | 17 +++++++++++++---- Documentation/error-no-new-changes.txt | 17 +++++++++-------- .../gerrit/server/git/ReceiveCommits.java | 14 +++++++++++--- 3 files changed, 33 insertions(+), 15 deletions(-) diff --git a/Documentation/error-commit-already-exists.txt b/Documentation/error-commit-already-exists.txt index dc32c4c1a1..78ce5f2a20 100644 --- a/Documentation/error-commit-already-exists.txt +++ b/Documentation/error-commit-already-exists.txt @@ -1,10 +1,19 @@ commit already exists ===================== -With this error message Gerrit rejects to push a commit to an -existing change via `refs/changes/n` if the commit was already -successfully pushed to the change. In this case there is no -new commit and consequently there is nothing for Gerrit to do. +With "commit already exists (as current patchset)" or +"commit already exists (in the change)" error message +Gerrit rejects to push a commit to an existing change via +`refs/changes/n` if the commit was already successfully +pushed to the change. + +With "commit already exists (in the project)" error message +Gerrit rejects to push a commit to an existing change via +`refs/changes/n` if the commit was already successfully +pushed to a change in project scope. + +In any above case there is no new commit and consequently +there is nothing for Gerrit to do. For further information about how to resolve this error, please refer to link:error-no-new-changes.html[no new changes]. diff --git a/Documentation/error-no-new-changes.txt b/Documentation/error-no-new-changes.txt index 8e409efa55..a421e1aa85 100644 --- a/Documentation/error-no-new-changes.txt +++ b/Documentation/error-no-new-changes.txt @@ -2,8 +2,9 @@ no new changes ============== With this error message Gerrit rejects to push a commit if the pushed -commit was already successfully pushed to Gerrit. In this case there -is no new change and consequently there is nothing for Gerrit to do. +commit was already successfully pushed to Gerrit in project scope. +In this case there is no new change and consequently there is nothing +for Gerrit to do. If your push is failing with this error message, you normally don't have to do anything since the commit was already successfully @@ -31,7 +32,7 @@ means: . you cannot reset a change to an old patch set by pushing the old commit for this change again . if a commit was pushed to one branch you cannot push this commit - to another branch + to another branch in project scope. . if a commit was pushed directly to a branch (without going through code review) you cannot push this commit once again for code review (please note that in this case searching by the commit ID @@ -39,11 +40,11 @@ means: If you need to re-push a commit you may rewrite this commit by link:http://www.kernel.org/pub/software/scm/git/docs/git-commit.html[amending] it or doing an interactive link:http://www.kernel.org/pub/software/scm/git/docs/git-rebase.html[git rebase]. By rewriting the -commit you actually create a new commit (with a new commit ID) which -can then be pushed to Gerrit. If the old commit contains a Change-Id -in the commit message you also need to replace it with a new -Change-Id (case 1. and 3. above), otherwise the push will fail with -another error message. +commit you actually create a new commit (with a new commit ID in +project scope) which can then be pushed to Gerrit. If the old commit +contains a Change-Id in the commit message you also need to replace +it with a new Change-Id (case 1. and 3. above), otherwise the push +will fail with another error message. GERRIT 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 b323b95c06..b2b810eec9 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 @@ -17,7 +17,6 @@ package com.google.gerrit.server.git; import static com.google.gerrit.server.git.MultiProgressMonitor.UNKNOWN; import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromApprovals; import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromFooters; - import static org.eclipse.jgit.lib.Constants.R_HEADS; import static org.eclipse.jgit.lib.RefDatabase.ALL; import static org.eclipse.jgit.transport.ReceiveCommand.Result.NOT_ATTEMPTED; @@ -128,9 +127,9 @@ import org.eclipse.jgit.transport.AdvertiseRefsHook; import org.eclipse.jgit.transport.AdvertiseRefsHookChain; import org.eclipse.jgit.transport.BaseReceivePack; import org.eclipse.jgit.transport.ReceiveCommand; -import org.eclipse.jgit.transport.RefFilter; import org.eclipse.jgit.transport.ReceiveCommand.Result; import org.eclipse.jgit.transport.ReceivePack; +import org.eclipse.jgit.transport.RefFilter; import org.eclipse.jgit.transport.ServiceMayNotContinueException; import org.eclipse.jgit.transport.UploadPack; import org.kohsuke.args4j.CmdLineException; @@ -1694,6 +1693,7 @@ public class ReceiveCommits { if (newCommit == priorCommit) { // Ignore requests to make the change its current state. skip = true; + reject(inputCommand, "commit already exists (as current patchset)"); return false; } @@ -1705,10 +1705,18 @@ public class ReceiveCommits { reject(inputCommand, "change " + ontoChange + " closed"); return false; } else if (revisions.containsKey(newCommit)) { - reject(inputCommand, "commit already exists"); + reject(inputCommand, "commit already exists (in the change)"); return false; } + for (final Ref r : rp.getRepository().getRefDatabase() + .getRefs("refs/changes").values()) { + if (r.getObjectId().equals(inputCommand.getNewId())) { + reject(inputCommand, "commit already exists (in the project)"); + return false; + } + } + for (RevCommit prior : revisions.keySet()) { // Don't allow a change to directly depend upon itself. This is a // very common error due to users making a new commit rather than From bd867ae0c3aee51908b243ab53afb2cf54549aca Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Tue, 11 Mar 2014 10:37:07 +0900 Subject: [PATCH 5/6] Bump version to 2.8.2 in plugin API and archetypes Change-Id: Iaa71fe340597703b2effd60369aecd09335f4d3e --- gerrit-plugin-archetype/pom.xml | 2 +- gerrit-plugin-gwt-archetype/pom.xml | 2 +- gerrit-plugin-gwtui/pom.xml | 2 +- gerrit-plugin-js-archetype/pom.xml | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/gerrit-plugin-archetype/pom.xml b/gerrit-plugin-archetype/pom.xml index 6d2a8441df..6d94f1cc28 100644 --- a/gerrit-plugin-archetype/pom.xml +++ b/gerrit-plugin-archetype/pom.xml @@ -20,7 +20,7 @@ limitations under the License. com.google.gerrit gerrit-plugin-archetype - 2.8.1 + 2.8.2 Gerrit Code Review - Plugin Archetype diff --git a/gerrit-plugin-gwt-archetype/pom.xml b/gerrit-plugin-gwt-archetype/pom.xml index 8240a7320c..8af6287baf 100644 --- a/gerrit-plugin-gwt-archetype/pom.xml +++ b/gerrit-plugin-gwt-archetype/pom.xml @@ -20,7 +20,7 @@ limitations under the License. com.google.gerrit gerrit-plugin-gwt-archetype - 2.8.1 + 2.8.2 Gerrit Code Review - Web Ui GWT Plugin Archetype diff --git a/gerrit-plugin-gwtui/pom.xml b/gerrit-plugin-gwtui/pom.xml index 686466d722..dddb53bd62 100644 --- a/gerrit-plugin-gwtui/pom.xml +++ b/gerrit-plugin-gwtui/pom.xml @@ -21,7 +21,7 @@ limitations under the License. com.google.gerrit gerrit-plugin-gwtui - 2.8.1 + 2.8.2 Gerrit Code Review - Plugin GWT UI diff --git a/gerrit-plugin-js-archetype/pom.xml b/gerrit-plugin-js-archetype/pom.xml index c2fe7aa3a7..d5196d9bfb 100644 --- a/gerrit-plugin-js-archetype/pom.xml +++ b/gerrit-plugin-js-archetype/pom.xml @@ -20,7 +20,7 @@ limitations under the License. com.google.gerrit gerrit-plugin-js-archetype - 2.8.1 + 2.8.2 Gerrit Code Review - Web UI JavaScript Plugin Archetype From 2115a69e426cceed2b9862d61498126487663390 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Tue, 11 Mar 2014 17:57:49 +0900 Subject: [PATCH 6/6] Update 2.8.2 release notes Change-Id: I120587908d05e61a87c6058a481c6085f9586c3e --- ReleaseNotes/ReleaseNotes-2.8.2.txt | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ReleaseNotes/ReleaseNotes-2.8.2.txt b/ReleaseNotes/ReleaseNotes-2.8.2.txt index f3f1741aa1..98a4ea7304 100644 --- a/ReleaseNotes/ReleaseNotes-2.8.2.txt +++ b/ReleaseNotes/ReleaseNotes-2.8.2.txt @@ -130,6 +130,13 @@ When a review includes both new label scores and updates to existing label scores, use `upsert` to record them all at the same time, rather than in separate `update` and `insert` operations. +* link:https://code.google.com/p/gerrit/issues/detail?id=2374[Issue 2374]: +Prevent duplicate commits in same project when uploading to `refs/changes/n`. ++ +Under certain circumstances, when pushing to `refs/changes/n`, the same +commit could be pushed onto multiple changes even if the changes were on the +same branch. + * Remove dependency on joda time library in gerrit launcher. + The joda time library was being unnecessarily packaged in the root of