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 <markus.duft@ssi-schaefer.com>

diff --git a/org.eclipse.jgit.lfs/META-INF/MANIFEST.MF b/org.eclipse.jgit.lfs/META-INF/MANIFEST.MF
index acc35325956a34179af49578f456a66d9c0dabdc..41450ac973db0f0ce2f5e1458bfe05bd4380ff9c 100644 (file)
--- 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 0e00f146aec5e3f874990444cffddc988b2a19f5..91f0bdb0945790ff36d7aa8048a2627ad8ad9efa 100644 (file)
--- 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 3ac69923f239de2d53f9ea31e6e49ae83e496df8..bac980bb8d8409c542aba0a557768fcf398a711b 100644 (file)
--- 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<String, AuthCache> sshAuthCache = new TreeMap<>();
@@ -162,7 +167,7 @@ public class LfsConnectionFactory {
                        Map<String, String> 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 d7d0fe186aaade1693a73da8c339dac215ac1caf..d51203a740d6c6de509ca03bff48ddd2a6c73cd9 100644 (file)
--- 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 96123ea670b0a418fae3ab76c41b8684e65e79d4..40a576ed22fd66193582cb7930a27d9d6ac83ca3 100644 (file)
--- 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();
                        }