Skip to content

Add --format option to borg diff, resolve issue #4634#7440

Closed
RF-Tar-Railt wants to merge 0 commit intoborgbackup:masterfrom
RF-Tar-Railt:master
Closed

Add --format option to borg diff, resolve issue #4634#7440
RF-Tar-Railt wants to merge 0 commit intoborgbackup:masterfrom
RF-Tar-Railt:master

Conversation

@RF-Tar-Railt
Copy link
Copy Markdown
Contributor

@RF-Tar-Railt RF-Tar-Railt commented Mar 13, 2023

!!! warning
this pull request may cause conflict with #7414

  • add --format in diff_cmd.py
  • rewrite ItemDIff in item.pyx
  • keep --content-only and --json-lines
  • rewrite part of BaseFormatter and subclass
  • write DiffFormatter
  • use DiffFormatter in diff_cmd
  • test

@RF-Tar-Railt RF-Tar-Railt marked this pull request as ready for review March 15, 2023 15:18
@RF-Tar-Railt RF-Tar-Railt changed the title [WIP] Add --format option to borg diff, resolve issue #4634 [Need More Discuss] Add --format option to borg diff, resolve issue #4634 Mar 15, 2023
@RF-Tar-Railt RF-Tar-Railt changed the title [Need More Discuss] Add --format option to borg diff, resolve issue #4634 Add --format option to borg diff, resolve issue #4634 Mar 19, 2023
ThomasWaldmann added a commit that referenced this pull request Mar 20, 2023
…elete

delete: remove --cache-only option, fixes #7440
@RF-Tar-Railt
Copy link
Copy Markdown
Contributor Author

@ThomasWaldmann maybe you close the wrong pr! I guess #7449 is the issue you really want to close

@ThomasWaldmann
Copy link
Copy Markdown
Member

Oops, you're right @RF-Tar-Railt, thanks for the hint!

@ThomasWaldmann
Copy link
Copy Markdown
Member

BTW, you know you can run the test locally also? Just run tox(or e.g. tox -e flake8 for just the flake8 tests).

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 2, 2023

Codecov Report

Merging #7440 (38d7b51) into master (cca8280) will increase coverage by 0.07%.
The diff coverage is 93.85%.

❗ Current head 38d7b51 differs from pull request most recent head 08e69aa. Consider uploading reports for the commit 08e69aa 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    #7440      +/-   ##
==========================================
+ Coverage   83.82%   83.90%   +0.07%     
==========================================
  Files          67       67              
  Lines       11849    11899      +50     
  Branches     2162     2175      +13     
==========================================
+ Hits         9933     9984      +51     
+ Misses       1341     1338       -3     
- Partials      575      577       +2     
Impacted Files Coverage Δ
src/borg/helpers/parseformat.py 90.54% <92.75%> (-0.20%) ⬇️
src/borg/archiver/diff_cmd.py 94.82% <95.45%> (-1.10%) ⬇️
src/borg/archive.py 84.63% <100.00%> (+0.48%) ⬆️
src/borg/archiver/list_cmd.py 100.00% <100.00%> (ø)
src/borg/archiver/prune_cmd.py 87.05% <100.00%> (ø)
src/borg/archiver/rlist_cmd.py 100.00% <100.00%> (ø)
src/borg/helpers/__init__.py 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

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

@ThomasWaldmann
Copy link
Copy Markdown
Member

Can you check why this fails on windows?

>           assert "input/hardlink_target_removed" in output  # FIXME: unknown perf in other platforms
E           assert 'input/hardlink_target_removed' in "--chunker-params might be different between archives, diff will be slow.\r\nIf you know for certain that they are the same, pass --same-chunker-params to override this check.\r\nmodified:  (can't get size) input/file_replaced\r\nchanged link                input/link_changed\r\nmodified:  (can't get size) input/empty\r\nmodified:  (can't get size) input/hardlink_contents_changed\r\nadded directory             input/dir_added\r\nadded link                  input/link_added\r\nadded:                  0 B input/file_empty_added\r\nadded:              2.05 kB input/file_added\r\nadded:              2.05 kB input/hardlink_added\r\nremoved link                input/link_removed\r\nremoved directory           input/dir_removed\r\nremoved:              512 B input/file_removed2\r\nremoved:              256 B input/file_removed\r\nremoved:              256 B input/hardlink_removed\r\n"
src/borg/testsuite/archiver/diff_cmd.py:133: AssertionError

os.link should work on windows, hardlinks are supported.

@RF-Tar-Railt
Copy link
Copy Markdown
Contributor Author

Can you check why this fails on windows?


>           assert "input/hardlink_target_removed" in output  # FIXME: unknown perf in other platforms

E           assert 'input/hardlink_target_removed' in "--chunker-params might be different between archives, diff will be slow.\r\nIf you know for certain that they are the same, pass --same-chunker-params to override this check.\r\nmodified:  (can't get size) input/file_replaced\r\nchanged link                input/link_changed\r\nmodified:  (can't get size) input/empty\r\nmodified:  (can't get size) input/hardlink_contents_changed\r\nadded directory             input/dir_added\r\nadded link                  input/link_added\r\nadded:                  0 B input/file_empty_added\r\nadded:              2.05 kB input/file_added\r\nadded:              2.05 kB input/hardlink_added\r\nremoved link                input/link_removed\r\nremoved directory           input/dir_removed\r\nremoved:              512 B input/file_removed2\r\nremoved:              256 B input/file_removed\r\nremoved:              256 B input/hardlink_removed\r\n"

src/borg/testsuite/archiver/diff_cmd.py:133: AssertionError

os.link should work on windows, hardlinks are supported.

I'm not sure about this

In old version

# Another link (marked previously as the source in borg) to the
                # same inode was removed. This should only change the ctime since removing
                # the link would result in the decrementation of the inode's hard-link count.
                assert "input/hardlink_target_removed" not in output

which should be the correct behave?

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.

some stuff I noticed.

@ThomasWaldmann
Copy link
Copy Markdown
Member

ThomasWaldmann commented Apr 3, 2023

BTW, you work in your master branch, this has the issue of not being able to update your master to the state seen in upstream repo master.

I'ld recommend to read/follow #7495.

@RF-Tar-Railt
Copy link
Copy Markdown
Contributor Author

BTW, you work in your master branch, this has the issue of not being able to update your master to the state seen in upstream repo master.

github can keep the latest states for the fork repo, is it relevant to this?

@ThomasWaldmann
Copy link
Copy Markdown
Member

@RF-Tar-Railt i think it is always relevant, otherwise you'll get in the way of yourself.

@ThomasWaldmann
Copy link
Copy Markdown
Member

ThomasWaldmann commented Apr 8, 2023

Oops, looks like I did not resolve the merge conflict correctly: the json parameter has to be removed.

@RF-Tar-Railt
Copy link
Copy Markdown
Contributor Author

Oops, looks like I did not resolve the merge conflict correctly: the json parameter has to be removed.

I'll try to fix it...

I noticed that,and I'm doing some fix

@RF-Tar-Railt
Copy link
Copy Markdown
Contributor Author

RF-Tar-Railt commented Apr 8, 2023

maybe fixed in 0a1aaa8

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.

another review (still to do: parseformat and tests).

@RF-Tar-Railt
Copy link
Copy Markdown
Contributor Author

RF-Tar-Railt commented Apr 19, 2023

Oops!Serious misoperation

@RF-Tar-Railt
Copy link
Copy Markdown
Contributor Author

try to fix....

@ThomasWaldmann
Copy link
Copy Markdown
Member

I hope you have a (borg?) backup. :-)

@RF-Tar-Railt
Copy link
Copy Markdown
Contributor Author

I hope you have a (borg?) backup. :-)

Luckily it hasn't changed in my local env

@RF-Tar-Railt
Copy link
Copy Markdown
Contributor Author

I'm trying to create a new branch and push

@RF-Tar-Railt
Copy link
Copy Markdown
Contributor Author

#7534

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.

3 participants