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

Fix environment variable expansion in absPathify #495

Closed
wants to merge 1 commit into from

Conversation

biochimia
Copy link
Contributor

  • Don't expand user home directory for variable names that simply have a
    HOME prefix;
  • Support expansion of variables not followed by the path separator.

- Don't expand user home directory for variable names that simply have a
  HOME prefix;
- Support expansion of variables not followed by the path separator.
@qjcg
Copy link
Contributor

qjcg commented Jul 31, 2020

@sagikazarmark Please review this PR when you have a moment, it fixes a real issue with variable expansion (see my lefthook mention above for an example).

Copy link
Collaborator

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

@qjcg thanks for the ping and the detailed explanation on the linked issue. I had a comment on the PR, but it generally looks good. It needs a rebase though, since it's quite old.

Dunno if @biochimia is up for it, so feel free to resubmit it if you want to speed things up.

@@ -91,13 +91,23 @@ func insensitiviseMap(m map[string]interface{}) {
func absPathify(inPath string) string {
jww.INFO.Println("Trying to resolve absolute path to", inPath)

if strings.HasPrefix(inPath, "$HOME") {
if strings.HasPrefix(inPath, "$HOME") &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this be rewritten as something like inPath == "$HOME" || strings.HasPrefix(inPath, "$HOME"+os.PathSeparator) or equivalent (or even using some sort of splitting)? The current condition is not really expressive.

Side note: looking at the path/filepath package there isn't much that can help here, but ideally it would be nice.

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