From ec3ee59b43ef5747727e0a11e6224fcc03c4aa44 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Fri, 5 Aug 2016 15:38:16 +0900 Subject: [PATCH 1/5] RestApiServlet: Allow HEAD requests Change I3be4aeb009 replaced the "Get file type" REST API endpoint with the /content endpoint. According to documentation [1], the content type can be retrieved by making a HEAD request to the endpoint. This doesn't work because RestApiServlet is only checking for GET, and HEAD requests result in 404. Modify it to also allow HEAD requests. Add a test to make sure it works. [1] https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#get-content Bug: Issue 4136 Change-Id: If5eaaba2fec94499f332970ce6b8c6db2ac9c571 --- .../google/gerrit/acceptance/HttpResponse.java | 10 ++++++++++ .../google/gerrit/acceptance/RestSession.java | 3 +++ .../acceptance/api/revision/RevisionIT.java | 18 ++++++++++++++++-- .../gerrit/httpd/restapi/RestApiServlet.java | 3 +-- 4 files changed, 30 insertions(+), 4 deletions(-) diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/HttpResponse.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/HttpResponse.java index 872c91277f..390cae34a6 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/HttpResponse.java +++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/HttpResponse.java @@ -51,6 +51,16 @@ public class HttpResponse { return response.getStatusLine().getStatusCode(); } + public String getContentType() { + return response.getFirstHeader("X-FYI-Content-Type").getValue(); + } + + public boolean hasContent() { + Preconditions.checkNotNull(response, + "Response is not initialized."); + return response.getEntity() != null; + } + public String getEntityContent() throws IOException { Preconditions.checkNotNull(response, "Response is not initialized."); diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/RestSession.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/RestSession.java index aee629db33..4b22d0aed3 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/RestSession.java +++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/RestSession.java @@ -114,6 +114,9 @@ public class RestSession extends HttpSession { return execute(Request.Delete(url + "/a" + endPoint)); } + public RestResponse head(String endPoint) throws IOException { + return execute(Request.Head(url + "/a" + endPoint)); + } public static RawInput newRawInput(String content) { return newRawInput(content.getBytes(UTF_8)); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java index 3e970e434b..23a2844ffc 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java @@ -19,13 +19,14 @@ import static com.google.gerrit.acceptance.PushOneCommit.FILE_CONTENT; import static com.google.gerrit.acceptance.PushOneCommit.FILE_NAME; import static com.google.gerrit.acceptance.PushOneCommit.PATCH; import static java.nio.charset.StandardCharsets.UTF_8; +import static org.apache.http.HttpStatus.SC_OK; import static org.eclipse.jgit.lib.Constants.HEAD; import com.google.common.base.Predicate; import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.AbstractDaemonTest; -import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.acceptance.RestResponse; import com.google.gerrit.acceptance.TestAccount; import com.google.gerrit.extensions.api.changes.ChangeApi; import com.google.gerrit.extensions.api.changes.CherryPickInput; @@ -63,7 +64,6 @@ import java.util.List; import java.util.Locale; import java.util.Map; -@NoHttpd public class RevisionIT extends AbstractDaemonTest { private TestAccount admin2; @@ -433,6 +433,20 @@ public class RevisionIT extends AbstractDaemonTest { assertThat(res).isEqualTo(FILE_CONTENT); } + @Test + public void contentType() throws Exception { + PushOneCommit.Result r = createChange(); + + String endPoint = "/changes/" + r.getChangeId() + + "/revisions/" + r.getCommit().name() + + "/files/" + FILE_NAME + + "/content"; + RestResponse response = adminSession.head(endPoint); + assertThat(response.getStatusCode()).isEqualTo(SC_OK); + assertThat(response.getContentType()).startsWith("text/plain"); + assertThat(response.hasContent()).isFalse(); + } + private void assertMergeable(String id, boolean expected) throws Exception { MergeableInfo m = gApi.changes().id(id).current().mergeable(); assertThat(m.mergeable).isEqualTo(expected); diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java index 7347cd99c8..91a16b896c 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java @@ -317,8 +317,7 @@ public class RestApiServlet extends HttpServlet { return; } - if (viewData.view instanceof RestReadView - && "GET".equals(req.getMethod())) { + if (viewData.view instanceof RestReadView && isGetOrHead(req)) { result = ((RestReadView) viewData.view).apply(rsrc); } else if (viewData.view instanceof RestModifyView) { @SuppressWarnings("unchecked") From 6a89d8cfab00511bad2c3057d48c484a36673eb9 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 9 Aug 2016 15:01:32 +0200 Subject: [PATCH 2/5] Confirm email: Return proper error when email is used by other account So far trying to confirm an email address that is already in use by another account was failing with 500 Internal Server Error. Change-Id: I97830491cc198f582fa4192de1276a4af7d12ce2 Signed-off-by: Edwin Kempin --- .../gerrit/acceptance/rest/config/ConfirmEmailIT.java | 8 ++++++++ .../com/google/gerrit/server/config/ConfirmEmail.java | 2 ++ 2 files changed, 10 insertions(+) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/config/ConfirmEmailIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/config/ConfirmEmailIT.java index 9d8320ad0e..e51ed081df 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/config/ConfirmEmailIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/config/ConfirmEmailIT.java @@ -63,4 +63,12 @@ public class ConfirmEmailIT extends AbstractDaemonTest { RestResponse r = adminSession.put("/config/server/email.confirm", in); assertThat(r.getStatusCode()).isEqualTo(HttpStatus.SC_UNPROCESSABLE_ENTITY); } + + @Test + public void confirmAlreadyInUse_UnprocessableEntity() throws Exception { + ConfirmEmail.Input in = new ConfirmEmail.Input(); + in.token = emailTokenVerifier.encode(admin.getId(), user.email); + RestResponse r = adminSession.put("/config/server/email.confirm", in); + assertThat(r.getStatusCode()).isEqualTo(HttpStatus.SC_UNPROCESSABLE_ENTITY); + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/ConfirmEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/ConfirmEmail.java index 24d28c7eca..df3c37f089 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/ConfirmEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/ConfirmEmail.java @@ -77,6 +77,8 @@ public class ConfirmEmail implements RestModifyView { } } catch (EmailTokenVerifier.InvalidTokenException e) { throw new UnprocessableEntityException("invalid token"); + } catch (AccountException e) { + throw new UnprocessableEntityException(e.getMessage()); } } } From 3554e9433216a1a9db4e1e43d5b42eca3a0c5a83 Mon Sep 17 00:00:00 2001 From: Yuxuan 'fishy' Wang Date: Tue, 9 Aug 2016 14:48:39 -0700 Subject: [PATCH 3/5] Handle an NPE in ToolsCatalog.java Also added @Nullable annotation to that file. Change-Id: I25e799c6b6cc17b8ad9c77b0a95a118902997587 --- .../com/google/gerrit/server/tools/ToolsCatalog.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/tools/ToolsCatalog.java b/gerrit-server/src/main/java/com/google/gerrit/server/tools/ToolsCatalog.java index f59bba947c..4810b3df9c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/tools/ToolsCatalog.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/tools/ToolsCatalog.java @@ -16,6 +16,8 @@ package com.google.gerrit.server.tools; import static java.nio.charset.StandardCharsets.UTF_8; +import com.google.common.base.Strings; +import com.google.gerrit.common.Nullable; import com.google.gerrit.common.Version; import com.google.inject.Inject; import com.google.inject.Singleton; @@ -60,7 +62,11 @@ public class ToolsCatalog { * @param name path of the item, relative to the root of the catalog. * @return the entry; null if the item is not part of the catalog. */ - public Entry get(String name) { + @Nullable + public Entry get(@Nullable String name) { + if (Strings.isNullOrEmpty(name)) { + return null; + } if (name.startsWith("/")) { name = name.substring(1); } @@ -109,6 +115,7 @@ public class ToolsCatalog { return Collections.unmodifiableSortedMap(toc); } + @Nullable private static byte[] read(String path) { String name = "root/" + path; try (InputStream in = ToolsCatalog.class.getResourceAsStream(name)) { @@ -128,6 +135,7 @@ public class ToolsCatalog { } } + @Nullable private static String dirOf(String path) { final int s = path.lastIndexOf('/'); return s < 0 ? null : path.substring(0, s); From 34d9c85dde1594488e57f51cc2dbe3e09ad05b7b Mon Sep 17 00:00:00 2001 From: Alexandre Philbert Date: Fri, 5 Aug 2016 11:59:14 -0400 Subject: [PATCH 4/5] Fix RebaseIfNecessary strategy introducing duplicate commit Since RebaseIfNecessary ends up acting like MergeIfNecessary if any merge commits are present, we're better off sorting and pruning accordingly before doing any work. Before this change, the commit inside a merge commit would be committed under a different commit hash beforehand and then the merge commit was added on top of that. More details in the ticket itself. Bug: Issue 4318 Change-Id: I3c1b2e8d452a0adea005a8a14123347cb0dbbc75 --- .../change/SubmitByRebaseIfNecessaryIT.java | 64 +++++++++++++++++++ .../git/strategy/RebaseIfNecessary.java | 12 ++++ 2 files changed, 76 insertions(+) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java index 6d10b6189e..222816e026 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/SubmitByRebaseIfNecessaryIT.java @@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.gerrit.acceptance.GitUtil.getChangeId; import static com.google.gerrit.acceptance.GitUtil.pushHead; +import com.google.common.collect.ImmutableList; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.TestProjectInput; import com.google.gerrit.extensions.client.InheritableBoolean; @@ -25,7 +26,10 @@ import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Project; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevWalk; import org.junit.Test; public class SubmitByRebaseIfNecessaryIT extends AbstractSubmit { @@ -75,6 +79,66 @@ public class SubmitByRebaseIfNecessaryIT extends AbstractSubmit { assertPersonEquals(admin.getIdent(), head.getCommitterIdent()); } + @Test + public void submitWithRebaseMergeCommit() throws Exception { + /* + * (HEAD, origin/master, origin/HEAD) Merge changes X,Y + |\ + | * Merge branch 'master' into origin/master + | |\ + | | * SHA Added a + | |/ + * | Before + |/ + * Initial empty repository + */ + RevCommit initialHead = getRemoteHead(); + PushOneCommit.Result change1 = createChange("Added a", "a.txt", ""); + + PushOneCommit change2Push = pushFactory.create(db, admin.getIdent(), testRepo, + "Merge to master", "m.txt", ""); + change2Push.setParents(ImmutableList.of(initialHead, change1.getCommit())); + PushOneCommit.Result change2 = change2Push.to("refs/for/master"); + + testRepo.reset(initialHead); + PushOneCommit.Result change3 = createChange("Before", "b.txt", ""); + + approve(change3.getChangeId()); + submit(change3.getChangeId()); + + approve(change1.getChangeId()); + approve(change2.getChangeId()); + submit(change2.getChangeId()); + + RevCommit newHead = getRemoteHead(); + assertThat(newHead.getParentCount()).isEqualTo(2); + + RevCommit headParent1 = parse(newHead.getParent(0).getId()); + RevCommit headParent2 = parse(newHead.getParent(1).getId()); + + assertThat(headParent1.getId()).isEqualTo(change3.getCommit().getId()); + assertThat(headParent1.getParentCount()).isEqualTo(1); + assertThat(headParent1.getParent(0)).isEqualTo(initialHead); + + assertThat(headParent2.getId()).isEqualTo(change2.getCommit().getId()); + assertThat(headParent2.getParentCount()).isEqualTo(2); + + RevCommit headGrandparent1 = parse(headParent2.getParent(0).getId()); + RevCommit headGrandparent2 = parse(headParent2.getParent(1).getId()); + + assertThat(headGrandparent1.getId()).isEqualTo(initialHead.getId()); + assertThat(headGrandparent2.getId()).isEqualTo(change1.getCommit().getId()); + } + + private RevCommit parse(ObjectId id) throws Exception { + try (Repository repo = repoManager.openRepository(project); + RevWalk rw = new RevWalk(repo)) { + RevCommit c = rw.parseCommit(id); + rw.parseBody(c); + return c; + } + } + @Test @TestProjectInput(useContentMerge = InheritableBoolean.TRUE) public void submitWithContentMerge() throws Exception { 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 0672e8cee1..f9a7e6e796 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 @@ -68,6 +68,18 @@ public class RebaseIfNecessary extends SubmitStrategy { final Collection toMerge) throws IntegrationException { MergeTip mergeTip = new MergeTip(branchTip, toMerge); List sorted = sort(toMerge); + + for (CodeReviewCommit c : sorted) { + if (c.getParentCount() > 1) { + // Since there is a merge commit, sort and prune again using + // MERGE_IF_NECESSARY semantics to avoid creating duplicate + // commits. + // + sorted = args.mergeUtil.reduceToMinimalMerge(args.mergeSorter, sorted); + break; + } + } + while (!sorted.isEmpty()) { CodeReviewCommit n = sorted.remove(0); From 199bbae076b36fadf2e9542346210607c6e27e76 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 10 Aug 2016 11:15:34 +0900 Subject: [PATCH 5/5] Update 2.12.4 release notes Change-Id: I242e130b1bd8084d18807ae56c72d0a0ff9c322c --- ReleaseNotes/ReleaseNotes-2.12.4.txt | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/ReleaseNotes/ReleaseNotes-2.12.4.txt b/ReleaseNotes/ReleaseNotes-2.12.4.txt index ce2f3690c5..3ba17f4f5d 100644 --- a/ReleaseNotes/ReleaseNotes-2.12.4.txt +++ b/ReleaseNotes/ReleaseNotes-2.12.4.txt @@ -65,6 +65,26 @@ changes. Fix internal error when a query does not contain any token. * link:https://bugs.chromium.org/p/gerrit/issues/detail?id=4241[Issue 4241]: -Fix 'Cannot format velocity template' error. +Fix 'Cannot format velocity template' error when sending notification emails. * Fix `sshd.idleTimeout` setting being ignored. ++ +Ths `sshd.idleTimeout` setting was not being correctly set on the SSHD +backend, causing idle sessions to not time out. + +* link:https://bugs.chromium.org/p/gerrit/issues/detail?id=4324[Issue 4324]: +Set the correct uploader on new patch sets created via the inline editor. + +* Log a warning instead of failing when invalid commentlinks are configured. + +* link:https://bugs.chromium.org/p/gerrit/issues/detail?id=4136[Issue 4136]: +Fix support for `HEAD` requests in the REST API. ++ +Sending a `HEAD` request failed with '404 Not Found'. + +* Return proper error response when trying to confirm an email that is already +used by another user. + +* link:https://bugs.chromium.org/p/gerrit/issues/detail?id=4318[Issue 4318] +Fix 'Rebase if Necessary' merge strategy to prevent introducing a duplicate +commit when submitting a merge commit.