diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/T0003_BasicTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/T0003_BasicTest.java index a3a435674..ab86bc2e2 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/T0003_BasicTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/T0003_BasicTest.java @@ -56,7 +56,6 @@ import static org.junit.Assert.fail; import java.io.File; import java.io.FileInputStream; -import java.io.FileReader; import java.io.IOException; import java.io.UnsupportedEncodingException; @@ -83,6 +82,7 @@ import org.eclipse.jgit.storage.file.FileBasedConfig; import org.eclipse.jgit.storage.file.FileRepositoryBuilder; import org.eclipse.jgit.test.resources.SampleDataRepositoryTestCase; import org.eclipse.jgit.util.FileUtils; +import org.eclipse.jgit.util.IO; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -361,11 +361,15 @@ public class T0003_BasicTest extends SampleDataRepositoryTestCase { assertEquals("a many line\ncomment\n to test", c.getString("user", null, "defaultCheckInComment")); c.save(); - final FileReader fr = new FileReader(cfg); - final char[] cbuf = new char[configStr.length()]; - fr.read(cbuf); - fr.close(); - assertEquals(configStr, new String(cbuf)); + + // Saving normalizes out the weird "\\n\\\n" to a single escaped newline, + // and quotes the whole string. + final String expectedStr = " [core];comment\n\tfilemode = yes\n" + + "[user]\n" + + " email = A U Thor # Just an example...\n" + + " name = \"A Thor \\\\ \\\"\\t \"\n" + + " defaultCheckInComment = \"a many line\\ncomment\\n to test\"\n"; + assertEquals(expectedStr, new String(IO.readFully(cfg), Constants.CHARSET)); } @Test 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 712e9df44..44714faa7 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 @@ -89,7 +89,10 @@ import org.junit.rules.TemporaryFolder; /** * Test reading of git config */ +@SuppressWarnings("boxing") public class ConfigTest { + // A non-ASCII whitespace character: U+2002 EN QUAD. + private static final char WS = '\u2002'; @Rule public ExpectedException expectedEx = ExpectedException.none(); @@ -666,28 +669,6 @@ public class ConfigTest { assertTrue("Subsection should contain \"B\"", names.contains("B")); } - @Test - public void testQuotingForSubSectionNames() { - String resultPattern = "[testsection \"{0}\"]\n\ttestname = testvalue\n"; - String result; - - Config config = new Config(); - config.setString("testsection", "testsubsection", "testname", - "testvalue"); - - result = MessageFormat.format(resultPattern, "testsubsection"); - assertEquals(result, config.toText()); - config.clear(); - - config.setString("testsection", "#quotable", "testname", "testvalue"); - result = MessageFormat.format(resultPattern, "#quotable"); - assertEquals(result, config.toText()); - config.clear(); - - config.setString("testsection", "with\"quote", "testname", "testvalue"); - result = MessageFormat.format(resultPattern, "with\\\"quote"); - assertEquals(result, config.toText()); - } @Test public void testNoFinalNewline() throws ConfigInvalidException { @@ -956,53 +937,187 @@ public class ConfigTest { @Test public void testEscapeSpacesOnly() throws ConfigInvalidException { + // Empty string is read back as null, so this doesn't round-trip. assertEquals("", Config.escapeValue("")); - assertEquals("\" \"", Config.escapeValue(" ")); - assertEquals("\" \"", Config.escapeValue(" ")); - assertParseRoundTrip(" "); - assertParseRoundTrip(" "); + assertValueRoundTrip(" ", "\" \""); + assertValueRoundTrip(" ", "\" \""); } @Test public void testEscapeLeadingSpace() throws ConfigInvalidException { - assertEquals("x", Config.escapeValue("x")); - assertEquals("\" x\"", Config.escapeValue(" x")); - assertEquals("\" x\"", Config.escapeValue(" x")); - - assertParseRoundTrip("x"); - assertParseRoundTrip(" x"); - assertParseRoundTrip(" x"); + assertValueRoundTrip("x", "x"); + assertValueRoundTrip(" x", "\" x\""); + assertValueRoundTrip(" x", "\" x\""); } @Test public void testEscapeTrailingSpace() throws ConfigInvalidException { - assertEquals("x", Config.escapeValue("x")); - assertEquals("\"x \"", Config.escapeValue("x ")); - assertEquals("x\" \"", Config.escapeValue("x ")); - - assertParseRoundTrip("x"); - assertParseRoundTrip("x "); - assertParseRoundTrip("x "); + assertValueRoundTrip("x", "x"); + assertValueRoundTrip("x ","\"x \""); + assertValueRoundTrip("x ","\"x \""); } @Test public void testEscapeLeadingAndTrailingSpace() throws ConfigInvalidException { - assertEquals("\" x \"", Config.escapeValue(" x ")); - assertEquals("\" x \"", Config.escapeValue(" x ")); - assertEquals("\" x \"", Config.escapeValue(" x ")); - assertEquals("\" x \"", Config.escapeValue(" x ")); + assertValueRoundTrip(" x ", "\" x \""); + assertValueRoundTrip(" x ", "\" x \""); + assertValueRoundTrip(" x ", "\" x \""); + assertValueRoundTrip(" x ", "\" x \""); + } + + @Test + public void testNoEscapeInternalSpaces() throws ConfigInvalidException { + assertValueRoundTrip("x y"); + assertValueRoundTrip("x y"); + assertValueRoundTrip("x y"); + assertValueRoundTrip("x y z"); + assertValueRoundTrip("x " + WS + " y"); + } + + @Test + public void testEscapeSpecialCharacters() throws ConfigInvalidException { + assertValueRoundTrip("x#y", "\"x#y\""); + assertValueRoundTrip("x;y", "\"x;y\""); + assertValueRoundTrip("x\\y", "\"x\\\\y\""); + assertValueRoundTrip("x\"y", "\"x\\\"y\""); + assertValueRoundTrip("x\ny", "\"x\\ny\""); + assertValueRoundTrip("x\ty", "\"x\\ty\""); + assertValueRoundTrip("x\by", "\"x\\by\""); + } + + @Test + public void testEscapeValueInvalidCharacters() { + assertIllegalArgumentException(() -> Config.escapeSubsection("x\0y")); + } + + @Test + public void testEscapeSubsectionInvalidCharacters() { + assertIllegalArgumentException(() -> Config.escapeSubsection("x\ny")); + assertIllegalArgumentException(() -> Config.escapeSubsection("x\0y")); + } + + @Test + public void testParseMultipleQuotedRegions() throws ConfigInvalidException { + assertEquals("b a z; \n", parseEscapedValue("b\" a\"\" z; \\n\"")); + } + + @Test + public void testParseComments() throws ConfigInvalidException { + assertEquals("baz", parseEscapedValue("baz; comment")); + assertEquals("baz", parseEscapedValue("baz# comment")); + assertEquals("baz", parseEscapedValue("baz ; comment")); + assertEquals("baz", parseEscapedValue("baz # comment")); + + assertEquals("baz", parseEscapedValue("baz ; comment")); + assertEquals("baz", parseEscapedValue("baz # comment")); + assertEquals("baz", parseEscapedValue("baz " + WS + " ; comment")); + assertEquals("baz", parseEscapedValue("baz " + WS + " # comment")); + } + + @Test + public void testEscapeSubsection() throws ConfigInvalidException { + assertSubsectionRoundTrip("", "\"\""); + assertSubsectionRoundTrip("x", "\"x\""); + assertSubsectionRoundTrip(" x", "\" x\""); + assertSubsectionRoundTrip("x ", "\"x \""); + assertSubsectionRoundTrip(" x ", "\" x \""); + assertSubsectionRoundTrip("x y", "\"x y\""); + assertSubsectionRoundTrip("x y", "\"x y\""); + assertSubsectionRoundTrip("x\\y", "\"x\\\\y\""); + assertSubsectionRoundTrip("x\"y", "\"x\\\"y\""); + + // Unlike for values, \b and \t are not escaped. + assertSubsectionRoundTrip("x\by", "\"x\by\""); + assertSubsectionRoundTrip("x\ty", "\"x\ty\""); + } + + @Test + public void testParseInvalidValues() { + assertInvalidValue(JGitText.get().newlineInQuotesNotAllowed, "x\"\n\"y"); + assertInvalidValue(JGitText.get().endOfFileInEscape, "x\\"); + assertInvalidValue( + MessageFormat.format(JGitText.get().badEscape, 'q'), "x\\q"); + } + + @Test + public void testParseInvalidSubsections() { + assertInvalidSubsection( + JGitText.get().newlineInQuotesNotAllowed, "\"x\ny\""); + assertInvalidSubsection( + MessageFormat.format(JGitText.get().badEscape, 'q'), "\"x\\q\""); + + // Unlike for values, \b, \n, and \t are not valid escape sequences. + assertInvalidSubsection( + MessageFormat.format(JGitText.get().badEscape, 'b'), "\"x\\b\""); + assertInvalidSubsection( + MessageFormat.format(JGitText.get().badEscape, 'n'), "\"x\\n\""); + assertInvalidSubsection( + MessageFormat.format(JGitText.get().badEscape, 't'), "\"x\\t\""); + } + + private static void assertValueRoundTrip(String value) + throws ConfigInvalidException { + assertValueRoundTrip(value, value); + } + + private static void assertValueRoundTrip(String value, String expectedEscaped) + throws ConfigInvalidException { + String escaped = Config.escapeValue(value); + assertEquals("escape failed;", expectedEscaped, escaped); + assertEquals("parse failed;", value, parseEscapedValue(escaped)); + } + + private static String parseEscapedValue(String escapedValue) + throws ConfigInvalidException { + String text = "[foo]\nbar=" + escapedValue; + Config c = parse(text); + return c.getString("foo", null, "bar"); + } + + private static void assertInvalidValue(String expectedMessage, + String escapedValue) { + try { + parseEscapedValue(escapedValue); + fail("expected ConfigInvalidException"); + } catch (ConfigInvalidException e) { + assertEquals(expectedMessage, e.getMessage()); + } + } - assertParseRoundTrip(" x "); - assertParseRoundTrip(" x "); - assertParseRoundTrip(" x "); - assertParseRoundTrip(" x "); + private static void assertSubsectionRoundTrip(String subsection, + String expectedEscaped) throws ConfigInvalidException { + String escaped = Config.escapeSubsection(subsection); + assertEquals("escape failed;", expectedEscaped, escaped); + assertEquals("parse failed;", subsection, parseEscapedSubsection(escaped)); } - private static void assertParseRoundTrip(String value) + private static String parseEscapedSubsection(String escapedSubsection) throws ConfigInvalidException { - Config c = parse("[foo]\nbar = " + Config.escapeValue(value)); - assertEquals(value, c.getString("foo", null, "bar")); + String text = "[foo " + escapedSubsection + "]\nbar = value"; + Config c = parse(text); + Set subsections = c.getSubsections("foo"); + assertEquals("only one section", 1, subsections.size()); + return subsections.iterator().next(); + } + + private static void assertIllegalArgumentException(Runnable r) { + try { + r.run(); + fail("expected IllegalArgumentException"); + } catch (IllegalArgumentException e) { + // Expected. + } + } + + private static void assertInvalidSubsection(String expectedMessage, + String escapedSubsection) { + try { + parseEscapedSubsection(escapedSubsection); + fail("expected ConfigInvalidException"); + } catch (ConfigInvalidException e) { + assertEquals(expectedMessage, e.getMessage()); + } } } diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties index 85d87349f..dacdd0542 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -130,6 +130,9 @@ commitMessageNotSpecified=commit message not specified commitOnRepoWithoutHEADCurrentlyNotSupported=Commit on repo without HEAD currently not supported commitAmendOnInitialNotPossible=Amending is not possible on initial commit. compressingObjects=Compressing objects +configSubsectionContainsNewline=config subsection name contains newline +configSubsectionContainsNullByte=config subsection name contains byte 0x00 +configValueContainsNullByte=config value contains byte 0x00 connectionFailed=connection failed connectionTimeOut=Connection time out: {0} contextMustBeNonNegative=context must be >= 0 diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java index 35c8076b3..836312276 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -191,6 +191,9 @@ public class JGitText extends TranslationBundle { /***/ public String commitOnRepoWithoutHEADCurrentlyNotSupported; /***/ public String commitAmendOnInitialNotPossible; /***/ public String compressingObjects; + /***/ public String configSubsectionContainsNewline; + /***/ public String configSubsectionContainsNullByte; + /***/ public String configValueContainsNullByte; /***/ public String connectionFailed; /***/ public String connectionTimeOut; /***/ public String contextMustBeNonNegative; 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 3e28184b5..b98586cfd 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java @@ -144,81 +144,91 @@ public class Config { * the value to escape * @return the escaped value */ - static String escapeValue(final String x) { + static String escapeValue(String x) { if (x.isEmpty()) { return ""; //$NON-NLS-1$ } - boolean inquote = false; - int lineStart = 0; - final StringBuilder r = new StringBuilder(x.length()); + + boolean needQuote = x.charAt(0) == ' ' || x.charAt(x.length() - 1) == ' '; + StringBuilder r = new StringBuilder(x.length()); for (int k = 0; k < x.length(); k++) { - final char c = x.charAt(k); + char c = x.charAt(k); + boolean thisCharNeedsQuote = true; + + // git-config(1) lists the limited set of supported escape sequences. switch (c) { + case '\0': + // Unix command line calling convention cannot pass a '\0' as an + // argument, so there is no equivalent way in C git to store a null byte + // in a config value. + throw new IllegalArgumentException( + JGitText.get().configValueContainsNullByte); + case '\n': - if (inquote) { - r.append('"'); - inquote = false; - } - r.append("\\n\\\n"); //$NON-NLS-1$ - lineStart = r.length(); + r.append('\\').append('n'); break; case '\t': - r.append("\\t"); //$NON-NLS-1$ + r.append('\\').append('t'); break; case '\b': - r.append("\\b"); //$NON-NLS-1$ + r.append('\\').append('b'); break; case '\\': - r.append("\\\\"); //$NON-NLS-1$ - break; - case '"': - r.append("\\\""); //$NON-NLS-1$ + r.append('\\').append(c); break; - case ';': case '#': - if (!inquote) { - r.insert(lineStart, '"'); - inquote = true; - } + case ';': r.append(c); break; - case ' ': - if (!inquote && (r.length() == 0 || r.charAt(r.length() - 1) == ' ')) { - r.insert(lineStart, '"'); - inquote = true; - } - r.append(' '); - break; - default: + thisCharNeedsQuote = false; r.append(c); break; } + needQuote |= thisCharNeedsQuote; } - if (!inquote) { - // Ensure any trailing whitespace is quoted. - int s = x.length(); - while (s > 0 && x.charAt(s - 1) == ' ') { - s--; - } - if (s != x.length()) { - // Can't insert at lineStart since there may be intervening quotes. - r.insert(s, '"'); - inquote = true; - } + return needQuote ? '"' + r.toString() + '"' : r.toString(); + } + + static String escapeSubsection(String x) { + if (x.isEmpty()) { + return "\"\""; //$NON-NLS-1$ } - if (inquote) { - r.append('"'); + StringBuilder r = new StringBuilder(x.length() + 2).append('"'); + for (int k = 0; k < x.length(); k++) { + char c = x.charAt(k); + + // git-config(1) lists the limited set of supported escape sequences + // (which is even more limited for subsection names than for values). + switch (c) { + case '\0': + throw new IllegalArgumentException( + JGitText.get().configSubsectionContainsNullByte); + + case '\n': + throw new IllegalArgumentException( + JGitText.get().configSubsectionContainsNewline); + + case '\\': + case '"': + r.append('\\').append(c); + break; + + default: + r.append(c); + break; + } } - return r.toString(); + + return r.append('"').toString(); } /** @@ -1068,7 +1078,7 @@ public class Config { e.section = readSectionName(in); input = in.read(); if ('"' == input) { - e.subsection = readValue(in, true, '"'); + e.subsection = readSubsectionName(in); input = in.read(); } if (']' != input) @@ -1085,7 +1095,7 @@ public class Config { e.name = e.name.substring(0, e.name.length() - 1); e.value = MAGIC_EMPTY_VALUE; } else - e.value = readValue(in, false, -1); + e.value = readValue(in); if (e.section.equals("include")) { //$NON-NLS-1$ addIncludedConfig(newEntries, e, depth); @@ -1252,10 +1262,9 @@ public class Config { return name.toString(); } - private static String readValue(final StringReader in, boolean quote, - final int eol) throws ConfigInvalidException { - final StringBuilder value = new StringBuilder(); - boolean space = false; + private static String readSubsectionName(StringReader in) + throws ConfigInvalidException { + StringBuilder r = new StringBuilder(); for (;;) { int c = in.read(); if (c < 0) { @@ -1263,30 +1272,80 @@ public class Config { } if ('\n' == c) { - if (quote) - throw new ConfigInvalidException(JGitText.get().newlineInQuotesNotAllowed); + throw new ConfigInvalidException( + JGitText.get().newlineInQuotesNotAllowed); + } + if ('\\' == c) { + c = in.read(); + switch (c) { + case -1: + throw new ConfigInvalidException(JGitText.get().endOfFileInEscape); + + case '\\': + case '"': + r.append((char) c); + continue; + + default: + throw new ConfigInvalidException(MessageFormat.format( + JGitText.get().badEscape, + Character.valueOf(((char) c)))); + } + } + if ('"' == c) { + break; + } + + r.append((char) c); + } + return r.toString(); + } + + private static String readValue(final StringReader in) + throws ConfigInvalidException { + StringBuilder value = new StringBuilder(); + StringBuilder trailingSpaces = null; + boolean quote = false; + boolean inLeadingSpace = true; + + for (;;) { + int c = in.read(); + if (c < 0) { + break; + } + if ('\n' == c) { + if (quote) { + throw new ConfigInvalidException( + JGitText.get().newlineInQuotesNotAllowed); + } in.reset(); break; } - if (eol == c) + if (!quote && (';' == c || '#' == c)) { + if (trailingSpaces != null) { + trailingSpaces.setLength(0); + } + in.reset(); break; + } - if (!quote) { - if (Character.isWhitespace((char) c)) { - space = true; + char cc = (char) c; + if (Character.isWhitespace(cc)) { + if (inLeadingSpace) { continue; } - if (';' == c || '#' == c) { - in.reset(); - break; + if (trailingSpaces == null) { + trailingSpaces = new StringBuilder(); + } + trailingSpaces.append(cc); + continue; + } else { + inLeadingSpace = false; + if (trailingSpaces != null) { + value.append(trailingSpaces); + trailingSpaces.setLength(0); } - } - - if (space) { - if (value.length() > 0) - value.append(' '); - space = false; } if ('\\' == c) { @@ -1323,7 +1382,7 @@ public class Config { continue; } - value.append((char) c); + value.append(cc); } return value.length() > 0 ? value.toString() : null; }