Skip to content

fix(clone): reject --no-checkout for non-bare layouts and cd into root on success#458

Merged
avihut merged 3 commits intomasterfrom
daft-443/fix/reject-no-co-for-non-bare-layout-clones
May 2, 2026
Merged

fix(clone): reject --no-checkout for non-bare layouts and cd into root on success#458
avihut merged 3 commits intomasterfrom
daft-443/fix/reject-no-co-for-non-bare-layout-clones

Conversation

@avihut
Copy link
Copy Markdown
Owner

@avihut avihut commented May 2, 2026

Summary

Two related --no-checkout fixes for git-worktree-clone:

  • Reject --no-checkout for non-bare layouts. The flag promises a bare clone with no worktree, but for non-bare layouts (sibling, nested, centralized, contained-classic) the resolved repo_path is the working tree — there's no separate bare directory to leave behind. The flag silently degraded to a regular clone with an empty working tree. Now bails after layout resolution with an error naming the resolved layout and pointing to bare alternatives (contained, contained-flat). Cleans up the bare clone (already on disk from clone_bare_phase) so a rejected clone leaves no orphan directory.
  • Cd into project root after successful --no-checkout. Previously the cd-redirect was gated on worktree_dir.is_some() (or branch_not_found), so the bare-only success case fell through and left the user in their original directory. Now writes parent_dir to DAFT_CD_FILE so the user lands in the new project root immediately. --no-cd still suppresses via the existing autocd check inside output.cd_path.

Updated --no-checkout help text and regenerated man pages + CLI docs to describe the bare-layout constraint.

Fixes #443

Test plan

  • Unit tests cover all 6 builtin layouts plus a custom non-bare layout (cargo test --lib commands::clone::tests)
  • YAML scenario clone:no-checkout-non-bare-rejected covers each non-bare builtin and asserts no orphan directory is left after rejection
  • clone:no-checkout extended to assert cd target is the project root via DAFT_CD_FILE
  • clone:empty-repo-no-checkout regression check still passes
  • mise run fmt, mise run clippy, cargo clippy --all-targets -- -D warnings clean
  • Live binary test confirmed cleanup preserves unrelated files in cwd

🤖 Generated with Claude Code

avihut and others added 2 commits May 2, 2026 15:00
`--no-checkout` promises a bare clone with no worktree, but for non-bare
layouts (sibling, nested, centralized, contained-classic) the resolved
repo_path *is* the working tree — there's no separate bare directory to
leave behind, so the flag silently degraded to a regular non-bare clone
with an empty working tree. Validate after layout resolution and bail
with an error naming the resolved layout and pointing to bare-using
alternatives. Clean up the bare clone (already on disk from
clone_bare_phase) so a rejected clone leaves no orphan directory.

Fixes #443

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously a successful `--no-checkout` clone left the user in their
original directory because the cd-redirect path was gated on
`worktree_dir.is_some()` (and `branch_not_found`). Neither branch fires
for the bare-only success case, so the cd silently did nothing — the
user had to manually `cd` into the new repo to interact with it.

Add a third branch that writes the parent_dir (the directory holding
the bare `.git`) to DAFT_CD_FILE when the clone succeeds with
no_checkout=true. `--no-cd` still suppresses it via the existing
`autocd` check inside `output.cd_path`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@avihut avihut added this to the Public Launch milestone May 2, 2026
@avihut avihut added the fix Bug fix label May 2, 2026
@avihut avihut self-assigned this May 2, 2026
@avihut
Copy link
Copy Markdown
Owner Author

avihut commented May 2, 2026

/claude review

@claude
Copy link
Copy Markdown

claude Bot commented May 2, 2026

Claude finished @avihut's task in 2m 29s —— View job


PR Review

  • Gather context: read diff and changed files
  • Review src/commands/clone.rs for Rust correctness and idioms
  • Review test coverage and regression tests
  • Review YAML scenarios
  • Review docs/man pages

Overall this is a clean, correct fix with solid test coverage. The two changes are well-scoped and the implementation follows existing patterns throughout the codebase. A few observations below, roughly ordered by priority.

Correctness

Cleanup path is correct. The check_no_checkout_compat error handler at clone.rs:284-290 correctly:

  1. Returns to original_dir before removing the clone directory (avoiding a dangling-cwd after remove_directory).
  2. Uses .ok() on both cleanup calls — appropriate, since masking the real error with a cleanup failure would be worse.
  3. Returns the original error so the caller sees the meaningful message.

--no-cd is respected. output.cd_path (cli.rs:268-278) gates on self.config.autocd, which is set from settings.autocd && !args.no_cd. The new --no-checkout cd branch at clone.rs:504-509 goes through the same output.cd_path, so --no-cd suppresses it correctly — consistent with the existing branches.

result.parent_dir is canonical at the cd call. The code at clone.rs:347-349 captures canonical_parent_dir from std::env::current_dir() immediately after Phase 3, and clone.rs:428 overwrites result.parent_dir with it before the final output block. So the --no-checkout cd call at line 508 uses an absolute path. No issue.


Minor Issues

1. Eager layout validation opportunity (UX improvement, not a bug)

When --layout is passed explicitly on the CLI, the resolved layout is known before any network or disk work begins. Currently the flow is:

  1. clone_bare_phase — full network clone to disk
  2. Read daft.yml from bare clone
  3. Resolve layout
  4. check_no_checkout_compat — reject, clean up

For the explicit-layout case (most common user error), all that disk/network work is wasted. You could call check_no_checkout_compat immediately after validate_arg_combinations when args.layout.is_some(), giving users instant feedback with no clone:

// In validate_arg_combinations or just after it in run():
if args.no_checkout {
    if let Some(ref layout_str) = args.layout {
        if let Some(builtin) = BuiltinLayout::from_name(layout_str) {
            let layout = builtin.to_layout();
            check_no_checkout_compat(true, &layout)?;
        }
    }
}

This is an enhancement, not a blocker — the current behaviour is correct, just slower for the reject path.

Fix this →

2. YAML scenario: orphan-check only covers sibling

no-checkout-non-bare-rejected.yml has test ! -e $WORK_DIR/test-repo-no-co-sibling && echo CLEAN after the sibling rejection, but no equivalent assertions for nested, centralized, or contained-classic. Since the cleanup logic is unconditional (same code path for all non-bare layouts), this is a minor coverage gap, not a behaviour issue.

Fix this →

3. Custom-layout test asserts fewer properties than the builtin test

no_checkout_rejects_custom_non_bare_layout (clone.rs:1499-1508) only asserts err.to_string().contains("'my-custom'"). The parallel builtin test at clone.rs:1471-1495 also verifies the message contains --no-checkout and the bare-layout suggestions. Since the error message is generated by the same check_no_checkout_compat function (no special-casing for builtins vs. custom), a custom layout will always produce the same message shape, but the test doesn't confirm it. Minor consistency gap.


Nits

  • BuiltinLayout::ContainedFlat acceptance not covered in YAMLno-checkout.yml uses --layout contained for its success test. Adding a step with contained-flat would mirror the unit test's no_checkout_accepts_bare_layouts coverage at the integration level.

  • Comment in check_no_checkout_compat — The doc comment references -n as an alias (-n in the comment, --no-checkout in the error message). The flag is actually declared short = 'n', so this is accurate, but the comment refers to the bare-clone concern generically. Fine as-is; just noting the comment is about the function's contract, not the implementation — consistent with project style.


Summary

Two correct fixes, well-scoped. Test coverage hits all 6 builtin layouts at the unit level and the four non-bare builtins at the YAML integration level. Man pages and CLI docs are updated. The suggestions above are improvements (faster failure, fuller YAML coverage) but nothing here blocks the merge.

Address PR review feedback by extending coverage of the existing
rejection paths:

- YAML scenario asserts no orphan directory is left for nested,
  centralized, and contained-classic rejections (previously only
  sibling). The cleanup branch is unconditional, but the parallel
  assertions catch future regressions if the cleanup logic changes.
- Custom-layout unit test now mirrors the builtin test's assertions:
  message includes --no-checkout and the bare-layout suggestions, not
  just the layout name.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@avihut avihut merged commit cec9002 into master May 2, 2026
11 checks passed
@avihut avihut deleted the daft-443/fix/reject-no-co-for-non-bare-layout-clones branch May 2, 2026 12:28
This was referenced May 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(clone): reject --no-checkout on non-bare layouts

1 participant