Code review comment for lp:~fwereade/juju-core/fix-1129319

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)
+}