diff --git a/gitoxide-core/src/repository/status.rs b/gitoxide-core/src/repository/status.rs index 75bf33f9343..ab2812a6407 100644 --- a/gitoxide-core/src/repository/status.rs +++ b/gitoxide-core/src/repository/status.rs @@ -135,11 +135,6 @@ pub fn show( )?; continue; } - gix::diff::index::Change::Unmerged { .. } => { - // Unmerged entries from the worktree-index are displayed as part of the index-worktree comparison. - // Here we have nothing to do with them and can ignore. - continue; - } }; writeln!( out, diff --git a/gix-diff/src/index/change.rs b/gix-diff/src/index/change.rs index da5d98d1f4e..5ce52990b8c 100644 --- a/gix-diff/src/index/change.rs +++ b/gix-diff/src/index/change.rs @@ -70,19 +70,6 @@ impl ChangeRef<'_, '_> { id: Cow::Owned(id.into_owned()), copy, }, - ChangeRef::Unmerged { - location, - stage, - index, - entry_mode, - id, - } => ChangeRef::Unmerged { - location: Cow::Owned(location.into_owned()), - stage, - index, - entry_mode, - id: Cow::Owned(id.into_owned()), - }, } } } @@ -120,13 +107,6 @@ impl ChangeRef<'_, '_> { entry_mode, id, .. - } - | ChangeRef::Unmerged { - location, - index, - entry_mode, - id, - .. } => (location.as_ref(), *index, *entry_mode, id), } } @@ -138,7 +118,7 @@ impl rewrites::tracker::Change for ChangeRef<'_, '_> { ChangeRef::Addition { id, .. } | ChangeRef::Deletion { id, .. } | ChangeRef::Modification { id, .. } => { id.as_ref() } - ChangeRef::Rewrite { .. } | ChangeRef::Unmerged { .. } => { + ChangeRef::Rewrite { .. } => { unreachable!("BUG") } } @@ -156,9 +136,6 @@ impl rewrites::tracker::Change for ChangeRef<'_, '_> { ChangeRef::Rewrite { .. } => { unreachable!("BUG: rewrites can't be determined ahead of time") } - ChangeRef::Unmerged { .. } => { - unreachable!("BUG: unmerged don't participate in rename tracking") - } } } @@ -167,8 +144,7 @@ impl rewrites::tracker::Change for ChangeRef<'_, '_> { ChangeRef::Addition { entry_mode, .. } | ChangeRef::Deletion { entry_mode, .. } | ChangeRef::Modification { entry_mode, .. } - | ChangeRef::Rewrite { entry_mode, .. } - | ChangeRef::Unmerged { entry_mode, .. } => { + | ChangeRef::Rewrite { entry_mode, .. } => { entry_mode .to_tree_entry_mode() // Default is for the impossible case - just don't let it participate in rename tracking. @@ -182,8 +158,7 @@ impl rewrites::tracker::Change for ChangeRef<'_, '_> { ChangeRef::Addition { id, entry_mode, .. } | ChangeRef::Deletion { id, entry_mode, .. } | ChangeRef::Modification { id, entry_mode, .. } - | ChangeRef::Rewrite { id, entry_mode, .. } - | ChangeRef::Unmerged { id, entry_mode, .. } => { + | ChangeRef::Rewrite { id, entry_mode, .. } => { ( id, entry_mode diff --git a/gix-diff/src/index/function.rs b/gix-diff/src/index/function.rs index bbf2a9a3140..6a4f63bc548 100644 --- a/gix-diff/src/index/function.rs +++ b/gix-diff/src/index/function.rs @@ -1,6 +1,6 @@ use super::{Action, ChangeRef, Error, RewriteOptions}; use crate::rewrites; -use bstr::{BStr, BString, ByteSlice}; +use bstr::BStr; use gix_filter::attributes::glob::pattern::Case; use std::borrow::Cow; use std::cell::RefCell; @@ -20,7 +20,8 @@ use std::cmp::Ordering; /// Return the outcome of the rewrite tracker if it was enabled. /// /// Note that only `rhs` may contain unmerged entries, as `rhs` is expected to be the index read from `.git/index`. -/// Unmerged entries are always provided as changes, one stage at a time, up to three stages for *base*, *ours* and *theirs*. +/// Unmerged entries are skipped entirely. +/// /// Conceptually, `rhs` is *ours*, and `lhs` is *theirs*. /// The entries in `lhs` and `rhs` are both expected to be sorted like index entries are typically sorted. /// @@ -74,23 +75,6 @@ where .filter(|(_, path, e)| pattern_matches.borrow_mut()(path, e)), ); - let mut conflicting_paths = Vec::::new(); - let mut cb = move |change: ChangeRef<'lhs, 'rhs>| { - let (location, ..) = change.fields(); - if let ChangeRef::Unmerged { .. } = &change { - if let Err(insert_idx) = conflicting_paths.binary_search_by(|p| p.as_bstr().cmp(location)) { - conflicting_paths.insert(insert_idx, location.to_owned()); - } - cb(change) - } else if conflicting_paths - .binary_search_by(|p| p.as_bstr().cmp(location)) - .is_err() - { - cb(change) - } else { - Ok(Action::Continue) - } - }; let mut resource_cache_storage = None; let mut tracker = rewrite_options.map( |RewriteOptions { @@ -107,15 +91,6 @@ where loop { match (lhs_storage, rhs_storage) { (Some(lhs), Some(rhs)) => { - match emit_unmerged_ignore_intent_to_add(rhs, &mut cb)? { - None => {} - Some(Action::Cancel) => return Ok(None), - Some(Action::Continue) => { - rhs_storage = rhs_iter.next(); - continue; - } - }; - let (lhs_idx, lhs_path, lhs_entry) = lhs; let (rhs_idx, rhs_path, rhs_entry) = rhs; match lhs_path.cmp(rhs_path) { @@ -126,6 +101,11 @@ where Action::Cancel => return Ok(None), }, Ordering::Equal => { + if ignore_unmerged_and_intent_to_add(rhs) { + rhs_storage = rhs_iter.next(); + lhs_storage = lhs_iter.next(); + continue; + } if lhs_entry.id != rhs_entry.id || lhs_entry.mode != rhs_entry.mode { let change = ChangeRef::Modification { location: Cow::Borrowed(rhs_path), @@ -274,8 +254,8 @@ fn emit_addition<'rhs, 'lhs: 'rhs, E>( where E: Into>, { - if let Some(action) = emit_unmerged_ignore_intent_to_add((idx, path, entry), &mut cb)? { - return Ok(action); + if ignore_unmerged_and_intent_to_add((idx, path, entry)) { + return Ok(Action::Continue); } let change = ChangeRef::Addition { @@ -296,29 +276,9 @@ where cb(change).map_err(|err| Error::Callback(err.into())) } -fn emit_unmerged_ignore_intent_to_add<'rhs, 'lhs: 'rhs, E>( - (idx, path, entry): (usize, &'rhs BStr, &'rhs gix_index::Entry), - cb: &mut impl FnMut(ChangeRef<'lhs, 'rhs>) -> Result, -) -> Result, Error> -where - E: Into>, -{ - if entry.flags.contains(gix_index::entry::Flags::INTENT_TO_ADD) { - return Ok(Some(Action::Continue)); - } +fn ignore_unmerged_and_intent_to_add<'rhs, 'lhs: 'rhs>( + (_idx, _path, entry): (usize, &'rhs BStr, &'rhs gix_index::Entry), +) -> bool { let stage = entry.stage(); - if stage == gix_index::entry::Stage::Unconflicted { - return Ok(None); - } - - Ok(Some( - cb(ChangeRef::Unmerged { - location: Cow::Borrowed(path), - stage, - index: idx, - entry_mode: entry.mode, - id: Cow::Borrowed(entry.id.as_ref()), - }) - .map_err(|err| Error::Callback(err.into()))?, - )) + entry.flags.contains(gix_index::entry::Flags::INTENT_TO_ADD) || stage != gix_index::entry::Stage::Unconflicted } diff --git a/gix-diff/src/index/mod.rs b/gix-diff/src/index/mod.rs index 0a66b3ca1e7..67dbe07a812 100644 --- a/gix-diff/src/index/mod.rs +++ b/gix-diff/src/index/mod.rs @@ -43,8 +43,6 @@ where } /// Identify a change that would have to be applied to `lhs` to obtain `rhs`, as provided in [`index()`](crate::index()). -/// -/// Note that all variants are unconflicted entries, unless it's the [`Self::Unmerged`] one. #[derive(Clone, Debug, PartialEq, Eq)] pub enum ChangeRef<'lhs, 'rhs> { /// An entry was added to `rhs`. @@ -116,22 +114,6 @@ pub enum ChangeRef<'lhs, 'rhs> { /// or similar content. copy: bool, }, - /// One of up to three unmerged entries that are provided in order, one for each stage, ordered - /// by `location` and `stage`. - /// - /// Unmerged entries also don't participate in rename tracking, and they are never present in `lhs`. - Unmerged { - /// The current location of the entry in `rhs`. - location: Cow<'rhs, BStr>, - /// The stage of the entry, either *base*, *ours*, or *theirs*. - stage: gix_index::entry::Stage, - /// The index into the entries array of `rhs` for full access. - index: usize, - /// The mode of the entry in `rhs`. - entry_mode: gix_index::entry::Mode, - /// The object id of the entry in `rhs`. - id: Cow<'rhs, gix_hash::oid>, - }, } /// The fully-owned version of [`ChangeRef`]. diff --git a/gix-diff/tests/diff/index.rs b/gix-diff/tests/diff/index.rs index 7e936a309cf..9b6810698e7 100644 --- a/gix-diff/tests/diff/index.rs +++ b/gix-diff/tests/diff/index.rs @@ -1191,56 +1191,14 @@ fn unmerged_entries_and_intent_to_add() -> crate::Result { }), )?; - // each unmerged entry is emitted separately, and no entry is emitted for - // paths that are mentioned there. Intent-to-add is transparent. + // Intent-to-add is transparent. And unmerged entries aren't emitted either, along with + // their sibling paths. // All that with rename tracking… - insta::assert_debug_snapshot!(changes.into_iter().collect::>(), @r#" - [ - Unmerged { - location: "src/plumbing-renamed/main.rs", - stage: Base, - index: 2, - entry_mode: Mode( - FILE, - ), - id: Sha1(d00491fd7e5bb6fa28c517a0bb32b8b506539d4d), - }, - Unmerged { - location: "src/plumbing-renamed/main.rs", - stage: Ours, - index: 3, - entry_mode: Mode( - FILE, - ), - id: Sha1(d00491fd7e5bb6fa28c517a0bb32b8b506539d4d), - }, - ] - "#); + insta::assert_debug_snapshot!(changes.into_iter().collect::>(), @"[]"); let changes = collect_changes_no_renames("r4-dir-rename-non-identity", ".git/index")?; // …or without - insta::assert_debug_snapshot!(changes.into_iter().collect::>(), @r#" - [ - Unmerged { - location: "src/plumbing-renamed/main.rs", - stage: Base, - index: 2, - entry_mode: Mode( - FILE, - ), - id: Sha1(d00491fd7e5bb6fa28c517a0bb32b8b506539d4d), - }, - Unmerged { - location: "src/plumbing-renamed/main.rs", - stage: Ours, - index: 3, - entry_mode: Mode( - FILE, - ), - id: Sha1(d00491fd7e5bb6fa28c517a0bb32b8b506539d4d), - }, - ] - "#); + insta::assert_debug_snapshot!(changes.into_iter().collect::>(), @"[]"); let (index, _, _, _, _) = repo_with_indices(".git/index", ".git/index", None)?; assert_eq!( diff --git a/gix-filter/src/pipeline/convert.rs b/gix-filter/src/pipeline/convert.rs index 049ccdfbbc7..37c0071a6d2 100644 --- a/gix-filter/src/pipeline/convert.rs +++ b/gix-filter/src/pipeline/convert.rs @@ -76,7 +76,7 @@ impl Pipeline { where R: std::io::Read, { - let bstr_path = gix_path::into_bstr(rela_path); + let bstr_rela_path = gix_path::to_unix_separators_on_windows(gix_path::into_bstr(rela_path)); let Configuration { driver, digest, @@ -84,7 +84,7 @@ impl Pipeline { encoding, apply_ident_filter, } = Configuration::at_path( - bstr_path.as_ref(), + bstr_rela_path.as_ref(), &self.options.drivers, &mut self.attrs, attributes, @@ -109,7 +109,7 @@ impl Pipeline { driver, &mut src, driver::Operation::Clean, - self.context.with_path(bstr_path.as_ref()), + self.context.with_path(bstr_rela_path.as_ref()), )? { if !apply_ident_filter && encoding.is_none() && !would_convert_eol { // Note that this is not typically a benefit in terms of saving memory as most filters diff --git a/gix-object/src/object/convert.rs b/gix-object/src/object/convert.rs index 99046b6e2d3..73c77dfc191 100644 --- a/gix-object/src/object/convert.rs +++ b/gix-object/src/object/convert.rs @@ -78,6 +78,17 @@ impl From> for tree::Entry { } } +impl<'a> From<&'a tree::Entry> for tree::EntryRef<'a> { + fn from(other: &'a tree::Entry) -> tree::EntryRef<'a> { + let tree::Entry { mode, filename, oid } = other; + tree::EntryRef { + mode: *mode, + filename: filename.as_ref(), + oid, + } + } +} + impl From> for Object { fn from(v: ObjectRef<'_>) -> Self { match v { diff --git a/gix-object/tests/object/tree/editor.rs b/gix-object/tests/object/tree/editor.rs index 2565a319cb0..09f76f62cea 100644 --- a/gix-object/tests/object/tree/editor.rs +++ b/gix-object/tests/object/tree/editor.rs @@ -619,6 +619,7 @@ fn from_existing_add() -> crate::Result { let root_tree = find_tree(&odb, root_tree_id)?; odb.access_count_and_clear(); let mut edit = gix_object::tree::Editor::new(root_tree.clone(), &odb, gix_hash::Kind::Sha1); + assert!(edit.get(["bin"]).is_some(), "the root is immediately available"); let actual = edit.write(&mut write).expect("no changes are fine"); assert_eq!(actual, root_tree_id, "it rewrites the same tree"); diff --git a/gix/src/object/tree/editor.rs b/gix/src/object/tree/editor.rs index 48fd57167df..6d8694f8ce1 100644 --- a/gix/src/object/tree/editor.rs +++ b/gix/src/object/tree/editor.rs @@ -177,6 +177,21 @@ impl<'repo> Cursor<'_, 'repo> { pub fn write(&mut self) -> Result, write::Error> { write_cursor(self) } + + /// Obtain the entry at `rela_path` or return `None` if none was found, or the tree wasn't yet written + /// to that point. + /// The root tree is always available. + /// Note that after [writing](Self::write) only the root path remains, all other intermediate trees are removed. + /// The entry can be anything that can be stored in a tree, but may have a null-id if it's a newly + /// inserted tree. Also, ids of trees might not be accurate as they may have been changed in memory. + pub fn get(&self, rela_path: impl ToComponents) -> Option> { + self.inner + .get(rela_path.to_components()) + .map(|entry| crate::object::tree::EntryRef { + inner: entry.into(), + repo: self.repo, + }) + } } /// Operations @@ -242,6 +257,21 @@ impl<'repo> super::Editor<'repo> { pub fn write(&mut self) -> Result, write::Error> { write_cursor(&mut self.to_cursor()) } + + /// Obtain the entry at `rela_path` or return `None` if none was found, or the tree wasn't yet written + /// to that point. + /// The root tree is always available. + /// Note that after [writing](Self::write) only the root path remains, all other intermediate trees are removed. + /// The entry can be anything that can be stored in a tree, but may have a null-id if it's a newly + /// inserted tree. Also, ids of trees might not be accurate as they may have been changed in memory. + pub fn get(&self, rela_path: impl ToComponents) -> Option> { + self.inner + .get(rela_path.to_components()) + .map(|entry| crate::object::tree::EntryRef { + inner: entry.into(), + repo: self.repo, + }) + } } fn write_cursor<'repo>(cursor: &mut Cursor<'_, 'repo>) -> Result, write::Error> { diff --git a/gix/src/repository/object.rs b/gix/src/repository/object.rs index 7c1308c91c8..cbe6c18b6aa 100644 --- a/gix/src/repository/object.rs +++ b/gix/src/repository/object.rs @@ -205,12 +205,10 @@ impl crate::Repository { /// we avoid writing duplicate objects using slow disks that will eventually have to be garbage collected. /// /// If that is prohibitive, use the object database directly. - pub fn write_blob_stream( - &self, - mut bytes: impl std::io::Read + std::io::Seek, - ) -> Result, object::write::Error> { + pub fn write_blob_stream(&self, mut bytes: impl std::io::Read) -> Result, object::write::Error> { let mut buf = self.empty_reusable_buffer(); - std::io::copy(&mut bytes, buf.deref_mut()).expect("write to memory works"); + std::io::copy(&mut bytes, buf.deref_mut()) + .map_err(|err| Box::new(err) as Box)?; self.write_blob_stream_inner(&buf) } diff --git a/gix/src/status/iter/mod.rs b/gix/src/status/iter/mod.rs index 6df82c5f8cd..4b81261befb 100644 --- a/gix/src/status/iter/mod.rs +++ b/gix/src/status/iter/mod.rs @@ -19,6 +19,20 @@ where /// /// * `patterns` /// - Optional patterns to use to limit the paths to look at. If empty, all paths are considered. + /// + /// ### Important: Undefined ordering + /// + /// When compiled with the `parallel` feature, three operations are run at once: + /// + /// * a dirwalk to find untracked and possibly ignored files + /// * an entry-by-entry check to see which of the tracked files changed, and how + /// * a tree-index comparison + /// + /// All of these generate distinct events which may now happen in any order, so consumers + /// that are ordering dependent have to restore their desired order. + /// + /// This isn't feasible to do here as it would mean that returned items would have to be delayed, + /// degrading performance for everyone who isn't order-dependent. #[doc(alias = "diff_index_to_workdir", alias = "git2")] pub fn into_iter( self, @@ -274,12 +288,15 @@ impl Iter { impl Iter { fn maybe_keep_index_change(&mut self, item: Item) -> Option { - let change = match item { + match item { Item::IndexWorktree(index_worktree::Item::Modification { status: EntryStatus::NeedsUpdate(stat), entry_index, .. - }) => (entry_index, ApplyChange::NewStat(stat)), + }) => { + self.index_changes.push((entry_index, ApplyChange::NewStat(stat))); + return None; + } Item::IndexWorktree(index_worktree::Item::Modification { status: EntryStatus::Change(Change::Modification { @@ -288,12 +305,12 @@ impl Iter { }), entry_index, .. - }) if set_entry_stat_size_zero => (entry_index, ApplyChange::SetSizeToZero), - _ => return Some(item), + }) if set_entry_stat_size_zero => { + self.index_changes.push((entry_index, ApplyChange::SetSizeToZero)); + } + _ => {} }; - - self.index_changes.push(change); - None + Some(item) } } diff --git a/gix/tests/fixtures/generated-archives/make_status_repos.tar b/gix/tests/fixtures/generated-archives/make_status_repos.tar index 2666626e624..0fbbc9a60e7 100644 Binary files a/gix/tests/fixtures/generated-archives/make_status_repos.tar and b/gix/tests/fixtures/generated-archives/make_status_repos.tar differ diff --git a/gix/tests/fixtures/make_status_repos.sh b/gix/tests/fixtures/make_status_repos.sh index 3a4c3d535f7..cf8514bd496 100755 --- a/gix/tests/fixtures/make_status_repos.sh +++ b/gix/tests/fixtures/make_status_repos.sh @@ -21,3 +21,12 @@ git init git-mv git mv file renamed ) + +git init racy-git +(cd racy-git + echo hi >file + git add file && git commit -m "init" + + echo ho >file && git add file + echo ha >file +) \ No newline at end of file diff --git a/gix/tests/gix/status.rs b/gix/tests/gix/status.rs index 981bc34a392..c0bb4f8e079 100644 --- a/gix/tests/gix/status.rs +++ b/gix/tests/gix/status.rs @@ -73,6 +73,16 @@ mod into_iter { Ok(()) } + #[test] + fn tree_index_modification_worktree_modification_racy_git() -> crate::Result { + let repo = repo("racy-git")?; + let mut status = repo.status(gix::progress::Discard)?.into_iter(None)?; + let mut items: Vec<_> = status.by_ref().filter_map(Result::ok).collect(); + items.sort_by(|a, b| a.location().cmp(b.location())); + assert_eq!(items.len(), 2, "1 modified in index, the same in worktree"); + Ok(()) + } + #[test] fn error_during_tree_traversal_causes_failure() -> crate::Result { let repo = repo("untracked-only")?;