Repository: borgbackup/borg

Issue:
Comparing modified items incorrectly assumes comparable chunk ids
This came up in #2842 (see during https://github.com/borgbackup/borg/pull/2842/files#diff-0c1790c20ac3974e84f97b2ded80507bR449) but the problem was there before https://github.com/borgbackup/borg/blob/master/src/borg/archiver.py#L1028.
The current solution of comparing just chunk ids is fine as long as the two archives were created with the same chunker params, but doesn't work otherwise.
I am not sure what would be the cheapest way to get matching contents from two chunk iterators. Maybe creating new chunks from the chunks with the same params?

Discussion:
codecov-io: # [Codecov](https://codecov.io/gh/borgbackup/borg/pull/2900?src=pr&el=h1) Report
> Merging [#2900](https://codecov.io/gh/borgbackup/borg/pull/2900?src=pr&el=desc) into [master](https://codecov.io/gh/borgbackup/borg/commit/4f57e3a7c4ff9ae2384b6c9b913780136e7e2d75?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/borgbackup/borg/pull/2900/graphs/tree.svg?src=pr&token=LssEpmW3o4&width=650&height=150)](https://codecov.io/gh/borgbackup/borg/pull/2900?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##           master   #2900      +/-   ##
=========================================
- Coverage   86.31%   86.3%   -0.01%     
=========================================
  Files          23      23              
  Lines        8796    8798       +2     
  Branches     1480    1481       +1     
=========================================
+ Hits         7592    7593       +1     
+ Misses        814     813       -1     
- Partials      390     392       +2
```


| [Impacted Files](https://codecov.io/gh/borgbackup/borg/pull/2900?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/borg/archiver.py](https://codecov.io/gh/borgbackup/borg/pull/2900?src=pr&el=tree#diff-c3JjL2JvcmcvYXJjaGl2ZXIucHk=) | `87.58% <100%> (+0.29%)` | :arrow_up: |
| [src/borg/archive.py](https://codecov.io/gh/borgbackup/borg/pull/2900?src=pr&el=tree#diff-c3JjL2JvcmcvYXJjaGl2ZS5weQ==) | `83.37% <0%> (-0.4%)` | :arrow_down: |
| [src/borg/helpers.py](https://codecov.io/gh/borgbackup/borg/pull/2900?src=pr&el=tree#diff-c3JjL2JvcmcvaGVscGVycy5weQ==) | `88.37% <0%> (-0.08%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/borgbackup/borg/pull/2900?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/borgbackup/borg/pull/2900?src=pr&el=footer). Last update [4f57e3a...a724f2f](https://codecov.io/gh/borgbackup/borg/pull/2900?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/src/borg/archiver.py b/src/borg/archiver.py
index 0f2ebd9a..ebb4b5e1 100644
--- a/src/borg/archiver.py
+++ b/src/borg/archiver.py
@@ -1021,18 +1021,19 @@ def contents_changed(item1, item2):
         def compare_content(path, item1, item2):
             if contents_changed(item1, item2):
                 if item1.get('deleted'):
-                    return ('added {:>13}'.format(format_file_size(sum_chunk_size(item2))))
-                elif item2.get('deleted'):
-                    return ('removed {:>11}'.format(format_file_size(sum_chunk_size(item1))))
-                else:
-                    chunk_ids1 = {c.id for c in item1.chunks}
-                    chunk_ids2 = {c.id for c in item2.chunks}
-                    added_ids = chunk_ids2 - chunk_ids1
-                    removed_ids = chunk_ids1 - chunk_ids2
-                    added = sum_chunk_size(item2, added_ids)
-                    removed = sum_chunk_size(item1, removed_ids)
-                    return ('{:>9} {:>9}'.format(format_file_size(added, precision=1, sign=True),
-                                                 format_file_size(-removed, precision=1, sign=True)))
+                    return 'added {:>13}'.format(format_file_size(sum_chunk_size(item2)))
+                if item2.get('deleted'):
+                    return 'removed {:>11}'.format(format_file_size(sum_chunk_size(item1)))
+                if not can_compare_chunk_ids:
+                    return 'modified'
+                chunk_ids1 = {c.id for c in item1.chunks}
+                chunk_ids2 = {c.id for c in item2.chunks}
+                added_ids = chunk_ids2 - chunk_ids1
+                removed_ids = chunk_ids1 - chunk_ids2
+                added = sum_chunk_size(item2, added_ids)
+                removed = sum_chunk_size(item1, removed_ids)
+                return '{:>9} {:>9}'.format(format_file_size(added, precision=1, sign=True),
+                                            format_file_size(-removed, precision=1, sign=True))
 
         def compare_directory(item1, item2):
             if item2.get('deleted') and not item1.get('deleted'):


Test patch:
diff --git a/src/borg/testsuite/archiver.py b/src/borg/testsuite/archiver.py
index 3d2783e1..0d28eb31 100644
--- a/src/borg/testsuite/archiver.py
+++ b/src/borg/testsuite/archiver.py
@@ -3276,9 +3276,10 @@ def test_basic_functionality(self):
         self.cmd('create', self.repository_location + '::test1a', 'input')
         self.cmd('create', '--chunker-params', '16,18,17,4095', self.repository_location + '::test1b', 'input')
 
-        def do_asserts(output, archive):
+        def do_asserts(output, can_compare_ids):
             # File contents changed (deleted and replaced with a new file)
-            assert 'B input/file_replaced' in output
+            change = 'B' if can_compare_ids else '{:<19}'.format('modified')
+            assert '{} input/file_replaced'.format(change) in output
 
             # File unchanged
             assert 'input/file_unchanged' not in output
@@ -3307,9 +3308,10 @@ def do_asserts(output, archive):
             # The inode has two links and the file contents changed. Borg
             # should notice the changes in both links. However, the symlink
             # pointing to the file is not changed.
-            assert '0 B input/empty' in output
+            change = '0 B' if can_compare_ids else '{:<19}'.format('modified')
+            assert '{} input/empty'.format(change) in output
             if are_hardlinks_supported():
-                assert '0 B input/hardlink_contents_changed' in output
+                assert '{} input/hardlink_contents_changed'.format(change) in output
             if are_symlinks_supported():
                 assert 'input/link_target_contents_changed' not in output
 
@@ -3336,9 +3338,9 @@ def do_asserts(output, archive):
             if are_hardlinks_supported():
                 assert 'input/hardlink_target_replaced' not in output
 
-        do_asserts(self.cmd('diff', self.repository_location + '::test0', 'test1a'), '1a')
+        do_asserts(self.cmd('diff', self.repository_location + '::test0', 'test1a'), True)
         # We expect exit_code=1 due to the chunker params warning
-        do_asserts(self.cmd('diff', self.repository_location + '::test0', 'test1b', exit_code=1), '1b')
+        do_asserts(self.cmd('diff', self.repository_location + '::test0', 'test1b', exit_code=1), False)
 
     def test_sort_option(self):
         self.cmd('init', '--encryption=repokey', self.repository_location)