Skip to content

[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

Merged
merged 1 commit into from
Apr 1, 2022

Conversation

julienfalque
Copy link
Contributor

@julienfalque julienfalque commented Sep 28, 2021

Q A
Branch? 5.4
Bug fix? no
New feature? yes-ish
Deprecations? no
Tickets #43150 (comment)
License MIT
Doc PR -

@carsonbot carsonbot added this to the 5.4 milestone Sep 28, 2021
@julienfalque julienfalque force-pushed the finder-recursive-gitignore branch 2 times, most recently from 89cd65d to a8ea69f Compare September 28, 2021 20:14
@julienfalque julienfalque marked this pull request as draft September 28, 2021 20:14
@julienfalque julienfalque force-pushed the finder-recursive-gitignore branch from a8ea69f to 6ec6a5b Compare September 28, 2021 22:08
@julienfalque julienfalque force-pushed the finder-recursive-gitignore branch from 6ec6a5b to f102c90 Compare September 29, 2021 06:42
@julienfalque julienfalque marked this pull request as ready for review September 29, 2021 06:51
@julienfalque julienfalque force-pushed the finder-recursive-gitignore branch from f102c90 to b3aa147 Compare September 29, 2021 07:01
@julienfalque julienfalque changed the title [Finder] Improve .gitignore support tests [Finder] Fix recursive .gitignore support for Windows and root directory search Sep 29, 2021
@julienfalque julienfalque force-pushed the finder-recursive-gitignore branch 2 times, most recently from 4f9c361 to 5a5db1d Compare September 30, 2021 06:25
@julienfalque julienfalque force-pushed the finder-recursive-gitignore branch 2 times, most recently from 229d961 to 550d7ab Compare October 2, 2021 12:04
@julienfalque
Copy link
Contributor Author

I think this PR is ready but I'd like to have more feedback on the open discussions before addressing them.

@nicolas-grekas
Copy link
Member

I think I agree with this comment:
#43239 (comment)

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 25, 2021

Could you please send a PR that only fixes the infinite loop before this one is finished?
Tests on appveyor are failing since a month and it'd be great making them green asap.

nicolas-grekas added a commit that referenced this pull request Nov 2, 2021
…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
@julienfalque julienfalque force-pushed the finder-recursive-gitignore branch from 1f16455 to e4a38aa Compare November 2, 2021 16:47
@julienfalque julienfalque changed the title [Finder] Improve .gitignore support [Finder] Look for gitignore patterns up to git root Nov 2, 2021
@nicolas-grekas
Copy link
Member

(don't forget to remove the draft status when you're ready)

@fabpot fabpot modified the milestones: 5.4, 6.1 Nov 16, 2021
@julienfalque julienfalque force-pushed the finder-recursive-gitignore branch from e4a38aa to 08ebeab Compare December 19, 2021 16:22
@julienfalque julienfalque force-pushed the finder-recursive-gitignore branch 4 times, most recently from 1fe0fe4 to d6ae6aa Compare December 19, 2021 17:19
@julienfalque julienfalque marked this pull request as ready for review December 19, 2021 17:29
nicolas-grekas added a commit that referenced this pull request Dec 27, 2021
… (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
@fabpot
Copy link
Member

fabpot commented Mar 26, 2022

What's the status of this PR? Is it ready to be merged?

@julienfalque
Copy link
Contributor Author

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.

@julienfalque julienfalque force-pushed the finder-recursive-gitignore branch from d6ae6aa to c8923fa Compare March 26, 2022 12:12
@fabpot fabpot force-pushed the finder-recursive-gitignore branch from c8923fa to 744392d Compare April 1, 2022 06:24
Copy link
Member

@fabpot fabpot left a 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).

@fabpot
Copy link
Member

fabpot commented Apr 1, 2022

Thank you @julienfalque.

@fabpot fabpot merged commit 9fbe216 into symfony:6.1 Apr 1, 2022
@julienfalque julienfalque deleted the finder-recursive-gitignore branch April 1, 2022 07:04
@fabpot fabpot mentioned this pull request Apr 15, 2022
fabpot added a commit that referenced this pull request Apr 27, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants