From cb90ed08526bd51f04e5d72e3ba3cf5bd30c11e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Fri, 21 Oct 2022 16:32:03 +0200 Subject: [PATCH 1/3] Allow to perform PackedBatchRefUpdate without locking loose refs Add another newBatchUpdate method in the RefDirectory where we can control if the created PackedBatchRefUpdate will lock the loose refs or not. This can be useful in cases when we run programs which have exclusive access to a Git repository and we know that locking loose refs is unnecessary and just a performance loss. Change-Id: I7d0932eb1598a3871a2281b1a049021380234df9 --- .../storage/file/PackedBatchRefUpdate.java | 12 +++++++++--- .../jgit/internal/storage/file/RefDirectory.java | 15 +++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java index 9c1d33dc3..8b0ea4fcd 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java @@ -86,10 +86,16 @@ */ class PackedBatchRefUpdate extends BatchRefUpdate { private RefDirectory refdb; + private boolean shouldLockLooseRefs; PackedBatchRefUpdate(RefDirectory refdb) { - super(refdb); - this.refdb = refdb; + this(refdb, true); + } + + PackedBatchRefUpdate(RefDirectory refdb, boolean shouldLockLooseRefs) { + super(refdb); + this.refdb = refdb; + this.shouldLockLooseRefs = shouldLockLooseRefs; } /** {@inheritDoc} */ @@ -155,7 +161,7 @@ public void execute(RevWalk walk, ProgressMonitor monitor, refdb.inProcessPackedRefsLock.lock(); try { PackedRefList oldPackedList; - if (!refdb.isInClone()) { + if (!refdb.isInClone() && shouldLockLooseRefs) { locks = lockLooseRefs(pending); if (locks == null) { return; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java index 07e38147f..b46ffe367 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java @@ -587,6 +587,21 @@ public PackedBatchRefUpdate newBatchUpdate() { return new PackedBatchRefUpdate(this); } + /** + * Create a new batch update to attempt on this database. + * + * @param shouldLockLooseRefs + * whether loose refs should be locked during the batch ref + * update. Note that this should only be set to {@code false} if + * the application using this ensures that no other ref updates + * run concurrently to avoid lost updates caused by a race. In + * such cases it can improve performance. + * @return a new batch update object + */ + public PackedBatchRefUpdate newBatchUpdate(boolean shouldLockLooseRefs) { + return new PackedBatchRefUpdate(this, shouldLockLooseRefs); + } + /** {@inheritDoc} */ @Override public boolean performsAtomicTransactions() { From ac127a7932e3dfa5ac09ccc527123fc0223be5ba Mon Sep 17 00:00:00 2001 From: Sven Selberg Date: Wed, 9 Nov 2022 16:58:17 +0100 Subject: [PATCH 2/3] Don't handle internal git errors as an HTTP error The fix that fixed the propagation of error-codes: 8984e1f66 HTTP Smart: set correct HTTP status on error [1] made some faulty assumptions. "Wants not valid", can be an intermittent git error and the HTTP response should be 200 and not 400 since the request isn't necessary faulty. [1] https://git.eclipse.org/r/c/jgit/jgit/+/192677 Bug: 579676 Change-Id: I461bc78ff6e450636811ece50d21c57a2a7f2ae3 --- .../eclipse/jgit/http/server/UploadPackServlet.java | 8 ++++---- .../jgit/http/test/SmartClientSmartServerTest.java | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java index 23a398f9d..509ea4cf5 100644 --- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java +++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java @@ -10,9 +10,9 @@ package org.eclipse.jgit.http.server; -import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST; import static javax.servlet.http.HttpServletResponse.SC_FORBIDDEN; import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR; +import static javax.servlet.http.HttpServletResponse.SC_OK; import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED; import static javax.servlet.http.HttpServletResponse.SC_UNSUPPORTED_MEDIA_TYPE; import static org.eclipse.jgit.http.server.GitSmartHttpTools.UPLOAD_PACK; @@ -49,7 +49,6 @@ import org.eclipse.jgit.transport.ServiceMayNotContinueException; import org.eclipse.jgit.transport.UploadPack; import org.eclipse.jgit.transport.UploadPackInternalServerErrorException; -import org.eclipse.jgit.transport.WantNotValidException; import org.eclipse.jgit.transport.resolver.ServiceNotAuthorizedException; import org.eclipse.jgit.transport.resolver.ServiceNotEnabledException; import org.eclipse.jgit.transport.resolver.UploadPackFactory; @@ -157,8 +156,9 @@ private static int statusCodeForThrowable(Throwable error) { if (error instanceof ServiceNotEnabledException) { return SC_FORBIDDEN; } - if (error instanceof WantNotValidException) { - return SC_BAD_REQUEST; + if (error instanceof PackProtocolException) { + // Internal git errors is not an error from an HTTP standpoint. + return SC_OK; } return SC_INTERNAL_SERVER_ERROR; } diff --git a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java index 8f3888e4d..b9b10b45d 100644 --- a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java +++ b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java @@ -537,9 +537,9 @@ public void testFetchBySHA1Unreachable() throws Exception { Collections.singletonList( new RefSpec(unreachableCommit.name())))); assertTrue(e.getMessage().contains( - "Bad Request")); + "want " + unreachableCommit.name() + " not valid")); } - assertLastRequestStatusCode(400); + assertLastRequestStatusCode(200); } @Test @@ -560,9 +560,9 @@ protected Map getAdvertisedRefs(Repository repository, () -> t.fetch(NullProgressMonitor.INSTANCE, Collections.singletonList(new RefSpec(A.name())))); assertTrue( - e.getMessage().contains("Bad Request")); + e.getMessage().contains("want " + A.name() + " not valid")); } - assertLastRequestStatusCode(400); + assertLastRequestStatusCode(200); } @Test @@ -1610,9 +1610,9 @@ public void testInvalidWant() throws Exception { fail("Server accepted want " + id.name()); } catch (TransportException err) { assertTrue(err.getMessage() - .contains("Bad Request")); + .contains("want " + id.name() + " not valid")); } - assertLastRequestStatusCode(400); + assertLastRequestStatusCode(200); } @Test From 5bf7472e0ca967893f9fdb653c3f1b22fb6d56e5 Mon Sep 17 00:00:00 2001 From: Sven Selberg Date: Wed, 9 Nov 2022 18:28:45 +0100 Subject: [PATCH 3/3] Extract Exception -> HTTP status code mapping for reuse Extract private static method UploadPackServlet#statusCodeForThrowable to a public static method in the UploadPackErrorHandler interface so that implementers of this interface can reuse the default mapping. Change-Id: Ie4a0a006b0148d5b828d610c55d19ce407aab055 --- .../.settings/.api_filters | 11 +++++++++ .../http/server/UploadPackErrorHandler.java | 24 +++++++++++++++++++ .../jgit/http/server/UploadPackServlet.java | 14 +---------- 3 files changed, 36 insertions(+), 13 deletions(-) create mode 100644 org.eclipse.jgit.http.server/.settings/.api_filters diff --git a/org.eclipse.jgit.http.server/.settings/.api_filters b/org.eclipse.jgit.http.server/.settings/.api_filters new file mode 100644 index 000000000..2c32c9864 --- /dev/null +++ b/org.eclipse.jgit.http.server/.settings/.api_filters @@ -0,0 +1,11 @@ + + + + + + + + + + + diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackErrorHandler.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackErrorHandler.java index 03be0873b..2aadbbc98 100644 --- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackErrorHandler.java +++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackErrorHandler.java @@ -9,13 +9,19 @@ */ package org.eclipse.jgit.http.server; +import static javax.servlet.http.HttpServletResponse.SC_FORBIDDEN; +import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR; +import static javax.servlet.http.HttpServletResponse.SC_OK; + import java.io.IOException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.eclipse.jgit.errors.PackProtocolException; import org.eclipse.jgit.transport.ServiceMayNotContinueException; import org.eclipse.jgit.transport.UploadPack; +import org.eclipse.jgit.transport.resolver.ServiceNotEnabledException; /** * Handle git-upload-pack errors. @@ -34,6 +40,24 @@ * @since 5.6 */ public interface UploadPackErrorHandler { + /** + * Maps a thrown git related Exception to an appropriate HTTP status code. + * + * @param error + * The thrown Exception. + * @return the HTTP status code as an int + * @since 6.1.1 + */ + public static int statusCodeForThrowable(Throwable error) { + if (error instanceof ServiceNotEnabledException) { + return SC_FORBIDDEN; + } + if (error instanceof PackProtocolException) { + // Internal git errors are not errors from an HTTP standpoint. + return SC_OK; + } + return SC_INTERNAL_SERVER_ERROR; + } /** * @param req * The HTTP request diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java index 509ea4cf5..b0a07f1d5 100644 --- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java +++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java @@ -11,8 +11,6 @@ package org.eclipse.jgit.http.server; import static javax.servlet.http.HttpServletResponse.SC_FORBIDDEN; -import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR; -import static javax.servlet.http.HttpServletResponse.SC_OK; import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED; import static javax.servlet.http.HttpServletResponse.SC_UNSUPPORTED_MEDIA_TYPE; import static org.eclipse.jgit.http.server.GitSmartHttpTools.UPLOAD_PACK; @@ -23,6 +21,7 @@ import static org.eclipse.jgit.http.server.ServletUtils.consumeRequestBody; import static org.eclipse.jgit.http.server.ServletUtils.getInputStream; import static org.eclipse.jgit.http.server.ServletUtils.getRepository; +import static org.eclipse.jgit.http.server.UploadPackErrorHandler.statusCodeForThrowable; import static org.eclipse.jgit.util.HttpSupport.HDR_USER_AGENT; import java.io.IOException; @@ -152,17 +151,6 @@ public void destroy() { } } - private static int statusCodeForThrowable(Throwable error) { - if (error instanceof ServiceNotEnabledException) { - return SC_FORBIDDEN; - } - if (error instanceof PackProtocolException) { - // Internal git errors is not an error from an HTTP standpoint. - return SC_OK; - } - return SC_INTERNAL_SERVER_ERROR; - } - private final UploadPackErrorHandler handler; UploadPackServlet(@Nullable UploadPackErrorHandler handler) {