Skip to content

update regex to support BSD sed (for macOS devs) and GNU sed #293

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

update regex to support BSD sed (for macOS devs) and GNU sed #293

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a1bb74c
update regex to support BSD sed (for macOS devs) and GNU sed
Sep 30, 2024
5c1bde6
use a simpler solution of source coupled with echo in a bash subshell
Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/shared-functions.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,11 +180,11 @@ function check_sharelatex_env_vars() {
function read_variable() {
local name=$1
grep -E "^$name=" "$TOOLKIT_ROOT/config/variables.env" \
| sed -r "s/^$name=([\"']?)(.+)\1\$/\2/"
| sed -r "s/^$name=([\"']?)([^\"']+)[\"']?$/\2/"
Copy link
Member

Choose a reason for hiding this comment

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

This will not trim in case there are quotes inside the value, e.g. foo='Instance managed by "Department"'.

before:

$ name=foo; cat | sed -r "s/^$name=([\"']?)(.+)\1\$/\2/"
foo='Instance managed by "Department"'

// output
Instance managed by "Department"

after:

$ name=foo; cat | sed -r "s/^$name=([\"']?)([^\"']+)[\"']?$/\2/"
foo='Instance managed by "Department"'

// output
foo='Instance managed by "Department"'

Using a non-greedy matching group instead of the "non-quote"-group in the middle does not work either:

$ name=foo; cat | sed -r "s/^$name=([\"']?)(.+?)[\"']?$/\2/"
foo='Instance managed by "Department"'

// output
Instance managed by "Department"'
                                ^-- unmatched quote

Copy link
Contributor

Choose a reason for hiding this comment

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

How about using bash to do the parsing in a subshell?

function read_variable() {
  local name=$1
  (
    source "$TOOLKIT_ROOT/config/variables.env"
    echo "${!name:-}"
  )
}

Copy link
Author

Choose a reason for hiding this comment

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

@das7pad good point, I didn't think values for environment variables would have quotes in most cases, but I agree it makes sense to support that as well. Thanks for pointing that out.

@briangough thank you for looking into this. I think your solution is much simpler and cleaner and works well for the above mentioned edge case as well. I'll update the PR with it.

Copy link
Author

Choose a reason for hiding this comment

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

@das7pad just wanted to check-in, if we can merge this PR or is there anything else I need to do prior to that? Thanks.

}

function read_configuration() {
local name=$1
grep -E "^$name=" "$TOOLKIT_ROOT/config/overleaf.rc" \
| sed -r "s/^$name=([\"']?)(.+)\1\$/\2/"
| sed -r "s/^$name=([\"']?)([^\"']+)[\"']?$/\2/"
}