fix(updating): exclude deleted paths on update

[lkubb]

Previously, paths that matched a pattern in skip_if_exists and that were deleted in the generated project were recreated during each update.

These deletions are contained in the diff between old vanilla generated project and the (pre-update) current one. They are not executed though since the paths are excluded from the diff application.

Previously, if a file that was deleted intentionally in a generated project was changed in the template, an update would have regenerated it (but not if it did not have any changes).

This patch ensures that update behavior is consistent for paths that are not listed in skip_if_exists - don't regenerate deleted paths during update - by acquiring a list of deleted paths and excluding them in the copy operations that are relevant for acquiring the update diff. skip_if_exists paths are not excluded, their presence is always ensured, even by copier update (#1718 (comment)).

Fixes: #1721

Thanks a lot for maintaining this very neat tool btw!

[sisp]

Thanks for raising this problem and offering a PR to fix it, @lkubb! 🙏

I wonder whether we could take a different approach involving programmatic use of exclude to avoid unnecessarily re-creating files in the first place rather than deleting re-created files as a post-processing step. Similar to your current implementation, we need to determine which files in the skip_if_exists list have been deleted by the user. But if we moved building this list above overwriting the subproject with the new template version, then we could pass it via the exclude argument to that worker and also to the worker creating a fresh copy with the new template version. As a results, the diff should not contain files from skip_if_exists, as they don't get created at all.

I haven't tested this idea, so I might have overlooked something.

WDYT, @lkubb?

[lkubb]

Thanks for raising this problem and offering a PR to fix it, @lkubb! 🙏

@sisp Thanks for taking the time to consider my proposal. :) Responding to your issue comment caused me to find another related inconsistency (imho), #1721.

I wonder whether we could take a different approach involving programmatic use of exclude to avoid unnecessarily re-creating files in the first place rather than deleting re-created files as a post-processing step.

Your proposal additionally allows to fix #1721 elegantly by excluding all deleted paths (if solution (a) from the issue is deemed acceptable).

I haven't tested this idea, so I might have overlooked something.

An issue I can see with your proposal in comparison to mine is that tasks (not migrations, here it's an issue for both) might assume the presence of a specific file, but that's common to all exclude usage.

I will implement your proposal as described above, fixing the new issue as well. Happy to iterate on this if there is some disagreement. Thanks again for your time!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.61%. Comparing base (6fd3ad4) to head (2150516).
Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1719      +/-   ##
==========================================
- Coverage   97.71%   97.61%   -0.10%     
==========================================
  Files          49       49              
  Lines        4980     5042      +62     
==========================================
+ Hits         4866     4922      +56     
- Misses        114      120       +6     

Flag	Coverage Δ
unittests	97.61% <100.00%> (-0.10%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sisp]

Looks great! Just a few minor remarks, and we need to escape the file names to be excluded (see inline comment).

[yajo]

Sorry, I reviewed the PR before the issues (my fault).

Let's triage the issues first. I'm not yet convinced they're bugs. Thanks!

[yajo]

Thanks! This looks excellent!

Would you please remove those references to issues, so I can merge?

[sisp]

Excellent work, @lkubb, thanks! 👌

And thanks for taking over, @yajo; I have been on vacation.