Make patch date parsing simpler and stricter (#15)

Removed the distinction between parsed and raw dates and simply return
an error for unsupported date formats. Arbitrary date support was
probably a premature optimization and would be better supported by a
method to register custom date parsing functions.

Simple time.Time fields make the structure easier to use and mean that
a zero value always indicates the patch did not include a date.

Author: bluekeyes

gitdiff/patch_header.go

@@ -25,15 +25,15 @@ type PatchHeader struct {
 	// not included in the header.
 	SHA string
 
-	// The author details of the patch. Nil if author information is not
-	// included in the header.
+	// The author details of the patch. If these details are not included in
+	// the header, Author is nil and AuthorDate is the zero time.
 	Author     *PatchIdentity
-	AuthorDate *PatchDate
+	AuthorDate time.Time
 
-	// The committer details of the patch. Nil if committer information is not
-	// included in the header.
+	// The committer details of the patch. If these details are not included in
+	// the header, Committer is nil and CommitterDate is the zero time.
 	Committer     *PatchIdentity
-	CommitterDate *PatchDate
+	CommitterDate time.Time
 
 	// The title and body of the commit message describing the changes in the
 	// patch. Empty if no message is included in the header.
@@ -104,25 +104,11 @@ func ParsePatchIdentity(s string) (PatchIdentity, error) {
 	return PatchIdentity{Name: name, Email: email}, nil
 }
 
-// PatchDate is the timestamp when a patch was authored or committed. It
-// contains a raw string version of the date and a parsed version if the date
-// is in a known format.
-type PatchDate struct {
-	Parsed time.Time
-	Raw    string
-}
-
-// IsParsed returns true if the PatchDate has a parsed time.
-func (d PatchDate) IsParsed() bool {
-	return !d.Parsed.IsZero()
-}
-
-// ParsePatchDate parses a patch date string. If s is in a supported format,
-// the PatchDate has both the Raw and Parsed initialized.
-//
-// ParsePatchDate supports the iso, rfc, short, raw, unix, and default formats
-// (with local variants) used by the --date flag in Git.
-func ParsePatchDate(s string) PatchDate {
+// ParsePatchDate parses a patch date string. It returns the parsed time or an
+// error if s has an unknown format. ParsePatchDate supports the iso, rfc,
+// short, raw, unix, and default formats (with local variants) used by the
+// --date flag in Git.
+func ParsePatchDate(s string) (time.Time, error) {
 	const (
 		isoFormat          = "2006-01-02 15:04:05 -0700"
 		isoStrictFormat    = "2006-01-02T15:04:05-07:00"
@@ -132,7 +118,9 @@ func ParsePatchDate(s string) PatchDate {
 		defaultLocalFormat = "Mon Jan 2 15:04:05 2006"
 	)
 
-	d := PatchDate{Raw: s}
+	if s == "" {
+		return time.Time{}, nil
+	}
 
 	for _, fmt := range []string{
 		isoFormat,
@@ -143,28 +131,25 @@ func ParsePatchDate(s string) PatchDate {
 		defaultLocalFormat,
 	} {
 		if t, err := time.ParseInLocation(fmt, s, time.Local); err == nil {
-			d.Parsed = t
-			return d
+			return t, nil
 		}
 	}
 
 	// unix format
 	if unix, err := strconv.ParseInt(s, 10, 64); err == nil {
-		d.Parsed = time.Unix(unix, 0)
-		return d
+		return time.Unix(unix, 0), nil
 	}
 
 	// raw format
 	if space := strings.IndexByte(s, ' '); space > 0 {
 		unix, uerr := strconv.ParseInt(s[:space], 10, 64)
 		zone, zerr := time.Parse("-0700", s[space+1:])
 		if uerr == nil && zerr == nil {
-			d.Parsed = time.Unix(unix, 0).In(zone.Location())
-			return d
+			return time.Unix(unix, 0).In(zone.Location()), nil
 		}
 	}
 
-	return d
+	return time.Time{}, fmt.Errorf("unknown date format: %s", s)
 }
 
 // ParsePatchHeader parses a preamble string as returned by Parse into a
@@ -251,16 +236,25 @@ func parseHeaderPretty(prettyLine string, r io.Reader) (*PatchHeader, error) {
 			h.Committer = &u
 
 		case strings.HasPrefix(line, datePrefix):
-			d := ParsePatchDate(strings.TrimSpace(line[len(datePrefix):]))
-			h.AuthorDate = &d
+			d, err := ParsePatchDate(strings.TrimSpace(line[len(datePrefix):]))
+			if err != nil {
+				return nil, err
+			}
+			h.AuthorDate = d
 
 		case strings.HasPrefix(line, authorDatePrefix):
-			d := ParsePatchDate(strings.TrimSpace(line[len(authorDatePrefix):]))
-			h.AuthorDate = &d
+			d, err := ParsePatchDate(strings.TrimSpace(line[len(authorDatePrefix):]))
+			if err != nil {
+				return nil, err
+			}
+			h.AuthorDate = d
 
 		case strings.HasPrefix(line, commitDatePrefix):
-			d := ParsePatchDate(strings.TrimSpace(line[len(commitDatePrefix):]))
-			h.CommitterDate = &d
+			d, err := ParsePatchDate(strings.TrimSpace(line[len(commitDatePrefix):]))
+			if err != nil {
+				return nil, err
+			}
+			h.CommitterDate = d
 		}
 	}
 	if s.Err() != nil {
@@ -358,8 +352,11 @@ func parseHeaderMail(mailLine string, r io.Reader) (*PatchHeader, error) {
 
 	date := msg.Header.Get("Date")
 	if date != "" {
-		d := ParsePatchDate(date)
-		h.AuthorDate = &d
+		d, err := ParsePatchDate(date)
+		if err != nil {
+			return nil, err
+		}
+		h.AuthorDate = d
 	}
 
 	h.Title = msg.Header.Get("Subject")

gitdiff/patch_header_test.go

@@ -69,84 +69,62 @@ func TestParsePatchDate(t *testing.T) {
 
 	tests := map[string]struct {
 		Input  string
-		Output PatchDate
+		Output time.Time
+		Err    interface{}
 	}{
 		"default": {
-			Input: "Thu Apr 9 01:07:06 2020 -0700",
-			Output: PatchDate{
-				Parsed: expected,
-				Raw:    "Thu Apr 9 01:07:06 2020 -0700",
-			},
+			Input:  "Thu Apr 9 01:07:06 2020 -0700",
+			Output: expected,
 		},
 		"defaultLocal": {
-			Input: "Thu Apr 9 01:07:06 2020",
-			Output: PatchDate{
-				Parsed: time.Date(2020, 4, 9, 1, 7, 6, 0, time.Local),
-				Raw:    "Thu Apr 9 01:07:06 2020",
-			},
+			Input:  "Thu Apr 9 01:07:06 2020",
+			Output: time.Date(2020, 4, 9, 1, 7, 6, 0, time.Local),
 		},
 		"iso": {
-			Input: "2020-04-09 01:07:06 -0700",
-			Output: PatchDate{
-				Parsed: expected,
-				Raw:    "2020-04-09 01:07:06 -0700",
-			},
+			Input:  "2020-04-09 01:07:06 -0700",
+			Output: expected,
 		},
 		"isoStrict": {
-			Input: "2020-04-09T01:07:06-07:00",
-			Output: PatchDate{
-				Parsed: expected,
-				Raw:    "2020-04-09T01:07:06-07:00",
-			},
+			Input:  "2020-04-09T01:07:06-07:00",
+			Output: expected,
 		},
 		"rfc": {
-			Input: "Thu, 9 Apr 2020 01:07:06 -0700",
-			Output: PatchDate{
-				Parsed: expected,
-				Raw:    "Thu, 9 Apr 2020 01:07:06 -0700",
-			},
+			Input:  "Thu, 9 Apr 2020 01:07:06 -0700",
+			Output: expected,
 		},
 		"short": {
-			Input: "2020-04-09",
-			Output: PatchDate{
-				Parsed: time.Date(2020, 4, 9, 0, 0, 0, 0, time.Local),
-				Raw:    "2020-04-09",
-			},
+			Input:  "2020-04-09",
+			Output: time.Date(2020, 4, 9, 0, 0, 0, 0, time.Local),
 		},
 		"raw": {
-			Input: "1586419626 -0700",
-			Output: PatchDate{
-				Parsed: expected,
-				Raw:    "1586419626 -0700",
-			},
+			Input:  "1586419626 -0700",
+			Output: expected,
 		},
 		"unix": {
-			Input: "1586419626",
-			Output: PatchDate{
-				Parsed: expected,
-				Raw:    "1586419626",
-			},
+			Input:  "1586419626",
+			Output: expected,
 		},
 		"unknownFormat": {
 			Input: "4/9/2020 01:07:06 PDT",
-			Output: PatchDate{
-				Raw: "4/9/2020 01:07:06 PDT",
-			},
+			Err:   "unknown date format",
 		},
 		"empty": {
-			Input:  "",
-			Output: PatchDate{},
+			Input: "",
 		},
 	}
 
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {
-			d := ParsePatchDate(test.Input)
-			if test.Output.Raw != d.Raw {
-				t.Errorf("incorrect raw date: expected %q, actual %q", test.Output.Raw, d.Raw)
+			d, err := ParsePatchDate(test.Input)
+			if test.Err != nil {
+				assertError(t, test.Err, err, "parsing date")
+				return
 			}
-			if !test.Output.Parsed.Equal(d.Parsed) {
-				t.Errorf("incorrect parsed date: expected %v, actual %v", test.Output.Parsed, d.Parsed)
+			if err != nil {
+				t.Fatalf("unexpected error parsing date: %v", err)
+			}
+			if !test.Output.Equal(d) {
+				t.Errorf("incorrect parsed date: expected %v, actual %v", test.Output, d)
 			}
 		})
 	}
@@ -158,10 +136,7 @@ func TestParsePatchHeader(t *testing.T) {
 		Name:  "Morton Haypenny",
 		Email: "[email protected]",
 	}
-	expectedDate := &PatchDate{
-		Parsed: time.Date(2020, 04, 11, 15, 21, 23, 0, time.FixedZone("PDT", -7*60*60)),
-		Raw:    "Sat Apr 11 15:21:23 2020 -0700",
-	}
+	expectedDate := time.Date(2020, 04, 11, 15, 21, 23, 0, time.FixedZone("PDT", -7*60*60))
 	expectedTitle := "A sample commit to test header parsing"
 	expectedBody := "The medium format shows the body, which\nmay wrap on to multiple lines.\n\nAnother body line."
 
@@ -258,14 +233,11 @@ may wrap on to multiple lines.
 Another body line.
 `,
 			Header: PatchHeader{
-				SHA:    expectedSHA,
-				Author: expectedIdentity,
-				AuthorDate: &PatchDate{
-					Parsed: expectedDate.Parsed,
-					Raw:    "Sat, 11 Apr 2020 15:21:23 -0700",
-				},
-				Title: "[PATCH] " + expectedTitle,
-				Body:  expectedBody,
+				SHA:        expectedSHA,
+				Author:     expectedIdentity,
+				AuthorDate: expectedDate,
+				Title:      "[PATCH] " + expectedTitle,
+				Body:       expectedBody,
 			},
 		},
 		"unwrapTitle": {
@@ -346,10 +318,14 @@ Author: Morton Haypenny <[email protected]>
 			}
 
 			assertPatchIdentity(t, "author", exp.Author, act.Author)
-			assertPatchDate(t, "author", exp.AuthorDate, act.AuthorDate)
+			if !exp.AuthorDate.Equal(act.AuthorDate) {
+				t.Errorf("incorrect parsed author date: expected %v, but got %v", exp.AuthorDate, act.AuthorDate)
+			}
 
 			assertPatchIdentity(t, "committer", exp.Committer, act.Committer)
-			assertPatchDate(t, "committer", exp.CommitterDate, act.CommitterDate)
+			if !exp.CommitterDate.Equal(act.CommitterDate) {
+				t.Errorf("incorrect parsed committer date: expected %v, but got %v", exp.CommitterDate, act.CommitterDate)
+			}
 
 			if exp.Title != act.Title {
 				t.Errorf("incorrect parsed title:\n  expected: %q\n    actual: %q", exp.Title, act.Title)
@@ -372,15 +348,3 @@ func assertPatchIdentity(t *testing.T, kind string, exp, act *PatchIdentity) {
 		t.Errorf("incorrect parsed %s, expected %+v, bot got %+v", kind, exp, act)
 	}
 }
-
-func assertPatchDate(t *testing.T, kind string, exp, act *PatchDate) {
-	switch {
-	case exp == nil && act == nil:
-	case exp == nil && act != nil:
-		t.Errorf("incorrect parsed %s date: expected nil, but got %+v", kind, act)
-	case exp != nil && act == nil:
-		t.Errorf("incorrect parsed %s date: expected %+v, but got nil", kind, exp)
-	case exp.Raw != act.Raw || !exp.Parsed.Equal(act.Parsed):
-		t.Errorf("incorrect parsed %s date, expected %+v, bot got %+v", kind, exp, act)
-	}
-}