Merge pull request #73 from MilesCranmer/compat-helpful-error

Improve handling of timestamps and headers, with explicit errors for old rip format

src/lib.rs

@@ -103,12 +103,8 @@ pub fn run(cli: Args, mode: impl util::TestingMode, stream: &mut impl Write) ->
         let gravepath = util::join_absolute(graveyard, dunce::canonicalize(cwd)?);
         writeln!(stream, "{: <19}\tpath", "deletion_time")?;
         for grave in record.seance(&gravepath)? {
-            let parsed_time = chrono::DateTime::parse_from_rfc3339(&grave.time)
-                .expect("Failed to parse time from RFC3339 format")
-                .format("%Y-%m-%dT%H:%M:%S")
-                .to_string();
-            // Get the path separator:
-            writeln!(stream, "{}\t{}", parsed_time, grave.dest.display())?;
+            let formatted_time = grave.format_time_for_display()?;
+            writeln!(stream, "{}\t{}", formatted_time, grave.dest.display())?;
         }
     } else if cli.targets.is_empty() {
         Args::command().print_help()?;

src/record.rs

@@ -1,4 +1,4 @@
-use chrono::Local;
+use chrono::{DateTime, Local};
 use fs4::fs_std::FileExt;
 use std::io::{BufRead, BufReader, Error, ErrorKind, Read, Write};
 use std::path::{Path, PathBuf};
@@ -28,6 +28,45 @@ impl RecordItem {
             dest: PathBuf::from(dest),
         }
     }
+
+    /// Parse the timestamp in this record, which could be in either RFC3339 format (from rip2)
+    /// or the old rip format --- in which case we return a helpful error.
+    fn parse_timestamp(&self) -> Result<DateTime<Local>, Error> {
+        // Try parsing as RFC3339 first
+        if let Ok(dt) = DateTime::parse_from_rfc3339(&self.time) {
+            return Ok(dt.with_timezone(&Local));
+        }
+
+        // Roughly check if it matches the old rip format (e.g., "Sun Dec  1 02:15:56 2024")
+        let is_old_format = self.time.split_whitespace().count() == 5
+            && self
+                .time
+                .chars()
+                .all(|c| c.is_ascii_alphanumeric() || c.is_whitespace() || c == ':');
+        if is_old_format {
+            Err(Error::new(
+                ErrorKind::InvalidData,
+                format!(
+                    "Found timestamp '{}' from old rip format. \
+                    You will need to delete the `.record` file \
+                    and start over with rip2. \
+                    You can see the path with `rip graveyard`.",
+                    self.time
+                ),
+            ))
+        } else {
+            Err(Error::new(
+                ErrorKind::InvalidData,
+                format!("Failed to parse time '{}' as RFC3339 format", self.time),
+            ))
+        }
+    }
+
+    /// Format this record's timestamp for display in the seance output
+    pub fn format_time_for_display(&self) -> Result<String, Error> {
+        self.parse_timestamp()
+            .map(|dt| dt.format("%Y-%m-%dT%H:%M:%S").to_string())
+    }
 }
 
 /// A record of file operations maintained in the graveyard directory
@@ -50,6 +89,8 @@ pub const DEFAULT_FILE_LOCK: bool = false;
 // TODO: Investigate why this is needed. Does Windows not support file locks?
 
 impl<const FILE_LOCK: bool> Record<FILE_LOCK> {
+    const HEADER: &'static str = "Time\tOriginal\tDestination";
+
     pub fn new(graveyard: &Path) -> Record<FILE_LOCK> {
         let path = graveyard.join(RECORD);
         // Create the record file if it doesn't exist
@@ -64,8 +105,7 @@ impl<const FILE_LOCK: bool> Record<FILE_LOCK> {
             if FILE_LOCK {
                 record_file.lock_exclusive().unwrap();
             }
-            record_file
-                .write_all(b"Time\tOriginal\tDestination\n")
+            writeln!(record_file, "{}", Self::HEADER)
                 .expect("Failed to write header to record file");
         }
         Record { path }
@@ -87,14 +127,15 @@ impl<const FILE_LOCK: bool> Record<FILE_LOCK> {
         // record: impl AsRef<Path>
         let record_file = self.open()?;
         let mut contents = String::new();
-        BufReader::new(&record_file).read_to_string(&mut contents)?;
+        {
+            let mut reader = self.skip_header(BufReader::new(&record_file))?;
+            reader.read_to_string(&mut contents)?;
+        }
 
         // This will be None if there is nothing, or Some
         // if there is items in the vector
         let mut graves_to_exhume: Vec<PathBuf> = Vec::new();
-        let mut lines = contents.lines();
-        lines.next();
-        for entry in lines.rev().map(RecordItem::new) {
+        for entry in contents.lines().rev().map(RecordItem::new) {
             // Check that the file is still in the graveyard.
             // If it is, return the corresponding line.
             if util::symlink_exists(&entry.dest) {
@@ -116,29 +157,24 @@ impl<const FILE_LOCK: bool> Record<FILE_LOCK> {
 
     /// Takes a vector of grave paths and removes the respective lines from the record
     fn delete_lines(&self, record_file: fs::File, graves: &[PathBuf]) -> Result<(), Error> {
-        let record_path = &self.path;
         // Get the lines to write back to the record, which is every line except
         // the ones matching the exhumed graves. Store them in a vector
         // since we'll be overwriting the record in-place.
-        let mut reader = BufReader::new(record_file).lines();
-        let header = reader
-            .next()
-            .unwrap_or_else(|| Ok(String::new()))
-            .unwrap_or_default(); // Capture the header
+        let reader = self.skip_header(BufReader::new(record_file))?;
         let lines_to_write: Vec<String> = reader
+            .lines()
             .map_while(Result::ok)
             .filter(|line| !graves.iter().any(|y| *y == RecordItem::new(line).dest))
             .collect();
-        // let mut new_record_file = fs::File::create(record_path)?;
         let mut new_record_file = fs::OpenOptions::new()
             .create(true)
             .truncate(true)
             .write(true)
-            .open(record_path)?;
+            .open(&self.path)?;
         if FILE_LOCK {
             new_record_file.lock_exclusive().unwrap();
         }
-        writeln!(new_record_file, "{}", header)?; // Write the header back
+        writeln!(new_record_file, "{}", Self::HEADER)?; // Write the header back
         for line in lines_to_write {
             writeln!(new_record_file, "{}", line)?;
         }
@@ -163,9 +199,9 @@ impl<const FILE_LOCK: bool> Record<FILE_LOCK> {
         graves: &'a [PathBuf],
     ) -> impl Iterator<Item = String> + 'a {
         let record_file = self.open().unwrap();
-        let mut reader = BufReader::new(record_file).lines();
-        reader.next();
+        let reader = self.skip_header(BufReader::new(record_file)).unwrap();
         reader
+            .lines()
             .map_while(Result::ok)
             .filter(move |line| graves.iter().any(|y| *y == RecordItem::new(line).dest))
     }
@@ -176,9 +212,9 @@ impl<const FILE_LOCK: bool> Record<FILE_LOCK> {
         gravepath: &'a PathBuf,
     ) -> io::Result<impl Iterator<Item = RecordItem> + 'a> {
         let record_file = self.open()?;
-        let mut reader = BufReader::new(record_file).lines();
-        reader.next();
+        let reader = self.skip_header(BufReader::new(record_file))?;
         Ok(reader
+            .lines()
             .map_while(Result::ok)
             .map(|line| RecordItem::new(&line))
             .filter(move |record_item| record_item.dest.starts_with(gravepath)))
@@ -188,29 +224,12 @@ impl<const FILE_LOCK: bool> Record<FILE_LOCK> {
     pub fn write_log(&self, source: impl AsRef<Path>, dest: impl AsRef<Path>) -> io::Result<()> {
         let (source, dest) = (source.as_ref(), dest.as_ref());
 
-        let already_existed = self.path.exists();
-
-        // TODO: The tiny amount of time between the check and the open
-        //       could allow for a race condition. But maybe I'm being overkill.
-
-        let mut record_file = if already_existed {
-            fs::OpenOptions::new().append(true).open(&self.path)?
-        } else {
-            fs::OpenOptions::new()
-                .create(true)
-                .truncate(true)
-                .write(true)
-                .open(&self.path)?
-        };
+        let mut record_file = fs::OpenOptions::new().append(true).open(&self.path)?;
 
         if FILE_LOCK {
             record_file.lock_exclusive().unwrap();
         }
 
-        if !already_existed {
-            writeln!(record_file, "Time\tOriginal\tDestination")?;
-        }
-
         writeln!(
             record_file,
             "{}\t{}\t{}",
@@ -227,6 +246,26 @@ impl<const FILE_LOCK: bool> Record<FILE_LOCK> {
 
         Ok(())
     }
+
+    /// Validates the header of a record file and returns the reader positioned after the header.
+    /// Returns Err if the header is invalid.
+    fn skip_header<R: BufRead>(&self, mut reader: R) -> io::Result<R> {
+        let mut header = String::new();
+        reader.read_line(&mut header)?;
+
+        if header.trim() != Self::HEADER {
+            return Err(Error::new(
+                ErrorKind::InvalidData,
+                format!(
+                    "Invalid record file header at {}:\n  Expected: '{}'\n  Got:      '{}'",
+                    &self.path.display(),
+                    Self::HEADER,
+                    header.trim()
+                ),
+            ));
+        }
+        Ok(reader)
+    }
 }
 
 impl<const FILE_LOCK: bool> Clone for Record<FILE_LOCK> {

tests/integration_tests.rs

@@ -1,5 +1,6 @@
 use lazy_static::lazy_static;
 use predicates::str::is_match;
+use predicates::Predicate;
 use rand::distributions::Alphanumeric;
 use rand::{Rng, SeedableRng};
 use rip2::args::Args;
@@ -1079,3 +1080,118 @@ fn _test_concurrent_writes<const FILE_LOCK: bool>() {
         assert!(corrupted_lines > 0);
     }
 }
+
+#[rstest]
+fn test_no_header() {
+    let _env_lock = aquire_lock();
+    let test_env = TestEnv::new();
+    fs::create_dir_all(&test_env.graveyard).unwrap();
+    let record_file = test_env.graveyard.join(".record");
+    fs::write(
+        &record_file,
+        b"2024-12-21T16:47:21.922660-05:00\toldpath\tnewpath\n",
+    )
+    .unwrap();
+
+    // Attempt to run `seance`, which will parse `.record`. We expect it to fail with
+    // a helpful error message.
+    let mut log = Vec::new();
+    let result = rip2::run(
+        Args {
+            seance: true,
+            graveyard: Some(test_env.graveyard.clone()),
+            ..Args::default()
+        },
+        TestMode,
+        &mut log,
+    );
+
+    // Check that we got the right error
+    let err = result.expect_err("Expected an error due to missing header");
+    assert_eq!(err.kind(), ErrorKind::InvalidData);
+
+    // Ensure the error message alerts the user to the old format
+    let err_msg = err.to_string();
+    assert!(
+        is_match(r"Invalid record file header at .+:\s+Expected: 'Time\tOriginal\tDestination'\s+Got:\s+'.*'")
+            .unwrap()
+            .eval(&err_msg),
+        "Unexpected error message: {err_msg}"
+    );
+
+    // Now, add the header to the top of the file and try again
+    let header = "Time\tOriginal\tDestination\n";
+    let existing_content = fs::read_to_string(&record_file).unwrap();
+    fs::write(&record_file, format!("{}{}", header, existing_content)).unwrap();
+
+    // Try running seance again - it should work this time
+    let mut log = Vec::new();
+    rip2::run(
+        Args {
+            seance: true,
+            graveyard: Some(test_env.graveyard.clone()),
+            ..Args::default()
+        },
+        TestMode,
+        &mut log,
+    )
+    .unwrap();
+}
+
+#[rstest]
+fn test_legacy_date_format() {
+    let _env_lock = aquire_lock();
+    let test_env = TestEnv::new();
+    fs::create_dir_all(&test_env.graveyard).unwrap();
+
+    // Create source and destination paths with actual files
+    let src_dir = test_env.src.join("nested").join("dir");
+    fs::create_dir_all(&src_dir).unwrap();
+    let src_path = src_dir.join("testfile.txt");
+    fs::write(&src_path, "").unwrap();
+
+    // Create destination path in graveyard mirroring source structure
+    let dest_path =
+        util::join_absolute(&test_env.graveyard, dunce::canonicalize(&src_path).unwrap());
+    fs::create_dir_all(dest_path.parent().unwrap()).unwrap();
+    // Put the actual contents here:
+    fs::write(&dest_path, "test content").unwrap();
+    // And delete the src file
+    fs::remove_file(&src_path).unwrap();
+
+    // Write record file with old format timestamp but new header
+    let record_file = test_env.graveyard.join(".record");
+    fs::write(
+        record_file,
+        format!(
+            "Time\tOriginal\tDestination\nSat Dec 21 16:48:22 2024\t{}\t{}\n",
+            src_path.display(),
+            dest_path.display()
+        ),
+    )
+    .unwrap();
+
+    let cur_dir = env::current_dir().unwrap();
+    env::set_current_dir(&test_env.src).unwrap();
+    let mut log = Vec::new();
+    let result = rip2::run(
+        Args {
+            seance: true,
+            graveyard: Some(test_env.graveyard.clone()),
+            ..Args::default()
+        },
+        TestMode,
+        &mut log,
+    );
+    env::set_current_dir(cur_dir).unwrap();
+
+    // Expect error about old format
+    let err = result.expect_err("Expected error from old rip format line");
+    assert_eq!(err.kind(), ErrorKind::InvalidData);
+
+    let err_msg = err.to_string();
+    assert!(
+        err_msg.contains("Found timestamp 'Sat Dec 21 16:48:22 2024' from old rip format"),
+        "Unexpected error message: {err_msg}"
+    );
+}