Cookiecutter: _run_hook_from_repo_dir should not remove files if they already existed before

[ionelmc]

The current behavior makes --overwrite-if-exists remove all the files (undesirable) if some hook fails.

[rsyring]

I'm not sure if this is the same issue, but I have had directories with content already in them completely disappear twice when using --overwrite-if-exists. The last example:

$ cookiecutter --overwrite-if-exists devops-pyapp
project_namespace [filebrowser]: 
project_dirname [filebrowser-src]: 
python_version [3.5]: 
python_tox_envname [py35]: 
python_executable [python3.5]: 
gh_repo_path []: 
gh_repo_name []: 
circle_badge_token []: 
codecov_api_token []: 
codecov_badge_token []: 
Unable to create file 'tox.ini'
Error message: 'dict object' has no attribute 'python_tox'
Context: {
    "cookiecutter": {
        "circle_badge_token": "", 
        "codecov_api_token": "", 
        "codecov_badge_token": "", 
        "gh_repo_name": "", 
        "gh_repo_path": "", 
        "project_dirname": "filebrowser-src", 
        "project_namespace": "filebrowser", 
        "python_executable": "python3.5", 
        "python_tox_envname": "py35", 
        "python_version": "3.5"
    }
}

After this happens, my filebrowser-src directory is completely wiped out. I've been forced to restore from backups b/c I had local-only configuration files get deleted that I didn't want to have to recreate.

IMO, this is really bad. I want to use CC to add devops files to already existing application repos. But my devs run the risk of deleting their app directory if something in the config goes wrong, that's a hard sell for me.

I can work on a PR if someone can confirm _run_hook_from_repo_dir is the problem and commit to reviewing and merging the PR. With 33 open PRs, I'm hesitant to start a new one. That's not a criticism, I realize that's how OSS goes sometimes. I just don't want to spend on the time on the PR until this issue is acknowledged.

Thanks.

[michaeljoseph]

@rsyring I don't think that the rmtree in generate. _run_hook_from_repo_dir is the culprit in your scenario. The code is raising jinja2.exceptions.UndefinedError, so the problem looks to be here:
https://github.com/audreyr/cookiecutter/blob/master/cookiecutter/generate.py#L352.
Introduced in #598.

[rsyring]

@michaeljoseph interesting...I made the following adjustment to cookiecutter locally and thought that this fixed the bug: https://github.com/rsyring/cookiecutter/commit/a14570c5c36400f441a377622ef8f6aa1ab35234

But I can revisit.

[hackebrot]

The error message reads

Unable to create file 'tox.ini'
Error message: 'dict object' has no attribute 'python_tox'
Context: {
    "cookiecutter": {
        "circle_badge_token": "", 
        "codecov_api_token": "", 
        "codecov_badge_token": "", 
        "gh_repo_name": "", 
        "gh_repo_path": "", 
        "project_dirname": "filebrowser-src", 
        "project_namespace": "filebrowser", 
        "python_executable": "python3.5", 
        "python_tox_envname": "py35", 
        "python_version": "3.5"
    }
}

Cookiecutter can't find a variable named python_tox in the context and exits gracefully by cleaning up the directory it was supposed to generate to. At this point this is implemented using a rmtree rather than removing each file and directory that has been created. Changing this may go hand in hand with #633.

[eliasdorneles]

Would it make sense to handle this case making the overwrite in a secondary step (i.e., writing into a temp dir first, before overwriting to destination)?
This would be simpler than tracking every file that was created.

Another idea would be to prompt for confirmation before deleting when the directory already existed before.

[rsyring]

writing to a temp directory and then copying to the new location does seem like it might be a good idea but outside the scope of this issue as it would be a larger architectural decision IMO.

Promoting to delete IMO doesn't win much. If the directory existed, it seems better to me to simply leave it alone and alert the user to the fact that a partial update occurred. Let them deal with it.