Conversation
…main `ygg new` fetched origin (advancing origin/<default>) but then created the worktree from the local default branch via `git worktree add -b <name> <path> main`, where the bare name resolves to the *local* branch. When the local default branch was un-pulled, the new worktree was based on a stale commit. The `Pull()` method the help text implied was run was never called. Create() now resolves the base via baseRef(): - local behind origin (forgot to pull) -> base on origin/<default> - local ahead of origin (unpushed/offline) or diverged -> keep local, so local-only commits are never silently dropped - no remote -> local default branch Also: - `--no-track` so the new branch doesn't adopt origin/<default> as upstream - existing-branch detection uses an up-front ref check instead of a locale-fragile "already exists" string match - the success line reports the actual resolved base - removed the dead Pull() method; corrected the misleading help text/comment Tests cover both base directions, no-track, no-remote fallback, and existing- branch checkout. Claude-Session: https://claude.ai/code/session_01SMjzH78VMHpvo7LdtqRbPE
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1b1293c5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if m.isAncestor(defaultBranch, remoteRef) { | ||
| return remoteRef |
There was a problem hiding this comment.
When this returns origin/<default> because local main is behind, the local default branch is intentionally left stale. ygg clean still computes merged branches with the bare local name via wm.MergedBranches(defaultBranch) in internal/cli/clean.go:60-61, so a no-op worktree created at origin/main (or one later merged upstream) is not considered merged until the user manually pulls local main. That regresses cleanup in the stale-main scenario this change is fixing; the merge check needs to use the same resolved base/ref or otherwise refresh local main before cleaning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in c3ac260. Exported baseRef → BaseRef and routed both merge-detection paths (ygg clean and ygg remove via IsBranchMerged) through it, so they measure against the same resolved default tip (origin/<default> when local main is stale) that ygg new now bases worktrees on. A branch merged upstream is now detected before a local pull; behavior is unchanged when local is ahead, diverged, or there is no remote. Added TestIsBranchMergedUsesResolvedBaseForStaleLocalMain covering it.
Caveat: git branch --merged against any ref still does not detect squash-merged branches (the squashed commit is not an ancestor of the branch tip), so this does not change squash-merge cleanup — gone-branch cleanup remains the mechanism for that.
`ygg clean` and `ygg remove` (via IsBranchMerged) detected merged branches against the bare local default branch, so when `ygg new` bases worktrees on origin/<default> (stale local main), a branch merged upstream was not seen as merged until the user pulled local main. Export baseRef -> BaseRef and route both merge-detection paths through it, so clean/remove measure against the same resolved tip new worktrees are based on. For true/fast-forward merges this detects upstream merges before a local pull; behavior is unchanged when local is ahead, diverged, or there is no remote. Note: squash merges are not detected by `git branch --merged` against any ref, so this does not change squash-merge cleanup (use gone-branch cleanup for that). Claude-Session: https://claude.ai/code/session_01SMjzH78VMHpvo7LdtqRbPE
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3ac2603c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // diverged, the local branch wins so those commits are never silently dropped. | ||
| // Falls back to the local branch name when no remote-tracking ref exists. | ||
| func (m *Manager) BaseRef(defaultBranch string) string { | ||
| remoteRef := "origin/" + defaultBranch |
There was a problem hiding this comment.
When a repository has a local branch named origin/<default> (for example refs/heads/origin/main) in addition to the remote-tracking ref, returning the short origin/main makes the ref passed to git worktree add and git branch --merged ambiguous; I verified git worktree add ... origin/main exits with fatal: ambiguous object name: 'origin/main' in that case. Since this helper has already checked refs/remotes/..., return the fully qualified remote ref internally and keep any shorter name only for display so ygg new and cleanup still work in repos with valid slash-containing branch names.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in df0de3d. Reproduced your exact failure (fatal: ambiguous object name: 'origin/main') in a test with a local refs/heads/origin/main. BaseRef now returns the fully-qualified ref (refs/remotes/origin/<default> / refs/heads/<default>) so git worktree add and git branch --merged stay unambiguous, and a new DisplayRef helper shortens it back to origin/main for the user-facing "(based on …)" message. Covered by TestBaseRefQualifiesRemoteRef.
BaseRef returned the short "origin/<default>", which is ambiguous when a local branch literally named "origin/<default>" exists (refs/heads/origin/main vs refs/remotes/origin/main). git then fails with "fatal: ambiguous object name". Return the fully-qualified ref (refs/remotes/origin/<default> or refs/heads/<default>) so git commands stay unambiguous, and add DisplayRef to shorten it for the user-facing "(based on …)" message. Claude-Session: https://claude.ai/code/session_01SMjzH78VMHpvo7LdtqRbPE
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df0de3df3b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Find merged branches, measured against the resolved default tip | ||
| // (origin/<default> when local is stale) so branches merged upstream are | ||
| // detected consistently with how `ygg new` bases new worktrees. | ||
| mergedBranches, err := wm.MergedBranches(wm.BaseRef(defaultBranch)) |
There was a problem hiding this comment.
When BaseRef returns a fully-qualified ref here, MergedBranches no longer filters out the default branch itself: for example, running ygg clean from a non-primary worktree checked out on main makes git branch --merged refs/heads/main print * main, which MergedBranches normalizes to main and compares against refs/heads/main. That puts the default branch in mergedSet, so the current default-branch worktree is scheduled for removal; the previous MergedBranches(defaultBranch) call filtered main out.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 61a7d2c. You're right — once merge detection uses a fully-qualified ref, MergedBranches no longer self-filters the default branch. Extracted the candidate selection into selectMergedWorktrees and now skip both the primary worktree and any worktree on the default branch, independent of the merged set. Added TestSelectMergedWorktreesExcludesDefaultBranch (which fails on the old logic, selecting the main worktree, and passes now). IsBranchMerged was unaffected (it returns early for branch == defaultBranch).
Now that merge detection measures against a fully-qualified ref, MergedBranches no longer self-filters the default branch, so `git branch --merged` reporting `main` could put a non-primary default-branch worktree into the clean candidates and schedule it for removal. Extract the candidate selection into selectMergedWorktrees and skip both the primary worktree and any worktree on the default branch, regardless of what the merged set contains. Add a unit test (internal/cli now has coverage). Claude-Session: https://claude.ai/code/session_01SMjzH78VMHpvo7LdtqRbPE
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Problem
ygg newrunsgit fetch origin(advancingorigin/<default>) but then created the worktree from the local default branch viagit worktree add -b <name> <path> main— the bare name resolves to the local branch. If you hadn't pulledmain, the new worktree was based on a stale commit. ThePull()method the help text and a// Pull default branchcomment implied was running was never called anywhere.Reproduced: after fetch,
origin/mainat commit B but the new worktree came out at the stale local commit A.Fix
Create()now resolves the base via a newbaseRef()helper:origin/<default>✅It selects origin only when the local branch is an ancestor of
origin/<default>(git merge-base --is-ancestor).Also in this PR:
--no-trackso the new branch doesn't adoptorigin/<default>as its upstream"already exists"string matchCreated worktree at … (based on origin/main))Pull()method; corrected the misleading help text/commentTests
New tests cover both base directions (stale→origin, ahead→local),
--no-track, no-remote fallback, and existing-branch checkout. Verified locally withgo test -race ./...,go vet, andstaticcheck(matches CI), plus an end-to-end run of the built binary against the stale-main scenario.https://claude.ai/code/session_01SMjzH78VMHpvo7LdtqRbPE