Skip to content

discard changes: hunks #7677

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 8 commits into from
Mar 21, 2025
Merged

discard changes: hunks #7677

merged 8 commits into from
Mar 21, 2025

Conversation

Byron
Copy link
Collaborator

@Byron Byron commented Mar 18, 2025

Add V3 facilities for discarding changes in the worktree or index, this time it's about hunks specifically.

Follow-up of #7597.

Tasks

  • figure out what's going on with the test and 'object not found' (and fix it)
  • fix content-conversions and hunk-references into said content for commit-engine
  • fix tree/index/worktree bug
    • assure tests for no-ops
    • check with symlinks at least once
    • deduplicate match - no need, will just be harder to read then

For next PR

Notes for the reviewer

  • I find it difficult to imagine how meaningful discarding parts of hunks in whole-file deletions and additions can be so I chose to disallow it. The UI can still offer discarding the single whole hunk that a deletion or addition of a file would come with, but internally it, as always, would discard the file instead. Discarding of lines or selections would have to be disallowed on the UI level though, as this is currently only available for modified (or renamed + modified) files.

For the next PR

  • See what happens if one tries to commit these special cases (i.e. the 'pretend there is no index' changes)
    • implement remaining ignored cases
  • gitoxide informs about diff-filter changes and this information is passed to the frontend
    • differentiate binary-to-text , otherwise it applies worktree conversions (which is fine). We ignore external diff programs anyway.
  • reset individual lines (but as free selections of sub-hunks)

Shortcomings

  • Index-handling is low-level and I think there needs to be something like a tree-editor, but for indices. Maybe it's a mix of using the pretty-neat tree-editor, and a way to transfer index information from one index to another, similar to 'apply', to avoid loosing information.

Out of Scope

  • snapshot integration - should be left to @krlvi to warm up with the new code.

Research

  • V2 implementation
    • spread across unapply_lines, unapply_ownership and reset_files

Copy link

vercel bot commented Mar 18, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
gitbutler-components ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 21, 2025 8:23am

@vercel vercel bot temporarily deployed to Preview – gitbutler-components March 18, 2025 08:01 Inactive
Copy link

vercel bot commented Mar 18, 2025

@Byron is attempting to deploy a commit to the GitButler Team on Vercel.

A member of the Team first needs to authorize it.

Byron added 8 commits March 21, 2025 16:21
If an index modification is dropped beecause there also is an overriding
modification to the worktree, assure that we use the higher-value modification
which can happen if the to-be-dropped modification is a rename.

Also add some protection for other cases that we might not know about.
That way it's easier to review or manipulate hunks as they wouild occur
in testing.
When comparing hunks we must be sure these refer these have been created
from the same content.

Currently, `unified_diff()` produces the content by applying binary-to-text filters,
which is good for presentation of binary files, but bad if hunks actually get selected there.

For now, rely on hunks having to match perfectly to be applicable, but assure that the content
used for file generation is the same (so the hunks that match actually refer to the right thing).
Previously, if there were tree/index changes and index/worktree changes,
the index/worktree changes would inform the states to diff.

This is correct for Git, but wrong for GitButler which is only interested
in the 'total' change.

Now we always use the previous change of a tree/index modification
as the previous change of the corresponding index/worktree modification.
@Byron Byron marked this pull request as ready for review March 21, 2025 08:23
@Byron Byron enabled auto-merge March 21, 2025 08:23
@Byron Byron merged commit 354fea3 into gitbutlerapp:master Mar 21, 2025
18 of 19 checks passed
@Byron
Copy link
Collaborator Author

Byron commented Mar 21, 2025

@Caleb-T-Owens The issue mentioned in Discord is now fixed, and led to the realisation that an entire bag of problems with worktree changes was nearly entirely ignored previously. Now I see that what GitButler really is doing is to "pretend the index doesn't exist". For the typical user and GitButler-exclusive operation that is the case even, and the index is always matching the tree it was created from.
When users start to bypass GitButler, it's possible that git add is invoked which adds new complexity. Previously GitButler would just ignore these tree/index changes, but also erroneously show diffs only between the index and the worktree.
Now both views are merged in a match statement with 16 cases, with only 6 that I don't think can actually happen. 4 of these possible cases are a bit unclear to me still and I left them for another day.
It's notable that this merge essentially synthesises an index/worktree change.

Every handled case (even those that aren't implemented yet) has a specific test-case to trigger it, which makes deciding what should be done very easy.

Follow-up PRs have to deal with the todo!() cases, but additionally I am interested in seeing what the commit-engine does when these synthetic changes are passed in. The question is really around index handling, and I would assume that it won't work correctly. Yet my hope is that it 'somewhat' works and that it resolves itself without being (too) incorrect. After all, these cases can be considered rare for now, even though they are a possibility.

Thinking about it, now that this is implemented, the UI could be used for testing how certain edge-cases will work quite easily, the test-cases show how these states can be triggered.

And yes, I guess you can take this as invitation to break it :D.

@krlvi I am merging this without review as this is only refactors in preparation for chunk-handling, along with plenty of fixes.

@Byron Byron deleted the discard branch March 21, 2025 08:38
@Byron Byron mentioned this pull request Mar 21, 2025
6 tasks
@Caleb-T-Owens
Copy link
Contributor

Thanks @Byron.

I'm starting to use the v3 client where possible, so I can hopefully exercise most of these

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants