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: Only remove options for actual URL in URIUtils::Split (fixes #17627) #12962

Merged
merged 1 commit into from
Oct 29, 2017

Conversation

koying
Copy link
Contributor

@koying koying commented Oct 26, 2017

@mkortstiege @razzeee

Quick fix for the issue we discussed about.
Untested, so if you could make it tested, somehow...

Rechi
Rechi previously requested changes Oct 26, 2017
Copy link
Member

@Rechi Rechi left a comment

Choose a reason for hiding this comment

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

TestURIUtils.Split fails on all platforms

Copy link
Member

@mkortstiege mkortstiege left a comment

Choose a reason for hiding this comment

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

Actual change looks good to me.

@razzeee
Copy link
Member

razzeee commented Oct 28, 2017

@koying can you try razzeee@9403799

@koying
Copy link
Contributor Author

koying commented Oct 28, 2017

@Rechi

URIUtils::Split("/path/to/movie.avi?showinfo=true", varpathOptional, varfileOptional);

The test case is basically what this PR is trying to "solve".
AFAIK, plain filenames are not URI, so something like "/path/to/movie.avi?showinfo=true" is actually invalid.

I'd remove the testcase, or replace by "file:///path/to/movie.avi?showinfo=true", but your thoughts are welcome

@koying koying force-pushed the fix17627 branch 2 times, most recently from e96e42f to 8900ca8 Compare October 28, 2017 08:34
…c#17627)

It occurs that URIUtils::Split is not only used for URI
@koying
Copy link
Contributor Author

koying commented Oct 28, 2017

Added an additional testcase for the specific one we're trying to solve

@koying
Copy link
Contributor Author

koying commented Oct 28, 2017

jenkins build this please

@koying
Copy link
Contributor Author

koying commented Oct 28, 2017

@Rechi Test cases ok. Good for you?

@Rechi Rechi dismissed their stale review October 29, 2017 10:48

test suite working again

@Rechi
Copy link
Member

Rechi commented Oct 29, 2017

wrong revision was build
jenkins build this please

@koying koying merged commit cbd11f1 into xbmc:master Oct 29, 2017
@koying koying deleted the fix17627 branch October 29, 2017 13:15
@Rechi Rechi added this to the L 18.0-alpha1 milestone Oct 29, 2017
@MartijnKaijser
Copy link
Member

@koying you want this backported or risk too high?

@koying
Copy link
Contributor Author

koying commented Nov 11, 2017

Actually, the code introducing the issue is not in K, so irrelevant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants