Fix parsing of multiple authors in PersonIdent.

PersonIdent should be parsable for an invalid commit which
contains multiple authors, like "A <a@a.org>, B <b@b.org>".
PersonIdent(String) constructor now delegates to
RawParseUtils.parsePersonIdent().

Change-Id: Ie9798d36d9ecfcc0094ca795f5a44b003136eaf7

org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Commit.java

@@ -47,6 +47,7 @@
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.util.RawParseUtils;
 import org.kohsuke.args4j.Option;
 
 @Command(common = true, usage = "usage_recordChangesToRepository")
@@ -65,7 +66,7 @@ protected void run() throws NoHeadException, NoMessageException,
 			ConcurrentRefUpdateException, JGitInternalException, Exception {
 		CommitCommand commitCmd = new Git(db).commit();
 		if (author != null)
-			commitCmd.setAuthor(new PersonIdent(author));
+			commitCmd.setAuthor(RawParseUtils.parsePersonIdent(author));
 		if (message != null)
 			commitCmd.setMessage(message);
 		Ref head = db.getRef(Constants.HEAD);

org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/T0001_PersonIdent.java

@@ -55,57 +55,17 @@ public void test001_NewIdent() {
 		assertEquals("A U Thor", p.getName());
 		assertEquals("author@example.com", p.getEmailAddress());
 		assertEquals(1142878501000L, p.getWhen().getTime());
-		assertEquals("A U Thor <author@example.com> 1142878501 -0500", p
+		assertEquals("A U Thor <author@example.com> 1142878501 -0500",
-				.toExternalString());
+				p.toExternalString());
 	}
 
-	public void test002_ParseIdent() {
+	public void test002_NewIdent() {
-		final String i = "A U Thor <author@example.com> 1142878501 -0500";
+		final PersonIdent p = new PersonIdent("A U Thor", "author@example.com",
-		final PersonIdent p = new PersonIdent(i);
+				new Date(1142878501000L), TimeZone.getTimeZone("GMT+0230"));
-		assertEquals(i, p.toExternalString());
-		assertEquals("A U Thor", p.getName());
-		assertEquals("author@example.com", p.getEmailAddress());
-		assertEquals(1142878501000L, p.getWhen().getTime());
-	}
-
-	public void test003_ParseIdent() {
-		final String i = "A U Thor <author@example.com> 1142878501 +0230";
-		final PersonIdent p = new PersonIdent(i);
-		assertEquals(i, p.toExternalString());
-		assertEquals("A U Thor", p.getName());
-		assertEquals("author@example.com", p.getEmailAddress());
-		assertEquals(1142878501000L, p.getWhen().getTime());
-	}
-
-	public void test004_ParseIdent() {
-		final String i = "A U Thor<author@example.com> 1142878501 +0230";
-		final PersonIdent p = new PersonIdent(i);
-		assertEquals("A U Thor", p.getName());
-		assertEquals("author@example.com", p.getEmailAddress());
-		assertEquals(1142878501000L, p.getWhen().getTime());
-	}
-
-	public void test005_ParseIdent() {
-		final String i = "A U Thor<author@example.com>1142878501 +0230";
-		final PersonIdent p = new PersonIdent(i);
-		assertEquals("A U Thor", p.getName());
-		assertEquals("author@example.com", p.getEmailAddress());
-		assertEquals(1142878501000L, p.getWhen().getTime());
-	}
-
-	public void test006_ParseIdent() {
-		final String i = "A U Thor   <author@example.com>1142878501 +0230";
-		final PersonIdent p = new PersonIdent(i);
-		assertEquals("A U Thor", p.getName());
-		assertEquals("author@example.com", p.getEmailAddress());
-		assertEquals(1142878501000L, p.getWhen().getTime());
-	}
-
-	public void test007_ParseIdent() {
-		final String i = "A U Thor<author@example.com>1142878501 +0230 ";
-		final PersonIdent p = new PersonIdent(i);
 		assertEquals("A U Thor", p.getName());
 		assertEquals("author@example.com", p.getEmailAddress());
 		assertEquals(1142878501000L, p.getWhen().getTime());
+		assertEquals("A U Thor <author@example.com> 1142878501 +0230",
+				p.toExternalString());
 	}
 }

org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevCommitParseTest.java

@@ -45,6 +45,7 @@
 
 import java.io.ByteArrayOutputStream;
 import java.io.UnsupportedEncodingException;
+import java.util.TimeZone;
 
 import org.eclipse.jgit.lib.CommitBuilder;
 import org.eclipse.jgit.lib.Constants;
@@ -59,10 +60,12 @@ public void testParse_NoParents() throws Exception {
 		final String authorName = "A U. Thor";
 		final String authorEmail = "a_u_thor@example.com";
 		final int authorTime = 1218123387;
+		final String authorTimeZone = "+0700";
 
 		final String committerName = "C O. Miter";
 		final String committerEmail = "comiter@example.com";
 		final int committerTime = 1218123390;
+		final String committerTimeZone = "-0500";
 		final StringBuilder body = new StringBuilder();
 
 		body.append("tree ");
@@ -75,7 +78,9 @@ public void testParse_NoParents() throws Exception {
 		body.append(authorEmail);
 		body.append("> ");
 		body.append(authorTime);
-		body.append(" +0700\n");
+		body.append(" ");
+		body.append(authorTimeZone);
+		body.append(" \n");
 
 		body.append("committer ");
 		body.append(committerName);
@@ -83,7 +88,9 @@ public void testParse_NoParents() throws Exception {
 		body.append(committerEmail);
 		body.append("> ");
 		body.append(committerTime);
-		body.append(" -0500\n");
+		body.append(" ");
+		body.append(committerTimeZone);
+		body.append("\n");
 
 		body.append("\n");
 
@@ -107,11 +114,15 @@ public void testParse_NoParents() throws Exception {
 		assertNotNull(cAuthor);
 		assertEquals(authorName, cAuthor.getName());
 		assertEquals(authorEmail, cAuthor.getEmailAddress());
+		assertEquals((long)authorTime * 1000, cAuthor.getWhen().getTime());
+		assertEquals(TimeZone.getTimeZone("GMT" + authorTimeZone), cAuthor.getTimeZone());
 
 		final PersonIdent cCommitter = c.getCommitterIdent();
 		assertNotNull(cCommitter);
 		assertEquals(committerName, cCommitter.getName());
 		assertEquals(committerEmail, cCommitter.getEmailAddress());
+		assertEquals((long)committerTime * 1000, cCommitter.getWhen().getTime());
+		assertEquals(TimeZone.getTimeZone("GMT" + committerTimeZone), cCommitter.getTimeZone());
 	}
 
 	private RevCommit create(final String msg) throws Exception {

org.eclipse.jgit.test/tst/org/eclipse/jgit/util/ChangeIdUtilTest.java

@@ -61,10 +61,10 @@ public class ChangeIdUtilTest extends TestCase {
 
 	private final String SOB2 = "Signed-off-by: J Committer <jc@example.com>\n";
 
-	final PersonIdent p = new PersonIdent(
+	final PersonIdent p = RawParseUtils.parsePersonIdent(
 			"A U Thor <author@example.com> 1142878501 -0500");
 
-	final PersonIdent q = new PersonIdent(
+	final PersonIdent q = RawParseUtils.parsePersonIdent(
 			"W Riter <writer@example.com> 1142878502 -0500");
 
 	ObjectId treeId = ObjectId

org.eclipse.jgit.test/tst/org/eclipse/jgit/util/RawParseUtils_ParsePersonIdentTest.java

@@ -43,58 +43,97 @@
 
 package org.eclipse.jgit.util;
 
-import java.io.UnsupportedEncodingException;
 import java.util.Date;
 import java.util.TimeZone;
 
-import org.eclipse.jgit.lib.PersonIdent;
-
 import junit.framework.TestCase;
 
+import org.eclipse.jgit.lib.PersonIdent;
+
 public class RawParseUtils_ParsePersonIdentTest extends TestCase {
 
-	public void testParsePersonIdent_legalCases()
+	public void testParsePersonIdent_legalCases() {
-			throws UnsupportedEncodingException {
 		final Date when = new Date(1234567890000l);
 		final TimeZone tz = TimeZone.getTimeZone("GMT-7");
 
-		assertPersonIdent("Me <me@example.com> 1234567890 -0700", 0,
+		assertPersonIdent("Me <me@example.com> 1234567890 -0700",
 				new PersonIdent("Me", "me@example.com", when, tz));
 
-		assertPersonIdent(" Me <me@example.com> 1234567890 -0700", 1,
+		assertPersonIdent(" Me <me@example.com> 1234567890 -0700",
-				new PersonIdent("Me", "me@example.com", when, tz));
+				new PersonIdent(" Me", "me@example.com", when, tz));
 
-		assertPersonIdent("Me <> 1234567890 -0700", 0, new PersonIdent("Me",
+		assertPersonIdent("A U Thor <author@example.com> 1234567890 -0700",
-				"", when, tz));
+				new PersonIdent("A U Thor", "author@example.com", when, tz));
 
-		assertPersonIdent(" <me@example.com> 1234567890 -0700", 0,
+		assertPersonIdent("A U Thor<author@example.com> 1234567890 -0700",
+				new PersonIdent("A U Thor", "author@example.com", when, tz));
+
+		assertPersonIdent("A U Thor<author@example.com>1234567890 -0700",
+				new PersonIdent("A U Thor", "author@example.com", when, tz));
+
+		assertPersonIdent(
+				" A U Thor   < author@example.com > 1234567890 -0700",
+				new PersonIdent(" A U Thor  ", " author@example.com ", when, tz));
+
+		assertPersonIdent("A U Thor<author@example.com>1234567890 -0700",
+				new PersonIdent("A U Thor", "author@example.com", when, tz));
+	}
+
+	public void testParsePersonIdent_fuzzyCases() {
+		final Date when = new Date(1234567890000l);
+		final TimeZone tz = TimeZone.getTimeZone("GMT-7");
+
+		assertPersonIdent(
+				"A U Thor <author@example.com>,  C O. Miter <comiter@example.com> 1234567890 -0700",
+				new PersonIdent("A U Thor", "author@example.com", when, tz));
+
+		assertPersonIdent(
+				"A U Thor <author@example.com> and others 1234567890 -0700",
+				new PersonIdent("A U Thor", "author@example.com", when, tz));
+	}
+
+	public void testParsePersonIdent_incompleteCases() {
+		final Date when = new Date(1234567890000l);
+		final TimeZone tz = TimeZone.getTimeZone("GMT-7");
+
+		assertPersonIdent("Me <> 1234567890 -0700", new PersonIdent("Me", "",
+				when, tz));
+
+		assertPersonIdent(" <me@example.com> 1234567890 -0700",
 				new PersonIdent("", "me@example.com", when, tz));
 
-		assertPersonIdent(" <> 1234567890 -0700", 0, new PersonIdent("", "",
+		assertPersonIdent(" <> 1234567890 -0700", new PersonIdent("", "", when,
-				when, tz));
+				tz));
+
+		assertPersonIdent("<>", new PersonIdent("", "", 0, 0));
+
+		assertPersonIdent(" <>", new PersonIdent("", "", 0, 0));
+
+		assertPersonIdent("<me@example.com>", new PersonIdent("",
+				"me@example.com", 0, 0));
+
+		assertPersonIdent(" <me@example.com>", new PersonIdent("",
+				"me@example.com", 0, 0));
+
+		assertPersonIdent("Me <>", new PersonIdent("Me", "", 0, 0));
+
+		assertPersonIdent("Me <me@example.com>", new PersonIdent("Me",
+				"me@example.com", 0, 0));
+
+		assertPersonIdent("Me <me@example.com> 1234567890", new PersonIdent(
+				"Me", "me@example.com", 0, 0));
+
+		assertPersonIdent("Me <me@example.com> 1234567890 ", new PersonIdent(
+				"Me", "me@example.com", 0, 0));
 	}
 
-	public void testParsePersonIdent_malformedCases()
+	public void testParsePersonIdent_malformedCases() {
-			throws UnsupportedEncodingException {
+		assertPersonIdent("Me me@example.com> 1234567890 -0700", null);
-		assertPersonIdent("Me me@example.com> 1234567890 -0700", 0, null);
+		assertPersonIdent("Me <me@example.com 1234567890 -0700", null);
-		assertPersonIdent("Me <me@example.com 1234567890 -0700", 0, null);
-
-		assertPersonIdent("<>", 0, null);
-		assertPersonIdent("<me@example.com>", 0, null);
-		assertPersonIdent(" <>", 0, null);
-		assertPersonIdent(" <me@example.com>", 0, null);
-		assertPersonIdent("Me <>", 0, null);
-		assertPersonIdent("Me <me@example.com>", 0, null);
-
-		assertPersonIdent("Me <me@example.com> 1234567890", 0, null);
-		assertPersonIdent("<me@example.com> 1234567890 -0700", 0, null);
-		assertPersonIdent("<> 1234567890 -0700", 0, null);
 	}
 
-	private void assertPersonIdent(String line, int nameB, PersonIdent expected)
+	private void assertPersonIdent(String line, PersonIdent expected) {
-			throws UnsupportedEncodingException {
+		PersonIdent actual = RawParseUtils.parsePersonIdent(line);
-		PersonIdent actual = RawParseUtils.parsePersonIdent(line
-				.getBytes("UTF-8"), nameB);
 		assertEquals(expected, actual);
 	}
 }

org.eclipse.jgit/src/org/eclipse/jgit/lib/PersonIdent.java

@@ -52,6 +52,7 @@
 import java.util.TimeZone;
 
 import org.eclipse.jgit.JGitText;
+import org.eclipse.jgit.util.RawParseUtils;
 import org.eclipse.jgit.util.SystemReader;
 
 /**
@@ -195,37 +196,19 @@ public PersonIdent(final PersonIdent pi, final long aWhen, final int aTZ) {
 	 *
 	 * @param in
 	 *            a Git internal format author/committer string.
+	 *
+	 * @deprecated Use {@link RawParseUtils#parsePersonIdent(String)} instead.
 	 */
 	public PersonIdent(final String in) {
-		final int lt = in.indexOf('<');
+		final PersonIdent self = RawParseUtils.parsePersonIdent(in);
-		if (lt == -1) {
+		if (self == null)
 			throw new IllegalArgumentException(MessageFormat.format(
 					JGitText.get().malformedpersonIdentString, in));
-		}
-		final int gt = in.indexOf('>', lt);
-		if (gt == -1) {
-			throw new IllegalArgumentException(MessageFormat.format(
-					JGitText.get().malformedpersonIdentString, in));
-		}
-		final int sp = in.indexOf(' ', gt + 2);
-		if (sp == -1) {
-			when = 0;
-			tzOffset = -1;
-		} else {
-			final String tzHoursStr = in.substring(sp + 1, sp + 4).trim();
-			final int tzHours;
-			if (tzHoursStr.charAt(0) == '+') {
-				tzHours = Integer.parseInt(tzHoursStr.substring(1));
-			} else {
-				tzHours = Integer.parseInt(tzHoursStr);
-			}
-			final int tzMins = Integer.parseInt(in.substring(sp + 4).trim());
-			when = Long.parseLong(in.substring(gt + 1, sp).trim()) * 1000;
-			tzOffset = tzHours * 60 + tzMins;
-		}
 
-		name = in.substring(0, lt).trim();
+		this.name = self.name;
-		emailAddress = in.substring(lt + 1, gt).trim();
+		this.emailAddress = self.emailAddress;
+		this.when = self.when;
+		this.tzOffset = self.tzOffset;
 	}
 
 	/**

org.eclipse.jgit/src/org/eclipse/jgit/util/RawParseUtils.java

@@ -652,6 +652,21 @@ public static Charset parseEncoding(final byte[] b) {
 		return Charset.forName(decode(Constants.CHARSET, b, enc, lf - 1));
 	}
 
+	/**
+	 * Parse a name string (e.g. author, committer, tagger) into a PersonIdent.
+	 * <p>
+	 * Leading spaces won't be trimmed from the string, i.e. will show up in the
+	 * parsed name afterwards.
+	 *
+	 * @param in
+	 *            the string to parse a name from.
+	 * @return the parsed identity or null in case the identity could not be
+	 *         parsed.
+	 */
+	public static PersonIdent parsePersonIdent(final String in) {
+		return parsePersonIdent(Constants.encode(in), 0);
+	}
+
 	/**
 	 * Parse a name line (e.g. author, committer, tagger) into a PersonIdent.
 	 * <p>
@@ -667,32 +682,42 @@ public static Charset parseEncoding(final byte[] b) {
 	 *            first position after the space which delimits the header field
 	 *            name (e.g. "author" or "committer") from the rest of the
 	 *            identity line.
-	 * @return the parsed identity. Never null.
+	 * @return the parsed identity or null in case the identity could not be
+	 *         parsed.
 	 */
 	public static PersonIdent parsePersonIdent(final byte[] raw, final int nameB) {
 		final Charset cs = parseEncoding(raw);
 		final int emailB = nextLF(raw, nameB, '<');
 		final int emailE = nextLF(raw, emailB, '>');
-		if (emailB <= nameB + 1 || // No name
+		if (emailB >= raw.length || raw[emailB] == '\n' ||
-			emailB >= raw.length || // No email start
+				(emailE >= raw.length - 1 && raw[emailE - 1] != '>'))
-			raw[emailB] == '\n' ||
-			emailE >= raw.length - 1 || // No email end at all or no trailing date
-			raw[emailE] == '\n') {
 			return null;
-		}
 
-		final String name = decode(cs, raw, nameB, emailB - 2);
+		final int nameEnd = emailB - 2 >= 0 && raw[emailB - 2] == ' ' ? emailB - 2
+				: emailB - 1;
+		final String name = decode(cs, raw, nameB, nameEnd);
 		final String email = decode(cs, raw, emailB, emailE - 1);
 
-		final MutableInteger ptrout = new MutableInteger();
+		// Start searching from end of line, as after first name-email pair,
-		final long when = parseLongBase10(raw, emailE + 1, ptrout);
+		// another name-email pair may occur. We will ignore all kinds of
-		final int whenE = ptrout.value;
+		// "junk" following the first email.
-		if (whenE >= raw.length || // No trailing timezone
+		//
-			raw[whenE] == '\n') {
+		// We've to use (emailE - 1) for the case that raw[email] is LF,
-			return null;
+		// otherwise we would run too far. "-2" is necessary to position
-		}
+		// before the LF in case of LF termination resp. the penultimate
+		// character if there is no trailing LF.
+		final int tzBegin = lastIndexOfTrim(raw, ' ',
+				nextLF(raw, emailE - 1) - 2) + 1;
+		if (tzBegin <= emailE) // No time/zone, still valid
+			return new PersonIdent(name, email, 0, 0);
 
-		final int tz = parseTimeZoneOffset(raw, whenE);
+		final int whenBegin = Math.max(emailE,
+				lastIndexOfTrim(raw, ' ', tzBegin - 1) + 1);
+		if (whenBegin >= tzBegin - 1) // No time/zone, still valid
+			return new PersonIdent(name, email, 0, 0);
+
+		final long when = parseLongBase10(raw, whenBegin, null);
+		final int tz = parseTimeZoneOffset(raw, tzBegin);
 		return new PersonIdent(name, email, when * 1000L, tz);
 	}
 
@@ -713,7 +738,8 @@ public static PersonIdent parsePersonIdent(final byte[] raw, final int nameB) {
 	 *            identity line.
 	 * @return the parsed identity. Never null.
 	 */
-	public static PersonIdent parsePersonIdentOnly(final byte[] raw, final int nameB) {
+	public static PersonIdent parsePersonIdentOnly(final byte[] raw,
+			final int nameB) {
 		int stop = nextLF(raw, nameB);
 		int emailB = nextLF(raw, nameB, '<');
 		int emailE = nextLF(raw, emailB, '>');
@@ -1022,6 +1048,16 @@ public static final int endOfParagraph(final byte[] b, final int start) {
 		return ptr;
 	}
 
+	private static int lastIndexOfTrim(byte[] raw, char ch, int pos) {
+		while (pos >= 0 && raw[pos] == ' ')
+			pos--;
+
+		while (pos >= 0 && raw[pos] != ch)
+			pos--;
+
+		return pos;
+	}
+
 	private RawParseUtils() {
 		// Don't create instances of a static only utility.
 	}