feat: include local dirty changes by default. fixes #184 #520

sabard · 2021-12-29T21:27:09Z

A quick attempt at implementing the changes described in this comment.

if a template is vcs-tracked and dirty, the changes are committed automatically with a "wip" commit and propagated to the subproject
user is warned in this scenario
tests for copy and update on dirty templates

I made this the default behavior, but would be happy to add a flag for this behavior and maintain the current default.

Also, think this will have to rebase off of #519 to pass CI

copier/template.py

tests/test_dirty_local.py

pawamoy · 2021-12-29T22:29:26Z

So, just to be clear: this change creates the wip commits in a temporary copy/clone, right? Not in the original template repository?

If it does so in the original:
I'm really not in favor of automatic wip commits in the original repository. I believe this is error prone and too invasive. Each time I change something and test project generation again, it will create an additional wip commit (amending the previous one or resetting before committing again would be even more dangerous IMO). Once my changes are OK, I'll have to reset all these wip commits myself? That is as annoying as having to commit manually, maybe even more. (Sorry for not thinking about all this earlier, in the linked issue)

Let me know what you all think (and correct me if I'm wrong!)

sabard · 2021-12-30T09:01:17Z

I originally was creating the commit in the original template repository because that was easier, but I agree that it is too intrusive.

The current code modifies vcs::clone instead of template::local_abspath and copies a dirty repo into a temporary directory so that it can add, commit, and clone.

copier/vcs.py

pawamoy · 2021-12-30T09:24:17Z

Docs failure is because mkdocs-autorefs 0.3.1 was installed, while mkdocstrings 0.17.0 was not. I should probably have tagged mkdocs-autorefs 0.4.0 to prevent that. Or better yet, upper bounds could be removed on Copier's dependencies 😛

sabard · 2021-12-30T18:07:16Z

Got it—should I just add mkdocs-autorefs = "^0.4.0" to pyproject.toml? I always use pip-tools and not too familiar with poetry.

pawamoy · 2021-12-30T19:56:21Z

I wasn't clear: mkdocstrings should be updated (up to 0.17.0), not mkdocs-autorefs. But it should probably be done in another PR (it's very easy, just manually update mkdocstrings accepted range in pyproject.toml and regen the lock file with poetry lock).

sabard · 2021-12-30T21:40:37Z

I saw this was fixed in #518, so I just rebased instead.

pawamoy · 2021-12-31T01:08:29Z

Oh yes, good catch.

codecov-commenter · 2022-01-01T12:59:37Z

Codecov Report

Merging #520 (d74ec86) into master (a283bc4) will decrease coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #520      +/-   ##
==========================================
- Coverage   96.45%   96.32%   -0.14%     
==========================================
  Files          40       41       +1     
  Lines        2623     2693      +70     
==========================================
+ Hits         2530     2594      +64     
- Misses         93       99       +6

Flag	Coverage Δ
unittests	`96.32% <100.00%> (-0.14%)`	⬇️

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

Impacted Files	Coverage Δ
copier/template.py	`97.43% <ø> (ø)`
copier/errors.py	`97.36% <100.00%> (+0.07%)`	⬆️
copier/vcs.py	`97.40% <100.00%> (+0.52%)`	⬆️
tests/test_dirty_local.py	`100.00% <100.00%> (ø)`
tests/conftest.py	`84.61% <0.00%> (-7.70%)`	⬇️
copier/tools.py	`87.95% <0.00%> (-4.82%)`	⬇️
copier/main.py	`94.15% <0.00%> (-0.33%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a283bc4...d74ec86. Read the comment docs.

yajo

Hi @sabard! Welcome to Copier 😊

This PR is very nice already. There are some improvements to do below, but overall it's almost ready to merge.

Thank you very much for your contribution!

PS: Please make that CI happy 😉

README.md

copier/main.py

copier/vcs.py

tests/test_dirty_local.py

sabard · 2022-01-03T01:23:56Z

Removed Python 3.7 incompatible dirs_exist_ok option in shutil.copytree and fixed a bug in test_dirty_local.py causing CI to error.

Should be ready for another CI run.

yajo · 2022-01-03T05:46:25Z

copier/vcs.py

+                        if os.path.isdir(s):
+                            copytree(s, d)
+                        else:
+                            copy2(s, d)


Looking closer to the algorithm, it's not much efficient. Luckily it's very easy to improve. It should:

clone

cd into url.

git diff --binary HEAD to get a patch.

cd into new clone

git apply the patch

wip commit.

This way you don't have to create yet aother clone just to add 1 extra commit. Besides, you avoid copying manually instead of using git clone (which is safer and more efficient too).

P,S. please avoid naming variables with a single letter. Makes the code harder to understand.

This turned out to be not so simple for a few reasons:

git diff does not, by default, give a diff of the untracked files (of interested here doing a dirty copy)

HEAD might not exist (e.g., git init was just run)

That said, I was able to get something closer to what you are asking for working by using git diff --no-index and then excluding */.git/* on the apply. This will of course copy over other unwanted files to the temporary clone directory, but they shouldn't make it into the subdirectory since they will be in the clone's .gitignore. I think this is arguably just as inefficient in the worst-case (large .gitignored file getting needlessly copied).

I tried getting something like git --git-dir=<clone_dst>/.git --work-tree=<template_src> diff HEAD SO source to work to no avail. Seems that anytime git-diff is used without --no-index it ignores untracked files.

Need to make a couple fixes for CI still, but pushed latest changes to avoid losing progress.

AFAIK git stash works both with staged and untracked files. Have you tried with it?

tests/test_dirty_local.py

yajo · 2022-01-03T06:09:11Z

Please also add a note in the changelog.

sabard · 2022-01-03T21:46:30Z

The most recent commit should probably pass CI. However:

I'm seeing error: git diff header lacks filename information when removing 1 leading pathname component from the test_worker_config_precedence test (attempting to copy the copier template itself). This is only triggered when the working tree is dirty, of course, but would appreciate any help figuring out how to fix this. Should be able to reproduce the error by checking out the latest commit here, making a change, and running the tests.

sabard · 2022-01-04T00:28:19Z

After some thinking, I figured it would be a better solution to create a new .git dir in a temporary location, but point it to the files in the template dir. This lets us add and commit the dirty (and untracked!) changes while avoiding the extra copy. Please see the latest code and let me know if you have any questions.

CHANGELOG.md

tests/test_dirty_local.py

copier/vcs.py

tests/test_dirty_local.py

yajo

Looks much nicer now!

Just remove that leftover line and one question to merge!

Thanks again, great job!

copier/vcs.py

yajo · 2022-01-06T12:23:28Z

copier/vcs.py

+
+    with local.cwd(location):
+        git("checkout", ref or "HEAD")
+        git("submodule", "update", "--checkout", "--init", "--recursive", "--force")


I've got the feeling that this block should be done before importing the ditty changes. However, if the test goes green I guess I'm just wrong.

I think it shouldn't matter if it runs before or after. But if there is no HEAD in the case of a recently intialized repo, then this fails and would need to be called after as well. I also realized that specifying ref means that dirty changes won't be considered and so added that to the dirty checking logic.

sabard · 2022-01-07T00:42:55Z

Any tips for figuring out what's wrong with CI on windows? I cloned and ran the tests on my windows box and they passed, so having trouble replicating to get more info on the error.

EDIT: tried a filecmp.clear_cache() in the latest code. don't have any way to test this on my end other than running CI again unfortunately.

tests/test_dirty_local.py

yajo

Looks good! Just make it green to merge.

Thanks.

pawamoy reviewed Dec 29, 2021

View reviewed changes

copier/template.py Outdated Show resolved Hide resolved

pawamoy reviewed Dec 29, 2021

View reviewed changes

copier/template.py Outdated Show resolved Hide resolved

pawamoy reviewed Dec 29, 2021

View reviewed changes

tests/test_dirty_local.py Outdated Show resolved Hide resolved

pawamoy reviewed Dec 30, 2021

View reviewed changes

copier/vcs.py Outdated Show resolved Hide resolved

sabard force-pushed the sabard/dirty-local branch from 960455b to 3fe1f42 Compare December 30, 2021 21:37

yajo requested changes Jan 1, 2022

View reviewed changes

sabard added 6 commits January 2, 2022 14:26

feat: include local dirty changes by default

94e712c

fix: create commit in temporary directory

4f27e2b

fix: add and test DirtyLocalWarning

d925445

fix: remove tmp dir in clone

75d943d

style: rebase changes

8c7cddc

style: PR changes

15177a8

sabard force-pushed the sabard/dirty-local branch from 3fe1f42 to 15177a8 Compare January 2, 2022 19:43

fix: remove python 3.7 incomaptiable code and fix CI error

b78a85d

yajo requested changes Jan 3, 2022

View reviewed changes

sabard added 4 commits January 3, 2022 19:17

refactor: create new git dir for dirty changes, but do not copy

7f68825

fix: PR change

32dad69

docs: update changelog

2d072d5

fix: typo

49e4fbd

sabard force-pushed the sabard/dirty-local branch from ae3470f to 49e4fbd Compare January 4, 2022 00:23

sabard requested a review from yajo January 4, 2022 00:28

yajo reviewed Jan 4, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

tests/test_dirty_local.py Outdated Show resolved Hide resolved

copier/vcs.py Outdated Show resolved Hide resolved

fix: final small PR changes

1973f52

sabard changed the title ~~feat: include local dirty changes by default~~ feat: include local dirty changes by default. fixes #184 Jan 5, 2022

yajo reviewed Jan 5, 2022

View reviewed changes

copier/vcs.py Outdated Show resolved Hide resolved

tests/test_dirty_local.py Show resolved Hide resolved

fix: make dirty copy work on update by cloning first

3cb102b

sabard requested a review from yajo January 5, 2022 19:46

yajo reviewed Jan 6, 2022

View reviewed changes

fix: potential windows CI fix and style

4c58e62

yajo reviewed Jan 9, 2022

View reviewed changes

tests/test_dirty_local.py Outdated Show resolved Hide resolved

yajo approved these changes Jan 9, 2022

View reviewed changes

fix: get better error message on windows failing CI

d74ec86

sabard requested a review from yajo January 10, 2022 17:43

yajo linked an issue Jan 11, 2022 that may be closed by this pull request

Don't checkout anything by default on local templates #184

Closed

yajo approved these changes Jan 11, 2022

View reviewed changes

yajo merged commit 6cc94c6 into copier-org:master Jan 11, 2022

sabard deleted the sabard/dirty-local branch January 11, 2022 19:20

garu57 mentioned this pull request Feb 25, 2022

bug: error "is not a relative path" changing answers_file path #579

Closed

yajo added this to the v6.0.0 milestone Mar 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: include local dirty changes by default. fixes #184 #520

feat: include local dirty changes by default. fixes #184 #520

sabard commented Dec 29, 2021

pawamoy commented Dec 29, 2021 •

edited

Loading

sabard commented Dec 30, 2021

pawamoy commented Dec 30, 2021

sabard commented Dec 30, 2021

pawamoy commented Dec 30, 2021 •

edited

Loading

sabard commented Dec 30, 2021

pawamoy commented Dec 31, 2021

codecov-commenter commented Jan 1, 2022 •

edited

Loading

yajo left a comment •

edited

Loading

sabard commented Jan 3, 2022

yajo Jan 3, 2022

sabard Jan 3, 2022 •

edited

Loading

yajo Jan 4, 2022

yajo commented Jan 3, 2022

sabard commented Jan 3, 2022

sabard commented Jan 4, 2022

yajo left a comment

yajo Jan 6, 2022

sabard Jan 7, 2022

sabard commented Jan 7, 2022 •

edited

Loading

yajo left a comment

feat: include local dirty changes by default. fixes #184 #520

feat: include local dirty changes by default. fixes #184 #520

Conversation

sabard commented Dec 29, 2021

pawamoy commented Dec 29, 2021 • edited Loading

sabard commented Dec 30, 2021

pawamoy commented Dec 30, 2021

sabard commented Dec 30, 2021

pawamoy commented Dec 30, 2021 • edited Loading

sabard commented Dec 30, 2021

pawamoy commented Dec 31, 2021

codecov-commenter commented Jan 1, 2022 • edited Loading

Codecov Report

yajo left a comment • edited Loading

Choose a reason for hiding this comment

sabard commented Jan 3, 2022

yajo Jan 3, 2022

Choose a reason for hiding this comment

sabard Jan 3, 2022 • edited Loading

Choose a reason for hiding this comment

yajo Jan 4, 2022

Choose a reason for hiding this comment

yajo commented Jan 3, 2022

sabard commented Jan 3, 2022

sabard commented Jan 4, 2022

yajo left a comment

Choose a reason for hiding this comment

yajo Jan 6, 2022

Choose a reason for hiding this comment

sabard Jan 7, 2022

Choose a reason for hiding this comment

sabard commented Jan 7, 2022 • edited Loading

yajo left a comment

Choose a reason for hiding this comment

pawamoy commented Dec 29, 2021 •

edited

Loading

pawamoy commented Dec 30, 2021 •

edited

Loading

codecov-commenter commented Jan 1, 2022 •

edited

Loading

yajo left a comment •

edited

Loading

sabard Jan 3, 2022 •

edited

Loading

sabard commented Jan 7, 2022 •

edited

Loading