Find/Replace in Files: don't exclude items that trigger the exclude filter above the search location

Review Request #127908 - Created May 13, 2016 and updated

Information
René J.V. Bertin
kdevplatform
Reviewers
kdevelop

"Find/Replace in Files" has an include and an exclude filter. The former serves to consider only files matching a pattern under the search location. The latter serves to exclude files matching a pattern, but considers the full path.

The standard exclude pattern contains the pattern /build/, to exclude the build directory. Filtering on the full path means that it is not possible to search in a location that has an ancestor called build.

This patch attempts to mask the patch above the search location from the exclude filter.

It could even mask the search location itself from that filter, which would allow to search e.g. in a typical build directory by setting the location to that directory, and without having to edit the exclude filter.

I'm not very proud of the expression used to determine the location's dirName, but I saw no better way since it presumably has to be a canonical (normalised) path and there is no dirName() method.

Search now works as I'd expect (when searching the files of a project) regardless of where the search location is stored.

René J.V. Bertin
Review request changed

Change Summary:

Somehow KDevelop5's ReviewBoard upload feature failed to include the actual diff. It's been wonky on OS X for a while now (but usually it only fails to show the RR URL in the confirmation dialog).

Diff:

Revision 1 (+5 -2)

Milian Wolff

personally, I think you should instead check the exclude filter against the start location of the search. If that is already excluded, show a helpful message to the user explaining that nothing can possibly be matched and that he needs to change the filter then

  1. If we ignore opinions on whether there is an issue for an instant, does my patch seem an appropriate and correct solution?

    I don't get why you would apply an exclude filter on data that is irrelevant. You're searching in a location, anything outside that location is excluded by default and it simply doesn't make sense to apply filters on it IMHO. There are also performance implications to it, though that's probably only a theoretical argument.
    Take the case of users who have to work on say, a shared partition called "build", on which each has his/her own space for development. Supposing they put a proper build directory inside the source tree, they will just have to put up with a search that includes that directory if we follow your suggestion.

    IIRC there's also a discrepancy between the regular and the "search only project files" modes, I'm pretty sure that latter mode wasn't affected (I'm running my own patch so cannot verify this quickly).

    There is of course a way to work around the particular instance of the issue that made me realise there is an issue to begin with. If the CMake project manager uses out-of-source build directories by default the default exclude filter no longer needs to include the build directory.
    I think this is a safe thing to do. At MacPorts we've made out-of-source building the default for all ports that use CMake because it's something that CMake supports inherently. If there are projects where this is not supported they are so few that I didn't pick up any echos of it. It's always been the default for all KDE projects.

  2. I would agree that this is a bug: the exclude filter should only match against the part of the path which is below the root dirs where the search is done (like e.g. the arguments to "find" cmdl tool also apply on the dir names traversed, and not the starting one).
    Not yet run into this personally, but I can see how a few people might have "build" in their root dir, or trying to exclude other custom subdirs which are generic enough to also be used for other purposes in high level dir structures ("kde", "docs", "templates", "other", "data", "src"). Chance is low, but there is. And fix should not be too complicated.

    Not sure about the patch though, only quickly looked at it. Will have to look into how QDir::match actually works and then at the context code.

  3. Thanks for reviving this. I'm still using the patch (which still applies cleanly to the 5.1 branch at least)

Loading...