From fecf4eaf49ea58cfee2d62d2c2b1f4b672c81b1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Wed, 25 May 2016 23:45:23 -0400 Subject: [PATCH 01/16] Upgrade replication plugin to latest revision * Double check if a ref is missing locally before deleting from remote Change-Id: I6b36dd1e2b547c9cf67a051674a7f5a10814b8f9 --- plugins/replication | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/replication b/plugins/replication index b4584ad311..7cbcfd2a3a 160000 --- a/plugins/replication +++ b/plugins/replication @@ -1 +1 @@ -Subproject commit b4584ad31125a13649de9a953bd931364fd5ae0b +Subproject commit 7cbcfd2a3a507472eb244cf1a773b177d291d253 From 534cde9323c0bfc1d3638a1c356bfb5dba21ca8f Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Fri, 27 May 2016 09:54:09 +0900 Subject: [PATCH 02/16] Eclipse: Update compiler warnings settings from Eclipse MARS.2 New settings have been added. Leave them all as-is except unusedExceptionParameter, which causes numerous warnings. Set it to "Ignore". Change-Id: Iee480d925fa384e2e7db82711acb2efd8d10583f --- .settings/org.eclipse.jdt.core.prefs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.settings/org.eclipse.jdt.core.prefs b/.settings/org.eclipse.jdt.core.prefs index 68976e7461..8f5678f493 100644 --- a/.settings/org.eclipse.jdt.core.prefs +++ b/.settings/org.eclipse.jdt.core.prefs @@ -5,10 +5,17 @@ org.eclipse.jdt.core.compiler.annotation.nonnull=org.eclipse.jdt.annotation.NonN org.eclipse.jdt.core.compiler.annotation.nonnullbydefault=org.eclipse.jdt.annotation.NonNullByDefault org.eclipse.jdt.core.compiler.annotation.nullable=org.eclipse.jdt.annotation.Nullable org.eclipse.jdt.core.compiler.annotation.nullanalysis=disabled +org.eclipse.jdt.core.compiler.codegen.inlineJsrBytecode=enabled +org.eclipse.jdt.core.compiler.codegen.methodParameters=do not generate org.eclipse.jdt.core.compiler.codegen.targetPlatform=1.7 +org.eclipse.jdt.core.compiler.codegen.unusedLocal=preserve org.eclipse.jdt.core.compiler.compliance=1.7 +org.eclipse.jdt.core.compiler.debug.lineNumber=generate +org.eclipse.jdt.core.compiler.debug.localVariable=generate +org.eclipse.jdt.core.compiler.debug.sourceFile=generate org.eclipse.jdt.core.compiler.doc.comment.support=enabled org.eclipse.jdt.core.compiler.problem.annotationSuperInterface=ignore +org.eclipse.jdt.core.compiler.problem.assertIdentifier=error org.eclipse.jdt.core.compiler.problem.autoboxing=ignore org.eclipse.jdt.core.compiler.problem.comparingIdentical=warning org.eclipse.jdt.core.compiler.problem.deadCode=warning @@ -17,6 +24,7 @@ org.eclipse.jdt.core.compiler.problem.deprecationInDeprecatedCode=disabled org.eclipse.jdt.core.compiler.problem.deprecationWhenOverridingDeprecatedMethod=disabled org.eclipse.jdt.core.compiler.problem.discouragedReference=warning org.eclipse.jdt.core.compiler.problem.emptyStatement=warning +org.eclipse.jdt.core.compiler.problem.enumIdentifier=error org.eclipse.jdt.core.compiler.problem.explicitlyClosedAutoCloseable=warning org.eclipse.jdt.core.compiler.problem.fallthroughCase=warning org.eclipse.jdt.core.compiler.problem.fatalOptionalError=disabled @@ -91,6 +99,7 @@ org.eclipse.jdt.core.compiler.problem.unusedDeclaredThrownException=warning org.eclipse.jdt.core.compiler.problem.unusedDeclaredThrownExceptionExemptExceptionAndThrowable=enabled org.eclipse.jdt.core.compiler.problem.unusedDeclaredThrownExceptionIncludeDocCommentReference=enabled org.eclipse.jdt.core.compiler.problem.unusedDeclaredThrownExceptionWhenOverriding=disabled +org.eclipse.jdt.core.compiler.problem.unusedExceptionParameter=ignore org.eclipse.jdt.core.compiler.problem.unusedImport=warning org.eclipse.jdt.core.compiler.problem.unusedLabel=warning org.eclipse.jdt.core.compiler.problem.unusedLocal=warning From 4a127f395ef33db92234ee86387c1cfed780d5cc Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Fri, 27 May 2016 10:01:06 +0900 Subject: [PATCH 03/16] AbstractSubmit: Add stricter assertions about ref-updated event content Instead of storing only the value of the new ref, store the entire ref update object that contains both the old ref and new ref values. Add assertions to verify that both the old ref and new ref values are set, and that they differ. Change-Id: I72945a837103ecbadcbaae6795ce3c40bd45a9a6 --- .../acceptance/rest/change/AbstractSubmit.java | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java index b9fb3937f7..25af0e1933 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java @@ -86,7 +86,7 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { } private Map mergeResults; - private Map refUpdatedEvents; + private Map refUpdatedEvents; @Inject private ChangeNotes.Factory notesFactory; @@ -116,7 +116,7 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { } else if (event instanceof RefUpdatedEvent) { RefUpdatedEvent e = (RefUpdatedEvent) event; RefUpdateAttribute r = e.refUpdate; - refUpdatedEvents.put(r.project + "-" + r.refName, r.newRev); + refUpdatedEvents.put(r.project + "-" + r.refName, r); } } @@ -252,9 +252,13 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { String newRev = mergeResults.get(Integer.toString(change._number)); assertThat(newRev).isNotNull(); assertThat(branch.revision).isEqualTo(newRev); - newRev = refUpdatedEvents.get(change.project + "-" + branch.ref); - assertThat(newRev).isNotNull(); - assertThat(branch.revision).isEqualTo(newRev); + RefUpdateAttribute refUpdate = + refUpdatedEvents.get(change.project + "-" + branch.ref); + assertThat(refUpdate).isNotNull(); + assertThat(refUpdate.newRev).isNotNull(); + assertThat(refUpdate.oldRev).isNotNull(); + assertThat(refUpdate.newRev).isNotEqualTo(refUpdate.oldRev); + assertThat(branch.revision).isEqualTo(refUpdate.newRev); } b.consume(); } From f4ad79e60f8c99aa20b3bcffab16cea14b319773 Mon Sep 17 00:00:00 2001 From: Mike Frysinger Date: Fri, 27 May 2016 17:35:50 -0400 Subject: [PATCH 04/16] Submit: fix typo in user message Change-Id: Ie979be8bd61a0971f37e3c4179f1fc7288353975 --- .../com/google/gerrit/acceptance/rest/change/ActionsIT.java | 2 +- .../src/main/java/com/google/gerrit/server/change/Submit.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java index e26747b580..ddab184183 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java @@ -103,7 +103,7 @@ public class ActionsIT extends AbstractDaemonTest { assertThat(info.label).isEqualTo("Submit whole topic"); assertThat(info.method).isEqualTo("POST"); assertThat(info.title).isEqualTo( - "See the \"Submitted Together\" tab for problems, specially see: 2"); + "See the \"Submitted Together\" tab for problems, specifically see: 2"); } else { noSubmitWholeTopicAssertions(actions, 1); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java index d63b0804c3..2a3b7c199c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java @@ -96,7 +96,7 @@ public class Submit implements RestModifyView, private static final String CLICK_FAILURE_TOOLTIP = "Clicking the button would fail"; private static final String CHANGES_NOT_MERGEABLE = - "See the \"Submitted Together\" tab for problems, specially see: "; + "See the \"Submitted Together\" tab for problems, specifically see: "; public static class Output { transient Change change; From 1de95f4de9fb4bbe3babdee500e1d6e5d5d918ea Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Mon, 30 May 2016 22:44:48 +0200 Subject: [PATCH 05/16] Bazel: Fix MisusedWeekYear error detected by ErrorProne Building Gerrit code with Bazel revealed MisusedWeekYear error detected by ErrorProne: [1]. [1] http://errorprone.info/bugpattern/MisusedWeekYear Change-Id: I95edf77a1922840e9e37a6125ba7d094be297516 --- .../test/java/com/google/gerrit/gpg/PublicKeyCheckerTest.java | 4 ++-- .../com/google/gerrit/gpg/PushCertificateCheckerTest.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PublicKeyCheckerTest.java b/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PublicKeyCheckerTest.java index 742bf1aa8b..99e96a26d8 100644 --- a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PublicKeyCheckerTest.java +++ b/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PublicKeyCheckerTest.java @@ -229,7 +229,7 @@ public class PublicKeyCheckerTest { "Key is revoked (key material has been compromised): test6 compromised"; assertProblems(k, problem); - SimpleDateFormat df = new SimpleDateFormat("YYYY-MM-dd HH:mm:ss"); + SimpleDateFormat df = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss"); PublicKeyChecker checker = new PublicKeyChecker() .setStore(store) .setEffectiveTime(df.parse("2010-01-01 12:00:00")); @@ -385,7 +385,7 @@ public class PublicKeyCheckerTest { } private static Date parseDate(String str) throws Exception { - return new SimpleDateFormat("YYYY-MM-dd HH:mm:ss Z").parse(str); + return new SimpleDateFormat("yyyy-MM-dd HH:mm:ss Z").parse(str); } private static List list(String first, String[] rest) { diff --git a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PushCertificateCheckerTest.java b/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PushCertificateCheckerTest.java index ee07d5580d..12a911e5bb 100644 --- a/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PushCertificateCheckerTest.java +++ b/gerrit-gpg/src/test/java/com/google/gerrit/gpg/PushCertificateCheckerTest.java @@ -139,7 +139,7 @@ public class PushCertificateCheckerTest { @Test public void signatureByExpiredKeyBeforeExpiration() throws Exception { TestKey key3 = expiredKey(); - Date now = new SimpleDateFormat("YYYY-MM-dd HH:mm:ss Z") + Date now = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss Z") .parse("2005-07-10 12:00:00 -0400"); PushCertificate cert = newSignedCert(validNonce(), key3, now); assertNoProblems(cert); From f0b5de52f770d1c526b1d29b0e69a0b3c45d8e9d Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 30 May 2016 16:57:02 +0200 Subject: [PATCH 06/16] ACL screen: Show error when group to be added is not found At the moment trying to add a non-existing group for a permission provides no user feedback. The "Working" label is shortly shown, but the user is not told that the entered group doesn't exist. Change-Id: Ifa600fea649298066864deabc0e1e98782e5a8ca Signed-off-by: Edwin Kempin --- .../com/google/gerrit/client/admin/PermissionEditor.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionEditor.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionEditor.java index b9baccc76c..7678097066 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionEditor.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionEditor.java @@ -14,6 +14,8 @@ package com.google.gerrit.client.admin; +import com.google.gerrit.client.ErrorDialog; +import com.google.gerrit.client.Gerrit; import com.google.gerrit.client.rpc.GerritCallback; import com.google.gerrit.client.ui.SuggestUtil; import com.google.gerrit.common.data.AccessSection; @@ -219,7 +221,7 @@ public class PermissionEditor extends Composite implements Editor, addStage2.getStyle().setDisplay(Display.NONE); } - private void addGroup(GroupReference ref) { + private void addGroup(final GroupReference ref) { if (ref.getUUID() != null) { if (value.getRule(ref) == null) { PermissionRule newRule = value.getRule(ref, true); @@ -251,6 +253,8 @@ public class PermissionEditor extends Composite implements Editor, addGroup(result.get(0)); } else { groupToAdd.setFocus(true); + new ErrorDialog(Gerrit.M.noSuchGroupMessage(ref.getName())) + .center(); } } From b5989ec838f20afe6cc4239fa72dbc55a0f7061a Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Tue, 31 May 2016 23:39:54 +0200 Subject: [PATCH 07/16] Bazel: Fix CheckReturnValue error detected by ErrorProne Building Gerrit code with Bazel revealed CheckReturnValue error detected by ErrorProne: [1]. [1] http://errorprone.info/bugpattern/CheckReturnValue Change-Id: I00dc1e3dfa7f135890d0e5f862c7498fda11ed1c --- .../google/gwtexpui/safehtml/client/SafeHtmlBuilderTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gerrit-gwtexpui/src/test/java/com/google/gwtexpui/safehtml/client/SafeHtmlBuilderTest.java b/gerrit-gwtexpui/src/test/java/com/google/gwtexpui/safehtml/client/SafeHtmlBuilderTest.java index 9c9cf060f1..086271173e 100644 --- a/gerrit-gwtexpui/src/test/java/com/google/gwtexpui/safehtml/client/SafeHtmlBuilderTest.java +++ b/gerrit-gwtexpui/src/test/java/com/google/gwtexpui/safehtml/client/SafeHtmlBuilderTest.java @@ -61,7 +61,7 @@ public class SafeHtmlBuilderTest { final SafeHtmlBuilder b = new SafeHtmlBuilder(); assertThat(b).isSameAs(b.append('a')); assertThat(b).isSameAs(b.append('b')); - assertThat("ab"); + assertThat(b.asString()).isEqualTo("ab"); } @Test From 43fe38183b99b0104ed0e40ed5c5e548305b113b Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Tue, 31 May 2016 14:35:36 -0700 Subject: [PATCH 08/16] Submit: Move check for enabling button after rechecking mergeability This moves the check for mergeability after we refresh caches from all caches related to this change, such that we do not need to recompute the mergeable ourselves. Document it though. Change-Id: Ic37d94aefa24b73fda2c0e599b6109c1475b83b2 Signed-off-by: Stefan Beller --- .../google/gerrit/server/change/Submit.java | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java index 2a3b7c199c..bda348dbce 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java @@ -310,13 +310,6 @@ public class Submit implements RestModifyView, .setVisible(false); } - Boolean enabled; - try { - enabled = cd.isMergeable(); - } catch (OrmException e) { - throw new OrmRuntimeException("Could not determine mergeability", e); - } - ChangeSet cs; try { cs = mergeSuperSet.completeChangeSet(db, cd.change()); @@ -335,6 +328,22 @@ public class Submit implements RestModifyView, String submitProblems = problemsForSubmittingChangeset(cs, resource.getUser()); + + Boolean enabled; + try { + // Recheck mergeability rather than using value stored in the index, + // which may be stale. + // TODO(dborowitz): This is ugly; consider providing a way to not read + // stored fields from the index in the first place. + // cd.setMergeable(null); + // That was done in unmergeableChanges which was called by + // problemsForSubmittingChangeset, so now it is safe to read from + // the cache, as it yields the same result. + enabled = cd.isMergeable(); + } catch (OrmException e) { + throw new OrmRuntimeException("Could not determine mergeability", e); + } + if (submitProblems != null) { return new UiAction.Description() .setLabel(treatWithTopic From 4b989b977e4168769fc6e7a0da762fbafa95d647 Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Tue, 31 May 2016 14:43:36 -0700 Subject: [PATCH 09/16] Submit: Give another error message when the change itself has problems Andrew reported that he saw the disabled submit button and the hover text saying "See the 'Submitted Together' tab for problems specially see 78110" while looking at 78110 already and it confused him. This introduces a custom error message when the error message is generated by the change that the user is looking at. The root cause, which is inconsistency with other UI elements ("Ready to submit") is not fixed in this change. This will be less confusing as listing the same change in a list of changes though. Change-Id: I585a243f04dc19a6ea0247baa08b63dca09449f1 --- .../com/google/gerrit/server/change/Submit.java | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java index bda348dbce..65c9dff259 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java @@ -95,6 +95,8 @@ public class Submit implements RestModifyView, "This change depends on other hidden changes which are not ready"; private static final String CLICK_FAILURE_TOOLTIP = "Clicking the button would fail"; + private static final String CHANGE_UNMERGEABLE = + "Problems with integrating this change"; private static final String CHANGES_NOT_MERGEABLE = "See the \"Submitted Together\" tab for problems, specifically see: "; @@ -222,12 +224,13 @@ public class Submit implements RestModifyView, } /** + * @param cd the change the user is currently looking at * @param cs set of changes to be submitted at once * @param identifiedUser the user who is checking to submit * @return a reason why any of the changes is not submittable or null */ - private String problemsForSubmittingChangeset( - ChangeSet cs, IdentifiedUser identifiedUser) { + private String problemsForSubmittingChangeset(ChangeData cd, ChangeSet cs, + IdentifiedUser identifiedUser) { try { @SuppressWarnings("resource") ReviewDb db = dbProvider.get(); @@ -249,6 +252,11 @@ public class Submit implements RestModifyView, if (unmergeable == null) { return CLICK_FAILURE_TOOLTIP; } else if (!unmergeable.isEmpty()) { + for (ChangeData c : unmergeable) { + if (c.change().getKey().equals(cd.change().getKey())) { + return CHANGE_UNMERGEABLE; + } + } return CHANGES_NOT_MERGEABLE + Joiner.on(", ").join( Iterables.transform(unmergeable, new Function() { @@ -326,8 +334,8 @@ public class Submit implements RestModifyView, && !Strings.isNullOrEmpty(topic) && topicSize > 1; - String submitProblems = problemsForSubmittingChangeset(cs, - resource.getUser()); + String submitProblems = + problemsForSubmittingChangeset(cd, cs, resource.getUser()); Boolean enabled; try { From 6c53c794d56863e2172b1f159c2ecdf1317b70ec Mon Sep 17 00:00:00 2001 From: Stefan Beller Date: Tue, 31 May 2016 14:52:49 -0700 Subject: [PATCH 10/16] Submit: Don't assume the "Submitted Together" Tab exists. Change-Id: I85b5198d9b46fb42624a7ac6f581438078e2fcc0 --- .../com/google/gerrit/acceptance/rest/change/ActionsIT.java | 3 +-- .../src/main/java/com/google/gerrit/server/change/Submit.java | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java index ddab184183..ba98963a17 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java @@ -102,8 +102,7 @@ public class ActionsIT extends AbstractDaemonTest { assertThat(info.enabled).isNull(); assertThat(info.label).isEqualTo("Submit whole topic"); assertThat(info.method).isEqualTo("POST"); - assertThat(info.title).isEqualTo( - "See the \"Submitted Together\" tab for problems, specifically see: 2"); + assertThat(info.title).isEqualTo("Problems with change(s): 2"); } else { noSubmitWholeTopicAssertions(actions, 1); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java index 65c9dff259..22bdae506f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java @@ -98,7 +98,7 @@ public class Submit implements RestModifyView, private static final String CHANGE_UNMERGEABLE = "Problems with integrating this change"; private static final String CHANGES_NOT_MERGEABLE = - "See the \"Submitted Together\" tab for problems, specifically see: "; + "Problems with change(s): "; public static class Output { transient Change change; From a0d78e524d09d3d7919f989dbc731ec6f4ace8df Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Wed, 1 Jun 2016 09:10:07 +0200 Subject: [PATCH 11/16] Bazel: Fix CheckReturnValue error detected by ErrorProne Change-Id: I479bd380b77ff84dba43739b0e5f2b58b4c25a9c --- .../java/com/google/gerrit/acceptance/api/group/GroupsIT.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java index c72edd71dc..8d1d89f7e7 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/group/GroupsIT.java @@ -498,7 +498,7 @@ public class GroupsIT extends AbstractDaemonTest { } private void assertNoMembers(String group) throws Exception { - assertThat(gApi.groups().id(group).members().isEmpty()); + assertThat(gApi.groups().id(group).members()).isEmpty(); } private void assertIncludes(String group, String... expectedNames) @@ -521,7 +521,7 @@ public class GroupsIT extends AbstractDaemonTest { } private void assertNoIncludes(String group) throws Exception { - assertThat(gApi.groups().id(group).includedGroups().isEmpty()); + assertThat(gApi.groups().id(group).includedGroups()).isEmpty(); } private AccountGroup getFromCache(String name) throws Exception { From c0f406782803c377b071a000b1f7be80681b37e2 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 1 Jun 2016 17:21:51 +0200 Subject: [PATCH 12/16] Do not keep reference to non-singleton ListTags in singleton TagsCollection Due to this options set on previous requests are applied for follow-up requests that do not specify these options. Bug: Issue 4155 Change-Id: I06ef38be480ff60cba21901978bcc63bdbfaaad8 Signed-off-by: Edwin Kempin --- .../com/google/gerrit/server/project/TagsCollection.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/TagsCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/TagsCollection.java index 0c70285de8..1b6b8880ae 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/TagsCollection.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/TagsCollection.java @@ -20,6 +20,7 @@ import com.google.gerrit.extensions.restapi.IdString; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestView; import com.google.inject.Inject; +import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; @@ -28,24 +29,24 @@ import java.io.IOException; public class TagsCollection implements ChildCollection { private final DynamicMap> views; - private final ListTags list; + private final Provider list; @Inject public TagsCollection(DynamicMap> views, - ListTags list) { + Provider list) { this.views = views; this.list = list; } @Override public RestView list() throws ResourceNotFoundException { - return list; + return list.get(); } @Override public TagResource parse(ProjectResource resource, IdString id) throws ResourceNotFoundException, IOException { - return new TagResource(resource.getControl(), list.get(resource, id)); + return new TagResource(resource.getControl(), list.get().get(resource, id)); } @Override From 929f7bb7a9c566210d5e1ba54ecc423aa27f5ff8 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Fri, 3 Jun 2016 09:02:44 +0900 Subject: [PATCH 13/16] ChangeData: Prevent possible NPE when loading current patch set Since change I7aa0ae0fa, Change.Id.fromEditRefPart can return null, but ChangeData#ensureCurrentPatchSetLoaded dereferences its return value without any check. Bug: Issue 4112 Change-Id: I750b803e38de8638255c3aa174670531db348f75 --- .../com/google/gerrit/server/query/change/ChangeData.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index 8ef888cd9d..0618db9216 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.query.change; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.gerrit.server.ApprovalsUtil.sortApprovals; import com.google.auto.value.AutoValue; @@ -802,11 +803,11 @@ public class ChangeData { return Collections.emptySet(); } editsByUser = new HashSet<>(); - Change.Id id = change.getId(); + Change.Id id = checkNotNull(change.getId()); try (Repository repo = repoManager.openRepository(change.getProject())) { for (String ref : repo.getRefDatabase().getRefs(RefNames.REFS_USERS).keySet()) { - if (Change.Id.fromEditRefPart(ref).equals(id)) { + if (id.equals(Change.Id.fromEditRefPart(ref))) { editsByUser.add(Account.Id.fromRefPart(ref)); } } From 65357a13adda4523f2f01ec6ad8d6731aeadd260 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 8 Jun 2016 11:26:24 +0900 Subject: [PATCH 14/16] AbstractSubmit: Refactor ref-updated event tests to be more precise In the current implementation we only store the latest ref-updated event, and are only able to assert that at least one was received. Refactor it to store all the received events. Update the submit by fast forward test to explicitly assert that there was only one event received and it contained the expected old and new revs. Change-Id: I3d6bb10b8807a13208d87627550218915019f08e --- .../rest/change/AbstractSubmit.java | 23 ++++++++++--------- .../rest/change/SubmitByFastForwardIT.java | 19 +++++++++++---- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java index 25af0e1933..bee9091931 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java @@ -23,9 +23,11 @@ import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVI import static com.google.gerrit.extensions.client.ListChangesOption.DETAILED_LABELS; import com.google.common.base.Strings; +import com.google.common.collect.HashMultimap; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import com.google.common.collect.Multimap; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.RestResponse; @@ -76,6 +78,7 @@ import org.junit.Test; import java.io.ByteArrayOutputStream; import java.io.IOException; +import java.util.Collection; import java.util.List; import java.util.Map; @@ -86,7 +89,7 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { } private Map mergeResults; - private Map refUpdatedEvents; + protected Multimap refUpdatedEvents; @Inject private ChangeNotes.Factory notesFactory; @@ -103,7 +106,7 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { @Before public void setUp() throws Exception { mergeResults = Maps.newHashMap(); - refUpdatedEvents = Maps.newHashMap(); + refUpdatedEvents = HashMultimap.create(); CurrentUser listenerUser = factory.create(user.id); source.addEventListener(new EventListener() { @@ -239,7 +242,7 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { private void checkMergeResult(ChangeInfo change) throws IOException { // Get the revision of the branch after the submit to compare with the - // newRev of the ChangeMergedEvent and RefUpdatedEvent. + // newRev of the ChangeMergedEvent. RestResponse b = adminSession.get("/projects/" + change.project + "/branches/" + change.branch); @@ -248,17 +251,9 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { newGson().fromJson(b.getReader(), new TypeToken() {}.getType()); assertThat(mergeResults).isNotEmpty(); - assertThat(refUpdatedEvents).isNotEmpty(); String newRev = mergeResults.get(Integer.toString(change._number)); assertThat(newRev).isNotNull(); assertThat(branch.revision).isEqualTo(newRev); - RefUpdateAttribute refUpdate = - refUpdatedEvents.get(change.project + "-" + branch.ref); - assertThat(refUpdate).isNotNull(); - assertThat(refUpdate.newRev).isNotNull(); - assertThat(refUpdate.oldRev).isNotNull(); - assertThat(refUpdate.newRev).isNotEqualTo(refUpdate.oldRev); - assertThat(branch.revision).isEqualTo(refUpdate.newRev); } b.consume(); } @@ -380,6 +375,12 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { return getRemoteLog(project, "master"); } + protected RefUpdateAttribute getOneRefUpdate(String key) { + Collection refUpdates = refUpdatedEvents.get(key); + assertThat(refUpdates).hasSize(1); + return refUpdates.iterator().next(); + } + private RevCommit getHead(Repository repo, String name) throws IOException { try (RevWalk rw = new RevWalk(repo)) { return rw.parseCommit(repo.getRef(name).getObjectId()); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java index a87b7d911f..374fb13edb 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByFastForwardIT.java @@ -20,6 +20,7 @@ import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.common.ActionInfo; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.server.data.RefUpdateAttribute; import org.eclipse.jgit.revwalk.RevCommit; import org.junit.Test; @@ -46,6 +47,8 @@ public class SubmitByFastForwardIT extends AbstractSubmit { @Test public void submitTwoChangesWithFastForward() throws Exception { + RevCommit originalHead = getRemoteHead(); + PushOneCommit.Result change = createChange(); PushOneCommit.Result change2 = createChange(); @@ -54,15 +57,21 @@ public class SubmitByFastForwardIT extends AbstractSubmit { approve(id1); submit(id2); - RevCommit head = getRemoteHead(); - assertThat(head.getId()).isEqualTo(change2.getCommitId()); - assertThat(head.getParent(0).getId()).isEqualTo(change.getCommitId()); + RevCommit updatedHead = getRemoteHead(); + assertThat(updatedHead.getId()).isEqualTo(change2.getCommit()); + assertThat(updatedHead.getParent(0).getId()).isEqualTo(change.getCommit()); assertSubmitter(change.getChangeId(), 1); assertSubmitter(change2.getChangeId(), 1); - assertPersonEquals(admin.getIdent(), head.getAuthorIdent()); - assertPersonEquals(admin.getIdent(), head.getCommitterIdent()); + assertPersonEquals(admin.getIdent(), updatedHead.getAuthorIdent()); + assertPersonEquals(admin.getIdent(), updatedHead.getCommitterIdent()); assertSubmittedTogether(id1, id2, id1); assertSubmittedTogether(id2, id2, id1); + + RefUpdateAttribute refUpdate = getOneRefUpdate( + project.get() + "-refs/heads/master"); + assertThat(refUpdate).isNotNull(); + assertThat(refUpdate.oldRev).isEqualTo(originalHead.name()); + assertThat(refUpdate.newRev).isEqualTo(updatedHead.name()); } @Test From e2f906ca114ee95d00439e2a6b632a1309d1c909 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Pedersen?= Date: Wed, 8 Jun 2016 17:15:21 +0200 Subject: [PATCH 15/16] Fix gitweb backlinks Use the correct path syntax. Bug: Issue 4176 Change-Id: Ia498e8d51c131875d809a43d446c2f6be1dc894e --- .../java/com/google/gerrit/httpd/gitweb/GitwebServlet.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java index 9a85562618..b80b69f51a 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java @@ -281,9 +281,9 @@ class GitwebServlet extends HttpServlet { p.print(" my $h = shift;\n"); p.print(" my $q;\n"); p.print(" if (!$h || $h eq 'HEAD') {\n"); - p.print(" $q = qq{#q,project:$ENV{'GERRIT_PROJECT_NAME'}};\n"); + p.print(" $q = qq{#/q/project:$ENV{'GERRIT_PROJECT_NAME'}};\n"); p.print(" } elsif ($h =~ /^refs\\/heads\\/([-\\w]+)$/) {\n"); - p.print(" $q = qq{#q,project:$ENV{'GERRIT_PROJECT_NAME'}"); + p.print(" $q = qq{#/q/project:$ENV{'GERRIT_PROJECT_NAME'}"); p.print("+branch:$1};\n"); // wrapped p.print(" } elsif ($h =~ /^refs\\/changes\\/\\d{2}\\/(\\d+)\\/\\d+$/) "); p.print("{\n"); // wrapped From 5d592e885de1a6d062c440031f2e952b24733301 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 9 Jun 2016 05:12:26 +0000 Subject: [PATCH 16/16] Documentation: Fix anchor for gitweb.urlEncode Change-Id: Id87dc588d736b650771bee384e47df06446faf50 --- Documentation/config-gerrit.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 008c7c2bfe..09870325cf 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -1925,7 +1925,7 @@ using the property 'gitweb.pathSeparator'. + Valid values are the characters '*', '(' and ')'. -[[gitweb.linkDrafts]]gitweb.urlEncode:: +[[gitweb.urlEncode]]gitweb.urlEncode:: + Whether or not Gerrit should encode the generated viewer URL. +