Conversation
There was a problem hiding this comment.
The lock change itself is right. common_dir() over git_dir() is the correct call for repo-wide mutual exclusion, and a_worktree_can_acquire_and_shares_the_main_repository_lock pins both halves of that.
Blocking: ferrflow release still cannot run from a worktree, and now it fails dirty.
Checkpoint::path in src/monorepo/run/checkpoint.rs is still repo_root.join(".git").join(CHECKPOINT_FILENAME). In a linked worktree root is the worktree dir and its .git is a file, so reading <worktree>/.git/ferrflow.checkpoint.json returns ENOTDIR, not NotFound. Checkpoint::load therefore takes its Err(e) => arm and run_release_logic aborts at the Checkpoint::load(root)? call.
That call sits after the per-package loop, which has by then run pre- and post-bump hooks, written every version file and appended to every changelog. Before this PR the lock rejected the worktree before any of that ran; after it, the worktree user gets a half-bumped working tree, no commit and no tags. #793 is not closed by this, and the failure mode is worse than the one being fixed.
Suggested shape: give Checkpoint the open Repository the same way ReleaseLock now takes it, but resolve against repo.git_dir(), not common_dir(). A checkpoint is pinned to a specific HEAD and two worktrees have different HEADs, so a shared checkpoint file would make a crashed release in one worktree greet the next worktree with the "stale release checkpoint ... but HEAD is now" error. The lock already guarantees only one release runs per repository at a time, so per-worktree checkpoint state is safe. checkpoint.rs's init_test_repo needs the same git init treatment this PR gave lock.rs, since an empty .git directory is not openable.
Doing it as a fifth commit on the stack is fine if you would rather keep this PR to the lock, but then #793 should stay open until that lands.
Nit: the git() test helper sends stdout and stderr to Stdio::null(), so a failure panics with git ["worktree", "add", ...] failed and nothing else. Switching to .output() and putting the captured stderr in the assert message would pay for itself the first time this breaks on a CI image with a different git.
SonarQube — aucune nouvelle issueComparaison entre le projet bac à sable de cette PR et la branche par défaut : SonarQube Community n'analyse pas les PR, ce delta est calculé côté CI. Détail |
e2678d4 to
6dc6fc8
Compare
6dc6fc8 to
a6b8d2c
Compare
|
Fixed. That line now reads "delete Worth recording why it was split that way rather than left to the layer above: each PR should update the paragraph its own change invalidates, so #1054 carries the common-dir wording and #1055 carries the takeover rule. The two edits land on the same paragraph, so rebasing #1055 onto this conflicted; resolved in favour of #1055's rewrite, which already used the corrected path. |
63a4518 to
3e02cb1
Compare
Closes #793.
ReleaseLock::acquirebuilt its path asrepo_root/.git/ferrflow.lockand refused to run unless.gitwas a directory. In a linked worktree.gitis a file holdinggitdir: ..., so every mutating command aborted before doing any work andferrflow releasesimply could not be used from a worktree.The lock now takes the already-open
Repository(the caller inrun/mod.rshad one two lines above) and usesrepo.common_dir(), matching howcache.rsandcommit_graph.rsalready resolve the git dir instead of joining.gitby hand.common_dir()rather thangit_dir()is deliberate. Two worktrees of the same repository push to the same remote and compete on the same refs, so a per-worktree lock would not prevent the race the lock exists for. The common dir is shared by every linked worktree, so one repository gets one lock.Bare repositories still error, but the message now says "bare repository" instead of blaming worktrees, which was the reason the old message existed.
Verification
a_worktree_can_acquire_and_shares_the_main_repository_lockdoes a realgit worktree add, acquires from the worktree, and asserts the lockfile lands in the main checkout's.git/. It also asserts the worktree's.gitis not a directory, which is precisely the condition the old guard rejected, so the test cannot pass against the previous implementation. It then asserts the main checkout is refused while the worktree holds the lock, which is what pins thecommon_dirchoice overgit_dir.The test helpers now build real repositories with
git initinstead of an empty.gitdirectory, since aRepositoryhas to be openable.errors_when_git_dir_missingis gone:acquiretakes an openRepository, so "the path has no.git" is no longer representable and the test could only have asserted something the type system now guarantees.a_bare_repo_errors_and_says_socovers the remaining error path.Third layer of a stack of four. Based on #1053, and #1055 sits on top.