Skip to content
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

fix: disallow child unignoring parent directory ignore #116

Merged
merged 1 commit into from
Nov 28, 2023
Merged

fix: disallow child unignoring parent directory ignore #116

merged 1 commit into from
Nov 28, 2023

Conversation

mohd-akram
Copy link
Contributor

@mohd-akram mohd-akram commented Nov 24, 2023

This solves issues with npm pack (this library is used by npm-packlist which implements the command).

References

Related to npm/cli#7007

@mohd-akram mohd-akram requested a review from a team as a code owner November 24, 2023 13:12
@wraithgar wraithgar self-assigned this Nov 28, 2023
@wraithgar
Copy link
Member

Perhaps I'm not seeing it clearly enough, but I don't see how this distinguishes between a file that was excluded in the parent which is now being included, and a file that is in a directory that was excluded in the parent which is now being included.

The former should still work, if I read the spec correctly.

@mohd-akram
Copy link
Contributor Author

You're right. In my testing I thought git was ignoring both categories but it seems I mis-tested. I've adjusted the PR and added new tests. The added test fails on the main branch as expected.

@wraithgar
Copy link
Member

I think you're on the right path here. Since included can only be set to false inside

if (this.parent && this.parent.filterEntry) {

perhaps this new if statement can also move inside that block?

Also I think an explicit test for this would be better than trying to shoehorn into an existing one. The existing one was almost certainly not made with this in mind, and a new test would be best for isolating exactly the behavior we want to test for.

@mohd-akram
Copy link
Contributor Author

Makes sense. I've updated the PR.

@wraithgar
Copy link
Member

Great! Almost there. It looks like the test includes "directories that are ignored can't have files in it be unignored" but does it have "files that are ignored can be unignored"?

@mohd-akram
Copy link
Contributor Author

@wraithgar Those are tested in test/nested-ignores.js already.

@wraithgar wraithgar changed the title fix: disallow child going against parent ignore fix: disallow child going against parent directory ignore Nov 28, 2023
@wraithgar wraithgar changed the title fix: disallow child going against parent directory ignore fix: disallow child unignoring parent directory ignore Nov 28, 2023
@wraithgar wraithgar merged commit 0bb0972 into npm:main Nov 28, 2023
28 checks passed
@github-actions github-actions bot mentioned this pull request Nov 28, 2023
@mohd-akram mohd-akram deleted the fix-misbehaving-child branch November 28, 2023 18:31
@wraithgar
Copy link
Member

This closed #17 I believe. If not we can reopen it.

wraithgar pushed a commit that referenced this pull request Nov 29, 2023
The fix in #116 was not quite complete. git does allow unignoring
directories as long as its path to the root is unignored. This modifies
the check to handle this case. The test case in `nested-ignores.js` was
indeed wrong because it interpreted unignoring a specific file
`!/h/c/d/hcd` as unignoring the directory `!/h/c/d`. I added new tests
that specifically unignore a directory and therefore allow unignoring
anything else within it.

## References
Related to #116
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants