Skip to content

Commit ac9c97b

Browse files
committed
worktree(fix): Delegate creation under ownership
why: Fetch, target resolution, and linked checkout creation must share one native ownership lock. what: - Delegate creation to the libvcs worktree operation. - Retain partial failures and report the created native position. - Verify remote-only targets, relative refs, and lock failures.
1 parent 7f382e3 commit ac9c97b

5 files changed

Lines changed: 92 additions & 187 deletions

File tree

‎CHANGES‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,8 @@ machine output. CLI synchronization requires POSIX process groups.
6464
resolve typed targets, honor drift and dirty policy, and retain native
6565
recovery results. Existing tag and commit checkouts move when their target
6666
changes. Parent Git options, selected remotes, and explicit detachment apply
67-
to worktree updates. Missing worktrees use native linked-worktree creation.
67+
to worktree updates. Missing worktrees fetch and create under the same native
68+
ownership lock; requested lock failures retain the completed creation result.
6869

6970
Configured discard requires `--yes`; included worktree policies are checked
7071
before the main checkout changes. JSON and NDJSON retain exact recovery

‎MIGRATION‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,12 @@ accepts `--timeout`. A timed-out worktree prevents further worktrees for that
3636
repository from starting. Interrupted worktree metadata remains unknown when
3737
the worker did not return it.
3838

39+
### Guarded worktree creation
40+
41+
Worktree creation fetches configured remotes before selecting its initial
42+
target. A failed requested worktree lock returns an error with completed
43+
creation state; inspect that result before retrying an occupied destination.
44+
3945
### Explicit checkout policy and sync results
4046

4147
The main checkout passes its selector kind, remote, and `sync` policy to

‎docs/cli/worktree/sync.md‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,9 @@ Sync all configured worktrees:
2222
$ vcspull worktree sync '*'
2323
```
2424

25-
Missing worktrees require locally available target metadata and use Git's
26-
linked-worktree operation. Existing
25+
Missing worktrees fetch configured remotes and establish their initial target
26+
with Git's linked-worktree operation. Creation holds the repository's ownership
27+
lock through fetch, target selection, worktree creation, and lock setup. Existing
2728
worktrees follow their configured `branch`, `tag`, `commit`, or native `rev`;
2829
changing a tag or commit target moves the checkout to that resolved commit.
2930
Branch updates are fast-forward only. A matching commit is not drift, even
@@ -56,6 +57,10 @@ does not automatically restore interrupted operations into their source.
5657

5758
## Timeouts and interruption
5859

60+
A requested worktree lock that fails reports an error even when the checkout
61+
was created. Inspect `update_state` before retrying; failed creation can leave
62+
refs or a partial checkout.
63+
5964
Each worktree has its own deadline, including native creation and update.
6065
Set it with `--timeout` or `VCSPULL_SYNC_TIMEOUT_SECONDS`; the default is
6166
10 seconds:

‎src/vcspull/_internal/worktree_sync.py‎

Lines changed: 16 additions & 149 deletions
Original file line numberDiff line numberDiff line change
@@ -777,14 +777,6 @@ def plan_worktree_sync(
777777
return entries
778778

779779

780-
def _prepare_worktree(project: GitSync) -> SyncResult:
781-
"""Check existing native ownership before the separate worktree-add command."""
782-
return project.update_repo(
783-
target=SyncTarget(commit=project.get_position().revision),
784-
policy=SyncPolicy(drift="keep"),
785-
)
786-
787-
788780
def sync_worktree(
789781
repo_path: pathlib.Path,
790782
wt_config: WorktreeConfigDict,
@@ -826,6 +818,7 @@ def sync_worktree(
826818
try:
827819
validate_worktree_config(wt_config)
828820
target, policy = checkout_settings(wt_config, worktree=True)
821+
assert target is not None
829822
path = _resolve_worktree_path(wt_config, workspace_root)
830823
exists = _worktree_exists(repo_path, path)
831824
if path.exists() and not exists:
@@ -835,28 +828,19 @@ def sync_worktree(
835828
repo_path, path if exists else repo_path, repo_config
836829
)
837830
if not exists:
838-
guard = _prepare_worktree(project)
839-
if not guard.ok:
840-
entry.result = guard
841-
else:
842-
resolved = project.resolve_target(target)
843-
entry.target_position = resolved
844-
attach = resolved.follows and not wt_config.get("detach", False)
845-
_create_worktree(
846-
repo_path,
847-
path,
848-
"branch" if attach else "commit",
849-
resolved.ref_name if attach else resolved.revision,
850-
{**wt_config, "detach": not attach},
851-
start_point=resolved.revision,
852-
)
853-
project = _worktree_project(repo_path, path, repo_config)
854-
if entry.result is None:
855-
execution_policy = policy if exists else SyncPolicy(dirty=policy.dirty)
831+
entry.result = project.create_worktree(
832+
path,
833+
target=target,
834+
detach=wt_config.get("detach", False),
835+
lock=wt_config.get("lock", False),
836+
lock_reason=wt_config.get("lock_reason"),
837+
set_remotes=repo_config is not None,
838+
)
839+
else:
856840
entry.result = project.update_repo(
857841
set_remotes=repo_config is not None,
858842
target=target,
859-
policy=execution_policy,
843+
policy=policy,
860844
detach=wt_config.get("detach", False),
861845
)
862846
result = entry.result
@@ -871,7 +855,11 @@ def sync_worktree(
871855
)
872856
entry.detail = entry.error
873857
else:
874-
entry.target_position = project.resolve_target(target)
858+
entry.target_position = (
859+
project.resolve_target(target)
860+
if exists
861+
else _worktree_project(repo_path, path, repo_config).get_position()
862+
)
875863
if entry.position is not None:
876864
entry.drifted = (
877865
entry.position.revision != entry.target_position.revision
@@ -904,127 +892,6 @@ def sync_worktree(
904892
return entry
905893

906894

907-
def _create_worktree(
908-
repo_path: pathlib.Path,
909-
worktree_path: pathlib.Path,
910-
ref_type: str,
911-
ref_value: str,
912-
wt_config: WorktreeConfigDict,
913-
*,
914-
start_point: str | None = None,
915-
) -> None:
916-
"""Create a new worktree.
917-
918-
Parameters
919-
----------
920-
repo_path : pathlib.Path
921-
Path to the main repository.
922-
worktree_path : pathlib.Path
923-
Path for the new worktree.
924-
ref_type : str
925-
Type of reference: 'tag', 'branch', or 'commit'.
926-
ref_value : str
927-
The reference value.
928-
wt_config : WorktreeConfigDict
929-
Full worktree configuration.
930-
931-
Raises
932-
------
933-
subprocess.CalledProcessError
934-
If the git command fails (e.g., ref not found).
935-
FileNotFoundError
936-
If the repository path does not exist.
937-
938-
Examples
939-
--------
940-
This function requires a valid git repository. When called with an invalid
941-
path, it raises FileNotFoundError:
942-
943-
>>> import pathlib
944-
>>> _create_worktree(
945-
... pathlib.Path("/nonexistent"),
946-
... pathlib.Path("/tmp/wt"),
947-
... "tag",
948-
... "v1.0.0",
949-
... {"dir": "../wt", "tag": "v1.0.0"},
950-
... ) # doctest: +ELLIPSIS
951-
Traceback (most recent call last):
952-
...
953-
FileNotFoundError: ...
954-
"""
955-
cmd = ["git", "worktree", "add"]
956-
957-
# Determine if we should detach
958-
detach = wt_config.get("detach")
959-
if detach is None:
960-
# Default: detach for tags and commits, not for branches
961-
detach = ref_type in ("tag", "commit", "rev")
962-
963-
if detach:
964-
cmd.append("--detach")
965-
966-
# Handle locking
967-
# git worktree add --lock does NOT support --reason, so when lock_reason
968-
# is specified, we skip --lock here and use "git worktree lock --reason" after
969-
lock_reason = wt_config.get("lock_reason")
970-
should_lock = wt_config.get("lock")
971-
if should_lock and not lock_reason:
972-
# Lock without reason - can use --lock flag directly
973-
cmd.append("--lock")
974-
975-
if ref_type == "branch" and not detach and start_point is not None:
976-
branch_exists = (
977-
subprocess.run(
978-
["git", "show-ref", "--verify", "--quiet", f"refs/heads/{ref_value}"],
979-
cwd=repo_path,
980-
capture_output=True,
981-
check=False,
982-
).returncode
983-
== 0
984-
)
985-
if not branch_exists:
986-
cmd.extend(["-b", ref_value])
987-
ref_value = start_point
988-
cmd.append(str(worktree_path))
989-
cmd.append(ref_value)
990-
991-
subprocess.run(
992-
cmd,
993-
cwd=repo_path,
994-
check=True,
995-
capture_output=True,
996-
text=True,
997-
)
998-
999-
# Apply lock with reason via separate command
1000-
# This handles both cases:
1001-
# 1. lock=True with lock_reason - lock with reason
1002-
# 2. lock_reason without explicit lock=True - also locks with reason
1003-
if lock_reason:
1004-
lock_cmd = [
1005-
"git",
1006-
"worktree",
1007-
"lock",
1008-
"--reason",
1009-
lock_reason,
1010-
str(worktree_path),
1011-
]
1012-
try:
1013-
subprocess.run(
1014-
lock_cmd,
1015-
cwd=repo_path,
1016-
check=True,
1017-
capture_output=True,
1018-
text=True,
1019-
)
1020-
except subprocess.CalledProcessError as e:
1021-
log.warning(
1022-
"Worktree created at %s but lock failed: %s",
1023-
worktree_path,
1024-
e.stderr.strip() if e.stderr else str(e),
1025-
)
1026-
1027-
1028895
def _update_worktree(worktree_path: pathlib.Path, branch: str) -> None:
1029896
"""Follow a branch with native fast-forward and dirty-abort safeguards.
1030897

‎tests/test_worktree.py‎

Lines changed: 61 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -2917,9 +2917,9 @@ def test_sync_worktree_oserror_exception(
29172917

29182918
wt_config: WorktreeConfigDict = {"dir": str(worktree_path), "tag": "v-oserror-test"}
29192919

2920-
# Mock _create_worktree to raise OSError
2920+
# Fail the native creation boundary to verify CLI error conversion.
29212921
mocker.patch(
2922-
"vcspull._internal.worktree_sync._create_worktree",
2922+
"libvcs.sync.git.GitSync.create_worktree",
29232923
side_effect=OSError("Mocked OSError: permission denied"),
29242924
)
29252925

@@ -3359,50 +3359,76 @@ def test_sync_exit_on_error_worktree_failures(
33593359
)
33603360

33613361

3362-
def test_sync_worktree_lock_failure_still_creates(
3362+
def test_sync_worktree_lock_failure_reports_partial_creation(
33633363
git_repo: GitSync,
33643364
tmp_path: pathlib.Path,
3365-
mocker: MockerFixture,
3365+
monkeypatch: pytest.MonkeyPatch,
33663366
) -> None:
3367-
"""Test worktree creation succeeds even if lock fails.
3367+
"""A failed requested lock retains the completed creation and reports failure."""
3368+
from libvcs.cmd.git import Git
33683369

3369-
Regression test: lock failure after successful creation marked the entire
3370-
operation as ERROR. Lock is non-critical — the worktree is still usable.
3371-
"""
3372-
workspace_root = git_repo.path.parent
3373-
worktree_path = workspace_root / "lock-fail-test-wt"
3370+
path = tmp_path / "lock-failed"
3371+
original_run = Git.run
33743372

3375-
# Create a tag
3376-
subprocess.run(
3377-
["git", "tag", "v-lock-fail-test"],
3378-
cwd=git_repo.path,
3379-
check=True,
3380-
capture_output=True,
3381-
)
3373+
def fail_lock(command: Git, args: list[str], **kwargs: t.Any) -> str:
3374+
if args[:2] == ["worktree", "lock"]:
3375+
message = "worktree lock failed"
3376+
raise OSError(message)
3377+
return original_run(command, args, **kwargs)
33823378

3383-
wt_config: WorktreeConfigDict = {
3384-
"dir": str(worktree_path),
3385-
"tag": "v-lock-fail-test",
3386-
"lock": True,
3387-
"lock_reason": "This lock will fail",
3388-
}
3379+
# Fail only lock metadata after native worktree creation has completed.
3380+
monkeypatch.setattr(Git, "run", fail_lock)
3381+
entry = sync_worktree(
3382+
git_repo.path,
3383+
{"dir": str(path), "commit": "HEAD", "lock": True},
3384+
tmp_path,
3385+
)
3386+
assert entry.action == WorktreeAction.ERROR
3387+
assert path.is_dir()
3388+
assert entry.result is not None and entry.result.update_state == "completed"
3389+
assert entry.result.errors[0].step == "worktree-lock"
33893390

3390-
# Mock only the lock command to fail (let creation succeed)
3391-
original_run = subprocess.run
33923391

3393-
def mock_run(*args: t.Any, **kwargs: t.Any) -> t.Any:
3394-
cmd = args[0] if args else kwargs.get("args", [])
3395-
if isinstance(cmd, list) and "lock" in cmd:
3396-
raise subprocess.CalledProcessError(1, cmd, stderr="lock failed")
3397-
return original_run(*args, **kwargs)
3392+
def test_worktree_creation_fetches_configured_remote(
3393+
policy_worktree: tuple[GitSync, pathlib.Path, str, str],
3394+
) -> None:
3395+
"""Creation establishes a branch that exists only in a configured remote."""
3396+
from libvcs.sync.git import GitSync
33983397

3399-
mocker.patch("vcspull._internal.worktree_sync.subprocess.run", side_effect=mock_run)
3398+
repo, existing, _, target = policy_worktree
3399+
upstream = GitSync(url=str(repo.path), path=existing.parent / "new-upstream")
3400+
upstream.obtain()
3401+
upstream.cmd.run(["branch", "new-remote-branch", target], check_returncode=True)
3402+
path = existing.parent / "new-remote-worktree"
3403+
entry = sync_worktree(
3404+
repo.path,
3405+
{"dir": str(path), "branch": "new-remote-branch", "remote": "upstream"},
3406+
path.parent,
3407+
repo_config={"url": repo.url, "remotes": {"upstream": str(upstream.path)}},
3408+
)
3409+
assert entry.action == WorktreeAction.CREATE, entry.error
3410+
assert entry.result is not None and entry.result.ok
3411+
assert entry.target_position is not None
3412+
assert entry.target_position.revision == target
3413+
assert repo.cmd.run(["-C", str(path), "symbolic-ref", "HEAD"]).strip() == (
3414+
"refs/heads/new-remote-branch"
3415+
)
34003416

3401-
entry = sync_worktree(git_repo.path, wt_config, workspace_root)
34023417

3403-
# Creation should succeed despite lock failure
3404-
assert entry.action == WorktreeAction.CREATE
3405-
assert worktree_path.exists()
3418+
def test_worktree_creation_reports_resolved_relative_revision(
3419+
policy_worktree: tuple[GitSync, pathlib.Path, str, str],
3420+
) -> None:
3421+
"""Reporting cannot resolve HEAD~1 again against the new detached checkout."""
3422+
repo, existing, base, _ = policy_worktree
3423+
path = existing.parent / "relative-worktree"
3424+
entry = sync_worktree(
3425+
repo.path, {"dir": str(path), "commit": "HEAD~1"}, path.parent
3426+
)
3427+
assert entry.action == WorktreeAction.CREATE, entry.error
3428+
assert entry.result is not None and entry.result.ok
3429+
assert entry.target_position is not None
3430+
assert entry.target_position.revision == base
3431+
assert repo.cmd.run(["-C", str(path), "rev-parse", "HEAD"]).strip() == base
34063432

34073433

34083434
def test_sync_worktree_empty_plan_returns_error(

0 commit comments

Comments
 (0)