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] nvm_strip_path: Preserve leading/trailing colons #3145

Merged
merged 1 commit into from
Aug 22, 2023

Conversation

oliverhenshaw
Copy link
Contributor

Preserve leading & trailing colons in awk invocation.

Path lists in environmental variables often give special meaning to
empty entries (e.g. in PATH or MANPATH). These are represented by
leading or trailing colons, or by doubled colons in the middle of the
list.

Adjust the awk invocation to correctly deal with trailing colons by
printing the separator before every field except the first, and then
printing the final separator that is read from the input - this will
either be a colon or the null string. This preserves leading and
trailing colons in all cases while not adding extra colons in the wrong
place.

Add test to confirm the correct behaviour.

Fixes nvm-sh#3144
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This makes sense for MANPATH, but I'm not sure it makes sense for PATH.

Also, what happens if the thing being removed is at the end? We wouldn't want to leave behind a double colon.

@oliverhenshaw
Copy link
Contributor Author

oliverhenshaw commented Jul 8, 2023

This makes sense for MANPATH, but I'm not sure it makes sense for PATH.

As noted in the bug, empty directory names in PATH do have some meaning, even if it seems less useful. So colons should be preserved there too.

Also, what happens if the thing being removed is at the end? We wouldn't want to leave behind a double colon.
What this awk invocation does it put a colon before every unstripped directory name apart from the first. And then checks the end of the input to see if it needs to add a trailing colon. So I don't think there's a way to end up with a double colon, unless there's one in the input - unless you can see a case where it would?

I think the existing tests already cover this case anyway, they have entries in $NVM_DIR at the end of $TEST_PATH.

@ljharb
Copy link
Member

ljharb commented Jul 8, 2023

The other potential problem case is when it's not at the beginning or end.

@oliverhenshaw
Copy link
Contributor Author

The other potential problem case is when it's not at the beginning or end.

This works fine I think - with the patch I get:

$ nvm_strip_path /a/bin:$NVM_DIR/b/bin:/c/bin /bin
/a/bin:/c/bin

because awk continues to the next record when it finds a path entry to remove, and skips the part that prints anything.

Note that when the MANPATH or PATH documentation talks about a trailing colon, they mean when a colon is the last character in the input env variable, not about every colon used as a record separator. I feel like there's a bit of subtlety here, and half the challenge is to explain it well in the commit log.

@ljharb
Copy link
Member

ljharb commented Jul 8, 2023

Hmm, it's also possible that I'm stuck on my mental model from the original implementation, which did not use awk - maybe the awk-based solution doesn't have this fragility.

@ljharb ljharb changed the title [Fix] nvm_strip_path: Preserve leading/trailing colons [Fix] nvm_strip_path: Preserve leading/trailing colons Aug 22, 2023
@ljharb ljharb merged commit 15eba7b into nvm-sh:master Aug 22, 2023
@oliverhenshaw oliverhenshaw deleted the strip_no_mangle_path branch September 22, 2023 18:17
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.

2 participants