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

Mad patch 1 #13

Closed
wants to merge 12 commits into from
Closed

Mad patch 1 #13

wants to merge 12 commits into from

Conversation

bahamas10
Copy link
Contributor

  • Quote all potentially dangerous expansions
  • Sanity check all calls to chdir(2)
  • Prefer PAGER env variable and fallback on less(1)
    • Passing exit code from PAGER properly, removing unnecessary call to exit
  • Prefer bash redirects over cat(1)
  • Use bash arrays to load and iterate over paths safely
  • Replace external programs with bash built-ins
  • Use bash builtin regex and paramater expansion to filter files
  • Drop the dependency to perl in list_pages()

* Use bash arrays to load and iterate over paths
* Use bash builtin regex and paramater expansion to filter files
* Drop the dependency to perl in this function
* Dirname can be done with parameter expansion
* All variables should be quoted to avoid wordsplitting
** http://mywiki.wooledge.org/BashFAQ/028
* Use bash arrays to load and iterate over paths safely
* Bad things can happen if you don't sanity check cd
@mirlord
Copy link
Contributor

mirlord commented May 26, 2012

$PAGER-related fix won't work. Option -R only required for less, all other pagers don't need it and event don't support. i.e. vimpager and more will fail instantly with "unknown option" error message.

Also current text highlighting defaults are incompatible with more. See pull request #12 for details and a bit better impl.

@tj
Copy link
Owner

tj commented May 27, 2012

hmm some of these should be separate pull-requests but i'll take a look

@tj
Copy link
Owner

tj commented May 27, 2012

cherry-picked some but now a few wont apply properly

@bahamas10
Copy link
Contributor Author

I was back and forth between submitting multiple pull-requests, or just breaking it into separate commits.

I went with separate commits simply because I'm not adding any new features per se. I wanted to optimize this for speed, sanity check all lines that could possibly error out, as well as minimize the call to outside programs and prefer bash built-ins.

I realize now that some of the commits are dependent on the others, and cherry-picking a select few most likely won't work. If you want to pull-in some of the changes I can modify this for separate pull-requests to clearly identify/separate the changes.

@bahamas10 bahamas10 closed this Jun 4, 2014
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

3 participants