Config.set-methods should not touch lines from included files Bug: 538270 Change-Id: I4128213e83e267eb2667f451b8fb3301dd251656 Signed-off-by: Marc Strapetz <marc.strapetz@syntevo.com>
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConfigTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConfigTest.java index c4c4da8..6e2cddb 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConfigTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ConfigTest.java
@@ -66,11 +66,15 @@ import java.io.IOException; import java.nio.file.Files; import java.text.MessageFormat; +import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.Iterator; import java.util.LinkedList; +import java.util.List; import java.util.Set; import java.util.concurrent.TimeUnit; +import java.util.function.Consumer; import org.eclipse.jgit.api.MergeCommand.FastForwardMode; import org.eclipse.jgit.errors.ConfigInvalidException; @@ -78,6 +82,7 @@ import org.eclipse.jgit.junit.MockSystemReader; import org.eclipse.jgit.merge.MergeConfig; import org.eclipse.jgit.storage.file.FileBasedConfig; +import org.eclipse.jgit.transport.RefSpec; import org.eclipse.jgit.util.FS; import org.eclipse.jgit.util.SystemReader; import org.junit.After; @@ -94,6 +99,12 @@ public class ConfigTest { // A non-ASCII whitespace character: U+2002 EN QUAD. private static final char WS = '\u2002'; + private static final String REFS_ORIGIN = "+refs/heads/*:refs/remotes/origin/*"; + + private static final String REFS_UPSTREAM = "+refs/heads/*:refs/remotes/upstream/*"; + + private static final String REFS_BACKUP = "+refs/heads/*:refs/remotes/backup/*"; + @Rule public ExpectedException expectedEx = ExpectedException.none(); @@ -802,10 +813,8 @@ public void testIncludeTooManyRecursions() throws IOException { File config = tmp.newFile("config"); String include = "[include]\npath=" + pathToString(config) + "\n"; Files.write(config.toPath(), include.getBytes()); - FileBasedConfig fbConfig = new FileBasedConfig(null, config, - FS.DETECTED); try { - fbConfig.load(); + loadConfig(config); fail(); } catch (ConfigInvalidException cie) { for (Throwable t = cie; t != null; t = t.getCause()) { @@ -841,9 +850,7 @@ public void testIncludeCaseInsensitiveSection() content = "[Include]\npath=" + pathToString(included) + "\n"; Files.write(config.toPath(), content.getBytes()); - FileBasedConfig fbConfig = new FileBasedConfig(null, config, - FS.DETECTED); - fbConfig.load(); + FileBasedConfig fbConfig = loadConfig(config); assertTrue(fbConfig.getBoolean("foo", "bar", false)); } @@ -858,9 +865,7 @@ public void testIncludeCaseInsensitiveKey() content = "[include]\nPath=" + pathToString(included) + "\n"; Files.write(config.toPath(), content.getBytes()); - FileBasedConfig fbConfig = new FileBasedConfig(null, config, - FS.DETECTED); - fbConfig.load(); + FileBasedConfig fbConfig = loadConfig(config); assertTrue(fbConfig.getBoolean("foo", "bar", false)); } @@ -886,10 +891,8 @@ public void testIncludeExceptionContainsFile() throws IOException { File config = tmp.newFile("config"); String include = "[include]\npath=" + includedPath + "\n"; Files.write(config.toPath(), include.getBytes()); - FileBasedConfig fbConfig = new FileBasedConfig(null, config, - FS.DETECTED); try { - fbConfig.load(); + loadConfig(config); fail("Expected ConfigInvalidException"); } catch (ConfigInvalidException e) { // Check that there is some exception in the chain that contains @@ -904,6 +907,306 @@ public void testIncludeExceptionContainsFile() throws IOException { } } + @Test + public void testIncludeSetValueMustNotTouchIncludedLines1() + throws IOException, ConfigInvalidException { + File includedFile = createAllTypesIncludedContent(); + + File configFile = tmp.newFile("config"); + String content = createAllTypesSampleContent("Alice Parker", false, 11, + 21, 31, CoreConfig.AutoCRLF.FALSE, + "+refs/heads/*:refs/remotes/origin/*") + "\n[include]\npath=" + + pathToString(includedFile); + Files.write(configFile.toPath(), content.getBytes()); + + FileBasedConfig fbConfig = loadConfig(configFile); + assertValuesAsIncluded(fbConfig, REFS_ORIGIN, REFS_UPSTREAM); + assertSections(fbConfig, "user", "core", "remote", "include"); + + setAllValuesNew(fbConfig); + assertValuesAsIsSaveLoad(fbConfig, config -> { + assertValuesAsIncluded(config, REFS_BACKUP, REFS_UPSTREAM); + assertSections(fbConfig, "user", "core", "remote", "include"); + }); + } + + @Test + public void testIncludeSetValueMustNotTouchIncludedLines2() + throws IOException, ConfigInvalidException { + File includedFile = createAllTypesIncludedContent(); + + File configFile = tmp.newFile("config"); + String content = "[include]\npath=" + pathToString(includedFile) + "\n" + + createAllTypesSampleContent("Alice Parker", false, 11, 21, 31, + CoreConfig.AutoCRLF.FALSE, + "+refs/heads/*:refs/remotes/origin/*"); + Files.write(configFile.toPath(), content.getBytes()); + + FileBasedConfig fbConfig = loadConfig(configFile); + assertValuesAsConfig(fbConfig, REFS_UPSTREAM, REFS_ORIGIN); + assertSections(fbConfig, "include", "user", "core", "remote"); + + setAllValuesNew(fbConfig); + assertValuesAsIsSaveLoad(fbConfig, config -> { + assertValuesAsNew(config, REFS_UPSTREAM, REFS_BACKUP); + assertSections(fbConfig, "include", "user", "core", "remote"); + }); + } + + @Test + public void testIncludeSetValueOnFileWithJustContainsInclude() + throws IOException, ConfigInvalidException { + File includedFile = createAllTypesIncludedContent(); + + File configFile = tmp.newFile("config"); + String content = "[include]\npath=" + pathToString(includedFile); + Files.write(configFile.toPath(), content.getBytes()); + + FileBasedConfig fbConfig = loadConfig(configFile); + assertValuesAsIncluded(fbConfig, REFS_UPSTREAM); + assertSections(fbConfig, "include", "user", "core", "remote"); + + setAllValuesNew(fbConfig); + assertValuesAsIsSaveLoad(fbConfig, config -> { + assertValuesAsNew(config, REFS_UPSTREAM, REFS_BACKUP); + assertSections(fbConfig, "include", "user", "core", "remote"); + }); + } + + @Test + public void testIncludeSetValueOnFileWithJustEmptySection1() + throws IOException, ConfigInvalidException { + File includedFile = createAllTypesIncludedContent(); + + File configFile = tmp.newFile("config"); + String content = "[user]\n[include]\npath=" + + pathToString(includedFile); + Files.write(configFile.toPath(), content.getBytes()); + + FileBasedConfig fbConfig = loadConfig(configFile); + assertValuesAsIncluded(fbConfig, REFS_UPSTREAM); + assertSections(fbConfig, "user", "include", "core", "remote"); + + setAllValuesNew(fbConfig); + assertValuesAsIsSaveLoad(fbConfig, config -> { + assertValuesAsNewWithName(config, "Alice Muller", REFS_UPSTREAM, + REFS_BACKUP); + assertSections(fbConfig, "user", "include", "core", "remote"); + }); + } + + @Test + public void testIncludeSetValueOnFileWithJustEmptySection2() + throws IOException, ConfigInvalidException { + File includedFile = createAllTypesIncludedContent(); + + File configFile = tmp.newFile("config"); + String content = "[include]\npath=" + pathToString(includedFile) + + "\n[user]"; + Files.write(configFile.toPath(), content.getBytes()); + + FileBasedConfig fbConfig = loadConfig(configFile); + assertValuesAsIncluded(fbConfig, REFS_UPSTREAM); + assertSections(fbConfig, "include", "user", "core", "remote"); + + setAllValuesNew(fbConfig); + assertValuesAsIsSaveLoad(fbConfig, config -> { + assertValuesAsNew(config, REFS_UPSTREAM, REFS_BACKUP); + assertSections(fbConfig, "include", "user", "core", "remote"); + }); + } + + @Test + public void testIncludeSetValueOnFileWithJustExistingSection1() + throws IOException, ConfigInvalidException { + File includedFile = createAllTypesIncludedContent(); + + File configFile = tmp.newFile("config"); + String content = "[user]\nemail=alice@home\n[include]\npath=" + + pathToString(includedFile); + Files.write(configFile.toPath(), content.getBytes()); + + FileBasedConfig fbConfig = loadConfig(configFile); + assertValuesAsIncluded(fbConfig, REFS_UPSTREAM); + assertSections(fbConfig, "user", "include", "core", "remote"); + + setAllValuesNew(fbConfig); + assertValuesAsIsSaveLoad(fbConfig, config -> { + assertValuesAsNewWithName(config, "Alice Muller", REFS_UPSTREAM, + REFS_BACKUP); + assertSections(fbConfig, "user", "include", "core", "remote"); + }); + } + + @Test + public void testIncludeSetValueOnFileWithJustExistingSection2() + throws IOException, ConfigInvalidException { + File includedFile = createAllTypesIncludedContent(); + + File configFile = tmp.newFile("config"); + String content = "[include]\npath=" + pathToString(includedFile) + + "\n[user]\nemail=alice@home\n"; + Files.write(configFile.toPath(), content.getBytes()); + + FileBasedConfig fbConfig = loadConfig(configFile); + assertValuesAsIncluded(fbConfig, REFS_UPSTREAM); + assertSections(fbConfig, "include", "user", "core", "remote"); + + setAllValuesNew(fbConfig); + assertValuesAsIsSaveLoad(fbConfig, config -> { + assertValuesAsNew(config, REFS_UPSTREAM, REFS_BACKUP); + assertSections(fbConfig, "include", "user", "core", "remote"); + }); + } + + @Test + public void testIncludeUnsetSectionMustNotTouchIncludedLines() + throws IOException, ConfigInvalidException { + File includedFile = tmp.newFile("included"); + RefSpec includedRefSpec = new RefSpec(REFS_UPSTREAM); + String includedContent = "[remote \"origin\"]\n" + "fetch=" + + includedRefSpec; + Files.write(includedFile.toPath(), includedContent.getBytes()); + + File configFile = tmp.newFile("config"); + RefSpec refSpec = new RefSpec(REFS_ORIGIN); + String content = "[include]\npath=" + pathToString(includedFile) + "\n" + + "[remote \"origin\"]\n" + "fetch=" + refSpec; + Files.write(configFile.toPath(), content.getBytes()); + + FileBasedConfig fbConfig = loadConfig(configFile); + + Consumer<FileBasedConfig> assertion = config -> { + assertEquals(Arrays.asList(includedRefSpec, refSpec), + config.getRefSpecs("remote", "origin", "fetch")); + }; + assertion.accept(fbConfig); + + fbConfig.unsetSection("remote", "origin"); + assertValuesAsIsSaveLoad(fbConfig, config -> { + assertEquals(Collections.singletonList(includedRefSpec), + config.getRefSpecs("remote", "origin", "fetch")); + }); + } + + private File createAllTypesIncludedContent() throws IOException { + File includedFile = tmp.newFile("included"); + String includedContent = createAllTypesSampleContent("Alice Muller", + true, 10, 20, 30, CoreConfig.AutoCRLF.TRUE, + "+refs/heads/*:refs/remotes/upstream/*"); + Files.write(includedFile.toPath(), includedContent.getBytes()); + return includedFile; + } + + private static void assertValuesAsIsSaveLoad(FileBasedConfig fbConfig, + Consumer<FileBasedConfig> assertion) + throws IOException, ConfigInvalidException { + assertion.accept(fbConfig); + + fbConfig.save(); + assertion.accept(fbConfig); + + fbConfig = loadConfig(fbConfig.getFile()); + assertion.accept(fbConfig); + } + + private static void setAllValuesNew(Config config) { + config.setString("user", null, "name", "Alice Bauer"); + config.setBoolean("core", null, "fileMode", false); + config.setInt("core", null, "deltaBaseCacheLimit", 12); + config.setLong("core", null, "packedGitLimit", 22); + config.setLong("core", null, "repositoryCacheExpireAfter", 32); + config.setEnum("core", null, "autocrlf", CoreConfig.AutoCRLF.FALSE); + config.setString("remote", "origin", "fetch", + "+refs/heads/*:refs/remotes/backup/*"); + } + + private static void assertValuesAsIncluded(Config config, String... refs) { + assertAllTypesSampleContent("Alice Muller", true, 10, 20, 30, + CoreConfig.AutoCRLF.TRUE, config, refs); + } + + private static void assertValuesAsConfig(Config config, String... refs) { + assertAllTypesSampleContent("Alice Parker", false, 11, 21, 31, + CoreConfig.AutoCRLF.FALSE, config, refs); + } + + private static void assertValuesAsNew(Config config, String... refs) { + assertValuesAsNewWithName(config, "Alice Bauer", refs); + } + + private static void assertValuesAsNewWithName(Config config, String name, + String... refs) { + assertAllTypesSampleContent(name, false, 12, 22, 32, + CoreConfig.AutoCRLF.FALSE, config, refs); + } + + private static void assertSections(Config config, String... sections) { + assertEquals(Arrays.asList(sections), + new ArrayList<>(config.getSections())); + } + + private static String createAllTypesSampleContent(String name, + boolean fileMode, int deltaBaseCacheLimit, long packedGitLimit, + long repositoryCacheExpireAfter, CoreConfig.AutoCRLF autoCRLF, + String fetchRefSpec) { + final StringBuilder builder = new StringBuilder(); + builder.append("[user]\n"); + builder.append("name="); + builder.append(name); + builder.append("\n"); + + builder.append("[core]\n"); + builder.append("fileMode="); + builder.append(fileMode); + builder.append("\n"); + + builder.append("deltaBaseCacheLimit="); + builder.append(deltaBaseCacheLimit); + builder.append("\n"); + + builder.append("packedGitLimit="); + builder.append(packedGitLimit); + builder.append("\n"); + + builder.append("repositoryCacheExpireAfter="); + builder.append(repositoryCacheExpireAfter); + builder.append("\n"); + + builder.append("autocrlf="); + builder.append(autoCRLF.name()); + builder.append("\n"); + + builder.append("[remote \"origin\"]\n"); + builder.append("fetch="); + builder.append(fetchRefSpec); + builder.append("\n"); + return builder.toString(); + } + + private static void assertAllTypesSampleContent(String name, + boolean fileMode, int deltaBaseCacheLimit, long packedGitLimit, + long repositoryCacheExpireAfter, CoreConfig.AutoCRLF autoCRLF, + Config config, String... fetchRefSpecs) { + assertEquals(name, config.getString("user", null, "name")); + assertEquals(fileMode, + config.getBoolean("core", "fileMode", !fileMode)); + assertEquals(deltaBaseCacheLimit, + config.getInt("core", "deltaBaseCacheLimit", -1)); + assertEquals(packedGitLimit, + config.getLong("core", "packedGitLimit", -1)); + assertEquals(repositoryCacheExpireAfter, config.getTimeUnit("core", + null, "repositoryCacheExpireAfter", -1, MILLISECONDS)); + assertEquals(autoCRLF, config.getEnum("core", null, "autocrlf", + CoreConfig.AutoCRLF.INPUT)); + final List<RefSpec> refspecs = new ArrayList<>(); + for (String fetchRefSpec : fetchRefSpecs) { + refspecs.add(new RefSpec(fetchRefSpec)); + } + + assertEquals(refspecs, config.getRefSpecs("remote", "origin", "fetch")); + } + private static void assertReadLong(long exp) throws ConfigInvalidException { assertReadLong(exp, String.valueOf(exp)); } @@ -1217,4 +1520,12 @@ private static void assertInvalidSubsection(String expectedMessage, assertEquals(expectedMessage, e.getMessage()); } } + + private static FileBasedConfig loadConfig(File file) + throws IOException, ConfigInvalidException { + final FileBasedConfig config = new FileBasedConfig(null, file, + FS.DETECTED); + config.load(); + return config; + } }
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java index 97acac2..8558e3d 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java
@@ -868,7 +868,7 @@ private ConfigSnapshot unsetSection(final ConfigSnapshot srcState, boolean lastWasMatch = false; for (ConfigLine e : srcState.entryList) { - if (e.match(section, subsection)) { + if (e.includedFrom == null && e.match(section, subsection)) { // Skip this record, it's for the section we are removing. lastWasMatch = true; continue; @@ -923,7 +923,7 @@ private ConfigSnapshot replaceStringList(final ConfigSnapshot srcState, // while (entryIndex < entries.size() && valueIndex < values.size()) { final ConfigLine e = entries.get(entryIndex); - if (e.match(section, subsection, name)) { + if (e.includedFrom == null && e.match(section, subsection, name)) { entries.set(entryIndex, e.forValue(values.get(valueIndex++))); insertPosition = entryIndex + 1; } @@ -935,7 +935,8 @@ private ConfigSnapshot replaceStringList(final ConfigSnapshot srcState, if (valueIndex == values.size() && entryIndex < entries.size()) { while (entryIndex < entries.size()) { final ConfigLine e = entries.get(entryIndex++); - if (e.match(section, subsection, name)) + if (e.includedFrom == null + && e.match(section, subsection, name)) entries.remove(--entryIndex); } } @@ -948,7 +949,8 @@ private ConfigSnapshot replaceStringList(final ConfigSnapshot srcState, // is already a section available that matches. Insert // after the last key of that section. // - insertPosition = findSectionEnd(entries, section, subsection); + insertPosition = findSectionEnd(entries, section, subsection, + true); } if (insertPosition < 0) { // We didn't find any matching section header for this key, @@ -985,9 +987,14 @@ private static List<ConfigLine> copy(final ConfigSnapshot src, } private static int findSectionEnd(final List<ConfigLine> entries, - final String section, final String subsection) { + final String section, final String subsection, + boolean skipIncludedLines) { for (int i = 0; i < entries.size(); i++) { ConfigLine e = entries.get(i); + if (e.includedFrom != null && skipIncludedLines) { + continue; + } + if (e.match(section, subsection, null)) { i++; while (i < entries.size()) {