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

#467: quickfix for tilde expansion #468

Merged
merged 4 commits into from
Sep 24, 2020
Merged

#467: quickfix for tilde expansion #468

merged 4 commits into from
Sep 24, 2020

Conversation

hohwille
Copy link
Member

This is a "fix" for #467.
However, it currently only replaces the tilde (~) if the variable value starts with it and is not quoted. Further for such variables it reintroduces the bug #100 and #395. So assuming that we only have cases like

export M2_REPO=~/.m2/repository

Or

PROJECT_HOME=~

But not something like

FOO="~/path with whitespaces"

this PR could be merged.
If we want a clean solution, further investigation is required (and IMHO a real CMD guru is needed as I run out of ideas).

@hohwille
Copy link
Member Author

Compared to the current state on master that is entirely nuts this change IMHO is already a reasonable improvement.

@hohwille hohwille requested a review from maybeec September 22, 2020 15:35
@hohwille hohwille added this to the release:2020.08.002 milestone Sep 22, 2020
@hohwille
Copy link
Member Author

So it more or less works now.
Input from devon.properties:

export M2_REPO=~/.m2/repository
M3_REPO=~
M4_REPO="~/path with whitespaces"
M5_REPO=~/path with whitespaces and many words

Output from set command after executing devon:

M2_REPO=C:\Users\hohwille/.m2/repository
M3_REPO=C:\Users\hohwille
M4_REPO=C:\Users\hohwille/path with whitespaces
M5_REPO=C:\Users\hohwille/path

We could fix the case with M5_REPO by using this code instead:

  set line=%replacement%%USERPROFILE%!value:~1! %~3 %~4 %~5 %~6 %~7 %~8 %~9

However, if the additional arguments are empty, trailing whitespaces are added to the variable value and I am unsure what other side-effects this may have. Preventing this with IF statements does not work as described in #467.
I think this is already "good enough" as people are used to quote paths containing whitespaces (even though bash prevents ~ expansion inside quotes what could confuse bash experts)...

@hohwille hohwille added bug Something isn't working enhancement New feature or request scripts related to shell scripts (bash and CMD) windows specific for Microsoft Windows OS labels Sep 22, 2020
set "search=%~1% "
)
set "replacement=%~1%="
set value=%~2
Copy link
Member

Choose a reason for hiding this comment

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

isn't here a % missing at the end?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. %2 is the second argument and %~2 expands it and unquotes.
https://ss64.com/nt/syntax-args.html

So your question might be why is there an extra percent sign after the first arg.
I can only reasonable answer for the %~1%= case as here the % sign is an escape for the equals sign.
In the other case (line 92) it may be obsolete to add the extra percent.
I can test that and can also not tell if I initially wrote this code as Rohit supported the fix #395

Copy link
Member Author

Choose a reason for hiding this comment

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

So also the equals sign does not need to be escaped with percent sign.
To make you happy and avoid further confusion I removed the pointless percent signs.
According to my tests in Win VM it does not make any difference so fingers crossed that this will not break anything in this fragile CMD universe...
Can we resolve our conversations now and merge this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Well done to remove the trailing percent here. They should not be used after positional parameters.

Only variables within scripts must be surrounded by percents - for positional parameters this is just accepted for "convenience". (The percent is no escape char here)
A "%1%2" still resolves to first pos. param followed by 2nd,

From today's view point it is easy to say, this was a bad design decision - but in the 80'ies this was not so easy to forsee.

set value=%~2
if "!value:~0,1!" == "~" (
rem variable value starts with tilde that needs to be replaced with users home dir (USERPROFILE)
set line=%replacement%%USERPROFILE%!value:~1!
Copy link
Member

Choose a reason for hiding this comment

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

for me it's fine to just support ~ replacement at the first character of the value. Anyhow just in case you havent seen it, there are solutions to a more generic algorithm, which anyhow underlines how crappy batch is :( https://stackoverrun.com/de/q/9716479

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the link. After rethinking it might even be wrong to resolve C:/data/file~name to C:/data/fileC:\users\hohwillename. So I would rather consider this as a feature :)

@hohwille hohwille merged commit d3847cb into devonfw:master Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request scripts related to shell scripts (bash and CMD) windows specific for Microsoft Windows OS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expansion of ~ stopped working on windows CMD (M2_HOME not properly set)
3 participants