Fix multilingual reload when shortcode changes

This commit also refines the partial rebuild logic, to make sure we do not do more work than needed.

Updates #2309

6 files changed, 135 insertions, 74 deletions

diff --git a/hugolib/handler_page.go b/hugolib/handler_page.go
index fcc5b9561..c1f83d716 100644
--- a/hugolib/handler_page.go
+++ b/hugolib/handler_page.go
@@ -15,6 +15,7 @@ package hugolib
 
 import (
 	"bytes"
+	"fmt"
 
 	"github.com/spf13/hugo/helpers"
 	"github.com/spf13/hugo/source"
@@ -67,6 +68,10 @@ type htmlHandler struct {
 
 func (h htmlHandler) Extensions() []string { return []string{"html", "htm"} }
 func (h htmlHandler) PageConvert(p *Page, t tpl.Template) HandledResult {
+	if p.rendered {
+		panic(fmt.Sprintf("Page %q already rendered, does not need conversion", p.BaseFileName()))
+	}
+
 	p.ProcessShortcodes(t)
 
 	return HandledResult{err: nil}
@@ -100,6 +105,11 @@ func (h mmarkHandler) PageConvert(p *Page, t tpl.Template) HandledResult {
 }
 
 func commonConvert(p *Page, t tpl.Template) HandledResult {
+
+	if p.rendered {
+		panic(fmt.Sprintf("Page %q already rendered, does not need conversion", p.BaseFileName()))
+	}
+
 	p.ProcessShortcodes(t)
 
 	// TODO(bep) these page handlers need to be re-evaluated, as it is hard to
diff --git a/hugolib/hugo_sites.go b/hugolib/hugo_sites.go
index 8fe27e506..36b797e7e 100644
--- a/hugolib/hugo_sites.go
+++ b/hugolib/hugo_sites.go
@@ -220,7 +220,7 @@ func (h *HugoSites) Build(config BuildCfg) error {
 		}
 	}
 
-	if err := h.preRender(); err != nil {
+	if err := h.preRender(config, whatChanged{source: true, other: true}); err != nil {
 		return err
 	}
 
@@ -261,6 +261,10 @@ func (h *HugoSites) Rebuild(config BuildCfg, events ...fsnotify.Event) error {
 		return errors.New("Rebuild does not support 'ResetState'. Use Build.")
 	}
 
+	if !config.Watching {
+		return errors.New("Rebuild called when not in watch mode")
+	}
+
 	h.runMode.Watching = config.Watching
 
 	firstSite := h.Sites[0]
@@ -270,7 +274,7 @@ func (h *HugoSites) Rebuild(config BuildCfg, events ...fsnotify.Event) error {
 		s.resetBuildState()
 	}
 
-	sourceChanged, err := firstSite.reBuild(events)
+	changed, err := firstSite.reBuild(events)
 
 	if err != nil {
 		return err
@@ -279,7 +283,7 @@ func (h *HugoSites) Rebuild(config BuildCfg, events ...fsnotify.Event) error {
 	// Assign pages to sites per translation.
 	h.setupTranslations(firstSite)
 
-	if sourceChanged {
+	if changed.source {
 		for _, s := range h.Sites {
 			if err := s.postProcess(); err != nil {
 				return err
@@ -287,7 +291,7 @@ func (h *HugoSites) Rebuild(config BuildCfg, events ...fsnotify.Event) error {
 		}
 	}
 
-	if err := h.preRender(); err != nil {
+	if err := h.preRender(config, changed); err != nil {
 		return err
 	}
 
@@ -391,7 +395,7 @@ func (h *HugoSites) setupTranslations(master *Site) {
 // preRender performs build tasks that need to be done as late as possible.
 // Shortcode handling is the main task in here.
 // TODO(bep) We need to look at the whole handler-chain construct with he below in mind.
-func (h *HugoSites) preRender() error {
+func (h *HugoSites) preRender(cfg BuildCfg, changed whatChanged) error {
 
 	for _, s := range h.Sites {
 		if err := s.setCurrentLanguageConfig(); err != nil {
@@ -416,13 +420,13 @@ func (h *HugoSites) preRender() error {
 		if err := s.setCurrentLanguageConfig(); err != nil {
 			return err
 		}
-		renderShortcodesForSite(s)
+		s.preparePagesForRender(cfg, changed)
 	}
 
 	return nil
 }
 
-func renderShortcodesForSite(s *Site) {
+func (s *Site) preparePagesForRender(cfg BuildCfg, changed whatChanged) {
 	pageChan := make(chan *Page)
 	wg := &sync.WaitGroup{}
 
@@ -431,14 +435,37 @@ func renderShortcodesForSite(s *Site) {
 		go func(pages <-chan *Page, wg *sync.WaitGroup) {
 			defer wg.Done()
 			for p := range pages {
+
+				if !changed.other && p.rendered {
+					// No need to process it again.
+					continue
+				}
+
+				// If we got this far it means that this is either a new Page pointer
+				// or a template or similar has changed so wee need to do a rerendering
+				// of the shortcodes etc.
+
+				// Mark it as rendered
+				p.rendered = true
+
+				// If in watch mode, we need to keep the original so we can
+				// repeat this process on rebuild.
+				if cfg.Watching {
+					p.rawContentCopy = make([]byte, len(p.rawContent))
+					copy(p.rawContentCopy, p.rawContent)
+				} else {
+					// Just reuse the same slice.
+					p.rawContentCopy = p.rawContent
+				}
+
 				if err := handleShortcodes(p, s.owner.tmpl); err != nil {
 					jww.ERROR.Printf("Failed to handle shortcodes for page %s: %s", p.BaseFileName(), err)
 				}
 
 				if p.Markup == "markdown" {
-					tmpContent, tmpTableOfContents := helpers.ExtractTOC(p.rawContent)
+					tmpContent, tmpTableOfContents := helpers.ExtractTOC(p.rawContentCopy)
 					p.TableOfContents = helpers.BytesToHTML(tmpTableOfContents)
-					p.rawContent = tmpContent
+					p.rawContentCopy = tmpContent
 				}
 
 				if p.Markup != "html" {
@@ -449,19 +476,25 @@ func renderShortcodesForSite(s *Site) {
 					if err != nil {
 						jww.ERROR.Printf("Failed to set use defined summary: %s", err)
 					} else if summaryContent != nil {
-						p.rawContent = summaryContent.content
+						p.rawContentCopy = summaryContent.content
 					}
 
-					p.Content = helpers.BytesToHTML(p.rawContent)
-					p.rendered = true
+					p.Content = helpers.BytesToHTML(p.rawContentCopy)
 
 					if summaryContent == nil {
 						p.setAutoSummary()
 					}
+
+				} else {
+					p.Content = helpers.BytesToHTML(p.rawContentCopy)
 				}
 
+				// no need for this anymore
+				p.rawContentCopy = nil
+
 				//analyze for raw stats
 				p.analyzePage()
+
 			}
 		}(pageChan, wg)
 	}
@@ -490,7 +523,7 @@ func handleShortcodes(p *Page, t tpl.Template) error {
 			return err
 		}
 
-		p.rawContent, err = replaceShortcodeTokens(p.rawContent, shortcodePlaceholderPrefix, shortcodes)
+		p.rawContentCopy, err = replaceShortcodeTokens(p.rawContentCopy, shortcodePlaceholderPrefix, shortcodes)
 
 		if err != nil {
 			jww.FATAL.Printf("Failed to replace short code tokens in %s:\n%s", p.BaseFileName(), err.Error())
diff --git a/hugolib/hugo_sites_test.go b/hugolib/hugo_sites_test.go
index bfbcd914e..b19e9ea2e 100644
--- a/hugolib/hugo_sites_test.go
+++ b/hugolib/hugo_sites_test.go
@@ -51,7 +51,7 @@ func doTestMultiSitesMainLangInRoot(t *testing.T, defaultInSubDir bool) {
 	testCommonResetState()
 	viper.Set("DefaultContentLanguageInSubdir", defaultInSubDir)
 
-	sites := createMultiTestSites(t, multiSiteTomlConfig)
+	sites := createMultiTestSites(t, multiSiteTOMLConfig)
 
 	err := sites.Build(BuildCfg{})
 
@@ -166,7 +166,7 @@ func TestMultiSitesBuild(t *testing.T) {
 		content string
 		suffix  string
 	}{
-		{multiSiteTomlConfig, "toml"},
+		{multiSiteTOMLConfig, "toml"},
 		{multiSiteYAMLConfig, "yml"},
 		{multiSiteJSONConfig, "json"},
 	} {
@@ -323,8 +323,8 @@ func doTestMultiSitesBuild(t *testing.T, configContent, configSuffix string) {
 
 func TestMultiSitesRebuild(t *testing.T) {
 	testCommonResetState()
-	sites := createMultiTestSites(t, multiSiteTomlConfig)
-	cfg := BuildCfg{}
+	sites := createMultiTestSites(t, multiSiteTOMLConfig)
+	cfg := BuildCfg{Watching: true}
 
 	err := sites.Build(cfg)
 
@@ -350,6 +350,10 @@ func TestMultiSitesRebuild(t *testing.T) {
 	docFr := readDestination(t, "public/fr/sect/doc1/index.html")
 	assert.True(t, strings.Contains(docFr, "Bonjour"), "No Bonjour")
 
+	// check single page content
+	assertFileContent(t, "public/fr/sect/doc1/index.html", true, "Single", "Shortcode: Bonjour")
+	assertFileContent(t, "public/en/sect/doc1-slug/index.html", true, "Single", "Shortcode: Hello")
+
 	for i, this := range []struct {
 		preFunc    func(t *testing.T)
 		events     []fsnotify.Event
@@ -474,6 +478,22 @@ func TestMultiSitesRebuild(t *testing.T) {
 
 			},
 		},
+		// Change a shortcode
+		{
+			func(t *testing.T) {
+				writeSource(t, "layouts/shortcodes/shortcode.html", "Modified Shortcode: {{ i18n \"hello\" }}")
+			},
+			[]fsnotify.Event{
+				{Name: "layouts/shortcodes/shortcode.html", Op: fsnotify.Write},
+			},
+			func(t *testing.T) {
+				assert.Len(t, enSite.Pages, 4)
+				assert.Len(t, enSite.AllPages, 10)
+				assert.Len(t, frSite.Pages, 4)
+				assertFileContent(t, "public/fr/sect/doc1/index.html", true, "Single", "Modified Shortcode: Salut")
+				assertFileContent(t, "public/en/sect/doc1-slug/index.html", true, "Single", "Modified Shortcode: Hello")
+			},
+		},
 	} {
 
 		if this.preFunc != nil {
@@ -516,7 +536,7 @@ func assertShouldNotBuild(t *testing.T, sites *HugoSites) {
 func TestAddNewLanguage(t *testing.T) {
 	testCommonResetState()
 
-	sites := createMultiTestSites(t, multiSiteTomlConfig)
+	sites := createMultiTestSites(t, multiSiteTOMLConfig)
 	cfg := BuildCfg{}
 
 	err := sites.Build(cfg)
@@ -525,7 +545,7 @@ func TestAddNewLanguage(t *testing.T) {
 		t.Fatalf("Failed to build sites: %s", err)
 	}
 
-	newConfig := multiSiteTomlConfig + `
+	newConfig := multiSiteTOMLConfig + `
 
 [Languages.sv]
 weight = 15
@@ -573,7 +593,7 @@ title = "Svenska"
 
 }
 
-var multiSiteTomlConfig = `
+var multiSiteTOMLConfig = `
 DefaultExtension = "html"
 baseurl = "http://example.com/blog"
 DisableSitemap = false
diff --git a/hugolib/page.go b/hugolib/page.go
index b09e2b1f7..7892f222d 100644
--- a/hugolib/page.go
+++ b/hugolib/page.go
@@ -48,28 +48,42 @@ var (
 )
 
 type Page struct {
-	Params              map[string]interface{}
-	Content             template.HTML
-	Summary             template.HTML
-	Aliases             []string
-	Status              string
-	Images              []Image
-	Videos              []Video
-	TableOfContents     template.HTML
-	Truncated           bool
-	Draft               bool
-	PublishDate         time.Time
-	ExpiryDate          time.Time
-	Markup              string
-	translations        Pages
-	extension           string
-	contentType         string
-	renderable          bool
-	Layout              string
-	layoutsCalculated   []string
-	linkTitle           string
-	frontmatter         []byte
-	rawContent          []byte
+	Params            map[string]interface{}
+	Content           template.HTML
+	Summary           template.HTML
+	Aliases           []string
+	Status            string
+	Images            []Image
+	Videos            []Video
+	TableOfContents   template.HTML
+	Truncated         bool
+	Draft             bool
+	PublishDate       time.Time
+	ExpiryDate        time.Time
+	Markup            string
+	translations      Pages
+	extension         string
+	contentType       string
+	renderable        bool
+	Layout            string
+	layoutsCalculated []string
+	linkTitle         string
+	frontmatter       []byte
+
+	// rawContent isn't "raw" as in the same as in the content file.
+	// Hugo cares about memory consumption, so we make changes to it to do
+	// markdown rendering etc., but it is "raw enough" so we can do rebuilds
+	// when shortcode changes etc.
+	rawContent []byte
+
+	// When running Hugo in watch mode, we do partial rebuilds and have to make
+	// a copy of the rawContent to be prepared for rebuilds when shortcodes etc.
+	// have changed.
+	rawContentCopy []byte
+
+	// state telling if this is a "new page" or if we have rendered it previously.
+	rendered bool
+
 	contentShortCodes   map[string]func() (string, error)
 	shortcodes          map[string]shortcode
 	plain               string // TODO should be []byte
@@ -84,7 +98,6 @@ type Page struct {
 	Source
 	Position `json:"-"`
 	Node
-	rendered bool
 }
 
 type Source struct {
@@ -220,7 +233,7 @@ var (
 // Returns the page as summary and main if a user defined split is provided.
 func (p *Page) setUserDefinedSummaryIfProvided() (*summaryContent, error) {
 
-	sc := splitUserDefinedSummaryAndContent(p.Markup, p.rawContent)
+	sc := splitUserDefinedSummaryAndContent(p.Markup, p.rawContentCopy)
 
 	if sc == nil {
 		// No divider found
@@ -1024,19 +1037,9 @@ func (p *Page) SaveSource() error {
 }
 
 func (p *Page) ProcessShortcodes(t tpl.Template) {
-
-	// these short codes aren't used until after Page render,
-	// but processed here to avoid coupling
-	// TODO(bep) Move this and remove p.contentShortCodes
-	if !p.rendered {
-		tmpContent, tmpContentShortCodes, _ := extractAndRenderShortcodes(string(p.rawContent), p, t)
-		p.rawContent = []byte(tmpContent)
-		p.contentShortCodes = tmpContentShortCodes
-	} else {
-		// shortcode template may have changed, rerender
-		p.contentShortCodes = renderShortcodes(p.shortcodes, p, t)
-	}
-
+	tmpContent, tmpContentShortCodes, _ := extractAndRenderShortcodes(string(p.rawContent), p, t)
+	p.rawContent = []byte(tmpContent)
+	p.contentShortCodes = tmpContentShortCodes
 }
 
 func (p *Page) FullFilePath() string {
diff --git a/hugolib/shortcode.go b/hugolib/shortcode.go
index 52cd6893b..854f89899 100644
--- a/hugolib/shortcode.go
+++ b/hugolib/shortcode.go
@@ -281,10 +281,6 @@ func renderShortcode(sc shortcode, parent *ShortcodeWithPage, p *Page, t tpl.Tem
 
 func extractAndRenderShortcodes(stringToParse string, p *Page, t tpl.Template) (string, map[string]func() (string, error), error) {
 
-	if p.rendered {
-		panic("Illegal state: Page already marked as rendered, please reuse the shortcodes")
-	}
-
 	content, shortcodes, err := extractShortcodes(stringToParse, p, t)
 
 	if err != nil {
diff --git a/hugolib/site.go b/hugolib/site.go
index 9f435218c..22b22773d 100644
--- a/hugolib/site.go
+++ b/hugolib/site.go
@@ -450,9 +450,14 @@ func (s *Site) timerStep(step string) {
 	s.timer.Step(step)
 }
 
+type whatChanged struct {
+	source bool
+	other  bool
+}
+
 // reBuild partially rebuilds a site given the filesystem events.
 // It returns whetever the content source was changed.
-func (s *Site) reBuild(events []fsnotify.Event) (bool, error) {
+func (s *Site) reBuild(events []fsnotify.Event) (whatChanged, error) {
 
 	jww.DEBUG.Printf("Rebuild for events %q", events)
 
@@ -500,7 +505,6 @@ func (s *Site) reBuild(events []fsnotify.Event) (bool, error) {
 	}
 
 	if len(i18nChanged) > 0 {
-		// TODO(bep ml
 		s.readI18nSources()
 	}
 
@@ -564,16 +568,6 @@ func (s *Site) reBuild(events []fsnotify.Event) (bool, error) {
 	go incrementalReadCollator(s, readResults, pageChan, fileConvChan, coordinator, errs)
 	go converterCollator(s, convertResults, errs)
 
-	if len(tmplChanged) > 0 || len(dataChanged) > 0 {
-		// Do not need to read the files again, but they need conversion
-		// for shortocde re-rendering.
-		for _, p := range s.rawAllPages {
-			if p.shouldBuild() {
-				pageChan <- p
-			}
-		}
-	}
-
 	for _, ev := range sourceReallyChanged {
 
 		file, err := s.reReadFile(ev.Name)
@@ -610,7 +604,12 @@ func (s *Site) reBuild(events []fsnotify.Event) (bool, error) {
 
 	s.timerStep("read & convert pages from source")
 
-	return len(sourceChanged) > 0, nil
+	changed := whatChanged{
+		source: len(sourceChanged) > 0,
+		other:  len(tmplChanged) > 0 || len(i18nChanged) > 0 || len(dataChanged) > 0,
+	}
+
+	return changed, nil
 
 }