From 7fedd15c82eda8c8ff694b6e809e83bd64a5ab33 Mon Sep 17 00:00:00 2001 From: Martin Fick Date: Thu, 5 Dec 2024 15:53:01 -0800 Subject: FileSnapshot: silence "Stale file handle" exceptions Sometimes a FileSystemException with "Stale file handle" can be thrown while creating a FileSnapshot, likely because the file or directory was deleted. Since NoSuchFileExceptions are already silenced, and the FileSnapshot already handles all IOExceptions, there is likely no value in seeing this info in the logs, treat these situation the same and silence them also. Change-Id: I922f4edf2d332cd704e60276f41a76df242f281c Signed-off-by: Martin Fick --- .../eclipse/jgit/internal/storage/file/FileSnapshot.java | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileSnapshot.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileSnapshot.java index d34baf358c..d752e315db 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileSnapshot.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileSnapshot.java @@ -28,6 +28,7 @@ import java.util.Objects; import java.util.concurrent.TimeUnit; import org.eclipse.jgit.annotations.NonNull; +import org.eclipse.jgit.util.FileUtils; import org.eclipse.jgit.util.FS; import org.eclipse.jgit.util.FS.FileStoreAttributes; import org.slf4j.Logger; @@ -580,11 +581,18 @@ public class FileSnapshot { private static BasicFileAttributes getFileAttributes(File path) throws NoSuchElementException { try { - return FS.DETECTED.fileAttributes(path); + try { + return FS.DETECTED.fileAttributes(path); + } catch (IOException e) { + if (!FileUtils.isStaleFileHandle(e)) { + throw e; + } + } } catch (NoSuchFileException e) { } catch (FileSystemException e) { - if (!e.getMessage().endsWith("Not a directory")) { - LOG.error(e.getMessage(), e); + String msg = e.getMessage(); + if (!msg.endsWith("Not a directory")) { + LOG.error(msg, e); } } catch (IOException e) { LOG.error(e.getMessage(), e); -- cgit v1.2.3 From 2b229df06c94fa351ce00227a107835e3b21f5ce Mon Sep 17 00:00:00 2001 From: Antonio Barone Date: Mon, 9 Dec 2024 16:22:18 +0100 Subject: Advertise "agent" capability when using protocol v2 The "agent" capability was not advertised by the server when executing git-upload-pack over protocol v2. This, in turn, prevented the clients from advertising its client agent too, as documented [1]: "The client may optionally send its own agent string by including the agent capability with a value Y (in the form agent=Y) in its request to the server (but it MUST NOT do so if the server did not advertise the agent capability)." When using jgit with Gerrit this had the effect of preventing the logging of the git client agent in the sshd_log. [1] https://git-scm.com/docs/protocol-v2#_agent Bug: jgit-118 Change-Id: Ifb6ea65fde020425920338f7dd9cc683fed6a4a4 --- .../tst/org/eclipse/jgit/transport/UploadPackTest.java | 12 ++++++------ .../src/org/eclipse/jgit/transport/UploadPack.java | 1 + 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java index 017104c527..2bd40031cc 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java @@ -502,15 +502,15 @@ public class UploadPackTest { assertThat(hook.capabilitiesRequest, notNullValue()); assertThat(pckIn.readString(), is("version 2")); assertThat( - Arrays.asList(pckIn.readString(), pckIn.readString(), - pckIn.readString()), + Arrays.asList(pckIn.readString(),pckIn.readString(), + pckIn.readString(), pckIn.readString()), // TODO(jonathantanmy) This check is written this way // to make it simple to see that we expect this list of // capabilities, but probably should be loosened to // allow additional commands to be added to the list, // and additional capabilities to be added to existing // commands without requiring test changes. - hasItems("ls-refs", "fetch=shallow", "server-option")); + hasItems("agent=" + UserAgent.get() ,"ls-refs", "fetch=shallow", "server-option")); assertTrue(PacketLineIn.isEnd(pckIn.readString())); } @@ -536,7 +536,7 @@ public class UploadPackTest { lines.add(line); } } - assertThat(lines, containsInAnyOrder("ls-refs", "fetch", "server-option")); + assertThat(lines, containsInAnyOrder("ls-refs", "fetch", "server-option", "agent=" + UserAgent.get())); } private void checkUnadvertisedIfUnallowed(String configSection, @@ -643,9 +643,9 @@ public class UploadPackTest { assertThat(pckIn.readString(), is("version 2")); assertThat( - Arrays.asList(pckIn.readString(), pckIn.readString(), + Arrays.asList(pckIn.readString(),pckIn.readString(), pckIn.readString(), pckIn.readString()), - hasItems("ls-refs", "fetch=shallow", "server-option")); + hasItems("agent="+ UserAgent.get(),"ls-refs", "fetch=shallow", "server-option")); assertTrue(PacketLineIn.isEnd(pckIn.readString())); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index 968728de8d..fbea7044f3 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -1441,6 +1441,7 @@ public class UploadPack implements Closeable { if (transferConfig.isAdvertiseObjectInfo()) { caps.add(COMMAND_OBJECT_INFO); } + caps.add(OPTION_AGENT + "=" + UserAgent.get()); return caps; } -- cgit v1.2.3 From d2ff398bcbd0177200651f684920b513a923ab82 Mon Sep 17 00:00:00 2001 From: Nasser Grainawi Date: Tue, 10 Dec 2024 17:36:26 -0700 Subject: Fix potential NPE in TreeWalk#getFilterCommandDefinition Change-Id: Ic6f0fd26153b47b4aeeec105bac431225d9bf8bf --- org.eclipse.jgit/src/org/eclipse/jgit/treewalk/TreeWalk.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/TreeWalk.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/TreeWalk.java index aaac2a72e0..b789c5120e 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/TreeWalk.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/TreeWalk.java @@ -1587,6 +1587,9 @@ public class TreeWalk implements AutoCloseable, AttributesProvider { */ private String getFilterCommandDefinition(String filterDriverName, String filterCommandType) { + if (config == null) { + return null; + } String key = filterDriverName + "." + filterCommandType; //$NON-NLS-1$ String filterCommand = filterCommandsByNameDotType.get(key); if (filterCommand != null) -- cgit v1.2.3 From ae53d63837857d886cdf15442118597be717ff33 Mon Sep 17 00:00:00 2001 From: Martin Fick Date: Wed, 11 Dec 2024 17:00:17 -0800 Subject: Pack: fix threading bug getting idx When converting to Optionally, a threading bug was introduced, fix it. The Optionally class itself is not thread safe, and previously it was being called from idx() without any thread synchronization mechanism. This was because previously the variable which held the cached index was volatile. The Optionally kept the volatile attribute, but that only synchronizes the reference to the Optionally, and not the element inside the Optionally. A clear() from another thread could thus be missed by the idx() call, potentially allowing the idx() call to interact poorly with the Optionally, potentially even causing a memory leak. To fix this, extend the synchronized inside the idx() method to the entire method. Then, additionally remove the now redundant Optional fetch in idx() and the no longer needed volatile. Change-Id: I6077e1aaed96c53200805b4c87a67afb49c2b373 Signed-off-by: Martin Fick --- .../eclipse/jgit/internal/storage/file/Pack.java | 86 ++++++++++------------ 1 file changed, 40 insertions(+), 46 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java index 61577a956c..8d2a86386f 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java @@ -119,7 +119,7 @@ public class Pack implements Iterable { private byte[] packChecksum; - private volatile Optionally loadedIdx = Optionally.empty(); + private Optionally loadedIdx = Optionally.empty(); private Optionally reverseIdx = Optionally.empty(); @@ -159,60 +159,54 @@ public class Pack implements Iterable { length = Long.MAX_VALUE; } - private PackIndex idx() throws IOException { + private synchronized PackIndex idx() throws IOException { Optional optional = loadedIdx.getOptional(); if (optional.isPresent()) { return optional.get(); } - synchronized (this) { - optional = loadedIdx.getOptional(); - if (optional.isPresent()) { - return optional.get(); - } - if (invalid) { - throw new PackInvalidException(packFile, invalidatingCause); + if (invalid) { + throw new PackInvalidException(packFile, invalidatingCause); + } + try { + long start = System.currentTimeMillis(); + PackFile idxFile = packFile.create(INDEX); + PackIndex idx = PackIndex.open(idxFile); + if (LOG.isDebugEnabled()) { + LOG.debug(String.format( + "Opening pack index %s, size %.3f MB took %d ms", //$NON-NLS-1$ + idxFile.getAbsolutePath(), + Float.valueOf(idxFile.length() + / (1024f * 1024)), + Long.valueOf(System.currentTimeMillis() + - start))); } - try { - long start = System.currentTimeMillis(); - PackFile idxFile = packFile.create(INDEX); - PackIndex idx = PackIndex.open(idxFile); - if (LOG.isDebugEnabled()) { - LOG.debug(String.format( - "Opening pack index %s, size %.3f MB took %d ms", //$NON-NLS-1$ - idxFile.getAbsolutePath(), - Float.valueOf(idxFile.length() - / (1024f * 1024)), - Long.valueOf(System.currentTimeMillis() - - start))); - } - if (packChecksum == null) { - packChecksum = idx.getChecksum(); - fileSnapshot.setChecksum( - ObjectId.fromRaw(packChecksum)); - } else if (!Arrays.equals(packChecksum, - idx.getChecksum())) { - throw new PackMismatchException(MessageFormat - .format(JGitText.get().packChecksumMismatch, - packFile.getPath(), - PackExt.PACK.getExtension(), - Hex.toHexString(packChecksum), - PackExt.INDEX.getExtension(), - Hex.toHexString(idx.getChecksum()))); - } - loadedIdx = optionally(idx); - return idx; - } catch (InterruptedIOException e) { - // don't invalidate the pack, we are interrupted from - // another thread - throw e; - } catch (IOException e) { - invalid = true; - invalidatingCause = e; - throw e; + packChecksum = idx.getChecksum(); + fileSnapshot.setChecksum( + ObjectId.fromRaw(packChecksum)); + } else if (!Arrays.equals(packChecksum, + idx.getChecksum())) { + throw new PackMismatchException(MessageFormat + .format(JGitText.get().packChecksumMismatch, + packFile.getPath(), + PackExt.PACK.getExtension(), + Hex.toHexString(packChecksum), + PackExt.INDEX.getExtension(), + Hex.toHexString(idx.getChecksum()))); } + loadedIdx = optionally(idx); + return idx; + } catch (InterruptedIOException e) { + // don't invalidate the pack, we are interrupted from + // another thread + throw e; + } catch (IOException e) { + invalid = true; + invalidatingCause = e; + throw e; } } + /** * Get the File object which locates this pack on disk. * -- cgit v1.2.3 From cdbea5ea96f7e473a48e6bff15e84ecccd521800 Mon Sep 17 00:00:00 2001 From: Martin Fick Date: Wed, 11 Dec 2024 17:26:57 -0800 Subject: Optionally.Hard: avoid Optional creation on every use, Previously the getOptional() call created an new Optional for its element, and this appears to be somewhat expensive. During a time when a server is heavily loaded because of a poorly maintained repository with potentially 2K+ pack files, calls to Optionally.ofNullable() from within the getOptional() method appeared extensively in the Stacktrace. Reduce the getOptional() call overhead by storing an already created Optional of the element instead of the element itself. This trades the extra space of one extra reference for a potential speed gain in a hotspot. Since the current users of Optionally.Hard reference objects significantly larger than a single reference (and most users are likely to be, else why would they be using an Optionally?), this is likely a good tradeoff. Change-Id: I7adccc915480cbb97a43dcbe074cfb620888c936 Signed-off-by: Martin Fick --- .../src/org/eclipse/jgit/internal/util/Optionally.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/util/Optionally.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/util/Optionally.java index 3447f669ab..270b760562 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/util/Optionally.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/util/Optionally.java @@ -53,24 +53,24 @@ public interface Optionally { /** * The mutable optional object */ - protected T element; + protected Optional optional; /** * @param element * the mutable optional object */ public Hard(T element) { - this.element = element; + optional = Optional.ofNullable(element); } @Override public void clear() { - element = null; + optional = Optional.empty(); } @Override public Optional getOptional() { - return Optional.ofNullable(element); + return optional; } } -- cgit v1.2.3 From fef56c3c81cce48819fdc8247203717d87ba17c9 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Sun, 15 Dec 2024 20:51:34 +0100 Subject: FileSnapshot: fix warnings - comment empty code block - suppress non-translatable text warning Change-Id: Id49b4a56bbe5454edfe1ea8b79ceeaf51ceac370 --- .../src/org/eclipse/jgit/internal/storage/file/FileSnapshot.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileSnapshot.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileSnapshot.java index d752e315db..7f34aed902 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileSnapshot.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileSnapshot.java @@ -589,9 +589,10 @@ public class FileSnapshot { } } } catch (NoSuchFileException e) { + // ignore } catch (FileSystemException e) { String msg = e.getMessage(); - if (!msg.endsWith("Not a directory")) { + if (!msg.endsWith("Not a directory")) { //$NON-NLS-1$ LOG.error(msg, e); } } catch (IOException e) { -- cgit v1.2.3