From 01c52a58f66e1582e5c0cea17801fb347f3163c9 Mon Sep 17 00:00:00 2001 From: Markus Duft Date: Mon, 11 Jun 2018 17:12:00 +0200 Subject: [PATCH] Fix issues with LFS on GitHub (SSH) * URIish seems to have a tiny feature (bug?). The path of the URI starts with a '/' only if the URI has a port set (it seems). * GitHub does not return SSH authorization on a single line as Gerrit does - need to account for that. * Increase the SSH git-lfs-authenticate timeout, as GitHub sometimes responds slower than expected. * Guard against NPE in case the download action does not contain any additional headers. Change-Id: Icd1ead3d015479fd4b8bbd42ed42129b0abfb95c Signed-off-by: Markus Duft --- org.eclipse.jgit.lfs/META-INF/MANIFEST.MF | 3 +- .../jgit/lfs/internal/LfsText.properties | 1 + .../lfs/internal/LfsConnectionFactory.java | 24 +++++++++--- .../eclipse/jgit/lfs/internal/LfsText.java | 1 + .../src/org/eclipse/jgit/util/SshSupport.java | 38 ++++++++++++++----- 5 files changed, 51 insertions(+), 16 deletions(-) diff --git a/org.eclipse.jgit.lfs/META-INF/MANIFEST.MF b/org.eclipse.jgit.lfs/META-INF/MANIFEST.MF index acc3532595..41450ac973 100644 --- a/org.eclipse.jgit.lfs/META-INF/MANIFEST.MF +++ b/org.eclipse.jgit.lfs/META-INF/MANIFEST.MF @@ -32,4 +32,5 @@ Import-Package: com.google.gson;version="[2.8.2,3.0.0)", org.eclipse.jgit.treewalk;version="[5.0.0,5.1.0)", org.eclipse.jgit.treewalk.filter;version="[5.0.0,5.1.0)", org.eclipse.jgit.util;version="[5.0.0,5.1.0)", - org.eclipse.jgit.util.io;version="[5.0.0,5.1.0)" + org.eclipse.jgit.util.io;version="[5.0.0,5.1.0)", + org.slf4j;version="[1.7.0,2.0.0)" diff --git a/org.eclipse.jgit.lfs/resources/org/eclipse/jgit/lfs/internal/LfsText.properties b/org.eclipse.jgit.lfs/resources/org/eclipse/jgit/lfs/internal/LfsText.properties index 0e00f146ae..91f0bdb094 100644 --- a/org.eclipse.jgit.lfs/resources/org/eclipse/jgit/lfs/internal/LfsText.properties +++ b/org.eclipse.jgit.lfs/resources/org/eclipse/jgit/lfs/internal/LfsText.properties @@ -1,3 +1,4 @@ +cannotDiscoverLfs=Cannot discover LFS URI corruptLongObject=The content hash ''{0}'' of the long object ''{1}'' doesn''t match its id, the corrupt object will be deleted. incorrectLONG_OBJECT_ID_LENGTH=Incorrect LONG_OBJECT_ID_LENGTH. inconsistentMediafileLength=Mediafile {0} has unexpected length; expected {1} but found {2}. diff --git a/org.eclipse.jgit.lfs/src/org/eclipse/jgit/lfs/internal/LfsConnectionFactory.java b/org.eclipse.jgit.lfs/src/org/eclipse/jgit/lfs/internal/LfsConnectionFactory.java index 3ac69923f2..bac980bb8d 100644 --- a/org.eclipse.jgit.lfs/src/org/eclipse/jgit/lfs/internal/LfsConnectionFactory.java +++ b/org.eclipse.jgit.lfs/src/org/eclipse/jgit/lfs/internal/LfsConnectionFactory.java @@ -68,13 +68,18 @@ import org.eclipse.jgit.transport.URIish; import org.eclipse.jgit.transport.http.HttpConnection; import org.eclipse.jgit.util.HttpSupport; import org.eclipse.jgit.util.SshSupport; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * Provides means to get a valid LFS connection for a given repository. */ public class LfsConnectionFactory { - private static final int SSH_AUTH_TIMEOUT_SECONDS = 5; + private static final Logger log = LoggerFactory + .getLogger(LfsConnectionFactory.class); + + private static final int SSH_AUTH_TIMEOUT_SECONDS = 30; private static final String SCHEME_HTTPS = "https"; //$NON-NLS-1$ private static final String SCHEME_SSH = "ssh"; //$NON-NLS-1$ private static final Map sshAuthCache = new TreeMap<>(); @@ -162,7 +167,7 @@ public class LfsConnectionFactory { Map additionalHeaders, String remoteUrl) { try { URIish u = new URIish(remoteUrl); - if (SCHEME_SSH.equals(u.getScheme())) { + if (u.getScheme() == null || SCHEME_SSH.equals(u.getScheme())) { Protocol.ExpiringAction action = getSshAuthentication( db, purpose, remoteUrl, u); additionalHeaders.putAll(action.header); @@ -171,6 +176,7 @@ public class LfsConnectionFactory { return remoteUrl + Protocol.INFO_LFS_ENDPOINT; } } catch (Exception e) { + log.error(LfsText.get().cannotDiscoverLfs, e); return null; // could not discover } } @@ -226,8 +232,10 @@ public class LfsConnectionFactory { .create(contentUrl, HttpSupport .proxyFor(ProxySelector.getDefault(), contentUrl)); contentServerConn.setRequestMethod(method); - action.header - .forEach((k, v) -> contentServerConn.setRequestProperty(k, v)); + if (action.header != null) { + action.header.forEach( + (k, v) -> contentServerConn.setRequestProperty(k, v)); + } if (contentUrl.getProtocol().equals(SCHEME_HTTPS) && !repo.getConfig().getBoolean(HttpConfig.HTTP, HttpConfig.SSL_VERIFY_KEY, true)) { @@ -241,7 +249,13 @@ public class LfsConnectionFactory { } private static String extractProjectName(URIish u) { - String path = u.getPath().substring(1); + String path = u.getPath(); + + // begins with a slash if the url contains a port (gerrit vs. github). + if (path.startsWith("/")) { //$NON-NLS-1$ + path = path.substring(1); + } + if (path.endsWith(org.eclipse.jgit.lib.Constants.DOT_GIT)) { return path.substring(0, path.length() - 4); } else { diff --git a/org.eclipse.jgit.lfs/src/org/eclipse/jgit/lfs/internal/LfsText.java b/org.eclipse.jgit.lfs/src/org/eclipse/jgit/lfs/internal/LfsText.java index d7d0fe186a..d51203a740 100644 --- a/org.eclipse.jgit.lfs/src/org/eclipse/jgit/lfs/internal/LfsText.java +++ b/org.eclipse.jgit.lfs/src/org/eclipse/jgit/lfs/internal/LfsText.java @@ -60,6 +60,7 @@ public class LfsText extends TranslationBundle { } // @formatter:off + /***/ public String cannotDiscoverLfs; /***/ public String corruptLongObject; /***/ public String inconsistentMediafileLength; /***/ public String inconsistentContentLength; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/SshSupport.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/SshSupport.java index 96123ea670..40a576ed22 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/SshSupport.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/SshSupport.java @@ -42,12 +42,9 @@ */ package org.eclipse.jgit.util; -import java.io.BufferedReader; import java.io.IOException; -import java.io.InputStreamReader; import org.eclipse.jgit.annotations.Nullable; -import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.transport.CredentialsProvider; import org.eclipse.jgit.transport.RemoteSession; import org.eclipse.jgit.transport.SshSessionFactory; @@ -76,8 +73,9 @@ public class SshSupport { * @param command * the remote command to execute. * @param timeout - * a timeout in seconds. - * @return The first line of output read from stdout. Stderr is discarded. + * a timeout in seconds. The timeout may be exceeded in corner + * cases. + * @return The entire output read from stdout. Stderr is discarded. * @throws IOException */ public static String runSshCommand(URIish sshUri, @@ -86,17 +84,28 @@ public class SshSupport { RemoteSession session = null; Process process = null; StreamCopyThread errorThread = null; - try (MessageWriter stderr = new MessageWriter()) { + StreamCopyThread outThread = null; + try (MessageWriter stderr = new MessageWriter(); + MessageWriter stdout = new MessageWriter()) { session = SshSessionFactory.getInstance().getSession(sshUri, provider, fs, 1000 * timeout); process = session.exec(command, 0); errorThread = new StreamCopyThread(process.getErrorStream(), stderr.getRawStream()); errorThread.start(); - try (BufferedReader reader = new BufferedReader( - new InputStreamReader(process.getInputStream(), - Constants.CHARSET))) { - return reader.readLine(); + outThread = new StreamCopyThread(process.getInputStream(), + stdout.getRawStream()); + outThread.start(); + try { + // waitFor with timeout has a bug - JSch' exitValue() throws the + // wrong exception type :( + if (process.waitFor() == 0) { + return stdout.toString(); + } else { + return null; // still running after timeout + } + } catch (InterruptedException e) { + return null; // error } } finally { if (errorThread != null) { @@ -108,6 +117,15 @@ public class SshSupport { errorThread = null; } } + if (outThread != null) { + try { + outThread.halt(); + } catch (InterruptedException e) { + // Stop waiting and return anyway. + } finally { + outThread = null; + } + } if (process != null) { process.destroy(); } -- 2.39.5