Repository: audreyr/cookiecutter (python)

Issue:
_run_hook_from_repo_dir should not remove files if they already existed before
The current behavior makes `--overwrite-if-exists` remove all the files (undesirable) if some hook fails.


Pull request description:
Preserve existing output directory on failure
If a generated output directory already exists, we don't delete it on project generation failure.

Supersedes #684.
Fixes #629. 

Patch:
diff --git a/cookiecutter/generate.py b/cookiecutter/generate.py
index e42be0f..3bdd1e0 100644
--- a/cookiecutter/generate.py
+++ b/cookiecutter/generate.py
@@ -2,19 +2,20 @@
 
 """Functions for generating a project from a project template."""
 from __future__ import unicode_literals
-from collections import OrderedDict
+
 import fnmatch
 import io
 import json
 import logging
 import os
 import shutil
+from collections import OrderedDict
 
+from binaryornot.check import is_binary
 from jinja2 import FileSystemLoader
-from cookiecutter.environment import StrictEnvironment
 from jinja2.exceptions import TemplateSyntaxError, UndefinedError
-from binaryornot.check import is_binary
 
+from .environment import StrictEnvironment
 from .exceptions import (
     NonTemplatedInputDirException,
     ContextDecodingException,
@@ -23,8 +24,8 @@ from .exceptions import (
     UndefinedVariableInTemplate
 )
 from .find import find_template
-from .utils import make_sure_path_exists, work_in, rmtree
 from .hooks import run_hook
+from .utils import make_sure_path_exists, work_in, rmtree
 
 logger = logging.getLogger(__name__)
 
@@ -196,19 +197,19 @@ def render_and_create_dir(dirname, context, output_dir, environment,
 
     output_dir_exists = os.path.exists(dir_to_create)
 
-    if overwrite_if_exists:
-        if output_dir_exists:
+    if output_dir_exists:
+        if overwrite_if_exists:
             logger.debug(
                 'Output directory {} already exists,'
                 'overwriting it'.format(dir_to_create)
             )
-    else:
-        if output_dir_exists:
+        else:
             msg = 'Error: "{}" directory already exists'.format(dir_to_create)
             raise OutputDirExistsException(msg)
+    else:
+        make_sure_path_exists(dir_to_create)
 
-    make_sure_path_exists(dir_to_create)
-    return dir_to_create
+    return dir_to_create, not output_dir_exists
 
 
 def ensure_dir_is_templated(dirname):
@@ -219,19 +220,23 @@ def ensure_dir_is_templated(dirname):
         raise NonTemplatedInputDirException
 
 
-def _run_hook_from_repo_dir(repo_dir, hook_name, project_dir, context):
+def _run_hook_from_repo_dir(repo_dir, hook_name, project_dir, context,
+                            delete_project_on_failure):
     """Run hook from repo directory, clean project directory if hook fails.
 
     :param repo_dir: Project template input directory.
     :param hook_name: The hook to execute.
     :param project_dir: The directory to execute the script from.
     :param context: Cookiecutter project context.
+    :param delete_project_on_failure: Delete the project directory on hook
+        failure?
     """
     with work_in(repo_dir):
         try:
             run_hook(hook_name, project_dir, context)
         except FailedHookException:
-            rmtree(project_dir)
+            if delete_project_on_failure:
+                rmtree(project_dir)
             logger.error(
                 "Stopping generation because {} hook "
                 "script didn't exit successfully".format(hook_name)
@@ -260,7 +265,7 @@ def generate_files(repo_dir, context=None, output_dir='.',
         keep_trailing_newline=True,
     )
     try:
-        project_dir = render_and_create_dir(
+        project_dir, output_directory_created = render_and_create_dir(
             unrendered_dir,
             context,
             output_dir,
@@ -281,7 +286,17 @@ def generate_files(repo_dir, context=None, output_dir='.',
     project_dir = os.path.abspath(project_dir)
     logger.debug('Project directory is {}'.format(project_dir))
 
-    _run_hook_from_repo_dir(repo_dir, 'pre_gen_project', project_dir, context)
+    # if we created the output directory, then it's ok to remove it
+    # if rendering fails
+    delete_project_on_failure = output_directory_created
+
+    _run_hook_from_repo_dir(
+        repo_dir,
+        'pre_gen_project',
+        project_dir,
+        context,
+        delete_project_on_failure
+    )
 
     with work_in(template_dir):
         env.loader = FileSystemLoader('.')
@@ -326,7 +341,8 @@ def generate_files(repo_dir, context=None, output_dir='.',
                         overwrite_if_exists
                     )
                 except UndefinedError as err:
-                    rmtree(project_dir)
+                    if delete_project_on_failure:
+                        rmtree(project_dir)
                     _dir = os.path.relpath(unrendered_dir, output_dir)
                     msg = "Unable to create directory '{}'".format(_dir)
                     raise UndefinedVariableInTemplate(msg, err, context)
@@ -347,10 +363,17 @@ def generate_files(repo_dir, context=None, output_dir='.',
                 try:
                     generate_file(project_dir, infile, context, env)
                 except UndefinedError as err:
-                    rmtree(project_dir)
+                    if delete_project_on_failure:
+                        rmtree(project_dir)
                     msg = "Unable to create file '{}'".format(infile)
                     raise UndefinedVariableInTemplate(msg, err, context)
 
-    _run_hook_from_repo_dir(repo_dir, 'post_gen_project', project_dir, context)
+    _run_hook_from_repo_dir(
+        repo_dir,
+        'post_gen_project',
+        project_dir,
+        context,
+        delete_project_on_failure
+    )
 
     return project_dir


Test patch:
diff --git a/tests/test_generate_files.py b/tests/test_generate_files.py
index f71192f..65b582f 100644
--- a/tests/test_generate_files.py
+++ b/tests/test_generate_files.py
@@ -239,6 +239,26 @@ def test_raise_undefined_variable_file_name(tmpdir, undefined_context):
     assert not output_dir.join('testproject').exists()
 
 
+def test_raise_undefined_variable_file_name_existing_project(
+        tmpdir, undefined_context):
+    output_dir = tmpdir.mkdir('output')
+
+    output_dir.join('testproject').mkdir()
+
+    with pytest.raises(exceptions.UndefinedVariableInTemplate) as err:
+        generate.generate_files(
+            repo_dir='tests/undefined-variable/file-name/',
+            output_dir=str(output_dir),
+            context=undefined_context,
+            overwrite_if_exists=True
+        )
+    error = err.value
+    assert "Unable to create file '{{cookiecutter.foobar}}'" == error.message
+    assert error.context == undefined_context
+
+    assert output_dir.join('testproject').exists()
+
+
 def test_raise_undefined_variable_file_content(tmpdir, undefined_context):
     output_dir = tmpdir.mkdir('output')
 
@@ -275,6 +295,30 @@ def test_raise_undefined_variable_dir_name(tmpdir, undefined_context):
     assert not output_dir.join('testproject').exists()
 
 
+def test_raise_undefined_variable_dir_name_existing_project(
+        tmpdir, undefined_context):
+    output_dir = tmpdir.mkdir('output')
+
+    output_dir.join('testproject').mkdir()
+
+    with pytest.raises(exceptions.UndefinedVariableInTemplate) as err:
+        generate.generate_files(
+            repo_dir='tests/undefined-variable/dir-name/',
+            output_dir=str(output_dir),
+            context=undefined_context,
+            overwrite_if_exists=True
+        )
+    error = err.value
+
+    directory = os.path.join('testproject', '{{cookiecutter.foobar}}')
+    msg = "Unable to create directory '{}'".format(directory)
+    assert msg == error.message
+
+    assert error.context == undefined_context
+
+    assert output_dir.join('testproject').exists()
+
+
 def test_raise_undefined_variable_project_dir(tmpdir):
     output_dir = tmpdir.mkdir('output')
 
diff --git a/tests/test_generate_hooks.py b/tests/test_generate_hooks.py
index 7617899..b9b2950 100644
--- a/tests/test_generate_hooks.py
+++ b/tests/test_generate_hooks.py
@@ -31,12 +31,16 @@ def remove_additional_folders(request):
     Remove some special folders which are created by the tests.
     """
     def fin_remove_additional_folders():
-        if os.path.exists('tests/test-pyhooks/inputpyhooks'):
-            utils.rmtree('tests/test-pyhooks/inputpyhooks')
-        if os.path.exists('inputpyhooks'):
-            utils.rmtree('inputpyhooks')
-        if os.path.exists('tests/test-shellhooks'):
-            utils.rmtree('tests/test-shellhooks')
+        directories_to_delete = [
+            'tests/test-pyhooks/inputpyhooks',
+            'inputpyhooks',
+            'inputhooks',
+            'tests/test-shellhooks',
+            'tests/test-hooks',
+        ]
+        for directory in directories_to_delete:
+            if os.path.exists(directory):
+                utils.rmtree(directory)
     request.addfinalizer(fin_remove_additional_folders)
 
 
@@ -115,6 +119,65 @@ def test_oserror_hooks(mocker):
     assert message in str(excinfo.value)
 
 
+@pytest.mark.usefixtures('clean_system', 'remove_additional_folders')
+def test_run_failing_hook_removes_output_directory():
+    repo_path = os.path.abspath('tests/test-hooks/')
+    hooks_path = os.path.abspath('tests/test-hooks/hooks')
+
+    hook_dir = os.path.join(repo_path, 'hooks')
+    template = os.path.join(repo_path, 'input{{cookiecutter.hooks}}')
+    os.mkdir(repo_path)
+    os.mkdir(hook_dir)
+    os.mkdir(template)
+
+    hook_path = os.path.join(hooks_path, 'pre_gen_project.py')
+
+    with open(hook_path, 'w') as f:
+        f.write("#!/usr/bin/env python\n")
+        f.write("import sys; sys.exit(1)\n")
+
+    with pytest.raises(FailedHookException) as excinfo:
+        generate.generate_files(
+            context={
+                'cookiecutter': {'hooks': 'hooks'}
+            },
+            repo_dir='tests/test-hooks/',
+            overwrite_if_exists=True
+        )
+        assert 'Hook script failed' in str(excinfo.value)
+    assert not os.path.exists('inputhooks')
+
+
+@pytest.mark.usefixtures('clean_system', 'remove_additional_folders')
+def test_run_failing_hook_preserves_existing_output_directory():
+    repo_path = os.path.abspath('tests/test-hooks/')
+    hooks_path = os.path.abspath('tests/test-hooks/hooks')
+
+    hook_dir = os.path.join(repo_path, 'hooks')
+    template = os.path.join(repo_path, 'input{{cookiecutter.hooks}}')
+    os.mkdir(repo_path)
+    os.mkdir(hook_dir)
+    os.mkdir(template)
+
+    hook_path = os.path.join(hooks_path, 'pre_gen_project.py')
+
+    with open(hook_path, 'w') as f:
+        f.write("#!/usr/bin/env python\n")
+        f.write("import sys; sys.exit(1)\n")
+
+    os.mkdir('inputhooks')
+    with pytest.raises(FailedHookException) as excinfo:
+        generate.generate_files(
+            context={
+                'cookiecutter': {'hooks': 'hooks'}
+            },
+            repo_dir='tests/test-hooks/',
+            overwrite_if_exists=True
+        )
+        assert 'Hook script failed' in str(excinfo.value)
+    assert os.path.exists('inputhooks')
+
+
 def make_test_repo(name):
     hooks = os.path.join(name, 'hooks')
     template = os.path.join(name, 'input{{cookiecutter.shellhooks}}')