Avoid variable expansion and stripping leading spaces in 'read_line'

[Fraser999]

On Windows, read_line

• can't handle a leading = character (it crashes)
• strips any leading spaces from the input
• expands any variables (e.g. if the user enters %path%, the stored result is the value of the user's path environment variable rather than the string %path%).

This patch fixes these issues.

[toeb]

thats cool. I'll pull it soon. I am still thinking about if it makes sense to expand cmd variables - what if you want to enter %path% and mean it?. possibly i'll add a second version of the function or a flag which allows expansion.
(do you know if something similar (variable expansion) is possible in bash?)

[Fraser999]

Don't worry about rejecting this pull request if you don't think it's right :-) I'll not be offended in the least.

I haven't tried this on any platform but Windows so far, so I can't speak for bash. From my own perspective, we can expand the environment variables whenever we want, so it makes sense to not do it automatically in the read_line call.

That aside, I'm more concerned with the other two points in my list than the variable expansion.

My main concern would be that we experience the same behaviour on all platforms, since that's what CMake strives for, and generally achieves.

[toeb]

I agree with the crash on = that has to be fixed. Could you explain what fixes that (I am very bad at cmd) does the EnabledDelayedExpansion have to be used for this?

Removing the ending LF is a good idea but I will make it optional and allow windows line ending CRLF: string(REGEX REPLACE "(.*)(\n |\r\n)?$" "\1" line "${line}") this way it will stay platform

I am torn on leading whitspace (maybe it has to be input in some cases) and you can also do a string_trim("${line}") or string(STRIP line "${line}") after the read_line...

Btw thanks for humoring me

[Fraser999]

I agree with the crash on = that has to be fixed. Could you explain what fixes that

The code which causes the crash is this: echo | set /p dummyName=\"%val%\" It's using a hack to avoid the extra CRLF which bare echo always adds to the output.

set /p dummyName="%val%" is basically looking for the user to enter a value which will be assigned to dummyName and is passing the contents of val as the prompt string. It would normally be used like set /p name="Enter your name: ". The hack takes advantage of the fact that prompt string doesn't have a CRLF appended, but this is where the limitations start. The prompt string may not start with = (that's an error), and it also has all leading spaces stripped automatically.

So, in my patch I changed this hack to just use normal echo !val! which appends a CRLF to the output, but which is subsequently stripped with string(REGEX REPLACE "(.*)\n$" "\\1" line "${line}"). I don't think we need the slightly more complex regex "(.*)(\n |\r\n)?$" you proposed, but I could be wrong.

The other aspect is the EnabledDelayedExpansion. This not only avoids environment variables from being expanded, but perhaps more importantly allows some special characters to passed through properly. Without it (you'd need to change echo !val! to echo %val% if you go that way) read_line won't handle >, <, |, ^. There may be others too.

From my own perspective, I'd find read_line most useful if it faithfully copies exactly what is entered... no variable expansions and no stripping whitespace. These extra things can easily be done once the input has been entered, but there'd obviously be no way to "put back on" leading whitespace if it was stripped before we got it, and it'd be pretty painful to try to put an expanded variable back into its originally-entered form if that's what was intended by the user.

Btw thanks for humoring me

Thank you for open sourcing this :-)

[toeb]

Alright! now I got it all (thanks for the explanation) I was a bit confused with the first post thinking that you wanted to strip the whitespace and expand the variables (knowing how to read helps). now I see what you have done and think it is great alltogether and will merge promptly.

[toeb]

I am putting in a

if("${line}" MATCHES "(\n|\r\n)$")
    string(REGEX REPLACE "(\n|\r\n)$" "" line "${line}")
  endif()

just for peace of mind and (I do not know if more platforms might need this) and since the shellscript execution is the bottleneck for performance this will not make read_line significantly slower.

Ha! first pull request completed. I'll be drinking a beer in you honor tonight!

[Fraser999]

Nice one. I may just have a wee whisky myself :-)