Issue 90810043: code review 90810043: archive/zip Document ModTime is always UTC
Description archive/zip: Document ModTime is always UTC
Fixes issue 7592
Patch Set 1 #Patch Set 2 : diff -r fa39a7dac971 https://code.google.com/p/go #Patch Set 3 : diff -r b0443478e712 https://code.google.com/p/go #
Total comments: 2
Patch Set 4 : diff -r 2e591e82a8c8 https://code.google.com/p/go #
Total comments: 4
Patch Set 5 : diff -r 2e591e82a8c8 https://code.google.com/p/go #MessagesTotal messages: 12
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
Please also modify the CL description as follows, sans dashes: --- archive/zip: Document ModTime is always UTC Fixes issue 7592. --- The colon is just established convention. The "Fixes issue" line serves several purposes purposes: - It allows http://go-dev.appspot.com to properly display your CL as fixing an issue. - It adds a comment in the issue tracker that there is a CL addressing this issue. - When the issue is closed, it will automatically close the bug in the issue tracker with a descriptive comment of your commit. https://codereview.appspot.com/90810043/diff/40001/src/pkg/archive/zip/struct.go File src/pkg/archive/zip/struct.go (right): https://codereview.appspot.com/90810043/diff/40001/src/pkg/archive/zip/struct... src/pkg/archive/zip/struct.go:179: // The timezone of the returned time is always converted to UTC. maybe: The time is assumed to have been stored as UTC? I don't know if I love my wording above, but I'm trying to convey that no real conversion is done to UTC as it is in timeToMsDosTime. It is just assumed the modified time is already UTC. Also trying to convey that depending on what program zipped the file, this value may not actually be in UTC, and the programmer should be aware. https://codereview.appspot.com/90810043/diff/40001/src/pkg/archive/zip/struct... src/pkg/archive/zip/struct.go:186: // The timezone of provided time is always converted to UTC. maybe: s/timezone of provided time/provided time t/?
Sign in to reply to this message.
I've made the appropriate changes and uploaded them. Please review again. Thanks! On May 3, 2014 at 7:15:02 AM, robert.hencke@gmail.com (robert.hencke@gmail.com) wrote: Please also modify the CL description as follows, sans dashes: --- archive/zip: Document ModTime is always UTC Fixes issue 7592. --- The colon is just established convention. The "Fixes issue" line serves several purposes purposes: - It allows http://go-dev.appspot.com to properly display your CL as fixing an issue. - It adds a comment in the issue tracker that there is a CL addressing this issue. - When the issue is closed, it will automatically close the bug in the issue tracker with a descriptive comment of your commit. https://codereview.appspot.com/90810043/diff/40001/src/pkg/archive/zip/struct.go File src/pkg/archive/zip/struct.go (right): https://codereview.appspot.com/90810043/diff/40001/src/pkg/archive/zip/struct... src/pkg/archive/zip/struct.go:179: // The timezone of the returned time is always converted to UTC. maybe: The time is assumed to have been stored as UTC? I don't know if I love my wording above, but I'm trying to convey that no real conversion is done to UTC as it is in timeToMsDosTime. It is just assumed the modified time is already UTC. Also trying to convey that depending on what program zipped the file, this value may not actually be in UTC, and the programmer should be aware. https://codereview.appspot.com/90810043/diff/40001/src/pkg/archive/zip/struct... src/pkg/archive/zip/struct.go:186: // The timezone of provided time is always converted to UTC. maybe: s/timezone of provided time/provided time t/? https://codereview.appspot.com/90810043/
Sign in to reply to this message.
LGTM - leaving for adg as he is the original author. Thanks!
Sign in to reply to this message.
R=adg@golang.org (assigned by robert.hencke@gmail.com)
Sign in to reply to this message.
https://codereview.appspot.com/90810043/diff/60001/src/pkg/archive/zip/struct.go File src/pkg/archive/zip/struct.go (right): https://codereview.appspot.com/90810043/diff/60001/src/pkg/archive/zip/struct... src/pkg/archive/zip/struct.go:177: // ModTime returns the modification time. "...time in UTC." https://codereview.appspot.com/90810043/diff/60001/src/pkg/archive/zip/struct... src/pkg/archive/zip/struct.go:179: // Assumes the time was set as UTC. delete https://codereview.appspot.com/90810043/diff/60001/src/pkg/archive/zip/struct... src/pkg/archive/zip/struct.go:184: // SetModTime sets the ModifiedTime and ModifiedDate fields to the given time. "... given time in UTC." https://codereview.appspot.com/90810043/diff/60001/src/pkg/archive/zip/struct... src/pkg/archive/zip/struct.go:186: // The provided time t is always converted to UTC. delete
Sign in to reply to this message.
Keep it simple, stupid. :) Fixed and uploaded. On May 4, 2014 at 7:44:43 PM, adg@golang.org (adg@golang.org) wrote: https://codereview.appspot.com/90810043/diff/60001/src/pkg/archive/zip/struct.go File src/pkg/archive/zip/struct.go (right): https://codereview.appspot.com/90810043/diff/60001/src/pkg/archive/zip/struct... src/pkg/archive/zip/struct.go:177: // ModTime returns the modification time. "...time in UTC." https://codereview.appspot.com/90810043/diff/60001/src/pkg/archive/zip/struct... src/pkg/archive/zip/struct.go:179: // Assumes the time was set as UTC. delete https://codereview.appspot.com/90810043/diff/60001/src/pkg/archive/zip/struct... src/pkg/archive/zip/struct.go:184: // SetModTime sets the ModifiedTime and ModifiedDate fields to the given time. "... given time in UTC." https://codereview.appspot.com/90810043/diff/60001/src/pkg/archive/zip/struct... src/pkg/archive/zip/struct.go:186: // The provided time t is always converted to UTC. delete https://codereview.appspot.com/90810043/
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=969a38842696 *** archive/zip: Document ModTime is always UTC Fixes issue 7592 LGTM=robert.hencke, adg R=golang-codereviews, robert.hencke, gobot, adg CC=golang-codereviews https://codereview.appspot.com/90810043 Committer: Andrew Gerrand <adg@golang.org>
Sign in to reply to this message.
This CL appears to have broken the darwin-386-cheney builder. See http://build.golang.org/log/49be1c190ba8817a4c41e780d1da0d2e6d8bfc2c
Sign in to reply to this message.
Nah, this is a false alarm. Might be time to reboot this bulder On Mon, May 5, 2014 at 1:03 PM, <gobot@golang.org> wrote: > This CL appears to have broken the darwin-386-cheney builder. > See http://build.golang.org/log/49be1c190ba8817a4c41e780d1da0d2e6d8bfc2c > > > https://codereview.appspot.com/90810043/ > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout.
Sign in to reply to this message.
On 2014/05/05 03:03:36, gobot wrote: > This CL appears to have broken the darwin-386-cheney builder. > See http://build.golang.org/log/49be1c190ba8817a4c41e780d1da0d2e6d8bfc2c Unrelated.
Sign in to reply to this message.
|
