fix: file::MutableMultiValue escapes input values and maintains key separator specific whitespace. (#331)

Author: Byron

git-config/src/file/mutable/mod.rs

@@ -1,2 +1,108 @@
+use crate::file::SectionBody;
+use crate::parse::Event;
+use bstr::{BStr, BString};
+use bstr::{ByteSlice, ByteVec};
+use std::borrow::Cow;
+
+fn escape_value(value: &BStr) -> BString {
+    let starts_with_whitespace = value.get(0).map_or(false, |b| b.is_ascii_whitespace());
+    let ends_with_whitespace = value
+        .get(value.len().saturating_sub(1))
+        .map_or(false, |b| b.is_ascii_whitespace());
+    let contains_comment_indicators = value.find_byteset(b";#").is_some();
+    let quote = starts_with_whitespace || ends_with_whitespace || contains_comment_indicators;
+
+    let mut buf: BString = Vec::with_capacity(value.len()).into();
+    if quote {
+        buf.push(b'"');
+    }
+
+    for b in value.iter().copied() {
+        match b {
+            b'\n' => buf.push_str("\\n"),
+            b'\t' => buf.push_str("\\t"),
+            b'"' => buf.push_str("\\\""),
+            b'\\' => buf.push_str("\\\\"),
+            _ => buf.push(b),
+        }
+    }
+
+    if quote {
+        buf.push(b'"');
+    }
+    buf
+}
+
+#[derive(PartialEq, Eq, Hash, PartialOrd, Ord, Debug)]
+struct Whitespace<'a> {
+    pre_key: Option<Cow<'a, BStr>>,
+    pre_sep: Option<Cow<'a, BStr>>,
+    post_sep: Option<Cow<'a, BStr>>,
+}
+
+impl Default for Whitespace<'_> {
+    fn default() -> Self {
+        Whitespace {
+            pre_key: Some(b"\t".as_bstr().into()),
+            pre_sep: Some(b" ".as_bstr().into()),
+            post_sep: Some(b" ".as_bstr().into()),
+        }
+    }
+}
+
+impl<'a> Whitespace<'a> {
+    fn key_value_separators(&self) -> Vec<Event<'a>> {
+        let mut out = Vec::with_capacity(3);
+        if let Some(ws) = &self.pre_sep {
+            out.push(Event::Whitespace(ws.clone()));
+        }
+        out.push(Event::KeyValueSeparator);
+        if let Some(ws) = &self.post_sep {
+            out.push(Event::Whitespace(ws.clone()));
+        }
+        out
+    }
+}
+
+impl<'a> From<&SectionBody<'a>> for Whitespace<'a> {
+    fn from(s: &SectionBody<'a>) -> Self {
+        let key_pos =
+            s.0.iter()
+                .enumerate()
+                .find_map(|(idx, e)| matches!(e, Event::SectionKey(_)).then(|| idx));
+        key_pos
+            .map(|key_pos| {
+                let pre_key = s.0[..key_pos].iter().rev().next().and_then(|e| match e {
+                    Event::Whitespace(s) => Some(s.clone()),
+                    _ => None,
+                });
+                let from_key = &s.0[key_pos..];
+                let (pre_sep, post_sep) = from_key
+                    .iter()
+                    .enumerate()
+                    .find_map(|(idx, e)| matches!(e, Event::KeyValueSeparator).then(|| idx))
+                    .map(|sep_pos| {
+                        (
+                            from_key.get(sep_pos - 1).and_then(|e| match e {
+                                Event::Whitespace(ws) => Some(ws.clone()),
+                                _ => None,
+                            }),
+                            from_key.get(sep_pos + 1).and_then(|e| match e {
+                                Event::Whitespace(ws) => Some(ws.clone()),
+                                _ => None,
+                            }),
+                        )
+                    })
+                    .unwrap_or_default();
+                Whitespace {
+                    pre_key,
+                    pre_sep,
+                    post_sep,
+                }
+            })
+            .unwrap_or_default()
+    }
+}
+
 pub(crate) mod section;
 pub(crate) mod value;

git-config/src/file/mutable/section.rs

@@ -4,8 +4,9 @@ use std::{
     ops::{Deref, Range},
 };
 
-use bstr::{BStr, BString, ByteSlice, ByteVec};
+use bstr::{BStr, BString, ByteVec};
 
+use crate::file::mutable::{escape_value, Whitespace};
 use crate::{
     file::{Index, Size},
     lookup, parse,
@@ -30,7 +31,7 @@ impl<'a, 'event> MutableSection<'a, 'event> {
         }
 
         self.section.0.push(Event::SectionKey(key));
-        self.section.0.extend(self.key_value_separators());
+        self.section.0.extend(self.whitespace.key_value_separators());
         self.section.0.push(Event::Value(escape_value(value.as_ref()).into()));
         if self.implicit_newline {
             self.section.0.push(Event::Newline(BString::from("\n").into()));
@@ -150,7 +151,7 @@ impl<'a, 'event> MutableSection<'a, 'event> {
 // Internal methods that may require exact indices for faster operations.
 impl<'a, 'event> MutableSection<'a, 'event> {
     pub(crate) fn new(section: &'a mut SectionBody<'event>) -> Self {
-        let whitespace = compute_whitespace(section);
+        let whitespace = (&*section).into();
         Self {
             section,
             implicit_newline: true,
@@ -195,7 +196,7 @@ impl<'a, 'event> MutableSection<'a, 'event> {
         self.section.0.insert(index.0, Event::Value(escape_value(value).into()));
         size += 1;
 
-        let sep_events = self.key_value_separators();
+        let sep_events = self.whitespace.key_value_separators();
         size += sep_events.len();
         self.section.0.insert_many(index.0, sep_events.into_iter().rev());
 
@@ -217,102 +218,6 @@ impl<'a, 'event> MutableSection<'a, 'event> {
                 acc
             })
     }
-
-    fn key_value_separators(&self) -> Vec<Event<'event>> {
-        let mut out = Vec::with_capacity(3);
-        if let Some(ws) = &self.whitespace.pre_sep {
-            out.push(Event::Whitespace(ws.clone()));
-        }
-        out.push(Event::KeyValueSeparator);
-        if let Some(ws) = &self.whitespace.post_sep {
-            out.push(Event::Whitespace(ws.clone()));
-        }
-        out
-    }
-}
-
-#[derive(PartialEq, Eq, Hash, PartialOrd, Ord, Debug)]
-struct Whitespace<'a> {
-    pre_key: Option<Cow<'a, BStr>>,
-    pre_sep: Option<Cow<'a, BStr>>,
-    post_sep: Option<Cow<'a, BStr>>,
-}
-
-impl Default for Whitespace<'_> {
-    fn default() -> Self {
-        Whitespace {
-            pre_key: Some(b"\t".as_bstr().into()),
-            pre_sep: Some(b" ".as_bstr().into()),
-            post_sep: Some(b" ".as_bstr().into()),
-        }
-    }
-}
-
-fn compute_whitespace<'a>(s: &mut SectionBody<'a>) -> Whitespace<'a> {
-    let key_pos =
-        s.0.iter()
-            .enumerate()
-            .find_map(|(idx, e)| matches!(e, Event::SectionKey(_)).then(|| idx));
-    key_pos
-        .map(|key_pos| {
-            let pre_key = s.0[..key_pos].iter().rev().next().and_then(|e| match e {
-                Event::Whitespace(s) => Some(s.clone()),
-                _ => None,
-            });
-            let from_key = &s.0[key_pos..];
-            let (pre_sep, post_sep) = from_key
-                .iter()
-                .enumerate()
-                .find_map(|(idx, e)| matches!(e, Event::KeyValueSeparator).then(|| idx))
-                .map(|sep_pos| {
-                    (
-                        from_key.get(sep_pos - 1).and_then(|e| match e {
-                            Event::Whitespace(ws) => Some(ws.clone()),
-                            _ => None,
-                        }),
-                        from_key.get(sep_pos + 1).and_then(|e| match e {
-                            Event::Whitespace(ws) => Some(ws.clone()),
-                            _ => None,
-                        }),
-                    )
-                })
-                .unwrap_or_default();
-            Whitespace {
-                pre_key,
-                pre_sep,
-                post_sep,
-            }
-        })
-        .unwrap_or_default()
-}
-
-fn escape_value(value: &BStr) -> BString {
-    let starts_with_whitespace = value.get(0).map_or(false, |b| b.is_ascii_whitespace());
-    let ends_with_whitespace = value
-        .get(value.len().saturating_sub(1))
-        .map_or(false, |b| b.is_ascii_whitespace());
-    let contains_comment_indicators = value.find_byteset(b";#").is_some();
-    let quote = starts_with_whitespace || ends_with_whitespace || contains_comment_indicators;
-
-    let mut buf: BString = Vec::with_capacity(value.len()).into();
-    if quote {
-        buf.push(b'"');
-    }
-
-    for b in value.iter().copied() {
-        match b {
-            b'\n' => buf.push_str("\\n"),
-            b'\t' => buf.push_str("\\t"),
-            b'"' => buf.push_str("\\\""),
-            b'\\' => buf.push_str("\\\\"),
-            _ => buf.push(b),
-        }
-    }
-
-    if quote {
-        buf.push(b'"');
-    }
-    buf
 }
 
 impl<'event> Deref for MutableSection<'_, 'event> {

git-config/src/file/mutable/value.rs

@@ -3,6 +3,7 @@ use std::{borrow::Cow, collections::HashMap, ops::DerefMut};
 use bstr::{BStr, BString, ByteVec};
 
 use crate::file::mutable::section::{MutableSection, SectionBody};
+use crate::file::mutable::{escape_value, Whitespace};
 use crate::{
     file::{Index, SectionBodyId, Size},
     lookup,
@@ -249,12 +250,14 @@ impl<'borrow, 'lookup, 'event> MutableMultiValue<'borrow, 'lookup, 'event> {
         value: Cow<'a, BStr>,
     ) {
         let (offset, size) = MutableMultiValue::index_and_size(offsets, section_id, offset_index);
+        let whitespace: Whitespace<'_> = (&*section).into();
         let section = section.as_mut();
         section.drain(offset..offset + size);
 
-        MutableMultiValue::set_offset(offsets, section_id, offset_index, 3);
-        section.insert(offset, Event::Value(value));
-        section.insert(offset, Event::KeyValueSeparator);
+        let key_sep_events = whitespace.key_value_separators();
+        MutableMultiValue::set_offset(offsets, section_id, offset_index, 2 + key_sep_events.len());
+        section.insert(offset, Event::Value(escape_value(value.as_ref()).into()));
+        section.insert_many(offset, key_sep_events.into_iter().rev());
         section.insert(offset, Event::SectionKey(key.to_owned()));
     }
 

git-config/tests/file/mutable/multi_value.rs

@@ -46,16 +46,35 @@ mod access {
 }
 
 mod set {
+    use crate::file::cow_str;
     use crate::file::mutable::multi_value::init_config;
 
+    #[test]
+    fn values_are_escaped() -> crate::Result {
+        for value in ["a b", " a b", "a b\t", ";c", "#c", "a\nb\n\tc"] {
+            let mut config = init_config();
+            let mut values = config.raw_values_mut("core", None, "a")?;
+            values.set_values_all(value.into());
+
+            let config_str = config.to_string();
+            let config: git_config::File = config_str.parse()?;
+            assert_eq!(
+                config.raw_values("core", None, "a")?,
+                vec![cow_str(value), cow_str(value), cow_str(value)],
+                "{config_str:?}"
+            );
+        }
+        Ok(())
+    }
+
     #[test]
     fn single_at_start() -> crate::Result {
         let mut config = init_config();
         let mut values = config.raw_values_mut("core", None, "a")?;
         values.set_string(0, "Hello".into());
         assert_eq!(
             config.to_string(),
-            "[core]\n    a=Hello\n    [core]\n        a=d\n        a=f"
+            "[core]\n    a = Hello\n    [core]\n        a =d\n        a= f"
         );
         Ok(())
     }
@@ -67,7 +86,7 @@ mod set {
         values.set_string(2, "Hello".into());
         assert_eq!(
             config.to_string(),
-            "[core]\n    a=b\"100\"\n    [core]\n        a=d\n        a=Hello"
+            "[core]\n    a = b\"100\"\n    [core]\n        a =d\n        a= Hello"
         );
         Ok(())
     }
@@ -79,7 +98,7 @@ mod set {
         values.set_owned_values_all("Hello");
         assert_eq!(
             config.to_string(),
-            "[core]\n    a=Hello\n    [core]\n        a=Hello\n        a=Hello"
+            "[core]\n    a = Hello\n    [core]\n        a= Hello\n        a =Hello"
         );
         Ok(())
     }
@@ -89,7 +108,10 @@ mod set {
         let mut config = init_config();
         let mut values = config.raw_values_mut("core", None, "a")?;
         values.set_owned_values_all("");
-        assert_eq!(config.to_string(), "[core]\n    a=\n    [core]\n        a=\n        a=");
+        assert_eq!(
+            config.to_string(),
+            "[core]\n    a = \n    [core]\n        a= \n        a ="
+        );
         Ok(())
     }
 }
@@ -103,12 +125,15 @@ mod delete {
         {
             let mut values = config.raw_values_mut("core", None, "a")?;
             values.delete(0);
-            assert_eq!(config.to_string(), "[core]\n    \n    [core]\n        a=d\n        a=f",);
+            assert_eq!(
+                config.to_string(),
+                "[core]\n    \n    [core]\n        a =d\n        a= f",
+            );
         }
 
         let mut values = config.raw_values_mut("core", None, "a")?;
         values.delete(1);
-        assert_eq!(config.to_string(), "[core]\n    \n    [core]\n        a=d\n        ",);
+        assert_eq!(config.to_string(), "[core]\n    \n    [core]\n        a =d\n        ");
         Ok(())
     }
 
@@ -118,17 +143,17 @@ mod delete {
         let mut values = config.raw_values_mut("core", None, "a")?;
         values.delete_all();
         assert!(values.get().is_err());
-        assert_eq!(config.to_string(), "[core]\n    \n    [core]\n        \n        ",);
+        assert_eq!(config.to_string(), "[core]\n    \n    [core]\n        \n        ");
         Ok(())
     }
 }
 
 fn init_config() -> git_config::File<'static> {
     r#"[core]
-    a=b"100"
+    a = b"100"
     [core]
-        a=d
-        a=f"#
+        a =d
+        a= f"#
         .parse()
         .unwrap()
 }