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

Add warning for dotfiles in info plugin #7142

Merged
merged 1 commit into from
May 3, 2024

Conversation

kamilkrzyskow
Copy link
Collaborator

@kamilkrzyskow kamilkrzyskow commented May 1, 2024

Attempt at handling #7125

I admit the PR is a bit "noisy", for such a small feature, but I think this is the best I can do, given that there are 2 separate for loops for directory/files paths.
There are 2 warnings, during the processing phase and later in the summary. I want to make sure people see it.
Perhaps the display of the list at the end is unnecessary and not so convenient, as I think it is?

Please name your bug report (2-4 words): gmc
WARNING -  The following .dotpath will be included: C:\MyFiles\Gothic Modding\git\gmc\.git
WARNING -  The following .dotpath will be included: C:\MyFiles\Gothic Modding\git\gmc\.github
WARNING -  The following .dotpath will be included: C:\MyFiles\Gothic Modding\git\gmc\.gitignore
Processing: C:\MyFiles\Gothic Modding\git\gmc\.git\objects\pack
List of potentially sensitive files:
  - C:\MyFiles\Gothic Modding\git\gmc\.git
  - C:\MyFiles\Gothic Modding\git\gmc\.github
  - C:\MyFiles\Gothic Modding\git\gmc\.gitignore
WARNING -  Archive contains potentially sensitive dotfiles.
           Please review and share only necessary data to reproduce the issue.

EDIT:
I will try to modify the actual summary file list and change color of the dotfiles. This should remove the need for another list.

@kamilkrzyskow kamilkrzyskow marked this pull request as draft May 1, 2024 22:42
@squidfunk
Copy link
Owner

PR looks good so far, thanks! This also means that a .meta.yml will trigger a warning, right? I guess this is okay, let's see how users will react to it.

@kamilkrzyskow kamilkrzyskow marked this pull request as ready for review May 2, 2024 21:16
@kamilkrzyskow
Copy link
Collaborator Author

kamilkrzyskow commented May 2, 2024

I removed the second list, and instead changed the color of the dotpaths. This has lead to creating a second list anyway, because the file names are sorted, and the yellow color has a higher ANSI value, therefore all dotpaths are ordered last. I also removed some conditionals, when clearing the indicator, because it turned out to be unnecessary.

I wanted to print a count of the dotpaths, but the results highlight also the contents of dotdirectories, so the count wouldn't match from what was initially detected, to keep it simple I don't count anything now.

I think it's cleaner than the initial PR, but let me know what you think ✌️

This also means that a .meta.yml will trigger a warning, right?

Yes, I didn't really plan to handle such cases, because we'd have to read config setting from other plugins. Even though it's only meta and blog now 🤔
But as you pointed out it might detect false positives, so I changed the wording a bit.

@squidfunk squidfunk self-requested a review May 3, 2024 06:12
Copy link
Owner

@squidfunk squidfunk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I guess we can give it a spin and see if reports for .meta.yml and other non-critical files lead to confusion. The info plugin is something we should try different approaches on until we find the one that works best ☺️

@squidfunk
Copy link
Owner

@kamilkrzyskow you can merge it if it's ready to go 🚀

@kamilkrzyskow
Copy link
Collaborator Author

Okey dokey 👌

@kamilkrzyskow kamilkrzyskow merged commit 0e0a678 into squidfunk:master May 3, 2024
4 checks passed
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.

None yet

2 participants