From 391c7a25fa8d247f9f6087f9f19ee330540ad602 Mon Sep 17 00:00:00 2001 From: Michael Keppler Date: Sat, 9 Mar 2019 12:37:10 +0100 Subject: [PATCH 1/3] Avoid NPE in ObjectId.isId() That method can easily be invoked with a null argument (e.g. isId(repo.getFullBranch()), therefore it should handle null arguments. Change was suggested in https://git.eclipse.org/r/#/c/137918/, which tried to fix the same in egit only. Bug:544982 Change-Id: I32d1df6e9b2946ab324eda7008721159019316b3 Signed-off-by: Michael Keppler --- org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectId.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectId.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectId.java index 764f89015..0e96138c0 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectId.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectId.java @@ -49,6 +49,7 @@ import java.io.ObjectInputStream; import java.io.ObjectOutputStream; import java.io.Serializable; +import org.eclipse.jgit.annotations.Nullable; import org.eclipse.jgit.errors.InvalidObjectIdException; import org.eclipse.jgit.util.NB; import org.eclipse.jgit.util.RawParseUtils; @@ -86,7 +87,10 @@ public class ObjectId extends AnyObjectId implements Serializable { * the string to test. * @return true if the string can converted into an ObjectId. */ - public static final boolean isId(String id) { + public static final boolean isId(@Nullable String id) { + if (id == null) { + return false; + } if (id.length() != Constants.OBJECT_ID_STRING_LENGTH) return false; try { From d44225d85cd8be171c5612f9001a2e455c7d21be Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Sat, 9 Mar 2019 01:21:40 +0100 Subject: [PATCH 2/3] Use SystemReader in JSchConfigSessionFactoryTest This isolates the test from the concrete system it's running on. SshSessionFactory reads the user also through SystemReader. Change-Id: I1c796aa1c498fe3967456d8589e6be0a82ab8f44 Signed-off-by: Matthias Sohn --- .../jgit/transport/JschConfigSessionFactoryTest.java | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/JschConfigSessionFactoryTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/JschConfigSessionFactoryTest.java index 1e65a20d7..3de4210ba 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/JschConfigSessionFactoryTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/JschConfigSessionFactoryTest.java @@ -49,8 +49,11 @@ import java.nio.file.Files; import java.util.Arrays; import java.util.concurrent.TimeUnit; +import org.eclipse.jgit.junit.MockSystemReader; import org.eclipse.jgit.util.FS; +import org.eclipse.jgit.util.SystemReader; import org.junit.After; +import org.junit.Before; import org.junit.Test; import com.jcraft.jsch.Session; @@ -67,8 +70,14 @@ public class JschConfigSessionFactoryTest { DefaultSshSessionFactory factory = new DefaultSshSessionFactory(); + @Before + public void setup() { + SystemReader.setInstance(new MockSystemReader()); + } + @After public void removeTmpConfig() { + SystemReader.setInstance(null); if (tmpConfigFile == null) { return; } @@ -87,7 +96,8 @@ public class JschConfigSessionFactoryTest { Session session = createSession("ssh://egit/egit/egit"); assertEquals("egit", session.getHost()); // No user in URI, none in ssh config: default is OS user name - assertEquals(System.getProperty("user.name"), session.getUserName()); + assertEquals(SystemReader.getInstance().getProperty("user.name"), + session.getUserName()); assertEquals(22, session.getPort()); } From 19c43806890a5b7556d4d12b87f7ba2bb878cbf8 Mon Sep 17 00:00:00 2001 From: Juergen Denner Date: Tue, 15 Jan 2019 13:22:54 +0100 Subject: [PATCH 3/3] Reduce contention on PackFile.idx() function. In case of concurrent pack file access, threads may wait on the idx() function even for already open files. This happens especially with a slow file system. Performance numbers are listed in the bug report. Bug: 543739 Change-Id: Iff328d347fa65ae07ecce3267d44184161248978 Signed-off-by: Juergen Denner Signed-off-by: Matthias Sohn --- .../jgit/internal/storage/file/PackFile.java | 61 +++++++++++-------- 1 file changed, 35 insertions(+), 26 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java index d28c04fd6..3e5ef21cc 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackFile.java @@ -139,7 +139,7 @@ public class PackFile implements Iterable { private byte[] packChecksum; - private PackIndex loadedIdx; + private volatile PackIndex loadedIdx; private PackReverseIndex reverseIdx; @@ -174,35 +174,44 @@ public class PackFile implements Iterable { length = Long.MAX_VALUE; } - private synchronized PackIndex idx() throws IOException { - if (loadedIdx == null) { - if (invalid) - throw new PackInvalidException(packFile); - - try { - final PackIndex idx = PackIndex.open(extFile(INDEX)); - - if (packChecksum == null) { - packChecksum = idx.packChecksum; - } else if (!Arrays.equals(packChecksum, idx.packChecksum)) { - throw new PackMismatchException(MessageFormat.format( - JGitText.get().packChecksumMismatch, - packFile.getPath(), - ObjectId.fromRaw(packChecksum).name(), - ObjectId.fromRaw(idx.packChecksum).name())); + private PackIndex idx() throws IOException { + PackIndex idx = loadedIdx; + if (idx == null) { + synchronized (this) { + idx = loadedIdx; + if (idx == null) { + if (invalid) { + throw new PackInvalidException(packFile); + } + try { + idx = PackIndex.open(extFile(INDEX)); + + if (packChecksum == null) { + packChecksum = idx.packChecksum; + } else if (!Arrays.equals(packChecksum, + idx.packChecksum)) { + throw new PackMismatchException(MessageFormat + .format(JGitText.get().packChecksumMismatch, + packFile.getPath(), + ObjectId.fromRaw(packChecksum) + .name(), + ObjectId.fromRaw(idx.packChecksum) + .name())); + } + loadedIdx = idx; + } catch (InterruptedIOException e) { + // don't invalidate the pack, we are interrupted from + // another thread + throw e; + } catch (IOException e) { + invalid = true; + throw e; + } } - loadedIdx = idx; - } catch (InterruptedIOException e) { - // don't invalidate the pack, we are interrupted from another thread - throw e; - } catch (IOException e) { - invalid = true; - throw e; } } - return loadedIdx; + return idx; } - /** @return the File object which locates this pack on disk. */ public File getPackFile() { return packFile;