archive/tar: ignore ChangeTime and AccessTime unless Format is specified

CL 59230 changed Writer.WriteHeader to ignore the ChangeTime and AccessTime
fields when considering using the USTAR format when the format is unspecified.
This policy is confusing and leads to unexpected behavior where some files
have ModTime only, while others have ModTime+AccessTime+ChangeTime if the
format became PAX for some unrelated reason (e.g., long pathname).

Change the policy to simply always ignore ChangeTime, AccessTime, and
sub-second time resolutions unless the user explicitly specifies a format.
This is a safe policy change since WriteHeader had no support for the
above features in any Go release.

Support for ChangeTime and AccessTime was added in CL 55570.
Support for sub-second times was added in CL 55552.
Both CLs landed after the latest Go release (i.e., Go1.9), which was
cut from the master branch around August 6th, 2017.

Change-Id: Ib82baa1bf9dd4573ed4f674b7d55d15f733a4843
Reviewed-on: https://go-review.googlesource.com/69296Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

src/archive/tar/common.go

@@ -152,10 +152,11 @@ type Header struct {
 	Uname string // User name of owner
 	Gname string // Group name of owner
 
-	// The PAX format encodes the timestamps with sub-second resolution,
-	// while the other formats (USTAR and GNU) truncate to the nearest second.
-	// If the Format is unspecified, then Writer.WriteHeader ignores
-	// AccessTime and ChangeTime when using the USTAR format.
+	// If the Format is unspecified, then Writer.WriteHeader rounds ModTime
+	// to the nearest second and ignores the AccessTime and ChangeTime fields.
+	//
+	// To use AccessTime or ChangeTime, specify the Format as PAX or GNU.
+	// To use sub-second resolution, specify the Format as PAX.
 	ModTime    time.Time // Modification time
 	AccessTime time.Time // Access time (requires either PAX or GNU support)
 	ChangeTime time.Time // Change time (requires either PAX or GNU support)
@@ -340,7 +341,8 @@ type fileState interface {
 //
 // As a by-product of checking the fields, this function returns paxHdrs, which
 // contain all fields that could not be directly encoded.
-func (h *Header) allowedFormats() (format Format, paxHdrs map[string]string, err error) {
+// A value receiver ensures that this method does not mutate the source Header.
+func (h Header) allowedFormats() (format Format, paxHdrs map[string]string, err error) {
 	format = FormatUSTAR | FormatPAX | FormatGNU
 	paxHdrs = make(map[string]string)
 
@@ -402,8 +404,7 @@ func (h *Header) allowedFormats() (format Format, paxHdrs map[string]string, err
 		}
 		isMtime := paxKey == paxMtime
 		fitsOctal := fitsInOctal(size, ts.Unix())
-		noACTime := !isMtime && h.Format != FormatUnknown
-		if (isMtime && !fitsOctal) || noACTime {
+		if (isMtime && !fitsOctal) || !isMtime {
 			whyNoUSTAR = fmt.Sprintf("USTAR cannot encode %s=%v", name, ts)
 			format.mustNotBe(FormatUSTAR)
 		}
@@ -452,7 +453,7 @@ func (h *Header) allowedFormats() (format Format, paxHdrs map[string]string, err
 	case TypeXHeader, TypeGNULongName, TypeGNULongLink:
 		return FormatUnknown, nil, headerError{"cannot manually encode TypeXHeader, TypeGNULongName, or TypeGNULongLink headers"}
 	case TypeXGlobalHeader:
-		if !reflect.DeepEqual(h, &Header{Typeflag: h.Typeflag, Xattrs: h.Xattrs, PAXRecords: h.PAXRecords, Format: h.Format}) {
+		if !reflect.DeepEqual(h, Header{Typeflag: h.Typeflag, Xattrs: h.Xattrs, PAXRecords: h.PAXRecords, Format: h.Format}) {
 			return FormatUnknown, nil, headerError{"only PAXRecords may be set for TypeXGlobalHeader"}
 		}
 		whyOnlyPAX = "only PAX supports TypeXGlobalHeader"

src/archive/tar/tar_test.go

@@ -696,7 +696,7 @@ func TestHeaderAllowedFormats(t *testing.T) {
 	}, {
 		header:  &Header{AccessTime: time.Unix(0, 0)},
 		paxHdrs: map[string]string{paxAtime: "0"},
-		formats: FormatUSTAR | FormatPAX | FormatGNU,
+		formats: FormatPAX | FormatGNU,
 	}, {
 		header:  &Header{AccessTime: time.Unix(0, 0), Format: FormatUSTAR},
 		paxHdrs: map[string]string{paxAtime: "0"},
@@ -712,7 +712,7 @@ func TestHeaderAllowedFormats(t *testing.T) {
 	}, {
 		header:  &Header{AccessTime: time.Unix(-123, 0)},
 		paxHdrs: map[string]string{paxAtime: "-123"},
-		formats: FormatUSTAR | FormatPAX | FormatGNU,
+		formats: FormatPAX | FormatGNU,
 	}, {
 		header:  &Header{AccessTime: time.Unix(-123, 0), Format: FormatPAX},
 		paxHdrs: map[string]string{paxAtime: "-123"},
@@ -720,7 +720,7 @@ func TestHeaderAllowedFormats(t *testing.T) {
 	}, {
 		header:  &Header{ChangeTime: time.Unix(123, 456)},
 		paxHdrs: map[string]string{paxCtime: "123.000000456"},
-		formats: FormatUSTAR | FormatPAX | FormatGNU,
+		formats: FormatPAX | FormatGNU,
 	}, {
 		header:  &Header{ChangeTime: time.Unix(123, 456), Format: FormatUSTAR},
 		paxHdrs: map[string]string{paxCtime: "123.000000456"},

src/archive/tar/writer.go

@@ -70,8 +70,19 @@ func (tw *Writer) WriteHeader(hdr *Header) error {
 	if err := tw.Flush(); err != nil {
 		return err
 	}
-
 	tw.hdr = *hdr // Shallow copy of Header
+
+	// Round ModTime and ignore AccessTime and ChangeTime unless
+	// the format is explicitly chosen.
+	// This ensures nominal usage of WriteHeader (without specifying the format)
+	// does not always result in the PAX format being chosen, which
+	// causes a 1KiB increase to every header.
+	if tw.hdr.Format == FormatUnknown {
+		tw.hdr.ModTime = tw.hdr.ModTime.Round(time.Second)
+		tw.hdr.AccessTime = time.Time{}
+		tw.hdr.ChangeTime = time.Time{}
+	}
+
 	allowedFormats, paxHdrs, err := tw.hdr.allowedFormats()
 	switch {
 	case allowedFormats.has(FormatUSTAR):