hostsfile: remove has_content_changed in favor of comparing old and n…

…ew sections

hostsfile/src/lib.rs

@@ -1,11 +1,12 @@
 use std::{
     collections::BTreeMap,
     fmt,
-    fs::{File, OpenOptions},
-    io::{self, BufRead, BufReader, ErrorKind, Read, Write},
+    fs::OpenOptions,
+    io::{self, BufRead, BufReader, ErrorKind, Write},
     net::IpAddr,
     path::{Path, PathBuf},
     result,
+    str::from_utf8,
     time::{SystemTime, UNIX_EPOCH},
 };
 
@@ -172,24 +173,6 @@ impl HostsBuilder {
         Ok(hosts_dir.with_file_name(temp_filename))
     }
 
-    fn has_content_changed(hosts_path: &Path, new_contents: &[u8]) -> io::Result<bool> {
-        let hosts_file = File::open(hosts_path)?;
-
-        if hosts_file.metadata()?.len() != new_contents.len() as u64 {
-            return Ok(true);
-        }
-
-        let hosts_buf = BufReader::new(hosts_file);
-
-        for (b1, b2) in hosts_buf.bytes().zip(new_contents) {
-            if b1.unwrap() != *b2 {
-                return Ok(true);
-            }
-        }
-
-        Ok(false)
-    }
-
     /// Inserts a new section to the specified hosts file.  If there is a section with the same tag
     /// name already, it will be replaced with the new list instead.
     ///
@@ -227,9 +210,36 @@ impl HostsBuilder {
         let begin = lines.iter().position(|line| line.trim() == begin_marker);
         let end = lines.iter().position(|line| line.trim() == end_marker);
 
+        let mut lines_to_insert = vec![];
+        if !self.hostname_map.is_empty() {
+            writeln!(lines_to_insert, "{begin_marker}")?;
+            for (ip, hostnames) in &self.hostname_map {
+                if cfg!(windows) {
+                    // windows only allows one hostname per line
+                    for hostname in hostnames {
+                        writeln!(lines_to_insert, "{ip} {hostname}")?;
+                    }
+                } else {
+                    // assume the same format as Unix
+                    writeln!(lines_to_insert, "{} {}", ip, hostnames.join(" "))?;
+                }
+            }
+            writeln!(lines_to_insert, "{end_marker}")?;
+        }
+
         let insert = match (begin, end) {
             (Some(begin), Some(end)) => {
-                lines.drain(begin..end + 1);
+                // Add '\n's in between lines
+                let old_section: String = lines
+                    .drain(begin..end + 1)
+                    .fold(String::new(), |acc, line| acc + &line + "\n");
+
+                let sections_are_eq = old_section == from_utf8(&lines_to_insert).unwrap();
+
+                if sections_are_eq {
+                    return Ok(false);
+                }
+
                 begin
             },
             (None, None) => {
@@ -254,21 +264,10 @@ impl HostsBuilder {
         for line in &lines[..insert] {
             writeln!(s, "{line}")?;
         }
-        if !self.hostname_map.is_empty() {
-            writeln!(s, "{begin_marker}")?;
-            for (ip, hostnames) in &self.hostname_map {
-                if cfg!(windows) {
-                    // windows only allows one hostname per line
-                    for hostname in hostnames {
-                        writeln!(s, "{ip} {hostname}")?;
-                    }
-                } else {
-                    // assume the same format as Unix
-                    writeln!(s, "{} {}", ip, hostnames.join(" "))?;
-                }
-            }
-            writeln!(s, "{end_marker}")?;
-        }
+
+        // Append hostnames_map section
+        s.append(&mut lines_to_insert);
+
         for line in &lines[insert..] {
             writeln!(s, "{line}")?;
         }
@@ -277,26 +276,21 @@ impl HostsBuilder {
             Err(_) => {
                 Self::write_clobber(hosts_path, &s)?;
                 log::debug!("wrote hosts file with the clobber fallback strategy");
-                Ok(true)
             },
-            Ok(has_written) => {
-                if has_written {
-                    log::debug!("wrote hosts file with the write-and-swap strategy");
-                }
-                Ok(has_written)
+            _ => {
+                log::debug!("wrote hosts file with the write-and-swap strategy");
             },
-        }
+        };
+
+        Ok(true)
     }
 
-    fn write_and_swap(temp_path: &Path, hosts_path: &Path, contents: &[u8]) -> io::Result<bool> {
-        if Self::has_content_changed(hosts_path, contents)? {
-            // Copy the file we plan on modifying so its permissions and metadata are preserved.
-            std::fs::copy(hosts_path, temp_path)?;
-            Self::write_clobber(temp_path, contents)?;
-            std::fs::rename(temp_path, hosts_path)?;
-            return Ok(true);
-        }
-        Ok(false)
+    fn write_and_swap(temp_path: &Path, hosts_path: &Path, contents: &[u8]) -> io::Result<()> {
+        // Copy the file we plan on modifying so its permissions and metadata are preserved.
+        std::fs::copy(hosts_path, temp_path)?;
+        Self::write_clobber(temp_path, contents)?;
+        std::fs::rename(temp_path, hosts_path)?;
+        Ok(())
     }
 
     fn write_clobber(hosts_path: &Path, contents: &[u8]) -> io::Result<()> {
@@ -335,25 +329,14 @@ mod tests {
         assert!(HostsBuilder::get_temp_path(hosts_path).is_err());
     }
 
-    #[test]
-    fn test_has_content_changed() {
-        let (mut temp_file, temp_path) = tempfile::NamedTempFile::new().unwrap().into_parts();
-        temp_file.write_all(b"preexisting\ncontent").unwrap();
-
-        let new_content = b"preexisting\ncontent\nnew\ncontent";
-        assert!(HostsBuilder::has_content_changed(&temp_path, new_content).unwrap());
-
-        let same_content = b"preexisting\ncontent";
-        assert!(!HostsBuilder::has_content_changed(&temp_path, same_content).unwrap());
-    }
-
     #[test]
     fn test_write() {
         let (mut temp_file, temp_path) = tempfile::NamedTempFile::new().unwrap().into_parts();
         temp_file.write_all(b"preexisting\ncontent").unwrap();
         let mut builder = HostsBuilder::new("foo");
         builder.add_hostname([1, 1, 1, 1].into(), "whatever");
-        builder.write_to(&temp_path).unwrap();
+        assert!(builder.write_to(&temp_path).unwrap());
+        assert!(!builder.write_to(&temp_path).unwrap());
 
         let contents = std::fs::read_to_string(&temp_path).unwrap();
         println!("contents: {contents}");