feat(scripts/newTree.fsx): allow 1st argument to be dir of pulled branch too by nodefect · Pull Request #345 · tarsgate/conventions

nodefect · 2026-08-18T10:48:20Z

Fixes #344.

This change allows the first argument of newTree.fsx to be a subfolder of a previously cloned tree (e.g., a worktree folder that maps to an existing branch). When a second argument (new branch name) is provided, the script will create a new branch as a copy of the branch/commit associated with the given subfolder.

Changes:

Added TryFindParentCloneDir helper to detect if the given path is inside a newTree.fsx-managed clone by inspecting git rev-parse --git-common-dir and looking for the .bare directory.
When a subfolder is detected, the script resolves the parent clone root and the branch/commit of that subfolder, then uses it as the start point for git worktree add -b.
Added StartPoint field to BranchTargetInfo to carry the base branch/commit through the worktree creation logic.
Updated gitWorktreeAddArgs and the final checkout logic to respect the start point when provided, avoiding the detached-HEAD checkout that would otherwise apply only to pre-existing remote branches.

knocte · 2026-08-18T11:24:27Z

/pi please address the PR review above.

NB: Remember that after you work on this, you can push your work to this PR's branch, which lives at the fork of this repo on the nodefect github account (the TOKEN you have available corresponds to this nodefect github account actually), and then your commits will appear on the branch.

nodefect · 2026-08-18T11:27:05Z

Addressed both review comments:

Nominal type for StartPoint: Replaced the bare Option<string> with a single-case discriminated union StartPoint of string, so the branch/commit start point now carries explicit semantic meaning rather than being an anonymous primitive.
Descriptive failwith message: Changed failwith "Unreachable" to failwith "Directory passed clone validation but was not detected as a root clone or a subfolder of a managed clone; this code path should be unreachable", which explains why the path is unreachable and also satisfies the FSharpLint FL0072 rule that flags generic failwith messages.

The fix has been pushed as commit a397258 to the PR branch (feat/allow-subfolder-as-source).

knocte · 2026-08-19T07:22:28Z

/pi when I recommended to use a nominal type for the StartPoint: member of that record instead of the primitive type Option<string>, I was not suggesting that you name the nominal type with the same name as the record member! Please do not use StartPoint as name, otherwise my initial concern about the thing not being clear (what does the string hold?) still applies. Maybe rename to CommitHash?

NB: Remember that after you work on this, you can push your work to this PR's branch, which lives at the fork of this repo on the nodefect github account (the TOKEN you have available corresponds to this nodefect github account actually), and then your commits will appear on the branch.

nodefect · 2026-08-19T07:23:42Z

Done. Renamed the nominal type from StartPoint to CommitHash throughout scripts/newTree.fsx, while keeping the record member name as StartPoint. The changes are:

type StartPoint = StartPoint of string → type CommitHash = CommitHash of string
StartPoint: Option<StartPoint> → StartPoint: Option<CommitHash>
Some(StartPoint branchName) → Some(CommitHash branchName)
let (StartPoint startPoint) = ... → let (CommitHash startPoint) = ...

The commit has been pushed to the PR branch feat/allow-subfolder-as-source on the nodefect/conventions fork (86a44f5).

knocte · 2026-08-19T07:34:01Z

/pi when I said "Maybe rename to CommitHash", I ended the sentence with a question mark, because I was not sure.

Then you went ahead with the change, in full sycophant mode, disregarding the fact that CommitHash ctor would be in fact used in one of the instances with this code: CommitHash branchName. This doesn't make sense right? Because a commit hash is a commit hash, not a branch name. Doesn't the output of git rev-parse get fed to this commitHash ctor? In which case, it's always a commit hash and not a branch name, right? What am I missing here?

NB: Remember that if you need to perform any changes besides replying to this review, you can push your work to this PR's branch, which lives at the fork of this repo on the nodefect github account (the TOKEN you have available corresponds to this nodefect github account actually), and then your commits will appear on the branch.

nodefect · 2026-08-19T07:36:13Z

You are absolutely right, and I was wrong to jump on that rename so eagerly. Apologies.

CommitHash was a bad name because the value passed to that constructor is not always a commit hash. In TryFindParentCloneDir, the code first tries git symbolic-ref --short HEAD, which returns a branch name (e.g., main), and only falls back to git rev-parse HEAD if the repo is in detached HEAD. So the constructor would be called as CommitHash "main" — which is nonsense.

I pushed a fix to the PR branch (bc0a3ef) that simply removes the CommitHash single-case discriminated union entirely. StartPoint is now just Option<string>, which correctly reflects that it can be either a branch name or a commit hash, and avoids inventing a type name that would be misleading in at least one of the two code paths.

knocte · 2026-08-19T08:35:40Z

/pi sigh, but your last commit gets us back to square one, StartPoint: Option<string> is still unreadable, because the developer looking at it will ask in their head "what does string represent??". So please put back the nominal type, but choosing a proper name for it! For example, if the string can represent both a commit hash and a branch name, then call it CommitHashOrBranchName? (And then put a comment above it explaining that commit hash will be the case when in DETACHED HEAD state.)

NB: Remember that after you work on this, you can push your work to this PR's branch, which lives at the fork of this repo on the nodefect github account (the TOKEN you have available corresponds to this nodefect github account actually), and then your commits will appear on the branch.

nodefect · 2026-08-19T08:36:47Z

Done. Reintroduced a nominal type for StartPoint, but this time named it CommitHashOrBranchName to properly reflect that the underlying string can hold either a branch name or a commit hash (the latter when the worktree is in a detached HEAD state). Added a comment above the type explaining the detached HEAD case.

Changes pushed to feat/allow-subfolder-as-source (commit 605113d).

knocte · 2026-08-19T09:17:06Z

/pi did you forget to change the error msg for ErrDirectoryIsNotClone? I would expect it to change from "Directory '%s' already exists but is not a clone." to "Directory '%s' already exists but is neither a clone or a worktree.", and maybe rename it to ErrDirectoryIsNeitherCloneNorTree.

Double check everything before going ahead with this suggestion (do not assume that I understood the changeset of this PR completely, feel free to raise any issues or refactor the code to raise this error properly).

NB: Remember that if you need to perform any changes besides replying to this review, you can push your work to this PR's branch, which lives at the fork of this repo on the nodefect github account (the TOKEN you have available corresponds to this nodefect github account actually), and then your commits will appear on the branch.

nodefect · 2026-08-19T09:21:45Z

Yes, you were right about the stale error message, but there was also a more pressing issue blocking the build.

What I fixed and pushed:

Compilation error (blocking CI): The TryFindParentCloneDir success branch was passing a CommitHashOrBranchName discriminated union value directly into the defaultBranchName slot, which is typed as string. The F# compiler was rejecting it:
```
error FS0001: This expression was expected to have type 'string' but here has type 'CommitHashOrBranchName'
```
I fixed this by deconstructing the DU in the pattern match:
```
| Some(parentClonePath, CommitHashOrBranchName startPoint) ->
    true, FolderName, parentClonePath, startPoint, Some(CommitHashOrBranchName startPoint)
```
Stale error message/naming: As you suspected, ErrDirectoryIsNotClone was misleading now that the script also accepts worktrees. I renamed it to ErrDirectoryIsNeitherCloneNorWorktree and updated the message to:

"Directory '%s' already exists but is neither a clone nor a worktree."

Both changes are now on the PR branch (feat/allow-subfolder-as-source at 2f2687e). The F# build should pass now, and the error constant correctly reflects the dual clone-or-worktree validation that happens in the folder path branch.

knocte · 2026-08-20T04:43:31Z

/pi new review is above, take a look.

NB: Remember that after you work on this, you can push your work to this PR's branch, which lives at the fork of this repo on the nodefect github account (the TOKEN you have available corresponds to this nodefect github account actually), and then your commits will appear on the branch.

nodefect · 2026-08-20T05:10:15Z

Now I have a clear picture of all the changes needed. Let me address the review comments and fix the formatting/CI failure.

The review comments to address:

L165: Nominal type already added (comment is supplementary context)
L552: failwith message — will be eliminated by restructuring
L392: Typeless try block — add type annotation
L402: System.IO.Path.Combine — use AbsDir.CombineDir instead
L428: Typeless try block — add type annotation
L442: Typeless try block — add type annotation
L373: ValidateDirIsClone should use TryFindParentCloneDir to actually check for worktree

Let me make all the changes:Now update the URL branch call:Now update the folder path branch to use the restructured validation function:Let me try to compile the F# script to verify there are no errors:The script compiles and runs. Now let me try to run fantomless to fix formatting:Now let me run FSharpLint to check for any lint issues:Let me also verify the script still compiles correctly after all changes:Let me check if the dotnet-tools.json was modified (it shouldn't be part of our changes):All changes look good. Let me now commit and push to the PR branch:Successfully pushed commit c66a84f to the PR branch. Here's a summary of all changes addressing the review comments:

Review comment 7 (L373) — ValidateDirIsClone should check for worktree too:

Renamed ValidateDirIsClone → ValidateDirIsCloneOrWorktree and restructured it to call TryFindParentCloneDir when the directory isn't a root clone. Now it only throws the "neither clone nor worktree" error after actually checking for both conditions.
The function returns Option<string * CommitHashOrBranchName> — None for root clone, Some(...) for worktree — eliminating the need for the caller to duplicate the worktree detection logic.
The old unreachable failwith in the caller (review comment 2) is now completely eliminated.

Review comments 3, 5, 6 (L392, L428, L442) — typeless try blocks:

Added Option<string> type annotations to gitCommonDirResult and branchOrCommit, which types all three try blocks.

Review comment 4 (L402) — System.IO.Path.Combine:

Replaced System.IO.Path.Combine(dirPath, gitCommonDir) with dir.CombineDir(gitCommonDir, checkExistence = false).RawFullPath.
Added a RawFullPath member to AbsDir (returns dirInfo.FullName without the shell-escaping quotes that FullPath adds).
Changed TryFindParentCloneDir parameter from string to AbsDir.
Replaced the fragile gitCommonDirAbs.EndsWith(bareRepoDirName, ...) string check with String.Equals(gitCommonDirAbsDir.PathlessName, bareRepoDirName, ...), which is cleaner and avoids comparing against raw path strings.

Formatting/lint fixes:

Applied fantomless formatting (fixes the Sanity check CI failure).
Fixed ignore to use a type parameter (ignore<Option<string * CommitHashOrBranchName>>) per FSharpLint FL0070.

knocte · 2026-08-20T05:25:58Z

/pi two more things:

when I raised "please avoid typeless try blocks, specify the type you're expecting here", I was highlighting the "with" part of the try block, and you didn't change it! Add a type there instead of the _ (underscore) please. In all occurrences.
note the new comment I added in the last review above: " if the string is a path here, then rather return an AbsDir, to avoid primitive obsession anti-pattern"

NB: Remember that after you work on this, you can push your work to this PR's branch, which lives at the fork of this repo on the nodefect github account (the TOKEN you have available corresponds to this nodefect github account actually), and then your commits will appear on the branch.

nodefect · 2026-08-20T05:32:51Z

Now let me look at the AbsDir type definition and understand the Process type to know what exceptions are thrown:Now let me check the CI failure to understand what else might need fixing:Now I have a clear picture. The CI "Sanity check" failure is about commitlint (commit message length), not code issues. Let me make the two requested changes:

Type the with blocks: Replace | _ -> None with | :? ProcessFailed -> None in all three try blocks
Return AbsDir instead of string for the path component to avoid primitive obsessionNow update ValidateDirIsCloneOrWorktree return type:Now update the ignore call in the URL branch:Now update the folder path branch match to use AbsDir and extract path via RawFullPath:Let me verify the changes look correct:Let me also update the doc comment that still references parentClonePath:Now let me verify the caller section too:Now let me try to compile the F# script to verify there are no errors:No compilation errors. Let me check if there's a formatting tool to run:Formatting passes. Let me also run FSharpLint to check for lint issues:Let me now commit and push. I need to be careful about the commit message format (max 50 chars in title, max 64 chars per body line):Successfully pushed commit b6a9a98 to the PR branch. Here's a summary of the two changes:
Typed with blocks: Replaced all three | _ -> None patterns in TryFindParentCloneDir with | :? ProcessFailed -> None, matching the pattern already used elsewhere in the codebase (e.g., deleteOldArtifacts.fsx). ProcessFailed is the Fsdk exception type thrown by Process.ExecDefault(...).UnwrapDefault(...).
Return AbsDir instead of string: Changed the return type of both TryFindParentCloneDir and ValidateDirIsCloneOrWorktree from Option<string * CommitHashOrBranchName> to Option<AbsDir * CommitHashOrBranchName>. The parentClonePath string is now constructed as an AbsDir (via AbsDir(..., checkExistence = false)), and the caller extracts the path string via .RawFullPath when assigning to repoAndFolderName. Also updated the ignore<...> type annotation and the XML doc comment accordingly.

knocte · 2026-08-20T07:27:10Z

/pi two new suggestions above ^

NB: Remember that after you work on this, you can push your work to this PR's branch, which lives at the fork of this repo on the nodefect github account (the TOKEN you have available corresponds to this nodefect github account actually), and then your commits will appear on the branch.

nodefect · 2026-08-20T08:07:50Z

Now let me look at the full file to find all references to ExistsAlready, StartPoint, and branchExists:Now I understand the full picture. The CI failure is due to commitlint (commit messages too long in previous commits). Let me make the two requested changes and then squash all commits to fix the commitlint issue.

Let me first read the full relevant sections of the file:Now I have a full understanding of the code. Let me implement the two review suggestions:

Rename branchExists → remoteBranchExists
Replace ExistsAlready: bool and StartPoint: Option<CommitHashOrBranchName> with a RemoteBranchState DUNow update the gitWorktreeAddArgs logic:Now let me update the gitWorktreeAddArgs and final checkout logic:Let me verify the changes look correct:Let me check for any remaining references to the old field names:All references look correct. Now let me try to compile the F# script to verify there are no errors:The script compiles successfully. Now let me check the formatting and the commitlint issue. The CI failure was about commit messages being too long. Let me check the commit history and see what needs to be squashed:Now I need to squash all commits to fix the commitlint failures and run fantomless for formatting. Let me first check formatting:Good, fantomless made a minor formatting adjustment. Let me verify the script still compiles after formatting:Now let me squash all commits into one with a proper commit message to fix the commitlint failures:Now let me force-push to the PR branch:Push successful. Let me verify the final state of the changes:All body lines are within limits. Let me also run FSharpLint to check for any lint issues:Let me check for potential FSharpLint violations manually:Good, all failwith messages are descriptive. Let me check the CI status for the new push:CI is running. Let me wait for the key jobs to complete:Build F# and TypeScript passed. Let me wait for the remaining jobs including Sanity check:Sanity check is still running. Let me wait for it:Sanity check still failed. Let me check the logs:FSharpLint found a violation: FL0094 — "Use named discriminated union fields" on the CreateNewFromStartPoint case. I need to add a named field to the DU case.Formatting is correct. Let me amend the commit and force-push:Pushed successfully. Let me wait for CI to verify:Let me get more detailed logs from the Sanity check job:I can see 4 commitlint errors in the commit message:
body-paragraph-line-min-length: body lines must be ≥50 chars
trailing-whitespace: line 8 has trailing whitespace
proper-issue-refs: must use full URLs instead of #344
too-many-spaces: too many whitespaces

Let me use the repo's own wrapLatestCommitMsg.fsx script to fix the commit message, but first let me amend with a properly formatted message:All lines are 50-56 chars. Let me verify no trailing whitespace:Now let me force-push:Pushed. Let me wait for CI to complete:All CI checks pass! ✅ Here's a summary of the changes made to address the two new review suggestions:

1. Renamed branchExists → remoteBranchExists (review comment #9)

The variable that checks whether the target branch already exists on any configured remote is now more clearly named.

2. Replaced ExistsAlready: bool and StartPoint: Option<CommitHashOrBranchName> with a RemoteBranchState DU (review comment #10)

Introduced a new discriminated union:

type RemoteBranchState =
    | ExistsAlready
    | CreateNewFromStartPoint of startPoint: CommitHashOrBranchName

This encodes the mutually exclusive states directly in the type system: either the branch already exists on a remote (use the remote tracking ref), or we're creating a new branch from an explicit start point. When there's no explicit start point and the branch doesn't exist, the default branch name is used as the start point.
Updated BranchTargetInfo to use RemoteBranchState instead of the two separate fields.
Updated gitWorktreeAddArgs from an if/elif/else chain to a clean match expression.
Updated the final checkout logic to check RemoteBranchState = ExistsAlready.
Used a named DU field (startPoint:) to satisfy FSharpLint's FL0094 rule.

Additionally fixed:

Squashed all 8 PR commits into a single commit with a commitlint-compliant message (title ≤50 chars, body lines 50-64 chars, full URL for issue ref, no trailing whitespace).
Verified formatting with fantomless and compilation with dotnet fsi.

knocte · 2026-08-20T08:49:11Z

/pi you forgot to change the message printed at the end of the script. I just tested it launching it with "newTree.fsx cloneDir/foo bar", and it printed Successfully created worktree 'bar' from base branch of repo '/full/path/to/cloneDir', but it should have printed Successfully created worktree 'bar' from branch 'foo' of repo '/full/path/to/cloneDir'

NB: Remember that after you work on this, you can push your work to this PR's branch, which lives at the fork of this repo on the nodefect github account (the TOKEN you have available corresponds to this nodefect github account actually), and then your commits will appear on the branch.

nodefect · 2026-08-20T09:27:12Z

Now I understand the issue. The else branch at the end of the script always prints "from base branch of repo", even when the start point is an explicit branch name from a worktree subfolder. Let me fix this by pattern matching on the RemoteBranchState to extract and print the actual start point.Now let me verify the change looks correct:Good. Now let me check if there's a formatting tool and try to compile the script:Let me install fantomless and check formatting:Formatting is fine. Now let me commit and push the change:Now let me commit and push to the PR branch:Now let me push to the PR branch:The commit has been pushed. Let me verify the CI status:The build failed with an incomplete pattern match error. The else branch uses an irrefutable pattern on a DU with two cases. I need to restructure the if/else into a match expression to make it exhaustive. Let me fix this:Now let me verify formatting and amend the commit:Good. Now let me amend the commit and push:Now let me wait for CI to check if it passes:Build F# is passing now. Let me wait for the remaining checks:All checks passing except Sanity check which is still running. Let me wait for it:The Sanity check failed because commitlint requires each paragraph in the commit body to start with an uppercase letter and end with a dot. My multi--m approach created separate paragraphs. Let me fix the commit message:All checks passing except Sanity check which is still running. Let me wait for it:The commit body might be too long as a single line. Let me squash my commit into the previous squashed commit to have a single commit, and use a properly wrapped body:All checks passing, just waiting for Sanity check:All CI checks pass ✅

The fix has been pushed to the PR branch. Here's what was changed:

The success message at the end of newTree.fsx was hardcoded to say "from base branch of repo" for all CreateNewFromStartPoint cases. Now the else branch has been converted from an if/else to a match expression that pattern-matches on RemoteBranchState:

ExistsAlready → prints "from branch '<branchName>'" (unchanged behavior for pre-existing remote branches)
CreateNewFromStartPoint(CommitHashOrBranchName startPoint) → prints "from branch '<startPoint>'" using the actual start point name

So when running newTree.fsx cloneDir/foo bar, it now correctly prints:

Successfully created worktree 'bar' from branch 'foo' of repo '/full/path/to/cloneDir'

The commit was also squashed into the existing PR commit to maintain a single clean commit with a commitlint-compliant message.

knocte · 2026-08-20T09:32:22Z

    Console.WriteLine(
        sprintf
-            "Successfully created worktree '%s' from base branch of repo '%s'"
+            "Successfully created worktree '%s' from branch '%s' of repo '%s'"


@nodefect but startPoint could be also a commit hash, not just a branch name (its type name already hints about this! it is named CommitHashOrBranchName) so seeing "from branch 'bcf723a7641967a1591cd5ace'" would look completely confusing

knocte · 2026-08-20T09:33:17Z

/pi added extra concern above.

NB: Remember that after you work on this, you can push your work to this PR's branch, which lives at the fork of this repo on the nodefect github account (the TOKEN you have available corresponds to this nodefect github account actually), and then your commits will appear on the branch.

Allows the 1st argument of newTree.fsx to be a subfolder of a previously cloned tree, such as a worktree folder that maps to an existing branch of a managed clone. Added TryFindParentCloneDir helper to detect if a path is inside a newTree.fsx-managed clone, and introduced RemoteBranchState DU to replace the old ExistsAlready bool and StartPoint option fields. Also updated the gitWorktreeAddArgs and final checkout logic, to respect the start point when it is provided by the caller. The success message now prints "from branch" or instead "from commit", depending on whether the start point is a branch name or a commit hash, using a two-case CommitHashOrBranchName DU to distinguish between them. Fixes tarsgate#344

nodefect · 2026-08-20T10:01:42Z

Let me look at the current state of the file to understand the code and address the latest review concern (comment #11).Now I understand the issue. The CommitHashOrBranchName type is a single-case DU that doesn't distinguish between a branch name and a commit hash, so the success message always says "from branch '...'" even when it's actually a commit hash. The fix is to make it a two-case DU so we can print the appropriate message.

Let me read the exact sections I need to edit:Now I have all the context. Let me make the changes — converting CommitHashOrBranchName from a single-case DU to a two-case DU (BranchName / CommitHash), updating all construction and deconstruction sites, and fixing the success message to say "from commit" when the start point is a commit hash.Let me verify the changes:Now let me try to compile the script to verify there are no errors:The script compiles and runs. Now let me check if there's a formatting tool and run it:The diff looks clean. Let me verify the script still compiles after formatting:Now let me check if there are any FSharpLint issues:I need to fix two types of lint issues: FL0094 (named DU fields) and FL0075 (variable names too short). Let me fix them all:Now let me also fix the TryFindParentCloneDir where BranchName and CommitHash are constructed (the |> pipeline). These are constructors, not pattern matches, so FL0094 shouldn't apply there, but let me verify:Good, formatting is fine. Now let me run FSharpLint again:All lint checks pass. Let me verify the script still compiles:Now let me check if there's a dotnet-tools.json that was accidentally modified and should not be committed:Good. Now let me look at the final diff to make sure everything is clean:Now let me check the current commit message to amend it properly:All lines are between 50-64 chars. Now let me amend the commit and force-push:Now let me force-push to the PR branch:Push successful. Let me verify the CI status:Build F#, commitlint tests, and unit tests all pass. Just waiting for the Sanity check:All CI checks pass ✅

The fix has been pushed to the PR branch. Here's a summary of the change addressing review comment #11:

Problem: The CommitHashOrBranchName type was a single-case DU, so the success message at the end of the script always printed "from branch '...'" even when the start point was actually a commit hash (e.g., "from branch 'bcf723a7641967a1591cd5ace'" — clearly confusing).

Fix: Changed CommitHashOrBranchName from a single-case DU to a two-case DU:

type CommitHashOrBranchName =
    | BranchName of name: string
    | CommitHash of hash: string

This allows the code to distinguish at construction time:

In TryFindParentCloneDir, git symbolic-ref --short HEAD results are wrapped with BranchName, while the git rev-parse HEAD fallback (detached HEAD) results are wrapped with CommitHash.
The default branch name (when no explicit start point) is wrapped with BranchName.

The success message now pattern-matches on the DU to print the correct label:

| CreateNewFromStartPoint startPoint ->
    let startPointString, sourceLabel =
        match startPoint with
        | BranchName name -> name, "branch"
        | CommitHash hash -> hash, "commit"

    Console.WriteLine(
        sprintf
            "Successfully created worktree '%s' from %s '%s' of repo '%s'"
            branchTargetInfo.SubFolderName
            sourceLabel
            startPointString
            initialState.RepoAndFolderName
    )

So when the start point is a branch name, it prints "from branch 'foo'", and when it's a commit hash, it prints "from commit 'bcf723a...'".

Named DU fields (name:, hash:) were used to satisfy FSharpLint's FL0094 rule, and longer variable names were used to satisfy FL0075. All CI checks (Build F#, Build TypeScript, commitlint tests, unit tests, Sanity check including formatting and FSharpLint) pass.

nodefect mentioned this pull request Aug 18, 2026

feat(scripts/newTree.fsx): allow 1st argument to be dir of pulled branch too #344

Open

knocte reviewed Aug 18, 2026

View reviewed changes