Skip to content
This repository has been archived by the owner on Jul 12, 2022. It is now read-only.

Escape all sources when they need it #967

Merged
merged 1 commit into from
May 7, 2020

Conversation

AnthonySteele
Copy link
Member

@AnthonySteele AnthonySteele commented May 6, 2020

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

Fix potential bug with remote sources where the url contains a space
The need for escaping the source in an arg doesn't depend on if it's a local (file) or remote (http/s) uri. It depends only on if there is a space in it or not. This can be in either, although it's more likely in a file path with "C:\Program Files" etc
ArgumentEscaper will check for spaces and handle it when needed. Code

Follow-on from #943
particularly #943 (comment)

⤵️ What is the current behavior?

🆕 What is the new behavior (if this is a feature change)?

💥 Does this PR introduce a breaking change?

🐛 Recommendations for testing

📝 Links to relevant issues/docs

🤔 Checklist before submitting

  • All projects build
  • Relevant documentation was updated

The need for escaping doesn't depend on if it's a local (file) or remote (http/s) uri
It depends only on if there is a space in it or not. This can be in either, although it's more likely in a file path with "C:\Program Files" etc
ArgumentEscaper will check for spaces and handle it when needed
@skolima
Copy link
Collaborator

skolima commented May 6, 2020

I do remember there was a reason to escape only local urls. I really can't remember it and I have no test for it.

@AnthonySteele
Copy link
Member Author

AnthonySteele commented May 6, 2020

We had to escape http urls in #943 (finally got one with a space in it), in a different place in the code.
We can park this for now, and see if they verify that #943 is in fact fixed now, and in case the reason come back to mind.

@skolima
Copy link
Collaborator

skolima commented May 6, 2020

I'm thinking this should be safe to merge. If there's a case which fails, we'll get a test. Maybe I'm misremembering.

@AnthonySteele
Copy link
Member Author

AnthonySteele commented May 7, 2020

I think that the argument was that local file:// paths can contain a space ( the "C:\Program Files" case) , and that http urls do not.

It turns out that 1) that's not always true about http urls, and 2) ArgumentEscaper does the right thing at a lower level by checking for spaces. So existing behaviour for http urls that do not contain spaces will be preserved.

@AnthonySteele AnthonySteele merged commit 4d9caad into NuKeeperDotNet:master May 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants