Modified Item.pyx to include diffs in ctime and mtime

[Michael-Girma]

Modified Item.pyx to include diffs in ctime and mtime. Fixes #7248
Created flag to switch this feature on/off

[Michael-Girma]

@ThomasWaldmann could use some help with the help text for the diff cmd.

[ThomasWaldmann]

Thanks for the PR, some stuff I found...

[ThomasWaldmann]

BTW, some tests fail in a not very informative way. Guess this could be improved in a separate commit.

[ThomasWaldmann]

Would like to include this in next borg2 beta if it can be finished soon.

@Michael-Girma are you still working on this?

[Michael-Girma]

@ThomasWaldmann pushed changes. Take a look when possible.

[ThomasWaldmann]

Windows tests are failing, check why. ctime on windows is creation time, not inode change time as on unix.

[Michael-Girma]

@ThomasWaldmann Working with the docs but still struggling to set up a windows dev environment using Cygwin. Is there another preferred method to set it up or at the very least mimic it for the tests?

[ThomasWaldmann]

@Michael-Girma the windows CI works with msys2, not sure if test results are same for cygwin.

Maybe @RayyanAnsari can help better with the Windows stuff than me.

[RayyanAnsari]

@Michael-Girma yes, on Windows we use MSYS2 for the build environment.
You can check the windows.yml workflow file to see how it works: https://github.com/borgbackup/borg/blob/master/.github/workflows/windows.yml
In short, you need to:

• Install MSYS2
• Open a UCRT64 terminal
• Define this environment variable export SETUPTOOLS_USE_DISTUTILS=stdlib
• Install dependencies (using the script)
• Build (using the script)
• Install the package in-place
• Run pytest

@ThomasWaldmann I should probably add this to the documentation, is it just installation.rst that I need to add this to?

[ThomasWaldmann]

@RayyanAnsari yes, it would be good to have it in the docs.

installation.rst is the end-user stuff and the development section usually builds on that and adds developer instructions.

we have also that old windows todo file in the repo root dir, guess it is outdated?

[Michael-Girma]

@ThomasWaldmann. Been puzzled by an issue regarding the ctime attribute on windows for the past couple of days. One thing I landed on is that there is a different behavior for temporary dirs. When creating a file manually, a difference in ctime is picked up by borg if a file is recreated with the same filename. However, this behavior is not the same if the parent dir is created with mktemp -d. I've attached a screenshot showing this behavior. In the first two tabs, I created the parent dir with mkdir and used mktemp -d for the third one. Any thoughts on what might have caused this?

[codecov-commenter]

Codecov Report

Merging #7335 (026dfd4) into master (85cc741) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

❗ Current head 026dfd4 differs from pull request most recent head 2b97111. Consider uploading reports for the commit 2b97111 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #7335      +/-   ##
==========================================
- Coverage   83.91%   83.84%   -0.08%     
==========================================
  Files          67       67              
  Lines       11710    11722      +12     
  Branches     2133     2134       +1     
==========================================
+ Hits         9827     9828       +1     
- Misses       1318     1327       +9     
- Partials      565      567       +2     

Impacted Files	Coverage Δ
src/borg/archive.py	84.62% <100.00%> (ø)
src/borg/archiver/diff_cmd.py	95.91% <100.00%> (+0.17%)	⬆️
src/borg/helpers/parseformat.py	90.74% <100.00%> (ø)
src/borg/archiver/debug_cmd.py	72.13% <0.00%> (-1.99%)	⬇️
src/borg/platform/base.py	79.83% <0.00%> (-1.62%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[ThomasWaldmann]

@Michael-Girma that's strange. like it did not delete the file, but continued with the same one (3rd screenshot).

one difference is maybe the fs type you work on:

• home dir, ext4 maybe?
• /tmp, tmpfs maybe?

maybe you could try the same as in 1st screenshot, but manually use /tmp/something.

of course, /tmp is also mode +t ("sticky"), but i don't see how what you found could be related to that.

ehrm, i was still thinking linux, but you used windows. so what i said might be irrelevant. not sure how msys2 is working, the paths looked like on linux.

[Michael-Girma]

@ThomasWaldmann , I thought linux as well. That's why I tried a third archive with the file modified. In that scenario, there would have been a ctime in the last diff output since ctime would change on linux after content manipulation. But the above reasons do seem interesting. I'll have a deeper look.

[Michael-Girma]

@ThomasWaldmann after landing on an issue hinting that ctime isn't refreshed if the file is recreated within 15 seconds after deletion, I tested this locally and found out why we were having the issue. After adding 15 seconds between the unlink and recreation in the test case, borg seems to be spitting out the ctime diff and the tests seem to be passing.

[ThomasWaldmann]

@Michael-Girma that 15s thing is interesting, like "wtf did they think when implementing it like that?".

the issue you linked to reads rather confusing and i guess we should focus on python os.stat(fn) -> stat_result:

https://docs.python.org/3/library/os.html#os.stat_result.st_ctime

So, is that "creation time" (as documented) under msys2? Or "metadata change time" as on Unix?

Also, whatever you find out should be in a source code comment, so it is easier accessible without having to dig into git / github. There should be a comment explaining it right above that sleep(15).

Also: does this explain the different behaviour you found in home and in /tmp (was it just the timing of your manual commands)?

Also interesting would be to research whether we get stat_result.birthtime?

[Michael-Girma]

@ThomasWaldmann I know, crazy idea.
In msys2, it is creation time and metadata change time on UNIX. os.stat_result doesn't have the st_birthtime attribute on windows but it is populated when running the same code on linux.

Yeah, I'm also getting ctime on tmp dirs. Must have been my timing.

[Michael-Girma]

@ThomasWaldmann I've added in the comment above sleep and I've also rebased to clean up the commit history, add the issue number and bring in the latest changes from master.

[ThomasWaldmann]

some more, sometimes minor, stuff i found.

also the hardlink stuff needs a thorough check as borg is handling these a bit differently in master:

• there is no "source" or "target" any more for hardlinks in the borg item (looks like some comments were wrong before you change already, maybe you could check and fix them)
• hardlinks that originally pointed to same inode have the same "hlid" value in the borg item

[Michael-Girma]

@ThomasWaldmann Checked the hard-link changes. So what the previous test case was trying to do is create 2 hard-links and then remove one of them. It was expected that this shouldn't change the link that hasn't been removed in any way, which was right since we weren't considering ctime and mtime. The difference now is that we are taking these two fields into account. The decrementation on the hard link count in the inode registers as metadata change thus changing the ctime. This means that it's gonna show up on the diff unless we are checking with --content-only. Will add a comment to the test case to explain it if this is fine.

[ThomasWaldmann]

Ah, good explanation, yes, please add some short comment in the src.

[ThomasWaldmann]

LGTM.

[ThomasWaldmann]

Thanks!

[ThomasWaldmann]

If you like, check how easy/hard this is to backport to 1.2-maint.
We could add it for release in 1.2.4 or it has to wait for borg2 depending on that.

[Michael-Girma]

Okay will do!

[real-yfprojects]

@ThomasWaldmann The All about JSON page needs an update because of this.

[ThomasWaldmann]

@real-yfprojects thanks for the hints, see #7486.