Skip to content

Show ctime and mtime on borg diff. Fixes #7248 (Backport of #7335)#7414

Merged
ThomasWaldmann merged 2 commits intoborgbackup:1.2-maintfrom
Michael-Girma:backport/1.2-maint-7248
Mar 20, 2023
Merged

Show ctime and mtime on borg diff. Fixes #7248 (Backport of #7335)#7414
ThomasWaldmann merged 2 commits intoborgbackup:1.2-maintfrom
Michael-Girma:backport/1.2-maint-7248

Conversation

@Michael-Girma
Copy link
Copy Markdown
Contributor

@Michael-Girma Michael-Girma commented Mar 7, 2023

Show ctime and mtime on borg diff. Fixes #7248 (Backport of #7335)

  • Added assert_line_exists helper in BaseTestCase
  • JSON strings in diff output are now sorted alphabetically
  • Modified diff test cases to confirm to new output format
  • Added a test case to test ctime and mtime inclusion
  • Mode, ctime & mtime are now only displayed on diff if --content-only flag is used

---

- Added assert_line_exists helper in BaseTestCase
- JSON strings in diff output are now sorted alphabetically
- Modified diff test cases to confirm to new output format
- Added a test case to test ctime and mtime inclusion
- Mode, ctime & mtime are now only displayed on diff if --content-only flag is used
@Michael-Girma Michael-Girma force-pushed the backport/1.2-maint-7248 branch from 535ce0d to 6c042d7 Compare March 7, 2023 07:54
@ThomasWaldmann
Copy link
Copy Markdown
Member

Did you see the tests are failing?

@ThomasWaldmann
Copy link
Copy Markdown
Member

BTW: I likely will do a 1.2.4 release soon.

@Michael-Girma
Copy link
Copy Markdown
Contributor Author

@ThomasWaldmann was away from my rig. Taking a look now.

@Michael-Girma
Copy link
Copy Markdown
Contributor Author

@ThomasWaldmann couldn't recreate the issue locally. I tried with py39, py39-fuse2, py39-fuse3, py310, py310-fuse2, py310-fuse3

System information.

Operating system (distribution) and version.

Ubuntu 22.04.1 LTS

Hardware / network configuration, and filesystems used.

Intel® Core™ i5-8300H CPU @ 2.30GHz × 8
12GiB Memory
Ext4 1TB

Full borg commandline that lead to the problem (leave away excludes and passwords)

python setup.py clean clean2
pip install -r requirements.d/development.txt
pip install -e .
tox --skip-missing-interpreters -- borg.testsuite.archiver::DiffArchiverTestCase

@ThomasWaldmann
Copy link
Copy Markdown
Member

I restarted the CI, let's see if it happens again.

@Michael-Girma
Copy link
Copy Markdown
Contributor Author

Ahh, able to recreate after pulling in latest 1.2-maint. Taking a look

@Michael-Girma
Copy link
Copy Markdown
Contributor Author

Michael-Girma commented Mar 17, 2023

Getting two entries for a single file path on both text and JSON outputs. I couldn't get a hint as to why this happens (especially why it's only on linux). Any idea why this might be @ThomasWaldmann?

@Michael-Girma
Copy link
Copy Markdown
Contributor Author

Michael-Girma commented Mar 18, 2023

@ThomasWaldmann Figured out why we're having double entries. Shouldn't the last line be referencing the item's path as opposed to just path? Since it's referring to path when addressing deferred items, it adds a tuple with the path of the last seen item, leading to two entries for a single item. Changing it to item1.path fixed the issue and all the tests are passing. Should I push this?

quick edit:
Just checked. It's misleading but the reason the same test isn't failing on mac is because that block of code is never executed on macOS.

image

@ThomasWaldmann
Copy link
Copy Markdown
Member

@Michael-Girma congrats, you found a bug!

I think right above that line, there should be a

assert item1.path == item2.path

And then it should use item1.path instead of path.

Can you check master for that problem and if present there, make a separate bugfix PR just for that?
Then (after successful tests) this could be merged and backported to 1.2-maint.
Then, this PR here could be rebased on updated 1.2-maint branch.

@ThomasWaldmann
Copy link
Copy Markdown
Member

I just checked: due to the simplifications in master related to "hardlink masters", that code is not present any more.

So guess it is fine if you just fix it here, in a separate commit please.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 20, 2023

Codecov Report

Merging #7414 (19bb277) into 1.2-maint (48713eb) will decrease coverage by 0.04%.
The diff coverage is 80.00%.

📣 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              @@
##           1.2-maint    #7414      +/-   ##
=============================================
- Coverage      82.43%   82.40%   -0.04%     
=============================================
  Files             38       38              
  Lines          10596    10626      +30     
  Branches        2035     2040       +5     
=============================================
+ Hits            8735     8756      +21     
- Misses          1322     1330       +8     
- Partials         539      540       +1     
Impacted Files Coverage Δ
src/borg/helpers/parseformat.py 89.88% <0.00%> (ø)
src/borg/archive.py 80.98% <100.00%> (+0.07%) ⬆️
src/borg/archiver.py 79.36% <100.00%> (-0.19%) ⬇️

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

Copy link
Copy Markdown
Member

@ThomasWaldmann ThomasWaldmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@real-yfprojects
Copy link
Copy Markdown

  • Mode, ctime & mtime are now only displayed on diff if --content-only flag is used

I think these change types are only displayed if the flag is not used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants