feat(updating): propagate exec bit on new template files when core.fileMode=false#2640
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2640 +/- ##
========================================
Coverage 97.17% 97.17%
========================================
Files 60 60
Lines 7147 7269 +122
========================================
+ Hits 6945 7064 +119
- Misses 202 205 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Correct me if I'm wrong, but doesn't this PR only mark files added by the update algorithm as intended to be added if |
|
Ah I misunderstood then, I thought we wanted to deviate as little as possible from existing behaviour. Happy to adjust, brb! |
88beeaa to
43892eb
Compare
|
I've adjusted the PR to always intent-to-add new files:
Tests — updated to match:
Verified locally: 1077 passed, 5 skipped, 8 xfailed, 1 xpassed on macOS. ruff/mypy/editorconfig-checker all clean. |
43892eb to
91797c0
Compare
…leMode=false Files newly introduced by the template on `copier update` were not yet tracked in the destination's index, so the existing `_sync_git_index_executable_bit` short-circuited for them. With `core.fileMode=false` (Windows default) the user's eventual `git add` would record `100644` regardless of the template's intended mode, silently losing the executable bit. When an untracked file with desired exec bits is rendered into a `core.fileMode=false` repo, copier now registers an intent-to-add entry via `git add --intent-to-add` and rewrites its mode to `100755` via `update-index --cacheinfo`. The empty blob is preserved, so the user's eventual `git add` replaces it with the file's actual content while preserving `100755`. The intent-to-add marker (`A` in `git status`) is also a more accurate signal than `??` for files that are deliberate template output rather than random untracked content. When `core.fileMode=true`, copier still leaves new files untracked — git would have picked up the on-disk chmod naturally, and pre-staging would be inconsistent with copier's normal "leave rendered changes unstaged for user review" behavior. Closes copier-org#2630
91797c0 to
830bd06
Compare
There was a problem hiding this comment.
In addition to my inline comment, I've been giving this PR some more thought. Once #2376 lands, the update algorithm will use git merge instead of a complex sequence of low-level git commands. git merge automatically tracks new files (added in the source branch), so marking new files with "intent-to-add" would be inconsistent with the future (and arguably more correct) behavior. This means, auto-staging will naturally occur even when core.fileMode=true (we currently assert files to remain unstaged)
copier/tests/test_updatediff.py
Lines 2293 to 2300 in c4310ac
so the future behavior will be fully consistent for both core.fileMode=true and core.fileMode=false.
However, the Worker._render_file method is currently not aware of the operation mode, so staging (with or without "intent-to-add") applies to both initial copying and updating. I wonder what the best behavior in anticipation of #2376 is. 🤔
And another note that I had missed when reviewing #2605: We need to support copier copy even when git is not installed. I've restored this regression with #2651.
| # TODO: simplify with ``--format %(objectmode)`` once the | ||
| # minimum git version is raised to 2.38+ (--format cannot be | ||
| # combined with --stage; see git-ls-files(1)). | ||
| result = git("ls-files", "--stage", "--", str(dst_relpath)).strip() |
There was a problem hiding this comment.
I think this check is not sufficient because it also passes for files with merge conflicts which shouldn't be marked with "intent-to-add" because it changes the state. Perhaps we can check the status with git status --porcelain=v1 <dst_relpath> and only proceed for files without merge conflicts.
There was a problem hiding this comment.
I think the existing check already excludes conflicted files, so I'd argue the porcelain check is redundant — but happy to add it as belt-and-suspenders if you prefer.
Conflicted files are tracked: they have stages 1, 2, and 3 in the index. git ls-files --stage <path> returns three lines for them (one per stage), so result is non-empty and the early return at line 932 triggers. The cases are:
- Tracked, clean:
100644 abc 0\tfile(1 line) → return - Tracked, conflicted: stages 1/2/3 (3 lines) → return
- Untracked: empty → proceed to intent-to-add
The function is called from _render_file for every rendered file, but only untracked files reach the git add --intent-to-add line.
As a backstop, git add --intent-to-add itself refuses to run on conflicted files (fatal: cannot add the path '<file>' because of a merge conflict), and our try/except (OSError, ProcessExecutionError) would swallow that.
That said: under the current update flow, I don't think _register_in_git_index can even be reached with a conflicted file. Conflicts get registered at L1487-L1556 of _apply_update, which runs after current_worker.run_copy() (and thus after _render_file for every file). So at the time our function runs, the index has only stage 0 entries.
Let me know if there's a scenario I'm missing — happy to swap in git status --porcelain=v1 if you'd prefer.
|
#2376 looks great — leveraging I had a look and ported the exec-bit tests from #2605 and this PR onto Concrete results running the tests on
So you're right: the Happy to open a follow-up PR against One observation while I was poking around: |
|
It turns out that the test suite (specifically Line 959 in b80196e returns an empty string because none of the files are staged in those empty repos. This leads to incorrect permission updates: Lines 1621 to 1646 in b80196e Staging (not just as intent-to-add) all rendered files would fix the failing test. It would also imitate a regular Git 3-way merge better as we discussed before. What do you think about staging all rendered files (if the destination is a Git repo)? This would also propagate the exec bit on new template files as well as on newly generated projects whose destination is already a Git repo. |
Summary
Closes #2630. Follow-up to #2605.
#2605 fixed executable-bit propagation for files that already exist in the destination. Files introduced by the template for the first time on
copier updatewere still affected whencore.fileMode=false:script.shwith100755in the index.core.fileMode=false(Windows default).chmods it on disk (invisible to git withfileMode=false), then calls_sync_git_index_executable_bit— which short-circuited because the file is untracked.git add script.shrecords100644because git ignores on-disk mode withfileMode=false. Exec bit lost.Changes
copier/_main.py—_sync_git_index_executable_bit:For untracked files that should be executable, run
git add --intent-to-add <path>to register an empty index entry, then fall through to the existing--cacheinfopath to set the mode to100755. The user's eventualgit addreplaces the empty blob with the file's actual content while preserving the mode bit. The intent-to-add marker (Aingit status) is also a more accurate signal than??for files that are deliberate template output.git addwould record100644either way).core.fileMode=true(git picks up the on-disk chmod naturally — pre-staging would be inconsistent with copier's "leave changes unstaged for user review" behavior).tests/test_updatediff.py— 3 new tests:test_update_introduces_new_executable_file[True|False]100755onfileMode=false; file stays untracked onfileMode=truetest_update_introduces_new_non_executable_file[True|False]test_update_introduces_new_executable_file_then_user_commitscore.fileMode=false: copier update →git add→git commit→ committed tree records100755and the file's actual content. The template setup usesgit update-index --chmod=+x(rather thanPath.chmod) so the test runs identically on every platform — Linux, macOS, and Windows.Test plan