Config: Rewrite subsection and value escaping and parsing

Previously, Config was using the same method for both escaping and
parsing subsection names and config values. The goal was presumably code
savings, but unfortunately, these two pieces of the git config format
are simply different.

In git v2.15.1, Documentation/config.txt says the following about
subsection names:

  "Subsection names are case sensitive and can contain any characters
  except newline (doublequote `"` and backslash can be included by
  escaping them as `\"` and `\\`, respectively).  Section headers cannot
  span multiple lines.  Variables may belong directly to a section or to
  a given subsection."

And, later in the same documentation section, about values:

  "A line that defines a value can be continued to the next line by
  ending it with a `\`; the backquote and the end-of-line are stripped.
  Leading whitespaces after 'name =', the remainder of the line after
  the first comment character '#' or ';', and trailing whitespaces of
  the line are discarded unless they are enclosed in double quotes.
  Internal whitespaces within the value are retained verbatim.

  Inside double quotes, double quote `"` and backslash `\` characters
  must be escaped: use `\"` for `"` and `\\` for `\`.

  The following escape sequences (beside `\"` and `\\`) are recognized:
  `\n` for newline character (NL), `\t` for horizontal tabulation (HT,
  TAB) and `\b` for backspace (BS).  Other char escape sequences
  (including octal escape sequences) are invalid."

The main important differences are that subsection names have a limited
set of supported escape sequences, and do not support newlines at all,
either escaped or unescaped. Arguably, it would be easy to support
escaped newlines, but C git simply does not:

  $ git config -f foo.config $'foo.bar\nbaz.quux' value
  error: invalid key (newline): foo.bar
  baz.quux

I468106ac was an attempt to fix one bug in escapeValue, around leading
whitespace, without having to rewrite the whole escaping/parsing code.
Unfortunately, because escapeValue was used for escaping subsection
names as well, this made it possible to write invalid config files, any
time Config#toText is called with a subsection name with trailing
whitespace, like {foo }.

Rather than pile hacks on top of hacks, fix it for real by largely
rewriting the escaping and parsing code.

In addition to fixing escape sequences, fix (and write tests for) a few
more issues in the old implementation:

* Now that we can properly parse it, always emit newlines as "\n" from
  escapeValue, rather than the weird (but still supported) syntax with a
  non-quoted trailing literal "\n\" before the newline. In addition to
  producing more readable output and matching the behavior of C git,
  this makes the escaping code much simpler.
* Disallow '\0' entirely within both subsection names and values, since
  due to Unix command line argument conventions it is impossible to pass
  such values to "git config".
* Properly preserve intra-value whitespace when parsing, rather than
  collapsing it all to a single space.

Change-Id: I304f626b9d0ad1592c4e4e449a11b136c0f8b3e3

2 files changed, 176 insertions, 57 deletions

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 a3a435674d..ab86bc2e27 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 <thor@example.com> # 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 712e9df445..44714faa70 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<String> 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());
+		}
 	}
 }