Issue 5440130: code review 5440130: archive/zip: make zip understand os.FileMode.
Descriptionarchive/zip: make zip understand os.FileMode. Fixes implicit dependency on underlying os file modes. Patch Set 1 #Patch Set 2 : diff -r 8e8c4fb05171 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r 8e8c4fb05171 https://go.googlecode.com/hg/ # Total comments: 7 Patch Set 4 : diff -r 8e8c4fb05171 https://go.googlecode.com/hg/ #Patch Set 5 : diff -r 8e8c4fb05171 https://go.googlecode.com/hg/ #Patch Set 6 : diff -r 8e8c4fb05171 https://go.googlecode.com/hg/ #Patch Set 7 : diff -r 8e8c4fb05171 https://go.googlecode.com/hg/ #
MessagesTotal messages: 17
Hello rsc@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/ Sign in to reply to this message.
LGTM modulo one tiny edit no need to single out team members for generic reviews such as this. just use the group alias. http://codereview.appspot.com/5440130/diff/1002/src/pkg/archive/zip/struct.go File src/pkg/archive/zip/struct.go (right): http://codereview.appspot.com/5440130/diff/1002/src/pkg/archive/zip/struct.go... src/pkg/archive/zip/struct.go:117: // traditional names for unix constants s/unix/Unix/ Sign in to reply to this message.
Nice, thanks for taking care of this Roger. http://codereview.appspot.com/5440130/diff/1002/src/pkg/archive/zip/struct.go File src/pkg/archive/zip/struct.go (right): http://codereview.appspot.com/5440130/diff/1002/src/pkg/archive/zip/struct.go... src/pkg/archive/zip/struct.go:133: switch { This switch may be built as: switch mode&os.ModeType { case os.ModeDevice: ... } http://codereview.appspot.com/5440130/diff/1002/src/pkg/archive/zip/struct.go... src/pkg/archive/zip/struct.go:135: m = s_IFBLK It's a bit unfortunate that getting and setting the same mode may corrupt the data. Sign in to reply to this message.
http://codereview.appspot.com/5440130/diff/1002/src/pkg/archive/zip/struct.go File src/pkg/archive/zip/struct.go (right): http://codereview.appspot.com/5440130/diff/1002/src/pkg/archive/zip/struct.go... src/pkg/archive/zip/struct.go:119: s_IFMT = 0xf000 Does the ZIP format define these? I have no idea where they came from. Sign in to reply to this message.
> src/pkg/archive/zip/struct.go:119: s_IFMT = 0xf000 > Does the ZIP format define these? > I have no idea where they came from. It's unfortunately a bit vague indeed, and the documentation doesn't mention the actual format despite saying that the compatibility of the file mode attributes is "UNIX". These constants seem to be generally accepted for that, though. E.g. zipinfo seems to agree. Sign in to reply to this message.
On Wed, Dec 7, 2011 at 16:19, <n13m3y3r@gmail.com> wrote: > It's unfortunately a bit vague indeed, and the documentation > doesn't mention the actual format despite saying that the > compatibility of the file mode attributes is "UNIX". These > constants seem to be generally accepted for that, though. > E.g. zipinfo seems to agree. So where did the numbers come from? You say that zipinfo agrees, but not what it agrees with. Russ Sign in to reply to this message.
> So where did the numbers come from? > You say that zipinfo agrees, but not what it agrees with. I didn't write the code, so I don't know where Roger got the numbers from. What I know is that the documentation doesn't mention the constants, and that a well known application for printing information about zip files uses the same constants. Sign in to reply to this message.
http://codereview.appspot.com/5440130/diff/1002/src/pkg/archive/zip/struct.go File src/pkg/archive/zip/struct.go (right): http://codereview.appspot.com/5440130/diff/1002/src/pkg/archive/zip/struct.go... src/pkg/archive/zip/struct.go:119: s_IFMT = 0xf000 On 2011/12/07 20:11:05, rsc wrote: > Does the ZIP format define these? > I have no idea where they came from. i got them from syscall/zerrors_darwin_amd64.go as it happens, but all the other Unix files have the same definitions. presumably they're standard, to some degree. http://codereview.appspot.com/5440130/diff/1002/src/pkg/archive/zip/struct.go... src/pkg/archive/zip/struct.go:133: switch { On 2011/12/07 20:09:21, niemeyer wrote: > This switch may be built as: > > switch mode&os.ModeType { > case os.ModeDevice: > ... > } this isn't quite equivalent if mode has more than one bit set. i thought about doing it your way, but decided that if a file had more than one type mode set, i'd prefer one case to trigger than none. i'm happy to change it if people prefer it the other way though. http://codereview.appspot.com/5440130/diff/1002/src/pkg/archive/zip/struct.go... src/pkg/archive/zip/struct.go:135: m = s_IFBLK On 2011/12/07 20:09:21, niemeyer wrote: > It's a bit unfortunate that getting and setting the same mode may corrupt the > data. if you're zipping device files, you're corrupting them anyway... Sign in to reply to this message.
> this isn't quite equivalent if mode has more than one bit set. All of the type bits covered in that switch are mutually exclusive. > if you're zipping device files, you're corrupting them anyway... The usefulness of this information is indeed a dubious, but there's still a difference between having missing information and having incorrect information. Sign in to reply to this message.
Why are we bothering with these bits at all? Russ Sign in to reply to this message.
> Why are we bothering with these bits at all? True. The useful bits are directory vs. regular, and all of the permission bits (including setuid/setgid). -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I'm not absolutely sure of anything. Sign in to reply to this message.
Hello rsc@golang.org, r@golang.org, n13m3y3r@gmail.com, gustavo@niemeyer.net (cc: golang-dev@googlegroups.com), Please take another look. Sign in to reply to this message.
On 7 December 2011 23:14, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:>> Why are we bothering with these bits at all?>> True. The useful bits are directory vs. regular, and all of the> permission bits (including setuid/setgid). following that suggestion, i've significantly changed the mode conversion semantics. modes other than the directory mode are ignored, and i've added some behaviour based on actual zip implementations. looking into the old zip source code was a hoot. it would be nice to add more zip files generated on other platforms to testdata. Sign in to reply to this message.
LGTM, but since rsc had issues before I'll let him submit. WHAT A WONDERFUL FILE FORMAT ZIP IS. Jeeze. Sign in to reply to this message.
lgtm. yuck. Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=264fb4d09991 *** archive/zip: make zip understand os.FileMode. Fixes implicit dependency on underlying os file modes. R=rsc, r, n13m3y3r, gustavo, adg CC=golang-dev http://codereview.appspot.com/5440130 Committer: Russ Cox <rsc@golang.org> Sign in to reply to this message. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
