Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

archive/zip: record time stamps in "Extended Timestamp Extra Field" #10242

Closed
mattn opened this issue Mar 25, 2015 · 14 comments
Closed

archive/zip: record time stamps in "Extended Timestamp Extra Field" #10242

mattn opened this issue Mar 25, 2015 · 14 comments
Labels
FrozenDueToAge
Milestone
Go1.8Maybe

Comments

@mattn
Copy link
Member

mattn commented Mar 25, 2015 

    out, err := ioutil.TempFile(os.TempDir(), "zip")
    if err != nil {
        t.Fatal("creating:", err)
    }
    zw := NewWriter(out)

    in, err := os.Open("writer_test.go")
    if err != nil {
        t.Fatal("opening:", err)
    }
    info, err := in.Stat()
    if err != nil {
        t.Fatal("stat:", err)
    }
    ftime := info.ModTime().Unix()

    header, err := FileInfoHeader(info)
    if err != nil {
        t.Fatal("FileInfoHeader:", err)
    }
    header.Name = info.Name()
    zf, err := zw.CreateHeader(header)
    if err != nil {
        t.Fatal("CreateHeader:", err)
    }
    if _, err = io.Copy(zf, in); err != nil {
        t.Fatal("copying:", err)
    }
    in.Close()
    zw.Close()
    out.Close()

timestamp of the file in the zip, is UTC timezone. So +9 hours in my location.
@mattn
Copy link
Member Author

mattn commented Mar 25, 2015

Most of zip archiver store timestamp as local timezone.

https://go-review.googlesource.com/#/c/8070/

@bradfitz
Copy link
Contributor

I don't think we can safely make this change under the Go 1 compatibility guarantee.

@mikioh mikioh changed the title archive/zip make strange file timestamp archive/zip: make strange file timestamp Mar 30, 2015
@mattn
Copy link
Member Author

mattn commented Apr 1, 2015

I'm thinking this is a bug. So we must not keep compatibility.

$ LC_ALL=C ls -la
total 12
drwxrwxr-x  2 mattn mattn 4096 Apr  1 14:55 .
drwxr-x--- 49 mattn mattn 4096 Apr  1 14:55 ..
-rw-r--r--  1 mattn mattn  487 Apr  1 14:55 ziptest.go

make test.zip contained ziptest.go

$ zip test.zip ziptest.go
  adding: ziptest.go (deflated 45%)

package main

import (
        "archive/zip"
        "fmt"
        "log"
        "os"
)

func main() {
        in, err := os.Open("test.zip")
        if err != nil {
                log.Fatal("opening:", err)
        }
        defer in.Close()
        info, err := in.Stat()
        if err != nil {
                log.Fatal("stat:", err)
        }
        zr, err := zip.NewReader(in, info.Size())
        if err != nil {
                log.Fatal("NewReader:", err)
        }
        if len(zr.File) != 1 {
                log.Fatalf("File count should be 1 but %v", len(zr.File))
        }
        ztime := zr.File[0].ModTime()
        fmt.Println(ztime.String())
}
$ go run ziptest.go
2015-04-01 14:55:12 +0000 UTC

Note that this is not a joke at april fool.

@mattn
Copy link
Member Author

mattn commented Apr 1, 2015

I'm in JST+0900 offset

@ianlancetaylor
Copy link
Contributor

As I understand it, the only possibly correct fix is to use the "Extended Timestamp Extra Field" defined at http://www.opensource.apple.com/source/zip/zip-6/unzip/unzip/proginfo/extra.fld .

@rsc
Copy link
Contributor

rsc commented Apr 10, 2015

This came up in #7592, which we fixed by updating the documentation for the ModTime and SetModTime methods. It is not possible to change the UTC default.

It may be that if we set this extended field as well, enough clients will use the Unix form in preference to the MS-DOS form that they will end up doing the right thing.

@rsc rsc changed the title archive/zip: make strange file timestamp archive/zip: record time stamps in "Extended Timestamp Extra Field" Apr 10, 2015
@rsc rsc added this to the Go1.5Maybe milestone Apr 10, 2015
@mattn
Copy link
Member Author

mattn commented Apr 15, 2015

I looked code of unzip.

unzip command can extract timestamp as local timezon without looking extra field.
So we need to to add extra variable to change behavior of Reader/Writer into archive/zip.

Proposal

  1. add DefaultTimeZone into archive/zip

    var DefaultTimeZone *time.Location

    This will be used as default timezone for Reader/ReadCloser/Writer.

  2. add SetTimeZone() into Reader/ReadCloser/Writer. DefaultTimeZone is used as default timezone for them.

How do you think?

@ianlancetaylor
Copy link
Contributor

Which version of unzip did you look at?

I don't know if extending the API is the way to go, but, if it is, why would we need both DefaultTimeZone and SetTimeZone?

@mattn
Copy link
Member Author

mattn commented Apr 15, 2015

I looked master branch of https://github.com/Distrotech/unzip

@rsc
Copy link
Contributor

rsc commented Jun 29, 2015

Too late for Go 1.5.

@rsc rsc modified the milestones: Unplanned, Go1.5Maybe Jun 29, 2015
@gopherbot
Copy link

CL https://golang.org/cl/18274 mentions this issue.

@bradfitz bradfitz modified the milestones: Go1.8Maybe, Unplanned Aug 22, 2016
@gopherbot
Copy link

CL https://golang.org/cl/34651 mentions this issue.

gopherbot pushed a commit that referenced this issue Dec 20, 2016
@dsnet
Revert: "archive/zip: handle mtime in NTFS/UNIX/ExtendedTS extra fields"
5df59a4 
This change reverts the following CLs:
	CL/18274: handle mtime in NTFS/UNIX/ExtendedTS extra fields
	CL/30811: only use Extended Timestamp on non-zero MS-DOS timestamps

We are reverting support for extended timestamps since the support was not
not complete. CL/18274 added full support for reading extended timestamp fields
and minimal support for writing them. CL/18274 is incomplete because it made
no changes to the FileHeader struct, so timezone information was lost when
reading and/or writing.

While CL/18274 was a step in the right direction, we should provide full
support for high precision timestamps in both the reader and writer.
This will probably require that we add a new field of type time.Time.
The complete fix is too involved to add in the time remaining for Go 1.8
and will be completed in Go 1.9.

Updates #10242
Updates #17403
Updates #18359
Fixes #18378

Change-Id: Icf6d028047f69379f7979a29bfcb319a02f4783e
Reviewed-on: https://go-review.googlesource.com/34651
Run-TryBot: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@dsnet dsnet mentioned this issue Dec 20, 2016
Closed
@gopherbot
Copy link

Change https://golang.org/cl/44394 mentions this issue: archive/zip: Store the main timestamp as local time.

@gopherbot
Copy link

Change https://golang.org/cl/36078 mentions this issue: archive/zip: handle mtime in NTFS/UNIX/ExtendedTS extra fields

@golang golang locked and limited conversation to collaborators Nov 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.8Maybe
Development

No branches or pull requests
5 participants
@bradfitz @mattn @rsc @ianlancetaylor @gopherbot