Warn when files are overwritten in the build directory (#12612)

CHANGES.rst

@@ -4,6 +4,8 @@ Release 7.4.7 (in development)
 Bugs fixed
 ----------
 
+* #12096: Warn when files are overwritten in the build directory.
+  Patch by Adam Turner.
 
 Release 7.4.6 (released Jul 18, 2024)
 =====================================

sphinx/builders/html/__init__.py

@@ -815,8 +815,8 @@ def copy_stemmer_js(self) -> None:
 
     def copy_theme_static_files(self, context: dict[str, Any]) -> None:
         def onerror(filename: str, error: Exception) -> None:
-            logger.warning(__('Failed to copy a file in html_static_file: %s: %r'),
-                           filename, error)
+            msg = __("Failed to copy a file in the theme's 'static' directory: %s: %r")
+            logger.warning(msg, filename, error)
 
         if self.theme:
             for entry in reversed(self.theme.get_theme_dirs()):
@@ -1142,7 +1142,8 @@ def js_tag(js: _JavaScript | str) -> str:
             source_name = path.join(self.outdir, '_sources',
                                     os_path(ctx['sourcename']))
             ensuredir(path.dirname(source_name))
-            copyfile(self.env.doc2path(pagename), source_name)
+            copyfile(self.env.doc2path(pagename), source_name,
+                     __overwrite_warning__=False)
 
     def update_page_context(self, pagename: str, templatename: str,
                             ctx: dict[str, Any], event_arg: Any) -> None:

sphinx/util/fileutil.py

@@ -8,12 +8,15 @@
 
 from docutils.utils import relative_path
 
+from sphinx.util import logging
 from sphinx.util.osutil import copyfile, ensuredir
 
 if TYPE_CHECKING:
     from sphinx.util.template import BaseRenderer
     from sphinx.util.typing import PathMatcher
 
+logger = logging.getLogger(__name__)
+
 
 def _template_basename(filename: str | os.PathLike[str]) -> str | None:
     """Given an input filename:
@@ -30,7 +33,8 @@ def _template_basename(filename: str | os.PathLike[str]) -> str | None:
 
 def copy_asset_file(source: str | os.PathLike[str], destination: str | os.PathLike[str],
                     context: dict[str, Any] | None = None,
-                    renderer: BaseRenderer | None = None) -> None:
+                    renderer: BaseRenderer | None = None,
+                    *, __overwrite_warning__: bool = True) -> None:
     """Copy an asset file to destination.
 
     On copying, it expands the template variables if context argument is given and
@@ -56,17 +60,36 @@ def copy_asset_file(source: str | os.PathLike[str], destination: str | os.PathLi
             renderer = SphinxRenderer()
 
         with open(source, encoding='utf-8') as fsrc:
-            destination = _template_basename(destination) or destination
-            with open(destination, 'w', encoding='utf-8') as fdst:
-                fdst.write(renderer.render_string(fsrc.read(), context))
+            template_content = fsrc.read()
+        rendered_template = renderer.render_string(template_content, context)
+
+        if (
+            __overwrite_warning__
+            and os.path.exists(destination)
+            and template_content != rendered_template
+        ):
+            # Consider raising an error in Sphinx 8.
+            # Certainly make overwriting user content opt-in.
+            # xref: RemovedInSphinx80Warning
+            # xref: https://github.com/sphinx-doc/sphinx/issues/12096
+            msg = ('Copying the rendered template %s to %s will overwrite data, '
+                   'as a file already exists at the destination path '
+                   'and the content does not match.')
+            logger.info(msg, os.fsdecode(source), os.fsdecode(destination),
+                        type='misc', subtype='copy_overwrite')
+
+        destination = _template_basename(destination) or destination
+        with open(destination, 'w', encoding='utf-8') as fdst:
+            fdst.write(rendered_template)
     else:
-        copyfile(source, destination)
+        copyfile(source, destination, __overwrite_warning__=__overwrite_warning__)
 
 
 def copy_asset(source: str | os.PathLike[str], destination: str | os.PathLike[str],
                excluded: PathMatcher = lambda path: False,
                context: dict[str, Any] | None = None, renderer: BaseRenderer | None = None,
-               onerror: Callable[[str, Exception], None] | None = None) -> None:
+               onerror: Callable[[str, Exception], None] | None = None,
+               *, __overwrite_warning__: bool = True) -> None:
     """Copy asset files to destination recursively.
 
     On copying, it expands the template variables if context argument is given and
@@ -88,7 +111,8 @@ def copy_asset(source: str | os.PathLike[str], destination: str | os.PathLike[st
 
     ensuredir(destination)
     if os.path.isfile(source):
-        copy_asset_file(source, destination, context, renderer)
+        copy_asset_file(source, destination, context, renderer,
+                        __overwrite_warning__=__overwrite_warning__)
         return
 
     for root, dirs, files in os.walk(source, followlinks=True):
@@ -104,7 +128,8 @@ def copy_asset(source: str | os.PathLike[str], destination: str | os.PathLike[st
                 try:
                     copy_asset_file(posixpath.join(root, filename),
                                     posixpath.join(destination, reldir),
-                                    context, renderer)
+                                    context, renderer,
+                                    __overwrite_warning__=__overwrite_warning__)
                 except Exception as exc:
                     if onerror:
                         onerror(posixpath.join(root, filename), exc)

sphinx/util/osutil.py

@@ -88,7 +88,12 @@ def copytimes(source: str | os.PathLike[str], dest: str | os.PathLike[str]) -> N
         os.utime(dest, (st.st_atime, st.st_mtime))
 
 
-def copyfile(source: str | os.PathLike[str], dest: str | os.PathLike[str]) -> None:
+def copyfile(
+    source: str | os.PathLike[str],
+    dest: str | os.PathLike[str],
+    *,
+    __overwrite_warning__: bool = True,
+) -> None:
     """Copy a file and its modification times, if possible.
 
     :param source: An existing source to copy.
@@ -101,7 +106,19 @@ def copyfile(source: str | os.PathLike[str], dest: str | os.PathLike[str]) -> No
         msg = f'{os.fsdecode(source)} does not exist'
         raise FileNotFoundError(msg)
 
-    if not path.exists(dest) or not filecmp.cmp(source, dest):
+    if not (dest_exists := path.exists(dest)) or not filecmp.cmp(source, dest):
+        if __overwrite_warning__ and dest_exists:
+            # sphinx.util.logging imports sphinx.util.osutil,
+            # so use a local import to avoid circular imports
+            from sphinx.util import logging
+            logger = logging.getLogger(__name__)
+
+            msg = ('Copying the source path %s to %s will overwrite data, '
+                   'as a file already exists at the destination path '
+                   'and the content does not match.')
+            logger.info(msg, os.fsdecode(source), os.fsdecode(dest),
+                        type='misc', subtype='copy_overwrite')
+
         shutil.copyfile(source, dest)
         with contextlib.suppress(OSError):
             # don't do full copystat because the source may be read-only

tests/roots/test-util-copyasset_overwrite/conf.py

@@ -0,0 +1,7 @@
+import os
+import sys
+sys.path.insert(0, os.path.abspath('.'))
+
+extensions = ['myext']
+html_static_path = ['user_static']
+html_theme = 'basic'

tests/roots/test-util-copyasset_overwrite/myext.py

@@ -0,0 +1,22 @@
+from pathlib import Path
+
+from sphinx.util.fileutil import copy_asset
+
+
+def _copy_asset_overwrite_hook(app):
+    css = app.outdir / '_static' / 'custom-styles.css'
+    # html_static_path is copied by default
+    assert css.read_text() == '/* html_static_path */\n'
+    # warning generated by here
+    copy_asset(
+        Path(__file__).parent.joinpath('myext_static', 'custom-styles.css'),
+        app.outdir / '_static',
+    )
+    # This demonstrates the overwriting
+    assert css.read_text() == '/* extension styles */\n'
+    return []
+
+
+def setup(app):
+    app.connect('html-collect-pages', _copy_asset_overwrite_hook)
+    app.add_css_file('custom-styles.css')

tests/roots/test-util-copyasset_overwrite/myext_static/custom-styles.css

@@ -0,0 +1 @@
+/* extension styles */

tests/roots/test-util-copyasset_overwrite/user_static/custom-styles.css

@@ -0,0 +1 @@
+/* html_static_path */

tests/test_util/test_util_fileutil.py

@@ -2,7 +2,10 @@
 
 from unittest import mock
 
+import pytest
+
 from sphinx.jinja2glue import BuiltinTemplateLoader
+from sphinx.util import strip_colors
 from sphinx.util.fileutil import _template_basename, copy_asset, copy_asset_file
 
 
@@ -103,6 +106,20 @@ def excluded(path):
     assert not (destdir / '_templates' / 'sidebar.html').exists()
 
 
+@pytest.mark.xfail(reason='Filesystem chicanery(?)')
+@pytest.mark.sphinx('html', testroot='util-copyasset_overwrite')
+def test_copy_asset_overwrite(app):
+    app.build()
+    warnings = strip_colors(app.warning.getvalue())
+    src = app.srcdir / 'myext_static' / 'custom-styles.css'
+    dst = app.outdir / '_static' / 'custom-styles.css'
+    assert warnings == (
+        f'WARNING: Copying the source path {src} to {dst} will overwrite data, '
+        'as a file already exists at the destination path '
+        'and the content does not match.\n'
+    )
+
+
 def test_template_basename():
     assert _template_basename('asset.txt') is None
     assert _template_basename('asset.txt.jinja') == 'asset.txt'