From fed0ab9baad30a58bfae30ce8b45e5ba3b1b1a10 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Tue, 28 Dec 2021 23:53:35 +0000 Subject: [PATCH] Use FileSnapshot without using configs for FileBasedConfig FileBasedConfig should not rely on auto-detection of the file-snapshot attribute computation based on config. The check was already performed when a new FileBasedConfig is created at L158: // don't use config in this snapshot to avoid endless recursion newSnapshot = FileSnapshot.saveNoConfig(getFile()); The check was missing though when the FileBasedConfig is saved to disk and the new snapshot is obtained from the associated LockFile. This change fixes the issue by keeping a non-config based FileSnapshot also after a FileBasedConfig is saved. Bug: 577983 Change-Id: Id1e410ba687e683ff2b2643af31e1110b103b356 --- .../storage/file/FileBasedConfigTest.java | 40 ++++++++++++++++++- .../jgit/internal/storage/file/LockFile.java | 25 +++++++++++- .../jgit/storage/file/FileBasedConfig.java | 2 +- 3 files changed, 62 insertions(+), 5 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/FileBasedConfigTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/FileBasedConfigTest.java index f3cd61da6..cfb2735a0 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/FileBasedConfigTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/FileBasedConfigTest.java @@ -46,6 +46,7 @@ import static org.eclipse.jgit.util.FileUtils.pathToString; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -53,6 +54,8 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardOpenOption; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.junit.MockSystemReader; @@ -82,11 +85,13 @@ public class FileBasedConfigTest { private Path trash; + private MockSystemReader mockSystemReader; + @Before public void setUp() throws Exception { - SystemReader.setInstance(new MockSystemReader()); + mockSystemReader = new MockSystemReader(); + SystemReader.setInstance(mockSystemReader); trash = Files.createTempDirectory("tmp_"); - FS.getFileStoreAttributes(trash.getParent()); } @After @@ -245,6 +250,37 @@ public void testIncludeWithTilde() assertEquals(ALICE, config.getString(USER, null, NAME)); } + @Test + public void testSavedConfigFileShouldNotReadUserGitConfig() + throws IOException { + AtomicBoolean userConfigTimeRead = new AtomicBoolean(false); + + Path userConfigFile = createFile(CONTENT1.getBytes(), "home"); + mockSystemReader.setUserGitConfig( + new FileBasedConfig(userConfigFile.toFile(), FS.DETECTED) { + + @Override + public long getTimeUnit(String section, String subsection, + String name, long defaultValue, TimeUnit wantUnit) { + userConfigTimeRead.set(true); + return super.getTimeUnit(section, subsection, name, + defaultValue, wantUnit); + } + }); + + Path file = createFile(CONTENT2.getBytes(), "repo"); + FileBasedConfig fileBasedConfig = new FileBasedConfig(file.toFile(), + FS.DETECTED); + fileBasedConfig.save(); + + // Needed to trigger the read of FileSnapshot filesystem settings + fileBasedConfig.isOutdated(); + assertFalse( + "User config should not be read when accessing config files " + + "for avoiding deadlocks", + userConfigTimeRead.get()); + } + private Path createFile(byte[] content) throws IOException { return createFile(content, null); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java index f7e78b94f..548ea5af5 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LockFile.java @@ -137,6 +137,8 @@ public boolean accept(File dir, String name) { private boolean needSnapshot; + private boolean snapshotNoConfig; + boolean fsync; private FileSnapshot commitSnapshot; @@ -406,6 +408,21 @@ public void setNeedSnapshot(boolean on) { needSnapshot = on; } + /** + * Request that {@link #commit()} remember the + * {@link org.eclipse.jgit.internal.storage.file.FileSnapshot} without using + * config file to get filesystem timestamp resolution. + * This method should be invoked before the file is accessed. + * It is used by FileBasedConfig to avoid endless recursion. + * + * @param on + * true if the commit method must remember the FileSnapshot. + */ + public void setNeedSnapshotNoConfig(boolean on) { + needSnapshot = on; + snapshotNoConfig = on; + } + /** * Request that {@link #commit()} force dirty data to the drive. * @@ -482,8 +499,12 @@ private void closeToken() { } private void saveStatInformation() { - if (needSnapshot) - commitSnapshot = FileSnapshot.save(lck); + if (needSnapshot) { + commitSnapshot = snapshotNoConfig ? + // don't use config in this snapshot to avoid endless recursion + FileSnapshot.saveNoConfig(lck) + : FileSnapshot.save(lck); + } } /** diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java index e7b0941f2..34829a226 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/FileBasedConfig.java @@ -236,7 +236,7 @@ public void save() throws IOException { if (!lf.lock()) throw new LockFailedException(getFile()); try { - lf.setNeedSnapshot(true); + lf.setNeedSnapshotNoConfig(true); lf.write(out); if (!lf.commit()) throw new IOException(MessageFormat.format(JGitText.get().cannotCommitWriteTo, getFile()));