WIP: Do not generate insights for files ignored by VCS

[shrink]

Q	A
Bug fix?	no
New feature?	yes

Per #248

I wanted to add the VCS ignore as a configurable option and it looked pretty difficult to do with the current implementation, so I did a little refactor and moved the VCS ignore setting on the Finder into the class responsible for instantiating the LocalFileRepository.

This is marked as WIP because I still need to determine the right approach to make it configurable, but I've run out of time today. I'll come back to this ASAP but if anyone else wants to take over it, feel free. I think it needs to be configurable to maintain backwards compatibility.

Also if this refactor doesn't meet expectations -- i.e: isn't the approach that is preferred -- I won't be offended by a rejection or requested changes :-)

[olivernybroe]

I agree, keep it enabled by default as this will be part of v2 and I think it's an advantage to have this enabled then disabled by default 👍

[shrink]

Thank you very much for the feedback @Jibbarth -- I'll make these changes in the next hour, and leave it enabled by default per the feedback from @olivernybroe :)

[shrink]

@Jibbarth

The "ignoreVCSIgnored" option cannot be used by the Finder as the "/home/barth/Projects/phpinsights/src/.gitignore" file is not readable.

Regarding this error: as it turns out, the Symfony Finder is checking for a .gitignore directory in every directory it is told to search when this option is enabled... so it looks like this option is not suitable for a use-case where Finder is being used to search outside of the project root. Unfortunately, as it looks right now, I don't see a way around this issue with ignoreVCSIgnored without submitting a change to the Finder library itself. I think we can still implement this .gitignore feature if we implement new logic to look for a .gitignore file in just the project root and then copy the underlying Finder behaviour by using Finder's Gitignore::toRegex() to add the files to the exclude list before we do a search.

I think that's a reasonable solution that doesn't introduce much complexity, but not sure if that's beyond the scope of what we're trying to achieve here: is this functionality important enough to introduce .gitignore logic? I think so but wanted to check before doing it -- and to check I haven't missed an obvious workaround! :-)

[50bhan]

Yes, I believe we can have this by default and it isn't necessary to be strict about BC in V2.0. Good job @shrink :+1:

[Jibbarth]

@shrink

Found the related issue in Symfony repo https://github.com/symfony/symfony/issues/30739

I think it would be more complicated than expected :thinking:

We should also handle .gitignore that could be in subdirectory, in addition of the main .gitignore. For example :

.
├── .gitignore
├── public
│   ├── .gitignore
│   └── index.php
└── src
    ├── Controller
    │   └── .gitignore
    └── Kernel.php

And when we launch analyse on subdirectory, should we go back up the tree to find a .gitignore that could be used ?

I can't prevent you to work on this, because the feature could be really awesome, but it should be really well tested :wink:

Let me know if I can help :slightly_smiling_face:

[JustSteveKing]

Closing as stale PR