Issue #5352 - generate_static_pages cannot deal with directories being turned into regular pages

cms/bin/generate_static_pages.py

Index: cms/bin/generate_static_pages.py
diff --git a/cms/bin/generate_static_pages.py b/cms/bin/generate_static_pages.py
index 5eaeb1b9af236f2833bb27005a0ffe89a3e0b7d2..b02aaa3077147455a34594f2c0880c78c2b0daca 100644
--- a/cms/bin/generate_static_pages.py
+++ b/cms/bin/generate_static_pages.py
@@ -20,6 +20,7 @@ import codecs
 import ConfigParser
 import logging
 from argparse import ArgumentParser
+import shutil
 
 from cms.utils import get_page_params, process_page
 from cms.sources import create_source
@@ -38,16 +39,25 @@ def generate_pages(repo, output_dir):
             return
         known_files.add(outfile)
 
-        if os.path.exists(outfile):
+        if os.path.isfile(outfile):

[Vasily Kuznetsov, 2018/09/24 11:58:47]

Are you sure if the two branches of this if statement exhaust all the
possibilities? What if it's some other weird object like a named pipe? We should
probably do something sensible there, like raise an error right away.

[Tudor Avram, 2018/09/24 14:04:51]

Also, one more thing: do we want to raise an error if the old version of the
website has some kind of a weird object or just remove it altogether (and assume
the new version is correct and should replace it)?

[Vasily Kuznetsov, 2018/09/24 14:42:23]

I think if there's some kind of weird object in there, we're probably dealing
with some unexpected situation and it's probably safer to stop. It seems that
this scenario is very unlikely anyway so it's probably fine.

[Tudor Avram, 2018/09/24 16:10:00]

Done.

             with codecs.open(outfile, 'rb', encoding=encoding) as handle:
                 if handle.read() == contents:
                     return
+        elif os.path.isdir(outfile):
+            shutil.rmtree(outfile)
 
         try:

[Vasily Kuznetsov, 2018/09/24 11:58:47]

I think it would be good to extract this whole block into a separate function.
Otherwise it's a bit too much logic altogether. This could also take care of
repeated `os.path.dirname(outfile)`.

We can also do a quick check: `if dir != output_dir:` and call the directory
creation only when it's actually needed.

After the code is in a separate method, we might also notice that the loop in
line 55 is doing more or less the same thing as `os.makedirs()` and maybe we can
just use `os.mkdir()` since we're already doing the iteration anyway. I leave it
up to you to decide which option makes more sense.

[Tudor Avram, 2018/09/24 13:26:45]

Yeah, I think it makes sense to put this into a different function. 

As your suggestion with replacing os.makedirs() comes, I chose to do it only if
the path already exists, because: 
1) It would rarely be the case for `os.path.isfile(path_so_far)` to be true and
then it os.makedirs() would only be called the second time if we would have a
file in the previous version of the website that now would become a directory.
For example, having `en/foo` as a file in the initial version and then the new
website being `en/foo/bar/index`,  would be a case where that loop actually
changes anything.
2) Yeah, I can replace `os.makedirs()` with multiple `os.mkdir`, but I just
assumed that os.makedirs() does something more clever in the backend (making it
more efficient) than my rough for loop.

[Vasily Kuznetsov, 2018/09/24 14:42:23]

I don't expect many directories to be created when any website is generated and
in creating directories the slowest part is the filesystem, so I don't think
`os.makedirs()` can be much faster than your loop.

I'm also not against using `os.makedirs()` as long as the code is simple -- I
was just thinking that it might become simpler and more straightforward with
`os.mkdir()`. 

[Tudor Avram, 2018/09/24 16:10:00]

Done.

             os.makedirs(os.path.dirname(outfile))
         except OSError as e:
             if e.errno != errno.EEXIST:
                 raise
+            path_so_far = output_dir
+            for part in path_parts[:-1]:
+                path_so_far = os.path.join(path_so_far, part)
+                if os.path.isfile(path_so_far):
+                    os.remove(path_so_far)
+                    os.makedirs(os.path.dirname(outfile))
+                    break
 
         with codecs.open(outfile, 'wb', encoding=encoding) as handle:
             handle.write(contents)