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

5 files changed, 307 insertions, 123 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());
+		}
 	}
 }
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 85d87349f5..dacdd05428 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 35c8076b3d..8363122765 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 3e28184b5d..b98586cfd4 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;
 	}