Issue 69110043: utils/zip: new package

Can't Edit
Can't Publish+Mail
Start Review

Created:
10 years, 2 months ago by fwereade

Modified:
10 years, 2 months ago

Reviewers:
mp+208327, dimitern, rog

Visibility:
Public.

Description

utils/zip: new package Apart from Walk, which looked useful, everything the package exports is either currently or imminently useful in state/apiserver and/or charm. Will let us drop the near-duplicated zip-extraction code in both places, and help us implement both charm bundle manifests and git-free charm upgrades. https://code.launchpad.net/~fwereade/juju-core/zip-utils-package/+merge/208327 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 45

Patch Set 2 : utils/zip: new package #

Patch Set 3 : utils/zip: new package #

Created: 10 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+643 lines, -0 lines)			Patch
A	[revision details]	View	1 2	1 chunk	+2 lines, -0 lines	0 comments	Download
A	utils/zip/package_test.go	View	1	1 chunk	+117 lines, -0 lines	0 comments	Download
A	utils/zip/zip.go	View	1 2	1 chunk	+197 lines, -0 lines	0 comments	Download
A	utils/zip/zip_test.go	View	1 2	1 chunk	+327 lines, -0 lines	0 comments	Download

Messages

Total messages: 6

Expand All Messages | Collapse All Messages

dimitern

LGTM with some suggestions. I'd really like if we can add a couple of windows-created ...

10 years, 2 months ago (2014-02-26 12:42:54 UTC) #2

dimitern

LGTM with some suggestions. I'd really like if we can add a couple of windows-created ...

10 years, 2 months ago (2014-02-26 12:42:54 UTC) #3

rog

Mostly great, with some suggestions below. https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go File utils/zip/zip.go (right): https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode19 utils/zip/zip.go:19: func Walk(reader *zip.Reader, ...

10 years, 2 months ago (2014-02-26 13:18:44 UTC) #4

Mostly great, with some suggestions below.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go
File utils/zip/zip.go (right):

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode19
utils/zip/zip.go:19: func Walk(reader *zip.Reader, callback func(zipFile
*zip.File) error) error {
I don't really see what this offers over an explicit
"range" statement. Even the cleanName part is arguably
not that useful - if something fails, perhaps it's
because of something to do with the non-cleaned name.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode43
utils/zip/zip.go:43: callback := func(zipFile *zip.File) error {
    for _, file := range reader.File {
        cleanPath := path.Clean(zipFile.Name)
        baseName := path.Base(cleanPath)
        if ... {
             matches = append(matches, cleanPath)
        }
    }

is shorter and more obvious IMHO

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode64
utils/zip/zip.go:64: // only where necessary. If the source path identifies a
single file, it will be
This doesn't seem to describe the semantics correctly. In particular,
it doesn't specify that the source path prefix is also removed when extracting.

How about:

// Extract extracts any files from the supplied zip
// reader within the given sourcePath directory to
// the target path, stripping that prefix before doing so.
// If sourcePath does not specify a directory, targetPath
// will be created with the contents of sourcePath.
//
// The target path is OS-specific, but sourcePath should
// be a slash-separated path.

?

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode69
utils/zip/zip.go:69: sourcePath = ""
Why not just use the output of Clean as the canonical representation?
I think we're going to need a slightly more sophisticated prefix
check anyway (see below).

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode73
utils/zip/zip.go:73: return Walk(reader, extractor{targetPath,
sourcePath}.expand)
even here, Walk doesn't really help readability much.
With the explicit loop, it's easier to see what
happens on error, and it's also easier for us
to add a more appropriate annotation to the
error if we want ("expand" is more self-explanatory
than "process")

    e := extractor{targetPath, sourcePath}
    for _, file := range reader.File {
        if err := e.expand(file); err != nil {
            return fmt.Errorf("cannot expand %q: %v", path.Clean(file.Name),
err)
        }
    }
    return nil

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode81
utils/zip/zip.go:81: func (x extractor) path(zipFile *zip.File) (string, bool) {
// path returns the target path for a given zip file
// and reports whether it should be extracted.
func (x extractor) path(zipFile *zip.File) (targetPath string, ok bool) {

?

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode83
utils/zip/zip.go:83: if !strings.HasPrefix(cleanPath, x.sourcePath) {
I'm sure this is quite right.

If I do Extract(r, "somedir", "hooks"), I probably
don't want to extract hooksReadMe.txt too.

Something like this, perhaps? (it would be nice
if something like this existed in the path package)

// hasPrefix reports whether the given
// cleaned, slash separated path p has
// the given directory prefix.
func hasPrefix(p, prefix string) bool {
    if prefix == "." {
        return true
    }
    if !strings.HasPrefix(p, prefix) {
        return false
    }
    if len(p) == len(prefix) {
        return true
    }
    return p[len(prefix)] == '/'
}

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode94
utils/zip/zip.go:94: filePath, ok := x.path(zipFile)
I wonder if "targetPath" might be a more obvious name
than this (I found myself here wondering what the name
actually signified). I realise that clashes with extractor.targetPath,
but perhaps extractor.targetRoot and extractor.sourceRoot
might work well there.

The difference between filepath and filePath is really quite
subtle, and filePath doesn't really say much about
the path.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode99
utils/zip/zip.go:99: if err := os.MkdirAll(parentPath, os.ModePerm); err != nil
{
I think it's probably more idiomatic to use 0777 here
rather than os.ModePerm (I had to look up ModePerm
to check whether it had included anything else)

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode122
utils/zip/zip.go:122: if mode&os.ModePerm != modePerm {
We're going to need to be careful about zip files
created under Windows here. Specifically, Windows
doesn't do group and user modes well - this could
potentially be problematic, but I'm not sure what
to do about it other than mode &= 022.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode138
utils/zip/zip.go:138: if err := os.RemoveAll(filePath); err != nil {
I'm not entirely convinced we want to remove every file
before overwriting it with possibly exactly the same contents.

I wonder if it would be better to write to a new file
and atomically rename to the target. That way we don't
have a time period where the file is removed, or is
an incomplete state.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode167
utils/zip/zip.go:167: targetPath := buffer.String()
s/targetPath/symlinkTarget/ ?

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip_test.go
File utils/zip/zip_test.go (right):

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip_test.go#newcode36
utils/zip/zip_test.go:36: "file", "symlink", "dir", "dir/dir", "dir/dir/file",
"dir/dir/symlink",
On 2014/02/26 12:42:55, dimitern wrote:
> i'd prefer it if you insert line breaks after each string, to make it easier
to
> match the list of creators above.

+1

fwereade

Please take a look. https://codereview.appspot.com/69110043/diff/1/utils/zip/package_test.go File utils/zip/package_test.go (right): https://codereview.appspot.com/69110043/diff/1/utils/zip/package_test.go#newcode1 utils/zip/package_test.go:1: // Copyright 2013 Canonical Ltd. ...

10 years, 2 months ago (2014-02-26 20:18:14 UTC) #5

Please take a look.

https://codereview.appspot.com/69110043/diff/1/utils/zip/package_test.go
File utils/zip/package_test.go (right):

https://codereview.appspot.com/69110043/diff/1/utils/zip/package_test.go#newc...
utils/zip/package_test.go:1: // Copyright 2013 Canonical Ltd.
On 2014/02/26 12:42:55, dimitern wrote:
> 2014

I'll do 2011-2014 across the board, a lot of this code is copied/tweaked/rebuilt
rather than clean-roomed.

https://codereview.appspot.com/69110043/diff/1/utils/zip/package_test.go#newc...
utils/zip/package_test.go:36: cmd := exec.Command("/bin/sh", "-c",
fmt.Sprintf("cd %s; zip --fifo --symlinks -r %s .", basePath, outPath))
On 2014/02/26 12:42:55, dimitern wrote:
> s/%s/%q/ ?

Done.

https://codereview.appspot.com/69110043/diff/1/utils/zip/package_test.go#newc...
utils/zip/package_test.go:92: if !c.Check(err, gc.IsNil) {
On 2014/02/26 12:42:55, dimitern wrote:
> ha! i didn't know c.Check actually returns anything.

It's *really* useful sometimes :).

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go
File utils/zip/zip.go (right):

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode1
utils/zip/zip.go:1: // Copyright 2011, 2012, 2013, 2014 Canonical Ltd.
On 2014/02/26 12:42:55, dimitern wrote:
> 2014 only?
Done as discussed.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode19
utils/zip/zip.go:19: func Walk(reader *zip.Reader, callback func(zipFile
*zip.File) error) error {
On 2014/02/26 13:18:44, rog wrote:
> I don't really see what this offers over an explicit
> "range" statement. Even the cleanName part is arguably
> not that useful - if something fails, perhaps it's
> because of something to do with the non-cleaned name.

Eh, I see it both ways. Walk honestly feels more natural to me, but I'll live
without it and see if it really upsets me ;).

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode43
utils/zip/zip.go:43: callback := func(zipFile *zip.File) error {
On 2014/02/26 13:18:44, rog wrote:
>     for _, file := range reader.File {
>         cleanPath := path.Clean(zipFile.Name)
>         baseName := path.Base(cleanPath)
>         if ... {
>              matches = append(matches, cleanPath)
>         }
>     }
> 
> is shorter and more obvious IMHO

Done.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode64
utils/zip/zip.go:64: // only where necessary. If the source path identifies a
single file, it will be
On 2014/02/26 13:18:44, rog wrote:
> This doesn't seem to describe the semantics correctly. In particular,
> it doesn't specify that the source path prefix is also removed when
extracting.
> 
> How about:
> 
> // Extract extracts any files from the supplied zip
> // reader within the given sourcePath directory to
> // the target path, stripping that prefix before doing so.
> // If sourcePath does not specify a directory, targetPath
> // will be created with the contents of sourcePath.
> //
> // The target path is OS-specific, but sourcePath should
> // be a slash-separated path.
> 
> ?

Done, approximately.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode69
utils/zip/zip.go:69: sourcePath = ""
On 2014/02/26 13:18:44, rog wrote:
> Why not just use the output of Clean as the canonical representation?
> I think we're going to need a slightly more sophisticated prefix
> check anyway (see below).

I experimented with that while noodling (see below), but it didn't really seem
to help much.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode73
utils/zip/zip.go:73: return Walk(reader, extractor{targetPath,
sourcePath}.expand)
On 2014/02/26 13:18:44, rog wrote:
> even here, Walk doesn't really help readability much.
> With the explicit loop, it's easier to see what
> happens on error, and it's also easier for us
> to add a more appropriate annotation to the
> error if we want ("expand" is more self-explanatory
> than "process")
> 
>     e := extractor{targetPath, sourcePath}
>     for _, file := range reader.File {
>         if err := e.expand(file); err != nil {
>             return fmt.Errorf("cannot expand %q: %v", path.Clean(file.Name),
> err)
>         }
>     }
>     return nil
> 

Done.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode81
utils/zip/zip.go:81: func (x extractor) path(zipFile *zip.File) (string, bool) {
On 2014/02/26 13:18:44, rog wrote:
> // path returns the target path for a given zip file
> // and reports whether it should be extracted.
> func (x extractor) path(zipFile *zip.File) (targetPath string, ok bool) {
> 
> ?

Done.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode83
utils/zip/zip.go:83: if !strings.HasPrefix(cleanPath, x.sourcePath) {
On 2014/02/26 13:18:44, rog wrote:
> I'm sure this is quite right.
> 
> If I do Extract(r, "somedir", "hooks"), I probably
> don't want to extract hooksReadMe.txt too.
> 
> Something like this, perhaps? (it would be nice
> if something like this existed in the path package)
> 
> // hasPrefix reports whether the given
> // cleaned, slash separated path p has
> // the given directory prefix.
> func hasPrefix(p, prefix string) bool {
>     if prefix == "." {
>         return true
>     }
>     if !strings.HasPrefix(p, prefix) {
>         return false
>     }
>     if len(p) == len(prefix) {
>         return true
>     }
>     return p[len(prefix)] == '/'
> }
>    

Thanks, well spotted. Added test, noodled around extensively, ended up with
something quite dissimilar but (I think) clear and correct.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode87
utils/zip/zip.go:87: if strings.HasPrefix(relativePath, "/") {
On 2014/02/26 12:42:55, dimitern wrote:
> how about:
> 
> relativePath, err := filepath.Rel(x.sourcePath, cleanPath)
> if err != nil {
>     logger.Debugf("%q is not relative to zip root %q: %v", cleanPath,
> x.sourcePath, err)
>     return "", false
> }
> // Now relativePath can start with ".." and we don't need that.
> if !isSanePath(relativePath) {
>     logger.Debugf("%q points outside the zip root %q", cleanPath,
x.sourcePath)
>     return "", false
> }
> return ... as before (no need for relativePath[:1])

The trouble there is that "path" doesn't have Rel, but "filepath" isn't
necessarily appropriate because it doesn't always use / as a separator.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode94
utils/zip/zip.go:94: filePath, ok := x.path(zipFile)
On 2014/02/26 13:18:44, rog wrote:
> I wonder if "targetPath" might be a more obvious name
> than this (I found myself here wondering what the name
> actually signified). I realise that clashes with extractor.targetPath,
> but perhaps extractor.targetRoot and extractor.sourceRoot
> might work well there.
> 
> The difference between filepath and filePath is really quite
> subtle, and filePath doesn't really say much about
> the path.

SGTM. Done.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode96
utils/zip/zip.go:96: return nil
On 2014/02/26 12:42:55, dimitern wrote:
> this seems wrong - we should report an error if we can, and path can return it
> (see previous).

I don't think it's an error -- we're just walking the zip file and ignoring the
bits outside the scope we've been asked to care about.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode99
utils/zip/zip.go:99: if err := os.MkdirAll(parentPath, os.ModePerm); err != nil
{
On 2014/02/26 13:18:44, rog wrote:
> I think it's probably more idiomatic to use 0777 here
> rather than os.ModePerm (I had to look up ModePerm
> to check whether it had included anything else)

Fair enough. Done.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode127
utils/zip/zip.go:127: fallthrough
On 2014/02/26 12:42:55, dimitern wrote:
> why fallthrough?

if err was nil, and we didn't have a dir, it must have been a file or symlink,
so we should delete it in order to create the file.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode138
utils/zip/zip.go:138: if err := os.RemoveAll(filePath); err != nil {
On 2014/02/26 13:18:44, rog wrote:
> I'm not entirely convinced we want to remove every file
> before overwriting it with possibly exactly the same contents.
> 
> I wonder if it would be better to write to a new file
> and atomically rename to the target. That way we don't
> have a time period where the file is removed, or is
> an incomplete state.

If we complete without error, we're good; if not the state of the whole target
is indeterminate anyway, so worries about the integrity of any single file seem
misplaced to me.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip.go#newcode167
utils/zip/zip.go:167: targetPath := buffer.String()
On 2014/02/26 13:18:44, rog wrote:
> s/targetPath/symlinkTarget/ ?

Done.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip_test.go
File utils/zip/zip_test.go (right):

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip_test.go#newcode1
utils/zip/zip_test.go:1: // Copyright 2011, 2012, 2013, 2014 Canonical Ltd.
On 2014/02/26 12:42:55, dimitern wrote:
> just 2014?

same

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip_test.go#newcode36
utils/zip/zip_test.go:36: "file", "symlink", "dir", "dir/dir", "dir/dir/file",
"dir/dir/symlink",
On 2014/02/26 13:18:44, rog wrote:
> On 2014/02/26 12:42:55, dimitern wrote:
> > i'd prefer it if you insert line breaks after each string, to make it easier
> to
> > match the list of creators above.
> 
> +1

Done.

https://codereview.appspot.com/69110043/diff/1/utils/zip/zip_test.go#newcode277
utils/zip/zip_test.go:277: c.Assert(err, gc.IsNil)
On 2014/02/26 12:42:55, dimitern wrote:
> for the bad cases there should be some logging at least, otherwise we might
> puzzle the user what happened to her outside-of-root entries?

I don't think so -- sourcePath is just a filter. Repeatedly logging "skipped
file you asked me to skip" doesn't seem that valuable to me.

Please take a look.

Expand All Messages | Collapse All Messages