-
Notifications
You must be signed in to change notification settings - Fork 50
fix: use full DAG walk for explain on default branch #207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
On the default branch, checkpoint commits from merged feature branches live on second parents of merge commits. First-parent-only traversal misses these entirely. Restore the old repo.Log() full DAG walk for the default branch case, keeping first-parent walk for feature branches where it's needed to avoid walking into main's history. Add TestGetBranchCheckpoints_DefaultBranchFindsMergedCheckpoints to verify checkpoints from merged feature branches are found. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 2c679ac78905
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Fixes a regression where entire explain on the default branch missed checkpoints introduced via merged feature branches by switching the default-branch scan to a full DAG walk.
Changes:
- Use
repo.Log()(full DAG walk) on the default branch while keeping first-parent traversal + main filtering on feature branches. - Extract a
collectCheckpointhelper to share checkpoint→RewindPointconstruction. - Add a regression test covering merged (second-parent) checkpoint discovery on the default branch.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/entire/cli/explain.go | Restores full DAG traversal on default branch and factors out checkpoint collection logic. |
| cmd/entire/cli/explain_test.go | Adds regression test ensuring merged-branch checkpoints are found on default branch. |
iter.ForEach propagates callback errors to the caller, unlike walkFirstParentCommits which handles errStopIteration internally. Use go-git's storer.ErrStop sentinel which ForEach treats as normal termination (returns nil). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 684463275ef4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Summary
Follow-up to #203 — fixes a regression where
entire explainon the default branch (main/master) only showed checkpoints from first-parent commits, missing checkpoint commits that came in via merged feature branches (second parents of merge commits).repo.Log()full DAG walk (no branch filtering needed), finding checkpoints on all reachable commits including merged feature brancheswalkFirstParentCommitswithreachableFromMainfiltering (unchanged from fix: use first-parent traversal in explain to handle merge commits #203)collectCheckpointhelper to avoid duplicating the checkpoint-to-RewindPoint conversion between the two code pathsContext
The original
repo.Log()code worked on both default and feature branches because on the default branch,reachableFromMainwas empty so no filtering happened. #203 replacedrepo.Log()withwalkFirstParentCommitseverywhere, which fixed feature branches but broke the default branch by only walking the first-parent chain.Test plan
TestGetBranchCheckpoints_DefaultBranchFindsMergedCheckpoints— creates merge commit on master with checkpoint-tagged feature branch commit as second parent, verifies it's found-race🤖 Generated with Claude Code
Note
Low Risk
Behavior change is limited to how
getBranchCheckpointsenumerates commits on the default branch, with added test coverage for the merge-parent case; minimal impact outsideentire explainoutput.Overview
Fixes a regression in
entire explainbranch view where, on the default branch, checkpoint commits that arrived via merge commits (i.e., on non-first-parent paths) were not shown.getBranchCheckpointsnow uses a fullrepo.Log()DAG walk (with a scan limit) when on the default branch, while keeping the existing first-parent traversal plusreachableFromMainfiltering on feature branches; the checkpoint-to-RewindPointconversion is factored into a sharedcollectCheckpointhelper. Adds a regression test that constructs a real merge commit onmasterand asserts merged feature-branch checkpoints are discovered.Written by Cursor Bugbot for commit 5cb78b8. This will update automatically on new commits. Configure here.