Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Sort rsync include/exclude options according to specificity (#561, #1420) #1895

Draft
wants to merge 79 commits into
base: dev
Choose a base branch
from

Conversation

DerekVeit
Copy link
Contributor

This branch is to address issues #561 and #1420 and possibly other circumstances that might arise where the existing strategy for ordering the --include= and --exclude= options to rsync don't quite work.

The existing code has ordered the options in groups. There has been some trial-and-error trying to get that ordering correct where fixing it for one case can easily break another. This proposed fix is to solve it with a more general strategy.

The general principle is that the user would expect more-specific configuration to supersede less-specific. For example, if one rule (include or exclude) applies to "/home" and another rule contradicts that for "/home/user", it is straightforward to assume that the user wants the second rule to apply to "/home/user" and the first rule to apply to anything else in "/home". The rsync command follows its include/exclude options as first-match-wins, so adding the options in order from most to least specific, irrespective of whether they are including or excluding, should give us the correct result. This is accomplished by reverse sorting the paths plus some special treatment for glob patterns and relative paths.

The only change is in Snapshots.rsyncSuffix, and the sorting is done in a new method, Snapshots.pathSelection. (It looks like I should rename this new method in snake case for the project's movement toward PEP 8.)

There are integration-level tests (test_rsync_selections.py) that check various cases, including for #561 and #1420. They call the Snapshots.backup method. For now, the revised rsyncSuffix still uses the original strategy unless a SELECTIONS_MODE attribute of the Config object exists and is set to "sorted". The tests use this to test cases with both strategies, original and sorted.

To make it easy to test various cases of existing file structures and include/exclude configurations, there is a test/filetree.py module for describing file structures with a string, e.g.:

"""
    var/
        log/
            dmesg
            syslog
"""

And the test cases are specified in a text format that uses those strings, e.g. in test/selection_cases. This is to make it easy to add more cases and also to make it very clear what each case is testing.

:exclude-name
    includes
        /var
    excludes
        log
    files_tree
        var/
            lib/
                foo
                log
            log/
                dmesg
                syslog
    expected_tree
        var/
            lib/
                foo

The tests use pytest for parametrize and fixtures, and there are fixtures defined in test/conftest.py for the Config object and the Snapshots object.

For some verbose logging from the test functions there is a simple log function in test/logging.py.

The new code does not yet include handling the EncFS feature. This might be as simple as adding encode = self.config.ENCODE and applying that in the right places as in rsyncInclude and rsyncExclude, but I haven't looked at that closely enough to see what I need for testing it, so I have left it alone so far. In this respect, the new code is definitely not ready. (I know the consideration of removing EncFS is only a maybe-sometime idea for now.)

I'm interested in BiT maintainers' thoughts, questions, and opinions.

SELECTION_MODE is probably only for temporary development testing purposes and can be eliminated, and rsyncSuffix might be simplified. The note I put in the docstring might be addressed or removed.

Based on re-reading CONTRIBUTING.md, I will convert the new tests from pytest to unittest. Let me know if you would prefer that I not add the ddt library for the parameterizing.

The pathSelections method is more sprawling than I would like.

The log function is not essential to anything. I find it useful to have easy, verbose logging from my tests, especially while working on them, but it can be removed.

I have a note in pathSelections suggesting that the GUI somehow alert the user if they include and exclude the exact same path. I think the GUI should refuse to accept the configuration update until the conflict is resolved too.

I thought I had read the CONTRIBUTING guidelines already, but I see some items in those for me to address.

Ironic that I have always used mostly single quotes and just earlier this year forced myself to start using mostly double quotes to be more aligned with what I started to think was a bit more generally favored, e.g. with black. Easy fix, though. I do use black on my new code, interactively, not blindly. It does have a setting to not force the double quotes.

Derek Veit added 30 commits October 3, 2024 16:30
@DerekVeit
Copy link
Contributor Author

Updates since 52c7478:

  • converted the integration tests from pytest to unittest.
  • added ddt for those tests.
  • added pyyaml to replace my made-up format for the test data.

I still need to convert my other pytest tests and address some other things.

@buhtz
Copy link
Member

buhtz commented Oct 18, 2024

I see I have a lot to learn with this PR. Very nice.

@buhtz
Copy link
Member

buhtz commented Oct 18, 2024

Hello Derek,
for your info. Before merging your PR and PR #1897 I will do release 1.5.3. This will take some time because I will do a release candidate first hope to find some testers for it.

After 1.5.3 I can work and test yours and PR #1897. When they are merged I will soon target a 1.6.0 release.

Regards,
Christian

@DerekVeit
Copy link
Contributor Author

OK. Although the tests pass, I'm still working on this pretty much. I'm pushing it here mainly for visibility of what I'm doing. I'm also probably going to revise my tests to use pyfakefs. And it is still not taking EncFS into account. So I don't see it as ready for merging yet.

@buhtz buhtz added the PR: Modifications requested Maintenance team requested modifications and waiting for their implementation label Nov 14, 2024
@buhtz buhtz changed the title Sort rsync include/exclude options according to specificity (#561, #1420) fix: Sort rsync include/exclude options according to specificity (#561, #1420) Nov 19, 2024
@buhtz
Copy link
Member

buhtz commented Nov 27, 2024

Hello Derek,
Just for your information. I do plan the next release (1.6.0) before Christmas.

How is your schedule? Can I plan your PR for this release (1.6.0) or for the release after it (1.6.1)?
No need to rush. I just need to know for the overall planing.

Regards,
Christian

@DerekVeit
Copy link
Contributor Author

No, unfortunately I have been away from it for all of this month, so it still needs the handling of EncFS, etc. I will be getting back to it soon, though.

@buhtz
Copy link
Member

buhtz commented Nov 27, 2024

No, problem. Thank you for reaching out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Modifications requested Maintenance team requested modifications and waiting for their implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants