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.

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

Specifty LC_ALL=C when dealing with sorted lists. #1996

Merged
merged 2 commits into from Feb 24, 2018
Merged

Specifty LC_ALL=C when dealing with sorted lists. #1996

merged 2 commits into from Feb 24, 2018

Conversation

PromoFaux
Copy link
Member

@PromoFaux PromoFaux commented Feb 21, 2018 •
edited

By submitting this pull request, I confirm the following:
please fill any appropriate checkboxes, e.g: [X]

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes, and have included unit tests where possible.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)

Please make sure you Sign Off all commits. Pi-hole enforces the DCO.

What does this PR aim to accomplish?:
An alternative to #1995, without the overhead of re-sorting list.preEventHorizon when comparing it to the whitelist

How does this PR accomplish the above?:
export LC_ALL=C at the top of the script to ensure user locales don't mess thigns up!

What documentation changes (if any) are needed to support this PR?:
None

  • You must follow the template instructions. Failure to do so will result in your pull request being closed.
  • Please respect that Pi-hole is developed by volunteers, who can only reply in their spare time.

Specifty LC_ALL=C when dealing with sorted lists.
d4ac818 
Signed-off-by: Adam Warner <adamw@rner.email>
@PromoFaux PromoFaux mentioned this pull request Feb 21, 2018
Closed
8 tasks
@PromoFaux PromoFaux changed the title Specifty LC_ALL=C when dealing with sorted lists. [Optional/Alternative to# 1995] Specifty LC_ALL=C when dealing with sorted lists. Feb 21, 2018
@PromoFaux PromoFaux changed the title [Optional/Alternative to# 1995] Specifty LC_ALL=C when dealing with sorted lists. [Optional/Alternative to #1995] Specifty LC_ALL=C when dealing with sorted lists. Feb 21, 2018
@PromoFaux
Copy link
Member Author

Failing this one, we could just add export LC_ALL=C to the top of the script, which may be a better solution than picking and choosing where it goes.

much more elegant to export LC_ALL at the beginning of the script!
6b4685b 
Signed-off-by: Adam Warner <adamw@rner.email>
@PromoFaux PromoFaux changed the title [Optional/Alternative to #1995] Specifty LC_ALL=C when dealing with sorted lists. [Preferred alternative] Specifty LC_ALL=C when dealing with sorted lists. Feb 21, 2018
@DL6ER DL6ER changed the title [Preferred alternative] Specifty LC_ALL=C when dealing with sorted lists. Specifty LC_ALL=C when dealing with sorted lists. Feb 21, 2018
DL6ER
DL6ER approved these changes Feb 21, 2018
View reviewed changes
Copy link
Member

@DL6ER DL6ER left a comment

Choose a reason for hiding this comment

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

Confirmed working on my setup. Whitelisting is now possible from the blocking page as well as the Query Log which was a problem before.

@DL6ER
Copy link
Member

DL6ER commented Feb 21, 2018

This was introduced in 8761707 and 55e10d8.

The test on PR #1910 might have been insufficient (they seem to have focused on availability of the newly used tools on all supported platforms rather than on the proper functionality under all possible circumstances).

@dschaper dschaper added PR: Approval Required Open Pull Request, needs approval PR: Approved Open Pull Request, Approved by required number of reviewers Bug: fixed Contains a bug resolution and removed PR: Approval Required Open Pull Request, needs approval labels Feb 23, 2018
@dschaper dschaper merged commit 41d9d57 into development Feb 24, 2018
@AzureMarker AzureMarker deleted the fix/AlternativeWhitelistFromWebFix branch February 25, 2018 02:55
@technicalpyro technicalpyro mentioned this pull request Feb 26, 2018
Closed
3 tasks
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/psa-having-whitelisting-issues-with-3-3-read-here/7393/11

@technicalpyro technicalpyro mentioned this pull request Mar 22, 2018
Closed
3 tasks
@dschaper dschaper removed the PR: Approved Open Pull Request, Approved by required number of reviewers label May 3, 2018
DL6ER added a commit that referenced this pull request Dec 20, 2021
@DL6ER
Merge pull request #4461 from pi-hole/qr_iframe
3a67d1c 
Companion to pi-hole/adminlte #1996
@DL6ER DL6ER mentioned this pull request Dec 22, 2021
Merged
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-ftl-v5-12-web-v5-9-and-core-v5-7-released/51795/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AzureMarker AzureMarker AzureMarker approved these changes

@DL6ER DL6ER DL6ER approved these changes

@dschaper dschaper Awaiting requested review from dschaper

@WaLLy3K WaLLy3K Awaiting requested review from WaLLy3K

@jacobsalmela jacobsalmela Awaiting requested review from jacobsalmela

Assignees
No one assigned
Labels
Bug: fixed Contains a bug resolution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@PromoFaux @DL6ER @pralor-bot @AzureMarker @dschaper