It would be great if we can move the file filter check into its own helper method that we could reuse across the local and s3 listing methods. So something like this:

def _should_ignore_from_file_filter(self, path):
      return (
           self.file_filter is None and
           not list(self.file_filter.call([FileInfo(path)]))
      )

This will make it a bit more reusable and improve readability as we are reducing the level of nesting/complexity in the if statements.

It would be great if we could consolidate the calls to the _list_objects_raw() to a single instance to help simplify the logic in list_objects():

def list_objects(self, s3_path, dir_op):
     for path, data in self._list_objects_raw(s3_path, dir_op):
         if self._should_ignore_from_file_filter(path):
             continue
         yield path, data

For this method let's add an underscore to it to make it _list_objects_raw to indicate it's a private method and is only used within this class.

I'm still not sure I understand the case this logic is optimizing for after reading this comment: #5425 (comment) and the code. Would it be possible to illustrate the use/edge case with a sample directory structure and CLI commands? I think that would be really helpful.

In general, I'd prefer we do not change anything in the filters.py logic if we do not need to. Mainly the filters.py module is one of the older modules in the s3 package and we rarely update it. I think before adding any additional complexity to it, we do some refactoring of it to make it easier to follow/work with.

Thanks. I have to focus on another project for the time being, but I will circle back to this later for the technical changes requested.

I'm still not sure I understand the case this logic is optimizing for after reading this comment: #5425 (comment) and the code. Would it be possible to illustrate the use/edge case with a sample directory structure and CLI commands?

The use case is fairly simple:

aws s3 sync dir s3://bucket/dir '--exclude excluded/*' --include 'excluded/included/*'

If we make the FileGenerator no longer recurse into excluded directories, then excluded/included would never be examined and any files therein would have no chance to be included. What is needed is a specific include filter for excluded/included. We could make the user do it:

aws s3 sync dir s3://bucket/dir '--exclude excluded/*' --include excluded/included --include 'excluded/included/*'

...but that seems onerous and would break current behavior. In my PR, this is now verified by the commit with description add a functional test for "aws s3 sync" include/exclude.

The best solution I thought of was to auto-insert the necessary filter, since we know the existing filtering logic does what we need. I wanted to keep the special-case code to a minimum.

Thanks for the detailed examples that was really helpful in understanding it! In general with this PR, we definitely need to make sure we do not break current behavior (i.e. given any set of filters + file set, the CLI will continue to include/exclude the same set of files after this change with no needing to adjust the filters provided at the command line).

While it looks like the coded that got added does handle the missing edge case that you illustrated, I am concerned that we are going to be missing other edge cases after seeing the edge case you raised. For example, suppose we had this directory layout:

$ tree .
.
├── directory
│   ├── another-dir
│   │   ├── exclude-me.txt
│   │   └── include-me.py
│   ├── exclude-me.txt
│   └── include-me.py
├── exclude-me.txt
└── include-me.py

If we only wanted to upload the .py files, a user would likely run a command like this:

$ aws s3 cp . s3://mybucket --recursive --exclude '*' --include '*.py' --dryrun

This would result in all of the .py files being uploaded:

(dryrun) upload: directory/another-dir/include-me.py to s3://mybucket/directory/another-dir/include-me.py
(dryrun) upload: directory/include-me.py to s3://mybucket/directory/include-me.py
(dryrun) upload: ./include-me.py to s3://mybucket/include-me.py

However, with the approach we are currently taking, we would still not traversing the directory and another-dir to get all of the .py files:

$ aws s3 cp . s3://mybucket --recursive --exclude '*' --include '*.py' --dryrun
(dryrun) upload: ./include-me.py to s3://mybucket/include-me.py

In general, I'm not optimistic the original approach of just applying the filters to the directory before traversing it is going to work. In terms of options to pursue, I think we either:

• Need to expand the logic of whether we traverse a directory to skip traversing a directory only if it is impossible for a file inside of it to be included based on the filters. I'm not sure if there is a straightforward way of doing this as we would need to account for all supported pattern symbols: https://awscli.amazonaws.com/v2/documentation/api/latest/reference/s3/index.html#use-of-exclude-and-include-filters

• Scope the optimization of not traversing directories to simple set of exclude/include patterns (e.g. we only apply this optimization to only filters that have only --exclude flags and/or apply the optimizations if there are no special characters used in the filter (e.g. *, ?, [], [!])

Thoughts? I want to think about this a bit more to see if I have any better suggestions, but I at least wanted to put this realization down now to start the discussion.

Hmm, that is rough.

Your first idea seems best, but I don't know how to do that either.

A third idea would be to give the user control, either by a global option --no-traverse-excludes or a separate argument --hard-exclude <path> (to replace --exclude <path>). This isn't ideal, since it would be best if the program does the right thing automatically, but it may be more straightforward to implement and would not have unexpected consequences for current uses.