Skip to content

[Finder]: Regression of gitIgnore unable to exclude a directory and its content #45048

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.

Sign up for GitHub

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

Jump to bottom
Closed
carlalexander opened this issue Jan 17, 2022 · 4 comments
Closed

[Finder]: Regression of gitIgnore unable to exclude a directory and its content #45048

carlalexander opened this issue Jan 17, 2022 · 4 comments
Labels
Bug Finder Status: Needs Review

Comments

@carlalexander
Copy link

carlalexander commented Jan 17, 2022 •
edited
Loading

Symfony version(s) affected

5.4.0 or greater

Description

I'm opening this as a regression of #39257. .gitignore parsing for this use case is broken again on Symfony 5.4.

How to reproduce

The example from the issue doesn't work anymore:

/example/**
!/example/example.txt
!/example/packages
!/example/packages/example.yaml
!/example/test/

Possible Solution

I've tried to narrow down the issue. It seems to be related to this PR. Specifically, this code. This seems to abort the gitignore processing even though the file should be included.

I think the issue is that the code doesn't check if the .gitgnore file that we're processing is in an ignored directory or not. It just aborts if we're an ignored directory which makes it ignore all the exclude rules. I think the fix for this is just to change the order of the guard clauses like so:

if (null === $regexps = $this->readGitignoreFile("{$parentDirectory}/.gitignore")) {
    continue;
}

if ($this->isIgnored($parentDirectory)) {
    $ignored = true;

    // rules in ignored directories are ignored, no need to check further.
    break;
}

$fileRelativePath = substr($fileRealPath, \strlen($parentDirectory) + 1);

This would mean that the isIgnored check only kicks in if we found a .gitignore in the parentDirectory. In my testing, it seemed to solve the issue. But I don't have a complete understanding of the PR to know if I broke anything with it. If @julienfalque approves, happy to make a PR for this.

Additional Context

No response
carlalexander added a commit to ymirapp/cli that referenced this issue Jan 17, 2022
@carlalexander
revert: revert symfony finder to 4.4 branch due to bug
2bfe710 
see: symfony/symfony#45048
@julienfalque
Copy link
Contributor

Is the issue the same as #39257, i.e. keeping the example/test directory but not its content?

@carlalexander
Copy link
Author

It doesn't keep the directory either

@julienfalque julienfalque mentioned this issue Jan 20, 2022
Merged
@julienfalque
Copy link
Contributor

See #45095.

nicolas-grekas added a commit that referenced this issue Jan 26, 2022
@nicolas-grekas
bug #45095 [Finder] Fix finding VCS re-included files in excluded dir…
79d1101 
…ectory (julienfalque)

This PR was squashed before being merged into the 5.4 branch.

Discussion
----------

[Finder] Fix finding VCS re-included files in excluded directory

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | #45048
| License       | MIT
| Doc PR        | -

This PR fixes a bug where `Finder` would not return a file inside a directory when gitignore rules exclude the directory but re-include the file.

The first commit makes `Finder` return the file. Though, as shown by the second commit, the directory is still not returned. Strictly speaking, the directory *is* ignored, so in some sense `Finder` is correct. But then it contains a file that is not ignored. In this case, Git would see the directory anyway. Should `Finder` return any ignored directory as soon as it contains a file that is not ignored? Even if that file is not part of `Finder` results?

Commits
-------

d108ac5 [Finder] Fix finding VCS re-included files in excluded directory
@carlalexander
Copy link
Author

Thank you @julienfalque! I just tested the new release, and it works great! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Finder Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@nicolas-grekas @carlalexander @julienfalque @carsonbot