twpayne / chezmoi

Manage your dotfiles across multiple diverse machines, securely.
https://www.chezmoi.io/
MIT License
13.4k stars 493 forks source link

"create" prefix causes a template to not be processed #1495

Closed Peterdes closed 3 years ago

Peterdes commented 3 years ago

Problem: no file starting with create_ is being processed as a template. Example: create a file named "create_dumb.tmpl" with contents

{{ .chezmoi.os }}

then run chezmoi apply -nv. The template is not processed.

It is because newCreateTargetStateEntryFunc in internal/chezmoi/sourcestate.go does not involve any template processing.

The behavior is fixed by the following change:

diff --git a/internal/chezmoi/sourcestate.go b/internal/chezmoi/sourcestate.go
index 15fc1e64..c78184ea 100644
--- a/internal/chezmoi/sourcestate.go
+++ b/internal/chezmoi/sourcestate.go
@@ -1184,7 +1184,7 @@ func (s *SourceState) newSourceStateDir(sourceRelPath SourceRelPath, dirAttr Dir
 // newCreateTargetStateEntryFunc returns a targetStateEntryFunc that returns a
 // file with sourceLazyContents if the file does not already exist, or returns
 // the actual file's contents unchanged if the file already exists.
-func (s *SourceState) newCreateTargetStateEntryFunc(fileAttr FileAttr, sourceLazyContents *lazyContents) targetStateEntryFunc {
+func (s *SourceState) newCreateTargetStateEntryFunc(sourceRelPath SourceRelPath, fileAttr FileAttr, sourceLazyContents *lazyContents) targetStateEntryFunc {
    return func(destSystem System, destAbsPath AbsPath) (TargetStateEntry, error) {
        contents, err := destSystem.ReadFile(destAbsPath)
        switch {
@@ -1194,6 +1194,12 @@ func (s *SourceState) newCreateTargetStateEntryFunc(fileAttr FileAttr, sourceLaz
            if err != nil {
                return nil, err
            }
+           if fileAttr.Template {
+               contents, err = s.ExecuteTemplateData(sourceRelPath.String(), contents)
+               if err != nil {
+                   return nil, err
+               }
+           }
        default:
            return nil, err
        }
@@ -1386,7 +1392,7 @@ func (s *SourceState) newSourceStateFile(sourceRelPath SourceRelPath, fileAttr F
    var targetStateEntryFunc targetStateEntryFunc
    switch fileAttr.Type {
    case SourceFileTypeCreate:
-       targetStateEntryFunc = s.newCreateTargetStateEntryFunc(fileAttr, sourceLazyContents)
+       targetStateEntryFunc = s.newCreateTargetStateEntryFunc(sourceRelPath, fileAttr, sourceLazyContents)
    case SourceFileTypeFile:
        targetStateEntryFunc = s.newFileTargetStateEntryFunc(sourceRelPath, fileAttr, sourceLazyContents)
    case SourceFileTypeModify:

but this doesn't pass some tests which is not cery surprising, but I don't understand what these tests are meant to check.

--- FAIL: TestManagedCmd (0.01s)
    --- FAIL: TestManagedCmd/create_template (0.00s)
        managedcmd_test.go:82: 
                Error Trace:    managedcmd_test.go:82
                                            chezmoitest.go:130
                                            managedcmd_test.go:80
                Error:          Received unexpected error:
                                template: create_dot_file.tmpl:1:3: executing "create_dot_file.tmpl" at <fail "Template should not be executed">: error calling fail: Template should not be executed
                Test:           TestManagedCmd/create_template
--- FAIL: TestScript (0.00s)
    --- FAIL: TestScript/managed (1.22s)
        testscript.go:397: 
            # test chezmoi managed (0.240s)
            # test chezmoi managed --include=all (0.152s)
            # test chezmoi managed --include=dirs (0.135s)
            # test chezmoi managed --include=files (0.176s)
            # test chezmoi managed --include=symlinks (0.205s)
            # test chezmoi managed --exclude=files (0.150s)
            # test that chezmoi managed does not evaluate templates (0.156s)
            > chezmoi managed --include=all
            [stderr]
            chezmoi: template: create_dot_create.tmpl:1: unterminated quoted string
            [exit status 1]
            FAIL: testdata/scripts/managed.txt:30: unexpected command failure

Is this behavior intentional? If yes, why? If no, could you fix it?

twpayne commented 3 years ago

Thanks so much for this, this is definitely a bug/oversight in chezmoi.

but this doesn't pass some tests which is not cery surprising, but I don't understand what these tests are meant to check.

The tests check chezmoi's lazy execution of templates. chezmoi should only loads, parse, and execute templates if their contents are needed, for both performance and usability reasons. The usability aspect is that some templates might call password manager template functions, which might require user interaction to be evaluated, and you only want to trigger this user interaction if it is absolutely needed.

In the specific tests that fail here chezmoi uses invalid templates that trigger an error if they are executed as a trap to ensure that the templates are not even parsed. For example, the chezmoi managed command should only deal with filenames, and should never parse or execute any templates.

Your fix is really close to correct and revealed a subtle bug in chezmoi where templates were loaded (but not parsed) when they didn't need to be. #1496 completes the fix, but I would like to ensure that you get credit for your thorough investigation and work here. I couldn't find a recent email address in your GitHub profile. Could you please send me an email (address on my GitHub profile) so I can update the commits in #1496 to include credit to you?

Peterdes commented 3 years ago

Thanks for tackling that so quickly! I'm glad I could help.

Took a look at the fix, good job. I did consider copying the lazycontents stuff from newFileTargetStateEntryFunc but I didn't know what to do in other cases to stay in the lazy parsing scenario (lazyContents = newLazyContents(contents)). Anyway, I added my email to my profile, thanks for the credit :)

twpayne commented 3 years ago

Fixed with @Peterdes in #1496.