10000 utils: merkletrie, Fix diff on sparse-checkout index. Fixes #1406 by onee-only · Pull Request #1492 · go-git/go-git · GitHub
[go: up one dir, main page]
Skip to content

utils: merkletrie, Fix diff on sparse-checkout index. Fixes #1406 #1492

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 14, 2025
Merged

utils: merkletrie, Fix diff on sparse-checkout index. Fixes #1406 #1492

merged 2 commits into from
Apr 14, 2025

Conversation

onee-only
Copy link
Contributor
@onee-only onee-only commented Apr 7, 2025 •
edited
Loading

Fixes #1406

This PR depends on #1484, as it partially fixes a diff issue in the sparse-checkout index.
Building on top of that, I’ve added a small check in the early return of addRecursive() to handle an edge case where the skipped node is a first-level node.
Please let me know if this check should instead be introduced in #1484. I'm not sure where it belongs.

There's a case where two trees are not the same, and one of them has nodes with skip == true, such as in comparisons between a sparse-checkout index and the worktree.
In this case, we shouldn't call ii.nextBoth(), since we might be comparing different nodes. So I added a check for that as well.

go-git/utils/merkletrie/difftree.go

Lines 319 to 341 in 4e67bbe

case bothHaveNodes:
if from.Skip() {
if err = ret.AddRecursiveDelete(from); err != nil {
return nil, err
}
if err := ii.nextBoth(); err != nil {
return nil, err
}
break
}
if to.Skip() {
if err = ret.AddRecursiveDelete(to); err != nil {
return nil, err
}
if err := ii.nextBoth(); err != nil {
return nil, err
}
break
}
if err = diffNodes(&ret, ii); err != nil {
return nil, err
}

Since this PR depends on #1484, I'll rebase this after that one is merged.

@Copilot Copilot AI review requested due to automatic review settings April 7, 2025 09:06
Copilot
Copilot AI reviewed Apr 7, 2025
View reviewed changes
Copy link
Contributor
@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

pjbgf
pjbgf reviewed Apr 13, 2025
View reviewed changes
pjbgf
pjbgf reviewed Apr 13, 2025
View reviewed changes
pjbgf
pjbgf requested changes Apr 13, 2025
View reviewed changes
Copy link
Member
@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@onee-only thanks for resolving this issue, just found two small nits. Otherwise, LGTM. Please rebase.

@pjbgf pjbgf mentioned this pull request Apr 13, 2025
Closed
@onee-only onee-only force-pushed the fix-sparse-checkout-status branch from 6a62b0c to af58bc3 Compare April 14, 2025 04:34
@onee-only onee-only force-pushed the fix-sparse-checkout-status branch from af58bc3 to 9dc151a Compare April 14, 2025 04:35
@onee-only
Copy link
Contributor Author

@pjbgf rebased and applied the suggestions. But the CI is failing on code that's unrelated to my changes. Could you please re-run it?

@onee-only
Copy link
Contributor Author

Just found out that we don't need to call AddRecursiveXXX when the node has to be skipped.

@pjbgf
Copy link
Member
pjbgf commented Apr 14, 2025

@onee-only thanks for rebasing. 🙇

Just found out that we don't need to call AddRecursiveXXX when the node has to be skipped.

Please add a comment explaining the reasons behind that change, so that it is easier for future contributors to understand it.

@onee-only
8000 Copy link
Contributor Author

Please add a comment explaining the reasons behind that change, so that it is easier for future contributors to understand it.

I don't think the comment in the code is necessary. Because the code before was more difficult to understand. Instead I'll leave a comment on the commit explaining why this change was made.

@onee-only
utils: merkletrie, Simplify DiffTreeContext.
e8c9fcd 
When node.Skip() == true, it means the node and all its descendants should be skipped.
Previously, we were still calling AddRecursiveDelete() on such nodes, which is redundant,
since addRecursive() skips the entire subtree.

This change removes those unnecessary AddRecursiveDelete() calls,
and also adds a missing check to skip the 'from' node in the 'onlyFromRemains' case.

Additionally, prettified the if-else structure in the switch case for better readability.
@onee-only onee-only force-pushed the fix-sparse-checkout-status branch from cebe050 to e8c9fcd Compare April 14, 2025 14:12
pjbgf
pjbgf approved these changes Apr 14, 2025
View reviewed changes
Copy link
Member
@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@onee-only fair point, LGTM. Thanks for working on this. 🙇

@pjbgf pjbgf merged commit 93ed29d into go-git:main Apr 14, 2025
16 checks passed
kane8n pushed a commit to kane8n/go-git that referenced this pull request Jun 4, 2025
@pjbgf @kane8n
Merge pull request go-git#1492 from onee-only/fix-sparse-checkout-status
c45824a 
 utils: merkletrie, Fix diff on sparse-checkout index. Fixes go-git#1406
kane8n pushed a commit to kane8n/go-git that referenced this pull request Jun 4, 2025
@pjbgf @kane8n
Merge pull request go-git#1492 from onee-only/fix-sparse-checkout-status
29a92a7 
 utils: merkletrie, Fix diff on sparse-checkout index. Fixes go-git#1406
@kane8n kane8n mentioned this pull request Jun 4, 2025
Merged
kane8n pushed a commit to kane8n/go-git that referenced this pull request Jun 4, 2025
@pjbgf @kane8n
Merge pull request go-git#1492 from onee-only/fix-sparse-checkout-status
c3c8410 
 utils: merkletrie, Fix diff on sparse-checkout index. Fixes go-git#1406
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@pjbgf pjbgf pjbgf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

File is marked for delete in worktree.Status() during sparse-checkout
2 participants
@onee-only @pjbgf
0