add test for preserved intermediate folder permissions#2477
add test for preserved intermediate folder permissions#2477enkore merged 2 commits intoborgbackup:masterfrom
Conversation
This tests whether the permissions metadata is preserved when a folder is excluded but still recursed into to find a matching file in a subfolder.
|
btw, while writing this test I noticed some strange (possibly incorrect) code at https://github.com/edgimar/borg/blob/cf80cc5afe9586291510e74cc2f53bed6085bd17/src/borg/testsuite/archiver.py#L2735 and https://github.com/edgimar/borg/blob/cf80cc5afe9586291510e74cc2f53bed6085bd17/src/borg/testsuite/archiver.py#L2761 -- what is the purpose of ORing the stat values here? |
Codecov Report
@@ Coverage Diff @@
## master #2477 +/- ##
======================================
Coverage 83.1% 83.1%
======================================
Files 21 21
Lines 7586 7586
Branches 1289 1289
======================================
Hits 6304 6304
Misses 926 926
Partials 356 356Continue to review full report at Codecov.
|
|
@edgimar right, the stat.S_IF* part at both places looks wrong. stat.S_IF* is for interpreting the type-part of the mode and can not be used to set it (AFAIK), so that should be removed at both places. Separate PR please. |
|
any thoughts on whether this test is OK? Is it legitimately testing for metadata preservation? If so, does it mean that we don't need to do anything to try to ensure that "intermediate" nested folders are explicitly included in the backup? |
|
|
||
| def test_create_pattern_intermediate_folder_permissions(self): | ||
| """test for correct metadata when patterns exclude a parent folder but include a child""" | ||
| self.patterns_file_path2 = os.path.join(self.tmpdir, 'patterns2') |
There was a problem hiding this comment.
is "self." needed for this?
src/borg/testsuite/archiver.py
Outdated
| self.create_regular_file('x/b/foo_b', size=1024 * 80) | ||
| with changedir('input'): | ||
| os.chmod('x/a', 0o750) | ||
| os.chmod('x/b', 0o700) |
There was a problem hiding this comment.
maybe use 0o600 here so we have a difference in owner x bit, which is usually never masked by umask.
src/borg/testsuite/archiver.py
Outdated
|
|
||
| for fname, mode in [('x/a', 0o750), ('x/b', 0o700)]: | ||
| st = os.stat(fname) | ||
| self.assert_equal(st.st_mode & 0o777, mode) |
There was a problem hiding this comment.
hmm, wasn't the point of this test to make an assertion about 'x' (which is excluded) rather than 'x/a' and 'x/b', which both are normally included files?
|
I'd approach this differently. Tests using chmod/chown/chgrp and such frequently fall apart (though it does pass) for various reasons. Since the test wants to assert that the parent directories are included, I'd just |
|
@enkore yup, sounds good and easy. |
|
@edgimar can you update your PR? |
Removed last test, and replaced with simpler test to verify that an intermediate folder is included in the archive prior to its contents when a folder is excluded but still recursed into to find a matching file in a subfolder.
|
@ThomasWaldmann done. |
ThomasWaldmann
left a comment
ThomasWaldmann
left a comment
There was a problem hiding this comment.
looks good. before merging, we need to collapse the changesets.
|
Thanks! |
4 participants
This tests whether the permissions metadata is preserved when a folder is excluded but still recursed into to find a matching file in a subfolder.
To my surprise, it passes, which seems to indicate that the concern raised by @ThomasWaldmann in PR #2322 may not be a problem after all.