Merge lp:~fwereade/juju-core/fix-1129319 into lp:~go-bot/juju-core/trunk

[William Reade]

charm: local repos follow symlinks in series dirs

fixes lp:1129319

https://codereview.appspot.com/10302043/

charm: local repos follow symlinks in series dirs

fixes lp:1129319

https://codereview.appspot.com/10302043/

[William Reade (fwereade)]

Reviewers: mp+169625_code.launchpad.net,

Message:
Please take a look.

Description:
charm: local repos follow symlinks in series dirs

fixes lp:1129319

https://code.launchpad.net/~fwereade/juju-core/fix-1129319/+merge/169625

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/10302043/

Affected files:
   A [revision details]
   M charm/charm_test.go
   M charm/repo.go
   M charm/repo_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20130614080827-ro5z9b8h1i5k37ow
+New revision: <email address hidden>

Index: charm/charm_test.go
=== modified file 'charm/charm_test.go'
--- charm/charm_test.go 2013-05-02 15:55:42 +0000
+++ charm/charm_test.go 2013-06-15 14:46:57 +0000
@@ -71,7 +71,6 @@
   switch f := f.(type) {
   case *charm.Bundle:
    c.Assert(f.Path, Equals, path)
-
   case *charm.Dir:
    c.Assert(f.Path, Equals, path)
   }

Index: charm/repo.go
=== modified file 'charm/repo.go'
--- charm/repo.go 2013-06-10 20:50:42 +0000
+++ charm/repo.go 2013-06-15 14:46:57 +0000
@@ -295,11 +295,21 @@
   return &NotFoundError{fmt.Sprintf("charm not found in %q: %s", repoPath,
curl)}
  }

-func mightBeCharm(info os.FileInfo) bool {
+func mightBeCharm(chPath string, info os.FileInfo) (bool, error) {
+ repoName := info.Name()
+ if info.Mode()&os.ModeSymlink != 0 {
+ var err error
+ if chPath, err = os.Readlink(chPath); err != nil {
+ return false, err
+ }
+ if info, err = os.Stat(chPath); err != nil {
+ return false, err
+ }
+ }
   if info.IsDir() {
- return !strings.HasPrefix(info.Name(), ".")
+ return !strings.HasPrefix(repoName, "."), nil
   }
- return strings.HasSuffix(info.Name(), ".charm")
+ return strings.HasSuffix(repoName, ".charm"), nil
  }

  // Get returns a charm matching curl, if one exists. If curl has a
revision of
@@ -326,10 +336,12 @@
   }
   var latest Charm
   for _, info := range infos {
- if !mightBeCharm(info) {
+ chPath := filepath.Join(path, info.Name())
+ if ok, err := mightBeCharm(chPath, info); err != nil {
+ return nil, err
+ } else if !ok {
     continue
    }
- chPath := filepath.Join(path, info.Name())
    if ch, err := Read(chPath); err != nil {
     log.Warningf("charm: failed to load charm at %q: %s", chPath, err)
    } else if ch.Meta().Name == curl.Name {

Index: charm/repo_test.go
=== modified file 'charm/repo_test.go'
--- charm/repo_test.go 2013-06-11 23:32:05 +0000
+++ charm/repo_test.go 2013-06-15 14:46:57 +0000
@@ -531,3 +531,13 @@
   s.checkNotFoundErr(c, err, charmURL)
   c.Assert(c.GetTestLog(), Equals, "")
  }
+
+func (s *LocalRepoSuite) TestFindsSymlinks(c *C) {
+ realPath := testing.Charms.ClonedDirPath(c.MkDir(), "dummy")
+ linkPath := filepath.Join(s.seriesPath, "dummy")
+ err := os.Symlink(realPath, linkPath)
+ c.Assert(err, IsNil)
+ ch, err := s.repo.Get(charm.MustParseURL("local:series/dummy"))
+ c.Assert(err, IsNil)
+ checkDummy(c, ch, linkPath)
+}

[Frank Mueller (themue)]

LGTM, including Daves comments.

https://codereview.appspot.com/10302043/

[William Reade (fwereade)]

Please take a look.

https://codereview.appspot.com/10302043/diff/1/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/10302043/diff/1/charm/repo.go#newcode299
charm/repo.go:299: repoName := info.Name()
On 2013/06/17 07:18:30, dfc wrote:
> move below the if block

The if block might change what info points to; what I care about here is
the name inside the series dir. I'm not going to get hung up on the name
of what's pointed to, just on whether what it is matches what we expect
from looking at the link name.

https://codereview.appspot.com/10302043/diff/1/charm/repo.go#newcode302
charm/repo.go:302: if chPath, err = os.Readlink(chPath); err != nil {
On 2013/06/17 07:18:30, dfc wrote:
> if chPath, err := ...

I need chPath just below.

https://codereview.appspot.com/10302043/diff/1/charm/repo.go#newcode305
charm/repo.go:305: if info, err = os.Stat(chPath); err != nil {
On 2013/06/17 07:18:30, dfc wrote:
> same

I need info just below.

...but I can do all this simpler. Bah.

https://codereview.appspot.com/10302043/

[Frank Mueller (themue)]

Nice, LGTM.

https://codereview.appspot.com/10302043/

=== modified file 'charm/charm_test.go'
--- charm/charm_test.go	2013-05-02 15:55:42 +0000
+++ charm/charm_test.go	2013-06-19 12:33:45 +0000
@@ -71,7 +71,6 @@
 	switch f := f.(type) {
 	case *charm.Bundle:
 		c.Assert(f.Path, Equals, path)
-
 	case *charm.Dir:
 		c.Assert(f.Path, Equals, path)
 	}

=== modified file 'charm/repo.go'
--- charm/repo.go	2013-06-10 20:50:42 +0000
+++ charm/repo.go	2013-06-19 12:33:45 +0000
@@ -326,10 +326,16 @@
 	}
 	var latest Charm
 	for _, info := range infos {
+		chPath := filepath.Join(path, info.Name())
+		if info.Mode()&os.ModeSymlink != 0 {
+			var err error
+			if info, err = os.Stat(chPath); err != nil {
+				return nil, err
+			}
+		}
 		if !mightBeCharm(info) {
 			continue
 		}
-		chPath := filepath.Join(path, info.Name())
 		if ch, err := Read(chPath); err != nil {
 			log.Warningf("charm: failed to load charm at %q: %s", chPath, err)
 		} else if ch.Meta().Name == curl.Name {

=== modified file 'charm/repo_test.go'
--- charm/repo_test.go	2013-06-11 23:32:05 +0000
+++ charm/repo_test.go	2013-06-19 12:33:45 +0000
@@ -531,3 +531,13 @@
 	s.checkNotFoundErr(c, err, charmURL)
 	c.Assert(c.GetTestLog(), Equals, "")
 }
+
+func (s *LocalRepoSuite) TestFindsSymlinks(c *C) {
+	realPath := testing.Charms.ClonedDirPath(c.MkDir(), "dummy")
+	linkPath := filepath.Join(s.seriesPath, "dummy")
+	err := os.Symlink(realPath, linkPath)
+	c.Assert(err, IsNil)
+	ch, err := s.repo.Get(charm.MustParseURL("local:series/dummy"))
+	c.Assert(err, IsNil)
+	checkDummy(c, ch, linkPath)
+}