Skip to content

discard changes #7597

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

Merged
merged 7 commits into from
Mar 18, 2025
Merged

discard changes #7597

merged 7 commits into from
Mar 18, 2025

Conversation

Byron
Copy link
Collaborator

@Byron Byron commented Mar 12, 2025

Add V3 facilities for discarding changes in the worktree or index.

Tasks

  • reset whole files (with safety-check as change must be matched)
    • addition
    • deletion
      • symlinks
      • submodules
      • executable bit
    • modification
      • symlinks
      • submodules
      • executable bit
      • type-change
    • fix: re-generate submodule from local clone only
    • restore type change from file to dir (dir is to be restored)
    • replace file with directory (with tests for single and multi-discard)
    • renamed file
      • onto nothing
      • onto directory
      • onto other file
    • renamed directory onto file
    • conflicted (aren't listed, so don't need handling, double-check)
    • walk-through-symlink-safety
  • index-update support (.git/index needs to represent the worktree) as needed
  • update gix to point to main
  • CLI integration
  • fix disk-space issue by avoiding an additional docker invocation, and fix the now failing tests.
  • tauri-integration to satisfy current UI capabilities
  • restore rust-test parallelism - no need, the cache works well and a full test-run is now below 5 minutes

Next PR

  • reset changed hunks in a file
  • reset individual lines

Notes for the Reviewer

  • I flattened the Rust test setup, removing indirections and seemingly speeding it up, while reducing the chance that the runner runs out of disk space. For this PR, this now happened all the time, probably because running through docker consumes more disk space? Having a 'dumber' CI setup also seems like a win to me, despite less elegant handling of dependencies.

    • Installing dependencies takes 34s, and pulling the container takes 36. However, the 36s contains the Rust cache, which isn't included in the 34s count.
    • Overall Usage for Rust jobs is reduced from 36 minutes to 24 19 minutes, and as less jobs are run more PRs can run in parallel.
    • Admittedly, removing docker from the mix was just an attempt to make CI work again, and it does indeed work now.
    • An uncached Rust test CI run can take 12 minutes, but with a cache this goes down to 4m 50s, which is less than the usual docker run (which seems a little slower under the same circumstances) Screenshot 2025-03-18 at 14 50 24
    • A CI run still takes just a little more than 5 minutes, so it remains at the sweet spot.
  • It turns out that the frequent CI failures were caused by disguised out-of-disk-space errors. They appear to be an LLVM-LD crash, but apparently that is just a C-Programs way of saying that the disk is full.

    • Clearling the GH caches usually does the trick, but it's bound to re-occur.

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.
  • UX: submodules aren't automatically checked out if they are part of a directory. It's a relatively rare case as well.
  • UX: Around renames, there can be a special case that makes the user discard a set of files twice before they go away. It's rare, and it's fixable if one wants to put in the time.

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 12, 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 18, 2025 6:52am

@vercel vercel bot temporarily deployed to Preview – gitbutler-components March 12, 2025 02:45 Inactive
Copy link

vercel bot commented Mar 12, 2025

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

A member of the Team first needs to authorize it.

@krlvi
Copy link
Member

krlvi commented Mar 12, 2025

Awesome! One random thought - when it comes time to wire this as a tauri /CLI command, we could consider having the guard passed instead of it being created deeper in the call stack. Mainly because the snapshotting would need the same guard too - here's what i have in mind
e60e0d8 d3e1f42

@Byron
Copy link
Collaborator Author

Byron commented Mar 13, 2025

Right, thanks for the reminder! I remember the dead-lock problem that taught us that lesson 😅.
Maybe it's fair to say that these plumbing crates should never create this guard themselves, so either CLI or Tauri have to create it.

Byron added 7 commits March 18, 2025 14:51
For now, no hunks or hunk-ranges.
That way the discard implementation can be triggered in the real world.
It seems to take quite some time on CI even though it's only for benches,
which already have a feature toggle.

Also remove unused dependencies everywhere.
…-space.

We run out of disk-space so often now that the system is barely usable.
To keep the change minimal, this is only applied to Rust checks.
It's an interesting side-effect that running in Docker seems to work
better, maybe due to running as root?

Try to emulate what seems to be happening on CI by changing the desired
umask, which usually does the right thing when running locally.
@krlvi
Copy link
Member

krlvi commented Mar 18, 2025

Looking great, thanks Byron!

) -> Result<Vec<but_workspace::discard::ui::DiscardSpec>, Error> {
let project = projects.get(project_id)?;
let repo = but_core::open_repo(&project.worktree_path())?;
let _guard = project.exclusive_worktree_access();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This implementation is made around "discard only uses Git and probably there aren't issues with parallelism", so it doesn't take the write-permission itself (yet). The CLI doesn't obtain it, for instance, for that reason.

Something I thought about is to create a second, slightly higher-level function of discard that basically just takes a write-permission as argument, to nudge callers to use that instead. It felt, however, that it's enough to do that in tauri for now.

I could totally understand that there should rather be the habit of passing write permissions, even though I usually avoid any extras on the function under test, so a separate one would be easier to handle.

@krlvi krlvi merged commit 3afeb0e into gitbutlerapp:master Mar 18, 2025
18 of 19 checks passed
@Byron Byron deleted the discard branch March 18, 2025 07:11
@Byron Byron mentioned this pull request Mar 18, 2025
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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