Merge changes I0f1df93b,Ifd40129b,I1b059e1a

* changes:
  ConfigTest: Add some additional comment parsing tests
  Config: Drop backslash in invalid escape sequences in subsections
  Config: Match C git behavior more closely in escaping values

3 files changed, 56 insertions, 24 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 ab86bc2e27..aa50697172 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
@@ -368,7 +368,7 @@ public class T0003_BasicTest extends SampleDataRepositoryTestCase {
 				+ "[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";
+				+ "    defaultCheckInComment = a many line\\ncomment\\n to test\n";
 		assertEquals(expectedStr, new String(IO.readFully(cfg), Constants.CHARSET));
 	}
 
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 44714faa70..fb1ee8cadb 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
@@ -977,14 +977,25 @@ public class ConfigTest {
 	}
 
 	@Test
-	public void testEscapeSpecialCharacters() throws ConfigInvalidException {
+	public void testNoEscapeSpecialCharacters() throws ConfigInvalidException {
+		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 testParseLiteralBackspace() throws ConfigInvalidException {
+		// This is round-tripped with an escape sequence by JGit, but C git writes
+		// it out as a literal backslash.
+		assertEquals("x\by", parseEscapedValue("x\by"));
+	}
+
+	@Test
+	public void testEscapeCommentCharacters() 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
@@ -1014,6 +1025,11 @@ public class ConfigTest {
 		assertEquals("baz", parseEscapedValue("baz # comment"));
 		assertEquals("baz", parseEscapedValue("baz " + WS + " ; comment"));
 		assertEquals("baz", parseEscapedValue("baz " + WS + " # comment"));
+
+		assertEquals("baz ", parseEscapedValue("\"baz \"; comment"));
+		assertEquals("baz ", parseEscapedValue("\"baz \"# comment"));
+		assertEquals("baz ", parseEscapedValue("\"baz \" ; comment"));
+		assertEquals("baz ", parseEscapedValue("\"baz \" # comment"));
 	}
 
 	@Test
@@ -1045,16 +1061,17 @@ public class ConfigTest {
 	public void testParseInvalidSubsections() {
 		assertInvalidSubsection(
 				JGitText.get().newlineInQuotesNotAllowed, "\"x\ny\"");
-		assertInvalidSubsection(
-				MessageFormat.format(JGitText.get().badEscape, 'q'), "\"x\\q\"");
+	}
 
+	@Test
+	public void testDropBackslashFromInvalidEscapeSequenceInSubsectionName()
+			throws ConfigInvalidException {
+		assertEquals("x0", parseEscapedSubsection("\"x\\0\""));
+		assertEquals("xq", parseEscapedSubsection("\"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\"");
+		assertEquals("xb", parseEscapedSubsection("\"x\\b\""));
+		assertEquals("xn", parseEscapedSubsection("\"x\\n\""));
+		assertEquals("xt", parseEscapedSubsection("\"x\\t\""));
 	}
 
 	private static void assertValueRoundTrip(String value)
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 39161214de..a6313f0cc5 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Config.java
@@ -155,9 +155,18 @@ public class Config {
 		StringBuilder r = new StringBuilder(x.length());
 		for (int k = 0; k < x.length(); k++) {
 			char c = x.charAt(k);
-			boolean thisCharNeedsQuote = true;
-
-			// git-config(1) lists the limited set of supported escape sequences.
+			// git-config(1) lists the limited set of supported escape sequences, but
+			// the documentation is otherwise not especially normative. In particular,
+			// which ones of these produce and/or require escaping and/or quoting
+			// around them is not documented and was discovered by trial and error.
+			// In summary:
+			//
+			// * Quotes are only required if there is leading/trailing whitespace or a
+			//   comment character.
+			// * Bytes that have a supported escape sequence are escaped, except for
+			//   \b for some reason which isn't.
+			// * Needing an escape sequence is not sufficient reason to quote the
+			//   value.
 			switch (c) {
 			case '\0':
 				// Unix command line calling convention cannot pass a '\0' as an
@@ -175,25 +184,30 @@ public class Config {
 				break;
 
 			case '\b':
+				// Doesn't match `git config foo.bar $'x\by'`, which doesn't escape the
+				// \x08, but since both escaped and unescaped forms are readable, we'll
+				// prefer internal consistency here.
 				r.append('\\').append('b');
 				break;
 
 			case '\\':
+				r.append('\\').append('\\');
+				break;
+
 			case '"':
-				r.append('\\').append(c);
+				r.append('\\').append('"');
 				break;
 
 			case '#':
 			case ';':
+				needQuote = true;
 				r.append(c);
 				break;
 
 			default:
-				thisCharNeedsQuote = false;
 				r.append(c);
 				break;
 			}
-			needQuote |= thisCharNeedsQuote;
 		}
 
 		return needQuote ? '"' + r.toString() + '"' : r.toString();
@@ -1301,9 +1315,10 @@ public class Config {
 					continue;
 
 				default:
-					throw new ConfigInvalidException(MessageFormat.format(
-							JGitText.get().badEscape,
-							Character.valueOf(((char) c))));
+					// C git simply drops backslashes if the escape sequence is not
+					// recognized.
+					r.append((char) c);
+					continue;
 				}
 			}
 			if ('"' == c) {