diff: fix time format

[rogpeppe]

It seems that the fractional seconds part is optional, so change the format to accept whole seconds too.

Here's a sample diff output by the bzr command which fails to parse currently.

I started adding a test, but it involves more time than I have to spend right now, as the tests rely on reflect.DeepEqual working on time.Time instances, which won't work in general due to monotonic time. I'd suggest using https://godoc.org/github.com/google/go-cmp/cmp#Diff instead.

=== modified file 'encode.go'
--- encode.go	2011-11-24 19:47:20 +0000
+++ encode.go	2011-11-28 13:24:31 +0000
@@ -250,8 +250,7 @@
 	e.emitScalar("null", "", "", C.YAML_PLAIN_SCALAR_STYLE)
 }

-func (e *encoder) emitScalar(value, anchor, tag string,
-	style C.yaml_scalar_style_t) {
+func (e *encoder) emitScalar(value, anchor, tag string, style C.yaml_scalar_style_t) {
 	var canchor, ctag, cvalue *C.yaml_char_t
 	var cimplicit C.int
 	var free func()

=== modified file 'encode_test.go'
--- encode_test.go	2011-11-24 19:47:20 +0000
+++ encode_test.go	2011-11-28 13:24:39 +0000
@@ -76,6 +76,10 @@
 		A int "a,omitempty"
 		B int "b,omitempty"
 	}{0, 0}},
+	{"{}\n", &struct {
+		A *struct{ X int } "a,omitempty"
+		B int              "b,omitempty"
+	}{nil, 0}},

 	// Flow flag
 	{"a: [1, 2]\n", &struct {

=== modified file 'goyaml.go'
--- goyaml.go	2011-11-24 19:47:20 +0000
+++ goyaml.go	2011-11-28 13:24:39 +0000
@@ -256,7 +256,7 @@
 	switch v.Kind() {
 	case reflect.String:
 		return len(v.String()) == 0
-	case reflect.Interface:
+	case reflect.Interface, reflect.Ptr:
 		return v.IsNil()
 	case reflect.Slice:
 		return v.Len() == 0

[sqs]

Thanks! I’ll look into the test probably on Saturday. If anyone else happens to see this PR and wants to get some karma, test patches are happily accepted.

[dmitshur]

Here's a sample diff output by the bzr command which fails to parse currently.

@rogpeppe Can you clarify, how does it fail? I can't reproduce, that diff seems to parse successfully for me:

https://play.golang.org/p/mHwe-KX8y-

Edit: Never mind, I accidentally had your change applied when testing. It returns an error without your change:

parsing time "2011-11-24 19:47:20 +0000" as "2006-01-02 15:04:05.000000000 -0700": cannot parse " +0000" as ".000000000"

It seems that the fractional seconds part is optional, so change the format to accept whole seconds too.

The diffTimeFormat const you changed is used both for parsing and for printing:

go-diff/diff/parse.go

Line 270 in 748a069

ts, err := time.Parse(diffTimeFormat, parts[1])

go-diff/diff/print.go

Line 74 in 748a069

if _, err := fmt.Fprint(w, "\t", timestamp.Format(diffTimeFormat)); err != nil {

Changing it affects how diffs are printed, which doesn't seem to be what you intended with this PR, and causes the tests to fail.

[dmitshur]

I think the correct fix is to have 2 constants, one as the parse layout, and the other as the print layout. They're no longer going to be the same.

Adding a basic test should be easy, just add a new file to testdata and check that parsing it doesn't return an error.

[dmitshur]

Also, according to package time documentation:

When parsing (only), the input may contain a fractional second field immediately after the seconds field, even if the layout does not signify its presence. In that case a decimal point followed by a maximal series of digits is parsed as a fractional second.

So the parse layout string 2006-01-02 15:04:05.999999999 -0700 can be simplified to just 2006-01-02 15:04:05 -0700.

I'll send a PR that resolves this soon.

[dmitshur]

Sent PR #18 that should resolve this. It resolves the failing tests, fixes the parsing behavior when header timestamps don't have fractional seconds (as allowed by https://www.gnu.org/software/diffutils/manual/html_node/Detailed-Unified.html; see "The fractional seconds are omitted on hosts that do not support fractional timestamps."), and adds a test for it.

[rogpeppe]

I think the correct fix is to have 2 constants, one as the parse layout, and the other as the print layout. They're no longer going to be the same.

I'm not entirely convinced. This will cause all output to contain the full nanosecond precision decimals, but often we don't want that. For example, this change means that reading the example diff output above and printing it will change the output - if you add testdata/sample_file_no_fractional_seconds.diff to the TestParseMultiFileDiffAndPrintMultiFileDiff tests, then that test fails.

Given that the internal representation loses information about the original precision of the time stamp, I'd suggest that the .9999 format is probably best, as it preserves all the original information while still printing sensibly formatted timestamps.

BTW I'm wondering if that test data (with the all the trailing zeros) was produced by an actual tool or was constructed specifically for the tests?

[dmitshur]

I'm not entirely convinced. This will cause all output to contain the full nanosecond precision decimals, but often we don't want that. For example, this change means that reading the example diff output above and printing it will change the output - if you add testdata/sample_file_no_fractional_seconds.diff to the TestParseMultiFileDiffAndPrintMultiFileDiff tests, then that test fails.

Given that the internal representation loses information about the original precision of the time stamp, I'd suggest that the .9999 format is probably best, as it preserves all the original information while still printing sensibly formatted timestamps.

I agree. However, I would describe that as a low impact, low priority optional enhancement. It can be done, but it'll require making changes to tests or testdata so they can continue to pass. It seems the value is low because most unified diffs I see these days have fractional seconds.

BTW I'm wondering if that test data (with the all the trailing zeros) was produced by an actual tool or was constructed specifically for the tests?

I'm not sure. The more recent testdata files that I added came from latest versions of git, but they have fractional seconds.