-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Finder] Look for gitignore patterns up to git root #43239
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
Conversation
julienfalque
commented
Sep 28, 2021
•
edited
Loading
edited
Q | A |
---|---|
Branch? | 5.4 |
Bug fix? | no |
New feature? | yes-ish |
Deprecations? | no |
Tickets | #43150 (comment) |
License | MIT |
Doc PR | - |
89cd65d
to
a8ea69f
Compare
a8ea69f
to
6ec6a5b
Compare
src/Symfony/Component/Finder/Iterator/VcsIgnoredFilterIterator.php
Outdated
Show resolved
Hide resolved
6ec6a5b
to
f102c90
Compare
f102c90
to
b3aa147
Compare
src/Symfony/Component/Finder/Iterator/VcsIgnoredFilterIterator.php
Outdated
Show resolved
Hide resolved
4f9c361
to
5a5db1d
Compare
src/Symfony/Component/Finder/Iterator/VcsIgnoredFilterIterator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Finder/Iterator/VcsIgnoredFilterIterator.php
Outdated
Show resolved
Hide resolved
229d961
to
550d7ab
Compare
src/Symfony/Component/Finder/Iterator/VcsIgnoredFilterIterator.php
Outdated
Show resolved
Hide resolved
550d7ab
to
e3037c3
Compare
e3037c3
to
475d099
Compare
I think this PR is ready but I'd like to have more feedback on the open discussions before addressing them. |
I think I agree with this comment: |
Could you please send a PR that only fixes the infinite loop before this one is finished? |
…t (julienfalque) This PR was merged into the 5.4 branch. Discussion ---------- [Finder] Add .gitignore nested negated patterns support | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Extracted from #43239. Commits ------- 559639b [Finder] Add .gitignore nested negated patterns support
1f16455
to
e4a38aa
Compare
(don't forget to remove the draft status when you're ready) |
e4a38aa
to
08ebeab
Compare
1fe0fe4
to
d6ae6aa
Compare
… (julienfalque) This PR was merged into the 4.4 branch. Discussion ---------- [Process] Run `open_basedir` tests in separate processes | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - While working on #43239, I noticed that when a test sets `open_basedir`, the setting is not reset to its initial value after the test, which can have an impact on subsequent tests. I could not find the reason why the setting cannot be reset. I assume this is for security reasons, so the solution is to always run such test in a separate process. This only concerns two tests for now. Before running those in separate process, the first one prevented subsequent tests in the same class from being run because they are skipped when `open_basedir` is set, which was always true. Commits ------- 07fbfbf Run `open_basedir` tests in separate processes
What's the status of this PR? Is it ready to be merged? |
As I remember the test failure looked unrelated to changes here so yes, it is ready to be merged. I'm going to rebase it to make sure all tests pass. |
d6ae6aa
to
c8923fa
Compare
c8923fa
to
744392d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just rebased again to be sure tests pass (too much noise currently).
Thank you @julienfalque. |
…ectory (nickvergessen) This PR was merged into the 6.4 branch. Discussion ---------- [Finder] Also consider .git inside the basedir of in() directory | Q | A | ------------- | --- | Branch? |6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix described below | License | MIT ### Bug description Previously when the `in()` directory was the root of your repository the `.gitignore` was skipped from consideration, which meant potentially all local files are ignored. E.g. in the Nextcloud community we have the server locally with an `.gitignore` for the `apps/` directory. Now when an app developer develops their own app locally, they put their own git repository inside this ignored `apps/` folder. The CS Fixer rules we use since a long time are: ```php (new Finder()) ->ignoreVCSIgnored(true) ->notPath('l10n') ->notPath('vendor') ->notPath('vendor-bin') ->in(__DIR__); ``` and this worked pretty well with 5.4 and previous versions of the finder. Now that the Nextcloud project finally dropped PHP 8.0 for the latest version and we get the update to Symfony 6.4 the bug become visible locally, but our CI still works as we don't need the "server" repository as a parent when running the CS fixer. ### Locally - nested inside "server" repository ``` ~/Nextcloud/server/apps/spreed$ composer run cs:check > php-cs-fixer fix --dry-run --diff PHP CS Fixer 3.54.0 15 Keys Accelerate by Fabien Potencier, Dariusz Ruminski and contributors. PHP runtime: 8.2.18 Loaded config default from "/home/nickv/Nextcloud/30/server/appsbabies/spreed/.php-cs-fixer.dist.php". Using cache file ".php-cs-fixer.cache". 0 [░░░░░░░░░░░░░░░░░░░░░░░░░░░░] ``` :x: No files found, all ignored by `server/.gitignore` ### CI without a parent repository ``` Run composer run cs:check || ( echo 'Please run `composer run cs:fix` to format your code' && exit 1 ) > php-cs-fixer fix --dry-run --diff PHP CS Fixer 3.54.0 15 Keys Accelerate by Fabien Potencier, Dariusz Ruminski and contributors. PHP runtime: 8.1.2-1ubuntu2.13 Loaded config default from "/home/runner/actions-runner/_work/spreed/spreed/.php-cs-fixer.dist.php". 0/502 [░░░░░░░░░░░░░░░░░░░░░░░░░░░░] 0% ``` :white_check_mark: Correct file list found ### 5.4 - From my POV this is a regression from #43239 (so 6.1+ only) and it becomes more "obvious" when backporting the unit test to 5.4 as it passes there without patching `VcsIgnoredFilterIterator.php`. - Therefore cc-ing `@julienfalque` as author, to clarify if this was intentional Commits ------- af870c6 [Finder] Also consider .git inside the basedir of in() directory