Reviews.

internal/charmstore/archive.go

@@ -115,6 +115,9 @@ func (r *multiReadSeekCloser) Close() error {
 // If no such file was found, it returns an error
 // with a params.ErrNotFound cause.
 //
+// If the file is a symlink, it returns an error ErrRedirect that holds the path
+// that the symbolic link refers to.
+//
 // If the file is actually a directory in the blob, it returns
 // an error with a params.ErrForbidden cause.
 func (s *Store) OpenBlobFile(blob *Blob, filePath string) (io.ReadCloser, int64, error) {
@@ -143,19 +146,22 @@ func (s *Store) OpenBlobFile(blob *Blob, filePath string) (io.ReadCloser, int64,
 			if err != nil {
 				return nil, 0, errgo.Notef(err, "cannot read archive data for symlink %s", file.Name)
 			}
-			return nil, 0, ErrRedirect{URL: string(url)}
+			return nil, 0, &ErrRedirect{Path: string(url)}
 		}
 		return content, fileInfo.Size(), nil
 	}
 	return nil, 0, errgo.WithCausef(nil, params.ErrNotFound, "file %q not found in the archive", filePath)
 }
 
+// ErrRedirect holds the error returned by OpenBlobFile when a symbolic link
+// is opened. The Path field holds the name of the file that the symbolic link
+// refers to.
 type ErrRedirect struct {
-	URL string
+	Path string
 }
 
 func (e ErrRedirect) Error() string {
-	return fmt.Sprintf("redirect to %v", e.URL)
+	return fmt.Sprintf("file is a symbolic link to %q", e.Path)
 }
 
 // OpenCachedBlobFile opens a file from the given entity's archive blob.

internal/v5/archive.go

@@ -10,7 +10,6 @@ import (
 	"io/ioutil"
 	"mime"
 	"net/http"
-	"path"
 	"path/filepath"
 	"strconv"
 	"time"
@@ -24,6 +23,7 @@ import (
 	"gopkg.in/juju/charmstore.v5/internal/charmstore"
 	"gopkg.in/juju/charmstore.v5/internal/mongodoc"
 	"gopkg.in/juju/charmstore.v5/internal/router"
+	"path"
 )
 
 // GET id/archive
@@ -334,18 +334,18 @@ func (h *ReqHandler) serveArchiveFile(id *router.ResolvedURL, w http.ResponseWri
 // with the given id.
 func (h *ReqHandler) ServeBlobFile(w http.ResponseWriter, req *http.Request, id *router.ResolvedURL, blob *charmstore.Blob) error {
 	r, size, err := h.Store.OpenBlobFile(blob, req.URL.Path)
-	if err != nil {
-		if redirect, ok := err.(charmstore.ErrRedirect); ok {
-			p, err := router.RelativeURLPath(req.URL.Path, path.Join(path.Dir(req.URL.Path), redirect.URL))
-			if err != nil {
-				return errgo.Notef(err, "cannot make relative URL from %q and %q", req.URL.Path, path.Join(path.Dir(req.URL.Path), redirect.URL))
-			}
-			// We can't use the http.redirect function because it tries to build an absolute url but the path in the
-			// request URL has been changed.
-			w.Header().Set("Location", p)
-			w.WriteHeader(http.StatusMovedPermanently)
-			return nil
+	if redirect, ok := err.(*charmstore.ErrRedirect); ok {
+		p, err := router.RelativeURLPath(req.URL.Path, path.Join(path.Dir(req.URL.Path), redirect.Path))
+		if err != nil {
+			return errgo.Notef(err, "cannot make relative URL from %q and %q", req.URL.Path, path.Join(path.Dir(req.URL.Path), redirect.Path))
 		}
+		// We can't use the http.redirect function because it tries to build an absolute url but the path in the
+		// request URL has been changed.
+		w.Header().Set("Location", p)
+		w.WriteHeader(http.StatusMovedPermanently)
+		return nil
+	}
+	if err != nil {
 		return errgo.Mask(err, errgo.Is(params.ErrNotFound), errgo.Is(params.ErrForbidden))
 	}
 	defer r.Close()