From d3116e7b7ed5bb989c11f5274313853f292f88d9 Mon Sep 17 00:00:00 2001 From: Billy Keyes Date: Mon, 15 Jun 2020 22:07:06 -0700 Subject: [PATCH] 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. --- gitdiff/patch_header.go | 79 +++++++++++------------ gitdiff/patch_header_test.go | 118 ++++++++++++----------------------- 2 files changed, 79 insertions(+), 118 deletions(-) diff --git a/gitdiff/patch_header.go b/gitdiff/patch_header.go index 2dc05a0..6d3ef05 100644 --- a/gitdiff/patch_header.go +++ b/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,15 +131,13 @@ 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 @@ -159,12 +145,11 @@ func ParsePatchDate(s string) PatchDate { 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") diff --git a/gitdiff/patch_header_test.go b/gitdiff/patch_header_test.go index 8eeabdf..77c541d 100644 --- a/gitdiff/patch_header_test.go +++ b/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: "mhaypenny@example.com", } - 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 } 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) - } -}