Conversation
|
Several tests are failing; it seems that |
|
Yes, you're totally right. My bad, was confused with |
tnijboer
changed the title
|
@sisp what do you think of this approach? |
| assert (dst / "skip-if-exists.txt").read_text() == "baz" | ||
|
|
||
|
|
||
| def test_exclude_with_gitignore(tmp_path_factory: pytest.TempPathFactory) -> None: |
There was a problem hiding this comment.
This test passes even without the changes in the update algorithm, so it doesn't seem to cover the real scenario under test.
This test looks way too complex for the scenario under test. Could you please scope it down to the scenario where too many exclude patterns cause the OSError: [Errno 7] Argument list too long error?
| Thanks for your attention. | ||
| """ | ||
| ), | ||
| (repo / ".gitignore.jinja"): ".venv/\n**/.cache/\n", |
There was a problem hiding this comment.
I don't think this added file serves any purpose in this test case.
| (repo / ".gitignore.jinja"): ".venv/\n**/.cache/\n", |
| gitignore_path = Path(subproject_top, ".gitignore") | ||
| dir_patterns = set() | ||
| if gitignore_path.exists(): | ||
| with gitignore_path.open() as gitignore_file: | ||
| for line in gitignore_file: | ||
| line = line.strip() | ||
| if not line or line.startswith("#"): | ||
| continue | ||
| # Matches foo/, foo/*, foo/**/, **/test/, **/test/*, etc. | ||
| if ( | ||
| line.endswith("/") | ||
| or line.endswith("/*") | ||
| or line.endswith("/**/") | ||
| or re.match(r".*\*\*/.*", line) | ||
| ): | ||
| cleaned = re.sub(r"(\/\*|\/\*\*\/|\/$)", "", line) | ||
| dir_patterns.add(cleaned) | ||
| # Single loop: process ignored files and build exclude lists | ||
| ignored_dirs = set() | ||
| extra_exclude = [] | ||
| for filename in ignored_files.splitlines(): | ||
| if filename.startswith("!! "): | ||
| fname = filename.split("!! ").pop() | ||
| matched_dir = False | ||
| for p in dir_patterns: | ||
| if fname == p or fname.startswith(p + "/"): | ||
| if fname.endswith("/"): | ||
| ignored_dirs.add(fname.rstrip("/")) | ||
| matched_dir = True | ||
| break | ||
| if not matched_dir: | ||
| extra_exclude.append(fname) | ||
| for dir_pattern in ignored_dirs: | ||
| apply_cmd = apply_cmd["--exclude", dir_pattern + "/*"] | ||
| for skip_pattern in map(self._render_string, self.all_skip_if_exists): | ||
| apply_cmd = apply_cmd["--exclude", skip_pattern] | ||
| for skip_pattern in extra_exclude: |
There was a problem hiding this comment.
TBH, this looks quite complex for a problem, I'm a bit worried that we're missing some edge cases.
I haven't looked into it in detail, but I think there are two ways to exclude files in the update:
- Exclude files when applying the patch via
git apply --exclude, which has the limitation you've encountered. - Omit the files to be excluded from the patch.
Option 2 sounds potentially simpler and more robust. WDYT?
2 participants
Fixes #2244.
Problem
Running
copier updatefails withOSError: [Errno 7] Argument list too longon projects with extensive.gitignorefiles or_skip_if_existslists.The cause is that
copierbuilds thegit applycommand by adding an individual--exclude={pattern}argument for every exclusion. This exceeds the OS command-line argument length limit.Solution
This patch refactors the exclusion handling for
git apply:.gitignore,all_skip_if_exists,answers_relpath) are consolidated into a single list.git applycommand is modified to use a single--exclude-from=<temp_file>argument, pointing to the new file.try...finallyblock is used to guarantee the temporary file is cleaned up.This change avoids the argument length limit and allows
copier updateto succeed on projects with many exclusions.