diff --git a/src/ctemplate/template_cache.h.in b/src/ctemplate/template_cache.h.in index 99af467..8b331bd 100644 --- a/src/ctemplate/template_cache.h.in +++ b/src/ctemplate/template_cache.h.in @@ -243,8 +243,7 @@ class @ac_windows_dllexport@ TemplateCache { // If a file with the same name as an existing template-file, is added // in another search path, ReloadAllIfChanged will pick up the file in the - // earlier search-path. The file will only be reloaded if the orginal file - // is updated (touched, updated, deleted etc). See .cc file for more detail. + // earlier search-path. enum ReloadType { LAZY_RELOAD, IMMEDIATE_RELOAD }; void ReloadAllIfChanged(ReloadType reload_tyle); diff --git a/src/template_cache.cc b/src/template_cache.cc index 92a9c3d..5bcce3f 100644 --- a/src/template_cache.cc +++ b/src/template_cache.cc @@ -300,13 +300,17 @@ TemplateCache::RefcountedTemplate* TemplateCache::GetTemplateLocked( assert(it != parsed_template_cache_->end()); } if (it->second.should_reload) { - // check if the template has changed on disk: + // check if the template has changed on disk or if a new template with the + // same name has been added earlier in the search path: + const string resolved = FindTemplateFilename( + it->second.refcounted_tpl->tpl()->original_filename()); FileStat statbuf; if (it->second.template_type == CachedTemplate::FILE_BASED && - HasTemplateChangedOnDisk( - it->second.refcounted_tpl->tpl()->template_file(), - it->second.refcounted_tpl->tpl()->mtime(), - &statbuf)) { + (resolved != it->second.refcounted_tpl->tpl()->template_file() || + HasTemplateChangedOnDisk( + it->second.refcounted_tpl->tpl()->template_file(), + it->second.refcounted_tpl->tpl()->mtime(), + &statbuf))) { // Create a new template, insert it into the cache under // template_cache_key, and DecRef() the old one to indicate // the cache no longer has a reference to it. @@ -658,19 +662,12 @@ void TemplateCache::DoneWithGetTemplatePtrs() { // LAZY_RELOAD just sets the reload bit in the cache so that the next // GetTemplate will reload and parse the template, if it changed. -// NOTE: ReloadAllIfChanged can have surprising behavior if the -// same file exists in several places on the template search path. -// Suppose the search path is "dira:dirb", and a template is +// NOTE: Suppose the search path is "dira:dirb", and a template is // created with name "foo", which resolves to "dirb/foo" because -// dira/foo does not exist. Then suppose dira/foo is created and, -// dirb/foo is updated, and then ReloadAllIfChanged() is called. -// Then ReloadAllIfChanged() will replace the contents of the -// template with dira/foo, *not* dirb/foo. On the other hand, if -// dira/foo is added but dirb/foo is *not* updated, -// ReloadAllIfChanged() will be a noop, and the template contents -// will stay at dirb/foo. The possible lesson to draw from this is -// to not have the same template filename in different places on -// the search path. +// dira/foo does not exist. Then suppose dira/foo is created and then +// ReloadAllIfChanged() is called. Then ReloadAllIfChanged() will replace +// the contents of the template with dira/foo, *not* dirb/foo, even if +// dirb/foo hasn't changed. // ---------------------------------------------------------------------- void TemplateCache::ReloadAllIfChanged(ReloadType reload_type) { diff --git a/src/tests/template_cache_test.cc b/src/tests/template_cache_test.cc index d8b7790..5a23716 100644 --- a/src/tests/template_cache_test.cc +++ b/src/tests/template_cache_test.cc @@ -520,7 +520,8 @@ class TemplateCacheUnittest { StringToFile("b/template_foo", path_b_foo); ASSERT_STREQ(path_b_foo.c_str(), cache1.FindTemplateFilename("template_foo").c_str()); - const Template* b_foo = cache1.GetTemplate("template_foo", DO_NOT_STRIP); + // Add b/foo to the template cache. + cache1.GetTemplate("template_foo", DO_NOT_STRIP); // Add a/foo const string path_a_foo = PathJoin(pathA, "template_foo"); @@ -528,14 +529,6 @@ class TemplateCacheUnittest { ASSERT_STREQ(path_a_foo.c_str(), cache1.FindTemplateFilename("template_foo").c_str()); - // Since we didn't touch b/foo, on reload it doesn't get overridden by a/foo - cache1.ReloadAllIfChanged(TemplateCache::IMMEDIATE_RELOAD); - // Ensure that foo contains b/foo - ASSERT(b_foo = cache1.GetTemplate("template_foo", DO_NOT_STRIP)); - - // Now update (or touch) b/foo - StringToFile("{updated b/template_foo}", path_b_foo); - // Now, on reload we pick up foo from the earlier search path: a/foo cache1.ReloadAllIfChanged(TemplateCache::IMMEDIATE_RELOAD); const Template* foo_post_reload = cache_peer.GetTemplate("template_foo", @@ -548,7 +541,7 @@ class TemplateCacheUnittest { cache1.ReloadAllIfChanged(TemplateCache::IMMEDIATE_RELOAD); foo_post_reload = cache_peer.GetTemplate("template_foo", STRIP_WHITESPACE); - AssertExpandIs(foo_post_reload, &dict, "{updated b/template_foo}", + AssertExpandIs(foo_post_reload, &dict, "b/template_foo", true); } @@ -572,7 +565,8 @@ class TemplateCacheUnittest { StringToFile("b/template_foo", path_b_foo); ASSERT_STREQ(path_b_foo.c_str(), cache1.FindTemplateFilename("template_foo").c_str()); - const Template* b_foo = cache1.GetTemplate("template_foo", DO_NOT_STRIP); + // Add b/foo to the template cache. + cache1.GetTemplate("template_foo", DO_NOT_STRIP); // Add a/foo const string path_a_foo = PathJoin(pathA, "template_foo"); @@ -580,14 +574,6 @@ class TemplateCacheUnittest { ASSERT_STREQ(path_a_foo.c_str(), cache1.FindTemplateFilename("template_foo").c_str()); - // Since we didn't touch b/foo, on reload it doesn't get overridden by a/foo - cache1.ReloadAllIfChanged(TemplateCache::LAZY_RELOAD); - // Ensure that foo contains b/foo - ASSERT(b_foo = cache1.GetTemplate("template_foo", DO_NOT_STRIP)); - - // Now update (or touch) b/foo - StringToFile("{updated b/template_foo}", path_b_foo); - // Now, on reload we pick up foo from the earlier search path: a/foo cache1.ReloadAllIfChanged(TemplateCache::LAZY_RELOAD); const Template* foo_post_reload = cache_peer.GetTemplate("template_foo", @@ -600,7 +586,7 @@ class TemplateCacheUnittest { cache1.ReloadAllIfChanged(TemplateCache::LAZY_RELOAD); foo_post_reload = cache_peer.GetTemplate("template_foo", STRIP_WHITESPACE); - AssertExpandIs(foo_post_reload, &dict, "{updated b/template_foo}", + AssertExpandIs(foo_post_reload, &dict, "b/template_foo", true); } diff --git a/src/windows/ctemplate/template_cache.h b/src/windows/ctemplate/template_cache.h index 136a64e..2ee6b67 100644 --- a/src/windows/ctemplate/template_cache.h +++ b/src/windows/ctemplate/template_cache.h @@ -253,8 +253,7 @@ class CTEMPLATE_DLL_DECL TemplateCache { // If a file with the same name as an existing template-file, is added // in another search path, ReloadAllIfChanged will pick up the file in the - // earlier search-path. The file will only be reloaded if the orginal file - // is updated (touched, updated, deleted etc). See .cc file for more detail. + // earlier search-path. enum ReloadType { LAZY_RELOAD, IMMEDIATE_RELOAD }; void ReloadAllIfChanged(ReloadType reload_tyle);