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

docs: add precedence info to manpage & fix noblacklist example #6359

Conversation

smheidrich
Copy link
Contributor

This should resolve or at least begin to resolve issues mentioned in #6358.

Fixes #6358.

Copy link
Collaborator

@glitsj16 glitsj16 left a comment

Choose a reason for hiding this comment

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

Looks good. Holding of a bit on approval as there might be additional work incoming.

@@ -1729,6 +1736,10 @@ See --keep-config-pulse.
Disable blacklist for this directory or file.
.br

Note that this only applies to the path as written when it was blacklisted,
including unexpanded variables and wildcards.
Copy link
Collaborator

@rusty-snake rusty-snake May 26, 2024

Choose a reason for hiding this comment

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

Not sure this is correct.

If somebody wants to test, here are the test cases to construct the test matrix:

  • "simple" paths
  • macros w/o subpaths (e.g. ${PICTURES}, ${DOCUMENTS}, ...)
  • macros w/ subpaths (e.g. ${HOME})
  • macros with multiple maths (${PATH} - the handling of this macro is more special than the other ones)
  • wildcards (*, ...) in only (no)blacklist and both.

Or read the code to find out whether it is a sting comparison or and inode comparison (no it's not, it's just the other extreme) or anything in between.

Copy link
Contributor Author

@smheidrich smheidrich May 26, 2024

Choose a reason for hiding this comment

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

You're right, the behavior is different for wildcards, in that wildcard blacklists can be partially unblacklisted by --noblacklisting individual expansions instead of only the whole wildcard.

As for variables, I only tested it for ${PATH} - didn't realize this was a special case, but in retrospect it makes sense that it is 🤦

UPDATE Also tested it for ${HOME} now and the behavior is more similar to wildcards than ${PATH}, i.e. it's possible to unblacklist by giving the expanded path. Super confusing...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All right, I tested the ${PATH} situation again with another executable I created myself just to be sure that it isn't specific to nc for some reason, and yes, ${PATH} really does behave differently than all other expansions (variables & wildcards).

I'm inclined to say that this is probably a bug and hence shouldn't be documented but rather get an issue? I'll create one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason ${PATH} is special is because it can expand into multiple paths while ${HOME} and the like are only substituted in the path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue: #6360

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed the paragraph so it accounts for #6360 and links to it, just in case it's decided that #6360 either takes too long to fix or is not considered a bug.

command line, or both) or conflicts with a related option, the
precedence/behavior is option-specific and usually documented in the
\fBOPTIONS\fR section below. Note that any option specified in a profile can be
disabled on the command line using \fB--ignore\fR.
Copy link
Collaborator

Choose a reason for hiding this comment

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

FTR: --profile=firefox --ignore='net none' does not work. It works if you flip the order. Nevermind I think it makes things to complicated to explain every detail here.

Copy link
Contributor Author

@smheidrich smheidrich May 26, 2024

Choose a reason for hiding this comment

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

I've made the statement a bit weaker by adding "generally", so a more detailed explanation can be given in --ignore's own section.

@smheidrich smheidrich force-pushed the improve-manpage-precedence-an-noblacklist-explanation branch from 3ea5ec5 to 05eb2d5 Compare May 26, 2024 18:56
@smheidrich smheidrich force-pushed the improve-manpage-precedence-an-noblacklist-explanation branch from 05eb2d5 to f502359 Compare May 26, 2024 20:36
@kmk3 kmk3 changed the title Add precedence info to manpage & fix noblacklist example docs: add precedence info to manpage & fix noblacklist example May 27, 2024
@kmk3 kmk3 added the documentation Issues and pull requests related to the documentation label May 27, 2024
@kmk3 kmk3 force-pushed the improve-manpage-precedence-an-noblacklist-explanation branch from f502359 to 5eab578 Compare June 8, 2024 11:15
Copy link
Collaborator

@kmk3 kmk3 left a comment

Choose a reason for hiding this comment

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

LGTM.

It seems that the issues raised were addressed and the current wording seems
correct.

By the way, good work on all the testing and follow-ups. Some program behaviors
can be quite unexpected.

Copy link
Collaborator

@glitsj16 glitsj16 left a comment

Choose a reason for hiding this comment

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

LGTM

@kmk3 kmk3 merged commit 630972d into netblue30:master Jun 10, 2024
2 checks passed
kmk3 added a commit that referenced this pull request Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues and pull requests related to the documentation
Projects
Status: Done (on RELNOTES)
Development

Successfully merging this pull request may close these issues.

docs: manpage should explain precedence of CLI options vs profile settings
4 participants