Display modification times of input files in context and unified diff #33

TanmayPatil105 · 2024-03-27T17:23:31Z

Fixes #31

tanmay@tanmay-lenovo:~/Desktop/projects/uutils/diffutils$ cargo run -- -c fruits_old.txt fruits_new.txt 
    Finished dev [unoptimized + debuginfo] target(s) in 0.04s
     Running `target/debug/diffutils -c fruits_old.txt fruits_new.txt`
*** fruits_old.txt	2024-03-24 23:43:05.189597645 +0530
--- fruits_new.txt	2024-03-24 23:35:08.922581904 +0530
***************
*** 1,3 ****
  Apple
! Banana
  Cherry
--- 1,3 ----
  Apple
! Fig
  Cherry
tanmay@tanmay-lenovo:~/Desktop/projects/uutils/diffutils$ diff -c fruits_old.txt fruits_new.txt 
*** fruits_old.txt	2024-03-24 23:43:05.189597645 +0530
--- fruits_new.txt	2024-03-24 23:35:08.922581904 +0530
***************
*** 1,3 ****
  Apple
! Banana
  Cherry
--- 1,3 ----
  Apple
! Fig
  Cherry

Fixes uutils#31

sylvestre · 2024-03-27T17:39:26Z

Could you please add tests

sylvestre · 2024-03-27T17:39:31Z

Thanks

TanmayPatil105 · 2024-03-27T18:12:37Z

Okay! The current tests are failing.

error: test failed, to rerun pass `--lib`
---- context_diff::tests::test_permutations_missing_lines stdout ----
thread 'context_diff::tests::test_permutations_missing_lines' panicked at src/context_diff.rs:269:44:
Failed to get metadata: Os { code: 2, kind: NotFound, message: "No such file or directory" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- context_diff::tests::test_permutations_empty_lines stdout ----
thread 'context_diff::tests::test_permutations_empty_lines' panicked at src/context_diff.rs:269:44:
Failed to get metadata: Os { code: 2, kind: NotFound, message: "No such file or directory" }

---- context_diff::tests::test_permutations stdout ----
thread 'context_diff::tests::test_permutations' panicked at src/context_diff.rs:269:44:
Failed to get metadata: Os { code: 2, kind: NotFound, message: "No such file or directory" }

---- context_diff::tests::test_permutations_reverse stdout ----
thread 'context_diff::tests::test_permutations_reverse' panicked at src/context_diff.rs:269:44:
Failed to get metadata: Os { code: 2, kind: NotFound, message: "No such file or directory" }

---- context_diff::tests::test_stop_early stdout ----
thread 'context_diff::tests::test_stop_early' panicked at src/context_diff.rs:269:44:
Failed to get metadata: Os { code: 2, kind: NotFound, message: "No such file or directory" }

Isn't this strange? We check if file exists in main.rs : https://github.com/uutils/diffutils/blob/main/src/main.rs#L52

Edit: These are unit tests. My bad!

TanmayPatil105 · 2024-03-27T18:15:00Z

Could you please add tests

Can you guide me more on these 😅?
Current tests are quite difficult to understand

sylvestre · 2024-03-27T20:01:27Z

This one is pretty simple, no ? :)
https://github.com/uutils/diffutils/blob/main/tests/integration.rs#L127-L148

TanmayPatil105 · 2024-03-28T13:48:58Z

@sylvestre I was looking at the unit tests and I see we are not actually creating files but only passing filename literals to context_diff::diff. The tests are failing since we are looking for metadata for files which don't exist in the diff function. We are using expected_filename/actual_filename only for printing.
Should we create those files during the tests?

sylvestre · 2024-03-28T14:24:46Z

yes please :)

src/context_diff.rs

TanmayPatil105 · 2024-03-31T17:38:05Z

I'm not sure on how the tests are failing on CI, we are creating the files now. It should have worked. Strange 🤔

The tests are working fine on my machine:

test context_diff::tests::test_permutations_missing_lines ... ok
test context_diff::tests::test_permutations_empty_lines ... ok
test context_diff::tests::test_permutations_reverse ... ok
test context_diff::tests::test_permutations ... ok
test normal_diff::tests::test_stop_early ... ok

sylvestre · 2024-03-31T22:47:24Z

sorry, could you please fix the conflict? Sorry :(

Cargo.toml

codecov · 2024-04-03T05:35:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.85%. Comparing base (be66ff3) to head (5b814f8).

❗ Current head 5b814f8 differs from pull request most recent head aedd068. Consider uploading reports for the commit aedd068 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #33      +/-   ##
==========================================
- Coverage   76.29%   75.85%   -0.45%     
==========================================
  Files           9        9              
  Lines        2582     2597      +15     
  Branches      663      662       -1     
==========================================
  Hits         1970     1970              
- Misses        459      476      +17     
+ Partials      153      151       -2

Flag	Coverage Δ
macos_latest	`76.00% <100.00%> (?)`
ubuntu_latest	`75.81% <100.00%> (-0.49%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TanmayPatil105 · 2024-04-03T05:37:16Z

I realized that the tests were passing locally because files were generated during previous test runs.
wondering if cargo should clean up the temp files generated during tests 🤔 . Is there any way to specify it? OR we should manually delete them after every unit test.

sylvestre · 2024-04-03T07:11:35Z

Is there any way to specify it?

Yeah, just https://crates.io/crates/tempfile to create these files

src/context_diff.rs

oSoMoN

Thanks for the update, this is looking promising!
See inline for a few suggestions.
Also, note that the read_from_stdin() integration test is failing. The expectation on the output will need to be updated to ignore the timestamps.

Cargo.toml

src/utils.rs

src/context_diff.rs

…il105/diffutils into context-diff-modification-time

TanmayPatil105 · 2024-04-13T16:03:21Z

Hey @oSoMoN , I have made the necessary changes! Can you review the MR?
Thanks!

oSoMoN · 2024-04-13T21:55:19Z

src/utils.rs

+            let target = "target/utils";
+            let _ = std::fs::create_dir(target);
+            let filename = &format!("{target}/foo");
+            let temp = File::create(filename).unwrap();


Can you use a tempfile.NamedTempFile instead?

I don't think there are APIs to set modification time for a NamedTempFile.

NamedTempFile::as_file() exposes a reference to the underlying file, so you could do something like this:

use tempfile::NamedTempFile; let temp = NamedTempFile::new().unwrap(); let current = SystemTime::now(); let _ = temp.as_file().set_modified(current); … assert_eq!(current, get_modification_time(&temp.path().to_string_lossy()));

src/utils.rs

src/macros.rs

src/utils.rs

src/context_diff.rs

oSoMoN

Thanks Tanmay, that looks quite good!
I have left a handful of comments/suggestions, see inline.

TanmayPatil105 · 2024-04-14T13:06:21Z

Halfway through the changes. I'll complete them shortly!

Since we now returning SystemTime::now() for invalid file input, there is no need to crate dummy files

oSoMoN

Thanks, that looks really good now. Almost ready to merge, see only my suggestion on the use of NamedTempFile.

src/utils.rs

oSoMoN · 2024-04-15T20:54:10Z

src/utils.rs

+            let target = "target/utils";
+            let _ = std::fs::create_dir(target);
+            let filename = &format!("{target}/foo");
+            let temp = File::create(filename).unwrap();


NamedTempFile::as_file() exposes a reference to the underlying file, so you could do something like this:

use tempfile::NamedTempFile; let temp = NamedTempFile::new().unwrap(); let current = SystemTime::now(); let _ = temp.as_file().set_modified(current); … assert_eq!(current, get_modification_time(&temp.path().to_string_lossy()));

oSoMoN · 2024-04-15T21:03:28Z

It's worth noting that with the changes in this PR, one test from the GNU test suite which currently fails in main now passes (the timezone test). Nice improvement!

TanmayPatil105 · 2024-04-16T04:58:12Z

Done!

TanmayPatil105 · 2024-04-16T05:12:43Z

Sorry for the noise! I forgot to run cargo fmt.

oSoMoN · 2024-04-16T16:20:36Z

Merged, thanks Tanmay for the contribution!

Display modification times of input files in context diff

42eb15b

Fixes uutils#31

sylvestre force-pushed the context-diff-modification-time branch from 392f726 to 9d4346e Compare March 30, 2024 14:34

sylvestre reviewed Mar 30, 2024

View reviewed changes

src/context_diff.rs Outdated Show resolved Hide resolved

Fix tests

9ff8f89

TanmayPatil105 force-pushed the context-diff-modification-time branch from 9d4346e to 9ff8f89 Compare March 31, 2024 10:45

sylvestre and others added 2 commits March 31, 2024 16:17

Merge branch 'main' into context-diff-modification-time

043c5f9

Create foo/bar in target/context-diff

80c9944

Merge branch 'main' into context-diff-modification-time

88a7568

TanmayPatil105 force-pushed the context-diff-modification-time branch from 38bd174 to 88a7568 Compare April 1, 2024 07:36

sylvestre reviewed Apr 1, 2024

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

Fix tests

5b814f8

oSoMoN reviewed Apr 3, 2024

View reviewed changes

src/context_diff.rs Outdated Show resolved Hide resolved

oSoMoN reviewed Apr 3, 2024

View reviewed changes

src/context_diff.rs Outdated Show resolved Hide resolved

oSoMoN reviewed Apr 3, 2024

View reviewed changes

src/context_diff.rs Outdated Show resolved Hide resolved

Display modification times of input files in unified diff

a3a372f

TanmayPatil105 force-pushed the context-diff-modification-time branch from a760034 to a3a372f Compare April 3, 2024 18:44

TanmayPatil105 changed the title ~~Display modification times of input files in context diff~~ Display modification times of input files in context and unified diff Apr 4, 2024

Merge branch 'main' into context-diff-modification-time

61fb065

TanmayPatil105 requested a review from oSoMoN April 5, 2024 16:29

oSoMoN requested changes Apr 5, 2024

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

src/utils.rs Show resolved Hide resolved

src/context_diff.rs Outdated Show resolved Hide resolved

TanmayPatil105 and others added 4 commits April 10, 2024 22:13

Merge branch 'main' into context-diff-modification-time

f6eb083

Define assert_diff_eq macro for context&unified diff comparison

00e18a6

Merge branch 'context-diff-modification-time' of github.com:TanmayPat…

86bd05c

…il105/diffutils into context-diff-modification-time

Add tests for get_modification_time function

0a77fe1

TanmayPatil105 force-pushed the context-diff-modification-time branch from 81c1346 to 0a77fe1 Compare April 13, 2024 16:01