Partially revert glob support to Windows-only fallback #229
Conversation
The support for glob arguments introduced in v1.10.0 led to regressions reported in richardlehane#227. Special characters used for glob patterns are also allowed in filenames on Linux and to a reduced extent in Windows, consequently such files couldn't be matched directly anymore as before without escaping. This switches the treatment of arguments back to the pre-1.10 behavior of doing a direct match first. Glob support is retained as an attempted fallback on Windows when no direct match could be found.
|
thanks Bernhard! |
just realised os.Lstat may be insufficient to test existence of files on windows due to the long path name issue, see e.g. this old issue #58. I.e. if the given path is too long then a normal lstat will fail even if the path exists suggest adding a new You could then do |
Good call, totally overlooked that, done.
Right, I took the opportunity to follow the trail a bit and found that Go gradually added longpath handling since 1.8 and the current state even builds the native support available since Windows 10, Version 1607 into compiled Windows executables. At the same time, the handling implemented still leaves UNC paths untouched and doesn't handle relative paths, which makes understanding what the right thing to do really is quite confusing to me.
Given that the native longpath handling is incomplete I think this is probably still a good thing to do, thanks for the suggestion! I decided to implement this as Looking at the error display there I notice that the messages are somewhat redundant:
Do you think it might a good idea to do a |
👋🏻 Hi! In fact, I'm already looking at reverting ☝🏻 change as it broke some other stuff (unrelated to sf). We are eagerly interested in an |
I'll try to get a point release out over the weekend incorporating this fix + fixes for a few other things 1.10 broke (e.g. the deb packages) |
|
@richardlehane Much obliged! |
This implements the approach discussed in #227 to remedy the reported regression by reverting to treating arguments as literal paths first and only on Windows attempting to match glob patterns when no direct match has been found.
I decided to implement the Windows-only behavior by checking against
runtime.GOOSwhich is statically compiled into the executable as well and made more sense to me in this context than splitting this up across files. If you prefer I can certainly do that too. Also, to reduce the cases where a matching is attempted a bit I added a check for the relevant characters that could mark a valid pattern in the name.I retained the changed
matches/matchvariable names from #225 (which this PR subsumes) as you indicated you were fine with it, hope I got that right.As already discussed, this won't solve all of the potentially surprising cases but will help for the most urgent and reported ones, we can keep further discussion for those in the original issue or open a separate one as you prefer. Seeing sul-dlss/technical-metadata-service#437 (comment) I'm slightly worried that there could already be more users that adjusted their integrations to shell-escape the arguments which they will have to revert should this PR get merged and eventually released.
Finally, I was wondering about whether the
*coe"continue on errors" flag should have an effect on error handling across arguments and matches in addition to just the walk-function? I didn't add anything in this direction here, as this will be better discussed separately, just leaving this here for now while it's fresh in my head.