Merge pull request #1835 from GitoxideLabs/fixes

various fixes

Author: Byron

gix-config/src/file/access/mutate.rs

@@ -86,10 +86,8 @@ impl<'event> File<'event> {
             .section_ids_by_name_and_subname(name.as_ref(), subsection_name)
             .ok()
             .and_then(|it| {
-                it.rev().find(|id| {
-                    let s = &self.sections[id];
-                    filter(s.meta())
-                })
+                it.rev()
+                    .find(|id| self.sections.get(id).is_some_and(|s| filter(s.meta())))
             }) {
             Some(id) => {
                 let nl = self.detect_newline_style_smallvec();
@@ -305,7 +303,7 @@ impl<'event> File<'event> {
             .section_ids_by_name_and_subname(name, subsection_name)
             .ok()?
             .rev()
-            .find(|id| filter(self.sections.get(id).expect("each id has a section").meta()))?;
+            .find(|id| self.sections.get(id).is_some_and(|section| filter(section.meta())))?;
         self.section_order.remove(
             self.section_order
                 .iter()

gix-config/tests/config/file/access/mutate.rs

@@ -25,25 +25,59 @@ mod remove_section {
     }
 
     #[test]
-    fn removal_is_complete_and_sections_can_be_readded() {
+    fn removal_is_complete_and_sections_can_be_read() {
         let mut file = gix_config::File::try_from("[core] \na = b\nb=c\n\n[core \"name\"]\nd = 1\ne = 2").unwrap();
         assert_eq!(file.sections().count(), 2);
 
         let removed = file.remove_section("core", None).expect("removed correct section");
         assert_eq!(removed.header().name(), "core");
         assert_eq!(removed.header().subsection_name(), None);
         assert_eq!(file.sections().count(), 1);
+        assert_eq!(file.remove_section("core", None), None, "it's OK to try again");
 
         let removed = file.remove_section("core", Some("name".into())).expect("found");
         assert_eq!(removed.header().name(), "core");
         assert_eq!(removed.header().subsection_name().expect("present"), "name");
         assert_eq!(file.sections().count(), 0);
+        assert_eq!(file.remove_section("core", Some("name".into())), None);
 
         file.section_mut_or_create_new("core", None).expect("creation succeeds");
         file.section_mut_or_create_new("core", Some("name".into()))
             .expect("creation succeeds");
     }
 }
+mod remove_section_filter {
+    #[test]
+    fn removal_of_section_is_complete() {
+        let mut file = gix_config::File::try_from("[core] \na = b\nb=c\n\n[core \"name\"]\nd = 1\ne = 2").unwrap();
+        assert_eq!(file.sections().count(), 2);
+
+        let removed = file
+            .remove_section_filter("core", None, |_| true)
+            .expect("removed correct section");
+        assert_eq!(removed.header().name(), "core");
+        assert_eq!(removed.header().subsection_name(), None);
+        assert_eq!(file.sections().count(), 1);
+        let removed = file
+            .remove_section_filter("core", Some("name".into()), |_| true)
+            .expect("found");
+        assert_eq!(removed.header().name(), "core");
+        assert_eq!(removed.header().subsection_name().expect("present"), "name");
+        assert_eq!(file.sections().count(), 0);
+
+        assert_eq!(
+            file.remove_section_filter("core", None, |_| true),
+            None,
+            "it's OK to try again"
+        );
+        assert_eq!(file.remove_section_filter("core", Some("name".into()), |_| true), None);
+
+        file.section_mut_or_create_new("core", None).expect("creation succeeds");
+        file.section_mut_or_create_new("core", Some("name".into()))
+            .expect("creation succeeds");
+    }
+}
+
 mod rename_section {
     use std::borrow::Cow;
 

gix/src/repository/config/mod.rs

@@ -12,8 +12,8 @@ impl crate::Repository {
     /// Return a mutable snapshot of the configuration as seen upon opening the repository, starting a transaction.
     /// When the returned instance is dropped, it is applied in full, even if the reason for the drop is an error.
     ///
-    /// Note that changes to the configuration are in-memory only and are observed only the this instance
-    /// of the [`Repository`][crate::Repository].
+    /// Note that changes to the configuration are in-memory only and are observed only this instance
+    /// of the [`Repository`](crate::Repository).
     pub fn config_snapshot_mut(&mut self) -> config::SnapshotMut<'_> {
         let config = self.config.resolved.as_ref().clone();
         config::SnapshotMut {

gix/src/repository/object.rs

@@ -254,7 +254,7 @@ impl crate::Repository {
         self.tag_reference(name, tag_id, constraint).map_err(Into::into)
     }
 
-    /// Similar to [`commit(…)`][crate::Repository::commit()], but allows to create the commit with `committer` and `author` specified.
+    /// Similar to [`commit(…)`](crate::Repository::commit()), but allows to create the commit with `committer` and `author` specified.
     ///
     /// This forces setting the commit time and author time by hand. Note that typically, committer and author are the same.
     pub fn commit_as<'a, 'c, Name, E>(
@@ -307,28 +307,35 @@ impl crate::Repository {
         };
 
         let commit_id = self.write_object(&commit)?;
-        self.edit_reference(RefEdit {
-            change: Change::Update {
-                log: LogChange {
-                    mode: RefLog::AndReference,
-                    force_create_reflog: false,
-                    message: crate::reference::log::message("commit", commit.message.as_ref(), commit.parents.len()),
-                },
-                expected: match commit.parents.first().map(|p| Target::Object(*p)) {
-                    Some(previous) => {
-                        if reference.as_bstr() == "HEAD" {
-                            PreviousValue::MustExistAndMatch(previous)
-                        } else {
-                            PreviousValue::ExistingMustMatch(previous)
+        self.edit_references_as(
+            Some(RefEdit {
+                change: Change::Update {
+                    log: LogChange {
+                        mode: RefLog::AndReference,
+                        force_create_reflog: false,
+                        message: crate::reference::log::message(
+                            "commit",
+                            commit.message.as_ref(),
+                            commit.parents.len(),
+                        ),
+                    },
+                    expected: match commit.parents.first().map(|p| Target::Object(*p)) {
+                        Some(previous) => {
+                            if reference.as_bstr() == "HEAD" {
+                                PreviousValue::MustExistAndMatch(previous)
+                            } else {
+                                PreviousValue::ExistingMustMatch(previous)
+                            }
                         }
-                    }
-                    None => PreviousValue::MustNotExist,
+                        None => PreviousValue::MustNotExist,
+                    },
+                    new: Target::Object(commit_id.inner),
                 },
-                new: Target::Object(commit_id.inner),
-            },
-            name: reference,
-            deref: true,
-        })?;
+                name: reference,
+                deref: true,
+            }),
+            Some(committer),
+        )?;
         Ok(commit_id)
     }
 

gix/src/repository/reference.rs

@@ -10,7 +10,7 @@ use crate::{bstr::BString, ext::ReferenceExt, reference, Reference};
 impl crate::Repository {
     /// Create a lightweight tag with given `name` (and without `refs/tags/` prefix) pointing to the given `target`, and return it as reference.
     ///
-    /// It will be created with `constraint` which is most commonly to [only create it][PreviousValue::MustNotExist]
+    /// It will be created with `constraint` which is most commonly to [only create it](PreviousValue::MustNotExist)
     /// or to [force overwriting a possibly existing tag](PreviousValue::Any).
     pub fn tag_reference(
         &self,
@@ -135,25 +135,37 @@ impl crate::Repository {
 
     /// Edit one or more references as described by their `edits`.
     /// Note that one can set the committer name for use in the ref-log by temporarily
-    /// [overriding the git-config][crate::Repository::config_snapshot_mut()].
+    /// [overriding the git-config](crate::Repository::config_snapshot_mut()), or use
+    /// [`edit_references_as(committer)`](Self::edit_references_as()) for convenience.
     ///
     /// Returns all reference edits, which might be more than where provided due the splitting of symbolic references, and
     /// whose previous (_old_) values are the ones seen on in storage after the reference was locked.
     pub fn edit_references(
         &self,
         edits: impl IntoIterator<Item = RefEdit>,
+    ) -> Result<Vec<RefEdit>, reference::edit::Error> {
+        self.edit_references_as(edits, self.committer().transpose()?)
+    }
+
+    /// A way to apply reference `edits` similar to [edit_references(…)](Self::edit_references()), but set a specific
+    /// `commiter` for use in the reflog. It can be `None` if it's the purpose `edits` are configured to not update the
+    /// reference log, or cause a failure otherwise.
+    pub fn edit_references_as(
+        &self,
+        edits: impl IntoIterator<Item = RefEdit>,
+        committer: Option<gix_actor::SignatureRef<'_>>,
     ) -> Result<Vec<RefEdit>, reference::edit::Error> {
         let (file_lock_fail, packed_refs_lock_fail) = self.config.lock_timeout()?;
         self.refs
             .transaction()
             .prepare(edits, file_lock_fail, packed_refs_lock_fail)?
-            .commit(self.committer().transpose()?)
+            .commit(committer)
             .map_err(Into::into)
     }
 
     /// Return the repository head, an abstraction to help dealing with the `HEAD` reference.
     ///
-    /// The `HEAD` reference can be in various states, for more information, the documentation of [`Head`][crate::Head].
+    /// The `HEAD` reference can be in various states, for more information, the documentation of [`Head`](crate::Head).
     pub fn head(&self) -> Result<crate::Head<'_>, reference::find::existing::Error> {
         let head = self.find_reference("HEAD")?;
         Ok(match head.inner.target {
@@ -184,7 +196,7 @@ impl crate::Repository {
 
     /// Return the name to the symbolic reference `HEAD` points to, or `None` if the head is detached.
     ///
-    /// The difference to [`head_ref()`][Self::head_ref()] is that the latter requires the reference to exist,
+    /// The difference to [`head_ref()`](Self::head_ref()) is that the latter requires the reference to exist,
     /// whereas here we merely return a the name of the possibly unborn reference.
     pub fn head_name(&self) -> Result<Option<FullName>, reference::find::existing::Error> {
         Ok(self.head()?.referent_name().map(std::borrow::ToOwned::to_owned))
@@ -228,7 +240,7 @@ impl crate::Repository {
     /// Find the reference with the given partial or full `name`, like `main`, `HEAD`, `heads/branch` or `origin/other`,
     /// or return an error if it wasn't found.
     ///
-    /// Consider [`try_find_reference(…)`][crate::Repository::try_find_reference()] if the reference might not exist
+    /// Consider [`try_find_reference(…)`](crate::Repository::try_find_reference()) if the reference might not exist
     /// without that being considered an error.
     pub fn find_reference<'a, Name, E>(&self, name: Name) -> Result<Reference<'_>, reference::find::existing::Error>
     where
@@ -249,7 +261,7 @@ impl crate::Repository {
 
     /// Return a platform for iterating references.
     ///
-    /// Common kinds of iteration are [all][crate::reference::iter::Platform::all()] or [prefixed][crate::reference::iter::Platform::prefixed()]
+    /// Common kinds of iteration are [all](crate::reference::iter::Platform::all()) or [prefixed](crate::reference::iter::Platform::prefixed())
     /// references.
     pub fn references(&self) -> Result<reference::iter::Platform<'_>, reference::iter::Error> {
         Ok(reference::iter::Platform {
@@ -261,7 +273,7 @@ impl crate::Repository {
     /// Try to find the reference named `name`, like `main`, `heads/branch`, `HEAD` or `origin/other`, and return it.
     ///
     /// Otherwise return `None` if the reference wasn't found.
-    /// If the reference is expected to exist, use [`find_reference()`][crate::Repository::find_reference()].
+    /// If the reference is expected to exist, use [`find_reference()`](crate::Repository::find_reference()).
     pub fn try_find_reference<'a, Name, E>(&self, name: Name) -> Result<Option<Reference<'_>>, reference::find::Error>
     where
         Name: TryInto<&'a PartialNameRef, Error = E>,

gix/tests/gix/repository/object.rs

@@ -536,16 +536,14 @@ mod tag {
 mod commit_as {
     use gix_testtools::tempfile;
 
-    use crate::util::restricted_and_git;
-
     #[test]
     fn specify_committer_and_author() -> crate::Result {
         let tmp = tempfile::tempdir()?;
         let repo = gix::ThreadSafeRepository::init_opts(
             &tmp,
             gix::create::Kind::WithWorktree,
             Default::default(),
-            restricted_and_git(),
+            gix::open::Options::isolated(),
         )?
         .to_thread_local();
         let empty_tree = repo.empty_tree();