Add support for --changed-before and --changed-with for modification … #339

kimsnj · 2018-10-09T11:58:07Z

This PR aims at implementing #165

It adds two options changed-before and changed-with that can accept either a duration (such as 10min, 2d, …) or a date and time (such as 2018-10-09 10:00:00).

Some bikeshedding might be useful on the name on the options, as some of the combination reads quite well with:

fd --changed-within 10d
fd --changed-before "2018-10-09 10:00:00"

But the other are a bit less self-explanatory:

fd --changed-before 10d
fd --changed-within "2018-10-09 10:00:00"

Any thoughts ?

On a slightly different topic, as i was running clippy before committing my changes, I noticed some lints triggered. Would you welcome a PR that applies those?

…time based search

sharkdp

Thank you very much for your contribution.

This is really great work. I added a few (nit-picky) comments.

Edit: The command-line option naming-thing is a bit unfortunate, yes. I'll need to think about this.

sharkdp · 2018-10-09T20:50:21Z

src/internal.rs

+    fn from_str(s: &str) -> Option<SystemTime> {
+        use humantime;
+        humantime::parse_duration(s)
+            .map(|duration| SystemTime::now() - duration)


I think it would be cleaner if we could pass now: SystemTime into this function in order to make it side-effect free (or pure). This would make testing deterministic.

By getting SystemTime::now() just once in the main-function, we would also prevent really unlikely situations where we would end up with two different SystemTime::now() values in multiple TimeFilters.

sharkdp · 2018-10-09T20:52:11Z

src/internal.rs

+    }
+
+    pub fn before(s: &str) -> Option<TimeFilter> {
+        Some(TimeFilter::Before(TimeFilter::from_str(s)?))


Alternative implementation:

TimeFilter::from_str(s).map(TimeFilter::Before)

Choose whichever you prefer 😄. I was just shortly confused because I hadn't seen the ?.

I indeed find it more readable with map :)

sharkdp · 2018-10-09T20:54:18Z

src/internal.rs

@@ -162,6 +194,9 @@ pub struct FdOptions {

    /// The given constraints on the size of returned files
    pub size_constraints: Vec<SizeFilter>,
+
+    /// Constraints on last modification time of files
+    pub modification_constraints: Vec<TimeFilter>,


I would prefer modification_time_constaints or maybe just time_constraints for now.

sharkdp · 2018-10-09T20:56:00Z

src/internal.rs

+
+    #[test]
+    fn is_time_within() {
+        let now = SystemTime::now();


Even if this is rather unlikely to fail, it would be great if all tests could be completely deterministic by using specific (random) points in time instead of SystemTime::now().

sharkdp · 2018-10-09T20:57:07Z

src/internal.rs

+        Some(TimeFilter::After(TimeFilter::from_str(s)?))
+    }
+
+    pub fn is_within(&self, t: &SystemTime) -> bool {


Not completely happy with this function name. Maybe applies_to?

sharkdp · 2018-10-09T20:59:31Z

src/walk.rs

@@ -289,6 +289,21 @@ pub fn scan(path_vec: &[PathBuf], pattern: Arc<Regex>, config: Arc<FdOptions>) {
                }
            }

+            // Filter out unwanted modification times
+            if !config.modification_constraints.is_empty() {
+                if_chain!{


This crate/macro looks interesting, but I would prefer if we could just use plain nested if statements here (for consistency).

I wanted to avoid the 4 * else { return Continue }.
I removed the dependency and added a boolean flag instead.
I could put back the else { return } if you prefer.

sharkdp · 2018-10-09T21:01:03Z

src/walk.rs

+                    if entry_path.is_file();
+                    if let Ok(metadata) = entry_path.metadata();
+                    if let Ok(modified) = metadata.modified();
+                    if config.modification_constraints.iter().all(|tf| tf.is_within(&modified));


If we would invert the logic here by using .iter().any(|tf| !tf.is_within(&modified) we could profit from short-circuiting and also prevent the empty if/then clause.

all also benefits from the same short-circuiting (as-soon as a predicate fails, all returns).
in this case, it allows to have all the return WalkState::Continue as the else branch, and thus put them in common.

You are right, of course 👍

sharkdp · 2018-10-09T21:01:44Z

tests/tests.rs

@@ -1110,3 +1113,65 @@ fn test_size() {
    // Files with size equal 4 kibibytes.
    te.assert_output(&["", "--size", "+4ki", "--size", "-4ki"], "4_kibibytes.foo");
 }
+
+#[cfg(test)]
+fn create_file_with_modified<P: AsRef<Path>>(path: P, duration_in_secs: u64) {


sharkdp · 2018-10-09T21:03:16Z

tests/tests.rs

+    );
+
+    te.assert_output(
+        &["min", "--changed-within", "12h"],


This test really confused me for a while because I didn't see the first "min" argument. Maybe you could name some of the files foo_1_h and others bar_10_min and then additionally search for foo here?

sharkdp · 2018-10-10T17:52:27Z

Thank you very much for the updates! We can think about the command-line option names before the next release, but I'd like to get this merged 👍

(one option would be to add aliases changed-after = --changed-within and ??? == changed_before)

ptzz · 2018-10-16T15:24:01Z

git log has --since <date> and --until <date>, might provide inspiration for the naming.

Add support for --changed-before and --changed-with for modification …

3274169

…time based search

kimsnj force-pushed the time branch from 6e093a5 to 3274169 Compare October 9, 2018 14:04

sharkdp requested changes Oct 9, 2018

View reviewed changes

kimsnj force-pushed the time branch from 3d982c3 to 5ae25d7 Compare October 10, 2018 15:13

clean-up first implementation of modification date filter

574a16a

kimsnj force-pushed the time branch from 5ae25d7 to 574a16a Compare October 10, 2018 17:17

sharkdp approved these changes Oct 10, 2018

View reviewed changes

sharkdp merged commit abe8aa5 into sharkdp:master Oct 10, 2018

This was referenced Oct 10, 2018

Search based on file timestamps (atime, ctime, mtime) #165

Closed

Split internals.rs into module #340

Closed

Ruin0x11 mentioned this pull request Jan 17, 2019

Support for atime and ctime in --changed-within/--changed-before #393

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for --changed-before and --changed-with for modification … #339

Add support for --changed-before and --changed-with for modification … #339

kimsnj commented Oct 9, 2018 •

edited

Loading

sharkdp left a comment •

edited

Loading

sharkdp Oct 9, 2018

kimsnj Oct 10, 2018

sharkdp Oct 9, 2018

kimsnj Oct 10, 2018

sharkdp Oct 9, 2018

sharkdp Oct 9, 2018

sharkdp Oct 9, 2018

sharkdp Oct 9, 2018

kimsnj Oct 10, 2018

sharkdp Oct 9, 2018

kimsnj Oct 10, 2018

sharkdp Oct 10, 2018

sharkdp Oct 9, 2018

sharkdp Oct 9, 2018

sharkdp commented Oct 10, 2018

ptzz commented Oct 16, 2018

Add support for --changed-before and --changed-with for modification … #339

Add support for --changed-before and --changed-with for modification … #339

Conversation

kimsnj commented Oct 9, 2018 • edited Loading

sharkdp left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharkdp commented Oct 10, 2018

ptzz commented Oct 16, 2018

kimsnj commented Oct 9, 2018 •

edited

Loading

sharkdp left a comment •

edited

Loading