fix(clone): clones that were empty, cut off, or still ending are handled safely by the next sync #175
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: KofTwentyTwo/gclo#175
Reference in new issue
Block a user
What
The git client and the WSL clone handle three kinds of unfinished clone safely: one whose origin was empty, one whose first checkout was cut off, and one whose operation is still ending when the next sync starts. See the commit message for the six changes.
Why
Closes #174
Deferred from the review of #148. The first two are data-safety defects: a forced first checkout could replace a file the user had put into the folder, and a cut-off first checkout was reported as up to date with an empty or half-written tree.
How it was tested
InterruptedCloneTests(10 tests, real repositories on disk): origin's first commit on another branch; a file, a folder, and a file-where-a-folder-is-needed in the way (refused, nothing changed, the user's content intact); an unrelated file stays; a checkout cut off after its branch was created is completed by the next sync; a branch started by hand with staged files is left alone; an origin that names no default branch; a second operation in a busy folder is refused and works afterwards; a clone without an index.WslClonerTests: the script clones under a temporary name, moves it into place, keeps the cleanup armed until then, and removes only old leftovers.Notes for review
BeforeFirstCheckoutForTestingis a second internal test seam next toAfterFetchForTesting; it is how the suite produces the cut-off state deterministically.Checklist
git commit -S -s)🤖 Generated with Claude Code
Review round 1: DO NOT MERGE. Reworked in 9c53d98; round 2 is running.
The first review reproduced two ways in which this change itself replaced a user's files:
git switch --orphan) looked like an empty clone to the default-branch switch; HEAD was moved and the next sync force-wrote the tree. Now only a repository with no local branch at all follows origin's default branch, and nothing is changed before every check has passed.Also fixed from that review: submodule places (anything but an empty folder is refused); a link where a folder is needed; a marker without a branch no longer reports an empty folder as done; a branch's upstream is configured before the branch exists; the WSL clone uses a temporary folder of gclo's own, cleans up on signals, does not move into a destination that appeared meanwhile, and accepts an empty folder; the long-path spelling of a folder is the same folder for the one-operation rule.
Tests added: each scenario the reviewer reproduced, a marker-ordering test, and
WslCloneScriptRunTests, which runs the script for real with a POSIX shell (five cases). Six deliberate breakages of the fixes were tried; five fail a specific test, the sixth did not compile.Gates: build, 1,101 engine and 212 CLI tests, 100% line coverage, format, UI suite 13 of 13.
Review round 2: DO NOT MERGE. Redesigned in 099c2b5; round 3 is running.
Round 2 confirmed the round-1 fixes for the reported cases and found that the approach behind them does not hold:
core.autocrlfwith a.gitattributes), so an interrupted clone of such a repository failed for ever. Onmainit completed by forcing.shis not on the path, path aliases.What changed in the design
main.).git. A repository that has branches of its own is not an empty clone.Tests: one for each round-2 finding, among them a real checkout that fails half-way (a file held open) with line endings being converted, which also asserts that such a checkout leaves no index. Seven deliberate breakages were tried; six fail a specific test. The seventh (removing a signal trap from the WSL script) cannot be caught here because the test shell is bash, which cleans up on signals either way; the reviewer verified that case under dash and busybox.
Gates: build, 1,115 engine and 212 CLI tests, 100% line coverage, format, UI suite 13 of 13.
Third review: fixes pushed (
41f3712)The third independent review confirmed, under real process kills, that a checkout that was cut off leaves no index, and found no path left on which a file of the user's is replaced or deleted. Its verdict was merge after fixes: five narrow cases in which gclo refused, failed, or reported done where it used to complete the tree. All are fixed in
41f3712.gclo.checkoutwrittenbefore, the last 16), and its files are recognised by it as wellmainident(and other checkout filters): gclo's own files were not recognisedAlso: a link at a submodule's place is refused; a link is something with a target, not anything with a reparse point; the message of an ordinary update that would replace files no longer says who changed them; README and threat model corrected; one doc comment moved to the member it describes.
Tests. 45 in
InterruptedCloneTests(one per finding, a file stored with carriage returns, a completion that fails half-way with the file held open, the order of what following origin's default branch writes). Eight deliberate breakages of the new code were each caught by a test.Gates on
41f3712. Build with warnings as errors: 0. Engine 1,127 passed, CLI 212 passed, line coverage 100% on the three gated assemblies, format clean, UI suite 13 of 13.A narrow fourth review of this delta runs before the merge.
🤖 Generated with Claude Code
The fourth review looked only at the changes of the third and found two ways, both opened by those changes, in which finishing an unfinished checkout cost the user something, and a few smaller things. - A file somebody added to the index with git, and that the commit does not have, was removed by the forced checkout. The completion is now refused, names the file, and says how to go on ('git reset', or move the folder). - Where the repository has a symbolic link, the folders on the way to it were no longer checked for links, so a link behind a linked folder could be replaced outside the clone. The way is checked again, and a link in the folder is the checkout's own only when it leads where the repository's does (also on a first checkout, where any link was taken before). - Whether a marked folder holds its whole tree is asked of the files, not of the index: what the index says is whoever's git wrote it. - A path recovery of an unfinished checkout whose branch had to move accepts the files of the commit it left (whole content), and takes the list of those commits off when it has written the tree. - Tests for each, for a list that holds what is not a commit, for the list's cap and its entries being recorded once, for a file written before the attributes that change it, and for a link to a folder in a finished tree. The test of a rewritten history now writes the file it makes a statement about, and states what happens to it: it stays, untracked. - README and threat model say what the code does, and name what is not covered: a marked folder whose index somebody's git wrote and whose last file was cut off is taken for finished; files of a commit the branch left stay in the folder. - The tests that run the WSL clone script use the shell Git for Windows' launcher starts, not the launcher: it puts git's folders in front of the path, so the stand-in for git was never found when the tests were started from PowerShell (as CI starts them), and two of them failed there. Refs #174 Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> Signed-off-by: James Maes <james@kof22.com>Fourth review (of the third round's changes only): fixes pushed (
5124ed3)Verdict was merge after fixes. It found two ways, both opened by the third round's changes, in which finishing an unfinished checkout cost the user something, and some smaller things. All verified against the code and fixed.
git reset, or move the folder)Named as not covered (finding 7): a marked folder whose index somebody's git wrote and whose last file was cut off is taken for finished (git shows that file as changed); files of a commit the branch left that the new commit does not have stay in the folder, untracked.
Not pinned by a test: that a link is recognised by its target and not by the reparse-point attribute (a reparse point that is not a link cannot be made offline).
Also found while running the gates: the tests that run the WSL clone script failed when started from PowerShell (as CI starts them). They picked Git for Windows' launcher
bin\sh.exe, which puts git's folders in front of the path, so the stand-in for git was never used. They now use the shell the launcher starts, and pass from PowerShell and from Git Bash.Checks. Eleven deliberate breakages of the new code, each caught by a test. The reviewer's reproductions run against
5124ed3: those of every fixed finding pass; the two that do not are the limits named above. Gates on5124ed3, started from PowerShell: build with warnings as errors 0, engine 1,141, CLI 212, line coverage 100% on the three gated assemblies, format clean, UI suite 13 of 13.🤖 Generated with Claude Code