diff --git a/crates/gitbutler-branch-actions/src/integration.rs b/crates/gitbutler-branch-actions/src/integration.rs index 38796b7b69e..90c163d5faa 100644 --- a/crates/gitbutler-branch-actions/src/integration.rs +++ b/crates/gitbutler-branch-actions/src/integration.rs @@ -1,4 +1,4 @@ -use std::path::PathBuf; +use std::{collections::HashMap, path::PathBuf}; use anyhow::{Context as _, Result, anyhow}; use but_ctx::{Context, access::RepoExclusive}; @@ -170,6 +170,17 @@ pub(crate) fn update_workspace_commit_with_vb_state( None, )?; + // Taken before the checkout below, which writes `.git/index` itself and strips these flags + // from every path whose content it updates. Reading it back afterwards would find them + // already gone. + let per_file_flags = { + let mut index = repo.index()?; + // Only re-reads when the file changed underneath us, which is what picks up a + // `git update-index --skip-worktree` made outside the app. + index.read(false)?; + per_file_index_flags(&index) + }; + let checkout_res = if checkout_new_worktree && prev_head_id.is_some() { let res = but_core::worktree::safe_checkout_from_head( final_commit, @@ -200,6 +211,7 @@ pub(crate) fn update_workspace_commit_with_vb_state( let mut index = repo.index()?; index.read_tree(&repo.find_tree(workspace_tree.to_git2())?)?; + restore_per_file_index_flags(&mut index, &per_file_flags)?; index.write()?; // Everything is written out already, so if we fail here, we do so to surface the error @@ -213,6 +225,62 @@ pub(crate) fn update_workspace_commit_with_vb_state( Ok(final_commit) } +/// The per-file index state that lives only in the index: `skip-worktree`, which tells Git to +/// leave a tracked file's worktree copy alone, and `assume-unchanged`. +/// +/// A tree has nowhere to record either, so rebuilding the index from one drops them for every +/// path. Collecting them first lets [`restore_per_file_index_flags()`] put them back. +fn per_file_index_flags(index: &git2::Index) -> HashMap, (bool, bool)> { + let assume_unchanged_bit = git2::IndexEntryFlag::VALID.bits(); + let skip_worktree_bit = git2::IndexEntryExtendedFlag::SKIP_WORKTREE.bits(); + + index + .iter() + .filter_map(|entry| { + let assume_unchanged = entry.flags & assume_unchanged_bit != 0; + let skip_worktree = entry.flags_extended & skip_worktree_bit != 0; + (assume_unchanged || skip_worktree) + .then(|| (entry.path.clone(), (assume_unchanged, skip_worktree))) + }) + .collect() +} + +/// Put the flags that [`per_file_index_flags()`] collected back onto the paths that are still +/// tracked, leaving paths the rebuild dropped alone. +/// +/// Only the two flag bits are carried over. The rest of each entry belongs to the tree the index +/// was just rebuilt from. +fn restore_per_file_index_flags( + index: &mut git2::Index, + per_file_flags: &HashMap, (bool, bool)>, +) -> Result<()> { + if per_file_flags.is_empty() { + return Ok(()); + } + let assume_unchanged_bit = git2::IndexEntryFlag::VALID.bits(); + let skip_worktree_bit = git2::IndexEntryExtendedFlag::SKIP_WORKTREE.bits(); + + // Collected first because adding entries while iterating the index would invalidate it. + let restored: Vec = index + .iter() + .filter_map(|mut entry| { + let (assume_unchanged, skip_worktree) = per_file_flags.get(&entry.path)?; + if *assume_unchanged { + entry.flags |= assume_unchanged_bit; + } + if *skip_worktree { + entry.flags_extended |= skip_worktree_bit; + } + Some(entry) + }) + .collect(); + + for entry in restored { + index.add(&entry)?; + } + Ok(()) +} + pub fn verify_branch(ctx: &Context, perm: &mut RepoExclusive) -> Result<()> { verify_current_branch_name(ctx) .and_then(verify_head_is_set) diff --git a/crates/gitbutler-branch-actions/tests/branch-actions/workspace_commit.rs b/crates/gitbutler-branch-actions/tests/branch-actions/workspace_commit.rs index 8d7f61f8f96..88eedd2d202 100644 --- a/crates/gitbutler-branch-actions/tests/branch-actions/workspace_commit.rs +++ b/crates/gitbutler-branch-actions/tests/branch-actions/workspace_commit.rs @@ -3,7 +3,8 @@ reason = "VirtualBranchesHandle should be replaced with ctx.workspace_* helpers" )] -use anyhow::Result; +use anyhow::{Context as _, Result}; +use but_testsupport::CommandExt as _; use but_testsupport::visualize_tree; use gitbutler_stack::VirtualBranchesHandle; use gix::prelude::ObjectIdExt; @@ -192,3 +193,111 @@ fn update_workspace_commit_with_diverged_stacks_preserves_target_content() -> Re Ok(()) } + +/// `skip-worktree` tells Git to leave a tracked file's worktree copy alone. Sparse checkouts set +/// it, and it is also set by hand to keep local edits to a checked-in file out of the way. +/// Rebuilding `.git/index` from the workspace tree must not quietly drop it. +/// +/// `shared.txt` is untouched by either stack, so this also shows the flag going missing on a +/// path the workspace commit does not change at all. +#[test] +fn workspace_commit_preserves_skip_worktree() -> Result<()> { + let (ctx, _temp_dir) = command_ctx("adjacent-stacks")?; + let worktree_dir = ctx + .repo + .get()? + .workdir() + .expect("fixture repo has a worktree") + .to_owned(); + + but_testsupport::git_at_dir(&worktree_dir) + .args(["update-index", "--skip-worktree", "shared.txt"]) + .run(); + assert!( + index_flag_is_set(&ctx, "shared.txt", gix::index::entry::Flags::SKIP_WORKTREE)?, + "precondition: the flag is set before the workspace commit is rebuilt" + ); + + gitbutler_branch_actions::update_workspace_commit(&ctx, false)?; + + assert!( + index_flag_is_set(&ctx, "shared.txt", gix::index::entry::Flags::SKIP_WORKTREE)?, + "rebuilding the index from the workspace tree keeps per-file flags" + ); + Ok(()) +} + +fn index_flag_is_set(ctx: &Context, path: &str, flag: gix::index::entry::Flags) -> Result { + let repo = ctx.repo.get()?; + let index = repo.index()?; + let entry = index + .entry_by_path(path.into()) + .with_context(|| format!("{path} should be tracked"))?; + Ok(entry.flags.contains(flag)) +} + +/// `assume-unchanged` is the other per-file flag Git keeps only in the index, and the issue asks +/// for it alongside `skip-worktree`. +/// +/// This one uses `file`, which both stacks modify, because it is only lost when the rebuild +/// actually replaces the entry - libgit2 keeps an entry whose blob and mode are unchanged, flags +/// and all. `skip-worktree` above needs no such setup, being dropped either way. +#[test] +fn workspace_commit_preserves_assume_unchanged() -> Result<()> { + let (ctx, _temp_dir) = command_ctx("adjacent-stacks")?; + let worktree_dir = ctx + .repo + .get()? + .workdir() + .expect("fixture repo has a worktree") + .to_owned(); + + but_testsupport::git_at_dir(&worktree_dir) + .args(["update-index", "--assume-unchanged", "file"]) + .run(); + assert!( + index_flag_is_set(&ctx, "file", gix::index::entry::Flags::ASSUME_VALID)?, + "precondition: the flag is set before the workspace commit is rebuilt" + ); + + gitbutler_branch_actions::update_workspace_commit(&ctx, false)?; + + assert!( + index_flag_is_set(&ctx, "file", gix::index::entry::Flags::ASSUME_VALID)?, + "rebuilding the index from the workspace tree keeps per-file flags" + ); + Ok(()) +} + +/// The `checkout_new_worktree` leg checks the working tree out before the index is rebuilt, and +/// that checkout writes the index itself. The flags have to be taken before it runs, or they are +/// already gone by the time the rebuild sees them. +#[test] +fn workspace_commit_preserves_flags_when_checking_out() -> Result<()> { + let (ctx, _temp_dir) = command_ctx("adjacent-stacks")?; + let worktree_dir = ctx + .repo + .get()? + .workdir() + .expect("fixture repo has a worktree") + .to_owned(); + + but_testsupport::git_at_dir(&worktree_dir) + .args(["update-index", "--skip-worktree", "shared.txt"]) + .run(); + but_testsupport::git_at_dir(&worktree_dir) + .args(["update-index", "--assume-unchanged", "file"]) + .run(); + + gitbutler_branch_actions::update_workspace_commit(&ctx, true)?; + + assert!( + index_flag_is_set(&ctx, "shared.txt", gix::index::entry::Flags::SKIP_WORKTREE)?, + "skip-worktree survives the checkout leg" + ); + assert!( + index_flag_is_set(&ctx, "file", gix::index::entry::Flags::ASSUME_VALID)?, + "assume-unchanged survives the checkout leg" + ); + Ok(()) +}