Fix misguided semantics in local downloader (#459)

Background: when installing a template from a local directory, we call
it "canonical" if the template source directory and the destination
directory are in the same git repo. When it's canonical, that means that
relative path between the template source directory and the destinaton
directory will be the same for other users that check out the same git
repo. This means that future upgrades can be seamless, because we know
where to get the latest template version from (e.g.
"../../my_template").

When I implemented the code in LocalDownloader to detect whether a
template installation was "canonical" or not, I did it wrong. I
conflated the template directory and the destination directory. There
are actually *three* directories of interest in the LocalDownloader, not
just two:

1. the source (where the template files are stored that we want to
install)
2. the template directory (a temporary copy of the contents of the
source)
3. the destination directory (where the user is rendering the template
to).

The correct way to check canonical-ness is to compare (1) and (3). I was
comparing (1) and (2) before.

This never affected users because all parts of the upgrade logic,
including the canonical-ness calculation, are behind a hidden flag that
defaults to false.

This unblocks more work on the template upgrade feature and was
discovered while implementing that.

I filed #458 to reconsider whether we even need a template temp dir
(directory number 2 above) in the case where we're installing from a
local directory.

templates/commands/describe/describe.go

@@ -109,7 +109,7 @@ func (c *Command) realRun(ctx context.Context, rp *runParams) (rErr error) {
 		return err //nolint:wrapcheck
 	}
 
-	if _, err = downloader.Download(ctx, cwd, templateDir); err != nil {
+	if _, err = downloader.Download(ctx, cwd, templateDir, ""); err != nil {
 		return fmt.Errorf("failed to download/copy template: %w", err)
 	}
 

templates/common/fs.go

@@ -403,3 +403,12 @@ func IsStatNotExistErr(err error) bool {
 	return errors.Is(err, fs.ErrNotExist) ||
 		errors.Is(err, fs.ErrInvalid)
 }
+
+// JoinIfRelative returns path if it's an absolute path, otherwise
+// filepath.Join(cwd, path).
+func JoinIfRelative(cwd, path string) string {
+	if filepath.IsAbs(path) {
+		return path
+	}
+	return filepath.Join(cwd, path)
+}

templates/common/render/render.go

@@ -141,7 +141,7 @@ func Render(ctx context.Context, p *Params) (rErr error) {
 		"path", templateDir)
 
 	logger.DebugContext(ctx, "downloading/copying template")
-	dlMeta, err := p.Downloader.Download(ctx, p.Cwd, templateDir)
+	dlMeta, err := p.Downloader.Download(ctx, p.Cwd, templateDir, p.DestDir)
 	if err != nil {
 		return fmt.Errorf("failed to download/copy template: %w", err)
 	}

templates/common/templatesource/download.go

@@ -23,8 +23,10 @@ import (
 // A Downloader is returned by a sourceParser. It offers the ability to
 // download a template, and provides some metadata.
 type Downloader interface {
-	// Download downloads this template into the given directory.
-	Download(ctx context.Context, cwd, destDir string) (*DownloadMetadata, error)
+	// Download downloads this template into templateDir. templateDir should be
+	// a temporary directory. destDir is only used to determine if the template
+	// location is canonical, and is not written to.
+	Download(ctx context.Context, cwd, templateDir, destDir string) (*DownloadMetadata, error)
 }
 
 type DownloadMetadata struct {

templates/common/templatesource/localsource.go

@@ -83,20 +83,22 @@ type LocalDownloader struct {
 	allowDirty bool
 }
 
-func (l *LocalDownloader) Download(ctx context.Context, cwd, destDir string) (*DownloadMetadata, error) {
+// installedDir is only used to check for canonical-ness.
+func (l *LocalDownloader) Download(ctx context.Context, cwd, templateDir, destDir string) (*DownloadMetadata, error) {
 	logger := logging.FromContext(ctx).With("logger", "localTemplateSource.Download")
 
+	templateDir = common.JoinIfRelative(cwd, templateDir)
+
 	logger.DebugContext(ctx, "copying local template source",
 		"srcPath", l.SrcPath,
-		"destDir", destDir)
+		"templateDir", templateDir)
 	if err := common.CopyRecursive(ctx, nil, &common.CopyParams{
 		SrcRoot: l.SrcPath,
-		DstRoot: destDir,
+		DstRoot: templateDir,
 		FS:      &common.RealFS{},
 	}); err != nil {
 		return nil, err //nolint:wrapcheck
 	}
-
 	gitVars, err := gitTemplateVars(ctx, l.SrcPath)
 	if err != nil {
 		return nil, err
@@ -105,16 +107,13 @@ func (l *LocalDownloader) Download(ctx context.Context, cwd, destDir string) (*D
 	if err != nil {
 		return nil, err
 	}
-
 	dlMeta := &DownloadMetadata{
 		IsCanonical:     canonicalSource != "",
 		CanonicalSource: canonicalSource,
 		LocationType:    locType,
-
-		HasVersion: version != "",
-		Version:    version,
-
-		Vars: *gitVars,
+		HasVersion:      version != "",
+		Version:         version,
+		Vars:            *gitVars,
 	}
 	return dlMeta, nil
 }
@@ -123,46 +122,43 @@ func (l *LocalDownloader) Download(ctx context.Context, cwd, destDir string) (*D
 // directories qualify as a canonical source, and if so, returns the
 // canonicalized version of the source. See the docs on DownloadMetadata for an
 // explanation of canonical sources.
-func canonicalize(ctx context.Context, cwd, src, dest string, allowDirty bool) (canonicalSource, version, locType string, _ error) {
+func canonicalize(ctx context.Context, cwd, source, destDirUltimate string, allowDirty bool) (canonicalSource, version, locType string, _ error) {
 	logger := logging.FromContext(ctx).With("logger", "canonicalize")
 
-	absDest := dest
-	if !filepath.IsAbs(dest) {
-		absDest = filepath.Join(cwd, dest)
-	}
+	absSource := common.JoinIfRelative(cwd, source)
+	absDestDir := common.JoinIfRelative(cwd, destDirUltimate)
 
 	// See the docs on DownloadMetadata for an explanation of why we compare the git
 	// workspaces to decide if source is canonical.
-	sourceGitWorkspace, templateIsGit, err := git.Workspace(ctx, src)
+	sourceGitWorkspace, sourceIsGit, err := git.Workspace(ctx, absSource)
 	if err != nil {
 		return "", "", "", err //nolint:wrapcheck
 	}
-	destGitWorkspace, destIsGit, err := git.Workspace(ctx, absDest)
+	destGitWorkspace, destIsGit, err := git.Workspace(ctx, absDestDir)
 	if err != nil {
 		return "", "", "", err //nolint:wrapcheck
 	}
-	if !templateIsGit || !destIsGit || sourceGitWorkspace != destGitWorkspace {
+	if !sourceIsGit || !destIsGit || sourceGitWorkspace != destGitWorkspace {
 		logger.DebugContext(ctx, "local template source is not canonical, template dir and dest dir do not share a git workspace",
-			"source", src,
-			"dest", absDest,
+			"source_dir", absSource,
+			"dest_dir", absDestDir,
 			"source_git_workspace", sourceGitWorkspace,
 			"dest_git_workspace", destGitWorkspace)
 		return "", "", "", nil
 	}
 
 	logger.DebugContext(ctx, "local template source is canonical because template dir and dest dir are both in the same git workspace",
-		"source", src,
-		"dest", absDest,
+		"source_dir", absSource,
+		"dest", absDestDir,
 		"git_workspace", destGitWorkspace)
-	out, err := filepath.Rel(absDest, src)
+	out, err := filepath.Rel(absDestDir, absSource)
 	if err != nil {
-		return "", "", "", fmt.Errorf("filepath.Rel(%q,%q): %w", dest, src, err)
+		return "", "", "", fmt.Errorf("filepath.Rel(%q,%q): %w", absDestDir, absSource, err)
 	}
 
 	version, _, err = gitCanonicalVersion(ctx, sourceGitWorkspace, allowDirty)
 	if err != nil {
 		return "", "", "", err
 	}
-
 	return filepath.ToSlash(out), version, LocTypeLocalGit, nil
 }